Re: [PATCH 1/2] Tidy up git mergetool's backup file behaviour and variable names

From: Charles Bailey <charles@hashpling.org>
Date: Feb 18, 2008, 08:08 UTC
Message-ID: <20080218080841.GA12008@hashpling.org>
In-Reply-To: <7vy79jsza1.fsf@gitster.siamese.dyndns.org>

On Sun, Feb 17, 2008 at 07:45:42PM -0800, Junio C Hamano wrote:

Show 18 quoted lines

> Charles Bailey <charles@hashpling.org> writes:
> 
> > Currently a backup pre-merge file with conflict markers is sometimes
> > kept with a .orig extenstion and sometimes removed depending on the
> > particular merge tool used.
> >
> > This patch makes the handling consistent across all merge tools and
> > configurable via a new mergetool.keepBackup config variable
> >
> > Changed the merge file path variable to MERGED for consistency with the
> > names of the merge temporary filename variables. This done with the
> > intention of having these variables used by user scripts in a subsequent
> > custom merge tool patch.
> 
> I would have preferred two separate patches, one s/path/MERGED/
> and the other save/remove clean-up, which would be much easier
> way to review, but this is what we have, so let's work on this
> version.

Fair enough, I'll send an update.

Show 8 quoted lines

> 
> > +mergetool.keepBackup::
> > +	After performing a merge, the original file with conflict markers
> > +	can be saved as a file with a `.orig` extension.  If this variable
> > +	is set to `false` then this file is not preserved.
> > +
> 
> s/$/  Defaults to true (i.e. keep the backup files)/.

Noted.

> We might also want a command line option to override the user's
> usual default specified with this configuration but that can be
> left for later rounds.

Agreed, I'll tackle this later, given time.

Show 11 quoted lines

> > @@ -112,11 +112,11 @@ resolve_deleted_merge () {
> >  }
> >  
> >  check_unchanged () {
> > -    if test "$path" -nt "$BACKUP" ; then
> > +    if test "$MERGED" -nt "$BACKUP" ; then
> 
> I think this is the cause of your automated test sometimes
> needing 1 sec sleep.  This check should perhaps be done with a
> "!  cmp $MERGED $BACKUP >/dev/null", which would succeed if the
> user's interaction with the backend tool touched the file.

I don't think that we should spend too much time on this seeing as I now fail to reproduce it, but as the code was avoiding the interactive path (i.e. trusting the exit code), it shouldn't have been this check.

Show 6 quoted lines

> 
> The rest of the patch looked fine to me.
> 
> We might also want to add -y (assume "Yes" answer to "Did you
> resolve it" question without actually asking) command line
> option to the script, but that would be for later rounds.

Yes, and this would help the automated tests.