Re: [PATCH 4/4] core.abbrev: raise the default abbreviation to 12 hexdigits

Linus Torvalds writes: > On Fri, Sep 30, 2016 at 10:54 AM, Linus Torvalds > wrote: >> >>> So IMHO, the best combination is the init_default_abbrev() you posted in >>> [1], but initialized at the top of find_unique_abbrev(). And

Re: [PATCH 4/4] core.abbrev: raise the default abbreviation to 12 hexdigits

On Fri, Sep 30, 2016 at 3:01 AM, Linus Torvalds wrote: > On Thu, Sep 29, 2016 at 5:56 PM, Mike Hommey wrote: >> >> OTOH, how often does one refer to trees or blobs with abbreviated sha1s? >> Most of the time, you'd use abbreviated sha1s for

Re: [PATCH 4/4] core.abbrev: raise the default abbreviation to 12 hexdigits

Linus Torvalds writes: > Considering that TRANSPORT_SUMMARY and TRANSPORT_SUMMARY_WIDTH are > both used in exactly one place each, I'd suggest getting rid of that > crazy macro, and just expanding it in those places to avoid these > kinds of crazy "hiding variables

Re: [PATCH 4/4] core.abbrev: raise the default abbreviation to 12 hexdigits

On Fri, Sep 30, 2016 at 11:40 AM, Junio C Hamano wrote: > > There is another instance buried deep in an obscure macro. A > minimum fix may look like this, but I really hope somebody else > finds a better approach. Heh. Yeah, that's just ugly. I assume this is why the odd git

Re: [PATCH 4/4] core.abbrev: raise the default abbreviation to 12 hexdigits

Junio C Hamano writes: > From: Junio C Hamano > Date: Thu, 29 Sep 2016 21:19:20 -0700 > Subject: [PATCH] abbrev: adjust to the new world order > > The default_abbrev used to be a concrete value usable as the default > abbreviation length. The code that

Re: [PATCH 4/4] core.abbrev: raise the default abbreviation to 12 hexdigits

On Fri, Sep 30, 2016 at 10:54 AM, Linus Torvalds wrote: > >> So IMHO, the best combination is the init_default_abbrev() you posted in >> [1], but initialized at the top of find_unique_abbrev(). And cached >> there, obviously, in a similar way. > > That's certainly

Re: [PATCH 4/4] core.abbrev: raise the default abbreviation to 12 hexdigits

On Fri, Sep 30, 2016 at 10:54:16AM -0700, Linus Torvalds wrote: > On Fri, Sep 30, 2016 at 1:06 AM, Jeff King wrote: > > > > I agree that this deals with the performance concerns by caching the > > default_abbrev_len and starting there. I still think it's unnecessarily > > invasive

Re: [PATCH 4/4] core.abbrev: raise the default abbreviation to 12 hexdigits

Jeff King writes: > I agree that this deals with the performance concerns by caching the > default_abbrev_len and starting there. I still think it's unnecessarily > invasive to touch get_short_sha1() at all, which is otherwise only a > reading function. > > So IMHO, the best

Re: [PATCH 4/4] core.abbrev: raise the default abbreviation to 12 hexdigits

On Fri, Sep 30, 2016 at 1:06 AM, Jeff King wrote: > > I agree that this deals with the performance concerns by caching the > default_abbrev_len and starting there. I still think it's unnecessarily > invasive to touch get_short_sha1() at all, which is otherwise only a > reading

Re: [PATCH 4/4] core.abbrev: raise the default abbreviation to 12 hexdigits

On Thu, Sep 29, 2016 at 06:18:03PM -0700, Linus Torvalds wrote: > On Thu, Sep 29, 2016 at 5:57 PM, Linus Torvalds > wrote: > > > > Actually, all the other cases seem to be "parse a SHA1 with a known > > length", so they really don't have a negative length. So this

Re: [PATCH 4/4] core.abbrev: raise the default abbreviation to 12 hexdigits

