Re: [PATCH v2 2/3] transport.c: introduce core.alternateRefsCommand
On Wed, Sep 26, 2018 at 02:38:53PM -0400, Jeff King wrote:
> On Wed, Sep 26, 2018 at 06:39:56AM -0700, Taylor Blau wrote:
>
> > > A perl tangent if you're interested:
> > [...]
> >
> > To be clear, we ought to leave this function as:
> >
> > extract_haves () {
> > depacketize | perl -lne '/^(\S+) \.have/ and print $1'
> > }
>
> Yes, I agree. You cannot do the "$@" there because it relies on
> depacketize, which only handles stdin.
>
> > Or are you suggesting that we change it to:
> >
> > extract_haves () {
> > perl -lne '/^(\S+) \.have/ and print $1'
> > }
>
> No, sorry. I just used the ".have" snippet as filler text, but I see
> that muddied my meaning considerably. This really was just a tangent for
> the future. What you've written above is the best thing for this case.
I see, and I had assumed that you meant the later, not that including
" .have" was a good way to go forward. So I think that we're in
agreement here.
> > And call it as:
> >
> > printf "" | git receive-pack fork >actual &&
> > depacketize actual.packets
> > extract_haves 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".
>
> Yeah. I have no problem with the three-liner you wrote above, but I do
> not see any particular reason for it.
Good. That's the version that I'll send shortly, then.
Thanks,
Taylor
Re: [PATCH v2 2/3] transport.c: introduce core.alternateRefsCommand
On Wed, Sep 26, 2018 at 06:39:56AM -0700, Taylor Blau wrote:
> > A perl tangent if you're interested:
> [...]
>
> To be clear, we ought to leave this function as:
>
> extract_haves () {
> depacketize | perl -lne '/^(\S+) \.have/ and print $1'
> }
Yes, I agree. You cannot do the "$@" there because it relies on
depacketize, which only handles stdin.
> Or are you suggesting that we change it to:
>
> extract_haves () {
> perl -lne '/^(\S+) \.have/ and print $1'
> }
No, sorry. I just used the ".have" snippet as filler text, but I see
that muddied my meaning considerably. This really was just a tangent for
the future. What you've written above is the best thing for this case.
> And call it as:
>
> printf "" | git receive-pack fork >actual &&
> depacketize actual.packets
> extract_haves 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".
Yeah. I have no problem with the three-liner you wrote above, but I do
not see any particular reason for it.
-Peff
Re: [PATCH v2 2/3] transport.c: introduce core.alternateRefsCommand
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
> 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 "" | git receive-pack fork >actual &&
depacketize actual.packets
extract_haves 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
Re: [PATCH v2 2/3] transport.c: introduce core.alternateRefsCommand
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.
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 )" 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. ;)
Just skimming through test-lib-functions.sh, though, it does seem that
we often deviate from that pattern (e.g., all of the q_to_nul family).
And has seemed to mind.
> 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.
-Peff
Re: [PATCH v2 2/3] transport.c: introduce core.alternateRefsCommand
On Tue, Sep 25, 2018 at 06:06:06PM -0700, Taylor Blau wrote:
> > > +extract_haves () {
> > > + depacketize - | grep '\.have' | sed -e 's/\\0.*$//g'
> > > +}
> >
> > Don't pipe grep into sed, especially when both the pattern to filter
> > and the operation to perform are simple.
> >
> > I am not sure what you are trying to achive with 'g' in
> > s/pattern$//g; The anchor at the rightmost end of the pattern makes
> > sure that the pattern matches only once per line at the end anyway,
> > so "do this howmanyever times as we have match on each line" would
> > not make any difference, no?
>
> I admit to not fully understanding when the trailing `/g` is and is not
> useful. Anyway, I took Peff's suggestion below to convert this 'grep |
> sed' pipeline into a Perl invocation, which I think ended up much
> cleaner.
It makes the replacement global in the line. Without we substitute only
the first match. So try:
echo foo | sed s/o/X/
versus:
echo foo | sed s/o/X/g
-Peff
Re: [PATCH v2 2/3] transport.c: introduce core.alternateRefsCommand
On Sat, Sep 22, 2018 at 03:52:58PM -0400, Jeff King wrote:
> On Sat, Sep 22, 2018 at 06:02:31PM +, brian m. carlson wrote:
>
> > On Fri, Sep 21, 2018 at 02:47:43PM -0400, Taylor Blau wrote:
> > > +expect_haves () {
> > > + printf "%s .have\n" $(git rev-parse $@) >expect
> > > +}
> > > +
> > > +extract_haves () {
> > > + depacketize - | grep '\.have' | sed -e 's/\\0.*$//g'
> >
> > It looks like you're trying to match a NUL here in the sed expression,
> > but from my reading of it, POSIX doesn't permit BREs to match NUL.
>
> No, it's trying to literally match backslash followed by 0. The
> depacketize() script will have undone the NUL already. In perl, no less,
> making it more or less equivalent to your suggestion. ;)
>
> 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').
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.
> Or the equivalent in sed. I am happy with any solution that does the
> correct thing.
Me too :-). Thanks again.
Thanks,
Taylor
Re: [PATCH v2 2/3] transport.c: introduce core.alternateRefsCommand
On Fri, Sep 21, 2018 at 02:09:08PM -0700, Junio C Hamano wrote:
> Taylor Blau writes:
>
> > +core.alternateRefsCommand::
> > + When listing references from an alternate (e.g., in the case of
> > ".have"), use
>
> It is not clear how (e.g.,...) connects to what is said in the
> sentence. "When advertising tips of available history from an
> alternate, use ..." without saying ".have" may be less cryptic.
>
> I dunno.
Thanks, I think that I tend to overuse both "e.g.," and "i.e.,". I took
your suggestion as above, which I think looks better than my original
prose.
> > + the shell to execute the specified command instead of
> > + linkgit:git-for-each-ref[1]. The first argument is the path of the
> > alternate.
>
> "The path" meaning the absolute path? Relative to the original
> object store? Something else?
It's the absolute path, and I've updated the documentation to clarify it
as such.
> > + Output must be of the form: `%(objectname) SPC %(refname)`.
> > ++
> > +This is useful when a repository only wishes to advertise some of its
> > +alternate's references as ".have"'s. For example, to only advertise branch
> > +heads, configure `core.alternateRefsCommand` to the path of a script which
> > runs
> > +`git --git-dir="$1" for-each-ref refs/heads`.
> > +
> > core.bare::
> > If true this repository is assumed to be 'bare' and has no
> > working directory associated with it. If this is the case a
> > diff --git a/t/t5410-receive-pack.sh b/t/t5410-receive-pack.sh
> > new file mode 100755
> > index 00..2f21f1cb8f
> > --- /dev/null
> > +++ b/t/t5410-receive-pack.sh
> > @@ -0,0 +1,54 @@
> > +#!/bin/sh
> > +
> > +test_description='git receive-pack test'
> > +
> > +. ./test-lib.sh
> > +
> > +test_expect_success 'setup' '
> > + test_commit one &&
> > + git update-ref refs/heads/a HEAD &&
> > + test_commit two &&
> > + git update-ref refs/heads/b HEAD &&
> > + test_commit three &&
> > + git update-ref refs/heads/c HEAD &&
> > + git clone --bare . fork &&
> > + git clone fork pusher &&
> > + (
> > + cd fork &&
> > + git config receive.advertisealternates true &&
>
> Hmph. Do we have code to support this configuration variable?
We don't ;-). Peff's explanation of why is accurate, and the mistake is
mine.
> > + cat <<-EOF | git update-ref --stdin &&
>
> Style: writing "<<-\EOF" instead would allow readers' eyes to
> coast over without having to look for $variable_references in
> the here-doc.
>
> > + delete refs/heads/a
> > + delete refs/heads/b
> > + delete refs/heads/c
> > + delete refs/heads/master
> > + delete refs/tags/one
> > + delete refs/tags/two
> > + delete refs/tags/three
Thanks, it ended up being much cleaner to write <<-\EOF, and avoid the
unnecessary cat(1) entirely.
> So, the original created one/two/three/a/b/c/master, fork is a bare
> clone of it and has all these things, and then you deleted all of
> these? What does fork have after this is done? HEAD that is
> dangling?
>
> > + EOF
> > + echo "../../.git/objects" >objects/info/alternates
>
> When viewed from fork/objects, ../../.git is the GIT_DIR of the
> primary test repository, so that is where we borrow objects from.
>
> If we pruned the objects from fork's object store before this echo,
> we would have an almost empty repository that borrows from its
> alternates everything, which may make a more realistic sample case,
> but because you are only focusing on the ref advertisement, it does
> not matter that your fork is full of duplicate objects that are
> available from the alternates.
I could go either way. You're right in that we have only a dangling HEAD
reference in the fork, and that all of the objects are still there. I
suppose that we could gc the objects that are there, but I think (as you
note above) that it doesn't make a huge difference either way.
> > +expect_haves () {
> > + printf "%s .have\n" $(git rev-parse $@) >expect
>
> Quote $@ inside dq pair, like $(git rev-parse "$@").
Thanks, I fixed this (per your and Eric's suggestion), but ended up
removing the function entirely anyway.
> > +extract_haves () {
> > + depacketize - | grep '\.have' | sed -e 's/\\0.*$//g'
> > +}
>
> Don't pipe grep into sed, especially when both the pattern to filter
> and the operation to perform are simple.
>
> I am not sure what you are trying to achive with 'g' in
> s/pattern$//g; The anchor at the rightmost end of the pattern makes
> sure that the pattern matches only once per line at the end anyway,
> so "do this howmanyever times as we have match on each line" would
> not make any difference, no?
I admit to not fully understanding when the trailing `/g` is and is not
useful. Anyway, I took Peff's suggestion below to convert this 'grep |
sed' pipeline into a Perl invocation, which I think ended up much
cleaner.
Thanks,
Taylor
Re: [PATCH v2 2/3] transport.c: introduce core.alternateRefsCommand
On Fri, Sep 21, 2018 at 04:18:03PM -0400, Eric Sunshine wrote:
> On Fri, Sep 21, 2018 at 2:47 PM Taylor Blau wrote:
> > When in a repository containing one or more alternates, Git would
> > sometimes like to list references from its alternates. For example, 'git
> > receive-pack' list the objects pointed to by alternate references as
> > special ".have" references.
> > [...]
> > Signed-off-by: Taylor Blau
> > ---
> > diff --git a/t/t5410-receive-pack.sh b/t/t5410-receive-pack.sh
> > @@ -0,0 +1,54 @@
> > +expect_haves () {
> > + printf "%s .have\n" $(git rev-parse $@) >expect
> > +}
>
> Magic quoting behavior only kicks in when $@ is itself quoted, so this
> should be:
>
> printf "%s .have\n" $(git rev-parse "$@") >expect
>
> However, as it's unlikely that you need magic quoting in this case,
> you might get by with plain $* (unquoted).
Yep, thanks for catching my mistake. I rewrote my local copy with "$@"
(instead of $@), and also applied your suggestion of not redirecting to
`>expect`, and renaming the function.
These both ended up becoming moot points, though, because of the
Perl-ism that Peff suggested and I adopted throughout this thread.
The Perl Peff wrote does not capture the " .have" suffix at all, and
instead only the object identifiers. Hence, all we really need is a call
to 'git-rev-parse(1)'. I doubt that this will ever change, so I removed
the function entirely.
Thanks,
Taylor
Re: [PATCH v2 2/3] transport.c: introduce core.alternateRefsCommand
On Sat, Sep 22, 2018 at 03:52:58PM -0400, Jeff King wrote:
> On Sat, Sep 22, 2018 at 06:02:31PM +, brian m. carlson wrote:
>
> > On Fri, Sep 21, 2018 at 02:47:43PM -0400, Taylor Blau wrote:
> > > +expect_haves () {
> > > + printf "%s .have\n" $(git rev-parse $@) >expect
> > > +}
> > > +
> > > +extract_haves () {
> > > + depacketize - | grep '\.have' | sed -e 's/\\0.*$//g'
> >
> > It looks like you're trying to match a NUL here in the sed expression,
> > but from my reading of it, POSIX doesn't permit BREs to match NUL.
>
> No, it's trying to literally match backslash followed by 0. The
> depacketize() script will have undone the NUL already. In perl, no less,
> making it more or less equivalent to your suggestion. ;)
Ah, okay. That makes more sense.
> 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'
>
> Or the equivalent in sed. I am happy with any solution that does the
> correct thing.
Yeah, I agree that with that context, no change is needed.
--
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204
signature.asc
Description: PGP signature
Re: [PATCH v2 2/3] transport.c: introduce core.alternateRefsCommand
On Sat, Sep 22, 2018 at 06:02:31PM +, brian m. carlson wrote:
> On Fri, Sep 21, 2018 at 02:47:43PM -0400, Taylor Blau wrote:
> > +expect_haves () {
> > + printf "%s .have\n" $(git rev-parse $@) >expect
> > +}
> > +
> > +extract_haves () {
> > + depacketize - | grep '\.have' | sed -e 's/\\0.*$//g'
>
> It looks like you're trying to match a NUL here in the sed expression,
> but from my reading of it, POSIX doesn't permit BREs to match NUL.
No, it's trying to literally match backslash followed by 0. The
depacketize() script will have undone the NUL already. In perl, no less,
making it more or less equivalent to your suggestion. ;)
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'
Or the equivalent in sed. I am happy with any solution that does the
correct thing.
-Peff
Re: [PATCH v2 2/3] transport.c: introduce core.alternateRefsCommand
On Fri, Sep 21, 2018 at 02:47:43PM -0400, Taylor Blau wrote:
> +expect_haves () {
> + printf "%s .have\n" $(git rev-parse $@) >expect
> +}
> +
> +extract_haves () {
> + depacketize - | grep '\.have' | sed -e 's/\\0.*$//g'
It looks like you're trying to match a NUL here in the sed expression,
but from my reading of it, POSIX doesn't permit BREs to match NUL.
Perhaps someone can come up with a better solution, but I'd write this
as the following:
depacketize - | perl -ne 'next unless /\.have/; s/\0.*$//g; print'
--
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204
signature.asc
Description: PGP signature
Re: [PATCH v2 2/3] transport.c: introduce core.alternateRefsCommand
On Fri, Sep 21, 2018 at 03:23:40PM -0700, Junio C Hamano wrote: > >> > +git config receive.advertisealternates true && > >> > >> Hmph. Do we have code to support this configuration variable? > > > > Sorry, I should have caught that. Our existing solution is to disable > > alternates in the advertisement entirely (since the optimization > > backfires for us). So this line is a leftover from testing it against > > our fork, and should be dropped. > > > > If anybody is interested, we can share those patches, though they're > > unsurprisingly trivial. > > Heh, I guessed correctly what is going on ;-) > > Even though there may not be much interest in the "all-or-none" > boolean configuration, in order to upstream this custom thing, it > may be the cleanest to upstream that all-or-none thing as well. > Otherwise, you'd need to keep a patch to this test script that is > private for your "all-or-none" feature. That's your maintenance > burden so it ultimately is your call ;-) Easy one-liners in test scripts are the least of my ongoing maintenance burden. ;) I think in this case, though, the line is not even necessary, as our patches leave the default as "true" (which is certainly what we would want upstream, as well, for compatibility). -Peff
Re: [PATCH v2 2/3] transport.c: introduce core.alternateRefsCommand
Jeff King writes: > On Fri, Sep 21, 2018 at 02:09:08PM -0700, Junio C Hamano wrote: > >> > +test_expect_success 'setup' ' >> > + test_commit one && >> > + git update-ref refs/heads/a HEAD && >> > + test_commit two && >> > + git update-ref refs/heads/b HEAD && >> > + test_commit three && >> > + git update-ref refs/heads/c HEAD && >> > + git clone --bare . fork && >> > + git clone fork pusher && >> > + ( >> > + cd fork && >> > + git config receive.advertisealternates true && >> >> Hmph. Do we have code to support this configuration variable? > > Sorry, I should have caught that. Our existing solution is to disable > alternates in the advertisement entirely (since the optimization > backfires for us). So this line is a leftover from testing it against > our fork, and should be dropped. > > If anybody is interested, we can share those patches, though they're > unsurprisingly trivial. Heh, I guessed correctly what is going on ;-) Even though there may not be much interest in the "all-or-none" boolean configuration, in order to upstream this custom thing, it may be the cleanest to upstream that all-or-none thing as well. Otherwise, you'd need to keep a patch to this test script that is private for your "all-or-none" feature. That's your maintenance burden so it ultimately is your call ;-) > Also, useless-use-of-cat in the original, which could be: > > git update-ref --stdin <<-\EOF Yup. Thanks.
Re: [PATCH v2 2/3] transport.c: introduce core.alternateRefsCommand
On Fri, Sep 21, 2018 at 02:09:08PM -0700, Junio C Hamano wrote: > > +test_expect_success 'setup' ' > > + test_commit one && > > + git update-ref refs/heads/a HEAD && > > + test_commit two && > > + git update-ref refs/heads/b HEAD && > > + test_commit three && > > + git update-ref refs/heads/c HEAD && > > + git clone --bare . fork && > > + git clone fork pusher && > > + ( > > + cd fork && > > + git config receive.advertisealternates true && > > Hmph. Do we have code to support this configuration variable? Sorry, I should have caught that. Our existing solution is to disable alternates in the advertisement entirely (since the optimization backfires for us). So this line is a leftover from testing it against our fork, and should be dropped. If anybody is interested, we can share those patches, though they're unsurprisingly trivial. I suspect we may end up discarding them if this custom-command thing works, but it's possible we'll still need to be able to shut them off completely for some truly pathological cases. > > + cat <<-EOF | git update-ref --stdin && > > Style: writing "<<-\EOF" instead would allow readers' eyes to > coast over without having to look for $variable_references in > the here-doc. Also, useless-use-of-cat in the original, which could be: git update-ref --stdin <<-\EOF > [...] Yeah, I second all the other bits you mentioned. -Peff
Re: [PATCH v2 2/3] transport.c: introduce core.alternateRefsCommand
On Fri, Sep 21, 2018 at 2:47 PM Taylor Blau wrote:
> When in a repository containing one or more alternates, Git would
> sometimes like to list references from its alternates. For example, 'git
> receive-pack' list the objects pointed to by alternate references as
> special ".have" references.
> [...]
> Signed-off-by: Taylor Blau
> ---
> diff --git a/t/t5410-receive-pack.sh b/t/t5410-receive-pack.sh
> @@ -0,0 +1,54 @@
> +expect_haves () {
> + printf "%s .have\n" $(git rev-parse $@) >expect
> +}
> +
> +test_expect_success 'with core.alternateRefsCommand' '
> + [...]
> + expect_haves a c >expect &&
This is not great. Both the caller of expect_haves() and
expect_haves() itself redirect to a file named "expect". This works,
but only by accident.
Better would be to make expect_haves() simply a generator to stdout
and let the caller redirect to the file rather than hardcoding the
filename in the function itself (much as extract_haves() takes it its
input on stdin rather than hardcoding a filename). If you take this
approach, then you'd probably want to rename the function, as well;
perhaps call it emit_haves() or something.
> + printf "" | git receive-pack fork >actual &&
> + extract_haves actual.haves &&
> + test_cmp expect actual.haves
> +'
Re: [PATCH v2 2/3] transport.c: introduce core.alternateRefsCommand
Taylor Blau writes:
> +core.alternateRefsCommand::
> + When listing references from an alternate (e.g., in the case of
> ".have"), use
It is not clear how (e.g.,...) connects to what is said in the
sentence. "When advertising tips of available history from an
alternate, use ..." without saying ".have" may be less cryptic.
I dunno.
> + the shell to execute the specified command instead of
> + linkgit:git-for-each-ref[1]. The first argument is the path of the
> alternate.
"The path" meaning the absolute path? Relative to the original
object store? Something else?
> + Output must be of the form: `%(objectname) SPC %(refname)`.
> ++
> +This is useful when a repository only wishes to advertise some of its
> +alternate's references as ".have"'s. For example, to only advertise branch
> +heads, configure `core.alternateRefsCommand` to the path of a script which
> runs
> +`git --git-dir="$1" for-each-ref refs/heads`.
> +
> core.bare::
> If true this repository is assumed to be 'bare' and has no
> working directory associated with it. If this is the case a
> diff --git a/t/t5410-receive-pack.sh b/t/t5410-receive-pack.sh
> new file mode 100755
> index 00..2f21f1cb8f
> --- /dev/null
> +++ b/t/t5410-receive-pack.sh
> @@ -0,0 +1,54 @@
> +#!/bin/sh
> +
> +test_description='git receive-pack test'
> +
> +. ./test-lib.sh
> +
> +test_expect_success 'setup' '
> + test_commit one &&
> + git update-ref refs/heads/a HEAD &&
> + test_commit two &&
> + git update-ref refs/heads/b HEAD &&
> + test_commit three &&
> + git update-ref refs/heads/c HEAD &&
> + git clone --bare . fork &&
> + git clone fork pusher &&
> + (
> + cd fork &&
> + git config receive.advertisealternates true &&
Hmph. Do we have code to support this configuration variable?
> + cat <<-EOF | git update-ref --stdin &&
Style: writing "<<-\EOF" instead would allow readers' eyes to
coast over without having to look for $variable_references in
the here-doc.
> + delete refs/heads/a
> + delete refs/heads/b
> + delete refs/heads/c
> + delete refs/heads/master
> + delete refs/tags/one
> + delete refs/tags/two
> + delete refs/tags/three
So, the original created one/two/three/a/b/c/master, fork is a bare
clone of it and has all these things, and then you deleted all of
these? What does fork have after this is done? HEAD that is
dangling?
> + EOF
> + echo "../../.git/objects" >objects/info/alternates
When viewed from fork/objects, ../../.git is the GIT_DIR of the
primary test repository, so that is where we borrow objects from.
If we pruned the objects from fork's object store before this echo,
we would have an almost empty repository that borrows from its
alternates everything, which may make a more realistic sample case,
but because you are only focusing on the ref advertisement, it does
not matter that your fork is full of duplicate objects that are
available from the alternates.
> +expect_haves () {
> + printf "%s .have\n" $(git rev-parse $@) >expect
Quote $@ inside dq pair, like $(git rev-parse "$@").
> +extract_haves () {
> + depacketize - | grep '\.have' | sed -e 's/\\0.*$//g'
> +}
Don't pipe grep into sed, especially when both the pattern to filter
and the operation to perform are simple.
I am not sure what you are trying to achive with 'g' in
s/pattern$//g; The anchor at the rightmost end of the pattern makes
sure that the pattern matches only once per line at the end anyway,
so "do this howmanyever times as we have match on each line" would
not make any difference, no?
> diff --git a/transport.c b/transport.c
> index 24ae3f375d..e7d2cdf00b 100644
> --- a/transport.c
> +++ b/transport.c
> @@ -1328,10 +1328,21 @@ char *transport_anonymize_url(const char *url)
> static void fill_alternate_refs_command(struct child_process *cmd,
> const char *repo_path)
> {
> - cmd->git_cmd = 1;
> - argv_array_pushf(&cmd->args, "--git-dir=%s", repo_path);
> - argv_array_push(&cmd->args, "for-each-ref");
> - argv_array_push(&cmd->args, "--format=%(objectname) %(refname)");
> + const char *value;
> +
> + if (!git_config_get_value("core.alternateRefsCommand", &value)) {
> + cmd->use_shell = 1;
> +
> + argv_array_push(&cmd->args, value);
> + argv_array_push(&cmd->args, repo_path);
> + } else {
> + cmd->git_cmd = 1;
> +
> + argv_array_pushf(&cmd->args, "--git-dir=%s", repo_path);
> + argv_array_push(&cmd->args, "for-each-ref");
> + argv_array_push(&cmd->args, "--format=%(objectname)
> %(refname)");
> + }
> +
> cmd->env = local_repo_env;
> cmd->out = -1;
> }
Re: [PATCH v2 2/3] transport.c: introduce core.alternateRefsCommand
On Fri, Sep 21, 2018 at 2:47 PM Taylor Blau wrote:
> When in a repository containing one or more alternates, Git would
> sometimes like to list references from its alternates. For example, 'git
> receive-pack' list the objects pointed to by alternate references as
> special ".have" references.
> [...]
> Signed-off-by: Taylor Blau
> ---
> diff --git a/t/t5410-receive-pack.sh b/t/t5410-receive-pack.sh
> @@ -0,0 +1,54 @@
> +expect_haves () {
> + printf "%s .have\n" $(git rev-parse $@) >expect
> +}
Magic quoting behavior only kicks in when $@ is itself quoted, so this
should be:
printf "%s .have\n" $(git rev-parse "$@") >expect
However, as it's unlikely that you need magic quoting in this case,
you might get by with plain $* (unquoted).
