threads / rfc / 44617

RFC patchGIT-VERSION-GEN: set --abbrev=9 to match auto-scaling

Subject: [RFC PATCH] GIT-VERSION-GEN: set --abbrev=9 to match auto-scaling

## tl;dr

12 messages between Dec 4, 2016 and Dec 6, 2016. Diffs are folded; open one to read it.

replies: 11people: 3as markdown or json

Ramsay Jones· Dec 4, 2016, 20:45 UTC · lore
Signed-off-by: Ramsay Jones <ramsay@ramsayjones.plus.com>
---
Hi Junio,
I recently noticed that:
    $ make >pout 2>&1
    $ ./git version
    git version 2.11.0.286.g109e8a9
    $ git describe
    v2.11.0-286-g109e8a99d
    $

... for non-release builds, the commit part of the version string was still using an --abbrev=7.

I don't know that it actually matters too much (since it will show as many as necessary, thus the RFC), but it caused me to look twice. ;-)

ATB, Ramsay Jones

 GIT-VERSION-GEN | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to GIT-VERSION-GEN +1 −1
diff --git a/GIT-VERSION-GEN b/GIT-VERSION-GEN
index 520d6e66e..05601f753 100755
--- a/GIT-VERSION-GEN
+++ b/GIT-VERSION-GEN
@@ -12,7 +12,7 @@ if test -f version
 then
 	VN=$(cat version) || VN="$DEF_VER"
 elif test -d ${GIT_DIR:-.git} -o -f .git &&
-	VN=$(git describe --match "v[0-9]*" --abbrev=7 HEAD 2>/dev/null) &&
+	VN=$(git describe --match "v[0-9]*" --abbrev=9 HEAD 2>/dev/null) &&
 	case "$VN" in
 	*$LF*) (exit 1) ;;
 	v[0-9]*)
-- 
2.11.0
Jeff King· Dec 5, 2016, 05:32 UTC · re: Ramsay Jones · lore

Re: [RFC PATCH] GIT-VERSION-GEN: set --abbrev=9 to match auto-scaling

On Sun, Dec 04, 2016 at 08:45:59PM +0000, Ramsay Jones wrote:
Show 17 quoted lines
> 
> Signed-off-by: Ramsay Jones <ramsay@ramsayjones.plus.com>
> ---
> 
> Hi Junio,
> 
> I recently noticed that:
> 
>     $ make >pout 2>&1
>     $ ./git version
>     git version 2.11.0.286.g109e8a9
>     $ git describe
>     v2.11.0-286-g109e8a99d
>     $
> 
> ... for non-release builds, the commit part of the version
> string was still using an --abbrev=7.

It seems like this kind of discussion ought to go in the commit message. :)

That said, I think the right patch may be to just drop --abbrev entirely. Its use goes all the way back to 9b88fcef7 (Makefile: use git-describe to mark the git version., 2005-12-27), where it was --abbrev=4. That became --abbrev=7 in bf505158d (Git 1.7.10.1, 2012-05-01) without further comment.

I think at that point it was a noop, as 7 should have been the default. And now we probably ought to drop it, so that we can use the auto-scaling default.

-Peff
Ramsay Jones· Dec 5, 2016, 11:21 UTC · re: Jeff King · lore

Re: [RFC PATCH] GIT-VERSION-GEN: set --abbrev=9 to match auto-scaling

On 05/12/16 05:32, Jeff King wrote:
Show 18 quoted lines
> On Sun, Dec 04, 2016 at 08:45:59PM +0000, Ramsay Jones wrote:
>> I recently noticed that:
>>
>>     $ make >pout 2>&1
>>     $ ./git version
>>     git version 2.11.0.286.g109e8a9
>>     $ git describe
>>     v2.11.0-286-g109e8a99d
>>     $
>>
>> ... for non-release builds, the commit part of the version
>> string was still using an --abbrev=7.
> 
> It seems like this kind of discussion ought to go in the commit message.
> :)
> 
> That said, I think the right patch may be to just drop --abbrev
> entirely.

Heh, that was the first version of the patch. However, I got to thinking about why --abbrev=7 was there in the first place; the only reason I could think of was to defeat local configuration to get a measure of reproducibility.

Unfortunately, you can't get the 'auto' behaviour from --abbrev (on the pu branch):

    $ ./git describe --abbrev=-1
    v2.11.0-286-g109e8
    $ ./git describe --abbrev=0
    v2.11.0
    $ ./git describe
    v2.11.0-286-g109e8a99d
    $

I did think about using '-c core.abbrev=auto', but that would depend on Junio's patch (nothing wrong with that, of course):

    $ git version
    git version 2.11.0
    $ git -c core.abbrev=auto describe
    fatal: bad numeric config value 'auto' for 'core.abbrev': invalid unit
    $ ./git -c core.abbrev=auto describe
    v2.11.0-286-g109e8a99d
    $ 
