[PATCH] Offer to print changes while running git-mergetool

[PATCH] Offer to print changes while running git-mergetool

[Jonathan del Strother]

Add a "Show changes" option to each prompt in mergetool. This prints the conflicted changes on the current file, using 'git log -p --merge <file>'

Signed-off-by: Jonathan del Strother <jon.delStrother@bestbefore.tv>
---

I frequently find myself running git-mergetool, then finding halfway through that I need to review the changes that produced that conflict.
How about something like this, for showing the changes from within mergetool?

 git-mergetool.sh |   37 ++++++++++++++++++++++++++++++-------
 1 files changed, 30 insertions(+), 7 deletions(-)

diff --git a/git-mergetool.sh b/git-mergetool.sh
index 87fa88a..9df91d8 100755
--- a/git-mergetool.sh
+++ b/git-mergetool.sh
@@ -62,7 +62,7 @@ describe_file () {
 
 resolve_symlink_merge () {
     while true; do
-	printf "Use (l)ocal or (r)emote, or (a)bort? "
+	printf "Use (l)ocal or (r)emote, (s)how changes, or (a)bort? "
 	read ans
 	case "$ans" in
 	    [lL]*)
@@ -77,6 +77,12 @@ resolve_symlink_merge () {
 		cleanup_temp_files --save-backup
 		return 0
 		;;
+		[sS]*)
+		git log -p --merge "$MERGED"
+		printf "\n"
+		resolve_symlink_merge
+		return
+		;;
 	    [aA]*)
 		return 1
 		;;
@@ -87,9 +93,9 @@ resolve_symlink_merge () {
 resolve_deleted_merge () {
     while true; do
 	if base_present; then
-	    printf "Use (m)odified or (d)eleted file, or (a)bort? "
+	    printf "Use (m)odified or (d)eleted file, (s)how changes, or (a)bort? "
 	else
-	    printf "Use (c)reated or (d)eleted file, or (a)bort? "
+	    printf "Use (c)reated or (d)eleted file, (s)how changes, or (a)bort? "
 	fi
 	read ans
 	case "$ans" in
@@ -103,6 +109,12 @@ resolve_deleted_merge () {
 		cleanup_temp_files
 		return 0
 		;;
+		[sS]*)
+		git log -p --merge "$MERGED"
+		printf "\n"
+		resolve_deleted_merge
+		return
+		;;
 	    [aA]*)
 		return 1
 		;;
@@ -183,10 +195,21 @@ merge_file () {
     echo "Normal merge conflict for '$MERGED':"
     describe_file "$local_mode" "local" "$LOCAL"
     describe_file "$remote_mode" "remote" "$REMOTE"
-    if "$prompt" = true; then
-	printf "Hit return to start merge resolution tool (%s): " "$merge_tool"
-	read ans
-    fi
+	if "$prompt" = true; then
+		while true; do
+			printf "(S)how changes, or hit return to start merge resolution tool (%s): " "$merge_tool"
+			read ans
+			case "$ans" in
+				[sS]*)
+				git log -p --merge "$MERGED"
+				printf "\n"
+				;;
+				*)
+				break
+				;;
+			esac
+		done
+	fi
 
     case "$merge_tool" in
 	kdiff3)
-- 
1.6.1.2.390.gba743.dirty

Re: [PATCH] Offer to print changes while running git-mergetool

[Jonathan del Strother]

On Fri, Feb 6, 2009 at 2:32 PM, Jonathan del Strother
<jon.delStrother@bestbefore.tv> wrote:
> Add a "Show changes" option to each prompt in mergetool. This prints the conflicted changes on the current file, using 'git log -p --merge <file>'

Just discovered that this doesn't work so well when resolving merges
resulting from "git stash apply" - it produces "fatal: --merge without
MERGE_HEAD".  Should git-stash be setting MERGE_HEAD in this case, or
should I be using something other than 'git log --merge' to cover
these sorts of cases?

Re: [PATCH] Offer to print changes while running git-mergetool

[Junio C Hamano]

Jonathan del Strother <maillist@steelskies.com> writes:

