Re: [PATCH v5 3/4] submodule: port set_name_rev() from shell to C
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Sep 25, 2017, 03:51 UTC
- Message-ID
- <xmqqfubbfnan.fsf@gitster.mtv.corp.google.com>
- In-Reply-To
- <20170924120858.26813-4-pc44800@gmail.com>
Prathamesh Chavan <pc44800@gmail.com> writes:
Show 24 quoted lines
> +static char *compute_rev_name(const char *sub_path, const char* object_id)
> +{
> + struct strbuf sb = STRBUF_INIT;
> + const char ***d;
> +
> + static const char *describe_bare[] = {
> + NULL
> + };
> +...
> + static const char **describe_argv[] = {
> + describe_bare, describe_tags, describe_contains,
> + describe_all_always, NULL
> + };
> +
> + for (d = describe_argv; *d; d++) {
> + struct child_process cp = CHILD_PROCESS_INIT;
> + prepare_submodule_repo_env(&cp.env_array);
> + cp.dir = sub_path;
> + cp.git_cmd = 1;
> + cp.no_stderr = 1;
> +
> + argv_array_push(&cp.args, "describe");
> + argv_array_pushv(&cp.args, *d);
> + argv_array_push(&cp.args, object_id);Nicely done.
> + if (!capture_command(&cp, &sb, 0) && sb.len) {Observation: even if the command did not fail, if it returned an empty string, then you would reject that result and loop on here. This is different from the original scripted version, but it should be OK.
Show 7 quoted lines
> + strbuf_strip_suffix(&sb, "\n"); > + return strbuf_detach(&sb, NULL); > + } > + } > + > + strbuf_release(&sb); > + return NULL;
Observation: and if you do not find any non-empty-string answer, you return NULL.
Show 11 quoted lines
> +}
> +
> +static int get_rev_name(int argc, const char **argv, const char *prefix)
> +{
> + char *revname;
> + if (argc != 3)
> + die("get-rev-name only accepts two arguments: <path> <sha1>");
> +
> + revname = compute_rev_name(argv[1], argv[2]);
> + if (revname && revname[0])
> + printf(" (%s)", revname);So, while it is not wrong per-se, I do not think we need to check revname[0] here. The helper never returns a non-NULL pointer that points at an empty string, right?
On the other hand, if we dropped the "&& sb.len" check in the helper function to be more faithful to the original, then we must check revname[0] for an empty string.
Show 56 quoted lines
> + printf("\n");
> +
> + free(revname);
> + return 0;
> +}
> +
> struct module_list {
> const struct cache_entry **entries;
> int alloc, nr;
> @@ -1293,6 +1355,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},
> + {"get-rev-name", get_rev_name, 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 66d1ae8ef..5211361c5 100755
> --- a/git-submodule.sh
> +++ b/git-submodule.sh
> @@ -758,18 +758,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
> #
> @@ -1041,14 +1029,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 get-rev-name "$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 get-rev-name "$sm_path" "$sha1")
> say "+$sha1 $displaypath$revname"
> fi