> Jonathan Tan <jonathantanmy@xxxxxxxxxx> 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?
OK - I'll do this.
> >> > - 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,
Yes, there's a workaround (to disable packfile URIs from the client side
using a config variable).
> and how large is the affected population?
The only issues I've seen are within $DAYJOB, and there, we can carry
our own patch to fix this issue. So the affected population (right now)
is probably not much (if it even exists).
> 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.
I don't think it's too difficult - I think we'll only need to filter out
the --pack_header when we figure out the arguments to pass for the
packfiles given by URI. I'll take a look.
