* Re: [PATCH 4/4] core.abbrev: raise the default abbreviation to 12 hexdigits
From: Junio C Hamano @ 2016-09-30 3:54 UTC (permalink / raw)
To: Linus Torvalds; +Cc: Jeff King, Johannes Sixt, Git Mailing List
In-Reply-To: <CA+55aFyHn0Q-qPq4dPEJ7X_4jf5UbsVw2vE-4LoWYbPn6gS10g@mail.gmail.com>
Linus Torvalds <torvalds@linux-foundation.org> 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 the core.abbrev configuration,
without a recent fix in jc/worktree-config, i.e. d49028e6
("worktree: honor configuration variables", 2016-09-26).
* 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".
The first failure in t3203 seems to come from the same issue in
"rev-parse --short".
^ permalink raw reply
* Re: [PATCH 4/4] core.abbrev: raise the default abbreviation to 12 hexdigits
From: Junio C Hamano @ 2016-09-30 4:10 UTC (permalink / raw)
To: Linus Torvalds; +Cc: Jeff King, Johannes Sixt, Git Mailing List
In-Reply-To: <xmqqtwcyavou.fsf@gitster.mtv.corp.google.com>
Junio C Hamano <gitster@pobox.com> writes:
> Linus Torvalds <torvalds@linux-foundation.org> 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 the core.abbrev configuration,
> without a recent fix in jc/worktree-config, i.e. d49028e6
> ("worktree: honor configuration variables", 2016-09-26).
>
> * 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".
>
> The first failure in t3203 seems to come from the same issue in
> "rev-parse --short".
A quick and dirty fix for it may look like this.
We leave the variable abbrev to DEFAULT_ABBREV and let
find_unique_abbrev() react to "eh, -1? I need to do the
auto-scaling". "git diff-tree --abbrev" seems to have a similar
problem, and the fix is the same.
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 ;-)
builtin/rev-parse.c | 14 ++++++++------
diff.c | 2 +-
2 files changed, 9 insertions(+), 7 deletions(-)
diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
index 76cf05e2ad..f8c8c6c22e 100644
--- a/builtin/rev-parse.c
+++ b/builtin/rev-parse.c
@@ -642,13 +642,15 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
starts_with(arg, "--short=")) {
filter &= ~(DO_FLAGS|DO_NOREV);
verify = 1;
- abbrev = DEFAULT_ABBREV;
- if (arg[7] == '=')
+ if (arg[7] != '=') {
+ abbrev = DEFAULT_ABBREV;
+ } else {
abbrev = strtoul(arg + 8, NULL, 10);
- if (abbrev < MINIMUM_ABBREV)
- abbrev = MINIMUM_ABBREV;
- else if (40 <= abbrev)
- abbrev = 40;
+ if (abbrev < MINIMUM_ABBREV)
+ abbrev = MINIMUM_ABBREV;
+ else if (40 <= abbrev)
+ abbrev = 40;
+ }
continue;
}
if (!strcmp(arg, "--sq")) {
diff --git a/diff.c b/diff.c
index c6da383c56..cefc13eb8e 100644
--- a/diff.c
+++ b/diff.c
@@ -3399,7 +3399,7 @@ void diff_setup_done(struct diff_options *options)
*/
read_cache();
}
- if (options->abbrev <= 0 || 40 < options->abbrev)
+ if (40 < options->abbrev)
options->abbrev = 40; /* full */
/*
^ permalink raw reply related
* Re: [PATCH 4/4] core.abbrev: raise the default abbreviation to 12 hexdigits
From: Linus Torvalds @ 2016-09-30 4:11 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, Johannes Sixt, Git Mailing List
In-Reply-To: <xmqqtwcyavou.fsf@gitster.mtv.corp.google.com>
[-- Attachment #1: Type: text/plain, Size: 1048 bytes --]
On Thu, Sep 29, 2016 at 8:54 PM, Junio C Hamano <gitster@pobox.com> 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
abbrev = DEFAULT_ABBREV;
if (arg[7] == '=')
abbrev = strtoul(arg + 8, NULL, 10);
if (abbrev < MINIMUM_ABBREV)
abbrev = MINIMUM_ABBREV;
....
which now does something different than what it used to do because
DEFAULT_ABBREV is -1.
Putting the "sanity-check the abbrev range" tests inside the "if()"
statement that does strtoul() should fix it. Let me test...
[ short time passes ]
Yup. Incremental patch for that single issue attached. I made it do
an early "continue" instead of adding another level on indentation.
Linus
[-- Attachment #2: patch.diff --]
[-- Type: text/plain, Size: 629 bytes --]
builtin/rev-parse.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
index 4da1f1da2..cfb0f1510 100644
--- a/builtin/rev-parse.c
+++ b/builtin/rev-parse.c
@@ -671,8 +671,9 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
filter &= ~(DO_FLAGS|DO_NOREV);
verify = 1;
abbrev = DEFAULT_ABBREV;
- if (arg[7] == '=')
- abbrev = strtoul(arg + 8, NULL, 10);
+ if (!arg[7])
+ continue;
+ abbrev = strtoul(arg + 8, NULL, 10);
if (abbrev < MINIMUM_ABBREV)
abbrev = MINIMUM_ABBREV;
else if (40 <= abbrev)
^ permalink raw reply related
* Re: [PATCH 4/4] core.abbrev: raise the default abbreviation to 12 hexdigits
From: Linus Torvalds @ 2016-09-30 4:18 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, Johannes Sixt, Git Mailing List
In-Reply-To: <xmqqoa36auyx.fsf@gitster.mtv.corp.google.com>
On Thu, Sep 29, 2016 at 9:10 PM, Junio C Hamano <gitster@pobox.com> 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 negative DEFAULT_ABBREV. There are probably other things like
that.
Linus
^ permalink raw reply
* Re: [PATCH 4/4] core.abbrev: raise the default abbreviation to 12 hexdigits
From: Junio C Hamano @ 2016-09-30 4:27 UTC (permalink / raw)
To: Linus Torvalds; +Cc: Jeff King, Johannes Sixt, Git Mailing List
In-Reply-To: <xmqqoa36auyx.fsf@gitster.mtv.corp.google.com>
Junio C Hamano <gitster@pobox.com> 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 --
From: Junio C Hamano <gitster@pobox.com>
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 sets custom abbreviation length,
in response to command line argument, often did something like:
if (skip_prefix(arg, "--abbrev=", &arg))
abbrev = atoi(arg);
else if (!strcmp("--abbrev", &arg))
abbrev = DEFAULT_ABBREV;
/* make the value sane */
if (abbrev < 0 || 40 < abbrev)
abbrev = ... some sane value ...
The new world order however is that the default_abbrev is a negative
value that signals find_unique_abbrev() that it needs to dynamically
find out a good default value. We shouldn't coerce a negative value
into a random positive value like the above sample code.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
builtin/rev-parse.c | 5 +++--
diff.c | 2 +-
2 files changed, 4 insertions(+), 3 deletions(-)
diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
index 76cf05e2ad..17cbfabdde 100644
--- a/builtin/rev-parse.c
+++ b/builtin/rev-parse.c
@@ -643,8 +643,9 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
filter &= ~(DO_FLAGS|DO_NOREV);
verify = 1;
abbrev = DEFAULT_ABBREV;
- if (arg[7] == '=')
- abbrev = strtoul(arg + 8, NULL, 10);
+ if (!arg[7])
+ continue;
+ abbrev = strtoul(arg + 8, NULL, 10);
if (abbrev < MINIMUM_ABBREV)
abbrev = MINIMUM_ABBREV;
else if (40 <= abbrev)
diff --git a/diff.c b/diff.c
index c6da383c56..cefc13eb8e 100644
--- a/diff.c
+++ b/diff.c
@@ -3399,7 +3399,7 @@ void diff_setup_done(struct diff_options *options)
*/
read_cache();
}
- if (options->abbrev <= 0 || 40 < options->abbrev)
+ if (40 < options->abbrev)
options->abbrev = 40; /* full */
/*
--
2.10.0-612-g22341905f2
^ permalink raw reply related
* Re: [PATCH 4/4] core.abbrev: raise the default abbreviation to 12 hexdigits
From: Linus Torvalds @ 2016-09-30 4:29 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, Johannes Sixt, Git Mailing List
In-Reply-To: <CA+55aFw4=tGQZd0QO_8Zzs0AqPCpew_Wvnwft-JP2OzFbask8w@mail.gmail.com>
On Thu, Sep 29, 2016 at 9:18 PM, Linus Torvalds
<torvalds@linux-foundation.org> 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 output at a specific
place, and now it's at column 21 instead of column 22. Strange test,
but it still seems to be aligned, just in a different column.
But clearly something changed.
Linus
^ permalink raw reply
* Re: [PATCH 4/4] core.abbrev: raise the default abbreviation to 12 hexdigits
From: Junio C Hamano @ 2016-09-30 4:35 UTC (permalink / raw)
To: Git Mailing List; +Cc: Jeff King, Johannes Sixt, Linus Torvalds
In-Reply-To: <xmqqeg42au5w.fsf@gitster.mtv.corp.google.com>
Junio C Hamano <gitster@pobox.com> writes:
> Junio C Hamano <gitster@pobox.com> 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 --
> From: Junio C Hamano <gitster@pobox.com>
> Date: Thu, 29 Sep 2016 21:19:20 -0700
> Subject: [PATCH] abbrev: adjust to the new world order
To those who are following from sidelines, this builds on Linus's
third iteration patch (which is based on his first patch), applied
on Peff's "give disambiguation help when giving an ambiguity error"
series. I didn't merge the work-in-progress going back and forth
between Linus and I tonight to any of the integration branches, but
it is available as lt/abbrev-auto-2 branch of the "broken down"
repository, i.e.
git://github.com/gitster/git.git lt/abbrev-auto-2
^ permalink raw reply
* Re: [PATCH 10/10] get_short_sha1: list ambiguous objects on error
From: Jacob Keller @ 2016-09-30 5:51 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, Kyle J. McKay, Linus Torvalds, Git Mailing List
In-Reply-To: <xmqqbmz6hbdk.fsf@gitster.mtv.corp.google.com>
On Thu, Sep 29, 2016 at 10:19 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Jeff King <peff@peff.net> writes:
>> - "cat-file --batch-check" can show you the sha1 and type, but it
>> won't abbreviate sha1s, and it won't show you commit/tag information
>>
>> - "log --stdin --no-walk" will format the commit however you like, but
>> skips the trees and blobs entirely, and the tag can only be seen via
>> "%d"
>>
>> - "for-each-ref" has flexible formatting, too, but wants to format
>> refs, not objects (and doesn't read from stdin).
>
> - "name-rev" is used to give "describe --contains", and can read
> from its standard input, but has no format customization.
> Another downside of it is that it only wants to see
> committishes.
>
Some tool which reads standard input and can be formatted would be
nice. Extending name-rev with the same format options as for-each-ref
would be nice.
Thanks,
Jake
^ permalink raw reply
* Re: [PATCH 1/5] pretty: allow formatting DATE_SHORT
From: Jacob Keller @ 2016-09-30 6:17 UTC (permalink / raw)
To: Jeff King; +Cc: Kyle J. McKay, Git mailing list, Junio C Hamano
In-Reply-To: <20160929083342.ozo2tef45hu4ox7g@sigill.intra.peff.net>
On Thu, Sep 29, 2016 at 1:33 AM, Jeff King <peff@peff.net> wrote:
> There's no way to do this short of "%ad" and --date=short,
> but that limits you to having a single date format in the
> output.
>
> This would possibly be better done with something more like
> "%ad(short)".
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> pretty.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/pretty.c b/pretty.c
> index 493edb0..c532c17 100644
> --- a/pretty.c
> +++ b/pretty.c
> @@ -727,6 +727,9 @@ static size_t format_person_part(struct strbuf *sb, char part,
> case 'I': /* date, ISO 8601 strict */
> strbuf_addstr(sb, show_ident_date(&s, DATE_MODE(ISO8601_STRICT)));
> return placeholder_len;
> + case 's':
> + strbuf_addstr(sb, show_ident_date(&s, DATE_MODE(SHORT)));
> + return placeholder_len;
> }
>
> skip:
> --
> 2.10.0.566.g5365f87
>
Nice. I use date=short in some of my aliases and switching to this is
nicer. I assume this turns into "%(as)"?
What about documenting this in pretty-formats.txt?
Thanks,
Jake
^ permalink raw reply
* Re: [PATCH 4/4] core.abbrev: raise the default abbreviation to 12 hexdigits
From: Jeff King @ 2016-09-30 7:47 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Linus Torvalds, Johannes Sixt, Git Mailing List
In-Reply-To: <xmqqbmz6cna5.fsf@gitster.mtv.corp.google.com>
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 to have an access to the repository (i.e.
> this change cannot introduce a regression where a command used to
> work outside a repository but barf when prepare_packed_git() is
> called early):
>
> - builtin/describe.c
> - builtin/rev-list.c
> - builtin/rev-parse.c
>
> I thought that the one in diff.c might be problematic when the "git
> diff" command is run outside a repository with the "--no-index"
> option, but it appears that init_default_abbrev() seems to be OK
> when run outside a repository.
Actually, "diff --no-index" is currently buggy in this regard. In the
followup series to jk/setup-sequence-update (which I mentioned but
haven't posted yet), I teach get_object_dir() not to blindly default to
".git", and found that "diff --no-index" is perfectly happy to look in
".git/objects" for find_unique_abbrev(), even when we know there's no
repository (or it has an unknown vintage).
I fixed it there by just using the default abbrev value for out-of-repo
diffs, and skip calling find_unique_abbrev() at all. That would here,
too.
But if we add object-store initialization at other times, it's a
potential conflict. IMHO this should stay inside find_unique_abbrev(),
where we know we already must look at the object store.
-Peff
^ permalink raw reply
* Re: [PATCH 4/4] core.abbrev: raise the default abbreviation to 12 hexdigits
From: Jeff King @ 2016-09-30 8:06 UTC (permalink / raw)
To: Linus Torvalds; +Cc: Junio C Hamano, Johannes Sixt, Git Mailing List
In-Reply-To: <CA+55aFyHn0Q-qPq4dPEJ7X_4jf5UbsVw2vE-4LoWYbPn6gS10g@mail.gmail.com>
On Thu, Sep 29, 2016 at 06:18:03PM -0700, Linus Torvalds wrote:
> On Thu, Sep 29, 2016 at 5:57 PM, Linus Torvalds
> <torvalds@linux-foundation.org> 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
> > DEFAULT_ABBREV" thing. There's only a few callers, and it's a static
> > function so it's easy to check it locally in sha1_name.c.
>
> Here's my original patch with just a tiny change that instead of
> starting the automatic guessing at 7 each time, it starts at
> "default_automatic_abbrev", which is initialized to 7.
>
> The difference is that if we decide that "oh, that was too small, need
> to repeat", we also update that "default_automatic_abbrev" value, so
> that we won't start at the number that we now know was too small.
>
> So it still loops over the abbrev values, but now it only loops a
> couple of times.
>
> I actually verified the performance impact by doing
>
> time git rev-list --abbrev-commit HEAD > /dev/null
>
> on the kernel git tree, and it does actually matter. With my original
> patch, we wasted a noticeable amount of time on just the extra
> looping, with this it's down to the same performance as just doing it
> once at init time (it's about 12s vs 9s on my laptop).
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 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.
-Peff
[1] http://public-inbox.org/git/CA+55aFyVEQ+8TBBUm5KG9APtd9wy8cp_mRO=3nj12DXZNLAC9A@mail.gmail.com/
^ permalink raw reply
* Re: [PATCH v2] gpg-interface: use more status letters
From: Michael J Gruber @ 2016-09-30 9:33 UTC (permalink / raw)
To: git; +Cc: git, Alex, Ramsay Jones
In-Reply-To: <xmqqshsjiyn4.fsf@gitster.mtv.corp.google.com>
Junio C Hamano venit, vidit, dixit 28.09.2016 21:59:
> Michael J Gruber <git@drmicha.warpmail.net> writes:
>
>> - Use GNUPGHOME="$HOME/gnupg-home-not-used" just like in other tests (lib).
>
> If you are not using /dev/null, I expected you to do
>
> . ./test-lib.sh
> GNUPGHOME_saved=$GNPGHOME
> . "$TEST_DIRECTORY/lib-gpg.sh"
>
> and then use
>
> GNUPGHOME="$GNUPGHOME_saved" git log -1 ...
>
> in the test.
>
> Otherwise, you are not futureproofing your use and only adding to
> maintenance burden. The gnupg-home-not-used hack may turn out to be
> a problematic and test-lib.sh may update to point to somewhere else,
> which will leave your copy still pointing at the old problematic
> place).
Well, I understood you told me to do what test-lib.sh does.
You obviously wanted me to piggy-bak on test-lib.sh's behavior instead.
I don't know what's more likely to break - the latter relies on
test-lib.sh's setting GNUPGHOME to a non existing gpg home, which is
something funny to do if you don't even plan to use gpg.
>> - Do not parse for signer UID in the ERRSIG case (and test that we do not).
>
> Good.
>
>> - Retreat "rather" addition from the doc: good/valid are terms that we use
>> differently from gpg anyways.
>
> OK.
>
>> + "X" for a good expired signature, or good signature made by an expired key,
>
> As an attempt to clarify that we cover both EXPSIG and EXPKEYSIG
> cases, I think this is good enough. I may have phrased the former
> slightly differently, though: "a good signature that has expired".
>
> I have no strong opinion if we want to stress that we cover both
> cases, though, which is I think what Ramsay's comment was about.
I'll comment in the reply to his 2nd e-mail....
Michael
^ permalink raw reply
* Re: [PATCH v2] gpg-interface: use more status letters
From: Michael J Gruber @ 2016-09-30 9:41 UTC (permalink / raw)
To: Ramsay Jones, Junio C Hamano; +Cc: git, Alex
In-Reply-To: <24ecc903-3e5a-47f6-f073-00a1c709d5e8@ramsayjones.plus.com>
Ramsay Jones venit, vidit, dixit 28.09.2016 23:09:
>
>
> On 28/09/16 20:59, Junio C Hamano wrote:
>> Michael J Gruber <git@drmicha.warpmail.net> writes:
>
>>> + "X" for a good expired signature, or good signature made by an expired key,
>>
>> As an attempt to clarify that we cover both EXPSIG and EXPKEYSIG
>> cases, I think this is good enough. I may have phrased the former
>> slightly differently, though: "a good signature that has expired".
>>
>> I have no strong opinion if we want to stress that we cover both
>> cases, though, which is I think what Ramsay's comment was about.
>
> Kinda! ;-)
>
> I'm not sure that it is a good idea to mash both EXPSIG and EXPKEYSIG
> into one status letter, but I was also fishing for some information
> about EXPSIG. I was only vaguely aware that a signature could expire
> _independently_ of the key used to do the signing. Also, according to
> https://www.gnupg.org/documentation/manuals/gnupg/Automated-signature-checking.html
> for the EXPSIG case 'Note, that this case is currently not implemented.'
A key can have an expiration date.
A signature can have an expiration date.
The "goodness" of a signature is independent of the expiraton dates.
Signature expiration is implemented, I tested that (gpg 1 aka "classic").
> Hmm, I guess these are so closely related that a single status letter
> is OK, but I think I would prefer your phrasing; namely:
>
> "X" for a good signature that has expired, or a good signature made with an expired key,
>
I'm open to whatever phrasing you deem clearer.
Also, I'm open to using another letter for EXPKEYSIG but couldn't decide
between 'Y', 'Z', 'K'. 'K' could be confused with REVKEYSIG, I'm afraid.
'Y' is next to 'X' and contained in 'KEY', it would be my first choice.
Cheers,
Michael
^ permalink raw reply
* Re: [PATCH 1/5] pretty: allow formatting DATE_SHORT
From: SZEDER Gábor @ 2016-09-30 10:56 UTC (permalink / raw)
To: Jacob Keller
Cc: SZEDER Gábor, Jeff King, Kyle J. McKay, Git mailing list,
Junio C Hamano
In-Reply-To: <CA+P7+xoxTpqn=jkuHYp5pKCCWfKLP5OKCTBYkcTVw_RhEw0KVw@mail.gmail.com>
> On Thu, Sep 29, 2016 at 1:33 AM, Jeff King <peff@peff.net> wrote:
> > There's no way to do this short of "%ad" and --date=short,
> > but that limits you to having a single date format in the
> > output.
> >
> > This would possibly be better done with something more like
> > "%ad(short)".
> >
> > Signed-off-by: Jeff King <peff@peff.net>
> > ---
> > pretty.c | 3 +++
> > 1 file changed, 3 insertions(+)
> >
> > diff --git a/pretty.c b/pretty.c
> > index 493edb0..c532c17 100644
> > --- a/pretty.c
> > +++ b/pretty.c
> > @@ -727,6 +727,9 @@ static size_t format_person_part(struct strbuf *sb, char part,
> > case 'I': /* date, ISO 8601 strict */
> > strbuf_addstr(sb, show_ident_date(&s, DATE_MODE(ISO8601_STRICT)));
> > return placeholder_len;
> > + case 's':
> > + strbuf_addstr(sb, show_ident_date(&s, DATE_MODE(SHORT)));
> > + return placeholder_len;
> > }
> >
> > skip:
> > --
> > 2.10.0.566.g5365f87
> >
>
> Nice. I use date=short in some of my aliases and switching to this is
> nicer. I assume this turns into "%(as)"?
>
> What about documenting this in pretty-formats.txt?
Here you go :)
http://public-inbox.org/git/1444235305-8718-1-git-send-email-szeder@ira.uka.de/
^ permalink raw reply
* Re: [PATCH v2] gpg-interface: use more status letters
From: Junio C Hamano @ 2016-09-30 16:16 UTC (permalink / raw)
To: Michael J Gruber; +Cc: Ramsay Jones, git, Alex
In-Reply-To: <85fa6296-17f0-0e8c-ec1b-54cd48c45223@drmicha.warpmail.net>
Michael J Gruber <git@drmicha.warpmail.net> writes:
> Also, I'm open to using another letter for EXPKEYSIG but couldn't decide
> between 'Y', 'Z', 'K'. 'K' could be confused with REVKEYSIG, I'm afraid.
> 'Y' is next to 'X' and contained in 'KEY', it would be my first choice.
Sounds good enough to me. Thanks.
^ permalink raw reply
* "Purposes, Concepts,Misfits, and a Redesign of Git" (a research paper)
From: Konstantin Khomoutov @ 2016-09-30 16:14 UTC (permalink / raw)
To: git
The "It Will Never Work in Theory" blog has just posted a summary of a
study which tried to identify shortcomings in the design of Git.
In the hope it might be interesting, I post this summary here.
URL: http://neverworkintheory.org/2016/09/30/rethinking-git.html
The except from that resource written by Greg Wilson, the blog author:
---------------->8----------------
Santiago Perez De Rosso and Daniel Jackson: "[Purposes, Concepts,
Misfits, and a Redesign of Git]
(http://people.csail.mit.edu/sperezde/pre-print-oopsla16.pdf)", _SPLASH
2016_.
> Git is a widely used version control system that is powerful but
> complicated. Its complexity may not be an inevitable consequence of
> its power but rather evidence of flaws in its design. To explore this
> hypothesis, we analyzed the design of Git using a theory that
> identifies concepts, purposes, and misfits. Some well-known
> difficulties with Git are described, and explained as misfits in
> which underlying concepts fail to meet their intended purpose. Based
> on this analysis, we designed a reworking of Git (called Gitless)
> that attempts to remedy these flaws.
>
> To correlate misfits with issues reported by users, we conducted a
> study of Stack Overflow questions. And to determine whether users
> experienced fewer complications using Gitless in place of Git, we
> conducted a small user study. Results suggest our approach can be
> profitable in identifying, analyzing, and fixing design problems.
This paper presents a detailed, well-founded critique of one of the
most powerful, but frustrating, tools in widespread use today. A
follow-up to earlier work published in 2013, it is distinguished from
most other discussion of software design by three things:
1. It clearly describes its design paradigm, which comprises
_concepts_ (the major elements of the user's mental model of the
system), _purposes_ (which motivate the concepts), and _misfits_ (which
are instances where concepts do not satisfy purposes, or contradict one
another).
2. It lays out Git's concepts and purposes, analyzes its main
features in terms of them, and uses that analysis to identify
mis-matches.
3. Crucially, it then analyzes independent discussion of Git (on
Stack Overflow) to see if users are stumbling over the misfits
identified in step 2.
That would count as a major contribution on its own, but the authors go
further. They have designed a tool called Gitless that directly
addresses the shortcomings they have identified, and the penultimate
section of this paper presents a usability study that compares it to
standard Git. Overall, subjects found Gitles more satisfying and less
frustrating than Git, even though there was no big difference in
efficiency, difficulty, or confusion. Quoting the paper, "This apparent
contradiction might be due to the fact that all of the participants had
used Git before but were encountering Gitless for the first time
without any substantive training. Some participants (2 regular, 1
expert) commented that indeed their problems with Gitless were mostly
due to their lack of practice using it."
This paper is one of the best examples I have ever seen of how software
designs ought to be critiqued. It combines an explicit, coherent
conceptual base, detailed analysis of a specific system, design
grounded in that analysis, and an empirical check of that design.
Sadly, nothing shows the actual state of our profession more clearly
than the way this work has been greeted:
> In some respects, this project has been a fool's errand. We picked a
> product that was popular and widely used so as not to be investing
> effort in analyzing a strawman design; we thought that its popularity
> would mean that a larger audience would be interested in our
> experiment. In sharing our research with colleagues, however, we have
> discovered a significant polarization. Experts, who are deeply
> familiar with the product, have learned its many intricacies,
> developed complex, customized workflows, and regularly exploit its
> most elaborate features, are often defensive and resistant to the
> suggestion that the design has flaws. In contrast, less intensive
> users, who have given up on understanding the product, and rely on
> only a handful of memorized commands, are so frustrated by their
> experience that an analysis like ours seems to them belaboring the
> obvious.
---------------->8----------------
(This text is Copyright © Never Work in Theory, under the CC license.)
^ permalink raw reply
* Re: [PATCH v6 3/4] ls-files: pass through safe options for --recurse-submodules
From: Brandon Williams @ 2016-09-30 16:33 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, sbeller, peff
In-Reply-To: <xmqqy42ab5ww.fsf@gitster.mtv.corp.google.com>
On 09/29, Junio C Hamano wrote:
> Brandon Williams <bmwill@google.com> writes:
>
> > +static void compile_submodule_options(const struct dir_struct *dir, int show_tag)
> > +{
> > + if (line_terminator == '\0')
> > + argv_array_push(&submodules_options, "-z");
> > + if (show_tag)
> > + argv_array_push(&submodules_options, "-t");
> > + if (show_valid_bit)
> > + argv_array_push(&submodules_options, "-v");
> > + if (show_cached)
> > + argv_array_push(&submodules_options, "--cached");
> > + if (show_deleted)
> > + argv_array_push(&submodules_options, "--deleted");
> > + if (show_modified)
> > + argv_array_push(&submodules_options, "--modified");
> > + if (show_others)
> > + argv_array_push(&submodules_options, "--others");
> > + if (dir->flags & DIR_SHOW_IGNORED)
> > + argv_array_push(&submodules_options, "--ignored");
> > + if (show_stage)
> > + argv_array_push(&submodules_options, "--stage");
> > + if (show_killed)
> > + argv_array_push(&submodules_options, "--killed");
> > + if (dir->flags & DIR_SHOW_OTHER_DIRECTORIES)
> > + argv_array_push(&submodules_options, "--directory");
> > + if (!(dir->flags & DIR_SHOW_OTHER_DIRECTORIES))
> > + argv_array_push(&submodules_options, "--empty-directory");
> > + if (show_unmerged)
> > + argv_array_push(&submodules_options, "--unmerged");
> > + if (show_resolve_undo)
> > + argv_array_push(&submodules_options, "--resolve-undo");
> > + if (show_eol)
> > + argv_array_push(&submodules_options, "--eol");
> > + if (debug_mode)
> > + argv_array_push(&submodules_options, "--debug");
> > +}
>
> With this and 4/4 applied, the documentation still says "--cached"
> is the only supported option.
>
> Does it really make sense to pass all of these? I understand "-z"
> and I suspect things like "-t" and "-v" that affect "how" things are
> shown may also happen to work, but I am not sure how much it makes
> sense for options that affect "what" things are shown.
>
> What does it even mean to ask for say "--unmerged" to be shown, for
> example, from the superproject? Recurse into submodules whose cache
> entries in the index of the superproject are unmerged, or something
> else?
>
> I am inclined to say that it is probably better to keep the
> "--cached only" as documented, at least on the "what are shown"
> side.
>
> Thanks.
You're right that probably makes the most sense for now.
--
Brandon Williams
^ permalink raw reply
* Re: [PATCH v6 3/4] ls-files: pass through safe options for --recurse-submodules
From: Brandon Williams @ 2016-09-30 17:01 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, sbeller, peff
In-Reply-To: <20160930163347.GA11126@google.com>
Pass through some known-safe options when recursing into submodules.
(--cached, -v, -t, -z, --debug, --eol)
If other unsafe options are given the caller will be errored out.
Signed-off-by: Brandon Williams <bmwill@google.com>
---
Something more like this correct? I ditched the extra parameters and
reworded the commit msg to reflect this.
builtin/ls-files.c | 30 +++++++++++++++++++++++++++---
t/t3007-ls-files-recurse-submodules.sh | 16 ++++++++++++----
2 files changed, 39 insertions(+), 7 deletions(-)
diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index 63befed..b6144a5 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -30,6 +30,7 @@ static int line_terminator = '\n';
static int debug_mode;
static int show_eol;
static int recurse_submodules;
+static struct argv_array submodules_options = ARGV_ARRAY_INIT;
static const char *prefix;
static const char *super_prefix;
@@ -168,6 +169,25 @@ static void show_killed_files(struct dir_struct *dir)
}
}
+/*
+ * Compile an argv_array with all of the options supported by --recurse_submodules
+ */
+static void compile_submodule_options(const struct dir_struct *dir, int show_tag)
+{
+ if (line_terminator == '\0')
+ argv_array_push(&submodules_options, "-z");
+ if (show_tag)
+ argv_array_push(&submodules_options, "-t");
+ if (show_valid_bit)
+ argv_array_push(&submodules_options, "-v");
+ if (show_cached)
+ argv_array_push(&submodules_options, "--cached");
+ if (show_eol)
+ argv_array_push(&submodules_options, "--eol");
+ if (debug_mode)
+ argv_array_push(&submodules_options, "--debug");
+}
+
/**
* Recursively call ls-files on a submodule
*/
@@ -182,6 +202,9 @@ static void show_gitlink(const struct cache_entry *ce)
argv_array_push(&cp.args, "ls-files");
argv_array_push(&cp.args, "--recurse-submodules");
+ /* add supported options */
+ argv_array_pushv(&cp.args, submodules_options.argv);
+
cp.git_cmd = 1;
cp.dir = ce->name;
status = run_command(&cp);
@@ -567,11 +590,12 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)
if (require_work_tree && !is_inside_work_tree())
setup_work_tree();
+ if (recurse_submodules)
+ compile_submodule_options(&dir, show_tag);
+
if (recurse_submodules &&
(show_stage || show_deleted || show_others || show_unmerged ||
- show_killed || show_modified || show_resolve_undo ||
- show_valid_bit || show_tag || show_eol || with_tree ||
- (line_terminator == '\0')))
+ show_killed || show_modified || show_resolve_undo || with_tree))
die("ls-files --recurse-submodules unsupported mode");
if (recurse_submodules && error_unmatch)
diff --git a/t/t3007-ls-files-recurse-submodules.sh b/t/t3007-ls-files-recurse-submodules.sh
index b5a53c3..33a2ea7 100755
--- a/t/t3007-ls-files-recurse-submodules.sh
+++ b/t/t3007-ls-files-recurse-submodules.sh
@@ -34,6 +34,18 @@ test_expect_success 'ls-files correctly outputs files in submodule' '
test_cmp expect actual
'
+test_expect_success 'ls-files correctly outputs files in submodule with -z' '
+ lf_to_nul >expect <<-\EOF &&
+ .gitmodules
+ a
+ b/b
+ submodule/c
+ EOF
+
+ git ls-files --recurse-submodules -z >actual &&
+ test_cmp expect actual
+'
+
test_expect_success 'ls-files does not output files not added to a repo' '
cat >expect <<-\EOF &&
.gitmodules
@@ -86,15 +98,11 @@ test_incompatible_with_recurse_submodules () {
"
}
-test_incompatible_with_recurse_submodules -z
-test_incompatible_with_recurse_submodules -v
-test_incompatible_with_recurse_submodules -t
test_incompatible_with_recurse_submodules --deleted
test_incompatible_with_recurse_submodules --modified
test_incompatible_with_recurse_submodules --others
test_incompatible_with_recurse_submodules --stage
test_incompatible_with_recurse_submodules --killed
test_incompatible_with_recurse_submodules --unmerged
-test_incompatible_with_recurse_submodules --eol
test_done
--
2.10.0
^ permalink raw reply related
* Re: [PATCH v2 02/11] i18n: add--interactive: mark simple here documents for translation
From: Jakub Narębski @ 2016-09-30 17:26 UTC (permalink / raw)
To: Vasco Almeida, Junio C Hamano, git
Cc: Jiang Xin, Ævar Arnfjörð Bjarmason, David Aguilar
In-Reply-To: <1472646690-9699-3-git-send-email-vascomalmeida@sapo.pt>
W dniu 31.08.2016 o 14:31, Vasco Almeida pisze:
> Mark messages in here document without interpolation for translation.
>
> Marking for translation by removing here documents this way, rather than
> take advantage of "print __ << EOF" way, makes other instances of help
> messages in clean.c match the first two in this file. Otherwise,
> reusing here document would add a trailer newline to the message, making
> them not match 100%, hence creating two entries in pot template for
> translation rather than a single entry.
This is good catch, but I think it goes backwards with the solution.
If the text to be translated is multi-line, and it must end with newline,
why is this final newline not included in the msgid? This would involve
turning printf_ln into printf, and adding trailing newline in final
entry for builtin/clean.c:295, etc. - I think it is better solution than
uglyifing git-add--interactive.perl
Though it is not much of uglifying thanks to Perl support for embedded
newlines in double-quoted strings.
>
> Signed-off-by: Vasco Almeida <vascomalmeida@sapo.pt>
> ---
> git-add--interactive.perl | 24 ++++++++++++------------
> 1 file changed, 12 insertions(+), 12 deletions(-)
>
> diff --git a/git-add--interactive.perl b/git-add--interactive.perl
> index fb8e5de..e11a33d 100755
> --- a/git-add--interactive.perl
> +++ b/git-add--interactive.perl
> @@ -636,25 +636,25 @@ sub list_and_choose {
> }
>
> sub singleton_prompt_help_cmd {
> - print colored $help_color, <<\EOF ;
> -Prompt help:
> + print colored $help_color, __(
> +"Prompt help:
> 1 - select a numbered item
> foo - select item based on unique prefix
> - - (empty) select nothing
> -EOF
> + - (empty) select nothing"),
> +"\n";
> }
[... enough for information ...]
Regards,
--
Jakub Narębski
^ permalink raw reply
* Re: [PATCH v2 03/11] i18n: add--interactive: mark strings with interpolation for translation
From: Jakub Narębski @ 2016-09-30 17:52 UTC (permalink / raw)
To: Vasco Almeida, Junio C Hamano, git
Cc: Jiang Xin, Ævar Arnfjörð Bjarmason, David Aguilar
In-Reply-To: <1472646690-9699-4-git-send-email-vascomalmeida@sapo.pt>
W dniu 31.08.2016 o 14:31, Vasco Almeida pisze:
> Use of sprintf following die or error_msg is necessary for placeholder
> substitution take place.
No, it is not. Though I don't think that we have in out Git::I18N
the support for Perl i18n placeholder substitution.
From gettext manual:
https://www.gnu.org/software/gettext/manual/gettext.html#perl_002dformat
15.3.16 Perl Format Strings
There are two kinds format strings in Perl: those acceptable to the Perl
built-in function printf, labelled as ‘perl-format’, and those acceptable
to the libintl-perl function __x, labelled as ‘perl-brace-format’.
Perl printf format strings are described in the sprintf section of
‘man perlfunc’.
Perl brace format strings are described in the Locale::TextDomain(3pm)
manual page of the CPAN package libintl-perl. In brief, Perl format uses
placeholders put between braces (‘{’ and ‘}’). The placeholder must have
the syntax of simple identifiers.
Git doesn't use Locale::TextDomain, from what I understand, to provide
fallback in no-gettext case. Also, Locale::TextDomain is not in core.
The syntax, with the help of shorthand helper function, looks like this:
http://search.cpan.org/dist/libintl-perl/lib/Locale/TextDomain.pm#EXPORTED_FUNCTIONS
https://metacpan.org/pod/Locale::TextDomain#EXPORTED-FUNCTIONS
__x MSGID, ID1 => VAL1, ID2 => VAL2, ...
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
One of the nicest features in Perl is its capability to interpolate
variables into strings:
print "This is the $color $thing.\n";
This nice feature might con you into thinking that you could now write
print __"This is the $color $thing.\n";
[But this doesn't work...]
[...] The Perl backend to GNU gettext has defined an alternative format
[to using printf / sprintf] for interpolatable strings:
"This is the {color} {thing}.\n";
Instead of Perl variables you use place-holders (legal Perl variables
are also legal place-holders) in curly braces, and then you call
print __x ("This is the {color} {thing}.\n",
thing => $thang,
color => $color);
> Signed-off-by: Vasco Almeida <vascomalmeida@sapo.pt>
> ---
> git-add--interactive.perl | 26 ++++++++++++++------------
> 1 file changed, 14 insertions(+), 12 deletions(-)
>
> diff --git a/git-add--interactive.perl b/git-add--interactive.perl
> index e11a33d..4e1e857 100755
> --- a/git-add--interactive.perl
> +++ b/git-add--interactive.perl
> @@ -612,12 +612,12 @@ sub list_and_choose {
> else {
> $bottom = $top = find_unique($choice, @stuff);
> if (!defined $bottom) {
> - error_msg "Huh ($choice)?\n";
> + error_msg sprintf(__("Huh (%s)?\n"), $choice);
So this would be, self explained without need of comment
for translators:
+ error_msg __x ("Huh ({choice})?\n"), choice => $choice);
> next TOPLOOP;
> }
Though this is probably more work that you wanted to do.
The __x might be defined like this (borrowing from Locale::TextDomain),
which needs to be put into perl/Git/I18N.pm
sub __ ($);
sub __expand ($%);
# With interpolation.
sub __x ($@)
{
my ($msgid, %vars) = @_;
return __expand (__($msgid), %vars);
}
sub __expand ($%)
{
my ($translation, %args) = @_;
my $re = join '|', map { quotemeta $_ } keys %args;
$translation =~ s/\{($re)\}/defined $args{$1} ? $args{$1} : "{$1}"/ge;
return $translation;
}
Best regards,
--
Jakub Narębski
^ permalink raw reply
* Re: [PATCH 4/4] core.abbrev: raise the default abbreviation to 12 hexdigits
From: Linus Torvalds @ 2016-09-30 17:54 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, Johannes Sixt, Git Mailing List
In-Reply-To: <20160930080658.lyi7aovvazjmy346@sigill.intra.peff.net>
On Fri, Sep 30, 2016 at 1:06 AM, Jeff King <peff@peff.net> 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 function.
So the reason that d oesn't work is that the "disambiguate_state" data
where we keep the number of objects is only visible within
get_short_sha1().
So outside that function, you don't have any sane way to figure out
how many objects. So then you have to do the extra counting function..
> 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 possible, but I'm really not happy with how the
counting function looks. And nobody actually stood up to say "yeah,
that gets alternate loose objects right" or "if you have tons of those
alternate loose objects you have other issues anyway". I think
somebody would have to "own" that counting function, the advantage of
just putting it into disambiguate_state is that we just get the
counting for free..
Linus
^ permalink raw reply
* [PATCH] diff_unique_abbrev(): document its assumtion and limitation
From: Junio C Hamano @ 2016-09-30 17:54 UTC (permalink / raw)
To: git
This function is used to add "..." to displayed object names in
"diff --raw --abbrev[=<n>]" output. It bases its behaviour on an
untold assumption that the abbreviation length requested by the
caller is "reasonble", i.e. most of the objects will abbreviate
within the requested length and the resulting length would never
exceed it by more than a few hexdigits (otherwise the resulting
columns would not align). Explain that in a comment.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
* I had to scratch my head wondering what impact Linus's
auto-abbrev change will have on this code, which I wrote many
years ago in 47dd0d59 ("diff: --abbrev option", 2005-12-13).
diff.c | 23 ++++++++++++++++++++++-
1 file changed, 22 insertions(+), 1 deletion(-)
diff --git a/diff.c b/diff.c
index cefc13eb8e..428ed4f4c9 100644
--- a/diff.c
+++ b/diff.c
@@ -4108,7 +4108,8 @@ void diff_free_filepair(struct diff_filepair *p)
free(p);
}
-/* This is different from find_unique_abbrev() in that
+/*
+ * This is different from find_unique_abbrev() in that
* it stuffs the result with dots for alignment.
*/
const char *diff_unique_abbrev(const unsigned char *sha1, int len)
@@ -4120,6 +4121,26 @@ const char *diff_unique_abbrev(const unsigned char *sha1, int len)
abbrev = find_unique_abbrev(sha1, len);
abblen = strlen(abbrev);
+
+ /*
+ * In well-behaved cases, where the abbbreviated result is the
+ * same as the requested length, append three dots after the
+ * abbreviation (hence the whole logic is limited to the case
+ * where abblen < 37); when the actual abbreviated result is a
+ * bit longer than the requested length, we reduce the number
+ * of dots so that they match the well-behaved ones. However,
+ * if the actual abbreviation is longer than the requested
+ * length by more than three, we give up on aligning, and add
+ * three dots anyway, to indicate that the output is not the
+ * full object name. Yes, this may be suboptimal, but this
+ * appears only in "diff --raw --abbrev" output and it is not
+ * worth the effort to change it now. Note that this would
+ * likely to work fine when the automatic sizing of default
+ * abbreviation length is used--we would be fed -1 in "len" in
+ * that case, and will end up always appending three-dots, but
+ * the automatic sizing is supposed to give abblen that ensures
+ * uniqueness across all objects (statistically speaking).
+ */
if (abblen < 37) {
static char hex[41];
if (len < abblen && abblen <= len + 2)
--
2.10.0-612-g22341905f2
^ permalink raw reply related
* Re: [PATCH 4/4] core.abbrev: raise the default abbreviation to 12 hexdigits
From: Junio C Hamano @ 2016-09-30 17:56 UTC (permalink / raw)
To: Jeff King; +Cc: Linus Torvalds, Johannes Sixt, Git Mailing List
In-Reply-To: <20160930080658.lyi7aovvazjmy346@sigill.intra.peff.net>
Jeff King <peff@peff.net> 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 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.
Hmm. I am undecided; both approaches look OK to me.
^ permalink raw reply
* Re: [PATCH 4/4] core.abbrev: raise the default abbreviation to 12 hexdigits
From: Jeff King @ 2016-09-30 18:05 UTC (permalink / raw)
To: Linus Torvalds; +Cc: Junio C Hamano, Johannes Sixt, Git Mailing List
In-Reply-To: <CA+55aFxW1S6FNUh8YjSXkfC8=F5dka1rY-As6PWfG2rqmrsXXA@mail.gmail.com>
On Fri, Sep 30, 2016 at 10:54:16AM -0700, Linus Torvalds wrote:
> On Fri, Sep 30, 2016 at 1:06 AM, Jeff King <peff@peff.net> 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 function.
>
> So the reason that d oesn't work is that the "disambiguate_state" data
> where we keep the number of objects is only visible within
> get_short_sha1().
>
> So outside that function, you don't have any sane way to figure out
> how many objects. So then you have to do the extra counting function..
Right. I think you should do the extra counting function. It's a few
more lines, but the design is way less tangled.
> > 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 possible, but I'm really not happy with how the
> counting function looks. And nobody actually stood up to say "yeah,
> that gets alternate loose objects right" or "if you have tons of those
> alternate loose objects you have other issues anyway". I think
> somebody would have to "own" that counting function, the advantage of
> just putting it into disambiguate_state is that we just get the
> counting for free..
I don't think you _need_ get the alternate loose objects right. In fact,
I don't think you need to care about loose objects at all. For the
scales we're talking about, they're a rounding error. I would have done
it like this:
diff --git a/sha1_file.c b/sha1_file.c
index 65deaf9..1845502 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -1382,6 +1382,32 @@ static void prepare_packed_git_one(char *objdir, int local)
strbuf_release(&path);
}
+static int approximate_object_count_valid;
+
+/*
+ * Give a fast, rough count of the number of objects in the repository. This
+ * ignores loose objects completely. If you have a lot of them, then either
+ * you should repack because your performance will be awful, or they are
+ * all unreachable objects about to be pruned, in which case they're not really
+ * interesting as a measure of repo size in the first place.
+ */
+unsigned long approximate_object_count(void)
+{
+ static unsigned long count;
+ if (!approximate_object_count_valid) {
+ struct packed_git *p;
+
+ prepare_packed_git();
+ count = 0;
+ for (p = packed_git; p; p = p->next) {
+ if (open_pack_index(p))
+ continue;
+ count += p->num_objects;
+ }
+ }
+ return count;
+}
+
static void *get_next_packed_git(const void *p)
{
return ((const struct packed_git *)p)->next;
@@ -1456,6 +1482,7 @@ void prepare_packed_git(void)
void reprepare_packed_git(void)
{
+ approximate_object_count_valid = 0;
prepare_packed_git_run_once = 0;
prepare_packed_git();
}
^ permalink raw reply related
* Re: [PATCH] diff_unique_abbrev(): document its assumtion and limitation
From: Jeff King @ 2016-09-30 18:09 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqvaxd9ssy.fsf@gitster.mtv.corp.google.com>
On Fri, Sep 30, 2016 at 10:54:53AM -0700, Junio C Hamano wrote:
> This function is used to add "..." to displayed object names in
> "diff --raw --abbrev[=<n>]" output. It bases its behaviour on an
> untold assumption that the abbreviation length requested by the
> caller is "reasonble", i.e. most of the objects will abbreviate
> within the requested length and the resulting length would never
> exceed it by more than a few hexdigits (otherwise the resulting
> columns would not align). Explain that in a comment.
Heh, I have actually have a similar patch that renames it to
diff_aligned_abbrev(). Because I wanted to add another function:
static const char *diff_abbrev_oid(const struct object_id *oid,
int abbrev)
{
if (startup_info->have-repository)
return find_unique_abbrev(oid->hash, abbrev);
else {
char *hex = oid_to_hex(oid);
if (abbrev < 0) || abbrev > GIT_SHA1_HEXSZ)
die("BUG: oid abbreviation out of range: %d", abbrev);
hex[abbrev] = '\0';
return hex;
}
}
and I didn't want people to confuse the two. Now that function _would_
want to be updated as a result of the other conversation (it would need
to do something sensible with "-1", like turning it into "7", or
whatever else is deemed reasonable outside of a repository).
Anyway. I just wonder if you want to give it a better name while you are
at it.
-Peff
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox