git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / Atom feed
* [PATCH] transport-helper: enforce atomic in push_refs_with_push
@ 2019-07-02  0:53 Emily Shaffer
  2019-07-02 13:51 ` Johannes Schindelin
                   ` (4 more replies)
  0 siblings, 5 replies; 26+ messages in thread
From: Emily Shaffer @ 2019-07-02  0:53 UTC (permalink / raw)
  To: git; +Cc: Emily Shaffer, Junio C Hamano

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.

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 &&
+	up="$HTTPD_URL"/smart/atomic-branches.git &&
+	test_commit atomic1 &&
+	test_commit atomic2 &&
+	git push "$up" master &&
+	# 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
+'
+
+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
+'
+
+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
+'
+
 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) {
+			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) {
 			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);
-- 
2.22.0.410.gd8fdbe21b5-goog


^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH] transport-helper: enforce atomic in push_refs_with_push
  2019-07-02  0:53 [PATCH] transport-helper: enforce atomic in push_refs_with_push Emily Shaffer
@ 2019-07-02 13:51 ` Johannes Schindelin
  2019-07-02 18:27   ` Junio C Hamano
  2019-07-03 18:56   ` Emily Shaffer
  2019-07-02 19:06 ` Junio C Hamano
                   ` (3 subsequent siblings)
  4 siblings, 2 replies; 26+ messages in thread
From: Johannes Schindelin @ 2019-07-02 13:51 UTC (permalink / raw)
  To: Emily Shaffer; +Cc: git, Junio C Hamano

Hi Emily,

On Mon, 1 Jul 2019, Emily Shaffer wrote:

> 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.

> 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?

> +	# 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`?

> +'
> +
> +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.

> +
> +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.

> +
>  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...

> +			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.

>  			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
>
>

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH] transport-helper: enforce atomic in push_refs_with_push
  2019-07-02 13:51 ` Johannes Schindelin
@ 2019-07-02 18:27   ` Junio C Hamano
  2019-07-03 18:56   ` Emily Shaffer
  1 sibling, 0 replies; 26+ messages in thread
From: Junio C Hamano @ 2019-07-02 18:27 UTC (permalink / raw)
  To: Johannes Schindelin; +Cc: Emily Shaffer, git

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

>> +	# 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`?

They check different expectations.

Between making sure that the state observable in the resulting
repository matches what should have been left and checking what the
command says it did to its output, I agree that the former is a more
robust way to test the commands.

We obviously could test both, though ;-)


^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH] transport-helper: enforce atomic in push_refs_with_push
  2019-07-02  0:53 [PATCH] transport-helper: enforce atomic in push_refs_with_push Emily Shaffer
  2019-07-02 13:51 ` Johannes Schindelin
@ 2019-07-02 19:06 ` Junio C Hamano
  2019-07-02 20:16 ` Junio C Hamano
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 26+ messages in thread
From: Junio C Hamano @ 2019-07-02 19:06 UTC (permalink / raw)
  To: Emily Shaffer; +Cc: git

Emily Shaffer <emilyshaffer@google.com> writes:

> 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;

Ah, this looks vaguely familiar.  Thanks for resurrecting the topic.

The clearing is merely to avoid leaks, and the primary change to the
function is to immediately return 0.

>  		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;

Here, before reporting the push result, when we are doing ATOMIC,
we tweak the result we are going to report to atomic-push-failed.

> +		if ((flags & TRANSPORT_PUSH_ATOMIC) && err) {
> +			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;
> +				}
> +		}

This roughly corresponds to what send-pack.c::atomic_push_failure()
does.  Here, we avoid overwriting a status that already signals a
failure.  The list of "good" statuses used here match what is used
at the end of send_pack.c::send_pack(), which decides the final
outcome of "git push" for the native transport.

Looks good.

By the way, I rearranged the patch as I happen to agree with Dscho
that the additional {} was unwarranted and made it harder to review.
It is clear that we need to tweak the status before reporting.

>
> 		if (!quiet || err)
>  			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);

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH] transport-helper: enforce atomic in push_refs_with_push
  2019-07-02  0:53 [PATCH] transport-helper: enforce atomic in push_refs_with_push Emily Shaffer
  2019-07-02 13:51 ` Johannes Schindelin
  2019-07-02 19:06 ` Junio C Hamano
@ 2019-07-02 20:16 ` Junio C Hamano
  2019-07-03  0:09   ` Emily Shaffer
  2019-07-02 21:37 ` Junio C Hamano
  2019-07-09 21:10 ` [PATCH v2] " Emily Shaffer
  4 siblings, 1 reply; 26+ messages in thread
From: Junio C Hamano @ 2019-07-02 20:16 UTC (permalink / raw)
  To: Emily Shaffer; +Cc: git

Emily Shaffer <emilyshaffer@google.com> writes:

> +		if ((flags & TRANSPORT_PUSH_ATOMIC) && err) {
> +			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;
> +				}
> +		}


Let's write this more like so

		if ((flags & TRANSPORT_PUSH_ATOMIC) && err) {
			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;
					break;
				default:
					break;
				}
		}

to prevent compilers from giving "implicit fallthru" warnings.

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH] transport-helper: enforce atomic in push_refs_with_push
  2019-07-02  0:53 [PATCH] transport-helper: enforce atomic in push_refs_with_push Emily Shaffer
                   ` (2 preceding siblings ...)
  2019-07-02 20:16 ` Junio C Hamano
@ 2019-07-02 21:37 ` Junio C Hamano
  2019-07-03  0:08   ` Emily Shaffer
  2019-07-03  9:10   ` SZEDER Gábor
  2019-07-09 21:10 ` [PATCH v2] " Emily Shaffer
  4 siblings, 2 replies; 26+ messages in thread
From: Junio C Hamano @ 2019-07-02 21:37 UTC (permalink / raw)
  To: Emily Shaffer; +Cc: git

Emily Shaffer <emilyshaffer@google.com> writes:

> 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.
>
> 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 &&
> +	up="$HTTPD_URL"/smart/atomic-branches.git &&
> +	test_commit atomic1 &&
> +	test_commit atomic2 &&
> +	git push "$up" master &&
> +	# 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

Don't use test_must_fail on non-git things.  We are not in the
business of catching segfaulting system programs.

> +'
> +
> +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 &&

Don't rely on ">&output", which is an unnecessary bash-ism here.  It
breaks test run under shells like dash.

	>output 2>&1

should be OK.

> +	grep master output &&
> +	grep allrefs output
> +'
> +
> +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 &&

Ditto.

> +	grep "master -> master" output &&
> +	grep "collateral -> collateral" output
> +'
> +


^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH] transport-helper: enforce atomic in push_refs_with_push
  2019-07-02 21:37 ` Junio C Hamano
