difftool -d symlinks, under what conditions

difftool -d symlinks, under what conditions

[Matt McClure]

I'm finding the behavior of `git difftool -d` surprising. It seems that it
only uses symlinks to the working copy for files that are modified in the
working copy since the most recent commit. I would have expected it to use
symlinks for all files whose version under comparison is the working copy
version, regardless of whether the working copy differs from the HEAD.

I'm using

    $ git --version
    git version 1.8.0

on a Mac from Homebrew.

--
Matt McClure
http://www.matthewlmcclure.com
http://www.mapmyfitness.com/profile/matthewlmcclure

Re: difftool -d symlinks, under what conditions

[David Aguilar]

On Mon, Nov 26, 2012 at 12:23 PM, Matt McClure
<matthewlmcclure@gmail.com> wrote:
> I'm finding the behavior of `git difftool -d` surprising. It seems that it
> only uses symlinks to the working copy for files that are modified in the
> working copy since the most recent commit. I would have expected it to use
> symlinks for all files whose version under comparison is the working copy
> version, regardless of whether the working copy differs from the HEAD.
>
> I'm using
>
>     $ git --version
>     git version 1.8.0
>
> on a Mac from Homebrew.

cc:ing Tim since he probably remembers this feature.

This is a side-effect of how it's currently implemented,
and the general-purpose nature of the "diff" command.

diff can also be used for diffing arbitrary commits.
The simplest way to implement that is to create two temporary
directories containing "a/" and "b/" and then launch the tool
against them.  That's what difftool does; it creates a temporary
index and uses `git checkout-index` to populate these two dirs.

The worktree handling is a bolt-on that symlinks
(or copies (on windows or with --no-symlinks)) modified
worktree files into one of these temporary directories.

When symlinks are used (the default) we avoid needing to
copy these files back into the worktree; we can blindly
remove the temporary directories without checking whether
the tool edited any files.

When copies are used we check their content for changes
before deciding to copy them back into the worktree.

Files that are not modified are not considered part of the
set of files to check when copying back, or when symlinking,
mostly because that's just how it's implemented right now.

It seems that there is an edge case here that we are not
accounting for: unmodified worktree paths, when checked out
into the temporary directory, can be edited by the tool when
comparing against older commits.  These edits will be lost.

If we had a way to know that either a/ or b/ can be replaced
with the worktree itself then we could make it even simpler.

Right now we don't because difftool barely parses the
command-line at all; most of it is parsed by git-diff.
Originally, difftool was a read-only tool so it was able to
avoid needing to know too much about what diff is really doing.

We would need to a way to re-use git's diff command-line parsing
logic to answer: "is the worktree involved in this diff invocation?"

When we can do that then we avoid needing to have a temporary
directory altogether for any dir-diffs that involve the worktree.

Does anyone know of a good way to answer that question?

The input is the command-line provided to diff/difftool.
The output is one of ('a', 'b', 'x'), where 'a' means the left
side of the diff is the worktree, 'b' means the right side,
and 'x' means neither (e.g. the command-line contains two refs).

Assuming we can do this, it would also make dir-diff faster
since we can avoid needing to checkout the entire tree for
that side of the diff.
-- 
David

Re: difftool -d symlinks, under what conditions

[Matt McClure]

On Tuesday, November 27, 2012, David Aguilar wrote:
> It seems that there is an edge case here that we are not
> accounting for: unmodified worktree paths, when checked out
> into the temporary directory, can be edited by the tool when
> comparing against older commits.  These edits will be lost.

Yes. That is exactly my desired use case. I want to make edits while
I'm reviewing the diff.

>
> When we can do that then we avoid needing to have a temporary
> directory altogether for any dir-diffs that involve the worktree.

I think the temporary directory is still a good thing. Without it, the
directory diff tool would have no way to distinguish a file added in
the diff from a file that was preexisting and unmodified.

--
Matt McClure
http://www.matthewlmcclure.com
http://www.mapmyfitness.com/profile/matthewlmcclure

Re: difftool -d symlinks, under what conditions

[Matt McClure]

On Tue, Nov 27, 2012 at 7:41 AM, Matt McClure <matthewlmcclure@gmail.com> wrote:
>
> On Tuesday, November 27, 2012, David Aguilar wrote:
>>
>> It seems that there is an edge case here that we are not
>> accounting for: unmodified worktree paths, when checked out
>> into the temporary directory, can be edited by the tool when
>> comparing against older commits.  These edits will be lost.
>
>
> Yes. That is exactly my desired use case. I want to make edits while I'm reviewing the diff.

I took a crack at implementing the change to make difftool -d use
symlinks more aggressively. I've tested it lightly, and it works for
the limited cases I've tried. This is my first foray into the Git
source code, so it's entirely possible that there are unintended side
effects and regressions if other features depend on the same code path
and make different assumptions.

https://github.com/matthewlmcclure/git/compare/difftool-directory-symlink-work-tree

Your thoughts on the change?

--
Matt McClure
http://www.matthewlmcclure.com
http://www.mapmyfitness.com/profile/matthewlmcclure

Re: difftool -d symlinks, under what conditions

[John Keeping]

On Tue, Mar 12, 2013 at 02:12:29PM -0400, Matt McClure wrote:
> On Tue, Nov 27, 2012 at 7:41 AM, Matt McClure <matthewlmcclure@gmail.com> wrote:
> Your thoughts on the change?

Please include the patch in your message so that interested parties can
comment on it here, especially since the compare view on GitHub seems to
mangle the tabs.

For others' reference the patch is:

-- >8 --
From: Matt McClure <matt.mcclure@mapmyfitness.com>
Subject: [PATCH] difftool: Make directory diff symlink work tree

difftool -d formerly knew how to symlink to the work tree when the work
tree contains uncommitted changes. In practice, prior to this change, it
would not symlink to the work tree in case there were no uncommitted
changes, even when the user invoked difftool with the form:

    git difftool -d [--options] <commit> [--] [<path>...]
        This form is to view the changes you have in your working tree
        relative to the named <commit>. You can use HEAD to compare it
        with the latest commit, or a branch name to compare with the tip
        of a different branch.

Instead, prior to this change, difftool would use the file's HEAD blob
sha1 to find its content rather than the work tree content. This change
teaches `git diff --raw` to emit the null SHA1 for consumption by
difftool -d, so that difftool -d will use a symlink rather than a copy
of the file.

Before:

    $ git diff --raw HEAD^ -- diff-lib.c
    :100644 100644 f35de0f... ead9399... M  diff-lib.c

After:

    $ ./git diff --raw HEAD^ -- diff-lib.c
    :100644 100644 f35de0f... 0000000... M  diff-lib.c
---
 diff-lib.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/diff-lib.c b/diff-lib.c
index f35de0f..ead9399 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -319,6 +319,10 @@ static int show_modified(struct rev_info *revs,
 		return -1;
 	}
 
