git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: "Raymond E. Pasco" <ray@ameretat.dev>
To: "Junio C Hamano" <gitster@pobox.com>
Cc: <git@vger.kernel.org>
Subject: Re: [PATCH v2] apply: allow "new file" patches on i-t-a entries
Date: Tue, 04 Aug 2020 20:32:48 -0400	[thread overview]
Message-ID: <C4ON23BIKMVK.2ZESQJ1FB5PVA@ziyou.local> (raw)
In-Reply-To: <xmqqeeomq8dr.fsf@gitster.c.googlers.com>

On Tue Aug 4, 2020 at 7:49 PM EDT, Junio C Hamano wrote:
> How exactly does "git add -p" fail for such a patch? What operation
> does it exactly want to do ("apply --cached"???) and is it "apply"
> that is wrong, or is it "git add -p" that fails to remove the i-t-a
> entry from the index before running "git apply" that is at fault?

Yes, "add -p" uses "apply --cached". I do believe this belongs in apply,
both because all "add -p" really does is assemble things to be fed to
apply and also for the more detailed reasons below.

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

But the worktree lives in the filesystem, where there are no i-t-a
entries. So the question seems to me to be whether "no file" in the
worktree matches an i-t-a entry in the index for the purposes of "add
--index". I count a couple options here:

- Nothing on the filesystem can accurately match an i-t-a entry in the
  index, so all attempts at "apply --index" when there is an i-t-a in
  the index fail with "file: does not match index". "apply --cached",
  which "add -p" uses, applies only to the index and continues to work.
  I think I prefer this one; additionally, the comment in read-cache.c
  indicate that this is supposed to be the case already, so I just need
  to make sure this check is not skipped on "new file" patches.

- The current (as of this patch) behavior: a "new file" patch applies
  both to an i-t-a in the index, and to the lack of a file in the
  worktree. This may seem strange, but it may also seem strange that an
  identical new file patch, which can be applied either to just the
  worktree or just the index successfully, fails when applied to both at
  the same time with "apply --index". However, this is precisely what is
  done anyway by "apply --index" when there are no i-t-a entries
  involved, so I lean towards i-t-a entries never matching the worktree.

Patch for the first option in progress.

  reply	other threads:[~2020-08-05  1:37 UTC|newest]

Thread overview: 53+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-08-04 16:33 [PATCH] apply: Allow "new file" patches on i-t-a entries Raymond E. Pasco
2020-08-04 19:30 ` Junio C Hamano
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 [this message]
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=C4ON23BIKMVK.2ZESQJ1FB5PVA@ziyou.local \
    --to=ray@ameretat.dev \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    /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
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public 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).