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

[Charles Bailey <charles@hashpling.org>]

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.