Feature Request: user defined suffix for temp files created by git-mergetool

Feature Request: user defined suffix for temp files created by git-mergetool

[Josef Ridky]

Hello,

I would like to request for implementing feature described in subject.

In several projects, we are using git mergetool for comparing files from different folders. 
Unfortunately, when we have opened three files  for comparing using meld tool (e.q. Old_version -- Result -- New_version), 
we can see only name of temporary files created by mergetool in the labels (e.g. foo_REMOTE -- foo_BASE -- foo_LOCAL) 
and users (and sometime even we) are confused, which of the files should they edit and save.

If you will be so kind and approve this feature request, you will help us to solve this unfortunate issue by the easiest way.

I have already prepared patch, which can be applied to resolve this request (see in attachment).


Best regards

Josef Ridky
Associate Software Engineer
Core Services Team
Red Hat Czech, s.r.o.

Re: Feature Request: user defined suffix for temp files created by git-mergetool

[Anatoly Borodin]

Hi Josef,


On Mon, Oct 3, 2016 at 8:36 AM, Josef Ridky <jridky@redhat.com> wrote:
> In several projects, we are using git mergetool for comparing files from different folders.
> Unfortunately, when we have opened three files  for comparing using meld tool (e.q. Old_version -- Result -- New_version),
> we can see only name of temporary files created by mergetool in the labels (e.g. foo_REMOTE -- foo_BASE -- foo_LOCAL)
> and users (and sometime even we) are confused, which of the files should they edit and save.

`git mergetool` just creates temporary files (with some temporary
names) and calls `meld` (or `vimdiff`, etc) with the file names as
parameters. So why wouldn't you call `meld` with the file names you
want?

-- 
Mit freundlichen Grüßen,
Anatoly Borodin

Re: Feature Request: user defined suffix for temp files created by git-mergetool

[Josef Ridky]

Hi Anatoly,


| Sent: Monday, October 3, 2016 5:18:44 PM
| 
| Hi Josef,
| 
| 
| On Mon, Oct 3, 2016 at 8:36 AM, Josef Ridky <jridky@redhat.com> wrote:
| > In several projects, we are using git mergetool for comparing files from
| > different folders.
| > Unfortunately, when we have opened three files  for comparing using meld
| > tool (e.q. Old_version -- Result -- New_version),
| > we can see only name of temporary files created by mergetool in the labels
| > (e.g. foo_REMOTE -- foo_BASE -- foo_LOCAL)
| > and users (and sometime even we) are confused, which of the files should
| > they edit and save.
| 
| `git mergetool` just creates temporary files (with some temporary
| names) and calls `meld` (or `vimdiff`, etc) with the file names as
| parameters. So why wouldn't you call `meld` with the file names you
| want?


Because files, that we want, are temporary files created by git mergetool and we are not able to change their name.

Regards

Josef

Re: Feature Request: user defined suffix for temp files created by git-mergetool

[Josef Ridky]

Hi, 

I have just realize, that my attachment has been cut off from my previous message.
Below you can find patch with requested change.

Add support for user defined suffix part of name of temporary files
created by git mergetool
---
 Documentation/git-mergetool.txt | 26 ++++++++++++++++++++-
 git-mergetool.sh                | 52 +++++++++++++++++++++++++++++++++++++----
 2 files changed, 72 insertions(+), 6 deletions(-)

diff --git a/Documentation/git-mergetool.txt b/Documentation/git-mergetool.txt
index e846c2e..6cf5935 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,30 @@ 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 used or is equal with any
+	other (remote|backup|base), 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 used or is equal with any
+	other (local|backup|base), 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 used or is equal with any
+	other (local|remote|base), 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 used or is equal with any
+	other (local|remote|backup), 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..e3433ee 100755
--- a/git-mergetool.sh
+++ b/git-mergetool.sh
@@ -8,11 +8,18 @@
 # 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=
 TOOL_MODE=merge
+
+#optional name space convention
+local_name=""
+remote_name=""
+base_name=""
+backup_name=""
+
 . git-sh-setup
 . git-mergetool--lib
 
