Crash on MSYS2 with GIT_WORK_TREE

Crash on MSYS2 with GIT_WORK_TREE

[valtron]

Git 1.12.0. When GIT_WORK_TREE contains a drive-letter and
forward-slashes, some git commands crash:

C:\repo>set GIT_WORK_TREE=C:/repo
C:\repo>git rev-parse HEAD
     1 [main] git 2332 cygwin_exception::open_stackdumpfile: Dumping
stack trace to git.exe.stackdump
C:\repo>set GIT_WORK_TREE=
C:\repo>git rev-parse HEAD
a394e40861e1012a96f9578a1f0cf0c5a49ede11

On the other hand, "C:\repo" and "/c/repo" don't have this issue.

Stacktrace from GDB (on git-rev-parse) is:

#0  0x000000018019634d in strcmp (s1=0x600062080 "/c/repo", s2=0x0)
   at /msys_scripts/msys2-runtime/src/msys2-runtime/newlib/libc/string/strcmp.c:83
#1  0x00000001005239f1 in ?? ()
#2  0x0000000100523f36 in ?? ()
#3  0x000000010046c6fa in ?? ()
#4  0x0000000100401b6d in ?? ()
#5  0x0000000100401e4b in ?? ()
#6  0x0000000100563593 in ?? ()
#7  0x0000000180047c37 in _cygwin_exit_return ()
   at /msys_scripts/msys2-runtime/src/msys2-runtime/winsup/cygwin/dcrt0.cc:1031
#8  0x0000000180045873 in _cygtls::call2 (this=0xffffce00,
func=0x180046bd0 <dll_crt0_1(void*)>, arg=0x0,
   buf=buf@entry=0xffffcdf0) at
/msys_scripts/msys2-runtime/src/msys2-runtime/winsup/cygwin/cygtls.cc:40
#9  0x0000000180045924 in _cygtls::call (func=<optimized out>,
arg=<optimized out>)
   at /msys_scripts/msys2-runtime/src/msys2-runtime/winsup/cygwin/cygtls.cc:27
#10 0x0000000000000000 in ?? ()
Backtrace stopped: previous frame inner to this frame (corrupt stack?)

It seems "C:/repo" was changed to "/c/repo", but it crashes because it
gets compared to a nullptr.

Re: Crash on MSYS2 with GIT_WORK_TREE

[Johannes Schindelin]

Hi valtron,

On Tue, 7 Mar 2017, valtron wrote:

> Git 1.12.0.

I take it you meant 2.12.0. And you probably also meant to imply that you
are referring to MSYS2's git.exe in /usr/bin/.

> When GIT_WORK_TREE contains a drive-letter and forward-slashes, some git
> commands crash:
> 
> C:\repo>set GIT_WORK_TREE=C:/repo
> C:\repo>git rev-parse HEAD
>      1 [main] git 2332 cygwin_exception::open_stackdumpfile: Dumping
> stack trace to git.exe.stackdump

This suggests that my assumption above is correct: it looks as if you are
executing <MSYS2>\usr\bin\git.exe here.

The proof lies in the pudding, though, and you are the only person who can
do that unless you post the contents of that git.exe.stackdump.

> Stacktrace from GDB (on git-rev-parse) is:
> 
> #0  0x000000018019634d in strcmp (s1=0x600062080 "/c/repo", s2=0x0)
>    at /msys_scripts/msys2-runtime/src/msys2-runtime/newlib/libc/string/strcmp.c:83
> #1  0x00000001005239f1 in ?? ()
> #2  0x0000000100523f36 in ?? ()
> #3  0x000000010046c6fa in ?? ()
> #4  0x0000000100401b6d in ?? ()
> #5  0x0000000100401e4b in ?? ()
> #6  0x0000000100563593 in ?? ()
> #7  0x0000000180047c37 in _cygwin_exit_return ()
>    at /msys_scripts/msys2-runtime/src/msys2-runtime/winsup/cygwin/dcrt0.cc:1031
> #8  0x0000000180045873 in _cygtls::call2 (this=0xffffce00,
> func=0x180046bd0 <dll_crt0_1(void*)>, arg=0x0,
>    buf=buf@entry=0xffffcdf0) at
> /msys_scripts/msys2-runtime/src/msys2-runtime/winsup/cygwin/cygtls.cc:40
> #9  0x0000000180045924 in _cygtls::call (func=<optimized out>,
> arg=<optimized out>)
>    at /msys_scripts/msys2-runtime/src/msys2-runtime/winsup/cygwin/cygtls.cc:27
> #10 0x0000000000000000 in ?? ()
> Backtrace stopped: previous frame inner to this frame (corrupt stack?)

This may be the wrong thread, though. You can find out what other threads
there are with `info threads` and switch by `thread <id>`, the
backtrace(s) of the other thread(s) may be informative.

Please also note that installing the `msys2-runtime-devel` package via
Pacman may be able to get you nicer symbols.

In any case, this problem is squarely within the MSYS2 runtime. It has
nothing to do with Git except for the motivation to set an environment
variable to an absolute path as you outlined.

Having said that, the problem also occurs when using *MSYS2* git.exe (i.e.
/usr/bin/git.exe, not /mingw64/bin/git.exe) in the Git for Windows SDK.

Ciao,
Johannes

Re: Crash on MSYS2 with GIT_WORK_TREE

[Johannes Schindelin]

Hi valtron,

On Wed, 8 Mar 2017, Johannes Schindelin wrote:

> On Tue, 7 Mar 2017, valtron wrote:
> 
> > When GIT_WORK_TREE contains a drive-letter and forward-slashes, some git
> > commands crash:
> > 
> > C:\repo>set GIT_WORK_TREE=C:/repo
> > C:\repo>git rev-parse HEAD
> >      1 [main] git 2332 cygwin_exception::open_stackdumpfile: Dumping
> > stack trace to git.exe.stackdump
> 
> [...]
>
> In any case, this problem is squarely within the MSYS2 runtime. It has
> nothing to do with Git except for the motivation to set an environment
> variable to an absolute path as you outlined.

Oh boy was I *wrong*! I take that back and apologize for my premature
verdict.

It is true that you should not set GIT_WORKTREE=c:/repo if you want to
work with MSYS2 Git because MSYS2 expects pseudo Unix paths, i.e. /c/repo,
and it will simply try to guess correctly and convert Windows paths with
drive letters and backslashes to that form.

But that does not excuse a crash.

The problem is actually even worse: On *Linux*, this happens:

	$ GIT_WORK_TREE=c:/invalid git rev-parse HEAD
	Segmentation fault (core dumped)

