threads / patch / 25461

patchmerge-file: correctly find files when called in subdir

Subject: [PATCH] merge-file: correctly find files when called in subdir

## tl;dr

10 messages between Oct 16, 2010 and Oct 18, 2010. Diffs are folded; open one to read it.

replies: 9people: 4as markdown or json

Thomas Rast· Oct 16, 2010, 11:33 UTC · lore

Since b541248 (merge.conflictstyle: choose between "merge" and "diff3 -m" styles, 2008-08-29), git-merge-file uses setup_directory_gently(), thus cd'ing around to find any possible config files to use.

This broke merge-file when it is called from within a subdirectory of a repository, and the arguments are all relative paths.

Fix by prepending the prefix, as passed down from the main git executable, if there is any.

Signed-off-by: Thomas Rast <trast@student.ethz.ch>
---
 builtin/merge-file.c  |   11 ++++++++++-
 t/t6023-merge-file.sh |    8 ++++++++
 2 files changed, 18 insertions(+), 1 deletions(-)
Show changes to 2 files +18 −1

builtin/merge-file.c, t/t6023-merge-file.sh

diff --git a/builtin/merge-file.c b/builtin/merge-file.c
index b6664d4..b873fee 100644
--- a/builtin/merge-file.c
+++ b/builtin/merge-file.c
@@ -28,6 +28,7 @@ int cmd_merge_file(int argc, const char **argv, const char *prefix)
 	xmparam_t xmp = {{0}};
 	int ret = 0, i = 0, to_stdout = 0;
 	int quiet = 0;
+	int prefixlen;
 	struct option options[] = {
 		OPT_BOOLEAN('p', "stdout", &to_stdout, "send results to standard output"),
 		OPT_SET_INT(0, "diff3", &xmp.style, "use a diff3 based merge", XDL_MERGE_DIFF3),
@@ -65,10 +66,18 @@ int cmd_merge_file(int argc, const char **argv, const char *prefix)
 				     "%s\n", strerror(errno));
 	}
 