@ 2019-07-03  0:08   ` Emily Shaffer
  2019-07-03  9:10   ` SZEDER Gábor
  1 sibling, 0 replies; 26+ messages in thread
From: Emily Shaffer @ 2019-07-03  0:08 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git

On Tue, Jul 02, 2019 at 02:37:42PM -0700, Junio C Hamano wrote:
> >  
> > +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 &&
> > +	up="$HTTPD_URL"/smart/atomic-branches.git &&
> > +	test_commit atomic1 &&
> > +	test_commit atomic2 &&
> > +	git push "$up" master &&
> > +	# 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
> 
> Don't use test_must_fail on non-git things.  We are not in the
> business of catching segfaulting system programs.

Done:
  ! grep atomic up-remotes

> 
> > +'
> > +
> > +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 &&
> 
> Don't rely on ">&output", which is an unnecessary bash-ism here.  It
> breaks test run under shells like dash.
> 
> 	>output 2>&1
> 
> should be OK.

Done.

> 
> > +	grep master output &&
> > +	grep allrefs output
> > +'
> > +
> > +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 &&
> 
> Ditto.

Done.

 - Emily

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH] transport-helper: enforce atomic in push_refs_with_push
  2019-07-02 20:16 ` Junio C Hamano
@ 2019-07-03  0:09   ` Emily Shaffer
  0 siblings, 0 replies; 26+ messages in thread
From: Emily Shaffer @ 2019-07-03  0:09 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git

On Tue, Jul 02, 2019 at 01:16:46PM -0700, Junio C Hamano wrote:
> Emily Shaffer <emilyshaffer@google.com> writes:
> 
> > +		if ((flags & TRANSPORT_PUSH_ATOMIC) && err) {
> > +			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;
> > +				}
> > +		}
> 
> 
> Let's write this more like so
> 
> 		if ((flags & TRANSPORT_PUSH_ATOMIC) && err) {
> 			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;
> 					break;
> 				default:
> 					break;
> 				}
> 		}
> 
> to prevent compilers from giving "implicit fallthru" warnings.

Done, thanks.

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH] transport-helper: enforce atomic in push_refs_with_push
  2019-07-02 21:37 ` Junio C Hamano
  2019-07-03  0:08   ` Emily Shaffer
@ 2019-07-03  9:10   ` SZEDER Gábor
  2019-07-03 18:13     ` Junio C Hamano
  1 sibling, 1 reply; 26+ messages in thread
From: SZEDER Gábor @ 2019-07-03  9:10 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Emily Shaffer, git

On Tue, Jul 02, 2019 at 02:37:42PM -0700, Junio C Hamano wrote:
> > +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 &&
> 
> Don't rely on ">&output", which is an unnecessary bash-ism here.  It
> breaks test run under shells like dash.
> 
> 	>output 2>&1
> 
> should be OK.

'2>output' would be a tad better, because those refs should be printed
to stderr.


^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH] transport-helper: enforce atomic in push_refs_with_push
  2019-07-03  9:10   ` SZEDER Gábor
@ 2019-07-03 18:13     ` Junio C Hamano
  2019-07-03 18:58       ` Emily Shaffer
  0 siblings, 1 reply; 26+ messages in thread
From: Junio C Hamano @ 2019-07-03 18:13 UTC (permalink / raw)
  To: SZEDER Gábor; +Cc: Emily Shaffer, git

SZEDER Gábor <szeder.dev@gmail.com> writes:

> On Tue, Jul 02, 2019 at 02:37:42PM -0700, Junio C Hamano wrote:
>> > +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 &&
>> 
>> Don't rely on ">&output", which is an unnecessary bash-ism here.  It
>> breaks test run under shells like dash.
>> 
>> 	>output 2>&1
>> 
>> should be OK.
>
> '2>output' would be a tad better, because those refs should be printed
> to stderr.

Yeah; there are many existing uses of ">output 2>&1" in the same
script and I was following the suit.  There also are 2>err and I
agree that it is more appropriate in this case.

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH] transport-helper: enforce atomic in push_refs_with_push
  2019-07-02 13:51 ` Johannes Schindelin
  2019-07-02 18:27   ` Junio C Hamano
@ 2019-07-03 18:56   ` Emily Shaffer
  2019-07-03 19:01     ` Emily Shaffer
  2019-07-03 19:41     ` Johannes Schindelin
  1 sibling, 2 replies; 26+ messages in thread
From: Emily Shaffer @ 2019-07-03 18:56 UTC (permalink / raw)
  To: Johannes Schindelin; +Cc: git, Junio C Hamano

> > +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"`?
The example I had found below the new ones used --git-dir, but yeah, there's no
reason not to use -C instead. Changing.

> And why not `test_config`?

Done, didn't know about it and it's not used in the test I referred to
while writing this one ('push --all can push to empty repo'). Thanks, I
learned something new.

> 
> > +	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?

I'm not sure I would say "more succinct." This leaves the test with the
same number of lines, but now it says:

  Make some commits
  Name a Git directory
  Clone to the new Git directory
  Do some config on the new Git directory
  Name a remote URL
  Change some commits
  ...

In my opinion, it's more readable the way it is now:

  {Do some setup stuff.}
  Name a Git directory
  Init it
  Config it
  Name the remote URL
  {Do the test stuff.}
  Make some commits
  Push some commits
  Change some commits
  ...

I did add another comment to separate "Make 'up'" and "Make up/master",
which I hope expresses my intent in organizing it this way.

> 
> > +	# 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`?

Sure, changed.

> > > +'
> > +
> > +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.

