Re: [PATCH v2] difftool --dir-diff: always use identical working tree file
Kenichi Saita nito...@gmail.com writes:
When deciding whether or not we should link a working tree file into
the temporary right-hand directory for a directory diff, we
currently behave differently in the --symlink and --no-symlink
cases. If using symlinks any identical files are linked across but
with --no-symlink only files that contain unstaged changes are
copied back into the working tree.
I may have missed an earlier discussion, but I do not follow the
last sentence. The former part (i.e. symlinks) talks about what is
done to populate the temporary tree (i.e. everything is linked), but
the latter part (i.e. not symlinks) only talks about what is copied
back, i.e. it is not a contrast between the behaviour of symlink vs
no-symlink case wrt how the temporary tree is populated.
Confused...
Change this so that identical files are copied back as well. This
is beneficial because it widens the set of circumstances in which we
copy changes made by the user back into the working tree.
Ah, OK, you meant that the set of files we keep in @working_tree
array for later copying back are different between the two.
Signed-off-by: Kenichi Saita nito...@gmail.com
---
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
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] difftool --dir-diff: always use identical working tree file
On Tue, May 28, 2013 at 11:06:13AM -0700, Junio C Hamano wrote:
Kenichi Saita nito...@gmail.com writes:
When deciding whether or not we should link a working tree file into
the temporary right-hand directory for a directory diff, we
currently behave differently in the --symlink and --no-symlink
cases. If using symlinks any identical files are linked across but
with --no-symlink only files that contain unstaged changes are
copied back into the working tree.
I may have missed an earlier discussion, but I do not follow the
last sentence. The former part (i.e. symlinks) talks about what is
done to populate the temporary tree (i.e. everything is linked), but
the latter part (i.e. not symlinks) only talks about what is copied
back, i.e. it is not a contrast between the behaviour of symlink vs
no-symlink case wrt how the temporary tree is populated.
Confused...
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.
Change this so that identical files are copied back as well. This
is beneficial because it widens the set of circumstances in which we
copy changes made by the user back into the working tree.
Ah, OK, you meant that the set of files we keep in @working_tree
array for later copying back are different between the two.
Signed-off-by: Kenichi Saita nito...@gmail.com
---
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
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] difftool --dir-diff: always use identical working tree file
John Keeping j...@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. 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? -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] difftool --dir-diff: always use identical working tree file
On Tue, May 28, 2013 at 11:57:08AM -0700, Junio C Hamano wrote: John Keeping j...@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. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] difftool --dir-diff: always use identical working tree file
John Keeping j...@keeping.me.uk writes: - 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. Not necessarily. Let's assume that changing files in diff tool is a sensible thing to do, as long as we make sure such a change is not lost (which I may not 100% agree with, but let's put it aside for now). When you are viewing a file F in --dir-diff HEAD^^ HEAD^, if there is no change for F in between HEAD^ and HEAD and you notice a typo that may or may not be related to the differences between HEAD^^ and HEAD^, it would be tempting to fix that right there. And as long as F in the working tree matches that of HEAD^ and the modification you make in the temporary directory gets copied back to the working tree, your typofix will end up to be in the working tree. Which I _think_ is what people, who want to change files in diff tool, want to do. Of course, your working tree may have been in the middle of doing something entirely different and you may have to add [-p] to separate such a typofix with other changes you were working on, but that is a separate issue. And for that to work, the only think you need is does the blob we show on the RHS temporary tree match what is in the working tree? check. You do not need to know or care if you are comparing two old revisions, etc. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2] difftool --dir-diff: always use identical working tree file
When deciding whether or not we should link a working tree file into
the temporary right-hand directory for a directory diff, we
currently behave differently in the --symlink and --no-symlink
cases. If using symlinks any identical files are linked across but
with --no-symlink only files that contain unstaged changes are
copied back into the working tree.
Change this so that identical files are copied back as well. This
is beneficial because it widens the set of circumstances in which we
copy changes made by the user back into the working tree.
Signed-off-by: Kenichi Saita nito...@gmail.com
---
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
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
