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
Emily Shaffer <emilyshaffer@google.com>
Date
Jul 3, 2019, 18:56 UTC
Message-ID
<20190703185535.GC121233@google.com>
In-Reply-To
<nycvar.QRO.7.76.6.1907021540330.48@tvgsbejvaqbjf.bet>
Show 7 quoted lines
> > +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.

Show 8 quoted lines
> 
> > +	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.

Show 11 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`?
Sure, changed.
Show 26 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.
(Junio replied to this downthread... I have more to ask too.)
Show 84 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.
> 
> > +
> >  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.

Show 16 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: Junio C HamanoNext: Emily Shaffer
Message 4 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.