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

> 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,

> 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.