Johannes Schindelin <Johannes.Schindelin@xxxxxx> wrote:
> Hi,
>
> thank you very much for doing the extra step and using the original
> constant names. I appreciate that.
>
> On Sat, 24 Jun 2006, Timo Hirvonen wrote:
>
> > @@ -818,17 +817,12 @@ void show_combined_diff(struct combine_d
> > struct diff_options *opt = &rev->diffopt;
> > if (!p->len)
> > return;
> > - switch (opt->output_format) {
> > - case DIFF_FORMAT_RAW:
> > - case DIFF_FORMAT_NAME_STATUS:
> > - case DIFF_FORMAT_NAME:
> > + if (opt->output_format & (DIFF_FORMAT_RAW |
> > + DIFF_FORMAT_NAME |
> > + DIFF_FORMAT_NAME_STATUS)) {
> > show_raw_diff(p, num_parent, rev);
> > - return;
> > - case DIFF_FORMAT_PATCH:
> > + } else if (opt->output_format & DIFF_FORMAT_PATCH) {
>
> Not that it matters, but this "else" could go. (Otherwise, "--raw -p"
> would be the same as "--raw", right?)
Just tested, ./git log -p --raw displays both raw and patch. I think it
works because I changed diff_tree_combined() to use show_raw_diff() and
show_patch_diff() directly.
It feels 'wrong' to check flags and then call a function which checks
the flags again. This combined diff stuff is confusing.
> > @@ -856,19 +846,18 @@ void diff_tree_combined(const unsigned c
> > [...]
> >
> > - if (do_diffstat && rev->loginfo)
> > - show_log(rev, rev->loginfo,
> > - opt->with_stat ? "---\n" : "\n");
> > + if (opt->output_format & DIFF_FORMAT_DIFFSTAT && rev->loginfo)
> > + show_log(rev, rev->loginfo, "---\n");
> > diff_flush(&diffopts);
> > - if (opt->with_stat)
> > + if (opt->output_format & DIFF_FORMAT_DIFFSTAT)
> > putchar('\n');
> > }
>
> Just a remark: this hunk actually changes behaviour. "with_stat" meant
> that the stat was prepended before something like a patch, and therefore a
> separator was needed. If you pass only "--stat", the separator will be
> printed anyway now.
You are right, it now prints --- when it should print empty line.
> > + int inter_name_termination = '\t';
> > + int line_termination = options->line_termination;
> > +
> > + if (!line_termination)
> > + inter_name_termination = 0;
>
> <nit type=minor>
> This should be part of patch 1/7.
> </nit>
That clean up was possible only after I made other changes to the code,
I think. At least it wasn't obvious when I wrote 1/7.
> > show_stats(diffstat);
> > free(diffstat);
>
> Why not go the full nine yards, and make diffstat not a pointer, but the
> struct itself? You would avoid calloc()ing and free()ing. (Of course,
> instead of calloc()ing you have to memset() it to 0.)
I was blind :)
> > + if (output_format & DIFF_FORMAT_PATCH) {
> > + if (output_format & (DIFF_FORMAT_DIFFSTAT |
> > + DIFF_FORMAT_SUMMARY)) {
> > + if (options->stat_sep)
> > + fputs(options->stat_sep, stdout);
> > + else
> > + putchar(options->line_termination);
>
> Are we sure we do not want something like
>
> if (output_format / DIFF_FORMAT_DIFFSTAT > 1)
> /* output separator */
>
> after each format (this example being after the diffstat), the condition
> being: if there is still an output format to come, add the separator?
I'm not sure what you mean.
It outputs separator between (diffstat and/or summary) and patch.
There's no separator between diffstat and summary or raw and diffstat.
Should there be one?
Thanks for your comments. Should I patch the patch or send a fixed one?
I'm currently too tired to write any code.
--
http://onion.dynserv.net/~timo/
-
: 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