git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: "Raymond E. Pasco" <ray@ameretat.dev>
Cc: git@vger.kernel.org
Subject: Re: [PATCH] apply: Allow "new file" patches on i-t-a entries
Date: Tue, 04 Aug 2020 12:30:09 -0700	[thread overview]
Message-ID: <xmqqlfiuryym.fsf@gitster.c.googlers.com> (raw)
In-Reply-To: <20200804163320.61167-1-ray@ameretat.dev> (Raymond E. Pasco's message of "Tue, 4 Aug 2020 12:33:20 -0400")

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

  reply	other threads:[~2020-08-04 19:30 UTC|newest]

Thread overview: 53+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-08-04 16:33 Raymond E. Pasco
2020-08-04 19:30 ` Junio C Hamano [this message]
2020-08-04 20:59   ` Raymond E. Pasco
2020-08-04 22:31   ` [PATCH v2] apply: allow " Raymond E. Pasco
2020-08-04 23:40     ` [PATCH v3] " Raymond E. Pasco
2020-08-04 23:49     ` [PATCH v2] " Junio C Hamano
2020-08-05  0:32       ` Raymond E. Pasco
2020-08-06  6:01         ` [PATCH v4 0/3] apply: handle i-t-a entries in index Raymond E. Pasco
2020-08-06  6:01           ` [PATCH v4 1/3] apply: allow "new file" patches on i-t-a entries Raymond E. Pasco
2020-08-06  6:01           ` [PATCH v4 2/3] apply: make i-t-a entries never match worktree Raymond E. Pasco
2020-08-06 21:00             ` Junio C Hamano
2020-08-06 21:47               ` Raymond E. Pasco
2020-08-06  6:01           ` [PATCH v4 3/3] t4140: test apply with i-t-a paths Raymond E. Pasco
2020-08-06 21:07             ` Junio C Hamano
2020-08-07  3:44               ` Raymond E. Pasco
2020-08-08  7:49           ` [PATCH v5 0/3] apply: handle i-t-a entries in index Raymond E. Pasco
2020-08-08  7:49             ` [PATCH v5 1/3] apply: allow "new file" patches on i-t-a entries Raymond E. Pasco
2020-08-08 13:47               ` Phillip Wood
2020-08-08  7:49             ` [PATCH v5 2/3] apply: make i-t-a entries never match worktree Raymond E. Pasco
2020-08-08 13:46               ` Phillip Wood
2020-08-08 14:07                 ` Raymond E. Pasco
2020-08-08 15:48                   ` Phillip Wood
2020-08-08 15:58                     ` Raymond E. Pasco
2020-08-09 15:25                       ` Phillip Wood
2020-08-09 17:58                       ` Junio C Hamano
2020-08-10 11:03                   ` [PATCH] git-apply.txt: correct description of --cached Raymond E. Pasco
2020-08-10 16:18                     ` Junio C Hamano
2020-08-12 13:32                       ` Phillip Wood
2020-08-12 19:23                         ` Junio C Hamano
2020-08-12 20:52                           ` Raymond E. Pasco
2020-08-12 13:59                       ` Phillip Wood
2020-08-08  7:49             ` [PATCH v5 3/3] t4140: test apply with i-t-a paths Raymond E. Pasco
2020-08-23 15:58               ` Phillip Wood
2020-08-08  7:53           ` [PATCH 1/1] diff-lib: use worktree mode in diffs from i-t-a entries Raymond E. Pasco
2020-08-08  8:48             ` Martin Ågren
2020-08-08 10:46               ` Raymond E. Pasco
2020-08-08 14:21                 ` Martin Ågren
2020-08-09 18:09             ` Junio C Hamano
2020-08-10  8:53             ` [PATCH] t4069: test diff behavior with i-t-a paths Raymond E. Pasco
2020-08-10  8:57               ` [PATCH] diff-lib: use worktree mode in diffs from i-t-a entries Raymond E. Pasco
2020-08-10  9:03               ` [RESEND PATCH v2] " Raymond E. Pasco
2020-08-10 16:22               ` [PATCH] t4069: test diff behavior with i-t-a paths Junio C Hamano
2020-08-10 16:23               ` Eric Sunshine
2020-08-10 21:47                 ` Eric Sunshine
2020-08-10 22:09                   ` Junio C Hamano
2020-08-10 22:13                     ` Eric Sunshine
2020-08-10 22:22                       ` Junio C Hamano
2020-08-10 23:02                 ` Raymond E. Pasco
2020-08-10 23:21                   ` Eric Sunshine
2020-08-11  3:29                     ` Junio C Hamano
2020-08-08  7:58           ` [HYPOTHETICAL PATCH 0/2] apply: reject modification diffs to i-t-a entries Raymond E. Pasco
2020-08-08  7:58             ` [HYPOTHETICAL PATCH 1/2] " Raymond E. Pasco
2020-08-08  7:58             ` [HYPOTHETICAL PATCH 2/2] t4140: test failure of diff from empty blob to i-t-a path Raymond E. Pasco

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqqlfiuryym.fsf@gitster.c.googlers.com \
    --to=gitster@pobox.com \
    --cc=git@vger.kernel.org \
    --cc=ray@ameretat.dev \
    --subject='Re: [PATCH] apply: Allow "new file" patches on i-t-a entries' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).