* apply --cached --whitespace=fix now failing on items added with "add -N" @ 2015-06-22 14:29 Patrick Higgins 2015-06-22 14:45 ` Duy Nguyen 2015-06-23 12:34 ` [PATCH] apply: fix adding new files on i-t-a entries Nguyễn Thái Ngọc Duy 0 siblings, 2 replies; 28+ messages in thread From: Patrick Higgins @ 2015-06-22 14:29 UTC (permalink / raw) To: git I like to use git to remove trailing whitespace from my files. I use the following ~/.gitconfig to make this convenient: [alias] wsadd = "!sh -c 'git diff -- \"$@\" | git apply --cached --whitespace=fix;\ git checkout -- ${1-.} \"$@\"' -" The wsadd alias doesn't work with new files, so I have to use "git add -N" on them first. As of a week or two ago, the "apply --cached" step now fails with the following, assuming a new file named bar containing "foo " has been added with "add -N": $ git diff -- "$@" | git apply --cached --whitespace=fix <stdin>:7: trailing whitespace. foo error: bar: already exists in index The final "git checkout" at the end of wsadd truncates my file. I've changed the ";" to a "&&" to fix the truncation. Were there any recent changes to "git apply" that might have caused this? $ git --version git version 2.4.3.573.g4eafbef $ uname -a Linux <redacted> 3.13.0-53-generic #89-Ubuntu SMP Wed May 20 10:34:39 UTC 2015 x86_64 x86_64 x86_64 GNU/Linux ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: apply --cached --whitespace=fix now failing on items added with "add -N" 2015-06-22 14:29 apply --cached --whitespace=fix now failing on items added with "add -N" Patrick Higgins @ 2015-06-22 14:45 ` Duy Nguyen 2015-06-22 17:06 ` Junio C Hamano 2015-06-23 12:34 ` [PATCH] apply: fix adding new files on i-t-a entries Nguyễn Thái Ngọc Duy 1 sibling, 1 reply; 28+ messages in thread From: Duy Nguyen @ 2015-06-22 14:45 UTC (permalink / raw) To: Patrick Higgins; +Cc: Git Mailing List On Mon, Jun 22, 2015 at 9:29 PM, Patrick Higgins <phiggins@google.com> wrote: > I like to use git to remove trailing whitespace from my files. I use > the following ~/.gitconfig to make this convenient: > > [alias] > wsadd = "!sh -c 'git diff -- \"$@\" | git apply --cached > --whitespace=fix;\ > git checkout -- ${1-.} \"$@\"' -" > > The wsadd alias doesn't work with new files, so I have to use "git add > -N" on them first. As of a week or two ago, the "apply --cached" step > now fails with the following, assuming a new file named bar containing > "foo " has been added with "add -N": > > $ git diff -- "$@" | git apply --cached --whitespace=fix > <stdin>:7: trailing whitespace. > foo > error: bar: already exists in index > > The final "git checkout" at the end of wsadd truncates my file. I've > changed the ";" to a "&&" to fix the truncation. > > Were there any recent changes to "git apply" that might have caused this? Probably fallout from this one, merged to 'master' about 5 weeks ago. I'll have a look at it tomorrow unless somebody does it first d95d728 (diff-lib.c: adjust position of i-t-a entries in diff - 2015-03-16) -- Duy ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: apply --cached --whitespace=fix now failing on items added with "add -N" 2015-06-22 14:45 ` Duy Nguyen @ 2015-06-22 17:06 ` Junio C Hamano 0 siblings, 0 replies; 28+ messages in thread From: Junio C Hamano @ 2015-06-22 17:06 UTC (permalink / raw) To: Duy Nguyen; +Cc: Patrick Higgins, Git Mailing List Duy Nguyen <pclouds@gmail.com> writes: > On Mon, Jun 22, 2015 at 9:29 PM, Patrick Higgins <phiggins@google.com> wrote: >> I like to use git to remove trailing whitespace from my files. I use >> the following ~/.gitconfig to make this convenient: >> >> [alias] >> wsadd = "!sh -c 'git diff -- \"$@\" | git apply --cached >> --whitespace=fix;\ >> git checkout -- ${1-.} \"$@\"' -" >> >> The wsadd alias doesn't work with new files, so I have to use "git add >> -N" on them first. As of a week or two ago, the "apply --cached" step >> now fails with the following, assuming a new file named bar containing >> "foo " has been added with "add -N": >> >> $ git diff -- "$@" | git apply --cached --whitespace=fix >> <stdin>:7: trailing whitespace. >> foo >> error: bar: already exists in index >> >> The final "git checkout" at the end of wsadd truncates my file. I've >> changed the ";" to a "&&" to fix the truncation. >> >> Were there any recent changes to "git apply" that might have caused this? > > Probably fallout from this one, merged to 'master' about 5 weeks ago. > I'll have a look at it tomorrow unless somebody does it first > > d95d728 (diff-lib.c: adjust position of i-t-a entries in diff - 2015-03-16) Yeah, the world order has changed by that commit, and I would expect to see some more fallouts. After "add -N", "git diff" used to pretend that an empty blob was in the index, and showed a modification between an existing "empty" and the full file contents, and "git apply --cached" was happy to take that modification. In the new world order, "git diff" is instructed to pretend as if the path that was added by "add -N" does not exist yet in the index at all, but the index still knows about the path, which is a strange half-born state. It shows an addition of the full file contents as a new file. "git apply --cached" sees an addition of a new path, which requires that the path does not exist in the index. In the new world order, it should be taught to pretend that these "add -N" paths do not exist in the index, but that was not done. Something like the attached (totally untested) may be a good starting point. For another potential fallout, try this: $ cp COPYING RENAMING $ cp COPYING UNTRACKED $ >EMPTY $ git add -N RENAMING EMPTY $ git rm UNTRACKED fatal: pathspec 'UNTRACKED' did not match any files $ git rm EMPTY RENAMING error: the following file has staged content different from both the file and the HEAD: EMPTY RENAMING (use -f to force removal) One could argue that these three should behave the same way, if the new world order is "path added by 'add -N' does not exist in the index". I however think the new world order should be slightly different from "does not exist in the index", but should be more like "the index is aware of its presence but has not been told about its contents yet". The consequences of this are: - "git rm RENAMING" shouldn't say 'did not match any files'. - "git rm RENAMING" does not know about 'staged content', so complaining about "staged contents different from file and HEAD" feels wrong. Having said that, I do think erroring out and requiring -f is the right thing when remiving RENAMING and EMPTY. Unlike contents added by "git add" without "-N", we do not know what is in the working tree file at all. Given that we check and refuse when the contents are different between the working tree file, the index and the HEAD even when we know what was in the working tree when "git add" without "-N" was done, we should keep the safety to prevent accidental removal of the working tree file, which has the only existing copy of the user content. On the other hand, I am also OK if the behaviour was like this: $ git rm --cached RENAMING ... removed without complaints ... $ git rm RENAMING error: file 'RENAMING' does not have staged content yet. (use -f to force removal) In any case, here is a "how about this" weather-balloon patch for "git apply" builtin/apply.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/builtin/apply.c b/builtin/apply.c index 146be97..653191e 100644 --- a/builtin/apply.c +++ b/builtin/apply.c @@ -3546,12 +3546,23 @@ static int check_preimage(struct patch *patch, struct cache_entry **ce, struct s #define EXISTS_IN_INDEX 1 #define EXISTS_IN_WORKTREE 2 +static int exists_in_index(const char *new_name) +{ + int pos = cache_name_pos(new_name, strlen(new_name)); + + if (pos < 0) + return 0; + if (active_cache[pos]->ce_flags & CE_INTENT_TO_ADD) + return 0; + return 1; +} + static int check_to_create(const char *new_name, int ok_if_exists) { struct stat nst; if (check_index && - cache_name_pos(new_name, strlen(new_name)) >= 0 && + exists_in_index(new_name) && !ok_if_exists) return EXISTS_IN_INDEX; if (cached) ^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH] apply: fix adding new files on i-t-a entries 2015-06-22 14:29 apply --cached --whitespace=fix now failing on items added with "add -N" Patrick Higgins 2015-06-22 14:45 ` Duy Nguyen @ 2015-06-23 12:34 ` Nguyễn Thái Ngọc Duy 2015-06-23 16:50 ` Junio C Hamano 1 sibling, 1 reply; 28+ messages in thread From: Nguyễn Thái Ngọc Duy @ 2015-06-23 12:34 UTC (permalink / raw) To: git Cc: Junio C Hamano, phiggins, snoksrud, Nguyễn Thái Ngọc Duy Since d95d728 (diff-lib.c: adjust position of i-t-a entries in diff - 2015-03-16), a normal "git diff" on i-t-a entries would produce a diff that _adds_ those files, not just adding content to existing and empty files like before. This is correct. Unfortunately, applying such a patch on the same repository that has the same i-t-a entries will fail with message "already exists in index" because git-apply checks, sees those i-t-a entries and aborts. git-apply does not realize those are for bookkeeping only, they do not really exist in the index. This patch tightens the "exists in index" check, ignoring i-t-a entries. For fixing the above problem, only the change in check_to_create() is needed. For other changes, - load_current(), reporting "not exists in index" is better than "does not match index" - check_preimage(), similar to load_current(), but it may also use ce_mode from i-t-a entry which is always zero - get_current_sha1(), or actually build_fake_ancestor(), we should not add i-t-a entries to the temporary index, at least not without also adding i-t-a flag back Since "git add -p" and "git am" use "git apply" underneath, they are broken too before this patch and fixed now. Reported-by: Patrick Higgins <phiggins@google.com> Reported-by: Bjørnar Snoksrud <snoksrud@gmail.com> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com> --- I think I'm opening a can of worms with d95d728. There's nothing wrong with that patch per se, but with this issue popping up, I need to go over all {cache,index}_name_pos call sites and see what would be the sensible behavior when i-t-a entries are involved. So far blame, rm and checkout-entry and "checkout <paths>" are on my to-think-or-fix list. But this patch can get in early to fix a real regression instead of waiting for one big series. A lot more discussions will be had before that series gets in good shape. builtin/apply.c | 8 ++++---- cache.h | 2 ++ read-cache.c | 12 ++++++++++++ t/t2203-add-intent.sh | 10 ++++++++++ 4 files changed, 28 insertions(+), 4 deletions(-) diff --git a/builtin/apply.c b/builtin/apply.c index 146be97..4f813ac 100644 --- a/builtin/apply.c +++ b/builtin/apply.c @@ -3344,7 +3344,7 @@ static int load_current(struct image *image, struct patch *patch) if (!patch->is_new) die("BUG: patch to %s is not a creation", patch->old_name); - pos = cache_name_pos(name, strlen(name)); + pos = exists_in_cache(name, strlen(name)); if (pos < 0) return error(_("%s: does not exist in index"), name); ce = active_cache[pos]; @@ -3497,7 +3497,7 @@ static int check_preimage(struct patch *patch, struct cache_entry **ce, struct s } if (check_index && !previous) { - int pos = cache_name_pos(old_name, strlen(old_name)); + int pos = exists_in_cache(old_name, strlen(old_name)); if (pos < 0) { if (patch->is_new < 0) goto is_new; @@ -3551,7 +3551,7 @@ static int check_to_create(const char *new_name, int ok_if_exists) struct stat nst; if (check_index && - cache_name_pos(new_name, strlen(new_name)) >= 0 && + exists_in_cache(new_name, strlen(new_name)) >= 0 && !ok_if_exists) return EXISTS_IN_INDEX; if (cached) @@ -3824,7 +3824,7 @@ static int get_current_sha1(const char *path, unsigned char *sha1) if (read_cache() < 0) return -1; - pos = cache_name_pos(path, strlen(path)); + pos = exists_in_cache(path, strlen(path)); if (pos < 0) return -1; hashcpy(sha1, active_cache[pos]->sha1); diff --git a/cache.h b/cache.h index 571c98f..6a44cb6 100644 --- a/cache.h +++ b/cache.h @@ -341,6 +341,7 @@ extern void free_name_hash(struct index_state *istate); #define discard_cache() discard_index(&the_index) #define unmerged_cache() unmerged_index(&the_index) #define cache_name_pos(name, namelen) index_name_pos(&the_index,(name),(namelen)) +#define exists_in_cache(name, namelen) exists_in_index(&the_index,(name),(namelen)) #define add_cache_entry(ce, option) add_index_entry(&the_index, (ce), (option)) #define rename_cache_entry_at(pos, new_name) rename_index_entry_at(&the_index, (pos), (new_name)) #define remove_cache_entry_at(pos) remove_index_entry_at(&the_index, (pos)) @@ -512,6 +513,7 @@ extern int verify_path(const char *path); extern struct cache_entry *index_dir_exists(struct index_state *istate, const char *name, int namelen); extern struct cache_entry *index_file_exists(struct index_state *istate, const char *name, int namelen, int igncase); extern int index_name_pos(const struct index_state *, const char *name, int namelen); +extern int exists_in_index(const struct index_state *, const char *name, int namelen); #define ADD_CACHE_OK_TO_ADD 1 /* Ok to add */ #define ADD_CACHE_OK_TO_REPLACE 2 /* Ok to replace file/directory */ #define ADD_CACHE_SKIP_DFCHECK 4 /* Ok to skip DF conflict checks */ diff --git a/read-cache.c b/read-cache.c index 5dee4e2..d3b50cb 100644 --- a/read-cache.c +++ b/read-cache.c @@ -506,6 +506,18 @@ int index_name_pos(const struct index_state *istate, const char *name, int namel return index_name_stage_pos(istate, name, namelen, 0); } +/* This is the same as index_name_pos, except that i-t-a entries are invisible */ +int exists_in_index(const struct index_state *istate, const char *name, int namelen) +{ + int pos = index_name_stage_pos(istate, name, namelen, 0); + + if (pos < 0) + return pos; + if (istate->cache[pos]->ce_flags & CE_INTENT_TO_ADD) + return -pos-1; + return pos; +} + /* Remove entry, return true if there are more entries to go.. */ int remove_index_entry_at(struct index_state *istate, int pos) { diff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh index 7c641bf..a10a96a 100755 --- a/t/t2203-add-intent.sh +++ b/t/t2203-add-intent.sh @@ -97,5 +97,15 @@ test_expect_success 'cache-tree invalidates i-t-a paths' ' test_cmp expect actual ' +test_expect_success 'apply on i-t-a entries' ' + git init apply && + ( + cd apply && + echo content >file && + git add -N file && + git diff | git apply --cached + ) +' + test_done -- 2.3.0.rc1.137.g477eb31 ^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: [PATCH] apply: fix adding new files on i-t-a entries 2015-06-23 12:34 ` [PATCH] apply: fix adding new files on i-t-a entries Nguyễn Thái Ngọc Duy @ 2015-06-23 16:50 ` Junio C Hamano 2015-06-23 17:37 ` Junio C Hamano ` (2 more replies) 0 siblings, 3 replies; 28+ messages in thread From: Junio C Hamano @ 2015-06-23 16:50 UTC (permalink / raw) To: Nguyễn Thái Ngọc Duy; +Cc: git, phiggins, snoksrud Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes: > Since d95d728 (diff-lib.c: adjust position of i-t-a entries in diff - > 2015-03-16), a normal "git diff" on i-t-a entries would produce a diff > that _adds_ those files, not just adding content to existing and empty > files like before. > > This is correct. Unfortunately, applying such a patch on the same > repository that has the same i-t-a entries will fail with message > "already exists in index" because git-apply checks, sees those i-t-a > entries and aborts. git-apply does not realize those are for > bookkeeping only, they do not really exist in the index. > This patch tightens the "exists in index" check, ignoring i-t-a > entries. For fixing the above problem, only the change in > check_to_create() is needed. And the first thing I noticed and found a bit disturbing was that this change (which I think is correct, and happens to match what I sent out earlier) was the only thing necessary to make your new test pass. IOW, the other changes in this patch have no test coverage. > For other changes, > > - load_current(), reporting "not exists in index" is better than "does > not match index" Is that error reporting the only side effect from this change? This is only used when falling back to three-way merge while applying a creation patch. > - check_preimage(), similar to load_current(), but it may also use > ce_mode from i-t-a entry which is always zero This is for the normal (non three-way) application and the idea is the same as load_current() as you said above. > - get_current_sha1(), or actually build_fake_ancestor(), we should not > add i-t-a entries to the temporary index, at least not without also > adding i-t-a flag back This is part of "am" three-way fallback codepath. I do not think the merge-recursive three-way merge code knows and cares about, is capable of handling, or would even want to deal with i-t-a entries in the first place, so adding an entry as i-t-a bit would not help. What the ultimate caller wants from us in this codepath is a tree object, and that is written out from the temporary index---and that codepath ignores i-t-a entries, so it is correct to omit them from the temporary index in the first place. Unlike the previous two changes, I think this change deserves a new test. > I think I'm opening a can of worms with d95d728. There's nothing > wrong with that patch per se, but with this issue popping up, I need > to go over all {cache,index}_name_pos call sites and see what would be > the sensible behavior when i-t-a entries are involved. Yeah, I agree that d95d728 should have been a part of a larger series that changes the world order, instead of a single change that brings inconsistency to the system. I cannot offhand convince myself that "apply" is the only casualty; assuming it is, I think a reasonable way forward is to keep d95d728 and adjust "apply" to the new world order. Otherwise, i.e. if there are wider fallouts from d95d728, we may instead want to temporarily revert it off from 'master', deal with fallouts to "apply" and other things, before resurrecting it. Anything that internally uses "diff-index" is suspect, I think. What do others think? You seem to ... > So far blame, rm and checkout-entry and "checkout <paths>" are on my > to-think-or-fix list. But this patch can get in early to fix a real > regression instead of waiting for one big series. A lot more > discussions will be had before that series gets in good shape. ... think that the damage could be quite extensive, so I am inclined to say that we first revert d95d728 before going forward. > builtin/apply.c | 8 ++++---- > cache.h | 2 ++ > read-cache.c | 12 ++++++++++++ > t/t2203-add-intent.sh | 10 ++++++++++ > 4 files changed, 28 insertions(+), 4 deletions(-) > > diff --git a/builtin/apply.c b/builtin/apply.c > index 146be97..4f813ac 100644 > --- a/builtin/apply.c > +++ b/builtin/apply.c > @@ -3344,7 +3344,7 @@ static int load_current(struct image *image, struct patch *patch) > if (!patch->is_new) > die("BUG: patch to %s is not a creation", patch->old_name); > > - pos = cache_name_pos(name, strlen(name)); > + pos = exists_in_cache(name, strlen(name)); Something that is named as if it would return yes/no that returns a real value is not a very welcome idea. > +/* This is the same as index_name_pos, except that i-t-a entries are invisible */ > +int exists_in_index(const struct index_state *istate, const char *name, int namelen) > +{ > + int pos = index_name_stage_pos(istate, name, namelen, 0); > + > + if (pos < 0) > + return pos; > + if (istate->cache[pos]->ce_flags & CE_INTENT_TO_ADD) > + return -pos-1; This is a useless complexity. Your callers cannot use the returned value like this: pos = exists_in_cache(...); if (pos < 0) { if (active_cache[-pos-1]->ce_flags & CE_INTENT_TO_ADD) ; /* ah it actually exists but it is i-t-a */ else ; /* no it does not really exist */ } else { ; /* yes it is really there at pos */ } because they cannot tell two cases apart: (1) you do have i-t-a with the given name, (2) you do not have the entry but the location you would insert an entry with such a name is occupied by an unrelated entry (i.e. with a name that sorts adjacent) that happens to be i-t-a. > + return pos; > +} ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH] apply: fix adding new files on i-t-a entries 2015-06-23 16:50 ` Junio C Hamano @ 2015-06-23 17:37 ` Junio C Hamano 2015-06-24 4:48 ` Junio C Hamano 2015-06-24 10:11 ` Duy Nguyen 2 siblings, 0 replies; 28+ messages in thread From: Junio C Hamano @ 2015-06-23 17:37 UTC (permalink / raw) To: Nguyễn Thái Ngọc Duy; +Cc: git, phiggins, snoksrud Junio C Hamano <gitster@pobox.com> writes: >> I think I'm opening a can of worms with d95d728.... > > I cannot offhand convince myself that "apply" is the only casualty; > assuming it is, I think a reasonable way forward is to keep d95d728 > and adjust "apply" to the new world order. Otherwise, i.e. if there > are wider fallouts from d95d728, we may instead want to temporarily > revert it off from 'master', deal with fallouts to "apply" and other > things, before resurrecting it. > > Anything that internally uses "diff-index" is suspect, I think. > > What do others think? You seem to ... > >> So far blame, rm and checkout-entry and "checkout <paths>" are on my >> to-think-or-fix list. But this patch can get in early to fix a real >> regression instead of waiting for one big series. A lot more >> discussions will be had before that series gets in good shape. > > ... think that the damage could be quite extensive, so I am inclined > to say that we first revert d95d728 before going forward. Let's do this on 'nd/diff-i-t-a' topic and merge the result immediately to 'master' for now. -- >8 -- From: Junio C Hamano <gitster@pobox.com> Date: Tue, 23 Jun 2015 10:27:47 -0700 Subject: [PATCH] Revert "diff-lib.c: adjust position of i-t-a entries in diff" This reverts commit d95d728aba06a34394d15466045cbdabdada58a2. It turns out that many other commands that need to interact with the result of running diff-files and diff-index, e.g. "git apply", "git rm", etc., need to be adjusted to the new world order it brings in. For example, it would break this sequence to correct a whitespace breakage in the parts you changed: git add -N file git diff --cached file | git apply --cached --whitespace=fix git checkout file In the old world order, "diff" showed a patch to modify an existing empty file by adding its full contents, and "apply" updated the index by modifying the existing empty blob (which is what an Intent-to-Add entry records in the index) with that patch. In the new world order, "diff" shows a patch to create a new file with its full contents, but because "apply" thinks that the i-t-a entry already exists in the index, it refused to accept a creation. Adjusting "apply" to this new world order is easy, but we need to assess the extent of the damage to the rest of the system the new world order brought in before going forward and adjust them all, after which we can resurrect the commit being reverted here. --- builtin/add.c | 1 - diff-lib.c | 12 ------------ t/t2203-add-intent.sh | 23 ++++------------------- t/t4011-diff-symlink.sh | 10 ++++------ 4 files changed, 8 insertions(+), 38 deletions(-) diff --git a/builtin/add.c b/builtin/add.c index ee370b0..3390933 100644 --- a/builtin/add.c +++ b/builtin/add.c @@ -63,7 +63,6 @@ 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 714501a..a85c497 100644 --- a/diff-lib.c +++ b/diff-lib.c @@ -212,11 +212,6 @@ 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, @@ -381,13 +376,6 @@ 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 7c641bf..2a4a749 100755 --- a/t/t2203-add-intent.sh +++ b/t/t2203-add-intent.sh @@ -5,24 +5,10 @@ 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 -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 + git add elif ' test_expect_success 'check result of "add -N"' ' @@ -57,8 +43,7 @@ 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) = 0 && - test $(git diff --name-only -- nitfol | wc -l) = 1 + test $(git diff --name-only HEAD -- nitfol | wc -l) = 1 ' test_expect_success 'can commit with an unrelated i-t-a entry in index' ' @@ -87,13 +72,13 @@ test_expect_success 'cache-tree invalidates i-t-a paths' ' : >dir/bar && git add -N dir/bar && git diff --cached --name-only >actual && - >expect && + echo dir/bar >expect && test_cmp expect actual && git write-tree >/dev/null && git diff --cached --name-only >actual && - >expect && + echo dir/bar >expect && test_cmp expect actual ' diff --git a/t/t4011-diff-symlink.sh b/t/t4011-diff-symlink.sh index 7452fce..13e7f62 100755 --- a/t/t4011-diff-symlink.sh +++ b/t/t4011-diff-symlink.sh @@ -139,13 +139,11 @@ test_expect_success SYMLINKS 'setup symlinks with attributes' ' test_expect_success SYMLINKS 'symlinks do not respect userdiff config by path' ' cat >expect <<-\EOF && diff --git a/file.bin b/file.bin - new file mode 100644 - index 0000000..d95f3ad - Binary files /dev/null and b/file.bin differ + index e69de29..d95f3ad 100644 + Binary files a/file.bin and b/file.bin differ diff --git a/link.bin b/link.bin - new file mode 120000 - index 0000000..dce41ec - --- /dev/null + index e69de29..dce41ec 120000 + --- a/link.bin +++ b/link.bin @@ -0,0 +1 @@ +file.bin -- 2.4.4-598-g4ab0ce8 ^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: [PATCH] apply: fix adding new files on i-t-a entries 2015-06-23 16:50 ` Junio C Hamano 2015-06-23 17:37 ` Junio C Hamano @ 2015-06-24 4:48 ` Junio C Hamano 2015-06-24 10:11 ` Duy Nguyen 2 siblings, 0 replies; 28+ messages in thread From: Junio C Hamano @ 2015-06-24 4:48 UTC (permalink / raw) To: Nguyễn Thái Ngọc Cc: Git Mailing List, Patrick Higgins, Bjørnar Snoksrud On Tue, Jun 23, 2015 at 9:50 AM, Junio C Hamano <gitster@pobox.com> wrote: > Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes: > >> - pos = cache_name_pos(name, strlen(name)); >> + pos = exists_in_cache(name, strlen(name)); > > Something that is named as if it would return yes/no that returns a > real value is not a very welcome idea. > >> +/* This is the same as index_name_pos, except that i-t-a entries are invisible */ >> +int exists_in_index(const struct index_state *istate, const char *name, int namelen) >> +{ >> + int pos = index_name_stage_pos(istate, name, namelen, 0); >> + >> + if (pos < 0) >> + return pos; >> + if (istate->cache[pos]->ce_flags & CE_INTENT_TO_ADD) >> + return -pos-1; > > This is a useless complexity. Your callers cannot use the returned > value like this: > > pos = exists_in_cache(...); > if (pos < 0) { > if (active_cache[-pos-1]->ce_flags & CE_INTENT_TO_ADD) > ; /* ah it actually exists but it is i-t-a */ > else > ; /* no it does not really exist */ > } else { > ; /* yes it is really there at pos */ > } > > because they cannot tell two cases apart: (1) you do have i-t-a with > the given name, (2) you do not have the entry but the location you > would insert an entry with such a name is occupied by an unrelated > entry (i.e. with a name that sorts adjacent) that happens to be > i-t-a. Also, the callers cannot even use that return value in the usual way they would use the return value from index_name_pos(), either. pos = exists_in_cache(...); if (pos < 0) { /* ah, it does not exist, so... */ pos = -1 - pos; /* * ... it is OK to shift active_cache[pos..] by one and add our * entry at active_cache[pos] */ } else { /* it exists, so update in place */ ; } So, returning pos that smells like a return value from index_name_pos() only has an effect of confusing callers into buggy code, I am afraid. The callers that care need to be updated to check for ce_flags after finding the entry with index_name_pos() the usual way if you want to avoid search in the index_state->cache[] twice, and the callers that are only interested in knowing if an entry "exists" are better off with an exists_in_cache() that returns Yes/No and not a confusing and useless "pos", I think. ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH] apply: fix adding new files on i-t-a entries 2015-06-23 16:50 ` Junio C Hamano 2015-06-23 17:37 ` Junio C Hamano 2015-06-24 4:48 ` Junio C Hamano @ 2015-06-24 10:11 ` Duy Nguyen 2015-06-24 17:05 ` Junio C Hamano 2 siblings, 1 reply; 28+ messages in thread From: Duy Nguyen @ 2015-06-24 10:11 UTC (permalink / raw) To: Junio C Hamano; +Cc: Git Mailing List, phiggins, snoksrud On Tue, Jun 23, 2015 at 11:50 PM, Junio C Hamano <gitster@pobox.com> wrote: >> This patch tightens the "exists in index" check, ignoring i-t-a >> entries. For fixing the above problem, only the change in >> check_to_create() is needed. > > And the first thing I noticed and found a bit disturbing was that > this change (which I think is correct, and happens to match what I > sent out earlier) was the only thing necessary to make your new test > pass. IOW, the other changes in this patch have no test coverage. Because to be honest I don't understand these code well enough to know how to trigger it :) >> For other changes, >> >> - load_current(), reporting "not exists in index" is better than "does >> not match index" > > Is that error reporting the only side effect from this change? The only thing that I can see. If an i-t-a entry is returned, it can't get past verify_index_match because ce_match_stat(). Hmm.. no the on-disk version is gone, we'll get to checkout_target() where it will "restore" the entry with an empty file. This is related to checkout that I will continue later below. >> - get_current_sha1(), or actually build_fake_ancestor(), we should not >> add i-t-a entries to the temporary index, at least not without also >> adding i-t-a flag back > > This is part of "am" three-way fallback codepath. I do not think > the merge-recursive three-way merge code knows and cares about, is > capable of handling, or would even want to deal with i-t-a entries > in the first place, so adding an entry as i-t-a bit would not help. > What the ultimate caller wants from us in this codepath is a tree > object, and that is written out from the temporary index---and that > codepath ignores i-t-a entries, so it is correct to omit them from > the temporary index in the first place. Unlike the previous two > changes, I think this change deserves a new test. Will do, after I study some more about this apply.c. > >> I think I'm opening a can of worms with d95d728. There's nothing >> wrong with that patch per se, but with this issue popping up, I need >> to go over all {cache,index}_name_pos call sites and see what would be >> the sensible behavior when i-t-a entries are involved. > > Yeah, I agree that d95d728 should have been a part of a larger > series that changes the world order, instead of a single change that > brings inconsistency to the system. > > I cannot offhand convince myself that "apply" is the only casualty; > assuming it is, I think a reasonable way forward is to keep d95d728 > and adjust "apply" to the new world order. Otherwise, i.e. if there > are wider fallouts from d95d728, we may instead want to temporarily > revert it off from 'master', deal with fallouts to "apply" and other > things, before resurrecting it. > > Anything that internally uses "diff-index" is suspect, I think. Yeah that's one or two more grep runs and more reading. > What do others think? You seem to ... > >> So far blame, rm and checkout-entry and "checkout <paths>" are on my >> to-think-or-fix list. But this patch can get in early to fix a real >> regression instead of waiting for one big series. A lot more >> discussions will be had before that series gets in good shape. > > ... think that the damage could be quite extensive, so I am inclined > to say that we first revert d95d728 before going forward. I'm not opposed to reverting if you think it's the safest option and I will report back soon after grepping diff-index. But those I mentioned above have more to do with the fact that an i-t-a entry does exist in the index in a normal way, so reverting does not help. Take checkout for example, when you do "git checkout -- foo" where foo is i-t-a, the file foo on disk will be emptied because the SHA-1 in the i-t-a entry is an empty blob, mostly to help "git diff". I think it should behave as if foo is not i-t-a: checkout should error out about not matching pathspec, or at least not destroy "foo" on disk. To me, when "ce" is an i-t-a entry, only i-t-a flag and ce_name are valid, the rest of "ce" should never be accessed. blame.c's situation is close to check_preimage() where it may read zero from ce_mode. It may be ok for check_preimage() to take zero as mode, but I think this is like fixed size buffer vs strbuf again. It works now, but if the code is reorganized or refactored, then it may or may not work. Better be safe than sorry and avoid reading something we should not read in the first place. >> builtin/apply.c | 8 ++++---- >> cache.h | 2 ++ >> read-cache.c | 12 ++++++++++++ >> t/t2203-add-intent.sh | 10 ++++++++++ >> 4 files changed, 28 insertions(+), 4 deletions(-) >> >> diff --git a/builtin/apply.c b/builtin/apply.c >> index 146be97..4f813ac 100644 >> --- a/builtin/apply.c >> +++ b/builtin/apply.c >> @@ -3344,7 +3344,7 @@ static int load_current(struct image *image, struct patch *patch) >> if (!patch->is_new) >> die("BUG: patch to %s is not a creation", patch->old_name); >> >> - pos = cache_name_pos(name, strlen(name)); >> + pos = exists_in_cache(name, strlen(name)); > > Something that is named as if it would return yes/no that returns a > real value is not a very welcome idea. Yeah. But I don't want the caller to call cache_name_pos again to get "pos". If I can't find a better name, I'll probably go with cache_name_pos_without_ita(). >> +/* This is the same as index_name_pos, except that i-t-a entries are invisible */ >> +int exists_in_index(const struct index_state *istate, const char *name, int namelen) >> +{ >> + int pos = index_name_stage_pos(istate, name, namelen, 0); >> + >> + if (pos < 0) >> + return pos; >> + if (istate->cache[pos]->ce_flags & CE_INTENT_TO_ADD) >> + return -pos-1; > > This is a useless complexity. Your callers cannot use the returned > value like this: > > pos = exists_in_cache(...); > if (pos < 0) { > if (active_cache[-pos-1]->ce_flags & CE_INTENT_TO_ADD) > ; /* ah it actually exists but it is i-t-a */ > else > ; /* no it does not really exist */ > } else { > ; /* yes it is really there at pos */ > } > > because they cannot tell two cases apart: (1) you do have i-t-a with > the given name, (2) you do not have the entry but the location you > would insert an entry with such a name is occupied by an unrelated > entry (i.e. with a name that sorts adjacent) that happens to be > i-t-a. OK so either return -1 when the entry does not exist or is i-t-a and non-negative otherwise. -- Duy ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH] apply: fix adding new files on i-t-a entries 2015-06-24 10:11 ` Duy Nguyen @ 2015-06-24 17:05 ` Junio C Hamano 2015-06-25 12:26 ` Duy Nguyen 0 siblings, 1 reply; 28+ messages in thread From: Junio C Hamano @ 2015-06-24 17:05 UTC (permalink / raw) To: Duy Nguyen; +Cc: Git Mailing List, phiggins, snoksrud Duy Nguyen <pclouds@gmail.com> writes: > Take checkout for example, when you do "git checkout -- foo" where foo > is i-t-a, the file foo on disk will be emptied because the SHA-1 in > the i-t-a entry is an empty blob, mostly to help "git diff". I think > it should behave as if foo is not i-t-a: checkout should error out > about not matching pathspec, or at least not destroy "foo" on disk. To > me, when "ce" is an i-t-a entry, only i-t-a flag and ce_name are > valid, the rest of "ce" should never be accessed. > > blame.c's situation is close to check_preimage() where it may read > zero from ce_mode. It may be ok for check_preimage() to take zero as > mode, but I think this is like fixed size buffer vs strbuf again. It > works now, but if the code is reorganized or refactored, then it may > or may not work. Better be safe than sorry and avoid reading something > we should not read in the first place. All of the above say there _are_ some codepaths that want to treat the existence of a path in the index not as <exists, does not exist> boolean but as <exists, i-t-a, does not exist> tristate. I do not think we disagree on that. But that is different from saying that it is always OK to treat i-t-a entries the same way as "does not exist" non-entries. Internal "diff-index --cached" is used for another reason you did not mention (and scripted Porcelains literally use that externally for the same reason). When we start a mergy operation, we say it is OK if the working tree has local changes relative to the index, but we require the index does not have any modifications since the HEAD was read. Side note: some codepaths insist that "diff-index --cached" and "diff-files" are both clean, so d95d728a is harmless; the former may say "clean" on i-t-a entries more than before, but the latter will still catch the difference between the index and the working tree and stop the caller from going forward. With d95d728a (diff-lib.c: adjust position of i-t-a entries in diff, 2015-03-16)'s world view, an empty output from "diff-index --cached" no longer means that. Entries added with any "git add -N" are not reported, so we would go ahead to record the merge result on top of that "half-dirty" index. Side note: a merge based on unpack-trees has an extra layer of safety that unpack_trees() does not ignore i-t-a entry as "not exist (yet)" and notices that the path does exist in the index but not in HEAD. But that just shows that some parts of the world do need to consider that having an i-t-a in the index makes it different from HEAD. If the mergy operation goes without any conflict, the next thing we do typically is to write the index out as a tree (to record in a new commit, etc.) and we are OK only in that particular case, because i-t-a entries are ignored. But what would happen when the mergy operation conflicts? I haven't thought about that fully, but I doubt it is a safe thing to do in general. But that is just one example that there are also other codepaths that do not want to be fooled into thinking that i-t-a entries do not exist in the index at all. All we learned from the above discussion is that unconditionally declaring that adding i-t-a entries to the index without doing anything else should keep the index compare the same to HEAD. If d95d728a were only about what wt_status.c sees (and gets reported in "git status" output), which was what the change wanted to address anyway, we didn't have to have this discussion. Without realizing that two kinds of callers want different view out of "diff-index --cached" and "diff-files", we broke half the world by changing the world order unconditionally for everybody, I am afraid. Perhaps a good and safe way forward to resurrect what d95d728a wanted to do is to first add an option to tell run_diff_index() and run_diff_files() which behaviour the caller wants to see, add that only to the caller in wt-status.c? Then incrementally pass that option from more callsites that we are absolutely certain that want this different worldview with respect to i-t-a? ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH] apply: fix adding new files on i-t-a entries 2015-06-24 17:05 ` Junio C Hamano @ 2015-06-25 12:26 ` Duy Nguyen 2015-06-25 13:07 ` Junio C Hamano 0 siblings, 1 reply; 28+ messages in thread From: Duy Nguyen @ 2015-06-25 12:26 UTC (permalink / raw) To: Junio C Hamano; +Cc: Git Mailing List, Patrick Higgins, Bjørnar Snoksrud On Thu, Jun 25, 2015 at 12:05 AM, Junio C Hamano <gitster@pobox.com> wrote: > Internal "diff-index --cached" is used for another reason you did > not mention (and scripted Porcelains literally use that externally > for the same reason). When we start a mergy operation, we say it is > OK if the working tree has local changes relative to the index, but > we require the index does not have any modifications since the HEAD > was read. > > Side note: some codepaths insist that "diff-index --cached" > and "diff-files" are both clean, so d95d728a is harmless; > the former may say "clean" on i-t-a entries more than > before, but the latter will still catch the difference > between the index and the working tree and stop the caller > from going forward. > > With d95d728a (diff-lib.c: adjust position of i-t-a entries in diff, > 2015-03-16)'s world view, an empty output from "diff-index --cached" > no longer means that. Entries added with any "git add -N" are not > reported, so we would go ahead to record the merge result on top of > that "half-dirty" index. > > Side note: a merge based on unpack-trees has an extra layer > of safety that unpack_trees() does not ignore i-t-a entry as > "not exist (yet)" and notices that the path does exist in > the index but not in HEAD. But that just shows that some > parts of the world do need to consider that having an i-t-a > in the index makes it different from HEAD. > > If the mergy operation goes without any conflict, the next thing we > do typically is to write the index out as a tree (to record in a new > commit, etc.) and we are OK only in that particular case, because > i-t-a entries are ignored. But what would happen when the mergy > operation conflicts? I haven't thought about that fully, but I > doubt it is a safe thing to do in general. > > But that is just one example that there are also other codepaths > that do not want to be fooled into thinking that i-t-a entries do > not exist in the index at all. I think it's clear that you need to revert that commit. I didn't see this at all when I made the commit. > All we learned from the above discussion is that unconditionally > declaring that adding i-t-a entries to the index without doing > anything else should keep the index compare the same to HEAD. > > If d95d728a were only about what wt_status.c sees (and gets reported > in "git status" output), which was what the change wanted to address > anyway, we didn't have to have this discussion. Without realizing > that two kinds of callers want different view out of "diff-index > --cached" and "diff-files", we broke half the world by changing the > world order unconditionally for everybody, I am afraid. > > Perhaps a good and safe way forward to resurrect what d95d728a > wanted to do is to first add an option to tell run_diff_index() and > run_diff_files() which behaviour the caller wants to see, add that > only to the caller in wt-status.c? Then incrementally pass that > option from more callsites that we are absolutely certain that want > this different worldview with respect to i-t-a? Agreed. -- Duy ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH] apply: fix adding new files on i-t-a entries 2015-06-25 12:26 ` Duy Nguyen @ 2015-06-25 13:07 ` Junio C Hamano 2015-08-22 1:08 ` [PATCH 0/8] Resurrect "diff-lib.c: adjust position of i-t-a entries in diff" Nguyễn Thái Ngọc Duy 0 siblings, 1 reply; 28+ messages in thread From: Junio C Hamano @ 2015-06-25 13:07 UTC (permalink / raw) To: Duy Nguyen; +Cc: Git Mailing List, Patrick Higgins, Bjørnar Snoksrud Duy Nguyen <pclouds@gmail.com> writes: > I think it's clear that you need to revert that commit. I didn't see > this at all when I made the commit. I didn't either, and no other reviewers did. But now we know it was not sufficient, so let's see... >> Perhaps a good and safe way forward to resurrect what d95d728a >> wanted to do is to first add an option to tell run_diff_index() and >> run_diff_files() which behaviour the caller wants to see, add that >> only to the caller in wt-status.c? Then incrementally pass that >> option from more callsites that we are absolutely certain that want >> this different worldview with respect to i-t-a? > > Agreed. OK. Perhaps then first I should do that revert and we'll incrementally rebuild on top. Thanks. ^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH 0/8] Resurrect "diff-lib.c: adjust position of i-t-a entries in diff" 2015-06-25 13:07 ` Junio C Hamano @ 2015-08-22 1:08 ` Nguyễn Thái Ngọc Duy 2015-08-22 1:08 ` [PATCH 1/8] blame: remove obsolete comment Nguyễn Thái Ngọc Duy ` (5 more replies) 0 siblings, 6 replies; 28+ messages in thread From: Nguyễn Thái Ngọc Duy @ 2015-08-22 1:08 UTC (permalink / raw) To: git Cc: Junio C Hamano, phiggins, snoksrud, Nguyễn Thái Ngọc Duy On Thu, Jun 25, 2015 at 8:07 PM, Junio C Hamano <gitster@pobox.com> wrote: >>> Perhaps a good and safe way forward to resurrect what d95d728a >>> wanted to do is to first add an option to tell run_diff_index() and >>> run_diff_files() which behaviour the caller wants to see, add that >>> only to the caller in wt-status.c? Then incrementally pass that >>> option from more callsites that we are absolutely certain that want >>> this different worldview with respect to i-t-a? >> >> Agreed. > > OK. Perhaps then first I should do that revert and we'll > incrementally rebuild on top. Here comes the rebuild. This adds --shift-ita option (and an internal flag) to enable this behavior. Those only take the last two patches. The remaining is to make sure we handle i-t-a entries correctly in some commands. Nguyễn Thái Ngọc Duy (8): blame: remove obsolete comment Add and use convenient macro ce_intent_to_add() apply: fix adding new files on i-t-a entries apply: make sure check_preimage() does not leave empty file on error checkout(-index): do not checkout i-t-a entries grep: make it clear i-t-a entries are ignored diff.h: extend "flags" field to 64 bits because we're out of bits Resurrect "diff-lib.c: adjust position of i-t-a entries in diff" Documentation/diff-options.txt | 6 +++ builtin/apply.c | 13 ++++--- builtin/blame.c | 5 --- builtin/checkout-index.c | 5 ++- builtin/checkout.c | 2 + builtin/commit.c | 2 +- builtin/grep.c | 2 +- builtin/rm.c | 2 +- cache-tree.c | 2 +- cache.h | 1 + diff-lib.c | 18 ++++++++- diff.c | 4 +- diff.h | 9 +++-- read-cache.c | 4 +- t/t2203-add-intent.sh | 83 +++++++++++++++++++++++++++++++++++++++++- wt-status.c | 2 + 16 files changed, 134 insertions(+), 26 deletions(-) -- 2.3.0.rc1.137.g477eb31 ^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH 1/8] blame: remove obsolete comment 2015-08-22 1:08 ` [PATCH 0/8] Resurrect "diff-lib.c: adjust position of i-t-a entries in diff" Nguyễn Thái Ngọc Duy @ 2015-08-22 1:08 ` Nguyễn Thái Ngọc Duy 2015-08-22 1:08 ` [PATCH 2/8] Add and use convenient macro ce_intent_to_add() Nguyễn Thái Ngọc Duy ` (4 subsequent siblings) 5 siblings, 0 replies; 28+ messages in thread From: Nguyễn Thái Ngọc Duy @ 2015-08-22 1:08 UTC (permalink / raw) To: git Cc: Junio C Hamano, phiggins, snoksrud, Nguyễn Thái Ngọc Duy That "someday" in the comment happened two years later in b65982b (Optimize "diff-index --cached" using cache-tree - 2009-05-20) Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com> --- builtin/blame.c | 5 ----- 1 file changed, 5 deletions(-) diff --git a/builtin/blame.c b/builtin/blame.c index 4db01c1..942f302 100644 --- a/builtin/blame.c +++ b/builtin/blame.c @@ -2379,11 +2379,6 @@ static struct commit *fake_working_tree_commit(struct diff_options *opt, ce->ce_mode = create_ce_mode(mode); add_cache_entry(ce, ADD_CACHE_OK_TO_ADD|ADD_CACHE_OK_TO_REPLACE); - /* - * We are not going to write this out, so this does not matter - * right now, but someday we might optimize diff-index --cached - * with cache-tree information. - */ cache_tree_invalidate_path(&the_index, path); return commit; -- 2.3.0.rc1.137.g477eb31 ^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH 2/8] Add and use convenient macro ce_intent_to_add() 2015-08-22 1:08 ` [PATCH 0/8] Resurrect "diff-lib.c: adjust position of i-t-a entries in diff" Nguyễn Thái Ngọc Duy 2015-08-22 1:08 ` [PATCH 1/8] blame: remove obsolete comment Nguyễn Thái Ngọc Duy @ 2015-08-22 1:08 ` Nguyễn Thái Ngọc Duy 2015-08-22 1:08 ` [PATCH 3/8] apply: fix adding new files on i-t-a entries Nguyễn Thái Ngọc Duy ` (3 subsequent siblings) 5 siblings, 0 replies; 28+ messages in thread From: Nguyễn Thái Ngọc Duy @ 2015-08-22 1:08 UTC (permalink / raw) To: git Cc: Junio C Hamano, phiggins, snoksrud, Nguyễn Thái Ngọc Duy Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com> --- builtin/rm.c | 2 +- cache-tree.c | 2 +- cache.h | 1 + read-cache.c | 4 ++-- 4 files changed, 5 insertions(+), 4 deletions(-) diff --git a/builtin/rm.c b/builtin/rm.c index 80b972f..8829b09 100644 --- a/builtin/rm.c +++ b/builtin/rm.c @@ -211,7 +211,7 @@ static int check_local_mod(unsigned char *head, int index_only) * "intent to add" entry. */ if (local_changes && staged_changes) { - if (!index_only || !(ce->ce_flags & CE_INTENT_TO_ADD)) + if (!index_only || !ce_intent_to_add(ce)) string_list_append(&files_staged, name); } else if (!index_only) { diff --git a/cache-tree.c b/cache-tree.c index feace8b..c865bd2 100644 --- a/cache-tree.c +++ b/cache-tree.c @@ -377,7 +377,7 @@ static int update_one(struct cache_tree *it, * they are not part of generated trees. Invalidate up * to root to force cache-tree users to read elsewhere. */ - if (ce->ce_flags & CE_INTENT_TO_ADD) { + if (ce_intent_to_add(ce)) { to_invalidate = 1; continue; } diff --git a/cache.h b/cache.h index 4e25271..41d811c 100644 --- a/cache.h +++ b/cache.h @@ -241,6 +241,7 @@ static inline unsigned create_ce_flags(unsigned stage) #define ce_uptodate(ce) ((ce)->ce_flags & CE_UPTODATE) #define ce_skip_worktree(ce) ((ce)->ce_flags & CE_SKIP_WORKTREE) #define ce_mark_uptodate(ce) ((ce)->ce_flags |= CE_UPTODATE) +#define ce_intent_to_add(ce) ((ce)->ce_flags & CE_INTENT_TO_ADD) #define ce_permissions(mode) (((mode) & 0100) ? 0755 : 0644) static inline unsigned int create_ce_mode(unsigned int mode) diff --git a/read-cache.c b/read-cache.c index 89dbc08..f849cbd 100644 --- a/read-cache.c +++ b/read-cache.c @@ -327,7 +327,7 @@ int ie_match_stat(const struct index_state *istate, * by definition never matches what is in the work tree until it * actually gets added. */ - if (ce->ce_flags & CE_INTENT_TO_ADD) + if (ce_intent_to_add(ce)) return DATA_CHANGED | TYPE_CHANGED | MODE_CHANGED; changed = ce_match_stat_basic(ce, st); @@ -1251,7 +1251,7 @@ int refresh_index(struct index_state *istate, unsigned int flags, if (cache_errno == ENOENT) fmt = deleted_fmt; - else if (ce->ce_flags & CE_INTENT_TO_ADD) + else if (ce_intent_to_add(ce)) fmt = added_fmt; /* must be before other checks */ else if (changed & TYPE_CHANGED) fmt = typechange_fmt; -- 2.3.0.rc1.137.g477eb31 ^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH 3/8] apply: fix adding new files on i-t-a entries 2015-08-22 1:08 ` [PATCH 0/8] Resurrect "diff-lib.c: adjust position of i-t-a entries in diff" Nguyễn Thái Ngọc Duy 2015-08-22 1:08 ` [PATCH 1/8] blame: remove obsolete comment Nguyễn Thái Ngọc Duy 2015-08-22 1:08 ` [PATCH 2/8] Add and use convenient macro ce_intent_to_add() Nguyễn Thái Ngọc Duy @ 2015-08-22 1:08 ` Nguyễn Thái Ngọc Duy 2015-08-25 17:01 ` Junio C Hamano 2015-08-22 1:08 ` [PATCH 4/8] apply: make sure check_preimage() does not leave empty file on error Nguyễn Thái Ngọc Duy ` (2 subsequent siblings) 5 siblings, 1 reply; 28+ messages in thread From: Nguyễn Thái Ngọc Duy @ 2015-08-22 1:08 UTC (permalink / raw) To: git Cc: Junio C Hamano, phiggins, snoksrud, Nguyễn Thái Ngọc Duy Applying a patch that adds a file when that file is registered with "git add -N" will fail with message "already exists in index" because git-apply checks, sees those i-t-a entries and aborts. git-apply does not realize those are for bookkeeping only, they do not really exist in the index. This patch tightens the "exists in index" check, ignoring i-t-a entries. Reported-by: Patrick Higgins <phiggins@google.com> Reported-by: Bjørnar Snoksrud <snoksrud@gmail.com> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com> --- builtin/apply.c | 9 +++++---- t/t2203-add-intent.sh | 13 +++++++++++++ 2 files changed, 18 insertions(+), 4 deletions(-) diff --git a/builtin/apply.c b/builtin/apply.c index 54aba4e..76b58a1 100644 --- a/builtin/apply.c +++ b/builtin/apply.c @@ -3553,10 +3553,11 @@ static int check_to_create(const char *new_name, int ok_if_exists) { struct stat nst; - if (check_index && - cache_name_pos(new_name, strlen(new_name)) >= 0 && - !ok_if_exists) - return EXISTS_IN_INDEX; + if (check_index && !ok_if_exists) { + int pos = cache_name_pos(new_name, strlen(new_name)); + if (pos >= 0 && !ce_intent_to_add(active_cache[pos])) + return EXISTS_IN_INDEX; + } if (cached) return 0; diff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh index 2a4a749..bb5ef2b 100755 --- a/t/t2203-add-intent.sh +++ b/t/t2203-add-intent.sh @@ -82,5 +82,18 @@ test_expect_success 'cache-tree invalidates i-t-a paths' ' test_cmp expect actual ' +test_expect_success 'apply adds new file on i-t-a entry' ' + git init apply && + ( + cd apply && + echo newcontent >newfile && + git add newfile && + git diff --cached >patch && + rm .git/index && + git add -N newfile && + git apply --cached patch + ) +' + test_done -- 2.3.0.rc1.137.g477eb31 ^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: [PATCH 3/8] apply: fix adding new files on i-t-a entries 2015-08-22 1:08 ` [PATCH 3/8] apply: fix adding new files on i-t-a entries Nguyễn Thái Ngọc Duy @ 2015-08-25 17:01 ` Junio C Hamano 0 siblings, 0 replies; 28+ messages in thread From: Junio C Hamano @ 2015-08-25 17:01 UTC (permalink / raw) To: Nguyễn Thái Ngọc Duy; +Cc: git, phiggins, snoksrud Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes: > Applying a patch that adds a file when that file is registered with "git > add -N" will fail with message "already exists in index" because > git-apply checks, sees those i-t-a entries and aborts. git-apply does > not realize those are for bookkeeping only, they do not really exist in > the index. This patch tightens the "exists in index" check, ignoring > i-t-a entries. Suppose that you came up with some contents to register at path F in your working tree, told Git about your intention with "add -N F", and then tried to apply a patch that wants to _create_ F: Without this patch, we would say "F already exists so a patch to create is incompatible with our current state". With this patch, we do not say that, but instead we'll hopefully trigger "does it exist in the working tree" check, unless you are running under "--cached". Which means that this change will not lead to data loss in the "untracked" file F in the working tree that was merely added to the index with i-t-a bit. As you did not say the equivalent of the last paragraph above in the log message, my knee-jerk reaction was "why could this possibly be a good idea?" Please try again with a better log message. Thanks. ^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH 4/8] apply: make sure check_preimage() does not leave empty file on error 2015-08-22 1:08 ` [PATCH 0/8] Resurrect "diff-lib.c: adjust position of i-t-a entries in diff" Nguyễn Thái Ngọc Duy ` (2 preceding siblings ...) 2015-08-22 1:08 ` [PATCH 3/8] apply: fix adding new files on i-t-a entries Nguyễn Thái Ngọc Duy @ 2015-08-22 1:08 ` Nguyễn Thái Ngọc Duy 2015-08-25 17:16 ` Junio C Hamano 2015-08-22 1:08 ` [PATCH 5/8] checkout(-index): do not checkout i-t-a entries Nguyễn Thái Ngọc Duy 2015-08-22 1:08 ` [PATCH 6/8] grep: make it clear i-t-a entries are ignored Nguyễn Thái Ngọc Duy 5 siblings, 1 reply; 28+ messages in thread From: Nguyễn Thái Ngọc Duy @ 2015-08-22 1:08 UTC (permalink / raw) To: git Cc: Junio C Hamano, phiggins, snoksrud, Nguyễn Thái Ngọc Duy The test case probably describes the test scenario the best. We have a patch to modify some file but the base file is gone. Because check_preimage() finds an index entry with the same old_name, it tries to restore the on-disk base file with cached content with checkout_target() and move on. If this old_name is i-t-a, before this patch "apply -3" will call checkout_target() which will write an empty file (i-t-a entries always have "empty blob" SHA-1), then it'll fail at verify_index_match() (i-t-a entries are always "modified") and the operation is aborted. This empty file should not be created. Compare to the case where old_name does not exist in index, "apply -3" fails with a different error message "...: does not exist" but it does not touch worktree. This patch makes sure the same happens to i-t-a entries. load_current() shares the same code pattern (look up an index entry, if on-disk version is not found, restore using the cached version). Fix it too (even though it's not exercised in any test case) Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com> --- builtin/apply.c | 4 ++-- t/t2203-add-intent.sh | 16 ++++++++++++++++ 2 files changed, 18 insertions(+), 2 deletions(-) diff --git a/builtin/apply.c b/builtin/apply.c index 76b58a1..b185c97 100644 --- a/builtin/apply.c +++ b/builtin/apply.c @@ -3348,7 +3348,7 @@ static int load_current(struct image *image, struct patch *patch) die("BUG: patch to %s is not a creation", patch->old_name); pos = cache_name_pos(name, strlen(name)); - if (pos < 0) + if (pos < 0 || ce_intent_to_add(active_cache[pos])) return error(_("%s: does not exist in index"), name); ce = active_cache[pos]; if (lstat(name, &st)) { @@ -3501,7 +3501,7 @@ static int check_preimage(struct patch *patch, struct cache_entry **ce, struct s if (check_index && !previous) { int pos = cache_name_pos(old_name, strlen(old_name)); - if (pos < 0) { + if (pos < 0 || ce_intent_to_add(active_cache[pos])) { if (patch->is_new < 0) goto is_new; return error(_("%s: does not exist in index"), old_name); diff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh index bb5ef2b..96c8755 100755 --- a/t/t2203-add-intent.sh +++ b/t/t2203-add-intent.sh @@ -95,5 +95,21 @@ test_expect_success 'apply adds new file on i-t-a entry' ' ) ' +test_expect_success 'apply:check_preimage() not creating empty file' ' + git init check-preimage && + ( + cd check-preimage && + echo oldcontent >newfile && + git add newfile && + echo newcontent >newfile && + git diff >patch && + rm .git/index && + git add -N newfile && + rm newfile && + test_must_fail git apply -3 patch && + ! test -f newfile + ) +' + test_done -- 2.3.0.rc1.137.g477eb31 ^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: [PATCH 4/8] apply: make sure check_preimage() does not leave empty file on error 2015-08-22 1:08 ` [PATCH 4/8] apply: make sure check_preimage() does not leave empty file on error Nguyễn Thái Ngọc Duy @ 2015-08-25 17:16 ` Junio C Hamano 0 siblings, 0 replies; 28+ messages in thread From: Junio C Hamano @ 2015-08-25 17:16 UTC (permalink / raw) To: Nguyễn Thái Ngọc Duy; +Cc: git, phiggins, snoksrud Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes: > The test case probably describes the test scenario the best. We have a > patch to modify some file but the base file is gone. Because > check_preimage() finds an index entry with the same old_name, it tries > to restore the on-disk base file with cached content with > checkout_target() and move on. > > If this old_name is i-t-a, before this patch "apply -3" will call > checkout_target() which will write an empty file (i-t-a entries always > have "empty blob" SHA-1), then it'll fail at verify_index_match() > (i-t-a entries are always "modified") and the operation is aborted. > > This empty file should not be created. Compare to the case where > old_name does not exist in index, "apply -3" fails with a different > error message "...: does not exist" but it does not touch > worktree. This patch makes sure the same happens to i-t-a entries. This part (unlike 3/8) is very well explained, and makes sense to me. > load_current() shares the same code pattern (look up an index entry, > if on-disk version is not found, restore using the cached > version). The "fall-back to the cached one" is very much deliberate to allow us to work with an empty working tree as long as the index is correct, so this perhaps makes sense. If the user said "add -N F", and F is involved in the patch application, we do not want to consider F exists in the index and we would instead want to pretend that we do not have F ourselves. It does not even make sense to do "apply -3" for such a path, and rejecting with "does not exist" is a good thing to do. Good. > Fix it too (even though it's not exercised in any test case) Hmm, as this is adding new test for the other case, perhaps this case is covered by another one, too? > Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com> > --- > builtin/apply.c | 4 ++-- > t/t2203-add-intent.sh | 16 ++++++++++++++++ > 2 files changed, 18 insertions(+), 2 deletions(-) > > diff --git a/builtin/apply.c b/builtin/apply.c > index 76b58a1..b185c97 100644 > --- a/builtin/apply.c > +++ b/builtin/apply.c > @@ -3348,7 +3348,7 @@ static int load_current(struct image *image, struct patch *patch) > die("BUG: patch to %s is not a creation", patch->old_name); > > pos = cache_name_pos(name, strlen(name)); > - if (pos < 0) > + if (pos < 0 || ce_intent_to_add(active_cache[pos])) > return error(_("%s: does not exist in index"), name); > ce = active_cache[pos]; > if (lstat(name, &st)) { > @@ -3501,7 +3501,7 @@ static int check_preimage(struct patch *patch, struct cache_entry **ce, struct s > > if (check_index && !previous) { > int pos = cache_name_pos(old_name, strlen(old_name)); > - if (pos < 0) { > + if (pos < 0 || ce_intent_to_add(active_cache[pos])) { > if (patch->is_new < 0) > goto is_new; > return error(_("%s: does not exist in index"), old_name); > diff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh > index bb5ef2b..96c8755 100755 > --- a/t/t2203-add-intent.sh > +++ b/t/t2203-add-intent.sh > @@ -95,5 +95,21 @@ test_expect_success 'apply adds new file on i-t-a entry' ' > ) > ' > > +test_expect_success 'apply:check_preimage() not creating empty file' ' > + git init check-preimage && > + ( > + cd check-preimage && > + echo oldcontent >newfile && > + git add newfile && > + echo newcontent >newfile && > + git diff >patch && > + rm .git/index && > + git add -N newfile && > + rm newfile && > + test_must_fail git apply -3 patch && > + ! test -f newfile > + ) > +' > + > test_done ^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH 5/8] checkout(-index): do not checkout i-t-a entries 2015-08-22 1:08 ` [PATCH 0/8] Resurrect "diff-lib.c: adjust position of i-t-a entries in diff" Nguyễn Thái Ngọc Duy ` (3 preceding siblings ...) 2015-08-22 1:08 ` [PATCH 4/8] apply: make sure check_preimage() does not leave empty file on error Nguyễn Thái Ngọc Duy @ 2015-08-22 1:08 ` Nguyễn Thái Ngọc Duy 2015-08-25 17:36 ` Junio C Hamano 2015-08-22 1:08 ` [PATCH 6/8] grep: make it clear i-t-a entries are ignored Nguyễn Thái Ngọc Duy 5 siblings, 1 reply; 28+ messages in thread From: Nguyễn Thái Ngọc Duy @ 2015-08-22 1:08 UTC (permalink / raw) To: git Cc: Junio C Hamano, phiggins, snoksrud, Nguyễn Thái Ngọc Duy The cached blob of i-t-a entries are empty blob. By checkout, we delete the content we have. Don't do it. This is done higher up instead of inside checkout_entry() because we would have limited options in there: silently ignore, loudly ignore, die. At higher level we can do better reporting. For example, "git checkout -- foo" will complain that "foo" does not match pathspec, just like when "foo" is not registered with "git add -N" Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com> --- builtin/checkout-index.c | 5 ++++- builtin/checkout.c | 2 ++ t/t2203-add-intent.sh | 34 ++++++++++++++++++++++++++++++++++ 3 files changed, 40 insertions(+), 1 deletion(-) diff --git a/builtin/checkout-index.c b/builtin/checkout-index.c index 8028c37..eca975d 100644 --- a/builtin/checkout-index.c +++ b/builtin/checkout-index.c @@ -56,7 +56,8 @@ static int checkout_file(const char *name, const char *prefix) while (pos < active_nr) { struct cache_entry *ce = active_cache[pos]; if (ce_namelen(ce) != namelen || - memcmp(ce->name, name, namelen)) + memcmp(ce->name, name, namelen) || + ce_intent_to_add(ce)) break; has_same_name = 1; pos++; @@ -99,6 +100,8 @@ static void checkout_all(const char *prefix, int prefix_length) if (ce_stage(ce) != checkout_stage && (CHECKOUT_ALL != checkout_stage || !ce_stage(ce))) continue; + if (ce_intent_to_add(ce)) + continue; if (prefix && *prefix && (ce_namelen(ce) <= prefix_length || memcmp(prefix, ce->name, prefix_length))) diff --git a/builtin/checkout.c b/builtin/checkout.c index e1403be..02889d4 100644 --- a/builtin/checkout.c +++ b/builtin/checkout.c @@ -300,6 +300,8 @@ static int checkout_paths(const struct checkout_opts *opts, * anything to this entry at all. */ continue; + if (ce_intent_to_add(ce)) + continue; /* * Either this entry came from the tree-ish we are * checking the paths out of, or we are checking out diff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh index 96c8755..d0f36a4 100755 --- a/t/t2203-add-intent.sh +++ b/t/t2203-add-intent.sh @@ -111,5 +111,39 @@ test_expect_success 'apply:check_preimage() not creating empty file' ' ) ' +test_expect_success 'checkout ignores i-t-a' ' + git init checkout && + ( + cd checkout && + echo data >file && + git add -N file && + test_must_fail git checkout -- file && + echo data >expected && + test_cmp expected file + ) +' + +test_expect_success 'checkout-index ignores i-t-a' ' + ( + cd checkout && + git checkout-index file && + echo data >expected && + test_cmp expected file + ) +' + +test_expect_success 'checkout-index --all ignores i-t-a' ' + ( + cd checkout && + echo data >anotherfile && + git add anotherfile && + rm anotherfile && + git checkout-index --all && + echo data >expected && + test_cmp expected file && + test_cmp expected anotherfile + ) +' + test_done -- 2.3.0.rc1.137.g477eb31 ^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: [PATCH 5/8] checkout(-index): do not checkout i-t-a entries 2015-08-22 1:08 ` [PATCH 5/8] checkout(-index): do not checkout i-t-a entries Nguyễn Thái Ngọc Duy @ 2015-08-25 17:36 ` Junio C Hamano 2015-11-29 15:31 ` Duy Nguyen 0 siblings, 1 reply; 28+ messages in thread From: Junio C Hamano @ 2015-08-25 17:36 UTC (permalink / raw) To: Nguyễn Thái Ngọc Duy; +Cc: git, phiggins, snoksrud Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes: > The cached blob of i-t-a entries are empty blob. By checkout, we delete > the content we have. Don't do it. > > This is done higher up instead of inside checkout_entry() because we > would have limited options in there: silently ignore, loudly ignore, > die. At higher level we can do better reporting. For example, "git > checkout -- foo" will complain that "foo" does not match pathspec, just > like when "foo" is not registered with "git add -N" Hmm, I am not sure about the example, though. The user _told_ Git that 'foo' is a path she cares about so "does not match pathspec" is a very unfriendly thing to say. "foo does not exist in the index hence we cannot check it out" is a very good thing to say, though (and of course, I agree that not touching the working tree is a good thing to do). > Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com> > --- > builtin/checkout-index.c | 5 ++++- > builtin/checkout.c | 2 ++ > t/t2203-add-intent.sh | 34 ++++++++++++++++++++++++++++++++++ > 3 files changed, 40 insertions(+), 1 deletion(-) > > diff --git a/builtin/checkout-index.c b/builtin/checkout-index.c > index 8028c37..eca975d 100644 > --- a/builtin/checkout-index.c > +++ b/builtin/checkout-index.c > @@ -56,7 +56,8 @@ static int checkout_file(const char *name, const char *prefix) > while (pos < active_nr) { > struct cache_entry *ce = active_cache[pos]; > if (ce_namelen(ce) != namelen || > - memcmp(ce->name, name, namelen)) > + memcmp(ce->name, name, namelen) || > + ce_intent_to_add(ce)) > break; > has_same_name = 1; > pos++; 'pos' here comes from cache_name_pos(), and it is either the location an entry with the same name exists, or the location a new entry with the given name would be inserted at. When we detect that the entry at pos is different from the given name, we break out of the loop, because we _know_ that an entry with the same name cannot exist. Does the same hold for an i-t-a entry that exactly records the given name? The answer is yes, as you cannot have an unmerged i-t-a entry. If a path marked as i-t-a is in the index, no other entry with the same name would exist. So this change is good. I would actually have introduced another bool has_i_t_a to make the reporting richer, as it is your primary reason why you are touching this part of the code instead of checkout_entry(). The reporting part then would become fprintf(stderr, "git checkout-index: %s ", name); - if (!has_same_name) + if (has_i_t_a) + fprintf(stderr, "is not yet in the cache); else if (!has_same_name) fprintf(stderr, "is not in the cache"); ;-) > @@ -99,6 +100,8 @@ static void checkout_all(const char *prefix, int prefix_length) > if (ce_stage(ce) != checkout_stage > && (CHECKOUT_ALL != checkout_stage || !ce_stage(ce))) > continue; > + if (ce_intent_to_add(ce)) > + continue; This one is obviously good ;-) > diff --git a/builtin/checkout.c b/builtin/checkout.c > index e1403be..02889d4 100644 > --- a/builtin/checkout.c > +++ b/builtin/checkout.c > @@ -300,6 +300,8 @@ static int checkout_paths(const struct checkout_opts *opts, > * anything to this entry at all. > */ > continue; > + if (ce_intent_to_add(ce)) > + continue; > /* > * Either this entry came from the tree-ish we are > * checking the paths out of, or we are checking out Hmm, while this does prevent the later code from checking it out, I am not sure how well this interacts with ps_matched[] logic here. If the user told Git that 'foo' is a path that she cares about with "add -N foo", and said "git checkout -- foo", should we be somehow saying that 'foo' did match but there is nothing to check out, or something? ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 5/8] checkout(-index): do not checkout i-t-a entries 2015-08-25 17:36 ` Junio C Hamano @ 2015-11-29 15:31 ` Duy Nguyen 2015-11-30 19:17 ` Junio C Hamano 0 siblings, 1 reply; 28+ messages in thread From: Duy Nguyen @ 2015-11-29 15:31 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, phiggins, snoksrud Sorry for this waaay too late response, everything (of the series nd/ita-cleanup) is addressed so far except this.. On Tue, Aug 25, 2015 at 10:36:52AM -0700, Junio C Hamano wrote: > > diff --git a/builtin/checkout.c b/builtin/checkout.c > > index e1403be..02889d4 100644 > > --- a/builtin/checkout.c > > +++ b/builtin/checkout.c > > @@ -300,6 +300,8 @@ static int checkout_paths(const struct checkout_opts *opts, > > * anything to this entry at all. > > */ > > continue; > > + if (ce_intent_to_add(ce)) > > + continue; > > /* > > * Either this entry came from the tree-ish we are > > * checking the paths out of, or we are checking out > > Hmm, while this does prevent the later code from checking it out, I > am not sure how well this interacts with ps_matched[] logic here. > If the user told Git that 'foo' is a path that she cares about with > "add -N foo", and said "git checkout -- foo", should we be somehow > saying that 'foo' did match but there is nothing to check out, or > something? How about this? It does not mess with ps_matched logic. But it does not say "nothing to checkout" at the end either. While we could do that (in general case, not just because all we are checking out is ita entries), I'm not sure if such verbosity helps anyone. I'm thinking of dropping the new warning I added here too.. -- 8< -- diff --git a/builtin/checkout.c b/builtin/checkout.c index 3e141fc..c11fe71 100644 --- a/builtin/checkout.c +++ b/builtin/checkout.c @@ -328,12 +328,17 @@ static int checkout_paths(const struct checkout_opts *opts, if (opts->merge) unmerge_marked_index(&the_index); - /* Any unmerged paths? */ for (pos = 0; pos < active_nr; pos++) { const struct cache_entry *ce = active_cache[pos]; if (ce->ce_flags & CE_MATCHED) { - if (!ce_stage(ce)) + if (!ce_stage(ce)) { + if (ce_intent_to_add(ce)) { + warning(_("path '%s' is only intended to add"), ce->name); + ce->ce_flags &= ~CE_MATCHED; + } continue; + } + /* Any unmerged paths? */ if (opts->force) { warning(_("path '%s' is unmerged"), ce->name); } else if (opts->writeout_stage) { -- 8< -- -- Duy ^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: [PATCH 5/8] checkout(-index): do not checkout i-t-a entries 2015-11-29 15:31 ` Duy Nguyen @ 2015-11-30 19:17 ` Junio C Hamano 0 siblings, 0 replies; 28+ messages in thread From: Junio C Hamano @ 2015-11-30 19:17 UTC (permalink / raw) To: Duy Nguyen; +Cc: git, phiggins, snoksrud Duy Nguyen <pclouds@gmail.com> writes: > Sorry for this waaay too late response, everything (of the series > nd/ita-cleanup) is addressed so far except this.. > > On Tue, Aug 25, 2015 at 10:36:52AM -0700, Junio C Hamano wrote: >> > diff --git a/builtin/checkout.c b/builtin/checkout.c >> > index e1403be..02889d4 100644 >> > --- a/builtin/checkout.c >> > +++ b/builtin/checkout.c >> > @@ -300,6 +300,8 @@ static int checkout_paths(const struct checkout_opts *opts, >> > * anything to this entry at all. >> > */ >> > continue; >> > + if (ce_intent_to_add(ce)) >> > + continue; >> > /* >> > * Either this entry came from the tree-ish we are >> > * checking the paths out of, or we are checking out >> >> Hmm, while this does prevent the later code from checking it out, I >> am not sure how well this interacts with ps_matched[] logic here. >> If the user told Git that 'foo' is a path that she cares about with >> "add -N foo", and said "git checkout -- foo", should we be somehow >> saying that 'foo' did match but there is nothing to check out, or >> something? > > How about this? It does not mess with ps_matched logic. But it does > not say "nothing to checkout" at the end either. While we could do > that (in general case, not just because all we are checking out is ita > entries), I'm not sure if such verbosity helps anyone. I'm thinking of > dropping the new warning I added here too.. I agree that these warnings are unwanted when you run "checkout ." in a repository with tons of i-t-a paths (but on the other hand, having tons of i-t-a paths is unusual so the user might want to be reminded of them--I dunno). With or without the new warning(), this one looks an improvement over the previous one to me. Thanks. > -- 8< -- > diff --git a/builtin/checkout.c b/builtin/checkout.c > index 3e141fc..c11fe71 100644 > --- a/builtin/checkout.c > +++ b/builtin/checkout.c > @@ -328,12 +328,17 @@ static int checkout_paths(const struct checkout_opts *opts, > if (opts->merge) > unmerge_marked_index(&the_index); > > - /* Any unmerged paths? */ > for (pos = 0; pos < active_nr; pos++) { > const struct cache_entry *ce = active_cache[pos]; > if (ce->ce_flags & CE_MATCHED) { > - if (!ce_stage(ce)) > + if (!ce_stage(ce)) { > + if (ce_intent_to_add(ce)) { > + warning(_("path '%s' is only intended to add"), ce->name); > + ce->ce_flags &= ~CE_MATCHED; > + } > continue; > + } > + /* Any unmerged paths? */ > if (opts->force) { > warning(_("path '%s' is unmerged"), ce->name); > } else if (opts->writeout_stage) { > -- 8< -- > -- > Duy ^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH 6/8] grep: make it clear i-t-a entries are ignored 2015-08-22 1:08 ` [PATCH 0/8] Resurrect "diff-lib.c: adjust position of i-t-a entries in diff" Nguyễn Thái Ngọc Duy ` (4 preceding siblings ...) 2015-08-22 1:08 ` [PATCH 5/8] checkout(-index): do not checkout i-t-a entries Nguyễn Thái Ngọc Duy @ 2015-08-22 1:08 ` Nguyễn Thái Ngọc Duy 2015-08-22 1:11 ` [PATCH 7/8] diff.h: extend "flags" field to 64 bits because we're out of bits Nguyễn Thái Ngọc Duy ` (2 more replies) 5 siblings, 3 replies; 28+ messages in thread From: Nguyễn Thái Ngọc Duy @ 2015-08-22 1:08 UTC (permalink / raw) To: git Cc: Junio C Hamano, phiggins, snoksrud, Nguyễn Thái Ngọc Duy The expression "!S_ISREG(ce)" covers i-t-a entries as well because ce->ce_mode would be zero then. I could make a comment saying that, but it's probably better just to comment with code, in case i-t-a entry content changes in future. Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com> --- builtin/grep.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/builtin/grep.c b/builtin/grep.c index d04f440..f508179 100644 --- a/builtin/grep.c +++ b/builtin/grep.c @@ -375,7 +375,7 @@ static int grep_cache(struct grep_opt *opt, const struct pathspec *pathspec, int for (nr = 0; nr < active_nr; nr++) { const struct cache_entry *ce = active_cache[nr]; - if (!S_ISREG(ce->ce_mode)) + if (!S_ISREG(ce->ce_mode) || ce_intent_to_add(ce)) continue; if (!ce_path_match(ce, pathspec, NULL)) continue; -- 2.3.0.rc1.137.g477eb31 ^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH 7/8] diff.h: extend "flags" field to 64 bits because we're out of bits 2015-08-22 1:08 ` [PATCH 6/8] grep: make it clear i-t-a entries are ignored Nguyễn Thái Ngọc Duy @ 2015-08-22 1:11 ` Nguyễn Thái Ngọc Duy 2015-08-25 17:39 ` Junio C Hamano 2015-08-25 17:37 ` [PATCH 6/8] grep: make it clear i-t-a entries are ignored Junio C Hamano 2015-08-25 17:37 ` Junio C Hamano 2 siblings, 1 reply; 28+ messages in thread From: Nguyễn Thái Ngọc Duy @ 2015-08-22 1:11 UTC (permalink / raw) To: git Cc: Junio C Hamano, plamen.totev, l.s.r, Eric Sunshine, tboegi, Nguyễn Thái Ngọc Duy I renamed both "flags" and "touched_flags" fields while making this patch to make sure I was aware of how these flags were manipulated (besides DIFF_OPT* macros). So hopefully I didn't miss anything. Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com> --- builtin/commit.c | 2 +- diff-lib.c | 4 ++-- diff.c | 2 +- diff.h | 8 +++++--- 4 files changed, 9 insertions(+), 7 deletions(-) diff --git a/builtin/commit.c b/builtin/commit.c index 4cbd5ff..95f7296 100644 --- a/builtin/commit.c +++ b/builtin/commit.c @@ -895,7 +895,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix, * submodules which were manually staged, which would * be really confusing. */ - int diff_flags = DIFF_OPT_OVERRIDE_SUBMODULE_CONFIG; + diff_flags_t diff_flags = DIFF_OPT_OVERRIDE_SUBMODULE_CONFIG; if (ignore_submodule_arg && !strcmp(ignore_submodule_arg, "all")) diff_flags |= DIFF_OPT_IGNORE_SUBMODULES; diff --git a/diff-lib.c b/diff-lib.c index 241a843..ae09034 100644 --- a/diff-lib.c +++ b/diff-lib.c @@ -71,7 +71,7 @@ static int match_stat_with_submodule(struct diff_options *diffopt, { int changed = ce_match_stat(ce, st, ce_option); if (S_ISGITLINK(ce->ce_mode)) { - unsigned orig_flags = diffopt->flags; + diff_flags_t orig_flags = diffopt->flags; if (!DIFF_OPT_TST(diffopt, OVERRIDE_SUBMODULE_CONFIG)) set_diffopt_flags_from_submodule_config(diffopt, ce->name); if (DIFF_OPT_TST(diffopt, IGNORE_SUBMODULES)) @@ -516,7 +516,7 @@ int do_diff_cache(const unsigned char *tree_sha1, struct diff_options *opt) return 0; } -int index_differs_from(const char *def, int diff_flags) +int index_differs_from(const char *def, diff_flags_t diff_flags) { struct rev_info rev; struct setup_revision_opt opt; diff --git a/diff.c b/diff.c index 7deac90..2485870 100644 --- a/diff.c +++ b/diff.c @@ -4912,7 +4912,7 @@ int diff_can_quit_early(struct diff_options *opt) static int is_submodule_ignored(const char *path, struct diff_options *options) { int ignored = 0; - unsigned orig_flags = options->flags; + diff_flags_t orig_flags = options->flags; if (!DIFF_OPT_TST(options, OVERRIDE_SUBMODULE_CONFIG)) set_diffopt_flags_from_submodule_config(options, path); if (DIFF_OPT_TST(options, IGNORE_SUBMODULES)) diff --git a/diff.h b/diff.h index f7208ad..4241aa5 100644 --- a/diff.h +++ b/diff.h @@ -110,13 +110,15 @@ enum diff_words_type { DIFF_WORDS_COLOR }; +typedef uint64_t diff_flags_t; + struct diff_options { const char *orderfile; const char *pickaxe; const char *single_follow; const char *a_prefix, *b_prefix; - unsigned flags; - unsigned touched_flags; + diff_flags_t flags; + diff_flags_t touched_flags; /* diff-filter bits */ unsigned int filter; @@ -347,7 +349,7 @@ extern int diff_result_code(struct diff_options *, int); extern void diff_no_index(struct rev_info *, int, const char **, const char *); -extern int index_differs_from(const char *def, int diff_flags); +extern int index_differs_from(const char *def, diff_flags_t diff_flags); extern size_t fill_textconv(struct userdiff_driver *driver, struct diff_filespec *df, -- 2.3.0.rc1.137.g477eb31 ^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: [PATCH 7/8] diff.h: extend "flags" field to 64 bits because we're out of bits 2015-08-22 1:11 ` [PATCH 7/8] diff.h: extend "flags" field to 64 bits because we're out of bits Nguyễn Thái Ngọc Duy @ 2015-08-25 17:39 ` Junio C Hamano 2015-08-31 10:22 ` Duy Nguyen 0 siblings, 1 reply; 28+ messages in thread From: Junio C Hamano @ 2015-08-25 17:39 UTC (permalink / raw) To: Nguyễn Thái Ngọc Duy Cc: git, plamen.totev, l.s.r, Eric Sunshine, tboegi Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes: > I renamed both "flags" and "touched_flags" fields while making this > patch to make sure I was aware of how these flags were manipulated > (besides DIFF_OPT* macros). So hopefully I didn't miss anything. It is a bad taste to use user_defined_t typedef (I think it actually is a standard violation), isn't it? The diff-struct is not like objects where we need million copies of in-core while running. What do you need many more flags for? ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 7/8] diff.h: extend "flags" field to 64 bits because we're out of bits 2015-08-25 17:39 ` Junio C Hamano @ 2015-08-31 10:22 ` Duy Nguyen 0 siblings, 0 replies; 28+ messages in thread From: Duy Nguyen @ 2015-08-31 10:22 UTC (permalink / raw) To: Junio C Hamano Cc: Git Mailing List, Plamen Totev, René Scharfe, Eric Sunshine, Torsten Bögershausen On Wed, Aug 26, 2015 at 12:39 AM, Junio C Hamano <gitster@pobox.com> wrote: > Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes: > >> I renamed both "flags" and "touched_flags" fields while making this >> patch to make sure I was aware of how these flags were manipulated >> (besides DIFF_OPT* macros). So hopefully I didn't miss anything. > > It is a bad taste to use user_defined_t typedef (I think it actually > is a standard violation), isn't it? Yeah I think you posted a patch somewhere updating CodingGuidelines about this.. > The diff-struct is not like objects where we need million copies of > in-core while running. What do you need many more flags for? We already use all 32 bit flags and I need one more flag. I guess I go with flags because it's how we add features in diff struct. Adding a new field instead of extending flags could be dangerous: elsewhere people copy flags out to a temporary place, do something then restore. If it's a separate field, it's left in place and bad things could happen. -- Duy ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 6/8] grep: make it clear i-t-a entries are ignored 2015-08-22 1:08 ` [PATCH 6/8] grep: make it clear i-t-a entries are ignored Nguyễn Thái Ngọc Duy 2015-08-22 1:11 ` [PATCH 7/8] diff.h: extend "flags" field to 64 bits because we're out of bits Nguyễn Thái Ngọc Duy @ 2015-08-25 17:37 ` Junio C Hamano 2015-08-25 17:37 ` Junio C Hamano 2 siblings, 0 replies; 28+ messages in thread From: Junio C Hamano @ 2015-08-25 17:37 UTC (permalink / raw) To: Nguyễn Thái Ngọc Duy; +Cc: git, phiggins, snoksrud Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes: > The expression "!S_ISREG(ce)" covers i-t-a entries as well because > ce->ce_mode would be zero then. I could make a comment saying that, but > it's probably better just to comment with code, in case i-t-a entry > content changes in future. OK. Thansk. > Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com> > --- > builtin/grep.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/builtin/grep.c b/builtin/grep.c > index d04f440..f508179 100644 > --- a/builtin/grep.c > +++ b/builtin/grep.c > @@ -375,7 +375,7 @@ static int grep_cache(struct grep_opt *opt, const struct pathspec *pathspec, int > > for (nr = 0; nr < active_nr; nr++) { > const struct cache_entry *ce = active_cache[nr]; > - if (!S_ISREG(ce->ce_mode)) > + if (!S_ISREG(ce->ce_mode) || ce_intent_to_add(ce)) > continue; > if (!ce_path_match(ce, pathspec, NULL)) > continue; ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 6/8] grep: make it clear i-t-a entries are ignored 2015-08-22 1:08 ` [PATCH 6/8] grep: make it clear i-t-a entries are ignored Nguyễn Thái Ngọc Duy 2015-08-22 1:11 ` [PATCH 7/8] diff.h: extend "flags" field to 64 bits because we're out of bits Nguyễn Thái Ngọc Duy 2015-08-25 17:37 ` [PATCH 6/8] grep: make it clear i-t-a entries are ignored Junio C Hamano @ 2015-08-25 17:37 ` Junio C Hamano 2 siblings, 0 replies; 28+ messages in thread From: Junio C Hamano @ 2015-08-25 17:37 UTC (permalink / raw) To: Nguyễn Thái Ngọc Duy; +Cc: git, phiggins, snoksrud Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes: > The expression "!S_ISREG(ce)" covers i-t-a entries as well because > ce->ce_mode would be zero then. I could make a comment saying that, but > it's probably better just to comment with code, in case i-t-a entry > content changes in future. OK. Thanks. > Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com> > --- > builtin/grep.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/builtin/grep.c b/builtin/grep.c > index d04f440..f508179 100644 > --- a/builtin/grep.c > +++ b/builtin/grep.c > @@ -375,7 +375,7 @@ static int grep_cache(struct grep_opt *opt, const struct pathspec *pathspec, int > > for (nr = 0; nr < active_nr; nr++) { > const struct cache_entry *ce = active_cache[nr]; > - if (!S_ISREG(ce->ce_mode)) > + if (!S_ISREG(ce->ce_mode) || ce_intent_to_add(ce)) > continue; > if (!ce_path_match(ce, pathspec, NULL)) > continue; ^ permalink raw reply [flat|nested] 28+ messages in thread
end of thread, other threads:[~2015-11-30 19:17 UTC | newest] Thread overview: 28+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-06-22 14:29 apply --cached --whitespace=fix now failing on items added with "add -N" Patrick Higgins 2015-06-22 14:45 ` Duy Nguyen 2015-06-22 17:06 ` Junio C Hamano 2015-06-23 12:34 ` [PATCH] apply: fix adding new files on i-t-a entries Nguyễn Thái Ngọc Duy 2015-06-23 16:50 ` Junio C Hamano 2015-06-23 17:37 ` Junio C Hamano 2015-06-24 4:48 ` Junio C Hamano 2015-06-24 10:11 ` Duy Nguyen 2015-06-24 17:05 ` Junio C Hamano 2015-06-25 12:26 ` Duy Nguyen 2015-06-25 13:07 ` Junio C Hamano 2015-08-22 1:08 ` [PATCH 0/8] Resurrect "diff-lib.c: adjust position of i-t-a entries in diff" Nguyễn Thái Ngọc Duy 2015-08-22 1:08 ` [PATCH 1/8] blame: remove obsolete comment Nguyễn Thái Ngọc Duy 2015-08-22 1:08 ` [PATCH 2/8] Add and use convenient macro ce_intent_to_add() Nguyễn Thái Ngọc Duy 2015-08-22 1:08 ` [PATCH 3/8] apply: fix adding new files on i-t-a entries Nguyễn Thái Ngọc Duy 2015-08-25 17:01 ` Junio C Hamano 2015-08-22 1:08 ` [PATCH 4/8] apply: make sure check_preimage() does not leave empty file on error Nguyễn Thái Ngọc Duy 2015-08-25 17:16 ` Junio C Hamano 2015-08-22 1:08 ` [PATCH 5/8] checkout(-index): do not checkout i-t-a entries Nguyễn Thái Ngọc Duy 2015-08-25 17:36 ` Junio C Hamano 2015-11-29 15:31 ` Duy Nguyen 2015-11-30 19:17 ` Junio C Hamano 2015-08-22 1:08 ` [PATCH 6/8] grep: make it clear i-t-a entries are ignored Nguyễn Thái Ngọc Duy 2015-08-22 1:11 ` [PATCH 7/8] diff.h: extend "flags" field to 64 bits because we're out of bits Nguyễn Thái Ngọc Duy 2015-08-25 17:39 ` Junio C Hamano 2015-08-31 10:22 ` Duy Nguyen 2015-08-25 17:37 ` [PATCH 6/8] grep: make it clear i-t-a entries are ignored Junio C Hamano 2015-08-25 17:37 ` Junio C Hamano
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.