Re: [PATCH v3 1/2] show-branch: refactor in preparation for next commit

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

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> Move the code in cmd_show_branch() that formats a reflog message for
> us into a function,

Makes quite a lot of sense.

"factoring out the formatting of reflog message, given a single
reflog entry", is a very good unit of work that is independently
good and can be evaluated as such.

> and change the "flags" variable that we never
> change into a "const", in addition to moving it up a scope in
> preparation for the subsequent commit.

Perhaps doing that as part of the next step is better (or not doing
it if it can be helped)?  Mixing it in contaminates a good isolation
done with an unrelated change.  We somehow ended up seeing too many
"while at it" in the recent patches, and I think that needs to stop.
We should go back to better patch hygiene.

Other than that, looks sensible.  Let's see what 2/2 does.

Thanks.


> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  builtin/show-branch.c | 34 +++++++++++++++++++++-------------
>  1 file changed, 21 insertions(+), 13 deletions(-)
>
> diff --git a/builtin/show-branch.c b/builtin/show-branch.c
> index 330b0553b9d..499ef76a508 100644
> --- a/builtin/show-branch.c
> +++ b/builtin/show-branch.c
> @@ -618,6 +618,24 @@ static int parse_reflog_param(const struct option *opt, const char *arg,
>  	return 0;
>  }
>  
> +static char *fmt_reflog(char *const logmsg, const timestamp_t ts, const int tz,
> +			const char *fmt)
> +{
> +	char *const end = strchr(logmsg, '\n');
> +	const char *msg;
> +	char *ret;
> +
> +	if (end)
> +		*end = '\0';
> +
> +	msg = *logmsg ? logmsg : "(none)";
> +
> +	ret = xstrfmt(fmt, show_date(ts, tz, DATE_MODE(RELATIVE)), msg);
> +	free(logmsg);
> +
> +	return ret;
> +}
> +
>  int cmd_show_branch(int ac, const char **av, const char *prefix)
>  {
>  	struct commit *rev[MAX_REVS], *commit;
> @@ -640,6 +658,7 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)
>  	int topics = 0;
>  	int dense = 1;
>  	const char *reflog_base = NULL;
> +	const unsigned int flags = 0;
>  	struct option builtin_show_branch_options[] = {
>  		OPT_BOOL('a', "all", &all_heads,
>  			 N_("show remote-tracking and local branches")),
> @@ -720,7 +739,6 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)
>  		struct object_id oid;
>  		char *ref;
>  		int base = 0;
> -		unsigned int flags = 0;
>  
>  		if (ac == 0) {
>  			static const char *fake_av[2];
> @@ -761,8 +779,6 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)
>  		for (i = 0; i < reflog; i++) {
>  			char *logmsg;
>  			char *nth_desc;
> -			const char *msg;
> -			char *end;
>  			timestamp_t timestamp;
>  			int tz;
>  
> @@ -773,16 +789,8 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)
>  				break;
>  			}
>  
> -			end = strchr(logmsg, '\n');
> -			if (end)
> -				*end = '\0';
> -
> -			msg = (*logmsg == '\0') ? "(none)" : logmsg;
> -			reflog_msg[i] = xstrfmt("(%s) %s",
> -						show_date(timestamp, tz,
> -							  DATE_MODE(RELATIVE)),
> -						msg);
> -			free(logmsg);
> +			reflog_msg[i] = fmt_reflog(logmsg, timestamp, tz,
> +						   "(%s) %s");
>  
>  			nth_desc = xstrfmt("%s@{%d}", *av, base+i);
>  			append_ref(nth_desc, &oid, 1);