* Regression in v2.26.0-rc0 and Magit @ 2020-03-12 22:55 Jean-Noël AVILA 2020-03-12 23:35 ` Jonathan Nieder 0 siblings, 1 reply; 9+ messages in thread From: Jean-Noël AVILA @ 2020-03-12 22:55 UTC (permalink / raw) To: git Hi all, When trying the latest rc with magit, I found that git segfaults under Magit with auto-revert enabled. The message in emacs is Error in post-command-hook (magit-auto-revert-mode-check-buffers): (wrong-type-argument number-or-marker-p "Segmentation Fault") I was able to bisect the issue to commit e0020b2f82910f50bc697d86aff70c3796fbdc41 but unfortunately, it seems difficult to print the exact command from magit. Reverting this patch solves the issue. Most probably emacs runs commands with not all env variables set. JN ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: Regression in v2.26.0-rc0 and Magit 2020-03-12 22:55 Regression in v2.26.0-rc0 and Magit Jean-Noël AVILA @ 2020-03-12 23:35 ` Jonathan Nieder 2020-03-13 0:02 ` Junio C Hamano 0 siblings, 1 reply; 9+ messages in thread From: Jonathan Nieder @ 2020-03-12 23:35 UTC (permalink / raw) To: Jean-Noël AVILA; +Cc: git, Junio C Hamano, Emily Shaffer Hi, Jean-Noël AVILA wrote: > When trying the latest rc with magit, I found that git segfaults > under Magit with auto-revert enabled. The message in emacs is > > Error in post-command-hook (magit-auto-revert-mode-check-buffers): > (wrong-type-argument number-or-marker-p "Segmentation Fault") > > I was able to bisect the issue to commit > e0020b2f82910f50bc697d86aff70c3796fbdc41 but unfortunately, it seems > difficult to print the exact command from magit. Thanks for reporting. This is fixed by commit e6c57b49eb63e77ccf72215229744c4beaf04204 (es/outside-repo-errmsg-hints) Author: Emily Shaffer <emilyshaffer@google.com> Date: Mon Mar 2 20:05:06 2020 -0800 prefix_path: show gitdir if worktree unavailable Junio, can you fast-track that fix to "master"? Emily, can you add a test? Thanks, Jonathan ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: Regression in v2.26.0-rc0 and Magit 2020-03-12 23:35 ` Jonathan Nieder @ 2020-03-13 0:02 ` Junio C Hamano 2020-03-13 18:27 ` Junio C Hamano 0 siblings, 1 reply; 9+ messages in thread From: Junio C Hamano @ 2020-03-13 0:02 UTC (permalink / raw) To: Jonathan Nieder; +Cc: Jean-Noël AVILA, git, Emily Shaffer Jonathan Nieder <jrnieder@gmail.com> writes: > Junio, can you fast-track that fix to "master"? Emily, can you add a > test? Thanks, indeed it has been waiting for tests. We have a few more business days before -rc2, so... * es/outside-repo-errmsg-hints (2020-03-03) 1 commit - prefix_path: show gitdir if worktree unavailable An earlier update to show the location of working tree in the error message did not consider the possibility that a git command may be run in a bare repository, which has been corrected. May want a test or two. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: Regression in v2.26.0-rc0 and Magit 2020-03-13 0:02 ` Junio C Hamano @ 2020-03-13 18:27 ` Junio C Hamano 2020-03-13 19:02 ` Jonathan Nieder ` (2 more replies) 0 siblings, 3 replies; 9+ messages in thread From: Junio C Hamano @ 2020-03-13 18:27 UTC (permalink / raw) To: Jonathan Nieder; +Cc: Jean-Noël AVILA, git, Emily Shaffer Junio C Hamano <gitster@pobox.com> writes: > Jonathan Nieder <jrnieder@gmail.com> writes: > >> Junio, can you fast-track that fix to "master"? Emily, can you add a >> test? > > Thanks, indeed it has been waiting for tests. We have a few more > business days before -rc2, so... > > * es/outside-repo-errmsg-hints (2020-03-03) 1 commit > - prefix_path: show gitdir if worktree unavailable > > An earlier update to show the location of working tree in the error > message did not consider the possibility that a git command may be > run in a bare repository, which has been corrected. > > May want a test or two. If nobody complains in the coming 4 hours or so, I'll squash this in to e6c57b49 ("prefix_path: show gitdir if worktree unavailable", 2020-03-02) and mark the topic as "ready for 'next'". Thanks. t/t6136-pathspec-in-bare.sh | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/t/t6136-pathspec-in-bare.sh b/t/t6136-pathspec-in-bare.sh new file mode 100755 index 0000000000..d9e03132b7 --- /dev/null +++ b/t/t6136-pathspec-in-bare.sh @@ -0,0 +1,30 @@ +#!/bin/sh + +test_description='diagnosing out-of-scope pathspec' + +. ./test-lib.sh + +test_expect_success 'setup a bare and non-bare repository' ' + test_commit file1 && + git clone --bare . bare +' + +test_expect_success 'log and ls-files in a bare repository' ' + ( + cd bare && + test_must_fail git log -- .. && + test_must_fail git ls-files -- .. + ) >out 2>err && + test_i18ngrep "outside repository" err +' + +test_expect_success 'log and ls-files in .git directory' ' + ( + cd .git && + test_must_fail git log -- .. && + test_must_fail git ls-files -- .. + ) >out 2>err && + test_i18ngrep "outside repository" err +' + +test_done ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: Regression in v2.26.0-rc0 and Magit 2020-03-13 18:27 ` Junio C Hamano @ 2020-03-13 19:02 ` Jonathan Nieder 2020-03-13 19:07 ` Junio C Hamano 2020-03-15 10:58 ` SZEDER Gábor 2 siblings, 0 replies; 9+ messages in thread From: Jonathan Nieder @ 2020-03-13 19:02 UTC (permalink / raw) To: Junio C Hamano; +Cc: Jean-Noël AVILA, git, Emily Shaffer Junio C Hamano wrote: > If nobody complains in the coming 4 hours or so, I'll squash this in > to e6c57b49 ("prefix_path: show gitdir if worktree unavailable", > 2020-03-02) and mark the topic as "ready for 'next'". > > Thanks. > > t/t6136-pathspec-in-bare.sh | 30 ++++++++++++++++++++++++++++++ > 1 file changed, 30 insertions(+) Reviewed-by: Jonathan Nieder <jrnieder@gmail.com> Emily's out of office today, so this is well timed. Thanks for tying that loose end. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: Regression in v2.26.0-rc0 and Magit 2020-03-13 18:27 ` Junio C Hamano 2020-03-13 19:02 ` Jonathan Nieder @ 2020-03-13 19:07 ` Junio C Hamano 2020-03-14 5:57 ` Kyle Meyer 2020-03-15 10:58 ` SZEDER Gábor 2 siblings, 1 reply; 9+ messages in thread From: Junio C Hamano @ 2020-03-13 19:07 UTC (permalink / raw) To: Jonathan Nieder, Emily Shaffer; +Cc: Jean-Noël AVILA, git Junio C Hamano <gitster@pobox.com> writes: > Junio C Hamano <gitster@pobox.com> writes: > ... > If nobody complains in the coming 4 hours or so, I'll squash this in > to e6c57b49 ("prefix_path: show gitdir if worktree unavailable", > 2020-03-02) and mark the topic as "ready for 'next'". > > Thanks. > > t/t6136-pathspec-in-bare.sh | 30 ++++++++++++++++++++++++++++++ > 1 file changed, 30 insertions(+) > ... > +test_expect_success 'log and ls-files in .git directory' ' > + ( > + cd .git && > + test_must_fail git log -- .. && > + test_must_fail git ls-files -- .. > + ) >out 2>err && > + test_i18ngrep "outside repository" err > +' > + > +test_done This is outside the scope of fixing the regression e0020b2f ("prefix_path: show gitdir when arg is outside repo", 2020-02-14) brought in, but I wonder if this last piece should even fail in the first place. If you give "." instead of ".." to these commands, they behave as if we did so from the top-level of the working tree, i.e. these are equivalent: git -C .git ls-files -- . git -C .git/info/ ls-files -- . git ls-files -- . which somehow does not sound quite right, but that is how tools written in the past 15 years expect and is hard to change? That does not still explain why Magit (which is sufficiently mature) is expecting "cd .git && ls-files .." to show the entire working tree, though. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: Regression in v2.26.0-rc0 and Magit 2020-03-13 19:07 ` Junio C Hamano @ 2020-03-14 5:57 ` Kyle Meyer 0 siblings, 0 replies; 9+ messages in thread From: Kyle Meyer @ 2020-03-14 5:57 UTC (permalink / raw) To: Junio C Hamano; +Cc: Jean-Noël AVILA, git, Jonathan Nieder, Emily Shaffer Junio C Hamano <gitster@pobox.com> writes: > That does not still explain why Magit (which is sufficiently mature) > is expecting "cd .git && ls-files .." to show the entire working > tree, though. The specific ls-files call that seems to trigger the segfault on Magit's end is equivalent to cd .git && git ls-files --error-unmatch -- $PWD/COMMIT_EDITMSG The code is running ls-files to ask whether the file is tracked, which of course isn't a sensible thing to do outside of the working tree. I'll propose a change on Magit's end to avoid doing so. [ A few more specifics that might be of interest to Magit users ] This happens in magit-auto-revert-mode. When visiting a file in .git/ (e.g., COMMIT_EDITMSG when committing), magit-auto-revert-mode decides whether to turn on auto-revert-mode in the same way it does for other files, magit-turn-on-auto-revert-mode-if-desired. That function doesn't distinguish whether the file is in .git or the working tree, leading to the odd "is tracked file?" query above. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: Regression in v2.26.0-rc0 and Magit 2020-03-13 18:27 ` Junio C Hamano 2020-03-13 19:02 ` Jonathan Nieder 2020-03-13 19:07 ` Junio C Hamano @ 2020-03-15 10:58 ` SZEDER Gábor 2020-03-15 16:35 ` Junio C Hamano 2 siblings, 1 reply; 9+ messages in thread From: SZEDER Gábor @ 2020-03-15 10:58 UTC (permalink / raw) To: Junio C Hamano; +Cc: Jonathan Nieder, Jean-Noël AVILA, git, Emily Shaffer On Fri, Mar 13, 2020 at 11:27:26AM -0700, Junio C Hamano wrote: > Junio C Hamano <gitster@pobox.com> writes: > > > Jonathan Nieder <jrnieder@gmail.com> writes: > > > >> Junio, can you fast-track that fix to "master"? Emily, can you add a > >> test? > > > > Thanks, indeed it has been waiting for tests. We have a few more > > business days before -rc2, so... > > > > * es/outside-repo-errmsg-hints (2020-03-03) 1 commit > > - prefix_path: show gitdir if worktree unavailable > > > > An earlier update to show the location of working tree in the error > > message did not consider the possibility that a git command may be > > run in a bare repository, which has been corrected. > > > > May want a test or two. > > If nobody complains in the coming 4 hours or so, I'll squash this in > to e6c57b49 ("prefix_path: show gitdir if worktree unavailable", > 2020-03-02) and mark the topic as "ready for 'next'". > > Thanks. > > t/t6136-pathspec-in-bare.sh | 30 ++++++++++++++++++++++++++++++ > 1 file changed, 30 insertions(+) > > diff --git a/t/t6136-pathspec-in-bare.sh b/t/t6136-pathspec-in-bare.sh > new file mode 100755 > index 0000000000..d9e03132b7 > --- /dev/null > +++ b/t/t6136-pathspec-in-bare.sh > @@ -0,0 +1,30 @@ > +#!/bin/sh > + > +test_description='diagnosing out-of-scope pathspec' > + > +. ./test-lib.sh > + > +test_expect_success 'setup a bare and non-bare repository' ' > + test_commit file1 && > + git clone --bare . bare > +' > + > +test_expect_success 'log and ls-files in a bare repository' ' > + ( > + cd bare && > + test_must_fail git log -- .. && > + test_must_fail git ls-files -- .. > + ) >out 2>err && > + test_i18ngrep "outside repository" err I think it would be better to write this test as: ( cd bare && test_must_fail git log -- .. 2>err && test_i18ngrep "outside repository" err && test_must_fail git ls-files -- .. 2>err && test_i18ngrep "outside repository" err ) because this way we make sure that both commands fail with the error we expect. > +' > + > +test_expect_success 'log and ls-files in .git directory' ' > + ( > + cd .git && > + test_must_fail git log -- .. && > + test_must_fail git ls-files -- .. > + ) >out 2>err && > + test_i18ngrep "outside repository" err > +' > + > +test_done ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: Regression in v2.26.0-rc0 and Magit 2020-03-15 10:58 ` SZEDER Gábor @ 2020-03-15 16:35 ` Junio C Hamano 0 siblings, 0 replies; 9+ messages in thread From: Junio C Hamano @ 2020-03-15 16:35 UTC (permalink / raw) To: SZEDER Gábor Cc: Jonathan Nieder, Jean-Noël AVILA, git, Emily Shaffer SZEDER Gábor <szeder.dev@gmail.com> writes: >> +test_expect_success 'log and ls-files in a bare repository' ' >> + ( >> + cd bare && >> + test_must_fail git log -- .. && >> + test_must_fail git ls-files -- .. >> + ) >out 2>err && >> + test_i18ngrep "outside repository" err > > I think it would be better to write this test as: > > ( > cd bare && > test_must_fail git log -- .. 2>err && > test_i18ngrep "outside repository" err && > test_must_fail git ls-files -- .. 2>err && > test_i18ngrep "outside repository" err > ) > > because this way we make sure that both commands fail with the error > we expect. True. Otherwise one may fail expectedly, and the other one may fail in an unexpected but still clean way. Thanks for carefully reading. We could also split it into two separate tests, but I think it would be an overkill. The primary point of using must-fail is to ensure that the command does not segfault, so in a sense, checking what is in err is somewhat, but not completely, a redundant thing to do. About checking redundantly, as we grab the standard output, we can also make sure that it contains nothing, because we expect that the failure happens way before the command is set up to compute what they are asked to produce. Below, I follow your suggestion to keep the log/ls-files pair in a single test, as I think splitting it into two is an overkill, but I kept the "truly bare repository" case and the "non-bare repository, but we stepped into $GIT_DIR ourselves" case separate, and that is deliberate. We might want to rethink the behaviour in the latter case. diff --git a/t/t6136-pathspec-in-bare.sh b/t/t6136-pathspec-in-bare.sh new file mode 100755 index 0000000000..b117251366 --- /dev/null +++ b/t/t6136-pathspec-in-bare.sh @@ -0,0 +1,38 @@ +#!/bin/sh + +test_description='diagnosing out-of-scope pathspec' + +. ./test-lib.sh + +test_expect_success 'setup a bare and non-bare repository' ' + test_commit file1 && + git clone --bare . bare +' + +test_expect_success 'log and ls-files in a bare repository' ' + ( + cd bare && + test_must_fail git log -- .. >out 2>err && + test_must_be_empty out && + test_i18ngrep "outside repository" err && + + test_must_fail git ls-files -- .. >out 2>err && + test_must_be_empty out && + test_i18ngrep "outside repository" err + ) +' + +test_expect_success 'log and ls-files in .git directory' ' + ( + cd .git && + test_must_fail git log -- .. >out 2>err && + test_must_be_empty out && + test_i18ngrep "outside repository" err && + + test_must_fail git ls-files -- .. >out 2>err && + test_must_be_empty out && + test_i18ngrep "outside repository" err + ) +' + +test_done ^ permalink raw reply related [flat|nested] 9+ messages in thread
end of thread, other threads:[~2020-03-15 16:37 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-03-12 22:55 Regression in v2.26.0-rc0 and Magit Jean-Noël AVILA 2020-03-12 23:35 ` Jonathan Nieder 2020-03-13 0:02 ` Junio C Hamano 2020-03-13 18:27 ` Junio C Hamano 2020-03-13 19:02 ` Jonathan Nieder 2020-03-13 19:07 ` Junio C Hamano 2020-03-14 5:57 ` Kyle Meyer 2020-03-15 10:58 ` SZEDER Gábor 2020-03-15 16:35 ` Junio C Hamano
Code repositories for project(s) associated with this public inbox https://80x24.org/mirrors/git.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).