Re: [PATCH 3/3] pretty: add abbrev option to %(describe)

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 24, 2021, 05:15 UTC
Message-ID: <xmqqv91n57g4.fsf@gitster.g>
In-Reply-To: <20211024014256.3569322-4-eschwartz@archlinux.org>

Eli Schwartz <eschwartz@archlinux.org> writes:

> The %(describe) placeholder by default, like `git describe`, uses a
> seven-character abbreviated commit hash. This may not be sufficient to

"hash" -> "object name".

> fully describe all git repos, resulting in a placeholder replacement

"all git repos" -> "all commits in a given repository" (there may be other valid way to clarify, but the point is that 'describe' does not describe 'git repos' in the sense that my repository gets description X while your repository gets description Y).

Show 6 quoted lines

> changing its length because the repository grew in size. This could
> cause the output of git-archive to change.
>
> Add the --abbrev option to `git describe` to the placeholder interface
> in order to provide tools to the user for fine-tuning project defaults
> and ensure reproducible archives.

Note that it is sad that --abbrev=<n> does not necessarily ensure reproducibility. To be more precise, I do not think it sacrifices uniqueness to make the output reproducible. You can get more than N hex-digits in the output if N is too small to ensure uniquness.

So it indeed is that this line of thought ...

> One alternative would be to just always specify --abbrev=40 but this may
> be a bit too biased...

... to use --abbrev=999 (because 40 is not the length of a full object name in the SHA-2 world) is the only reasonable way, if what you care about is the reproducibility.

    Side note.  I think "git describe --no-abbrev" is buggy in that
    it does not give a full object name; I didn't check the code,
    but it appears to be behaving the same way as "git describe
    --abbrev=0" (show no hexdigits).  Fixing this bug may possibly
    be a low-hanging fruit.

But even if the feature cannot be used to guarantee a full reproducibility, it is a good thing that we can now add this feature with minimum effort thanks to the previous two steps.

The refactoring I suggested in my review for the previous step will shine, if we want to do a good job parsing the --abbrev=<n> option, since such a code organization would make it a fairly easy addition to introduce "integer" type that calls match_placeholder_arg_value() to read the option value (like "string" does) and validate that the value is indeed an integer.

Would we want to support "--contains" as another boolean type? How about "--all" and "--long"? All three sound plausible candidates.

Thanks.