git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH] transport-helper: enforce atomic in push_refs_with_push

From
Johannes Schindelin <johannes.schindelin@gmx.de>
Date
Jul 2, 2019, 13:51 UTC
Message-ID
<nycvar.QRO.7.76.6.1907021540330.48@tvgsbejvaqbjf.bet>
In-Reply-To
<20190702005340.66615-1-emilyshaffer@google.com>
Hi Emily,
On Mon, 1 Jul 2019, Emily Shaffer wrote:
Show 7 quoted lines
> Teach transport-helper how to notice if skipping a ref during push would
> violate atomicity on the client side. We notice that a ref would be
> rejected, and choose not to send it, but don't notice that if the client
> has asked for --atomic we are violating atomicity if all the other
> pushes we are sending would succeed. Asking the server end to uphold
> atomicity wouldn't work here as the server doesn't have any idea that we
> tried to update a ref that's broken.
Makes sense.
Show 27 quoted lines
> The added test-case is a succinct way to reproduce this issue that fails
> today. The same steps work fine when we aren't using a transport-helper
> to get to the upstream, i.e. when we've added a local repository as a
> remote:
>
>   git remote add ~/upstream upstream
>
> Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
> ---
>  t/t5541-http-push-smart.sh | 58 ++++++++++++++++++++++++++++++++++++++
>  transport-helper.c         |  6 ++++
>  transport.c                | 15 +++++++++-
>  3 files changed, 78 insertions(+), 1 deletion(-)
>
> diff --git a/t/t5541-http-push-smart.sh b/t/t5541-http-push-smart.sh
> index 8ef8763e06..b57f6d480f 100755
> --- a/t/t5541-http-push-smart.sh
> +++ b/t/t5541-http-push-smart.sh
> @@ -177,6 +177,64 @@ test_expect_success 'push (chunked)' '
>  	 test $HEAD = $(git rev-parse --verify HEAD))
>  '
>
> +test_expect_success 'push --atomic also prevents branch creation' '
> +	# Make up/master
> +	d=$HTTPD_DOCUMENT_ROOT_PATH/atomic-branches.git &&
> +	git init --bare "$d" &&
> +	git --git-dir="$d" config http.receivepack true &&
Why not `-C "$d"`? And why not `test_config`?
> +	up="$HTTPD_URL"/smart/atomic-branches.git &&
> +	test_commit atomic1 &&
> +	test_commit atomic2 &&
> +	git push "$up" master &&

It would be more succinct to do a `git clone --bare . "$d"` here, instead of a `git init --bare` and a `git push` no?

Show 8 quoted lines
> +	# Make master incompatible with up/master
> +	git reset --hard HEAD^ &&
> +	# Add a new branch
> +	git branch atomic &&
> +	# --atomic should roll back creation of up/atomic
> +	test_must_fail git push --atomic "$up" master atomic &&
> +	git ls-remote "$up" >up-remotes &&
> +	test_must_fail grep atomic up-remotes
Why not `test_must_fail git -C "$d" rev-parse refs/heads/atomic`?
Show 22 quoted lines
> +'
> +
> +test_expect_success 'push --atomic shows all failed refs' '
> +	# Make up/master, up/allrefs
> +	d=$HTTPD_DOCUMENT_ROOT_PATH/atomic-failed-refs.git &&
> +	git init --bare "$d" &&
> +	git --git-dir="$d" config http.receivepack true &&
> +	up="$HTTPD_URL"/smart/atomic-failed-refs.git &&
> +	test_commit allrefs1 &&
> +	test_commit allrefs2 &&
> +	git branch allrefs &&
> +	git push "$up" master allrefs &&
> +	# Make master and allrefs incompatible with up/master, up/allrefs
> +	git checkout allrefs &&
> +	git reset --hard HEAD^ &&
> +	git checkout master &&
> +	git reset --hard HEAD^ &&
> +	# --atomic should complain about both master and allrefs
> +	test_must_fail git push --atomic "$up" master allrefs >&output &&
> +	grep master output &&
> +	grep allrefs output
> +'

I have the impression that the setup these two new test cases perform are _very_ similar, making it most likely that a combined test case would be more succinct, yet still complete and easily readable.

