{"thread":{"id":"33938","subject":"[PATCH] difftool --dir-diff: copy back all files matching the working tree","startedAt":"2013-05-26T15:00:46Z","lastAt":"2013-05-29T19:52:20Z","messageCount":10,"participants":["Kenichi Saita","John Keeping","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"218565","messageId":"1369580446-32597-1-git-send-email-nitoyon@gmail.com","threadId":"33938","inReplyTo":null,"subject":"[PATCH] difftool --dir-diff: copy back all files matching the working tree","fromName":"Kenichi Saita","fromEmail":"nitoyon@gmail.com","sentAt":"2013-05-26T15:00:46Z","receivedAt":"2013-05-26T15:00:46Z","isPatch":true,"sender":{"key":"nitoyon@gmail.com","avatar":"https://avatars.githubusercontent.com/u/39562?v=4"},"body":"After running the user's diff tool, \"git difftool --dir-dif --no-symlink\"\ncurrently copied back a temporary file to working tree only when a file\ncontains unstaged changes in the working tree.\n\nChange this behavior so that temporary files are copied back to working\ntree whenever the right-hand side of the comparison has the same SHA1\nas the file in the working tree.\n\nSigned-off-by: Kenichi Saita <nitoyon@gmail.com>\n---\n git-difftool.perl   |    9 ++-------\n t/t7800-difftool.sh |   19 +++++++++++++++++++\n 2 files changed, 21 insertions(+), 7 deletions(-)\n\ndiff --git a/git-difftool.perl b/git-difftool.perl\nindex 8a75205..e57d3d1 100755\n--- a/git-difftool.perl\n+++ b/git-difftool.perl\n@@ -85,13 +85,9 @@ sub exit_cleanup\n \n sub use_wt_file\n {\n-\tmy ($repo, $workdir, $file, $sha1, $symlinks) = @_;\n+\tmy ($repo, $workdir, $file, $sha1) = @_;\n \tmy $null_sha1 = '0' x 40;\n \n-\tif ($sha1 ne $null_sha1 and not $symlinks) {\n-\t\treturn 0;\n-\t}\n-\n \tif (! -e \"$workdir/$file\") {\n \t\t# If the file doesn't exist in the working tree, we cannot\n \t\t# use it.\n@@ -213,8 +209,7 @@ EOF\n \n \t\tif ($rmode ne $null_mode) {\n \t\t\tmy ($use, $wt_sha1) = use_wt_file($repo, $workdir,\n-\t\t\t\t\t\t\t  $dst_path, $rsha1,\n-\t\t\t\t\t\t\t  $symlinks);\n+\t\t\t\t\t\t\t  $dst_path, $rsha1);\n \t\t\tif ($use) {\n \t\t\t\tpush @working_tree, $dst_path;\n \t\t\t\t$wtindex .= \"$rmode $wt_sha1\\t$dst_path\\0\";\ndiff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh\nindex d46f041..2418528 100755\n--- a/t/t7800-difftool.sh\n+++ b/t/t7800-difftool.sh\n@@ -385,6 +385,25 @@ test_expect_success PERL,SYMLINKS 'difftool --dir-diff --symlink without unstage\n \ttest_cmp actual expect\n '\n \n+write_script modify-right-file <<\\EOF\n+echo \"new content\" >\"$2/file\"\n+EOF\n+\n+run_dir_diff_test 'difftool --dir-diff syncs worktree with unstaged change' '\n+\ttest_when_finished git reset --hard &&\n+\techo \"orig content\" >file &&\n+\tgit difftool -d $symlinks --extcmd \"$(pwd)/modify-right-file\" branch &&\n+\techo \"new content\" >expect &&\n+\ttest_cmp expect file\n+'\n+\n+run_dir_diff_test 'difftool --dir-diff syncs worktree without unstaged change' '\n+\ttest_when_finished git reset --hard &&\n+\tgit difftool -d $symlinks --extcmd \"$(pwd)/modify-right-file\" branch &&\n+\techo \"new content\" >expect &&\n+\ttest_cmp expect file\n+'\n+\n write_script modify-file <<\\EOF\n echo \"new content\" >file\n EOF\n-- \n1.7.1\n"},{"id":"218568","messageId":"20130526154429.GK27005@serenity.lan","threadId":"33938","inReplyTo":"1369580446-32597-1-git-send-email-nitoyon@gmail.com","subject":"Re: [PATCH] difftool --dir-diff: copy back all files matching the working tree","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-05-26T15:44:29Z","receivedAt":"2013-05-26T15:44:29Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"On Mon, May 27, 2013 at 12:00:46AM +0900, Kenichi Saita wrote:\n> After running the user's diff tool, \"git difftool --dir-dif --no-symlink\"\n> currently copied back a temporary file to working tree only when a file\n> contains unstaged changes in the working tree.\n> \n> Change this behavior so that temporary files are copied back to working\n> tree whenever the right-hand side of the comparison has the same SHA1\n> as the file in the working tree.\n> \n> Signed-off-by: Kenichi Saita <nitoyon@gmail.com>\n\nThis change looks sensible to me, but I found the commit message quite\nconfusing.\n\nThe code being changed here is to do with choosing whether to copy the\nworking tree file to the temporary directory (or symlink it) and hence\nonly indirectly related to whether it will be copied back.  It might be\nclearer to phrase it like this:\n\n    difftool --dir-diff: always use identical working tree file\n\n    When deciding whether or not we should link a working tree file into\n    the temporary right-hand directory for a directory diff, we\n    currently behave differently in the --symlink and --no-symlink\n    cases.  If using symlinks any identical files are linked across but\n    with --no-symlink only files that contain unstaged changes are\n    copied.\n\n    Change this so that identical files are copied across as well.  This\n    is beneficial because it widens the set of circumstances in which we\n    copy changes made by the user back into the working tree.\n\n> ---\n>  git-difftool.perl   |    9 ++-------\n>  t/t7800-difftool.sh |   19 +++++++++++++++++++\n>  2 files changed, 21 insertions(+), 7 deletions(-)\n> \n> diff --git a/git-difftool.perl b/git-difftool.perl\n> index 8a75205..e57d3d1 100755\n> --- a/git-difftool.perl\n> +++ b/git-difftool.perl\n> @@ -85,13 +85,9 @@ sub exit_cleanup\n>  \n>  sub use_wt_file\n>  {\n> -\tmy ($repo, $workdir, $file, $sha1, $symlinks) = @_;\n> +\tmy ($repo, $workdir, $file, $sha1) = @_;\n>  \tmy $null_sha1 = '0' x 40;\n>  \n> -\tif ($sha1 ne $null_sha1 and not $symlinks) {\n> -\t\treturn 0;\n> -\t}\n> -\n>  \tif (! -e \"$workdir/$file\") {\n>  \t\t# If the file doesn't exist in the working tree, we cannot\n>  \t\t# use it.\n> @@ -213,8 +209,7 @@ EOF\n>  \n>  \t\tif ($rmode ne $null_mode) {\n>  \t\t\tmy ($use, $wt_sha1) = use_wt_file($repo, $workdir,\n> -\t\t\t\t\t\t\t  $dst_path, $rsha1,\n> -\t\t\t\t\t\t\t  $symlinks);\n> +\t\t\t\t\t\t\t  $dst_path, $rsha1);\n>  \t\t\tif ($use) {\n>  \t\t\t\tpush @working_tree, $dst_path;\n>  \t\t\t\t$wtindex .= \"$rmode $wt_sha1\\t$dst_path\\0\";\n> diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh\n> index d46f041..2418528 100755\n> --- a/t/t7800-difftool.sh\n> +++ b/t/t7800-difftool.sh\n> @@ -385,6 +385,25 @@ test_expect_success PERL,SYMLINKS 'difftool --dir-diff --symlink without unstage\n>  \ttest_cmp actual expect\n>  '\n>  \n> +write_script modify-right-file <<\\EOF\n> +echo \"new content\" >\"$2/file\"\n> +EOF\n> +\n> +run_dir_diff_test 'difftool --dir-diff syncs worktree with unstaged change' '\n> +\ttest_when_finished git reset --hard &&\n> +\techo \"orig content\" >file &&\n> +\tgit difftool -d $symlinks --extcmd \"$(pwd)/modify-right-file\" branch &&\n> +\techo \"new content\" >expect &&\n> +\ttest_cmp expect file\n> +'\n> +\n> +run_dir_diff_test 'difftool --dir-diff syncs worktree without unstaged change' '\n> +\ttest_when_finished git reset --hard &&\n> +\tgit difftool -d $symlinks --extcmd \"$(pwd)/modify-right-file\" branch &&\n> +\techo \"new content\" >expect &&\n> +\ttest_cmp expect file\n> +'\n> +\n>  write_script modify-file <<\\EOF\n>  echo \"new content\" >file\n>  EOF\n> -- \n> 1.7.1\n"},{"id":"218611","messageId":"1369668697-1016-1-git-send-email-nitoyon@gmail.com","threadId":"33938","inReplyTo":"20130526154429.GK27005@serenity.lan","subject":"[PATCH v2] difftool --dir-diff: always use identical working tree file","fromName":"Kenichi Saita","fromEmail":"nitoyon@gmail.com","sentAt":"2013-05-27T15:31:37Z","receivedAt":"2013-05-27T15:31:37Z","isPatch":true,"sender":{"key":"nitoyon@gmail.com","avatar":"https://avatars.githubusercontent.com/u/39562?v=4"},"body":"When deciding whether or not we should link a working tree file into\nthe temporary right-hand directory for a directory diff, we\ncurrently behave differently in the --symlink and --no-symlink\ncases.  If using symlinks any identical files are linked across but\nwith --no-symlink only files that contain unstaged changes are\ncopied back into the working tree.\n\nChange this so that identical files are copied back as well.  This\nis beneficial because it widens the set of circumstances in which we\ncopy changes made by the user back into the working tree.\n\nSigned-off-by: Kenichi Saita <nitoyon@gmail.com>\n---\n git-difftool.perl   |    9 ++-------\n t/t7800-difftool.sh |   19 +++++++++++++++++++\n 2 files changed, 21 insertions(+), 7 deletions(-)\n\ndiff --git a/git-difftool.perl b/git-difftool.perl\nindex 8a75205..e57d3d1 100755\n--- a/git-difftool.perl\n+++ b/git-difftool.perl\n@@ -85,13 +85,9 @@ sub exit_cleanup\n \n sub use_wt_file\n {\n-\tmy ($repo, $workdir, $file, $sha1, $symlinks) = @_;\n+\tmy ($repo, $workdir, $file, $sha1) = @_;\n \tmy $null_sha1 = '0' x 40;\n \n-\tif ($sha1 ne $null_sha1 and not $symlinks) {\n-\t\treturn 0;\n-\t}\n-\n \tif (! -e \"$workdir/$file\") {\n \t\t# If the file doesn't exist in the working tree, we cannot\n \t\t# use it.\n@@ -213,8 +209,7 @@ EOF\n \n \t\tif ($rmode ne $null_mode) {\n \t\t\tmy ($use, $wt_sha1) = use_wt_file($repo, $workdir,\n-\t\t\t\t\t\t\t  $dst_path, $rsha1,\n-\t\t\t\t\t\t\t  $symlinks);\n+\t\t\t\t\t\t\t  $dst_path, $rsha1);\n \t\t\tif ($use) {\n \t\t\t\tpush @working_tree, $dst_path;\n \t\t\t\t$wtindex .= \"$rmode $wt_sha1\\t$dst_path\\0\";\ndiff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh\nindex d46f041..2418528 100755\n--- a/t/t7800-difftool.sh\n+++ b/t/t7800-difftool.sh\n@@ -385,6 +385,25 @@ test_expect_success PERL,SYMLINKS 'difftool --dir-diff --symlink without unstage\n \ttest_cmp actual expect\n '\n \n+write_script modify-right-file <<\\EOF\n+echo \"new content\" >\"$2/file\"\n+EOF\n+\n+run_dir_diff_test 'difftool --dir-diff syncs worktree with unstaged change' '\n+\ttest_when_finished git reset --hard &&\n+\techo \"orig content\" >file &&\n+\tgit difftool -d $symlinks --extcmd \"$(pwd)/modify-right-file\" branch &&\n+\techo \"new content\" >expect &&\n+\ttest_cmp expect file\n+'\n+\n+run_dir_diff_test 'difftool --dir-diff syncs worktree without unstaged change' '\n+\ttest_when_finished git reset --hard &&\n+\tgit difftool -d $symlinks --extcmd \"$(pwd)/modify-right-file\" branch &&\n+\techo \"new content\" >expect &&\n+\ttest_cmp expect file\n+'\n+\n write_script modify-file <<\\EOF\n echo \"new content\" >file\n EOF\n-- \n1.7.1\n"},{"id":"218690","messageId":"7vbo7v0yju.fsf@alter.siamese.dyndns.org","threadId":"33938","inReplyTo":"1369668697-1016-1-git-send-email-nitoyon@gmail.com","subject":"Re: [PATCH v2] difftool --dir-diff: always use identical working tree file","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-05-28T18:06:13Z","receivedAt":"2013-05-28T18:06:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kenichi Saita <nitoyon@gmail.com> writes:\n\n> When deciding whether or not we should link a working tree file into\n> the temporary right-hand directory for a directory diff, we\n> currently behave differently in the --symlink and --no-symlink\n> cases.  If using symlinks any identical files are linked across but\n> with --no-symlink only files that contain unstaged changes are\n> copied back into the working tree.\n\nI may have missed an earlier discussion, but I do not follow the\nlast sentence.  The former part (i.e. symlinks) talks about what is\ndone to populate the temporary tree (i.e. everything is linked), but\nthe latter part (i.e. not symlinks) only talks about what is copied\nback, i.e. it is not a contrast between the behaviour of symlink vs\nno-symlink case wrt how the temporary tree is populated.\n\nConfused...\n\n> Change this so that identical files are copied back as well.  This\n> is beneficial because it widens the set of circumstances in which we\n> copy changes made by the user back into the working tree.\n\nAh, OK, you meant that the set of files we keep in @working_tree\narray for later copying back are different between the two.\n\n> Signed-off-by: Kenichi Saita <nitoyon@gmail.com>\n> ---\n>  git-difftool.perl   |    9 ++-------\n>  t/t7800-difftool.sh |   19 +++++++++++++++++++\n>  2 files changed, 21 insertions(+), 7 deletions(-)\n>\n> diff --git a/git-difftool.perl b/git-difftool.perl\n> index 8a75205..e57d3d1 100755\n> --- a/git-difftool.perl\n> +++ b/git-difftool.perl\n> @@ -85,13 +85,9 @@ sub exit_cleanup\n>  \n>  sub use_wt_file\n>  {\n> -\tmy ($repo, $workdir, $file, $sha1, $symlinks) = @_;\n> +\tmy ($repo, $workdir, $file, $sha1) = @_;\n>  \tmy $null_sha1 = '0' x 40;\n>  \n> -\tif ($sha1 ne $null_sha1 and not $symlinks) {\n> -\t\treturn 0;\n> -\t}\n> -\n>  \tif (! -e \"$workdir/$file\") {\n>  \t\t# If the file doesn't exist in the working tree, we cannot\n>  \t\t# use it.\n> @@ -213,8 +209,7 @@ EOF\n>  \n>  \t\tif ($rmode ne $null_mode) {\n>  \t\t\tmy ($use, $wt_sha1) = use_wt_file($repo, $workdir,\n> -\t\t\t\t\t\t\t  $dst_path, $rsha1,\n> -\t\t\t\t\t\t\t  $symlinks);\n> +\t\t\t\t\t\t\t  $dst_path, $rsha1);\n>  \t\t\tif ($use) {\n>  \t\t\t\tpush @working_tree, $dst_path;\n>  \t\t\t\t$wtindex .= \"$rmode $wt_sha1\\t$dst_path\\0\";\n> diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh\n> index d46f041..2418528 100755\n> --- a/t/t7800-difftool.sh\n> +++ b/t/t7800-difftool.sh\n> @@ -385,6 +385,25 @@ test_expect_success PERL,SYMLINKS 'difftool --dir-diff --symlink without unstage\n>  \ttest_cmp actual expect\n>  '\n>  \n> +write_script modify-right-file <<\\EOF\n> +echo \"new content\" >\"$2/file\"\n> +EOF\n> +\n> +run_dir_diff_test 'difftool --dir-diff syncs worktree with unstaged change' '\n> +\ttest_when_finished git reset --hard &&\n> +\techo \"orig content\" >file &&\n> +\tgit difftool -d $symlinks --extcmd \"$(pwd)/modify-right-file\" branch &&\n> +\techo \"new content\" >expect &&\n> +\ttest_cmp expect file\n> +'\n> +\n> +run_dir_diff_test 'difftool --dir-diff syncs worktree without unstaged change' '\n> +\ttest_when_finished git reset --hard &&\n> +\tgit difftool -d $symlinks --extcmd \"$(pwd)/modify-right-file\" branch &&\n> +\techo \"new content\" >expect &&\n> +\ttest_cmp expect file\n> +'\n> +\n>  write_script modify-file <<\\EOF\n>  echo \"new content\" >file\n>  EOF\n"},{"id":"218692","messageId":"20130528181525.GA17475@serenity.lan","threadId":"33938","inReplyTo":"7vbo7v0yju.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2] difftool --dir-diff: always use identical working tree file","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-05-28T18:15:25Z","receivedAt":"2013-05-28T18:15:25Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"On Tue, May 28, 2013 at 11:06:13AM -0700, Junio C Hamano wrote:\n> Kenichi Saita <nitoyon@gmail.com> writes:\n> \n> > When deciding whether or not we should link a working tree file into\n> > the temporary right-hand directory for a directory diff, we\n> > currently behave differently in the --symlink and --no-symlink\n> > cases.  If using symlinks any identical files are linked across but\n> > with --no-symlink only files that contain unstaged changes are\n> > copied back into the working tree.\n> \n> I may have missed an earlier discussion, but I do not follow the\n> last sentence.  The former part (i.e. symlinks) talks about what is\n> done to populate the temporary tree (i.e. everything is linked), but\n> the latter part (i.e. not symlinks) only talks about what is copied\n> back, i.e. it is not a contrast between the behaviour of symlink vs\n> no-symlink case wrt how the temporary tree is populated.\n> \n> Confused...\n\nYeah, the commit message is still quite focused on the end effect of\ncopying files back.  But that's not what's being changed here.\n\nIn my suggested commit message I tried to make it clear that we're\nchanging when we decide to copy a file across to the temporary tree.\nThis has the beneficial (side-)effect of changing the set of files we\nconsider for copying back into the working tree after the diff tool has\nbeen run.\n\n> > Change this so that identical files are copied back as well.  This\n> > is beneficial because it widens the set of circumstances in which we\n> > copy changes made by the user back into the working tree.\n> \n> Ah, OK, you meant that the set of files we keep in @working_tree\n> array for later copying back are different between the two.\n> \n> > Signed-off-by: Kenichi Saita <nitoyon@gmail.com>\n> > ---\n> >  git-difftool.perl   |    9 ++-------\n> >  t/t7800-difftool.sh |   19 +++++++++++++++++++\n> >  2 files changed, 21 insertions(+), 7 deletions(-)\n> >\n> > diff --git a/git-difftool.perl b/git-difftool.perl\n> > index 8a75205..e57d3d1 100755\n> > --- a/git-difftool.perl\n> > +++ b/git-difftool.perl\n> > @@ -85,13 +85,9 @@ sub exit_cleanup\n> >  \n> >  sub use_wt_file\n> >  {\n> > -\tmy ($repo, $workdir, $file, $sha1, $symlinks) = @_;\n> > +\tmy ($repo, $workdir, $file, $sha1) = @_;\n> >  \tmy $null_sha1 = '0' x 40;\n> >  \n> > -\tif ($sha1 ne $null_sha1 and not $symlinks) {\n> > -\t\treturn 0;\n> > -\t}\n> > -\n> >  \tif (! -e \"$workdir/$file\") {\n> >  \t\t# If the file doesn't exist in the working tree, we cannot\n> >  \t\t# use it.\n> > @@ -213,8 +209,7 @@ EOF\n> >  \n> >  \t\tif ($rmode ne $null_mode) {\n> >  \t\t\tmy ($use, $wt_sha1) = use_wt_file($repo, $workdir,\n> > -\t\t\t\t\t\t\t  $dst_path, $rsha1,\n> > -\t\t\t\t\t\t\t  $symlinks);\n> > +\t\t\t\t\t\t\t  $dst_path, $rsha1);\n> >  \t\t\tif ($use) {\n> >  \t\t\t\tpush @working_tree, $dst_path;\n> >  \t\t\t\t$wtindex .= \"$rmode $wt_sha1\\t$dst_path\\0\";\n> > diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh\n> > index d46f041..2418528 100755\n> > --- a/t/t7800-difftool.sh\n> > +++ b/t/t7800-difftool.sh\n> > @@ -385,6 +385,25 @@ test_expect_success PERL,SYMLINKS 'difftool --dir-diff --symlink without unstage\n> >  \ttest_cmp actual expect\n> >  '\n> >  \n> > +write_script modify-right-file <<\\EOF\n> > +echo \"new content\" >\"$2/file\"\n> > +EOF\n> > +\n> > +run_dir_diff_test 'difftool --dir-diff syncs worktree with unstaged change' '\n> > +\ttest_when_finished git reset --hard &&\n> > +\techo \"orig content\" >file &&\n> > +\tgit difftool -d $symlinks --extcmd \"$(pwd)/modify-right-file\" branch &&\n> > +\techo \"new content\" >expect &&\n> > +\ttest_cmp expect file\n> > +'\n> > +\n> > +run_dir_diff_test 'difftool --dir-diff syncs worktree without unstaged change' '\n> > +\ttest_when_finished git reset --hard &&\n> > +\tgit difftool -d $symlinks --extcmd \"$(pwd)/modify-right-file\" branch &&\n> > +\techo \"new content\" >expect &&\n> > +\ttest_cmp expect file\n> > +'\n> > +\n> >  write_script modify-file <<\\EOF\n> >  echo \"new content\" >file\n> >  EOF\n> --\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at  http://vger.kernel.org/majordomo-info.html\n"},{"id":"218693","messageId":"7v7gij0w6z.fsf@alter.siamese.dyndns.org","threadId":"33938","inReplyTo":"20130528181525.GA17475@serenity.lan","subject":"Re: [PATCH v2] difftool --dir-diff: always use identical working tree file","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-05-28T18:57:08Z","receivedAt":"2013-05-28T18:57:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"John Keeping <john@keeping.me.uk> writes:\n\n> Yeah, the commit message is still quite focused on the end effect of\n> copying files back.  But that's not what's being changed here.\n>\n> In my suggested commit message I tried to make it clear that we're\n> changing when we decide to copy a file across to the temporary tree.\n> This has the beneficial (side-)effect of changing the set of files we\n> consider for copying back into the working tree after the diff tool has\n> been run.\n\nI actually think the effect of copying files back _is_ the primary\nmotivation of this change, and stressing that end effect is a much\nbetter description.  After all, if the working tree files do not\nhave any difference from the RHS of the comparison, copying from the\nworking tree and stuffing the $rsha1 to the RHS temporary index and\nrunning \"checkout -f\" should produce identical temporary directory\nfor the user to start viewing.\n\nThe _only_ difference in behaviour before and after this patch that\nmatters to the end user is if that path is in @working_tree, which\nis returned to @worktree of dir_diff sub to be later copied back,\nno?  I would view this change as a mere means, an implementation\ndetail, to achieve that end of stuffing more paths in the @worktree\narray.\n\nPerhaps\n\n\tdifftool --dir-diff: allow changing any clean working tree file\n\n\tThe temporary directory prepared by \"difftool --dir-diff\" to\n\tshow the result of a change can be modified by the user via\n\tthe tree diff program, and we try hard not to lose changes\n\tto them after tree diff program returns to us.\n\n        However, the set of files to be copied back is computed\n\tdifferently between --symlinks and --no-symlinks modes.  The\n\tformer checks all paths that start out as identical to the\n\tworking tree file, while the latter checks paths that\n\talready had a local modification in the working tree,\n\tallowing changes made in the tree diff program to paths that\n\tdid not have any local change to be lost.\n\nor something.  This invites a few questions, though.\n\n - By allowing these files in the temporary directory to be\n   modified, aren't we making the user's life harder by forcing them\n   to handle \"working tree file was already modified, made different\n   changes in the temporary directory, now these changes need to be\n   consolidated\" case?\n\n - When comparing two revisions, e.g. \"--dir-diff HEAD^^ HEAD^\",\n   that checks out (via $rsha1 to \"checkout -f\" codepath) a blob\n   that does not match what is in the working tree of HEAD to the\n   temporary directory, we still allow modifications to the copy in\n   the temporary directory, but what can the user do with these\n   changes that are _not_ based on HEAD, short of checking out HEAD^\n   and apply the difference first?\n\nI still cannot shake this nagging feeling that giving a writable\ntemporary directory might have been a mistake in the first place.\nPerhaps it may be a better design to make the ones that the user\nshouldn't touch (or will lead to the above confusion) read-only,\nwhile the ones that match the working tree read-write?\n"},{"id":"218694","messageId":"20130528190829.GB17475@serenity.lan","threadId":"33938","inReplyTo":"7v7gij0w6z.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2] difftool --dir-diff: always use identical working tree file","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-05-28T19:08:29Z","receivedAt":"2013-05-28T19:08:29Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"On Tue, May 28, 2013 at 11:57:08AM -0700, Junio C Hamano wrote:\n> John Keeping <john@keeping.me.uk> writes:\n> \n> > Yeah, the commit message is still quite focused on the end effect of\n> > copying files back.  But that's not what's being changed here.\n> >\n> > In my suggested commit message I tried to make it clear that we're\n> > changing when we decide to copy a file across to the temporary tree.\n> > This has the beneficial (side-)effect of changing the set of files we\n> > consider for copying back into the working tree after the diff tool has\n> > been run.\n> \n> I actually think the effect of copying files back _is_ the primary\n> motivation of this change, and stressing that end effect is a much\n> better description.  After all, if the working tree files do not\n> have any difference from the RHS of the comparison, copying from the\n> working tree and stuffing the $rsha1 to the RHS temporary index and\n> running \"checkout -f\" should produce identical temporary directory\n> for the user to start viewing.\n> \n> The _only_ difference in behaviour before and after this patch that\n> matters to the end user is if that path is in @working_tree, which\n> is returned to @worktree of dir_diff sub to be later copied back,\n> no?  I would view this change as a mere means, an implementation\n> detail, to achieve that end of stuffing more paths in the @worktree\n> array.\n\nI agree with this, but like you I found it confusing that the patch\ntouched code seemingly unrelated to copying files back.  I went toward\ndescribing the patch more literally and giving the motivation in the\nfinal paragraph.  Your message below is better, but I think it needs to\nsay that the set of files considered for copying back is the set that is\ncopied across to begin with.\n\n> Perhaps\n> \n> \tdifftool --dir-diff: allow changing any clean working tree file\n> \n> \tThe temporary directory prepared by \"difftool --dir-diff\" to\n> \tshow the result of a change can be modified by the user via\n> \tthe tree diff program, and we try hard not to lose changes\n> \tto them after tree diff program returns to us.\n> \n>         However, the set of files to be copied back is computed\n> \tdifferently between --symlinks and --no-symlinks modes.  The\n> \tformer checks all paths that start out as identical to the\n> \tworking tree file, while the latter checks paths that\n> \talready had a local modification in the working tree,\n> \tallowing changes made in the tree diff program to paths that\n> \tdid not have any local change to be lost.\n> \n> or something.  This invites a few questions, though.\n> \n>  - By allowing these files in the temporary directory to be\n>    modified, aren't we making the user's life harder by forcing them\n>    to handle \"working tree file was already modified, made different\n>    changes in the temporary directory, now these changes need to be\n>    consolidated\" case?\n> \n>  - When comparing two revisions, e.g. \"--dir-diff HEAD^^ HEAD^\",\n>    that checks out (via $rsha1 to \"checkout -f\" codepath) a blob\n>    that does not match what is in the working tree of HEAD to the\n>    temporary directory, we still allow modifications to the copy in\n>    the temporary directory, but what can the user do with these\n>    changes that are _not_ based on HEAD, short of checking out HEAD^\n>    and apply the difference first?\n> \n> I still cannot shake this nagging feeling that giving a writable\n> temporary directory might have been a mistake in the first place.\n> Perhaps it may be a better design to make the ones that the user\n> shouldn't touch (or will lead to the above confusion) read-only,\n> while the ones that match the working tree read-write?\n\nMy ideal scenario would be that we only allow users to edit files when\nthey are comparing against the working tree, but that would require\ngit-difftool to fully understand all git-diff options since it just\npasses through any it doesn't recognise.  I don't think there's an easy\nway to do that, which leaves us with this confusing situation.\n"},{"id":"218695","messageId":"7vtxlmzyt9.fsf@alter.siamese.dyndns.org","threadId":"33938","inReplyTo":"20130528190829.GB17475@serenity.lan","subject":"Re: [PATCH v2] difftool --dir-diff: always use identical working tree file","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-05-28T19:31:14Z","receivedAt":"2013-05-28T19:31:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"John Keeping <john@keeping.me.uk> writes:\n\n>>  - When comparing two revisions, e.g. \"--dir-diff HEAD^^ HEAD^\",\n>>    that checks out (via $rsha1 to \"checkout -f\" codepath) a blob\n>>    that does not match what is in the working tree of HEAD to the\n>>    temporary directory, we still allow modifications to the copy in\n>>    the temporary directory, but what can the user do with these\n>>    changes that are _not_ based on HEAD, short of checking out HEAD^\n>>    and apply the difference first?\n>> \n>> I still cannot shake this nagging feeling that giving a writable\n>> temporary directory might have been a mistake in the first place.\n>> Perhaps it may be a better design to make the ones that the user\n>> shouldn't touch (or will lead to the above confusion) read-only,\n>> while the ones that match the working tree read-write?\n>\n> My ideal scenario would be that we only allow users to edit files when\n> they are comparing against the working tree, but that would require\n> git-difftool to fully understand all git-diff options since it just\n> passes through any it doesn't recognise.  I don't think there's an easy\n> way to do that, which leaves us with this confusing situation.\n\nNot necessarily.\n\nLet's assume that changing files in \"diff\" tool is a sensible thing\nto do, as long as we make sure such a change is not lost (which I\nmay not 100% agree with, but let's put it aside for now).\n\nWhen you are viewing a file F in \"--dir-diff HEAD^^ HEAD^\", if there\nis no change for F in between HEAD^ and HEAD and you notice a typo\nthat may or may not be related to the differences between HEAD^^ and\nHEAD^, it would be tempting to fix that right there.  And as long as\nF in the working tree matches that of HEAD^ and the modification you\nmake in the temporary directory gets copied back to the working tree,\nyour typofix will end up to be in the working tree.\n\nWhich I _think_ is what people, who want to change files in \"diff\"\ntool, want to do.\n\nOf course, your working tree may have been in the middle of doing\nsomething entirely different and you may have to \"add [-p]\" to\nseparate such a typofix with other changes you were working on, but\nthat is a separate issue.\n\nAnd for that to work, the only think you need is \"does the blob we\nshow on the RHS temporary tree match what is in the working tree?\"\ncheck.  You do not need to know or care if you are comparing two old\nrevisions, etc.\n"},{"id":"218847","messageId":"1369843283-2328-1-git-send-email-nitoyon@gmail.com","threadId":"33938","inReplyTo":"7v7gij0w6z.fsf@alter.siamese.dyndns.org","subject":"[PATCH v3] difftool --dir-diff: allow changing any clean working tree file","fromName":"Kenichi Saita","fromEmail":"nitoyon@gmail.com","sentAt":"2013-05-29T16:01:23Z","receivedAt":"2013-05-29T16:01:23Z","isPatch":true,"sender":{"key":"nitoyon@gmail.com","avatar":"https://avatars.githubusercontent.com/u/39562?v=4"},"body":"The temporary directory prepared by \"difftool --dir-diff\" to\nshow the result of a change can be modified by the user via\nthe tree diff program, and we try hard not to lose changes\nto them after tree diff program returns to us.\n\nHowever, the set of files to be copied back is computed\ndifferently between --symlinks and --no-symlinks modes.  The\nformer checks all paths that start out as identical to the\nworking tree file, while the latter checks paths that\nalready had a local modification in the working tree,\nallowing changes made in the tree diff program to paths that\ndid not have any local change to be lost.\n\nSigned-off-by: Kenichi Saita <nitoyon@gmail.com>\n---\n git-difftool.perl   |    9 ++-------\n t/t7800-difftool.sh |   19 +++++++++++++++++++\n 2 files changed, 21 insertions(+), 7 deletions(-)\n\ndiff --git a/git-difftool.perl b/git-difftool.perl\nindex 8a75205..e57d3d1 100755\n--- a/git-difftool.perl\n+++ b/git-difftool.perl\n@@ -85,13 +85,9 @@ sub exit_cleanup\n \n sub use_wt_file\n {\n-\tmy ($repo, $workdir, $file, $sha1, $symlinks) = @_;\n+\tmy ($repo, $workdir, $file, $sha1) = @_;\n \tmy $null_sha1 = '0' x 40;\n \n-\tif ($sha1 ne $null_sha1 and not $symlinks) {\n-\t\treturn 0;\n-\t}\n-\n \tif (! -e \"$workdir/$file\") {\n \t\t# If the file doesn't exist in the working tree, we cannot\n \t\t# use it.\n@@ -213,8 +209,7 @@ EOF\n \n \t\tif ($rmode ne $null_mode) {\n \t\t\tmy ($use, $wt_sha1) = use_wt_file($repo, $workdir,\n-\t\t\t\t\t\t\t  $dst_path, $rsha1,\n-\t\t\t\t\t\t\t  $symlinks);\n+\t\t\t\t\t\t\t  $dst_path, $rsha1);\n \t\t\tif ($use) {\n \t\t\t\tpush @working_tree, $dst_path;\n \t\t\t\t$wtindex .= \"$rmode $wt_sha1\\t$dst_path\\0\";\ndiff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh\nindex d46f041..2418528 100755\n--- a/t/t7800-difftool.sh\n+++ b/t/t7800-difftool.sh\n@@ -385,6 +385,25 @@ test_expect_success PERL,SYMLINKS 'difftool --dir-diff --symlink without unstage\n \ttest_cmp actual expect\n '\n \n+write_script modify-right-file <<\\EOF\n+echo \"new content\" >\"$2/file\"\n+EOF\n+\n+run_dir_diff_test 'difftool --dir-diff syncs worktree with unstaged change' '\n+\ttest_when_finished git reset --hard &&\n+\techo \"orig content\" >file &&\n+\tgit difftool -d $symlinks --extcmd \"$(pwd)/modify-right-file\" branch &&\n+\techo \"new content\" >expect &&\n+\ttest_cmp expect file\n+'\n+\n+run_dir_diff_test 'difftool --dir-diff syncs worktree without unstaged change' '\n+\ttest_when_finished git reset --hard &&\n+\tgit difftool -d $symlinks --extcmd \"$(pwd)/modify-right-file\" branch &&\n+\techo \"new content\" >expect &&\n+\ttest_cmp expect file\n+'\n+\n write_script modify-file <<\\EOF\n echo \"new content\" >file\n EOF\n-- \n1.7.1\n"},{"id":"218890","messageId":"7vfvx5sgwb.fsf@alter.siamese.dyndns.org","threadId":"33938","inReplyTo":"1369843283-2328-1-git-send-email-nitoyon@gmail.com","subject":"Re: [PATCH v3] difftool --dir-diff: allow changing any clean working tree file","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-05-29T19:52:20Z","receivedAt":"2013-05-29T19:52:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kenichi Saita <nitoyon@gmail.com> writes:\n\n> The temporary directory prepared by \"difftool --dir-diff\" to\n> show the result of a change can be modified by the user via\n> the tree diff program, and we try hard not to lose changes\n> to them after tree diff program returns to us.\n\nThanks; will queue (unless there are other suggestions, in which\ncase we can polish it further while it is on 'pu').\n\n>\n> However, the set of files to be copied back is computed\n> differently between --symlinks and --no-symlinks modes.  The\n> former checks all paths that start out as identical to the\n> working tree file, while the latter checks paths that\n> already had a local modification in the working tree,\n> allowing changes made in the tree diff program to paths that\n> did not have any local change to be lost.\n>\n> Signed-off-by: Kenichi Saita <nitoyon@gmail.com>\n> ---\n>  git-difftool.perl   |    9 ++-------\n>  t/t7800-difftool.sh |   19 +++++++++++++++++++\n>  2 files changed, 21 insertions(+), 7 deletions(-)\n>\n> diff --git a/git-difftool.perl b/git-difftool.perl\n> index 8a75205..e57d3d1 100755\n> --- a/git-difftool.perl\n> +++ b/git-difftool.perl\n> @@ -85,13 +85,9 @@ sub exit_cleanup\n>  \n>  sub use_wt_file\n>  {\n> -\tmy ($repo, $workdir, $file, $sha1, $symlinks) = @_;\n> +\tmy ($repo, $workdir, $file, $sha1) = @_;\n>  \tmy $null_sha1 = '0' x 40;\n>  \n> -\tif ($sha1 ne $null_sha1 and not $symlinks) {\n> -\t\treturn 0;\n> -\t}\n> -\n>  \tif (! -e \"$workdir/$file\") {\n>  \t\t# If the file doesn't exist in the working tree, we cannot\n>  \t\t# use it.\n> @@ -213,8 +209,7 @@ EOF\n>  \n>  \t\tif ($rmode ne $null_mode) {\n>  \t\t\tmy ($use, $wt_sha1) = use_wt_file($repo, $workdir,\n> -\t\t\t\t\t\t\t  $dst_path, $rsha1,\n> -\t\t\t\t\t\t\t  $symlinks);\n> +\t\t\t\t\t\t\t  $dst_path, $rsha1);\n>  \t\t\tif ($use) {\n>  \t\t\t\tpush @working_tree, $dst_path;\n>  \t\t\t\t$wtindex .= \"$rmode $wt_sha1\\t$dst_path\\0\";\n> diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh\n> index d46f041..2418528 100755\n> --- a/t/t7800-difftool.sh\n> +++ b/t/t7800-difftool.sh\n> @@ -385,6 +385,25 @@ test_expect_success PERL,SYMLINKS 'difftool --dir-diff --symlink without unstage\n>  \ttest_cmp actual expect\n>  '\n>  \n> +write_script modify-right-file <<\\EOF\n> +echo \"new content\" >\"$2/file\"\n> +EOF\n> +\n> +run_dir_diff_test 'difftool --dir-diff syncs worktree with unstaged change' '\n> +\ttest_when_finished git reset --hard &&\n> +\techo \"orig content\" >file &&\n> +\tgit difftool -d $symlinks --extcmd \"$(pwd)/modify-right-file\" branch &&\n> +\techo \"new content\" >expect &&\n> +\ttest_cmp expect file\n> +'\n> +\n> +run_dir_diff_test 'difftool --dir-diff syncs worktree without unstaged change' '\n> +\ttest_when_finished git reset --hard &&\n> +\tgit difftool -d $symlinks --extcmd \"$(pwd)/modify-right-file\" branch &&\n> +\techo \"new content\" >expect &&\n> +\ttest_cmp expect file\n> +'\n> +\n>  write_script modify-file <<\\EOF\n>  echo \"new content\" >file\n>  EOF\n"}]}