{"thread":{"id":"17469","subject":"[PATCH 1/2] mergetool: Add a test for running mergetool in a sub-directory","startedAt":"2009-01-30T23:20:10Z","lastAt":"2009-02-07T21:48:55Z","messageCount":5,"participants":["Charles Bailey","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"102657","messageId":"1233357611-31820-1-git-send-email-charles@hashpling.org","threadId":"17469","inReplyTo":null,"subject":"[PATCH 1/2] mergetool: Add a test for running mergetool in a sub-directory","fromName":"Charles Bailey","fromEmail":"charles@hashpling.org","sentAt":"2009-01-30T23:20:10Z","receivedAt":"2009-01-30T23:20:10Z","isPatch":true,"sender":{"key":"charles@hashpling.org","avatar":"https://avatars.githubusercontent.com/u/1668475?v=4"},"body":"Signed-off-by: Charles Bailey <charles@hashpling.org>\n---\n t/t7610-mergetool.sh |   70 ++++++++++++++++++++++++++++++++++++-------------\n 1 files changed, 51 insertions(+), 19 deletions(-)\n\ndiff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh\nindex edb6a57..ee8589e 100755\n--- a/t/t7610-mergetool.sh\n+++ b/t/t7610-mergetool.sh\n@@ -9,49 +9,81 @@ Testing basic merge tool invocation'\n \n . ./test-lib.sh\n \n+# All the mergetool test work by checking out a temporary branch based\n+# off 'branch1' and then merging in master and checking the results of\n+# running mergetool\n+\n test_expect_success 'setup' '\n     echo master >file1 &&\n-    git add file1 &&\n+    mkdir subdir &&\n+    echo master sub >subdir/file3 &&\n+    git add file1 subdir/file3 &&\n     git commit -m \"added file1\" &&\n+\n     git checkout -b branch1 master &&\n     echo branch1 change >file1 &&\n     echo branch1 newfile >file2 &&\n-    git add file1 file2 &&\n+    echo branch1 sub >subdir/file3 &&\n+    git add file1 file2 subdir/file3 &&\n     git commit -m \"branch1 changes\" &&\n-    git checkout -b branch2 master &&\n-    echo branch2 change >file1 &&\n-    echo branch2 newfile >file2 &&\n-    git add file1 file2 &&\n-    git commit -m \"branch2 changes\" &&\n+\n     git checkout master &&\n     echo master updated >file1 &&\n     echo master new >file2 &&\n-    git add file1 file2 &&\n-    git commit -m \"master updates\"\n-'\n+    echo master new sub >subdir/file3 &&\n+    git add file1 file2 subdir/file3 &&\n+    git commit -m \"master updates\" &&\n \n-test_expect_success 'custom mergetool' '\n     git config merge.tool mytool &&\n     git config mergetool.mytool.cmd \"cat \\\"\\$REMOTE\\\" >\\\"\\$MERGED\\\"\" &&\n-    git config mergetool.mytool.trustExitCode true &&\n-    git checkout branch1 &&\n+    git config mergetool.mytool.trustExitCode true\n+'\n+\n+test_expect_success 'custom mergetool' '\n+    git checkout -b test1 branch1 &&\n     test_must_fail git merge master >/dev/null 2>&1 &&\n-    ( yes \"\" | git mergetool file1>/dev/null 2>&1 ) &&\n-    ( yes \"\" | git mergetool file2>/dev/null 2>&1 ) &&\n+    ( yes \"\" | git mergetool file1 >/dev/null 2>&1 ) &&\n+    ( yes \"\" | git mergetool file2 >/dev/null 2>&1 ) &&\n+    ( yes \"\" | git mergetool subdir/file3 >/dev/null 2>&1 ) &&\n     test \"$(cat file1)\" = \"master updated\" &&\n     test \"$(cat file2)\" = \"master new\" &&\n+    test \"$(cat subdir/file3)\" = \"master new sub\" &&\n     git commit -m \"branch1 resolved with mergetool\"\n '\n \n test_expect_success 'mergetool crlf' '\n     git config core.autocrlf true &&\n-    git reset --hard HEAD^\n+    git checkout -b test2 branch1\n     test_must_fail git merge master >/dev/null 2>&1 &&\n-    ( yes \"\" | git mergetool file1>/dev/null 2>&1 ) &&\n-    ( yes \"\" | git mergetool file2>/dev/null 2>&1 ) &&\n+    ( yes \"\" | git mergetool file1 >/dev/null 2>&1 ) &&\n+    ( yes \"\" | git mergetool file2 >/dev/null 2>&1 ) &&\n+    ( yes \"\" | git mergetool subdir/file3 >/dev/null 2>&1 ) &&\n     test \"$(printf x | cat file1 -)\" = \"$(printf \"master updated\\r\\nx\")\" &&\n     test \"$(printf x | cat file2 -)\" = \"$(printf \"master new\\r\\nx\")\" &&\n-    git commit -m \"branch1 resolved with mergetool - autocrlf\"\n+    test \"$(printf x | cat subdir/file3 -)\" = \"$(printf \"master new sub\\r\\nx\")\" &&\n+    git commit -m \"branch1 resolved with mergetool - autocrlf\" &&\n+    git config core.autocrlf false &&\n+    git reset --hard\n+'\n+\n+test_expect_failure 'mergetool in subdir' '\n+    git checkout -b test3 branch1\n+    cd subdir && (\n+    test_must_fail git merge master >/dev/null 2>&1 &&\n+    ( yes \"\" | git mergetool file3 >/dev/null 2>&1 ) &&\n+    test \"$(cat file3)\" = \"master new sub\" )\n '\n \n+# We can't merge files from parent directories when running mergetool\n+# from a subdir. Is this a bug?\n+#\n+#test_expect_failure 'mergetool in subdir' '\n+#    cd subdir && (\n+#    ( yes \"\" | git mergetool ../file1 >/dev/null 2>&1 ) &&\n+#    ( yes \"\" | git mergetool ../file2 >/dev/null 2>&1 ) &&\n+#    test \"$(cat ../file1)\" = \"master updated\" &&\n+#    test \"$(cat ../file2)\" = \"master new\" &&\n+#    git commit -m \"branch1 resolved with mergetool - subdir\" )\n+#'\n+\n test_done\n-- \n1.6.1.235.gc9d403\n"},{"id":"102659","messageId":"1233357611-31820-2-git-send-email-charles@hashpling.org","threadId":"17469","inReplyTo":"1233357611-31820-1-git-send-email-charles@hashpling.org","subject":"[PATCH 2/2] mergetool: fix running mergetool in sub-directories","fromName":"Charles Bailey","fromEmail":"charles@hashpling.org","sentAt":"2009-01-30T23:20:11Z","receivedAt":"2009-01-30T23:20:11Z","isPatch":true,"sender":{"key":"charles@hashpling.org","avatar":"https://avatars.githubusercontent.com/u/1668475?v=4"},"body":"The previous fix to mergetool to use checkout-index instead of cat-file\nbroke running mergetool anywhere except the root of the repository.\n\nThis fixes it by using the correct relative paths for temporary files\nand index paths.\n\nSigned-off-by: Charles Bailey <charles@hashpling.org>\n---\n git-mergetool.sh     |    9 ++++-----\n t/t7610-mergetool.sh |    2 +-\n 2 files changed, 5 insertions(+), 6 deletions(-)\n\ndiff --git a/git-mergetool.sh b/git-mergetool.sh\nindex aefdca7..87fa88a 100755\n--- a/git-mergetool.sh\n+++ b/git-mergetool.sh\n@@ -13,7 +13,6 @@ SUBDIRECTORY_OK=Yes\n OPTIONS_SPEC=\n . git-sh-setup\n require_work_tree\n-prefix=$(git rev-parse --show-prefix)\n \n # Returns true if the mode reflects a symlink\n is_symlink () {\n@@ -131,7 +130,7 @@ checkout_staged_file () {\n     tmpfile=$(expr \"$(git checkout-index --temp --stage=\"$1\" \"$2\")\" : '\\([^\t]*\\)\t')\n \n     if test $? -eq 0 -a -n \"$tmpfile\" ; then\n-\tmv -- \"$tmpfile\" \"$3\"\n+\tmv -- \"$(git rev-parse --show-cdup)$tmpfile\" \"$3\"\n     fi\n }\n \n@@ -161,9 +160,9 @@ merge_file () {\n     local_mode=`git ls-files -u -- \"$MERGED\" | awk '{if ($3==2) print $1;}'`\n     remote_mode=`git ls-files -u -- \"$MERGED\" | awk '{if ($3==3) print $1;}'`\n \n-    base_present   && checkout_staged_file 1 \"$prefix$MERGED\" \"$BASE\"\n-    local_present  && checkout_staged_file 2 \"$prefix$MERGED\" \"$LOCAL\"\n-    remote_present && checkout_staged_file 3 \"$prefix$MERGED\" \"$REMOTE\"\n+    base_present   && checkout_staged_file 1 \"$MERGED\" \"$BASE\"\n+    local_present  && checkout_staged_file 2 \"$MERGED\" \"$LOCAL\"\n+    remote_present && checkout_staged_file 3 \"$MERGED\" \"$REMOTE\"\n \n     if test -z \"$local_mode\" -o -z \"$remote_mode\"; then\n \techo \"Deleted merge conflict for '$MERGED':\"\ndiff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh\nindex ee8589e..e768c3e 100755\n--- a/t/t7610-mergetool.sh\n+++ b/t/t7610-mergetool.sh\n@@ -66,7 +66,7 @@ test_expect_success 'mergetool crlf' '\n     git reset --hard\n '\n \n-test_expect_failure 'mergetool in subdir' '\n+test_expect_success 'mergetool in subdir' '\n     git checkout -b test3 branch1\n     cd subdir && (\n     test_must_fail git merge master >/dev/null 2>&1 &&\n-- \n1.6.1.235.gc9d403\n"},{"id":"102738","messageId":"7v63jv0wn6.fsf@gitster.siamese.dyndns.org","threadId":"17469","inReplyTo":"1233357611-31820-2-git-send-email-charles@hashpling.org","subject":"Re: [PATCH 2/2] mergetool: fix running mergetool in sub-directories","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-02-01T01:32:45Z","receivedAt":"2009-02-01T01:32:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Charles Bailey <charles@hashpling.org> writes:\n\n> diff --git a/git-mergetool.sh b/git-mergetool.sh\n> index aefdca7..87fa88a 100755\n> --- a/git-mergetool.sh\n> +++ b/git-mergetool.sh\n> @@ -13,7 +13,6 @@ SUBDIRECTORY_OK=Yes\n>  OPTIONS_SPEC=\n>  . git-sh-setup\n>  require_work_tree\n> -prefix=$(git rev-parse --show-prefix)\n>  \n>  # Returns true if the mode reflects a symlink\n>  is_symlink () {\n> @@ -131,7 +130,7 @@ checkout_staged_file () {\n>      tmpfile=$(expr \"$(git checkout-index --temp --stage=\"$1\" \"$2\")\" : '\\([^\t]*\\)\t')\n>  \n>      if test $? -eq 0 -a -n \"$tmpfile\" ; then\n> -\tmv -- \"$tmpfile\" \"$3\"\n> +\tmv -- \"$(git rev-parse --show-cdup)$tmpfile\" \"$3\"\n>      fi\n>  }\n\nLooking at the above change together with this disabled patch in the previous,\n\n> +# We can't merge files from parent directories when running mergetool\n> +# from a subdir. Is this a bug?\n> +#\n> +#test_expect_failure 'mergetool in subdir' '\n> +#    cd subdir && (\n> +#    ( yes \"\" | git mergetool ../file1 >/dev/null 2>&1 ) &&\n> +#    ( yes \"\" | git mergetool ../file2 >/dev/null 2>&1 ) &&\n> +#    test \"$(cat ../file1)\" = \"master updated\" &&\n> +#    test \"$(cat ../file2)\" = \"master new\" &&\n> +#    git commit -m \"branch1 resolved with mergetool - subdir\" )\n> +#'\n\nI wonder if it would be cleaner to keep $prefix and instead cd_to_toplevel\nat the beginning.\n\nThere are two ways for Porcelain scripts to work from inside a\nsubdirectory of a project.\n\n * Stay in the original subdirectory and use show-cdup to convert a path\n   that is relative to the repository root to a path relative to your\n   current subdirectory.\n\n * Go up to the root of the work tree, use prefix to convert a path that\n   is relative to the original subdirectory to a path relative to the root\n   of the work tree.\n\nThe former way is probably the clunkier one between the two.  I think the\nlatter is how most of the Porcelain scripts work when they need to worry\nabout the Plumbing commands that return/accept repository relative paths.\nAlso, all the plumbing commands work in the latter way.\n"},{"id":"102907","messageId":"49877F68.30509@hashpling.org","threadId":"17469","inReplyTo":"7v63jv0wn6.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH 2/2] mergetool: fix running mergetool in sub-directories","fromName":"Charles Bailey","fromEmail":"charles@hashpling.org","sentAt":"2009-02-02T23:19:04Z","receivedAt":"2009-02-02T23:19:04Z","isPatch":true,"sender":{"key":"charles@hashpling.org","avatar":"https://avatars.githubusercontent.com/u/1668475?v=4"},"body":"Junio C Hamano wrote:\n> Charles Bailey <charles@hashpling.org> writes:\n> \n>> diff --git a/git-mergetool.sh b/git-mergetool.sh\n>> index aefdca7..87fa88a 100755\n>> --- a/git-mergetool.sh\n>> +++ b/git-mergetool.sh\n>> @@ -13,7 +13,6 @@ SUBDIRECTORY_OK=Yes\n>>  OPTIONS_SPEC=\n>>  . git-sh-setup\n>>  require_work_tree\n>> -prefix=$(git rev-parse --show-prefix)\n>>  \n>>  # Returns true if the mode reflects a symlink\n>>  is_symlink () {\n>> @@ -131,7 +130,7 @@ checkout_staged_file () {\n>>      tmpfile=$(expr \"$(git checkout-index --temp --stage=\"$1\" \"$2\")\" : '\\([^\t]*\\)\t')\n>>  \n>>      if test $? -eq 0 -a -n \"$tmpfile\" ; then\n>> -\tmv -- \"$tmpfile\" \"$3\"\n>> +\tmv -- \"$(git rev-parse --show-cdup)$tmpfile\" \"$3\"\n>>      fi\n>>  }\n> \n> Looking at the above change together with this disabled patch in the previous,\n> \n>> +# We can't merge files from parent directories when running mergetool\n>> +# from a subdir. Is this a bug?\n>> +#\n> \n> I wonder if it would be cleaner to keep $prefix and instead cd_to_toplevel\n> at the beginning.\n> \n> There are two ways for Porcelain scripts to work from inside a\n> subdirectory of a project.\n> \n>  * Stay in the original subdirectory and use show-cdup to convert a path\n>    that is relative to the repository root to a path relative to your\n>    current subdirectory.\n> \n>  * Go up to the root of the work tree, use prefix to convert a path that\n>    is relative to the original subdirectory to a path relative to the root\n>    of the work tree.\n> \n> The former way is probably the clunkier one between the two.  I think the\n> latter is how most of the Porcelain scripts work when they need to worry\n> about the Plumbing commands that return/accept repository relative paths.\n> Also, all the plumbing commands work in the latter way.\n\nPossibly, and I certainly wouldn't object if mergetool moved to the\nlatter mode of operation in the medium term.\n\nIn general, though, mergetool is pretty happy running from the users\ndirectory and there may be cases in which users expect the merge tool to\nbe running from the directory that they started in (saving extra files\nduring the merge process?).\n\nAt the moment it is really a 'bugette' in the eyes of mergetool that\ncheckout-index echoes the name of the temporary file relative to the\nrepository root even if you called it from somewhere else. Without this,\nmergetool doesn't require any prefix wizardry at all.\n\nWhen I said 'is this a bug?', the counter argument is that it might be\nconsidered a more useful feature that cd'ing into a subdirectory and\nrunning mergetool restricts the files merged to an expected subset.\n\nI meant to put in a cover note about the test changes. The reason that I\ncommented out the test_expect_failure was that I thought it not\ncompletely obvious that it even counted as a failure rather than a\nfeature - certainly not one worth adding one to git's overall 'broken'\ncount - but that there might be some alternative opinions.\n\nThe pu patch is certainly necessary on top of the cat-file ->\ncheckout-index change in next and it is better tested now. It's\ncertainly not the last word on mergetool.\n\nCharles.\n"},{"id":"103658","messageId":"7v3aepuddk.fsf@gitster.siamese.dyndns.org","threadId":"17469","inReplyTo":"49877F68.30509@hashpling.org","subject":"Re: [PATCH 2/2] mergetool: fix running mergetool in sub-directories","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-02-07T21:48:55Z","receivedAt":"2009-02-07T21:48:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Charles Bailey <charles@hashpling.org> writes:\n\n> Junio C Hamano wrote:\n>> \n>> I wonder if it would be cleaner to keep $prefix and instead cd_to_toplevel\n>> at the beginning.\n>> ...\n> Possibly, and I certainly wouldn't object if mergetool moved to the\n> latter mode of operation in the medium term.\n>\n> In general, though, mergetool is pretty happy running from the users\n> directory and there may be cases in which users expect the merge tool to\n> be running from the directory that they started in (saving extra files\n> during the merge process?).\n\nA very sensible consideration that I agree with.  I am happy that you\nwere the one who volunteered to take this script over.\n"}]}