+	if (!cached && hashcmp(old->sha1, new->sha1)) {
+		sha1 = null_sha1;
+	}
+
 	if (revs->combine_merges && !cached &&
 	    (hashcmp(sha1, old->sha1) || hashcmp(old->sha1, new->sha1))) {
 		struct combine_diff_path *p;
-- 
1.8.2.rc2.4.g7799588

Re: difftool -d symlinks, under what conditions

[David Aguilar]

On Tue, Mar 12, 2013 at 12:09 PM, John Keeping <john@keeping.me.uk> wrote:
> On Tue, Mar 12, 2013 at 02:12:29PM -0400, Matt McClure wrote:
>> On Tue, Nov 27, 2012 at 7:41 AM, Matt McClure <matthewlmcclure@gmail.com> wrote:
>> Your thoughts on the change?
>
> Please include the patch in your message so that interested parties can
> comment on it here, especially since the compare view on GitHub seems to
> mangle the tabs.
>
> For others' reference the patch is:
>
> -- >8 --
> From: Matt McClure <matt.mcclure@mapmyfitness.com>
> Subject: [PATCH] difftool: Make directory diff symlink work tree
>
> difftool -d formerly knew how to symlink to the work tree when the work
> tree contains uncommitted changes. In practice, prior to this change, it
> would not symlink to the work tree in case there were no uncommitted
> changes, even when the user invoked difftool with the form:
>
>     git difftool -d [--options] <commit> [--] [<path>...]
>         This form is to view the changes you have in your working tree
>         relative to the named <commit>. You can use HEAD to compare it
>         with the latest commit, or a branch name to compare with the tip
>         of a different branch.
>
> Instead, prior to this change, difftool would use the file's HEAD blob
> sha1 to find its content rather than the work tree content. This change
> teaches `git diff --raw` to emit the null SHA1 for consumption by
> difftool -d, so that difftool -d will use a symlink rather than a copy
> of the file.
>
> Before:
>
>     $ git diff --raw HEAD^ -- diff-lib.c
>     :100644 100644 f35de0f... ead9399... M  diff-lib.c
>
> After:
>
>     $ ./git diff --raw HEAD^ -- diff-lib.c
>     :100644 100644 f35de0f... 0000000... M  diff-lib.c


Interesting approach.  While this does get the intended behavior
for difftool, I'm afraid this would be a grave regression for
existing "git diff --raw" users who cannot have such behavior.

I don't think we could do this without adding an additional flag
to trigger this change in behavior (e.g. --null-sha1-for-....?)
so that existing users are unaffected by the change.

It feels like forcing the null SHA-1 is heavy-handed, but I
haven't thought it through enough.

While this may be a quick way to get this behavior,
I wonder if there is a better way.

Does anybody else have any comments/suggestions on how to
better accomplish this?


> ---
>  diff-lib.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/diff-lib.c b/diff-lib.c
> index f35de0f..ead9399 100644
> --- a/diff-lib.c
> +++ b/diff-lib.c
> @@ -319,6 +319,10 @@ static int show_modified(struct rev_info *revs,
>                 return -1;
>         }
>
> +       if (!cached && hashcmp(old->sha1, new->sha1)) {
> +               sha1 = null_sha1;
> +       }
> +
>         if (revs->combine_merges && !cached &&
>             (hashcmp(sha1, old->sha1) || hashcmp(old->sha1, new->sha1))) {
>                 struct combine_diff_path *p;
> --
> 1.8.2.rc2.4.g7799588
>



-- 
David

Re: [PATCH] difftool: Make directory diff symlink work tree

[John Keeping]

> difftool -d formerly knew how to symlink to the work tree when the work
> tree contains uncommitted changes. In practice, prior to this change, it
> would not symlink to the work tree in case there were no uncommitted
> changes, even when the user invoked difftool with the form:
> 
>     git difftool -d [--options] <commit> [--] [<path>...]
>         This form is to view the changes you have in your working tree
>         relative to the named <commit>. You can use HEAD to compare it
>         with the latest commit, or a branch name to compare with the tip
>         of a different branch.
> 
> Instead, prior to this change, difftool would use the file's HEAD blob
> sha1 to find its content rather than the work tree content. This change
> teaches `git diff --raw` to emit the null SHA1 for consumption by
> difftool -d, so that difftool -d will use a symlink rather than a copy
> of the file.
> 
> Before:
> 
>     $ git diff --raw HEAD^ -- diff-lib.c
>     :100644 100644 f35de0f... ead9399... M  diff-lib.c
> 
> After:
> 
>     $ ./git diff --raw HEAD^ -- diff-lib.c
>     :100644 100644 f35de0f... 0000000... M  diff-lib.c

When I tried this I got the expected behaviour even without this patch.

It turns out that an uncommitted, but *staged* change emits the SHA1 of
the blob rather than the null SHA1.  Do you get the desired behaviour if
you "git reset" before using difftool?

If so I think you want some new mode of operation for difftool instead
of this patch which will also affect unrelated commands.

> ---
>  diff-lib.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/diff-lib.c b/diff-lib.c
> index f35de0f..ead9399 100644
> --- a/diff-lib.c
> +++ b/diff-lib.c
> @@ -319,6 +319,10 @@ static int show_modified(struct rev_info *revs,
>  		return -1;
>  	}
>  
> +	if (!cached && hashcmp(old->sha1, new->sha1)) {
> +		sha1 = null_sha1;
> +	}
> +
>  	if (revs->combine_merges && !cached &&
>  	    (hashcmp(sha1, old->sha1) || hashcmp(old->sha1, new->sha1))) {
>  		struct combine_diff_path *p;
> -- 
> 1.8.2.rc2.4.g7799588
> 

Re: difftool -d symlinks, under what conditions

[John Keeping]

On Tue, Mar 12, 2013 at 12:23:52PM -0700, David Aguilar wrote:
> I don't think we could do this without adding an additional flag
> to trigger this change in behavior (e.g. --null-sha1-for-....?)
> so that existing users are unaffected by the change.
> 
> It feels like forcing the null SHA-1 is heavy-handed, but I
> haven't thought it through enough.
> 
> While this may be a quick way to get this behavior,
> I wonder if there is a better way.
> 
> Does anybody else have any comments/suggestions on how to
> better accomplish this?

How about something like "--symlink-all" where the everything in the
right-hand tree is symlink'd?

Something like this perhaps:

-- >8 --
diff --git a/git-difftool.perl b/git-difftool.perl
index 0a90de4..cab7c45 100755
--- a/git-difftool.perl
+++ b/git-difftool.perl
@@ -85,7 +85,7 @@ sub exit_cleanup
 
 sub setup_dir_diff
 {
-	my ($repo, $workdir, $symlinks) = @_;
+	my ($repo, $workdir, $symlinks, $symlink_all) = @_;
 
 	# Run the diff; exit immediately if no diff found
 	# 'Repository' and 'WorkingCopy' must be explicitly set to insure that
@@ -159,10 +159,10 @@ EOF
 		}
 
 		if ($rmode ne $null_mode) {
-			if ($rsha1 ne $null_sha1) {
-				$rindex .= "$rmode $rsha1\t$dst_path\0";
-			} else {
+			if ($symlink_all or $rsha1 eq $null_sha1) {
 				push(@working_tree, $dst_path);
+			} else {
+				$rindex .= "$rmode $rsha1\t$dst_path\0";
 			}
 		}
 	}
@@ -299,6 +299,7 @@ sub main
 		prompt => undef,
 		symlinks => $^O ne 'cygwin' &&
 				$^O ne 'MSWin32' && $^O ne 'msys',
+		symlink_all => undef,
 		tool_help => undef,
 	);
 	GetOptions('g|gui!' => \$opts{gui},
@@ -308,6 +309,7 @@ sub main
 		'y' => sub { $opts{prompt} = 0; },
 		'symlinks' => \$opts{symlinks},
 		'no-symlinks' => sub { $opts{symlinks} = 0; },
+		'symlink-all' => \$opts{symlink_all},
 		't|tool:s' => \$opts{difftool_cmd},
 		'tool-help' => \$opts{tool_help},
 		'x|extcmd:s' => \$opts{extcmd});
@@ -346,7 +348,7 @@ sub main
 	# will invoke a separate instance of 'git-difftool--helper' for
 	# each file that changed.
 	if (defined($opts{dirdiff})) {
-		dir_diff($opts{extcmd}, $opts{symlinks});
+		dir_diff($opts{extcmd}, $opts{symlinks}, $opts{symlink_all});
 	} else {
 		file_diff($opts{prompt});
 	}
@@ -354,13 +356,13 @@ sub main
 
 sub dir_diff
 {
-	my ($extcmd, $symlinks) = @_;
+	my ($extcmd, $symlinks, $symlink_all) = @_;
 	my $rc;
 	my $error = 0;
 	my $repo = Git->repository();
 	my $workdir = find_worktree($repo);
 	my ($a, $b, $tmpdir, @worktree) =
-		setup_dir_diff($repo, $workdir, $symlinks);
+		setup_dir_diff($repo, $workdir, $symlinks, $symlink_all);
 
 	if (defined($extcmd)) {
 		$rc = system($extcmd, $a, $b);

Re: difftool -d symlinks, under what conditions

[Junio C Hamano]

David Aguilar <davvid@gmail.com> writes:

> Interesting approach.  While this does get the intended behavior
> for difftool, I'm afraid this would be a grave regression for
> existing "git diff --raw" users who cannot have such behavior.

The 0{40} in RHS of --raw output is to say that we do not know what
object name the contents at the path hashes to.

If you run "git diff HEAD^" for a path that is different between
HEAD and the index for which you do not have a local change in the
working tree, we have to show the path (because it is different
between the working tree and HEAD^), but we know the object name for
copy in the working tree, simply because we know it matches what is
in the index.  Showing 0{40} on the RHS in such a case loses
information, making us say "We don't know" when we perfectly well
know.  That is a regression.

If the user is allowed to touch any random file in the working tree,
I do not see a workable solution other than John Keeping's follow-up
patch to make symlinks of all paths involved.

Re: difftool -d symlinks, under what conditions

[Junio C Hamano]

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

> How about something like "--symlink-all" where the everything in the
> right-hand tree is symlink'd?

Does it even have to be conditional?  What is the situation when you
do not want symbolic links?

Re: difftool -d symlinks, under what conditions

[John Keeping]

On Tue, Mar 12, 2013 at 01:39:17PM -0700, Junio C Hamano wrote:
> John Keeping <john@keeping.me.uk> writes:
> 
> > How about something like "--symlink-all" where the everything in the
> > right-hand tree is symlink'd?
> 
> Does it even have to be conditional?  What is the situation when you
> do not want symbolic links?

When you're not comparing the working tree.

If we can reliably say "the RHS is the working tree" then it could be
unconditional, but I haven't thought about how to do that - I can't see
a particularly easy way to check for that; is it sufficient to say
"there is no more than one non-option to the left of '--' and '--cached'
is not among the options"?


John

Re: difftool -d symlinks, under what conditions

[Junio C Hamano]

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

>> Does it even have to be conditional?  What is the situation when you
>> do not want symbolic links?
>
> When you're not comparing the working tree.

OK, so what you want is essentially:

 * If you see 0{40} in "diff --raw", you *know* you are showing the working tree
   file on the RHS, and you want to symlink, so that the edit made
   by the user will be reflected back to theh working tree copy.
 
 * If your working tree file match what is in the index, you won't
   see 0{40} but you still want to symlink, for the same reason.

 * If you are comparing two trees, and especially if your RHS is not
   HEAD, you will send everything to a temporary without
   symlinks. Any edit made by the user will be lost.

If that is the case, perhaps the safest way to go may be to write
the object out when you see non 0{40}, compare it with the working
tree version and then turn that into symlink?  That way, you not
only cover the second bullet point, but also cover half of the third
one where the user may find a bug in the RHS, update it in difftool.

I am assuming that you are write-protecting the non-symlink files in
the temporary tree (i.e. those that do not match the working tree)
to prevent users from accidentally modifying something there is no
place to save back to.

Hrm?

Re: difftool -d symlinks, under what conditions

[Matt McClure]

On Tue, Mar 12, 2013 at 5:06 PM, John Keeping <john@keeping.me.uk> wrote:
> On Tue, Mar 12, 2013 at 01:39:17PM -0700, Junio C Hamano wrote:
>>
>> What is the situation when you do not want symbolic links?
>
> When you're not comparing the working tree.
>
> If we can reliably say "the RHS is the working tree" then it could be
> unconditional, but I haven't thought about how to do that - I can't see
> a particularly easy way to check for that;

Agreed. From what I can see, the only form of the diff options that
compares against the working tree is

    git difftool -d [--options] <commit> [--] [<path>...]

At first, I thought that the following cases were also working tree
cases, but actually they use the HEAD commit.

    git difftool -d commit1..
    git difftool -d commit1...

> is it sufficient to say
> "there is no more than one non-option to the left of '--' and '--cached'
> is not among the options"?

An alternative approach would be to reuse git-diff's option parsing
and make it tell git-difftool when git-diff sees the working tree
case. At this point, I haven't seen an obvious place in the source
where git-diff makes that choice, but if someone could point me in the
right direction, I think I'd actually prefer that approach. What do
you think?

--
Matt McClure
http://www.matthewlmcclure.com
http://www.mapmyfitness.com/profile/matthewlmcclure

Re: difftool -d symlinks, under what conditions

[Matt McClure]

On Tue, Mar 12, 2013 at 5:43 PM, Matt McClure <matthewlmcclure@gmail.com> wrote:
> On Tue, Mar 12, 2013 at 5:06 PM, John Keeping <john@keeping.me.uk> wrote:
>>
>> is it sufficient to say
>> "there is no more than one non-option to the left of '--' and '--cached'
>> is not among the options"?
>
> An alternative approach would be to reuse git-diff's option parsing
> and make it tell git-difftool when git-diff sees the working tree
> case. At this point, I haven't seen an obvious place in the source
> where git-diff makes that choice, but if someone could point me in the
> right direction, I think I'd actually prefer that approach. What do
> you think?

There's an interesting comment in cmd_diff:

/*
* We could get N tree-ish in the rev.pending_objects list.
* Also there could be M blobs there, and P pathspecs.
*
* N=0, M=0:
* cache vs files (diff-files)
* N=0, M=2:
*      compare two random blobs.  P must be zero.
* N=0, M=1, P=1:
* compare a blob with a working tree file.
*
* N=1, M=0:
*      tree vs cache (diff-index --cached)
*
* N=2, M=0:
*      tree vs tree (diff-tree)
*
* N=0, M=0, P=2:
*      compare two filesystem entities (aka --no-index).
*
* Other cases are errors.
*/

whereas inspecting rev.pending in the "compare against working tree"
case, I see:

(gdb) p rev.pending
$3 = {
  nr = 1,
  alloc = 64,
  objects = 0x100807a00
}
(gdb) p *rev.pending.objects
$4 = {
  item = 0x100831a48,
  name = 0x7fff5fbff8f8 "HEAD^",
  mode = 12288
}

Given the cases listed in the comment, I assume cmd_diff must
interpret this case as:

* N=1, M=0:
*      tree vs cache (diff-index --cached)

The description of that case is confusing or wrong given that
git-diff-index(1) says:

       --cached
           do not consider the on-disk file at all

***

cmd_diff executes this case:

else if (ents == 1)
    result = builtin_diff_index(&rev, argc, argv);

So it looks like I could short-circuit in builtin_diff_index or
something it calls -- e.g., run_diff_index -- to get git-diff to tell
git-difftool that it's the working tree case. I see that
run_diff_index does:

    diff_set_mnemonic_prefix(&revs->diffopt, "c/", cached ? "i/" : "w/");

So that looks like a good place where the code is already deciding
that it's the working tree case -- "w/", though surprisingly to me:

(gdb) p revs->diffopt
$12 = {
...
  a_prefix = 0x1001c25aa "a/",
  b_prefix = 0x1001c25ad "b/",
...

So diff_set_mnemonic_prefix doesn't actually use the "w/" value passed
to it because:

if (!options->b_prefix)
    options->b_prefix = b;

Maybe if I could prevent b_prefix from getting set earlier, I could
get some variant of git-diff to emit the "w/" for git-difftool.

--
Matt McClure
http://www.matthewlmcclure.com
http://www.mapmyfitness.com/profile/matthewlmcclure

Re: difftool -d symlinks, under what conditions

[Junio C Hamano]

Matt McClure <matthewlmcclure@gmail.com> writes:

> An alternative approach would be to reuse git-diff's option parsing
> and make it tell git-difftool when git-diff sees the working tree
> case. At this point, I haven't seen an obvious place in the source
> where git-diff makes that choice, but if someone could point me in the
> right direction, I think I'd actually prefer that approach. What do
> you think?

I do not think you want to go there.  That wouldn't solve the third
case in my previous message, no?

Re: difftool -d symlinks, under what conditions

[Matt McClure]

On Mar 12, 2013, at 4:16 PM, Junio C Hamano <gitster@pobox.com> wrote:

> Matt McClure <matthewlmcclure@gmail.com> writes:
>
>> An alternative approach would be to reuse git-diff's option parsing
>
> I do not think you want to go there.  That wouldn't solve the third
> case in my previous message, no?

I think I don't fully understand your third bullet.

> * If you are comparing two trees, and especially if your RHS is not
>   HEAD, you will send everything to a temporary without
>   symlinks. Any edit made by the user will be lost.

I think you're suggesting to use a symlink any time the content of any
given RHS revision is the same as the working tree.

I imagine that might confuse me as a user. It would create
circumstances where some files are symlinked and others aren't for
reasons that won't be straightforward.

I imagine solving that case, I might instead implement a copy back to
the working tree with conflict detection/resolution. Some earlier
iterations of the directory diff feature used copy back without
conflict detection and created situations where I clobbered my own
changes by finishing a directory diff after making edits concurrently.

Re: [PATCH] difftool: Make directory diff symlink work tree

[Matt McClure]

On Mar 12, 2013, at 1:25 PM, John Keeping <john@keeping.me.uk> wrote:

> When I tried this I got the expected behaviour even without this patch.

    git diff --raw commit

emits the null SHA1 if the working tree file's stat differs from the
blob corresponding to commit. Is that the case you observed?

Re: [PATCH] difftool: Make directory diff symlink work tree

[John Keeping]

On Tue, Mar 12, 2013 at 05:12:28PM -0600, Matt McClure wrote:
> On Mar 12, 2013, at 1:25 PM, John Keeping <john@keeping.me.uk> wrote:
> 
> > When I tried this I got the expected behaviour even without this patch.
> 
>     git diff --raw commit
> 
> emits the null SHA1 if the working tree file's stat differs from the
> blob corresponding to commit. Is that the case you observed?

Yes, although it's slightly more subtle than that - the null SHA1 only
occurs if the working tree file has unstaged changes; if you add the
changes to the index then the null SHA1 is no longer used since the blob
is now available in Git's object store.


John

Re: difftool -d symlinks, under what conditions

[John Keeping]

On Tue, Mar 12, 2013 at 04:48:16PM -0600, Matt McClure wrote:
> On Mar 12, 2013, at 4:16 PM, Junio C Hamano <gitster@pobox.com> wrote:
> 
> > Matt McClure <matthewlmcclure@gmail.com> writes:
> >
> > * If you are comparing two trees, and especially if your RHS is not
> >   HEAD, you will send everything to a temporary without
> >   symlinks. Any edit made by the user will be lost.
> 
> I think you're suggesting to use a symlink any time the content of any
> given RHS revision is the same as the working tree.
> 
> I imagine that might confuse me as a user. It would create
> circumstances where some files are symlinked and others aren't for
> reasons that won't be straightforward.
> 
> I imagine solving that case, I might instead implement a copy back to
> the working tree with conflict detection/resolution. Some earlier
> iterations of the directory diff feature used copy back without
> conflict detection and created situations where I clobbered my own
> changes by finishing a directory diff after making edits concurrently.

The code to copy back working tree files is already there, it just
triggers using the same logic as the creation of symlinks in the first
place and doesn't attempt any conflict detection.  I suspect that any
more comprehensive solution will need to restrict the use of "git
difftool -d" whenever the index contains unmerged entries or when there
are both staged and unstaged changes, since the merge resolution will
cause these states to be lost.

The implementation of Junio's suggestion is relatively straightforward
(this is untested, although t7800 passes, and can probably be improved
by someone better versed in Perl).  Does this work for your original
scenario?

-- >8 --
diff --git a/git-difftool.perl b/git-difftool.perl
index 0a90de4..5f093ae 100755
--- a/git-difftool.perl
+++ b/git-difftool.perl
@@ -83,6 +83,21 @@ sub exit_cleanup
 	exit($status | ($status >> 8));
 }
 
+sub use_wt_file
+{
+	my ($repo, $workdir, $file, $sha1, $symlinks) = @_;
+	my $null_sha1 = '0' x 40;
+
+	if ($sha1 eq $null_sha1) {
+		return 1;
+	} elsif (not $symlinks) {
+		return 0;
+	}
+
+	my $wt_sha1 = $repo->command_oneline('hash-object', "$workdir/$file");
+	return $sha1 eq $wt_sha1;
+}
+
 sub setup_dir_diff
 {
 	my ($repo, $workdir, $symlinks) = @_;
@@ -159,10 +174,10 @@ EOF
 		}
 
 		if ($rmode ne $null_mode) {
-			if ($rsha1 ne $null_sha1) {
-				$rindex .= "$rmode $rsha1\t$dst_path\0";
-			} else {
+			if (use_wt_file($repo, $workdir, $dst_path, $rsha1, $symlinks)) {
 				push(@working_tree, $dst_path);
+			} else {
+				$rindex .= "$rmode $rsha1\t$dst_path\0";
 			}
 		}
 	}
-- 
1.8.2.rc2.4.g7799588

Re: [PATCH] difftool: Make directory diff symlink work tree

[Matt McClure]

On Tue, Mar 12, 2013 at 3:24 PM, John Keeping <john@keeping.me.uk> wrote:
> When I tried this I got the expected behaviour even without this patch.
>
> It turns out that an uncommitted, but *staged* change emits the SHA1 of
> the blob rather than the null SHA1.  Do you get the desired behaviour if
> you "git reset" before using difftool?

I tried this:

$ git diff --raw HEAD^
:100644 100644 f35de0f... ead9399... M  diff-lib.c

$ git reset HEAD^
Unstaged changes after reset:
M diff-lib.c

$ git diff --raw
:100644 100644 f35de0f... 0000000... M  diff-lib.c

$ git difftool -d

and the last command did indeed create symlinks into my working tree
rather than file copies.

So... it seems that using git-reset is at least a workaround to get
the symlink behavior I want as a user, though the dance I have to do
is a little more awkward than `git difftool -d HEAD^` would be.

> If so I think you want some new mode of operation for difftool instead
> of this patch which will also affect unrelated commands.

Are you suggesting that difftool do the reset work above given a new
option or by default?

--
Matt McClure
http://www.matthewlmcclure.com
http://www.mapmyfitness.com/profile/matthewlmcclure

Re: difftool -d symlinks, under what conditions

[Matt McClure]

On Tue, Mar 12, 2013 at 8:17 PM, John Keeping <john@keeping.me.uk> wrote:
> Does this work for your original scenario?

Yes. Thanks!

-- 
Matt McClure
http://www.matthewlmcclure.com
http://www.mapmyfitness.com/profile/matthewlmcclure

Re: difftool -d symlinks, under what conditions

[David Aguilar]

On Tue, Mar 12, 2013 at 5:17 PM, John Keeping <john@keeping.me.uk> wrote:
> On Tue, Mar 12, 2013 at 04:48:16PM -0600, Matt McClure wrote:
>> On Mar 12, 2013, at 4:16 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>
>> > Matt McClure <matthewlmcclure@gmail.com> writes:
>> >
>> > * If you are comparing two trees, and especially if your RHS is not
>> >   HEAD, you will send everything to a temporary without
>> >   symlinks. Any edit made by the user will be lost.
>>
>> I think you're suggesting to use a symlink any time the content of any
>> given RHS revision is the same as the working tree.
>>
>> I imagine that might confuse me as a user. It would create
>> circumstances where some files are symlinked and others aren't for
>> reasons that won't be straightforward.
>>
>> I imagine solving that case, I might instead implement a copy back to
>> the working tree with conflict detection/resolution. Some earlier
>> iterations of the directory diff feature used copy back without
>> conflict detection and created situations where I clobbered my own
>> changes by finishing a directory diff after making edits concurrently.
>
> The code to copy back working tree files is already there, it just
> triggers using the same logic as the creation of symlinks in the first
> place and doesn't attempt any conflict detection.  I suspect that any
> more comprehensive solution will need to restrict the use of "git
> difftool -d" whenever the index contains unmerged entries or when there
> are both staged and unstaged changes, since the merge resolution will
> cause these states to be lost.
>
> The implementation of Junio's suggestion is relatively straightforward
> (this is untested, although t7800 passes, and can probably be improved
> by someone better versed in Perl).  Does this work for your original
> scenario?

This is a nice straightforward approach.

As Junio mentioned, a good next step would be this patch
in combination with making the truly temporary files
created by dir-diff readonly.

Will that need a win32 platform check?
Does anyone want to take this and whip it into a proper patch?

> -- >8 --
> diff --git a/git-difftool.perl b/git-difftool.perl
> index 0a90de4..5f093ae 100755
> --- a/git-difftool.perl
> +++ b/git-difftool.perl
> @@ -83,6 +83,21 @@ sub exit_cleanup
>         exit($status | ($status >> 8));
>  }
>
> +sub use_wt_file
> +{
> +       my ($repo, $workdir, $file, $sha1, $symlinks) = @_;
> +       my $null_sha1 = '0' x 40;
> +
> +       if ($sha1 eq $null_sha1) {
> +               return 1;
> +       } elsif (not $symlinks) {
> +               return 0;
> +       }
> +
> +       my $wt_sha1 = $repo->command_oneline('hash-object', "$workdir/$file");
> +       return $sha1 eq $wt_sha1;
> +}
> +
>  sub setup_dir_diff
>  {
>         my ($repo, $workdir, $symlinks) = @_;
> @@ -159,10 +174,10 @@ EOF
>                 }
>
>                 if ($rmode ne $null_mode) {
> -                       if ($rsha1 ne $null_sha1) {
> -                               $rindex .= "$rmode $rsha1\t$dst_path\0";
> -                       } else {
> +                       if (use_wt_file($repo, $workdir, $dst_path, $rsha1, $symlinks)) {
>                                 push(@working_tree, $dst_path);
> +                       } else {
> +                               $rindex .= "$rmode $rsha1\t$dst_path\0";
>                         }
>                 }
>         }
> --
> 1.8.2.rc2.4.g7799588
>
-- 
David

Re: [PATCH] difftool: Make directory diff symlink work tree

[John Keeping]

On Tue, Mar 12, 2013 at 08:26:21PM -0400, Matt McClure wrote:
> On Tue, Mar 12, 2013 at 3:24 PM, John Keeping <john@keeping.me.uk> wrote:
> > If so I think you want some new mode of operation for difftool instead
> > of this patch which will also affect unrelated commands.
> 
> Are you suggesting that difftool do the reset work above given a new
> option or by default?

I was suggesting something like the "--symlink-all" option discussed in
the parallel thread, but it looks like we now have a better solution
than that.


John

Re: difftool -d symlinks, under what conditions

[Junio C Hamano]

David Aguilar <davvid@gmail.com> writes:

>> The implementation of Junio's suggestion is relatively straightforward
>> (this is untested, although t7800 passes, and can probably be improved
>> by someone better versed in Perl).  Does this work for your original
>> scenario?
>
> This is a nice straightforward approach.
>
> As Junio mentioned, a good next step would be this patch in
> combination with making the truly temporary files created by
> dir-diff readonly.

Even though I agree that the idea Matt McClure mentioned to run a
three-way merge to take the modification back when the path checked
out to the temporary tree as a temporary file (because it does not
match the working tree version) gets edited by the user might be a
better longer-term direction to go, marking the temporaries that the
users should not modify clearly as such needs to be done in the
shorter term.  This thread wouldn't have had to happen if we had
such a safety measure in the first place.

Re: difftool -d symlinks, under what conditions

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> David Aguilar <davvid@gmail.com> writes:
>
>>> The implementation of Junio's suggestion is relatively straightforward
>>> (this is untested, although t7800 passes, and can probably be improved
>>> by someone better versed in Perl).  Does this work for your original
>>> scenario?
>>
>> This is a nice straightforward approach.
>>
>> As Junio mentioned, a good next step would be this patch in
>> combination with making the truly temporary files created by
>> dir-diff readonly.
>
> Even though I agree that the idea Matt McClure mentioned to run a
> three-way merge to take the modification back when the path checked
> out to the temporary tree as a temporary file (because it does not
> match the working tree version) gets edited by the user might be a
> better longer-term direction to go, marking the temporaries that the
> users should not modify clearly as such needs to be done in the
> shorter term.  This thread wouldn't have had to happen if we had
> such a safety measure in the first place.

One thing I forgot to add.  I suspect the patch in the thread will
not work if the path needs smudge filter and end-of-line conversion,
as it seems to just hash-object what is in the working tree (which
should be _after_ these transformations) and compare with the object
name.  The comparison should go the other way around.  Try to check
out the object with these transformations applied, and compare the
resulting file with what is in the working tree.

Does the temporary checkout correctly apply the smudge filter and
crlf conversion, by the way?  If not, regardless of the topic in
this thread, that may want to be fixed as well.  I didn't check.

Re: difftool -d symlinks, under what conditions

[John Keeping]

On Wed, Mar 13, 2013 at 09:45:47AM -0700, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
> 
> > David Aguilar <davvid@gmail.com> writes:
> >
> >>> The implementation of Junio's suggestion is relatively straightforward
> >>> (this is untested, although t7800 passes, and can probably be improved
> >>> by someone better versed in Perl).  Does this work for your original
> >>> scenario?
> >>
> >> This is a nice straightforward approach.
> >>
> >> As Junio mentioned, a good next step would be this patch in
> >> combination with making the truly temporary files created by
> >> dir-diff readonly.
> >
> > Even though I agree that the idea Matt McClure mentioned to run a
> > three-way merge to take the modification back when the path checked
> > out to the temporary tree as a temporary file (because it does not
> > match the working tree version) gets edited by the user might be a
> > better longer-term direction to go, marking the temporaries that the
> > users should not modify clearly as such needs to be done in the
> > shorter term.  This thread wouldn't have had to happen if we had
> > such a safety measure in the first place.
> 
> One thing I forgot to add.  I suspect the patch in the thread will
> not work if the path needs smudge filter and end-of-line conversion,
> as it seems to just hash-object what is in the working tree (which
> should be _after_ these transformations) and compare with the object
> name.  The comparison should go the other way around.  Try to check
> out the object with these transformations applied, and compare the
> resulting file with what is in the working tree.

git-hash-object(1) implies that it will apply the clean filter and EOL
conversion when it's given a path to a file in the working tree (as it
is here).  Is that not the case?


John

Re: difftool -d symlinks, under what conditions

[Junio C Hamano]

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

> git-hash-object(1) implies that it will apply the clean filter and EOL
> conversion when it's given a path to a file in the working tree (as it
> is here).  Is that not the case?

Applying clean to smudged contents _ought to_ recover clean version,
but is that "ought to" something you would want to rely on?

Re: difftool -d symlinks, under what conditions

[John Keeping]

On Wed, Mar 13, 2013 at 10:40:55AM -0700, Junio C Hamano wrote:
> John Keeping <john@keeping.me.uk> writes:
> 
> > git-hash-object(1) implies that it will apply the clean filter and EOL
> > conversion when it's given a path to a file in the working tree (as it
> > is here).  Is that not the case?
> 
> Applying clean to smudged contents _ought to_ recover clean version,
> but is that "ought to" something you would want to rely on?

