Re: [RFC PATCH 0/4] git-gui: support SHA-256 repositories

From: Carlo Arenas <carenas@gmail.com>
Date: Oct 11, 2021, 19:47 UTC
Message-ID: <CAPUEspjFZDKtP8oJmuA6dCcX9XF1WBFvFikkZTNcKfHbOxJwPA@mail.gmail.com>
In-Reply-To: <87bl3vlk0j.fsf@evledraar.gmail.com>

On Mon, Oct 11, 2021 at 7:21 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

Show 22 quoted lines

>
> On Mon, Oct 11 2021, Carlo Marcelo Arenas Belón wrote:
>
> > [1] https://lore.kernel.org/git/20211011114723.204-1-carenas@gmail.com/
> >
> > Carlo Marcelo Arenas Belón (4):
> >   blame: prefer null_sha1 over nullid and retire later
> >   rename all *_sha1 variables and make null_oid hash aware
> >   expand regexp matching an oid to be hash agnostic
> >   track oid_size to allow for checks that are hash agnostic
> >
> >  git-gui.sh                   | 30 ++++++++++++++++--------------
> >  lib/blame.tcl                | 18 +++++++++---------
> >  lib/checkout_op.tcl          |  4 ++--
> >  lib/choose_repository.tcl    |  2 +-
> >  lib/commit.tcl               |  3 ++-
> >  lib/remote_branch_delete.tcl |  2 +-
> >  6 files changed, 31 insertions(+), 28 deletions(-)
>
> There was a similar series earlier this year which didn't make it that
> fixes some of the same issues:
> https://lore.kernel.org/git/pull.979.git.1623687519832.gitgitgadget@gmail.com/

This specific series is for git-gui, and the one posted before is for gitk, but the code is still similar enough, and indeed the gitk part was included in a reference.

Show 8 quoted lines

> Just seems like a lot of needless work as opposed to just matching
> x{40,64} or whatever.  Yes that's not the same regex semantically, but I
> think the current code is just being overly strict, i.e. it's parsing
> some plumbing output, we can trust that the thing that looks like the
> OID in that position is the OID.
>
> If anything I'd think we could just match [0-9a-f]{4,} in most/all of
> these cases, would make things like this easier to read:

It makes me nervous though to see checks like the one I fixed on commit[1] that use logic to check the correct size of the SHA as an implication of it being a valid value.

considering the code is very old, maybe that was relevant long ago?, but agree some checks seem to be unnecessarily strict.

I have relaxed some of the checks in the gitk patch and will be posting it soon, so hopefully reviews from people that know the code better could be collected.

Carlo

[1] https://lore.kernel.org/git/20211011121757.627-5-carenas@gmail.com/