{"thread":{"id":"47020","subject":"[PATCH 0/2] Fix an error-handling path when locking refs","startedAt":"2017-10-24T15:16:45Z","lastAt":"2017-10-29T16:12:20Z","messageCount":14,"participants":["Michael Haggerty","Eric Sunshine","Jeff King","Junio C Hamano","brian m. carlson"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"330915","messageId":"cover.1508856679.git.mhagger@alum.mit.edu","threadId":"47020","inReplyTo":null,"subject":"[PATCH 0/2] Fix an error-handling path when locking refs","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-10-24T15:16:23Z","receivedAt":"2017-10-24T15:16:45Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"In [1], Peff described a bug that he found that could cause a\nreference transaction to be partly carried-out and partly not, with a\nsuccessful return. The scenario that he discussed was the deletion of\none reference and creation of another, where the two references D/F\nconflict with each other.\n\nBut in fact the problem is more general; it can happen whenever a\nreference deletion within a transaction is processed successfully, and\nthen another reference update in the same transaction fails in\n`lock_ref_for_update()`. In such a case the failed update and any\nsubsequent ones could be silently ignored.\n\nThere is a longer explanation in the second patch's commit message.\n\nThe tests that I added probe a bunch of D/F update scenarios, which I\nthink should be characteristic of the scenarios that would trigger\nthis bug. It would be nice to have tests that examine other types of\nfailures (e.g., wrong `old_oid` values).\n\nBit since the fix is obviously a strict improvement and can prevent\ndata loss, and since the release process is already pretty far along,\nI wanted to send this out ASAP. We can follow it up later with\nadditional tests.\n\nThese patches apply to current master. They are also available from my\nGitHub fork [2] as branch `ref-locking-fix`.\n\nWhile looking at this code again, I realized that the new code\nrewrites the `packed-refs` file more often than did the old code.\nSpecifically, after dc39e09942 (files_ref_store: use a transaction to\nupdate packed refs, 2017-09-08), the `packed-refs` file is overwritten\nfor any transaction that includes a reference delete. Prior to that\ncommit, `packed-refs` was only overwritten if a deleted reference was\nactually present in the existing `packed-refs` file. This is even the\ncase if there was previously no `packed-refs` file at all; now any\nreference deletion causes an empty `packed-refs` file to be created.\n\nI think this is a conscionable regression, since deleting references\nthat are purely loose is probably not all that common, and the old\ncode had to read the whole `packed-refs` file anyway. Nevertheless, it\nis obviously something that I would like to fix (though maybe not for\n2.15? I'm open to input about its urgency.)\n\n[1] https://public-inbox.org/git/20171024082409.smwsd6pla64jjlua@sigill.intra.peff.net/\n[2] https://github.com/mhagger/git\n\nMichael Haggerty (2):\n t1404: add a bunch of tests of D/F conflicts\n files_transaction_prepare(): fix handling of ref lock failure\n\n refs/files-backend.c | 2 +-\n t/t1404-update-ref-errors.sh | 146 +++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 147 insertions(+), 1 deletion(-)\n\n-- \n2.14.1\n\n"},{"id":"330916","messageId":"be088bd57e61f4ea0dc974a65829a928ecd30534.1508856679.git.mhagger@alum.mit.edu","threadId":"47020","inReplyTo":"cover.1508856679.git.mhagger@alum.mit.edu","subject":"[PATCH 1/2] t1404: add a bunch of tests of D/F conflicts","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-10-24T15:16:24Z","receivedAt":"2017-10-24T15:16:49Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"It is currently not allowed, in a single transaction, to add one\nreference and delete another reference if the two reference names D/F\nconflict with each other (e.g., like `refs/foo/bar` and `refs/foo`).\nThe reason is that the code would need to take locks\n\n $GIT_DIR/refs/foo.lock\n $GIT_DIR/refs/foo/bar.lock\n\nBut the latter lock couldn't coexist with the loose reference file\n\n $GIT_DIR/refs/foo\n\n, because `$GIT_DIR/refs/foo` cannot be both a directory and a file at\nthe same time (hence the name \"D/F conflict).\n\nAdd a bunch of tests that we cleanly reject such transactions.\n\nIn fact, many of the new tests currently fail. They will be fixed in\nthe next commit along with an explanation.\n\nReported-by: Jeff King \nSigned-off-by: Michael Haggerty \n---\n t/t1404-update-ref-errors.sh | 146 +++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 146 insertions(+)\n\ndiff --git a/t/t1404-update-ref-errors.sh b/t/t1404-update-ref-errors.sh\nindex 100d50e362..8b5e9a83c5 100755\n--- a/t/t1404-update-ref-errors.sh\n+++ b/t/t1404-update-ref-errors.sh\n@@ -34,6 +34,86 @@ test_update_rejected () {\n \n Q=\"'\"\n \n+# Test adding and deleting D/F-conflicting references in a single\n+# transaction.\n+df_test() {\n+\tlocal prefix=\"$1\"\n+\tshift\n+\tlocal pack=:\n+\tlocal symadd=false\n+\tlocal symdel=false\n+\tlocal add_del=false\n+\tlocal addref\n+\tlocal delref\n+\twhile test $# -gt 0\n+\tdo\n+\t\tcase \"$1\" in\n+\t\t--pack)\n+\t\t\tpack=\"git pack-refs --all\"\n+\t\t\tshift\n+\t\t\t;;\n+\t\t--sym-add)\n+\t\t\t# Perform the add via a symbolic reference\n+\t\t\tsymadd=true\n+\t\t\tshift\n+\t\t\t;;\n+\t\t--sym-del)\n+\t\t\t# Perform the del via a symbolic reference\n+\t\t\tsymdel=true\n+\t\t\tshift\n+\t\t\t;;\n+\t\t--del-add)\n+\t\t\t# Delete first reference then add second\n+\t\t\tadd_del=false\n+\t\t\tdelref=\"$prefix/r/$2\"\n+\t\t\taddref=\"$prefix/r/$3\"\n+\t\t\tshift 3\n+\t\t\t;;\n+\t\t--add-del)\n+\t\t\t# Add first reference then delete second\n+\t\t\tadd_del=true\n+\t\t\taddref=\"$prefix/r/$2\"\n+\t\t\tdelref=\"$prefix/r/$3\"\n+\t\t\tshift 3\n+\t\t\t;;\n+\t\t*)\n+\t\t\techo 1>&2 \"Extra args to df_test: $*\"\n+\t\t\treturn 1\n+\t\t\t;;\n+\t\tesac\n+\tdone\n+\tgit update-ref \"$delref\" $C &&\n+\tif $symadd\n+\tthen\n+\t\taddname=\"$prefix/s/symadd\" &&\n+\t\tgit symbolic-ref \"$addname\" \"$addref\"\n+\telse\n+\t\taddname=\"$addref\"\n+\tfi &&\n+\tif $symdel\n+\tthen\n+\t\tdelname=\"$prefix/s/symdel\" &&\n+\t\tgit symbolic-ref \"$delname\" \"$delref\"\n+\telse\n+\t\tdelname=\"$delref\"\n+\tfi &&\n+\tcat >expected-err <<-EOF &&\n+\tfatal: cannot lock ref $Q$addname$Q: $Q$delref$Q exists; cannot create $Q$addref$Q\n+\tEOF\n+\t$pack &&\n+\tif $add_del\n+\tthen\n+\t\tprintf \"%s\\n\" \"create $addname $D\" \"delete $delname\"\n+\telse\n+\t\tprintf \"%s\\n\" \"delete $delname\" \"create $addname $D\"\n+\tfi >commands &&\n+\ttest_must_fail git update-ref --stdin output.err &&\n+\ttest_cmp expected-err output.err &&\n+\tprintf \"%s\\n\" \"$C $delref\" >expected-refs &&\n+\tgit for-each-ref --format=\"%(objectname) %(refname)\" $prefix/r >actual-refs &&\n+\ttest_cmp expected-refs actual-refs\n+}\n+\n test_expect_success 'setup' '\n \n \tgit commit --allow-empty -m Initial &&\n@@ -188,6 +268,72 @@ test_expect_success 'empty directory should not fool 1-arg delete' '\n \tgit update-ref --stdin\n '\n \n+test_expect_success 'D/F conflict prevents add long + delete short' '\n+\tdf_test refs/df-al-ds --add-del foo/bar foo\n+'\n+\n+test_expect_success 'D/F conflict prevents add short + delete long' '\n+\tdf_test refs/df-as-dl --add-del foo foo/bar\n+'\n+\n+test_expect_failure 'D/F conflict prevents delete long + add short' '\n+\tdf_test refs/df-dl-as --del-add foo/bar foo\n+'\n+\n+test_expect_failure 'D/F conflict prevents delete short + add long' '\n+\tdf_test refs/df-ds-al --del-add foo foo/bar\n+'\n+\n+test_expect_success 'D/F conflict prevents add long + delete short packed' '\n+\tdf_test refs/df-al-dsp --pack --add-del foo/bar foo\n+'\n+\n+test_expect_success 'D/F conflict prevents add short + delete long packed' '\n+\tdf_test refs/df-as-dlp --pack --add-del foo foo/bar\n+'\n+\n+test_expect_failure 'D/F conflict prevents delete long packed + add short' '\n+\tdf_test refs/df-dlp-as --pack --del-add foo/bar foo\n+'\n+\n+test_expect_failure 'D/F conflict prevents delete short packed + add long' '\n+\tdf_test refs/df-dsp-al --pack --del-add foo foo/bar\n+'\n+\n+# Try some combinations involving symbolic refs...\n+\n+test_expect_failure 'D/F conflict prevents indirect add long + delete short' '\n+\tdf_test refs/df-ial-ds --sym-add --add-del foo/bar foo\n+'\n+\n+test_expect_success 'D/F conflict prevents indirect add long + indirect delete short' '\n+\tdf_test refs/df-ial-ids --sym-add --sym-del --add-del foo/bar foo\n+'\n+\n+test_expect_success 'D/F conflict prevents indirect add short + indirect delete long' '\n+\tdf_test refs/df-ias-idl --sym-add --sym-del --add-del foo foo/bar\n+'\n+\n+test_expect_failure 'D/F conflict prevents indirect delete long + indirect add short' '\n+\tdf_test refs/df-idl-ias --sym-add --sym-del --del-add foo/bar foo\n+'\n+\n+test_expect_failure 'D/F conflict prevents indirect add long + delete short packed' '\n+\tdf_test refs/df-ial-dsp --sym-add --pack --add-del foo/bar foo\n+'\n+\n+test_expect_success 'D/F conflict prevents indirect add long + indirect delete short packed' '\n+\tdf_test refs/df-ial-idsp --sym-add --sym-del --pack --add-del foo/bar foo\n+'\n+\n+test_expect_success 'D/F conflict prevents add long + indirect delete short packed' '\n+\tdf_test refs/df-al-idsp --sym-del --pack --add-del foo/bar foo\n+'\n+\n+test_expect_failure 'D/F conflict prevents indirect delete long packed + indirect add short' '\n+\tdf_test refs/df-idlp-ias --sym-add --sym-del --pack --del-add foo/bar foo\n+'\n+\n # Test various errors when reading the old values of references...\n \n test_expect_success 'missing old value blocks update' '\n-- \n2.14.1\n\n"},{"id":"330917","messageId":"6214107e1232a7fe7ca4b7440733ff496f07e537.1508856679.git.mhagger@alum.mit.edu","threadId":"47020","inReplyTo":"cover.1508856679.git.mhagger@alum.mit.edu","subject":"[PATCH 2/2] files_transaction_prepare(): fix handling of ref lock failure","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-10-24T15:16:25Z","receivedAt":"2017-10-24T15:16:53Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Since dc39e09942 (files_ref_store: use a transaction to update packed\nrefs, 2017-09-08), failure to lock a reference has been handled\nincorrectly by `files_transaction_prepare()`. If\n`lock_ref_for_update()` fails in the lock-acquisition loop of that\nfunction, it sets `ret` then breaks out of that loop. Prior to\ndc39e09942, that was OK, because the only thing following the loop was\nthe cleanup code. But dc39e09942 added another blurb of code between\nthe loop and the cleanup. That blurb sometimes resets `ret` to zero,\nmaking the cleanup code think that the locking was successful.\n\nSpecifically, whenever\n\n* One or more reference deletions have been processed successfully in\n the lock-acquisition loop. (Processing the first such reference\n causes a packed-ref transaction to be initialized.)\n\n* Then `lock_ref_for_update()` fails for a subsequent reference. Such\n a failure can happen for a number of reasons, such as the old SHA-1\n not being correct, lock contention, etc. This causes a `break` out\n of the lock-acquisition loop.\n\n* The `packed-refs` lock is acquired successfully and\n `ref_transaction_prepare()` succeeds for the packed-ref transaction.\n This has the effect of resetting `ret` back to 0, and making the\n cleanup code think that lock acquisition was successful.\n\nIn that case, any reference updates that were processed prior to\nbreaking out of the loop would be carried out (loose and packed), but\nthe reference that couldn't be locked and any subsequent references\nwould silently be ignored.\n\nThis can easily cause data loss if, for example, the user was trying\nto push a new name for an existing branch while deleting the old name.\nAfter the push, the branch could be left unreachable, and could even\nsubsequently be garbage-collected.\n\nThis problem was noticed in the context of deleting one reference and\ncreating another in a single transaction, when the two references D/F\nconflict with each other, like\n\n git update-ref --stdin <\n---\n refs/files-backend.c | 2 +-\n t/t1404-update-ref-errors.sh | 16 ++++++++--------\n 2 files changed, 9 insertions(+), 9 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 014dabb0bf..8cc1e07fdb 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -2570,7 +2570,7 @@ static int files_transaction_prepare(struct ref_store *ref_store,\n \t\tret = lock_ref_for_update(refs, update, transaction,\n \t\t\t\t\t head_ref, &affected_refnames, err);\n \t\tif (ret)\n-\t\t\tbreak;\n+\t\t\tgoto cleanup;\n \n \t\tif (update->flags & REF_DELETING &&\n \t\t !(update->flags & REF_LOG_ONLY) &&\ndiff --git a/t/t1404-update-ref-errors.sh b/t/t1404-update-ref-errors.sh\nindex 8b5e9a83c5..b7838967b8 100755\n--- a/t/t1404-update-ref-errors.sh\n+++ b/t/t1404-update-ref-errors.sh\n@@ -276,11 +276,11 @@ test_expect_success 'D/F conflict prevents add short + delete long' '\n \tdf_test refs/df-as-dl --add-del foo foo/bar\n '\n \n-test_expect_failure 'D/F conflict prevents delete long + add short' '\n+test_expect_success 'D/F conflict prevents delete long + add short' '\n \tdf_test refs/df-dl-as --del-add foo/bar foo\n '\n \n-test_expect_failure 'D/F conflict prevents delete short + add long' '\n+test_expect_success 'D/F conflict prevents delete short + add long' '\n \tdf_test refs/df-ds-al --del-add foo foo/bar\n '\n \n@@ -292,17 +292,17 @@ test_expect_success 'D/F conflict prevents add short + delete long packed' '\n \tdf_test refs/df-as-dlp --pack --add-del foo foo/bar\n '\n \n-test_expect_failure 'D/F conflict prevents delete long packed + add short' '\n+test_expect_success 'D/F conflict prevents delete long packed + add short' '\n \tdf_test refs/df-dlp-as --pack --del-add foo/bar foo\n '\n \n-test_expect_failure 'D/F conflict prevents delete short packed + add long' '\n+test_expect_success 'D/F conflict prevents delete short packed + add long' '\n \tdf_test refs/df-dsp-al --pack --del-add foo foo/bar\n '\n \n # Try some combinations involving symbolic refs...\n \n-test_expect_failure 'D/F conflict prevents indirect add long + delete short' '\n+test_expect_success 'D/F conflict prevents indirect add long + delete short' '\n \tdf_test refs/df-ial-ds --sym-add --add-del foo/bar foo\n '\n \n@@ -314,11 +314,11 @@ test_expect_success 'D/F conflict prevents indirect add short + indirect delete\n \tdf_test refs/df-ias-idl --sym-add --sym-del --add-del foo foo/bar\n '\n \n-test_expect_failure 'D/F conflict prevents indirect delete long + indirect add short' '\n+test_expect_success 'D/F conflict prevents indirect delete long + indirect add short' '\n \tdf_test refs/df-idl-ias --sym-add --sym-del --del-add foo/bar foo\n '\n \n-test_expect_failure 'D/F conflict prevents indirect add long + delete short packed' '\n+test_expect_success 'D/F conflict prevents indirect add long + delete short packed' '\n \tdf_test refs/df-ial-dsp --sym-add --pack --add-del foo/bar foo\n '\n \n@@ -330,7 +330,7 @@ test_expect_success 'D/F conflict prevents add long + indirect delete short pack\n \tdf_test refs/df-al-idsp --sym-del --pack --add-del foo/bar foo\n '\n \n-test_expect_failure 'D/F conflict prevents indirect delete long packed + indirect add short' '\n+test_expect_success 'D/F conflict prevents indirect delete long packed + indirect add short' '\n \tdf_test refs/df-idlp-ias --sym-add --sym-del --pack --del-add foo/bar foo\n '\n \n-- \n2.14.1\n\n"},{"id":"330923","messageId":"CAPig+cRLB=dGD=+Af=yYL3M709LRpeUrtvcDLo9iBKYy2HAW-w@mail.gmail.com","threadId":"47020","inReplyTo":"be088bd57e61f4ea0dc974a65829a928ecd30534.1508856679.git.mhagger@alum.mit.edu","subject":"Re: [PATCH 1/2] t1404: add a bunch of tests of D/F conflicts","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2017-10-24T16:19:26Z","receivedAt":"2017-10-24T16:19:32Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Oct 24, 2017 at 11:16 AM, Michael Haggerty wrote:\n> diff --git a/t/t1404-update-ref-errors.sh b/t/t1404-update-ref-errors.sh\n> @@ -34,6 +34,86 @@ test_update_rejected () {\n> +# Test adding and deleting D/F-conflicting references in a single\n> +# transaction.\n> +df_test() {\n> + local prefix=\"$1\"\n> + shift\n> + local pack=:\n\nIsn't \"local\" a Bash-ism we want to keep out of the test scripts?\n\n> + local symadd=false\n> + local symdel=false\n> + local add_del=false\n> + local addref\n> + local delref\n> + while test $# -gt 0\n> + do\n"},{"id":"330957","messageId":"20171024194507.h4xduhx3pq2y2yyw@sigill.intra.peff.net","threadId":"47020","inReplyTo":"cover.1508856679.git.mhagger@alum.mit.edu","subject":"Re: [PATCH 0/2] Fix an error-handling path when locking refs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-10-24T19:45:08Z","receivedAt":"2017-10-24T19:45:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Oct 24, 2017 at 05:16:23PM +0200, Michael Haggerty wrote:\n\n> But in fact the problem is more general; it can happen whenever a\n> reference deletion within a transaction is processed successfully, and\n> then another reference update in the same transaction fails in\n> `lock_ref_for_update()`. In such a case the failed update and any\n> subsequent ones could be silently ignored.\n\nThanks for digging this down to the root cause. It sounds like this\nmight not be all that hard to trigger for users of \"push --atomic\", then\n(most of the rest of the code is saved by the fact that it still uses\none ref per transaction, which I think can't trigger this).\n\n> There is a longer explanation in the second patch's commit message.\n> \n> The tests that I added probe a bunch of D/F update scenarios, which I\n> think should be characteristic of the scenarios that would trigger\n> this bug. It would be nice to have tests that examine other types of\n> failures (e.g., wrong `old_oid` values).\n\nWhat you added makes sense to me for now. I do admit I was a little\nsurprised that we didn't have better test coverage for partial\ntransaction failures. I think in general our test suite is weak on\nchecking failures, and covering more cases (like old_oid) would be\nwelcome. Those will be valuable especially as we started playing with\nmore storage backends.\n\nI also agree that those tests can come post-release.\n\n> While looking at this code again, I realized that the new code\n> rewrites the `packed-refs` file more often than did the old code.\n> Specifically, after dc39e09942 (files_ref_store: use a transaction to\n> update packed refs, 2017-09-08), the `packed-refs` file is overwritten\n> for any transaction that includes a reference delete. Prior to that\n> commit, `packed-refs` was only overwritten if a deleted reference was\n> actually present in the existing `packed-refs` file. This is even the\n> case if there was previously no `packed-refs` file at all; now any\n> reference deletion causes an empty `packed-refs` file to be created.\n> \n> I think this is a conscionable regression, since deleting references\n> that are purely loose is probably not all that common, and the old\n> code had to read the whole `packed-refs` file anyway. Nevertheless, it\n> is obviously something that I would like to fix (though maybe not for\n> 2.15? I'm open to input about its urgency.)\n\nI agree it's not nearly as urgent as the fix you have here. It might\nshow up on a busy system as increased lock contention over packed-refs.\nOr if you have really gigantic packed-refs files, a possible performance\nregression. But as you say, it should be a pretty rare case.\n\nSo I'd be OK with leaving it to post-2.15. OTOH, I suspect it's a pretty\nsmall patch. If you happen to produce it quickly, it might be worth\nseeing before evaluating.\n\n-Peff\n"},{"id":"330958","messageId":"20171024194634.pigmvgoqtdexvjhc@sigill.intra.peff.net","threadId":"47020","inReplyTo":"CAPig+cRLB=dGD=+Af=yYL3M709LRpeUrtvcDLo9iBKYy2HAW-w@mail.gmail.com","subject":"Re: [PATCH 1/2] t1404: add a bunch of tests of D/F conflicts","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-10-24T19:46:34Z","receivedAt":"2017-10-24T19:46:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Oct 24, 2017 at 12:19:26PM -0400, Eric Sunshine wrote:\n\n> On Tue, Oct 24, 2017 at 11:16 AM, Michael Haggerty wrote:\n> > diff --git a/t/t1404-update-ref-errors.sh b/t/t1404-update-ref-errors.sh\n> > @@ -34,6 +34,86 @@ test_update_rejected () {\n> > +# Test adding and deleting D/F-conflicting references in a single\n> > +# transaction.\n> > +df_test() {\n> > + local prefix=\"$1\"\n> > + shift\n> > + local pack=:\n> \n> Isn't \"local\" a Bash-ism we want to keep out of the test scripts?\n\nYeah. It's supported by dash and many other shells, but we do try to\navoid it[1]. I think in this case we could just drop it (but keep\nsetting the \"local foo\" ones to empty with \"foo=\".\n\n-Peff\n\n[1] There's some more discussion in the thread at:\n\n https://public-inbox.org/git/20160601163747.GA10721@sigill.intra.peff.net/\n"},{"id":"330959","messageId":"20171024194921.mnrx3sxsd3w3pe3v@sigill.intra.peff.net","threadId":"47020","inReplyTo":"6214107e1232a7fe7ca4b7440733ff496f07e537.1508856679.git.mhagger@alum.mit.edu","subject":"Re: [PATCH 2/2] files_transaction_prepare(): fix handling of ref lock failure","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-10-24T19:49:21Z","receivedAt":"2017-10-24T19:49:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Oct 24, 2017 at 05:16:25PM +0200, Michael Haggerty wrote:\n\n> The fix is simple: instead of just breaking out of the loop, jump\n> directly to the cleanup code. This fixes some tests in t1404 that were\n> added in the previous commit.\n\nNicely explained.\n\nI think that fix makes sense, and matches how the rest of the function\nbehaved. The other option would be to switch the recently-added code to:\n\n if (!ret && packed_transaction)\n ...\n\nbut it seems like the \"goto\" means one less thing for new code to\nremember if it gets added.\n\n-Peff\n"},{"id":"330981","messageId":"xmqqwp3khsek.fsf@gitster.mtv.corp.google.com","threadId":"47020","inReplyTo":"6214107e1232a7fe7ca4b7440733ff496f07e537.1508856679.git.mhagger@alum.mit.edu","subject":"Re: [PATCH 2/2] files_transaction_prepare(): fix handling of ref lock failure","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-10-25T02:29:07Z","receivedAt":"2017-10-25T02:29:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> ... But dc39e09942 added another blurb of code between\n> the loop and the cleanup. That blurb sometimes resets `ret` to zero,\n> making the cleanup code think that the locking was successful.\n> ...\n> The fix is simple: instead of just breaking out of the loop, jump\n> directly to the cleanup code. This fixes some tests in t1404 that were\n> added in the previous commit.\n\nOK. Now because we do not break and start packed_transaction but\ninstead jump over that if statement, we'll leave packed_transation\ninstance that we got from transaction_begin() that we called\nadd_update() on, but haven't called transaction_prepare() on\nbehind. That instance is pointed by backend_data pointer which is\npart of transaction, so presumably transaction_cleanup() called on\nit in the section labelled \"cleanup:\" should take care of it?\n\nThanks for catching the issue and fixing it quickly. Will queue.\n"},{"id":"330999","messageId":"e66694a3-98e2-33aa-4cdd-dac41d2a89d5@alum.mit.edu","threadId":"47020","inReplyTo":"20171024194634.pigmvgoqtdexvjhc@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] t1404: add a bunch of tests of D/F conflicts","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-10-25T08:03:09Z","receivedAt":"2017-10-25T08:03:23Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 10/24/2017 09:46 PM, Jeff King wrote:\n> On Tue, Oct 24, 2017 at 12:19:26PM -0400, Eric Sunshine wrote:\n> \n>> On Tue, Oct 24, 2017 at 11:16 AM, Michael Haggerty wrote:\n>>> diff --git a/t/t1404-update-ref-errors.sh b/t/t1404-update-ref-errors.sh\n>>> @@ -34,6 +34,86 @@ test_update_rejected () {\n>>> +# Test adding and deleting D/F-conflicting references in a single\n>>> +# transaction.\n>>> +df_test() {\n>>> + local prefix=\"$1\"\n>>> + shift\n>>> + local pack=:\n>>\n>> Isn't \"local\" a Bash-ism we want to keep out of the test scripts?\n> \n> Yeah. It's supported by dash and many other shells, but we do try to\n> avoid it[1]. I think in this case we could just drop it (but keep\n> setting the \"local foo\" ones to empty with \"foo=\".\n\nI do wish that we could allow \"local\", as it avoids a lot of headaches\nand potential breakage. According to [1],\n\n> However, every single POSIX-compliant shell I've tested implements the\n> 'local' keyword, which lets you declare variables that won't be\n> returned from the current function. So nowadays you can safely count\n> on it working.\n\nHe mentions that ksh93 doesn't support \"local\", but that it differs from\nPOSIX in many other ways, too.\n\nPerhaps we could slip in a couple of \"local\" as a compatibility test to\nsee if anybody complains, like we did with a couple of C features recently.\n\nBut I agree that this bug fix is not the correct occasion to change\npolicy on something like this, so I'll reroll without \"local\".\n\nMichael\n\n[1] http://apenwarr.ca/log/?m=201102#28\n"},{"id":"331000","messageId":"371b9dfd-8aaf-c539-df8a-2d0ae05208e4@alum.mit.edu","threadId":"47020","inReplyTo":"e66694a3-98e2-33aa-4cdd-dac41d2a89d5@alum.mit.edu","subject":"Re: [PATCH 1/2] t1404: add a bunch of tests of D/F conflicts","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-10-25T08:23:10Z","receivedAt":"2017-10-25T08:23:22Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 10/25/2017 10:03 AM, Michael Haggerty wrote:\n> On 10/24/2017 09:46 PM, Jeff King wrote:\n>> On Tue, Oct 24, 2017 at 12:19:26PM -0400, Eric Sunshine wrote:\n>>\n>>> On Tue, Oct 24, 2017 at 11:16 AM, Michael Haggerty wrote:\n>>>> diff --git a/t/t1404-update-ref-errors.sh b/t/t1404-update-ref-errors.sh\n>>>> @@ -34,6 +34,86 @@ test_update_rejected () {\n>>>> +# Test adding and deleting D/F-conflicting references in a single\n>>>> +# transaction.\n>>>> +df_test() {\n>>>> + local prefix=\"$1\"\n>>>> + shift\n>>>> + local pack=:\n>>>\n>>> Isn't \"local\" a Bash-ism we want to keep out of the test scripts?\n>>\n>> Yeah. It's supported by dash and many other shells, but we do try to\n>> avoid it[1]. I think in this case we could just drop it (but keep\n>> setting the \"local foo\" ones to empty with \"foo=\".\n> [...]\n> \n> But I agree that this bug fix is not the correct occasion to change\n> policy on something like this, so I'll reroll without \"local\".\n\nOh, I see Junio has already made this fix in the version that he picked\nup, so I won't bother rerolling.\n\nMichael\n"},{"id":"331068","messageId":"20171026063250.dckc22ocr3zjmsxv@sigill.intra.peff.net","threadId":"47020","inReplyTo":"e66694a3-98e2-33aa-4cdd-dac41d2a89d5@alum.mit.edu","subject":"Re: [PATCH 1/2] t1404: add a bunch of tests of D/F conflicts","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-10-26T06:32:51Z","receivedAt":"2017-10-26T06:32:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Oct 25, 2017 at 10:03:09AM +0200, Michael Haggerty wrote:\n\n> > Yeah. It's supported by dash and many other shells, but we do try to\n> > avoid it[1]. I think in this case we could just drop it (but keep\n> > setting the \"local foo\" ones to empty with \"foo=\".\n> \n> I do wish that we could allow \"local\", as it avoids a lot of headaches\n> and potential breakage. According to [1],\n\nAgreed.\n\n> > However, every single POSIX-compliant shell I've tested implements the\n> > 'local' keyword, which lets you declare variables that won't be\n> > returned from the current function. So nowadays you can safely count\n> > on it working.\n> \n> He mentions that ksh93 doesn't support \"local\", but that it differs from\n> POSIX in many other ways, too.\n\nYes, the conclusion we came to in the thread I linked earlier is the\nsame: ksh is affected, but that shell is a problem for other reasons. I\ndon't know if anybody tested with \"modern\" ksh like mksh, though. Should\nbe easy enough:\n\n cat >foo.sh <<\\EOF\n fun() {\n local x=3\n echo inside: $x\n }\n x=2\n fun\n echo after: $x\n EOF\n\n mksh foo.sh\n\nwhich produces the expected:\n\n inside: 3\n after: 2\n\nSo that's good.\n\n> Perhaps we could slip in a couple of \"local\" as a compatibility test to\n> see if anybody complains, like we did with a couple of C features recently.\n\nThat sounds reasonable to me. But from the earlier conversation, beware\nthat:\n\n local x\n ...\n x=5\n\nis not necessarily enough to notice the problem on broken shells (they\nmay complain that \"local\" is not a command, and quietly stomp on the\nglobal). I think:\n\n local x=5\n\nwould be enough (though depend on how you use $x, the failure mode might\nbe pretty subtle). Or we could even add an explicit test in t0000 like\nthe example above.\n\n> But I agree that this bug fix is not the correct occasion to change\n> policy on something like this, so I'll reroll without \"local\".\n\nAlso agreed.\n\n-Peff\n"},{"id":"331227","messageId":"20171028164249.ufro5weobwadfonv@genre.crustytoothpaste.net","threadId":"47020","inReplyTo":"20171026063250.dckc22ocr3zjmsxv@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] t1404: add a bunch of tests of D/F conflicts","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2017-10-28T16:42:49Z","receivedAt":"2017-10-28T16:43:04Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On Wed, Oct 25, 2017 at 11:32:51PM -0700, Jeff King wrote:\n> On Wed, Oct 25, 2017 at 10:03:09AM +0200, Michael Haggerty wrote:\n> \n> > > Yeah. It's supported by dash and many other shells, but we do try to\n> > > avoid it[1]. I think in this case we could just drop it (but keep\n> > > setting the \"local foo\" ones to empty with \"foo=\".\n> > \n> > I do wish that we could allow \"local\", as it avoids a lot of headaches\n> > and potential breakage. According to [1],\n> \n> Agreed.\n\nThis would be useful. Debian requires that all implementations that\nimplement /bin/sh support local and a small number of other features.\n\nThere is discussion in the Austin Group issue tracker about adding this\nfeature to POSIX, but it's gotten bogged down over lexical versus\ndynamic scoping. Everyone agrees that it's a desirable feature, though.\n\n> > He mentions that ksh93 doesn't support \"local\", but that it differs from\n> > POSIX in many other ways, too.\n> \n> Yes, the conclusion we came to in the thread I linked earlier is the\n> same: ksh is affected, but that shell is a problem for other reasons. I\n> don't know if anybody tested with \"modern\" ksh like mksh, though. Should\n> be easy enough:\n\nAs far as I can tell, bash, dash, posh, mksh, pdksh, zsh, and busybox sh\nall support local. From my reading of the documentation, so does sh on\nFreeBSD, NetBSD, and OpenBSD. Not all of these are good choices for a\nPOSIXy sh, though.\n\nksh93 will support local if you alias it to typeset, but only when\ncalled from functions defined with \"function\", not normal shell-style\nfunctions. I have a gist[0] that does absurd things to work around\nthat, but I wouldn't recommend that for production use.\n\nSolaris 11.1's man page doesn't document local in sh (which is a ksh88\nvariant) and ksh is ksh93, so it doesn't appear to support it. Solaris\n11.3 documents bash, so it's a non-issue there.\n\nIt's my understanding that using ksh as a POSIXy sh variant is very\ncommon on proprietary Unices, so its lack of compatibility may be a\ndealbreaker. Then again, many of those systems may have bash installed.\n\n> > Perhaps we could slip in a couple of \"local\" as a compatibility test to\n> > see if anybody complains, like we did with a couple of C features recently.\n> \n> That sounds reasonable to me. But from the earlier conversation, beware\n> that:\n> \n> local x\n> ...\n> x=5\n> \n> is not necessarily enough to notice the problem on broken shells (they\n> may complain that \"local\" is not a command, and quietly stomp on the\n> global). I think:\n> \n> local x=5\n> \n> would be enough (though depend on how you use $x, the failure mode might\n> be pretty subtle). Or we could even add an explicit test in t0000 like\n> the example above.\n\nI'd recommend an explicit test for this. It's much easier to track down\nthat way than seeing other failure scenarios. People will also usually\ncomplain about failing tests.\n\n[0] https://gist.github.com/bk2204/9dd24df0e4499a02a300578ebdca4728\n-- \nbrian m. carlson / brian with sandals: Houston, Texas, US\nhttps://www.crustytoothpaste.net/~bmc | My opinion only\nOpenPGP: https://keybase.io/bk2204\n"},{"id":"331248","messageId":"xmqqk1zebyxz.fsf@gitster.mtv.corp.google.com","threadId":"47020","inReplyTo":"20171028164249.ufro5weobwadfonv@genre.crustytoothpaste.net","subject":"Re: [PATCH 1/2] t1404: add a bunch of tests of D/F conflicts","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-10-29T00:05:44Z","receivedAt":"2017-10-29T00:05:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"brian m. carlson\" writes:\n\n> There is discussion in the Austin Group issue tracker about adding this\n> feature to POSIX, but it's gotten bogged down over lexical versus\n> dynamic scoping. Everyone agrees that it's a desirable feature, though.\n> ...\n\nIn short, unless you are a binary packager on a platform whose\nnative shell is ksh and who refuses to depend on tools that are not\ndefault/native on the platform, you'd be OK?\n\n> I'd recommend an explicit test for this. It's much easier to track down\n> that way than seeing other failure scenarios. People will also usually\n> complain about failing tests.\n\nHopefully. \n\nStarting from an explicit test, gradually using more \"local\" in\ntests that cover more important parts of the system, and then start\nusing \"local\" as appropriate in the main tools would be a good way\nforward. \n\nBy that time, we might have a lot less scripted Porcelains, though\n;-)\n"},{"id":"331273","messageId":"20171029161206.soieyxfiru2fvi7j@genre.crustytoothpaste.net","threadId":"47020","inReplyTo":"xmqqk1zebyxz.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 1/2] t1404: add a bunch of tests of D/F conflicts","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2017-10-29T16:12:07Z","receivedAt":"2017-10-29T16:12:20Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On Sun, Oct 29, 2017 at 09:05:44AM +0900, Junio C Hamano wrote:\n> In short, unless you are a binary packager on a platform whose\n> native shell is ksh and who refuses to depend on tools that are not\n> default/native on the platform, you'd be OK?\n\nYes.\n\n> > I'd recommend an explicit test for this. It's much easier to track down\n> > that way than seeing other failure scenarios. People will also usually\n> > complain about failing tests.\n> \n> Hopefully.\n> \n> Starting from an explicit test, gradually using more \"local\" in\n> tests that cover more important parts of the system, and then start\n> using \"local\" as appropriate in the main tools would be a good way\n> forward.\n\nI completely agree. I just wanted to ensure that if we failed, it was\nat least obvious to packagers who ran the tests. I'm not sure there's\nanything we can do if people don't run them.\n\nI do think people may run the tests more frequently than you think,\nthough. I always do in my packaging at $DAYJOB, but any failures\n(usually missing SANITY) tend to already be patched by the time I get\noff work and write a patch.\n-- \nbrian m. carlson / brian with sandals: Houston, Texas, US\nhttps://www.crustytoothpaste.net/~bmc | My opinion only\nOpenPGP: https://keybase.io/bk2204\n"}]}