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

From: Taylor Blau <me@ttaylorr.com>
Date: Sep 26, 2018, 13:39 UTC
Message-ID: <20180926133956.GA25697@syl>
In-Reply-To: <20180926033337.GC32064@sigill.intra.peff.net>

On Tue, Sep 25, 2018 at 11:33:37PM -0400, Jeff King wrote:

Show 14 quoted lines

> On Tue, Sep 25, 2018 at 06:09:35PM -0700, Taylor Blau wrote:
>
> > > So I think this is fine (modulo that the grep and sed can be combined).
> > > Yet another option would be to simply strip away everything except the
> > > object id (which is all we care about), like:
> > >
> > >   depacketize | perl -lne '/^(\S+) \.have/ and print $1'
> >
> > Thanks for this. This is the suggestion I ended up taking (modulo taking
> > '-' as the first argument to 'depacketize').
>
> I don't think depacketize takes any arguments. It always reads from
> stdin directly, doesn't it? Your "-" is not hurting anything, but it is
> totally ignored.

Yep, certainly. I think that I was drawn to this claim because I watched t5410 fail after applying the above recommendation, so thusly assumed that it was my fault for not passing `-` to 'depacketize()`.

In the end, I'm not sure why the test failed originally (it's likely that I hadn't removed the ".have" part of 'expect_haves()', yet). But, I removed the `-` in my local copy of v3, and the tests passes on all revisions of this series that have it.

Show 24 quoted lines

> A perl tangent if you're interested:
>
>   Normally for shell functions like this that are just wrappers around
>   perl snippets, I would suggest to pass "$@" from the function's
>   arguments to perl. So for example if we had:
>
>     haves_from_packets () {
> 	perl -lne '/^(\S+) \.have/ and print $1' "$@"
>     }
>
>   then you could call it with a filename:
>
>     haves_from_packets packets
>
>   or input on stdin:
>
>     haves_from_packets <packets
>
>   and either works (this is magic from perl's "-p" loop, but you get the
>   same if you write "while (<>)" explicitly in your program).
>
>   But because depacketize() has to use byte-wise read() calls, it
>   doesn't get that magic for free. And it did not seem worth the effort
>   to implement, when shell redirections are so easy. ;)

To be clear, we ought to leave this function as:

  extract_haves () {
    depacketize | perl -lne '/^(\S+) \.have/ and print $1'
  }

Or are you suggesting that we change it to:

  extract_haves () {
    perl -lne '/^(\S+) \.have/ and print $1'
  }

And call it as:

  printf "0000" | git receive-pack fork >actual &&
  depacketize <actual >actual.packets
  extract_haves <actual.packets >actual.haves &&

Frankly, (and I think that this is what you're getting at in your reply above), I think that the former (e.g., calling 'depacketize()' in 'extract_haves()') is cleaner. This approach leaves us with "actual" and "actual.haves", and obviates the need for another intermediary, "actual.packets".

Show 6 quoted lines

> > The 'print $1' part of this makes things a lot nicer, actually, having
> > removed the " .have" suffix. We can get rid of the expect_haves()
> > function above, and instead call 'git rev-parse' inline and get the
> > right results.
>
> Yes. You can even do it all in a single rev-parse call.

Indeed.

Thanks, Taylor