Re: [GSoC][PATCH 3/5 v3] submodule: port set_name_rev() from shell to C

[Junio C Hamano]

Prathamesh Chavan  writes:

> Function set_name_rev() is ported from git-submodule to the
> submodule--helper builtin. The function get_name_rev() generates the
> value of the revision name as required, and the function
> print_name_rev() handles the formating and printing of the obtained
> revision name.
>
> Mentored-by: Christian Couder 
> Mentored-by: Stefan Beller 
> Signed-off-by: Prathamesh Chavan 
> ---
>  builtin/submodule--helper.c | 69 
> +
>  git-submodule.sh| 16 ++-
>  2 files changed, 71 insertions(+), 14 deletions(-)
>
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index c4286aac5..4103e40e4 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -244,6 +244,74 @@ static char *get_submodule_displaypath(const char *path, 
> const char *prefix)
>   }
>  }
>  
> +enum describe_step {
> + step_bare,
> + step_tags,
> + step_contains,
> + step_all_always,
> + step_end
> +};

Hmph.

The only difference between the steps being what subcommand is run,
a better implementation might be to do

static const char *describe_bare[] = {
NULL
};

...

static const char **describe_argv[] = {
describe_bare, describe_tags, describe_contains, 
describe_all_always, NULL
};

const char ***d;

for (d = describe_argv; *d; d++) {
argv_array_pushl(, "describe");
argv_array_pushv(, *d);
... do the thing ...
}

but unfortunately C is a bit klunky to do so; we cannot easily make
the contents of describe_argv[] as anonymous arrays.  An otherwise
useless enum stil bothers me, but I do not think of anything better
offhand.

> +
> +static char *get_name_rev(const char *sub_path, const char* object_id)
> +{
> + struct strbuf sb = STRBUF_INIT;
> + enum describe_step cur_step;
> +
> + for (cur_step = step_bare; cur_step < step_end; cur_step++) {
> + struct child_process cp = CHILD_PROCESS_INIT;
> + prepare_submodule_repo_env(_array);
> + cp.dir = sub_path;
> + cp.git_cmd = 1;
> + cp.no_stderr = 1;
> +
> + switch (cur_step) {
> + case step_bare:
> + argv_array_pushl(, "describe",
> +  object_id, NULL);
> + break;
> + case step_tags: 
> + argv_array_pushl(, "describe",
> +  "--tags", object_id, NULL);
> + break;
> + case step_contains:
> + argv_array_pushl(, "describe",
> +  "--contains", object_id,
> +  NULL);
> + break;
> + case step_all_always:
> + argv_array_pushl(, "describe",
> +  "--all", "--always",
> +  object_id, NULL);
> + break;
> + default:
> + BUG("unknown describe step '%d'", cur_step);
> + }

Dedent the body of switch() by one level, i.e.

switch (var) {
case val1:
do_this();
break;
case val2:
do_that();
...
}

Otherwise looking good.

> @@ -1042,14 +1030,14 @@ cmd_status()
>   fi
>   if git diff-files --ignore-submodules=dirty --quiet -- 
> "$sm_path"
>   then
> - set_name_rev "$sm_path" "$sha1"
> + revname=$(git submodule--helper print-name-rev 
> "$sm_path" "$sha1")
>   say " $sha1 $displaypath$revname"
>   else
>   if test -z "$cached"
>   then
>   sha1=$(sanitize_submodule_env; cd "$sm_path" && 
> git rev-parse --verify HEAD)
>   fi
> - set_name_rev "$sm_path" "$sha1"
> + revname=$(git submodule--helper print-name-rev 
> "$sm_path" "$sha1")
>   say "+$sha1 $displaypath$revname"
>   fi

[GSoC][PATCH 3/5 v3] submodule: port set_name_rev() from shell to C

[Prathamesh Chavan]

Function set_name_rev() is ported from git-submodule to the
submodule--helper builtin. The function get_name_rev() generates the
value of the revision name as required, and the function
print_name_rev() handles the formating and printing of the obtained
revision name.

Mentored-by: Christian Couder 
Mentored-by: Stefan Beller 
Signed-off-by: Prathamesh Chavan 
---
 builtin/submodule--helper.c | 69 +
 git-submodule.sh| 16 ++-
 2 files changed, 71 insertions(+), 14 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index c4286aac5..4103e40e4 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -244,6 +244,74 @@ static char *get_submodule_displaypath(const char *path, 
const char *prefix)
}
 }
 