What do you think?

ATB, Ramsay Jones

Junio C Hamano· Dec 5, 2016, 18:10 UTC · re: Ramsay Jones · lore

Re: [RFC PATCH] GIT-VERSION-GEN: set --abbrev=9 to match auto-scaling

Ramsay Jones <ramsay@ramsayjones.plus.com> writes:
Show 15 quoted lines
> Heh, that was the first version of the patch. However, I got to thinking
> about why --abbrev=7 was there in the first place; the only reason I
> could think of was to defeat local configuration to get a measure of
> reproducibility.
>
> Unfortunately, you can't get the 'auto' behaviour from --abbrev
> (on the pu branch):
>
>     $ ./git describe --abbrev=-1
>     v2.11.0-286-g109e8
>     $ ./git describe --abbrev=0
>     v2.11.0
>     $ ./git describe
>     v2.11.0-286-g109e8a99d
>     $

What is the reason why the last one is undesirable? Is it because the user may have core.abbrev set to some value in the configuration and you want to override it to force "auto"?

I am not sure how rigid GIT-VERSION-GEN wants to be to countermand such an explicit user preference (i.e. existing configuration).

> I did think about using '-c core.abbrev=auto', 

Having said that, if countermanding end-user's configuration is desireble, I agree that "-c core.abbrev=auto" is the way to do so.

> but that would depend on Junio's patch (nothing wrong with that,
> of course):

You caught me. I'll need to polish that into a usable shape soon then. And that is orthogonal to the "does it make sense to force 'auto' in this context?" question.

Ramsay Jones· Dec 5, 2016, 20:30 UTC · re: Junio C Hamano · lore

Re: [RFC PATCH] GIT-VERSION-GEN: set --abbrev=9 to match auto-scaling

On 05/12/16 18:10, Junio C Hamano wrote:
Show 21 quoted lines
> Ramsay Jones <ramsay@ramsayjones.plus.com> writes:
> 
>> Heh, that was the first version of the patch. However, I got to thinking
>> about why --abbrev=7 was there in the first place; the only reason I
>> could think of was to defeat local configuration to get a measure of
>> reproducibility.
>>
>> Unfortunately, you can't get the 'auto' behaviour from --abbrev
>> (on the pu branch):
>>
>>     $ ./git describe --abbrev=-1
>>     v2.11.0-286-g109e8
>>     $ ./git describe --abbrev=0
>>     v2.11.0
>>     $ ./git describe
>>     v2.11.0-286-g109e8a99d
>>     $
> 
> What is the reason why the last one is undesirable?  Is it because
> the user may have core.abbrev set to some value in the configuration
> and you want to override it to force "auto"?

As I said, the original version of the patch just removed the --abbrev=7, but then I started to think about why you might have used --abbrev in the first place (first in commit 9b88fcef7 and again in commit bf505158d). Making sure to override the configuration was the only thing I could come up with. So, I was hoping you could remember why! :-P

(I assumed it was to force a measure of uniformity/reproducibility).

ATB, Ramsay Jones

Junio C Hamano· Dec 5, 2016, 22:24 UTC · re: Ramsay Jones · lore

Re: [RFC PATCH] GIT-VERSION-GEN: set --abbrev=9 to match auto-scaling

Ramsay Jones <ramsay@ramsayjones.plus.com> writes:
Show 6 quoted lines
> As I said, the original version of the patch just removed the
> --abbrev=7, but then I started to think about why you might have
> used --abbrev in the first place (first in commit 9b88fcef7 and
> again in commit bf505158d). Making sure to override the configuration
> was the only thing I could come up with. So, I was hoping you could
> remember why! :-P

Nope. As a maintainer support script, the only thing I cared about it is that there is no -gXXXX at the end for anything I release ;-)

> (I assumed it was to force a measure of uniformity/reproducibility).

You cannot force uniformity/reproducibility with fixed abbrev, unless you set abbreviation length to 40, so you are correct to add "a measure of" there ;-) The first choice (i.e. 4) may have had a justification to force absolute minimum, and the second one (i.e. 7) may have had a justifiation to make it clear that we are using the same setting as the default, so in post-1.7.10 era, I think it is fine for us to just say "we have been using the same as default, so let's not specify anything explicitly".

Ramsay Jones· Dec 5, 2016, 22:42 UTC · re: Junio C Hamano · lore

Re: [RFC PATCH] GIT-VERSION-GEN: set --abbrev=9 to match auto-scaling