@@ -271,10 +278,33 @@ 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"
+	if [ "$local_name" != "" ]  && [ "$local_name" != "$remote_name" ] && [ "$local_name" != "$backup_name" ] && [ "$local_name" != "$base_name" ]
+	then
+		LOCAL="$MERGETOOL_TMPDIR/${BASE}_${local_name}_$$$ext"
+	else
+		LOCAL="$MERGETOOL_TMPDIR/${BASE}_LOCAL_$$$ext"
+	fi
+
+	if [ "$remote_name" != "" ] && [ "$remote_name" != "$local_name" ] && [ "$remote_name" != "$backup_name" ] && [ "$remote_name" != "$base_name" ]
+	then
+		REMOTE="$MERGETOOL_TMPDIR/${BASE}_${remote_name}_$$$ext"
+	else
+		REMOTE="$MERGETOOL_TMPDIR/${BASE}_REMOTE_$$$ext"
+	fi
+
+	if [ "$backup_name" != "" ] && [ "$backup_name" != "$local_name" ] && [ "$backup_name" != "$remote_name" ] && [ "$backup_name" != "$base_name" ]
+	then
+		BACKUP="$MERGETOOL_TMPDIR/${BASE}_${backup_name}_$$$ext"
+	else
+		BACKUP="$MERGETOOL_TMPDIR/${BASE}_BACKUP_$$$ext"
+	fi
+
+	if [ "$base_name" != "" ] && [ "$base_name" != "$local_name" ] && [ "$base_name" != "$remote_name" ] && [ "$base_name" != "$backup_name" ]
+	then
+		BASE="$MERGETOOL_TMPDIR/${BASE}_${base_name}_$$$ext"
+	else
+		BASE="$MERGETOOL_TMPDIR/${BASE}_BASE_$$$ext"
+	fi
 
 	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 +426,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
-- 
2.7.4

Re: Feature Request: user defined suffix for temp files created by git-mergetool

[David Aguilar]

On Tue, Oct 04, 2016 at 01:18:47AM -0400, Josef Ridky wrote:
> Hi Anatoly,
> 
> 
> | Sent: Monday, October 3, 2016 5:18:44 PM
> | 
> | Hi Josef,
> | 
> | 
> | On Mon, Oct 3, 2016 at 8:36 AM, Josef Ridky <jridky@redhat.com> wrote:
> | > In several projects, we are using git mergetool for comparing files from
> | > different folders.
> | > Unfortunately, when we have opened three files  for comparing using meld
> | > tool (e.q. Old_version -- Result -- New_version),
> | > we can see only name of temporary files created by mergetool in the labels
> | > (e.g. foo_REMOTE -- foo_BASE -- foo_LOCAL)
> | > and users (and sometime even we) are confused, which of the files should
> | > they edit and save.
> | 
> | `git mergetool` just creates temporary files (with some temporary
> | names) and calls `meld` (or `vimdiff`, etc) with the file names as
> | parameters. So why wouldn't you call `meld` with the file names you
> | want?
> 
> 
> Because files, that we want, are temporary files created by
> git mergetool and we are not able to change their name.

[I didn't see your original patch, but we actually prefer inline
 patches in the email, as sent via `git send-email`.
 Documentation/SubmittingPatches has more details.

 Please also make sure to add a test to t/t7610-mergetool.sh
 exercising any new features.]

Are you proposing support for config variables to control how
the temporary files are named?

e.g. something like "mergetool.strings.{local,remote,base}" for
overriding the hard-coded {LOCAL,REMOTE,BASE} strings?

I don't want to over-engineer it, but do you want to support
executing a command to get the name, or is having a replacement
sufficient?

Now I'm curious... if replacing the strings is sufficient, what
do you plan to call them?  I can imagine maybe something like
OURS, and THEIRS might be helpful since it matches the
nomenclature already used by Git, e.g. "git merge -s ours".

Since these are temporary files, changing these names might not
be entirely out of the question.  This might be a case where
using the same words as a related Git feature might help reduce
the mental burden of using mergetool.  OURS and THEIRS are
probably the only names that fit that category, IMO.
BASE is already good enough (merge-base).

