| Message ID | 449c58009ae46d43ec2bd0499674c9858b706d04.1572897736.git.liu.denton@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | learn the "summary" pretty format | expand |
Denton Liu <liu.denton@gmail.com> writes: > Subject: Re: [PATCH 6/8] reflog-walk.c: don't print last newline with oneline Please find a better phrasing that does not mislead the readers to think "eek, so now output from 'git log -g --oneline' does not get LF terminated?" I think you are just changing the place where the LF is given from the callee that shows a single record (and then used to show LF after the record) to the caller that calls the callee (and now it shows LF after the call returns), in order to preserve the output, but with the given proposed log message, it is not clear if that is what you meant to do (I can read from the code what _is_ going on, but the log is to record what you _wanted_ to do, which can be different). > @@ -661,8 +661,10 @@ void show_log(struct rev_info *opt) In the pre-context before this line there is a guard to ensure that we do this only when showing an entry from the reflog, so the effect of this hunk is "when showing from reflog in --oneline format, show LF after showing a single record" ... > opt->commit_format == CMIT_FMT_ONELINE, > &opt->date_mode, > opt->date_mode_explicit); > - if (opt->commit_format == CMIT_FMT_ONELINE) > + if (opt->commit_format == CMIT_FMT_ONELINE) { > + putc('\n', opt->diffopt.file); > return; > + } > } > } > > diff --git a/reflog-walk.c b/reflog-walk.c > index 3a25b27d8f..e2b4c0b290 100644 > --- a/reflog-walk.c > +++ b/reflog-walk.c > @@ -285,7 +285,11 @@ void show_reflog_message(struct reflog_walk_info *reflog_info, int oneline, > info = &commit_reflog->reflogs->items[commit_reflog->recno+1]; > get_reflog_selector(&selector, reflog_info, dmode, force_date, 0); > if (oneline) { > - printf("%s: %s", selector.buf, info->message); > + struct strbuf message = STRBUF_INIT; > + strbuf_addstr(&message, info->message); > + strbuf_trim_trailing_newline(&message); > + printf("%s: %s", selector.buf, message.buf); > + strbuf_release(&message); ... and this one is what gets called from the above caller; we lost the final LF (if there is) from the output, that is compensated by the caller. How well do we work with the "-z" output format, by the way, with the hardcoded '\n' on the output codepath? Making a new allocation feels inefficient just to omit the trailing newlines. I wonder if a helper function that takes a "const char *" and returns a "const char *" in that string that points at the CRLF or LF or NUL at its end would be warranted. If we do not care about CRLF, this part would become something like... if (oneline) { const char *endp = strchrnul(info->message, '\n'); printf("%s: %.*s", selector.buf, (int)(endp - info->message), info->message); } but I didn't trace the code/data flow to see where info->message comes from well enough to tell if we need to worry about CRLF. I thought reflogs are always written with LF termination, though. Thanks.
Hi Junio, On Wed, Nov 06, 2019 at 02:12:17PM +0900, Junio C Hamano wrote: > Denton Liu <liu.denton@gmail.com> writes: > > > Subject: Re: [PATCH 6/8] reflog-walk.c: don't print last newline with oneline > > Please find a better phrasing that does not mislead the readers to > think "eek, so now output from 'git log -g --oneline' does not get > LF terminated?" > > I think you are just changing the place where the LF is given from > the callee that shows a single record (and then used to show LF > after the record) to the caller that calls the callee (and now it > shows LF after the call returns), in order to preserve the output, > but with the given proposed log message, it is not clear if that is > what you meant to do (I can read from the code what _is_ going on, > but the log is to record what you _wanted_ to do, which can be > different). Yep, I'll reword the subject line. > > > @@ -661,8 +661,10 @@ void show_log(struct rev_info *opt) > > In the pre-context before this line there is a guard to ensure that > we do this only when showing an entry from the reflog, so the effect > of this hunk is "when showing from reflog in --oneline format, show > LF after showing a single record" ... > > > opt->commit_format == CMIT_FMT_ONELINE, > > &opt->date_mode, > > opt->date_mode_explicit); > > - if (opt->commit_format == CMIT_FMT_ONELINE) > > + if (opt->commit_format == CMIT_FMT_ONELINE) { > > + putc('\n', opt->diffopt.file); > > return; > > + } > > } > > } > > > > diff --git a/reflog-walk.c b/reflog-walk.c > > index 3a25b27d8f..e2b4c0b290 100644 > > --- a/reflog-walk.c > > +++ b/reflog-walk.c > > @@ -285,7 +285,11 @@ void show_reflog_message(struct reflog_walk_info *reflog_info, int oneline, > > info = &commit_reflog->reflogs->items[commit_reflog->recno+1]; > > get_reflog_selector(&selector, reflog_info, dmode, force_date, 0); > > if (oneline) { > > - printf("%s: %s", selector.buf, info->message); > > + struct strbuf message = STRBUF_INIT; > > + strbuf_addstr(&message, info->message); > > + strbuf_trim_trailing_newline(&message); > > + printf("%s: %s", selector.buf, message.buf); > > + strbuf_release(&message); > > ... and this one is what gets called from the above caller; we lost > the final LF (if there is) from the output, that is compensated by > the caller. Yep, thats correct. > > How well do we work with the "-z" output format, by the way, with > the hardcoded '\n' on the output codepath? From my tests, `-z` plays well with my changes. I'll add some tests to be sure, though. > > Making a new allocation feels inefficient just to omit the trailing > newlines. I wonder if a helper function that takes a "const char *" > and returns a "const char *" in that string that points at the CRLF > or LF or NUL at its end would be warranted. If we do not care about > CRLF, this part would become something like... > > if (oneline) { > const char *endp = strchrnul(info->message, '\n'); > printf("%s: %.*s", selector.buf, > (int)(endp - info->message), info->message); > } Yep, I think that this works. In fact, we can probably just do if (oneline) { const char *endp = strchrnul(info->message, '\n'); int len = strlen(info->message); if (len > 0) len--; printf("%s: %.*s", selector.buf, len, info->message); } because... > but I didn't trace the code/data flow to see where info->message > comes from well enough to tell if we need to worry about CRLF. I > thought reflogs are always written with LF termination, though. I think that you are right. Reflogs are always written with LF termination. This can be seen in get_reflog_message(): void get_reflog_message(struct strbuf *sb, struct reflog_walk_info *reflog_info) { struct commit_reflog *commit_reflog = reflog_info->last_commit_reflog; struct reflog_info *info; size_t len; if (!commit_reflog) return; info = &commit_reflog->reflogs->items[commit_reflog->recno+1]; len = strlen(info->message); if (len > 0) => len--; /* strip away trailing newline */ strbuf_add(sb, info->message, len); } > > Thanks.
diff --git a/log-tree.c b/log-tree.c index 923a299e70..4a7d668af6 100644 --- a/log-tree.c +++ b/log-tree.c @@ -661,8 +661,10 @@ void show_log(struct rev_info *opt) opt->commit_format == CMIT_FMT_ONELINE, &opt->date_mode, opt->date_mode_explicit); - if (opt->commit_format == CMIT_FMT_ONELINE) + if (opt->commit_format == CMIT_FMT_ONELINE) { + putc('\n', opt->diffopt.file); return; + } } } diff --git a/reflog-walk.c b/reflog-walk.c index 3a25b27d8f..e2b4c0b290 100644 --- a/reflog-walk.c +++ b/reflog-walk.c @@ -285,7 +285,11 @@ void show_reflog_message(struct reflog_walk_info *reflog_info, int oneline, info = &commit_reflog->reflogs->items[commit_reflog->recno+1]; get_reflog_selector(&selector, reflog_info, dmode, force_date, 0); if (oneline) { - printf("%s: %s", selector.buf, info->message); + struct strbuf message = STRBUF_INIT; + strbuf_addstr(&message, info->message); + strbuf_trim_trailing_newline(&message); + printf("%s: %s", selector.buf, message.buf); + strbuf_release(&message); } else { printf("Reflog: %s (%s)\nReflog message: %s",
In a future commit, we want to possibly be able to continue the reflog message on the same line without breaking the line. As a result, when `oneline == 1`, strip any trailing new lines. Add these missing newlines back in show_log(). Signed-off-by: Denton Liu <liu.denton@gmail.com> --- log-tree.c | 4 +++- reflog-walk.c | 6 +++++- 2 files changed, 8 insertions(+), 2 deletions(-)