The reason is this: when set_git_work_tree() was converted from using
xstrdup(real_path()) to real_pathdup(), we completely missed the fact that
the former passed die_on_error = 1 to strbuf_realpath(), while the latter
passed die_on_error = 0. As a consequence, work_tree can be NULL now, and
the current code does not expect set_git_work_tree() to return
successfully after setting work_tree to NULL.

I Cc:ed Brandon, the author of 4ac9006f832 (real_path: have callers use
real_pathdup and strbuf_realpath, 2016-12-12).

Brandon, I have a hunch that pretty much all of the xstrdup(real_path())
-> real_pathdup() sites have a problem now. The previous contract was that
real_path() would die() if the passed path is invalid. The new contract is
that real_pathdup() returns NULL in such a case. I believe that the
following call sites are problematic in particular:

builtin/init-db.c: init_db():
	char *original_git_dir = real_pathdup(git_dir);

builtin/init-db.c: cmd_init_db():
	real_git_dir = real_pathdup(real_git_dir);
	...
	git_work_tree_cfg = real_pathdup(rel);

environment.c: set_git_work_tree():
	work_tree = real_pathdup(new_work_tree);

setup.c: setup_discovered_git_dir():
	gitdir = real_pathdup(gitdir);

submodule.c: connect_work_tree_and_git_dir():
	const char *real_work_tree = real_pathdup(work_tree);

transport.c: refs_from_alternate_cb():
	other = real_pathdup(e->path);

worktree.c: find_worktree():
	path = real_pathdup(arg);

I verified that all calls are still there, except for the submodule.c one
which simply moved to dir.c and the transport.c one which apparently now
no longer die()s but simply ignores non-existing paths now.

That leaves six places to patch, methinks... This diff may serve as an
initial version, but I have not really had a deep look at all call sites
(and it is an unwise idea to trust me at this hour anyway, look at the
time when I sent this mail):

-- snipsnap --
diff --git a/abspath.c b/abspath.c
index 2f0c26e0e2c..b02e068aa34 100644
--- a/abspath.c
+++ b/abspath.c
@@ -214,12 +214,12 @@ const char *real_path_if_valid(const char *path)
 	return strbuf_realpath(&realpath, path, 0);
 }
 
