| Message ID | 20200806060119.74587-3-ray@ameretat.dev (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | apply: handle i-t-a entries in index | expand |
"Raymond E. Pasco" <ray@ameretat.dev> writes: > 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 | 15 +++++++++++++++ > 1 file changed, 15 insertions(+) At first glance, it feels somewhat sad that this check is not done in check_preimage(); after all, the caller of check_preimage() feeds it to all kind of patches, without excluding path creation, so the helper should be allowed to say "heh, you are trying to create path F with this patch, but there already is F in the index", "you are renaming into F but there is F already", etc. But ensuring the state of the path to be patched is done by check_to_create() for paths that are to be created, even before this patch, because it simply needs to do different checks from what is done on modification patch, and also because we need to resolve patch->is_new bit for non-git patches before being able to perform the checks done in check_to_create(). So I agree with the location of this additonal logic. It is somewhat unsatisfactory that we need to do the same index_name_pos probing twice. I wonder if we somehow can consolidate them? Perhaps something along this line, instead of this patch? 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 Thu Aug 6, 2020 at 5:00 PM EDT, Junio C Hamano wrote: > At first glance, it feels somewhat sad that this check is not done > in check_preimage(); after all, the caller of check_preimage() feeds > it to all kind of patches, without excluding path creation, so the > helper should be allowed to say "heh, you are trying to create path > F with this patch, but there already is F in the index", "you are > renaming into F but there is F already", etc. I spent some time trying to put it in there before deciding it was better off in check_to_create(). > It is somewhat unsatisfactory that we need to do the same > index_name_pos probing twice. I wonder if we somehow can > consolidate them? > > Perhaps something along this line, instead of this patch? I think this logic can be consolidated and still readable, yeah. I'll send a patch.
diff --git a/apply.c b/apply.c index 4cba4ce71a..6328591489 100644 --- a/apply.c +++ b/apply.c @@ -3754,6 +3754,21 @@ static int check_to_create(struct apply_state *state, return EXISTS_IN_INDEX; } + /* If the new path to be added has an intent-to-add entry, then + * by definition it does not match what is in the work tree. So + * "apply --index" should always fail in this case. Patches other + * than creation patches are already held to this standard by + * check_preimage() calling verify_index_match(). + */ + if (state->check_index && !state->cached) { + 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 error(_("%s: does not match index"), new_name); + } + + if (state->cached) return 0;
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 | 15 +++++++++++++++ 1 file changed, 15 insertions(+)