Re: [PATCH 2/3] transport.c: introduce core.alternateRefsCommand
On Fri, Sep 21, 2018 at 12:59:16PM -0700, Junio C Hamano wrote:
> Taylor Blau writes:
>
> > In fact, I think that we can go even further: since we don't need to
> > catch the beginning '^.*' (without -o), we can instead:
> >
> > extract_haves () {
> > depacketize - | grep '\.have' | sed -e 's/\\0.*$//g'
> > }
>
> Do not pipe grep into sed, unless you have an overly elaborate set
> of patterns to filter with, e.g. something along the lines of...
>
> sed -ne '/\.have/s/...//p'
Thanks, I'm not sure why I thought that this was a good idea to send
(even after discussing it to myself twice publicly on the list
beforehand).
Anyway, in my local copy, I adopted Peff's suggestion below in the
thread, which is:
extract_haves () {
depacketize - | perl -lne '/^(\S+) \.have/ and print $1'
}
I think that that should be OK, but I sent it here to double check
before sending you real patches.
Thanks,
Taylor
Re: [PATCH 2/3] transport.c: introduce core.alternateRefsCommand
Taylor Blau writes:
> In fact, I think that we can go even further: since we don't need to
> catch the beginning '^.*' (without -o), we can instead:
>
> extract_haves () {
> depacketize - | grep '\.have' | sed -e 's/\\0.*$//g'
> }
Do not pipe grep into sed, unless you have an overly elaborate set
of patterns to filter with, e.g. something along the lines of...
sed -ne '/\.have/s/...//p'
Re: [PATCH 2/3] transport.c: introduce core.alternateRefsCommand
On Fri, Sep 21, 2018 at 01:48:25PM -0400, Taylor Blau wrote:
> On Fri, Sep 21, 2018 at 09:39:14AM -0700, Junio C Hamano wrote:
> > Taylor Blau writes:
> >
> > > +extract_haves () {
> > > + depacketize - | grep -o '^.* \.have'
> >
> > Not portable, isn't it?
> >
> > cf. http://pubs.opengroup.org/onlinepubs/9699919799/utilities/grep.html
>
> Good catch. Definitely not portable, per the link that you shared above.
>
> Since 'depacketize()' will give us a "\0", we can pull it and anything
> after it out with 'sed', instead. Any lines that don't contain a "\0"
> only contain an OID and the literal, ".have", and are fine as-is.
>
> Something like this:
>
> extract_haves () {
> depacketize - | grep '^.* \.have' | sed -e 's/\\0.*$//g'
> }
>
> Harder to read--at least for me--but infinitely more portable.
In fact, I think that we can go even further: since we don't need to
catch the beginning '^.*' (without -o), we can instead:
extract_haves () {
depacketize - | grep '\.have' | sed -e 's/\\0.*$//g'
}
Thanks,
Taylor
Re: [PATCH 2/3] transport.c: introduce core.alternateRefsCommand
On Fri, Sep 21, 2018 at 09:39:14AM -0700, Junio C Hamano wrote:
> Taylor Blau writes:
>
> > +extract_haves () {
> > + depacketize - | grep -o '^.* \.have'
>
> Not portable, isn't it?
>
> cf. http://pubs.opengroup.org/onlinepubs/9699919799/utilities/grep.html
Good catch. Definitely not portable, per the link that you shared above.
Since 'depacketize()' will give us a "\0", we can pull it and anything
after it out with 'sed', instead. Any lines that don't contain a "\0"
only contain an OID and the literal, ".have", and are fine as-is.
Something like this:
extract_haves () {
depacketize - | grep '^.* \.have' | sed -e 's/\\0.*$//g'
}
Harder to read--at least for me--but infinitely more portable.
I'll wait until a little later today, and then send you v2. Thanks for
reviewing :-).
Thanks,
Taylor
Re: [PATCH 2/3] transport.c: introduce core.alternateRefsCommand
Taylor Blau writes:
> +extract_haves () {
> + depacketize - | grep -o '^.* \.have'
Not portable, isn't it?
cf. http://pubs.opengroup.org/onlinepubs/9699919799/utilities/grep.html
Re: [PATCH 2/3] transport.c: introduce core.alternateRefsCommand
On Thu, Sep 20, 2018 at 04:00:34PM -0400, Taylor Blau wrote: > > 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. > > 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). > > > + cat >expect <<-EOF && > > > + $(git rev-parse a) .have > > > + $(git rev-parse c) .have > > > + EOF > > > + printf "" | 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
Re: [PATCH 2/3] transport.c: introduce core.alternateRefsCommand
On Thu, Sep 20, 2018 at 03:37:51PM -0400, Jeff King wrote: > On Thu, Sep 20, 2018 at 02:04:11PM -0400, Taylor Blau wrote: > > > diff --git a/Documentation/config.txt b/Documentation/config.txt > > index 112041f407..b908bc5825 100644 > > --- a/Documentation/config.txt > > +++ b/Documentation/config.txt > > @@ -616,6 +616,12 @@ core.preferSymlinkRefs:: > > This is sometimes needed to work with old scripts that > > expect HEAD to be a symbolic link. > > > > +core.alternateRefsCommand:: > > + When listing references from an alternate (e.g., in the case of > > ".have"), use > > + the shell to execute the specified command instead of > > + linkgit:git-for-each-ref[1]. The first argument is the path of the > > alternate. > > + Output must be of the form: `%(objectname) SPC %(refname)`. > > We discussed off-list the notion that this could just be the objectname, > since the ".have" mechanism doesn't care about the actual refnames. > > There's a little prior discussion from the list: > > https://public-inbox.org/git/xmqqefzraqbu@gitster.mtv.corp.google.com/ > > 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. > > +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 && > > + git update-ref -d refs/heads/a && > > + git update-ref -d refs/heads/b && > > + git update-ref -d refs/heads/c && > > + git update-ref -d refs/heads/master && > > + git update-ref -d refs/tags/one && > > + git update-ref -d refs/tags/two && > > + git update-ref -d refs/tags/three && > > 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 ;-). > > + printf "../../.git/objects" >objects/info/alternates > > Also a nitpick, but I think "echo" would be more usual here (we handle > the lack of a trailing newline just fine, but any use of printf makes me > wonder if something tricky is going on with line endings). 'echo' indeed seems to be the way to go. This 'printf' preference is a Git LFS-ism ;-). > > +test_expect_success 'with core.alternateRefsCommand' ' > > + test_config -C fork core.alternateRefsCommand \ > > + "git --git-dir=\"\$1\" for-each-ref \ > > + --format=\"%(objectname) %(refname)\" \ > > + refs/heads/a refs/heads/c;:" && > > This is cute and all, but might it be more readable to use > write_script() to stick it into its own script? Good idea, I'll do that. > > + cat >expect <<-EOF && > > + $(git rev-parse a) .have > > + $(git rev-parse c) .have > > + EOF > > + printf "" | 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 > like asan/valgrind errors). So maybe: > >... receive-pack fork >actual && >extract_haves actual.haves && >test_cmp expect actual.haves > > or similar? Sure, I agree that it's a good idea to not miss the exit code (since we don't have pipefail on), etc. I adopted your suggestion into my local copy. > > diff --git a/transport.c b/transport.c > > index 24ae3f375d..e7d2cdf00b 100644 > > --- a/transport.c
Re: [PATCH 2/3] transport.c: introduce core.alternateRefsCommand
On Thu, Sep 20, 2018 at 02:04:11PM -0400, Taylor Blau wrote:
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 112041f407..b908bc5825 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -616,6 +616,12 @@ core.preferSymlinkRefs::
> This is sometimes needed to work with old scripts that
> expect HEAD to be a symbolic link.
>
> +core.alternateRefsCommand::
> + When listing references from an alternate (e.g., in the case of
> ".have"), use
> + the shell to execute the specified command instead of
> + linkgit:git-for-each-ref[1]. The first argument is the path of the
> alternate.
> + Output must be of the form: `%(objectname) SPC %(refname)`.
We discussed off-list the notion that this could just be the objectname,
since the ".have" mechanism doesn't care about the actual refnames.
There's a little prior discussion from the list:
https://public-inbox.org/git/xmqqefzraqbu@gitster.mtv.corp.google.com/
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.
> +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 &&
> + git update-ref -d refs/heads/a &&
> + git update-ref -d refs/heads/b &&
> + git update-ref -d refs/heads/c &&
> + git update-ref -d refs/heads/master &&
> + git update-ref -d refs/tags/one &&
> + git update-ref -d refs/tags/two &&
> + git update-ref -d refs/tags/three &&
Probably not worth nit-picking process count, but this could done with a
single "update-ref --stdin".
> + printf "../../.git/objects" >objects/info/alternates
Also a nitpick, but I think "echo" would be more usual here (we handle
the lack of a trailing newline just fine, but any use of printf makes me
wonder if something tricky is going on with line endings).
> +test_expect_success 'with core.alternateRefsCommand' '
> + test_config -C fork core.alternateRefsCommand \
> + "git --git-dir=\"\$1\" for-each-ref \
> + --format=\"%(objectname) %(refname)\" \
> + refs/heads/a refs/heads/c;:" &&
This is cute and all, but might it be more readable to use
write_script() to stick it into its own script?
> + cat >expect <<-EOF &&
> + $(git rev-parse a) .have
> + $(git rev-parse c) .have
> + EOF
> + printf "" | 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
like asan/valgrind errors). So maybe:
... receive-pack fork >actual &&
extract_haves actual.haves &&
test_cmp expect actual.haves
or similar?
> 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(>args, "--git-dir=%s", repo_path);
> - argv_array_push(>args, "for-each-ref");
> - argv_array_push(>args, "--format=%(objectname) %(refname)");
> + const char *value;
> +
> + if (!git_config_get_value("core.alternateRefsCommand", )) {
> + cmd->use_shell = 1;
> +
> + argv_array_push(>args, value);
> + argv_array_push(>args, repo_path);
Setting use_shell allows the shell trickery in your test, and matches
the modern way we run config-based commands. Good.
> + } else {
> + cmd->git_cmd = 1;
> +
> + argv_array_pushf(>args, "--git-dir=%s", repo_path);
> + argv_array_push(>args, "for-each-ref");
> + argv_array_push(>args, "--format=%(objectname)
> %(refname)");
> + }
> +
> cmd->env = local_repo_env;
> cmd->out = -1;
And we still clear local_repo_env for the custom
[PATCH 2/3] transport.c: introduce core.alternateRefsCommand
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.
Listing ".have" references is designed to make pushing changes from
upstream to a fork a lightweight operation, by advertising to the pusher
that the fork already has the objects (via its alternate). Thus, the
client can avoid sending them.
However, when the alternate has a pathologically large number of
references, the initial advertisement is too expensive. In fact, it can
dominate any such optimization where the pusher avoids sending certain
objects.
Introduce "core.alternateRefsCommand" in order to provide a facility to
limit or filter alternate references. This can be used, for example, to
filter out "uninteresting" references from the initial advertisement in
the above scenario.
Let the repository that has alternates configure this command to avoid
trusting the alternate to provide us a safe command to run in the shell.
To behave differently on each alternate (e.g., only list tags from
alternate A, only heads from B) provide the path of the alternate as the
first argument.
Signed-off-by: Taylor Blau
---
Documentation/config.txt | 6 +
t/t5410-receive-pack.sh | 47
transport.c | 19
3 files changed, 68 insertions(+), 4 deletions(-)
create mode 100755 t/t5410-receive-pack.sh
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 112041f407..b908bc5825 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -616,6 +616,12 @@ core.preferSymlinkRefs::
This is sometimes needed to work with old scripts that
expect HEAD to be a symbolic link.
+core.alternateRefsCommand::
+ When listing references from an alternate (e.g., in the case of
".have"), use
+ the shell to execute the specified command instead of
+ linkgit:git-for-each-ref[1]. The first argument is the path of the
alternate.
+ Output must be of the form: `%(objectname) SPC %(refname)`.
+
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..09fb3f39a1
--- /dev/null
+++ b/t/t5410-receive-pack.sh
@@ -0,0 +1,47 @@
+#!/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 &&
+ git update-ref -d refs/heads/a &&
+ git update-ref -d refs/heads/b &&
+ git update-ref -d refs/heads/c &&
+ git update-ref -d refs/heads/master &&
+ git update-ref -d refs/tags/one &&
+ git update-ref -d refs/tags/two &&
+ git update-ref -d refs/tags/three &&
+ printf "../../.git/objects" >objects/info/alternates
+ )
+'
+
+extract_haves () {
+ depacketize - | grep -o '^.* \.have'
+}
+
+test_expect_success 'with core.alternateRefsCommand' '
+ test_config -C fork core.alternateRefsCommand \
+ "git --git-dir=\"\$1\" for-each-ref \
+ --format=\"%(objectname) %(refname)\" \
+ refs/heads/a refs/heads/c;:" &&
+ cat >expect <<-EOF &&
+ $(git rev-parse a) .have
+ $(git rev-parse c) .have
+ EOF
+ printf "" | git receive-pack fork | extract_haves >actual &&
+ test_cmp expect actual
+'
+
+test_done
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(>args, "--git-dir=%s", repo_path);
- argv_array_push(>args, "for-each-ref");
- argv_array_push(>args, "--format=%(objectname) %(refname)");
+ const char *value;
+
+ if (!git_config_get_value("core.alternateRefsCommand", )) {
+ cmd->use_shell = 1;
+
+ argv_array_push(>args, value);
+ argv_array_push(>args, repo_path);
+ } else {
+ cmd->git_cmd = 1;
+
+ argv_array_pushf(>args, "--git-dir=%s", repo_path);
+ argv_array_push(>args, "for-each-ref");
+ argv_array_push(>args, "--format=%(objectname)
