Junio C Hamano <junkio@xxxxxxx> wrote:
> > diff --git a/builtin-log.c b/builtin-log.c
> > index 5a8a50b..e4a6385 100644
> > --- a/builtin-log.c
> > +++ b/builtin-log.c
> > @@ -26,8 +26,8 @@ static int cmd_log_wc(int argc, const ch
> > if (rev->always_show_header) {
> > if (rev->diffopt.pickaxe || rev->diffopt.filter) {
> > rev->always_show_header = 0;
> > - if (rev->diffopt.output_format == DIFF_FORMAT_RAW)
> > - rev->diffopt.output_format = DIFF_FORMAT_NO_OUTPUT;
> > + if (rev->diffopt.output_fmt & OUTPUT_FMT_RAW)
> > + rev->diffopt.output_fmt |= OUTPUT_FMT_NONE;
> > }
> > }
>
> The original code is saying "For git-log command (i.e. when
> always-show-header is on), if the command line did not override
> but ended up asking for diff only because it wanted to do -S or
> --diff-filter, do not show any diff" which is quite an opaque
> logic.
I'll just remove this change from the fixed patch 2/5. New version of
the patch 3/5 should then fix this logic.
> > @@ -1371,23 +1371,26 @@ int diff_setup_done(struct diff_options
> > (0 <= options->rename_limit && !options->detect_rename))
> > return -1;
> >
> > + if (options->output_fmt & OUTPUT_FMT_NONE)
> > + options->output_fmt = 0;
> > +
> > + if (options->output_fmt & (OUTPUT_FMT_NAME |
> > + OUTPUT_FMT_CHECKDIFF |
> > + OUTPUT_FMT_NONE))
> > + options->output_fmt &= ~(OUTPUT_FMT_RAW |
> > + OUTPUT_FMT_DIFFSTAT |
> > + OUTPUT_FMT_SUMMARY |
> > + OUTPUT_FMT_PATCH);
> > +
>
> Maybe doing the same for --name-status?
Will fix. Originally I made --name-status imply --name-only but changed
it and forgot to fix this.
> I wonder if the --name,
> --name-status and --check should be mutually exclusive. What
> happens when you specify more than one of them?
I'll just make it die() then. If it breaks something then the code is
really dumb anyway.
> > diff --git a/revision.c b/revision.c
> > index b963f2a..4ad2272 100644
> > --- a/revision.c
> > +++ b/revision.c
> > @@ -852,8 +852,8 @@ int setup_revisions(int argc, const char
> > if (revs->combine_merges) {
> > revs->ignore_merges = 0;
> > if (revs->dense_combined_merges &&
> > - (revs->diffopt.output_format != DIFF_FORMAT_DIFFSTAT))
> > - revs->diffopt.output_format = DIFF_FORMAT_PATCH;
> > + !(revs->diffopt.output_fmt & OUTPUT_FMT_DIFFSTAT))
> > + revs->diffopt.output_fmt |= OUTPUT_FMT_PATCH;
> > }
> > revs->diffopt.abbrev = revs->abbrev;
> > diff_setup_done(&revs->diffopt);
>
> This tells it to default to patch format unless we are asked to
> do diffstat only, in which case we just show stat without patch.
> The new logic seems to be fishy.
If we first initialize it to 0 instead of DIFF_FORMAT_RAW and after
command line flags have been parsed, if it still is 0, then default to
DIFF_FORMAT_PATCH.
> - could I ask you to redo a patch to do only the clean-up part
> first, so that I can accept it for either "next" or "master".
>
> - Then after I take the clean-up, could you rebase four
> remainder patches ("Rework diff options" to "Add --patch
> option for diff-*") on the result? The patches this round
> are already split quite well in that the first one does the
> enum to bit conversion and the latter three cleans things up
> (all of which I like a lot). As Johannes suggested, it might
> be easier to review if they reused the same preprocessor
> symbols instead of renaming them. I'd take them for "next".
Yes, all this makes sense.
--
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