Re: [PATCH 18/19] submodule: use submodule repos for object lookup
On Wed, Oct 31, 2018 at 6:38 AM Derrick Stolee 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(, path) || repo_init(out, sb.buf, NULL)) { > > + strbuf_release(); > > + return -1; > > + } > > + > > + out->submodule_prefix = xstrdup(path); > > + out->submodule_prefix = xstrfmt("%s%s/", > > + the_repository->submodule_prefix ? > > + the_repository->submodule_prefix : > > + "", path); > > + > > + strbuf_release(); > > + 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);
Re: [PATCH 18/19] submodule: use submodule repos for object lookup
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(, path) || repo_init(out, sb.buf, NULL)) { + strbuf_release(); + return -1; + } + + out->submodule_prefix = xstrdup(path); + out->submodule_prefix = xstrfmt("%s%s/", + the_repository->submodule_prefix ? + the_repository->submodule_prefix : + "", path); + + strbuf_release(); + 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-e6d69cac1...@gmail.com/
Re: [PATCH 18/19] submodule: use submodule repos for object lookup
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 --- expected2018-10-25 09:10:00.541610016 + +++ actual 2018-10-25 09:10:00.537609885 + @@ -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..000 --- a/sm1/foo2 +++ /dev/null @@ -1 +0,0 @@ -foo2 diff --git a/sm1/foo3 b/sm1/foo3 deleted file mode 100644 index c1ec6c6..000 --- 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 --- expected2018-10-25 09:10:38.854868800 + +++ actual 2018-10-25 09:10:38.854868800 + @@ -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..000 error: last command exited with $?=1 not ok 10 - modified submodule(backward)
Re: [PATCH 18/19] submodule: use submodule repos for object lookup
> 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(, path) || repo_init(out, sb.buf, NULL)) { > + strbuf_release(); > + 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-etkf0yiw3e40u3vct4qm...@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 = > + > + if (open_submodule(, 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-jonathanta...@google.com/