The downside of making it configurable is that it can confuse
users who use mergetool at someone else's desk where they've
named these strings to "catty", "wombat", and "jimbo".  This
doesn't seem like the kind of place where we want to allow users
to be creative, but we do care about having a good default.

OURS and THEIRS are intuitive names, so switching existing users
to those would not have much downside IMO, and it's a little
less "I just merged a REMOTE branch" centric, which is good.

Do you think these names should be changed?
If so, did you have those names in mind, or something else
entirely?

cheers,
-- 
David

Re: Feature Request: user defined suffix for temp files created by git-mergetool

[Josef Ridky]

Hi David,

thank you very much for your reply.
Today I realized, that my attachment has been cut off, so I sent it in the morning [1].

I believe, that most of answer can be find in my previous email from this morning.
Only change, that should be done by this request is add possibility to edit hard-coded suffix of temporary files. 
I don't think, that change hard-coded suffix to switch between two hard-coded suffix of temporary files is suitable solution, but as well it's not bad solution.

Please look at the patch [1] and tell me, what do you think about this change.

[1] http://marc.info/?l=git&m=147565363609649&w=2

Regards
Josef

| Sent: Wednesday, October 5, 2016 11:47:06 AM
| 
| On Tue, Oct 04, 2016 at 01:18:47AM -0400, Josef Ridky wrote:
| > Hi Anatoly,
| > 
| > 
| > | Sent: Monday, October 3, 2016 5:18:44 PM
| > | 
| > | Hi Josef,
| > | 
| > | 
| > | On Mon, Oct 3, 2016 at 8:36 AM, Josef Ridky <jridky@redhat.com> wrote:
| > | > In several projects, we are using git mergetool for comparing files
| > | > from
| > | > different folders.
| > | > Unfortunately, when we have opened three files  for comparing using
| > | > meld
| > | > tool (e.q. Old_version -- Result -- New_version),
| > | > we can see only name of temporary files created by mergetool in the
| > | > labels
| > | > (e.g. foo_REMOTE -- foo_BASE -- foo_LOCAL)
| > | > and users (and sometime even we) are confused, which of the files
| > | > should
| > | > they edit and save.
| > | 
| > | `git mergetool` just creates temporary files (with some temporary
| > | names) and calls `meld` (or `vimdiff`, etc) with the file names as
| > | parameters. So why wouldn't you call `meld` with the file names you
| > | want?
| > 
| > 
| > Because files, that we want, are temporary files created by
| > git mergetool and we are not able to change their name.
| 
| [I didn't see your original patch, but we actually prefer inline
|  patches in the email, as sent via `git send-email`.
|  Documentation/SubmittingPatches has more details.
| 
|  Please also make sure to add a test to t/t7610-mergetool.sh
|  exercising any new features.]
| 
| Are you proposing support for config variables to control how
| the temporary files are named?
| 
| e.g. something like "mergetool.strings.{local,remote,base}" for
| overriding the hard-coded {LOCAL,REMOTE,BASE} strings?
| 
| I don't want to over-engineer it, but do you want to support
| executing a command to get the name, or is having a replacement
| sufficient?
| 
| Now I'm curious... if replacing the strings is sufficient, what
| do you plan to call them?  I can imagine maybe something like
| OURS, and THEIRS might be helpful since it matches the
| nomenclature already used by Git, e.g. "git merge -s ours".
| 
| Since these are temporary files, changing these names might not
| be entirely out of the question.  This might be a case where
| using the same words as a related Git feature might help reduce
| the mental burden of using mergetool.  OURS and THEIRS are
| probably the only names that fit that category, IMO.
| BASE is already good enough (merge-base).
| 
| The downside of making it configurable is that it can confuse
| users who use mergetool at someone else's desk where they've
| named these strings to "catty", "wombat", and "jimbo".  This
| doesn't seem like the kind of place where we want to allow users
| to be creative, but we do care about having a good default.
| 
| OURS and THEIRS are intuitive names, so switching existing users
| to those would not have much downside IMO, and it's a little
| less "I just merged a REMOTE branch" centric, which is good.
| 
| Do you think these names should be changed?
| If so, did you have those names in mind, or something else
| entirely?
| 
| cheers,
| --
| David
| 

