I've rerolled this to considerably expand the commit messages, because it took me a while to learn apply.c and a while to relearn it as well, so the messages should be as specific as possible to aid the next person.
A note about this series: the code changes (first two patches) apply fine to cff5dc09ed, which introduced the feature in a broken state, and fix it there. However, that is from 2018, and t4140 did not exist yet at that time (I wrote it in 2020) and the docs have moved around some in the interim as well. If it were a year-old, and not a nearly seven-year-old, issue, I would prioritize making it all work there, but I'm not sure it's worth adding an entirely new test. Unsure what the correct thing to do here is.
Raymond E. Pasco (4):
apply: read in the index in --intent-to-add mode
apply: only write intents to add for new files
t4140: test apply --intent-to-add interactions
apply docs: clarify wording for --intent-to-add
Documentation/git-apply.adoc | 9 +++++----
apply.c | 4 ++--
t/t4140-apply-ita.sh | 31 ++++++++++++++++++++++++++++++-
3 files changed, 37 insertions(+), 7 deletions(-)
Range-diff against v2:
1: 71ec291fba ! 1: f3120189b3 apply: read in the index in --intent-to-add mode
@@ Commit message
only to the index (--cached).
The --intent-to-add flag modifies the first of these modes, applying
- only to the worktree, in a way which touches the index, because
- intents to add are special index entries. However, it has not ever
+ only to the worktree, in a way which touches the index, because intents
+ to add are special index entries. However, since its introduction
+ in cff5dc09ed (apply: add --intent-to-add, 2018-05-26), it has not
worked correctly in any but the most trivial (empty repository)
- cases, because the index was never read in (in apply, this is done
- in read_apply_cache()) before writing to it.
+ cases, because the index is never read in (in apply, this is done in
+ read_apply_cache()) before writing to it.
This causes the operation to clobber the old, correct index with a
new empty-tree index before writing intent-to-add entries to this
@@ Commit message
existing file in the repository as deleted, which is incorrect.
This error can be corrected by first reading the index. The
- update_index flag is correctly set if ita_only is true, because
- this mode updates the index. However, if we merely gate the call
- to read_apply_cache() behind update_index, then it will not be read
- when state->apply is false, even if it must be checked. Therefore,
- we instead read the index if it will be either checked or updated,
- because reading the index is a prerequisite to either.
+ update_index flag is correctly set if ita_only is true, because this
+ flag causes the index to be updated. However, if we merely gate the
+ call to read_apply_cache() behind update_index, then it will not be
+ read when state->apply is false, even if it must be checked due to
+ being in --index or --cached mode. Therefore, we instead read the
+ index if it will be either checked or updated, because reading the
+ index is a prerequisite to either.
Reported-by: Ryan Hodges <rhodges@cisco.com>
Original-patch-by: Johannes Altmanninger <aclopte@gmail.com>
2: 5f49ff9035 ! 2: 907a90d849 apply: only write intents to add for new files
@@ Metadata
## Commit message ##
apply: only write intents to add for new files
- In the "update only the worktree" mode, the index should not be touched
- except to record intents to add when --intent-to-add is on. Because
- having --intent-to-add on sets update_index, to indicate that we are
- touching the index, we can't rely only on that flag to decide whether to
- write an index entry.
+ In the "apply only to files" mode (i.e., neither --index nor --cached
+ mode), the index should not be touched except to record intents to
+ add when --intent-to-add is on. Because having --intent-to-add on sets
+ update_index, to indicate that we may touch the index, we can't rely
+ only on that flag in create_file() (which is called to write both new
+ files and updated files) to decide whether to write an index entry;
+ if we did, we would write an index entry for every file being patched
+ (which would moreover be an intent-to-add entry despite not being a
+ new file, because we are going to turn on the CE_INTENT_TO_ADD flag
+ in add_index_entry() if we enter it here and ita_only is true).
- Because we have already entered write_out_results() and are performing
- writes, we know that state->apply is true. If state->check_index is
- additionally true, we are in a mode which updates the index and should
+ To decide whether to touch the index, we need to check the
+ specific reason the index would be updated, rather than merely
+ their aggregate in the update_index flag. Because we have already
+ entered write_out_results() and are performing writes, we know that
+ state->apply is true. If state->check_index is additionally true, we
+ are in --index or --cached mode, which updates the index and should
always write, whereas if we are merely in ita_only mode we must only
write if the patch is a new file creation patch.
3: f8a6d8032f ! 3: b7603de201 t4140: test apply --intent-to-add interactions
@@ Metadata
## Commit message ##
t4140: test apply --intent-to-add interactions
- Test that applying a new file creation patch to an existing index works,
- and that applying a patch with both modifications and new file creations
- works.
+ Test that applying a new file creation patch with --intent-to-add to
+ an existing index does not modify the index outside adding the correct
+ intents-to-add, and that applying a patch with both modifications
+ and new file creations with --intent-to-add correctly only adds
+ intents-to-add to the index.
Signed-off-by: Raymond E. Pasco <ray@ameretat.dev>
4: ad42992d03 = 4: 4c786a77c9 apply docs: clarify wording for --intent-to-add
Raymond E. Pasco
