| Message ID | 20181011211754.31369-18-sbeller@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Bring more repository handles into our code base | expand |
> +/* > + * Initialize 'out' based on the provided submodule path. > + * > + * Unlike repo_submodule_init, this tolerates submodules not present > + * in .gitmodules. NEEDSWORK: The repo_submodule_init behavior is > + * preferrable. This function exists only to preserve historical behavior. What do you mean by "The repo_submodule_init behavior is preferable"? If we need to preserve historical behavior, then it seems that the most preferable one is something that meets our needs (like open_submodule() in this patch). > +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); Do we need to set submodule_prefix? > @@ -507,7 +533,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 (open_submodule(sub, path) < 0) { This function, as a side effect, writes the open repository to "sub" for its caller to use. I think it's better if its callers open "sub" and then pass it to show_submodule_header() if successful. If opening the submodule is expected to fail sometimes (like it seems here), then we can allow callers to also pass NULL, and document what happens when NULL is passed. Also, repositories open in this way should also be freed.
On Thu, Oct 11, 2018 at 3:41 PM Jonathan Tan <jonathantanmy@google.com> wrote: > > > +/* > > + * Initialize 'out' based on the provided submodule path. > > + * > > + * Unlike repo_submodule_init, this tolerates submodules not present > > + * in .gitmodules. NEEDSWORK: The repo_submodule_init behavior is > > + * preferrable. This function exists only to preserve historical behavior. > > What do you mean by "The repo_submodule_init behavior is preferable"? Preferable in the sense that it follows the definition of a submodule, but this here works for "any repo" that happens to be at the gitlink. > If > we need to preserve historical behavior, then it seems that the most > preferable one is something that meets our needs (like open_submodule() > in this patch). Yes, I'll reword to drop the preferrable, but still state the difference. I wonder if instead we'd want to introduce a repo_submodule_init(struct repository *submodule \ struct repository *superproject \ const char *path, \ int tolerate_lookalikes) Another patch proposes to replace the path by a struct submodule, but for lookalikes, we do not have a struct submodule to begin with (though in the other patches we cook up a fake entry in the submodule config) > > +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); > > Do we need to set submodule_prefix? Good point! Thanks for catching this. > > > @@ -507,7 +533,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 (open_submodule(sub, path) < 0) { > > This function, as a side effect, writes the open repository to "sub" for > its caller to use. I think it's better if its callers open "sub" and > then pass it to show_submodule_header() if successful. If opening the > submodule is expected to fail sometimes (like it seems here), then we > can allow callers to also pass NULL, and document what happens when NULL > is passed. That looks intriguing, I'll take a look. Note that we also pass in **left and **right to have it assigned in there. > > Also, repositories open in this way should also be freed. Yes, thanks!
On Thu, Oct 11, 2018 at 3:41 PM Jonathan Tan <jonathantanmy@google.com> wrote: > > > +/* > > + * Initialize 'out' based on the provided submodule path. > > + * > > + * Unlike repo_submodule_init, this tolerates submodules not present > > + * in .gitmodules. NEEDSWORK: The repo_submodule_init behavior is > > + * preferrable. This function exists only to preserve historical behavior. > > What do you mean by "The repo_submodule_init behavior is preferable"? If > we need to preserve historical behavior, then it seems that the most > preferable one is something that meets our needs (like open_submodule() > in this patch). > > > +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); > > Do we need to set submodule_prefix? > > > @@ -507,7 +533,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 (open_submodule(sub, path) < 0) { > > This function, as a side effect, writes the open repository to "sub" for > its caller to use. I think it's better if its callers open "sub" and > then pass it to show_submodule_header() if successful. If opening the > submodule is expected to fail sometimes (like it seems here), then we > can allow callers to also pass NULL, and document what happens when NULL > is passed. Thanks for the review of the whole series! I have redone this series, addressing all your comments. I addressed this comment differently than suggested, and put the submodule repository argument at the end of the parameter list, such that it goes with all the other arguments to be filled in. I was about to resend the series, but test-merged with the other submodule series in flight (origin/sb/submodule-recursive-fetch-gets-the-tip) which had some conflicts that I can easily resolve by rebasing on top. It conflicts a lot when merging to next, due to the latest patch ("Apply semantic patches from previous patches"), so I am not sure how to proceed properly. Maybe we'd omit that patch and run 'make coccicheck' on next to apply the semantic patches there instead.
> Thanks for the review of the whole series! > > I have redone this series, addressing all your comments. I addressed > this comment differently than suggested, and put the submodule > repository argument at the end of the parameter list, such that it > goes with all the other arguments to be filled in. Sounds good. > I was about to resend the series, but test-merged with the other > submodule series in flight (origin/sb/submodule-recursive-fetch-gets-the-tip) > which had some conflicts that I can easily resolve by rebasing on top. I presume you are talking about [1]? Maybe consider rebasing that one on top of this instead, since this is just a refactoring whereas submodule-recursive-fetch-gets-the-tip changes functionality, from what I understand of patches 8 and 9. [1] https://public-inbox.org/git/20181016181327.107186-1-sbeller@google.com/ > It conflicts a lot when merging to next, due to the latest patch > ("Apply semantic patches from previous patches"), so I am not sure > how to proceed properly. Maybe we'd omit that patch and > run 'make coccicheck' on next to apply the semantic patches > there instead. Omitting the patch sounds good to me. For me, just stating that you have excluded any coccinelle-generated patches in order to ease merging into the various branches is sufficient, and people can test the coccinelle patches included by running "make coccicheck" then "patch -p1 <contrib/coccinelle/the_repository.cocci.patch".
On Tue, Oct 16, 2018 at 4:13 PM Jonathan Tan <jonathantanmy@google.com> wrote: > > > Thanks for the review of the whole series! > > > > I have redone this series, addressing all your comments. I addressed > > this comment differently than suggested, and put the submodule > > repository argument at the end of the parameter list, such that it > > goes with all the other arguments to be filled in. > > Sounds good. Actually I changed my mind on that after figuring out how to free the submodule repository appropriately and went with your original suggestion. > > > I was about to resend the series, but test-merged with the other > > submodule series in flight (origin/sb/submodule-recursive-fetch-gets-the-tip) > > which had some conflicts that I can easily resolve by rebasing on top. > > I presume you are talking about [1]? Maybe consider rebasing that one on > top of this instead, since this is just a refactoring whereas > submodule-recursive-fetch-gets-the-tip changes functionality, from what > I understand of patches 8 and 9. From my understanding, that series is further along than this one, so I would not want to mix up their order. Currently I am rebasing this on top of select topics from next, (ds/reachable) as that are the other conflicts that I'd have to deal with. > [1] https://public-inbox.org/git/20181016181327.107186-1-sbeller@google.com/ > > > It conflicts a lot when merging to next, due to the latest patch > > ("Apply semantic patches from previous patches"), so I am not sure > > how to proceed properly. Maybe we'd omit that patch and > > run 'make coccicheck' on next to apply the semantic patches > > there instead. > > Omitting the patch sounds good to me. For me, just stating that you have > excluded any coccinelle-generated patches in order to ease merging into > the various branches is sufficient, and people can test the coccinelle > patches included by running "make coccicheck" then "patch -p1 > <contrib/coccinelle/the_repository.cocci.patch". ok. Thanks, Stefan
diff --git a/submodule.c b/submodule.c index 442229bb49..5e1a6c0b7c 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,12 +482,37 @@ void prepare_submodule_repo_env(struct argv_array *out) DEFAULT_GIT_DIR_ENVIRONMENT); } +/* + * Initialize 'out' based on the provided submodule path. + * + * Unlike repo_submodule_init, this tolerates submodules not present + * in .gitmodules. NEEDSWORK: The repo_submodule_init behavior is + * preferrable. 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); + + 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 objects directory it will * attempt to 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, struct repository *sub, + const char *path, struct object_id *one, struct object_id *two, unsigned dirty_submodule, struct commit **left, struct commit **right, @@ -507,7 +533,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 (open_submodule(sub, path) < 0) { if (!message) message = "(commits not present)"; goto output_header; @@ -517,8 +543,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 +554,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,8 +588,9 @@ 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; - show_submodule_header(o, path, one, two, dirty_submodule, + show_submodule_header(o, &sub, path, one, two, dirty_submodule, &left, &right, &merge_bases); /* @@ -580,7 +607,7 @@ 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) @@ -598,8 +625,9 @@ 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; - show_submodule_header(o, path, one, two, dirty_submodule, + show_submodule_header(o, &sub, path, one, two, dirty_submodule, &left, &right, &merge_bases); /* We need a valid left and right commit to display a difference */
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 | 48 ++++++++++++++++++++++++++++++++++++++---------- 1 file changed, 38 insertions(+), 10 deletions(-)