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

From: David Aguilar <davvid@gmail.com>
Date: Oct 13, 2016, 04:55 UTC
Message-ID: <20161013045532.GA25745@gmail.com>
In-Reply-To: <xmqqpon54fe5.fsf@gitster.mtv.corp.google.com>

On Wed, Oct 12, 2016 at 10:59:46AM -0700, Junio C Hamano wrote:

Show 37 quoted lines

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

Show 5 quoted lines

> +# 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:

Show 8 quoted lines

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

Show 30 quoted lines

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