| Message ID | 20200322131815.11872-2-worldhello.net@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | New proc-receive hook for centralized workflow | expand |
Jiang Xin <worldhello.net@gmail.com> writes: > +# NOTE: Avoid calling this function from a subshell since variable > +# assignments will disappear when subshell exits. s/Avoid/Never/; the test_tick helper used in this function has the same issue. > +create_commits_in () { > + repo="$1" && > + if ! parent=$(git -C "$repo" rev-parse HEAD^{} 2>/dev/null) Do we need to discard the standard error stream like this? As long as this helper function is called inside a test_expect_thing, the error will not be seen and when debugging the test, we would want to see a failure (which indicates that we are creating a root commit). > +format_git_output () { I suspect this is to make output from _some_ "git" subcommand into a symbolic form so that exact object names would not have to be used in comparison, but this obviously cannot take _any_ git subcommand, but a specific one. The name does not say which one, which is disservice to readers. > + sed \ > + -e "s/ *\$//g" \ What's the point of /g? You are anchoring the pattern (i.e. one or more SP) to the right end of the line, so it's not like it would transform "a b c " into "abc". Also it would be sufficient to say "zero or more" and that would be shorter, I think, i.e. -e 's/ *$//' \ > + -e "s/$A/<COMMIT-A>/g" \ > + -e "s/$B/<COMMIT-B>/g" \ > + -e "s/$TAG/<COMMIT-T>/g" \ A and B are commits, so the symbolic <COMMIT-A> and <COMMIT-B> do make sense, but wouldn't TAG be an annotated tag? Wouldn't it be <TAG-A> perhaps? > + -e "s/$ZERO_OID/<ZERO-OID>/g" \ Maekes sense. > + -e "s/'/\"/g" I am not sure what is going on here. Why turn <don't> into <don"t>? Without exactly knowing what is getting munged, a reader cannot tell what is going on here. Let's read on to figure it out. In any case, I think most of this helper is not about "formatting" output, but hiding, getting rid of, redacting or censoring the details for easier comparison. I'd prefer to see some different verb to be used for its name. > +} > + > +test_expect_success "setup" ' > + git init --bare upstream && > + git init workbench && > + create_commits_in workbench A B && > + ( > + cd workbench && > + git remote add origin ../upstream && > + git config core.abbrev 7 && This '7' is "use at least seven hexdigits"; is that really what we want? Depending on chance, some objects may be shown using 8 or more---is our "munge output into symbolic form for comparison" script prepared for such a case? > + git update-ref refs/heads/master $A && > + git tag -m "v1.0.0" v1.0.0 $A && > + git push origin \ > + $B:refs/heads/master \ > + $A:refs/heads/next > + ) && > + TAG=$(cd workbench; git rev-parse v1.0.0) && Why not "git -C workbench rev-parse v1.0.0"? So, at this point, there are two repositories, upstream and workbench, and two commits A and B (B is newer). workbench has A at the tip of its 'master'; upstream has A at the tip of 'next' and B (which is newer than A) at the tip of 'master'. > + > + # setup pre-receive hook > + cat >upstream/hooks/pre-receive <<-EOF && Wouldn't it make it easier to read the resulting text if you quoted the end-of-here-text marker here, i.e. "<<\-EOF"? That way, you can lose backslash before $old, $new and $ref. > + #!/bin/sh > + > + printf >&2 "# pre-receive hook\n" > + > + while read old new ref > + do > + printf >&2 "pre-receive< \$old \$new \$ref\n" > + done > + EOF > + # setup post-receive hook > + cat >upstream/hooks/post-receive <<-EOF && Likewise. > +test_expect_success "normal git-push command" ' > + ( > + cd workbench && > + git push -f origin \ > + refs/tags/v1.0.0 \ > + :refs/heads/next \ > + HEAD:refs/heads/master \ > + HEAD:refs/review/master/topic \ > + HEAD:refs/heads/a/b/c > + ) >out 2>&1 && Do we need to worry about progress output (which we would want to squelch, I presume, for the purpose of comparing with the "expectation")? Would it be just the matter of giving --no-progress? > + format_git_output <out >actual && > + cat >expect <<-EOF && > + remote: # pre-receive hook > + remote: pre-receive< <COMMIT-B> <COMMIT-A> refs/heads/master > + remote: pre-receive< <COMMIT-A> <ZERO-OID> refs/heads/next > + remote: pre-receive< <ZERO-OID> <COMMIT-T> refs/tags/v1.0.0 > + remote: pre-receive< <ZERO-OID> <COMMIT-A> refs/review/master/topic > + remote: pre-receive< <ZERO-OID> <COMMIT-A> refs/heads/a/b/c Do we guarantee the order in which these received refs are reported, or do we somehow need to sort (presumably inside the hook)? The same question applies to the post-receive side, too. > + remote: # post-receive hook > + remote: post-receive< <COMMIT-B> <COMMIT-A> refs/heads/master > + remote: post-receive< <COMMIT-A> <ZERO-OID> refs/heads/next > + remote: post-receive< <ZERO-OID> <COMMIT-T> refs/tags/v1.0.0 > + remote: post-receive< <ZERO-OID> <COMMIT-A> refs/review/master/topic > + remote: post-receive< <ZERO-OID> <COMMIT-A> refs/heads/a/b/c > + To ../upstream > + + ce858e6...1029397 HEAD -> master (forced update) > + - [deleted] next > + * [new tag] v1.0.0 -> v1.0.0 > + * [new reference] HEAD -> refs/review/master/topic > + * [new branch] HEAD -> a/b/c > + EOF > + test_cmp expect actual && > + ( > + cd upstream && > + git show-ref This one I know we give output in a reliable order, but I do not offhand know if we give any written guarantee. Perhaps we should document it if we haven't already (i.e. it is OK the "expected" output below assumes that the output is sorted by full refnames in ASCII order). > + ) >out && > + format_git_output <out >actual && > + cat >expect <<-EOF && > + <COMMIT-A> refs/heads/a/b/c > + <COMMIT-A> refs/heads/master > + <COMMIT-A> refs/review/master/topic > + <COMMIT-T> refs/tags/v1.0.0 > + EOF > + test_cmp expect actual > +' > + > +test_done > diff --git a/transport.c b/transport.c > index 1fdc7dac1a..b5b7bb841e 100644 > --- a/transport.c > +++ b/transport.c > @@ -501,7 +501,8 @@ static void print_ok_ref_status(struct ref *ref, int porcelain, int summary_widt > else if (is_null_oid(&ref->old_oid)) > print_ref_status('*', > (starts_with(ref->name, "refs/tags/") ? "[new tag]" : > - "[new branch]"), > + (starts_with(ref->name, "refs/heads/") ? "[new branch]" : > + "[new reference]")), > ref, ref->peer_ref, NULL, porcelain, summary_width); The original is largely to blame, but I couldn't read the above and understand what the above is doing, before reformatting it like so: else if (is_null_oid(&ref->old_oid)) print_ref_status('*', (starts_with(ref->name, "refs/tags/") ? "[new tag]" : (starts_with(ref->name, "refs/heads/") ? "[new branch]" : "[new reference]")), ref, ref->peer_ref, NULL, porcelain, summary_width); When long subexpressions are involved, ternary operator ?: is easier to read, especially when nested, if you can see its parse tree when you tilt your head 90-degrees sideways (i.e. the same direction as you can see a smile in ;-). Thanks.
diff --git a/t/t5411-proc-receive-hook.sh b/t/t5411-proc-receive-hook.sh new file mode 100755 index 0000000000..fe2861f2e6 --- /dev/null +++ b/t/t5411-proc-receive-hook.sh @@ -0,0 +1,144 @@ +#!/bin/sh +# +# Copyright (c) 2020 Jiang Xin +# + +test_description='Test proc-receive hook' + +. ./test-lib.sh + +# Create commits in <repo> and assign each commit's oid to shell variables +# given in the arguments (A, B, and C). E.g.: +# +# create_commits_in <repo> A B C +# +# NOTE: Avoid calling this function from a subshell since variable +# assignments will disappear when subshell exits. +create_commits_in () { + repo="$1" && + if ! parent=$(git -C "$repo" rev-parse HEAD^{} 2>/dev/null) + then + parent= + fi && + T=$(git -C "$repo" write-tree) && + shift && + while test $# -gt 0 + do + name=$1 && + test_tick && + if test -z "$parent" + then + oid=$(echo $name | git -C "$repo" commit-tree $T) + else + oid=$(echo $name | git -C "$repo" commit-tree -p $parent $T) + fi && + eval $name=$oid && + parent=$oid && + shift || + return 1 + done && + git -C "$repo" update-ref refs/heads/master $oid +} + +format_git_output () { + sed \ + -e "s/ *\$//g" \ + -e "s/$A/<COMMIT-A>/g" \ + -e "s/$B/<COMMIT-B>/g" \ + -e "s/$TAG/<COMMIT-T>/g" \ + -e "s/$ZERO_OID/<ZERO-OID>/g" \ + -e "s/'/\"/g" +} + +test_expect_success "setup" ' + git init --bare upstream && + git init workbench && + create_commits_in workbench A B && + ( + cd workbench && + git remote add origin ../upstream && + git config core.abbrev 7 && + git update-ref refs/heads/master $A && + git tag -m "v1.0.0" v1.0.0 $A && + git push origin \ + $B:refs/heads/master \ + $A:refs/heads/next + ) && + TAG=$(cd workbench; git rev-parse v1.0.0) && + + # setup pre-receive hook + cat >upstream/hooks/pre-receive <<-EOF && + #!/bin/sh + + printf >&2 "# pre-receive hook\n" + + while read old new ref + do + printf >&2 "pre-receive< \$old \$new \$ref\n" + done + EOF + + # setup post-receive hook + cat >upstream/hooks/post-receive <<-EOF && + #!/bin/sh + + printf >&2 "# post-receive hook\n" + + while read old new ref + do + printf >&2 "post-receive< \$old \$new \$ref\n" + done + EOF + + chmod a+x \ + upstream/hooks/pre-receive \ + upstream/hooks/post-receive +' + +test_expect_success "normal git-push command" ' + ( + cd workbench && + git push -f origin \ + refs/tags/v1.0.0 \ + :refs/heads/next \ + HEAD:refs/heads/master \ + HEAD:refs/review/master/topic \ + HEAD:refs/heads/a/b/c + ) >out 2>&1 && + format_git_output <out >actual && + cat >expect <<-EOF && + remote: # pre-receive hook + remote: pre-receive< <COMMIT-B> <COMMIT-A> refs/heads/master + remote: pre-receive< <COMMIT-A> <ZERO-OID> refs/heads/next + remote: pre-receive< <ZERO-OID> <COMMIT-T> refs/tags/v1.0.0 + remote: pre-receive< <ZERO-OID> <COMMIT-A> refs/review/master/topic + remote: pre-receive< <ZERO-OID> <COMMIT-A> refs/heads/a/b/c + remote: # post-receive hook + remote: post-receive< <COMMIT-B> <COMMIT-A> refs/heads/master + remote: post-receive< <COMMIT-A> <ZERO-OID> refs/heads/next + remote: post-receive< <ZERO-OID> <COMMIT-T> refs/tags/v1.0.0 + remote: post-receive< <ZERO-OID> <COMMIT-A> refs/review/master/topic + remote: post-receive< <ZERO-OID> <COMMIT-A> refs/heads/a/b/c + To ../upstream + + ce858e6...1029397 HEAD -> master (forced update) + - [deleted] next + * [new tag] v1.0.0 -> v1.0.0 + * [new reference] HEAD -> refs/review/master/topic + * [new branch] HEAD -> a/b/c + EOF + test_cmp expect actual && + ( + cd upstream && + git show-ref + ) >out && + format_git_output <out >actual && + cat >expect <<-EOF && + <COMMIT-A> refs/heads/a/b/c + <COMMIT-A> refs/heads/master + <COMMIT-A> refs/review/master/topic + <COMMIT-T> refs/tags/v1.0.0 + EOF + test_cmp expect actual +' + +test_done diff --git a/transport.c b/transport.c index 1fdc7dac1a..b5b7bb841e 100644 --- a/transport.c +++ b/transport.c @@ -501,7 +501,8 @@ static void print_ok_ref_status(struct ref *ref, int porcelain, int summary_widt else if (is_null_oid(&ref->old_oid)) print_ref_status('*', (starts_with(ref->name, "refs/tags/") ? "[new tag]" : - "[new branch]"), + (starts_with(ref->name, "refs/heads/") ? "[new branch]" : + "[new reference]")), ref, ref->peer_ref, NULL, porcelain, summary_width); else { struct strbuf quickref = STRBUF_INIT;