> On Fri, Feb 6, 2009 at 2:32 PM, Jonathan del Strother
> <jon.delStrother@bestbefore.tv> wrote:
>> Add a "Show changes" option to each prompt in mergetool. This prints the conflicted changes on the current file, using 'git log -p --merge <file>'
>
> Just discovered that this doesn't work so well when resolving merges
> resulting from "git stash apply" - it produces "fatal: --merge without
> MERGE_HEAD".  Should git-stash be setting MERGE_HEAD in this case,

No no no, please absolutely don't.  MERGE_HEAD is an instruction to the
eventual commit to create a merge commit and use the commits recorded
there as other parents when it does so.  You do *NOT* want to end up with
a merge with random state after unstashing.  None among cherry-pick,
rebase, checkout -m (branch switching), nor am -3 should.

I'd suggest making the new action conditionally available, by using the
presense of MERGE_HEAD as a cue.

The thing is, these commands that can potentially end in conflict operate
only at the tree level, and not at the level of commit ancestry graph.
"log --merge" is all about following the commit ancestry graph, and for
conflicts left by these commands it is not a useful way to review.

Re: [PATCH] Offer to print changes while running git-mergetool

[Jonathan del Strother]

On Fri, Feb 6, 2009 at 5:47 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Jonathan del Strother <maillist@steelskies.com> writes:
>
>> On Fri, Feb 6, 2009 at 2:32 PM, Jonathan del Strother
>> <jon.delStrother@bestbefore.tv> wrote:
>>> Add a "Show changes" option to each prompt in mergetool. This prints the conflicted changes on the current file, using 'git log -p --merge <file>'
>>
>> Just discovered that this doesn't work so well when resolving merges
>> resulting from "git stash apply" - it produces "fatal: --merge without
>> MERGE_HEAD".  Should git-stash be setting MERGE_HEAD in this case,
>
> No no no, please absolutely don't.  MERGE_HEAD is an instruction to the
> eventual commit to create a merge commit and use the commits recorded
> there as other parents when it does so.  You do *NOT* want to end up with
> a merge with random state after unstashing.  None among cherry-pick,
> rebase, checkout -m (branch switching), nor am -3 should.
>
> I'd suggest making the new action conditionally available, by using the
> presense of MERGE_HEAD as a cue.
>
> The thing is, these commands that can potentially end in conflict operate
> only at the tree level, and not at the level of commit ancestry graph.
> "log --merge" is all about following the commit ancestry graph, and for
> conflicts left by these commands it is not a useful way to review.
>

Maybe I'm misunderstanding the issue, but it seems like showing the
conflicted changes somehow could still be useful.  eg If I've made
changes on branch A, stash, switch to branch B, apply the stash and
get conflicts, I'd still like to see the commits that produced those
conflicts.
Obviously for a stash 'merge', one side of the merge isn't that
interesting : it's just going to be all the stuff from my working copy
before it was stashed.  But wouldn't the other side of the merge (ie
changes made on branch B since A & B diverged) produce useful
information?

Off the top of my head, I guess I want something like  "git log -p
^stash HEAD", but obviously that doesn't work when dealing with, say,
"git stash apply stash@{2}".

Or is this not workable for some reason I'm not thinking of?

Re: [PATCH] Offer to print changes while running git-mergetool

[Junio C Hamano]

Jonathan del Strother <maillist@steelskies.com> writes:

> On Fri, Feb 6, 2009 at 5:47 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Jonathan del Strother <maillist@steelskies.com> writes:
>>
>>> On Fri, Feb 6, 2009 at 2:32 PM, Jonathan del Strother
>>> <jon.delStrother@bestbefore.tv> wrote:
>>>> Add a "Show changes" option to each prompt in mergetool. This prints the conflicted changes on the current file, using 'git log -p --merge <file>'
>>>
>>> Just discovered that this doesn't work so well when resolving merges
>>> resulting from "git stash apply" - it produces "fatal: --merge without
>>> MERGE_HEAD".  Should git-stash be setting MERGE_HEAD in this case,
>>
>> No no no, please absolutely don't.  MERGE_HEAD is an instruction to the
>> eventual commit to create a merge commit and use the commits recorded
>> there as other parents when it does so.  You do *NOT* want to end up with
>> a merge with random state after unstashing.  None among cherry-pick,
>> rebase, checkout -m (branch switching), nor am -3 should.
>>
>> I'd suggest making the new action conditionally available, by using the
>> presense of MERGE_HEAD as a cue.
>>
>> The thing is, these commands that can potentially end in conflict operate
>> only at the tree level, and not at the level of commit ancestry graph.
>> "log --merge" is all about following the commit ancestry graph, and for
>> conflicts left by these commands it is not a useful way to review.
>
> Maybe I'm misunderstanding the issue,...

