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

[Phillip Wood <phillip.wood123@gmail.com>]

Hi brian

On 03/04/2022 19:22, brian m. carlson wrote:
> 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, 21 insertions(+), 13 deletions(-)
> 
> diff --git a/builtin/stash.c b/builtin/stash.c
> index 5897febfbe..4c281a5781 100644
> --- a/builtin/stash.c
> +++ b/builtin/stash.c
> @@ -130,6 +130,24 @@ 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_init(revision, 0);

I think this should become strbuf_reset() and the caller should call 
strbuf_init() once before calling this function (or use STASH_INFO_INIT 
from ab/plug-leak-in-revisions). That should fix one of the leaks Ævar 
was talking about, otherwise we reallocate the strbuf each time this 
function is called and leak the previous allocation.

Best Wishes

Phillip

> +	if (!commit) {
> +		if (!ref_exists(ref_stash)) {
> +			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;
> @@ -157,19 +175,9 @@ static int get_stash_info(struct stash_info *info, int argc, const char **argv)
>   	if (argc == 1)
>   		commit = argv[0];
>   
> -	strbuf_init(&info->revision, 0);
> -	if (!commit) {
> -		if (!ref_exists(ref_stash)) {
> -			free_stash_info(info);
> -			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);
> +	if (parse_revision(&info->revision, commit, 0)) {
> +		free_stash_info(info);
> +		return -1;
>   	}
>   
>   	revision = info->revision.buf;