Re: [PATCH] checkout -p: handle new files correctly

From: Jeff King <peff@peff.net>
Date: May 27, 2020, 23:03 UTC
Message-ID: <20200527230357.GB546534@coredump.intra.peff.net>
In-Reply-To: <pull.646.git.1590613746507.gitgitgadget@gmail.com>

On Wed, May 27, 2020 at 09:09:06PM +0000, Johannes Schindelin via GitGitGadget wrote:

Show 5 quoted lines

> However, since the same machinery was used for `git checkout -p` &
> friends, we can see new files.
> 
> Handle this case specifically, adding a new prompt for it that is
> modeled after the `deleted file` case.

Thanks! I was planning to dig further into this topic today, and here it is all wrapped up with a bow. :)

>  add-patch.c                | 30 +++++++++++++++++++++++-------
>  git-add--interactive.perl  | 21 +++++++++++++++++++--

Ooh, you even fixed the perl version, too. I was just going to leave it in the dust and add a test that set GIT_TEST_ADD_I_USE_BUILTIN.

Both versions look good, and are similar to what I expected from looking at it last night.

Show 7 quoted lines

> The original patch selection code was written for `git add -p`, and the
> fundamental unit on which it works is a hunk.
> 
> We hacked around that to handle deletions back in 24ab81ae4d
> (add-interactive: handle deletion of empty files, 2009-10-27). But `git
> add -p` would never see a new file, since we only consider the set of
> tracked files in the index.

I lied a little with "would never see a new file". There _is_ a related case with "add -p" that might be worth thinking about: intent-to-add files.

  $ git init
  $ >empty
  $ echo content >not-empty
  $ git add -N .
  $ git add -p
  diff --git a/not-empty b/not-empty
  index e69de29..d95f3ad 100644
  --- a/not-empty
  +++ b/not-empty
  @@ -0,0 +1 @@
  +content
  (1/1) Stage this hunk [y,n,q,a,d,e,?]? n

  [no mention of empty file!]

I think the culprit here is diff-files, though, which doesn't show a patch for intent-to-add:

  $ git diff-files
  :100644 100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0000000000000000000000000000000000000000 M	empty
  :100644 100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0000000000000000000000000000000000000000 M	not-empty

  $ git diff-files -p
  diff --git a/not-empty b/not-empty
  index e69de29..d95f3ad 100644
  --- a/not-empty
  +++ b/not-empty
  @@ -0,0 +1 @@
  +content

I don't think this really intersects with the patch here at all, because diff-files is not producing "new file" lines for these entries (even for the non-empty one).

The solution _might_ be to convince diff-files to treat i-t-a entries as creations. And then with your patch here, we'd naturally do the right thing. So I don't think this needs to hold up your patch in any way, nor do we necessarily need to deal with i-t-a now. I was mostly curious how they worked, since we don't support added files. The answer is just that they don't always. ;)

-Peff