Atharva Raykar [off-list ref] writes:
This is meant to be a faithful conversion from shell to C, with only one
minor change: A warning is emitted if no value is specified in
'submodule.active', ie, the config looks like: "[submodule] active\n",
... meaning that submodule.active is *not* a boolean.
In scripted porcelain, I think we let "submodule--helper is-active"
to inspect its value(s), which may end up feeding a NULL as one of
the pathspec elements when calling parse_pathspec(), so this may
even be a bugfix. In any case, I think "submodule--helper
is-active" is where such a fix should happen and in the longer term,
the code that says "if submodule.active exists, ask is-active and
set submodule.*.active accordingly, otherwise activate everything"
we see in this patch should be simplified to always ask is-active
and let is-active worry about cases like missing submodule.active
and submodule.active being valueless true, so let's not worry too
much about what happens in this patch, because it needs to be
cleaned up anyway after the dust settles.
quoted hunk
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 862053c9f2..9658804d24 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -2936,6 +2936,130 @@ static int add_clone(int argc, const char **argv, const char *prefix)
return 0;
}
+static void config_submodule_in_gitmodules(const char *name, const char *var, const char *value)
+{
+ char *key;
+
+ if (!is_writing_gitmodules_ok())
+ die(_("please make sure that the .gitmodules file is in the working tree"));
+
+ key = xstrfmt("submodule.%s.%s", name, var);
+ config_set_in_gitmodules_file_gently(key, value);
This uses _gently() to avoid dying, but does it discard error return
and hide it from our callers?
+ free(key);
+}
+
+static void configure_added_submodule(struct add_data *add_data)
+{
+ char *key, *submod_pathspec = NULL;
+ struct child_process add_submod = CHILD_PROCESS_INIT;
+ struct child_process add_gitmodules = CHILD_PROCESS_INIT;
+ int pathspec_key_exists, activate = 0;
+
+ key = xstrfmt("submodule.%s.url", add_data->sm_name);
+ git_config_set_gently(key, add_data->realrepo);
+ free(key);
+
+ add_submod.git_cmd = 1;
+ strvec_pushl(&add_submod.args, "add",
+ "--no-warn-embedded-repo", NULL);
+ if (add_data->force)
+ strvec_push(&add_submod.args, "--force");
+ strvec_pushl(&add_submod.args, "--", add_data->sm_path, NULL);
+
+ if (run_command(&add_submod))
+ die(_("Failed to add submodule '%s'"), add_data->sm_path);
+
+ config_submodule_in_gitmodules(add_data->sm_name, "path", add_data->sm_path);
+ config_submodule_in_gitmodules(add_data->sm_name, "url", add_data->repo);
+ if (add_data->branch)
+ config_submodule_in_gitmodules(add_data->sm_name,
+ "branch", add_data->branch);
A failure in any of the above in the scripted version used to result
in "failed to register submodule" error, but they are now ignored.
Intended?
+ add_gitmodules.git_cmd = 1;
+ strvec_pushl(&add_gitmodules.args,
+ "add", "--force", "--", ".gitmodules", NULL);
+
+ if (run_command(&add_gitmodules))
+ die(_("Failed to register submodule '%s'"), add_data->sm_path);
+
+ /*
+ * NEEDSWORK: In a multi-working-tree world this needs to be
+ * set in the per-worktree config.
+ */
+ pathspec_key_exists = !git_config_get_string("submodule.active",
+ &submod_pathspec);
+ if (pathspec_key_exists && !submod_pathspec) {
+ warning(_("The submodule.active configuration exists, but the "
+ "pathspec was unset. If the submodule is not already "
+ "active, the value of submodule.%s.active will be "
+ "be set to 'true'."), add_data->sm_name);
+ activate = 1;
+ }
+
+ /*
+ * If submodule.active does not exist, or if the pathspec was unset,
+ * we will activate this module unconditionally.
+ *
+ * Otherwise, we ask is_submodule_active(), which iterates
+ * through all the values of 'submodule.active' to determine
+ * if this module is already active.
+ */
+ if (!pathspec_key_exists || activate ||
+ !is_submodule_active(the_repository, add_data->sm_path)) {
+ key = xstrfmt("submodule.%s.active", add_data->sm_name);
+ git_config_set_gently(key, "true");
+ free(key);
+ }
This is the part I discussed earlier. I think this "optimize so
that we can avoid calling is_submodule_active()" should go away in
the long run. In the current code, is_submodule_active() needs to
find out the value of submodule.active itself anyway, so the
short-circuit is not working as an optimization.
Other than the "what happens when we see errors?" issue, the patch
looks quite straight-forward rewrite from the scripted version.
Thanks.
On 24/07/21 02:06, Junio C Hamano wrote:
Atharva Raykar [off-list ref] writes:
quoted
This is meant to be a faithful conversion from shell to C, with only one
minor change: A warning is emitted if no value is specified in
'submodule.active', ie, the config looks like: "[submodule] active\n",
... meaning that submodule.active is *not* a boolean.
In scripted porcelain, I think we let "submodule--helper is-active"
to inspect its value(s), which may end up feeding a NULL as one of
the pathspec elements when calling parse_pathspec(), so this may
even be a bugfix. In any case, I think "submodule--helper
is-active" is where such a fix should happen and in the longer term,
the code that says "if submodule.active exists, ask is-active and
set submodule.*.active accordingly, otherwise activate everything"
we see in this patch should be simplified to always ask is-active
and let is-active worry about cases like missing submodule.active
and submodule.active being valueless true, so let's not worry too
much about what happens in this patch, because it needs to be
cleaned up anyway after the dust settles.
Okay, that makes sense. I'll remove the extra warning and special
handling and make it a bug-for-bug conversion for now, so that the
cleanup can be handled afterwards. It will probably be more fitting to
have this change 'is_submodule_active()' afterwards. I'll maybe add a
NEEDSWORK comment for now?
quoted
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 862053c9f2..9658804d24 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -2936,6 +2936,130 @@ static int add_clone(int argc, const char **argv, const char *prefix)
return 0;
}
+static void config_submodule_in_gitmodules(const char *name, const char *var, const char *value)
+{
+ char *key;
+
+ if (!is_writing_gitmodules_ok())
+ die(_("please make sure that the .gitmodules file is in the working tree"));
+
+ key = xstrfmt("submodule.%s.%s", name, var);
+ config_set_in_gitmodules_file_gently(key, value);
This uses _gently() to avoid dying, but does it discard error return
and hide it from our callers?
quoted
+ free(key);
+}
+
+static void configure_added_submodule(struct add_data *add_data)
+{
+ char *key, *submod_pathspec = NULL;
+ struct child_process add_submod = CHILD_PROCESS_INIT;
+ struct child_process add_gitmodules = CHILD_PROCESS_INIT;
+ int pathspec_key_exists, activate = 0;
+
+ key = xstrfmt("submodule.%s.url", add_data->sm_name);
+ git_config_set_gently(key, add_data->realrepo);
+ free(key);
+
+ add_submod.git_cmd = 1;
+ strvec_pushl(&add_submod.args, "add",
+ "--no-warn-embedded-repo", NULL);
+ if (add_data->force)
+ strvec_push(&add_submod.args, "--force");
+ strvec_pushl(&add_submod.args, "--", add_data->sm_path, NULL);
+
+ if (run_command(&add_submod))
+ die(_("Failed to add submodule '%s'"), add_data->sm_path);
+
+ config_submodule_in_gitmodules(add_data->sm_name, "path", add_data->sm_path);
+ config_submodule_in_gitmodules(add_data->sm_name, "url", add_data->repo);
+ if (add_data->branch)
+ config_submodule_in_gitmodules(add_data->sm_name,
+ "branch", add_data->branch);
A failure in any of the above in the scripted version used to result
in "failed to register submodule" error, but they are now ignored.
Intended?
This was not intended. I think I did not notice those expressions were
chained in the scripted version. I'll fix this.
quoted
+ add_gitmodules.git_cmd = 1;
+ strvec_pushl(&add_gitmodules.args,
+ "add", "--force", "--", ".gitmodules", NULL);
+
+ if (run_command(&add_gitmodules))
+ die(_("Failed to register submodule '%s'"), add_data->sm_path);
+
+ /*
+ * NEEDSWORK: In a multi-working-tree world this needs to be
+ * set in the per-worktree config.
+ */
+ pathspec_key_exists = !git_config_get_string("submodule.active",
+ &submod_pathspec);
+ if (pathspec_key_exists && !submod_pathspec) {
+ warning(_("The submodule.active configuration exists, but the "
+ "pathspec was unset. If the submodule is not already "
+ "active, the value of submodule.%s.active will be "
+ "be set to 'true'."), add_data->sm_name);
+ activate = 1;
+ }
+
+ /*
+ * If submodule.active does not exist, or if the pathspec was unset,
+ * we will activate this module unconditionally.
+ *
+ * Otherwise, we ask is_submodule_active(), which iterates
+ * through all the values of 'submodule.active' to determine
+ * if this module is already active.
+ */
+ if (!pathspec_key_exists || activate ||
+ !is_submodule_active(the_repository, add_data->sm_path)) {
+ key = xstrfmt("submodule.%s.active", add_data->sm_name);
+ git_config_set_gently(key, "true");
+ free(key);
+ }
This is the part I discussed earlier. I think this "optimize so
that we can avoid calling is_submodule_active()" should go away in
the long run. In the current code, is_submodule_active() needs to
find out the value of submodule.active itself anyway, so the
short-circuit is not working as an optimization.
Agreed.
Other than the "what happens when we see errors?" issue, the patch
looks quite straight-forward rewrite from the scripted version.
Thanks.