| Message ID | 20181016233550.251311-19-sbeller@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Bring more repository handles into our code base | expand |
> 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. There is also a side effect in that the submodule now needs to pass all the checks done by repo_init() instead of merely having the "objects/" directory. Can you add information about this to the commit message? Also, which tests exercise this functionality? Mention them in the commit message. > +/* > + * 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 int open_submodule(struct repository *out, const char *path) > +{ > + struct strbuf sb = STRBUF_INIT; > + > + if (submodule_to_gitdir(&sb, path) || repo_init(out, sb.buf, NULL)) { > + strbuf_release(&sb); > + return -1; > + } > + > + out->submodule_prefix = xstrdup(path); > + out->submodule_prefix = xstrfmt("%s%s/", > + the_repository->submodule_prefix ? > + the_repository->submodule_prefix : > + "", path); You seem to say here [1] that we don't need submodule_prefix, but you're instead setting it twice? :-) [1] https://public-inbox.org/git/CAGZ79kZTxoNMUyX+EHg0q3Ss2M-Etkf0yiw3E40U3VCt4QMwrQ@mail.gmail.com/ > +/* > + * 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, > +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) > { Location of the submodule git directory is done by the caller of this function, not by this function. Also this function doesn't create any repository handles. The documentation probably needs to be updated. Also mention what happens if sub is NULL. > @@ -563,16 +596,20 @@ 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 subrepo, *sub = &subrepo; > + > + if (open_submodule(&subrepo, path) < 0) > + sub = NULL; Handling of the subrepo and *sub seems clumsy - it might be better to just let open_submodule() return a struct repository pointer. Previous 17 patches look good - most are the same as the previous version, which I already was happy with, and I see that the patch I requested in [2] was added. [2] https://public-inbox.org/git/20181011221114.186377-1-jonathantanmy@google.com/
On Tue, Oct 16, 2018 at 04:35:49PM -0700, 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. This patch breaks the test suite with 'GIT_TEST_COMMIT_GRAPH=1', in particular 't4041-diff-submodule-option.sh' fails with: 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-10-25 09:10:00.541610016 +0000 +++ actual 2018-10-25 09:10:00.537609885 +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) and 't4060-diff-submodule-option-diff-format.sh' fails with: expecting success: git diff-index -p --submodule=diff HEAD >actual && cat >expected <<-EOF && Submodule sm1 $head2..$head3 (rewind): diff --git a/sm1/foo2 b/sm1/foo2 deleted file mode 100644 index 54b060e..0000000 --- a/sm1/foo2 +++ /dev/null @@ -1 +0,0 @@ -foo2 diff --git a/sm1/foo3 b/sm1/foo3 deleted file mode 100644 index c1ec6c6..0000000 --- a/sm1/foo3 +++ /dev/null @@ -1 +0,0 @@ -foo3 EOF test_cmp expected actual + git diff-index -p --submodule=diff HEAD + cat + test_cmp expected actual + diff -u expected actual --- expected 2018-10-25 09:10:38.854868800 +0000 +++ actual 2018-10-25 09:10:38.854868800 +0000 @@ -1,4 +1,4 @@ -Submodule sm1 30b9670..dafb207 (rewind): +Submodule sm1 30b9670...dafb207: diff --git a/sm1/foo2 b/sm1/foo2 deleted file mode 100644 index 54b060e..0000000 error: last command exited with $?=1 not ok 10 - modified submodule(backward)
On 10/16/2018 7:35 PM, Stefan Beller wrote: > @@ -482,14 +483,46 @@ 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 int open_submodule(struct repository *out, const char *path) > +{ > + struct strbuf sb = STRBUF_INIT; > + > + if (submodule_to_gitdir(&sb, path) || repo_init(out, sb.buf, NULL)) { > + strbuf_release(&sb); > + return -1; > + } > + > + out->submodule_prefix = xstrdup(path); > + out->submodule_prefix = xstrfmt("%s%s/", > + the_repository->submodule_prefix ? > + the_repository->submodule_prefix : > + "", path); > + > + strbuf_release(&sb); > + return 0; > +} Based on the recent test coverage report [1], this xstrfmt() call is never run witha non-null the_repository->submodule_prefix. Is there a way we can exercise that branch? Thanks, -Stolee [1] https://public-inbox.org/git/62f0bcf6-aa73-c192-d804-e6d69cac146f@gmail.com/
On Wed, Oct 31, 2018 at 6:38 AM Derrick Stolee <stolee@gmail.com> wrote: > > On 10/16/2018 7:35 PM, Stefan Beller wrote: > > @@ -482,14 +483,46 @@ 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 int open_submodule(struct repository *out, const char *path) > > +{ > > + struct strbuf sb = STRBUF_INIT; > > + > > + if (submodule_to_gitdir(&sb, path) || repo_init(out, sb.buf, NULL)) { > > + strbuf_release(&sb); > > + return -1; > > + } > > + > > + out->submodule_prefix = xstrdup(path); > > + out->submodule_prefix = xstrfmt("%s%s/", > > + the_repository->submodule_prefix ? > > + the_repository->submodule_prefix : > > + "", path); > > + > > + strbuf_release(&sb); > > + return 0; > > +} > > Based on the recent test coverage report [1], this xstrfmt() call is never > run witha non-null the_repository->submodule_prefix. Is there a way we can > exercise that branch? No it's dead code, actually. the_repository never has submodule_prefix set as it is the main repository. So this is overly cautious to enable the 'any repo' case. In a resend we'll go with xstrdup(path);
diff --git a/submodule.c b/submodule.c index 4ee69cc014..7305ae2e10 100644 --- a/submodule.c +++ b/submodule.c @@ -444,7 +444,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; @@ -455,7 +455,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); @@ -482,14 +483,46 @@ 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 int open_submodule(struct repository *out, const char *path) +{ + struct strbuf sb = STRBUF_INIT; + + if (submodule_to_gitdir(&sb, path) || repo_init(out, sb.buf, NULL)) { + strbuf_release(&sb); + return -1; + } + + out->submodule_prefix = xstrdup(path); + out->submodule_prefix = xstrfmt("%s%s/", + the_repository->submodule_prefix ? + the_repository->submodule_prefix : + "", path); + + strbuf_release(&sb); + return 0; +} + +/* + * 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, +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) { @@ -508,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; @@ -518,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 @@ -529,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; @@ -563,16 +596,20 @@ 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 subrepo, *sub = &subrepo; + + if (open_submodule(&subrepo, path) < 0) + sub = NULL; 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 */ @@ -581,13 +618,15 @@ 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); } void show_submodule_inline_diff(struct diff_options *o, const char *path, @@ -599,9 +638,13 @@ 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 subrepo, *sub = &subrepo; + + if (open_submodule(&subrepo, path) < 0) + sub = NULL; 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)) || @@ -662,6 +705,8 @@ 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); } int should_update_submodules(void)
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 <sbeller@google.com> --- submodule.c | 75 ++++++++++++++++++++++++++++++++++++++++++----------- 1 file changed, 60 insertions(+), 15 deletions(-)