There actually are two independent issues.  Sorry for being unclear.

My objection is about the issue (1) below.  I am pointing out (2) merely
as an issue you need to address if you were to invent a solution that does
not have the issue (1); it is not an objection.

(1) Writing MERGE_HEAD from "git stash apply" (or "git stash pop") is
    absolutely a wrong thing to do.

    Typically you use stash this way.

	... work work work, oops, got interrupted.
        $ git stash save
        ... do some unrelated work.
        ... perhaps switch branches, perhaps not, it does not matter.
        ... the important thing is you conclude this and result in
        ... a clean work tree state.
        $ git stash pop
        ... inspect and resolve issues, perhaps using git log or
        ... git diff or git mergetool
        ... continue to work refining what you were doing
        $ git commit

    Imagine what the last "git commit" does, IF your "stash pop" wrote
    any commit object name in MERGE_HEAD.  It will record the tree created
    from the index in a commit that is a merge between the current HEAD
    and whatever commit you wrote in MERGE_HEAD.  But you obviously did
    not want any merge --- you wanted a straight and honest single parent
    commit.

    It is Ok if your design is to come up with a marker that is different
    from MERGE_HEAD for "git stash pop" to tell "git log --merge" where
    the potential conflicts may be coming from, but using MERGE_HEAD as
    that marker is unacceptable for this reason.

(2) The set of commits on the left and right side of "git log --merge"
    is tied very closely to the topology over the three commits:

        $(git merge-base HEAD MERGE_HEAD), which is the base,
        HEAD, which is the right-hand side, and
        MERGE_HEAD, which is the left-hand side.

    During a normal merge resolution, "git log --merge $paths..." looks at
    the topology of this graph:

          x---x---MERGE_HEAD--??? merge result?
         /                   /
	MB---o---o---o---HEAD

    and it simply runs

	git log MERGE_HEAD...HEAD -- $paths

    Hence, x would appear on the left hand side and o on the right hand
    side in the resulting traversal (this makes difference especially if
    you did "git log --left-right --merge").

But "stash pop" and "cherry-pick" (they are the same thing) work on a
quite different topology.

If you stashed while on 'master' (whose tip is A), "git stash save" will
create commit S without moving the tip of 'master' (S is saved at the tip
of refs/stash):

      x---x---B topic
     /
    o---o---o---A master
                 .
                  ...S

When you unstash it on B, "git stash pop" does a three way merge between A
and S and B, but it does *not* use the usual ancestry topology.

It merges S on top of B as if A is the common ancestor (i.e. merge base).
If you actually commited S on the master and cherry-picked it on B,
exactly the same thing happens.

Because of the way the cherry-picking 3-way merge has to happen, the
"virtual ancestory chain" becomes like this:

                  *---*---*---x---x---B
                 /                     \
                A master                \ 
                 .                       \
                  ...S..................??? merge result?

where * are "anti-commits" that reverse the effects of the commits o in
the original history that lead to A [*2*].

First of all, "git log A...B -- $path" won't show the virtual topology
depicted above.  It will only show commits x and would ignore o, hence it
does not explain the conflicts at all [*1*].  You somehow need to devise a
way to pretend that the topology is like the above one to achieve an
output equivalent to "git log --merge" for a true merge situation.

It is not just the matter of "echo A >.git/MERGE_HEAD" (which is an
absolute no-no for totally unrelated reason, which is (1) above) to tell
"git log --merge" to pretend the topology is like the above when it does
its traversal.

"git log -p A...B -- $path" won't show them without such a change either,
but even if you somehow convinced the revision traversal to pretend the
three commits o before A should be shown, you also would need to teach it
to show them in reverse because they are now anti-commits.

As I said, the longer explanation of issue (2) in this message is not
meant as an objection.  I am explaining why "git log --merge [-p]" cannot
be used as-is for what you are trying to do.

My primary objection is "don't mess with MERGE_HEAD", which is the issue
(1) above.


[Footnote]