+	if (prefix)
+		prefixlen = strlen(prefix);
+
 	for (i = 0; i < 3; i++) {
+		const char *name;
+		if (prefix)
+			name = prefix_filename(prefix, prefixlen, argv[i]);
+		else
+			name = argv[i];
 		if (!names[i])
 			names[i] = argv[i];
-		if (read_mmfile(mmfs + i, argv[i]))
+		if (read_mmfile(mmfs + i, name))
 			return -1;
 		if (buffer_is_binary(mmfs[i].ptr, mmfs[i].size))
 			return error("Cannot merge binary files: %s\n",
diff --git a/t/t6023-merge-file.sh b/t/t6023-merge-file.sh
index d486d73..d9f3439 100755
--- a/t/t6023-merge-file.sh
+++ b/t/t6023-merge-file.sh
@@ -64,6 +64,14 @@ cp new1.txt test.txt
 test_expect_success "merge without conflict" \
 	"git merge-file test.txt orig.txt new2.txt"
 
+test_expect_success 'works in subdirectory' '
+	mkdir dir &&
+	cp new1.txt dir/a.txt &&
+	cp orig.txt dir/o.txt &&
+	cp new2.txt dir/b.txt &&
+	( cd dir && git merge-file a.txt o.txt b.txt )
+'
+
 cp new1.txt test.txt
 test_expect_success "merge without conflict (--quiet)" \
 	"git merge-file --quiet test.txt orig.txt new2.txt"

-- 
1.7.3.1.266.g3c065
Bert Wesarg· Oct 16, 2010, 13:30 UTC · re: Thomas Rast · lore

Re: [PATCH] merge-file: correctly find files when called in subdir

On Sat, Oct 16, 2010 at 13:33, Thomas Rast <trast@student.ethz.ch> wrote:
Show 33 quoted lines
> Since b541248 (merge.conflictstyle: choose between "merge" and "diff3
> -m" styles, 2008-08-29), git-merge-file uses setup_directory_gently(),
> thus cd'ing around to find any possible config files to use.
>
> This broke merge-file when it is called from within a subdirectory of
> a repository, and the arguments are all relative paths.
>
> Fix by prepending the prefix, as passed down from the main git
> executable, if there is any.
>
> Signed-off-by: Thomas Rast <trast@student.ethz.ch>
> ---
>  builtin/merge-file.c  |   11 ++++++++++-
>  t/t6023-merge-file.sh |    8 ++++++++
>  2 files changed, 18 insertions(+), 1 deletions(-)
>
> diff --git a/builtin/merge-file.c b/builtin/merge-file.c
> index b6664d4..b873fee 100644
> --- a/builtin/merge-file.c
> +++ b/builtin/merge-file.c
> @@ -65,10 +66,18 @@ int cmd_merge_file(int argc, const char **argv, const char *prefix)
>                                     "%s\n", strerror(errno));
>        }
>
> +       if (prefix)
> +               prefixlen = strlen(prefix);
> +
>        for (i = 0; i < 3; i++) {
> +               const char *name;
> +               if (prefix)
> +                       name = prefix_filename(prefix, prefixlen, argv[i]);
> +               else
> +                       name = argv[i];
I think you can safe this condition, if you set prefixlen to 0.
Bert
Jonathan Nieder· Oct 17, 2010, 01:11 UTC · re: Bert Wesarg · lore

Re: [PATCH] merge-file: correctly find files when called in subdir

Bert Wesarg wrote:
> On Sat, Oct 16, 2010 at 13:33, Thomas Rast <trast@student.ethz.ch> wrote:
>> --- a/builtin/merge-file.c
>> +++ b/builtin/merge-file.c
>> @@ -65,10 +66,18 @@ int cmd_merge_file(int argc, const char **argv, const char *prefix)
[...]
Show 6 quoted lines
>> +               if (prefix)
>> +                       name = prefix_filename(prefix, prefixlen, argv[i]);
>> +               else
>> +                       name = argv[i];
>
> I think you can safe this condition, if you set prefixlen to 0.
I'm not so sure.  On Windows, prefix_filename() starts with
	char *p;
	/* don't add prefix to absolute paths, but still replace '\' by '/' */
	if (is_absolute_path(arg))
		pfx_len = 0;
	else
		memcpy(path, pfx, pfx_len);

and memcpy() has undefined behavior when pfx is NULL (even with pfx_len of 0; see WG14/N1256: 7.21.1 "String function conventions"). Even if the win32api memcpy does the right thing with NULL pfx, I cannot trust GCC when they are given that kind of license. :)

Thomas Rast· Oct 17, 2010, 10:39 UTC · re: Jonathan Nieder · lore

Re: [PATCH] merge-file: correctly find files when called in subdir

Jonathan Nieder wrote:
Show 27 quoted lines
> Bert Wesarg wrote:
> > On Sat, Oct 16, 2010 at 13:33, Thomas Rast <trast@student.ethz.ch> wrote:
> 
> >> --- a/builtin/merge-file.c
> >> +++ b/builtin/merge-file.c
> >> @@ -65,10 +66,18 @@ int cmd_merge_file(int argc, const char **argv, const char *prefix)
> [...]
> >> +               if (prefix)
> >> +                       name = prefix_filename(prefix, prefixlen, argv[i]);
> >> +               else
> >> +                       name = argv[i];
> >
> > I think you can safe this condition, if you set prefixlen to 0.
> 
> I'm not so sure.  On Windows, prefix_filename() starts with
> 
> 	char *p;
> 	/* don't add prefix to absolute paths, but still replace '\' by '/' */
> 	if (is_absolute_path(arg))
> 		pfx_len = 0;
> 	else
> 		memcpy(path, pfx, pfx_len);
> 
> and memcpy() has undefined behavior when pfx is NULL (even with
> pfx_len of 0; see WG14/N1256: 7.21.1 "String function conventions").
> Even if the win32api memcpy does the right thing with NULL pfx, I
> cannot trust GCC when they are given that kind of license. :)
Hmmh.

I was ready to say "you're right" to Bert's suggestion. So maybe, for the sake of foolproof-ness, we should do something like the patch below?

Show changes to diff +1 −1
diff --git i/setup.c w/setup.c
index a3b76de..7c8906d 100644
--- i/setup.c
+++ w/setup.c
@@ -55,7 +55,7 @@
 	/* don't add prefix to absolute paths, but still replace '\' by '/' */
 	if (is_absolute_path(arg))
 		pfx_len = 0;
