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

Josef Ridky [off-list ref] 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.

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 [off-list ref] 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.
|