On 10/30/2018 6:08 PM, Stefan Beller wrote:
This converts the 'show_submodule_header' function to use
the repository API properly, such that the submodule objects
are not added to the main object store.

Signed-off-by: Stefan Beller <sbel...@google.com>

A couple tests are broken in 'pu' when run with GIT_TEST_COMMIT_GRAPH=1, including t4041-diff-submodule-option.sh. The failure bisects to this patch.

Here is a verbose output of the first failure in that script:;


expecting success:
        git diff-index -p --submodule=log HEAD >actual &&
        cat >expected <<-EOF &&
        Submodule sm1 $head2..$head3 (rewind):
          < Add foo3 ($added foo3)
          < Add foo2 ($added foo2)
        EOF
        test_cmp expected actual

+ git diff-index -p --submodule=log HEAD
+ cat
+ test_cmp expected actual
+ diff -u expected actual
--- expected    2018-11-02 12:58:43.429262380 +0000
+++ actual      2018-11-02 12:58:43.429262380 +0000
@@ -1,3 +1,5 @@
-Submodule sm1 30b9670..dafb207 (rewind):
+Submodule sm1 30b9670...dafb207:
   < Add foo3 (hinzugefügt foo3)
   < Add foo2 (hinzugefügt foo2)
+  > Add foo1 (hinzugefügt foo1)
+  < Add foo1 (hinzugefügt foo1)
error: last command exited with $?=1
not ok 9 - modified submodule(backward)

I've been looking into the patch below to see if there is an obvious problem, but the best I can think is that open_submodule() creates an alternate 'struct repository' and somehow the commit-graph feature is interacting poorly with that struct.

Stefan, do you know what's going on?

Thanks,

-Stolee

---
  submodule.c | 76 ++++++++++++++++++++++++++++++++++++++++++-----------
  1 file changed, 61 insertions(+), 15 deletions(-)

diff --git a/submodule.c b/submodule.c
index d9d3046689..0fdda45252 100644
--- a/submodule.c
+++ b/submodule.c
@@ -443,7 +443,7 @@ static int prepare_submodule_summary(struct rev_info *rev, 
const char *path,
        return prepare_revision_walk(rev);
  }
-static void print_submodule_summary(struct rev_info *rev, struct diff_options *o)
+static void print_submodule_summary(struct repository *r, struct rev_info 
*rev, struct diff_options *o)
  {
        static const char format[] = "  %m %s";
        struct strbuf sb = STRBUF_INIT;
@@ -454,7 +454,8 @@ static void print_submodule_summary(struct rev_info *rev, 
struct diff_options *o
                ctx.date_mode = rev->date_mode;
                ctx.output_encoding = get_log_output_encoding();
                strbuf_setlen(&sb, 0);
-               format_commit_message(commit, format, &sb, &ctx);
+               repo_format_commit_message(r, commit, format, &sb,
+                                     &ctx);
                strbuf_addch(&sb, '\n');
                if (commit->object.flags & SYMMETRIC_LEFT)
                        diff_emit_submodule_del(o, sb.buf);
@@ -481,14 +482,47 @@ void prepare_submodule_repo_env(struct argv_array *out)
                         DEFAULT_GIT_DIR_ENVIRONMENT);
  }
-/* Helper function to display the submodule header line prior to the full
- * summary output. If it can locate the submodule objects directory it will
- * attempt to lookup both the left and right commits and put them into the
- * left and right pointers.
+/*
+ * Initialize 'out' based on the provided submodule path.
+ *
+ * Unlike repo_submodule_init, this tolerates submodules not present
+ * in .gitmodules. This function exists only to preserve historical behavior,
+ *
+ * Returns 0 on success, -1 when the submodule is not present.
   */
