{"thread":{"id":"38545","subject":"[PATCH v2] merge-file: correctly open files when in a subdir","startedAt":"2015-02-10T20:23:55Z","lastAt":"2015-02-11T21:20:40Z","messageCount":6,"participants":["Aleksander Boruch-Gruszecki","Junio C Hamano"],"isPatch":true,"patchVersion":2,"patchTotal":null},"messages":[{"id":"255898","messageId":"CAPHKiG7vzKbtH7=cXD-7Cta=a-iy-ViMustn98z+VEog5ep2sg@mail.gmail.com","threadId":"38545","inReplyTo":null,"subject":"[PATCH v2] merge-file: correctly open files when in a subdir","fromName":"Aleksander Boruch-Gruszecki","fromEmail":"aleksander.boruchgruszecki@gmail.com","sentAt":"2015-02-10T20:23:55Z","receivedAt":"2015-02-10T20:23:55Z","isPatch":true,"sender":{"key":"aleksander.boruchgruszecki@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8414373?v=4"},"body":"run_setup_gently() is called before merge-file. This may result in changing\ncurrent working directory, which wasn't taken into account when opening a file\nfor writing.\n\nFix by prepending the passed prefix. Previous var is left so that error\nmessages keep refering to the file from the user's working directory\nperspective.\n\nSigned-off-by: Aleksander Boruch-Gruszecki\n \n---\n builtin/merge-file.c | 3 ++-\n t/t6023-merge-file.sh | 6 ++++++\n 2 files changed, 8 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/merge-file.c b/builtin/merge-file.c\nindex 844f84f..232b768 100644\n--- a/builtin/merge-file.c\n+++ b/builtin/merge-file.c\n@@ -90,7 +90,8 @@ int cmd_merge_file(int argc, const char **argv,\nconst char *prefix)\n\n if (ret >= 0) {\n const char *filename = argv[0];\n- FILE *f = to_stdout ? stdout : fopen(filename, \"wb\");\n+ const char *fpath = prefix_filename(prefix, prefixlen, argv[0]);\n+ FILE *f = to_stdout ? stdout : fopen(fpath, \"wb\");\n\n if (!f)\n ret = error(\"Could not open %s for writing\", filename);\ndiff --git a/t/t6023-merge-file.sh b/t/t6023-merge-file.sh\nindex 3758961..fdd104c 100755\n--- a/t/t6023-merge-file.sh\n+++ b/t/t6023-merge-file.sh\n@@ -72,6 +72,12 @@ test_expect_success 'works in subdirectory' '\n ( cd dir && git merge-file a.txt o.txt b.txt )\n '\n\n+mkdir -p dir/deep\n+cp new1.txt orig.txt new2.txt dir/deep\n+test_expect_success 'accounts for subdirectory when writing' '\n+ (cd dir && git merge-file deep/new1.txt deep/orig.txt deep/new2.txt)\n+'\n+\n cp new1.txt test.txt\n test_expect_success \"merge without conflict (--quiet)\" \\\n \"git merge-file --quiet test.txt orig.txt new2.txt\"\n-- \n1.9.1\n"},{"id":"255900","messageId":"xmqqwq3pv60p.fsf@gitster.dls.corp.google.com","threadId":"38545","inReplyTo":"CAPHKiG7vzKbtH7=cXD-7Cta=a-iy-ViMustn98z+VEog5ep2sg@mail.gmail.com","subject":"Re: [PATCH v2] merge-file: correctly open files when in a subdir","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-02-10T20:57:42Z","receivedAt":"2015-02-10T20:57:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Aleksander Boruch-Gruszecki \nwrites:\n\n> run_setup_gently() is called before merge-file. This may result in changing\n> current working directory, which wasn't taken into account when opening a file\n> for writing.\n>\n> Fix by prepending the passed prefix. Previous var is left so that error\n> messages keep refering to the file from the user's working directory\n> perspective.\n>\n> Signed-off-by: Aleksander Boruch-Gruszecki\n> \n\nPlease don't line wrap the footer.\n\n> ---\n> builtin/merge-file.c | 3 ++-\n> t/t6023-merge-file.sh | 6 ++++++\n> 2 files changed, 8 insertions(+), 1 deletion(-)\n\nThis patch does not apply.\n\n> diff --git a/builtin/merge-file.c b/builtin/merge-file.c\n> index 844f84f..232b768 100644\n> --- a/builtin/merge-file.c\n> +++ b/builtin/merge-file.c\n> @@ -90,7 +90,8 @@ int cmd_merge_file(int argc, const char **argv,\n> const char *prefix)\n\nPlease do not line-wrap the patch, either.\n\n>\n> if (ret >= 0) {\n\nThe original has a single tab at the beginning of this line to\nindent, not four spaces.\n\n> const char *filename = argv[0];\n> - FILE *f = to_stdout ? stdout : fopen(filename, \"wb\");\n> + const char *fpath = prefix_filename(prefix, prefixlen, argv[0]);\n> + FILE *f = to_stdout ? stdout : fopen(fpath, \"wb\");\n>\n> if (!f)\n> ret = error(\"Could not open %s for writing\", filename);\n> diff --git a/t/t6023-merge-file.sh b/t/t6023-merge-file.sh\n> index 3758961..fdd104c 100755\n> --- a/t/t6023-merge-file.sh\n> +++ b/t/t6023-merge-file.sh\n> @@ -72,6 +72,12 @@ test_expect_success 'works in subdirectory' '\n> ( cd dir && git merge-file a.txt o.txt b.txt )\n> '\n>\n> +mkdir -p dir/deep\n> +cp new1.txt orig.txt new2.txt dir/deep\n> +test_expect_success 'accounts for subdirectory when writing' '\n> + (cd dir && git merge-file deep/new1.txt deep/orig.txt deep/new2.txt)\n> +'\n\nInteresting. Makes us wonder why the one before this new one you\nadded did not catch the issue, doesn't it?\n\n> +\n> cp new1.txt test.txt\n> test_expect_success \"merge without conflict (--quiet)\" \\\n> \"git merge-file --quiet test.txt orig.txt new2.txt\"\n"},{"id":"255923","messageId":"CAPHKiG6M9_fOjpx8Pt8UTpUcrS+tmqL3YcT11WyJJu8m6nkJ4A@mail.gmail.com","threadId":"38545","inReplyTo":"xmqqwq3pv60p.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v2] merge-file: correctly open files when in a subdir","fromName":"Aleksander Boruch-Gruszecki","fromEmail":"aleksander.boruchgruszecki@gmail.com","sentAt":"2015-02-11T09:58:51Z","receivedAt":"2015-02-11T09:58:51Z","isPatch":true,"sender":{"key":"aleksander.boruchgruszecki@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8414373?v=4"},"body":">> const char *filename = argv[0];\n>> - FILE *f = to_stdout ? stdout : fopen(filename, \"wb\");\n>> + const char *fpath = prefix_filename(prefix, prefixlen, argv[0]);\n>> + FILE *f = to_stdout ? stdout : fopen(fpath, \"wb\");\n>>\n>> if (!f)\n>> ret = error(\"Could not open %s for writing\", filename);\n>> diff --git a/t/t6023-merge-file.sh b/t/t6023-merge-file.sh\n>> index 3758961..fdd104c 100755\n>> --- a/t/t6023-merge-file.sh\n>> +++ b/t/t6023-merge-file.sh\n>> @@ -72,6 +72,12 @@ test_expect_success 'works in subdirectory' '\n>> ( cd dir && git merge-file a.txt o.txt b.txt )\n>> '\n>>\n>> +mkdir -p dir/deep\n>> +cp new1.txt orig.txt new2.txt dir/deep\n>> +test_expect_success 'accounts for subdirectory when writing' '\n>> + (cd dir && git merge-file deep/new1.txt deep/orig.txt deep/new2.txt)\n>> +'\n>\n> Interesting. Makes us wonder why the one before this new one you\n> added did not catch the issue, doesn't it?\n\nThe test before the one added by me does work because merge-file\ntries to open \"a.txt\" for writing in repo root directory, which will create\na file if it does not exist. If you add a directory before the file, trying to\nopen it will fail. I will additionally check of the file's content to make\nthe test more clear, however it does actually fail with git 2.3.0.\n\nThe issue with line-wrapping and tab changing to space is my own\nstupidity, I used an older config with my vim and it mangled the file.\nIt also probably is what caused the patch to not apply properly.\n"},{"id":"255937","messageId":"xmqqd25gux4r.fsf@gitster.dls.corp.google.com","threadId":"38545","inReplyTo":"CAPHKiG6M9_fOjpx8Pt8UTpUcrS+tmqL3YcT11WyJJu8m6nkJ4A@mail.gmail.com","subject":"Re: [PATCH v2] merge-file: correctly open files when in a subdir","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-02-11T18:21:56Z","receivedAt":"2015-02-11T18:21:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Aleksander Boruch-Gruszecki \nwrites:\n\n>>> @@ -72,6 +72,12 @@ test_expect_success 'works in subdirectory' '\n>>> ( cd dir && git merge-file a.txt o.txt b.txt )\n>>> '\n>>>\n>>> +mkdir -p dir/deep\n>>> +cp new1.txt orig.txt new2.txt dir/deep\n>>> +test_expect_success 'accounts for subdirectory when writing' '\n>>> + (cd dir && git merge-file deep/new1.txt deep/orig.txt deep/new2.txt)\n>>> +'\n>>\n>> Interesting. Makes us wonder why the one before this new one you\n>> added did not catch the issue, doesn't it?\n>\n> The test before the one added by me does work because merge-file\n> tries to open \"a.txt\" for writing in repo root directory, which will create\n> a file if it does not exist.\n\nAhh, this existing test\n\n>>> ( cd dir && git merge-file a.txt o.txt b.txt )\n\nimplicitly expects that dir/a.txt is written, but the broken\nimplementation writes to a.txt (i.e. outside dir). But the test\nonly checks the exit code from the command without making sure that\ndir/a.txt is written, it does not notice the breakage.\n\nThanks, that makes sense and it also makes sense that checking the\nresulting content in dir/a.txt would make sense. Then we many not\nneed to add a new dir/deep/* test---after all they are checking the\nsame thing.\n"},{"id":"255941","messageId":"xmqqtwystgoa.fsf@gitster.dls.corp.google.com","threadId":"38545","inReplyTo":"xmqqd25gux4r.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v2] merge-file: correctly open files when in a subdir","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-02-11T19:02:45Z","receivedAt":"2015-02-11T19:02:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Thanks, that makes sense and it also makes sense that checking the\n> resulting content in dir/a.txt would make sense. Then we many not\n> need to add a new dir/deep/* test---after all they are checking the\n> same thing.\n\nHere is what I have right now, queued somewhere in 'pu', after\nfixing it up a bit.\n\n-- >8 --\nFrom: Aleksander Boruch-Gruszecki \nDate: Sun, 8 Feb 2015 17:53:53 +0100\nSubject: [PATCH] merge-file: correctly open files when in a subdir\n\nrun_setup_gently() is called before merge-file. This may result in changing\ncurrent working directory, which wasn't taken into account when opening a file\nfor writing.\n\nFix by prepending the passed prefix. Previous var is left so that error\nmessages keep referring to the file from the user's working directory\nperspective.\n\nSigned-off-by: Aleksander Boruch-Gruszecki \nSigned-off-by: Junio C Hamano \n---\n builtin/merge-file.c | 3 ++-\n t/t6023-merge-file.sh | 3 ++-\n 2 files changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/merge-file.c b/builtin/merge-file.c\nindex 844f84f..232b768 100644\n--- a/builtin/merge-file.c\n+++ b/builtin/merge-file.c\n@@ -90,7 +90,8 @@ int cmd_merge_file(int argc, const char **argv, const char *prefix)\n \n \tif (ret >= 0) {\n \t\tconst char *filename = argv[0];\n-\t\tFILE *f = to_stdout ? stdout : fopen(filename, \"wb\");\n+\t\tconst char *fpath = prefix_filename(prefix, prefixlen, argv[0]);\n+\t\tFILE *f = to_stdout ? stdout : fopen(fpath, \"wb\");\n \n \t\tif (!f)\n \t\t\tret = error(\"Could not open %s for writing\", filename);\ndiff --git a/t/t6023-merge-file.sh b/t/t6023-merge-file.sh\nindex 432f086..d0df869 100755\n--- a/t/t6023-merge-file.sh\n+++ b/t/t6023-merge-file.sh\n@@ -69,7 +69,8 @@ test_expect_success 'works in subdirectory' '\n \tcp new1.txt dir/a.txt &&\n \tcp orig.txt dir/o.txt &&\n \tcp new2.txt dir/b.txt &&\n-\t( cd dir && git merge-file a.txt o.txt b.txt )\n+\t( cd dir && git merge-file a.txt o.txt b.txt ) &&\n+\ttest_path_is_missing a.txt\n '\n \n cp new1.txt test.txt\n-- \n2.3.0-185-g073f588\n"},{"id":"255944","messageId":"CAPHKiG6Hbz8fDNbXTN=_5m3bHxHQQfVFJREXyro=HxQPNOQ6=A@mail.gmail.com","threadId":"38545","inReplyTo":"xmqqtwystgoa.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v2] merge-file: correctly open files when in a subdir","fromName":"Aleksander Boruch-Gruszecki","fromEmail":"aleksander.boruchgruszecki@gmail.com","sentAt":"2015-02-11T21:20:40Z","receivedAt":"2015-02-11T21:20:40Z","isPatch":true,"sender":{"key":"aleksander.boruchgruszecki@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8414373?v=4"},"body":"> Here is what I have right now, queued somewhere in 'pu', after\n> fixing it up a bit.\n\nThat's awesome! Thanks for your help :)\n"}]}