On Thu, Sep 29, 2016 at 04:13:38PM -0700, Junio C Hamano wrote: > There are very early ones in the program startup sequence in the > following functions, but I do not think of a reason why our new and > early call to prepare_packed_git() might be problematic, given that > all of them require us

Re: [PATCH 4/4] core.abbrev: raise the default abbreviation to 12 hexdigits

Junio C Hamano writes: > Junio C Hamano writes: > >> There still are breakages seen in t5510 and t5526 that are about the >> verbose output of "git fetch". I'll stop digging at this point >> tonight, and welcome others who look into it ;-) > > OK, just

Re: [PATCH 4/4] core.abbrev: raise the default abbreviation to 12 hexdigits

On Thu, Sep 29, 2016 at 9:18 PM, Linus Torvalds wrote: > > There are probably other things like that. t5510-fetch.sh fails oddly, looks like the output is off by one character. not ok 77 - fetch aligned output It has a magic "cut -c 22-" that expects the

Re: [PATCH 4/4] core.abbrev: raise the default abbreviation to 12 hexdigits

Junio C Hamano writes: > There still are breakages seen in t5510 and t5526 that are about the > verbose output of "git fetch". I'll stop digging at this point > tonight, and welcome others who look into it ;-) OK, just before I leave the keyboard for the night... -- >8 --

Re: [PATCH 4/4] core.abbrev: raise the default abbreviation to 12 hexdigits

On Thu, Sep 29, 2016 at 9:10 PM, Junio C Hamano wrote: > > A quick and dirty fix for it may look like this. Crossed emails. Indeed, I just solved the builtin/rev-parse.c thing slightly differently. And you found another failure in the diff code similarly not liking the

Re: [PATCH 4/4] core.abbrev: raise the default abbreviation to 12 hexdigits

On Thu, Sep 29, 2016 at 8:54 PM, Junio C Hamano wrote: > > * The script uses "git rev-parse --short HEAD"; I suspect that it >says "ah, default_abbrev is -1 and minimum_abbrev is 4, so let's >try abbreviating to 4 hexdigits". Ahh, right you are. The logic there is

Re: [PATCH 4/4] core.abbrev: raise the default abbreviation to 12 hexdigits

Junio C Hamano writes: > Linus Torvalds writes: > >> So this patch may actually be "production ready" apart from the fact >> that some tests still fail (at least t2027-worktree-list.sh) because >> of different short SHA1 cases. > > t2027 has at

Re: [PATCH 4/4] core.abbrev: raise the default abbreviation to 12 hexdigits

Linus Torvalds writes: > So this patch may actually be "production ready" apart from the fact > that some tests still fail (at least t2027-worktree-list.sh) because > of different short SHA1 cases. t2027 has at least two problems. * "git worktree" does not read

Re: [PATCH 4/4] core.abbrev: raise the default abbreviation to 12 hexdigits

On Thu, Sep 29, 2016 at 12:06:23PM -0700, Linus Torvalds wrote: > On Thu, Sep 29, 2016 at 11:55 AM, Linus Torvalds > wrote: > > > > For the kernel, just the *math* right now actually gives 12 > > characters. For current git it actually seems to say that 8 is the > >

Re: [PATCH 4/4] core.abbrev: raise the default abbreviation to 12 hexdigits

On Thu, Sep 29, 2016 at 5:57 PM, Linus Torvalds wrote: > > Actually, all the other cases seem to be "parse a SHA1 with a known > length", so they really don't have a negative length. So this seems > ok, and is easier to verify than the "what all contexts might use

Re: [PATCH 4/4] core.abbrev: raise the default abbreviation to 12 hexdigits

On Thu, Sep 29, 2016 at 5:56 PM, Mike Hommey wrote: > > OTOH, how often does one refer to trees or blobs with abbreviated sha1s? > Most of the time, you'd use abbreviated sha1s for commits. And the number > of commits in git and the kernel repositories are much lower than the >

Re: [PATCH 4/4] core.abbrev: raise the default abbreviation to 12 hexdigits