-	else
+	else if (pfx && *pfx)
 		memcpy(path, pfx, pfx_len);
 	strcpy(path + pfx_len, arg);
 	for (p = path + pfx_len; *p; p++)

-- 
Thomas Rast
trast@{inf,student}.ethz.ch
Jonathan Nieder· Oct 17, 2010, 16:05 UTC · re: Thomas Rast · lore

Re: [PATCH] merge-file: correctly find files when called in subdir

Thomas Rast wrote:
Show 17 quoted lines
> I was ready to say "you're right" to Bert's suggestion.  So maybe, for
> the sake of foolproof-ness, we should do something like the patch
> below?
> 
> diff --git i/setup.c w/setup.c
> index a3b76de..7c8906d 100644
> --- i/setup.c
> +++ w/setup.c
> @@ -55,7 +55,7 @@
>  	/* don't add prefix to absolute paths, but still replace '\' by '/' */
>  	if (is_absolute_path(arg))
>  		pfx_len = 0;
> -	else
> +	else if (pfx && *pfx)
>  		memcpy(path, pfx, pfx_len);
>  	strcpy(path + pfx_len, arg);
>  	for (p = path + pfx_len; *p; p++)
Yes, please!  But I'd write it as
	if (pfx_len)
		memcpy(...)

and rely on the commit message to explain that this is about the !pfx case (so callers with pfx_len == 3, !pfx get a nice, quick segfault instead of heisenbugs from uninitialized data).

Thomas Rast· Oct 17, 2010, 19:23 UTC · re: Jonathan Nieder · lore

[PATCH v2 1/2] prefix_filename(): safely handle the case where pfx_len=0

Current prefix_filename() is proofed against the case where the prefix 'pfx' is NULL or a 0-length string, _except on Windows_.

Change the behaviour to work the same on both platforms, and only check pfx_len so that callers passing a NULL prefix with a nonzero pfx_len segfault early on both.

Signed-off-by: Thomas Rast <trast@student.ethz.ch>
---
Jonathan Nieder wrote:
Show 8 quoted lines
> Yes, please!  But I'd write it as
> 
>         if (pfx_len)
>                 memcpy(...)
> 
> and rely on the commit message to explain that this is about the !pfx
> case (so callers with pfx_len == 3, !pfx get a nice, quick segfault
> instead of heisenbugs from uninitialized data).

Hrm, why not. I think symmetry is important here, so I also changed the non-Windows arm, which feels wrong somehow. But it didn't break any tests.

In writing this I also realised that "it didn't break any tests" is pretty weak and checked them all by hand (there are only a dozen grep matches).

 setup.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)
Show changes to setup.c +2 −2
diff --git a/setup.c b/setup.c
index a3b76de..346ef2e 100644
--- a/setup.c
+++ b/setup.c
@@ -46,7 +46,7 @@
 {
 	static char path[PATH_MAX];
 #ifndef WIN32
-	if (!pfx || !*pfx || is_absolute_path(arg))
+	if (!pfx_len || is_absolute_path(arg))
 		return arg;
 	memcpy(path, pfx, pfx_len);
 	strcpy(path + pfx_len, arg);
@@ -55,7 +55,7 @@
 	/* don't add prefix to absolute paths, but still replace '\' by '/' */
 	if (is_absolute_path(arg))
 		pfx_len = 0;
-	else
+	else if (pfx_len)
 		memcpy(path, pfx, pfx_len);
 	strcpy(path + pfx_len, arg);
 	for (p = path + pfx_len; *p; p++)

-- 
1.7.3.1.266.g3c065
Thomas Rast· Oct 17, 2010, 19:23 UTC · re: Thomas Rast · lore

[PATCH v2 2/2] merge-file: correctly find files when called in subdir

Since b541248 (merge.conflictstyle: choose between "merge" and "diff3 -m" styles, 2008-08-29), git-merge-file uses setup_directory_gently(), thus cd'ing around to find any possible config files to use.

This broke merge-file when it is called from within a subdirectory of a repository, and the arguments are all relative paths.

Fix by prepending the prefix, as passed down from the main git setup code, if there is any.

Signed-off-by: Thomas Rast <trast@student.ethz.ch>
---

With the last patch, we can safely pass a zero prefix length to say "there was no prefix".

This is a slight semantic change on Windows, because it now substitutes / for \ in paths, but I suppose that's harmless?

 builtin/merge-file.c  |    7 ++++++-
 t/t6023-merge-file.sh |    8 ++++++++
 2 files changed, 14 insertions(+), 1 deletions(-)
Show changes to 2 files +14 −1

builtin/merge-file.c, t/t6023-merge-file.sh

diff --git a/builtin/merge-file.c b/builtin/merge-file.c
index b6664d4..6c4afb5 100644
--- a/builtin/merge-file.c
+++ b/builtin/merge-file.c
@@ -28,6 +28,7 @@ int cmd_merge_file(int argc, const char **argv, const char *prefix)
 	xmparam_t xmp = {{0}};
 	int ret = 0, i = 0, to_stdout = 0;
 	int quiet = 0;
+	int prefixlen = 0;
 	struct option options[] = {
 		OPT_BOOLEAN('p', "stdout", &to_stdout, "send results to standard output"),
 		OPT_SET_INT(0, "diff3", &xmp.style, "use a diff3 based merge", XDL_MERGE_DIFF3),
@@ -65,10 +66,14 @@ int cmd_merge_file(int argc, const char **argv, const char *prefix)
 				     "%s\n", strerror(errno));
 	}
 
