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

On Wed, Aug 12, 2026 at 05:42:00PM +0200, Christian Couder wrote:
> On Wed, Aug 12, 2026 at 1:22 PM Patrick Steinhardt <[email protected]> wrote:
> >
> > On Tue, Aug 11, 2026 at 02:14:46PM +0200, Christian Couder wrote:
> > > 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);
> >
> > Should we maybe condense this into a single line?
> >
> >     usage(_("no file given for '%s' option\n")), "--shallow-file")
> >
> > I think that also printing the usage string is only distracting and
> > doesn't really give the user a lot of extra context.
> 
> The goal of this patch is to fix the bug by using the same code as the
> other options that can be passed a value like "--git-dir",
> "--namespace", "--work-tree", and so on. Now all these options use the
> same pattern for the error message:
> 
> git grep -A3 'if (\*argc < 2)' git.c
> git.c:                  if (*argc < 2) {
> git.c-                          fprintf(stderr, _("no directory given
> for '%s' option\n" ), "--git-dir");
> git.c-                          usage(git_usage_string);
> git.c-                  }
> --
> git.c:                  if (*argc < 2) {
> git.c-                          fprintf(stderr, _("no namespace given
> for --namespace\n" ));
> git.c-                          usage(git_usage_string);
> git.c-                  }
> --
> git.c:                  if (*argc < 2) {
> git.c-                          fprintf(stderr, _("no directory given
> for '%s' option\n" ), "--work-tree");
> git.c-                          usage(git_usage_string);
> git.c-                  }
> --
> git.c:                  if (*argc < 2) {
> git.c-                          fprintf(stderr, _("-c expects a
> configuration string\n" ));
> git.c-                          usage(git_usage_string);
> git.c-                  }
> --
> git.c:                  if (*argc < 2) {
> git.c-                          fprintf(stderr, _("no config key given
> for --config-env\n" ));
> git.c-                          usage(git_usage_string);
> git.c-                  }
> --
> git.c:                  if (*argc < 2) {
> git.c-                          fprintf(stderr, _("no directory given
> for '%s' option\n" ), "-C");
> git.c-                          usage(git_usage_string);
> git.c-                  }
> --
> git.c:                  if (*argc < 2) {
> git.c-                          fprintf(stderr, _("no attribute source
> given for --attr-source\n" ));
> git.c-                          usage(git_usage_string);
> git.c-                  }
> 
> So I don't think it makes sense for "--shallow-file" to not be
> consistent with these other options.
> 
> I could perhaps add a patch to the series to convert all of these to
> something like what you suggest, but it could also be done in a
> separate patch series by someone else.
> 
> Anyway thanks for reviewing this patch.

No, I don't think that's really necessary. Given the existing usage I
think your patch looks sensible. Thanks!

Patrick