Re: [PATCH 0/8] fetch: refactor code that prints reference updates

[Jonathan Tan <jonathantanmy@google.com>]

Patrick Steinhardt <ps@pks.im> writes:
>     1. We want to take control of the reference updates so that we can
>        atomically update all or a subset of references that git-fetch
>        would have updated.
> 
>     2. We want to be able to quarantine objects in a fetch so that we
>        can e.g. perform consistency checks for them before they land in
>        the main repository.

If you want to do this, something that might be possible is to change
the RHS of the refspecs to put the refs in a namespace of your choice
(e.g. ...:refs/<UUID>/...) and then you can look at what's generated and
process them as you wish.

>     - There should be as few global state as possible. This is to reduce
>       confusion and having to repeat the same incantations in multiple
>       different locations.

Makes sense.

>     - The logic should be as self-contained as possible. This is so that
>       it can easily be changed in a subsequent patch series.

Also makes sense, but I think that some of your patches might be
contrary to this goal (more details below).

I've read all the patches, but will just summarize my thoughts here.

> Patrick Steinhardt (8):
>   fetch: rename `display` buffer to avoid name conflict

One other way, as others have discussed, is to just name the new
variable display_state. (I would prefer that, at the very least so
that in case someone else has a patch that contains the identifier
"display", problems would be more easily noticed. This is very unlikely
to happen but I think it's a good general direction for the Git project
to follow.)

>   fetch: move reference width calculation into `display_state`
>   fetch: move output format into `display_state`
>   fetch: pass the full local reference name to `format_display`

All these are good changes that I would be happy to see merged.

>   fetch: deduplicate handling of per-reference format

I'm not so sure that this is the correct abstraction. I think that this
and the last patch might be counterproductive to your stated goal of
having one more mode of printing the refs, in fact, since when we have
that new mode, the format would be different but the printing would
remain (so we should split the format and printing).

>   fetch: deduplicate logic to print remote URL

Makes sense, although I would need to consider only storing the
raw URL in the struct display_state and processing it when it needs
to be emitted (haven't checked if this is feasible, though).

>   fetch: fix inconsistent summary width for pruned and updated refs

This changes the behavior in that the summary width, even when printing
the summary of pruned refs, is computed based only on the updated refs.
The summary width might need to remain out of the struct display_state
for now.

>   fetch: centralize printing of reference updates

Same as "fetch: deduplicate handling of per-reference format".