Hi Junio,
On Wed, Nov 06, 2019 at 02:12:17PM +0900, Junio C Hamano wrote:
> Denton Liu <liu.denton@xxxxxxxxx> 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.