(Junio replied to this downthread... I have more to ask too.)

> 
> > +
> > +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.
> 
> > +
> >  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...
> 
> > +			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.

Oops, I thought I had omitted the new braces when I was staging the changes.
Really sorry for the distraction. You're right that it makes the diff
look weird.

> 
> >  			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
> >
> >

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH] transport-helper: enforce atomic in push_refs_with_push
  2019-07-03 18:13     ` Junio C Hamano
@ 2019-07-03 18:58       ` Emily Shaffer
  0 siblings, 0 replies; 26+ messages in thread
From: Emily Shaffer @ 2019-07-03 18:58 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: SZEDER Gábor, git

On Wed, Jul 03, 2019 at 11:13:45AM -0700, Junio C Hamano wrote:
> SZEDER Gábor <szeder.dev@gmail.com> writes:
> 
> > On Tue, Jul 02, 2019 at 02:37:42PM -0700, Junio C Hamano wrote:
> >> > +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 &&
> >> 
> >> Don't rely on ">&output", which is an unnecessary bash-ism here.  It
> >> breaks test run under shells like dash.
> >> 
> >> 	>output 2>&1
> >> 
> >> should be OK.
> >
> > '2>output' would be a tad better, because those refs should be printed
> > to stderr.
> 
> Yeah; there are many existing uses of ">output 2>&1" in the same
> script and I was following the suit.  There also are 2>err and I
> agree that it is more appropriate in this case.

Oh, this is a good point. I'll change it, thanks both.

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH] transport-helper: enforce atomic in push_refs_with_push
  2019-07-03 18:56   ` Emily Shaffer
@ 2019-07-03 19:01     ` Emily Shaffer
  2019-07-03 19:41     ` Johannes Schindelin
  1 sibling, 0 replies; 26+ messages in thread
From: Emily Shaffer @ 2019-07-03 19:01 UTC (permalink / raw)
  To: Johannes Schindelin; +Cc: git, Junio C Hamano

> > > > +'
> > > +
> > > +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.
> 
> (Junio replied to this downthread... I have more to ask too.)

Oops, I misremembered which comment he had replied to. Yeah, I'll think
about this and reword.

Reroll to come shortly. Thanks, all.

 - Emily

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH] transport-helper: enforce atomic in push_refs_with_push
  2019-07-03 18:56   ` Emily Shaffer
  2019-07-03 19:01     ` Emily Shaffer
@ 2019-07-03 19:41     ` Johannes Schindelin
  2019-07-03 20:57       ` Emily Shaffer
  1 sibling, 1 reply; 26+ messages in thread
From: Johannes Schindelin @ 2019-07-03 19:41 UTC (permalink / raw)
  To: Emily Shaffer; +Cc: git, Junio C Hamano

Hi Emily,

On Wed, 3 Jul 2019, Emily Shaffer wrote:

> > > +	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?
>
> I'm not sure I would say "more succinct." This leaves the test with the
> same number of lines,

No, it does not, as `git clone --bare . "$d"` does _both_ the initializing
and the object transfer.

It only saves one line, of course, but do keep in mind that anybody
running into any kind of regression with your test case needs to
understand what it does. And from experience I can tell you that reading
any test case longer than 5 lines is quite annoying when you actually
only care about fixing the regression, and not so much about the wonderful
story the test case tells.

So in a sense, I guess I would even suggest to move as much of the setup
for your test cases outside, preferably into an initial `setup` test case
that initializes the minimal scenario required by the regression test
case.

Thanks,
Dscho

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH] transport-helper: enforce atomic in push_refs_with_push
  2019-07-03 19:41     ` Johannes Schindelin
@ 2019-07-03 20:57       ` Emily Shaffer
  2019-07-04  8:29         ` Johannes Schindelin
  0 siblings, 1 reply; 26+ messages in thread
From: Emily Shaffer @ 2019-07-03 20:57 UTC (permalink / raw)
  To: Johannes Schindelin; +Cc: git, Junio C Hamano

On Wed, Jul 03, 2019 at 09:41:46PM +0200, Johannes Schindelin wrote:
> Hi Emily,
> 
> On Wed, 3 Jul 2019, Emily Shaffer wrote:
> 
> > > > +	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?
> >
> > I'm not sure I would say "more succinct." This leaves the test with the
> > same number of lines,
> 
> No, it does not, as `git clone --bare . "$d"` does _both_ the initializing
> and the object transfer.
> 
> It only saves one line, of course, but do keep in mind that anybody
> running into any kind of regression with your test case needs to
> understand what it does. And from experience I can tell you that reading
> any test case longer than 5 lines is quite annoying when you actually
> only care about fixing the regression, and not so much about the wonderful
> story the test case tells.

I suppose I'm confused, then, as I understood you were asking me to
combine my three test cases into one, which naturally makes the test
itself more complex and longer to read. Which do you prefer?

