Re: [PATCH 1/2] t1404: add a bunch of tests of D/F conflicts
On Sun, Oct 29, 2017 at 09:05:44AM +0900, Junio C Hamano wrote: > In short, unless you are a binary packager on a platform whose > native shell is ksh and who refuses to depend on tools that are not > default/native on the platform, you'd be OK? Yes. > > I'd recommend an explicit test for this. It's much easier to track down > > that way than seeing other failure scenarios. People will also usually > > complain about failing tests. > > Hopefully. > > Starting from an explicit test, gradually using more "local" in > tests that cover more important parts of the system, and then start > using "local" as appropriate in the main tools would be a good way > forward. I completely agree. I just wanted to ensure that if we failed, it was at least obvious to packagers who ran the tests. I'm not sure there's anything we can do if people don't run them. I do think people may run the tests more frequently than you think, though. I always do in my packaging at $DAYJOB, but any failures (usually missing SANITY) tend to already be patched by the time I get off work and write a patch. -- brian m. carlson / brian with sandals: Houston, Texas, US https://www.crustytoothpaste.net/~bmc | My opinion only OpenPGP: https://keybase.io/bk2204 signature.asc Description: PGP signature
Re: [PATCH 1/2] t1404: add a bunch of tests of D/F conflicts
"brian m. carlson" writes: > There is discussion in the Austin Group issue tracker about adding this > feature to POSIX, but it's gotten bogged down over lexical versus > dynamic scoping. Everyone agrees that it's a desirable feature, though. > ... In short, unless you are a binary packager on a platform whose native shell is ksh and who refuses to depend on tools that are not default/native on the platform, you'd be OK? > I'd recommend an explicit test for this. It's much easier to track down > that way than seeing other failure scenarios. People will also usually > complain about failing tests. Hopefully. Starting from an explicit test, gradually using more "local" in tests that cover more important parts of the system, and then start using "local" as appropriate in the main tools would be a good way forward. By that time, we might have a lot less scripted Porcelains, though ;-)
Re: [PATCH 1/2] t1404: add a bunch of tests of D/F conflicts
On Wed, Oct 25, 2017 at 11:32:51PM -0700, Jeff King wrote: > On Wed, Oct 25, 2017 at 10:03:09AM +0200, Michael Haggerty wrote: > > > > Yeah. It's supported by dash and many other shells, but we do try to > > > avoid it[1]. I think in this case we could just drop it (but keep > > > setting the "local foo" ones to empty with "foo=". > > > > I do wish that we could allow "local", as it avoids a lot of headaches > > and potential breakage. According to [1], > > Agreed. This would be useful. Debian requires that all implementations that implement /bin/sh support local and a small number of other features. There is discussion in the Austin Group issue tracker about adding this feature to POSIX, but it's gotten bogged down over lexical versus dynamic scoping. Everyone agrees that it's a desirable feature, though. > > He mentions that ksh93 doesn't support "local", but that it differs from > > POSIX in many other ways, too. > > Yes, the conclusion we came to in the thread I linked earlier is the > same: ksh is affected, but that shell is a problem for other reasons. I > don't know if anybody tested with "modern" ksh like mksh, though. Should > be easy enough: As far as I can tell, bash, dash, posh, mksh, pdksh, zsh, and busybox sh all support local. From my reading of the documentation, so does sh on FreeBSD, NetBSD, and OpenBSD. Not all of these are good choices for a POSIXy sh, though. ksh93 will support local if you alias it to typeset, but only when called from functions defined with "function", not normal shell-style functions. I have a gist[0] that does absurd things to work around that, but I wouldn't recommend that for production use. Solaris 11.1's man page doesn't document local in sh (which is a ksh88 variant) and ksh is ksh93, so it doesn't appear to support it. Solaris 11.3 documents bash, so it's a non-issue there. It's my understanding that using ksh as a POSIXy sh variant is very common on proprietary Unices, so its lack of compatibility may be a dealbreaker. Then again, many of those systems may have bash installed. > > Perhaps we could slip in a couple of "local" as a compatibility test to > > see if anybody complains, like we did with a couple of C features recently. > > That sounds reasonable to me. But from the earlier conversation, beware > that: > > local x > ... > x=5 > > is not necessarily enough to notice the problem on broken shells (they > may complain that "local" is not a command, and quietly stomp on the > global). I think: > > local x=5 > > would be enough (though depend on how you use $x, the failure mode might > be pretty subtle). Or we could even add an explicit test in t like > the example above. I'd recommend an explicit test for this. It's much easier to track down that way than seeing other failure scenarios. People will also usually complain about failing tests. [0] https://gist.github.com/bk2204/9dd24df0e4499a02a300578ebdca4728 -- brian m. carlson / brian with sandals: Houston, Texas, US https://www.crustytoothpaste.net/~bmc | My opinion only OpenPGP: https://keybase.io/bk2204 signature.asc Description: PGP signature
Re: [PATCH 1/2] t1404: add a bunch of tests of D/F conflicts
On Wed, Oct 25, 2017 at 10:03:09AM +0200, Michael Haggerty wrote: > > Yeah. It's supported by dash and many other shells, but we do try to > > avoid it[1]. I think in this case we could just drop it (but keep > > setting the "local foo" ones to empty with "foo=". > > I do wish that we could allow "local", as it avoids a lot of headaches > and potential breakage. According to [1], Agreed. > > However, every single POSIX-compliant shell I've tested implements the > > 'local' keyword, which lets you declare variables that won't be > > returned from the current function. So nowadays you can safely count > > on it working. > > He mentions that ksh93 doesn't support "local", but that it differs from > POSIX in many other ways, too. Yes, the conclusion we came to in the thread I linked earlier is the same: ksh is affected, but that shell is a problem for other reasons. I don't know if anybody tested with "modern" ksh like mksh, though. Should be easy enough: cat >foo.sh <<\EOF fun() { local x=3 echo inside: $x } x=2 fun echo after: $x EOF mksh foo.sh which produces the expected: inside: 3 after: 2 So that's good. > Perhaps we could slip in a couple of "local" as a compatibility test to > see if anybody complains, like we did with a couple of C features recently. That sounds reasonable to me. But from the earlier conversation, beware that: local x ... x=5 is not necessarily enough to notice the problem on broken shells (they may complain that "local" is not a command, and quietly stomp on the global). I think: local x=5 would be enough (though depend on how you use $x, the failure mode might be pretty subtle). Or we could even add an explicit test in t like the example above. > But I agree that this bug fix is not the correct occasion to change > policy on something like this, so I'll reroll without "local". Also agreed. -Peff
Re: [PATCH 1/2] t1404: add a bunch of tests of D/F conflicts
On 10/25/2017 10:03 AM, Michael Haggerty wrote: > On 10/24/2017 09:46 PM, Jeff King wrote: >> On Tue, Oct 24, 2017 at 12:19:26PM -0400, Eric Sunshine wrote: >> >>> On Tue, Oct 24, 2017 at 11:16 AM, Michael Haggerty >>> wrote: diff --git a/t/t1404-update-ref-errors.sh b/t/t1404-update-ref-errors.sh @@ -34,6 +34,86 @@ test_update_rejected () { +# Test adding and deleting D/F-conflicting references in a single +# transaction. +df_test() { + local prefix="$1" + shift + local pack=: >>> >>> Isn't "local" a Bash-ism we want to keep out of the test scripts? >> >> Yeah. It's supported by dash and many other shells, but we do try to >> avoid it[1]. I think in this case we could just drop it (but keep >> setting the "local foo" ones to empty with "foo=". > [...] > > But I agree that this bug fix is not the correct occasion to change > policy on something like this, so I'll reroll without "local". Oh, I see Junio has already made this fix in the version that he picked up, so I won't bother rerolling. Michael
Re: [PATCH 1/2] t1404: add a bunch of tests of D/F conflicts
On 10/24/2017 09:46 PM, Jeff King wrote: > On Tue, Oct 24, 2017 at 12:19:26PM -0400, Eric Sunshine wrote: > >> On Tue, Oct 24, 2017 at 11:16 AM, Michael Haggerty >> wrote: >>> diff --git a/t/t1404-update-ref-errors.sh b/t/t1404-update-ref-errors.sh >>> @@ -34,6 +34,86 @@ test_update_rejected () { >>> +# Test adding and deleting D/F-conflicting references in a single >>> +# transaction. >>> +df_test() { >>> + local prefix="$1" >>> + shift >>> + local pack=: >> >> Isn't "local" a Bash-ism we want to keep out of the test scripts? > > Yeah. It's supported by dash and many other shells, but we do try to > avoid it[1]. I think in this case we could just drop it (but keep > setting the "local foo" ones to empty with "foo=". I do wish that we could allow "local", as it avoids a lot of headaches and potential breakage. According to [1], > However, every single POSIX-compliant shell I've tested implements the > 'local' keyword, which lets you declare variables that won't be > returned from the current function. So nowadays you can safely count > on it working. He mentions that ksh93 doesn't support "local", but that it differs from POSIX in many other ways, too. Perhaps we could slip in a couple of "local" as a compatibility test to see if anybody complains, like we did with a couple of C features recently. But I agree that this bug fix is not the correct occasion to change policy on something like this, so I'll reroll without "local". Michael [1] http://apenwarr.ca/log/?m=201102#28
Re: [PATCH 1/2] t1404: add a bunch of tests of D/F conflicts
On Tue, Oct 24, 2017 at 12:19:26PM -0400, Eric Sunshine wrote: > On Tue, Oct 24, 2017 at 11:16 AM, Michael Haggerty > wrote: > > diff --git a/t/t1404-update-ref-errors.sh b/t/t1404-update-ref-errors.sh > > @@ -34,6 +34,86 @@ test_update_rejected () { > > +# Test adding and deleting D/F-conflicting references in a single > > +# transaction. > > +df_test() { > > + local prefix="$1" > > + shift > > + local pack=: > > Isn't "local" a Bash-ism we want to keep out of the test scripts? Yeah. It's supported by dash and many other shells, but we do try to avoid it[1]. I think in this case we could just drop it (but keep setting the "local foo" ones to empty with "foo=". -Peff [1] There's some more discussion in the thread at: https://public-inbox.org/git/20160601163747.ga10...@sigill.intra.peff.net/
Re: [PATCH 1/2] t1404: add a bunch of tests of D/F conflicts
On Tue, Oct 24, 2017 at 11:16 AM, Michael Haggerty wrote: > diff --git a/t/t1404-update-ref-errors.sh b/t/t1404-update-ref-errors.sh > @@ -34,6 +34,86 @@ test_update_rejected () { > +# Test adding and deleting D/F-conflicting references in a single > +# transaction. > +df_test() { > + local prefix="$1" > + shift > + local pack=: Isn't "local" a Bash-ism we want to keep out of the test scripts? > + local symadd=false > + local symdel=false > + local add_del=false > + local addref > + local delref > + while test $# -gt 0 > + do
[PATCH 1/2] t1404: add a bunch of tests of D/F conflicts
It is currently not allowed, in a single transaction, to add one reference and delete another reference if the two reference names D/F conflict with each other (e.g., like `refs/foo/bar` and `refs/foo`). The reason is that the code would need to take locks $GIT_DIR/refs/foo.lock $GIT_DIR/refs/foo/bar.lock But the latter lock couldn't coexist with the loose reference file $GIT_DIR/refs/foo , because `$GIT_DIR/refs/foo` cannot be both a directory and a file at the same time (hence the name "D/F conflict). Add a bunch of tests that we cleanly reject such transactions. In fact, many of the new tests currently fail. They will be fixed in the next commit along with an explanation. Reported-by: Jeff King Signed-off-by: Michael Haggerty --- t/t1404-update-ref-errors.sh | 146 +++ 1 file changed, 146 insertions(+) diff --git a/t/t1404-update-ref-errors.sh b/t/t1404-update-ref-errors.sh index 100d50e362..8b5e9a83c5 100755 --- a/t/t1404-update-ref-errors.sh +++ b/t/t1404-update-ref-errors.sh @@ -34,6 +34,86 @@ test_update_rejected () { Q="'" +# Test adding and deleting D/F-conflicting references in a single +# transaction. +df_test() { + local prefix="$1" + shift + local pack=: + local symadd=false + local symdel=false + local add_del=false + local addref + local delref + while test $# -gt 0 + do + case "$1" in + --pack) + pack="git pack-refs --all" + shift + ;; + --sym-add) + # Perform the add via a symbolic reference + symadd=true + shift + ;; + --sym-del) + # Perform the del via a symbolic reference + symdel=true + shift + ;; + --del-add) + # Delete first reference then add second + add_del=false + delref="$prefix/r/$2" + addref="$prefix/r/$3" + shift 3 + ;; + --add-del) + # Add first reference then delete second + add_del=true + addref="$prefix/r/$2" + delref="$prefix/r/$3" + shift 3 + ;; + *) + echo 1>&2 "Extra args to df_test: $*" + return 1 + ;; + esac + done + git update-ref "$delref" $C && + if $symadd + then + addname="$prefix/s/symadd" && + git symbolic-ref "$addname" "$addref" + else + addname="$addref" + fi && + if $symdel + then + delname="$prefix/s/symdel" && + git symbolic-ref "$delname" "$delref" + else + delname="$delref" + fi && + cat >expected-err <<-EOF && + fatal: cannot lock ref $Q$addname$Q: $Q$delref$Q exists; cannot create $Q$addref$Q + EOF + $pack && + if $add_del + then + printf "%s\n" "create $addname $D" "delete $delname" + else + printf "%s\n" "delete $delname" "create $addname $D" + fi >commands && + test_must_fail git update-ref --stdin output.err && + test_cmp expected-err output.err && + printf "%s\n" "$C $delref" >expected-refs && + git for-each-ref --format="%(objectname) %(refname)" $prefix/r >actual-refs && + test_cmp expected-refs actual-refs +} + test_expect_success 'setup' ' git commit --allow-empty -m Initial && @@ -188,6 +268,72 @@ test_expect_success 'empty directory should not fool 1-arg delete' ' git update-ref --stdin ' +test_expect_success 'D/F conflict prevents add long + delete short' ' + df_test refs/df-al-ds --add-del foo/bar foo +' + +test_expect_success 'D/F conflict prevents add short + delete long' ' + df_test refs/df-as-dl --add-del foo foo/bar +' + +test_expect_failure 'D/F conflict prevents delete long + add short' ' + df_test refs/df-dl-as --del-add foo/bar foo +' + +test_expect_failure 'D/F conflict prevents delete short + add long' ' + df_test refs/df-ds-al --del-add foo foo/bar +' + +test_expect_success 'D/F conflict prevents add long + delete short packed' ' + df_test refs/df-al-dsp --pack --add-del foo/bar foo +' + +test_expect_success 'D/F conflict prevents add short + delete long packed' ' + df_test refs/df-as-dlp --pack --add-del foo foo/bar +' + +test_expect_failure 'D/F conflict prevents delete long packed + add short' ' + df_test refs/df-dlp-as --pack --del-add foo/bar foo +' + +test_expect_failure 'D/F conflict prevents delete short packe