{"thread":{"id":"56312","subject":"[PATCH] fixup! propagate errno from failing read","startedAt":"2021-08-17T12:31:36Z","lastAt":"2021-08-19T20:26:36Z","messageCount":14,"participants":["Han-Wen Nienhuys via GitGitGadget","Jonathan Tan","Junio C Hamano","Han-Wen Nienhuys","Jonathan Nieder","Jeff King","Carlo Marcelo Arenas Belón","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"432946","messageId":"pull.1068.git.git.1629203489546.gitgitgadget@gmail.com","threadId":"56312","inReplyTo":null,"subject":"[PATCH] fixup! propagate errno from failing read","fromName":"Han-Wen Nienhuys via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-08-17T12:31:29Z","receivedAt":"2021-08-17T12:31:36Z","isPatch":true,"sender":{"key":"hanwen@google.com","avatar":"https://avatars.githubusercontent.com/u/31547?v=4"},"body":"From: Han-Wen Nienhuys \n\nThis fixes a crash triggered by the BUG() statement.\n\nThis can occur with symlinked .git/refs. To check availability,\nrefs_verify_refname_available will run refs_read_raw_ref() on each prefix,\nleading to a read() from .git/refs (which is a directory).\n\nWhen handling the symlink case, it is probably more robust to re-issue the\nlstat() as a normal stat(), in which case, we would fall back to the directory\ncase.\n\nFor now, propagating errno from strbuf_read() is simpler and avoids the crash.\n\nSigned-off-by: Han-Wen Nienhuys \n---\n fixup! propagate errno from failing read\n \n This fixes a crash triggered by the BUG() statement.\n \n This can occur with symlinked .git/refs. To check availability,\n refs_verify_refname_available will run refs_read_raw_ref() on each\n prefix, leading to a read() from .git/refs (which is a directory).\n \n When handling the symlink case, it is probably more robust to re-issue\n the lstat() as a normal stat(), in which case, we would fall back to the\n directory case.\n \n For now, propagating errno from strbuf_read() is simpler and avoids the\n crash.\n \n Signed-off-by: Han-Wen Nienhuys hanwen@google.com\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1068%2Fhanwen%2Ffiles-fixup-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1068/hanwen/files-fixup-v1\nPull-Request: https://github.com/git/git/pull/1068\n\n refs/files-backend.c | 1 +\n t/t3200-branch.sh | 14 ++++++++++++++\n 2 files changed, 15 insertions(+)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 0d96eeba61b..f546cc3cc3d 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -454,6 +454,7 @@ stat_ref:\n \t}\n \tstrbuf_reset(&sb_contents);\n \tif (strbuf_read(&sb_contents, fd, 256) < 0) {\n+\t\tmyerr = errno;\n \t\tclose(fd);\n \t\tgoto out;\n \t}\ndiff --git a/t/t3200-branch.sh b/t/t3200-branch.sh\nindex cc4b10236e2..dd17718160a 100755\n--- a/t/t3200-branch.sh\n+++ b/t/t3200-branch.sh\n@@ -109,6 +109,20 @@ test_expect_success 'git branch -m n/n n should work' '\n \tgit reflog exists refs/heads/n\n '\n \n+test_expect_success 'git branch -m with symlinked .git/refs' '\n+\tgit init subdir &&\n+\ttest_when_finished \"rm -rf subdir\" &&\n+\t(cd subdir &&\n+\t\tfor d in refs objects packed-refs ; do\n+\t\trm -rf .git/$d &&\n+\t\tln -s ../../.git/$d .git/$d ; done ) &&\n+\tgit --git-dir subdir/.git/ branch rename-src &&\n+\texpect=$(git rev-parse rename-src) &&\n+\tgit --git-dir subdir/.git/ branch -m rename-src rename-dest &&\n+\ttest $(git rev-parse rename-dest) = \"$expect\" &&\n+\tgit branch -D rename-dest\n+'\n+\n # The topmost entry in reflog for branch bbb is about branch creation.\n # Hence, we compare bbb@{1} (instead of bbb@{0}) with aaa@{0}.\n \n\nbase-commit: f000ecbed922c727382651490e75014f003c89ca\n-- \ngitgitgadget\n"},{"id":"432990","messageId":"20210817161419.3606302-1-jonathantanmy@google.com","threadId":"56312","inReplyTo":"pull.1068.git.git.1629203489546.gitgitgadget@gmail.com","subject":"Re: [PATCH] fixup! propagate errno from failing read","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2021-08-17T16:14:19Z","receivedAt":"2021-08-17T16:14:35Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> diff --git a/refs/files-backend.c b/refs/files-backend.c\n> index 0d96eeba61b..f546cc3cc3d 100644\n> --- a/refs/files-backend.c\n> +++ b/refs/files-backend.c\n> @@ -454,6 +454,7 @@ stat_ref:\n> \t}\n> \tstrbuf_reset(&sb_contents);\n> \tif (strbuf_read(&sb_contents, fd, 256) < 0) {\n> +\t\tmyerr = errno;\n> \t\tclose(fd);\n> \t\tgoto out;\n> \t}\n\nReviewed-by: Jonathan Tan \n\nThanks - a straightforward fixup. (I don't think we need the errno from\nclose() in this case.)\n"},{"id":"433037","messageId":"xmqq4kbn7l3l.fsf@gitster.g","threadId":"56312","inReplyTo":"pull.1068.git.git.1629203489546.gitgitgadget@gmail.com","subject":"Re: [PATCH] fixup! propagate errno from failing read","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-08-17T22:38:38Z","receivedAt":"2021-08-17T22:38:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Han-Wen Nienhuys via GitGitGadget\" writes:\n\n> From: Han-Wen Nienhuys \n>\n> This fixes a crash triggered by the BUG() statement.\n>\n> This can occur with symlinked .git/refs. To check availability,\n> refs_verify_refname_available will run refs_read_raw_ref() on each prefix,\n> leading to a read() from .git/refs (which is a directory).\n>\n> When handling the symlink case, it is probably more robust to re-issue the\n> lstat() as a normal stat(), in which case, we would fall back to the directory\n> case.\n>\n> For now, propagating errno from strbuf_read() is simpler and avoids the crash.\n>\n> Signed-off-by: Han-Wen Nienhuys \n> ---\n> fixup! propagate errno from failing read\n\nHmph, I do not see a commit with \"propagate errno from failing read\"\nin its title anywhere in 'seen'.\n\nI think the convention to assign errno to myerr in this codepath\noriginates in a0731250 (refs: explicitly return failure_errno from\nparse_loose_ref_contents, 2021-07-20), and it forgot the part of the\ncode being fixed with this patch. The commit being fixed is already\nis in 'next' as part of the hn/refs-errno-cleanup topic.\n\nUsually, a flaw in a topic that is already in 'next' is corrected by\na follow-up patch, but then they won't say \"fixup!\" (none of our\nbugfix patches do). But a post-release is a special time, as we\nwill soon be rewinding 'next', restarting it from the latest release\nand we have a choice to tentatively eject a topic, fix it up or\neven replace it, before merging the corrected topic to 'next'.\n\nDo you mean that you want me to squash this change into that commit\nbefore the topic graduates to 'master' during the new development\ncycle? If so please be a bit more explicit next time. Using the\ntitle of the commit after \"fixup!\" would be a good starting point.\n\nThanks.\n\n"},{"id":"433050","messageId":"CAFQ2z_Ni1bvj0Skgp_3p9htQfjn_M=3uF06pyZm_hkXgT_L61g@mail.gmail.com","threadId":"56312","inReplyTo":"xmqq4kbn7l3l.fsf@gitster.g","subject":"Re: [PATCH] fixup! propagate errno from failing read","fromName":"Han-Wen Nienhuys","fromEmail":"hanwen@google.com","sentAt":"2021-08-18T09:00:36Z","receivedAt":"2021-08-18T09:00:51Z","isPatch":true,"sender":{"key":"hanwen@google.com","avatar":"https://avatars.githubusercontent.com/u/31547?v=4"},"body":"On Wed, Aug 18, 2021 at 12:38 AM Junio C Hamano wrote:\n>\n> \"Han-Wen Nienhuys via GitGitGadget\" writes:\n>\n> > From: Han-Wen Nienhuys \n> >\n> > This fixes a crash triggered by the BUG() statement.\n> >\n> > This can occur with symlinked .git/refs. To check availability,\n> > refs_verify_refname_available will run refs_read_raw_ref() on each prefix,\n> > leading to a read() from .git/refs (which is a directory).\n> >\n> > When handling the symlink case, it is probably more robust to re-issue the\n> > lstat() as a normal stat(), in which case, we would fall back to the directory\n> > case.\n> >\n> > For now, propagating errno from strbuf_read() is simpler and avoids the crash.\n> >\n> > Signed-off-by: Han-Wen Nienhuys \n> > ---\n> > fixup! propagate errno from failing read\n>\n> Hmph, I do not see a commit with \"propagate errno from failing read\"\n> in its title anywhere in 'seen'.\n\n> I think the convention to assign errno to myerr in this codepath\n> originates in a0731250 (refs: explicitly return failure_errno from\n> parse_loose_ref_contents, 2021-07-20), and it forgot the part of the\n> code being fixed with this patch. The commit being fixed is already\n> is in 'next' as part of the hn/refs-errno-cleanup topic.\n>\n> Usually, a flaw in a topic that is already in 'next' is corrected by\n> a follow-up patch, but then they won't say \"fixup!\" (none of our\n> bugfix patches do). But a post-release is a special time, as we\n> will soon be rewinding 'next', restarting it from the latest release\n> and we have a choice to tentatively eject a topic, fix it up or\n> even replace it, before merging the corrected topic to 'next'.\n>\n> Do you mean that you want me to squash this change into that commit\n> before the topic graduates to 'master' during the new development\n> cycle? If so please be a bit more explicit next time. Using the\n> title of the commit after \"fixup!\" would be a good starting point.\n\nThe problem fixed here affects anyone who uses git-repo (ie. does\nAndroid development) and runs \"git-branch -m\", which is a large group\nof people, so I think it should not be allowed to get into a release.\n\nSo it could be squashed into commit a0731250, or put on top of next as\na separate commit (probably with 'fixup!' removed).\n\nNote that, even though commit a0731250 originates from a branch called\n\"hn/XXX\" and has me as Author, the BUG() call causing the crash was\nactually introduced by AEvar when he reworked the series.\n\n-- \nHan-Wen Nienhuys - Google Munich\nI work 80%. Don't expect answers from me on Fridays.\n--\n\nGoogle Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich\n\nRegistergericht und -nummer: Hamburg, HRB 86891\n\nSitz der Gesellschaft: Hamburg\n\nGeschäftsführer: Paul Manicle, Halimah DeLaine Prado\n"},{"id":"433076","messageId":"xmqqo89u61ww.fsf@gitster.g","threadId":"56312","inReplyTo":"CAFQ2z_Ni1bvj0Skgp_3p9htQfjn_M=3uF06pyZm_hkXgT_L61g@mail.gmail.com","subject":"Re: [PATCH] fixup! propagate errno from failing read","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-08-18T18:30:39Z","receivedAt":"2021-08-18T18:30:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Han-Wen Nienhuys writes:\n\n>> I think the convention to assign errno to myerr in this codepath\n>> originates in a0731250 (refs: explicitly return failure_errno from\n>> parse_loose_ref_contents, 2021-07-20), and it forgot the part of the\n>> code being fixed with this patch. The commit being fixed is already\n>> is in 'next' as part of the hn/refs-errno-cleanup topic.\n>>\n>> Usually, a flaw in a topic that is already in 'next' is corrected by\n>> a follow-up patch, but then they won't say \"fixup!\" (none of our\n>> bugfix patches do). But a post-release is a special time, as we\n>> will soon be rewinding 'next', restarting it from the latest release\n>> and we have a choice to tentatively eject a topic, fix it up or\n>> even replace it, before merging the corrected topic to 'next'.\n>>\n>> Do you mean that you want me to squash this change into that commit\n>> before the topic graduates to 'master' during the new development\n>> cycle? If so please be a bit more explicit next time. Using the\n>> title of the commit after \"fixup!\" would be a good starting point.\n>\n> The problem fixed here affects anyone who uses git-repo (ie. does\n> Android development) and runs \"git-branch -m\", which is a large group\n> of people, so I think it should not be allowed to get into a release.\n\nOK. The problem already is in 'next' and we want to make sure it\nwon't graduate to 'master' for the next release as-is. I agree with\nthat ;-)\n\n> So it could be squashed into commit a0731250, or put on top of next as\n> a separate commit (probably with 'fixup!' removed).\n\nI'd try the former first and will fall back on the latter, then.\n\n> Note that, even though commit a0731250 originates from a branch called\n> \"hn/XXX\" and has me as Author, the BUG() call causing the crash was\n> actually introduced by AEvar when he reworked the series.\n\nYup, I see his Sob after yours and it is quite understandable if a\nnew bug was introduced by his changes. It also would be\nunderstandable if his change was only to add a call to BUG() in\norder to assert that the original patch used myerr consistently, and\nit uncovered a bug in the original version he took from you.\n\nI do not care too much about how exactly the bug was introduced and\nuncovered---it matters more that the end result has one fewer bug\nthanks to the team effort.\n"},{"id":"433090","messageId":"YR2HVU4KBCihqoYZ@google.com","threadId":"56312","inReplyTo":"20210817161419.3606302-1-jonathantanmy@google.com","subject":"Re: [PATCH] fixup! propagate errno from failing read","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2021-08-18T22:19:01Z","receivedAt":"2021-08-18T22:19:07Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nJonathan Tan wrote:\n> Han-Wen Nienhuys wrote:\n\n>> diff --git a/refs/files-backend.c b/refs/files-backend.c\n>> index 0d96eeba61b..f546cc3cc3d 100644\n>> --- a/refs/files-backend.c\n>> +++ b/refs/files-backend.c\n>> @@ -454,6 +454,7 @@ stat_ref:\n>> \t}\n>> \tstrbuf_reset(&sb_contents);\n>> \tif (strbuf_read(&sb_contents, fd, 256) < 0) {\n>> +\t\tmyerr = errno;\n>> \t\tclose(fd);\n>> \t\tgoto out;\n>> \t}\n>\n> Reviewed-by: Jonathan Tan \n>\n> Thanks - a straightforward fixup. (I don't think we need the errno from\n> close() in this case.)\n\nThis looks good as far as it goes, but how do we know this has covered\nall the code paths? Let's see.\n\nThe only nonobvious paths are\n\n stat_ref:\n\t/*\n\t * We might have to loop back here to avoid a race\n\t * condition: first we lstat() the file, then we try\n\t * to read it as a link or as a file. But if somebody\n\t * changes the type of the file (file <-> directory\n\t * <-> symlink) between the lstat() and reading, then\n\t * we don't want to report that as an error but rather\n\t * try again starting with the lstat().\n\t *\n\t * We'll keep a count of the retries, though, just to avoid\n\t * any confusing situation sending us into an infinite loop.\n\t */\n\n\tif (remaining_retries-- <= 0)\n\t\tgoto out;\n\nand\n\n\tret = parse_loose_ref_contents(buf, oid, referent, type, &myerr);\n\n out:\n\tif (ret && !myerr)\n\t\tBUG(\"returning non-zero %d, should have set myerr!\", ret);\n\nFor the 'stat_ref' case, we have to check that whenever we 'goto\nstat_ref', we set myerr moments before. Fortunately, that is the\ncase.\n\nFor the 'fall through into out' case, we have the check the\nparse_loose_ref_contents always sets *failure_errno on error. That is\nalso the case.\n\nSo this indeed covers all our cases, and the BUG now correctly\nreflects an invariant we can count on. Thanks for the fix, and thanks\nfor looking it over.\n\nSincerely,\nJonathan\n"},{"id":"433095","messageId":"xmqq7dgi47k7.fsf@gitster.g","threadId":"56312","inReplyTo":"xmqqo89u61ww.fsf@gitster.g","subject":"Re: [PATCH] fixup! propagate errno from failing read","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-08-19T00:11:36Z","receivedAt":"2021-08-19T00:11:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n>> So it could be squashed into commit a0731250, or put on top of next as\n>> a separate commit (probably with 'fixup!' removed).\n>\n> I'd try the former first and will fall back on the latter, then.\n\nI've reverted a0731250 out of 'next', squashed the fix in, rebuilt\nthe topic and merged it back to 'next'. Thanks.\n"},{"id":"433107","messageId":"YR3WS1AnqCuiJWyX@coredump.intra.peff.net","threadId":"56312","inReplyTo":"pull.1068.git.git.1629203489546.gitgitgadget@gmail.com","subject":"Re: [PATCH] fixup! propagate errno from failing read","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-08-19T03:55:55Z","receivedAt":"2021-08-19T03:55:57Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Aug 17, 2021 at 12:31:29PM +0000, Han-Wen Nienhuys via GitGitGadget wrote:\n\n> diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh\n> index cc4b10236e2..dd17718160a 100755\n> --- a/t/t3200-branch.sh\n> +++ b/t/t3200-branch.sh\n> @@ -109,6 +109,20 @@ test_expect_success 'git branch -m n/n n should work' '\n> \tgit reflog exists refs/heads/n\n> '\n> \n> +test_expect_success 'git branch -m with symlinked .git/refs' '\n> +\tgit init subdir &&\n> +\ttest_when_finished \"rm -rf subdir\" &&\n> +\t(cd subdir &&\n> +\t\tfor d in refs objects packed-refs ; do\n> +\t\trm -rf .git/$d &&\n> +\t\tln -s ../../.git/$d .git/$d ; done ) &&\n> +\tgit --git-dir subdir/.git/ branch rename-src &&\n> +\texpect=$(git rev-parse rename-src) &&\n> +\tgit --git-dir subdir/.git/ branch -m rename-src rename-dest &&\n> +\ttest $(git rev-parse rename-dest) = \"$expect\" &&\n> +\tgit branch -D rename-dest\n> +'\n\nThis test presumably needs the SYMLINKS prerequisite. I noticed that the\nWindows CI for \"next\" is now failing.\n\n-Peff\n"},{"id":"433108","messageId":"20210819050103.25995-1-carenas@gmail.com","threadId":"56312","inReplyTo":"YR3WS1AnqCuiJWyX@coredump.intra.peff.net","subject":"[PATCH] t3200: refactor symlink test","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2021-08-19T05:01:03Z","receivedAt":"2021-08-19T05:01:56Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"d1931bcf0d (refs: make errno output explicit for refs_resolve_ref_unsafe,\n2021-07-20) add a test for a crash when refs is a symlink, but it fails\non windows.\n\nadd the missing SYMLINKS dependency and while at it, refactor it slightly\nto comply better with the CodingGuidelines.\n\nReported-by: Jeff King \nSigned-off-by: Carlo Marcelo Arenas Belón \n---\n t/t3200-branch.sh | 19 ++++++++++++-------\n 1 file changed, 12 insertions(+), 7 deletions(-)\n\ndiff --git a/t/t3200-branch.sh b/t/t3200-branch.sh\nindex dd17718160..6d8700664e 100755\n--- a/t/t3200-branch.sh\n+++ b/t/t3200-branch.sh\n@@ -109,17 +109,22 @@ test_expect_success 'git branch -m n/n n should work' '\n \tgit reflog exists refs/heads/n\n '\n \n-test_expect_success 'git branch -m with symlinked .git/refs' '\n+test_expect_success SYMLINKS 'git branch -m with symlinked .git/refs' '\n \tgit init subdir &&\n \ttest_when_finished \"rm -rf subdir\" &&\n-\t(cd subdir &&\n-\t\tfor d in refs objects packed-refs ; do\n-\t\trm -rf .git/$d &&\n-\t\tln -s ../../.git/$d .git/$d ; done ) &&\n+\t(\n+\t\tcd subdir &&\n+\t\tfor d in refs objects packed-refs\n+\t\tdo\n+\t\t\trm -rf .git/$d &&\n+\t\t\tln -s ../../.git/$d .git/$d\n+\t\tdone\n+\t) &&\n \tgit --git-dir subdir/.git/ branch rename-src &&\n-\texpect=$(git rev-parse rename-src) &&\n+\tgit rev-parse rename-src >expect &&\n \tgit --git-dir subdir/.git/ branch -m rename-src rename-dest &&\n-\ttest $(git rev-parse rename-dest) = \"$expect\" &&\n+\tgit rev-parse rename-dest >actual &&\n+\ttest_cmp expect actual &&\n \tgit branch -D rename-dest\n '\n \n\nbase-commit: d1931bcf0d5ef75cdaf836347f4aefce902a6a38\n-- \n2.33.0.476.gf000ecbed9\n\n"},{"id":"433111","messageId":"CAPig+cRjMYzw+Hu1wy20w8oJ-4jRmOwMB9Rt=QmNu+=8Mj7skQ@mail.gmail.com","threadId":"56312","inReplyTo":"20210819050103.25995-1-carenas@gmail.com","subject":"Re: [PATCH] t3200: refactor symlink test","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2021-08-19T05:51:58Z","receivedAt":"2021-08-19T05:52:12Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Aug 19, 2021 at 1:02 AM Carlo Marcelo Arenas Belón\n wrote:\n> d1931bcf0d (refs: make errno output explicit for refs_resolve_ref_unsafe,\n> 2021-07-20) add a test for a crash when refs is a symlink, but it fails\n> on windows.\n>\n> add the missing SYMLINKS dependency and while at it, refactor it slightly\n> to comply better with the CodingGuidelines.\n>\n> Signed-off-by: Carlo Marcelo Arenas Belón \n> ---\n> diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh\n> @@ -109,17 +109,22 @@ test_expect_success 'git branch -m n/n n should work' '\n> -test_expect_success 'git branch -m with symlinked .git/refs' '\n> +test_expect_success SYMLINKS 'git branch -m with symlinked .git/refs' '\n> git init subdir &&\n> test_when_finished \"rm -rf subdir\" &&\n> - (cd subdir &&\n> - for d in refs objects packed-refs ; do\n> - rm -rf .git/$d &&\n> - ln -s ../../.git/$d .git/$d ; done ) &&\n> + (\n> + cd subdir &&\n> + for d in refs objects packed-refs\n> + do\n> + rm -rf .git/$d &&\n> + ln -s ../../.git/$d .git/$d\n\nIdeally, the above line should be:\n\n ln -s ../../.git/$d .git/$d || exit 1\n\nto catch and signal a failure within the for-loop body (which happens\nto be in a subshell; if not in a subshell, you'd use `|| return 1`).\n\n> + done\n> + ) &&\n> git --git-dir subdir/.git/ branch rename-src &&\n> - expect=$(git rev-parse rename-src) &&\n> + git rev-parse rename-src >expect &&\n> git --git-dir subdir/.git/ branch -m rename-src rename-dest &&\n> - test $(git rev-parse rename-dest) = \"$expect\" &&\n> + git rev-parse rename-dest >actual &&\n> + test_cmp expect actual &&\n> git branch -D rename-dest\n> '\n"},{"id":"433113","messageId":"20210819075244.36776-1-carenas@gmail.com","threadId":"56312","inReplyTo":"20210819050103.25995-1-carenas@gmail.com","subject":"[PATCH v2] t3200: refactor symlink test from hn/refs-errno-cleanup","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2021-08-19T07:52:44Z","receivedAt":"2021-08-19T07:53:07Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"d1931bcf0d (refs: make errno output explicit for refs_resolve_ref_unsafe,\n2021-07-20) add a test for a crash when refs is a symlink, but it fails\non windows.\n\nadd the missing SYMLINKS dependency and while at it, refactor it slightly\nto comply better with the CodingGuidelines.\n\nReported-by: Jeff King \nHelped-by: Eric Sunshine \nSigned-off-by: Carlo Marcelo Arenas Belón \n---\nv2:\n* update subject for clarity; might be worth squashing instead into 3d63ce75e\n (refs: explicitly return failure_errno from parse_loose_ref_contents,\n 2021-07-20)\n* change from --git-dir to -C for clarity\n* add reporting for errors to the for loop as suggested by Eric\n\n t/t3200-branch.sh | 23 ++++++++++++++---------\n 1 file changed, 14 insertions(+), 9 deletions(-)\n\ndiff --git a/t/t3200-branch.sh b/t/t3200-branch.sh\nindex dd17718160..eacb6bcd35 100755\n--- a/t/t3200-branch.sh\n+++ b/t/t3200-branch.sh\n@@ -109,17 +109,22 @@ test_expect_success 'git branch -m n/n n should work' '\n \tgit reflog exists refs/heads/n\n '\n \n-test_expect_success 'git branch -m with symlinked .git/refs' '\n+test_expect_success SYMLINKS 'git branch -m with symlinked .git/refs' '\n \tgit init subdir &&\n \ttest_when_finished \"rm -rf subdir\" &&\n-\t(cd subdir &&\n-\t\tfor d in refs objects packed-refs ; do\n-\t\trm -rf .git/$d &&\n-\t\tln -s ../../.git/$d .git/$d ; done ) &&\n-\tgit --git-dir subdir/.git/ branch rename-src &&\n-\texpect=$(git rev-parse rename-src) &&\n-\tgit --git-dir subdir/.git/ branch -m rename-src rename-dest &&\n-\ttest $(git rev-parse rename-dest) = \"$expect\" &&\n+\t(\n+\t\tcd subdir &&\n+\t\tfor d in refs objects packed-refs\n+\t\tdo\n+\t\t\trm -rf .git/$d &&\n+\t\t\tln -s ../../.git/$d .git/$d || exit 1\n+\t\tdone\n+\t) &&\n+\tgit -C subdir branch rename-src &&\n+\tgit rev-parse rename-src >expect &&\n+\tgit -C subdir branch -m rename-src rename-dest &&\n+\tgit rev-parse rename-dest >actual &&\n+\ttest_cmp expect actual &&\n \tgit branch -D rename-dest\n '\n \n\nbase-commit: d1931bcf0d5ef75cdaf836347f4aefce902a6a38\n-- \n2.33.0.476.gf000ecbed9\n\n"},{"id":"433119","messageId":"CAFQ2z_MBQZOgMr8NC6Ju-8H0XZXe+Lr=ueH_r46C5QMmUfDneg@mail.gmail.com","threadId":"56312","inReplyTo":"xmqqo89u61ww.fsf@gitster.g","subject":"Re: [PATCH] fixup! propagate errno from failing read","fromName":"Han-Wen Nienhuys","fromEmail":"hanwen@google.com","sentAt":"2021-08-19T09:01:42Z","receivedAt":"2021-08-19T09:01:57Z","isPatch":true,"sender":{"key":"hanwen@google.com","avatar":"https://avatars.githubusercontent.com/u/31547?v=4"},"body":"On Wed, Aug 18, 2021 at 8:30 PM Junio C Hamano wrote:\n\n> I do not care too much about how exactly the bug was introduced and\n> uncovered---it matters more that the end result has one fewer bug\n> thanks to the team effort.\n\nagreed. Just wanted to give context on my bungled \"fixup!\" subject.\n\n-- \nHan-Wen Nienhuys - Google Munich\nI work 80%. Don't expect answers from me on Fridays.\n--\nGoogle Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich\nRegistergericht und -nummer: Hamburg, HRB 86891\nSitz der Gesellschaft: Hamburg\nGeschäftsführer: Paul Manicle, Halimah DeLaine Prado\n"},{"id":"433167","messageId":"xmqqeeapyye7.fsf@gitster.g","threadId":"56312","inReplyTo":"20210819075244.36776-1-carenas@gmail.com","subject":"Re: [PATCH v2] t3200: refactor symlink test from hn/refs-errno-cleanup","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-08-19T20:26:08Z","receivedAt":"2021-08-19T20:26:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Carlo Marcelo Arenas Belón writes:\n\n> d1931bcf0d (refs: make errno output explicit for refs_resolve_ref_unsafe,\n> 2021-07-20) add a test for a crash when refs is a symlink, but it fails\n> on windows.\n>\n> add the missing SYMLINKS dependency and while at it, refactor it slightly\n> to comply better with the CodingGuidelines.\n>\n> Reported-by: Jeff King \n> Helped-by: Eric Sunshine \n> Signed-off-by: Carlo Marcelo Arenas Belón \n> ---\n> v2:\n> * update subject for clarity; might be worth squashing instead into 3d63ce75e\n> (refs: explicitly return failure_errno from parse_loose_ref_contents,\n> 2021-07-20)\n> * change from --git-dir to -C for clarity\n> * add reporting for errors to the for loop as suggested by Eric\n\nThanks for a fix-up.\n\nLet's eject hn/refs-errno-cleanup and possibly ab/refs-files-cleanup\ntopics out of 'next' and give these 18 patches a chance for a fresh\nstart, as I've already failed a short-cut attempt to squash in the\n\"fix\" yesterday that forgot the SYMLINKS prerequisite and even\ntoday's attempts to fix it seem to be going back and forth X-<.\nEven worse, there is a t0031 breakage that seems to have come from\nthe latter topic.\n\nIOW, I won't be picking this up but asking Han-Wen and Ævar to come\nup with a clean reroll of the two series.\n\nThanks.\n"},{"id":"433168","messageId":"xmqqczq9yydi.fsf@gitster.g","threadId":"56312","inReplyTo":"20210819075244.36776-1-carenas@gmail.com","subject":"Re: [PATCH v2] t3200: refactor symlink test from hn/refs-errno-cleanup","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-08-19T20:26:33Z","receivedAt":"2021-08-19T20:26:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Carlo Marcelo Arenas Belón writes:\n\n> d1931bcf0d (refs: make errno output explicit for refs_resolve_ref_unsafe,\n> 2021-07-20) add a test for a crash when refs is a symlink, but it fails\n> on windows.\n>\n> add the missing SYMLINKS dependency and while at it, refactor it slightly\n> to comply better with the CodingGuidelines.\n>\n> Reported-by: Jeff King \n> Helped-by: Eric Sunshine \n> Signed-off-by: Carlo Marcelo Arenas Belón \n> ---\n> v2:\n> * update subject for clarity; might be worth squashing instead into 3d63ce75e\n> (refs: explicitly return failure_errno from parse_loose_ref_contents,\n> 2021-07-20)\n> * change from --git-dir to -C for clarity\n> * add reporting for errors to the for loop as suggested by Eric\n\nThanks for a fix-up.\n\nLet's eject hn/refs-errno-cleanup and possibly ab/refs-files-cleanup\ntopics out of 'next' and give these 18 patches a chance for a fresh\nstart, as I've already failed a short-cut attempt to squash in the\n\"fix\" yesterday that forgot the SYMLINKS prerequisite and even\ntoday's attempts to fix it seem to be going back and forth X-<.\nEven worse, there is a t0031 breakage that seems to have come from\nthe latter topic.\n\nIOW, I won't be picking this up but asking Han-Wen and Ævar to come\nup with a clean reroll of the two series, using this as an input\n(perhaps Helped-by: etc. would be needed).\n\nThanks.\n"}]}