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

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

Charles Bailey [off-list ref] writes:

quoted

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.

quoted

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

quoted

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

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.