How does git-status figure out that file that has been touch'd does not
have unstaged changes without relying on this?  Surely this case is no
different from that?


John

Re: difftool -d symlinks, under what conditions

[Junio C Hamano]

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

> On Wed, Mar 13, 2013 at 10:40:55AM -0700, Junio C Hamano wrote:
>> John Keeping <john@keeping.me.uk> writes:
>> 
>> > git-hash-object(1) implies that it will apply the clean filter and EOL
>> > conversion when it's given a path to a file in the working tree (as it
>> > is here).  Is that not the case?
>> 
>> Applying clean to smudged contents _ought to_ recover clean version,
>> but is that "ought to" something you would want to rely on?
>
> How does git-status figure out that file that has been touch'd does not
> have unstaged changes without relying on this?  Surely this case is no
> different from that?

I just checked.  ce_modified_check_fs() does ce_compare_data() which
does the same "hash the path and compare the resulting hash".  So I
think we are OK.

Thanks.

[PATCH 0/2] difftool --dir-diff: symlink all files matching the working tree

[John Keeping]

Here's the proper patch.  It grew into a series because I noticed a
minor formatting error in the difftool documentation, which the first
commit fixes.

The content of the second patch is the same as was previously posted.

John Keeping (2):
  git-difftool(1): fix formatting of --symlink description
  difftool --dir-diff: symlink all files matching the working tree

 Documentation/git-difftool.txt |  8 +++++---
 git-difftool.perl              | 21 ++++++++++++++++++---
 t/t7800-difftool.sh            | 14 ++++++++++++++
 3 files changed, 37 insertions(+), 6 deletions(-)