+enum describe_step {
+   step_bare,
+   step_tags,
+   step_contains,
+   step_all_always,
+   step_end
+};
+
+static char *get_name_rev(const char *sub_path, const char* object_id)
+{
+   struct strbuf sb = STRBUF_INIT;
+   enum describe_step cur_step;
+
+   for (cur_step = step_bare; cur_step < step_end; cur_step++) {
+   struct child_process cp = CHILD_PROCESS_INIT;
+   prepare_submodule_repo_env(_array);
+   cp.dir = sub_path;
+   cp.git_cmd = 1;
+   cp.no_stderr = 1;
+
+   switch (cur_step) {
+   case step_bare:
+   argv_array_pushl(, "describe",
+object_id, NULL);
+   break;
+   case step_tags: 
+   argv_array_pushl(, "describe",
+"--tags", object_id, NULL);
+   break;
+   case step_contains:
+   argv_array_pushl(, "describe",
+"--contains", object_id,
+NULL);
+   break;
+   case step_all_always:
+   argv_array_pushl(, "describe",
+"--all", "--always",
+object_id, NULL);
+   break;
+   default:
+   BUG("unknown describe step '%d'", cur_step);
+   }
+
+   if (!capture_command(, , 0) && sb.len) {
+   strbuf_strip_suffix(, "\n");
+   return strbuf_detach(, NULL);
+   }
+
+   }
+
+   strbuf_release();
+   return NULL;
+}
+
+static int print_name_rev(int argc, const char **argv, const char *prefix)
+{
+   char *namerev;
+   if (argc != 3)
+   die("print-name-rev only accepts two arguments:  ");
+
+   namerev = get_name_rev(argv[1], argv[2]);
+   if (namerev && namerev[0])
+   printf(" (%s)", namerev);
+   printf("\n");
+
+   return 0;
+}
+
 struct module_list {
const struct cache_entry **entries;
int alloc, nr;
@@ -1242,6 +1310,7 @@ static struct cmd_struct commands[] = {
{"relative-path", resolve_relative_path, 0},
{"resolve-relative-url", resolve_relative_url, 0},
{"resolve-relative-url-test", resolve_relative_url_test, 0},
+   {"print-name-rev", print_name_rev, 0},
{"init", module_init, SUPPORT_SUPER_PREFIX},
{"remote-branch", resolve_remote_submodule_branch, 0},
{"push-check", push_check, 0},
diff --git a/git-submodule.sh b/git-submodule.sh
index e131760ee..e988167e0 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -759,18 +759,6 @@ cmd_update()
}
 }
 
-set_name_rev () {
-   revname=$( (
-   sanitize_submodule_env
-   cd "$1" && {
-   git describe "$2" 2>/dev/null ||
-   git describe --tags "$2" 2>/dev/null ||
-   git describe --contains "$2" 2>/dev/null ||
-   git describe --all --always "$2"
-   }
-   ) )
-   test -z "$revname" || revname=" ($revname)"
-}
 #
 # Show commit summary for submodules in index or working tree
 #
@@ -1042,14 +1030,14 @@ cmd_status()
fi
if git diff-files --ignore-submodules=dirty --quiet -- 
"$sm_path"
then
-   set_name_rev "$sm_path" "$sha1"
+   revname=$(git submodule--helper print-name-rev 
"$sm_path" "$sha1")
say " $sha1 $displaypath$revname"
else
if test -z "$cached"
then
sha1=$(sanitize_submodule_env; cd