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

[John Keeping <john@keeping.me.uk>]

On Tue, May 28, 2013 at 11:57:08AM -0700, Junio C Hamano wrote:
> John Keeping <john@keeping.me.uk> writes:
> 
> > 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.

I agree with this, but like you I found it confusing that the patch
touched code seemingly unrelated to copying files back.  I went toward
describing the patch more literally and giving the motivation in the
final paragraph.  Your message below is better, but I think it needs to
say that the set of files considered for copying back is the set that is
copied across to begin with.

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

My ideal scenario would be that we only allow users to edit files when
they are comparing against the working tree, but that would require
git-difftool to fully understand all git-diff options since it just
passes through any it doesn't recognise.  I don't think there's an easy
way to do that, which leaves us with this confusing situation.