Re: Feature Request: user defined suffix for temp files created by git-mergetool

[Junio C Hamano]

Josef Ridky <jridky@redhat.com> writes:

> Hi, 
>
> I have just realize, that my attachment has been cut off from my previous message.
> Below you can find patch with requested change.
>
> Add support for user defined suffix part of name of temporary files
> created by git mergetool
> ---

The first two paragraphs above do not look like they are meant for
the commit log for this change.

Please sign-off your patch.

> -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=
>  TOOL_MODE=merge
> +
> +#optional name space convention
> +local_name=""
> +remote_name=""
> +base_name=""
> +backup_name=""

If you initialize these to LOCAL, REMOTE, etc. instead of empty,
wouldn't it make the remainder of the code a lot simpler?  For
example,

> +	if [ "$local_name" != "" ]  && [ "$local_name" != "$remote_name" ] && [ "$local_name" != "$backup_name" ] && [ "$local_name" != "$base_name" ]
> +	then
> +		LOCAL="$MERGETOOL_TMPDIR/${BASE}_${local_name}_$$$ext"
> +	else
> +		LOCAL="$MERGETOOL_TMPDIR/${BASE}_LOCAL_$$$ext"
> +	fi

This can just be made an unconditional

	LOCAL="$MERGETOOL_TMPDIR/${BASE}_${local_name}_$$$ext"

without any "if" check in front.  The same for all others.

The conditional you added is doing two unrelated things.  It is
trying to switch between an unset $local_name and default LOCAL,
while it tries to make sure the user did not give the same string
for two different things (which is a nonsense).  It is probably
better to check for nonsense just once just before all these
assuments of LOCAL, REMOTE, etc. begins, not at each point where
they are set like this patch does.

Re: Feature Request: user defined suffix for temp files created by git-mergetool

[Johannes Sixt]

Am 05.10.2016 um 09:47 schrieb Josef Ridky:
> Add support for user defined suffix part of name of temporary files
> created by git mergetool

Do I understand correctly that your users have problems to identify 
which of the "_BASE_", "_LOCAL_", "_REMOTE_" and "_BACKUP_" files they 
must edit? I agree that there is some room for improvement.

The goal is that you want the user to see the label, e.g., "_EDIT_THIS_" 
instead of "_LOCAL_". Now you have to teach your users that they have to 
pass --local=_EDIT_THIS_. Why don't you just teach your users to edit 
the file labeled "_LOCAL_"?

Therefore, I think that your patch as written does not help to reduce 
the confusion. It may be a building block for further improvement, but 
if you stop here, it is pointless.

>  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,30 @@ 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 used or is equal with any
> +	other (remote|backup|base), 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 used or is equal with any
> +	other (local|backup|base), 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 used or is equal with any
> +	other (local|remote|base), 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 used or is equal with any
> +	other (local|remote|backup), default value is used.
> +	Default suffix is BASE.

Re: Feature Request: user defined suffix for temp files created by git-mergetool

[Josef Ridky]

Hi Junio,

thank you very much for the tips. I'll make new patch with a simpler code.

Josef

