Re: [PATCH v5 2/4] builtin/stash: factor out revision parsing into a function

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

"brian m. carlson" <sandals@crustytoothpaste.net> writes:

> We allow several special forms of stash names in this code.  In the
> future, we'll want to allow these same forms without parsing a stash
> commit, so let's refactor this code out into a function for reuse.
>
> Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
> ---
>  builtin/stash.c | 34 +++++++++++++++++++++++-----------
>  1 file changed, 23 insertions(+), 11 deletions(-)
>
> diff --git a/builtin/stash.c b/builtin/stash.c
> index cfbd92852a..8ee6752efa 100644
> --- a/builtin/stash.c
> +++ b/builtin/stash.c
> @@ -169,6 +169,25 @@ static void assert_stash_like(struct stash_info *info, const char *revision)
>  		die(_("'%s' is not a stash-like commit"), revision);
>  }
>  
> +static int parse_revision(struct strbuf *revision, const char *commit, int quiet)
> +{
> +	strbuf_reset(revision);
> +	if (!commit) {
> +		if (!refs_ref_exists(get_main_ref_store(the_repository), ref_stash)) {
> +			if (!quiet)
> +				fprintf_ln(stderr, _("No stash entries found."));
> +			return -1;
> +		}
> +
> +		strbuf_addf(revision, "%s@{0}", ref_stash);
> +	} else if (strspn(commit, "0123456789") == strlen(commit)) {
> +		strbuf_addf(revision, "%s@{%s}", ref_stash, commit);
> +	} else {
> +		strbuf_addstr(revision, commit);
> +	}
> +	return 0;
> +}
> +
>  static int get_stash_info(struct stash_info *info, int argc, const char **argv)
>  {
>  	int ret;
> @@ -196,17 +215,10 @@ static int get_stash_info(struct stash_info *info, int argc, const char **argv)
>  	if (argc == 1)
>  		commit = argv[0];
>  
> -	if (!commit) {
> -		if (!refs_ref_exists(get_main_ref_store(the_repository), ref_stash)) {
> -			fprintf_ln(stderr, _("No stash entries found."));
> -			return -1;
> -		}
> -
> -		strbuf_addf(&info->revision, "%s@{0}", ref_stash);
> -	} else if (strspn(commit, "0123456789") == strlen(commit)) {
> -		strbuf_addf(&info->revision, "%s@{%s}", ref_stash, commit);
> -	} else {
> -		strbuf_addstr(&info->revision, commit);
> +	strbuf_init(&info->revision, 0);
> +	if (parse_revision(&info->revision, commit, 0)) {
> +		free_stash_info(info);
> +		return -1;
>  	}

Two comments:

 - It is file-scope static so it is not a huge deal, but it is a bit
   confusing that parse_revision() sounds like a helper function you
   would have in revision.c and call from everywhere.

 - The call to free_stash_info() from inside get_stash_info() is
   probably wrong.  The early error return when ref_stash is missing
   leaves info intact, and the callers of it share this pattern:

        if (get_stash_info(&info, argc - 1, argv + 1))
                goto cleanup;
        ...
    cleanup:
        free_stash_info(&info);
        return ret;

   Even if free_stah_info() happen to be idempotent right now, I do
   not think we want our code to rely on it.