[RFC/PATCH] mergetool: clarify local/remote terminology

[RFC/PATCH] mergetool: clarify local/remote terminology

[Jay Soffian]

Always calling the thing on the left-side of a merge "local" and on the
right side "remote" isn't correct. You could be rebasing, in which case
the left side is the new base/upstream and the right side is the saved
commits which are being re-applied. So,

- Use left/right instead of local/remote internally to make the code
  clearer.

- Add --left and --right options to allow the user to specify their own
  preferred names for the left/right sides of the merge. Use these when
  constructing the filenames that are passed to the external mergetool.

- By default call left "local" and right "upstream", unless a rebase
  is in progress in which case left=upstream and right=local.

Signed-off-by: Jay Soffian <jaysoffian@gmail.com>
---
I think this makes things clearer. In particular I found it confusing when I ran
mergetool during a rebase and the file tagged LOCAL was the new base, not my
local changes, and REMOTE was actually my saved off commits. Fixing just that
problem is a simpler change, but I don't really think using local/remote is
right anyway.

I played around with trying to use the branch name for left side default name,
but again, this became confusing during a rebase, so I think local/upstream make
the most sense in general.

Thoughts?

 git-mergetool.sh |  118 ++++++++++++++++++++++++++++++++++++------------------
 1 files changed, 79 insertions(+), 39 deletions(-)

diff --git a/git-mergetool.sh b/git-mergetool.sh
index cbbb707..a12b0a2 100755
--- a/git-mergetool.sh
+++ b/git-mergetool.sh
@@ -8,7 +8,7 @@
 # at the discretion of Junio C Hamano.
 #
 
-USAGE='[--tool=tool] [file to merge] ...'
+USAGE='[--tool=tool] [--left=name] [--right=name] [file to merge] ...'
 SUBDIRECTORY_OK=Yes
 OPTIONS_SPEC=
 . git-sh-setup
@@ -20,12 +20,12 @@ is_symlink () {
     test "$1" = 120000
 }
 
