Re: [PATCH 7/8] fetch: fix inconsistent summary width for pruned and updated refs

From: Patrick Steinhardt <ps@pks.im>
Date: Mar 16, 2023, 15:06 UTC
Message-ID: <ZBMwhLgGeUhtd5Zb@ncase>
In-Reply-To: <xmqqttylmww4.fsf@gitster.g>

On Wed, Mar 15, 2023 at 04:12:59PM -0700, Junio C Hamano wrote:

Show 14 quoted lines

> Patrick Steinhardt <ps@pks.im> writes:
> 
> > As the abbreviated hashes may have different lengths in order to be
> > unique we thus need to precompute the width of the summary's column by
> > iterating through all the objects. This is done in two locations: once
> > to compute the width for references that are to be pruned, and once for
> > all the other references. Consequentially, it can happen that the width
> > as calculated for these sets of references is different.
> 
> Hmph.  Use of ref_map vs stale_refs as the parameter to call
> transport_summary_width() is to come up with an appropriate width
> for showing the list of stored refs vs the list of pruned refs, so
> from that point of view, an appropriate width for each list is
> calculated to a different number may even be a feature, no?

I'd say it's not. Look at the following output generated by a `git fetch --prune --no-progress` with a deleted and an updated reference:

    From /tmp/repo
     - [deleted]         (none)     -> origin/to-be-deleted
       82307bb..107b50a  main       -> origin/main

Before my change, the width of the deletion and the reference update are calculated separately. Given that:

    - we don't even display the object IDs for deleted references

    - the width of the deleted reference's column is static anyway.

I'd argue that it's not a feature that the widths are computed separately. If it was, you could just skip calculating the width of deleted references and just print them with a static column width.

The current implementation tends to work in most cases as the column width is based on the minimum length where all abbreviated object IDs become unique. And I assume that it's the same for both sets of refs in the majority of cases. And in the other cases I guess that nobody cares much anyway.

Practically speaking we could go even further than the current version, as I now compute the width across _all_ reference updates, even those which are deletions. But theoretically speaking, we could just skip over any deletions completely as they won't ever contribute to the column width anyway.

> I do not mind either way all that much, but a change like this to
> update the presentation may want to be protected with a test from
> future breakages.

Fair, having a test for this would be great. But what kept me from adding one here is that the column width depends on the length of the longest shared prefix of two object IDs that are about to be updated. And I just have no clue how to generate those without brute forcing them for both SHA1 and SHA256.

Do we have any mechanism for this?

Patrick