Thread (21 messages) 21 messages, 2 authors, 2023-03-22

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

From: Jonathan Tan <hidden>
Date: 2023-03-17 20:24:59

Patrick Steinhardt [off-list ref] 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".
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help