Skip to content

Output fixes for --remerge-diff

Elijah Newren via GitGitGadget git 3 versions – New

Get series with b4
b4 am [email protected]

Versions newest first · linked by author and title

  1. v3 Latest3 patches 0 repliesNew
  2. v23 patches 1 replySuperseded
  3. v13 patches 10 repliesSuperseded

Changes in v3 from the cover letter

* Added a comment describing the rationale for the new diff_filepair_is_phoney() function.

Patches in v3 No applies recorded yet

#Subject and filesSizeRepliesApplied
1/3diff: have submodule_format logic avoid additional diff headersdiff.c t/t4069-remerge-diff.sh+31 -60 replies
2/3diff: fix filtering of additional headers under --remerge-diffdiff.c t/t4069-remerge-diff.sh+18 -10 replies
3/3diff: fix filtering of merge commits under --remerge-diffdiff.c t/t4069-remerge-diff.sh+6 -00 replies

Reviews trailers per patch

No Reviewed-by, Acked-by or Tested-by trailers for v3 yet.

Read the v3 thread

Cover letter [PATCH v3 0/3] Output fixes for --remerge-diff

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/[email protected]/

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

@@ 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
Signature · 1 line
gitgitgadget