*1* you handwaved this issue away in your message saying that what you did
is not interesting, but I think that is a cop-out.  Why are we showing our
side when inspecting the history in a true merge situation if it is really
uninteresting?

*2* In addition, there is this little problem that you can cherry-pick
(and unstash) between disjoint histories, in which case there won't even
be such a virtual ancestry chain that I obtained by rotating the history a
bit in the above example.

Re: [PATCH] Offer to print changes while running git-mergetool

[Junio C Hamano]

Jonathan del Strother <jon.delStrother@bestbefore.tv> writes:

> Add a "Show changes" option to each prompt in mergetool. This prints the
> conflicted changes on the current file, using 'git log -p --merge
> <file>'

I think the patch should look like this, given the recent conversation I
had with you.  It seems that the script thinks the unit of indentation is
4-places, and case arms are indented from case/esac (neither of which is
the standard git shell script convention), and I tried to match that style
used in the existing code.

No, I didn't test it.

Charles volunteered to take over mergetool, so he is on the Cc: list.

 git-mergetool.sh |   60 ++++++++++++++++++++++++++++++++++++++++++-----------
 1 files changed, 47 insertions(+), 13 deletions(-)

diff --git c/git-mergetool.sh w/git-mergetool.sh
index 87fa88a..b8604d6 100755
--- c/git-mergetool.sh
+++ w/git-mergetool.sh
@@ -14,6 +14,13 @@ OPTIONS_SPEC=
 . git-sh-setup
 require_work_tree
 
+if test -f "$GIT_DIR/MERGE_HEAD"
+then
+    in_merge=t show_changes=", (s)how changes"
+else
+    in_merge=f show_changes=
+fi
+
 # Returns true if the mode reflects a symlink
 is_symlink () {
     test "$1" = 120000
@@ -62,22 +69,28 @@ describe_file () {
 
 resolve_symlink_merge () {
     while true; do
-	printf "Use (l)ocal or (r)emote, or (a)bort? "
+	printf "Use (l)ocal or (r)emote$show_changes, or (a)bort? "
 	read ans
-	case "$ans" in
-	    [lL]*)
+	case "$in_merge,$ans" in
+	    ?,[lL]*)
 		git checkout-index -f --stage=2 -- "$MERGED"
 		git add -- "$MERGED"
 		cleanup_temp_files --save-backup
 		return 0
 		;;
-	    [rR]*)
+	    ?,[rR]*)
 		git checkout-index -f --stage=3 -- "$MERGED"
 		git add -- "$MERGED"
 		cleanup_temp_files --save-backup
 		return 0
 		;;
-	    [aA]*)
+	    t,[sS]*)
+		git log -p --merge "$MERGED"
+		printf "\n"
+		resolve_symlink_merge
+		return
+		;;
+	    ?,[aA]*)
 		return 1
 		;;
 	    esac
@@ -87,23 +100,29 @@ resolve_symlink_merge () {
 resolve_deleted_merge () {
     while true; do
 	if base_present; then
-	    printf "Use (m)odified or (d)eleted file, or (a)bort? "
+	    printf "Use (m)odified or (d)eleted file$show_changes, or (a)bort? "
 	else
-	    printf "Use (c)reated or (d)eleted file, or (a)bort? "
+	    printf "Use (c)reated or (d)eleted file$show changes, or (a)bort? "
 	fi
 	read ans
-	case "$ans" in
-	    [mMcC]*)
+	case "$in_merge,$ans" in
+	    ?,[mMcC]*)
 		git add -- "$MERGED"
 		cleanup_temp_files --save-backup
 		return 0
 		;;
-	    [dD]*)
+	    ?,[dD]*)
 		git rm -- "$MERGED" > /dev/null
 		cleanup_temp_files
 		return 0
 		;;
-	    [aA]*)
+	    t,[sS]*)
+		git log -p --merge "$MERGED"
+		printf "\n"
+		resolve_deleted_merge
+		return
+		;;
+	    ?,[aA]*)
 		return 1
 		;;
 	    esac
