Re: [PATCH v2] difftool --dir-diff: always use identical working tree file

From: Junio C Hamano <gitster@pobox.com>
Date: May 28, 2013, 18:57 UTC
Message-ID: <7v7gij0w6z.fsf@alter.siamese.dyndns.org>
In-Reply-To: <20130528181525.GA17475@serenity.lan>

John Keeping <john@keeping.me.uk> writes:

Show 8 quoted lines

> Yeah, the commit message is still quite focused on the end effect of
> copying files back.  But that's not what's being changed here.
>
> In my suggested commit message I tried to make it clear that we're
> changing when we decide to copy a file across to the temporary tree.
> This has the beneficial (side-)effect of changing the set of files we
> consider for copying back into the working tree after the diff tool has
> been run.

I actually think the effect of copying files back _is_ the primary motivation of this change, and stressing that end effect is a much better description. After all, if the working tree files do not have any difference from the RHS of the comparison, copying from the working tree and stuffing the $rsha1 to the RHS temporary index and running "checkout -f" should produce identical temporary directory for the user to start viewing.

The _only_ difference in behaviour before and after this patch that matters to the end user is if that path is in @working_tree, which is returned to @worktree of dir_diff sub to be later copied back, no? I would view this change as a mere means, an implementation detail, to achieve that end of stuffing more paths in the @worktree array.

Perhaps

	difftool --dir-diff: allow changing any clean working tree file

	The temporary directory prepared by "difftool --dir-diff" to
	show the result of a change can be modified by the user via
	the tree diff program, and we try hard not to lose changes
	to them after tree diff program returns to us.

        However, the set of files to be copied back is computed
	differently between --symlinks and --no-symlinks modes.  The
	former checks all paths that start out as identical to the
	working tree file, while the latter checks paths that
	already had a local modification in the working tree,
	allowing changes made in the tree diff program to paths that
	did not have any local change to be lost.

or something.  This invites a few questions, though.

 - By allowing these files in the temporary directory to be
   modified, aren't we making the user's life harder by forcing them
   to handle "working tree file was already modified, made different
   changes in the temporary directory, now these changes need to be
   consolidated" case?

 - When comparing two revisions, e.g. "--dir-diff HEAD^^ HEAD^",
   that checks out (via $rsha1 to "checkout -f" codepath) a blob
   that does not match what is in the working tree of HEAD to the
   temporary directory, we still allow modifications to the copy in
   the temporary directory, but what can the user do with these
   changes that are _not_ based on HEAD, short of checking out HEAD^
   and apply the difference first?

I still cannot shake this nagging feeling that giving a writable temporary directory might have been a mistake in the first place. Perhaps it may be a better design to make the ones that the user shouldn't touch (or will lead to the above confusion) read-only, while the ones that match the working tree read-write?