On Thu, Sep 29, 2016 at 5:28 PM, Linus Torvalds wrote: > > To be fair, my original patch had a different worry that I didn't > bother with: what if one of the _other_ callers of "get_short_sha1()" > passed in -1 to it. I only handled the -1 case in th eone path

Re: [PATCH 4/4] core.abbrev: raise the default abbreviation to 12 hexdigits

On Thu, Sep 29, 2016 at 5:20 PM, Linus Torvalds wrote: > > As you say, my original patch had neither of those issues. To be fair, my original patch had a different worry that I didn't bother with: what if one of the _other_ callers of "get_short_sha1()" passed in

Re: [PATCH 4/4] core.abbrev: raise the default abbreviation to 12 hexdigits

On Thu, Sep 29, 2016 at 4:13 PM, Junio C Hamano wrote: > > One thing that worries me is if we are ready to start accessing the > object store in all codepaths when we ask for DEFAULT_ABBREV. Yes. That was my main worry too. I also looked at just doing an explicit if

Re: [PATCH 4/4] core.abbrev: raise the default abbreviation to 12 hexdigits

Junio C Hamano writes: > Linus Torvalds writes: > >> The advantage of the >> previous patch was that it got the object counting right almost >> automatically, this actually has its own new object counting code and >> maybe I screwed it up. I

Re: [PATCH 4/4] core.abbrev: raise the default abbreviation to 12 hexdigits

Linus Torvalds writes: > Somebody should really double-check my heuristics, to see that I did > the pack counting etc right. It doesn't do alternate loose file > counting at all, and maybe it could matter. The advantage of the > previous patch was that it got the

Re: [PATCH 4/4] core.abbrev: raise the default abbreviation to 12 hexdigits

On Thu, Sep 29, 2016 at 12:45 PM, Junio C Hamano wrote: > > I think that is a reasonable way to go. > > #define DEFAULT_ABBREV get_default_abbrev() > > would help. So something like this that replaces the previous patch? Somebody should really double-check my heuristics, to

Re: [PATCH 4/4] core.abbrev: raise the default abbreviation to 12 hexdigits

Linus Torvalds writes: > But you could easily also just instead have it do something like > > if (default_abbrev < 0) > default_abbrev = initialize_abbrev(); > > at startup time if "abbrev_commit" is set, and just do it once and for > all rather

Re: [PATCH 4/4] core.abbrev: raise the default abbreviation to 12 hexdigits

On Thu, Sep 29, 2016 at 12:16 PM, Jeff King wrote: > > Hmm. So at length 7, we expect collisions at 2^14, which is 16384. That > seems really low. I mean, by the birthday paradox that's where expect > a 50% chance of a collision. But that's a single collision. We > definitely don't

Re: [PATCH 4/4] core.abbrev: raise the default abbreviation to 12 hexdigits

On Thu, Sep 29, 2016 at 11:55:46AM -0700, Linus Torvalds wrote: > I think the patch can speak for itself, but the basic core is this > section in get_short_sha1(): > > + if (len < 16 && !status && (flags & GET_SHA1_AUTOMATIC)) { > + unsigned int expect_collision = 1 <<

Re: [PATCH 4/4] core.abbrev: raise the default abbreviation to 12 hexdigits

On Thu, Sep 29, 2016 at 11:55 AM, Linus Torvalds wrote: > > For the kernel, just the *math* right now actually gives 12 > characters. For current git it actually seems to say that 8 is the > correct number. For small projects, you'll still see 7. Sorry, the git

Re: [PATCH 4/4] core.abbrev: raise the default abbreviation to 12 hexdigits

On Thu, Sep 29, 2016 at 11:37 AM, Linus Torvalds wrote: > > I'm playing with an early patch to make the default more dynamic. > Let's see how well it works in practice, but it looks fairly > promising. Let me test a bit more and send out an RFC patch.. Ok, this is

Re: [PATCH 4/4] core.abbrev: raise the default abbreviation to 12 hexdigits

On Thu, Sep 29, 2016 at 11:05 AM, Junio C Hamano wrote: > > Yes, "git log --oneline" looks somewhat different and strange for > me, too ;-) I'm playing with an early patch to make the default more dynamic. Let's see how well it works in practice, but it looks fairly promising.

