John Keeping [off-list ref] 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.
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?
On Tue, May 28, 2013 at 11:57:08AM -0700, Junio C Hamano wrote:
John Keeping [off-list ref] writes:
quoted
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.
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.
Signed-off-by: Kenichi Saita <redacted>
---
git-difftool.perl | 9 ++-------
t/t7800-difftool.sh | 19 +++++++++++++++++++
2 files changed, 21 insertions(+), 7 deletions(-)
diff --git a/git-difftool.perl b/git-difftool.perl
index 8a75205..e57d3d1 100755
--- a/git-difftool.perl
+++ b/git-difftool.perl
@@ -85,13 +85,9 @@ sub exit_cleanup
sub use_wt_file
{
- my ($repo, $workdir, $file, $sha1, $symlinks) = @_;
+ my ($repo, $workdir, $file, $sha1) = @_;
my $null_sha1 = '0' x 40;
- if ($sha1 ne $null_sha1 and not $symlinks) {
- return 0;
- }
-
if (! -e "$workdir/$file") {
# If the file doesn't exist in the working tree, we cannot
# use it.@@ -213,8 +209,7 @@ EOF
if ($rmode ne $null_mode) {
my ($use, $wt_sha1) = use_wt_file($repo, $workdir,
- $dst_path, $rsha1,
- $symlinks);
+ $dst_path, $rsha1);
if ($use) {
push @working_tree, $dst_path;
$wtindex .= "$rmode $wt_sha1\t$dst_path\0";diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
index d46f041..2418528 100755
--- a/t/t7800-difftool.sh
+++ b/t/t7800-difftool.sh
@@ -385,6 +385,25 @@ test_expect_success PERL,SYMLINKS 'difftool --dir-diff --symlink without unstage
test_cmp actual expect
'
+write_script modify-right-file <<\EOF
+echo "new content" >"$2/file"
+EOF
+
+run_dir_diff_test 'difftool --dir-diff syncs worktree with unstaged change' '
+ test_when_finished git reset --hard &&
+ echo "orig content" >file &&
+ git difftool -d $symlinks --extcmd "$(pwd)/modify-right-file" branch &&
+ echo "new content" >expect &&
+ test_cmp expect file
+'
+
+run_dir_diff_test 'difftool --dir-diff syncs worktree without unstaged change' '
+ test_when_finished git reset --hard &&
+ git difftool -d $symlinks --extcmd "$(pwd)/modify-right-file" branch &&
+ echo "new content" >expect &&
+ test_cmp expect file
+'
+
write_script modify-file <<\EOF
echo "new content" >file
EOF
--
1.7.1