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

From: Phillip Wood <phillip.wood123@gmail.com>
Date: Apr 4, 2022, 15:44 UTC
Message-ID: <e73dc9f4-ad32-d49b-9767-c23c78a7e681@gmail.com>
In-Reply-To: <20220403182250.904933-3-sandals@crustytoothpaste.net>

Hi brian

On 03/04/2022 19:22, brian m. carlson wrote:

Show 20 quoted lines

> 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

Show 41 quoted lines

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