Lucian Poston <lucian.poston@xxxxxxxxx> writes:
> Fixed issue with `git log --graph --stat -p` in which the "---" diff output
> header appears before the diff output prefix.
>
> Fixed issue where diff output prefix is absent on empty lines separating diff
> stats and patch.
>
> Added test to verify the graph decoration prefixes of
> `git log --pretty=short --stat -p --graph` are printed correctly.
>
> Signed-off-by: Lucian Poston <lucian.poston@xxxxxxxxx>
> ---
Same comment as the one for your other patch applies to this. Especially,
the Subject says "missing/buggy" but it is unclear if "missing" is the
only bugginess you are addressing, or there are other "bugginess" other
than "missing" from no description in the log message.
> diff --git a/diff.c b/diff.c
> index 377ec1e..29003eb 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -4399,6 +4399,12 @@ void diff_flush(struct diff_options *options)
>
> if (output_format & DIFF_FORMAT_PATCH) {
> if (separator) {
> + if (options->output_prefix) {
> + struct strbuf *msg = NULL;
> + msg = options->output_prefix(options,
> + options->output_prefix_data);
> + fwrite(msg->buf, msg->len, 1, stdout);
> + }
> putc(options->line_termination, options->file);
> if (options->stat_sep) {
> /* attach patch instead of inline */
Immediately before a separator (typically LF) that comes between the log
message and the patch text, we forgot to show the ancestry graph lines.
This corresponds to the second paragraph of your log message.
This is a tangent, but I wonder how --graph should interact with stat_sep
(i.e. patch is shown as an attachment) case. Perhaps the combination need
to be forbidden, but we probably do not care (in other words, the user
would get whatever the code happens to produce).
> diff --git a/log-tree.c b/log-tree.c
> index cea8756..3198503 100644
> --- a/log-tree.c
> +++ b/log-tree.c
> @@ -710,15 +710,16 @@ int log_tree_diff_flush(struct rev_info *opt)
> if ((opt->diffopt.output_format & ~DIFF_FORMAT_NO_OUTPUT) &&
> opt->verbose_header &&
> opt->commit_format != CMIT_FMT_ONELINE) {
> - int pch = DIFF_FORMAT_DIFFSTAT | DIFF_FORMAT_PATCH;
> - if ((pch & opt->diffopt.output_format) == pch)
> - printf("---");
> if (opt->diffopt.output_prefix) {
> struct strbuf *msg = NULL;
> msg = opt->diffopt.output_prefix(&opt->diffopt,
> opt->diffopt.output_prefix_data);
> fwrite(msg->buf, msg->len, 1, stdout);
> }
> + int pch = DIFF_FORMAT_DIFFSTAT | DIFF_FORMAT_PATCH;
Moving this line is unnecessary, and introduces decl-after-statement to
break compilation.
> + if ((pch & opt->diffopt.output_format) == pch) {
> + printf("---");
> + }
We used to show the "---" before ancestry graph lines, which was nonsense.
This corresponds to the first paragraph of your log message.
Looks good from a cursory review. Thanks.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html