Re: [PATCH v2 2/2] Feature Request: user defined suffix for temp files created by git-mergetool

2 messages, 2 authors, 2016-10-13 · open the first message on its own page

Re: [PATCH v2 2/2] Feature Request: user defined suffix for temp files created by git-mergetool

From: Junio C Hamano <hidden>
Date: 2016-10-12 18:05:05

Josef Ridky [off-list ref] writes:
This is update of the second variant for request to add option to change
suffix of name of temporary files generated by git mergetool. This
change is requested for cases, when is git mergetool used for local
comparison between two version of same package during package rebase.

Signed-off-by: Josef Ridky <redacted>
---
David, what do you think?

I don't think you were ever CC'ed on any of the messages in
this thread, and I don't think you've commented on the topic.  The
thread begins here:

  https://public-inbox.org/git/1329039097.128066.1475476591437.JavaMail.zimbra@redhat.com/


In any case, I suggest update to the log message to something like
this:

    Subject: mergetool: allow custom naming for temporary files

    A front-end program that is spawned by "git mergetool" is given
    three temporary files (e.g. it may get "x_LOCAL.txt",
    "x_REMOTE.txt", and "x_BASE.txt" while merging "x.txt").  

    Custom wrappers to "git mergetool" benefits if they are allowed
    to rename these hardcoded suffixes to match the workflow they
    implement.  For example, they may be used to compare and merge
    two versions that is available locally, and OLD/NEW may be more
    appropriate than LOCAL/REMOTE in such a context.

primarily because "the second variant" is meaningless thing to say
in our long term history, when the first variant never was recorded
there.  Josef may want to elaborate more on the latter paragraph.
quoted hunk
 Documentation/git-mergetool.txt | 22 ++++++++++++++-
 git-mergetool.sh                | 60 +++++++++++++++++++++++++++++++++++++----
 2 files changed, 76 insertions(+), 6 deletions(-)
diff --git a/Documentation/git-mergetool.txt b/Documentation/git-mergetool.txt
index e846c2e..a0466ac 100644
--- a/Documentation/git-mergetool.txt
+++ b/Documentation/git-mergetool.txt
@@ -8,7 +8,7 @@ git-mergetool - Run merge conflict resolution tools to resolve merge conflicts
 SYNOPSIS
 --------
 [verse]
-'git mergetool' [--tool=<tool>] [-y | --[no-]prompt] [<file>...]
+'git mergetool' [--tool=<tool>] [-y | --[no-]prompt] [--local=<name>] [--remote=<name>] [--backup=<name>] [--base=<name>] [<file>...]
 
 DESCRIPTION
 -----------
@@ -79,6 +79,26 @@ success of the resolution after the custom tool has exited.
 	Prompt before each invocation of the merge resolution program
 	to give the user a chance to skip the path.
 
+--local=<name>::
+	Use string from <name> as part of suffix of name of temporary
+	file (local) for merging. If not set, default value is used.
+	Default suffix is LOCAL.
+
+--remote=<name>::
+	Use string from <name> as part of suffix of name of temporary
+	file (remote) for merging. If not set, default value is used.
+	Default suffix is REMOTE.
+
+--backup=<name>::
+	Use string from <name> as part of suffix of name of temporary
+	file (backup) for merging. If not set, default value is used.
+	Default suffix is BACKUP.
+
+--base=<name>::
+	Use string from <name> as part of suffix of name of temporary
+	file (base) for merging. If not set, default value is used.
+	Default suffix is BASE.
+
 TEMPORARY FILES
 ---------------
 `git mergetool` creates `*.orig` backup files while resolving merges.
diff --git a/git-mergetool.sh b/git-mergetool.sh
index bf86270..ed9ba82 100755
--- a/git-mergetool.sh
+++ b/git-mergetool.sh
@@ -8,7 +8,7 @@
 # at the discretion of Junio C Hamano.
 #
 
-USAGE='[--tool=tool] [--tool-help] [-y|--no-prompt|--prompt] [file to merge] ...'
+USAGE='[--tool=tool] [--tool-help] [-y|--no-prompt|--prompt] [--local=name] [--remote=name] [--backup=name] [--base=name] [file to merge] ...'
 SUBDIRECTORY_OK=Yes
 NONGIT_OK=Yes
 OPTIONS_SPEC=
