Re: [PATCH v2 6/6] submodule--helper: introduce new update-module-mode helper

[Junio C Hamano]

SZEDER Gábor  writes:

>> +fprintf(stdout, submodule_strategy_to_string(_strategy));
>
> Various compilers warn about the potential insecurity of the above
> call:
>
>   CC builtin/submodule--helper.o
>   builtin/submodule--helper.c: In function ‘module_update_module_mode’:
>   builtin/submodule--helper.c:1502:2: error: format not a string literal and 
> no format arguments [-Werror=format-security]
> fprintf(stdout, submodule_strategy_to_string(_strategy));
> ^
>   cc1: all warnings being treated as errors
>   Makefile:2261: recipe for target 'builtin/submodule--helper.o' failed
>   make: *** [builtin/submodule--helper.o] Error 1
>
> I think it should either use an explicit format string:
>
>   fprintf(stdout, "%s", submodule_strategy_to_string(_strategy));
>
> or, perhaps better yet, simply use fputs().

Sounds good.

Re: [PATCH v2 6/6] submodule--helper: introduce new update-module-mode helper

[SZEDER Gábor]

> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index 034ba1bb2e0..d4cb7c72e33 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c


> +static int module_update_module_mode(int argc, const char **argv, const char 
> *prefix)
> +{
> + const char *path, *update = NULL;
> + int just_cloned;
> + struct submodule_update_strategy update_strategy = { .type = 
> SM_UPDATE_CHECKOUT };
> +
> + if (argc < 3 || argc > 4)
> + die("submodule--helper update-module-clone expects 
>   []");
> +
> + just_cloned = git_config_int("just_cloned", argv[1]);
> + path = argv[2];
> +
> + if (argc == 4)
> + update = argv[3];
> +
> + determine_submodule_update_strategy(the_repository,
> + just_cloned, path, update,
> + _strategy);
> + fprintf(stdout, submodule_strategy_to_string(_strategy));

Various compilers warn about the potential insecurity of the above
call:

  CC builtin/submodule--helper.o
  builtin/submodule--helper.c: In function ‘module_update_module_mode’:
  builtin/submodule--helper.c:1502:2: error: format not a string literal and no 
format arguments [-Werror=format-security]
fprintf(stdout, submodule_strategy_to_string(_strategy));
^
  cc1: all warnings being treated as errors
  Makefile:2261: recipe for target 'builtin/submodule--helper.o' failed
  make: *** [builtin/submodule--helper.o] Error 1

I think it should either use an explicit format string:

  fprintf(stdout, "%s", submodule_strategy_to_string(_strategy));

or, perhaps better yet, simply use fputs().


> +
> + return 0;
> +}

[PATCH v2 6/6] submodule--helper: introduce new update-module-mode helper

[Stefan Beller]

This chews off a bit of the shell part of the update command in
git-submodule.sh. When writing the C code, keep in mind that the
submodule--helper part will go away eventually and we want to have
a C function that is able to determine the submodule update strategy,
it as a nicety, make determine_submodule_update_strategy accessible
for arbitrary repositories.

Signed-off-by: Stefan Beller 
---
 builtin/submodule--helper.c | 61 +
 git-submodule.sh| 16 +-
 2 files changed, 62 insertions(+), 15 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 034ba1bb2e0..d4cb7c72e33 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -1444,6 +1444,66 @@ static int module_clone(int argc, const char **argv, 
const char *prefix)
return 0;
 }
 
+static void determine_submodule_update_strategy(struct repository *r,
+   int just_cloned,
+   const char *path,
+   const char *update,
+   struct 
submodule_update_strategy *out)
+{
+   const struct submodule *sub = submodule_from_path(r, _oid, path);
+   char *key;
+   const char *val;
+
+   key = xstrfmt("submodule.%s.update", sub->name);
+
+   if (update) {
+   trace_printf("parsing update");
+   if (parse_submodule_update_strategy(update, out) < 0)
+   die(_("Invalid update mode '%s' for submodule path 
'%s'"),
+   update, path);
+   } else if (!repo_config_get_string_const(r, key, )) {
+   if (parse_submodule_update_strategy(val, out) < 0)
+   die(_("Invalid update mode '%s' configured for 
submodule path '%s'"),
+   val, path);
+   } else if (sub->update_strategy.type != SM_UPDATE_UNSPECIFIED) {
+   trace_printf("loaded thing");
+   out->type = sub->update_strategy.type;
+   out->command = sub->update_strategy.command;
+   } else
+   out->type = SM_UPDATE_CHECKOUT;
+
+   if (just_cloned &&
+   (out->type == SM_UPDATE_MERGE ||
+out->type == SM_UPDATE_REBASE ||
+out->type == SM_UPDATE_NONE))
+   out->type = SM_UPDATE_CHECKOUT;
+
+   free(key);
+}
+
+static int module_update_module_mode(int argc, const char **argv, const char 
*prefix)
+{
+   const char *path, *update = NULL;
+   int just_cloned;
+   struct submodule_update_strategy update_strategy = { .type = 
SM_UPDATE_CHECKOUT };
+
+   if (argc < 3 || argc > 4)
+   die("submodule--helper update-module-clone expects 
  []");
+
+   just_cloned = git_config_int("just_cloned", argv[1]);
+   path = argv[2];
+
+   if (argc == 4)
+   update = argv[3];
+
+   determine_submodule_update_strategy(the_repository,
+   just_cloned, path, update,
+   _strategy);
+   fprintf(stdout, submodule_strategy_to_string(_strategy));
+
+   return 0;
+}
+
 struct update_clone_data {
const struct submodule *sub;
struct object_id oid;
@@ -2039,6 +2099,7 @@ static struct cmd_struct commands[] = {
{"list", module_list, 0},
{"name", module_name, 0},
{"clone", module_clone, 0},
+   {"update-module-mode", module_update_module_mode, 0},
{"update-clone", update_clone, 0},
{"relative-path", resolve_relative_path, 0},
{"resolve-relative-url", resolve_relative_url, 0},
diff --git a/git-submodule.sh b/git-submodule.sh
index 56588aa304d..215760898ce 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -535,27 +535,13 @@ cmd_update()
do
die_if_unmatched "$quickabort" "$sha1"
 
-   name=$(git submodule--helper name "$sm_path") || exit
-   if ! test -z "$update"
-   then
-   update_module=$update
-   else
-   update_module=$(git config submodule."$name".update)
-   if test -z "$update_module"
-   then
-   update_module="checkout"
-   fi
-   fi
+   update_module=$(git submodule--helper update-module-mode 
$just_cloned "$sm_path" $update)
 
displaypath=$(git submodule--helper relative-path 
"$prefix$sm_path" "$wt_prefix")
 
if test $just_cloned -eq 1
then
subsha1=
-   case "$update_module" in
-   merge | rebase | none)
-   update_module=checkout ;;
-   esac
else