Re: [PATCH] fetch-pack: do not mix --pack_header and packfile uri

[Junio C Hamano <gitster@pobox.com>]

Jonathan Tan <jonathantanmy@google.com> writes:

> I should probably have written more in the commit message to justify the
> unification, but it is also part of a bug fix (in particular,
> --fsck-objects wasn't being passed to the index-pack that indexed the
> packfiles linked by URI) and for code health purposes (to prevent future
> bugs by eliminating the divergence). So reverting that commit would
> reintroduce another bug.

Not necessarily.  Unifying two that do not inherently have to be
identical makes it impossible to pass two different things, and that
is what we are seeing in the bug this patch is trying to fix (by
forcing the two to be identical by eliminating the unpack-objects
codepath in certain cases).  

The right "fix" for the original bug would have been to keep them
still separate yet making it easy to pass args that must be used in
both of them, no?

>> > -		if (do_keep && (args->lock_pack || unpack_limit)) {
>> > +		if ((do_keep || index_pack_args) && (args->lock_pack || unpack_limit)) {
>> >  			char hostname[HOST_NAME_MAX + 1];
>> >  			if (xgethostname(hostname, sizeof(hostname)))
>> >  				xsnprintf(hostname, sizeof(hostname), "localhost");
>> 
>> I do not quite get what this hunk is doing.  Care to explain?
>
> The "do_keep" part was unnecessarily restrictive and I used a band-aid
> solution to loosen it. I think this started from 88e2f9ed8e ("introduce
> fetch-object: fetch one promisor object", 2017-12-05) where I might have
> misunderstood what do_keep was meant to do, and taught fetch-pack to use
> "index-pack" if do_keep is true or args->from_promisor is true. What I
> should have done is to set do_keep to true if args->from_promisor is
> true. Future commits continued to do that with fsck_objects and
> index_pack_args.

> Maybe what I can do is to refactor get_pack() so that do_keep retains
> its original meaning of whether to use "index-pack" or "unpack-objects",
> and then we wouldn't need this line. What do you think (code-wise and
> whether this fits in with the release schedule, if we want to get this
> in before release)?

How bad is the breakage this one is trying to fix?  I know it would
only affect folks who have to interact with the server that uses
packfile URI feature, but do they have a workaround, perhaps with a
configuration knob or command line option to ignore the packfile
URI, and how large is the affected population?

I cannot shake the feeling that we are seeing band-aid on top of
band-aid forced by having chosen to go in a wrong direction in the
beginning X-<, and prefer to see the code drift even further into
the same direction; hence my earlier suggestion to go back to the
root cause by first reverting the wrong fix that introduced this bug
and fixing the original bug in a different way.

I dunno how involved the necessary surgery would be, though.  If
this is easy to work around, perhaps it might be a better option for
the overall project to ship the upcoming release with this listed as
a known breakage.

Thanks.