Changes since v2:
* Modify comment to note testfile as well as testcase name; thanks to Junio
for spotting
* Clarify comment about skipping rename-to-self cases in process_renames()
in the second-to-last patch; I meant to do this as part of v2 but forgot.
* Add a new commit which clarifies the comment in merge_options_internal
about the interning of strings in the "path" member.
Changes since v1; many thanks to Patrick for reviewing v1 in detail:
* Several commit message cleanups
* Make the test in patch 3 have a single expectation while documenting that
a reasonable alternative exists, instead of trying to document that both
are valid and having the test accept both outcomes
* Moved a hunk accidentally squashed into patch 4 back to patch 3, and
adjusted a nearby incorrect comment related to the squashed change
* Fix style issues (grep -> test_grep, // -> /* */)
Original cover letter:
At GitHub, we've got a real-world repository that has been triggering
failures of the form:
git: merge-ort.c:3007: process_renames: Assertion `newinfo && !newinfo->merged.clean' failed.
Digging in, this particular corner case requires multiple things to trigger:
(1) a rename/delete of one file, and (2) a directory rename modifying an
unrelated rename such that this unrelated rename's target becomes the source
of the rename/delete from (1).
Unfortunately, looking around, it's not the only bug in the area. Plus, some
of our testcases in tangential situations were not checked closely enough or
were weird or buggy in various ways. Adding to the challenge was the fact
that the relevant renames optimization was sometimes triggering making
renames look like delete-and-add, and overlooking this meant I sometimes
wasn't triggering what I thought I was triggering.
The combination of challenges sometimes made me think my fixes were breaking
things when sometimes I was just unaware of other bugs. I went in circles a
few times and took a rather non-linear path to finding and fixing these
issues. While I think I've turned it into a nice linear progression of
patches, I might be a bit too deep in the mud and it might not be as linear
or clear as I think. Let me know and I'll try to clarify anything needed.
Elijah Newren (7):
merge-ort: update comments to modern testfile location
merge-ort: drop unnecessary temporary in check_for_directory_rename()
t6423: document two bugs with rename-to-self testcases
t6423: fix missed staging of file in testcases 12i,12j,12k
merge-ort: clarify the interning of strings in opt->priv->path
merge-ort: fix incorrect file handling
merge-ort: fix directory rename on top of source of other
rename/delete
merge-ort.c | 55 ++-
t/t6423-merge-rename-directories.sh | 519 +++++++++++++++++++++++++++-
2 files changed, 544 insertions(+), 30 deletions(-)
base-commit: 16bd9f20a403117f2e0d9bcda6c6e621d3763e77
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1943%2Fnewren%2Ffix-rename-corner-cases-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1943/newren/fix-rename-corner-cases-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/1943
Range-diff vs v2:
1: dccc2044305 = 1: dccc2044305 merge-ort: update comments to modern testfile location
2: 58df0710efc = 2: 58df0710efc merge-ort: drop unnecessary temporary in check_for_directory_rename()
3: 1ea7bfd3bff = 3: 1ea7bfd3bff t6423: document two bugs with rename-to-self testcases
4: 29b5e00c556 = 4: 29b5e00c556 t6423: fix missed staging of file in testcases 12i,12j,12k
-: ----------- > 5: c4caba16c72 merge-ort: clarify the interning of strings in opt->priv->path
5: a8a7535fa5e ! 6: c00b6821e76 merge-ort: fix incorrect file handling
@@ merge-ort.c: static int process_renames(struct merge_options *opt,
}
+ /*
-+ * Directory renames can result in rename-to-self, which we
-+ * want to skip so we don't mark oldpath for deletion.
++ * Directory renames can result in rename-to-self; the code
++ * below assumes we have A->B with different A & B, and tries
++ * to move all entries to path B. If A & B are the same path,
++ * the logic can get confused, so skip further processing when
++ * A & B are already the same path.
+ *
-+ * Note that we can avoid strcmp here because of prior
-+ * diligence in apply_directory_rename_modifications() to
-+ * ensure we reused existing paths from opt->priv->paths.
++ * As a reminder, we can avoid strcmp here because all paths
++ * are interned in opt->priv->paths; see the comment above
++ * "paths" in struct merge_options_internal.
+ */
+ if (oldpath == newpath)
+ continue;
6: 7238c8caf2b ! 7: c2eef5ef5dc merge-ort: fix directory rename on top of source of other rename/delete
@@ merge-ort.c: static char *apply_dir_rename(struct strmap_entry *rename_info,
INITIALIZE_CI(ci, mi);
- return mi->clean || (side_mask & (ci->filemask | ci->dirmask));
+ return mi->clean || (side_mask & (ci->filemask | ci->dirmask))
-+ /* See testcases 12{n,p,q} for more details on this next condition */
++ /* See testcases 12[npq] of t6423 for this next condition */
+ || ((ci->filemask & 0x01) &&
+ strcmp(p->one->path, path));
}
--
gitgitgadget
