Re: [PATCH v5 3/4] submodule: port set_name_rev() from shell to C

[Junio C Hamano <gitster@pobox.com>]

Prathamesh Chavan <pc44800@gmail.com> writes:

> +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.

> +			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.

> +}
> +
> +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.

> +	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