Allow empty, new files to be staged via git checkout -p

[dscho]

See https://lore.kernel.org/git/20200527075648.GA4006373@coredump.intra.peff.net for the report.

For ease of backporting, this patch is based on js/add-p-leftover-bits.

Cc: Merlin Büge toni@bluenox07.de, Jeff King peff@peff.net

[dscho]

/submit

[gitgitgadget]

Submitted as pull.646.git.1590613746507.gitgitgadget@gmail.com

[gitgitgadget]

On the Git mailing list, Junio C Hamano wrote (reply to this):

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> This also fixes the problem where added _empty_ files could not be
> staged via `git checkout -p`.

Nice.  Will queue.

>     For ease of backporting, this patch is based on js/add-p-leftover-bits.

Yup, I agree that it would be a sensible fork point.

[gitgitgadget]

This branch is now known as js/checkout-p-new-file.

[gitgitgadget]

This patch series was integrated into pu via git@b240e97.

[gitgitgadget]

On the Git mailing list, Jeff King wrote (reply to this):

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

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

> 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

[gitgitgadget]

On the Git mailing list, Johannes Schindelin wrote (reply to this):

Hi Peff,

On Wed, 27 May 2020, Jeff King wrote:

> On Wed, May 27, 2020 at 09:09:06PM +0000, Johannes Schindelin via GitGitGadget wrote:
>
> > 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.

As long as there is an escape hatch, I try to keep it working.

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

Thank you!

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

Indeed. Maybe I can leave that as #leftoverbits?

Ciao,
Dscho

>
>   $ 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
>

[gitgitgadget]

On the Git mailing list, Johannes Schindelin wrote (reply to this):

Hi,

On Wed, 27 May 2020, Johannes Schindelin wrote:

> On Wed, 27 May 2020, Jeff King wrote:
>
> > 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.
>
> Indeed. Maybe I can leave that as #leftoverbits?

Added as https://github.com/gitgitgadget/git/issues/647

Ciao,
Dscho

[gitgitgadget]

This patch series was integrated into pu via git@d8bdbce.

[gitgitgadget]

This patch series was integrated into pu via git@ec7bea8.

[gitgitgadget]

This patch series was integrated into next via git@017c325.

[gitgitgadget]

This patch series was integrated into pu via git@1635944.

[gitgitgadget]

This patch series was integrated into pu via git@7101793.

[gitgitgadget]

This patch series was integrated into pu via git@2bdf00e.

[gitgitgadget]

This patch series was integrated into next via git@2bdf00e.

[gitgitgadget]

This patch series was integrated into master via git@2bdf00e.

[gitgitgadget]

Closed via 2bdf00e.