@@ -184,8 +203,23 @@ merge_file () {
     describe_file "$local_mode" "local" "$LOCAL"
     describe_file "$remote_mode" "remote" "$REMOTE"
     if "$prompt" = true; then
-	printf "Hit return to start merge resolution tool (%s): " "$merge_tool"
-	read ans
+	while true; do
+	    case $in_merge in
+		t)	msg_head="(S)how changes, or h" ;;
+		f)	msg_head="H" ;;
+	    esac
+	    print "${msg_head}it return to start merge resolution tool (%s): " "$merge_tool"
+	    read ans
+	    case "$in_merge,$ans" in
+	        t,[sS]*)
+		    git log -p --merge "$MERGED"
+		    printf "\n"
+		    ;;
+		?,*)
+		    break
+		    ;;
+	    esac
+        done
     fi
 
     case "$merge_tool" in

Re: [PATCH] Offer to print changes while running git-mergetool

[Jonathan del Strother]

On Sat, Feb 7, 2009 at 8:11 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Jonathan del Strother <jon.delStrother@bestbefore.tv> writes:
>
>> Add a "Show changes" option to each prompt in mergetool. This prints the
>> conflicted changes on the current file, using 'git log -p --merge
>> <file>'
>
> I think the patch should look like this, given the recent conversation I
> had with you.  It seems that the script thinks the unit of indentation is
> 4-places, and case arms are indented from case/esac (neither of which is
> the standard git shell script convention), and I tried to match that style
> used in the existing code.
>
> No, I didn't test it.
>
> Charles volunteered to take over mergetool, so he is on the Cc: list.
>
>  git-mergetool.sh |   60 ++++++++++++++++++++++++++++++++++++++++++-----------
>  1 files changed, 47 insertions(+), 13 deletions(-)
>
> diff --git c/git-mergetool.sh w/git-mergetool.sh
> index 87fa88a..b8604d6 100755
> --- c/git-mergetool.sh
> +++ w/git-mergetool.sh
> @@ -14,6 +14,13 @@ OPTIONS_SPEC=
>  . git-sh-setup
>  require_work_tree
>
> +if test -f "$GIT_DIR/MERGE_HEAD"
> +then
> +    in_merge=t show_changes=", (s)how changes"
> +else
> +    in_merge=f show_changes=
> +fi
> +
>  # Returns true if the mode reflects a symlink
>  is_symlink () {
>     test "$1" = 120000
> @@ -62,22 +69,28 @@ describe_file () {
>
>  resolve_symlink_merge () {
>     while true; do
> -       printf "Use (l)ocal or (r)emote, or (a)bort? "
> +       printf "Use (l)ocal or (r)emote$show_changes, or (a)bort? "
>        read ans
> -       case "$ans" in
> -           [lL]*)
> +       case "$in_merge,$ans" in
> +           ?,[lL]*)
>                git checkout-index -f --stage=2 -- "$MERGED"
>                git add -- "$MERGED"
>                cleanup_temp_files --save-backup
>                return 0
>                ;;
> -           [rR]*)
> +           ?,[rR]*)
>                git checkout-index -f --stage=3 -- "$MERGED"
>                git add -- "$MERGED"
>                cleanup_temp_files --save-backup
>                return 0
>                ;;
> -           [aA]*)
> +           t,[sS]*)
> +               git log -p --merge "$MERGED"
> +               printf "\n"
> +               resolve_symlink_merge
> +               return
> +               ;;
> +           ?,[aA]*)
>                return 1
>                ;;
>            esac
> @@ -87,23 +100,29 @@ resolve_symlink_merge () {
>  resolve_deleted_merge () {
>     while true; do
>        if base_present; then
> -           printf "Use (m)odified or (d)eleted file, or (a)bort? "
> +           printf "Use (m)odified or (d)eleted file$show_changes, or (a)bort? "
>        else
> -           printf "Use (c)reated or (d)eleted file, or (a)bort? "
> +           printf "Use (c)reated or (d)eleted file$show changes, or (a)bort? "
>        fi
>        read ans
> -       case "$ans" in
> -           [mMcC]*)
> +       case "$in_merge,$ans" in
> +           ?,[mMcC]*)
>                git add -- "$MERGED"
>                cleanup_temp_files --save-backup
>                return 0
>                ;;
> -           [dD]*)
> +           ?,[dD]*)
>                git rm -- "$MERGED" > /dev/null
>                cleanup_temp_files
>                return 0
>                ;;
> -           [aA]*)
> +           t,[sS]*)
> +               git log -p --merge "$MERGED"
> +               printf "\n"
> +               resolve_deleted_merge
> +               return
> +               ;;
> +           ?,[aA]*)
>                return 1
>                ;;
>            esac
> @@ -184,8 +203,23 @@ merge_file () {
>     describe_file "$local_mode" "local" "$LOCAL"
>     describe_file "$remote_mode" "remote" "$REMOTE"
>     if "$prompt" = true; then
> -       printf "Hit return to start merge resolution tool (%s): " "$merge_tool"
> -       read ans
> +       while true; do
> +           case $in_merge in
> +               t)      msg_head="(S)how changes, or h" ;;
> +               f)      msg_head="H" ;;
> +           esac
> +           print "${msg_head}it return to start merge resolution tool (%s): " "$merge_tool"
> +           read ans
> +           case "$in_merge,$ans" in
> +               t,[sS]*)
> +                   git log -p --merge "$MERGED"
> +                   printf "\n"
> +                   ;;
> +               ?,*)
> +                   break
> +                   ;;
> +           esac
> +        done
>     fi
>
>     case "$merge_tool" in
>


