threads / patch / 23111

v2, 3 partscherry-abbrev

Subject: [PATCH v2 0/3] cherry-abbrev

## tl;dr

6 messages between Mar 20, 2010 and Mar 21, 2010. Diffs are folded; open one to read it.

replies: 5people: 2as markdown or json

Erik Faye-Lund· Mar 20, 2010, 18:55 UTC · lore
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(-)
Erik Faye-Lund· Mar 20, 2010, 18:55 UTC · re: Erik Faye-Lund · lore

[PATCH v2 1/3] cherry: support --abbrev option

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(-)
Show changes to builtin/log.c +21 −18
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
Erik Faye-Lund· Mar 20, 2010, 18:55 UTC · re: Erik Faye-Lund · lore

[PATCH v2 2/3] ls: remove redundant logic

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(-)
Show changes to 2 files +4 −9

builtin/ls-files.c, builtin/ls-tree.c

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
Erik Faye-Lund· Mar 20, 2010, 18:55 UTC · re: Erik Faye-Lund · lore

[PATCH v2 3/3] find_unique_abbrev: early out without a memcpy

Signed-off-by: Erik Faye-Lund <kusmabite@gmail.com>
---
 sha1_name.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)
Show changes to sha1_name.c +2 −2
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
Junio C Hamano· Mar 21, 2010, 20:23 UTC · re: Erik Faye-Lund · lore

Re: [PATCH v2 3/3] find_unique_abbrev: early out without a memcpy

Erik Faye-Lund <kusmabite@googlemail.com> writes:
Show 19 quoted lines
> 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.

Erik Faye-Lund· Mar 21, 2010, 21:09 UTC · re: Junio C Hamano · lore

Re: [PATCH v2 3/3] find_unique_abbrev: early out without a memcpy

On Sun, Mar 21, 2010 at 9:23 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 36 quoted lines
> 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

← back to recent threads