+	if (prefix)
+		prefixlen = strlen(prefix);
+
 	for (i = 0; i < 3; i++) {
+		const char *fname = prefix_filename(prefix, prefixlen, argv[i]);
 		if (!names[i])
 			names[i] = argv[i];
-		if (read_mmfile(mmfs + i, argv[i]))
+		if (read_mmfile(mmfs + i, fname))
 			return -1;
 		if (buffer_is_binary(mmfs[i].ptr, mmfs[i].size))
 			return error("Cannot merge binary files: %s\n",
diff --git a/t/t6023-merge-file.sh b/t/t6023-merge-file.sh
index d486d73..d9f3439 100755
--- a/t/t6023-merge-file.sh
+++ b/t/t6023-merge-file.sh
@@ -64,6 +64,14 @@ cp new1.txt test.txt
 test_expect_success "merge without conflict" \
 	"git merge-file test.txt orig.txt new2.txt"
 
+test_expect_success 'works in subdirectory' '
+	mkdir dir &&
+	cp new1.txt dir/a.txt &&
+	cp orig.txt dir/o.txt &&
+	cp new2.txt dir/b.txt &&
+	( cd dir && git merge-file a.txt o.txt b.txt )
+'
+
 cp new1.txt test.txt
 test_expect_success "merge without conflict (--quiet)" \
 	"git merge-file --quiet test.txt orig.txt new2.txt"

-- 
1.7.3.1.266.g3c065
Jonathan Nieder· Oct 17, 2010, 19:59 UTC · re: Thomas Rast · lore

Re: [PATCH v2 2/2] merge-file: correctly find files when called in subdir

Thomas Rast wrote:
> This is a slight semantic change on Windows, because it now
> substitutes / for \ in paths, but I suppose that's harmless?

Yeah, I think it doesn't matter either way. cc-ing msysgit@ just in case.

>  builtin/merge-file.c  |    7 ++++++-
>  t/t6023-merge-file.sh |    8 ++++++++
>  2 files changed, 14 insertions(+), 1 deletions(-)
FWIW I still like both patches.
Johannes Sixt· Oct 18, 2010, 06:10 UTC · re: Thomas Rast · lore

Re: [PATCH v2 2/2] merge-file: correctly find files when called in subdir

Am 10/17/2010 21:23, schrieb Thomas Rast:
Show 5 quoted lines
> With the last patch, we can safely pass a zero prefix length to say
> "there was no prefix".
> 
> This is a slight semantic change on Windows, because it now
> substitutes / for \ in paths, but I suppose that's harmless?
Yes, it should be harmless.
-- Hannes
Jonathan Nieder· Oct 17, 2010, 01:30 UTC · re: Thomas Rast · lore

Re: [PATCH] merge-file: correctly find files when called in subdir

Thomas Rast wrote:
Show 13 quoted lines
> --- a/builtin/merge-file.c
> +++ b/builtin/merge-file.c
> @@ -65,10 +66,18 @@ int cmd_merge_file(int argc, const char **argv, const char *prefix)
>  				     "%s\n", strerror(errno));
>  	}
>  
> +	if (prefix)
> +		prefixlen = strlen(prefix);
> +
>  	for (i = 0; i < 3; i++) {
> +		const char *name;
> +		if (prefix)
> +			name = prefix_filename(prefix, prefixlen, argv[i]);
Provokes a warning (silly gcc) which can be suppressed with
	int prefixlen = prefixlen;
(or = 0 to fit with the surrounding style).
Show 12 quoted lines
> --- a/t/t6023-merge-file.sh
> +++ b/t/t6023-merge-file.sh
> @@ -64,6 +64,14 @@ cp new1.txt test.txt
>  test_expect_success "merge without conflict" \
>  	"git merge-file test.txt orig.txt new2.txt"
>  
> +test_expect_success 'works in subdirectory' '
> +	mkdir dir &&
> +	cp new1.txt dir/a.txt &&
> +	cp orig.txt dir/o.txt &&
> +	cp new2.txt dir/b.txt &&
> +	( cd dir && git merge-file a.txt o.txt b.txt )
Thanks!  Here's a backport to maint-1.6.1 for kicks.
-- 8< --
From: Thomas Rast <trast@student.ethz.ch>
Subject: merge-file: correctly find files when called in subdir

Since b541248 (merge.conflictstyle: choose between "merge" and "diff3 -m" styles, 2008-08-29), git-merge-file uses setup_directory_gently(), thus cd'ing around to find any possible config files to use.

This broke merge-file when it is called from within a subdirectory of a repository, and the arguments are all relative paths.

Fix by prepending the prefix, as passed down from the main git executable, if there is any.

Signed-off-by: Thomas Rast <trast@student.ethz.ch>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 builtin-merge-file.c  |   11 ++++++++++-
 t/t6023-merge-file.sh |    8 ++++++++
 2 files changed, 18 insertions(+), 1 deletions(-)
Show changes to 2 files +18 −1

builtin-merge-file.c, t/t6023-merge-file.sh

diff --git a/builtin-merge-file.c b/builtin-merge-file.c
index 96edb97..848e3de 100644
--- a/builtin-merge-file.c
+++ b/builtin-merge-file.c
@@ -27,6 +27,7 @@ int cmd_merge_file(int argc, const char **argv, const char *prefix)
 	mmbuffer_t result = {NULL, 0};
 	xpparam_t xpp = {XDF_NEED_MINIMAL};
 	int ret = 0, i = 0, to_stdout = 0;
+	int prefixlen = 0;
 	int merge_level = XDL_MERGE_ZEALOUS_ALNUM;
 	int merge_style = 0, quiet = 0;
 	int nongit;
@@ -57,10 +58,18 @@ int cmd_merge_file(int argc, const char **argv, const char *prefix)
 				     "%s\n", strerror(errno));
 	}
 