-- 
1.8.2.rc2.4.g7799588

[PATCH 1/2] git-difftool(1): fix formatting of --symlink description

[John Keeping]

Signed-off-by: John Keeping <john@keeping.me.uk>
---
 Documentation/git-difftool.txt | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-difftool.txt b/Documentation/git-difftool.txt
index e0e12e9..e575fea 100644
--- a/Documentation/git-difftool.txt
+++ b/Documentation/git-difftool.txt
@@ -74,8 +74,8 @@ with custom merge tool commands and has the same value as `$MERGED`.
 	'git difftool''s default behavior is create symlinks to the
 	working tree when run in `--dir-diff` mode.
 +
-	Specifying `--no-symlinks` instructs 'git difftool' to create
-	copies instead.  `--no-symlinks` is the default on Windows.
+Specifying `--no-symlinks` instructs 'git difftool' to create copies
+instead.  `--no-symlinks` is the default on Windows.
 
 -x <command>::
 --extcmd=<command>::
-- 
1.8.2.rc2.4.g7799588

[PATCH 2/2] difftool --dir-diff: symlink all files matching the working tree

[John Keeping]

Some users like to edit files in their diff tool when using "git
difftool --dir-diff --symlink" to compare against the working tree but
difftool currently only created symlinks when a file contains unstaged
changes.

