Re: [PATCH 07/15] name-rev: extract creating/updating a 'struct name_rev' into a helper

[Derrick Stolee <stolee@gmail.com>]

On 9/19/2019 5:47 PM, SZEDER Gábor wrote:
> In a later patch in this series we'll want to do this in two places.
> 
> Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
> ---
>  builtin/name-rev.c | 40 +++++++++++++++++++++++++++-------------
>  1 file changed, 27 insertions(+), 13 deletions(-)
> 
> diff --git a/builtin/name-rev.c b/builtin/name-rev.c
> index dec2228cc7..cb8ac2fa64 100644
> --- a/builtin/name-rev.c
> +++ b/builtin/name-rev.c
> @@ -75,12 +75,36 @@ static int is_better_name(struct rev_name *name,
>  	return 0;
>  }
>  
> +static struct rev_name *create_or_update_name(struct commit *commit,
> +					      const char *tip_name,
> +					      timestamp_t taggerdate,
> +					      int generation, int distance,
> +					      int from_tag)
> +{
> +	struct rev_name *name = get_commit_rev_name(commit);
> +
> +	if (name == NULL) {
> +		name = xmalloc(sizeof(*name));
> +		set_commit_rev_name(commit, name);
> +		goto copy_data;
> +	} else if (is_better_name(name, taggerdate, distance, from_tag)) {
> +copy_data:
> +		name->tip_name = tip_name;
> +		name->taggerdate = taggerdate;
> +		name->generation = generation;
> +		name->distance = distance;
> +		name->from_tag = from_tag;
> +
> +		return name;
> +	} else
> +		return NULL;
> +}
> +
>  static void name_rev(struct commit *commit,
>  		const char *tip_name, timestamp_t taggerdate,
>  		int generation, int distance, int from_tag,
>  		int deref)
>  {
> -	struct rev_name *name = get_commit_rev_name(commit);

A perhaps small benefit: we delay this call until after some
other checks happen. It's just looking up data in a cache, but
it may help a little.

>  	struct commit_list *parents;
>  	int parent_number = 1;
>  	char *to_free = NULL;
> @@ -97,18 +121,8 @@ static void name_rev(struct commit *commit,
>  			die("generation: %d, but deref?", generation);
>  	}
>  
> -	if (name == NULL) {
> -		name = xmalloc(sizeof(*name));
> -		set_commit_rev_name(commit, name);
> -		goto copy_data;
> -	} else if (is_better_name(name, taggerdate, distance, from_tag)) {
> -copy_data:
> -		name->tip_name = tip_name;
> -		name->taggerdate = taggerdate;
> -		name->generation = generation;
> -		name->distance = distance;
> -		name->from_tag = from_tag;
> -	} else {
> +	if (!create_or_update_name(commit, tip_name, taggerdate, generation,
> +				   distance, from_tag)) {

Otherwise this method extraction looks correct.

Thanks,
-Stolee