| Sent: Wednesday, October 5, 2016 6:05:59 PM
| Josef Ridky <jridky@redhat.com> writes:
| 
| > Hi,
| >
| > I have just realize, that my attachment has been cut off from my previous
| > message.
| > Below you can find patch with requested change.
| >
| > Add support for user defined suffix part of name of temporary files
| > created by git mergetool
| > ---
| 
| The first two paragraphs above do not look like they are meant for
| the commit log for this change.
| 
| Please sign-off your patch.
| 
| > -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=
| >  TOOL_MODE=merge
| > +
| > +#optional name space convention
| > +local_name=""
| > +remote_name=""
| > +base_name=""
| > +backup_name=""
| 
| If you initialize these to LOCAL, REMOTE, etc. instead of empty,
| wouldn't it make the remainder of the code a lot simpler?  For
| example,
| 
| > +	if [ "$local_name" != "" ]  && [ "$local_name" != "$remote_name" ] && [
| > "$local_name" != "$backup_name" ] && [ "$local_name" != "$base_name" ]
| > +	then
| > +		LOCAL="$MERGETOOL_TMPDIR/${BASE}_${local_name}_$$$ext"
| > +	else
| > +		LOCAL="$MERGETOOL_TMPDIR/${BASE}_LOCAL_$$$ext"
| > +	fi
| 
| This can just be made an unconditional
| 
| 	LOCAL="$MERGETOOL_TMPDIR/${BASE}_${local_name}_$$$ext"
| 
| without any "if" check in front.  The same for all others.
| 
| The conditional you added is doing two unrelated things.  It is
| trying to switch between an unset $local_name and default LOCAL,
| while it tries to make sure the user did not give the same string
| for two different things (which is a nonsense).  It is probably
| better to check for nonsense just once just before all these
| assuments of LOCAL, REMOTE, etc. begins, not at each point where
| they are set like this patch does.
| 

Re: Feature Request: user defined suffix for temp files created by git-mergetool

[Josef Ridky]

Hi Johannes,

thank you very much for this reply.

| Sent: Wednesday, October 5, 2016 11:04:17 PM
| 
| Am 05.10.2016 um 09:47 schrieb Josef Ridky:
| > Add support for user defined suffix part of name of temporary files
| > created by git mergetool
| 
| Do I understand correctly that your users have problems to identify
| which of the "_BASE_", "_LOCAL_", "_REMOTE_" and "_BACKUP_" files they
| must edit? I agree that there is some room for improvement.
| 
| The goal is that you want the user to see the label, e.g., "_EDIT_THIS_"
| instead of "_LOCAL_". Now you have to teach your users that they have to
| pass --local=_EDIT_THIS_. Why don't you just teach your users to edit
| the file labeled "_LOCAL_"?

The goal of this request is not to teach our users, how to change labels using git mergetool.
The point is, that git mergetool is used by several of our packages in specific use-case. 

If I understand well, git mergetool expecting comparing LOCAL changes against REMOTE status in git repository.
We use git mergetool to compare LOCAL OLD version of package with LOCAL NEW version of package.

In this use-case is really nonsense to label old version of package as LOCAL and new version of package as REMOTE.
This is point of confusion. This is the reason, why I am asking for the possibility to change the suffix of temporary files, which are shown to user for comparison.

And as well, users do not open these files themselves, these files are automatically opened by our packages, so users are not responsible for the name of temporary files. They just see the results.

| 
| Therefore, I think that your patch as written does not help to reduce
| the confusion. It may be a building block for further improvement, but
| if you stop here, it is pointless.
| 

I agree, that this patch is written as general as possible and can possibly bring more confusion than benefits.
I will make new version of this patch, where will be just simple switch between LOCAL/REMOTE and NEW/OLD.
 
| >  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,30 @@ 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 used or is equal with any
| > +	other (remote|backup|base), 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 used or is equal with any
| > +	other (local|backup|base), 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 used or is equal with any
| > +	other (local|remote|base), 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 used or is equal with any
| > +	other (local|remote|backup), default value is used.
| > +	Default suffix is BASE.
| 
| 

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

[Josef Ridky]

This is the first of two 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
comparision between two version of same package during package rebase.

Signed-off-by: Josef Ridky <jridky@redhat.com>
---
 Documentation/git-mergetool.txt |  7 ++++++-
 git-mergetool.sh                | 23 ++++++++++++++++++-----
 2 files changed, 24 insertions(+), 6 deletions(-)

diff --git a/Documentation/git-mergetool.txt b/Documentation/git-mergetool.txt
index e846c2e..a212cef 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] [-l | --local] [<file>...]
 
 DESCRIPTION
 -----------
@@ -79,6 +79,11 @@ 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.
 
+-l::
+--local::
+	Change suffix of name of temporary files generated by git
+	mergetool from `_REMOTE_` and `_LOCAL_` to `_OLD_` and `_NEW_`.
+
 TEMPORARY FILES
 ---------------
 `git mergetool` creates `*.orig` backup files while resolving merges.