-local_present () {
-    test -n "$local_mode"
+left_present () {
+    test -n "$left_mode"
 }
 
-remote_present () {
-    test -n "$remote_mode"
+right_present () {
+    test -n "$right_mode"
 }
 
 base_present () {
@@ -35,9 +35,9 @@ base_present () {
 cleanup_temp_files () {
     if test "$1" = --save-backup ; then
 	mv -- "$BACKUP" "$path.orig"
-	rm -f -- "$LOCAL" "$REMOTE" "$BASE"
+	rm -f -- "$LEFT" "$RIGHT" "$BASE"
     else
-	rm -f -- "$LOCAL" "$REMOTE" "$BASE" "$BACKUP"
+	rm -f -- "$LEFT" "$RIGHT" "$BASE" "$BACKUP"
     fi
 }
 
@@ -63,7 +63,8 @@ describe_file () {
 
 resolve_symlink_merge () {
     while true; do
-	printf "Use (l)ocal or (r)emote, or (a)bort? "
+	printf "Use (l)eft {%s} or (r)ight {%s}, or (a)bort? " \
+		"$left_desc" "$right_desc"
 	read ans
 	case "$ans" in
 	    [lL]*)
@@ -154,73 +155,79 @@ merge_file () {
 
     ext="$$$(expr "$path" : '.*\(\.[^/]*\)$')"
     BACKUP="$path.BACKUP.$ext"
-    LOCAL="$path.LOCAL.$ext"
-    REMOTE="$path.REMOTE.$ext"
+    LEFT_DESC=`echo "$left_desc" | tr "[:lower:]" "[:upper:]" | tr -c '[:alnum:]\n' _`
+    RIGHT_DESC=`echo "$right_desc" | tr "[:lower:]" "[:upper:]" | tr -c '[:alnum:]\n' _`
+    LEFT="$path.$LEFT_DESC.$ext"
+    RIGHT="$path.$RIGHT_DESC.$ext"
     BASE="$path.BASE.$ext"
 
     mv -- "$path" "$BACKUP"
     cp -- "$BACKUP" "$path"
 
     base_mode=`git ls-files -u -- "$path" | awk '{if ($3==1) print $1;}'`
-    local_mode=`git ls-files -u -- "$path" | awk '{if ($3==2) print $1;}'`
-    remote_mode=`git ls-files -u -- "$path" | awk '{if ($3==3) print $1;}'`
+    left_mode=`git ls-files -u -- "$path" | awk '{if ($3==2) print $1;}'`
+    right_mode=`git ls-files -u -- "$path" | awk '{if ($3==3) print $1;}'`
 
-    base_present   && git cat-file blob ":1:$prefix$path" >"$BASE" 2>/dev/null
-    local_present  && git cat-file blob ":2:$prefix$path" >"$LOCAL" 2>/dev/null
-    remote_present && git cat-file blob ":3:$prefix$path" >"$REMOTE" 2>/dev/null
+    base_present  && git cat-file blob ":1:$prefix$path" >"$BASE" 2>/dev/null
+    left_present  && git cat-file blob ":2:$prefix$path" >"$LEFT" 2>/dev/null
+    right_present && git cat-file blob ":3:$prefix$path" >"$RIGHT" 2>/dev/null
 
-    if test -z "$local_mode" -o -z "$remote_mode"; then
+    if test -z "$left_mode" -o -z "$right_mode"; then
 	echo "Deleted merge conflict for '$path':"
-	describe_file "$local_mode" "local" "$LOCAL"
-	describe_file "$remote_mode" "remote" "$REMOTE"
+	describe_file "$left_mode" "$left_desc" "$LEFT"
+	describe_file "$right_mode" "$right_desc" "$RIGHT"
 	resolve_deleted_merge
 	return
     fi
 
-    if is_symlink "$local_mode" || is_symlink "$remote_mode"; then
+    if is_symlink "$left_mode" || is_symlink "$right_mode"; then
 	echo "Symbolic link merge conflict for '$path':"
-	describe_file "$local_mode" "local" "$LOCAL"
-	describe_file "$remote_mode" "remote" "$REMOTE"
+	describe_file "$left_mode" "$left_desc" "$LEFT"
+	describe_file "$right_mode" "$right_desc" "$RIGHT"
 	resolve_symlink_merge
 	return
     fi
 
     echo "Normal merge conflict for '$path':"
-    describe_file "$local_mode" "local" "$LOCAL"
-    describe_file "$remote_mode" "remote" "$REMOTE"
+    describe_file "$left_mode" "$left_desc" "$LEFT"
+    describe_file "$right_mode" "$right_desc" "$RIGHT"
     printf "Hit return to start merge resolution tool (%s): " "$merge_tool"
     read ans
 
     case "$merge_tool" in
 	kdiff3)
+	    Left=$(echo "$left_desc" | \
+	           awk '{printf("%s%s",toupper(substr($0,1,1)),substr($0,2))}')
+	    Right=$(echo "$right_desc" | \
+	            awk '{printf("%s%s",toupper(substr($0,1,1)),substr($0,2))}')
 	    if base_present ; then
-		("$merge_tool_path" --auto --L1 "$path (Base)" --L2 "$path (Local)" --L3 "$path (Remote)" \
-		    -o "$path" -- "$BASE" "$LOCAL" "$REMOTE" > /dev/null 2>&1)
+		("$merge_tool_path" --auto --L1 "$path (Base)" --L2 "$path ($Left)" --L3 "$path ($Right)" \
+		    -o "$path" -- "$BASE" "$LEFT" "$RIGHT" > /dev/null 2>&1)
 	    else
-		("$merge_tool_path" --auto --L1 "$path (Local)" --L2 "$path (Remote)" \
-		    -o "$path" -- "$LOCAL" "$REMOTE" > /dev/null 2>&1)
+		("$merge_tool_path" --auto --L1 "$path ($Left)" --L2 "$path ($Right)" \
+		    -o "$path" -- "$LEFT" "$RIGHT" > /dev/null 2>&1)
 	    fi
 	    status=$?
 	    remove_backup
 	    ;;
 	tkdiff)
 	    if base_present ; then
-		"$merge_tool_path" -a "$BASE" -o "$path" -- "$LOCAL" "$REMOTE"
+		"$merge_tool_path" -a "$BASE" -o "$path" -- "$LEFT" "$RIGHT"
 	    else
-		"$merge_tool_path" -o "$path" -- "$LOCAL" "$REMOTE"
+		"$merge_tool_path" -o "$path" -- "$LEFT" "$RIGHT"
 	    fi
 	    status=$?
 	    save_backup
 	    ;;
 	meld|vimdiff)
 	    touch "$BACKUP"
-	    "$merge_tool_path" -- "$LOCAL" "$path" "$REMOTE"
+	    "$merge_tool_path" -- "$LEFT" "$path" "$RIGHT"
 	    check_unchanged
 	    save_backup
 	    ;;
 	gvimdiff)
 		touch "$BACKUP"
-		"$merge_tool_path" -f -- "$LOCAL" "$path" "$REMOTE"
+		"$merge_tool_path" -f -- "$LEFT" "$path" "$RIGHT"
 		check_unchanged
 		save_backup
 		;;
@@ -231,13 +238,13 @@ merge_file () {
 		    -R 'Accel.SaveAsMerged: "Ctrl-S"' \
 		    -R 'Accel.Search: "Ctrl+F"' \
 		    -R 'Accel.SearchForward: "Ctrl-G"' \
-		    --merged-file "$path" -- "$LOCAL" "$BASE" "$REMOTE"
+		    --merged-file "$path" -- "$LEFT" "$BASE" "$RIGHT"
 	    else
 		"$merge_tool_path" -X --show-merged-pane \
 		    -R 'Accel.SaveAsMerged: "Ctrl-S"' \
 		    -R 'Accel.Search: "Ctrl+F"' \
 		    -R 'Accel.SearchForward: "Ctrl-G"' \
-		    --merged-file "$path" -- "$LOCAL" "$REMOTE"
+		    --merged-file "$path" -- "$LEFT" "$RIGHT"
 	    fi
 	    check_unchanged
 	    save_backup
@@ -245,9 +252,9 @@ merge_file () {
 	opendiff)
 	    touch "$BACKUP"
 	    if base_present; then
-		"$merge_tool_path" "$LOCAL" "$REMOTE" -ancestor "$BASE" -merge "$path" | cat
+		"$merge_tool_path" "$LEFT" "$RIGHT" -ancestor "$BASE" -merge "$path" | cat
 	    else
-		"$merge_tool_path" "$LOCAL" "$REMOTE" -merge "$path" | cat
+		"$merge_tool_path" "$LEFT" "$RIGHT" -merge "$path" | cat
 	    fi
 	    check_unchanged
 	    save_backup
@@ -255,18 +262,18 @@ merge_file () {
 	ecmerge)
 	    touch "$BACKUP"
 	    if base_present; then
-		"$merge_tool_path" "$BASE" "$LOCAL" "$REMOTE" --mode=merge3 --to="$path"
+		"$merge_tool_path" "$BASE" "$LEFT" "$RIGHT" --mode=merge3 --to="$path"
 	    else
-		"$merge_tool_path" "$LOCAL" "$REMOTE" --mode=merge2 --to="$path"
+		"$merge_tool_path" "$LEFT" "$RIGHT" --mode=merge2 --to="$path"
 	    fi
 	    check_unchanged
 	    save_backup
 	    ;;
 	emerge)
 	    if base_present ; then
-		"$merge_tool_path" -f emerge-files-with-ancestor-command "$LOCAL" "$REMOTE" "$BASE" "$(basename "$path")"
+		"$merge_tool_path" -f emerge-files-with-ancestor-command "$LEFT" "$RIGHT" "$BASE" "$(basename "$path")"
 	    else
-		"$merge_tool_path" -f emerge-files-command "$LOCAL" "$REMOTE" "$(basename "$path")"
+		"$merge_tool_path" -f emerge-files-command "$LEFT" "$RIGHT" "$(basename "$path")"
 	    fi
 	    status=$?
 	    save_backup
@@ -296,6 +303,30 @@ do
 		    shift ;;
 	    esac
 	    ;;
+	-l|--left*)
+	    case "$#,$1" in
+		*,*=*)
+		    left_desc=`expr "z$1" : 'z-[^=]*=\(.*\)'`
+		    ;;
+		1,*)
+		    usage ;;
+		*)
+		    left_desc="$2"
+		    shift ;;
+	    esac
+	    ;;
+	-r|--right*)
+	    case "$#,$1" in
+		*,*=*)
+		    right_desc=`expr "z$1" : 'z-[^=]*=\(.*\)'`
+		    ;;
+		1,*)
+		    usage ;;
+		*)
+		    right_desc="$2"
+		    shift ;;
+	    esac
+	    ;;
 	--)
 	    break
 	    ;;