Re: [PATCH 4/4] core.abbrev: raise the default abbreviation to 12 hexdigits

Johannes Sixt writes: > Am 29.09.2016 um 01:30 schrieb Junio C Hamano: >> As Peff said, responding in a thread started by Linus's suggestion >> to raise the default abbreviation to 12 hexdigits: > > This is waayy too large for a new default. The vast majority of > repositories is

Re: [PATCH 4/4] core.abbrev: raise the default abbreviation to 12 hexdigits

Quoting Jeff King : On Thu, Sep 29, 2016 at 04:44:00AM +0200, SZEDER Gábor wrote: > So 12 seems reasonable, and the only downside for it (or for "13", for > that matter) is a few extra bytes. I dunno, maybe people will really > hate that, but I have a feeling these

Re: [PATCH 4/4] core.abbrev: raise the default abbreviation to 12 hexdigits

Jeff King writes: > On Thu, Sep 29, 2016 at 04:44:00AM +0200, SZEDER Gábor wrote: > >> > So 12 seems reasonable, and the only downside for it (or for "13", for >> > that matter) is a few extra bytes. I dunno, maybe people will really >> > hate that, but I have a

Re: [PATCH 4/4] core.abbrev: raise the default abbreviation to 12 hexdigits

On Wed, Sep 28, 2016 at 04:30:47PM -0700, Junio C Hamano wrote: > As Peff said, responding in a thread started by Linus's suggestion > to raise the default abbreviation to 12 hexdigits: > > I actually think "12" might be sane for a long time. That's 48 bits of > sha1, so we'd expect a

Re: [PATCH 4/4] core.abbrev: raise the default abbreviation to 12 hexdigits

On Thu, Sep 29, 2016 at 07:27:01AM +0200, Lukas Fleischer wrote: > > Sure, users working on smaller repos are free to reset core.abbrev to > > its original value. I don't have any numbers, of course, but I > > suspect that there are many more smaller repos out there that this > > change will

Re: [PATCH 4/4] core.abbrev: raise the default abbreviation to 12 hexdigits

On Thu, Sep 29, 2016 at 04:44:00AM +0200, SZEDER Gábor wrote: > > So 12 seems reasonable, and the only downside for it (or for "13", for > > that matter) is a few extra bytes. I dunno, maybe people will really > > hate that, but I have a feeling these are mostly cut-and-pasted anyway.

Re: [PATCH 4/4] core.abbrev: raise the default abbreviation to 12 hexdigits

Am 29.09.2016 um 01:30 schrieb Junio C Hamano: As Peff said, responding in a thread started by Linus's suggestion to raise the default abbreviation to 12 hexdigits: This is waayy too large for a new default. The vast majority of repositories is smallish. For those, the long sequences of hex

Re: [PATCH 4/4] core.abbrev: raise the default abbreviation to 12 hexdigits

On Thu, 29 Sep 2016 at 04:44:00, SZEDER Gábor wrote: > I for one raise my hand in protest... > > "few extra bytes" is not the only downside, and it's not at all about > how many characters are copy-and-pasted. In my opinion it's much more > important that this change wastes 5 columns worth of

Re: [PATCH 4/4] core.abbrev: raise the default abbreviation to 12 hexdigits

> As Peff said, responding in a thread started by Linus's suggestion > to raise the default abbreviation to 12 hexdigits: > > I actually think "12" might be sane for a long time. That's 48 bits of > sha1, so we'd expect a 50% change of a _single_ collision at 2^24, or 16 s/change/chance/

[PATCH 4/4] core.abbrev: raise the default abbreviation to 12 hexdigits

As Peff said, responding in a thread started by Linus's suggestion to raise the default abbreviation to 12 hexdigits: I actually think "12" might be sane for a long time. That's 48 bits of sha1, so we'd expect a 50% change of a _single_ collision at 2^24, or 16 million. The biggest