Looks good to me, though there's a minor typo on this line :
print "${msg_head}it return to start merge resolution tool (%s): " "$merge_tool"

- print should be printf

Cheers,
Jonathan

Re: [PATCH] Offer to print changes while running git-mergetool

[Charles Bailey]

Jonathan del Strother wrote:
> On Sat, Feb 7, 2009 at 8:11 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> Jonathan del Strother <jon.delStrother@bestbefore.tv> writes:
>>
>>> Add a "Show changes" option to each prompt in mergetool. This prints the
>>> conflicted changes on the current file, using 'git log -p --merge
>>> <file>'
>> I think the patch should look like this, given the recent conversation I
>> had with you.  It seems that the script thinks the unit of indentation is
>> 4-places, and case arms are indented from case/esac (neither of which is
>> the standard git shell script convention), and I tried to match that style
>> used in the existing code.
>>
>> No, I didn't test it.
>>
>> Charles volunteered to take over mergetool, so he is on the Cc: list.

At the moment, I'm slightly cool towards this patch, but perhaps I don't
 really understand the underlying issue. I understand wanting to check
something (logs) in the middle of a mergetool run but I can't say that
I've ever wanted to specifically run 'git log -p --merge'. Perhaps some
users of mergetool - being visual people - would more naturally reach
for gitk?

Given that mergetool picks up from where it left off when run a second
time, what does this patch offer over Ctrl-c, run log tool of your
choice, re-run mergetool? Or just running git log in a different
terminal instance?

I've made couple of minor comments on the patch below.

Charles.


>>  git-mergetool.sh |   60 ++++++++++++++++++++++++++++++++++++++++++-----------
>>  1 files changed, 47 insertions(+), 13 deletions(-)
>>
>> diff --git c/git-mergetool.sh w/git-mergetool.sh
>> index 87fa88a..b8604d6 100755
>> --- c/git-mergetool.sh
>> +++ w/git-mergetool.sh
>> @@ -14,6 +14,13 @@ OPTIONS_SPEC=
>>  . git-sh-setup
>>  require_work_tree
>>
>> +if test -f "$GIT_DIR/MERGE_HEAD"
>> +then
>> +    in_merge=t show_changes=", (s)how changes"
>> +else
>> +    in_merge=f show_changes=
>> +fi
>> +
>>  # Returns true if the mode reflects a symlink
>>  is_symlink () {
>>     test "$1" = 120000
>> @@ -62,22 +69,28 @@ describe_file () {
>>
>>  resolve_symlink_merge () {
>>     while true; do

--- snip ---

>>                ;;
>> -           [aA]*)
>> +           t,[sS]*)
>> +               git log -p --merge "$MERGED"
>> +               printf "\n"
>> +               resolve_symlink_merge
>> +               return
>> +               ;;

Slightly unusual recursion, why not just drop out to the 'while true' loop?

>>            esac
>> @@ -87,23 +100,29 @@ resolve_symlink_merge () {
>>  resolve_deleted_merge () {
>>     while true; do

--- snip ---

>>                ;;
>> -           [aA]*)
>> +           t,[sS]*)
>> +               git log -p --merge "$MERGED"
>> +               printf "\n"
>> +               resolve_deleted_merge
>> +               return
>> +               ;;

Resursion as above, but why not fall out to the 'while true' again?

>> @@ -184,8 +203,23 @@ merge_file () {
>>     describe_file "$local_mode" "local" "$LOCAL"
>>     describe_file "$remote_mode" "remote" "$REMOTE"
>>     if "$prompt" = true; then
>> -       printf "Hit return to start merge resolution tool (%s): " "$merge_tool"
>> -       read ans
>> +       while true; do
>> +           case $in_merge in
>> +               t)      msg_head="(S)how changes, or h" ;;
>> +               f)      msg_head="H" ;;
>> +           esac
>> +           print "${msg_head}it return to start merge resolution tool (%s): " "$merge_tool"
>> +           read ans
>> +           case "$in_merge,$ans" in
>> +               t,[sS]*)
>> +                   git log -p --merge "$MERGED"
>> +                   printf "\n"
>> +                   ;;