Change this behaviour so that symlinks are created whenever the
right-hand side of the comparison has the same SHA1 as the file in the
working tree.

Note that textconv filters are handled in the same way as by git-diff
and if a clean filter is not the inverse of its smudge filter we already
get a null SHA1 from "diff --raw" and will symlink the file without
going through the new hash-object based check.

Reported-by: Matt McClure <matthewlmcclure@gmail.com>
Signed-off-by: John Keeping <john@keeping.me.uk>
---
 Documentation/git-difftool.txt |  4 +++-
 git-difftool.perl              | 21 ++++++++++++++++++---
 t/t7800-difftool.sh            | 14 ++++++++++++++
 3 files changed, 35 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-difftool.txt b/Documentation/git-difftool.txt
index e575fea..8361e6e 100644
--- a/Documentation/git-difftool.txt
+++ b/Documentation/git-difftool.txt
@@ -72,7 +72,9 @@ with custom merge tool commands and has the same value as `$MERGED`.
 --symlinks::
 --no-symlinks::
 	'git difftool''s default behavior is create symlinks to the
-	working tree when run in `--dir-diff` mode.
+	working tree when run in `--dir-diff` mode and the right-hand
+	side of the comparison yields the same content as the file in
+	the working tree.
 +
 Specifying `--no-symlinks` instructs 'git difftool' to create copies
 instead.  `--no-symlinks` is the default on Windows.
diff --git a/git-difftool.perl b/git-difftool.perl
index 0a90de4..5f093ae 100755
--- a/git-difftool.perl
+++ b/git-difftool.perl
@@ -83,6 +83,21 @@ sub exit_cleanup
 	exit($status | ($status >> 8));
 }
 
+sub use_wt_file
+{
+	my ($repo, $workdir, $file, $sha1, $symlinks) = @_;
+	my $null_sha1 = '0' x 40;
+
+	if ($sha1 eq $null_sha1) {
+		return 1;
+	} elsif (not $symlinks) {
+		return 0;
+	}
+
+	my $wt_sha1 = $repo->command_oneline('hash-object', "$workdir/$file");
+	return $sha1 eq $wt_sha1;
+}
+
 sub setup_dir_diff
 {
 	my ($repo, $workdir, $symlinks) = @_;
@@ -159,10 +174,10 @@ EOF
 		}
 
 		if ($rmode ne $null_mode) {
-			if ($rsha1 ne $null_sha1) {
-				$rindex .= "$rmode $rsha1\t$dst_path\0";
-			} else {
+			if (use_wt_file($repo, $workdir, $dst_path, $rsha1, $symlinks)) {
 				push(@working_tree, $dst_path);
+			} else {
+				$rindex .= "$rmode $rsha1\t$dst_path\0";
 			}
 		}
 	}
diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
index eb1d3f8..8102ce1 100755
--- a/t/t7800-difftool.sh
+++ b/t/t7800-difftool.sh
@@ -370,6 +370,20 @@ test_expect_success PERL 'difftool --dir-diff' '
 	echo "$diff" | stdin_contains file
 '
 
+write_script .git/CHECK_SYMLINKS <<\EOF &&
+#!/bin/sh
+test -L "$2/file" &&
+test -L "$2/file2" &&
+test -L "$2/sub/sub"
+echo $?
+EOF
+
+test_expect_success PERL,SYMLINKS 'difftool --dir-diff --symlink without unstaged changes' '
+	result=$(git difftool --dir-diff --symlink \
+		--extcmd "./.git/CHECK_SYMLINKS" branch HEAD) &&
+	test "$result" = 0
+'
+
 test_expect_success PERL 'difftool --dir-diff ignores --prompt' '
 	diff=$(git difftool --dir-diff --prompt --extcmd ls branch) &&
 	echo "$diff" | stdin_contains sub &&
-- 
1.8.2.rc2.4.g7799588

Re: [PATCH 2/2] difftool --dir-diff: symlink all files matching the working tree

[David Aguilar]

On Wed, Mar 13, 2013 at 1:33 PM, John Keeping <john@keeping.me.uk> wrote:
> Some users like to edit files in their diff tool when using "git
> difftool --dir-diff --symlink" to compare against the working tree but
> difftool currently only created symlinks when a file contains unstaged
> changes.
>
> Change this behaviour so that symlinks are created whenever the
> right-hand side of the comparison has the same SHA1 as the file in the
> working tree.
>
> Note that textconv filters are handled in the same way as by git-diff
> and if a clean filter is not the inverse of its smudge filter we already
> get a null SHA1 from "diff --raw" and will symlink the file without
> going through the new hash-object based check.
>
> Reported-by: Matt McClure <matthewlmcclure@gmail.com>
> Signed-off-by: John Keeping <john@keeping.me.uk>
> ---
>  Documentation/git-difftool.txt |  4 +++-
>  git-difftool.perl              | 21 ++++++++++++++++++---
>  t/t7800-difftool.sh            | 14 ++++++++++++++
>  3 files changed, 35 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/git-difftool.txt b/Documentation/git-difftool.txt
> index e575fea..8361e6e 100644
> --- a/Documentation/git-difftool.txt
> +++ b/Documentation/git-difftool.txt
> @@ -72,7 +72,9 @@ with custom merge tool commands and has the same value as `$MERGED`.
>  --symlinks::
>  --no-symlinks::
>         'git difftool''s default behavior is create symlinks to the
> -       working tree when run in `--dir-diff` mode.
> +       working tree when run in `--dir-diff` mode and the right-hand
> +       side of the comparison yields the same content as the file in
> +       the working tree.
>  +
>  Specifying `--no-symlinks` instructs 'git difftool' to create copies
>  instead.  `--no-symlinks` is the default on Windows.
> diff --git a/git-difftool.perl b/git-difftool.perl
> index 0a90de4..5f093ae 100755
> --- a/git-difftool.perl
> +++ b/git-difftool.perl
> @@ -83,6 +83,21 @@ sub exit_cleanup
>         exit($status | ($status >> 8));
>  }
>
> +sub use_wt_file
> +{
> +       my ($repo, $workdir, $file, $sha1, $symlinks) = @_;
> +       my $null_sha1 = '0' x 40;
> +
> +       if ($sha1 eq $null_sha1) {
> +               return 1;
> +       } elsif (not $symlinks) {
> +               return 0;
> +       }
> +
> +       my $wt_sha1 = $repo->command_oneline('hash-object', "$workdir/$file");
> +       return $sha1 eq $wt_sha1;
> +}
> +
>  sub setup_dir_diff
>  {
>         my ($repo, $workdir, $symlinks) = @_;
> @@ -159,10 +174,10 @@ EOF
>                 }
>
>                 if ($rmode ne $null_mode) {
> -                       if ($rsha1 ne $null_sha1) {
> -                               $rindex .= "$rmode $rsha1\t$dst_path\0";
> -                       } else {
> +                       if (use_wt_file($repo, $workdir, $dst_path, $rsha1, $symlinks)) {
>                                 push(@working_tree, $dst_path);
> +                       } else {
> +                               $rindex .= "$rmode $rsha1\t$dst_path\0";
>                         }
>                 }
>         }
> diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
> index eb1d3f8..8102ce1 100755
> --- a/t/t7800-difftool.sh
> +++ b/t/t7800-difftool.sh
> @@ -370,6 +370,20 @@ test_expect_success PERL 'difftool --dir-diff' '
>         echo "$diff" | stdin_contains file
>  '
>
> +write_script .git/CHECK_SYMLINKS <<\EOF &&

Tiny nit.  Is there any downside to leaving this file
at the root instead of inside the .git dir?

> +#!/bin/sh
> +test -L "$2/file" &&
> +test -L "$2/file2" &&
> +test -L "$2/sub/sub"
> +echo $?
> +EOF
> +
> +test_expect_success PERL,SYMLINKS 'difftool --dir-diff --symlink without unstaged changes' '
> +       result=$(git difftool --dir-diff --symlink \
> +               --extcmd "./.git/CHECK_SYMLINKS" branch HEAD) &&
> +       test "$result" = 0
> +'
> +

How about something like this?

+       echo 0 >expect &&
+       git difftool --dir-diff --symlink \
+               --extcmd ./CHECK_SYMLINKS branch HEAD >actual &&
+       test_cmp expect actual

(sans gmail whitespace damage) so that we can keep it chained with &&.
Ah.. it seems your branch is based on master, perhaps?

There's stuff cooking in next for difftool's tests.
I'm not sure if this patch is based on top of them.
Can you rebase the tests so that the chaining is done like it is in 'next'?
-- 
David

Re: [PATCH 2/2] difftool --dir-diff: symlink all files matching the working tree

[John Keeping]

On Wed, Mar 13, 2013 at 08:41:29PM -0700, David Aguilar wrote:
> On Wed, Mar 13, 2013 at 1:33 PM, John Keeping <john@keeping.me.uk> wrote:
> > diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
> > index eb1d3f8..8102ce1 100755
> > --- a/t/t7800-difftool.sh
> > +++ b/t/t7800-difftool.sh
> > @@ -370,6 +370,20 @@ test_expect_success PERL 'difftool --dir-diff' '
> >         echo "$diff" | stdin_contains file
> >  '
> >
> > +write_script .git/CHECK_SYMLINKS <<\EOF &&
> 
> Tiny nit.  Is there any downside to leaving this file
> at the root instead of inside the .git dir?

I followed what some of the other uses of write_script (in other tests)
did.  I think putting it under .git is slightly better because it won't
show up as untracked in the repository but that shouldn't matter here,
so I'm happy to change it in a re-roll.

> > +#!/bin/sh
> > +test -L "$2/file" &&
> > +test -L "$2/file2" &&
> > +test -L "$2/sub/sub"
> > +echo $?
> > +EOF
> > +
> > +test_expect_success PERL,SYMLINKS 'difftool --dir-diff --symlink without unstaged changes' '
> > +       result=$(git difftool --dir-diff --symlink \
> > +               --extcmd "./.git/CHECK_SYMLINKS" branch HEAD) &&
> > +       test "$result" = 0
> > +'
> > +
> 
> How about something like this?
> 
> +       echo 0 >expect &&
> +       git difftool --dir-diff --symlink \
> +               --extcmd ./CHECK_SYMLINKS branch HEAD >actual &&
> +       test_cmp expect actual
> 
> (sans gmail whitespace damage) so that we can keep it chained with &&.

I hadn't considered using test_cmp, if we go that way I wonder if we can
do slightly better for future debugging.  Something like this perhaps?

