Jiang Xin <worldhello.net@xxxxxxxxx> 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.