> 
> So in a sense, I guess I would even suggest to move as much of the setup
> for your test cases outside, preferably into an initial `setup` test case
> that initializes the minimal scenario required by the regression test
> case.
> 
> Thanks,
> Dscho

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH] transport-helper: enforce atomic in push_refs_with_push
  2019-07-03 20:57       ` Emily Shaffer
@ 2019-07-04  8:29         ` Johannes Schindelin
  2019-07-09 20:23           ` Emily Shaffer
  0 siblings, 1 reply; 26+ messages in thread
From: Johannes Schindelin @ 2019-07-04  8:29 UTC (permalink / raw)
  To: Emily Shaffer; +Cc: git, Junio C Hamano

Hi Emily,

On Wed, 3 Jul 2019, Emily Shaffer wrote:

> On Wed, Jul 03, 2019 at 09:41:46PM +0200, Johannes Schindelin wrote:
>
> > On Wed, 3 Jul 2019, Emily Shaffer wrote:
> >
> > > > > +	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?
> > >
> > > I'm not sure I would say "more succinct." This leaves the test with the
> > > same number of lines,
> >
> > No, it does not, as `git clone --bare . "$d"` does _both_ the initializing
> > and the object transfer.
> >
> > It only saves one line, of course, but do keep in mind that anybody
> > running into any kind of regression with your test case needs to
> > understand what it does. And from experience I can tell you that reading
> > any test case longer than 5 lines is quite annoying when you actually
> > only care about fixing the regression, and not so much about the wonderful
> > story the test case tells.
>
> I suppose I'm confused, then, as I understood you were asking me to
> combine my three test cases into one, which naturally makes the test
> itself more complex and longer to read. Which do you prefer?

If I were tasked with developing this further, I would try to move as much
of the setup into the initial test case (if there is already a `setup`
test case; otherwise I would create one). In fact, I would try to reuse as
much of the existing setup test case as possible, and only add commands if
really necessary. Then, I would try to combine the three test cases in the
patch into a single one, structuring it by white space and using comments
to clarify what is happening.

In my mind, even just adding an empty line before the comments like "Make
master incompatible with up/master" would make it much easier for me to
read, were I to analyze a test breakage.

I have to admit that I have a hard time understanding what the intention
of those three test cases is because I get confused: where does the set-up
end, where is the code that is expected to fail, where are the
expectations tested?

Also, I get confused by how similar the test cases look, and have a hard
time discerning what the differences are (i.e. what are the interesting
bits, where the entropy comes from).

I could imagine that I would have had an easier time reading something
like this:

test_expect_success 'push --atomic' '
	: set up two branches, one which will require a force push &&
	git checkout -b fast-forwarding master &&
	test_commit push-atomic &&
	git checkout -b non-fast-forwarding &&

	: now, initialize the bare repository to push to &&
	d=$HTTPD_DOCUMENT_ROOT_PATH/atomic.git &&
	git clone --bare . $d &&

	: modify the two branches and create a third one &&
	git reset --hard HEAD^ &&
	git checkout fast-forwarding &&
	test_commit no-need-to-force &&
	git branch new-branch &&

	: now the atomic push must fail &&
	test_must_fail git push --atomic "$HTTPD_URL"/smart/atomic.git \
		fast-forwarding non-fast-forwarding new-branch 2>err &&

	: verify that new-branch was not pushed &&
	test_must_fail git -C $d rev-parse --verify refs/heads/new-branch &&

	: fast-forwarding should be mentioned even if it would have been OK &&
	grep fast-forwarding err
'

Of course, everybody has their preferences, and their personal style. I do
not want you to imitate my style just to "pacify" me. That's not the point
of this example.

The point is that I need some structure to walk along, especially when I
am a bit annoyed at a test case that shows that I introduced a regression
and all I want is to understand as quickly as possible what I did wrong
again so that I can fix it and move on.

The point is that I want a regression test case to _not_ distract me from
the essential part, ideally I should be able to ignore all the set-up code
and deduce from the command-line of the failing command what is going on.

For example, if the test case fails in the line `grep fast-forwarding err`
above, that command-line alone does not tell me anything, it just
frustrates me. If there is a comment above that line (which is ideally
part of the `-x` trace, that's why I used `:` instead of `#`) that states
the intention in what I consider to be clear, simple English, it is a lot
easier to figure out what the heck is going wrong.

Of course, it would be even better if we had a function, say,
`must_contain` that runs that `grep` and shows the file contents and the
regular expression if that `grep` failed. That's of course outside of the
concern of your patch. But this idea illustrates what I want in a
regression test case: I want it to _help_ me figure out things when it
breaks.

Ciao,
Dscho

P.S.: Please note that the many test cases _I_ introduced into Git's test
suite mostly do not conform to what I wrote above. I learned _quite_ a
lot of how I want regression test cases to look like in the past six
months, not from writing them, but from analyzing literally hundreds of
Azure Pipelines builds. And your question forced me to think about my
learnings to formulate the above (hopefully clear) explanation of what I
want in a regression test, so: thank you.


^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH] transport-helper: enforce atomic in push_refs_with_push
  2019-07-04  8:29         ` Johannes Schindelin
@ 2019-07-09 20:23           ` Emily Shaffer
  0 siblings, 0 replies; 26+ messages in thread
From: Emily Shaffer @ 2019-07-09 20:23 UTC (permalink / raw)
  To: Johannes Schindelin; +Cc: git, Junio C Hamano

Firstly, sorry for the delay, I wasn't working for national holiday from
the 4th til yesterday.

> If I were tasked with developing this further, I would try to move as much
> of the setup into the initial test case (if there is already a `setup`
> test case; otherwise I would create one). In fact, I would try to reuse as
> much of the existing setup test case as possible, and only add commands if
> really necessary. Then, I would try to combine the three test cases in the
> patch into a single one, structuring it by white space and using comments
> to clarify what is happening.
> 
> In my mind, even just adding an empty line before the comments like "Make
> master incompatible with up/master" would make it much easier for me to
> read, were I to analyze a test breakage.
> 
> I have to admit that I have a hard time understanding what the intention
> of those three test cases is because I get confused: where does the set-up
> end, where is the code that is expected to fail, where are the
> expectations tested?
> 
> Also, I get confused by how similar the test cases look, and have a hard
> time discerning what the differences are (i.e. what are the interesting
> bits, where the entropy comes from).
> 
> I could imagine that I would have had an easier time reading something
> like this:
> 
> test_expect_success 'push --atomic' '
> 	: set up two branches, one which will require a force push &&
> 	git checkout -b fast-forwarding master &&
> 	test_commit push-atomic &&
> 	git checkout -b non-fast-forwarding &&
> 
> 	: now, initialize the bare repository to push to &&
> 	d=$HTTPD_DOCUMENT_ROOT_PATH/atomic.git &&
> 	git clone --bare . $d &&
> 
> 	: modify the two branches and create a third one &&
> 	git reset --hard HEAD^ &&
> 	git checkout fast-forwarding &&
> 	test_commit no-need-to-force &&
> 	git branch new-branch &&
> 
> 	: now the atomic push must fail &&
> 	test_must_fail git push --atomic "$HTTPD_URL"/smart/atomic.git \
> 		fast-forwarding non-fast-forwarding new-branch 2>err &&
> 
> 	: verify that new-branch was not pushed &&
> 	test_must_fail git -C $d rev-parse --verify refs/heads/new-branch &&
> 
> 	: fast-forwarding should be mentioned even if it would have been OK &&
> 	grep fast-forwarding err
> '