diff --git a/git-mergetool.sh b/git-mergetool.sh
index bf86270..1e04368 100755
--- a/git-mergetool.sh
+++ b/git-mergetool.sh
@@ -8,10 +8,11 @@
 # 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] [-l|--local] [file to merge] ...'
 SUBDIRECTORY_OK=Yes
 NONGIT_OK=Yes
 OPTIONS_SPEC=
+LOCAL_MODE=false
 TOOL_MODE=merge
 . git-sh-setup
 . git-mergetool--lib
@@ -271,10 +272,19 @@ 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"
+	if test "$LOCAL_MODE"
+	then
+
+		BACKUP="$MERGETOOL_TMPDIR/${BASE}_BACKUP_$$$ext"
+		LOCAL="$MERGETOOL_TMPDIR/${BASE}_NEW_$$$ext"
+		REMOTE="$MERGETOOL_TMPDIR/${BASE}_OLD_$$$ext"
+		BASE="$MERGETOOL_TMPDIR/${BASE}_BASE_$$$ext"
+	else
+		BACKUP="$MERGETOOL_TMPDIR/${BASE}_BACKUP_$$$ext"
+		LOCAL="$MERGETOOL_TMPDIR/${BASE}_LOCAL_$$$ext"
+		REMOTE="$MERGETOOL_TMPDIR/${BASE}_REMOTE_$$$ext"
+		BASE="$MERGETOOL_TMPDIR/${BASE}_BASE_$$$ext"
+	fi
 
 	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 +406,9 @@ do
 	--prompt)
 		prompt=true
 		;;
+	-l|--local)
+		LOCAL_MODE=true
+		;;
 	--)
 		shift
 		break
-- 
2.7.4

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

[Josef Ridky]

This is the second of two 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 <jridky@redhat.com>
---
 Documentation/git-mergetool.txt | 26 ++++++++++++++++++++++++-
 git-mergetool.sh                | 43 ++++++++++++++++++++++++++++++++++++-----
 2 files changed, 63 insertions(+), 6 deletions(-)

diff --git a/Documentation/git-mergetool.txt b/Documentation/git-mergetool.txt
index e846c2e..6cf5935 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,30 @@ 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 used or is equal with any
+	other (remote|backup|base), 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 used or is equal with any
+	other (local|backup|base), 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 used or is equal with any
+	other (local|remote|base), 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 used or is equal with any
+	other (local|remote|backup), 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..096ee5e 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,12 @@ 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 +277,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 +402,33 @@ do
 	--prompt)
 		prompt=true
 		;;
+	--local=*)
+		temp_name=${1#--local=}
+		if [ "$temp_name" != "" ] && [ "$temp_name" != "$REMOTE_NAME" ] && [ "$temp_name" != "$BASE_NAME" ] && [ "$temp_name" != "$BACKUP_NAME" ]
+		then
+			LOCAL_NAME=$temp_name
+		fi
+		;;
+	--remote=*)
+		temp_name=${1#--remote=}
+		if [ "$temp_name" != "" ] && [ "$temp_name" != "$LOCAL_NAME" ] && [ "$temp_name" != "$BASE_NAME" ] && [ "$temp_name" != "$BACKUP_NAME" ]
+		then
+			REMOTE_NAME=$temp_name
+		fi
+		;;
+	--base=*)
+		temp_name=${1#--base=}
+		if [ "$temp_name" != "" ] && [ "$temp_name" != "$LOCAL_NAME" ] && [ "$temp_name" != "$REMOTE_NAME" ] && [ "$temp_name" != "$BACKUP_NAME" ]
+		then
+			BASE_NAME=$temp_name
+		fi
+		;;
+	--backup=*)
+		temp_name=${1#--backup=}
+		if [ "$temp_name" != "" ] && [ "$temp_name" != "$LOCAL_NAME" ] && [ "$temp_name" != "$REMOTE_NAME" ] && [ "$temp_name" != "$BASE_NAME" ]
+			BACKUP_NAME=$temp_name
+		fi
+		;;
 	--)
 		shift
 		break
-- 
2.7.4

Re: Feature Request: user defined suffix for temp files created by git-mergetool

