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

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 6, 2019, 05:12 UTC
Message-ID: <xmqqwocdtwpa.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <449c58009ae46d43ec2bd0499674c9858b706d04.1572897736.git.liu.denton@gmail.com>

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" ...

Show 25 quoted lines

>  					    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.