Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> 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@xxxxxxxxx>
> ---
> 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);
