Prathamesh Chavan <pc44...@gmail.com> writes:
> Introduce function get_submodule_displaypath() to replace the code
> occurring in submodule_init() for generating displaypath of the
> submodule with a call to it.
Looks like a quite straight-forward refactoring.
> +static char *get_submodule_displaypath(const char *path, const char *prefix)
> +{
> + const char *super_prefix = get_super_prefix();
> +
> + if (prefix && super_prefix) {
> + BUG("cannot have prefix '%s' and superprefix '%s'",
> + prefix, super_prefix);
> + } else if (prefix) {
> + struct strbuf sb = STRBUF_INIT;
> + char *displaypath = xstrdup(relative_path(path, prefix, &sb));
> + strbuf_release(&sb);
> + return displaypath;
Use of xstrdup() would waste one extra allocation and copy, no? In
other words, wouldn't this do the same thing?
... {
struct strbuf sb = STRBUF_INIT;
relative_path(path, prefix, &sb);
return strbuf_detach(&sb, NULL);
> @@ -363,7 +375,6 @@ static void init_submodule(const char *path, const char
> *prefix, int quiet)
> * Set active flag for the submodule being initialized
> */
> if (!is_submodule_active(the_repository, path)) {
> - strbuf_reset(&sb);
> strbuf_addf(&sb, "submodule.%s.active", sub->name);
> git_config_set_gently(sb.buf, "true");
> }
This is because sb will stay untouched with the use of the new
helper. With the way this single strbuf is reused throughout this
function, I cannot help wondering if the prevailing pattern of
resetting and then using is a mistake. If the strbuf is mostly used
as a scratchpad, wouldn't it make more sense to use it and then
clean up when you are done with it?
I.e. something along this line that shows only two uses...
builtin/submodule--helper.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 0ff9dd0b85..84ded4b2e9 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -363,9 +363,9 @@ static void init_submodule(const char *path, const char
*prefix, int quiet)
* Set active flag for the submodule being initialized
*/
if (!is_submodule_active(the_repository, path)) {
- strbuf_reset(&sb);
strbuf_addf(&sb, "submodule.%s.active", sub->name);
git_config_set_gently(sb.buf, "true");
+ strbuf_reset(&sb);
}
/*
@@ -373,7 +373,6 @@ static void init_submodule(const char *path, const char
*prefix, int quiet)
* To look up the url in .git/config, we must not fall back to
* .gitmodules, so look it up directly.
*/
- strbuf_reset(&sb);
strbuf_addf(&sb, "submodule.%s.url", sub->name);
if (git_config_get_string(sb.buf, &url)) {
if (!sub->url)
@@ -410,9 +409,9 @@ static void init_submodule(const char *path, const char
*prefix, int quiet)
_("Submodule '%s' (%s) registered for path
'%s'\n"),
sub->name, url, displaypath);
}
+ strbuf_reset(&sb);
/* Copy "update" setting when it is not set yet */
- strbuf_reset(&sb);
strbuf_addf(&sb, "submodule.%s.update", sub->name);
if (git_config_get_string(sb.buf, &upd) &&
sub->update_strategy.type != SM_UPDATE_UNSPECIFIED) {