@@ -16,6 +16,13 @@ TOOL_MODE=merge
 . git-sh-setup
 . git-mergetool--lib
 
+# Can be changed by user
+LOCAL_NAME='LOCAL'
+BASE_NAME='BASE'
+BACKUP_NAME='BACKUP'
+REMOTE_NAME='REMOTE'
+
+
 # Returns true if the mode reflects a symlink
 is_symlink () {
 	test "$1" = 120000
@@ -271,10 +278,10 @@ merge_file () {
 		BASE=${BASE##*/}
 	fi
 
-	BACKUP="$MERGETOOL_TMPDIR/${BASE}_BACKUP_$$$ext"
-	LOCAL="$MERGETOOL_TMPDIR/${BASE}_LOCAL_$$$ext"
-	REMOTE="$MERGETOOL_TMPDIR/${BASE}_REMOTE_$$$ext"
-	BASE="$MERGETOOL_TMPDIR/${BASE}_BASE_$$$ext"
+	BACKUP="$MERGETOOL_TMPDIR/${BASE}_${BACKUP_NAME}_$$$ext"
+	LOCAL="$MERGETOOL_TMPDIR/${BASE}_${LOCAL_NAME}_$$$ext"
+	REMOTE="$MERGETOOL_TMPDIR/${BASE}_${REMOTE_NAME}_$$$ext"
+	BASE="$MERGETOOL_TMPDIR/${BASE}_${BASE_NAME}_$$$ext"
 
 	base_mode=$(git ls-files -u -- "$MERGED" | awk '{if ($3==1) print $1;}')
 	local_mode=$(git ls-files -u -- "$MERGED" | awk '{if ($3==2) print $1;}')
@@ -396,6 +403,18 @@ do
 	--prompt)
 		prompt=true
 		;;
+	--local=*)
+		LOCAL_NAME=${1#--local=}
+		;;
+	--remote=*)
+		REMOTE_NAME=${1#--remote=}
+		;;
+	--base=*)
+		BASE_NAME=${1#--base=}
+		;;
+	--backup=*)
+		BACKUP_NAME=${1#--backup=}
+		;;
 	--)
 		shift
 		break
@@ -410,6 +429,37 @@ do
 	shift
 done
 
+# sanity check after parsing command line
+case "" in
+"$LOCAL_NAME"|"$REMOTE_NAME"|"$BASE_NAME"|"$BACKUP_NAME")
+	die "You cannot set any of --local/remote/base/backup to empty."
+	;;
+esac
+
+case "$LOCAL_NAME" in
+"$REMOTE_NAME"|"$BASE_NAME"|"$BACKUP_NAME")
+	die "You cannot set any of --remote/base/backup to same as --local."
+	;;
+esac
+
+case "$REMOTE_NAME" in
+"$LOCAL_NAME"|"$BASE_NAME"|"$BACKUP_NAME")
+	die "You cannot set any of --local/base/backup to same as --remote."
+	;;
+esac
+
+case "$BASE_NAME" in
+"$LOCAL_NAME"|"$REMOTE_NAME"|"$BACKUP_NAME")
+	die "You cannot set any of --local/remote/backup to same as --base."
+	;;
+esac
+
+case "$BACKUP_NAME" in
+"$LOCAL_NAME"|"$REMOTE_NAME"|"$BASE_NAME")
+	die "You cannot set any of --local/remote/base to same as --backup."
+	;;
+esac
+
 prompt_after_failed_merge () {
 	while true
 	do

Re: [PATCH v2 2/2] Feature Request: user defined suffix for temp files created by git-mergetool

From: David Aguilar <hidden>
Date: 2016-10-13 04:55:58

On Wed, Oct 12, 2016 at 10:59:46AM -0700, Junio C Hamano wrote:
Josef Ridky [off-list ref] writes:
quoted
This is update of the second variant for request to add option to change
suffix of name of temporary files generated by git mergetool. This
change is requested for cases, when is git mergetool used for local
comparison between two version of same package during package rebase.

Signed-off-by: Josef Ridky <redacted>
---
David, what do you think?

I don't think you were ever CC'ed on any of the messages in
this thread, and I don't think you've commented on the topic.  The
thread begins here:

  https://public-inbox.org/git/1329039097.128066.1475476591437.JavaMail.zimbra@redhat.com/


In any case, I suggest update to the log message to something like
this:

    Subject: mergetool: allow custom naming for temporary files

    A front-end program that is spawned by "git mergetool" is given
    three temporary files (e.g. it may get "x_LOCAL.txt",
    "x_REMOTE.txt", and "x_BASE.txt" while merging "x.txt").  

    Custom wrappers to "git mergetool" benefits if they are allowed
    to rename these hardcoded suffixes to match the workflow they
    implement.  For example, they may be used to compare and merge
    two versions that is available locally, and OLD/NEW may be more
    appropriate than LOCAL/REMOTE in such a context.

primarily because "the second variant" is meaningless thing to say
in our long term history, when the first variant never was recorded
there.  Josef may want to elaborate more on the latter paragraph.

I do see why this can be helpful but what makes me reluctant is,
quoted
+'git mergetool' [--tool=<tool>] [-y | --[no-]prompt] [--local=<name>] [--remote=<name>] [--backup=<name>] [--base=<name>] [<file>...]
We're parking on 4 new flags that are really all about changing
one small aspect of the program.

I don't typically like going overboard with environment
variables but for this use case they seem to be a good fit.

It's already been expressed that the intention is for these to
be supplied by some other tool, and environment variables have
the nice property that you can update all your tools without
needing to touch anything except the environment.

Another nice thing is that the the user can choose to set it
globally, or to set it local to specific tools.

Another reason why I think it's a good fit is,
+# Can be changed by user
+LOCAL_NAME='LOCAL'
+BASE_NAME='BASE'
+BACKUP_NAME='BACKUP'
+REMOTE_NAME='REMOTE'
These lines would become simply,

LOCAL_NAME=${GIT_MERGETOOL_LOCAL_NAME:-LOCAL}
BASE_NAME=${GIT_MERGETOOL_BASE_NAME:-BASE}
BACKUP_NAME=${GIT_MERGETOOL_BACKUP_NAME:-BACKUP}
REMOTE_NAME=${GIT_MERGETOOL_REMOTE_NAME:-REMOTE}

and we wouldn't need any other change besides this part:
-	BACKUP="$MERGETOOL_TMPDIR/${BASE}_BACKUP_$$$ext"
-	LOCAL="$MERGETOOL_TMPDIR/${BASE}_LOCAL_$$$ext"
-	REMOTE="$MERGETOOL_TMPDIR/${BASE}_REMOTE_$$$ext"
-	BASE="$MERGETOOL_TMPDIR/${BASE}_BASE_$$$ext"
+	BACKUP="$MERGETOOL_TMPDIR/${BASE}_${BACKUP_NAME}_$$$ext"
+	LOCAL="$MERGETOOL_TMPDIR/${BASE}_${LOCAL_NAME}_$$$ext"
+	REMOTE="$MERGETOOL_TMPDIR/${BASE}_${REMOTE_NAME}_$$$ext"
+	BASE="$MERGETOOL_TMPDIR/${BASE}_${BASE_NAME}_$$$ext"

Then, just update the documentation to mention the environment
variables instead of the flags and we're all set.

We also won't need this part if we go down that route:
+# sanity check after parsing command line
+case "" in
+"$LOCAL_NAME"|"$REMOTE_NAME"|"$BASE_NAME"|"$BACKUP_NAME")
+	die "You cannot set any of --local/remote/base/backup to empty."
+	;;
+esac
+
+case "$LOCAL_NAME" in
+"$REMOTE_NAME"|"$BASE_NAME"|"$BACKUP_NAME")
+	die "You cannot set any of --remote/base/backup to same as --local."
+	;;
+esac
+
+case "$REMOTE_NAME" in
+"$LOCAL_NAME"|"$BASE_NAME"|"$BACKUP_NAME")
+	die "You cannot set any of --local/base/backup to same as --remote."
+	;;
+esac
+
+case "$BASE_NAME" in
+"$LOCAL_NAME"|"$REMOTE_NAME"|"$BACKUP_NAME")
+	die "You cannot set any of --local/remote/backup to same as --base."
+	;;
+esac
+
+case "$BACKUP_NAME" in
+"$LOCAL_NAME"|"$REMOTE_NAME"|"$BASE_NAME")
+	die "You cannot set any of --local/remote/base to same as --backup."
+	;;
+esac

What do you think?
-- 
David
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help