Re: [PATCH v3 0/5] Expanding tabs in "git log" output

From: Jeff King <peff@peff.net>
Date: Mar 24, 2016, 00:58 UTC
Message-ID: <20160324005849.GA8167@sigill.intra.peff.net>
In-Reply-To: <1458775426-2215-1-git-send-email-gitster@pobox.com>

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?).

Show 8 quoted lines

> 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