Bug report: git apply --cached --reject

Bug report: git apply --cached --reject

[Patrick Stevens]

Hi,

Below the fold is a `git bugreport`-generated report of a bug with `git
apply --cached --reject`, which I have reproduced in three different
environments. Summary: we do not correctly stage the removal of a file
if there is also an unrelated change that cannot be applied.

I don't think this behaviour is intended; in the report I give a couple
of variations which correctly do what I expected, and this one breaks
the semantics I expect, given the behaviour of those variations. I have
not tried to find the source of the bug.

Thanks,
Patrick Stevens

---

Thank you for filling out a Git bug report!
Please answer the following questions to help us understand your issue.

What did you do before the bug happened? (Steps to reproduce your issue)

Complete reproduction instructions:

```
mkdir badrepo && cd badrepo && git init
echo "start" >> file.txt && git add file.txt && git commit -m "Initial
commit"
git tag initialcommit
echo "another" >> another.txt && git add another.txt && git commit -m
"Something else"
git tag commit2
git rm another.txt && git rm file.txt && git commit -m "Remove both"
git tag commit3
git checkout initialcommit
git diff commit2 commit3 | git apply --reject --cached
```

What did you expect to happen? (Expected behavior)

We should be left detached at tag `initialcommit`, with the working copy
clean (i.e. containing only `file.txt`), and with the deletion of
`file.txt` staged.

What happened instead? (Actual behavior)

The final `git apply` correctly leaves us detached at tag
`initialcommit`, with a clean working copy, but incorrectly the deletion
of `file.txt` is not staged: the index is also clean.

What's different between what you expected and what actually happened?

The file `file.txt` should have had its deletion staged, because this is
part of the diff which could apply cleanly.

Anything else you want to add:

If I delete the `--cached` from the last line, the right thing happens:
the working copy has `file.txt` deleted in the working copy but the
deletion is not staged.
If instead I remove the `git rm another.txt`, then similarly the right
thing happens: now we end up with the deletion of `file.txt` staged (but
the working copy is correctly unchanged).

I have reproduced this using git version 2.24.1.windows.2, as well as
2.29.2 built on Windows using the WSL and invoked through WSL, as well
as from the Mac HomeBrew install where I produced this bug report.

Please review the rest of the bug report below.
You can delete any lines you don't wish to share.

[System Info]
git version:
git version 2.29.2
cpu: x86_64
no commit associated with this build
sizeof-long: 8
sizeof-size_t: 8
shell-path: /bin/sh
uname: Darwin 18.7.0 Darwin Kernel Version 18.7.0: Mon Aug 31 20:53:32
PDT 2020; root:xnu-4903.278.44~1/RELEASE_X86_64 x86_64
compiler info: clang: 11.0.0 (clang-1100.0.33.17)
libc info: no libc information available
$SHELL (typically, interactive shell): /bin/zsh


[Enabled Hooks]

Re: Bug report: git apply --cached --reject

[Junio C Hamano]

Patrick Stevens <patrick@patrickstevens.co.uk> writes:

> I don't think this behaviour is intended; in the report I give a couple
> of variations which correctly do what I expected, and this one breaks
> the semantics I expect, given the behaviour of those variations. I have
> not tried to find the source of the bug.

I do not think this is intended, either.  It certainly is not
something I would expect to see as an end user.  I somehow suspect
that when we added "--reject", we didn't even mean to make it work
with "--cached" in the first place.

A more plain-vanilla case like below, where an existing file sees a
two-hunk patch, but only one of the hunks apply cleanly,
demonstrates that the command, with an option to touch the index,
does not want to take any ...

--- >8 ------ >8 ------ >8 ------ >8 ------ >8 ------ >8 ---

seq 1 10 >file && git add file && git commit -m "1 to 10"
seq 1 10 | sed -e '/1/s/$/+/' >file
git diff --stat -p >patch

seq 1 10 | sed -e '/9/s/$/+/' >file
git commit -m "9 plus" file

rm -f file.rej
git reset --hard
git apply --reject patch
# inspect the index and working tree to see what happened here
# this one, that does not touch the index, works as expected.

rm -f file.rej
git reset --hard
git apply --cached --reject patch
# inspect the index and working tree to see what happened here

rm -f file.rej
git reset --hard
git apply --index --reject patch
# inspect the index and working tree to see what happened here

--- 8< ------ 8< ------ 8< ------ 8< ------ 8< ------ 8< ---

... half-baked changes.  The above prepares a 10-line file and a
patch that touches its first and the last line.  It then makes a
modification that textually conflicts the second last line to the
target file, and tries to apply the patch.

We do get .rej file in all cases, but in the mode that would touch
the index, the command refuses to put the result of an incomplete
patch application to the index, even though it updates the working
tree files with partial application.

The log message of 8938045a (git-apply --reject: finishing touches.,
2006-08-27) does mention that with "--index", it is the designed
behaviour to update the index for paths to which the patches were
cleanly applied, with a reasonable justification.  Even though it
also implies at the same time that it is also the designed behaviour
to leave the index untouched for paths that got .rej files, I do not
offhand see it explains the reason behind the design very well.  I
suspect that it was to avoid committing a half-resolved state by
mistake, but then it does not prevent the user from committing a
no-modification by mistake, and that is why I say "not ... very well"
here.  In any case, it does not even mention what ought to happen in
the "--cached" case, so...