threads / rfc / 12233

RFC patchmergetool: clarify local/remote terminology

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

## tl;dr

10 messages between Feb 21, 2008 and Mar 1, 2008. Diffs are folded; open one to read it.

replies: 9people: 3as markdown or json

Jay Soffian· Feb 21, 2008, 05:12 UTC · lore

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(-)
Show changes to git-mergetool.sh +79 −39
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
Jay Soffian· Feb 25, 2008, 14:31 UTC · re: Jay Soffian · lore

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

Anyone?
On Thu, Feb 21, 2008 at 12:12 AM, Jay Soffian <jaysoffian@gmail.com> wrote:
Show 289 quoted lines
> 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
>
>
Jeff King· Feb 25, 2008, 18:46 UTC · re: Jay Soffian · lore

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

On Mon, Feb 25, 2008 at 09:31:03AM -0500, Jay Soffian wrote:
Show 7 quoted lines
> 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
Jay Soffian· Feb 25, 2008, 19:07 UTC · re: Jeff King · lore

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

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?

Show 20 quoted lines
>  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.
Jeff King· Feb 25, 2008, 19:21 UTC · re: Jay Soffian · lore

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

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
Jay Soffian· Feb 25, 2008, 20:09 UTC · re: Jeff King · lore

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

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.
Jeff King· Feb 25, 2008, 20:11 UTC · re: Jay Soffian · lore

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

On Mon, Feb 25, 2008 at 03:09:04PM -0500, Jay Soffian wrote:
Show 7 quoted lines
> >  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
Jeff King· Feb 28, 2008, 08:43 UTC · re: Jeff King · lore

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

On Mon, Feb 25, 2008 at 01:46:48PM -0500, Jeff King wrote:
Show 7 quoted lines
> > 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
Junio C Hamano· Feb 29, 2008, 05:47 UTC · re: Jeff King · lore

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

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(-)
Show changes to 2 files +9 −0

git-rebase.sh, ll-merge.c

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,
Jeff King· Mar 1, 2008, 04:41 UTC · re: Junio C Hamano · lore

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

On Thu, Feb 28, 2008 at 09:47:35PM -0800, Junio C Hamano wrote:
Show 9 quoted lines
> 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

← back to recent threads