Re: [PATCH v2] merge-file: correctly open files when in a subdir
> Here is what I have right now, queued somewhere in 'pu', after > fixing it up a bit. That's awesome! Thanks for your help :) -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] merge-file: correctly open files when in a subdir
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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] merge-file: correctly open files when in a subdir
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. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] merge-file: correctly open files when in a subdir
>> 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. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] merge-file: correctly open files when in a subdir
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" -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html