| Message ID | 20190709211043.48597-1-emilyshaffer@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] transport-helper: enforce atomic in push_refs_with_push | expand |
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.
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.
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
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. ;-)
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
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
Carlo Arenas <carenas@gmail.com> writes: > 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 Does everything else compile OK with your rather old compiler on Centos 6? I was historically rather pedantic to stick to C89 but for past several years we've been experimenting with a bit more modern features of the language to see if anybody screams (namely, designated initializers in structs and arrays, and trailing comma in enum definition) but I think we still reject variable definition in for loop control (we saw and rewrote another patch that tried to use it late last year). Apparently, this one slipped our review process. Thanks. cf. https://public-inbox.org/git/xmqqwopgqsws.fsf@gitster-ct.c.googlers.com/ cf. https://public-inbox.org/git/xmqqmuuz9jih.fsf@gitster-ct.c.googlers.com/
On Tue, Jul 16, 2019 at 9:54 AM Junio C Hamano <gitster@pobox.com> wrote: > > Does everything else compile OK with your rather old compiler on > Centos 6? yes, they were a few -Wmaybe-uninitialized warnings but they were most likely false positives. gcc 4.4.7 aborts the build (even without -Werror) with the following message: transport.c: In function 'transport_push': transport.c:1232: error: 'for' loop initial declarations are only allowed in C99 mode transport.c:1232: note: use option -std=c99 or -std=gnu99 to compile your code make: *** [transport.o] Error 1 Carlo
On Tue, Jul 16, 2019 at 09:53:59AM -0700, Junio C Hamano wrote: > Carlo Arenas <carenas@gmail.com> writes: > >> + 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 > but I think we still reject variable definition in > for loop control (we saw and rewrote another patch that tried to use > it late last year). > > Apparently, this one slipped our review process. I expected that this will eventually happen after Travis CI's default Linux image recently changed from Ubuntu 14.04 to 16.04; explanation in the commit message below. With that patch issues like this could be caught earlier, while they are only in 'pu' but not yet in 'next'. But do we really want to do that, is that the right tradeoff? Dunno... Adding a dedicated CI job just to check that there are no 'for' loop initial declarations seems kind of excessive, even if it only builds but doesn't run the test suite. And I don't know whether there are any other undesired ("too new") constructs that GCC 4.8 would catch but later compilers quietly accept. --- >8 --- Subject: [PATCH] travis-ci: build with GCC 4.8 as well C99 'for' loop initial declaration, i.e. 'for (int i = 0; i < n; i++)', is not allowed in Git's codebase yet, to maintain compatibility with some older compilers. Our Travis CI builds used to catch 'for' loop initial declarations, because the GETTEXT_POISON job has always built Git with the default 'cc', which in Travis CI's previous default Linux image (based on Ubuntu 14.04 Trusty) is GCC 4.8, and that GCC version errors out on this construct (not only with DEVELOPER=1, but with our default CFLAGS as well). Alas, that's not the case anymore, becase after 14.04's EOL Travis CI's current default Linux image is based on Ubuntu 16.04 Xenial [1] and its default 'cc' is now GCC 5.4, which, just like all later GCC and Clang versions, simply accepts this construct, even if we don't explicitly specify '-std=c99'. Ideally we would adjust our CFLAGS used with DEVELOPER=1 to catch this undesired construct already when contributors build Git on their own machines. Unfortunately, however, there seems to be no compiler option that would catch only this particular construct without choking on many other things, e.g. while a later compiler with '-std=c90' and/or '-ansi' does catch this construct, it can't build Git because of several screenfulls of other errors. Add the 'linux-gcc-4.8' job to Travis CI, in order to build Git with GCC 4.8, and thus to timely catch any 'for' loop initial declarations. To catch those it's sufficient to only build Git with GCC 4.8, so don't run the test suite in this job, because 'make test' takes rather long [2], and it's already run five times in other jobs, so we wouldn't get our time's worth. [1] The Azure Pipelines builds have been using Ubuntu 16.04 images from the start, so I belive they never caught 'for' loop initial declarations. [2] On Travis CI 'make test' alone would take about 9 minutes in this new job (without running httpd, Subversion, and P4 tests). For comparison, starting the job and building Git with GCC 4.8 takes only about 2 minutes. Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com> --- .travis.yml | 4 ++++ ci/run-build-and-tests.sh | 17 +++++++++++++---- 2 files changed, 17 insertions(+), 4 deletions(-) diff --git a/.travis.yml b/.travis.yml index ffb1bc46f2..fc5730b085 100644 --- a/.travis.yml +++ b/.travis.yml @@ -21,6 +21,10 @@ matrix: compiler: addons: before_install: + - env: jobname=linux-gcc-4.8 + os: linux + dist: trusty + compiler: - env: jobname=Linux32 os: linux compiler: diff --git a/ci/run-build-and-tests.sh b/ci/run-build-and-tests.sh index cdd2913440..ff0ef7f08e 100755 --- a/ci/run-build-and-tests.sh +++ b/ci/run-build-and-tests.sh @@ -11,9 +11,9 @@ windows*) cmd //c mklink //j t\\.prove "$(cygpath -aw "$cache_dir/.prove")";; esac make -make test -if test "$jobname" = "linux-gcc" -then +case "$jobname" in +linux-gcc) + make test export GIT_TEST_SPLIT_INDEX=yes export GIT_TEST_FULL_IN_PACK_ARRAY=true export GIT_TEST_OE_SIZE=10 @@ -21,7 +21,16 @@ then export GIT_TEST_COMMIT_GRAPH=1 export GIT_TEST_MULTI_PACK_INDEX=1 make test -fi + ;; +linux-gcc-4.8) + # Don't run the tests; we only care about whether Git can be + # built with GCC 4.8, as it errors out on some undesired (C99) + # constructs that newer compilers seem to quietly accept. + ;; +*) + make test + ;; +esac check_unignored_build_artifacts
SZEDER Gábor <szeder.dev@gmail.com> writes: > With that patch issues like this could be caught earlier, while they > are only in 'pu' but not yet in 'next'. But do we really want to do > that, is that the right tradeoff? I am sort of in favor of having at least one build with an older compiler without "-std=c99", like the set-up you are proposing. I got an impression from an earlier message that Jonathan's preference is the opposite. I'd prefer to hear opinions from others, too. The main reason why we accept other new features like trailing comma in enum def and designated initializers while rejecting for loop init is because there apparently are those who build Git with compilers that accept & reject these combinations of features and who care enough to report compilation failure from their build. And apparently gcc4.8 can serve as a representative "old" compiler, so...
On Thu, Jul 18, 2019 at 11:22 AM SZEDER Gábor <szeder.dev@gmail.com> wrote: > C99 'for' loop initial declaration, i.e. 'for (int i = 0; i < n; i++)', > is not allowed in Git's codebase yet, to maintain compatibility with > some older compilers. > [...] > [1] The Azure Pipelines builds have been using Ubuntu 16.04 images > from the start, so I belive they never caught 'for' loop initial > declarations. s/belive/believe/ > [2] On Travis CI 'make test' alone would take about 9 minutes in this > new job (without running httpd, Subversion, and P4 tests). For > comparison, starting the job and building Git with GCC 4.8 takes > only about 2 minutes. > > Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
On Thu, Jul 18, 2019 at 09:12:52AM -0700, Junio C Hamano wrote: > SZEDER Gábor <szeder.dev@gmail.com> writes: > > > With that patch issues like this could be caught earlier, while they > > are only in 'pu' but not yet in 'next'. But do we really want to do > > that, is that the right tradeoff? > > I am sort of in favor of having at least one build with an older > compiler without "-std=c99", like the set-up you are proposing. Alternatively, we could sort-of restore the old state of the GETTEXT_POISON job, i.e. build Git with GCC 4.8 in that job. "Sort-of", because we don't necessarily have to go back to the Ubuntu 14.04 based Linux image, because GCC 4.8 is packaged in 16.04 as well, so it's just an 'apt-get install gcc-4.8' away.
SZEDER Gábor wrote: > I expected that this will eventually happen after Travis CI's default > Linux image recently changed from Ubuntu 14.04 to 16.04; explanation > in the commit message below. > > With that patch issues like this could be caught earlier, while they > are only in 'pu' but not yet in 'next'. But do we really want to do > that, is that the right tradeoff? Dunno... Adding a dedicated CI job > just to check that there are no 'for' loop initial declarations seems > kind of excessive, even if it only builds but doesn't run the test > suite. And I don't know whether there are any other undesired ("too > new") constructs that GCC 4.8 would catch but later compilers quietly > accept. This makes sense to me. Not really for the 'for' loop declaration aspect: for that, I'd want some more specialized tool that allows turning on such a check specifically. But more because Ubuntu trusty is still a platform that some people use (though hopefully not for long), so it's helpful as a representative old platform to see if we break the build on it. [...] > [2] On Travis CI 'make test' alone would take about 9 minutes in this > new job (without running httpd, Subversion, and P4 tests). For > comparison, starting the job and building Git with GCC 4.8 takes > only about 2 minutes. Nice. In an ideal world there would be some kind of "make fasttest" that runs some fast subset of tests. But this seems pretty safe. > Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com> > --- > .travis.yml | 4 ++++ > ci/run-build-and-tests.sh | 17 +++++++++++++---- > 2 files changed, 17 insertions(+), 4 deletions(-) Thanks. [...] > --- a/ci/run-build-and-tests.sh > +++ b/ci/run-build-and-tests.sh > @@ -11,9 +11,9 @@ windows*) cmd //c mklink //j t\\.prove "$(cygpath -aw "$cache_dir/.prove")";; > esac > > make > -make test > -if test "$jobname" = "linux-gcc" > -then > +case "$jobname" in > +linux-gcc) > + make test > export GIT_TEST_SPLIT_INDEX=yes > export GIT_TEST_FULL_IN_PACK_ARRAY=true > export GIT_TEST_OE_SIZE=10 > @@ -21,7 +21,16 @@ then > export GIT_TEST_COMMIT_GRAPH=1 > export GIT_TEST_MULTI_PACK_INDEX=1 > make test > -fi > + ;; > +linux-gcc-4.8) > + # Don't run the tests; we only care about whether Git can be > + # built with GCC 4.8, as it errors out on some undesired (C99) > + # constructs that newer compilers seem to quietly accept. > + ;; > +*) > + make test > + ;; > +esac Does what it says on the tin. Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
On Thu, Jul 18, 2019 at 6:31 PM Jonathan Nieder <jrnieder@gmail.com> wrote: > > This makes sense to me. Not really for the 'for' loop declaration > aspect: for that, I'd want some more specialized tool that allows > turning on such a check specifically. But more because Ubuntu trusty > is still a platform that some people use (though hopefully not for > long), so it's helpful as a representative old platform to see if we > break the build on it. FWIW this also breaks Centos 7 using gcc 4.8.5, as well as the one originally reported (Centos 6), and anything else that uses gcc 4 (tested up to 4.9.4) Carlo
Carlo Arenas <carenas@gmail.com> writes: > On Thu, Jul 18, 2019 at 6:31 PM Jonathan Nieder <jrnieder@gmail.com> wrote: >> >> This makes sense to me. Not really for the 'for' loop declaration >> aspect: for that, I'd want some more specialized tool that allows >> turning on such a check specifically. But more because Ubuntu trusty >> is still a platform that some people use (though hopefully not for >> long), so it's helpful as a representative old platform to see if we >> break the build on it. > > FWIW this also breaks Centos 7 using gcc 4.8.5, as well as the one > originally reported (Centos 6), and anything else that uses gcc 4 > (tested up to 4.9.4) Thanks.
Junio, On Thu, Jul 18, 2019 at 05:22:34PM +0200, SZEDER Gábor wrote: > Subject: [PATCH] travis-ci: build with GCC 4.8 as well This patch conflicts with topic 'js/trace2-json-schema', and the current conflict resolution in 'pu' is not quite correct. > diff --git a/ci/run-build-and-tests.sh b/ci/run-build-and-tests.sh > index cdd2913440..ff0ef7f08e 100755 > --- a/ci/run-build-and-tests.sh > +++ b/ci/run-build-and-tests.sh > @@ -11,9 +11,9 @@ windows*) cmd //c mklink //j t\\.prove "$(cygpath -aw "$cache_dir/.prove")";; > esac > > make > -make test > -if test "$jobname" = "linux-gcc" > -then > +case "$jobname" in > +linux-gcc) > + make test This 'make test' here is important, but the confict resolution accidentally removed it. > export GIT_TEST_SPLIT_INDEX=yes > export GIT_TEST_FULL_IN_PACK_ARRAY=true > export GIT_TEST_OE_SIZE=10 > @@ -21,7 +21,16 @@ then > export GIT_TEST_COMMIT_GRAPH=1 > export GIT_TEST_MULTI_PACK_INDEX=1 > make test > -fi > + ;; > +linux-gcc-4.8) > + # Don't run the tests; we only care about whether Git can be > + # built with GCC 4.8, as it errors out on some undesired (C99) > + # constructs that newer compilers seem to quietly accept. > + ;; > +*) > + make test > + ;; > +esac > > check_unignored_build_artifacts > > -- > 2.22.0.810.g50207c7d84 > >
SZEDER Gábor <szeder.dev@gmail.com> writes: > Junio, > > On Thu, Jul 18, 2019 at 05:22:34PM +0200, SZEDER Gábor wrote: >> Subject: [PATCH] travis-ci: build with GCC 4.8 as well > > This patch conflicts with topic 'js/trace2-json-schema', and the > current conflict resolution in 'pu' is not quite correct. Thanks. "git diff ...MERGE_HEAD" during a merge of js/trace2-json-schema gives me this patch: diff --git a/ci/run-build-and-tests.sh b/ci/run-build-and-tests.sh index cdd2913440..ec38bf379a 100755 --- a/ci/run-build-and-tests.sh +++ b/ci/run-build-and-tests.sh @@ -14,6 +14,8 @@ make make test if test "$jobname" = "linux-gcc" then + make -C t/trace_schema_validator + export GIT_TRACE2_EVENT=$(mktemp) export GIT_TEST_SPLIT_INDEX=yes export GIT_TEST_FULL_IN_PACK_ARRAY=true export GIT_TEST_OE_SIZE=10 @@ -21,6 +23,10 @@ then export GIT_TEST_COMMIT_GRAPH=1 export GIT_TEST_MULTI_PACK_INDEX=1 make test + t/trace_schema_validator/trace_schema_validator \ + --trace2_event_file=${GIT_TRACE2_EVENT} \ + --schema_file=t/trace_schema_validator/strict_schema.json \ + --progress=10000 fi check_unignored_build_artifacts i.e. they want to run an extra make in that validator directory, export another environment variable, and then run the validator *after* running the normal "make test", in linux-gcc job. >> diff --git a/ci/run-build-and-tests.sh b/ci/run-build-and-tests.sh >> index cdd2913440..ff0ef7f08e 100755 >> --- a/ci/run-build-and-tests.sh >> +++ b/ci/run-build-and-tests.sh >> @@ -11,9 +11,9 @@ windows*) cmd //c mklink //j t\\.prove "$(cygpath -aw "$cache_dir/.prove")";; >> esac >> >> make >> -make test >> -if test "$jobname" = "linux-gcc" >> -then >> +case "$jobname" in >> +linux-gcc) >> + make test > > This 'make test' here is important, but the confict resolution > accidentally removed it. Right. Thanks for spotting. I can see in "git diff pu~2 pu~1" that indeed the first 'make test' that we want to run without any of these environment variables is lost in the merge. We want to run two tests, with or without these environment variables, and the validator wants to piggyback on the second one. Will fix in the meantime, but I was expecting that this "validator in CI" business to be redone differently, perhaps with a different validator implementation and either in a separate job or just part of an existing job but trace enabled only for some subset of the tests and/or only for new tests specifically written for trace coverage, so after that happens this may turn into a moot point. The change the merge brings in to the file now reads like this. Thanks, again. diff --git a/ci/run-build-and-tests.sh b/ci/run-build-and-tests.sh index ff0ef7f08e..35ff4d3038 100755 --- a/ci/run-build-and-tests.sh +++ b/ci/run-build-and-tests.sh @@ -14,6 +14,9 @@ make case "$jobname" in linux-gcc) make test + + make -C t/trace_schema_validator + export GIT_TRACE2_EVENT=$(mktemp) export GIT_TEST_SPLIT_INDEX=yes export GIT_TEST_FULL_IN_PACK_ARRAY=true export GIT_TEST_OE_SIZE=10 @@ -21,6 +24,11 @@ linux-gcc) export GIT_TEST_COMMIT_GRAPH=1 export GIT_TEST_MULTI_PACK_INDEX=1 make test + + t/trace_schema_validator/trace_schema_validator \ + --trace2_event_file=${GIT_TRACE2_EVENT} \ + --schema_file=t/trace_schema_validator/strict_schema.json \ + --progress=10000 ;; linux-gcc-4.8) # Don't run the tests; we only care about whether Git can be
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,
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(+)