Re: [PATCH] git: avoid segfault on "git --shallow-file" without a value

On Tue, Aug 11, 2026 at 9:16 PM Junio C Hamano <[email protected]> wrote:
>
> Christian Couder <[email protected]> writes:

[...]

> >   $ git --shallow-file
> >   Segmentation fault (core dumped)
> > ...
> > diff --git a/git.c b/git.c
> > index e5f1811b6b..96df15b5cd 100644
> > --- a/git.c
> > +++ b/git.c
> > @@ -304,11 +304,15 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)
> >                       if (envchanged)
> >                               *envchanged = 1;
> >               } else if (!strcmp(cmd, "--shallow-file")) {
> > -                     (*argv)++;
> > -                     (*argc)--;
> > -                     setenv(GIT_SHALLOW_FILE_ENVIRONMENT, (*argv)[0], 1);
> > +                     if (*argc < 2) {
> > +                             fprintf(stderr, _("no file given for '%s' option\n" ), "--shallow-file");
> > +                             usage(git_usage_string);
> > +                     }
> > +                     setenv(GIT_SHALLOW_FILE_ENVIRONMENT, (*argv)[1], 1);
> >                       if (envchanged)
> >                               *envchanged = 1;
> > +                     (*argv)++;
> > +                     (*argc)--;
>
> It is curious that the fix needs to be so big, when the only change
> necessary, as far as I can tell from your problem description, is to
> insert 4 line "if (... not enough args ...) { ... barf and die ...}"
> block and without anything else.  I think the culprit is this "while
> at it" ...
>
> > While at it, let's also set the environment variable before advancing
> > past the option, instead of advancing first and using `(*argv)[0]`, so
> > that this option looks like the other ones.
>
> ... that made the patch more confusing to read than otherwise.

Sorry but the goal was to use similar code as other options that can
be passed a value like "--git-dir", "--namespace", "--work-tree", and
so on.

> But without reading the preimage of the patch, the result is just as
> understandable ;-)  Let's take the patch as-is.
>
> > +test_expect_success 'git --shallow-file without a value' '
> > +     test_must_fail git --shallow-file >actual 2>actual.err &&
> > +     test_line_count = 0 actual &&
> > +     test_grep "no file given for " actual.err &&
> > +     test_grep "usage" actual.err
> > +'
>
> Do we have similar "oops, you were supposed to give me a value" test
> for other things like "--config-env=", "-C", etc.?

In "t/t1300-config.sh" there is:

test_expect_success 'git --config-env with missing value' '
        test_must_fail env ENVVAR=value git --config-env 2>error &&
        test_grep "no config key given for --config-env" error &&
        test_must_fail env ENVVAR=value git --config-env config
core.name 2>error &&
        test_grep "invalid config format: config" error
'

I couldn't find anything else.

> Just being
> curious, because (1) if there are, this addition belongs there, not
> here,

I am not sure the `git --shallow-file` test belongs to "t/t1300-config.sh".

Maybe the new test for --shallow-file with no value should be at the
same place as other tests for --shallow-file, unfortunately there are
no such tests. It looks like this is an undocumented and internal only
option which is only tested indirectly in the following files:

- t5311-pack-bitmaps-shallow.sh
- t5537-fetch-shallow.sh
- t5538-push-shallow.sh
- t5539-fetch-http-shallow.sh
- t5542-push-http-shallow.sh
- t5614-clone-submodules-shallow.sh

> and (2) if there aren't, this addition may not be needed, and
> (3) if there aren't or if the existing coverage is incomplete,
> perhaps we should give a more complete coverage while at it.
>
> With (3), I mean something along the lines of ...
>
>         for opt in -C -c --git-dir --work-tree --namespace --config-env
>         do
>                 test_expect_success "git $opt without a value" '
>                         test_must_fail git $opt >actual 2>error &&
>                         test_line_count 0 actual &&
>                         test_grep usage error
>                 '
>         done
>
> I do not mean to say that (3) is my favorite among these three,
> though.

I am fine with (2) or (3), but they don't seem much better to me than
the test already in this patch.

Thanks.