@@ -386,6 +417,15 @@ else
     fi
 fi
 
+if test -z "$left_desc" || test -z "$right_desc"; then
+    if test -d "$GIT_DIR/../.dotest" || test -f "$GIT_DIR/.dotest-merge/head-name"; then
+	left_desc="${left_desc:-upstream}"
+	right_desc="${right_desc:-local}"
+    else
+	left_desc="${left_desc:-local}"
+	right_desc="${right_desc:-upstream}"
+    fi
+fi
 
 if test $# -eq 0 ; then
 	files=`git ls-files -u | sed -e 's/^[^	]*	//' | sort -u`
-- 
1.5.4.2.236.g77b4.dirty

Re: [RFC/PATCH] mergetool: clarify local/remote terminology

[Jay Soffian]

Anyone?

On Thu, Feb 21, 2008 at 12:12 AM, Jay Soffian <jaysoffian@gmail.com> wrote:
> Always calling the thing on the left-side of a merge "local" and on the
>  right side "remote" isn't correct. You could be rebasing, in which case
>  the left side is the new base/upstream and the right side is the saved
>  commits which are being re-applied. So,
>
>  - Use left/right instead of local/remote internally to make the code
>   clearer.
>
>  - Add --left and --right options to allow the user to specify their own
>   preferred names for the left/right sides of the merge. Use these when
>   constructing the filenames that are passed to the external mergetool.
>
>  - By default call left "local" and right "upstream", unless a rebase
>   is in progress in which case left=upstream and right=local.
>
>  Signed-off-by: Jay Soffian <jaysoffian@gmail.com>
>  ---
>  I think this makes things clearer. In particular I found it confusing when I ran
>  mergetool during a rebase and the file tagged LOCAL was the new base, not my
>  local changes, and REMOTE was actually my saved off commits. Fixing just that
>  problem is a simpler change, but I don't really think using local/remote is
>  right anyway.
>
>  I played around with trying to use the branch name for left side default name,
>  but again, this became confusing during a rebase, so I think local/upstream make
>  the most sense in general.
>
>  Thoughts?
>
>   git-mergetool.sh |  118 ++++++++++++++++++++++++++++++++++++------------------
>   1 files changed, 79 insertions(+), 39 deletions(-)
>
>  diff --git a/git-mergetool.sh b/git-mergetool.sh
>  index cbbb707..a12b0a2 100755
>  --- a/git-mergetool.sh
>  +++ b/git-mergetool.sh
>  @@ -8,7 +8,7 @@
>   # at the discretion of Junio C Hamano.
>   #
>
>  -USAGE='[--tool=tool] [file to merge] ...'
>  +USAGE='[--tool=tool] [--left=name] [--right=name] [file to merge] ...'
>   SUBDIRECTORY_OK=Yes
>   OPTIONS_SPEC=
>   . git-sh-setup
>  @@ -20,12 +20,12 @@ is_symlink () {
>      test "$1" = 120000
>   }
>
>  -local_present () {
>  -    test -n "$local_mode"
>  +left_present () {
>  +    test -n "$left_mode"
>   }
>
>  -remote_present () {
>  -    test -n "$remote_mode"
>  +right_present () {
>  +    test -n "$right_mode"
>   }
>
>   base_present () {
>  @@ -35,9 +35,9 @@ base_present () {
>   cleanup_temp_files () {
>      if test "$1" = --save-backup ; then
>         mv -- "$BACKUP" "$path.orig"
>  -       rm -f -- "$LOCAL" "$REMOTE" "$BASE"
>  +       rm -f -- "$LEFT" "$RIGHT" "$BASE"
>      else
>  -       rm -f -- "$LOCAL" "$REMOTE" "$BASE" "$BACKUP"
>  +       rm -f -- "$LEFT" "$RIGHT" "$BASE" "$BACKUP"
>      fi
>   }
>
>  @@ -63,7 +63,8 @@ describe_file () {
>
>   resolve_symlink_merge () {
>      while true; do
>  -       printf "Use (l)ocal or (r)emote, or (a)bort? "
>  +       printf "Use (l)eft {%s} or (r)ight {%s}, or (a)bort? " \
>  +               "$left_desc" "$right_desc"
>         read ans
>         case "$ans" in
>             [lL]*)
>  @@ -154,73 +155,79 @@ merge_file () {
>
>      ext="$$$(expr "$path" : '.*\(\.[^/]*\)$')"
>      BACKUP="$path.BACKUP.$ext"
>  -    LOCAL="$path.LOCAL.$ext"
>  -    REMOTE="$path.REMOTE.$ext"
>  +    LEFT_DESC=`echo "$left_desc" | tr "[:lower:]" "[:upper:]" | tr -c '[:alnum:]\n' _`
>  +    RIGHT_DESC=`echo "$right_desc" | tr "[:lower:]" "[:upper:]" | tr -c '[:alnum:]\n' _`
>  +    LEFT="$path.$LEFT_DESC.$ext"
>  +    RIGHT="$path.$RIGHT_DESC.$ext"
>      BASE="$path.BASE.$ext"
>
>      mv -- "$path" "$BACKUP"
>      cp -- "$BACKUP" "$path"
>
>      base_mode=`git ls-files -u -- "$path" | awk '{if ($3==1) print $1;}'`
>  -    local_mode=`git ls-files -u -- "$path" | awk '{if ($3==2) print $1;}'`
>  -    remote_mode=`git ls-files -u -- "$path" | awk '{if ($3==3) print $1;}'`
>  +    left_mode=`git ls-files -u -- "$path" | awk '{if ($3==2) print $1;}'`
>  +    right_mode=`git ls-files -u -- "$path" | awk '{if ($3==3) print $1;}'`
>
>  -    base_present   && git cat-file blob ":1:$prefix$path" >"$BASE" 2>/dev/null
>  -    local_present  && git cat-file blob ":2:$prefix$path" >"$LOCAL" 2>/dev/null
>  -    remote_present && git cat-file blob ":3:$prefix$path" >"$REMOTE" 2>/dev/null
>  +    base_present  && git cat-file blob ":1:$prefix$path" >"$BASE" 2>/dev/null
>  +    left_present  && git cat-file blob ":2:$prefix$path" >"$LEFT" 2>/dev/null
>  +    right_present && git cat-file blob ":3:$prefix$path" >"$RIGHT" 2>/dev/null
>
>  -    if test -z "$local_mode" -o -z "$remote_mode"; then
>  +    if test -z "$left_mode" -o -z "$right_mode"; then
>         echo "Deleted merge conflict for '$path':"
>  -       describe_file "$local_mode" "local" "$LOCAL"
>  -       describe_file "$remote_mode" "remote" "$REMOTE"
>  +       describe_file "$left_mode" "$left_desc" "$LEFT"
>  +       describe_file "$right_mode" "$right_desc" "$RIGHT"
>         resolve_deleted_merge
>         return
>      fi
>
>  -    if is_symlink "$local_mode" || is_symlink "$remote_mode"; then
>  +    if is_symlink "$left_mode" || is_symlink "$right_mode"; then
>         echo "Symbolic link merge conflict for '$path':"
>  -       describe_file "$local_mode" "local" "$LOCAL"
>  -       describe_file "$remote_mode" "remote" "$REMOTE"
>  +       describe_file "$left_mode" "$left_desc" "$LEFT"
>  +       describe_file "$right_mode" "$right_desc" "$RIGHT"
>         resolve_symlink_merge
>         return
>      fi
>
>      echo "Normal merge conflict for '$path':"
>  -    describe_file "$local_mode" "local" "$LOCAL"
>  -    describe_file "$remote_mode" "remote" "$REMOTE"
>  +    describe_file "$left_mode" "$left_desc" "$LEFT"
>  +    describe_file "$right_mode" "$right_desc" "$RIGHT"
>      printf "Hit return to start merge resolution tool (%s): " "$merge_tool"
>      read ans
>
>      case "$merge_tool" in
>         kdiff3)
>  +           Left=$(echo "$left_desc" | \
>  +                  awk '{printf("%s%s",toupper(substr($0,1,1)),substr($0,2))}')
>  +           Right=$(echo "$right_desc" | \
>  +                   awk '{printf("%s%s",toupper(substr($0,1,1)),substr($0,2))}')
>             if base_present ; then
>  -               ("$merge_tool_path" --auto --L1 "$path (Base)" --L2 "$path (Local)" --L3 "$path (Remote)" \
>  -                   -o "$path" -- "$BASE" "$LOCAL" "$REMOTE" > /dev/null 2>&1)
>  +               ("$merge_tool_path" --auto --L1 "$path (Base)" --L2 "$path ($Left)" --L3 "$path ($Right)" \
>  +                   -o "$path" -- "$BASE" "$LEFT" "$RIGHT" > /dev/null 2>&1)
>             else
>  -               ("$merge_tool_path" --auto --L1 "$path (Local)" --L2 "$path (Remote)" \
>  -                   -o "$path" -- "$LOCAL" "$REMOTE" > /dev/null 2>&1)
>  +               ("$merge_tool_path" --auto --L1 "$path ($Left)" --L2 "$path ($Right)" \
>  +                   -o "$path" -- "$LEFT" "$RIGHT" > /dev/null 2>&1)
>             fi
>             status=$?
>             remove_backup
>             ;;
>         tkdiff)
>             if base_present ; then
>  -               "$merge_tool_path" -a "$BASE" -o "$path" -- "$LOCAL" "$REMOTE"
>  +               "$merge_tool_path" -a "$BASE" -o "$path" -- "$LEFT" "$RIGHT"
>             else
>  -               "$merge_tool_path" -o "$path" -- "$LOCAL" "$REMOTE"
>  +               "$merge_tool_path" -o "$path" -- "$LEFT" "$RIGHT"
>             fi
>             status=$?
>             save_backup
>             ;;
>         meld|vimdiff)
>             touch "$BACKUP"
>  -           "$merge_tool_path" -- "$LOCAL" "$path" "$REMOTE"
>  +           "$merge_tool_path" -- "$LEFT" "$path" "$RIGHT"
>             check_unchanged
>             save_backup
>             ;;
>         gvimdiff)
>                 touch "$BACKUP"
>  -               "$merge_tool_path" -f -- "$LOCAL" "$path" "$REMOTE"
>  +               "$merge_tool_path" -f -- "$LEFT" "$path" "$RIGHT"
>                 check_unchanged
>                 save_backup
>                 ;;
>  @@ -231,13 +238,13 @@ merge_file () {
>                     -R 'Accel.SaveAsMerged: "Ctrl-S"' \
>                     -R 'Accel.Search: "Ctrl+F"' \
>                     -R 'Accel.SearchForward: "Ctrl-G"' \
>  -                   --merged-file "$path" -- "$LOCAL" "$BASE" "$REMOTE"
>  +                   --merged-file "$path" -- "$LEFT" "$BASE" "$RIGHT"
>             else
>                 "$merge_tool_path" -X --show-merged-pane \
>                     -R 'Accel.SaveAsMerged: "Ctrl-S"' \
>                     -R 'Accel.Search: "Ctrl+F"' \
>                     -R 'Accel.SearchForward: "Ctrl-G"' \
>  -                   --merged-file "$path" -- "$LOCAL" "$REMOTE"
>  +                   --merged-file "$path" -- "$LEFT" "$RIGHT"
>             fi
>             check_unchanged
>             save_backup
>  @@ -245,9 +252,9 @@ merge_file () {
>         opendiff)
>             touch "$BACKUP"
>             if base_present; then
>  -               "$merge_tool_path" "$LOCAL" "$REMOTE" -ancestor "$BASE" -merge "$path" | cat
>  +               "$merge_tool_path" "$LEFT" "$RIGHT" -ancestor "$BASE" -merge "$path" | cat
>             else
>  -               "$merge_tool_path" "$LOCAL" "$REMOTE" -merge "$path" | cat
>  +               "$merge_tool_path" "$LEFT" "$RIGHT" -merge "$path" | cat
>             fi
>             check_unchanged
>             save_backup
>  @@ -255,18 +262,18 @@ merge_file () {
>         ecmerge)
>             touch "$BACKUP"
>             if base_present; then
>  -               "$merge_tool_path" "$BASE" "$LOCAL" "$REMOTE" --mode=merge3 --to="$path"
>  +               "$merge_tool_path" "$BASE" "$LEFT" "$RIGHT" --mode=merge3 --to="$path"
>             else
>  -               "$merge_tool_path" "$LOCAL" "$REMOTE" --mode=merge2 --to="$path"
>  +               "$merge_tool_path" "$LEFT" "$RIGHT" --mode=merge2 --to="$path"
>             fi
>             check_unchanged
>             save_backup
>             ;;
>         emerge)
>             if base_present ; then
>  -               "$merge_tool_path" -f emerge-files-with-ancestor-command "$LOCAL" "$REMOTE" "$BASE" "$(basename "$path")"
>  +               "$merge_tool_path" -f emerge-files-with-ancestor-command "$LEFT" "$RIGHT" "$BASE" "$(basename "$path")"
>             else
>  -               "$merge_tool_path" -f emerge-files-command "$LOCAL" "$REMOTE" "$(basename "$path")"
>  +               "$merge_tool_path" -f emerge-files-command "$LEFT" "$RIGHT" "$(basename "$path")"
>             fi
>             status=$?
>             save_backup
>  @@ -296,6 +303,30 @@ do
>                     shift ;;
>             esac
>             ;;
>  +       -l|--left*)
>  +           case "$#,$1" in
>  +               *,*=*)
>  +                   left_desc=`expr "z$1" : 'z-[^=]*=\(.*\)'`
>  +                   ;;
>  +               1,*)
>  +                   usage ;;
>  +               *)
>  +                   left_desc="$2"
>  +                   shift ;;
>  +           esac
>  +           ;;
>  +       -r|--right*)
>  +           case "$#,$1" in
>  +               *,*=*)
>  +                   right_desc=`expr "z$1" : 'z-[^=]*=\(.*\)'`
>  +                   ;;
>  +               1,*)
>  +                   usage ;;
>  +               *)
>  +                   right_desc="$2"
>  +                   shift ;;
>  +           esac
>  +           ;;
>         --)
>             break
>             ;;
>  @@ -386,6 +417,15 @@ else
>      fi
>   fi
>
>  +if test -z "$left_desc" || test -z "$right_desc"; then
>  +    if test -d "$GIT_DIR/../.dotest" || test -f "$GIT_DIR/.dotest-merge/head-name"; then
>  +       left_desc="${left_desc:-upstream}"
>  +       right_desc="${right_desc:-local}"
>  +    else
>  +       left_desc="${left_desc:-local}"
>  +       right_desc="${right_desc:-upstream}"
>  +    fi
>  +fi
>
>   if test $# -eq 0 ; then
>         files=`git ls-files -u | sed -e 's/^[^  ]*      //' | sort -u`
>  --
>  1.5.4.2.236.g77b4.dirty
>
>