After I read your initial comment this is close to what my rewrite
started to look like; I suppose we're only the same page after all :)

> 
> Of course, everybody has their preferences, and their personal style. I do
> not want you to imitate my style just to "pacify" me. That's not the point
> of this example.
> 
> The point is that I need some structure to walk along, especially when I
> am a bit annoyed at a test case that shows that I introduced a regression
> and all I want is to understand as quickly as possible what I did wrong
> again so that I can fix it and move on.
> 
> The point is that I want a regression test case to _not_ distract me from
> the essential part, ideally I should be able to ignore all the set-up code
> and deduce from the command-line of the failing command what is going on.
> 
> For example, if the test case fails in the line `grep fast-forwarding err`
> above, that command-line alone does not tell me anything, it just
> frustrates me. If there is a comment above that line (which is ideally
> part of the `-x` trace, that's why I used `:` instead of `#`) that states
> the intention in what I consider to be clear, simple English, it is a lot
> easier to figure out what the heck is going wrong.
> 
> Of course, it would be even better if we had a function, say,
> `must_contain` that runs that `grep` and shows the file contents and the
> regular expression if that `grep` failed. That's of course outside of the
> concern of your patch. But this idea illustrates what I want in a
> regression test case: I want it to _help_ me figure out things when it
> breaks.
> 
> Ciao,
> Dscho
> 
> P.S.: Please note that the many test cases _I_ introduced into Git's test
> suite mostly do not conform to what I wrote above. I learned _quite_ a
> lot of how I want regression test cases to look like in the past six
> months, not from writing them, but from analyzing literally hundreds of
> Azure Pipelines builds. And your question forced me to think about my
> learnings to formulate the above (hopefully clear) explanation of what I
> want in a regression test, so: thank you.
> 

This writeup is great, and I think your example is very clear and
readable. I'll push a reroll with my similar-looking reformat soon.
Thanks, Johannes.

 - Emily

^ permalink raw reply	[flat|nested] 26+ messages in thread

* [PATCH v2] transport-helper: enforce atomic in push_refs_with_push
  2019-07-02  0:53 [PATCH] transport-helper: enforce atomic in push_refs_with_push Emily Shaffer
                   ` (3 preceding siblings ...)
  2019-07-02 21:37 ` Junio C Hamano
@ 2019-07-09 21:10 ` " Emily Shaffer
  2019-07-10 17:44   ` Junio C Hamano
                     ` (2 more replies)
  4 siblings, 3 replies; 26+ messages in thread
From: Emily Shaffer @ 2019-07-09 21:10 UTC (permalink / raw)
  To: git; +Cc: Emily Shaffer, Junio C Hamano, Johannes Schindelin

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.

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>
---
Since v2, I combined the test cases into one. Unfortunately the new test
case is checking a few things at once, but I hope the comments make it
clear enough. Johannes, I still left the setup at the top; for me in the
tradeoff between "one less line" and "setup happens first", the latter
wins.

I also removed the "try pushing two bad branches at the same time" case;
I don't think it's really covered by atomicity. So now we check that if
we push one bad ref, collateral damage in the form of 1) a new branch
and 2) a perfectly good push of another branch are both rejected and
reported correctly.

Also, got rid of the accidental new curly braces around unrelated part
of the code.

Thanks all.

 - Emily

 t/t5541-http-push-smart.sh | 39 ++++++++++++++++++++++++++++++++++++++
 transport-helper.c         |  6 ++++++
 transport.c                | 13 +++++++++++++
 3 files changed, 58 insertions(+)

diff --git a/t/t5541-http-push-smart.sh b/t/t5541-http-push-smart.sh
index 8ef8763e06..7f9ae1ef3f 100755
--- a/t/t5541-http-push-smart.sh
+++ b/t/t5541-http-push-smart.sh
@@ -177,6 +177,45 @@ test_expect_success 'push (chunked)' '
 	 test $HEAD = $(git rev-parse --verify HEAD))
 '
 
+test_expect_success 'push --atomic also prevents branch creation, reports collateral' '
+	# Setup upstream repo - empty for now
+	d=$HTTPD_DOCUMENT_ROOT_PATH/atomic-branches.git &&
+	git init --bare "$d" &&
+	test_config -C "$d" http.receivepack true &&
+	up="$HTTPD_URL"/smart/atomic-branches.git &&
+
+	# Tell up about two branches for now
+	test_commit atomic1 &&
+	test_commit atomic2 &&
+	git branch collateral &&
+	git push "$up" master collateral &&
+
+	# collateral is a valid push, but should be failed by atomic push
+	git checkout collateral &&
+	test_commit collateral1 &&
+
+	# Make master incompatible with upstream to provoke atomic
+	git checkout master &&
+	git reset --hard HEAD^ &&
+
+	# Add a new branch which should be failed by atomic push. This is a
+	# regression case.
+	git branch atomic &&
+
+	# --atomic should cause entire push to be rejected
+	test_must_fail git push --atomic "$up" master atomic collateral 2>output &&
+
+	# the new branch should not have been created upstream
+	test_must_fail git -C "$d" rev-parse refs/heads/atomic &&
+
+	# the failed refs should be indicated
+	grep "master -> master" output | grep rejected &&
+
+	# the collateral failure refs should be indicated
+	grep "atomic -> atomic" output | grep "atomic push failed" &&
+	grep "collateral -> collateral" output | grep "atomic push failed"
+'
+
 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..d768bc275e 100644
--- a/transport.c
+++ b/transport.c
@@ -1226,6 +1226,19 @@ int transport_push(struct repository *r,
 		err = push_had_errors(remote_refs);
 		ret = push_ret | err;
 
+		if ((flags & TRANSPORT_PUSH_ATOMIC) && err) {
+			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;
+					break;
+				default:
+					break;
+				}
+		}
+
 		if (!quiet || err)
 			transport_print_push_status(transport->url, remote_refs,
 					verbose | porcelain, porcelain,
-- 
2.22.0.410.gd8fdbe21b5-goog


^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH v2] transport-helper: enforce atomic in push_refs_with_push
  2019-07-09 21:10 ` [PATCH v2] " Emily Shaffer
@ 2019-07-10 17:44   ` Junio C Hamano
  2019-07-10 17:53     ` Junio C Hamano
  2019-07-11 20:57     ` Emily Shaffer
  2019-07-11 21:19   ` [PATCH v3] " Emily Shaffer
  2019-07-16  7:10   ` [PATCH v2] " Carlo Arenas
  2 siblings, 2 replies; 26+ messages in thread
From: Junio C Hamano @ 2019-07-10 17:44 UTC (permalink / raw)
  To: Emily Shaffer; +Cc: git, Johannes Schindelin

Emily Shaffer <emilyshaffer@google.com> writes:

> +test_expect_success 'push --atomic also prevents branch creation, reports collateral' '
> +	# Setup upstream repo - empty for now
> +	d=$HTTPD_DOCUMENT_ROOT_PATH/atomic-branches.git &&
> +	git init --bare "$d" &&
> +	test_config -C "$d" http.receivepack true &&
> +	up="$HTTPD_URL"/smart/atomic-branches.git &&
> +
> +	# Tell up about two branches for now

-ECANTPARSE "Tell up" part.

> +	test_commit atomic1 &&
> +	test_commit atomic2 &&
> +	git branch collateral &&
> +	git push "$up" master collateral &&

OK, so an initially empty directory $d that appears to network
clients as $up now has two branches, 'master' and 'collateral',
both pointing at the same history that ends with two commits,
atomic2 whose parent is atomic1.

> +	# collateral is a valid push, but should be failed by atomic push
> +	git checkout collateral &&
> +	test_commit collateral1 &&
> +
> +	# Make master incompatible with upstream to provoke atomic
> +	git checkout master &&
> +	git reset --hard HEAD^ &&

collateral grows, master rewinds.

> +	# Add a new branch which should be failed by atomic push. This is a
> +	# regression case.
> +	git branch atomic &&

Another branch atomic is added

> +	# --atomic should cause entire push to be rejected
> +	test_must_fail git push --atomic "$up" master atomic collateral 2>output &&

Attempt to push all three: collateral alone would be OK, so is
atomic, but because master rewinds, we expect none of the three to
go through.

> +	# the new branch should not have been created upstream
> +	test_must_fail git -C "$d" rev-parse refs/heads/atomic &&

The new branch should not have been created; if this rev-parse
succeeded, it would be a bug.

Up to point, I have no possible improvements to offer ;-)
Very well done.

> +	# the failed refs should be indicated
> +	grep "master -> master" output | grep rejected &&

I'd rather see the effect, i.e. what the command did that can be
observed externally, than the report, i.e. what the command claims
to have done, if it is equally straight-forward to verify either.

That can be done by making sure that the output from "git -C "$d"
rev-parse refs/heads/master" match output from "git rev-parse
atomic2", no?  That ensures 'master' in the receiving end stayed the
same.

> +	# the collateral failure refs should be indicated
> +	grep "atomic -> atomic" output | grep "atomic push failed" &&
> +	grep "collateral -> collateral" output | grep "atomic push failed"

Likewise for the other two.  

FWIW, these three can further lose a process each, i.e.

	grep "^ ! .*rejected.* master -> master" output

even if we for some reason do not want to check the effect and take
the claim by the command being tested at the face value (which I do
not think is a good idea).

Thanks.

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH v2] transport-helper: enforce atomic in push_refs_with_push
  2019-07-10 17:44   ` Junio C Hamano
@ 2019-07-10 17:53     ` Junio C Hamano
  2019-07-11 21:14       ` Emily Shaffer
  2019-07-11 20:57     ` Emily Shaffer
  1 sibling, 1 reply; 26+ messages in thread
From: Junio C Hamano @ 2019-07-10 17:53 UTC (permalink / raw)
  To: Emily Shaffer; +Cc: git, Johannes Schindelin

Junio C Hamano <gitster@pobox.com> writes:

>> +	# the new branch should not have been created upstream
>> +	test_must_fail git -C "$d" rev-parse refs/heads/atomic &&
>
> The new branch should not have been created; if this rev-parse
> succeeded, it would be a bug.

One thing I forgot.  If refs/heads/atomic did not exist, but say
refs/tags/refs/heads/atomic did, the rev-parse would succeed, which
is a rather unfortunate source of confusion.

Of course, we know we have never touched "$d" to cause such a funny
tag, so rev-parse is good enough in practice, but

    git -C "$d" show-ref --verify refs/heads/atomic

would not dwim (its --verify mode was invented specifically for
rectifying this issue with rev-parse, if I recall correctly), and is
more appropriate best-practice version to write here, especially if
we anticipate that future developers and Git users would treat this
line as an example to mimic.

> Up to point, I have no possible improvements to offer ;-)
> Very well done.

So, I lied, but still the tests looked quite well done.

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH v2] transport-helper: enforce atomic in push_refs_with_push
  2019-07-10 17:44   ` Junio C Hamano
  2019-07-10 17:53     ` Junio C Hamano
@ 2019-07-11 20:57     ` Emily Shaffer
  2019-07-11 21:13       ` Junio C Hamano
  1 sibling, 1 reply; 26+ messages in thread
From: Emily Shaffer @ 2019-07-11 20:57 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, Johannes Schindelin

On Wed, Jul 10, 2019 at 10:44:22AM -0700, Junio C Hamano wrote:
> Emily Shaffer <emilyshaffer@google.com> writes:
> 
> > +test_expect_success 'push --atomic also prevents branch creation, reports collateral' '
> > +	# Setup upstream repo - empty for now
> > +	d=$HTTPD_DOCUMENT_ROOT_PATH/atomic-branches.git &&
> > +	git init --bare "$d" &&
> > +	test_config -C "$d" http.receivepack true &&
> > +	up="$HTTPD_URL"/smart/atomic-branches.git &&
> > +
> > +	# Tell up about two branches for now
> 
> -ECANTPARSE "Tell up" part.

s/up/"$&"/ - thanks.

[snip]
> Up to point, I have no possible improvements to offer ;-)
> Very well done.

This is nice to hear, thanks! :)

> > +	# the failed refs should be indicated
> > +	grep "master -> master" output | grep rejected &&
> 
> I'd rather see the effect, i.e. what the command did that can be
> observed externally, than the report, i.e. what the command claims
> to have done, if it is equally straight-forward to verify either.

Hmm. I'd like to argue that part of the requirement is to show the user
what happened; for example, in an earlier iteration I was not
successfully reporting the "collateral damage" refs to the user, even
though they were not being pushed. To that end, I'd rather check both.

> 
> That can be done by making sure that the output from "git -C "$d"
> rev-parse refs/heads/master" match output from "git rev-parse
> atomic2", no?  That ensures 'master' in the receiving end stayed the
> same.

Sure, I agree.

> 
> > +	# the collateral failure refs should be indicated
> > +	grep "atomic -> atomic" output | grep "atomic push failed" &&
> > +	grep "collateral -> collateral" output | grep "atomic push failed"
> 
> Likewise for the other two.  
> 
> FWIW, these three can further lose a process each, i.e.
> 
> 	grep "^ ! .*rejected.* master -> master" output
> 
> even if we for some reason do not want to check the effect and take
> the claim by the command being tested at the face value (which I do
> not think is a good idea).

Will swap, wasn't sure on preference between regex or process count.
Thanks.

 - Emily

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH v2] transport-helper: enforce atomic in push_refs_with_push
  2019-07-11 20:57     ` Emily Shaffer
@ 2019-07-11 21:13       ` Junio C Hamano
  0 siblings, 0 replies; 26+ messages in thread
From: Junio C Hamano @ 2019-07-11 21:13 UTC (permalink / raw)
  To: Emily Shaffer; +Cc: git, Johannes Schindelin

Emily Shaffer <emilyshaffer@google.com> writes:

> Hmm. I'd like to argue that part of the requirement is to show the user
> what happened; for example, in an earlier iteration I was not
> successfully reporting the "collateral damage" refs to the user, even
> though they were not being pushed. To that end, I'd rather check both.

If we can afford to check both, making sure that we behave correctly
and report what we did correctly would of course be the best.

;-)



^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH v2] transport-helper: enforce atomic in push_refs_with_push
  2019-07-10 17:53     ` Junio C Hamano
@ 2019-07-11 21:14       ` Emily Shaffer
  0 siblings, 0 replies; 26+ messages in thread
From: Emily Shaffer @ 2019-07-11 21:14 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, Johannes Schindelin

On Wed, Jul 10, 2019 at 10:53:30AM -0700, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
> 
> >> +	# the new branch should not have been created upstream
> >> +	test_must_fail git -C "$d" rev-parse refs/heads/atomic &&
> >
> > The new branch should not have been created; if this rev-parse
> > succeeded, it would be a bug.
> 
> One thing I forgot.  If refs/heads/atomic did not exist, but say
> refs/tags/refs/heads/atomic did, the rev-parse would succeed, which
> is a rather unfortunate source of confusion.
> 
> Of course, we know we have never touched "$d" to cause such a funny
> tag, so rev-parse is good enough in practice, but
> 
>     git -C "$d" show-ref --verify refs/heads/atomic
> 
> would not dwim (its --verify mode was invented specifically for
> rectifying this issue with rev-parse, if I recall correctly), and is
> more appropriate best-practice version to write here, especially if
> we anticipate that future developers and Git users would treat this
> line as an example to mimic.
> 
> > Up to point, I have no possible improvements to offer ;-)
> > Very well done.
> 
> So, I lied, but still the tests looked quite well done.


Oh, that's very interesting! Thanks for pointing it out. :)

Reroll coming in a few. Thanks, Junio.

 - Emily

^ permalink raw reply	[flat|nested] 26+ messages in thread

* [PATCH v3] transport-helper: enforce atomic in push_refs_with_push
  2019-07-09 21:10 ` [PATCH v2] " Emily Shaffer
  2019-07-10 17:44   ` Junio C Hamano
@ 2019-07-11 21:19   ` " Emily Shaffer
  2019-07-12 16:25     ` Junio C Hamano
  2019-07-16  7:10   ` [PATCH v2] " Carlo Arenas
  2 siblings, 1 reply; 26+ messages in thread
From: Emily Shaffer @ 2019-07-11 21:19 UTC (permalink / raw)
  To: git; +Cc: Emily Shaffer, Junio C Hamano, Johannes Schindelin

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.

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>
---
Since the previous version, made a couple changes based on Junio's
comments on the testcase.

- Use show-ref instead of rev-parse to avoid a corner case where a tag
  is named the same thing as the branch we hope not to create
- Reduce number of grep processes by using a longer regex instead
- Check the real contents of the upstream repo instead of just relying
  on the client side's reporting
  - The check on the client side remains, however, to ensure the user is
    made aware of why their perfectly good ref failed to push.

Thanks, all.

 t/t5541-http-push-smart.sh | 49 ++++++++++++++++++++++++++++++++++++++
 transport-helper.c         |  6 +++++
 transport.c                | 13 ++++++++++
 3 files changed, 68 insertions(+)

diff --git a/t/t5541-http-push-smart.sh b/t/t5541-http-push-smart.sh
index 8ef8763e06..92bac43257 100755
--- a/t/t5541-http-push-smart.sh
+++ b/t/t5541-http-push-smart.sh
@@ -177,6 +177,55 @@ test_expect_success 'push (chunked)' '
 	 test $HEAD = $(git rev-parse --verify HEAD))
 '
 
