On Wed, Mar 23, 2016 at 04:23:41PM -0700, Junio C Hamano wrote:
> So here is the third try (previous round is found at $gmane/289166
> and the very first one is at $gmane/288987).
Is the plan to merge these as-is? The ordering is a bit funny (introduce
breakage, then repair it), and I think the first patch still breaks
t4201.8 (which is then repaired in the fourth one).
I think it would be a lot easier to review as:
1. Factor out pp_handle_indent(), and any other preparation.
2. Add --expand-tabs / --no-expand-tabs, with the logic going into
pp_handle_indent().
3. Flip the default for some formats to expand-tabs.
Other than that, the end result seems OK to me (I think adding
--expand-tabs would be nice, but I suspect it may need to be marked as
incompatible with some formats; do all formats end up in this same
writing code path?).
> By the way, I have to say that I hate how pretty formatting and
> revision machinery interact with each other.
>
> pretty.c::commit_formats ought to be the authoritative source of how
> each named format should work, but there are quite a many codepaths
> that just assign CMIT_FMT_SOMETHING to revs->commit_format without
> bothering with other fields in the cmt_fmt_map like is_tformat, and
> I am not sure if they are working correctly even before this patch.
I don't disagree with any of that. I suspect some of the logic may be
complicated for sticking in a table, though. Perhaps we need a:
void set_pp_format(struct pretty_print_context *ctx, enum cmit_fmt fmt);
that sets up the whole struct based on the given format, and then the
logic can live in C code. I haven't looked closely at that code in a
while, though, so maybe that is overkill.
-Peff
--
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