Regression in v2.26.0-rc0 and Magit

Regression in v2.26.0-rc0 and Magit

[Jean-Noël AVILA]

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

Re: Regression in v2.26.0-rc0 and Magit

[Jonathan Nieder]

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

Re: Regression in v2.26.0-rc0 and Magit

[Junio C Hamano]

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.

Re: Regression in v2.26.0-rc0 and Magit

[Junio C Hamano]

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

Re: Regression in v2.26.0-rc0 and Magit

[Jonathan Nieder]

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.

Re: Regression in v2.26.0-rc0 and Magit

[Junio C Hamano]

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.

Re: Regression in v2.26.0-rc0 and Magit

[Kyle Meyer]

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.

Re: Regression in v2.26.0-rc0 and Magit

[SZEDER Gábor]

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

Re: Regression in v2.26.0-rc0 and Magit

[Junio C Hamano]

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