Kaartic Sivaraam <kaartic.sivaraam@xxxxxxxxx> writes: > A refactoring[1] done as part of the recent conversion of > 'git submodule add' to builtin, changed the error message > shown when a Git directory already exists locally for a submodule > name. Before the refactoring, the error used to appear like so: > > --- START OF OUTPUT --- > $ git submodule add ../sub/ subm > A git directory for 'subm' is found locally with remote(s): > origin /me/git-repos-for-test/sub > If you want to reuse this local git directory instead of cloning again from > /me/git-repos-for-test/sub > use the '--force' option. If the local git directory is not the correct repo > or you are unsure what this means choose another name with the '--name' option. > --- END OF OUTPUT --- > > After the refactoring the error started appearing like so: > > --- START OF OUTPUT --- > $ git submodule add ../sub/ subm > A git directory for 'subm' is found locally with remote(s): origin /me/git-repos-for-test/sub > fatal: If you want to reuse this local git directory instead of cloning again from > /me/git-repos-for-test/sub > use the '--force' option. If the local git directory is not the correct repo > or if you are unsure what this means, choose another name with the '--name' option. > > --- END OF OUTPUT --- > > As one could observe the remote information is printed along with the > first line rather than on its own line. Also, there's an additional > newline following output. > > Make the error message consistent with the error message that used to be > printed before the refactoring. > > This also moves the 'fatal:' prefix that appears in the middle of the > error message to the first line as it would more appropriate to have > it in the first line. The output after the change would look like: > > --- START OF OUTPUT --- > $ git submodule add ../sub/ subm > fatal: A git directory for 'subm' is found locally with remote(s): > origin /me/git-repos-for-test/sub > If you want to reuse this local git directory instead of cloning again from > /me/git-repos-for-test/sub > use the '--force' option. If the local git directory is not the correct repo > or you are unsure what this means choose another name with the '--name' option. > --- END OF OUTPUT --- > > [1]: https://lore.kernel.org/git/20210710074801.19917-5-raykar.ath@xxxxxxxxx/#t > > Signed-off-by: Kaartic Sivaraam <kaartic.sivaraam@xxxxxxxxx> > --- > builtin/submodule--helper.c | 36 ++++++++++++++++++++++-------------- > 1 file changed, 22 insertions(+), 14 deletions(-) > > diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c > index 414fcb63ea..236da214c6 100644 > --- a/builtin/submodule--helper.c > +++ b/builtin/submodule--helper.c > @@ -2775,7 +2775,7 @@ struct add_data { > }; > #define ADD_DATA_INIT { .depth = -1 } > > -static void show_fetch_remotes(FILE *output, const char *sm_name, const char *git_dir_path) > +static void show_fetch_remotes(struct strbuf *msg, const char *sm_name, const char *git_dir_path) I like the change from using a strbuf instead of passing the output stream and printing to it. But maybe we should rename this function, now that it doesn't really 'show' anything? Probably something like 'append_fetch_remotes()'? > { > struct child_process cp_remote = CHILD_PROCESS_INIT; > struct strbuf sb_remote_out = STRBUF_INIT; > @@ -2791,7 +2791,7 @@ static void show_fetch_remotes(FILE *output, const char *sm_name, const char *gi > while ((next_line = strchr(line, '\n')) != NULL) { > size_t len = next_line - line; > if (strip_suffix_mem(line, &len, " (fetch)")) > - fprintf(output, " %.*s\n", (int)len, line); > + strbuf_addf(msg, " %.*s\n", (int)len, line); > line = next_line + 1; > } > } > @@ -2823,20 +2823,28 @@ static int add_submodule(const struct add_data *add_data) > > if (is_directory(submod_gitdir_path)) { > if (!add_data->force) { > - fprintf(stderr, _("A git directory for '%s' is found " > - "locally with remote(s):"), > - add_data->sm_name); > - show_fetch_remotes(stderr, add_data->sm_name, > + struct strbuf msg = STRBUF_INIT; > + char *die_msg; > + > + strbuf_addf(&msg, _("A git directory for '%s' is found " > + "locally with remote(s):\n"), > + add_data->sm_name); > + > + show_fetch_remotes(&msg, add_data->sm_name, > submod_gitdir_path); > free(submod_gitdir_path); > - die(_("If you want to reuse this local git " > - "directory instead of cloning again from\n" > - " %s\n" > - "use the '--force' option. If the local git " > - "directory is not the correct repo\n" > - "or if you are unsure what this means, choose " > - "another name with the '--name' option.\n"), > - add_data->realrepo); > + > + strbuf_addf(&msg, _("If you want to reuse this local git " > + "directory instead of cloning again from\n" > + " %s\n" > + "use the '--force' option. If the local git " > + "directory is not the correct repo\n" > + "or you are unsure what this means choose " > + "another name with the '--name' option."), > + add_data->realrepo); > + > + die_msg = strbuf_detach(&msg, NULL); > + die("%s", die_msg); > } else { > printf(_("Reactivating local git directory for " > "submodule '%s'\n"), add_data->sm_name); Other than that this patch is an improvement. Thanks for fixing this!
