apply: Allow "new file" patches on i-t-a entries
diff mbox series

Message ID 20200804163320.61167-1-ray@ameretat.dev
State New
Headers show
Series
  • apply: Allow "new file" patches on i-t-a entries
Related show

Commit Message

Raymond E. Pasco Aug. 4, 2020, 4:33 p.m. UTC
diff-files recently changed to treat "intent to add" entries as new file
diffs rather than diffs from the empty blob. However, apply refuses to
apply new file diffs on top of existing index entries, except in the
case of renames. This causes "git add -p", which uses apply, to fail
when attempting to stage hunks from a file when intent to add has been
recorded.

This adds an additional check to check_to_create() which tests if the
CE_INTENT_TO_ADD flag is set on an existing index entry, and allows the
apply to proceed if so.
---
cf. <5BDF4B85-7AC1-495F-85C3-D429E3E51106@gmail.com>
 apply.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

Comments

Junio C Hamano Aug. 4, 2020, 7:30 p.m. UTC | #1
"Raymond E. Pasco" <ray@ameretat.dev> writes:

> Subject: Re: [PATCH] apply: Allow "new file" patches on i-t-a entries

Please downcase "A"llow.

> diff-files recently changed to treat "intent to add" entries as new file
> diffs rather than diffs from the empty blob. However, apply refuses to
> apply new file diffs on top of existing index entries, except in the
> case of renames. This causes "git add -p", which uses apply, to fail
> when attempting to stage hunks from a file when intent to add has been
> recorded.
>
> This adds an additional check to check_to_create() which tests if the
> CE_INTENT_TO_ADD flag is set on an existing index entry, and allows the
> apply to proceed if so.
> ---

Please sign-off your patch (see Documentation/SubmittingPatches)

> cf. <5BDF4B85-7AC1-495F-85C3-D429E3E51106@gmail.com>
>  apply.c | 18 ++++++++++++++----
>  1 file changed, 14 insertions(+), 4 deletions(-)
>
> diff --git a/apply.c b/apply.c
> index 8bff604dbe..b31bd0e866 100644
> --- a/apply.c
> +++ b/apply.c
> @@ -3747,10 +3747,20 @@ static int check_to_create(struct apply_state *state,
>  {
>  	struct stat nst;
>  
> -	if (state->check_index &&
> -	    index_name_pos(state->repo->index, new_name, strlen(new_name)) >= 0 &&
> -	    !ok_if_exists)
> -		return EXISTS_IN_INDEX;
> +	if (state->check_index) {
> +		struct cache_entry *ce = NULL;
> +		int intent_to_add;
> +		int pos = index_name_pos(state->repo->index, new_name, strlen(new_name));
> +		if (pos >= 0)
> +			ce = state->repo->index->cache[pos];
> +		if (ce && (ce->ce_flags & CE_INTENT_TO_ADD))
> +			intent_to_add = 1;
> +		else
> +			intent_to_add = 0;
> +		if (pos >= 0 && !intent_to_add && !ok_if_exists)
> +			return EXISTS_IN_INDEX;
> +	}
> +

I think the new logic looks sound.  When we are applying a patch
that adds a new path, we do not want the path to already exist, so
we used to see if there is an existign cache entry with that name
and barfed if there is.  The spirit of the new code is the same,
except that we want to treat an i-t-a entry as "not yet exist".

How often do we pass ok_if_exists, I have to wonder.  If it is often
enough, then we can check that first way before we even check to see
if a cache entry for the path even exists or what its i-t-a flag
says.  Something along the lines of this untested code:

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

That is, only if we are told to make sure the path does not already exist,
we see if the path is in the index, and if the cache entry for the
path in the index is a real entry (as opposed to i-t-a aka "not
added yet"), we complain.  Otherwise we'd happily take the patch.

Whether ok_if_exists is frequently used or not, the resulting code
may be easier to understand, but I am of course biased, as I just
wrote it ;-)

Hmm?

Thanks.

>  	if (state->cached)
>  		return 0;
Raymond E. Pasco Aug. 4, 2020, 8:59 p.m. UTC | #2
On Tue Aug 4, 2020 at 3:30 PM EDT, Junio C Hamano wrote:
> How often do we pass ok_if_exists, I have to wonder. If it is often
> enough, then we can check that first way before we even check to see
> if a cache entry for the path even exists or what its i-t-a flag
> says. Something along the lines of this untested code:
>
> 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;
> }
>
> That is, only if we are told to make sure the path does not already
> exist,
> we see if the path is in the index, and if the cache entry for the
> path in the index is a real entry (as opposed to i-t-a aka "not
> added yet"), we complain. Otherwise we'd happily take the patch.
>
> Whether ok_if_exists is frequently used or not, the resulting code
> may be easier to understand, but I am of course biased, as I just
> wrote it ;-)

ok_if_exists gets passed in cases where a real entry does exist but
we're okay with a new file diff anyway due to other patches in the set
being applied making it valid (type-change diffs and rename diffs) - for
this reason, I didn't pass ok_if_exists, but instead checked here. I
think we're in agreement on this and your logic makes sense in that
light. I'll send an updated patch.

Patch
diff mbox series

diff --git a/apply.c b/apply.c
index 8bff604dbe..b31bd0e866 100644
--- a/apply.c
+++ b/apply.c
@@ -3747,10 +3747,20 @@  static int check_to_create(struct apply_state *state,
 {
 	struct stat nst;
 
-	if (state->check_index &&
-	    index_name_pos(state->repo->index, new_name, strlen(new_name)) >= 0 &&
-	    !ok_if_exists)
-		return EXISTS_IN_INDEX;
+	if (state->check_index) {
+		struct cache_entry *ce = NULL;
+		int intent_to_add;
+		int pos = index_name_pos(state->repo->index, new_name, strlen(new_name));
+		if (pos >= 0)
+			ce = state->repo->index->cache[pos];
+		if (ce && (ce->ce_flags & CE_INTENT_TO_ADD))
+			intent_to_add = 1;
+		else
+			intent_to_add = 0;
+		if (pos >= 0 && !intent_to_add && !ok_if_exists)
+			return EXISTS_IN_INDEX;
+	}
+
 	if (state->cached)
 		return 0;