[v5,2/3] apply: make i-t-a entries never match worktree
diff mbox series

Message ID 20200808074959.35943-3-ray@ameretat.dev
State New
Headers show
Series
  • apply: handle i-t-a entries in index
Related show

Commit Message

Raymond E. Pasco Aug. 8, 2020, 7:49 a.m. UTC
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(-)

Comments

Phillip Wood Aug. 8, 2020, 1:46 p.m. UTC | #1
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);
>
Raymond E. Pasco Aug. 8, 2020, 2:07 p.m. UTC | #2
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.)
Phillip Wood Aug. 8, 2020, 3:48 p.m. UTC | #3
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.)
>
Raymond E. Pasco Aug. 8, 2020, 3:58 p.m. UTC | #4
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.
Phillip Wood Aug. 9, 2020, 3:25 p.m. UTC | #5
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
Junio C Hamano Aug. 9, 2020, 5:58 p.m. UTC | #6
"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.

Patch
diff mbox series

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);