On 05/12/16 22:24, Junio C Hamano wrote:
Show 17 quoted lines
> Ramsay Jones <ramsay@ramsayjones.plus.com> writes:
> 
>> As I said, the original version of the patch just removed the
>> --abbrev=7, but then I started to think about why you might have
>> used --abbrev in the first place (first in commit 9b88fcef7 and
>> again in commit bf505158d). Making sure to override the configuration
>> was the only thing I could come up with. So, I was hoping you could
>> remember why! :-P
> 
> Nope.  As a maintainer support script, the only thing I cared about
> it is that there is no -gXXXX at the end for anything I release ;-)
> 
>> (I assumed it was to force a measure of uniformity/reproducibility).
> 
> You cannot force uniformity/reproducibility with fixed abbrev,
> unless you set abbreviation length to 40, so you are correct to add
> "a measure of" there ;-)
Indeed. ;-)
Show 6 quoted lines
>                            The first choice (i.e. 4) may have had a
> justification to force absolute minimum, and the second one (i.e. 7)
> may have had a justifiation to make it clear that we are using the
> same setting as the default, so in post-1.7.10 era, I think it is
> fine for us to just say "we have been using the same as default, so
> let's not specify anything explicitly".
So, you would be happy with just removing the '--abbrev' argument?
(That's fine by me; I don't set core.abbrev!)

ATB, Ramsay Jones

Junio C Hamano· Dec 5, 2016, 18:01 UTC · re: Jeff King · lore

Re: [RFC PATCH] GIT-VERSION-GEN: set --abbrev=9 to match auto-scaling

Jeff King <peff@peff.net> writes:
Show 29 quoted lines
> On Sun, Dec 04, 2016 at 08:45:59PM +0000, Ramsay Jones wrote:
>
>> 
>> Signed-off-by: Ramsay Jones <ramsay@ramsayjones.plus.com>
>> ---
>> 
>> Hi Junio,
>> 
>> I recently noticed that:
>> 
>>     $ make >pout 2>&1
>>     $ ./git version
>>     git version 2.11.0.286.g109e8a9
>>     $ git describe
>>     v2.11.0-286-g109e8a99d
>>     $
>> 
>> ... for non-release builds, the commit part of the version
>> string was still using an --abbrev=7.
>
> It seems like this kind of discussion ought to go in the commit message.
> :)
>
> That said, I think the right patch may be to just drop --abbrev
> entirely.
> ...
> I think at that point it was a noop, as 7 should have been the default.
> And now we probably ought to drop it, so that we can use the
> auto-scaling default.
Yeah, I agree.

It does mean that snapshot binaries built out of the same commit in the same repository before and after a repack have higher chances of getting named differently, which may surprise people, but that already is possible with a fixed length if the repacking involves pruning (albeit with lower probabilities), and I do not think it is a problem.

Jeff King· Dec 6, 2016, 14:03 UTC · re: Junio C Hamano · lore

Re: [RFC PATCH] GIT-VERSION-GEN: set --abbrev=9 to match auto-scaling

On Mon, Dec 05, 2016 at 10:01:19AM -0800, Junio C Hamano wrote:
Show 15 quoted lines
> > That said, I think the right patch may be to just drop --abbrev
> > entirely.
> > ...
> > I think at that point it was a noop, as 7 should have been the default.
> > And now we probably ought to drop it, so that we can use the
> > auto-scaling default.
> 
> Yeah, I agree.
> 
> It does mean that snapshot binaries built out of the same commit in
> the same repository before and after a repack have higher chances of
> getting named differently, which may surprise people, but that
> already is possible with a fixed length if the repacking involves
> pruning (albeit with lower probabilities), and I do not think it is
> a problem.

I think that the number is already unstable, even with --abbrev, as it just specifies a minimum. So any operation that creates objects has a possibility of increasing the length. Or more likely, two people describing the same version may end up with different strings because they have different objects in their repositories (e.g., I used to carry's trast's git-notes archive of the mailing list which added quite a few objects).

I agree that having it change over the course of a repack is slightly _more_ surprising than those cases, but ultimately the value just isn't stable.

-Peff
Junio C Hamano· Dec 6, 2016, 18:17 UTC · re: Jeff King · lore

Re: [RFC PATCH] GIT-VERSION-GEN: set --abbrev=9 to match auto-scaling

Jeff King <peff@peff.net> writes:
Show 29 quoted lines
> On Mon, Dec 05, 2016 at 10:01:19AM -0800, Junio C Hamano wrote:
>
>> > That said, I think the right patch may be to just drop --abbrev
>> > entirely.
>> > ...
>> > I think at that point it was a noop, as 7 should have been the default.
>> > And now we probably ought to drop it, so that we can use the
>> > auto-scaling default.
>> 
>> Yeah, I agree.
>> 
>> It does mean that snapshot binaries built out of the same commit in
>> the same repository before and after a repack have higher chances of
>> getting named differently, which may surprise people, but that
>> already is possible with a fixed length if the repacking involves
>> pruning (albeit with lower probabilities), and I do not think it is
>> a problem.
>
> I think that the number is already unstable, even with --abbrev, as it
> just specifies a minimum. So any operation that creates objects has a
> possibility of increasing the length. Or more likely, two people
> describing the same version may end up with different strings because
> they have different objects in their repositories (e.g., I used to
> carry's trast's git-notes archive of the mailing list which added quite
> a few objects).
>
> I agree that having it change over the course of a repack is slightly
> _more_ surprising than those cases, but ultimately the value just isn't
> stable.