No recursion here, this feels a but more natural to me.

Re: [PATCH] Offer to print changes while running git-mergetool

[Jonathan del Strother]

On 2/8/09, Charles Bailey <charles@hashpling.org> wrote:
> Jonathan del Strother wrote:
>> On Sat, Feb 7, 2009 at 8:11 AM, Junio C Hamano <gitster@pobox.com> wrote:
>>> Jonathan del Strother <jon.delStrother@bestbefore.tv> writes:
>>>
>>>> Add a "Show changes" option to each prompt in mergetool. This prints the
>>>> conflicted changes on the current file, using 'git log -p --merge
>>>> <file>'
>>> I think the patch should look like this, given the recent conversation I
>>> had with you.  It seems that the script thinks the unit of indentation is
>>> 4-places, and case arms are indented from case/esac (neither of which is
>>> the standard git shell script convention), and I tried to match that
>>> style
>>> used in the existing code.
>>>
>>> No, I didn't test it.
>>>
>>> Charles volunteered to take over mergetool, so he is on the Cc: list.
>
> At the moment, I'm slightly cool towards this patch, but perhaps I don't
>  really understand the underlying issue. I understand wanting to check
> something (logs) in the middle of a mergetool run but I can't say that
> I've ever wanted to specifically run 'git log -p --merge'. Perhaps some
> users of mergetool - being visual people - would more naturally reach
> for gitk?
>
> Given that mergetool picks up from where it left off when run a second
> time, what does this patch offer over Ctrl-c, run log tool of your
> choice, re-run mergetool? Or just running git log in a different
> terminal instance?
>

A large part of my motivation behind this patch was basically
education - my team (and myself) have made poor merge decisions in the
past, largely due to not being aware of a tool like "git log --merge".
The patch was attempting to get inexperienced users to make better use
of such tools. I certainly wouldn't be averse to using gitk instead.

Re: [PATCH] Offer to print changes while running git-mergetool

[Charles Bailey]

Jonathan del Strother wrote:
>> Given that mergetool picks up from where it left off when run a second
>> time, what does this patch offer over Ctrl-c, run log tool of your
>> choice, re-run mergetool? Or just running git log in a different
>> terminal instance?
>>
> 
> A large part of my motivation behind this patch was basically
> education - my team (and myself) have made poor merge decisions in the
> past, largely due to not being aware of a tool like "git log --merge".
> The patch was attempting to get inexperienced users to make better use
> of such tools. I certainly wouldn't be averse to using gitk instead.

My point wasn't that we should use gitk instead, I was just trying to
illustrate that there may be a whole raft of other commands or
activities that a user might want to run to help them with a merge so
why should we hard wire any one of them into mergetool?

The other doubt that I've since had about this patch is this. Is just
before running the merge tool the best place to offer to show the log?

In my usual workflows (not necessarily the best!), I usually want to
fire up the merge tool as quickly as possible to get the merge
resolutions done.

Only once I'm in the mergetool do I realise that this one's a bit
complex and I might need to consult the logs to help me resolve this one.

But now, mergetool is blocked waiting for the merge tool to finish. If I
abort the merge it's going to offer me the option to abort completely or
carry on with the next file. (Perhaps "try again" could be a future
direction that mergetool might offer, but it doesn't at the moment.)

I'm far more likely to want to consult the logs in a different terminal
(or gui) with the merge tool still running, especially if I've already
merged some of the easy chunks and have only hit difficulties later on
in the merge.

Charles.