git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Patrick Steinhardt <ps@pks.im>
To: Junio C Hamano <gitster@pobox.com>
Cc: git@vger.kernel.org
Subject: Re: [PATCH 7/8] fetch: fix inconsistent summary width for pruned and updated refs
Date: Thu, 16 Mar 2023 16:06:44 +0100	[thread overview]
Message-ID: <ZBMwhLgGeUhtd5Zb@ncase> (raw)
In-Reply-To: <xmqqttylmww4.fsf@gitster.g>

[-- Attachment #1: Type: text/plain, Size: 2750 bytes --]

On Wed, Mar 15, 2023 at 04:12:59PM -0700, Junio C Hamano wrote:
> 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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

  reply	other threads:[~2023-03-16 15:07 UTC|newest]

Thread overview: 39+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-03-15 11:21 [PATCH 0/8] fetch: refactor code that prints reference updates Patrick Steinhardt
2023-03-15 11:21 ` [PATCH 1/8] fetch: rename `display` buffer to avoid name conflict Patrick Steinhardt
2023-03-15 11:21 ` [PATCH 2/8] fetch: move reference width calculation into `display_state` Patrick Steinhardt
2023-03-15 20:59   ` Junio C Hamano
2023-03-16 15:05     ` Patrick Steinhardt
2023-03-16 16:18       ` Junio C Hamano
2023-03-17 10:03         ` Patrick Steinhardt
2023-03-16 16:19       ` Junio C Hamano
2023-03-15 11:21 ` [PATCH 3/8] fetch: move output format " Patrick Steinhardt
2023-03-15 11:21 ` [PATCH 4/8] fetch: pass the full local reference name to `format_display` Patrick Steinhardt
2023-03-15 22:18   ` Junio C Hamano
2023-03-15 11:21 ` [PATCH 5/8] fetch: deduplicate handling of per-reference format Patrick Steinhardt
2023-03-15 22:45   ` Junio C Hamano
2023-03-16 15:06     ` Patrick Steinhardt
2023-03-16 16:50       ` Junio C Hamano
2023-03-17  9:51         ` Patrick Steinhardt
2023-03-17 15:41           ` Junio C Hamano
2023-03-15 11:21 ` [PATCH 6/8] fetch: deduplicate logic to print remote URL Patrick Steinhardt
2023-03-15 23:02   ` Junio C Hamano
2023-03-16 15:06     ` Patrick Steinhardt
2023-03-15 11:21 ` [PATCH 7/8] fetch: fix inconsistent summary width for pruned and updated refs Patrick Steinhardt
2023-03-15 23:12   ` Junio C Hamano
2023-03-16 15:06     ` Patrick Steinhardt [this message]
2023-03-16 16:30       ` Junio C Hamano
2023-03-17  9:55         ` Patrick Steinhardt
2023-03-15 11:21 ` [PATCH 8/8] fetch: centralize printing of reference updates Patrick Steinhardt
2023-03-17 20:24 ` [PATCH 0/8] fetch: refactor code that prints " Jonathan Tan
2023-03-20  6:57   ` Patrick Steinhardt
2023-03-20 12:26   ` Patrick Steinhardt
2023-03-20 12:35 ` [PATCH v2 0/6] " Patrick Steinhardt
2023-03-20 12:35   ` [PATCH v2 1/6] fetch: move reference width calculation into `display_state` Patrick Steinhardt
2023-03-20 12:35   ` [PATCH v2 2/6] fetch: move output format " Patrick Steinhardt
2023-03-20 12:35   ` [PATCH v2 3/6] fetch: pass the full local reference name to `format_display` Patrick Steinhardt
2023-03-20 12:35   ` [PATCH v2 4/6] fetch: centralize handling of per-reference format Patrick Steinhardt
2023-03-20 12:35   ` [PATCH v2 5/6] fetch: centralize logic to print remote URL Patrick Steinhardt
2023-03-20 12:35   ` [PATCH v2 6/6] fetch: centralize printing of reference updates Patrick Steinhardt
2023-03-20 22:57     ` Jonathan Tan
2023-03-22  9:04       ` Patrick Steinhardt
2023-03-29 18:45       ` Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=ZBMwhLgGeUhtd5Zb@ncase \
    --to=ps@pks.im \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).