last-modified: fix bug when some paths remain unhandled

The recently introduced new subcommand git-last-modified(1) runs into an
error in some scenarios. It then would exit with the message:

    BUG: paths remaining beyond boundary in last-modified

This seems to happens for example when criss-cross merges are involved.
In that scenario, the function diff_tree_combined() gets called.

The function diff_tree_combined() copies the `struct diff_options` from
the input `struct rev_info` to override some flags. One flag is
`recursive`, which is always set to 1. This has been the case since the
inception of this function in af3feefa1d (diff-tree -c: show a merge
commit a bit more sensibly., 2006-01-24).

This behavior is incompatible with git-last-modified(1), when called
non-recursive (which is the default).

The last-modified machinery uses a hashmap for all the paths it wants to
get the last-modified commit for. Through log_tree_commit() the callback
mark_path() is called. The diff machinery uses diff_tree_combined()
internally, and due to it's recursive behavior the callback receives
entries inside subtrees, but not the subtree entries themselves. So a
directory is never expelled from the hashmap, and the BUG() statement
gets hit.

Because there are many callers calling into diff_tree_combined(), both
directly and indirectly, we cannot simply change it's behavior.

Instead, add a flag `no_recursive_diff_tree_combined` which supresses
the behavior of diff_tree_combined() to override `recursive` and set
this flag in builtin/last-modified.c.

Signed-off-by: Toon Claes <toon@iotcl.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/builtin/last-modified.c b/builtin/last-modified.c
index 886ba12cb5f1a36af3f927b46d0da35e011b33e8..ae8b36a2c3515c6a3931b04bad6c95fae36ac592 100644 (file)
--- a/builtin/last-modified.c
+++ b/builtin/last-modified.c
@@ -265,6 +265,7 @@ static int last_modified_init(struct last_modified *lm, struct repository *r,
        lm->rev.boundary = 1;
        lm->rev.no_commit_id = 1;
        lm->rev.diff = 1;
+       lm->rev.diffopt.flags.no_recursive_diff_tree_combined = 1;
        lm->rev.diffopt.flags.recursive = lm->recursive;
        lm->rev.diffopt.flags.tree_in_recursive = lm->show_trees;
 

diff --git a/combine-diff.c b/combine-diff.c
index 3878faabe7bb2f7c80cffbf3add6123f17960627..e779b86e0b84ffedf36e23deee4f413e84ce662b 100644 (file)
--- a/combine-diff.c
+++ b/combine-diff.c
@@ -1515,8 +1515,9 @@ void diff_tree_combined(const struct object_id *oid,
 
        diffopts = *opt;
        copy_pathspec(&diffopts.pathspec, &opt->pathspec);
-       diffopts.flags.recursive = 1;
        diffopts.flags.allow_external = 0;
+       if (!opt->flags.no_recursive_diff_tree_combined)
+               diffopts.flags.recursive = 1;
 
        /* find set of paths that everybody touches
         *

diff --git a/diff.h b/diff.h
index 9bb939a4f18ed2c2c1f4737424751d51590ba468..df8f7643b002cffbd74a10b1e72eb6ed31c23515 100644 (file)
--- a/diff.h
+++ b/diff.h
@@ -126,6 +126,13 @@ struct diff_flags {
        unsigned recursive;
        unsigned tree_in_recursive;
 
+       /*
+        * Historically diff_tree_combined() overrides recursive to 1. To
+        * suppress this behavior, set the flag below.
+        * It has no effect if recursive is already set to 1.
+        */
+       unsigned no_recursive_diff_tree_combined;
+
        /* Affects the way how a file that is seemingly binary is treated. */
        unsigned binary;
        unsigned text;

diff --git a/t/t8020-last-modified.sh b/t/t8020-last-modified.sh
index 5eb4cef0359212eeddc68578697ca432760e0be3..e13aad14398dd9055ff45ab57700329e7aab37c4 100755 (executable)
--- a/t/t8020-last-modified.sh
+++ b/t/t8020-last-modified.sh
@@ -128,6 +128,22 @@ test_expect_success 'only last-modified files in the current tree' '
        EOF
 '
 
+test_expect_success 'last-modified with subdir and criss-cross merge' '
+       git checkout -b branch-k1 1 &&
+       mkdir -p a k &&
+       test_commit k1 a/file2 &&
+       git checkout -b branch-k2 &&
+       test_commit k2 k/file2 &&
+       git checkout branch-k1 &&
+       test_merge km2 branch-k2 &&
+       test_merge km3 3 &&
+       check_last_modified <<-\EOF
+       km3 a
+       k2 k
+       1 file
+       EOF
+'
+
 test_expect_success 'cross merge boundaries in blaming' '
        git checkout HEAD^0 &&
        git rm -rf . &&