# [PATCH v2] merge-file: correctly open files when in a subdir 6 messages from 2015-02-10 to 2015-02-11. Participants: Aleksander Boruch-Gruszecki, Junio C Hamano. Thread: https://gitlist.dev/t/38545 ## Aleksander Boruch-Gruszecki, 2015-02-10 20:23 Subject: [PATCH v2] merge-file: correctly open files when in a subdir Message-ID: URL: https://gitlist.dev/e/CAPHKiG7vzKbtH7%3DcXD-7Cta%3Da-iy-ViMustn98z%2BVEog5ep2sg%40mail.gmail.com ``` run_setup_gently() is called before merge-file. This may result in changing current working directory, which wasn't taken into account when opening a file for writing. Fix by prepending the passed prefix. Previous var is left so that error messages keep refering to the file from the user's working directory perspective. Signed-off-by: Aleksander Boruch-Gruszecki --- builtin/merge-file.c | 3 ++- t/t6023-merge-file.sh | 6 ++++++ 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/builtin/merge-file.c b/builtin/merge-file.c index 844f84f..232b768 100644 --- a/builtin/merge-file.c +++ b/builtin/merge-file.c @@ -90,7 +90,8 @@ int cmd_merge_file(int argc, const char **argv, const char *prefix) if (ret >= 0) { const char *filename = argv[0]; - FILE *f = to_stdout ? stdout : fopen(filename, "wb"); + const char *fpath = prefix_filename(prefix, prefixlen, argv[0]); + FILE *f = to_stdout ? stdout : fopen(fpath, "wb"); if (!f) ret = error("Could not open %s for writing", filename); diff --git a/t/t6023-merge-file.sh b/t/t6023-merge-file.sh index 3758961..fdd104c 100755 --- a/t/t6023-merge-file.sh +++ b/t/t6023-merge-file.sh @@ -72,6 +72,12 @@ test_expect_success 'works in subdirectory' ' ( cd dir && git merge-file a.txt o.txt b.txt ) ' +mkdir -p dir/deep +cp new1.txt orig.txt new2.txt dir/deep +test_expect_success 'accounts for subdirectory when writing' ' + (cd dir && git merge-file deep/new1.txt deep/orig.txt deep/new2.txt) +' + cp new1.txt test.txt test_expect_success "merge without conflict (--quiet)" \ "git merge-file --quiet test.txt orig.txt new2.txt" -- 1.9.1 ``` ## Junio C Hamano, 2015-02-10 20:57 Subject: Re: [PATCH v2] merge-file: correctly open files when in a subdir Message-ID: URL: https://gitlist.dev/e/xmqqwq3pv60p.fsf%40gitster.dls.corp.google.com In-Reply-To: ``` Aleksander Boruch-Gruszecki writes: > run_setup_gently() is called before merge-file. This may result in changing > current working directory, which wasn't taken into account when opening a file > for writing. > > Fix by prepending the passed prefix. Previous var is left so that error > messages keep refering to the file from the user's working directory > perspective. > > Signed-off-by: Aleksander Boruch-Gruszecki > Please don't line wrap the footer. > --- > builtin/merge-file.c | 3 ++- > t/t6023-merge-file.sh | 6 ++++++ > 2 files changed, 8 insertions(+), 1 deletion(-) This patch does not apply. > diff --git a/builtin/merge-file.c b/builtin/merge-file.c > index 844f84f..232b768 100644 > --- a/builtin/merge-file.c > +++ b/builtin/merge-file.c > @@ -90,7 +90,8 @@ int cmd_merge_file(int argc, const char **argv, > const char *prefix) Please do not line-wrap the patch, either. > > if (ret >= 0) { The original has a single tab at the beginning of this line to indent, not four spaces. > const char *filename = argv[0]; > - FILE *f = to_stdout ? stdout : fopen(filename, "wb"); > + const char *fpath = prefix_filename(prefix, prefixlen, argv[0]); > + FILE *f = to_stdout ? stdout : fopen(fpath, "wb"); > > if (!f) > ret = error("Could not open %s for writing", filename); > diff --git a/t/t6023-merge-file.sh b/t/t6023-merge-file.sh > index 3758961..fdd104c 100755 > --- a/t/t6023-merge-file.sh > +++ b/t/t6023-merge-file.sh > @@ -72,6 +72,12 @@ test_expect_success 'works in subdirectory' ' > ( cd dir && git merge-file a.txt o.txt b.txt ) > ' > > +mkdir -p dir/deep > +cp new1.txt orig.txt new2.txt dir/deep > +test_expect_success 'accounts for subdirectory when writing' ' > + (cd dir && git merge-file deep/new1.txt deep/orig.txt deep/new2.txt) > +' Interesting. Makes us wonder why the one before this new one you added did not catch the issue, doesn't it? > + > cp new1.txt test.txt > test_expect_success "merge without conflict (--quiet)" \ > "git merge-file --quiet test.txt orig.txt new2.txt" ``` ## Aleksander Boruch-Gruszecki, 2015-02-11 09:58 Subject: Re: [PATCH v2] merge-file: correctly open files when in a subdir Message-ID: URL: https://gitlist.dev/e/CAPHKiG6M9_fOjpx8Pt8UTpUcrS%2BtmqL3YcT11WyJJu8m6nkJ4A%40mail.gmail.com In-Reply-To: ``` >> const char *filename = argv[0]; >> - FILE *f = to_stdout ? stdout : fopen(filename, "wb"); >> + const char *fpath = prefix_filename(prefix, prefixlen, argv[0]); >> + FILE *f = to_stdout ? stdout : fopen(fpath, "wb"); >> >> if (!f) >> ret = error("Could not open %s for writing", filename); >> diff --git a/t/t6023-merge-file.sh b/t/t6023-merge-file.sh >> index 3758961..fdd104c 100755 >> --- a/t/t6023-merge-file.sh >> +++ b/t/t6023-merge-file.sh >> @@ -72,6 +72,12 @@ test_expect_success 'works in subdirectory' ' >> ( cd dir && git merge-file a.txt o.txt b.txt ) >> ' >> >> +mkdir -p dir/deep >> +cp new1.txt orig.txt new2.txt dir/deep >> +test_expect_success 'accounts for subdirectory when writing' ' >> + (cd dir && git merge-file deep/new1.txt deep/orig.txt deep/new2.txt) >> +' > > Interesting. Makes us wonder why the one before this new one you > added did not catch the issue, doesn't it? The test before the one added by me does work because merge-file tries to open "a.txt" for writing in repo root directory, which will create a file if it does not exist. If you add a directory before the file, trying to open it will fail. I will additionally check of the file's content to make the test more clear, however it does actually fail with git 2.3.0. The issue with line-wrapping and tab changing to space is my own stupidity, I used an older config with my vim and it mangled the file. It also probably is what caused the patch to not apply properly. ``` ## Junio C Hamano, 2015-02-11 18:21 Subject: Re: [PATCH v2] merge-file: correctly open files when in a subdir Message-ID: URL: https://gitlist.dev/e/xmqqd25gux4r.fsf%40gitster.dls.corp.google.com In-Reply-To: ``` Aleksander Boruch-Gruszecki writes: >>> @@ -72,6 +72,12 @@ test_expect_success 'works in subdirectory' ' >>> ( cd dir && git merge-file a.txt o.txt b.txt ) >>> ' >>> >>> +mkdir -p dir/deep >>> +cp new1.txt orig.txt new2.txt dir/deep >>> +test_expect_success 'accounts for subdirectory when writing' ' >>> + (cd dir && git merge-file deep/new1.txt deep/orig.txt deep/new2.txt) >>> +' >> >> Interesting. Makes us wonder why the one before this new one you >> added did not catch the issue, doesn't it? > > The test before the one added by me does work because merge-file > tries to open "a.txt" for writing in repo root directory, which will create > a file if it does not exist. Ahh, this existing test >>> ( cd dir && git merge-file a.txt o.txt b.txt ) implicitly expects that dir/a.txt is written, but the broken implementation writes to a.txt (i.e. outside dir). But the test only checks the exit code from the command without making sure that dir/a.txt is written, it does not notice the breakage. Thanks, that makes sense and it also makes sense that checking the resulting content in dir/a.txt would make sense. Then we many not need to add a new dir/deep/* test---after all they are checking the same thing. ``` ## Junio C Hamano, 2015-02-11 19:02 Subject: Re: [PATCH v2] merge-file: correctly open files when in a subdir Message-ID: URL: https://gitlist.dev/e/xmqqtwystgoa.fsf%40gitster.dls.corp.google.com In-Reply-To: ``` Junio C Hamano writes: > Thanks, that makes sense and it also makes sense that checking the > resulting content in dir/a.txt would make sense. Then we many not > need to add a new dir/deep/* test---after all they are checking the > same thing. Here is what I have right now, queued somewhere in 'pu', after fixing it up a bit. -- >8 -- From: Aleksander Boruch-Gruszecki Date: Sun, 8 Feb 2015 17:53:53 +0100 Subject: [PATCH] merge-file: correctly open files when in a subdir run_setup_gently() is called before merge-file. This may result in changing current working directory, which wasn't taken into account when opening a file for writing. Fix by prepending the passed prefix. Previous var is left so that error messages keep referring to the file from the user's working directory perspective. Signed-off-by: Aleksander Boruch-Gruszecki Signed-off-by: Junio C Hamano --- builtin/merge-file.c | 3 ++- t/t6023-merge-file.sh | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/builtin/merge-file.c b/builtin/merge-file.c index 844f84f..232b768 100644 --- a/builtin/merge-file.c +++ b/builtin/merge-file.c @@ -90,7 +90,8 @@ int cmd_merge_file(int argc, const char **argv, const char *prefix) if (ret >= 0) { const char *filename = argv[0]; - FILE *f = to_stdout ? stdout : fopen(filename, "wb"); + const char *fpath = prefix_filename(prefix, prefixlen, argv[0]); + FILE *f = to_stdout ? stdout : fopen(fpath, "wb"); if (!f) ret = error("Could not open %s for writing", filename); diff --git a/t/t6023-merge-file.sh b/t/t6023-merge-file.sh index 432f086..d0df869 100755 --- a/t/t6023-merge-file.sh +++ b/t/t6023-merge-file.sh @@ -69,7 +69,8 @@ test_expect_success 'works in subdirectory' ' 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 ) + ( cd dir && git merge-file a.txt o.txt b.txt ) && + test_path_is_missing a.txt ' cp new1.txt test.txt -- 2.3.0-185-g073f588 ``` ## Aleksander Boruch-Gruszecki, 2015-02-11 21:20 Subject: Re: [PATCH v2] merge-file: correctly open files when in a subdir Message-ID: URL: https://gitlist.dev/e/CAPHKiG6Hbz8fDNbXTN%3D_5m3bHxHQQfVFJREXyro%3DHxQPNOQ6%3DA%40mail.gmail.com In-Reply-To: ``` > Here is what I have right now, queued somewhere in 'pu', after > fixing it up a bit. That's awesome! Thanks for your help :) ```