{"thread":{"id":"52985","subject":"Regression in v2.26.0-rc0 and Magit","startedAt":"2020-03-12T23:11:33Z","lastAt":"2020-03-15T16:37:23Z","messageCount":9,"participants":["Jean-Noël AVILA","Jonathan Nieder","Junio C Hamano","Kyle Meyer","SZEDER Gábor"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"393184","messageId":"3091652.KAqcNXvZJ4@cayenne","threadId":"52985","inReplyTo":null,"subject":"Regression in v2.26.0-rc0 and Magit","fromName":"Jean-Noël AVILA","fromEmail":"jn.avila@free.fr","sentAt":"2020-03-12T22:55:57Z","receivedAt":"2020-03-12T23:11:33Z","isPatch":false,"sender":{"key":"jn.avila@free.fr","avatar":"https://avatars.githubusercontent.com/u/156172?v=4"},"body":"Hi all, \n\nWhen trying the latest rc with magit, I found that git segfaults under Magit with auto-revert enabled. The message in emacs is\n\nError in post-command-hook (magit-auto-revert-mode-check-buffers): (wrong-type-argument number-or-marker-p \"Segmentation Fault\")\n\nI was able to bisect the issue to commit e0020b2f82910f50bc697d86aff70c3796fbdc41 but unfortunately, it seems difficult to print the exact command from magit.\n\nReverting this patch solves the issue.\n\nMost probably emacs runs commands with not all env variables set.\n\nJN\n\n\n"},{"id":"393186","messageId":"20200312233504.GH120942@google.com","threadId":"52985","inReplyTo":"3091652.KAqcNXvZJ4@cayenne","subject":"Re: Regression in v2.26.0-rc0 and Magit","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2020-03-12T23:35:04Z","receivedAt":"2020-03-12T23:35:09Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nJean-Noël AVILA wrote:\n\n> When trying the latest rc with magit, I found that git segfaults\n> under Magit with auto-revert enabled. The message in emacs is\n>\n> Error in post-command-hook (magit-auto-revert-mode-check-buffers):\n> (wrong-type-argument number-or-marker-p \"Segmentation Fault\")\n>\n> I was able to bisect the issue to commit\n> e0020b2f82910f50bc697d86aff70c3796fbdc41 but unfortunately, it seems\n> difficult to print the exact command from magit.\n\nThanks for reporting. This is fixed by\n\n commit e6c57b49eb63e77ccf72215229744c4beaf04204 (es/outside-repo-errmsg-hints)\n Author: Emily Shaffer \n Date: Mon Mar 2 20:05:06 2020 -0800\n\n prefix_path: show gitdir if worktree unavailable\n\nJunio, can you fast-track that fix to \"master\"? Emily, can you add a\ntest?\n\nThanks,\nJonathan\n"},{"id":"393187","messageId":"xmqqk13pdsw1.fsf@gitster.c.googlers.com","threadId":"52985","inReplyTo":"20200312233504.GH120942@google.com","subject":"Re: Regression in v2.26.0-rc0 and Magit","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-03-13T00:02:06Z","receivedAt":"2020-03-13T00:02:13Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> Junio, can you fast-track that fix to \"master\"? Emily, can you add a\n> test?\n\nThanks, indeed it has been waiting for tests. We have a few more\nbusiness days before -rc2, so...\n\n* es/outside-repo-errmsg-hints (2020-03-03) 1 commit\n - prefix_path: show gitdir if worktree unavailable\n\n An earlier update to show the location of working tree in the error\n message did not consider the possibility that a git command may be\n run in a bare repository, which has been corrected.\n\n May want a test or two.\n\n\n"},{"id":"393224","messageId":"xmqq36accdpt.fsf@gitster.c.googlers.com","threadId":"52985","inReplyTo":"xmqqk13pdsw1.fsf@gitster.c.googlers.com","subject":"Re: Regression in v2.26.0-rc0 and Magit","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-03-13T18:27:26Z","receivedAt":"2020-03-13T18:27:36Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Jonathan Nieder writes:\n>\n>> Junio, can you fast-track that fix to \"master\"? Emily, can you add a\n>> test?\n>\n> Thanks, indeed it has been waiting for tests. We have a few more\n> business days before -rc2, so...\n>\n> * es/outside-repo-errmsg-hints (2020-03-03) 1 commit\n> - prefix_path: show gitdir if worktree unavailable\n>\n> An earlier update to show the location of working tree in the error\n> message did not consider the possibility that a git command may be\n> run in a bare repository, which has been corrected.\n>\n> May want a test or two.\n\nIf nobody complains in the coming 4 hours or so, I'll squash this in\nto e6c57b49 (\"prefix_path: show gitdir if worktree unavailable\",\n2020-03-02) and mark the topic as \"ready for 'next'\".\n\nThanks.\n\n t/t6136-pathspec-in-bare.sh | 30 ++++++++++++++++++++++++++++++\n 1 file changed, 30 insertions(+)\n\ndiff --git a/t/t6136-pathspec-in-bare.sh b/t/t6136-pathspec-in-bare.sh\nnew file mode 100755\nindex 0000000000..d9e03132b7\n--- /dev/null\n+++ b/t/t6136-pathspec-in-bare.sh\n@@ -0,0 +1,30 @@\n+#!/bin/sh\n+\n+test_description='diagnosing out-of-scope pathspec'\n+\n+. ./test-lib.sh\n+\n+test_expect_success 'setup a bare and non-bare repository' '\n+\ttest_commit file1 &&\n+\tgit clone --bare . bare\n+'\n+\n+test_expect_success 'log and ls-files in a bare repository' '\n+\t(\n+\t\tcd bare &&\n+\t\ttest_must_fail git log -- .. &&\n+\t\ttest_must_fail git ls-files -- ..\n+\t) >out 2>err &&\n+\ttest_i18ngrep \"outside repository\" err\n+'\n+\n+test_expect_success 'log and ls-files in .git directory' '\n+\t(\n+\t\tcd .git &&\n+\t\ttest_must_fail git log -- .. &&\n+\t\ttest_must_fail git ls-files -- ..\n+\t) >out 2>err &&\n+\ttest_i18ngrep \"outside repository\" err\n+'\n+\n+test_done\n"},{"id":"393226","messageId":"20200313190211.GA178103@google.com","threadId":"52985","inReplyTo":"xmqq36accdpt.fsf@gitster.c.googlers.com","subject":"Re: Regression in v2.26.0-rc0 and Magit","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2020-03-13T19:02:11Z","receivedAt":"2020-03-13T19:02:18Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Junio C Hamano wrote:\n\n> If nobody complains in the coming 4 hours or so, I'll squash this in\n> to e6c57b49 (\"prefix_path: show gitdir if worktree unavailable\",\n> 2020-03-02) and mark the topic as \"ready for 'next'\".\n>\n> Thanks.\n>\n> t/t6136-pathspec-in-bare.sh | 30 ++++++++++++++++++++++++++++++\n> 1 file changed, 30 insertions(+)\n\nReviewed-by: Jonathan Nieder \n\nEmily's out of office today, so this is well timed. Thanks for tying\nthat loose end.\n"},{"id":"393227","messageId":"xmqqy2s4axa2.fsf@gitster.c.googlers.com","threadId":"52985","inReplyTo":"xmqq36accdpt.fsf@gitster.c.googlers.com","subject":"Re: Regression in v2.26.0-rc0 and Magit","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-03-13T19:07:49Z","receivedAt":"2020-03-13T19:07:53Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Junio C Hamano writes:\n> ...\n> If nobody complains in the coming 4 hours or so, I'll squash this in\n> to e6c57b49 (\"prefix_path: show gitdir if worktree unavailable\",\n> 2020-03-02) and mark the topic as \"ready for 'next'\".\n>\n> Thanks.\n>\n> t/t6136-pathspec-in-bare.sh | 30 ++++++++++++++++++++++++++++++\n> 1 file changed, 30 insertions(+)\n> ...\n> +test_expect_success 'log and ls-files in .git directory' '\n> +\t(\n> +\t\tcd .git &&\n> +\t\ttest_must_fail git log -- .. &&\n> +\t\ttest_must_fail git ls-files -- ..\n> +\t) >out 2>err &&\n> +\ttest_i18ngrep \"outside repository\" err\n> +'\n> +\n> +test_done\n\nThis is outside the scope of fixing the regression e0020b2f\n(\"prefix_path: show gitdir when arg is outside repo\", 2020-02-14)\nbrought in, but I wonder if this last piece should even fail in the\nfirst place.\n\nIf you give \".\" instead of \"..\" to these commands, they behave as if\nwe did so from the top-level of the working tree, i.e. these are\nequivalent:\n\n git -C .git ls-files -- .\n git -C .git/info/ ls-files -- .\n git ls-files -- .\n\nwhich somehow does not sound quite right, but that is how tools\nwritten in the past 15 years expect and is hard to change?\n\nThat does not still explain why Magit (which is sufficiently mature)\nis expecting \"cd .git && ls-files ..\" to show the entire working\ntree, though.\n\n\n"},{"id":"393240","messageId":"87h7yr8omj.fsf@kyleam.com","threadId":"52985","inReplyTo":"xmqqy2s4axa2.fsf@gitster.c.googlers.com","subject":"Re: Regression in v2.26.0-rc0 and Magit","fromName":"Kyle Meyer","fromEmail":"kyle@kyleam.com","sentAt":"2020-03-14T05:57:40Z","receivedAt":"2020-03-15T01:33:52Z","isPatch":false,"sender":{"key":"kyle@kyleam.com","avatar":"https://avatars.githubusercontent.com/u/1297788?v=4"},"body":"Junio C Hamano writes:\n\n> That does not still explain why Magit (which is sufficiently mature)\n> is expecting \"cd .git && ls-files ..\" to show the entire working\n> tree, though.\n\nThe specific ls-files call that seems to trigger the segfault on Magit's\nend is equivalent to\n\n cd .git && git ls-files --error-unmatch -- $PWD/COMMIT_EDITMSG\n\nThe code is running ls-files to ask whether the file is tracked, which\nof course isn't a sensible thing to do outside of the working tree.\nI'll propose a change on Magit's end to avoid doing so.\n\n[ A few more specifics that might be of interest to Magit users ]\n\nThis happens in magit-auto-revert-mode. When visiting a file in .git/\n(e.g., COMMIT_EDITMSG when committing), magit-auto-revert-mode decides\nwhether to turn on auto-revert-mode in the same way it does for other\nfiles, magit-turn-on-auto-revert-mode-if-desired. That function doesn't\ndistinguish whether the file is in .git or the working tree, leading to\nthe odd \"is tracked file?\" query above.\n"},{"id":"393264","messageId":"20200315105803.GJ3122@szeder.dev","threadId":"52985","inReplyTo":"xmqq36accdpt.fsf@gitster.c.googlers.com","subject":"Re: Regression in v2.26.0-rc0 and Magit","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2020-03-15T10:58:03Z","receivedAt":"2020-03-15T10:58:10Z","isPatch":false,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Fri, Mar 13, 2020 at 11:27:26AM -0700, Junio C Hamano wrote:\n> Junio C Hamano writes:\n> \n> > Jonathan Nieder writes:\n> >\n> >> Junio, can you fast-track that fix to \"master\"? Emily, can you add a\n> >> test?\n> >\n> > Thanks, indeed it has been waiting for tests. We have a few more\n> > business days before -rc2, so...\n> >\n> > * es/outside-repo-errmsg-hints (2020-03-03) 1 commit\n> > - prefix_path: show gitdir if worktree unavailable\n> >\n> > An earlier update to show the location of working tree in the error\n> > message did not consider the possibility that a git command may be\n> > run in a bare repository, which has been corrected.\n> >\n> > May want a test or two.\n> \n> If nobody complains in the coming 4 hours or so, I'll squash this in\n> to e6c57b49 (\"prefix_path: show gitdir if worktree unavailable\",\n> 2020-03-02) and mark the topic as \"ready for 'next'\".\n> \n> Thanks.\n> \n> t/t6136-pathspec-in-bare.sh | 30 ++++++++++++++++++++++++++++++\n> 1 file changed, 30 insertions(+)\n> \n> diff --git a/t/t6136-pathspec-in-bare.sh b/t/t6136-pathspec-in-bare.sh\n> new file mode 100755\n> index 0000000000..d9e03132b7\n> --- /dev/null\n> +++ b/t/t6136-pathspec-in-bare.sh\n> @@ -0,0 +1,30 @@\n> +#!/bin/sh\n> +\n> +test_description='diagnosing out-of-scope pathspec'\n> +\n> +. ./test-lib.sh\n> +\n> +test_expect_success 'setup a bare and non-bare repository' '\n> +\ttest_commit file1 &&\n> +\tgit clone --bare . bare\n> +'\n> +\n> +test_expect_success 'log and ls-files in a bare repository' '\n> +\t(\n> +\t\tcd bare &&\n> +\t\ttest_must_fail git log -- .. &&\n> +\t\ttest_must_fail git ls-files -- ..\n> +\t) >out 2>err &&\n> +\ttest_i18ngrep \"outside repository\" err\n\nI think it would be better to write this test as:\n\n (\n cd bare &&\n test_must_fail git log -- .. 2>err &&\n\ttest_i18ngrep \"outside repository\" err &&\n test_must_fail git ls-files -- .. 2>err &&\n\ttest_i18ngrep \"outside repository\" err\n )\n\nbecause this way we make sure that both commands fail with the error\nwe expect.\n\n> +'\n> +\n> +test_expect_success 'log and ls-files in .git directory' '\n> +\t(\n> +\t\tcd .git &&\n> +\t\ttest_must_fail git log -- .. &&\n> +\t\ttest_must_fail git ls-files -- ..\n> +\t) >out 2>err &&\n> +\ttest_i18ngrep \"outside repository\" err\n> +'\n> +\n> +test_done\n"},{"id":"393272","messageId":"xmqq4kupbmpg.fsf@gitster.c.googlers.com","threadId":"52985","inReplyTo":"20200315105803.GJ3122@szeder.dev","subject":"Re: Regression in v2.26.0-rc0 and Magit","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-03-15T16:35:23Z","receivedAt":"2020-03-15T16:37:23Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"SZEDER Gábor writes:\n\n>> +test_expect_success 'log and ls-files in a bare repository' '\n>> +\t(\n>> +\t\tcd bare &&\n>> +\t\ttest_must_fail git log -- .. &&\n>> +\t\ttest_must_fail git ls-files -- ..\n>> +\t) >out 2>err &&\n>> +\ttest_i18ngrep \"outside repository\" err\n>\n> I think it would be better to write this test as:\n>\n> (\n> cd bare &&\n> test_must_fail git log -- .. 2>err &&\n> \ttest_i18ngrep \"outside repository\" err &&\n> test_must_fail git ls-files -- .. 2>err &&\n> \ttest_i18ngrep \"outside repository\" err\n> )\n>\n> because this way we make sure that both commands fail with the error\n> we expect.\n\nTrue. Otherwise one may fail expectedly, and the other one may fail\nin an unexpected but still clean way. Thanks for carefully reading.\n\nWe could also split it into two separate tests, but I think it would\nbe an overkill. The primary point of using must-fail is to ensure\nthat the command does not segfault, so in a sense, checking what is\nin err is somewhat, but not completely, a redundant thing to do.\n\nAbout checking redundantly, as we grab the standard output, we can\nalso make sure that it contains nothing, because we expect that the\nfailure happens way before the command is set up to compute what\nthey are asked to produce.\n\nBelow, I follow your suggestion to keep the log/ls-files pair in a\nsingle test, as I think splitting it into two is an overkill, but I\nkept the \"truly bare repository\" case and the \"non-bare repository,\nbut we stepped into $GIT_DIR ourselves\" case separate, and that is\ndeliberate. We might want to rethink the behaviour in the latter\ncase.\n\ndiff --git a/t/t6136-pathspec-in-bare.sh b/t/t6136-pathspec-in-bare.sh\nnew file mode 100755\nindex 0000000000..b117251366\n--- /dev/null\n+++ b/t/t6136-pathspec-in-bare.sh\n@@ -0,0 +1,38 @@\n+#!/bin/sh\n+\n+test_description='diagnosing out-of-scope pathspec'\n+\n+. ./test-lib.sh\n+\n+test_expect_success 'setup a bare and non-bare repository' '\n+\ttest_commit file1 &&\n+\tgit clone --bare . bare\n+'\n+\n+test_expect_success 'log and ls-files in a bare repository' '\n+\t(\n+\t\tcd bare &&\n+\t\ttest_must_fail git log -- .. >out 2>err &&\n+\t\ttest_must_be_empty out &&\n+\t\ttest_i18ngrep \"outside repository\" err &&\n+\n+\t\ttest_must_fail git ls-files -- .. >out 2>err &&\n+\t\ttest_must_be_empty out &&\n+\t\ttest_i18ngrep \"outside repository\" err\n+\t)\n+'\n+\n+test_expect_success 'log and ls-files in .git directory' '\n+\t(\n+\t\tcd .git &&\n+\t\ttest_must_fail git log -- .. >out 2>err &&\n+\t\ttest_must_be_empty out &&\n+\t\ttest_i18ngrep \"outside repository\" err &&\n+\n+\t\ttest_must_fail git ls-files -- .. >out 2>err &&\n+\t\ttest_must_be_empty out &&\n+\t\ttest_i18ngrep \"outside repository\" err\n+\t)\n+'\n+\n+test_done\n"}]}