+test_expect_success 'push --atomic also prevents branch creation, reports collateral' '
+	# Setup upstream repo - empty for now
+	d=$HTTPD_DOCUMENT_ROOT_PATH/atomic-branches.git &&
+	git init --bare "$d" &&
+	test_config -C "$d" http.receivepack true &&
+	up="$HTTPD_URL"/smart/atomic-branches.git &&
+
+	# Tell "$up" about two branches for now
+	test_commit atomic1 &&
+	test_commit atomic2 &&
+	git branch collateral &&
+	git push "$up" master collateral &&
+
+	# collateral is a valid push, but should be failed by atomic push
+	git checkout collateral &&
+	test_commit collateral1 &&
+
+	# Make master incompatible with upstream to provoke atomic
+	git checkout master &&
+	git reset --hard HEAD^ &&
+
+	# Add a new branch which should be failed by atomic push. This is a
+	# regression case.
+	git branch atomic &&
+
+	# --atomic should cause entire push to be rejected
+	test_must_fail git push --atomic "$up" master atomic collateral 2>output &&
+
+	# the new branch should not have been created upstream
+	test_must_fail git -C "$d" show-ref --verify refs/heads/atomic &&
+
+	# upstream should still reflect atomic2, the last thing we pushed
+	# successfully
+	git rev-parse atomic2 >expected &&
+	# on master...
+	git -C "$d" rev-parse refs/heads/master >actual &&
+	test_cmp expected actual &&
+	# ...and collateral.
+	git -C "$d" rev-parse refs/heads/collateral >actual &&
+	test_cmp expected actual &&
+
+	# the failed refs should be indicated to the user
+	grep "^ ! .*rejected.* master -> master" output &&
+
+	# the collateral failure refs should be indicated to the user
+	grep "^ ! .*rejected.* atomic -> atomic .*atomic push failed" output &&
+	grep "^ ! .*rejected.* collateral -> collateral .*atomic push failed" output
+'
+
 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..d768bc275e 100644
