Re: [PATCH 6/8] reflog-walk.c: don't print last newline with oneline

From: Denton Liu <liu.denton@gmail.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: Git Mailing List <git@vger.kernel.org>
Subject: Re: [PATCH 6/8] reflog-walk.c: don't print last newline with oneline
Date: Fri, 8 Nov 2019 00:50:24 -0800	[thread overview]
Message-ID: <20191108085024.GA26530@generichostname> (raw)
In-Reply-To: <xmqqwocdtwpa.fsf@gitster-ct.c.googlers.com>

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.