Re: [RFC/PATCH] mergetool: clarify local/remote terminology

[Jeff King]

On Mon, Feb 25, 2008 at 09:31:03AM -0500, Jay Soffian wrote:

> Anyone?
> 
> On Thu, Feb 21, 2008 at 12:12 AM, Jay Soffian <jaysoffian@gmail.com> wrote:
> > Always calling the thing on the left-side of a merge "local" and on the
> >  right side "remote" isn't correct. You could be rebasing, in which case
> >  the left side is the new base/upstream and the right side is the saved
> >  commits which are being re-applied. So,

I have been annoyed by this in the past, too. And I think your change is
probably an improvement (though I haven't tried it yet). As a user, what
I would _really_ like to see, though, is simply the branch names:
"origin/next" versus "next". I looked at using the GITHEAD_* mechanism
in the past, but that relies on the environment, which can't get
directly to mergetool. See this thread:

http://mid.gmane.org/20070820075318.GA12478@coredump.intra.peff.net

I think the right thing to do is the oft-discussed-but-never-implemented
"what's happening right now" file/command that would say:

  1. there's a rebase happening now
  2. the upstream branch is X
  3. the rebased branch is Y

but that could also of course say

  1. there's a merge happening now
  2. the first parent is X
  3. the second parent is Y

and so on (including "we're in a git-am, git-cherry-pick, etc").

I'll try out your patch and comment next time I use it.

-Peff

Re: [RFC/PATCH] mergetool: clarify local/remote terminology

[Jay Soffian]

On Mon, Feb 25, 2008 at 1:46 PM, Jeff King <peff@peff.net> wrote:

>  I have been annoyed by this in the past, too. And I think your change is
>  probably an improvement (though I haven't tried it yet). As a user, what
>  I would _really_ like to see, though, is simply the branch names:
>  "origin/next" versus "next".

I was playing with that, figuring out branch names in the same way that the
code in bashcompletions computes ps1. However, what would you want to see if
you're doing an interactive rebase of say HEAD vs HEAD~5. They are both the
same branch, so what would you want to see?

>  I looked at using the GITHEAD_* mechanism
>  in the past, but that relies on the environment, which can't get
>  directly to mergetool. See this thread:
>
>  http://mid.gmane.org/20070820075318.GA12478@coredump.intra.peff.net
>
>  I think the right thing to do is the oft-discussed-but-never-implemented
>  "what's happening right now" file/command that would say:
>
>   1. there's a rebase happening now
>   2. the upstream branch is X
>   3. the rebased branch is Y
>
>  but that could also of course say
>
>   1. there's a merge happening now
>   2. the first parent is X
>   3. the second parent is Y
>
>  and so on (including "we're in a git-am, git-cherry-pick, etc").

Okay, I'll look into that.

>  I'll try out your patch and comment next time I use it.

Appreciated.

j.

Re: [RFC/PATCH] mergetool: clarify local/remote terminology

[Jeff King]

On Mon, Feb 25, 2008 at 02:07:11PM -0500, Jay Soffian wrote:

> I was playing with that, figuring out branch names in the same way that the
> code in bashcompletions computes ps1. However, what would you want to see if
> you're doing an interactive rebase of say HEAD vs HEAD~5. They are both the
> same branch, so what would you want to see?

Probably HEAD and HEAD~5 (perhaps along with their oneline logs). I
think the best we can do is "whatever the user said to get us here"
which is not something you can calculate after the fact; you have to
remember what the user said.

-Peff

Re: [RFC/PATCH] mergetool: clarify local/remote terminology

[Jay Soffian]

On Mon, Feb 25, 2008 at 2:21 PM, Jeff King <peff@peff.net> wrote:

>  Probably HEAD and HEAD~5 (perhaps along with their oneline logs). I
>  think the best we can do is "whatever the user said to get us here"
>  which is not something you can calculate after the fact; you have to
>  remember what the user said.

Right, so the porcelain would have to save this information somewhere
for mergetool to access (yes/no/maybe?).

Anyway, that's why the first iteration of my patch takes --left
and --right arguments so at least you can tell it how you got
there.

j.

Re: [RFC/PATCH] mergetool: clarify local/remote terminology

[Jeff King]

On Mon, Feb 25, 2008 at 03:09:04PM -0500, Jay Soffian wrote:

> >  Probably HEAD and HEAD~5 (perhaps along with their oneline logs). I
> >  think the best we can do is "whatever the user said to get us here"
> >  which is not something you can calculate after the fact; you have to
> >  remember what the user said.
> 
> Right, so the porcelain would have to save this information somewhere
> for mergetool to access (yes/no/maybe?).

Yes (that's what would go into a .git/status type of file).

> Anyway, that's why the first iteration of my patch takes --left
> and --right arguments so at least you can tell it how you got
> there.

Yes, I saw that. The problem is that git-mergetool is invoked by the
user. Is it really improving the usability to have to manually specify
--left and --right? Saving and restoring the state seems like the only
useful option.

-Peff

Re: [RFC/PATCH] mergetool: clarify local/remote terminology

[Jeff King]

On Mon, Feb 25, 2008 at 01:46:48PM -0500, Jeff King wrote:

> > On Thu, Feb 21, 2008 at 12:12 AM, Jay Soffian <jaysoffian@gmail.com> wrote:
> > > Always calling the thing on the left-side of a merge "local" and on the
> > >  right side "remote" isn't correct. You could be rebasing, in which case
> > >  the left side is the new base/upstream and the right side is the saved
> > >  commits which are being re-applied. So,
> 
> I'll try out your patch and comment next time I use it.

I finally got a chance to use this today (hey, I don't get a lot of
conflicts!).

I like it; I think it made it a lot more obvious which side was which
during the rebase. I checked with cherry-pick, as well; it more or less
makes sense, except that the cherry-picked commit is called "upstream."
Which sort of makes sense, but it would be nice to call it something
more obvious. Unfortunately I'm not sure that there is a good way to
determine we are in a failed cherry-pick (probably a failed 'revert' is
in a similar situation).

So like I said before, I think the eventual "right" thing is to have a
more verbose status file. But in the meantime, I think this patch is
sensible.

-Peff

Re: [RFC/PATCH] mergetool: clarify local/remote terminology

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I like it; I think it made it a lot more obvious which side was which
> during the rebase. I checked with cherry-pick, as well; it more or less
> makes sense, except that the cherry-picked commit is called "upstream."

While I like the fact that somebody is trying to tackle the
consistency issue, I do not like the approach itself.  Fudging
the issue at the merge-tool UI level may make things appear more
consistent when viewing the merge from within merge-tool, but it
makes the views merge-tool gives and vi/less gives inconsistent.

It would be a lot more sensible to make sure that we always show
the side that the end-user modified first and then the side the
other party changed.

A regular merge, cherry-pick and revert all start from user's
side and play the change from the other side, and conflicted
hunks are shown in the right order.  It is only "rebase" (with
or without -m) that gets it wrong.  Even though it internally
operates as if our own changes are coming from the other side,
we should show our change first and then theirs to make it
consistent and easier to read.

So how about doing it like this, instead?  I am not proud of the
use of an environment variable to pass this control around, but
the idea should be obvious.

Note that some tests are written to expect the current hunk
order and will start failing, so if we were to go this route, we
would need to adjust them as well.

--

 git-rebase.sh |    3 +++
 ll-merge.c    |    6 ++++++
 2 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/git-rebase.sh b/git-rebase.sh
index bdcea0e..422879f 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -48,6 +48,9 @@ prec=4
 verbose=
 git_am_opt=
 
+GITHEAD_SWAP_CONFLICT=in-rebase
+export GITHEAD_SWAP_CONFLICT
+
 continue_merge () {
 	test -n "$prev_head" || die "prev_head must be defined"
 	test -d "$dotest" || die "$dotest directory does not exist"
diff --git a/ll-merge.c b/ll-merge.c
index 5ae7433..1d0040f 100644
--- a/ll-merge.c
+++ b/ll-merge.c
@@ -76,6 +76,12 @@ static int ll_xdl_merge(const struct ll_merge_driver *drv_unused,
 				       virtual_ancestor);
 	}
 
+	if (getenv("GITHEAD_SWAP_CONFLICT")) {
+		mmfile_t *s;
+		const char *n;
+		s = src1; src1 = src2; src2 = s;
+		n = name1; name1 = name2; name2 = n;
+	}
 	memset(&xpp, 0, sizeof(xpp));
 	return xdl_merge(orig,
 			 src1, name1,

Re: [RFC/PATCH] mergetool: clarify local/remote terminology

[Jeff King]

On Thu, Feb 28, 2008 at 09:47:35PM -0800, Junio C Hamano wrote:

> While I like the fact that somebody is trying to tackle the
> consistency issue, I do not like the approach itself.  Fudging
> the issue at the merge-tool UI level may make things appear more
> consistent when viewing the merge from within merge-tool, but it
> makes the views merge-tool gives and vi/less gives inconsistent.
> 
> It would be a lot more sensible to make sure that we always show
> the side that the end-user modified first and then the side the
> other party changed.

I hadn't considered that, because I never pay attention to the order of
changes between the conflict markers; I look at the nice "HEAD" and
"abcdef... commit subject" messages.

But then I don't do a lot of conflict resolution. Usually I either work
with tiny teams on a central-ish repository, or work on projects where I
am just a contributor.

So I agree that a consistent view makes sense, and I can see that
ordering of hunks is a sensible context clue. However, should this not
extend further, to the index numbering? Or do we not care, because
mortals rarely touch the index? What about "git-rebase --strategy=ours",
which really means "theirs"?

Unfortunately just swapping the arguments to git-merge-* in
git-rebase.sh doesn't quite work ("merge-ours" doesn't actually load the
index and say "pick the 'ours' stage"; it just says "whatever is in the
working tree is fine.").

-Peff