+write_script .git/CHECK_SYMLINKS <<\EOF &&
+for f in file file2 sub/sub
+do
+	echo "$f"
+	readlink "$2/$f"
+done >actual
+EOF
+
+test_expect_success PERL,SYMLINKS 'difftool --dir-diff --symlink without unstaged changes' '
+	cat <<EOF >expect &&
+file
+$(pwd)/file
+file2
+$(pwd)/file2
+sub/sub
+$(pwd)/sub/sub
+EOF
+       git difftool --dir-diff --symlink \
+               --extcmd "./.git/CHECK_SYMLINKS" branch HEAD &&
+	test_cmp actual expect
+'

> Ah.. it seems your branch is based on master, perhaps?
>
> There's stuff cooking in next for difftool's tests.
> I'm not sure if this patch is based on top of them.
> Can you rebase the tests so that the chaining is done like it is in 'next'?

Yes it is based on master.  The cleanup on next looks good, I'll base
the re-roll on that.


John

Re: difftool -d symlinks, under what conditions

[John Keeping]

On Wed, Mar 13, 2013 at 09:45:47AM -0700, Junio C Hamano wrote:
> Does the temporary checkout correctly apply the smudge filter and
> crlf conversion, by the way?  If not, regardless of the topic in
> this thread, that may want to be fixed as well.  I didn't check.

I've had a look at this and I think it will be much quicker for someone
more familiar with git-checkout-index to answer.

What git-difftool does is to create a temporary index containing only
the files that have changed (using git-update-index --index-info) and
then check this out with "git checkout-index --prefix=...".  So I think
this question boils down to: does git-checkout-index still read
.gitattributes from the working tree if given --prefix?


John

Re: [PATCH 2/2] difftool --dir-diff: symlink all files matching the working tree

[Junio C Hamano]

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

> +write_script .git/CHECK_SYMLINKS <<\EOF &&
> +#!/bin/sh
> +test -L "$2/file" &&
> +test -L "$2/file2" &&
> +test -L "$2/sub/sub"
> +echo $?
> +EOF

Please drop "#!/bin/sh" from the above; it is misleading and
pointless.

After all, you are using "write_script" to avoid having to know
where the user's shell is.

Re: difftool -d symlinks, under what conditions

[John Keeping]

On Thu, Mar 14, 2013 at 09:43:00AM +0000, John Keeping wrote:
> On Wed, Mar 13, 2013 at 09:45:47AM -0700, Junio C Hamano wrote:
> > Does the temporary checkout correctly apply the smudge filter and
> > crlf conversion, by the way?  If not, regardless of the topic in
> > this thread, that may want to be fixed as well.  I didn't check.
> 
> What git-difftool does is to create a temporary index containing only
> the files that have changed (using git-update-index --index-info) and
> then check this out with "git checkout-index --prefix=...".  So I think
> this question boils down to: does git-checkout-index still read
> .gitattributes from the working tree if given --prefix?

Having looked at this a bit more, I think it does mostly do the right
thing, but there is bug in write_entry() that means it won't handle
.gitattributes correctly when using a streaming filter.

The path passed to get_stream_filter is only used to decide what filters
apply to the file, so shouldn't it be using "ce->name" and not "path"
for the same reason that the call to convert_to_working_tree() further
down the same function does?