[Junio C Hamano]

Johannes Sixt <j6t@kdbg.org> writes:

> Therefore, I think that your patch as written does not help to reduce
> the confusion. It may be a building block for further improvement, but
> if you stop here, it is pointless.

Yup, you're right.

Re: Feature Request: user defined suffix for temp files created by git-mergetool

[Junio C Hamano]

Josef Ridky <jridky@redhat.com> writes:

> I agree, that this patch is written as general as possible and can
> possibly bring more confusion than benefits.

I am not sure about that.  Other people would have similar but
different workflow needs where they compare local new one with local
old one that would be helped by renaming local to old and remote to
new (i.e. the other way around from your need).  If you just add a
toggle between local-remote vs new-old, that would be just an
additional code baggage that does not help people other than you.

I think J6t's "EDIT THIS" hits the center of the issue.  If users
are trained to know LOCAL is the one to be edited, would the current
UI work well enough for them thru your custom workflow tools?  If we
rename LOCAL to "EDIT THIS" and do nothing else, would such UI work
well for even untrained users thru your custom workflow tools?

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

[Junio C Hamano]

Josef Ridky <jridky@redhat.com> writes:

> +	--local=*)
> +		temp_name=${1#--local=}
> +		if [ "$temp_name" != "" ] && [ "$temp_name" != "$REMOTE_NAME" ] && [ "$temp_name" != "$BASE_NAME" ] && [ "$temp_name" != "$BACKUP_NAME" ]
> +		then
> +			LOCAL_NAME=$temp_name
> +		fi

It is not a good idea to ignore an explicit user request without
giving any indication and without giving any explanation.

You may have noticed that we do not say "[ cond ]" in this shell
script (we say "test" instead; see Documentation/CodingGuidelines).

Instead of having such a test all over the place, I'd suggest doing
it outside the loop:

    while test $# != 0
    do
	case "$1" in
	...
	--local=*)
		LOCAL_NAME=${1#--local=} ;;
	--remote=*)                
		REMOTE_NAME=${1#--local=} ;;
	...
        esac
    done
    
    # sanity check _after_ parsing the command line
    case ",$LOCAL_NAME,$REMOTE_NAME,$BASE_NAME,$BACKUP_NAME," in
    *,,*) die "You cannot set --local/remote/... to empty" ;;
    esac
    ... similarly, duplicate check comes here ...

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

[Josef Ridky]

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 <jridky@redhat.com>
---
 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
-- 
2.7.4

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

[Junio C Hamano]

Josef Ridky <jridky@redhat.com> 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 <jridky@redhat.com>
> ---

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.

>  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

[David Aguilar]

On Wed, Oct 12, 2016 at 10:59:46AM -0700, Junio C Hamano wrote:
> Josef Ridky <jridky@redhat.com> 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 <jridky@redhat.com>
> > ---
> 
> 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,

> > +'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

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

[David Aguilar]

On Thu, Oct 06, 2016 at 08:43:02AM -0400, Josef Ridky wrote:
> This is the first of two 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
> comparision between two version of same package during package rebase.
> 
> Signed-off-by: Josef Ridky <jridky@redhat.com>
> ---
>  Documentation/git-mergetool.txt |  7 ++++++-
>  git-mergetool.sh                | 23 ++++++++++++++++++-----
>  2 files changed, 24 insertions(+), 6 deletions(-)

While I do like that this variant only uses a single flag, I was
curious whether it would make sense for us to change our
defaults to OLD/NEW?  I'm thinking "no" since it's totally legit
to merge "old" branches so I'll stop there.

What I really wanted to mention was...

If the patch does not update t/t7610-mergetool.sh then there is
no guarantee that my clumsy fingers won't break the new feature
in the future ;-)  So please make sure to update the tests once
we decide on the final direction.  It makes sense we wouldn't
want to update them just yet (in this patch) since this is still
RFC, but the final one should include it.

I'm still leaning towards environment variables personally,
and would have no qualms against taking a patch that teaches it
to support environment variables as long as it adds a test case
for the new feature.

Thanks for sticking with it, Josef!

cheers,
-- 
David