Show 18 quoted lines
> +
> +test_expect_success 'push --atomic indicates collateral failures' '
> +	# Make up/master, up/collateral
> +	d=$HTTPD_DOCUMENT_ROOT_PATH/atomic-collateral.git &&
> +	git init --bare "$d" &&
> +	git --git-dir="$d" config http.receivepack true &&
> +	up="$HTTPD_URL"/smart/atomic-collateral.git &&
> +	test_commit collateral1 &&
> +	test_commit collateral2 &&
> +	git branch collateral &&
> +	git push "$up" master collateral &&
> +	# Make master incompatible with up/master
> +	git reset --hard HEAD^ &&
> +	# --atomic should mention collateral was OK but failed anyway
> +	test_must_fail git push --atomic "$up" master collateral >&output &&
> +	grep "master -> master" output &&
> +	grep "collateral -> collateral" output
> +'
Same here.
Show 38 quoted lines
> +
>  test_expect_success 'push --all can push to empty repo' '
>  	d=$HTTPD_DOCUMENT_ROOT_PATH/empty-all.git &&
>  	git init --bare "$d" &&
> diff --git a/transport-helper.c b/transport-helper.c
> index c7e17ec9cb..6b05a88faf 100644
> --- a/transport-helper.c
> +++ b/transport-helper.c
> @@ -853,6 +853,7 @@ static int push_refs_with_push(struct transport *transport,
>  {
>  	int force_all = flags & TRANSPORT_PUSH_FORCE;
>  	int mirror = flags & TRANSPORT_PUSH_MIRROR;
> +	int atomic = flags & TRANSPORT_PUSH_ATOMIC;
>  	struct helper_data *data = transport->data;
>  	struct strbuf buf = STRBUF_INIT;
>  	struct ref *ref;
> @@ -872,6 +873,11 @@ static int push_refs_with_push(struct transport *transport,
>  		case REF_STATUS_REJECT_NONFASTFORWARD:
>  		case REF_STATUS_REJECT_STALE:
>  		case REF_STATUS_REJECT_ALREADY_EXISTS:
> +			if (atomic) {
> +				string_list_clear(&cas_options, 0);
> +				return 0;
> +			} else
> +				continue;
>  		case REF_STATUS_UPTODATE:
>  			continue;
>  		default:
> diff --git a/transport.c b/transport.c
> index f1fcd2c4b0..f4d6b38f9d 100644
> --- a/transport.c
> +++ b/transport.c
> @@ -1226,10 +1226,23 @@ int transport_push(struct repository *r,
>  		err = push_had_errors(remote_refs);
>  		ret = push_ret | err;
>
> -		if (!quiet || err)
> +		if ((flags & TRANSPORT_PUSH_ATOMIC) && err) {
This looks funny. And it does so only...
Show 12 quoted lines
> +			for (struct ref *it = remote_refs; it; it = it->next)
> +				switch (it->status) {
> +				case REF_STATUS_NONE:
> +				case REF_STATUS_UPTODATE:
> +				case REF_STATUS_OK:
> +					it->status = REF_STATUS_ATOMIC_PUSH_FAILED;
> +				default:
> +					continue;
> +				}
> +		}
> +
> +		if (!quiet || err) {

... because a curly was introduced around a single-liner. This adds unnecessary noise to the patch.

This easily distracts reviewers like myself from more important questions such as: why was this conditional switch added before this conditional block, does it intend to influence the printed push status? Ah, yes, of course, even if `it->status` is changed, it actually modifies the data to which `remote_refs` points. So yes, this has to be done here.

Show 8 quoted lines
>  			transport_print_push_status(transport->url, remote_refs,
>  					verbose | porcelain, porcelain,
>  					reject_reasons);
> +		}
>
>  		if (flags & TRANSPORT_PUSH_SET_UPSTREAM)
>  			set_upstreams(transport, remote_refs, pretend);
> --

Apart from minor nits, I like it. Thanks, Dscho

> 2.22.0.410.gd8fdbe21b5-goog
>
>
Previous: Emily ShafferNext: Junio C Hamano
Message 2 of 45 in “transport-helper: enforce atomic in push_refs_with_push”
  1. transport-helper: enforce atomic in push_refs_with_pushEmily Shaffer, Jul 2, 2019
  2. Johannes SchindelinJul 2, 2019
  3. Junio C HamanoJul 2, 2019
  4. Emily ShafferJul 3, 2019
  5. Emily ShafferJul 3, 2019
  6. Johannes SchindelinJul 3, 2019
  7. Emily ShafferJul 3, 2019
  8. Johannes SchindelinJul 4, 2019
  9. Emily ShafferJul 9, 2019
  10. Junio C HamanoJul 2, 2019
  11. Junio C HamanoJul 2, 2019
  12. Emily ShafferJul 3, 2019
  13. Junio C HamanoJul 2, 2019
  14. Emily ShafferJul 3, 2019
  15. SZEDER GáborJul 3, 2019
  16. Junio C HamanoJul 3, 2019
  17. Emily ShafferJul 3, 2019
  18. transport-helper: enforce atomic in push_refs_with_pushEmily Shaffer, Jul 9, 2019
  19. Junio C HamanoJul 10, 2019
  20. Junio C HamanoJul 10, 2019
  21. Emily ShafferJul 11, 2019
  22. Emily ShafferJul 11, 2019
  23. Junio C HamanoJul 11, 2019
  24. transport-helper: enforce atomic in push_refs_with_pushEmily Shaffer, Jul 11, 2019
  25. Junio C HamanoJul 12, 2019
  26. Carlo ArenasJul 16, 2019
  27. Junio C HamanoJul 16, 2019
  28. CodingGuidelines: spell out post-C89 rulesJunio C Hamano, Jul 16, 2019
  29. Jonathan NiederJul 17, 2019
  30. Junio C HamanoJul 17, 2019
  31. Jonathan NiederJul 19, 2019
  32. Bryan TurnerJul 17, 2019
  33. Junio C HamanoJul 17, 2019
  34. Carlo ArenasJul 16, 2019
  35. transport-helper: avoid var decl in for () loop controlJunio C Hamano, Jul 16, 2019
  36. Jonathan NiederJul 17, 2019
  37. SZEDER GáborJul 18, 2019
  38. Junio C HamanoJul 18, 2019
  39. SZEDER GáborJul 18, 2019
  40. Eric SunshineJul 18, 2019
  41. Jonathan NiederJul 19, 2019
  42. Carlo ArenasJul 19, 2019
  43. Junio C HamanoJul 19, 2019
  44. SZEDER GáborJul 27, 2019
  45. Junio C HamanoJul 27, 2019

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.