Re: [PATCH 2/1] revision: simplify mark_argv_for_free() callers

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

Jeff King <peff@peff.net> writes:

> BTW, this is a small cleanup that I resisted putting into the earlier
> commit in order to keep it focused. But maybe worth doing on top?

I like it.  It is a tiny simplification but makes the callers easier
to read.

>
> -- >8 --
> Subject: revision: simplify mark_argv_for_free() callers
>
> You do not want to mark an argv element for freeing unless the caller
> has given us the free_removed_argv_elements flag. Originally we just
> called free() in this case, so each caller checked the flag itself. Now
> that we mark them via a helper function, we can push the check down into
> the helper. This saves a little bit of duplicated code, but also
> hopefully makes the result conceptually simpler.
>
> Every caller but one was already checking this flag. The exception is
> setup_revisions_from_strvec(), but it always sets the flag explicitly
> (since its whole purpose is managing argv memory). So even though it was
> not checking the flag, doing so is OK (it will always be set).
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  revision.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/revision.c b/revision.c
> index 7aee96bd8e..59d6372506 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -2317,8 +2317,11 @@ static timestamp_t parse_age(const char *arg)
>   * usual const argv array, even when opt->free_removed_argv_elements is set).
>   * We cast away the const on their behalf.
>   */
> -static void mark_argv_for_free(struct rev_info *revs, const char *str)
> +static void mark_argv_for_free(const struct setup_revision_opt *opt,
> +			       struct rev_info *revs, const char *str)
>  {
> +	if (!opt || !opt->free_removed_argv_elements)
> +		return;
>  	if (!str)
>  		return;
>  	strvec_push_nodup(&revs->argv_to_free, (char *)str);
> @@ -2335,8 +2338,7 @@ static void overwrite_argv(int *argc, const char **argv,
>  	 * cases around the free() and NULL operations.
>  	 */
>  	if (*value != argv[*argc]) {
> -		if (opt && opt->free_removed_argv_elements)
> -			mark_argv_for_free(revs, argv[*argc]);
> +		mark_argv_for_free(opt, revs, argv[*argc]);
>  		argv[*argc] = *value;
>  		*value = NULL;
>  	}
> @@ -3055,8 +3057,7 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
>  			const char *arg = argv[i];
>  			if (strcmp(arg, "--"))
>  				continue;
> -			if (opt && opt->free_removed_argv_elements)
> -				mark_argv_for_free(revs, argv[i]);
> +			mark_argv_for_free(opt, revs, argv[i]);
>  			argv[i] = NULL;
>  			argc = i;
>  			if (argv[i + 1])
> @@ -3260,8 +3261,7 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
>  	}
>  
>  	if (argv) {
> -		if (opt && opt->free_removed_argv_elements)
> -			mark_argv_for_free(revs, argv[left]);
> +		mark_argv_for_free(opt, revs, argv[left]);
>  		argv[left] = NULL;
>  	}
>  
> @@ -3283,7 +3283,7 @@ void setup_revisions_from_strvec(struct strvec *argv, struct rev_info *revs,
>  	ret = setup_revisions(argv->nr, argv->v, revs, opt);
>  
>  	for (size_t i = ret; i < argv->nr; i++)
> -		mark_argv_for_free(revs, argv->v[i]);
> +		mark_argv_for_free(opt, revs, argv->v[i]);
>  	argv->nr = ret;
>  }