| Message ID | patch-v3-1.2-879930b7a66-20220421T152704Z-avarab@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | show-brach: segfault fix from Gregory David | expand |
Æ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);
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);
Move the code in cmd_show_branch() that formats a reflog message for us into a function, 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. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- builtin/show-branch.c | 34 +++++++++++++++++++++------------- 1 file changed, 21 insertions(+), 13 deletions(-)