On Fri, Mar 03, 2023 at 02:31:05PM -0800, Junio C Hamano wrote:
> Jeff King <peff@xxxxxxxx> writes:
>
> > The most directed fix is this:
> >
> > diff --git a/builtin/bundle.c b/builtin/bundle.c
> > index acceef6200..145b814f48 100644
> > --- a/builtin/bundle.c
> > +++ b/builtin/bundle.c
> > @@ -59,7 +59,9 @@ static int parse_options_cmd_bundle(int argc,
> > PARSE_OPT_STOP_AT_NON_OPTION);
> > if (!argc)
> > usage_msg_opt(_("need a <file> argument"), usagestr, options);
> > - *bundle_file = prefix_filename(prefix, argv[0]);
> > + *bundle_file = strcmp(argv[0], "-") ?
> > + prefix_filename(prefix, argv[0]) :
> > + xstrdup(argv[0]);
> > return argc;
> > }
>
> This fell thru cracks, it seems.
I was waiting to give Michael a chance to respond to my offer. :)
> OPT_FILENAME() needs to do exactly this, and has its own helper
> function in parse-options.c::fix_filename(), but its memory
> ownership rules is somewhat screwed (it was perfectly fine in the
> original context of "we parse the bounded number of options the user
> would give us from the command line, and giving more than one
> instance of these last-one-wins option may leak but we do not
> care---if you do not like leaking, don't give duplicates", but with
> automated leak checking that does not care about such nuances, it
> will trigger unnecessary errors), and cannot be reused here.
Heh, I was worried about it kicking in for spots that "-" was not
meaningful, but I checked only prefix_filename() itself, and didn't
think to check OPT_FILENAME()'s full code path.
(I do still think we don't want to push it down into prefix_filename(),
because it gets used for paths and pathspecs given raw on the command
line. It does make me wonder if there are places where OPT_FILENAME() is
doing the wrong thing).
> Here is your "most directed fix" packaged into a call to a helper
> function. Given that we may want to slim the cache.h header, it may
> not want to be declared there, but for now, its declaration sits
> next to prefix_filename().
Yeah, a helper may be nice, though if this is the only spot, I'd be
tempted not to even bother until fix_filename() is fixed. The obvious
fix is for it to always allocate, but callers will need to be adjusted.
I suspect it will trigger a bunch of complaints from the leak-checking
tests (because the caller does not expect it to be allocated, so it's a
sometimes-leak now, but it will become an always-leak).
> diff --git c/t/t6020-bundle-misc.sh w/t/t6020-bundle-misc.sh
> index 7d40994991..d14f7cea91 100755
> --- c/t/t6020-bundle-misc.sh
> +++ w/t/t6020-bundle-misc.sh
> @@ -606,4 +606,15 @@ test_expect_success 'verify catches unreachable, broken prerequisites' '
> )
> '
>
> +test_expect_success 'send a bundle to standard output' '
> + git bundle create - --all HEAD >bundle-one &&
> + mkdir -p down &&
> + git -C down bundle create - --all HEAD >bundle-two &&
> + git bundle verify bundle-one &&
> + git bundle verify bundle-two &&
> + git ls-remote bundle-one >expect &&
> + git ls-remote bundle-two >actual &&
> + test_cmp expect actual
> +'
This test looks good to me. Let's also not forget about the doc fixes. I
don't think there's much urgency to get this into v2.40, but I can put
it together in the next day or three.
-Peff