-- >8 --
diff --git a/entry.c b/entry.c
index 17a6bcc..63c52ed 100644
--- a/entry.c
+++ b/entry.c
@@ -145,7 +145,7 @@ static int write_entry(struct cache_entry *ce, char *path, const struct checkout
 	struct stat st;
 
 	if (ce_mode_s_ifmt == S_IFREG) {
-		struct stream_filter *filter = get_stream_filter(path, ce->sha1);
+		struct stream_filter *filter = get_stream_filter(ce->name, ce->sha1);
 		if (filter &&
 		    !streaming_write_entry(ce, path, filter,
 					   state, to_tempfile,

Re: difftool -d symlinks, under what conditions

[Junio C Hamano]

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

> The path passed to get_stream_filter is only used to decide what filters
> apply to the file, so shouldn't it be using "ce->name" and not "path"
> for the same reason that the call to convert_to_working_tree() further
> down the same function does?

Correct and well spotted.

>
> -- >8 --
> diff --git a/entry.c b/entry.c
> index 17a6bcc..63c52ed 100644
> --- a/entry.c
> +++ b/entry.c
> @@ -145,7 +145,7 @@ static int write_entry(struct cache_entry *ce, char *path, const struct checkout
>  	struct stat st;
>  
>  	if (ce_mode_s_ifmt == S_IFREG) {
> -		struct stream_filter *filter = get_stream_filter(path, ce->sha1);
> +		struct stream_filter *filter = get_stream_filter(ce->name, ce->sha1);
>  		if (filter &&
>  		    !streaming_write_entry(ce, path, filter,
>  					   state, to_tempfile,

[PATCH 0/2] checkout-index: fix .gitattributes handling with --prefix

[John Keeping]

This is from the recent "difftool --dir-diff" discussion.  With these
patches applied I think "difftool --dir-diff" should correctly apply
filters to the files that it checks out with no changes to the
git-difftool code.

John Keeping (2):
  t2003: modernize style
  entry: fix streaming filter path

 entry.c                         |   2 +-
 t/t2003-checkout-cache-mkdir.sh | 169 +++++++++++++++++++++++-----------------
 2 files changed, 97 insertions(+), 74 deletions(-)

-- 
1.8.2.rc2.4.g7799588

[PATCH 1/2] t2003: modernize style

[John Keeping]

- Description goes on the test_expect_* line
- Open SQ of test goes on the test_expect_* line
- Closing SQ of test goes on its own line
- Use TAB for indent

Also remove three comments that appear to relate to the development of
the patch before it was committed.

Signed-off-by: John Keeping <john@keeping.me.uk>
---
 t/t2003-checkout-cache-mkdir.sh | 143 ++++++++++++++++++++--------------------
 1 file changed, 70 insertions(+), 73 deletions(-)

diff --git a/t/t2003-checkout-cache-mkdir.sh b/t/t2003-checkout-cache-mkdir.sh
index 02a4fc5..63fd0a8 100755
--- a/t/t2003-checkout-cache-mkdir.sh
+++ b/t/t2003-checkout-cache-mkdir.sh
@@ -12,85 +12,82 @@ the GIT controlled paths.
 
 . ./test-lib.sh
 
-test_expect_success \
-    'setup' \
-    'mkdir path1 &&
-    echo frotz >path0 &&
-    echo rezrov >path1/file1 &&
-    git update-index --add path0 path1/file1'
+test_expect_success 'setup' '
+	mkdir path1 &&
+	echo frotz >path0 &&
+	echo rezrov >path1/file1 &&
+	git update-index --add path0 path1/file1
+'
 
-test_expect_success SYMLINKS \
-    'have symlink in place where dir is expected.' \
-    'rm -fr path0 path1 &&
-     mkdir path2 &&
-     ln -s path2 path1 &&
-     git checkout-index -f -a &&
-     test ! -h path1 && test -d path1 &&
-     test -f path1/file1 && test ! -f path2/file1'
+test_expect_success SYMLINKS 'have symlink in place where dir is expected.' '
+	rm -fr path0 path1 &&
+	mkdir path2 &&
+	ln -s path2 path1 &&
+	git checkout-index -f -a &&
+	test ! -h path1 && test -d path1 &&
+	test -f path1/file1 && test ! -f path2/file1
+'
 
-test_expect_success \
-    'use --prefix=path2/' \
-    'rm -fr path0 path1 path2 &&
-     mkdir path2 &&
-     git checkout-index --prefix=path2/ -f -a &&
-     test -f path2/path0 &&
-     test -f path2/path1/file1 &&
-     test ! -f path0 &&
-     test ! -f path1/file1'
+test_expect_success 'use --prefix=path2/' '
+	rm -fr path0 path1 path2 &&
+	mkdir path2 &&
+	git checkout-index --prefix=path2/ -f -a &&
+	test -f path2/path0 &&
+	test -f path2/path1/file1 &&
+	test ! -f path0 &&
+	test ! -f path1/file1
+'
 
-test_expect_success \
-    'use --prefix=tmp-' \
-    'rm -fr path0 path1 path2 tmp* &&
-     git checkout-index --prefix=tmp- -f -a &&
-     test -f tmp-path0 &&
-     test -f tmp-path1/file1 &&
-     test ! -f path0 &&
-     test ! -f path1/file1'
+test_expect_success 'use --prefix=tmp-' '
+	rm -fr path0 path1 path2 tmp* &&
+	git checkout-index --prefix=tmp- -f -a &&
+	test -f tmp-path0 &&
+	test -f tmp-path1/file1 &&
+	test ! -f path0 &&
+	test ! -f path1/file1
+'
 
-test_expect_success \
-    'use --prefix=tmp- but with a conflicting file and dir' \
-    'rm -fr path0 path1 path2 tmp* &&
-     echo nitfol >tmp-path1 &&
-     mkdir tmp-path0 &&
-     git checkout-index --prefix=tmp- -f -a &&
-     test -f tmp-path0 &&
-     test -f tmp-path1/file1 &&
-     test ! -f path0 &&
-     test ! -f path1/file1'
+test_expect_success 'use --prefix=tmp- but with a conflicting file and dir' '
+	rm -fr path0 path1 path2 tmp* &&
+	echo nitfol >tmp-path1 &&
+	mkdir tmp-path0 &&
+	git checkout-index --prefix=tmp- -f -a &&
+	test -f tmp-path0 &&
+	test -f tmp-path1/file1 &&
+	test ! -f path0 &&
+	test ! -f path1/file1
+'
 
-# Linus fix #1
-test_expect_success SYMLINKS \
-    'use --prefix=tmp/orary/ where tmp is a symlink' \
-    'rm -fr path0 path1 path2 tmp* &&
-     mkdir tmp1 tmp1/orary &&
-     ln -s tmp1 tmp &&
-     git checkout-index --prefix=tmp/orary/ -f -a &&
-     test -d tmp1/orary &&
-     test -f tmp1/orary/path0 &&
-     test -f tmp1/orary/path1/file1 &&
-     test -h tmp'
+test_expect_success SYMLINKS 'use --prefix=tmp/orary/ where tmp is a symlink' '
+	rm -fr path0 path1 path2 tmp* &&
+	mkdir tmp1 tmp1/orary &&
+	ln -s tmp1 tmp &&
+	git checkout-index --prefix=tmp/orary/ -f -a &&
+	test -d tmp1/orary &&
+	test -f tmp1/orary/path0 &&
+	test -f tmp1/orary/path1/file1 &&
+	test -h tmp
+'
 
-# Linus fix #2
-test_expect_success SYMLINKS \
-    'use --prefix=tmp/orary- where tmp is a symlink' \
-    'rm -fr path0 path1 path2 tmp* &&
-     mkdir tmp1 &&
-     ln -s tmp1 tmp &&
-     git checkout-index --prefix=tmp/orary- -f -a &&
-     test -f tmp1/orary-path0 &&
-     test -f tmp1/orary-path1/file1 &&
-     test -h tmp'
+test_expect_success SYMLINKS 'use --prefix=tmp/orary- where tmp is a symlink' '
+	rm -fr path0 path1 path2 tmp* &&
+	mkdir tmp1 &&
+	ln -s tmp1 tmp &&
+	git checkout-index --prefix=tmp/orary- -f -a &&
+	test -f tmp1/orary-path0 &&
+	test -f tmp1/orary-path1/file1 &&
+	test -h tmp
+'
 
-# Linus fix #3
-test_expect_success SYMLINKS \
-    'use --prefix=tmp- where tmp-path1 is a symlink' \
-    'rm -fr path0 path1 path2 tmp* &&
-     mkdir tmp1 &&
-     ln -s tmp1 tmp-path1 &&
-     git checkout-index --prefix=tmp- -f -a &&
-     test -f tmp-path0 &&
-     test ! -h tmp-path1 &&
-     test -d tmp-path1 &&
-     test -f tmp-path1/file1'
+test_expect_success SYMLINKS 'use --prefix=tmp- where tmp-path1 is a symlink' '
+	rm -fr path0 path1 path2 tmp* &&
+	mkdir tmp1 &&
+	ln -s tmp1 tmp-path1 &&
+	git checkout-index --prefix=tmp- -f -a &&
+	test -f tmp-path0 &&
+	test ! -h tmp-path1 &&
+	test -d tmp-path1 &&
+	test -f tmp-path1/file1
+'
 
 test_done
-- 
1.8.2.rc2.4.g7799588

[PATCH 2/2] entry: fix filter lookup

[John Keeping]

When looking up the stream filter, write_entry() should be passing the
path of the file in the repository, not the path to which the content is
going to be written.  This allows the file to be correctly looked up
against the .gitattributes files in the working tree.

This change makes the streaming case match the non-streaming case which
passes ce->name to convert_to_working_tree later in the same function.

The two tests added here test the different paths through write_entry
since the CRLF filter is a streaming filter but the user-defined smudge
filter is not streamed.

Signed-off-by: John Keeping <john@keeping.me.uk>
---
 entry.c                         |  2 +-
 t/t2003-checkout-cache-mkdir.sh | 26 ++++++++++++++++++++++++++
 2 files changed, 27 insertions(+), 1 deletion(-)

diff --git a/entry.c b/entry.c
index 17a6bcc..63c52ed 100644
--- a/entry.c
+++ b/entry.c
@@ -145,7 +145,7 @@ static int write_entry(struct cache_entry *ce, char *path, const struct checkout
 	struct stat st;
 
 	if (ce_mode_s_ifmt == S_IFREG) {
-		struct stream_filter *filter = get_stream_filter(path, ce->sha1);
+		struct stream_filter *filter = get_stream_filter(ce->name, ce->sha1);
 		if (filter &&
 		    !streaming_write_entry(ce, path, filter,
 					   state, to_tempfile,
diff --git a/t/t2003-checkout-cache-mkdir.sh b/t/t2003-checkout-cache-mkdir.sh
index 63fd0a8..4c97468 100755
--- a/t/t2003-checkout-cache-mkdir.sh
+++ b/t/t2003-checkout-cache-mkdir.sh
@@ -90,4 +90,30 @@ test_expect_success SYMLINKS 'use --prefix=tmp- where tmp-path1 is a symlink' '
 	test -f tmp-path1/file1
 '
 
+test_expect_success 'apply filter from working tree .gitattributes with --prefix' '
+	rm -fr path0 path1 path2 tmp* &&
+	mkdir path1 &&
+	mkdir tmp &&
+	git config filter.replace-all.smudge "sed -e s/./=/g" &&
+	git config filter.replace-all.clean cat &&
+	git config filter.replace-all.required true &&
+	echo "file1 filter=replace-all" >path1/.gitattributes &&
+	git checkout-index --prefix=tmp/ -f -a &&
+	echo frotz >expected &&
+	test_cmp expected tmp/path0 &&
+	echo ====== >expected &&
+	test_cmp expected tmp/path1/file1
+'
+
+test_expect_success 'apply CRLF filter from working tree .gitattributes with --prefix' '
+	rm -fr path0 path1 path2 tmp* &&
+	mkdir path1 &&
+	mkdir tmp &&
+	echo "file1 eol=crlf" >path1/.gitattributes &&
+	git checkout-index --prefix=tmp/ -f -a &&
+	echo rezrovQ >expected &&
+	tr \\015 Q <tmp/path1/file1 >actual &&
+	test_cmp expected actual
+'
+
 test_done
-- 
1.8.2.rc2.4.g7799588

[PATCH v2 0/3] difftool --dir-diff: symlink all files matching the working tree

[John Keeping]

Changes since v1:
- A new second patch to make it safer to compare symlink targets in
  tests
- Test in patch 3 (formerly patch 2) re-written thanks to feedback from
  David Aguilar

This series is based on next, although I think Git's clever enough to
ignore the changes in the context of the t7800 hunk so it should apply
to master as well.

John Keeping (3):
  git-difftool(1): fix formatting of --symlink description
  difftool: avoid double slashes in symlink targets
  difftool --dir-diff: symlink all files matching the working tree

 Documentation/git-difftool.txt |  8 +++++---
 git-difftool.perl              | 25 +++++++++++++++++++++----
 t/t7800-difftool.sh            | 22 ++++++++++++++++++++++
 3 files changed, 48 insertions(+), 7 deletions(-)

-- 
1.8.2.396.g36b63d6

[PATCH v2 1/3] git-difftool(1): fix formatting of --symlink description

[John Keeping]

Signed-off-by: John Keeping <john@keeping.me.uk>
---
 Documentation/git-difftool.txt | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-difftool.txt b/Documentation/git-difftool.txt
index e0e12e9..e575fea 100644
--- a/Documentation/git-difftool.txt
+++ b/Documentation/git-difftool.txt
@@ -74,8 +74,8 @@ with custom merge tool commands and has the same value as `$MERGED`.
 	'git difftool''s default behavior is create symlinks to the
 	working tree when run in `--dir-diff` mode.
 +
-	Specifying `--no-symlinks` instructs 'git difftool' to create
-	copies instead.  `--no-symlinks` is the default on Windows.
+Specifying `--no-symlinks` instructs 'git difftool' to create copies
+instead.  `--no-symlinks` is the default on Windows.
 
 -x <command>::
 --extcmd=<command>::
-- 
1.8.2.396.g36b63d6

[PATCH v2 2/3] difftool: avoid double slashes in symlink targets

[John Keeping]

When we add tests for symlinks in "git difftool --dir-diff" it's easier
to check the target path if we don't have to worry about double slashes
separating directories.  Remove the trailing slash (if present) from
$workdir before creating the symlinks in order to avoid this.

Signed-off-by: John Keeping <john@keeping.me.uk>
---
 git-difftool.perl | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/git-difftool.perl b/git-difftool.perl
index 12231fb..e594f9c 100755
--- a/git-difftool.perl
+++ b/git-difftool.perl
@@ -209,7 +209,9 @@ EOF
 	delete($ENV{GIT_INDEX_FILE});
 
 	# Changes in the working tree need special treatment since they are
-	# not part of the index
+	# not part of the index. Remove any trailing slash from $workdir
+	# before starting to avoid double slashes in symlink targets.
+	$workdir =~ s|/$||;
 	for my $file (@working_tree) {
 		my $dir = dirname($file);
 		unless (-d "$rdir/$dir") {
-- 
1.8.2.396.g36b63d6

[PATCH v2 3/3] difftool --dir-diff: symlink all files matching the working tree

[John Keeping]

Some users like to edit files in their diff tool when using "git
difftool --dir-diff --symlink" to compare against the working tree but
difftool currently only created symlinks when a file contains unstaged
changes.

Change this behaviour so that symlinks are created whenever the
right-hand side of the comparison has the same SHA1 as the file in the
working tree.

Note that textconv filters are handled in the same way as by git-diff
and if a clean filter is not the inverse of its smudge filter we already
get a null SHA1 from "diff --raw" and will symlink the file without
going through the new hash-object based check.

Signed-off-by: John Keeping <john@keeping.me.uk>

---
 Documentation/git-difftool.txt |  4 +++-
 git-difftool.perl              | 21 ++++++++++++++++++---
 t/t7800-difftool.sh            | 22 ++++++++++++++++++++++
 3 files changed, 43 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-difftool.txt b/Documentation/git-difftool.txt
index e575fea..8361e6e 100644
--- a/Documentation/git-difftool.txt
+++ b/Documentation/git-difftool.txt
@@ -72,7 +72,9 @@ with custom merge tool commands and has the same value as `$MERGED`.
 --symlinks::
 --no-symlinks::
 	'git difftool''s default behavior is create symlinks to the
-	working tree when run in `--dir-diff` mode.
+	working tree when run in `--dir-diff` mode and the right-hand
+	side of the comparison yields the same content as the file in
+	the working tree.
 +
 Specifying `--no-symlinks` instructs 'git difftool' to create copies
 instead.  `--no-symlinks` is the default on Windows.
diff --git a/git-difftool.perl b/git-difftool.perl
index e594f9c..663640d 100755
--- a/git-difftool.perl
+++ b/git-difftool.perl
@@ -83,6 +83,21 @@ sub exit_cleanup
 	exit($status | ($status >> 8));
 }
 
+sub use_wt_file
+{
+	my ($repo, $workdir, $file, $sha1, $symlinks) = @_;
+	my $null_sha1 = '0' x 40;
+
+	if ($sha1 eq $null_sha1) {
+		return 1;
+	} elsif (not $symlinks) {
+		return 0;
+	}
+
+	my $wt_sha1 = $repo->command_oneline('hash-object', "$workdir/$file");
+	return $sha1 eq $wt_sha1;
+}
+
 sub setup_dir_diff
 {
 	my ($repo, $workdir, $symlinks) = @_;
@@ -159,10 +174,10 @@ EOF
 		}
 
 		if ($rmode ne $null_mode) {
-			if ($rsha1 ne $null_sha1) {
-				$rindex .= "$rmode $rsha1\t$dst_path\0";
-			} else {
+			if (use_wt_file($repo, $workdir, $dst_path, $rsha1, $symlinks)) {
 				push(@working_tree, $dst_path);
+			} else {
+				$rindex .= "$rmode $rsha1\t$dst_path\0";
 			}
 		}
 	}
diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
index 3aab6e1..70e09b6 100755
--- a/t/t7800-difftool.sh
+++ b/t/t7800-difftool.sh
@@ -340,6 +340,28 @@ test_expect_success PERL 'difftool --dir-diff' '
 	stdin_contains file <output
 '
 
+write_script .git/CHECK_SYMLINKS <<\EOF
+for f in file file2 sub/sub
+do
+	echo "$f"
+	readlink "$2/$f"
+done >actual
+EOF
+
+test_expect_success PERL,SYMLINKS 'difftool --dir-diff --symlink without unstaged changes' '
+	cat <<EOF >expect &&
+file
+$(pwd)/file
+file2
+$(pwd)/file2
+sub/sub
+$(pwd)/sub/sub
+EOF
+	git difftool --dir-diff --symlink \
+		--extcmd "./.git/CHECK_SYMLINKS" branch HEAD &&
+	test_cmp actual expect
+'
+
 test_expect_success PERL 'difftool --dir-diff ignores --prompt' '
 	git difftool --dir-diff --prompt --extcmd ls branch >output &&
 	stdin_contains sub <output &&
-- 
1.8.2.396.g36b63d6

Re: [PATCH v2 2/3] difftool: avoid double slashes in symlink targets

[Junio C Hamano]

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

> When we add tests for symlinks in "git difftool --dir-diff" it's easier
> to check the target path if we don't have to worry about double slashes
> separating directories.  Remove the trailing slash (if present) from
> $workdir before creating the symlinks in order to avoid this.

Yup, and it is a good basic hygiene even without tests that expect
the exact pathnames.

The code would work even when your $workdir is at the root of the
filesystem; the patch looks good.

Thanks.

> Signed-off-by: John Keeping <john@keeping.me.uk>
> ---
>  git-difftool.perl | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/git-difftool.perl b/git-difftool.perl
> index 12231fb..e594f9c 100755
> --- a/git-difftool.perl
> +++ b/git-difftool.perl
> @@ -209,7 +209,9 @@ EOF
>  	delete($ENV{GIT_INDEX_FILE});
>  
>  	# Changes in the working tree need special treatment since they are
> -	# not part of the index
> +	# not part of the index. Remove any trailing slash from $workdir
> +	# before starting to avoid double slashes in symlink targets.
> +	$workdir =~ s|/$||;
>  	for my $file (@working_tree) {
>  		my $dir = dirname($file);
>  		unless (-d "$rdir/$dir") {

Re: [PATCH v2 3/3] difftool --dir-diff: symlink all files matching the working tree

[Junio C Hamano]

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

> diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
> index 3aab6e1..70e09b6 100755
> --- a/t/t7800-difftool.sh
> +++ b/t/t7800-difftool.sh
> @@ -340,6 +340,28 @@ test_expect_success PERL 'difftool --dir-diff' '
>  	stdin_contains file <output
>  '
>  
> +write_script .git/CHECK_SYMLINKS <<\EOF
> +for f in file file2 sub/sub
> +do
> +	echo "$f"
> +	readlink "$2/$f"
> +done >actual
> +EOF

When you later want to enhance the test to check a combination of
difftool arguments where some paths are expected to become links and
others are expected to become real files, wouldn't this helper
become a bit awkward to use?  The element that expects a real file
could be an empty line to what corresponds to the output from
readlink, but still...

If t/ directory (or when the test is run with --root=<there>) is
aliased with symlinks in such a way that "cd <there> && $(pwd)" does
not match <there>, would this check with $(pwd) still work, I have
to wonder?

> +test_expect_success PERL,SYMLINKS 'difftool --dir-diff --symlink without unstaged changes' '
> +	cat <<EOF >expect &&
> +file
> +$(pwd)/file
> +file2
> +$(pwd)/file2
> +sub/sub
> +$(pwd)/sub/sub
> +EOF

You can do this to align them nicer (note the "-" before EOF):

	cat >expect <<-EOF &&
	file
        $(pwd)/file
        ...
        EOF

> +	git difftool --dir-diff --symlink \
> +		--extcmd "./.git/CHECK_SYMLINKS" branch HEAD &&
> +	test_cmp actual expect
> +'
> +

Thanks.

Re: [PATCH 2/2] entry: fix filter lookup

[Junio C Hamano]

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

> diff --git a/t/t2003-checkout-cache-mkdir.sh b/t/t2003-checkout-cache-mkdir.sh
> index 63fd0a8..4c97468 100755
> --- a/t/t2003-checkout-cache-mkdir.sh
> +++ b/t/t2003-checkout-cache-mkdir.sh
> @@ -90,4 +90,30 @@ test_expect_success SYMLINKS 'use --prefix=tmp- where tmp-path1 is a symlink' '
>  	test -f tmp-path1/file1
>  '
>  
> +test_expect_success 'apply filter from working tree .gitattributes with --prefix' '
> +	rm -fr path0 path1 path2 tmp* &&
> +	mkdir path1 &&
> +	mkdir tmp &&
> +	git config filter.replace-all.smudge "sed -e s/./=/g" &&
> +	git config filter.replace-all.clean cat &&
> +	git config filter.replace-all.required true &&
> +	echo "file1 filter=replace-all" >path1/.gitattributes &&
> +	git checkout-index --prefix=tmp/ -f -a &&
> +	echo frotz >expected &&
> +	test_cmp expected tmp/path0 &&
> +	echo ====== >expected &&
> +	test_cmp expected tmp/path1/file1
> +'
> +
> +test_expect_success 'apply CRLF filter from working tree .gitattributes with --prefix' '
> +	rm -fr path0 path1 path2 tmp* &&
> +	mkdir path1 &&
> +	mkdir tmp &&
> +	echo "file1 eol=crlf" >path1/.gitattributes &&
> +	git checkout-index --prefix=tmp/ -f -a &&
> +	echo rezrovQ >expected &&
> +	tr \\015 Q <tmp/path1/file1 >actual &&
> +	test_cmp expected actual
> +'

Nicely done.  Thanks.

Re: [PATCH v2 3/3] difftool --dir-diff: symlink all files matching the working tree

[John Keeping]

On Thu, Mar 14, 2013 at 02:28:31PM -0700, Junio C Hamano wrote:
> John Keeping <john@keeping.me.uk> writes:
> 
> > diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
> > index 3aab6e1..70e09b6 100755
> > --- a/t/t7800-difftool.sh
> > +++ b/t/t7800-difftool.sh
> > @@ -340,6 +340,28 @@ test_expect_success PERL 'difftool --dir-diff' '
> >  	stdin_contains file <output
> >  '
> >  
> > +write_script .git/CHECK_SYMLINKS <<\EOF
> > +for f in file file2 sub/sub
> > +do
> > +	echo "$f"
> > +	readlink "$2/$f"
> > +done >actual
> > +EOF
> 
> When you later want to enhance the test to check a combination of
> difftool arguments where some paths are expected to become links and
> others are expected to become real files, wouldn't this helper
> become a bit awkward to use?  The element that expects a real file
> could be an empty line to what corresponds to the output from
> readlink, but still...
> 
> If t/ directory (or when the test is run with --root=<there>) is
> aliased with symlinks in such a way that "cd <there> && $(pwd)" does
> not match <there>, would this check with $(pwd) still work, I have
> to wonder?

It looks like t3903 uses "ls -l" for this sort of test, perhaps
something like this covers these cases better:

    write_script .git/CHECK_SYMLINKS <<\EOF
    for f in file file2 sub/sub
    do
        ls -l "$2/$f" >"$f".actual
    done
    EOF

    ...

    workdir=$(git rev-parse --show-toplevel)
    grep "-> $workdir/file" file.actual
    grep "-> $workdir/file2" file2.actual
    grep "-> $workdir/sub/sub" sub/sub.actual

It looks like we already rely on that output format in t3903 so I think
that is safe, but it would be nice to have a better way to say "does
this link point to that file?".  I can't think of a way to do that that
doesn't seem far too complicated for what's required here.

> > +test_expect_success PERL,SYMLINKS 'difftool --dir-diff --symlink without unstaged changes' '
> > +	cat <<EOF >expect &&
> > +file
> > +$(pwd)/file
> > +file2
> > +$(pwd)/file2
> > +sub/sub
> > +$(pwd)/sub/sub
> > +EOF
> 
> You can do this to align them nicer (note the "-" before EOF):
> 
> 	cat >expect <<-EOF &&
> 	file
>         $(pwd)/file
>         ...
>         EOF
> 
> > +	git difftool --dir-diff --symlink \
> > +		--extcmd "./.git/CHECK_SYMLINKS" branch HEAD &&
> > +	test_cmp actual expect
> > +'
> > +
>
> Thanks.

Re: [PATCH v2 3/3] difftool --dir-diff: symlink all files matching the working tree

[Junio C Hamano]

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

>> > +for f in file file2 sub/sub
>> > +do
>> > +	echo "$f"
>> > +	readlink "$2/$f"
>> > +done >actual
>> > +EOF
>> 
>> When you later want to enhance the test to check a combination of
>> difftool arguments where some paths are expected to become links and
>> others are expected to become real files, wouldn't this helper
>> become a bit awkward to use?  The element that expects a real file
>> could be an empty line to what corresponds to the output from
>> readlink, but still...
>> ...
>
> It looks like t3903 uses "ls -l" for this sort of test, perhaps
> something like this covers these cases better:
> ...
>     grep "-> $workdir/file" file.actual

Writing it without -e would confuse some implementations of grep
into thinking "-" introduces an option, realizing it does not
support the "->" option, and then barfing ;-)

What I had in mind was more along the lines of...

	for f
        do
        	echo "$f"
                readlink "$2/$f" || echo "# not a link $f"
	done

so that your "expect" list can become

	file
        $(pwd)/realdir/file
        modifiedone.txt
        # not a link modifiedone.txt

In any case, this "say blank if you expect a non symlink" is not an
urgent issue that needs to be fixed or anything like that, so let's
queue the v2 for now and see what happens.

Thanks.