[6/8] reflog-walk.c: don't print last newline with oneline
diff mbox series

Message ID 449c58009ae46d43ec2bd0499674c9858b706d04.1572897736.git.liu.denton@gmail.com
State New
Headers show
Series
  • learn the "summary" pretty format
Related show

Commit Message

Denton Liu Nov. 4, 2019, 8:04 p.m. UTC
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(-)

Comments

Junio C Hamano Nov. 6, 2019, 5:12 a.m. UTC | #1
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.
Denton Liu Nov. 8, 2019, 8:50 a.m. UTC | #2
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.

Patch
diff mbox series

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