Re: `git bundle create -` may not write to `stdout`
- From
Jeff King <peff@peff.net>
- Date
- Mar 3, 2023, 22:54 UTC
- Message-ID
- <ZAJ6oI3clNH2O3R7@coredump.intra.peff.net>
- In-Reply-To
- <xmqqv8jhcvrq.fsf@gitster.g>
On Fri, Mar 03, 2023 at 02:31:05PM -0800, Junio C Hamano wrote:
Show 20 quoted lines
> Jeff King <peff@peff.net> 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. :)
Show 9 quoted lines
> 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).
Show 18 quoted lines
> 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