-char *real_pathdup(const char *path)
+char *real_pathdup(const char *path, int die_on_error)
 {
 	struct strbuf realpath = STRBUF_INIT;
 	char *retval = NULL;
 
-	if (strbuf_realpath(&realpath, path, 0))
+	if (strbuf_realpath(&realpath, path, die_on_error))
 		retval = strbuf_detach(&realpath, NULL);
 
 	strbuf_release(&realpath);
diff --git a/builtin/init-db.c b/builtin/init-db.c
index 1d4d6a00789..8a6acb0ec69 100644
--- a/builtin/init-db.c
+++ b/builtin/init-db.c
@@ -338,7 +338,7 @@ int init_db(const char *git_dir, const char *real_git_dir,
 {
 	int reinit;
 	int exist_ok = flags & INIT_DB_EXIST_OK;
-	char *original_git_dir = real_pathdup(git_dir);
+	char *original_git_dir = real_pathdup(git_dir, 1);
 
 	if (real_git_dir) {
 		struct stat st;
@@ -489,7 +489,7 @@ int cmd_init_db(int argc, const char **argv, const char *prefix)
 	argc = parse_options(argc, argv, prefix, init_db_options, init_db_usage, 0);
 
 	if (real_git_dir && !is_absolute_path(real_git_dir))
-		real_git_dir = real_pathdup(real_git_dir);
+		real_git_dir = real_pathdup(real_git_dir, 1);
 
 	if (argc == 1) {
 		int mkdir_tried = 0;
@@ -560,7 +560,7 @@ int cmd_init_db(int argc, const char **argv, const char *prefix)
 		const char *git_dir_parent = strrchr(git_dir, '/');
 		if (git_dir_parent) {
 			char *rel = xstrndup(git_dir, git_dir_parent - git_dir);
-			git_work_tree_cfg = real_pathdup(rel);
+			git_work_tree_cfg = real_pathdup(rel, 1);
 			free(rel);
 		}
 		if (!git_work_tree_cfg)
diff --git a/cache.h b/cache.h
index e7b57457e73..7168c1e5ff0 100644
--- a/cache.h
+++ b/cache.h
@@ -1160,7 +1160,7 @@ char *strbuf_realpath(struct strbuf *resolved, const char *path,
 		      int die_on_error);
 const char *real_path(const char *path);
 const char *real_path_if_valid(const char *path);
-char *real_pathdup(const char *path);
+char *real_pathdup(const char *path, int die_on_error);
 const char *absolute_path(const char *path);
 char *absolute_pathdup(const char *path);
 const char *remove_leading_path(const char *in, const char *prefix);
diff --git a/dir.c b/dir.c
index 4541f9e1460..aeeb5ce1049 100644
--- a/dir.c
+++ b/dir.c
@@ -2730,8 +2730,8 @@ void connect_work_tree_and_git_dir(const char *work_tree_, const char *git_dir_)
 {
 	struct strbuf file_name = STRBUF_INIT;
 	struct strbuf rel_path = STRBUF_INIT;
-	char *git_dir = real_pathdup(git_dir_);
-	char *work_tree = real_pathdup(work_tree_);
+	char *git_dir = real_pathdup(git_dir_, 1);
+	char *work_tree = real_pathdup(work_tree_, 1);
 
 	/* Update gitfile */
 	strbuf_addf(&file_name, "%s/.git", work_tree);
diff --git a/environment.c b/environment.c
index c07fb17fb70..42dc3106d2f 100644
--- a/environment.c
+++ b/environment.c
@@ -259,7 +259,7 @@ void set_git_work_tree(const char *new_work_tree)
 		return;
 	}
 	git_work_tree_initialized = 1;
-	work_tree = real_pathdup(new_work_tree);
+	work_tree = real_pathdup(new_work_tree, 1);
 }
 
 const char *get_git_work_tree(void)
diff --git a/setup.c b/setup.c
index 9118b48590a..d51549a6de3 100644
--- a/setup.c
+++ b/setup.c
@@ -698,7 +698,7 @@ static const char *setup_discovered_git_dir(const char *gitdir,
 	/* --work-tree is set without --git-dir; use discovered one */
 	if (getenv(GIT_WORK_TREE_ENVIRONMENT) || git_work_tree_cfg) {
 		if (offset != cwd->len && !is_absolute_path(gitdir))
-			gitdir = real_pathdup(gitdir);
+			gitdir = real_pathdup(gitdir, 1);
 		if (chdir(cwd->buf))
 			die_errno("Could not come back to cwd");
 		return setup_explicit_git_dir(gitdir, cwd, nongit_ok);
@@ -808,7 +808,7 @@ static int canonicalize_ceiling_entry(struct string_list_item *item,
 		/* Keep entry but do not canonicalize it */
 		return 1;
 	} else {
-		char *real_path = real_pathdup(ceil);
+		char *real_path = real_pathdup(ceil, 0);
 		if (!real_path) {
 			return 0;
 		}
diff --git a/submodule.c b/submodule.c
index 3b98766a6bc..1d4c0ce86ee 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1403,7 +1403,7 @@ static void relocate_single_git_dir_into_superproject(const char *prefix,
 		/* If it is an actual gitfile, it doesn't need migration. */
 		return;
 
-	real_old_git_dir = real_pathdup(old_git_dir);
+	real_old_git_dir = real_pathdup(old_git_dir, 0);
 
 	sub = submodule_from_path(null_sha1, path);
 	if (!sub)
@@ -1412,7 +1412,7 @@ static void relocate_single_git_dir_into_superproject(const char *prefix,
 	new_git_dir = git_path("modules/%s", sub->name);
 	if (safe_create_leading_directories_const(new_git_dir) < 0)
 		die(_("could not create directory '%s'"), new_git_dir);
-	real_new_git_dir = real_pathdup(new_git_dir);
+	real_new_git_dir = real_pathdup(new_git_dir, 0);
 
 	if (!prefix)
 		prefix = get_super_prefix();
@@ -1472,14 +1472,14 @@ void absorb_git_dir_into_superproject(const char *prefix,
 		new_git_dir = git_path("modules/%s", sub->name);
 		if (safe_create_leading_directories_const(new_git_dir) < 0)
 			die(_("could not create directory '%s'"), new_git_dir);
-		real_new_git_dir = real_pathdup(new_git_dir);
+		real_new_git_dir = real_pathdup(new_git_dir, 0);
 		connect_work_tree_and_git_dir(path, real_new_git_dir);
 
 		free(real_new_git_dir);
 	} else {
 		/* Is it already absorbed into the superprojects git dir? */
-		char *real_sub_git_dir = real_pathdup(sub_git_dir);
-		char *real_common_git_dir = real_pathdup(get_git_common_dir());
+		char *real_sub_git_dir = real_pathdup(sub_git_dir, 0);
+		char *real_common_git_dir = real_pathdup(get_git_common_dir(), 0);
 
 		if (!starts_with(real_sub_git_dir, real_common_git_dir))
 			relocate_single_git_dir_into_superproject(prefix, path);
diff --git a/worktree.c b/worktree.c
index d633761575b..0486e31ad4a 100644
--- a/worktree.c
+++ b/worktree.c
@@ -255,7 +255,7 @@ struct worktree *find_worktree(struct worktree **list,
 		return wt;
 
 	arg = prefix_filename(prefix, strlen(prefix), arg);
-	path = real_pathdup(arg);
+	path = real_pathdup(arg, 1);
 	for (; *list; list++)
 		if (!fspathcmp(path, real_path((*list)->path)))
 			break;

Re: Crash on MSYS2 with GIT_WORK_TREE

[Johannes Schindelin]

Hi valtron,

just to set the record straight on a few of my suggestions that turned out
to be incorrect:

On Wed, 8 Mar 2017, Johannes Schindelin wrote:

> On Tue, 7 Mar 2017, valtron wrote:
> 
> > Stacktrace from GDB (on git-rev-parse) is:
> > 
> > #0  0x000000018019634d in strcmp (s1=0x600062080 "/c/repo", s2=0x0)
> >    at /msys_scripts/msys2-runtime/src/msys2-runtime/newlib/libc/string/strcmp.c:83
> > #1  0x00000001005239f1 in ?? ()
> > #2  0x0000000100523f36 in ?? ()
> > #3  0x000000010046c6fa in ?? ()
> > #4  0x0000000100401b6d in ?? ()
> > #5  0x0000000100401e4b in ?? ()
> > #6  0x0000000100563593 in ?? ()
> > #7  0x0000000180047c37 in _cygwin_exit_return ()
> >    at /msys_scripts/msys2-runtime/src/msys2-runtime/winsup/cygwin/dcrt0.cc:1031
> > #8  0x0000000180045873 in _cygtls::call2 (this=0xffffce00,
> > func=0x180046bd0 <dll_crt0_1(void*)>, arg=0x0,
> >    buf=buf@entry=0xffffcdf0) at
> > /msys_scripts/msys2-runtime/src/msys2-runtime/winsup/cygwin/cygtls.cc:40
> > #9  0x0000000180045924 in _cygtls::call (func=<optimized out>,
> > arg=<optimized out>)
> >    at /msys_scripts/msys2-runtime/src/msys2-runtime/winsup/cygwin/cygtls.cc:27
> > #10 0x0000000000000000 in ?? ()
> > Backtrace stopped: previous frame inner to this frame (corrupt stack?)
> 
> This may be the wrong thread, though. You can find out what other threads
> there are with `info threads` and switch by `thread <id>`, the
> backtrace(s) of the other thread(s) may be informative.

It was actually the correct thread.

> Please also note that installing the `msys2-runtime-devel` package via
> Pacman may be able to get you nicer symbols.

I was wrong. The problem here is git.exe: you would have to uncomment the
'#options("debug","!strip")' line in the git/PKGBUILD file in
https://github.com/Alexpux/MSYS2-packages before rebuilding the package
[*1*], and then reinstalling it (which increases the installed size by
94.82MB to 119.63MB due to the unstripped symbols). But then gdb would
find the symbols alright and you'd see that the crash happens in

#1  0x000000010051aecb in setup_explicit_git_dir
    (gitdirenv=gitdirenv@entry=0x1005a00cc <sign_off_header+556> ".git",
     cwd=cwd@entry=0x10055e970 <cwd>, nongit_ok=nongit_ok@entry=0x0) at
    setup.c:669
669             if (!strcmp(cwd->buf, worktree)) { /* cwd == worktree */

and worktree indeed was NULL.

Ciao,
Johannes

Re: Crash on MSYS2 with GIT_WORK_TREE

[valtron]

Hi Johannes,

Thanks for looking at this! Yes, it's 2.12.0, sorry for the typo.

I only ran into this because of git-gui, where I eventually tracked it
down to line 1330:

    set env(GIT_WORK_TREE) $_gitworktree

With that line commented out, it works. I'll look into why git-gui
sets it to a windows-path-with-forward-slashes, but that's a separate
issue from the crash. Also, from the stack trace, I think git is still
able to understand the path, since it appears to correctly convert it
to /c/repo, but I might be wrong since I haven't look at the code.

On Tue, Mar 7, 2017 at 5:51 PM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> Hi valtron,
>
> On Wed, 8 Mar 2017, Johannes Schindelin wrote:
>
>> On Tue, 7 Mar 2017, valtron wrote:
>>
>> > When GIT_WORK_TREE contains a drive-letter and forward-slashes, some git
>> > commands crash:
>> >
>> > C:\repo>set GIT_WORK_TREE=C:/repo
>> > C:\repo>git rev-parse HEAD
>> >      1 [main] git 2332 cygwin_exception::open_stackdumpfile: Dumping
>> > stack trace to git.exe.stackdump
>>
>> [...]
>>
>> In any case, this problem is squarely within the MSYS2 runtime. It has
>> nothing to do with Git except for the motivation to set an environment
>> variable to an absolute path as you outlined.
>
> Oh boy was I *wrong*! I take that back and apologize for my premature
> verdict.
>
> It is true that you should not set GIT_WORKTREE=c:/repo if you want to
> work with MSYS2 Git because MSYS2 expects pseudo Unix paths, i.e. /c/repo,
> and it will simply try to guess correctly and convert Windows paths with
> drive letters and backslashes to that form.
>
> But that does not excuse a crash.
>
> The problem is actually even worse: On *Linux*, this happens:
>
>         $ GIT_WORK_TREE=c:/invalid git rev-parse HEAD
>         Segmentation fault (core dumped)
>
> The reason is this: when set_git_work_tree() was converted from using
> xstrdup(real_path()) to real_pathdup(), we completely missed the fact that
> the former passed die_on_error = 1 to strbuf_realpath(), while the latter
> passed die_on_error = 0. As a consequence, work_tree can be NULL now, and
> the current code does not expect set_git_work_tree() to return
> successfully after setting work_tree to NULL.
>
> I Cc:ed Brandon, the author of 4ac9006f832 (real_path: have callers use
> real_pathdup and strbuf_realpath, 2016-12-12).
>
> Brandon, I have a hunch that pretty much all of the xstrdup(real_path())
> -> real_pathdup() sites have a problem now. The previous contract was that
> real_path() would die() if the passed path is invalid. The new contract is
> that real_pathdup() returns NULL in such a case. I believe that the
> following call sites are problematic in particular:
>
> builtin/init-db.c: init_db():
>         char *original_git_dir = real_pathdup(git_dir);
>
> builtin/init-db.c: cmd_init_db():
>         real_git_dir = real_pathdup(real_git_dir);
>         ...
>         git_work_tree_cfg = real_pathdup(rel);
>
> environment.c: set_git_work_tree():
>         work_tree = real_pathdup(new_work_tree);
>
> setup.c: setup_discovered_git_dir():
>         gitdir = real_pathdup(gitdir);
>
> submodule.c: connect_work_tree_and_git_dir():
>         const char *real_work_tree = real_pathdup(work_tree);
>
> transport.c: refs_from_alternate_cb():
>         other = real_pathdup(e->path);
>
> worktree.c: find_worktree():
>         path = real_pathdup(arg);
>
> I verified that all calls are still there, except for the submodule.c one
> which simply moved to dir.c and the transport.c one which apparently now
> no longer die()s but simply ignores non-existing paths now.
>
> That leaves six places to patch, methinks... This diff may serve as an
> initial version, but I have not really had a deep look at all call sites
> (and it is an unwise idea to trust me at this hour anyway, look at the
> time when I sent this mail):
>
> -- snipsnap --
> diff --git a/abspath.c b/abspath.c
> index 2f0c26e0e2c..b02e068aa34 100644
> --- a/abspath.c
> +++ b/abspath.c
> @@ -214,12 +214,12 @@ const char *real_path_if_valid(const char *path)
>         return strbuf_realpath(&realpath, path, 0);
>  }
>
> -char *real_pathdup(const char *path)
> +char *real_pathdup(const char *path, int die_on_error)
>  {
>         struct strbuf realpath = STRBUF_INIT;
>         char *retval = NULL;
>
> -       if (strbuf_realpath(&realpath, path, 0))
> +       if (strbuf_realpath(&realpath, path, die_on_error))
>                 retval = strbuf_detach(&realpath, NULL);
>
>         strbuf_release(&realpath);
> diff --git a/builtin/init-db.c b/builtin/init-db.c
> index 1d4d6a00789..8a6acb0ec69 100644
> --- a/builtin/init-db.c
> +++ b/builtin/init-db.c
> @@ -338,7 +338,7 @@ int init_db(const char *git_dir, const char *real_git_dir,
>  {
>         int reinit;
>         int exist_ok = flags & INIT_DB_EXIST_OK;
> -       char *original_git_dir = real_pathdup(git_dir);
> +       char *original_git_dir = real_pathdup(git_dir, 1);
>
>         if (real_git_dir) {
>                 struct stat st;
> @@ -489,7 +489,7 @@ int cmd_init_db(int argc, const char **argv, const char *prefix)
>         argc = parse_options(argc, argv, prefix, init_db_options, init_db_usage, 0);
>
>         if (real_git_dir && !is_absolute_path(real_git_dir))
> -               real_git_dir = real_pathdup(real_git_dir);
> +               real_git_dir = real_pathdup(real_git_dir, 1);
>
>         if (argc == 1) {
>                 int mkdir_tried = 0;
> @@ -560,7 +560,7 @@ int cmd_init_db(int argc, const char **argv, const char *prefix)
>                 const char *git_dir_parent = strrchr(git_dir, '/');
>                 if (git_dir_parent) {
>                         char *rel = xstrndup(git_dir, git_dir_parent - git_dir);
> -                       git_work_tree_cfg = real_pathdup(rel);
> +                       git_work_tree_cfg = real_pathdup(rel, 1);
>                         free(rel);
>                 }
>                 if (!git_work_tree_cfg)
> diff --git a/cache.h b/cache.h
> index e7b57457e73..7168c1e5ff0 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -1160,7 +1160,7 @@ char *strbuf_realpath(struct strbuf *resolved, const char *path,
>                       int die_on_error);
>  const char *real_path(const char *path);
>  const char *real_path_if_valid(const char *path);
> -char *real_pathdup(const char *path);
> +char *real_pathdup(const char *path, int die_on_error);
>  const char *absolute_path(const char *path);
>  char *absolute_pathdup(const char *path);
>  const char *remove_leading_path(const char *in, const char *prefix);
> diff --git a/dir.c b/dir.c
> index 4541f9e1460..aeeb5ce1049 100644
> --- a/dir.c
> +++ b/dir.c
> @@ -2730,8 +2730,8 @@ void connect_work_tree_and_git_dir(const char *work_tree_, const char *git_dir_)
>  {
>         struct strbuf file_name = STRBUF_INIT;
>         struct strbuf rel_path = STRBUF_INIT;
> -       char *git_dir = real_pathdup(git_dir_);
> -       char *work_tree = real_pathdup(work_tree_);
> +       char *git_dir = real_pathdup(git_dir_, 1);
> +       char *work_tree = real_pathdup(work_tree_, 1);
>
>         /* Update gitfile */
>         strbuf_addf(&file_name, "%s/.git", work_tree);
> diff --git a/environment.c b/environment.c
> index c07fb17fb70..42dc3106d2f 100644
> --- a/environment.c
> +++ b/environment.c
> @@ -259,7 +259,7 @@ void set_git_work_tree(const char *new_work_tree)
>                 return;
>         }
>         git_work_tree_initialized = 1;
> -       work_tree = real_pathdup(new_work_tree);
> +       work_tree = real_pathdup(new_work_tree, 1);
>  }
>
>  const char *get_git_work_tree(void)
> diff --git a/setup.c b/setup.c
> index 9118b48590a..d51549a6de3 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -698,7 +698,7 @@ static const char *setup_discovered_git_dir(const char *gitdir,
>         /* --work-tree is set without --git-dir; use discovered one */
>         if (getenv(GIT_WORK_TREE_ENVIRONMENT) || git_work_tree_cfg) {
>                 if (offset != cwd->len && !is_absolute_path(gitdir))
> -                       gitdir = real_pathdup(gitdir);
> +                       gitdir = real_pathdup(gitdir, 1);
>                 if (chdir(cwd->buf))
>                         die_errno("Could not come back to cwd");
>                 return setup_explicit_git_dir(gitdir, cwd, nongit_ok);
> @@ -808,7 +808,7 @@ static int canonicalize_ceiling_entry(struct string_list_item *item,
>                 /* Keep entry but do not canonicalize it */
>                 return 1;
>         } else {
> -               char *real_path = real_pathdup(ceil);
> +               char *real_path = real_pathdup(ceil, 0);
>                 if (!real_path) {
>                         return 0;
>                 }
> diff --git a/submodule.c b/submodule.c
> index 3b98766a6bc..1d4c0ce86ee 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -1403,7 +1403,7 @@ static void relocate_single_git_dir_into_superproject(const char *prefix,
>                 /* If it is an actual gitfile, it doesn't need migration. */
>                 return;
>
> -       real_old_git_dir = real_pathdup(old_git_dir);
> +       real_old_git_dir = real_pathdup(old_git_dir, 0);
>
>         sub = submodule_from_path(null_sha1, path);
>         if (!sub)
> @@ -1412,7 +1412,7 @@ static void relocate_single_git_dir_into_superproject(const char *prefix,
>         new_git_dir = git_path("modules/%s", sub->name);
>         if (safe_create_leading_directories_const(new_git_dir) < 0)
>                 die(_("could not create directory '%s'"), new_git_dir);
> -       real_new_git_dir = real_pathdup(new_git_dir);
> +       real_new_git_dir = real_pathdup(new_git_dir, 0);
>
>         if (!prefix)
>                 prefix = get_super_prefix();
> @@ -1472,14 +1472,14 @@ void absorb_git_dir_into_superproject(const char *prefix,
>                 new_git_dir = git_path("modules/%s", sub->name);
>                 if (safe_create_leading_directories_const(new_git_dir) < 0)
>                         die(_("could not create directory '%s'"), new_git_dir);
> -               real_new_git_dir = real_pathdup(new_git_dir);
> +               real_new_git_dir = real_pathdup(new_git_dir, 0);
>                 connect_work_tree_and_git_dir(path, real_new_git_dir);
>
>                 free(real_new_git_dir);
>         } else {
>                 /* Is it already absorbed into the superprojects git dir? */
> -               char *real_sub_git_dir = real_pathdup(sub_git_dir);
> -               char *real_common_git_dir = real_pathdup(get_git_common_dir());
> +               char *real_sub_git_dir = real_pathdup(sub_git_dir, 0);
> +               char *real_common_git_dir = real_pathdup(get_git_common_dir(), 0);
>
>                 if (!starts_with(real_sub_git_dir, real_common_git_dir))
>                         relocate_single_git_dir_into_superproject(prefix, path);
> diff --git a/worktree.c b/worktree.c
> index d633761575b..0486e31ad4a 100644
> --- a/worktree.c
> +++ b/worktree.c
> @@ -255,7 +255,7 @@ struct worktree *find_worktree(struct worktree **list,
>                 return wt;
>
>         arg = prefix_filename(prefix, strlen(prefix), arg);
> -       path = real_pathdup(arg);
> +       path = real_pathdup(arg, 1);
>         for (; *list; list++)
>                 if (!fspathcmp(path, real_path((*list)->path)))
>                         break;

Re: Crash on MSYS2 with GIT_WORK_TREE

[Brandon Williams]

On 03/08, Johannes Schindelin wrote:
> Hi valtron,
> 
> On Wed, 8 Mar 2017, Johannes Schindelin wrote:
> 
> > On Tue, 7 Mar 2017, valtron wrote:
> > 
> > > When GIT_WORK_TREE contains a drive-letter and forward-slashes, some git
> > > commands crash:
> > > 
> > > C:\repo>set GIT_WORK_TREE=C:/repo
> > > C:\repo>git rev-parse HEAD
> > >      1 [main] git 2332 cygwin_exception::open_stackdumpfile: Dumping
> > > stack trace to git.exe.stackdump
> > 
> > [...]
> >
> > In any case, this problem is squarely within the MSYS2 runtime. It has
> > nothing to do with Git except for the motivation to set an environment
> > variable to an absolute path as you outlined.
> 
> Oh boy was I *wrong*! I take that back and apologize for my premature
> verdict.
> 
> It is true that you should not set GIT_WORKTREE=c:/repo if you want to
> work with MSYS2 Git because MSYS2 expects pseudo Unix paths, i.e. /c/repo,
> and it will simply try to guess correctly and convert Windows paths with
> drive letters and backslashes to that form.
> 
> But that does not excuse a crash.
> 
> The problem is actually even worse: On *Linux*, this happens:
> 
> 	$ GIT_WORK_TREE=c:/invalid git rev-parse HEAD
> 	Segmentation fault (core dumped)
> 
> The reason is this: when set_git_work_tree() was converted from using
> xstrdup(real_path()) to real_pathdup(), we completely missed the fact that
> the former passed die_on_error = 1 to strbuf_realpath(), while the latter
> passed die_on_error = 0. As a consequence, work_tree can be NULL now, and
> the current code does not expect set_git_work_tree() to return
> successfully after setting work_tree to NULL.
> 
> I Cc:ed Brandon, the author of 4ac9006f832 (real_path: have callers use
> real_pathdup and strbuf_realpath, 2016-12-12).
> 
> Brandon, I have a hunch that pretty much all of the xstrdup(real_path())
> -> real_pathdup() sites have a problem now. The previous contract was that
> real_path() would die() if the passed path is invalid. The new contract is
> that real_pathdup() returns NULL in such a case. I believe that the
> following call sites are problematic in particular:

Welp, looks like I missed that when I made the conversion.  You're
right, the semantics of getting the real_path were changed which would
cause a NULL to be returned instead of the program exiting with a call
to die().  

After a cursory look at your patch, I think all of your changes look
sane.  I would have to take a closer look at the call sites to see if
each caller would need to die or not.  I'm assuming you took a quick
glace to make your decision about each call site?

> 
> builtin/init-db.c: init_db():
> 	char *original_git_dir = real_pathdup(git_dir);
> 
> builtin/init-db.c: cmd_init_db():
> 	real_git_dir = real_pathdup(real_git_dir);
> 	...
> 	git_work_tree_cfg = real_pathdup(rel);
> 
> environment.c: set_git_work_tree():
> 	work_tree = real_pathdup(new_work_tree);
> 
> setup.c: setup_discovered_git_dir():
> 	gitdir = real_pathdup(gitdir);
> 
> submodule.c: connect_work_tree_and_git_dir():
> 	const char *real_work_tree = real_pathdup(work_tree);
> 
> transport.c: refs_from_alternate_cb():
> 	other = real_pathdup(e->path);
> 
> worktree.c: find_worktree():
> 	path = real_pathdup(arg);
> 
> I verified that all calls are still there, except for the submodule.c one
> which simply moved to dir.c and the transport.c one which apparently now
> no longer die()s but simply ignores non-existing paths now.
> 
> That leaves six places to patch, methinks... This diff may serve as an
> initial version, but I have not really had a deep look at all call sites
> (and it is an unwise idea to trust me at this hour anyway, look at the
> time when I sent this mail):
> 
> -- snipsnap --
> diff --git a/abspath.c b/abspath.c
> index 2f0c26e0e2c..b02e068aa34 100644
> --- a/abspath.c
> +++ b/abspath.c
> @@ -214,12 +214,12 @@ const char *real_path_if_valid(const char *path)
>  	return strbuf_realpath(&realpath, path, 0);
>  }
>  
> -char *real_pathdup(const char *path)
> +char *real_pathdup(const char *path, int die_on_error)
>  {
>  	struct strbuf realpath = STRBUF_INIT;
>  	char *retval = NULL;
>  
> -	if (strbuf_realpath(&realpath, path, 0))
> +	if (strbuf_realpath(&realpath, path, die_on_error))
>  		retval = strbuf_detach(&realpath, NULL);
>  
>  	strbuf_release(&realpath);
> diff --git a/builtin/init-db.c b/builtin/init-db.c
> index 1d4d6a00789..8a6acb0ec69 100644
> --- a/builtin/init-db.c
> +++ b/builtin/init-db.c
> @@ -338,7 +338,7 @@ int init_db(const char *git_dir, const char *real_git_dir,
>  {
>  	int reinit;
>  	int exist_ok = flags & INIT_DB_EXIST_OK;
> -	char *original_git_dir = real_pathdup(git_dir);
> +	char *original_git_dir = real_pathdup(git_dir, 1);
>  
>  	if (real_git_dir) {
>  		struct stat st;
> @@ -489,7 +489,7 @@ int cmd_init_db(int argc, const char **argv, const char *prefix)
>  	argc = parse_options(argc, argv, prefix, init_db_options, init_db_usage, 0);
>  
>  	if (real_git_dir && !is_absolute_path(real_git_dir))
> -		real_git_dir = real_pathdup(real_git_dir);
> +		real_git_dir = real_pathdup(real_git_dir, 1);
>  
>  	if (argc == 1) {
>  		int mkdir_tried = 0;
> @@ -560,7 +560,7 @@ int cmd_init_db(int argc, const char **argv, const char *prefix)
>  		const char *git_dir_parent = strrchr(git_dir, '/');
>  		if (git_dir_parent) {
>  			char *rel = xstrndup(git_dir, git_dir_parent - git_dir);
> -			git_work_tree_cfg = real_pathdup(rel);
> +			git_work_tree_cfg = real_pathdup(rel, 1);
>  			free(rel);
>  		}
>  		if (!git_work_tree_cfg)
> diff --git a/cache.h b/cache.h
> index e7b57457e73..7168c1e5ff0 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -1160,7 +1160,7 @@ char *strbuf_realpath(struct strbuf *resolved, const char *path,
>  		      int die_on_error);
>  const char *real_path(const char *path);
>  const char *real_path_if_valid(const char *path);
> -char *real_pathdup(const char *path);
> +char *real_pathdup(const char *path, int die_on_error);
>  const char *absolute_path(const char *path);
>  char *absolute_pathdup(const char *path);
>  const char *remove_leading_path(const char *in, const char *prefix);
> diff --git a/dir.c b/dir.c
> index 4541f9e1460..aeeb5ce1049 100644
> --- a/dir.c
> +++ b/dir.c
> @@ -2730,8 +2730,8 @@ void connect_work_tree_and_git_dir(const char *work_tree_, const char *git_dir_)
>  {
>  	struct strbuf file_name = STRBUF_INIT;
>  	struct strbuf rel_path = STRBUF_INIT;
> -	char *git_dir = real_pathdup(git_dir_);
> -	char *work_tree = real_pathdup(work_tree_);
> +	char *git_dir = real_pathdup(git_dir_, 1);
> +	char *work_tree = real_pathdup(work_tree_, 1);
>  
>  	/* Update gitfile */
>  	strbuf_addf(&file_name, "%s/.git", work_tree);
> diff --git a/environment.c b/environment.c
> index c07fb17fb70..42dc3106d2f 100644
> --- a/environment.c
> +++ b/environment.c
> @@ -259,7 +259,7 @@ void set_git_work_tree(const char *new_work_tree)
>  		return;
>  	}
>  	git_work_tree_initialized = 1;
> -	work_tree = real_pathdup(new_work_tree);
> +	work_tree = real_pathdup(new_work_tree, 1);
>  }
>  
>  const char *get_git_work_tree(void)
> diff --git a/setup.c b/setup.c
> index 9118b48590a..d51549a6de3 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -698,7 +698,7 @@ static const char *setup_discovered_git_dir(const char *gitdir,
>  	/* --work-tree is set without --git-dir; use discovered one */
>  	if (getenv(GIT_WORK_TREE_ENVIRONMENT) || git_work_tree_cfg) {
>  		if (offset != cwd->len && !is_absolute_path(gitdir))
> -			gitdir = real_pathdup(gitdir);
> +			gitdir = real_pathdup(gitdir, 1);
>  		if (chdir(cwd->buf))
>  			die_errno("Could not come back to cwd");
>  		return setup_explicit_git_dir(gitdir, cwd, nongit_ok);
> @@ -808,7 +808,7 @@ static int canonicalize_ceiling_entry(struct string_list_item *item,
>  		/* Keep entry but do not canonicalize it */
>  		return 1;
>  	} else {
> -		char *real_path = real_pathdup(ceil);
> +		char *real_path = real_pathdup(ceil, 0);
>  		if (!real_path) {
>  			return 0;
>  		}
> diff --git a/submodule.c b/submodule.c
> index 3b98766a6bc..1d4c0ce86ee 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -1403,7 +1403,7 @@ static void relocate_single_git_dir_into_superproject(const char *prefix,
>  		/* If it is an actual gitfile, it doesn't need migration. */
>  		return;
>  
> -	real_old_git_dir = real_pathdup(old_git_dir);
> +	real_old_git_dir = real_pathdup(old_git_dir, 0);
>  
>  	sub = submodule_from_path(null_sha1, path);
>  	if (!sub)
> @@ -1412,7 +1412,7 @@ static void relocate_single_git_dir_into_superproject(const char *prefix,
>  	new_git_dir = git_path("modules/%s", sub->name);
>  	if (safe_create_leading_directories_const(new_git_dir) < 0)
>  		die(_("could not create directory '%s'"), new_git_dir);
> -	real_new_git_dir = real_pathdup(new_git_dir);
> +	real_new_git_dir = real_pathdup(new_git_dir, 0);
>  
>  	if (!prefix)
>  		prefix = get_super_prefix();
> @@ -1472,14 +1472,14 @@ void absorb_git_dir_into_superproject(const char *prefix,
>  		new_git_dir = git_path("modules/%s", sub->name);
>  		if (safe_create_leading_directories_const(new_git_dir) < 0)
>  			die(_("could not create directory '%s'"), new_git_dir);
> -		real_new_git_dir = real_pathdup(new_git_dir);
> +		real_new_git_dir = real_pathdup(new_git_dir, 0);
>  		connect_work_tree_and_git_dir(path, real_new_git_dir);
>  
>  		free(real_new_git_dir);
>  	} else {
>  		/* Is it already absorbed into the superprojects git dir? */
> -		char *real_sub_git_dir = real_pathdup(sub_git_dir);
> -		char *real_common_git_dir = real_pathdup(get_git_common_dir());
> +		char *real_sub_git_dir = real_pathdup(sub_git_dir, 0);
> +		char *real_common_git_dir = real_pathdup(get_git_common_dir(), 0);
>  
>  		if (!starts_with(real_sub_git_dir, real_common_git_dir))
>  			relocate_single_git_dir_into_superproject(prefix, path);
> diff --git a/worktree.c b/worktree.c
> index d633761575b..0486e31ad4a 100644
> --- a/worktree.c
> +++ b/worktree.c
> @@ -255,7 +255,7 @@ struct worktree *find_worktree(struct worktree **list,
>  		return wt;
>  
>  	arg = prefix_filename(prefix, strlen(prefix), arg);
> -	path = real_pathdup(arg);
> +	path = real_pathdup(arg, 1);
>  	for (; *list; list++)
>  		if (!fspathcmp(path, real_path((*list)->path)))
>  			break;

-- 
Brandon Williams

Re: Crash on MSYS2 with GIT_WORK_TREE

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> The problem is actually even worse: On *Linux*, this happens:
>
> 	$ GIT_WORK_TREE=c:/invalid git rev-parse HEAD
> 	Segmentation fault (core dumped)
>
> The reason is this: when set_git_work_tree() was converted from using
> xstrdup(real_path()) to real_pathdup(), we completely missed the fact that
> the former passed die_on_error = 1 to strbuf_realpath(), while the latter
> passed die_on_error = 0. As a consequence, work_tree can be NULL now, and
> the current code does not expect set_git_work_tree() to return
> successfully after setting work_tree to NULL.

Ouch.

> Brandon, I have a hunch that pretty much all of the xstrdup(real_path())
> -> real_pathdup() sites have a problem now. The previous contract was that
> real_path() would die() if the passed path is invalid. The new contract is
> that real_pathdup() returns NULL in such a case.

OK, so it appears that we'd better audit all the callsites of
real_pathdup() and see if anybody _assumes_ that the return values
are not NULL.  They all need fixing.

Thanks for digging it through to the root cause.

Re: Crash on MSYS2 with GIT_WORK_TREE

[Johannes Schindelin]

Hi Brandon,

On Tue, 7 Mar 2017, Brandon Williams wrote:

> On 03/08, Johannes Schindelin wrote:
> > 
> > [...] On *Linux*, this happens:
> > 
> > 	$ GIT_WORK_TREE=c:/invalid git rev-parse HEAD
> > 	Segmentation fault (core dumped)
> > 
> > The reason is this: when set_git_work_tree() was converted from using
> > xstrdup(real_path()) to real_pathdup(), we completely missed the fact
> > that the former passed die_on_error = 1 to strbuf_realpath(), while
> > the latter passed die_on_error = 0. As a consequence, work_tree can be
> > NULL now, and the current code does not expect set_git_work_tree() to
> > return successfully after setting work_tree to NULL.
> > 
> > I Cc:ed Brandon, the author of 4ac9006f832 (real_path: have callers
> > use real_pathdup and strbuf_realpath, 2016-12-12).
> > 
> > Brandon, I have a hunch that pretty much all of the
> > xstrdup(real_path()) -> real_pathdup() sites have a problem now. The
> > previous contract was that real_path() would die() if the passed path
> > is invalid. The new contract is that real_pathdup() returns NULL in
> > such a case. I believe that the following call sites are problematic
> > in particular:
> 
> Welp, looks like I missed that when I made the conversion.  You're
> right, the semantics of getting the real_path were changed which would
> cause a NULL to be returned instead of the program exiting with a call
> to die().  
> 
> After a cursory look at your patch, I think all of your changes look
> sane.  I would have to take a closer look at the call sites to see if
> each caller would need to die or not.  I'm assuming you took a quick
> glace to make your decision about each call site?

I did take a quick glance, but did you have a look at the time of day I
sent this patch? You do not want to trust my judgement after that.

Another thing: may I ask you to delete the quoted parts of the mail that
you are actually not responding to? Junio also often simply keeps the rest
of the mail quoted, and I always have to scroll all the way to the end
just to verify that nothing more has been said, which can be slightly
annoying when you are tired. I do plan to read your mails in the future,
so culling the quoted-yet-unanswered part would save me trouble.

Thanks,
Dscho

Re: Crash on MSYS2 with GIT_WORK_TREE

[Johannes Schindelin]

Hi valtron,


On Tue, 7 Mar 2017, valtron wrote:

> I only ran into this because of git-gui, where I eventually tracked it
> down to line 1330:
> 
>     set env(GIT_WORK_TREE) $_gitworktree

As git-gui is a Tcl script, which in turn runs as a pure Windows
application, the path should use backslashes.

> With that line commented out, it works. I'll look into why git-gui
> sets it to a windows-path-with-forward-slashes, but that's a separate
> issue from the crash.

It is... please do contribute your fix when you have one.

> Also, from the stack trace, I think git is still able to understand the
> path, since it appears to correctly convert it to /c/repo, but I might
> be wrong since I haven't look at the code.

Git does not convert the path at all. It is the *MSYS2 runtime* that
converts Windows paths to POSIX paths, if any. And it does so selectively.
The current working directory is always transformed. PATH is always
transformed. Environment variables are transformed *when they look like
Windows paths*. And I am fairly certain that a GIT_WORK_TREE with a colon
and forward-slashes fails that test: the MSYS2 runtime thinks this is not
a Windows path and leaves it alone. Enter your problem.

Ciao,
Johannes

Re: Crash on MSYS2 with GIT_WORK_TREE

[Johannes Schindelin]

Hi Junio,

On Tue, 7 Mar 2017, Junio C Hamano wrote:

> Junio C Hamano <gitster@pobox.com> writes:
> 
> > OK, so it appears that we'd better audit all the callsites of
> > real_pathdup() and see if anybody _assumes_ that the return values are
> > not NULL.  They all need fixing.

Indeed.

> I just looked at 4ac9006f ("real_path: have callers use real_pathdup and
> strbuf_realpath", 2016-12-12) and it seems all hunks that replaces
> xstrdup(real_path(...)) with real_pathdup(...) in the commit share the
> same issue.  

Right, I tried to convey that information in my email to which you
replied.

> The one in canonicalize_ceiling_entry() looks OK, though.

Yes, it immediately tests whether NULL was returned.

> ec9629b3 ("submodule absorbing: fix worktree/gitdir pointers
> recursively for non-moves", 2017-01-25) introduces a new use of
> real_pathdup() and the result is immediately used to call
> connect_work_tree_and_git_dir() without checking its NULL-ness, but
> the argument to new_git_dir is something that came from git_path()
> that was successfully passed to safe_create_leading_directories(),
> so this one should be OK.
> 
> 1c16df23 ("Merge branch 'bw/realpath-wo-chdir'", 2017-01-18) turns a
> few xstrdup(real_path(...)) in dir.c without thinking.  I think that
> evil merge probably should be reverted.

Rather than a heavy-handed reversal, I would really prefer to perform a
diligent audit of all real_pathdup() callers and adjust them
appropriately.

Turns out that the canonicalize_ceiling_entry() caller is *the only one*
handling NULL correctly. All other callers need to be changed.

Will send something out in a moment.

Ciao,
Johannes

Re: Crash on MSYS2 with GIT_WORK_TREE

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> Rather than a heavy-handed reversal, I would really prefer to perform a
> diligent audit of all real_pathdup() callers and adjust them
> appropriately.
>
> Turns out that the canonicalize_ceiling_entry() caller is *the only one*
> handling NULL correctly. All other callers need to be changed.

The observation matches what I saw.  

I would have added a patch that introduces real_pathdup_gently()
that returns NULL and made real_pathdup() die as before immediately
on top of the Brandon's series (which would ensure that any branch
that would want to use real_pathdup() would get the dying version by
default) and flipped selected callers to call the gently() version
in that patch, but what you posted is a lot more apprporiate for a
regression fix.  By changing the function signature, the patch
ensures that it covers all the callsites.

Among the two-and-half regressions known to us post 2.12 so far,
this one probably needs to go into a maintenance release without
waiting for the other one-and-half, I think.

Thanks.

Re: Crash on MSYS2 with GIT_WORK_TREE

[Brandon Williams]

On 03/08, Johannes Schindelin wrote:
> I did take a quick glance, but did you have a look at the time of day I
> sent this patch? You do not want to trust my judgement after that.

Haha Yeah I did notice, and I trust your newer patch more than the one
you sent at 2am :)

> 
> Another thing: may I ask you to delete the quoted parts of the mail that
> you are actually not responding to? Junio also often simply keeps the rest
> of the mail quoted, and I always have to scroll all the way to the end
> just to verify that nothing more has been said, which can be slightly
> annoying when you are tired. I do plan to read your mails in the future,
> so culling the quoted-yet-unanswered part would save me trouble.

Of course, I usually try to clear the parts of the mail I'm not
responding to...though there are times where I forget or am a bit lazy.
I'll definitely work on remembering to do that for the future!

-- 
Brandon Williams

Re: Crash on MSYS2 with GIT_WORK_TREE

[Junio C Hamano]

Brandon Williams <bmwill@google.com> writes:

> Of course, I usually try to clear the parts of the mail I'm not
> responding to...though there are times where I forget or am a bit lazy.
> I'll definitely work on remembering to do that for the future!

This cuts both ways.  Sometimes it is very useful to be able to see
other parts that the responder is not _directly_ responding to when
you come as a third person to the discussion, which forces you to
find messages upthread, so do not overdo it.