Regression in v2.26.0-rc0 and Magit

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

Hi,

Jean-Noël AVILA wrote:

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

Jonathan Nieder <jrnieder@gmail.com> writes:

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.

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(+)

Junio C Hamano wrote:

Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Emily's out of office today, so this is well timed. Thanks for tying that loose end.

Junio C Hamano <gitster@pobox.com> writes:

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.

Junio C Hamano <gitster@pobox.com> writes:

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.

On Fri, Mar 13, 2020 at 11:27:26AM -0700, Junio C Hamano wrote:

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.

SZEDER Gábor <szeder.dev@gmail.com> writes:

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.