Re: [PATCH v2 01/18] cat-file: split expand_atom into 2 functions

From: Jeff King <peff@peff.net>
Date: Jan 15, 2018, 21:33 UTC
Message-ID: <20180115213335.GB4778@sigill.intra.peff.net>
In-Reply-To: <01020160df6dc499-0e6d11ec-1dcd-4a71-997b-ea231f33fae4-000000@eu-west-1.amazonses.com>

On Wed, Jan 10, 2018 at 09:36:41AM +0000, Olga Telezhnaya wrote:

> Split expand_atom function into 2 different functions,
> expand_atom_into_fields prepares variable for further filling,
> (new) expand_atom creates resulting string.
> Need that for further reusing of formatting logic from ref-filter.

This commit puzzled me, and I had to look ahead to try to figure out why we want this split (because on its face, the split is bad, since it duplicates the list).

Later, the preparation step goes away, but we still are left with expand_atom(). That's because the preparation was all moved into ref-filter.c, where we rely on populate_value() to fill in the values, and then we pick them out with our own formats.

That works, but I don't think it's where we want to end up in the long run. Because:

  1. We still have the set of formats duplicated between expand_atom()
     and the "preparation" step. It's just that the preparation is now
     in ref-filter.c. What happens if ref-filter.c learns new formatting
     placeholders (or options for those placeholders) that cat-file.c
     doesn't, or vice versa? The two have to be kept in sync.

  2. We're missing out on all of the other placeholders that ref-filter
     knows about. Not all of them are meaningful (e.g., %(refname)
     wouldn't make sense here), but part of our goal is to support the
     same set of placeholders as much as possible. Some obvious ones
     that ought to work for cat-file: %(objectname:short), %(if), and
     things like %(subject) when the appropriate object type is used.

In other words, I think the endgame is that expand_atom() isn't there at all, and we're calling the equivalent of format_ref_item() for each object (except that in a unified formatting world, it probably doesn't have the word "ref" in it, since that's just one of the items a caller might pass in).

-Peff