+	if (prefix)
+		prefixlen = strlen(prefix);
+
 	for (i = 0; i < 3; i++) {
+		const char *name;
+		if (prefix)
+			name = prefix_filename(prefix, prefixlen, argv[i]);
+		else
+			name = argv[i];
 		if (!names[i])
 			names[i] = argv[i];
-		if (read_mmfile(mmfs + i, argv[i]))
+		if (read_mmfile(mmfs + i, name))
 			return -1;
 		if (buffer_is_binary(mmfs[i].ptr, mmfs[i].size))
 			return error("Cannot merge binary files: %s\n",
diff --git a/t/t6023-merge-file.sh b/t/t6023-merge-file.sh
index f8942bc..f09430a 100755
--- a/t/t6023-merge-file.sh
+++ b/t/t6023-merge-file.sh
@@ -58,6 +58,14 @@ cp new1.txt test.txt
 test_expect_success "merge without conflict" \
 	"git merge-file test.txt orig.txt new2.txt"
 
+test_expect_success 'works in subdirectory' '
+	mkdir dir &&
+	cp new1.txt dir/a.txt &&
+	cp orig.txt dir/o.txt &&
+	cp new2.txt dir/b.txt &&
+	( cd dir && git merge-file a.txt o.txt b.txt )
+'
+
 cp new1.txt test2.txt
 test_expect_success "merge without conflict (missing LF at EOF)" \
 	"git merge-file test2.txt orig.txt new2.txt"

-- 
1.7.2.3

← back to recent threads