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

[Josef Ridky <jridky@redhat.com>]

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.
|