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

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 5, 2016, 16:05 UTC
Message-ID: <xmqqponerdaw.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <1499287628.1324571.1475653631366.JavaMail.zimbra@redhat.com>

Josef Ridky <jridky@redhat.com> writes:

Show 8 quoted lines

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

Show 12 quoted lines

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

Show 6 quoted lines

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