Re: [PATCH 2/3] transport.c: introduce core.alternateRefsCommand

From: Jeff King <peff@peff.net>
Date: Sep 20, 2018, 20:06 UTC
Message-ID: <20180920200627.GA536@sigill.intra.peff.net>
In-Reply-To: <20180920200034.GA83799@syl>

On Thu, Sep 20, 2018 at 04:00:34PM -0400, Taylor Blau wrote:

Show 24 quoted lines

> > My "rev-list --alternate-refs" patches _do_ use the refnames, since you
> > could do something like "--source" that cares about them. But there's
> > some awkwardness there, because the names are in a different namespace
> > than the rest of the refs. If we were to just say "nope, you do not get
> > to see the names of the alternates" then that awkwardness goes away. But
> > it also loses some information that could _possibly_ be of use to a
> > caller.
> >
> > Back in that earlier discussion I did not have a strong opinion, but
> > here we are cementing that decision into a user-visible interface. So it
> > probably makes sense to revisit and decide once and for all.
> 
> Interesting, and thanks for the link to the prior discussion. I think
> that I agree mostly with your rationale in [1], which boils down (for
> me) to:
> 
>   - Other callers (like 'rev-list --alternate-refs') might care about
>     them. Even if we don't have those patches in Git today, it's worth
>     keeping their use case(s) in mind.
> 
>   - I didn't measure either, but I can't imagine that we're paying a
>     huge price for this. So, it might be easy enough to keep saying,
>     "please write output as '%(objectname) SP %(refname)'", even if we
>     end up throwing out the refname, anyway.

TBH, the main advantage to me is that it makes the user-visible interface way simpler. We just say "give us a list of object ids, one per line". I guess the current spec is not too bad, especially given that we can just provide a for-each-ref format that generates it.

Show 6 quoted lines

> > Probably not worth nit-picking process count, but this could done with a
> > single "update-ref --stdin".
> 
> Sure, I don't think that 7 `update-ref`'s vs 2 (`cat` + `git update-ref
> --stdin`) will make or break the series, but I can happily shorten it as
> you suggest ;-).

Yeah, in retrospect I should have not have even mentioned it. test_commit() already adds a bunch of extra processes you may or may not care about (e.g., by making tags, or using "git add" when "commit -a" might do).

Show 8 quoted lines

> > > +	cat >expect <<-EOF &&
> > > +	$(git rev-parse a) .have
> > > +	$(git rev-parse c) .have
> > > +	EOF
> > > +	printf "0000" | git receive-pack fork | extract_haves >actual &&
> >
> > There's been a push lately to avoid having git on the left-hand side of
> > a fork, since we might otherwise miss its exit code (including things

Heh, I meant to say "left-hand side of a pipe", but you obviously figured out what I meant. :)

-Peff