On Tue, Sep 25, 2018 at 11:33:37PM -0400, Jeff King wrote:
> 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.
> 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".
> > 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
