* Status of conflicted files resolved with rerere @ 2010-08-12 21:28 Magnus Bäck 2010-08-12 21:36 ` Avery Pennarun 0 siblings, 1 reply; 16+ messages in thread From: Magnus Bäck @ 2010-08-12 21:28 UTC (permalink / raw) To: git I played around with git rerere today and was surprised by the results. When a conflict has been resolved automatically by rerere, the file isn't added to the index like other files are where Git just used one of the regular merge resolution algorithms. What's worse, if git mergetool is invoked -- which is what I normally do when git merge needs help -- it has no idea that the file actually has been merged already, and launches the merge tool with the three files involved in the merge. If the user hasn't been paying attention to each line of the git merge output (stating the files who were automatically resolved) it's easy to trash rerere's work. Would it make sense for git merge to add rerere'd files (where all hunks were recognized and resolved) to the index automatically? That would make it possible for the user to run git mergetool without reading every single line of output from previous commands just to figure out which files should be added to the index straight off and which files require merging. For users resolving conflicts by editing the file and fiddling with the <<<<<<<<< marks etc such a change wouldn't make that big difference, but for mergetool users it seems like a quite useful improvement. Or is there something I'm failing to grasp here? This is on Git 1.7.0.3 by the way. -- Magnus Bäck Opinions are my own and do not necessarily SW Configuration Manager represent the ones of my employer, etc. Sony Ericsson ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: Status of conflicted files resolved with rerere 2010-08-12 21:28 Status of conflicted files resolved with rerere Magnus Bäck @ 2010-08-12 21:36 ` Avery Pennarun 2010-08-13 17:19 ` Jay Soffian 2010-08-15 2:24 ` David Aguilar 0 siblings, 2 replies; 16+ messages in thread From: Avery Pennarun @ 2010-08-12 21:36 UTC (permalink / raw) To: Magnus Bäck; +Cc: git On Thu, Aug 12, 2010 at 5:28 PM, Magnus Bäck <magnus.back@sonyericsson.com> wrote: > I played around with git rerere today and was surprised by the results. > When a conflict has been resolved automatically by rerere, the file > isn't added to the index like other files are where Git just used one of > the regular merge resolution algorithms. What's worse, if git mergetool > is invoked -- which is what I normally do when git merge needs help -- > it has no idea that the file actually has been merged already, and > launches the merge tool with the three files involved in the merge. If > the user hasn't been paying attention to each line of the git merge > output (stating the files who were automatically resolved) it's easy to > trash rerere's work. The motivation behind the current behaviour, as I understand it, is that rerere speeds things up, but you don't necessarily want to trust that it has resolved your merge conflicts correctly. After all, they were unarguably *conflicts*, not just normal merge results, so you can't quite trust them. That said, I've never had a problem where rerere did the wrong thing for me. Maybe there could be an option to override it. Anyway, I never use a mergetool, so like you suspected, this has never been a major problem for me. It sounds like the real problem here though it the mergetool stuff. Why is it disregarding all the automated merging that git has done and starting over from scratch? If git, in its infinite cleverness, has resolved *some* parts of the file but not others, wouldn't we want it to keep those resolutions? It sounds like mergetool is actually making things *more* work instead of less. Is there some way to teach the mergetool stuff to be smarter? At the very least, having it auto-skip files that have no *remaining* conflicts might be a good idea. Have fun, Avery ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: Status of conflicted files resolved with rerere 2010-08-12 21:36 ` Avery Pennarun @ 2010-08-13 17:19 ` Jay Soffian 2010-08-15 2:24 ` David Aguilar 1 sibling, 0 replies; 16+ messages in thread From: Jay Soffian @ 2010-08-13 17:19 UTC (permalink / raw) To: Avery Pennarun; +Cc: Magnus Bäck, git On Thu, Aug 12, 2010 at 5:36 PM, Avery Pennarun <apenwarr@gmail.com> wrote: > That said, I've never had a problem where rerere did the wrong thing > for me. Maybe there could be an option to override it. $ git config rerere.autoupdate true j. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: Status of conflicted files resolved with rerere 2010-08-12 21:36 ` Avery Pennarun 2010-08-13 17:19 ` Jay Soffian @ 2010-08-15 2:24 ` David Aguilar 2010-08-15 6:59 ` Junio C Hamano 1 sibling, 1 reply; 16+ messages in thread From: David Aguilar @ 2010-08-15 2:24 UTC (permalink / raw) To: Avery Pennarun; +Cc: Magnus Bäck, git@vger.kernel.org On Aug 12, 2010, at 2:36 PM, Avery Pennarun <apenwarr@gmail.com> wrote: > On Thu, Aug 12, 2010 at 5:28 PM, Magnus Bäck > <magnus.back@sonyericsson.com> wrote: >> I played around with git rerere today and was surprised by the >> results. >> When a conflict has been resolved automatically by rerere, the file >> isn't added to the index like other files are where Git just used >> one of >> the regular merge resolution algorithms. What's worse, if git >> mergetool >> is invoked -- which is what I normally do when git merge needs help >> -- >> it has no idea that the file actually has been merged already, and >> launches the merge tool with the three files involved in the merge. >> If >> the user hasn't been paying attention to each line of the git merge >> output (stating the files who were automatically resolved) it's >> easy to >> trash rerere's work. > [...] > It sounds like the real problem here though it the mergetool stuff. > Why is it disregarding all the automated merging that git has done and > starting over from scratch? If git, in its infinite cleverness, has > resolved *some* parts of the file but not others, wouldn't we want it > to keep those resolutions? It sounds like mergetool is actually > making things *more* work instead of less. > > Is there some way to teach the mergetool stuff to be smarter? At the > very least, having it auto-skip files that have no *remaining* > conflicts might be a good idea. > > Have fun, > > Avery Right, that would be a great enhancement. The problem is that mergetool always uses local, remote, and base. It uses the unmerged flag in the index when deciding which files to consider. Here's what we'd need in order to improve rerere and mergetool interaction: the ability to answer the question, "has this file been rerere merged?" Once we have that then we can make mergetool skip these files by default. We would also need a command line flag to override the behaviour. Perhaps a naive grep for merge markers in the worktree file would suffice? Heuristics have gone a long way in git so doing something like that wouldn't seem too atrocious. It'd also have the benefit that mergetool would skip files merged by $EDITOR. If anyone has any tips I'm all ears. Does this seem reasonable, and if so, what would be a good name for the command line flag to get the existing behavior? My gut feeling is that it shouldn't have a corresponding config variable. Cheers, -- David ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: Status of conflicted files resolved with rerere 2010-08-15 2:24 ` David Aguilar @ 2010-08-15 6:59 ` Junio C Hamano 2010-08-15 16:00 ` Magnus Bäck 2010-08-17 9:22 ` [PATCH] mergetool: Skip autoresolved paths David Aguilar 0 siblings, 2 replies; 16+ messages in thread From: Junio C Hamano @ 2010-08-15 6:59 UTC (permalink / raw) To: David Aguilar; +Cc: Avery Pennarun, Magnus Bäck, git@vger.kernel.org David Aguilar <davvid@gmail.com> writes: > Here's what we'd need in order to improve rerere and mergetool > interaction: the ability to answer the question, "has this file been > rerere merged?" I do not quite understand why the user _runs_ mergetool on a file that has been already merged; isn't it an option not to do so in the first place? Having said that. I think you can use the fact that: - "ls-files -u" will list paths with conflicts; and - "rerere status" won't mention the ones that have been autoresolved if rerere is in effect (check for presense of .git/MERGE_RR). ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: Status of conflicted files resolved with rerere 2010-08-15 6:59 ` Junio C Hamano @ 2010-08-15 16:00 ` Magnus Bäck 2010-08-17 9:22 ` [PATCH] mergetool: Skip autoresolved paths David Aguilar 1 sibling, 0 replies; 16+ messages in thread From: Magnus Bäck @ 2010-08-15 16:00 UTC (permalink / raw) To: Junio C Hamano; +Cc: David Aguilar, Avery Pennarun, git On Sunday, August 15, 2010 at 08:59 CEST, Junio C Hamano <gitster@pobox.com> wrote: > David Aguilar <davvid@gmail.com> writes: > > > Here's what we'd need in order to improve rerere and mergetool > > interaction: the ability to answer the question, "has this file > > been rerere merged?" > > I do not quite understand why the user _runs_ mergetool on a file that > has been already merged; isn't it an option not to do so in the first > place? You have a point, but if there are conflicts in many files where only a subset were autoresolved I think it would be prudent to help the user. Grepping after remaining conflict markers or keeping the "git merge" output somewhere to see which files actually were autoresolved works but I think we can do better. On the other hand, hinting mergetool users about rerere.autoupdate is perhaps good enough? Doesn't help users who want to inspect the autoresolved results yet also want hassle-free mergetool usage, though. > Having said that. > > I think you can use the fact that: > > - "ls-files -u" will list paths with conflicts; and > > - "rerere status" won't mention the ones that have been autoresolved > > if rerere is in effect (check for presense of .git/MERGE_RR). Okay, I'll have a stab at a patch. -- Magnus Bäck Opinions are my own and do not necessarily SW Configuration Manager represent the ones of my employer, etc. Sony Ericsson ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH] mergetool: Skip autoresolved paths 2010-08-15 6:59 ` Junio C Hamano 2010-08-15 16:00 ` Magnus Bäck @ 2010-08-17 9:22 ` David Aguilar 2010-08-19 10:02 ` Thomas Rast 1 sibling, 1 reply; 16+ messages in thread From: David Aguilar @ 2010-08-17 9:22 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Magnus Bäck, Charles Bailey, Avery Pennarun When mergetool is run without path limiters it loops over each entry in 'git ls-files -u'. This includes autoresolved paths. Teach mergetool to only merge files listed in 'rerere status' when rerere is enabled. There are some subtle but harmless changes in behavior. We now call cd_to_toplevel when no paths are given. We do this because 'rerere status' paths are always relative to the root. This is beneficial for the non-rerere use as well in that mergetool now runs against all unmerged files regardless of the current directory. This also slightly tweaks the output when run without paths to be more readable. The old output: Merging the files: foo bar baz The new output: Merging: foo bar baz Signed-off-by: David Aguilar <davvid@gmail.com> --- git-mergetool.sh | 28 +++++++++++++++++++++++----- t/t7610-mergetool.sh | 46 ++++++++++++++++++++++++++++++++++------------ 2 files changed, 57 insertions(+), 17 deletions(-) diff --git a/git-mergetool.sh b/git-mergetool.sh index b52a741..bd7ab02 100755 --- a/git-mergetool.sh +++ b/git-mergetool.sh @@ -264,17 +264,35 @@ merge_keep_temporaries="$(git config --bool mergetool.keepTemporaries || echo fa last_status=0 rollup_status=0 +rerere=false + +files_to_merge() { + if test "$rerere" = true + then + git rerere status + else + git ls-files -u | sed -e 's/^[^ ]* //' | sort -u + fi +} + if test $# -eq 0 ; then - files=$(git ls-files -u | sed -e 's/^[^ ]* //' | sort -u) + cd_to_toplevel + + if test -e "$GIT_DIR/MERGE_RR" + then + rerere=true + fi + + files=$(files_to_merge) if test -z "$files" ; then echo "No files need merging" exit 0 fi - echo Merging the files: "$files" - git ls-files -u | - sed -e 's/^[^ ]* //' | - sort -u | + printf "Merging:\n" + printf "$files\n" + + files_to_merge | while IFS= read i do if test $last_status -ne 0; then diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh index e768c3e..f5a7bf4 100755 --- a/t/t7610-mergetool.sh +++ b/t/t7610-mergetool.sh @@ -14,6 +14,7 @@ Testing basic merge tool invocation' # running mergetool test_expect_success 'setup' ' + git config rerere.enabled true && echo master >file1 && mkdir subdir && echo master sub >subdir/file3 && @@ -71,19 +72,40 @@ test_expect_success 'mergetool in subdir' ' cd subdir && ( test_must_fail git merge master >/dev/null 2>&1 && ( yes "" | git mergetool file3 >/dev/null 2>&1 ) && - test "$(cat file3)" = "master new sub" ) + test "$(cat file3)" = "master new sub") && + cd .. ' -# We can't merge files from parent directories when running mergetool -# from a subdir. Is this a bug? -# -#test_expect_failure 'mergetool in subdir' ' -# cd subdir && ( -# ( yes "" | git mergetool ../file1 >/dev/null 2>&1 ) && -# ( yes "" | git mergetool ../file2 >/dev/null 2>&1 ) && -# test "$(cat ../file1)" = "master updated" && -# test "$(cat ../file2)" = "master new" && -# git commit -m "branch1 resolved with mergetool - subdir" ) -#' +test_expect_success 'mergetool on file in parent dir' ' + cd subdir && ( + ( yes "" | git mergetool ../file1 >/dev/null 2>&1 ) && + ( yes "" | git mergetool ../file2 >/dev/null 2>&1 ) && + test "$(cat ../file1)" = "master updated" && + test "$(cat ../file2)" = "master new" && + git commit -m "branch1 resolved with mergetool - subdir") && + cd .. +' + +test_expect_success 'mergetool skips autoresolved' ' + git checkout -b test4 branch1 && + test_must_fail git merge master && + test -n "$(git ls-files -u)" && + output="$(git mergetool --no-prompt)" && + test "$output" = "No files need merging" && + git reset --hard +' + +test_expect_success 'mergetool merges all from subdir' ' + cd subdir && ( + git config rerere.enabled false && + test_must_fail git merge master && + git mergetool --no-prompt && + test "$(cat ../file1)" = "master updated" && + test "$(cat ../file2)" = "master new" && + test "$(cat file3)" = "master new sub" && + git add ../file1 ../file2 file3 && + git commit -m "branch2 resolved by mergetool from subdir") && + cd .. +' test_done -- 1.7.2.1.98.gd9365.dirty ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH] mergetool: Skip autoresolved paths 2010-08-17 9:22 ` [PATCH] mergetool: Skip autoresolved paths David Aguilar @ 2010-08-19 10:02 ` Thomas Rast 2010-08-20 3:52 ` David Aguilar 0 siblings, 1 reply; 16+ messages in thread From: Thomas Rast @ 2010-08-19 10:02 UTC (permalink / raw) To: David Aguilar Cc: Junio C Hamano, git, Magnus Bäck, Charles Bailey, Avery Pennarun David Aguilar wrote: > When mergetool is run without path limiters it loops > over each entry in 'git ls-files -u'. This includes > autoresolved paths. [...] > +test_expect_success 'mergetool merges all from subdir' ' > + cd subdir && ( > + git config rerere.enabled false && > + test_must_fail git merge master && > + git mergetool --no-prompt && > + test "$(cat ../file1)" = "master updated" && > + test "$(cat ../file2)" = "master new" && > + test "$(cat file3)" = "master new sub" && > + git add ../file1 ../file2 file3 && > + git commit -m "branch2 resolved by mergetool from subdir") && > + cd .. > +' This test never worked in my automatic testing (it fails and bisects to this commit). It might be because the cronjob doesn't have a tty, as I'm seeing the output below (note the error at the end). Any insights? expecting success: cd subdir && ( git config rerere.enabled false && test_must_fail git merge master && git mergetool --no-prompt && test "$(cat ../file1)" = "master updated" && test "$(cat ../file2)" = "master new" && test "$(cat file3)" = "master new sub" && git add ../file1 ../file2 file3 && git commit -m "branch2 resolved by mergetool from subdir") && cd .. Merging: a8bf666 branch1 changes virtual master found 1 common ancestor(s): 775c381 added file1 Auto-merging file1 CONFLICT (content): Merge conflict in file1 Auto-merging file2 CONFLICT (add/add): Merge conflict in file2 Auto-merging subdir/file3 CONFLICT (content): Merge conflict in subdir/file3 Automatic merge failed; fix conflicts and then commit the result. Merging: file1 file2 subdir/file3 /local/home/trast/git/t/valgrind/bin/git-mergetool: line 302: /dev/tty: No such device or address /local/home/trast/git/t/valgrind/bin/git-mergetool: line 299: /dev/tty: No such device or address -- Thomas Rast trast@{inf,student}.ethz.ch ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] mergetool: Skip autoresolved paths 2010-08-19 10:02 ` Thomas Rast @ 2010-08-20 3:52 ` David Aguilar 2010-08-20 9:57 ` Charles Bailey 2010-08-20 11:17 ` [PATCH] mergetool: Remove explicit references to /dev/tty Charles Bailey 0 siblings, 2 replies; 16+ messages in thread From: David Aguilar @ 2010-08-20 3:52 UTC (permalink / raw) To: Thomas Rast Cc: Junio C Hamano, git, Magnus Bäck, Charles Bailey, Avery Pennarun On Thu, Aug 19, 2010 at 12:02:36PM +0200, Thomas Rast wrote: > David Aguilar wrote: > > When mergetool is run without path limiters it loops > > over each entry in 'git ls-files -u'. This includes > > autoresolved paths. > [...] > > +test_expect_success 'mergetool merges all from subdir' ' > > + cd subdir && ( > > + git config rerere.enabled false && > > + test_must_fail git merge master && > > + git mergetool --no-prompt && > > + test "$(cat ../file1)" = "master updated" && > > + test "$(cat ../file2)" = "master new" && > > + test "$(cat file3)" = "master new sub" && > > + git add ../file1 ../file2 file3 && > > + git commit -m "branch2 resolved by mergetool from subdir") && > > + cd .. > > +' > > This test never worked in my automatic testing (it fails and bisects > to this commit). > > It might be because the cronjob doesn't have a tty, as I'm seeing the > output below (note the error at the end). Any insights? It must be the tty. > expecting success: > cd subdir && ( > git config rerere.enabled false && > test_must_fail git merge master && > git mergetool --no-prompt && > test "$(cat ../file1)" = "master updated" && > test "$(cat ../file2)" = "master new" && > test "$(cat file3)" = "master new sub" && > git add ../file1 ../file2 file3 && > git commit -m "branch2 resolved by mergetool from subdir") && > cd .. > [...] > /local/home/trast/git/t/valgrind/bin/git-mergetool: line 302: /dev/tty: No such device > or address > /local/home/trast/git/t/valgrind/bin/git-mergetool: line 299: /dev/tty: No such device > or address git-mergetool lines 295-307: files_to_merge | while IFS= read i do if test $last_status -ne 0; then prompt_after_failed_merge < /dev/tty || exit 1 fi printf "\n" merge_file "$i" < /dev/tty > /dev/tty last_status=$? if test $last_status -ne 0; then rollup_status=1 fi done The reason the test fails without a tty is that we've never exercised this code in the past. This commit did not introduce the "< /dev/tty > /dev/tty" idiom. It was introduced in b0169d84 by Charles Bailey. What this commit did do was add test coverage to it, which is good because it uncovered this problem :-) Charles, is there another way we can write this? Is there a reason why we need the tty redirection? Can we drop it or is there a portability concern? FWIW, the merge_file call in the else clause that follows this section does not use tty redirection. -- David ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] mergetool: Skip autoresolved paths 2010-08-20 3:52 ` David Aguilar @ 2010-08-20 9:57 ` Charles Bailey 2010-08-20 10:09 ` Jonathan Nieder 2010-08-20 11:17 ` [PATCH] mergetool: Remove explicit references to /dev/tty Charles Bailey 1 sibling, 1 reply; 16+ messages in thread From: Charles Bailey @ 2010-08-20 9:57 UTC (permalink / raw) To: David Aguilar Cc: Thomas Rast, Junio C Hamano, git, Magnus Bäck, Avery Pennarun, Theodore Ts'o On 20/08/2010 04:52, David Aguilar wrote: > > git-mergetool lines 295-307: > > files_to_merge | > while IFS= read i > do > if test $last_status -ne 0; then > prompt_after_failed_merge< /dev/tty || exit 1 > fi > printf "\n" > merge_file "$i"< /dev/tty> /dev/tty > last_status=$? > if test $last_status -ne 0; then > rollup_status=1 > fi > done > > The reason the test fails without a tty is that we've never > exercised this code in the past. > > This commit did not introduce the "< /dev/tty> /dev/tty" > idiom. It was introduced in b0169d84 by Charles Bailey. > What this commit did do was add test coverage to it, > which is good because it uncovered this problem :-) > > Charles, is there another way we can write this? > Is there a reason why we need the tty redirection? > Can we drop it or is there a portability concern? > > FWIW, the merge_file call in the else clause that follows > this section does not use tty redirection. > Actually, it's been like this since c4b4a5af which is when mergetool was introduced. (b0169d84 didn't change this line, 0eea3451 but made only whitespace changes, it comes from the original mergetool code.) When you say "drop it" what are you proposing to replace it with? We're in the middle of a shell pipe which has replaced stdin and merge_file needs access to the human on it's stdin; hence the </dev/tty. Strictly. I believe that the >/dev/tty isn't needed. Is there some way of juggling file descriptors in shell? I had a quick play with this but suspect it's a bashism (and it might make mergetool less readable!). echo hidden | { echo lost | cat 0<&3- ; } 3<&0 mergetool has never really been very approachable for automatic testing as it's fundamentally an interactive script. It would be nice if sufficient of the guts of mergetool were in testable library code and mergetool was just an obviously correct slim shell UI. merge_file in the 'else' doesn't need the redirection as nobody has redirected the original stdin. Charles. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] mergetool: Skip autoresolved paths 2010-08-20 9:57 ` Charles Bailey @ 2010-08-20 10:09 ` Jonathan Nieder 0 siblings, 0 replies; 16+ messages in thread From: Jonathan Nieder @ 2010-08-20 10:09 UTC (permalink / raw) To: Charles Bailey Cc: David Aguilar, Thomas Rast, Junio C Hamano, git, Magnus Bäck, Avery Pennarun, Theodore Ts'o Charles Bailey wrote: > We're in the middle of a shell pipe which has replaced stdin and > merge_file needs access to the human on it's stdin; hence the > </dev/tty. Strictly. [...] > Is there some way of juggling file descriptors in shell? You can duplicate important fds, like so: exec 3<&0 foo | ( bar baz quuz <&3 ) > I had a > quick play with this but suspect it's a bashism It's standard, luckily. See http://unix.org/2008edition/ Hope that helps. ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH] mergetool: Remove explicit references to /dev/tty 2010-08-20 3:52 ` David Aguilar 2010-08-20 9:57 ` Charles Bailey @ 2010-08-20 11:17 ` Charles Bailey 2010-08-20 12:27 ` Jonathan Nieder 1 sibling, 1 reply; 16+ messages in thread From: Charles Bailey @ 2010-08-20 11:17 UTC (permalink / raw) To: David Aguilar, git Cc: Thomas Rast, Junio C Hamano, Magnus Bäck, Avery Pennarun, Jonathan Nieder, Charles Bailey mergetool used /dev/tty to switch back to receiving input from the user via inside a block with a redirected stdin. This harms testability, so change mergetool to save its original stdin to an alternative fd in this block and restore it for those sub-commands that need the original stdin. Signed-off-by: Charles Bailey <charles@hashpling.org> --- This works on my fedora 12 box with bash. The redirects should be standard but this could do with some testing on other bourne shell implementations. git-mergetool--lib.sh | 2 +- git-mergetool.sh | 7 ++++--- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh index 51dd0d6..b5e1943 100644 --- a/git-mergetool--lib.sh +++ b/git-mergetool--lib.sh @@ -35,7 +35,7 @@ check_unchanged () { while true; do echo "$MERGED seems unchanged." printf "Was the merge successful? [y/n] " - read answer < /dev/tty + read answer case "$answer" in y*|Y*) status=0; break ;; n*|N*) status=1; break ;; diff --git a/git-mergetool.sh b/git-mergetool.sh index bd7ab02..84edf7d 100755 --- a/git-mergetool.sh +++ b/git-mergetool.sh @@ -292,14 +292,15 @@ if test $# -eq 0 ; then printf "Merging:\n" printf "$files\n" - files_to_merge | + # Save original stdin to fd 3 + files_to_merge 3<&0 | while IFS= read i do if test $last_status -ne 0; then - prompt_after_failed_merge < /dev/tty || exit 1 + prompt_after_failed_merge <&3 || exit 1 fi printf "\n" - merge_file "$i" < /dev/tty > /dev/tty + merge_file "$i" <&3 last_status=$? if test $last_status -ne 0; then rollup_status=1 -- 1.7.2.2.110.gf04b9.dirty ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH] mergetool: Remove explicit references to /dev/tty 2010-08-20 11:17 ` [PATCH] mergetool: Remove explicit references to /dev/tty Charles Bailey @ 2010-08-20 12:27 ` Jonathan Nieder 2010-08-20 13:50 ` Charles Bailey 2010-08-20 15:25 ` [PATCH v2] " Charles Bailey 0 siblings, 2 replies; 16+ messages in thread From: Jonathan Nieder @ 2010-08-20 12:27 UTC (permalink / raw) To: Charles Bailey Cc: David Aguilar, git, Thomas Rast, Junio C Hamano, Magnus Bäck, Avery Pennarun Charles Bailey wrote: > mergetool used /dev/tty to switch back to receiving input from the user > via inside a block with a redirected stdin. > > This harms testability, so change mergetool to save its original stdin > to an alternative fd in this block and restore it for those sub-commands > that need the original stdin. Sounds good. > +++ b/git-mergetool--lib.sh > @@ -35,7 +35,7 @@ check_unchanged () { > while true; do > echo "$MERGED seems unchanged." > printf "Was the merge successful? [y/n] " > - read answer < /dev/tty > + read answer Part of the run_merge_tool codepath. The only place this is called with TOOL_MODE=merge is by merge_file which has stdin redirected, so this should be safe. Good. > +++ b/git-mergetool.sh > @@ -292,14 +292,15 @@ if test $# -eq 0 ; then > printf "Merging:\n" > printf "$files\n" > > - files_to_merge | > + # Save original stdin to fd 3 > + files_to_merge 3<&0 | I would think this should work, but it doesn't feel idiomatic. Why not save stdin a little earlier, so the reader does not have to track down whether it has been redirected? The test quietly passes for me with dash but fails with ksh: /home/jrn/src/git4/git-mergetool: line 303: 3: cannot open [Bad file descriptor] With the patch below on top, it passes with dash and ksh. --- diff --git a/git-mergetool.sh b/git-mergetool.sh index 84edf7d..2e82522 100755 --- a/git-mergetool.sh +++ b/git-mergetool.sh @@ -275,10 +275,13 @@ files_to_merge() { fi } if test $# -eq 0 ; then cd_to_toplevel + # Save original stdin + exec 3<&0 + if test -e "$GIT_DIR/MERGE_RR" then rerere=true @@ -292,8 +294,7 @@ if test $# -eq 0 ; then printf "Merging:\n" printf "$files\n" - # Save original stdin to fd 3 - files_to_merge 3<&0 | + files_to_merge | while IFS= read i do if test $last_status -ne 0; then -- ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH] mergetool: Remove explicit references to /dev/tty 2010-08-20 12:27 ` Jonathan Nieder @ 2010-08-20 13:50 ` Charles Bailey 2010-08-20 14:19 ` Jonathan Nieder 2010-08-20 15:25 ` [PATCH v2] " Charles Bailey 1 sibling, 1 reply; 16+ messages in thread From: Charles Bailey @ 2010-08-20 13:50 UTC (permalink / raw) To: Jonathan Nieder Cc: David Aguilar, git, Thomas Rast, Junio C Hamano, Magnus Bäck, Avery Pennarun On 20/08/2010 13:27, Jonathan Nieder wrote: >> +++ b/git-mergetool.sh >> @@ -292,14 +292,15 @@ if test $# -eq 0 ; then >> printf "Merging:\n" >> printf "$files\n" >> >> - files_to_merge | >> + # Save original stdin to fd 3 >> + files_to_merge 3<&0 | > > I would think this should work, but it doesn't feel idiomatic. Why > not save stdin a little earlier, so the reader does not have to track > down whether it has been redirected? No special reason, I just thought it was more natural to save it at the time that we do the redirect.. > The test quietly passes for me with dash but fails with ksh: > > /home/jrn/src/git4/git-mergetool: line 303: 3: cannot open [Bad file descriptor] ... but given that this approach is evidently less portable your way is clearly better. > With the patch below on top, it passes with dash and ksh. Thanks, I'll re-roll in a bit at squash your fixes in, if that's OK? Charles. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] mergetool: Remove explicit references to /dev/tty 2010-08-20 13:50 ` Charles Bailey @ 2010-08-20 14:19 ` Jonathan Nieder 0 siblings, 0 replies; 16+ messages in thread From: Jonathan Nieder @ 2010-08-20 14:19 UTC (permalink / raw) To: Charles Bailey Cc: David Aguilar, git, Thomas Rast, Junio C Hamano, Magnus Bäck, Avery Pennarun Charles Bailey wrote: > On 20/08/2010 13:27, Jonathan Nieder wrote: >> With the patch below on top, it passes with dash and ksh. > > Thanks, I'll re-roll in a bit at squash your fixes in, if that's OK? Yeah, that's okay. :) Thanks for your work. ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v2] mergetool: Remove explicit references to /dev/tty 2010-08-20 12:27 ` Jonathan Nieder 2010-08-20 13:50 ` Charles Bailey @ 2010-08-20 15:25 ` Charles Bailey 1 sibling, 0 replies; 16+ messages in thread From: Charles Bailey @ 2010-08-20 15:25 UTC (permalink / raw) To: David Aguilar, git Cc: Thomas Rast, Junio C Hamano, Magnus Bäck, Avery Pennarun, Jonathan Nieder, Charles Bailey mergetool used /dev/tty to switch back to receiving input from the user via inside a block with a redirected stdin. This harms testability, so change mergetool to save its original stdin to an alternative fd in this block and restore it for those sub-commands that need the original stdin. Includes additional compatibility fix from Jonathan Nieder. Tested-by: Jonathan Nieder <jrnieder@gmail.com> Signed-off-by: Charles Bailey <charles@hashpling.org> --- Now works on ksh as well as bash and dash. git-mergetool--lib.sh | 2 +- git-mergetool.sh | 7 +++++-- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh index 51dd0d6..b5e1943 100644 --- a/git-mergetool--lib.sh +++ b/git-mergetool--lib.sh @@ -35,7 +35,7 @@ check_unchanged () { while true; do echo "$MERGED seems unchanged." printf "Was the merge successful? [y/n] " - read answer < /dev/tty + read answer case "$answer" in y*|Y*) status=0; break ;; n*|N*) status=1; break ;; diff --git a/git-mergetool.sh b/git-mergetool.sh index bd7ab02..165b700 100755 --- a/git-mergetool.sh +++ b/git-mergetool.sh @@ -279,6 +279,9 @@ files_to_merge() { if test $# -eq 0 ; then cd_to_toplevel + # Save original stdin + exec 3<&0 + if test -e "$GIT_DIR/MERGE_RR" then rerere=true @@ -296,10 +299,10 @@ if test $# -eq 0 ; then while IFS= read i do if test $last_status -ne 0; then - prompt_after_failed_merge < /dev/tty || exit 1 + prompt_after_failed_merge <&3 || exit 1 fi printf "\n" - merge_file "$i" < /dev/tty > /dev/tty + merge_file "$i" <&3 last_status=$? if test $last_status -ne 0; then rollup_status=1 -- 1.7.2.2.110.gf04b9.dirty ^ permalink raw reply related [flat|nested] 16+ messages in thread
end of thread, other threads:[~2010-08-20 15:26 UTC | newest] Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2010-08-12 21:28 Status of conflicted files resolved with rerere Magnus Bäck 2010-08-12 21:36 ` Avery Pennarun 2010-08-13 17:19 ` Jay Soffian 2010-08-15 2:24 ` David Aguilar 2010-08-15 6:59 ` Junio C Hamano 2010-08-15 16:00 ` Magnus Bäck 2010-08-17 9:22 ` [PATCH] mergetool: Skip autoresolved paths David Aguilar 2010-08-19 10:02 ` Thomas Rast 2010-08-20 3:52 ` David Aguilar 2010-08-20 9:57 ` Charles Bailey 2010-08-20 10:09 ` Jonathan Nieder 2010-08-20 11:17 ` [PATCH] mergetool: Remove explicit references to /dev/tty Charles Bailey 2010-08-20 12:27 ` Jonathan Nieder 2010-08-20 13:50 ` Charles Bailey 2010-08-20 14:19 ` Jonathan Nieder 2010-08-20 15:25 ` [PATCH v2] " Charles Bailey
Code repositories for project(s) associated with this public inbox https://80x24.org/mirrors/git.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).