--- a/transport.c
+++ b/transport.c
@@ -1226,6 +1226,19 @@ int transport_push(struct repository *r,
 		err = push_had_errors(remote_refs);
 		ret = push_ret | err;
 
+		if ((flags & TRANSPORT_PUSH_ATOMIC) && err) {
+			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;
+					break;
+				default:
+					break;
+				}
+		}
+
 		if (!quiet || err)
 			transport_print_push_status(transport->url, remote_refs,
 					verbose | porcelain, porcelain,
-- 
2.22.0.410.gd8fdbe21b5-goog


^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH v3] transport-helper: enforce atomic in push_refs_with_push
  2019-07-11 21:19   ` [PATCH v3] " Emily Shaffer
@ 2019-07-12 16:25     ` Junio C Hamano
  0 siblings, 0 replies; 26+ messages in thread
From: Junio C Hamano @ 2019-07-12 16:25 UTC (permalink / raw)
  To: Emily Shaffer; +Cc: git, Johannes Schindelin

Emily Shaffer <emilyshaffer@google.com> writes:

> +test_expect_success 'push --atomic also prevents branch creation, reports collateral' '
> +	# Setup upstream repo - empty for now
> +	d=$HTTPD_DOCUMENT_ROOT_PATH/atomic-branches.git &&
> +	git init --bare "$d" &&
> +	test_config -C "$d" http.receivepack true &&
> +	up="$HTTPD_URL"/smart/atomic-branches.git &&
> +
> +	# Tell "$up" about two branches for now

