[PATCH v3 0/3] Output fixes for --remerge-diff
- From
Elijah Newren via GitGitGadget <gitgitgadget@gmail.com>
- Date
- Sep 2, 2022, 03:53 UTC
- Message-ID
- <pull.1342.v3.git.1662090810.gitgitgadget@gmail.com>
- In-Reply-To
- <pull.1342.v2.git.1662016087.gitgitgadget@gmail.com>
Philippe Blain found and reported a couple issues with the output of --remerge-diff[1]. After digging in, I think one of them actually counts as two separate issues, so here's a series with three patches to fix these issues. Each includes testcases to keep us from regressing.
Changes since v2:
* Added a comment describing the rationale for the new diff_filepair_is_phoney() function.
Changes since v1:
* Added a new diff_filepair_is_phoney() function to make the code more self-documenting, as suggested by Junio. * Note: Patch 2, as called out in its commit message, disables TEST_PASSES_SANITIZE_LEAK for t4069. This is not because I introduce any memory leaks, but because I add new testcases invoking additional parts of the code (pickaxe stuff) which already had pre-existing leaks. This is not a change since v1, but this somehow accidentally got munged out of Junio's application of my v1.
Note: The issue fixed by the third commit for --remerge-diff is also an issue exhibited by 'git log --cc $FILTER_RULES $COMMIT' (or by -c instead of --cc). However, as far as I can tell the causes are different and come from separate codepaths; this series focuses on --remerge-diff and hence makes no attempt to fix independent (even if similar) --cc or -c issues.
[1] https://lore.kernel.org/git/43cf2a1d-058a-fd79-befe-7d9bc62581ed@gmail.com/
Elijah Newren (3): diff: have submodule_format logic avoid additional diff headers diff: fix filtering of additional headers under --remerge-diff diff: fix filtering of merge commits under --remerge-diff
diff.c | 32 ++++++++++++++++++++++++++------ t/t4069-remerge-diff.sh | 30 +++++++++++++++++++++++++++++- 2 files changed, 55 insertions(+), 7 deletions(-)
base-commit: 6c8e4ee870332d11e4bba84901654b355a9ff016 Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1342%2Fnewren%2Fremerge-diff-output-fixes-v3 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1342/newren/remerge-diff-output-fixes-v3 Pull-Request: https://github.com/gitgitgadget/git/pull/1342
Range-diff vs v2:
1: aea0bbc1c6a ! 1: ed28acaed97 diff: have submodule_format logic avoid additional diff headers
@@ Commit message
The submodule_format handling is another codepath with the same issue;
it would operate on these additional headers and attempt to display them
- as submodule changes. Prevent that by explicitly checking for both
- modes being 0.
+ as submodule changes. Prevent that by explicitly checking for "phoney"
+ filepairs (i.e. filepairs with both modes being 0).
Reported-by: Philippe Blain <levraiphilippeblain@gmail.com>
Signed-off-by: Elijah Newren <newren@gmail.com>
@@ diff.c: static void add_formatted_headers(struct strbuf *msg,
+static int diff_filepair_is_phoney(struct diff_filespec *one,
+ struct diff_filespec *two)
+{
++ /*
++ * This function specifically looks for pairs injected by
++ * create_filepairs_for_header_only_notifications(). Such
++ * pairs are "phoney" in that they do not represent any
++ * content or even mode difference, but were inserted because
++ * diff_queued_diff previously had no pair associated with
++ * that path but we needed some pair to avoid losing the
++ * "remerge CONFLICT" header associated with the path.
++ */
+ return !DIFF_FILE_VALID(one) && !DIFF_FILE_VALID(two);
+}
+
@@ diff.c: static void builtin_diff(const char *name_a,
- if (!DIFF_FILE_VALID(one) && !DIFF_FILE_VALID(two)) {
+ if (diff_filepair_is_phoney(one, two)) {
/*
- * We should only reach this point for pairs from
+- * We should only reach this point for pairs from
++ * We should only reach this point for pairs generated from
* create_filepairs_for_header_only_notifications(). For
+- * these, we should avoid the "/dev/null" special casing
+- * above, meaning we avoid showing such pairs as either
++ * these, we want to avoid the "/dev/null" special casing
++ * above, because we do not want such pairs shown as either
+ * "new file" or "deleted file" below.
+ */
+ lbl[0] = a_one;
## t/t4069-remerge-diff.sh ##
@@ t/t4069-remerge-diff.sh: test_expect_success 'remerge-diff w/ diff-filter=U: all conflict headers, no dif
2: 3bd622d5b45 = 2: f91bea2bbc3 diff: fix filtering of additional headers under --remerge-diff
3: 7903f8380dc = 3: 084a037756d diff: fix filtering of merge commits under --remerge-diff-- gitgitgadget