Yup, that is what I meant to say with "that is already possible" and we are on the same page. As all three of us seem to be happy with just dropping --abbrev and letting describe do its default thing (as configured by whoever is doing the build), let's do so.

-- >8 --
From: Ramsay Jones <ramsay@ramsayjones.plus.com>
Date: Sun, 4 Dec 2016 20:45:59 +0000
Subject: [PATCH] GIT-VERSION-GEN: do not force abbreviation length used by 'describe'

The default version name for a Git binary is computed by running "git describe" on the commit the binary is made out of, basing on a tag whose name matches "v[0-9]*", e.g. v2.11.0-rc2-2-g7f1dc9.

In the very early days, with 9b88fcef7d ("Makefile: use git-describe to mark the git version.", 2005-12-27), we used "--abbrev=4" to get absolute minimum number of abbreviated commit object name. This was later changed to match the default minimum of 7 with bf505158d0 ("Git 1.7.10.1", 2012-05-01).

These days, the "default minimum" scales automatically depending on the size of the repository, and there is no point in specifying a particular abbreviation length; all we wanted since Git 1.7.10.1 days was to get "something reasonable we would use by default".

Just drop "--abbrev=<number>" from the invocation of "git describe" and let the command pick what it thinks is appropriate, taking the end user's configuration and the repository contents into account.

Signed-off-by: Ramsay Jones <ramsay@ramsayjones.plus.com>
Helped-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 GIT-VERSION-GEN | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to GIT-VERSION-GEN +1 −1
diff --git a/GIT-VERSION-GEN b/GIT-VERSION-GEN
index 556fbfc104..f95b04bb36 100755
--- a/GIT-VERSION-GEN
+++ b/GIT-VERSION-GEN
@@ -12,7 +12,7 @@ if test -f version
 then
 	VN=$(cat version) || VN="$DEF_VER"
 elif test -d ${GIT_DIR:-.git} -o -f .git &&
-	VN=$(git describe --match "v[0-9]*" --abbrev=7 HEAD 2>/dev/null) &&
+	VN=$(git describe --match "v[0-9]*" HEAD 2>/dev/null) &&
 	case "$VN" in
 	*$LF*) (exit 1) ;;
 	v[0-9]*)
-- 
2.11.0-263-gd89495280e
Jeff King· Dec 6, 2016, 18:26 UTC · re: Junio C Hamano · lore

Re: [RFC PATCH] GIT-VERSION-GEN: set --abbrev=9 to match auto-scaling

On Tue, Dec 06, 2016 at 10:17:55AM -0800, Junio C Hamano wrote:
Show 9 quoted lines
> Yup, that is what I meant to say with "that is already possible" and
> we are on the same page.  As all three of us seem to be happy with
> just dropping --abbrev and letting describe do its default thing (as
> configured by whoever is doing the build), let's do so.
> 
> -- >8 --
> From: Ramsay Jones <ramsay@ramsayjones.plus.com>
> Date: Sun, 4 Dec 2016 20:45:59 +0000
> Subject: [PATCH] GIT-VERSION-GEN: do not force abbreviation length used by 'describe'

Thanks, this is a nice summary of the discussion, and the patch is obviously correct.

-Peff
Ramsay Jones· Dec 6, 2016, 20:43 UTC · re: Jeff King · lore

Re: [RFC PATCH] GIT-VERSION-GEN: set --abbrev=9 to match auto-scaling

On 06/12/16 18:26, Jeff King wrote:
Show 14 quoted lines
> On Tue, Dec 06, 2016 at 10:17:55AM -0800, Junio C Hamano wrote:
> 
>> Yup, that is what I meant to say with "that is already possible" and
>> we are on the same page.  As all three of us seem to be happy with
>> just dropping --abbrev and letting describe do its default thing (as
>> configured by whoever is doing the build), let's do so.
>>
>> -- >8 --
>> From: Ramsay Jones <ramsay@ramsayjones.plus.com>
>> Date: Sun, 4 Dec 2016 20:45:59 +0000
>> Subject: [PATCH] GIT-VERSION-GEN: do not force abbreviation length used by 'describe'
> 
> Thanks, this is a nice summary of the discussion, and the patch is
> obviously correct.

Yep, looks very good to me! (I had started to write a commit message for this patch, when your email arrived. Needless to say, but your message is much better than mine!)

Thanks!

ATB, Ramsay Jones

← back to recent threads