{"thread":{"id":"24722","subject":"Status of conflicted files resolved with rerere","startedAt":"2010-08-12T21:28:28Z","lastAt":"2010-08-20T15:25:09Z","messageCount":16,"participants":["Magnus Bäck","Avery Pennarun","Jay Soffian","David Aguilar","Junio C Hamano","Thomas Rast","Charles Bailey","Jonathan Nieder"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"147949","messageId":"20100812212828.GA17825@jpl.local","threadId":"24722","inReplyTo":null,"subject":"Status of conflicted files resolved with rerere","fromName":"Magnus Bäck","fromEmail":"magnus.back@sonyericsson.com","sentAt":"2010-08-12T21:28:28Z","receivedAt":"2010-08-12T21:28:28Z","isPatch":false,"sender":{"key":"magnus.back@sonyericsson.com","avatar":null},"body":"I played around with git rerere today and was surprised by the results.\nWhen a conflict has been resolved automatically by rerere, the file\nisn't added to the index like other files are where Git just used one of\nthe regular merge resolution algorithms. What's worse, if git mergetool\nis invoked -- which is what I normally do when git merge needs help --\nit has no idea that the file actually has been merged already, and\nlaunches the merge tool with the three files involved in the merge. If\nthe user hasn't been paying attention to each line of the git merge\noutput (stating the files who were automatically resolved) it's easy to\ntrash rerere's work.\n\nWould it make sense for git merge to add rerere'd files (where all hunks\nwere recognized and resolved) to the index automatically? That would\nmake it possible for the user to run git mergetool without reading every\nsingle line of output from previous commands just to figure out which\nfiles should be added to the index straight off and which files require\nmerging.\n\nFor users resolving conflicts by editing the file and fiddling with the\n<<<<<<<<< marks etc such a change wouldn't make that big difference, but\nfor mergetool users it seems like a quite useful improvement. Or is\nthere something I'm failing to grasp here? This is on Git 1.7.0.3 by the\nway.\n\n-- \nMagnus Bäck                      Opinions are my own and do not necessarily\nSW Configuration Manager         represent the ones of my employer, etc.\nSony Ericsson\n"},{"id":"147952","messageId":"AANLkTi=tVV5gL2b2LfXALXahzabJXzVjB5z9-OSztOMJ@mail.gmail.com","threadId":"24722","inReplyTo":"20100812212828.GA17825@jpl.local","subject":"Re: Status of conflicted files resolved with rerere","fromName":"Avery Pennarun","fromEmail":"apenwarr@gmail.com","sentAt":"2010-08-12T21:36:53Z","receivedAt":"2010-08-12T21:36:53Z","isPatch":false,"sender":{"key":"apenwarr@gmail.com","avatar":"https://avatars.githubusercontent.com/u/20592?v=4"},"body":"On Thu, Aug 12, 2010 at 5:28 PM, Magnus Bäck\n<magnus.back@sonyericsson.com> wrote:\n> I played around with git rerere today and was surprised by the results.\n> When a conflict has been resolved automatically by rerere, the file\n> isn't added to the index like other files are where Git just used one of\n> the regular merge resolution algorithms. What's worse, if git mergetool\n> is invoked -- which is what I normally do when git merge needs help --\n> it has no idea that the file actually has been merged already, and\n> launches the merge tool with the three files involved in the merge. If\n> the user hasn't been paying attention to each line of the git merge\n> output (stating the files who were automatically resolved) it's easy to\n> trash rerere's work.\n\nThe motivation behind the current behaviour, as I understand it, is\nthat rerere speeds things up, but you don't necessarily want to trust\nthat it has resolved your merge conflicts correctly.  After all, they\nwere unarguably *conflicts*, not just normal merge results, so you\ncan't quite trust them.\n\nThat said, I've never had a problem where rerere did the wrong thing\nfor me.  Maybe there could be an option to override it.\n\nAnyway, I never use a mergetool, so like you suspected, this has never\nbeen a major problem for me.\n\nIt sounds like the real problem here though it the mergetool stuff.\nWhy is it disregarding all the automated merging that git has done and\nstarting over from scratch?  If git, in its infinite cleverness, has\nresolved *some* parts of the file but not others, wouldn't we want it\nto keep those resolutions?  It sounds like mergetool is actually\nmaking things *more* work instead of less.\n\nIs there some way to teach the mergetool stuff to be smarter?  At the\nvery least, having it auto-skip files that have no *remaining*\nconflicts might be a good idea.\n\nHave fun,\n\nAvery\n"},{"id":"148005","messageId":"AANLkTi=0dV54q6o3f-XrxKAHDKn8mhWFmsLDen_3+1G7@mail.gmail.com","threadId":"24722","inReplyTo":"AANLkTi=tVV5gL2b2LfXALXahzabJXzVjB5z9-OSztOMJ@mail.gmail.com","subject":"Re: Status of conflicted files resolved with rerere","fromName":"Jay Soffian","fromEmail":"jaysoffian@gmail.com","sentAt":"2010-08-13T17:19:47Z","receivedAt":"2010-08-13T17:19:47Z","isPatch":false,"sender":{"key":"jaysoffian@gmail.com","avatar":"https://avatars.githubusercontent.com/u/155970?v=4"},"body":"On Thu, Aug 12, 2010 at 5:36 PM, Avery Pennarun <apenwarr@gmail.com> wrote:\n> That said, I've never had a problem where rerere did the wrong thing\n> for me.  Maybe there could be an option to override it.\n\n$ git config rerere.autoupdate true\n\nj.\n"},{"id":"148120","messageId":"D06701C5-E581-46AA-BC7D-3DA7C4127D44@gmail.com","threadId":"24722","inReplyTo":"AANLkTi=tVV5gL2b2LfXALXahzabJXzVjB5z9-OSztOMJ@mail.gmail.com","subject":"Re: Status of conflicted files resolved with rerere","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2010-08-15T02:24:43Z","receivedAt":"2010-08-15T02:24:43Z","isPatch":false,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"\nOn Aug 12, 2010, at 2:36 PM, Avery Pennarun <apenwarr@gmail.com> wrote:\n\n> On Thu, Aug 12, 2010 at 5:28 PM, Magnus Bäck\n> <magnus.back@sonyericsson.com> wrote:\n>> I played around with git rerere today and was surprised by the  \n>> results.\n>> When a conflict has been resolved automatically by rerere, the file\n>> isn't added to the index like other files are where Git just used  \n>> one of\n>> the regular merge resolution algorithms. What's worse, if git  \n>> mergetool\n>> is invoked -- which is what I normally do when git merge needs help  \n>> --\n>> it has no idea that the file actually has been merged already, and\n>> launches the merge tool with the three files involved in the merge.  \n>> If\n>> the user hasn't been paying attention to each line of the git merge\n>> output (stating the files who were automatically resolved) it's  \n>> easy to\n>> trash rerere's work.\n> [...]\n> It sounds like the real problem here though it the mergetool stuff.\n> Why is it disregarding all the automated merging that git has done and\n> starting over from scratch?  If git, in its infinite cleverness, has\n> resolved *some* parts of the file but not others, wouldn't we want it\n> to keep those resolutions?  It sounds like mergetool is actually\n> making things *more* work instead of less.\n>\n> Is there some way to teach the mergetool stuff to be smarter?  At the\n> very least, having it auto-skip files that have no *remaining*\n> conflicts might be a good idea.\n>\n> Have fun,\n>\n> Avery\n\nRight, that would be a great enhancement.\n\nThe problem is that mergetool always uses local, remote, and base.  It  \nuses the unmerged flag in the index when deciding which files to  \nconsider.\n\nHere's what we'd need in order to improve rerere and mergetool  \ninteraction:  the ability to answer the question, \"has this file been  \nrerere merged?\"\n\nOnce we have that then we can make mergetool skip these files by  \ndefault. We would also need a command line flag to override the  \nbehaviour.\n\nPerhaps a naive grep for merge markers in the worktree file would  \nsuffice?  Heuristics have gone a long way in git so doing something  \nlike that wouldn't seem too atrocious.\n\nIt'd also have the benefit that mergetool would skip files merged by  \n$EDITOR.\n\nIf anyone has any tips I'm all ears.  Does this seem reasonable, and  \nif so, what would be a good name for the command line flag to get the  \nexisting behavior?  My gut feeling is that it shouldn't have a  \ncorresponding config variable.\n\nCheers,\n-- \n         David"},{"id":"148125","messageId":"7veie0gy3r.fsf@alter.siamese.dyndns.org","threadId":"24722","inReplyTo":"D06701C5-E581-46AA-BC7D-3DA7C4127D44@gmail.com","subject":"Re: Status of conflicted files resolved with rerere","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-08-15T06:59:36Z","receivedAt":"2010-08-15T06:59:36Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"David Aguilar <davvid@gmail.com> writes:\n\n> Here's what we'd need in order to improve rerere and mergetool  \n> interaction:  the ability to answer the question, \"has this file been  \n> rerere merged?\"\n\nI do not quite understand why the user _runs_ mergetool on a file that has\nbeen already merged; isn't it an option not to do so in the first place?\n\nHaving said that.\n\nI think you can use the fact that:\n\n - \"ls-files -u\" will list paths with conflicts; and\n\n - \"rerere status\" won't mention the ones that have been autoresolved\n\nif rerere is in effect (check for presense of .git/MERGE_RR).\n"},{"id":"148138","messageId":"20100815160022.GA31388@jpl.local","threadId":"24722","inReplyTo":"7veie0gy3r.fsf@alter.siamese.dyndns.org","subject":"Re: Status of conflicted files resolved with rerere","fromName":"Magnus Bäck","fromEmail":"magnus.back@sonyericsson.com","sentAt":"2010-08-15T16:00:22Z","receivedAt":"2010-08-15T16:00:22Z","isPatch":false,"sender":{"key":"magnus.back@sonyericsson.com","avatar":null},"body":"On Sunday, August 15, 2010 at 08:59 CEST,\n     Junio C Hamano <gitster@pobox.com> wrote:\n\n> David Aguilar <davvid@gmail.com> writes:\n> \n> > Here's what we'd need in order to improve rerere and mergetool\n> > interaction:  the ability to answer the question, \"has this file\n> > been rerere merged?\"\n> \n> I do not quite understand why the user _runs_ mergetool on a file that\n> has been already merged; isn't it an option not to do so in the first\n> place?\n\nYou have a point, but if there are conflicts in many files where only a\nsubset were autoresolved I think it would be prudent to help the user.\nGrepping after remaining conflict markers or keeping the \"git merge\"\noutput somewhere to see which files actually were autoresolved works\nbut I think we can do better.\n\nOn the other hand, hinting mergetool users about rerere.autoupdate\nis perhaps good enough? Doesn't help users who want to inspect the\nautoresolved results yet also want hassle-free mergetool usage, though.\n\n> Having said that.\n> \n> I think you can use the fact that:\n> \n>  - \"ls-files -u\" will list paths with conflicts; and\n> \n>  - \"rerere status\" won't mention the ones that have been autoresolved\n> \n> if rerere is in effect (check for presense of .git/MERGE_RR).\n\nOkay, I'll have a stab at a patch.\n\n-- \nMagnus Bäck                      Opinions are my own and do not necessarily\nSW Configuration Manager         represent the ones of my employer, etc.\nSony Ericsson\n"},{"id":"148262","messageId":"1282036966-26799-1-git-send-email-davvid@gmail.com","threadId":"24722","inReplyTo":"7veie0gy3r.fsf@alter.siamese.dyndns.org","subject":"[PATCH] mergetool: Skip autoresolved paths","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2010-08-17T09:22:46Z","receivedAt":"2010-08-17T09:22:46Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"When mergetool is run without path limiters it loops\nover each entry in 'git ls-files -u'.  This includes\nautoresolved paths.\n\nTeach mergetool to only merge files listed in 'rerere status'\nwhen rerere is enabled.\n\nThere are some subtle but harmless changes in behavior.\nWe now call cd_to_toplevel when no paths are given.\nWe do this because 'rerere status' paths are always relative\nto the root.  This is beneficial for the non-rerere use as\nwell in that mergetool now runs against all unmerged files\nregardless of the current directory.\n\nThis also slightly tweaks the output when run without paths\nto be more readable.\n\nThe old output:\n\nMerging the files: foo\nbar\nbaz\n\nThe new output:\n\nMerging:\nfoo\nbar\nbaz\n\nSigned-off-by: David Aguilar <davvid@gmail.com>\n---\n git-mergetool.sh     |   28 +++++++++++++++++++++++-----\n t/t7610-mergetool.sh |   46 ++++++++++++++++++++++++++++++++++------------\n 2 files changed, 57 insertions(+), 17 deletions(-)\n\ndiff --git a/git-mergetool.sh b/git-mergetool.sh\nindex b52a741..bd7ab02 100755\n--- a/git-mergetool.sh\n+++ b/git-mergetool.sh\n@@ -264,17 +264,35 @@ merge_keep_temporaries=\"$(git config --bool mergetool.keepTemporaries || echo fa\n \n last_status=0\n rollup_status=0\n+rerere=false\n+\n+files_to_merge() {\n+    if test \"$rerere\" = true\n+    then\n+\tgit rerere status\n+    else\n+\tgit ls-files -u | sed -e 's/^[^\t]*\t//' | sort -u\n+    fi\n+}\n+\n \n if test $# -eq 0 ; then\n-    files=$(git ls-files -u | sed -e 's/^[^\t]*\t//' | sort -u)\n+    cd_to_toplevel\n+\n+    if test -e \"$GIT_DIR/MERGE_RR\"\n+    then\n+\trerere=true\n+    fi\n+\n+    files=$(files_to_merge)\n     if test -z \"$files\" ; then\n \techo \"No files need merging\"\n \texit 0\n     fi\n-    echo Merging the files: \"$files\"\n-    git ls-files -u |\n-    sed -e 's/^[^\t]*\t//' |\n-    sort -u |\n+    printf \"Merging:\\n\"\n+    printf \"$files\\n\"\n+\n+    files_to_merge |\n     while IFS= read i\n     do\n \tif test $last_status -ne 0; then\ndiff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh\nindex e768c3e..f5a7bf4 100755\n--- a/t/t7610-mergetool.sh\n+++ b/t/t7610-mergetool.sh\n@@ -14,6 +14,7 @@ Testing basic merge tool invocation'\n # running mergetool\n \n test_expect_success 'setup' '\n+    git config rerere.enabled true &&\n     echo master >file1 &&\n     mkdir subdir &&\n     echo master sub >subdir/file3 &&\n@@ -71,19 +72,40 @@ test_expect_success 'mergetool in subdir' '\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+    test \"$(cat file3)\" = \"master new sub\") &&\n+    cd ..\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+test_expect_success 'mergetool on file in parent dir' '\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+    cd ..\n+'\n+\n+test_expect_success 'mergetool skips autoresolved' '\n+    git checkout -b test4 branch1 &&\n+    test_must_fail git merge master &&\n+    test -n \"$(git ls-files -u)\" &&\n+    output=\"$(git mergetool --no-prompt)\" &&\n+    test \"$output\" = \"No files need merging\" &&\n+    git reset --hard\n+'\n+\n+test_expect_success 'mergetool merges all from subdir' '\n+    cd subdir && (\n+    git config rerere.enabled false &&\n+    test_must_fail git merge master &&\n+    git mergetool --no-prompt &&\n+    test \"$(cat ../file1)\" = \"master updated\" &&\n+    test \"$(cat ../file2)\" = \"master new\" &&\n+    test \"$(cat file3)\" = \"master new sub\" &&\n+    git add ../file1 ../file2 file3 &&\n+    git commit -m \"branch2 resolved by mergetool from subdir\") &&\n+    cd ..\n+'\n \n test_done\n-- \n1.7.2.1.98.gd9365.dirty\n"},{"id":"148441","messageId":"201008191202.36508.trast@student.ethz.ch","threadId":"24722","inReplyTo":"1282036966-26799-1-git-send-email-davvid@gmail.com","subject":"Re: [PATCH] mergetool: Skip autoresolved paths","fromName":"Thomas Rast","fromEmail":"trast@student.ethz.ch","sentAt":"2010-08-19T10:02:36Z","receivedAt":"2010-08-19T10:02:36Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"David Aguilar wrote:\n> When mergetool is run without path limiters it loops\n> over each entry in 'git ls-files -u'.  This includes\n> autoresolved paths.\n[...]\n> +test_expect_success 'mergetool merges all from subdir' '\n> +    cd subdir && (\n> +    git config rerere.enabled false &&\n> +    test_must_fail git merge master &&\n> +    git mergetool --no-prompt &&\n> +    test \"$(cat ../file1)\" = \"master updated\" &&\n> +    test \"$(cat ../file2)\" = \"master new\" &&\n> +    test \"$(cat file3)\" = \"master new sub\" &&\n> +    git add ../file1 ../file2 file3 &&\n> +    git commit -m \"branch2 resolved by mergetool from subdir\") &&\n> +    cd ..\n> +'\n\nThis test never worked in my automatic testing (it fails and bisects\nto this commit).\n\nIt might be because the cronjob doesn't have a tty, as I'm seeing the\noutput below (note the error at the end).  Any insights?\n\nexpecting success: \n    cd subdir && (\n    git config rerere.enabled false &&\n    test_must_fail git merge master &&\n    git mergetool --no-prompt &&\n    test \"$(cat ../file1)\" = \"master updated\" &&\n    test \"$(cat ../file2)\" = \"master new\" &&\n    test \"$(cat file3)\" = \"master new sub\" &&\n    git add ../file1 ../file2 file3 &&\n    git commit -m \"branch2 resolved by mergetool from subdir\") &&\n    cd ..\n\nMerging:\na8bf666 branch1 changes\nvirtual master\nfound 1 common ancestor(s):\n775c381 added file1\nAuto-merging file1\nCONFLICT (content): Merge conflict in file1\nAuto-merging file2\nCONFLICT (add/add): Merge conflict in file2\nAuto-merging subdir/file3\nCONFLICT (content): Merge conflict in subdir/file3\nAutomatic merge failed; fix conflicts and then commit the result.\nMerging:\nfile1\nfile2\nsubdir/file3\n\n/local/home/trast/git/t/valgrind/bin/git-mergetool: line 302: /dev/tty: No such device\n or address\n/local/home/trast/git/t/valgrind/bin/git-mergetool: line 299: /dev/tty: No such device\n or address\n\n-- \nThomas Rast\ntrast@{inf,student}.ethz.ch\n"},{"id":"148499","messageId":"20100820035236.GA18267@gmail.com","threadId":"24722","inReplyTo":"201008191202.36508.trast@student.ethz.ch","subject":"Re: [PATCH] mergetool: Skip autoresolved paths","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2010-08-20T03:52:46Z","receivedAt":"2010-08-20T03:52:46Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"On Thu, Aug 19, 2010 at 12:02:36PM +0200, Thomas Rast wrote:\n> David Aguilar wrote:\n> > When mergetool is run without path limiters it loops\n> > over each entry in 'git ls-files -u'.  This includes\n> > autoresolved paths.\n> [...]\n> > +test_expect_success 'mergetool merges all from subdir' '\n> > +    cd subdir && (\n> > +    git config rerere.enabled false &&\n> > +    test_must_fail git merge master &&\n> > +    git mergetool --no-prompt &&\n> > +    test \"$(cat ../file1)\" = \"master updated\" &&\n> > +    test \"$(cat ../file2)\" = \"master new\" &&\n> > +    test \"$(cat file3)\" = \"master new sub\" &&\n> > +    git add ../file1 ../file2 file3 &&\n> > +    git commit -m \"branch2 resolved by mergetool from subdir\") &&\n> > +    cd ..\n> > +'\n> \n> This test never worked in my automatic testing (it fails and bisects\n> to this commit).\n> \n> It might be because the cronjob doesn't have a tty, as I'm seeing the\n> output below (note the error at the end).  Any insights?\n\nIt must be the tty.\n\n\n> expecting success: \n>     cd subdir && (\n>     git config rerere.enabled false &&\n>     test_must_fail git merge master &&\n>     git mergetool --no-prompt &&\n>     test \"$(cat ../file1)\" = \"master updated\" &&\n>     test \"$(cat ../file2)\" = \"master new\" &&\n>     test \"$(cat file3)\" = \"master new sub\" &&\n>     git add ../file1 ../file2 file3 &&\n>     git commit -m \"branch2 resolved by mergetool from subdir\") &&\n>     cd ..\n> [...]\n> /local/home/trast/git/t/valgrind/bin/git-mergetool: line 302: /dev/tty: No such device\n>  or address\n> /local/home/trast/git/t/valgrind/bin/git-mergetool: line 299: /dev/tty: No such device\n>  or address\n\n\ngit-mergetool lines 295-307:\n\n    files_to_merge |\n    while IFS= read i\n    do\n\tif test $last_status -ne 0; then\n\t    prompt_after_failed_merge < /dev/tty || exit 1\n\tfi\n\tprintf \"\\n\"\n\tmerge_file \"$i\" < /dev/tty > /dev/tty\n\tlast_status=$?\n\tif test $last_status -ne 0; then\n\t    rollup_status=1\n\tfi\n    done\n\nThe reason the test fails without a tty is that we've never\nexercised this code in the past.\n\nThis commit did not introduce the \"< /dev/tty > /dev/tty\"\nidiom.  It was introduced in b0169d84 by Charles Bailey.\nWhat this commit did do was add test coverage to it,\nwhich is good because it uncovered this problem :-)\n\nCharles, is there another way we can write this?\nIs there a reason why we need the tty redirection?\nCan we drop it or is there a portability concern?\n\nFWIW, the merge_file call in the else clause that follows\nthis section does not use tty redirection.\n\n-- \n\n\tDavid\n"},{"id":"148520","messageId":"4C6E519E.1080700@hashpling.org","threadId":"24722","inReplyTo":"20100820035236.GA18267@gmail.com","subject":"Re: [PATCH] mergetool: Skip autoresolved paths","fromName":"Charles Bailey","fromEmail":"charles@hashpling.org","sentAt":"2010-08-20T09:57:50Z","receivedAt":"2010-08-20T09:57:50Z","isPatch":true,"sender":{"key":"charles@hashpling.org","avatar":"https://avatars.githubusercontent.com/u/1668475?v=4"},"body":"On 20/08/2010 04:52, David Aguilar wrote:\n>\n> git-mergetool lines 295-307:\n>\n>      files_to_merge |\n>      while IFS= read i\n>      do\n> \tif test $last_status -ne 0; then\n> \t    prompt_after_failed_merge<  /dev/tty || exit 1\n> \tfi\n> \tprintf \"\\n\"\n> \tmerge_file \"$i\"<  /dev/tty>  /dev/tty\n> \tlast_status=$?\n> \tif test $last_status -ne 0; then\n> \t    rollup_status=1\n> \tfi\n>      done\n>\n> The reason the test fails without a tty is that we've never\n> exercised this code in the past.\n>\n> This commit did not introduce the \"<  /dev/tty>  /dev/tty\"\n> idiom.  It was introduced in b0169d84 by Charles Bailey.\n> What this commit did do was add test coverage to it,\n> which is good because it uncovered this problem :-)\n>\n> Charles, is there another way we can write this?\n> Is there a reason why we need the tty redirection?\n> Can we drop it or is there a portability concern?\n>\n> FWIW, the merge_file call in the else clause that follows\n> this section does not use tty redirection.\n>\n\nActually, it's been like this since c4b4a5af which is when mergetool was \nintroduced.\n\n(b0169d84 didn't change this line, 0eea3451 but made only whitespace \nchanges, it comes from the original mergetool code.)\n\nWhen you say \"drop it\" what are you proposing to replace it with? We're \nin the middle of a shell pipe which has replaced stdin and merge_file \nneeds access to the human on it's stdin; hence the </dev/tty. Strictly. \nI believe that the >/dev/tty isn't needed.\n\nIs there some way of juggling file descriptors in shell? I had a quick \nplay with this but suspect it's a bashism (and it might make mergetool \nless readable!).\n\necho hidden | { echo lost | cat 0<&3- ; } 3<&0\n\nmergetool has never really been very approachable for automatic testing \nas it's fundamentally an interactive script. It would be nice if \nsufficient of the guts of mergetool were in testable library code and \nmergetool was just an obviously correct slim shell UI.\n\nmerge_file in the 'else' doesn't need the redirection as nobody has \nredirected the original stdin.\n\nCharles.\n"},{"id":"148521","messageId":"20100820100957.GB32127@burratino","threadId":"24722","inReplyTo":"4C6E519E.1080700@hashpling.org","subject":"Re: [PATCH] mergetool: Skip autoresolved paths","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-08-20T10:09:57Z","receivedAt":"2010-08-20T10:09:57Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Charles Bailey wrote:\n\n> We're in the middle of a shell pipe which has replaced stdin and\n> merge_file needs access to the human on it's stdin; hence the\n> </dev/tty. Strictly.\n[...]\n> Is there some way of juggling file descriptors in shell?\n\nYou can duplicate important fds, like so:\n\n exec 3<&0\n\n foo |\n (\n\tbar\n\tbaz\n\tquuz <&3\n )\n\n> I had a\n> quick play with this but suspect it's a bashism\n\nIt's standard, luckily.  See http://unix.org/2008edition/\n\nHope that helps.\n"},{"id":"148541","messageId":"1282303049-11201-1-git-send-email-charles@hashpling.org","threadId":"24722","inReplyTo":"20100820035236.GA18267@gmail.com","subject":"[PATCH] mergetool: Remove explicit references to /dev/tty","fromName":"Charles Bailey","fromEmail":"charles@hashpling.org","sentAt":"2010-08-20T11:17:29Z","receivedAt":"2010-08-20T11:17:29Z","isPatch":true,"sender":{"key":"charles@hashpling.org","avatar":"https://avatars.githubusercontent.com/u/1668475?v=4"},"body":"mergetool used /dev/tty to switch back to receiving input from the user\nvia inside a block with a redirected stdin.\n\nThis harms testability, so change mergetool to save its original stdin\nto an alternative fd in this block and restore it for those sub-commands\nthat need the original stdin.\n\nSigned-off-by: Charles Bailey <charles@hashpling.org>\n---\n\nThis works on my fedora 12 box with bash. The redirects should be\nstandard but this could do with some testing on other bourne shell\nimplementations.\n\n git-mergetool--lib.sh |    2 +-\n git-mergetool.sh      |    7 ++++---\n 2 files changed, 5 insertions(+), 4 deletions(-)\n\ndiff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh\nindex 51dd0d6..b5e1943 100644\n--- a/git-mergetool--lib.sh\n+++ b/git-mergetool--lib.sh\n@@ -35,7 +35,7 @@ check_unchanged () {\n \t\twhile true; do\n \t\t\techo \"$MERGED seems unchanged.\"\n \t\t\tprintf \"Was the merge successful? [y/n] \"\n-\t\t\tread answer < /dev/tty\n+\t\t\tread answer\n \t\t\tcase \"$answer\" in\n \t\t\ty*|Y*) status=0; break ;;\n \t\t\tn*|N*) status=1; break ;;\ndiff --git a/git-mergetool.sh b/git-mergetool.sh\nindex bd7ab02..84edf7d 100755\n--- a/git-mergetool.sh\n+++ b/git-mergetool.sh\n@@ -292,14 +292,15 @@ if test $# -eq 0 ; then\n     printf \"Merging:\\n\"\n     printf \"$files\\n\"\n \n-    files_to_merge |\n+    # Save original stdin to fd 3\n+    files_to_merge 3<&0 |\n     while IFS= read i\n     do\n \tif test $last_status -ne 0; then\n-\t    prompt_after_failed_merge < /dev/tty || exit 1\n+\t    prompt_after_failed_merge <&3 || exit 1\n \tfi\n \tprintf \"\\n\"\n-\tmerge_file \"$i\" < /dev/tty > /dev/tty\n+\tmerge_file \"$i\" <&3\n \tlast_status=$?\n \tif test $last_status -ne 0; then\n \t    rollup_status=1\n-- \n1.7.2.2.110.gf04b9.dirty\n"},{"id":"148548","messageId":"20100820122724.GS10407@burratino","threadId":"24722","inReplyTo":"1282303049-11201-1-git-send-email-charles@hashpling.org","subject":"Re: [PATCH] mergetool: Remove explicit references to /dev/tty","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-08-20T12:27:25Z","receivedAt":"2010-08-20T12:27:25Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Charles Bailey wrote:\n\n> mergetool used /dev/tty to switch back to receiving input from the user\n> via inside a block with a redirected stdin.\n> \n> This harms testability, so change mergetool to save its original stdin\n> to an alternative fd in this block and restore it for those sub-commands\n> that need the original stdin.\n\nSounds good.\n\n> +++ b/git-mergetool--lib.sh\n> @@ -35,7 +35,7 @@ check_unchanged () {\n>  \t\twhile true; do\n>  \t\t\techo \"$MERGED seems unchanged.\"\n>  \t\t\tprintf \"Was the merge successful? [y/n] \"\n> -\t\t\tread answer < /dev/tty\n> +\t\t\tread answer\n\nPart of the run_merge_tool codepath.  The only place this is called\nwith TOOL_MODE=merge is by merge_file which has stdin redirected,\nso this should be safe.  Good.\n\n> +++ b/git-mergetool.sh\n> @@ -292,14 +292,15 @@ if test $# -eq 0 ; then\n>      printf \"Merging:\\n\"\n>      printf \"$files\\n\"\n>  \n> -    files_to_merge |\n> +    # Save original stdin to fd 3\n> +    files_to_merge 3<&0 |\n\nI would think this should work, but it doesn't feel idiomatic.  Why\nnot save stdin a little earlier, so the reader does not have to track\ndown whether it has been redirected?\n\nThe test quietly passes for me with dash but fails with ksh:\n\n /home/jrn/src/git4/git-mergetool: line 303: 3: cannot open [Bad file descriptor]\n\nWith the patch below on top, it passes with dash and ksh.\n---\ndiff --git a/git-mergetool.sh b/git-mergetool.sh\nindex 84edf7d..2e82522 100755\n--- a/git-mergetool.sh\n+++ b/git-mergetool.sh\n@@ -275,10 +275,13 @@ files_to_merge() {\n     fi\n }\n \n \n if test $# -eq 0 ; then\n     cd_to_toplevel\n \n+    # Save original stdin\n+    exec 3<&0\n+\n     if test -e \"$GIT_DIR/MERGE_RR\"\n     then\n \trerere=true\n@@ -292,8 +294,7 @@ if test $# -eq 0 ; then\n     printf \"Merging:\\n\"\n     printf \"$files\\n\"\n \n-    # Save original stdin to fd 3\n-    files_to_merge 3<&0 |\n+    files_to_merge |\n     while IFS= read i\n     do\n \tif test $last_status -ne 0; then\n-- \n"},{"id":"148566","messageId":"4C6E883A.2030301@hashpling.org","threadId":"24722","inReplyTo":"20100820122724.GS10407@burratino","subject":"Re: [PATCH] mergetool: Remove explicit references to /dev/tty","fromName":"Charles Bailey","fromEmail":"charles@hashpling.org","sentAt":"2010-08-20T13:50:50Z","receivedAt":"2010-08-20T13:50:50Z","isPatch":true,"sender":{"key":"charles@hashpling.org","avatar":"https://avatars.githubusercontent.com/u/1668475?v=4"},"body":"On 20/08/2010 13:27, Jonathan Nieder wrote:\n>> +++ b/git-mergetool.sh\n>> @@ -292,14 +292,15 @@ if test $# -eq 0 ; then\n>>       printf \"Merging:\\n\"\n>>       printf \"$files\\n\"\n>>\n>> -    files_to_merge |\n>> +    # Save original stdin to fd 3\n>> +    files_to_merge 3<&0 |\n>\n> I would think this should work, but it doesn't feel idiomatic.  Why\n> not save stdin a little earlier, so the reader does not have to track\n> down whether it has been redirected?\n\nNo special reason, I just thought it was more natural to save it at the \ntime that we do the redirect..\n\n> The test quietly passes for me with dash but fails with ksh:\n>\n>   /home/jrn/src/git4/git-mergetool: line 303: 3: cannot open [Bad file descriptor]\n\n... but given that this approach is evidently less portable your way is \nclearly better.\n\n> With the patch below on top, it passes with dash and ksh.\n\nThanks, I'll re-roll in a bit at squash your fixes in, if that's OK?\n\nCharles.\n"},{"id":"148569","messageId":"20100820141913.GE16190@burratino","threadId":"24722","inReplyTo":"4C6E883A.2030301@hashpling.org","subject":"Re: [PATCH] mergetool: Remove explicit references to /dev/tty","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-08-20T14:19:13Z","receivedAt":"2010-08-20T14:19:13Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Charles Bailey wrote:\n> On 20/08/2010 13:27, Jonathan Nieder wrote:\n\n>> With the patch below on top, it passes with dash and ksh.\n>\n> Thanks, I'll re-roll in a bit at squash your fixes in, if that's OK?\n\nYeah, that's okay. :)\n\nThanks for your work.\n"},{"id":"148577","messageId":"1282317909-13628-1-git-send-email-charles@hashpling.org","threadId":"24722","inReplyTo":"20100820122724.GS10407@burratino","subject":"[PATCH v2] mergetool: Remove explicit references to /dev/tty","fromName":"Charles Bailey","fromEmail":"charles@hashpling.org","sentAt":"2010-08-20T15:25:09Z","receivedAt":"2010-08-20T15:25:09Z","isPatch":true,"sender":{"key":"charles@hashpling.org","avatar":"https://avatars.githubusercontent.com/u/1668475?v=4"},"body":"mergetool used /dev/tty to switch back to receiving input from the user\nvia inside a block with a redirected stdin.\n\nThis harms testability, so change mergetool to save its original stdin\nto an alternative fd in this block and restore it for those sub-commands\nthat need the original stdin.\n\nIncludes additional compatibility fix from Jonathan Nieder.\n\nTested-by: Jonathan Nieder <jrnieder@gmail.com>\nSigned-off-by: Charles Bailey <charles@hashpling.org>\n---\n\nNow works on ksh as well as bash and dash.\n\n git-mergetool--lib.sh |    2 +-\n git-mergetool.sh      |    7 +++++--\n 2 files changed, 6 insertions(+), 3 deletions(-)\n\ndiff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh\nindex 51dd0d6..b5e1943 100644\n--- a/git-mergetool--lib.sh\n+++ b/git-mergetool--lib.sh\n@@ -35,7 +35,7 @@ check_unchanged () {\n \t\twhile true; do\n \t\t\techo \"$MERGED seems unchanged.\"\n \t\t\tprintf \"Was the merge successful? [y/n] \"\n-\t\t\tread answer < /dev/tty\n+\t\t\tread answer\n \t\t\tcase \"$answer\" in\n \t\t\ty*|Y*) status=0; break ;;\n \t\t\tn*|N*) status=1; break ;;\ndiff --git a/git-mergetool.sh b/git-mergetool.sh\nindex bd7ab02..165b700 100755\n--- a/git-mergetool.sh\n+++ b/git-mergetool.sh\n@@ -279,6 +279,9 @@ files_to_merge() {\n if test $# -eq 0 ; then\n     cd_to_toplevel\n \n+    # Save original stdin\n+    exec 3<&0\n+\n     if test -e \"$GIT_DIR/MERGE_RR\"\n     then\n \trerere=true\n@@ -296,10 +299,10 @@ if test $# -eq 0 ; then\n     while IFS= read i\n     do\n \tif test $last_status -ne 0; then\n-\t    prompt_after_failed_merge < /dev/tty || exit 1\n+\t    prompt_after_failed_merge <&3 || exit 1\n \tfi\n \tprintf \"\\n\"\n-\tmerge_file \"$i\" < /dev/tty > /dev/tty\n+\tmerge_file \"$i\" <&3\n \tlast_status=$?\n \tif test $last_status -ne 0; then\n \t    rollup_status=1\n-- \n1.7.2.2.110.gf04b9.dirty\n"}]}