# [PATCH] merge-file: correctly find files when called in subdir 10 messages from 2010-10-16 to 2010-10-18. Participants: Thomas Rast, Bert Wesarg, Jonathan Nieder, Johannes Sixt. Thread: https://gitlist.dev/t/25461 ## Thomas Rast, 2010-10-16 11:33 Subject: [PATCH] merge-file: correctly find files when called in subdir Message-ID: <33ab2f03ed522e1a9be202017b7bbfe35e6d7a99.1287228637.git.trast@student.ethz.ch> URL: https://gitlist.dev/e/33ab2f03ed522e1a9be202017b7bbfe35e6d7a99.1287228637.git.trast%40student.ethz.ch ``` 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 --- 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 @@ -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, 2010-10-16 13:30 Subject: Re: [PATCH] merge-file: correctly find files when called in subdir Message-ID: URL: https://gitlist.dev/e/AANLkTimzuF3Ku_Jjnq0qKFtrDfAfz8YxChomZr8%2BShm1%40mail.gmail.com In-Reply-To: <33ab2f03ed522e1a9be202017b7bbfe35e6d7a99.1287228637.git.trast@student.ethz.ch> ``` On Sat, Oct 16, 2010 at 13:33, Thomas Rast wrote: > 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 > --- >  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, 2010-10-17 01:11 Subject: Re: [PATCH] merge-file: correctly find files when called in subdir Message-ID: <20101017011111.GA26656@burratino> URL: https://gitlist.dev/e/20101017011111.GA26656%40burratino In-Reply-To: ``` Bert Wesarg wrote: > On Sat, Oct 16, 2010 at 13:33, Thomas Rast 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. :) ``` ## Jonathan Nieder, 2010-10-17 01:30 Subject: Re: [PATCH] merge-file: correctly find files when called in subdir Message-ID: <20101017013005.GB26656@burratino> URL: https://gitlist.dev/e/20101017013005.GB26656%40burratino In-Reply-To: <33ab2f03ed522e1a9be202017b7bbfe35e6d7a99.1287228637.git.trast@student.ethz.ch> ``` Thomas Rast 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) > "%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). > --- 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 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 Signed-off-by: Jonathan Nieder --- 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 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 ``` ## Thomas Rast, 2010-10-17 10:39 Subject: Re: [PATCH] merge-file: correctly find files when called in subdir Message-ID: <201010171239.13990.trast@student.ethz.ch> URL: https://gitlist.dev/e/201010171239.13990.trast%40student.ethz.ch In-Reply-To: <20101017011111.GA26656@burratino> ``` Jonathan Nieder wrote: > Bert Wesarg wrote: > > On Sat, Oct 16, 2010 at 13:33, Thomas Rast 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? 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, 2010-10-17 16:05 Subject: Re: [PATCH] merge-file: correctly find files when called in subdir Message-ID: <20101017160546.GD301@burratino> URL: https://gitlist.dev/e/20101017160546.GD301%40burratino In-Reply-To: <201010171239.13990.trast@student.ethz.ch> ``` Thomas Rast wrote: > 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, 2010-10-17 19:23 Subject: [PATCH v2 1/2] prefix_filename(): safely handle the case where pfx_len=0 Message-ID: <64b470380b8f7bd8afe8f019553cb99f72091c7d.1287342969.git.trast@student.ethz.ch> URL: https://gitlist.dev/e/64b470380b8f7bd8afe8f019553cb99f72091c7d.1287342969.git.trast%40student.ethz.ch In-Reply-To: <20101017160546.GD301@burratino> ``` 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 --- Jonathan Nieder wrote: > 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(-) 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, 2010-10-17 19:23 Subject: [PATCH v2 2/2] merge-file: correctly find files when called in subdir Message-ID: URL: https://gitlist.dev/e/d0e540fdced31557e983d7503da3dcb75c622dc9.1287342969.git.trast%40student.ethz.ch In-Reply-To: <64b470380b8f7bd8afe8f019553cb99f72091c7d.1287342969.git.trast@student.ethz.ch> ``` 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 --- 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(-) 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, 2010-10-17 19:59 Subject: Re: [PATCH v2 2/2] merge-file: correctly find files when called in subdir Message-ID: <20101017195914.GA3434@burratino> URL: https://gitlist.dev/e/20101017195914.GA3434%40burratino In-Reply-To: ``` 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, 2010-10-18 06:10 Subject: Re: [PATCH v2 2/2] merge-file: correctly find files when called in subdir Message-ID: <4CBBE4DB.6080307@viscovery.net> URL: https://gitlist.dev/e/4CBBE4DB.6080307%40viscovery.net In-Reply-To: ``` Am 10/17/2010 21:23, schrieb Thomas Rast: > 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 ```