Re: [PATCH v2] diff-lib.c: adjust position of i-t-a entries in diff
Duy Nguyen pclo...@gmail.com writes: read-tree -m does not invoke diff, does it? If I went with my previous approach (modifying unpack-trees to ignore i-t-a entries) then this could be a problem, but because unpack-trees is untouched, merge operations should not be impacted by this patch. Theoretically yes, but not quite. I wouldn't be surprised if an enterprising soul saw an optimization opportunity in the read-tree -m A B codepath. When it finds that a tree in A and a valid cache-tree entry that corresponds to the tree matches, it could blow away all index entries covered by the cache-tree entry and replace them with B, either (1) unconditionally when -u is not given; or (2) as long as the working tree matches the index inside that directory when running with -u. And such an optimization used to be a valid thing to do in the old world; but (1) will break in the new world, if we drop that invalidation---the i-t-a entries will be discarded from the index. As i-t-a is not a norm but an abberration, I'd rather keep the pessimizing invalidation to keep the door open for such an optimization for a more common case, and there may be other cases in which our correctness around i-t-a depends 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: [PATCH v2] diff-lib.c: adjust position of i-t-a entries in diff
Junio C Hamano gits...@pobox.com writes:
Ah, wait.
I suspect that it all cancels out.
...
OK, so I think I can see how the result could work without
invalidating the cache-tree entry that contains i-t-a entries.
It might be even the right thing to do in general. We can view that
invalidation a workaround to achieve the old behaviour of diff-cache,
which used to report that dir/ are different between HEAD and index.
So, we somehow left this hanging for a week without progress. Here
is the version I am going to queue.
Thanks.
-- 8 --
From: Nguyễn Thái Ngọc Duy pclo...@gmail.com
Date: Mon, 16 Mar 2015 20:56:46 +0700
Subject: [PATCH] diff-lib.c: adjust position of i-t-a entries in diff
Entries added by git add -N are reminder for the user so that they
don't forget to add them before committing. These entries appear in
the index even though they are not real. Their presence in the index
leads to a confusing git status like this:
On branch master
Changes to be committed:
new file: foo
Changes not staged for commit:
modified: foo
If you do a git commit, foo will not be included even though
status reports it as to be committed. This patch changes the
output to become
On branch master
Changes not staged for commit:
new file: foo
no changes added to commit
The two hunks in diff-lib.c adjust diff-index and diff-files so
that i-t-a entries appear as new files in diff-files and nothing in
diff-index.
Due to this change, diff-files may start to report new files for the
first time. add -u needs to be told about this or it will die in
denial, screaming new files can't exist! Reality is wrong. Luckily,
it's the only one among run_diff_files() callers that needs fixing.
Now in the new world order, a hierarchy in the index that contain
i-t-a paths is written out as a tree object as if these i-t-a
entries do not exist, and comparing the index with such a tree
object that would result from writing out the hierarchy will result
in no difference. Update a test in t2203 that expected the i-t-a
entries to appear as added to the index in the comparison to
instead expect no output.
An earlier change eec3e7e4 (cache-tree: invalidate i-t-a paths after
generating trees, 2012-12-16) becomes an unnecessary pessimization
in the new world order---a cache-tree in the index that corresponds
to a hierarchy with i-t-a paths can now be marked as valid and
record the object name of the tree that results from writing a tree
object out of that hierarchy, as it will compare equal to that tree.
Reverting the commit is left for the future, though, as it is purely
a performance issue and no longer affects correctness.
Helped-by: Michael J Gruber g...@drmicha.warpmail.net
Helped-by: Junio C Hamano gits...@pobox.com
Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com
Signed-off-by: Junio C Hamano gits...@pobox.com
---
builtin/add.c | 1 +
diff-lib.c | 12
t/t2203-add-intent.sh | 23 +++
t/t4011-diff-symlink.sh | 10 ++
4 files changed, 38 insertions(+), 8 deletions(-)
diff --git a/builtin/add.c b/builtin/add.c
index 3390933..ee370b0 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -63,6 +63,7 @@ static void update_callback(struct diff_queue_struct *q,
switch (fix_unmerged_status(p, data)) {
default:
die(_(unexpected diff status %c), p-status);
+ case DIFF_STATUS_ADDED:
case DIFF_STATUS_MODIFIED:
case DIFF_STATUS_TYPE_CHANGED:
if (add_file_to_index(the_index, path, data-flags)) {
diff --git a/diff-lib.c b/diff-lib.c
index a85c497..714501a 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -212,6 +212,11 @@ int run_diff_files(struct rev_info *revs, unsigned int
option)
ce-sha1,
!is_null_sha1(ce-sha1),
ce-name, 0);
continue;
+ } else if (ce-ce_flags CE_INTENT_TO_ADD) {
+ diff_addremove(revs-diffopt, '+', ce-ce_mode,
+ EMPTY_BLOB_SHA1_BIN, 0,
+ ce-name, 0);
+ continue;
}
changed = match_stat_with_submodule(revs-diffopt, ce,
st,
@@ -376,6 +381,13 @@ static void do_oneway_diff(struct unpack_trees_options *o,
struct rev_info *revs = o-unpack_data;
int match_missing, cached;
+ /* i-t-a entries do not actually exist in the index */
+ if (idx (idx-ce_flags CE_INTENT_TO_ADD)) {
+ idx = NULL;
+ if (!tree)
+ return; /* nothing to diff.. */
+ }
+
/* if the entry is not checked out, don't examine work tree */
cached = o-index_only ||
Re: [PATCH v2] diff-lib.c: adjust position of i-t-a entries in diff
On Thu, Mar 19, 2015 at 1:00 PM, Junio C Hamano gits...@pobox.com wrote: Junio C Hamano gits...@pobox.com writes: Ah, wait. I suspect that it all cancels out. ... Now, as you mentioned, there _may_ be codepaths that uses the same definition of what's in the index as what diff-cache used to take before your patches, and they may be broken by removing the invalidation. If we find such codepaths, we should treat their old behaviour as buggy and fix them, instead of reintroducing the invalidation and keep their current behaviour, as the new world order is i-t-a entries in the index does not yet exist. One potential negative consequence of the new world order I can immediately think of is this. In many operations, we try to be lenient to changes in the working tree as long as the index is clean. read-tree -m is the most visible one, where we require that the index and HEAD matches while allowing changes to working tree paths as long as they do not interfere with the paths that are involved in the merge. We need to make sure that the path dir/bar added by add -N dir/bar, which in the new world order does not count as index is not clean and there is a difference from HEAD, (1) does not interfere with the mergy operation that wants to touch dir (which _could_ even be expected to be a file) or dir/bar, and (2) is not lost because the mergy operation wants to touch dir or dir/bar, for example. read-tree -m does not invoke diff, does it? If I went with my previous approach (modifying unpack-trees to ignore i-t-a entries) then this could be a problem, but because unpack-trees is untouched, merge operations should not be impacted by this patch. Even if some other command does diff --cached first to abort early, if diff --cached fails to report HEAD and index are different as you described, I would expect unpack-trees to be able to deal with it anyway. PS. Sorry for the late response, busy fighting the evil last weekend. I blame Steam on Linux. -- Duy -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] diff-lib.c: adjust position of i-t-a entries in diff
Junio C Hamano gits...@pobox.com writes: Ah, wait. I suspect that it all cancels out. ... Now, as you mentioned, there _may_ be codepaths that uses the same definition of what's in the index as what diff-cache used to take before your patches, and they may be broken by removing the invalidation. If we find such codepaths, we should treat their old behaviour as buggy and fix them, instead of reintroducing the invalidation and keep their current behaviour, as the new world order is i-t-a entries in the index does not yet exist. One potential negative consequence of the new world order I can immediately think of is this. In many operations, we try to be lenient to changes in the working tree as long as the index is clean. read-tree -m is the most visible one, where we require that the index and HEAD matches while allowing changes to working tree paths as long as they do not interfere with the paths that are involved in the merge. We need to make sure that the path dir/bar added by add -N dir/bar, which in the new world order does not count as index is not clean and there is a difference from HEAD, (1) does not interfere with the mergy operation that wants to touch dir (which _could_ even be expected to be a file) or dir/bar, and (2) is not lost because the mergy operation wants to touch dir or dir/bar, for example. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] diff-lib.c: adjust position of i-t-a entries in diff
On Wed, Mar 18, 2015 at 12:57 AM, Junio C Hamano gits...@pobox.com wrote: Duy Nguyen pclo...@gmail.com writes: On Mon, Mar 16, 2015 at 09:05:45AM -0700, Junio C Hamano wrote: The offending one came from eec3e7e4 (cache-tree: invalidate i-t-a paths after generating trees, 2012-12-16), which was a fix to an earlier bug where a cache-tree written out of an index with i-t-a entries had incorrect information and still claimed it is fully valid after write-tree rebuilt it. The test probably should add another path without i-t-a bit, run the same diff --cached with updated expectation before write-tre, and run the diff --cached again to make sure it produces a result that match the updated expectation. Would adding another non-i-t-a entry help? Before this patch diff --cached after write-tree shows the i-t-a entry only when eec3e7e4 is applied. But with this patch we don't show i-t-a entry any more, before or after write-tree, eec3e7e4 makes no visible difference. We could even revert eec3e7e4 and the outcome of diff --cached would be the same because we just sort of move the invalidation part from cache-tree to do_oneway_diff(). Not invalidating would speed up diff --cached when i-t-a entries are present. Still it may be a good idea to invalidate i-t-a paths to be on the safe side. Perhaps a patch like this to resurrect the test? My unerstanding of what eec3e7e4 (cache-tree: invalidate i-t-a paths after generating trees, 2012-12-16) fixed was that in this sequence: ... So reverting the fix obviously is not the right thing to do. If the tests show different results from two invocations of diff --cached with your patch applied, there is something that is broken by your patch, because the index and the HEAD does not change across write-tree in that test. Right. I missed this but I think this is a less important test because I added a new test to make sure diff --cached (git status to be exact) outputs the right thing when i-t-a entries are present. If on the other hand the tests show the same result from these two diff --cached and the result is different from what the test expects, that means your patch changed the world order, i.e. an i-t-a entry used to be treated as if it were adding an empty blob to the index but it is now treated as non-existent, then that is a good thing and the only thing we need to update is what the test expects. I am guessing that instead of expecting dir/bar to be shown, it now should expect no output? Yes, no output. Does adding an non-i-t-a entry help? It does not hurt, and it makes the test uses a non-empty output, making its effect more visible, which may or may not count as helping. But still, if I revert the change in cache-tree.c and force write-tree to produce valid cache-tree when i-t-a entries are present, this test still passes incorrectly. This is why I changed the test. -- Duy -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] diff-lib.c: adjust position of i-t-a entries in diff
Duy Nguyen pclo...@gmail.com writes: Right. I missed this but I think this is a less important test because I added a new test to make sure diff --cached (git status to be exact) outputs the right thing when i-t-a entries are present. OK. If on the other hand the tests show the same result from these two diff --cached and the result is different from what the test expects, that means your patch changed the world order, i.e. an i-t-a entry used to be treated as if it were adding an empty blob to the index but it is now treated as non-existent, then that is a good thing and the only thing we need to update is what the test expects. I am guessing that instead of expecting dir/bar to be shown, it now should expect no output? Yes, no output. Good, as that means your changes did not break things, right? But still, if I revert the change in cache-tree.c and force write-tree to produce valid cache-tree when i-t-a entries are present, this test still passes incorrectly. This is worrysome. Doesn't that suggest that the diff-cache optimization is disabled and cache-tree that is marked as valid (because you revert the fix) is not looked at? That is the only explanation that makes sense to me---you write out a tree omitting i-t-a entries in the index and update the index without your earlier fix eec3e7e4 (cache-tree: invalidate i-t-a paths after generating trees, 2012-12-16), i.e. marking the cache-tree entries valid. A later invocation of diff-cache *should* trust that validity and just say the tree and the index match without even digging into the individual entries, at which point you should see a bogus result. If that is the case, it would be a performance regression, which may be already there before your patches or something your patches may have introduced. Ah, wait. I suspect that it all cancels out. * You add -N dir/bar. You write-tree. The tree is written as if dir/bar is not there. cache-tree is updated and it is marked as valid (because you deliberately broke the earlier fix you made at eec3e7e4). * You run diff-cache. It walks the HEAD and the cache-tree and finds that HEAD:dir and the cache-tree entry for dir records the same tree object name, and the cache-tree entry is marked valid. Hence you declare no differences. But for the updated definition of diff-cache, there indeed is no difference. The HEAD and the index matches, because dir/bar does not yet exist. OK, so I think I can see how the result could work without invalidating the cache-tree entry that contains i-t-a entries. It might be even the right thing to do in general. We can view that invalidation a workaround to achieve the old behaviour of diff-cache, which used to report that dir/ are different between HEAD and index. We can even argue that it is the right thing to mark the cache-tree entry for dir/ as valid, no matter how many i-t-a entries are in there, as long as writing out the tree for dir/ out of index results in the same tree as the tree that is stored as HEAD:dir/. And from that viewpoint, keeping the earlier fix (invalidation code) when these patches are applied _is_ introducing a performance bug. Now, as you mentioned, there _may_ be codepaths that uses the same definition of what's in the index as what diff-cache used to take before your patches, and they may be broken by removing the invalidation. If we find such codepaths, we should treat their old behaviour as buggy and fix them, instead of reintroducing the invalidation and keep their current behaviour, as the new world order is i-t-a entries in the index does not yet exist. Thanks for straightening my confusion out. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] diff-lib.c: adjust position of i-t-a entries in diff
Duy Nguyen pclo...@gmail.com writes: On Mon, Mar 16, 2015 at 09:05:45AM -0700, Junio C Hamano wrote: The offending one came from eec3e7e4 (cache-tree: invalidate i-t-a paths after generating trees, 2012-12-16), which was a fix to an earlier bug where a cache-tree written out of an index with i-t-a entries had incorrect information and still claimed it is fully valid after write-tree rebuilt it. The test probably should add another path without i-t-a bit, run the same diff --cached with updated expectation before write-tre, and run the diff --cached again to make sure it produces a result that match the updated expectation. Would adding another non-i-t-a entry help? Before this patch diff --cached after write-tree shows the i-t-a entry only when eec3e7e4 is applied. But with this patch we don't show i-t-a entry any more, before or after write-tree, eec3e7e4 makes no visible difference. We could even revert eec3e7e4 and the outcome of diff --cached would be the same because we just sort of move the invalidation part from cache-tree to do_oneway_diff(). Not invalidating would speed up diff --cached when i-t-a entries are present. Still it may be a good idea to invalidate i-t-a paths to be on the safe side. Perhaps a patch like this to resurrect the test? My unerstanding of what eec3e7e4 (cache-tree: invalidate i-t-a paths after generating trees, 2012-12-16) fixed was that in this sequence: - You prepare an index. - You write-tree out of the index, which involves: - updating the cache-tree to match the shape of the resulting from writing the index out. - create tree objects matching all levels of the cache-tree as needed on disk. - report the top-level tree object name - run diff-index --cached, which can and will take advantage of the fact that everything in a subtree below a known-to-be-valid cache-tree entry does not have to be checked one-by-one. If a cache-tree says everything under D/ in the index would hash to tree object T and the HEAD has tree object T at D/, then the diff machinery will bypass the entire section in the index under D/, which is a valid optimization. However, when there is an i-t-a entry, we excluded that entry from the tree object computation, its presence did not contribute to the tree object name, but still marked the cache-tree entries that contain it as valid by mistake. This old bug was what the commit fixed, so an invocation of diff --cached after a write-tree, even if the index contains an i-t-a entry, will not see cache-tree entries that are marked valid when they are not. Instead, diff --cached will bypass the optimization and makes comparison one-by-one for the index entries. So reverting the fix obviously is not the right thing to do. If the tests show different results from two invocations of diff --cached with your patch applied, there is something that is broken by your patch, because the index and the HEAD does not change across write-tree in that test. If on the other hand the tests show the same result from these two diff --cached and the result is different from what the test expects, that means your patch changed the world order, i.e. an i-t-a entry used to be treated as if it were adding an empty blob to the index but it is now treated as non-existent, then that is a good thing and the only thing we need to update is what the test expects. I am guessing that instead of expecting dir/bar to be shown, it now should expect no output? Does adding an non-i-t-a entry help? It does not hurt, and it makes the test uses a non-empty output, making its effect more visible, which may or may not count as helping. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] diff-lib.c: adjust position of i-t-a entries in diff
On Mon, Mar 16, 2015 at 09:05:45AM -0700, Junio C Hamano wrote: The offending one came from eec3e7e4 (cache-tree: invalidate i-t-a paths after generating trees, 2012-12-16), which was a fix to an earlier bug where a cache-tree written out of an index with i-t-a entries had incorrect information and still claimed it is fully valid after write-tree rebuilt it. The test probably should add another path without i-t-a bit, run the same diff --cached with updated expectation before write-tre, and run the diff --cached again to make sure it produces a result that match the updated expectation. Would adding another non-i-t-a entry help? Before this patch diff --cached after write-tree shows the i-t-a entry only when eec3e7e4 is applied. But with this patch we don't show i-t-a entry any more, before or after write-tree, eec3e7e4 makes no visible difference. We could even revert eec3e7e4 and the outcome of diff --cached would be the same because we just sort of move the invalidation part from cache-tree to do_oneway_diff(). Not invalidating would speed up diff --cached when i-t-a entries are present. Still it may be a good idea to invalidate i-t-a paths to be on the safe side. Perhaps a patch like this to resurrect the test? -- 8 -- Subject: t2203: enable 'cache-tree invalidates i-t-a paths' test This test is disabled in the previous patch because the diff --cached expectation is the same, with or without eec3e7e4 [1] where this test is added. But it still is a good idea to invalidate i-t-a paths because the index does _not_ match the cached-tree exactly. diff --cached may not care, but other operations might. Update the test to catch this invalidation instead. [1] cache-tree: invalidate i-t-a paths after generating trees, 2012-12-16 Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com diff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh index 42827b8..1a6c814 100755 --- a/t/t2203-add-intent.sh +++ b/t/t2203-add-intent.sh @@ -77,7 +77,7 @@ test_expect_success 'can commit -a with an i-t-a entry' ' git commit -a -m all ' -test_expect_failure 'cache-tree invalidates i-t-a paths' ' +test_expect_success 'cache-tree invalidates i-t-a paths' ' git reset --hard mkdir dir : dir/foo @@ -86,14 +86,14 @@ test_expect_failure 'cache-tree invalidates i-t-a paths' ' : dir/bar git add -N dir/bar - git diff --cached --name-only actual - echo dir/bar expect - test_cmp expect actual - git write-tree /dev/null - - git diff --cached --name-only actual - echo dir/bar expect + test-dump-cache-tree actual + cat expect -\EOF + invalid (1 subtrees) + invalid #(ref) (1 subtrees) + invalid dir/ (0 subtrees) + invalid #(ref) dir/ (0 subtrees) + EOF test_cmp expect actual ' -- 8 -- -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2] diff-lib.c: adjust position of i-t-a entries in diff
Entries added by git add -N are reminder for the user so that they
don't forget to add them before committing. These entries appear in
the index even though they are not real. Their presence in the index
leads to a confusing git status like this:
On branch master
Changes to be committed:
new file: foo
Changes not staged for commit:
modified: foo
If you do a git commit, foo will not be included even though
status reports it as to be committed. This patch changes the
output to become
On branch master
Changes not staged for commit:
new file: foo
no changes added to commit
The two hunks in diff-lib.c adjust diff-index and diff-files so
that i-t-a entries appear as new files in diff-files and nothing in
diff-index.
Due to this change, diff-files may start to report new files for the
first time. add -u needs to be told about this or it will die in
denial, screaming new files can't exist! Reality is wrong. Luckily,
it's the only one among run_diff_files() callers that needs fixing.
The test cache-tree invalidates i-t-a paths is marked failure
because I don't think removing --cached from git diff is the right
fix. This test relies on diff --cached behavior but the behavior now
has changed. We may need to revisit this test at some point in future.
Helped-by: Michael J Gruber g...@drmicha.warpmail.net
Helped-by: Junio C Hamano gits...@pobox.com
Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com
---
builtin/add.c | 1 +
diff-lib.c | 12
t/t2203-add-intent.sh | 21 ++---
t/t4011-diff-symlink.sh | 10 ++
4 files changed, 37 insertions(+), 7 deletions(-)
diff --git a/builtin/add.c b/builtin/add.c
index 3390933..ee370b0 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -63,6 +63,7 @@ static void update_callback(struct diff_queue_struct *q,
switch (fix_unmerged_status(p, data)) {
default:
die(_(unexpected diff status %c), p-status);
+ case DIFF_STATUS_ADDED:
case DIFF_STATUS_MODIFIED:
case DIFF_STATUS_TYPE_CHANGED:
if (add_file_to_index(the_index, path, data-flags)) {
diff --git a/diff-lib.c b/diff-lib.c
index a85c497..714501a 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -212,6 +212,11 @@ int run_diff_files(struct rev_info *revs, unsigned int
option)
ce-sha1,
!is_null_sha1(ce-sha1),
ce-name, 0);
continue;
+ } else if (ce-ce_flags CE_INTENT_TO_ADD) {
+ diff_addremove(revs-diffopt, '+', ce-ce_mode,
+ EMPTY_BLOB_SHA1_BIN, 0,
+ ce-name, 0);
+ continue;
}
changed = match_stat_with_submodule(revs-diffopt, ce,
st,
@@ -376,6 +381,13 @@ static void do_oneway_diff(struct unpack_trees_options *o,
struct rev_info *revs = o-unpack_data;
int match_missing, cached;
+ /* i-t-a entries do not actually exist in the index */
+ if (idx (idx-ce_flags CE_INTENT_TO_ADD)) {
+ idx = NULL;
+ if (!tree)
+ return; /* nothing to diff.. */
+ }
+
/* if the entry is not checked out, don't examine work tree */
cached = o-index_only ||
(idx ((idx-ce_flags CE_VALID) || ce_skip_worktree(idx)));
diff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh
index 2a4a749..42827b8 100755
--- a/t/t2203-add-intent.sh
+++ b/t/t2203-add-intent.sh
@@ -5,10 +5,24 @@ test_description='Intent to add'
. ./test-lib.sh
test_expect_success 'intent to add' '
+ test_commit 1
+ git rm 1.t
+ echo hello 1.t
echo hello file
echo hello elif
git add -N file
- git add elif
+ git add elif
+ git add -N 1.t
+'
+
+test_expect_success 'git status' '
+ git status --porcelain | grep -v actual actual
+ cat expect -\EOF
+ DA 1.t
+ A elif
+A file
+ EOF
+ test_cmp expect actual
'
test_expect_success 'check result of add -N' '
@@ -43,7 +57,8 @@ test_expect_success 'i-t-a entry is simply ignored' '
git add -N nitfol
git commit -m second
test $(git ls-tree HEAD -- nitfol | wc -l) = 0
- test $(git diff --name-only HEAD -- nitfol | wc -l) = 1
+ test $(git diff --name-only HEAD -- nitfol | wc -l) = 0
+ test $(git diff --name-only -- nitfol | wc -l) = 1
'
test_expect_success 'can commit with an unrelated i-t-a entry in index' '
@@ -62,7 +77,7 @@ test_expect_success 'can commit -a with an i-t-a entry' '
git commit -a -m all
'
-test_expect_success 'cache-tree invalidates i-t-a paths' '
Re: [PATCH v2] diff-lib.c: adjust position of i-t-a entries in diff
Michael J Gruber g...@drmicha.warpmail.net writes: Nguyễn Thái Ngọc Duy venit, vidit, dixit 16.03.2015 14:56: The test cache-tree invalidates i-t-a paths is marked failure because I don't think removing --cached from git diff is the right fix. This test relies on diff --cached behavior but the behavior now has changed. We may need to revisit this test at some point in future. I can't quite follow that reasoning. If the test describes expected behavior which the patch breaks, then we should not apply the patch. If the patch changes behavior in an expected way, then we should change the test to match. The offending one came from eec3e7e4 (cache-tree: invalidate i-t-a paths after generating trees, 2012-12-16), which was a fix to an earlier bug where a cache-tree written out of an index with i-t-a entries had incorrect information and still claimed it is fully valid after write-tree rebuilt it. The test probably should add another path without i-t-a bit, run the same diff --cached with updated expectation before write-tre, and run the diff --cached again to make sure it produces a result that match the updated expectation. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] diff-lib.c: adjust position of i-t-a entries in diff
Nguyễn Thái Ngọc Duy venit, vidit, dixit 16.03.2015 14:56:
Entries added by git add -N are reminder for the user so that they
don't forget to add them before committing. These entries appear in
the index even though they are not real. Their presence in the index
leads to a confusing git status like this:
On branch master
Changes to be committed:
new file: foo
Changes not staged for commit:
modified: foo
If you do a git commit, foo will not be included even though
status reports it as to be committed. This patch changes the
output to become
On branch master
Changes not staged for commit:
new file: foo
no changes added to commit
The two hunks in diff-lib.c adjust diff-index and diff-files so
that i-t-a entries appear as new files in diff-files and nothing in
diff-index.
Due to this change, diff-files may start to report new files for the
first time. add -u needs to be told about this or it will die in
denial, screaming new files can't exist! Reality is wrong. Luckily,
it's the only one among run_diff_files() callers that needs fixing.
The test cache-tree invalidates i-t-a paths is marked failure
because I don't think removing --cached from git diff is the right
fix. This test relies on diff --cached behavior but the behavior now
has changed. We may need to revisit this test at some point in future.
I can't quite follow that reasoning. If the test describes expected
behavior which the patch breaks, then we should not apply the patch. If
the patch changes behavior in an expected way, then we should change the
test to match.
Helped-by: Michael J Gruber g...@drmicha.warpmail.net
Helped-by: Junio C Hamano gits...@pobox.com
Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com
---
builtin/add.c | 1 +
diff-lib.c | 12
t/t2203-add-intent.sh | 21 ++---
t/t4011-diff-symlink.sh | 10 ++
4 files changed, 37 insertions(+), 7 deletions(-)
diff --git a/builtin/add.c b/builtin/add.c
index 3390933..ee370b0 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -63,6 +63,7 @@ static void update_callback(struct diff_queue_struct *q,
switch (fix_unmerged_status(p, data)) {
default:
die(_(unexpected diff status %c), p-status);
+ case DIFF_STATUS_ADDED:
case DIFF_STATUS_MODIFIED:
case DIFF_STATUS_TYPE_CHANGED:
if (add_file_to_index(the_index, path, data-flags)) {
diff --git a/diff-lib.c b/diff-lib.c
index a85c497..714501a 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -212,6 +212,11 @@ int run_diff_files(struct rev_info *revs, unsigned int
option)
ce-sha1,
!is_null_sha1(ce-sha1),
ce-name, 0);
continue;
+ } else if (ce-ce_flags CE_INTENT_TO_ADD) {
+ diff_addremove(revs-diffopt, '+', ce-ce_mode,
+EMPTY_BLOB_SHA1_BIN, 0,
+ce-name, 0);
+ continue;
}
changed = match_stat_with_submodule(revs-diffopt, ce,
st,
@@ -376,6 +381,13 @@ static void do_oneway_diff(struct unpack_trees_options
*o,
struct rev_info *revs = o-unpack_data;
int match_missing, cached;
+ /* i-t-a entries do not actually exist in the index */
+ if (idx (idx-ce_flags CE_INTENT_TO_ADD)) {
+ idx = NULL;
+ if (!tree)
+ return; /* nothing to diff.. */
+ }
+
/* if the entry is not checked out, don't examine work tree */
cached = o-index_only ||
(idx ((idx-ce_flags CE_VALID) || ce_skip_worktree(idx)));
diff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh
index 2a4a749..42827b8 100755
--- a/t/t2203-add-intent.sh
+++ b/t/t2203-add-intent.sh
@@ -5,10 +5,24 @@ test_description='Intent to add'
. ./test-lib.sh
test_expect_success 'intent to add' '
+ test_commit 1
+ git rm 1.t
+ echo hello 1.t
echo hello file
echo hello elif
git add -N file
- git add elif
+ git add elif
+ git add -N 1.t
+'
+
+test_expect_success 'git status' '
+ git status --porcelain | grep -v actual actual
+ cat expect -\EOF
+ DA 1.t
+ A elif
+ A file
+ EOF
+ test_cmp expect actual
'
test_expect_success 'check result of add -N' '
@@ -43,7 +57,8 @@ test_expect_success 'i-t-a entry is simply ignored' '
git add -N nitfol
git commit -m second
test $(git ls-tree HEAD -- nitfol | wc -l) = 0
- test $(git diff --name-only HEAD -- nitfol | wc -l) = 1
+ test $(git diff --name-only HEAD -- nitfol | wc -l) = 0
