* [PATCH v2 0/3] cherry-abbrev
@ 2010-03-20 18:55 Erik Faye-Lund
2010-03-20 18:55 ` [PATCH v2 1/3] cherry: support --abbrev option Erik Faye-Lund
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Erik Faye-Lund @ 2010-03-20 18:55 UTC (permalink / raw)
To: git; +Cc: rene.scharfe, gitster
This is the second round of ef/cherry-abbrev.
1/3 is the same as last round, but with the corrections suggested
by René Scharfe.
2/3 isn't strictly needed, but it makes two call-sites for f_u_a
more consistent with the others.
3/3 fixes the gains back the performance that was lost in 2/3:
Best of five runs of "git ls-tree v1.6.5" in a loop:
without 3/3:
real 0m0.082s
user 0m0.040s
sys 0m0.036s
with 3/3:
real 0m0.074s
user 0m0.040s
sys 0m0.040s
Erik Faye-Lund (3):
cherry: support --abbrev option
ls: remove redundant logic
find_unique_abbrev: early out without a memcpy
builtin/log.c | 39 +++++++++++++++++++++------------------
builtin/ls-files.c | 7 ++-----
builtin/ls-tree.c | 6 ++----
sha1_name.c | 4 ++--
4 files changed, 27 insertions(+), 29 deletions(-)
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v2 1/3] cherry: support --abbrev option
2010-03-20 18:55 [PATCH v2 0/3] cherry-abbrev Erik Faye-Lund
@ 2010-03-20 18:55 ` Erik Faye-Lund
2010-03-20 18:55 ` [PATCH v2 2/3] ls: remove redundant logic Erik Faye-Lund
2010-03-20 18:55 ` [PATCH v2 3/3] find_unique_abbrev: early out without a memcpy Erik Faye-Lund
2 siblings, 0 replies; 6+ messages in thread
From: Erik Faye-Lund @ 2010-03-20 18:55 UTC (permalink / raw)
To: git; +Cc: rene.scharfe, gitster
Switch to parse-options API while we're at it.
Signed-off-by: Erik Faye-Lund <kusmabite@gmail.com>
---
builtin/log.c | 39 +++++++++++++++++++++------------------
1 files changed, 21 insertions(+), 18 deletions(-)
diff --git a/builtin/log.c b/builtin/log.c
index b70d0f7..fd8d18c 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1286,8 +1286,11 @@ static int add_pending_commit(const char *arg, struct rev_info *revs, int flags)
return -1;
}
-static const char cherry_usage[] =
-"git cherry [-v] [<upstream> [<head> [<limit>]]]";
+static const char * const cherry_usage[] = {
+ "git cherry [-v] [<upstream> [<head> [<limit>]]]",
+ NULL
+};
+
int cmd_cherry(int argc, const char **argv, const char *prefix)
{
struct rev_info revs;
@@ -1298,26 +1301,25 @@ int cmd_cherry(int argc, const char **argv, const char *prefix)
const char *upstream;
const char *head = "HEAD";
const char *limit = NULL;
- int verbose = 0;
+ int verbose = 0, abbrev = 0;
- if (argc > 1 && !strcmp(argv[1], "-v")) {
- verbose = 1;
- argc--;
- argv++;
- }
+ struct option options[] = {
+ OPT__ABBREV(&abbrev),
+ OPT__VERBOSE(&verbose),
+ OPT_END()
+ };
- if (argc > 1 && !strcmp(argv[1], "-h"))
- usage(cherry_usage);
+ argc = parse_options(argc, argv, prefix, options, cherry_usage, 0);
switch (argc) {
- case 4:
- limit = argv[3];
- /* FALLTHROUGH */
case 3:
- head = argv[2];
+ limit = argv[2];
/* FALLTHROUGH */
case 2:
- upstream = argv[1];
+ head = argv[1];
+ /* FALLTHROUGH */
+ case 1:
+ upstream = argv[0];
break;
default:
current_branch = branch_get(NULL);
@@ -1327,7 +1329,7 @@ int cmd_cherry(int argc, const char **argv, const char *prefix)
fprintf(stderr, "Could not find a tracked"
" remote branch, please"
" specify <upstream> manually.\n");
- usage(cherry_usage);
+ usage_with_options(cherry_usage, options);
}
upstream = current_branch->merge[0]->dst;
@@ -1380,12 +1382,13 @@ int cmd_cherry(int argc, const char **argv, const char *prefix)
pretty_print_commit(CMIT_FMT_ONELINE, commit,
&buf, &ctx);
printf("%c %s %s\n", sign,
- sha1_to_hex(commit->object.sha1), buf.buf);
+ find_unique_abbrev(commit->object.sha1, abbrev),
+ buf.buf);
strbuf_release(&buf);
}
else {
printf("%c %s\n", sign,
- sha1_to_hex(commit->object.sha1));
+ find_unique_abbrev(commit->object.sha1, abbrev));
}
list = list->next;
--
1.7.0.2.456.g64f24
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH v2 2/3] ls: remove redundant logic
2010-03-20 18:55 [PATCH v2 0/3] cherry-abbrev Erik Faye-Lund
2010-03-20 18:55 ` [PATCH v2 1/3] cherry: support --abbrev option Erik Faye-Lund
@ 2010-03-20 18:55 ` Erik Faye-Lund
2010-03-20 18:55 ` [PATCH v2 3/3] find_unique_abbrev: early out without a memcpy Erik Faye-Lund
2 siblings, 0 replies; 6+ messages in thread
From: Erik Faye-Lund @ 2010-03-20 18:55 UTC (permalink / raw)
To: git; +Cc: rene.scharfe, gitster
find_unique_abbrev() already returns the full SHA-1 if abbrev = 0,
so we can remove the logic that avoids the call.
Signed-off-by: Erik Faye-Lund <kusmabite@gmail.com>
---
builtin/ls-files.c | 7 ++-----
builtin/ls-tree.c | 6 ++----
2 files changed, 4 insertions(+), 9 deletions(-)
diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index b065061..c0fbcdc 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -153,8 +153,7 @@ static void show_ce_entry(const char *tag, struct cache_entry *ce)
printf("%s%06o %s %d\t",
tag,
ce->ce_mode,
- abbrev ? find_unique_abbrev(ce->sha1,abbrev)
- : sha1_to_hex(ce->sha1),
+ find_unique_abbrev(ce->sha1,abbrev),
ce_stage(ce));
}
write_name_quoted(ce->name + offset, stdout, line_terminator);
@@ -176,9 +175,7 @@ static int show_one_ru(struct string_list_item *item, void *cbdata)
if (!ui->mode[i])
continue;
printf("%s%06o %s %d\t", tag_resolve_undo, ui->mode[i],
- abbrev
- ? find_unique_abbrev(ui->sha1[i], abbrev)
- : sha1_to_hex(ui->sha1[i]),
+ find_unique_abbrev(ui->sha1[i], abbrev),
i + 1);
write_name_quoted(path + offset, stdout, line_terminator);
}
diff --git a/builtin/ls-tree.c b/builtin/ls-tree.c
index 4484185..dc86b0d 100644
--- a/builtin/ls-tree.c
+++ b/builtin/ls-tree.c
@@ -103,13 +103,11 @@ static int show_tree(const unsigned char *sha1, const char *base, int baselen,
} else
strcpy(size_text, "-");
printf("%06o %s %s %7s\t", mode, type,
- abbrev ? find_unique_abbrev(sha1, abbrev)
- : sha1_to_hex(sha1),
+ find_unique_abbrev(sha1, abbrev),
size_text);
} else
printf("%06o %s %s\t", mode, type,
- abbrev ? find_unique_abbrev(sha1, abbrev)
- : sha1_to_hex(sha1));
+ find_unique_abbrev(sha1, abbrev));
}
write_name_quotedpfx(base + chomp_prefix, baselen - chomp_prefix,
pathname, stdout, line_termination);
--
1.7.0.2.456.g64f24
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH v2 3/3] find_unique_abbrev: early out without a memcpy
2010-03-20 18:55 [PATCH v2 0/3] cherry-abbrev Erik Faye-Lund
2010-03-20 18:55 ` [PATCH v2 1/3] cherry: support --abbrev option Erik Faye-Lund
2010-03-20 18:55 ` [PATCH v2 2/3] ls: remove redundant logic Erik Faye-Lund
@ 2010-03-20 18:55 ` Erik Faye-Lund
2010-03-21 20:23 ` Junio C Hamano
2 siblings, 1 reply; 6+ messages in thread
From: Erik Faye-Lund @ 2010-03-20 18:55 UTC (permalink / raw)
To: git; +Cc: rene.scharfe, gitster
Signed-off-by: Erik Faye-Lund <kusmabite@gmail.com>
---
sha1_name.c | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/sha1_name.c b/sha1_name.c
index bf92417..2b1be58 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -196,10 +196,10 @@ const char *find_unique_abbrev(const unsigned char *sha1, int len)
int status, exists;
static char hex[41];
+ if (len == 40 || !len)
+ return sha1_to_hex(sha1);
exists = has_sha1_file(sha1);
memcpy(hex, sha1_to_hex(sha1), 40);
- if (len == 40 || !len)
- return hex;
while (len < 40) {
unsigned char sha1_ret[20];
status = get_short_sha1(hex, len, sha1_ret, 1);
--
1.7.0.2.456.g64f24
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2 3/3] find_unique_abbrev: early out without a memcpy
2010-03-20 18:55 ` [PATCH v2 3/3] find_unique_abbrev: early out without a memcpy Erik Faye-Lund
@ 2010-03-21 20:23 ` Junio C Hamano
2010-03-21 21:09 ` Erik Faye-Lund
0 siblings, 1 reply; 6+ messages in thread
From: Junio C Hamano @ 2010-03-21 20:23 UTC (permalink / raw)
To: Erik Faye-Lund; +Cc: git, rene.scharfe
Erik Faye-Lund <kusmabite@googlemail.com> writes:
> Signed-off-by: Erik Faye-Lund <kusmabite@gmail.com>
> ---
> sha1_name.c | 4 ++--
> 1 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/sha1_name.c b/sha1_name.c
> index bf92417..2b1be58 100644
> --- a/sha1_name.c
> +++ b/sha1_name.c
> @@ -196,10 +196,10 @@ const char *find_unique_abbrev(const unsigned char *sha1, int len)
> int status, exists;
> static char hex[41];
>
> + if (len == 40 || !len)
> + return sha1_to_hex(sha1);
> exists = has_sha1_file(sha1);
> memcpy(hex, sha1_to_hex(sha1), 40);
> - if (len == 40 || !len)
> - return hex;
This is somewhat iffy. hex[] being static means there can only be one
outstanding return value from f-u-a being used, iow
printf("%s %s", f-u-a(a, 0), f-u-a(b, 0))
is a no-no. But at the same time, it means that you can use one more
recycled buffer than sha1_to_hex() gives us, so this may be safe:
char *ua = f-u-a(a, 0);
printf("%s %s %s %s %s", ua,
sha1_to_hex(b), sha1_to_hex(c), sha1_to_hex(d), sha1_to_hex(e));
but with the above it probably is not anymore, no?
As an optimization patch, I would buy that delaying the "exists" check
until the "no abbreviation" check returned early would make sense, though.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 3/3] find_unique_abbrev: early out without a memcpy
2010-03-21 20:23 ` Junio C Hamano
@ 2010-03-21 21:09 ` Erik Faye-Lund
0 siblings, 0 replies; 6+ messages in thread
From: Erik Faye-Lund @ 2010-03-21 21:09 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, rene.scharfe
On Sun, Mar 21, 2010 at 9:23 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Erik Faye-Lund <kusmabite@googlemail.com> writes:
>
>> Signed-off-by: Erik Faye-Lund <kusmabite@gmail.com>
>> ---
>> sha1_name.c | 4 ++--
>> 1 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/sha1_name.c b/sha1_name.c
>> index bf92417..2b1be58 100644
>> --- a/sha1_name.c
>> +++ b/sha1_name.c
>> @@ -196,10 +196,10 @@ const char *find_unique_abbrev(const unsigned char *sha1, int len)
>> int status, exists;
>> static char hex[41];
>>
>> + if (len == 40 || !len)
>> + return sha1_to_hex(sha1);
>> exists = has_sha1_file(sha1);
>> memcpy(hex, sha1_to_hex(sha1), 40);
>> - if (len == 40 || !len)
>> - return hex;
>
> This is somewhat iffy. hex[] being static means there can only be one
> outstanding return value from f-u-a being used, iow
>
> printf("%s %s", f-u-a(a, 0), f-u-a(b, 0))
>
> is a no-no. But at the same time, it means that you can use one more
> recycled buffer than sha1_to_hex() gives us, so this may be safe:
>
> char *ua = f-u-a(a, 0);
> printf("%s %s %s %s %s", ua,
> sha1_to_hex(b), sha1_to_hex(c), sha1_to_hex(d), sha1_to_hex(e));
>
> but with the above it probably is not anymore, no?
>
True, I didn't think of that.
But is this really a problem? I mean, we already have 3 guard-buffers
here, so it would only trigger when the result of more than four calls
to sha1_to_hex (directly, or through find_unique_abbrev) has to be
kept alive at the same time.
The only documentation I could find of this was the comment "/* static
buffer result! */" from cache.h, and it doesn't mention that there's
multiple buffers. So I'd say that any call-site that required four
buffers are, well, perhaps a little bit TOO tightly integrated with
that function ;)
The test-suite didn't catch any failures in it, but I guess I would
have to analyze the calling code a bit more to be sure.
> As an optimization patch, I would buy that delaying the "exists" check
> until the "no abbreviation" check returned early would make sense, though.
>
...or we could just do this. To be honest, this is probably where most
of the change in performance come from. Which in case makes the commit
message somewhat misleading.
Then again, the third option is the easiest one; just drop 2/3 and
3/3. I'd be perfectly fine with that, I just went a little OCD with
consistency here. Those two patches gain us very little. I haven't
measured performance change outside of ls-tree, though.
--
Erik "kusma" Faye-Lund
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2010-03-21 21:10 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-03-20 18:55 [PATCH v2 0/3] cherry-abbrev Erik Faye-Lund
2010-03-20 18:55 ` [PATCH v2 1/3] cherry: support --abbrev option Erik Faye-Lund
2010-03-20 18:55 ` [PATCH v2 2/3] ls: remove redundant logic Erik Faye-Lund
2010-03-20 18:55 ` [PATCH v2 3/3] find_unique_abbrev: early out without a memcpy Erik Faye-Lund
2010-03-21 20:23 ` Junio C Hamano
2010-03-21 21:09 ` Erik Faye-Lund
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.