Re: difftool -d symlinks, under what conditions
On Thu, Mar 14, 2013 at 09:43:00AM +, 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,
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: difftool -d symlinks, under what conditions
John Keeping j...@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,
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: difftool -d symlinks, under what conditions
On Tue, Mar 12, 2013 at 5:17 PM, John Keeping j...@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 gits...@pobox.com wrote:
Matt McClure matthewlmccl...@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
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: difftool -d symlinks, under what conditions
David Aguilar dav...@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. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: difftool -d symlinks, under what conditions
Junio C Hamano gits...@pobox.com writes: David Aguilar dav...@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. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: difftool -d symlinks, under what conditions
On Wed, Mar 13, 2013 at 09:45:47AM -0700, Junio C Hamano wrote: Junio C Hamano gits...@pobox.com writes: David Aguilar dav...@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 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: difftool -d symlinks, under what conditions
John Keeping j...@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? -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: difftool -d symlinks, under what conditions
On Wed, Mar 13, 2013 at 10:40:55AM -0700, Junio C Hamano wrote: John Keeping j...@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 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: difftool -d symlinks, under what conditions
John Keeping j...@keeping.me.uk writes: On Wed, Mar 13, 2013 at 10:40:55AM -0700, Junio C Hamano wrote: John Keeping j...@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. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: difftool -d symlinks, under what conditions
On Tue, Nov 27, 2012 at 7:41 AM, Matt McClure matthewlmccl...@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 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: difftool -d symlinks, under what conditions
On Tue, Mar 12, 2013 at 12:09 PM, John Keeping j...@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 matthewlmccl...@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.mccl...@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... 000... 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
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: difftool -d symlinks, under what conditions
David Aguilar dav...@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.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: difftool -d symlinks, under what conditions
John Keeping j...@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? -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: difftool -d symlinks, under what conditions
On Tue, Mar 12, 2013 at 01:39:17PM -0700, Junio C Hamano wrote: John Keeping j...@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 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: difftool -d symlinks, under what conditions
John Keeping j...@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?
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: difftool -d symlinks, under what conditions
On Tue, Mar 12, 2013 at 5:06 PM, John Keeping j...@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 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: difftool -d symlinks, under what conditions
On Tue, Mar 12, 2013 at 5:43 PM, Matt McClure matthewlmccl...@gmail.com wrote:
On Tue, Mar 12, 2013 at 5:06 PM, John Keeping j...@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
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: difftool -d symlinks, under what conditions
Matt McClure matthewlmccl...@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? -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: difftool -d symlinks, under what conditions
On Mar 12, 2013, at 4:16 PM, Junio C Hamano gits...@pobox.com wrote: Matt McClure matthewlmccl...@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. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: difftool -d symlinks, under what conditions
On Tue, Mar 12, 2013 at 04:48:16PM -0600, Matt McClure wrote:
On Mar 12, 2013, at 4:16 PM, Junio C Hamano gits...@pobox.com wrote:
Matt McClure matthewlmccl...@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
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: difftool -d symlinks, under what conditions
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 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: difftool -d symlinks, under what conditions
On Mon, Nov 26, 2012 at 12:23 PM, Matt McClure
matthewlmccl...@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
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
