[PATCH] log-tree.c: Make log_tree_diff_flush() honor line_termination.

Govind Salinas· Apr 22, 2008, 02:18 UTC · lore

Signed-off-by: Govind Salinas <blix@sophiasuchtig.com>
---
I sent this in a few weeks ago, but it was not eligible for inclusion on 1.5.5.
There was some discussion but I was never sure if the patch was acceptable
to everyone.  I would like to know if this could be done for the next version?

 log-tree.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

Show changes to log-tree.c +1 −1

diff --git a/log-tree.c b/log-tree.c
index 5b29639..374b277 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -347,7 +347,7 @@ int log_tree_diff_flush(struct rev_info *opt)
 			int pch = DIFF_FORMAT_DIFFSTAT | DIFF_FORMAT_PATCH;
 			if ((pch & opt->diffopt.output_format) == pch)
 				printf("---");
-			putchar('\n');
+			putchar(opt->diffopt.line_termination);
 		}
 	}
 	diff_flush(&opt->diffopt);

-- 
1.5.5.rc2.131.g3d2f

Junio C Hamano· Apr 22, 2008, 02:58 UTC · re: Govind Salinas · lore

Re: [PATCH] log-tree.c: Make log_tree_diff_flush() honor line_termination.

"Govind Salinas" <govind@sophiasuchtig.com> writes:

[no justification here]

> Signed-off-by: Govind Salinas <blix@sophiasuchtig.com>

Show 13 quoted lines

> diff --git a/log-tree.c b/log-tree.c
> index 5b29639..374b277 100644
> --- a/log-tree.c
> +++ b/log-tree.c
> @@ -347,7 +347,7 @@ int log_tree_diff_flush(struct rev_info *opt)
>  			int pch = DIFF_FORMAT_DIFFSTAT | DIFF_FORMAT_PATCH;
>  			if ((pch & opt->diffopt.output_format) == pch)
>  				printf("---");
> -			putchar('\n');
> +			putchar(opt->diffopt.line_termination);
>  		}
>  	}
>  	diff_flush(&opt->diffopt);

What this "\n" separates are the commit log message (potentially followed by the three-dash marker to signal "the patch follows") and the textual diff (potentially with diffstat, which also is textual).

Lines in the textual diff part is always separated with "\n". The patch is line oriented by definition, and it does not make it any easier for the tools to grok even if you made it NUL terminated. The log message when given by log-tree is typically indented by four spaces, so the beginning of diff/patch part which is not indented can be detected easily without the help fro NUL termination. In other words, I do not think the tool downstream you are writing is helped much with this change. While I can understand why you wanted to do it (i.e. "being consistent"), I do not think the consistency buys us much in this particular case.

However, the tool downstream other people have already written to read from the log-tree output already knows that there will be LF at this place even if they drive log-tree with a "-z" option, as that has been the way from the beginning. I have a suspicion that tools like qgit may start barfing with this change if they read from "-z" output.

Which makes the purist in me feel somewhat sad, but the pragmatist in me is not convinced until he is shown how this will help the downstream tools that read from the output from log-tree, which you didn't do with zero line of a proposed commit log message.

A possible defense for this patch is that it _could_ make the output easier to parse in the presense of a commit log message with a line that begins with "diff --git" when log-tree is driven with --pretty=raw.

patchlog-tree.c: Make log_tree_diff_flush() honor line_termination.