Ahh, that's what you meant ;-)  Makes sense.

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH v2] transport-helper: enforce atomic in push_refs_with_push
  2019-07-09 21:10 ` [PATCH v2] " Emily Shaffer
  2019-07-10 17:44   ` Junio C Hamano
  2019-07-11 21:19   ` [PATCH v3] " Emily Shaffer
@ 2019-07-16  7:10   ` " Carlo Arenas
  2 siblings, 0 replies; 26+ messages in thread
From: Carlo Arenas @ 2019-07-16  7:10 UTC (permalink / raw)
  To: Emily Shaffer; +Cc: git, Junio C Hamano, Johannes Schindelin

On Tue, Jul 9, 2019 at 2:16 PM Emily Shaffer <emilyshaffer@google.com> wrote:
>
> diff --git a/transport.c b/transport.c
> index f1fcd2c4b0..d768bc275e 100644
> --- a/transport.c
> +++ b/transport.c
> @@ -1226,6 +1226,19 @@ int transport_push(struct repository *r,
>                 err = push_had_errors(remote_refs);
>                 ret = push_ret | err;
>
> +               if ((flags & TRANSPORT_PUSH_ATOMIC) && err) {
> +                       for (struct ref *it = remote_refs; it; it = it->next)

moving "struct ref it" out of the loop, allows for building with ancient
compilers that don't support C90 (even if only by default) as I found
out while building pu in a Centos 6 box

Carlo

^ permalink raw reply	[flat|nested] 26+ messages in thread

end of thread, back to index

Thread overview: 26+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-07-02  0:53 [PATCH] transport-helper: enforce atomic in push_refs_with_push Emily Shaffer
2019-07-02 13:51 ` Johannes Schindelin
2019-07-02 18:27   ` Junio C Hamano
2019-07-03 18:56   ` Emily Shaffer
2019-07-03 19:01     ` Emily Shaffer
2019-07-03 19:41     ` Johannes Schindelin
2019-07-03 20:57       ` Emily Shaffer
2019-07-04  8:29         ` Johannes Schindelin
2019-07-09 20:23           ` Emily Shaffer
2019-07-02 19:06 ` Junio C Hamano
2019-07-02 20:16 ` Junio C Hamano
2019-07-03  0:09   ` Emily Shaffer
2019-07-02 21:37 ` Junio C Hamano
2019-07-03  0:08   ` Emily Shaffer
2019-07-03  9:10   ` SZEDER Gábor
2019-07-03 18:13     ` Junio C Hamano
2019-07-03 18:58       ` Emily Shaffer
2019-07-09 21:10 ` [PATCH v2] " Emily Shaffer
2019-07-10 17:44   ` Junio C Hamano
2019-07-10 17:53     ` Junio C Hamano
2019-07-11 21:14       ` Emily Shaffer
2019-07-11 20:57     ` Emily Shaffer
2019-07-11 21:13       ` Junio C Hamano
2019-07-11 21:19   ` [PATCH v3] " Emily Shaffer
2019-07-12 16:25     ` Junio C Hamano
2019-07-16  7:10   ` [PATCH v2] " Carlo Arenas

git@vger.kernel.org list mirror (unofficial, one of many)

Archives are clonable:
	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.org/gmane.comp.version-control.git

 note: .onion URLs require Tor: https://www.torproject.org/

AGPL code for this site: git clone https://public-inbox.org/ public-inbox