-static void show_submodule_header(struct diff_options *o, const char *path,
+static struct repository *open_submodule(const char *path)
+{
+       struct strbuf sb = STRBUF_INIT;
+       struct repository *out = xmalloc(sizeof(*out));
+
+       if (submodule_to_gitdir(&sb, path) || repo_init(out, sb.buf, NULL)) {
+               strbuf_release(&sb);
+               free(out);
+               return NULL;
+       }
+
+       out->submodule_prefix = xstrfmt("%s%s/",
+                                       the_repository->submodule_prefix ?
+                                       the_repository->submodule_prefix :
+                                       "", path);
+
+       strbuf_release(&sb);
+       return out;
+}
+
+/*
+ * Helper function to display the submodule header line prior to the full
+ * summary output.
+ *
+ * If it can locate the submodule git directory it will create a repository
+ * handle for the submodule and lookup both the left and right commits and
+ * put them into the left and right pointers.
+ */
+static void show_submodule_header(struct diff_options *o,
+               const char *path,
                struct object_id *one, struct object_id *two,
                unsigned dirty_submodule,
+               struct repository *sub,
                struct commit **left, struct commit **right,
                struct commit_list **merge_bases)
  {
@@ -507,7 +541,7 @@ static void show_submodule_header(struct diff_options *o, 
const char *path,
        else if (is_null_oid(two))
                message = "(submodule deleted)";
- if (add_submodule_odb(path)) {
+       if (!sub) {
                if (!message)
                        message = "(commits not present)";
                goto output_header;
@@ -517,8 +551,8 @@ static void show_submodule_header(struct diff_options *o, 
const char *path,
         * Attempt to lookup the commit references, and determine if this is
         * a fast forward or fast backwards update.
         */
-       *left = lookup_commit_reference(the_repository, one);
-       *right = lookup_commit_reference(the_repository, two);
+       *left = lookup_commit_reference(sub, one);
+       *right = lookup_commit_reference(sub, two);
/*
         * Warn about missing commits in the submodule project, but only if
@@ -528,7 +562,7 @@ static void show_submodule_header(struct diff_options *o, 
const char *path,
             (!is_null_oid(two) && !*right))
                message = "(commits not present)";
- *merge_bases = get_merge_bases(*left, *right);
+       *merge_bases = repo_get_merge_bases(sub, *left, *right);
        if (*merge_bases) {
                if ((*merge_bases)->item == *left)
                        fast_forward = 1;
@@ -562,16 +596,18 @@ void show_submodule_summary(struct diff_options *o, const 
char *path,
        struct rev_info rev;
        struct commit *left = NULL, *right = NULL;
        struct commit_list *merge_bases = NULL;
+       struct repository *sub;
+ sub = open_submodule(path);
        show_submodule_header(o, path, one, two, dirty_submodule,
-                             &left, &right, &merge_bases);
+                             sub, &left, &right, &merge_bases);
/*
         * If we don't have both a left and a right pointer, there is no
         * reason to try and display a summary. The header line should contain
         * all the information the user needs.
         */
-       if (!left || !right)
+       if (!left || !right || !sub)
                goto out;
/* Treat revision walker failure the same as missing commits */
@@ -580,13 +616,17 @@ void show_submodule_summary(struct diff_options *o, const 
char *path,
                goto out;
        }
- print_submodule_summary(&rev, o);
+       print_submodule_summary(sub, &rev, o);
out:
        if (merge_bases)
                free_commit_list(merge_bases);
        clear_commit_marks(left, ~0);
        clear_commit_marks(right, ~0);
+       if (sub) {
+               repo_clear(sub);
+               free(sub);
+       }
  }
void show_submodule_inline_diff(struct diff_options *o, const char *path,
@@ -598,9 +638,11 @@ void show_submodule_inline_diff(struct diff_options *o, 
const char *path,
        struct commit_list *merge_bases = NULL;
        struct child_process cp = CHILD_PROCESS_INIT;
        struct strbuf sb = STRBUF_INIT;
+       struct repository *sub;
+ sub = open_submodule(path);
        show_submodule_header(o, path, one, two, dirty_submodule,
-                             &left, &right, &merge_bases);
+                             sub, &left, &right, &merge_bases);
/* We need a valid left and right commit to display a difference */
        if (!(left || is_null_oid(one)) ||
@@ -661,6 +703,10 @@ void show_submodule_inline_diff(struct diff_options *o, 
const char *path,
                clear_commit_marks(left, ~0);
        if (right)
                clear_commit_marks(right, ~0);
+       if (sub) {
+               repo_clear(sub);
+               free(sub);
+       }
  }
int should_update_submodules(void)

Reply via email to