| Message ID | 20200808074959.35943-3-ray@ameretat.dev (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | e3cc41b4f939a64c74b6d4a2d59f6efe006c4e4b |
| Headers | show |
| Series | apply: handle i-t-a entries in index | expand |
Hi Raymond On 08/08/2020 08:49, Raymond E. Pasco wrote: > By definition, an intent-to-add index entry can never match the > worktree, because worktrees have no concept of intent-to-add entries. > Therefore, "apply --index" should always fail on intent-to-add paths. I'm not sure I understand the logic for this. If I run 'git add -N <path>' and <path> does not exist in the worktree what's the reason to stop a patch that creates <path> from applying? I was relieved to see from the next patch that this does not affect --cached even though the documentation says it implies --index. It might be worth mentioning that in the commit message. Also it would be easier to follow if the tests were in the same patch (this is what we usually do). How this does it affect --check? `git add -p` uses --check to verify that hunks that the user has edited still apply. It does not let the user edit the hunk for a newly added file at the moment but that is something I'm thinking of adding. Best Wishes Phillip > Because check_preimage() calls verify_index_match(), it already fails > for patches other than creation patches, which check_preimage() ignores. > This patch adds a check to check_preimage()'s rough equivalent for > creation patches, check_to_create(). > > Helped-by: Junio C Hamano <gitster@pobox.com> > Signed-off-by: Raymond E. Pasco <ray@ameretat.dev> > --- > apply.c | 24 +++++++++++++++++++----- > 1 file changed, 19 insertions(+), 5 deletions(-) > > diff --git a/apply.c b/apply.c > index 4cba4ce71a..c5ecb64102 100644 > --- a/apply.c > +++ b/apply.c > @@ -3740,6 +3740,7 @@ static int check_preimage(struct apply_state *state, > > #define EXISTS_IN_INDEX 1 > #define EXISTS_IN_WORKTREE 2 > +#define EXISTS_IN_INDEX_AS_ITA 3 > > static int check_to_create(struct apply_state *state, > const char *new_name, > @@ -3747,11 +3748,21 @@ static int check_to_create(struct apply_state *state, > { > struct stat nst; > > - if (state->check_index && !ok_if_exists) { > - int pos = index_name_pos(state->repo->index, new_name, strlen(new_name)); > - if (pos >= 0 && > - !(state->repo->index->cache[pos]->ce_flags & CE_INTENT_TO_ADD)) > - return EXISTS_IN_INDEX; > + if (state->check_index && (!ok_if_exists || !state->cached)) { > + int pos; > + > + pos = index_name_pos(state->repo->index, new_name, strlen(new_name)); > + if (pos >= 0) { > + struct cache_entry *ce = state->repo->index->cache[pos]; > + > + /* allow ITA, as they do not yet exist in the index */ > + if (!ok_if_exists && !(ce->ce_flags & CE_INTENT_TO_ADD)) > + return EXISTS_IN_INDEX; > + > + /* ITA entries can never match working tree files */ > + if (!state->cached && (ce->ce_flags & CE_INTENT_TO_ADD)) > + return EXISTS_IN_INDEX_AS_ITA; > + } > } > > if (state->cached) > @@ -3938,6 +3949,9 @@ static int check_patch(struct apply_state *state, struct patch *patch) > case EXISTS_IN_INDEX: > return error(_("%s: already exists in index"), new_name); > break; > + case EXISTS_IN_INDEX_AS_ITA: > + return error(_("%s: does not match index"), new_name); > + break; > case EXISTS_IN_WORKTREE: > return error(_("%s: already exists in working directory"), > new_name); >
On Sat Aug 8, 2020 at 9:46 AM EDT, Phillip Wood wrote: > > By definition, an intent-to-add index entry can never match the > > worktree, because worktrees have no concept of intent-to-add entries. > > Therefore, "apply --index" should always fail on intent-to-add paths. > > I'm not sure I understand the logic for this. If I run 'git add -N > <path>' and <path> does not exist in the worktree what's the reason to > stop a patch that creates <path> from applying? "apply --index" requires the index and worktree to match, and applies the same path to both to get the same result in both. I brainstormed the logic a few emails upthread, and that's what's consistent with everything else. > I was relieved to see from the next patch that this does not affect > --cached even though the documentation says it implies --index. It might > be worth mentioning that in the commit message. Also it would be easier > to follow if the tests were in the same patch (this is what we usually > do). --cached doesn't really imply --index - the docs are wrong and should be changed. If anything, --index is closer to implying --cached - but really, [no flags], --cached, and --index are three different modes with different behavior. (Just removing "this implies --index" would be sufficient to make the docs correct.) > How this does it affect --check? `git add -p` uses --check to verify > that hunks that the user has edited still apply. It does not let the > user edit the hunk for a newly added file at the moment but that is > something I'm thinking of adding. --check goes through all the same code, it just doesn't actually touch anything in the index or worktree. Splittable/editable new file patches are a logical related feature, IMO. (This is just to squash an error that shouldn't happen.)
Hi Raymond On 08/08/2020 15:07, Raymond E. Pasco wrote: > On Sat Aug 8, 2020 at 9:46 AM EDT, Phillip Wood wrote: >>> By definition, an intent-to-add index entry can never match the >>> worktree, because worktrees have no concept of intent-to-add entries. >>> Therefore, "apply --index" should always fail on intent-to-add paths. >> >> I'm not sure I understand the logic for this. If I run 'git add -N >> <path>' and <path> does not exist in the worktree what's the reason to >> stop a patch that creates <path> from applying? > > "apply --index" requires the index and worktree to match, and applies > the same path to both to get the same result in both. I brainstormed the > logic a few emails upthread, and that's what's consistent with > everything else. I had a quick scan of the earlier email and found > The index and the filesystem are both able to represent "no file" > and "a file exists" states, but the index has an additional > state (i-t-a) with no direct representation in the > worktree. By (correctly) emitting "new file" patches when > comparing a file to an i-t-a index entry, we are setting down the > rule that a "new file" patch is not merely the diff between "no > file" and "a file exists", but also the diff between i-t-a and "a > file exists". > > Similarly, "deleted file" patches are the diff between "a file > exists" and "no file exists", but they are also the diff between > i-t-a and "no file exists" - if you add -N a file and then delete > it from the worktree, "deleted file" is what git diff (correctly) > shows. As a consequence of these rules, "new file" and "deleted > file" diffs are now the only diffs that validly apply to an i-t-a > entry. So apply needs to handle them (in "--cached" mode, > anyway). If I've understood correctly an i-t-a entry in the index combined with nothing in the worktree is a deletion and that is why we don't want --index to succeed when applying a creation patch? If so an expanded explanation in the commit message to this patch would help rather than just saying 'by definition'. I'm still a bit confused as we don't count it as a deletion when using --cached or applying to the worktree. >> I was relieved to see from the next patch that this does not affect >> --cached even though the documentation says it implies --index. It might >> be worth mentioning that in the commit message. Also it would be easier >> to follow if the tests were in the same patch (this is what we usually >> do). > > --cached doesn't really imply --index - the docs are wrong and should be > changed. If anything, --index is closer to implying --cached - but > really, [no flags], --cached, and --index are three different modes with > different behavior. (Just removing "this implies --index" would be > sufficient to make the docs correct.) > >> How this does it affect --check? `git add -p` uses --check to verify >> that hunks that the user has edited still apply. It does not let the >> user edit the hunk for a newly added file at the moment but that is >> something I'm thinking of adding. > > --check goes through all the same code, The same code as --cached or --index? (I assume it's the former but wanted to be sure) Thanks Phillip >it just doesn't actually touch > anything in the index or worktree. Splittable/editable new file patches > are a logical related feature, IMO. (This is just to squash an error > that shouldn't happen.) >
On Sat Aug 8, 2020 at 11:48 AM EDT, Phillip Wood wrote: > If I've understood correctly an i-t-a entry in the index combined with > nothing in the worktree is a deletion and that is why we don't want > --index to succeed when applying a creation patch? If so an expanded > explanation in the commit message to this patch would help rather than > just saying 'by definition'. I'm still a bit confused as we don't count > it as a deletion when using --cached or applying to the worktree. Nothing that complicated - --index requires that the index and worktree be identical, and nothing that can possibly be in a worktree is identical to an i-t-a entry. > > --check goes through all the same code, > > The same code as --cached or --index? (I assume it's the former but > wanted to be sure) "--cached --check" goes through the same code paths as "--cached", "--cached --index" goes through the same code paths as "--index", "--check" goes through the same code paths as [no options]. The option just makes it skip the part where it writes things out.
Hi Raymond On 08/08/2020 16:58, Raymond E. Pasco wrote: > On Sat Aug 8, 2020 at 11:48 AM EDT, Phillip Wood wrote: >> If I've understood correctly an i-t-a entry in the index combined with >> nothing in the worktree is a deletion and that is why we don't want >> --index to succeed when applying a creation patch? If so an expanded >> explanation in the commit message to this patch would help rather than >> just saying 'by definition'. I'm still a bit confused as we don't count >> it as a deletion when using --cached or applying to the worktree. > > Nothing that complicated - --index requires that the index and worktree > be identical, and nothing that can possibly be in a worktree is > identical to an i-t-a entry. Oh, so --index requires the index and worktree to match and the worktree cannot represent i-t-a so they don't match. I'm still not totally convinced that it is useful to prevent a creation patch from applying when the path is missing from the worktree but is marked as i-t-a in the index but I guess it matches the behavior the general behavior where a patch can be successfully applied with 'apply' and 'apply --cached' but not with 'apply --index' >>> --check goes through all the same code, >> >> The same code as --cached or --index? (I assume it's the former but >> wanted to be sure) > > "--cached --check" goes through the same code paths as "--cached", > "--cached --index" goes through the same code paths as "--index", > "--check" goes through the same code paths as [no options]. The option > just makes it skip the part where it writes things out. Thanks for clarifying that Best Wishes Phillip
"Raymond E. Pasco" <ray@ameretat.dev> writes: > On Sat Aug 8, 2020 at 11:48 AM EDT, Phillip Wood wrote: >> If I've understood correctly an i-t-a entry in the index combined with >> nothing in the worktree is a deletion and that is why we don't want >> --index to succeed when applying a creation patch? If so an expanded >> explanation in the commit message to this patch would help rather than >> just saying 'by definition'. I'm still a bit confused as we don't count >> it as a deletion when using --cached or applying to the worktree. > > Nothing that complicated - --index requires that the index and worktree > be identical, and nothing that can possibly be in a worktree is > identical to an i-t-a entry. > >> > --check goes through all the same code, >> >> The same code as --cached or --index? (I assume it's the former but >> wanted to be sure) > > "--cached --check" goes through the same code paths as "--cached", > "--cached --index" goes through the same code paths as "--index", > "--check" goes through the same code paths as [no options]. The option > just makes it skip the part where it writes things out. Well explained. Thanks.
diff --git a/apply.c b/apply.c index 4cba4ce71a..c5ecb64102 100644 --- a/apply.c +++ b/apply.c @@ -3740,6 +3740,7 @@ static int check_preimage(struct apply_state *state, #define EXISTS_IN_INDEX 1 #define EXISTS_IN_WORKTREE 2 +#define EXISTS_IN_INDEX_AS_ITA 3 static int check_to_create(struct apply_state *state, const char *new_name, @@ -3747,11 +3748,21 @@ static int check_to_create(struct apply_state *state, { struct stat nst; - if (state->check_index && !ok_if_exists) { - int pos = index_name_pos(state->repo->index, new_name, strlen(new_name)); - if (pos >= 0 && - !(state->repo->index->cache[pos]->ce_flags & CE_INTENT_TO_ADD)) - return EXISTS_IN_INDEX; + if (state->check_index && (!ok_if_exists || !state->cached)) { + int pos; + + pos = index_name_pos(state->repo->index, new_name, strlen(new_name)); + if (pos >= 0) { + struct cache_entry *ce = state->repo->index->cache[pos]; + + /* allow ITA, as they do not yet exist in the index */ + if (!ok_if_exists && !(ce->ce_flags & CE_INTENT_TO_ADD)) + return EXISTS_IN_INDEX; + + /* ITA entries can never match working tree files */ + if (!state->cached && (ce->ce_flags & CE_INTENT_TO_ADD)) + return EXISTS_IN_INDEX_AS_ITA; + } } if (state->cached) @@ -3938,6 +3949,9 @@ static int check_patch(struct apply_state *state, struct patch *patch) case EXISTS_IN_INDEX: return error(_("%s: already exists in index"), new_name); break; + case EXISTS_IN_INDEX_AS_ITA: + return error(_("%s: does not match index"), new_name); + break; case EXISTS_IN_WORKTREE: return error(_("%s: already exists in working directory"), new_name);
By definition, an intent-to-add index entry can never match the worktree, because worktrees have no concept of intent-to-add entries. Therefore, "apply --index" should always fail on intent-to-add paths. Because check_preimage() calls verify_index_match(), it already fails for patches other than creation patches, which check_preimage() ignores. This patch adds a check to check_preimage()'s rough equivalent for creation patches, check_to_create(). Helped-by: Junio C Hamano <gitster@pobox.com> Signed-off-by: Raymond E. Pasco <ray@ameretat.dev> --- apply.c | 24 +++++++++++++++++++----- 1 file changed, 19 insertions(+), 5 deletions(-)