threads / patch / 19605

v2add --abbrev to 'git cherry'

Subject: [PATCH v2] add --abbrev to 'git cherry'

## tl;dr

9 messages between May 30, 2009 and Jun 1, 2009. Diffs are folded; open one to read it.

replies: 8people: 4as markdown or json

Jeff Epler· May 30, 2009, 14:03 UTC · lore

Abbreviating ids makes 'git cherry -v' more useful, since you can see more
of the commit message summary:
    git cherry -v --abbrev | less -S

Signed-off-by: Jeff Epler <jepler@unpythonic.net>
---

An earlier version of this patch added multiple different flags to 'git cherry', but --abbrev (was -a) is really the important one. Thanks to Jakub Narebski and Michael J Gruber for comments on the first patch.

 Documentation/git-cherry.txt |    5 ++++-
 builtin-log.c                |   24 +++++++++++++++++++-----
 2 files changed, 23 insertions(+), 6 deletions(-)

Show changes to 2 files +23 −6

Documentation/git-cherry.txt, builtin-log.c

diff --git a/Documentation/git-cherry.txt b/Documentation/git-cherry.txt
index 7deefda..5c03da0 100644
--- a/Documentation/git-cherry.txt
+++ b/Documentation/git-cherry.txt
@@ -7,7 +7,7 @@ git-cherry - Find commits not merged upstream
 
 SYNOPSIS
 --------
-'git cherry' [-v] [<upstream> [<head> [<limit>]]]
+'git cherry' [-v] [--abbrev[=<n>]] [<upstream> [<head> [<limit>]]]
 
 DESCRIPTION
 -----------
@@ -49,6 +49,9 @@ OPTIONS
 -v::
 	Verbose.
 
+--abbrev[=<n>]::
+	Abbreviate commit ids to the given number of characters
+
 <upstream>::
 	Upstream branch to compare against.
 	Defaults to the first tracked remote branch, if available.
diff --git a/builtin-log.c b/builtin-log.c
index f10cfeb..1f3093e 100644
--- a/builtin-log.c
+++ b/builtin-log.c
@@ -1130,7 +1130,7 @@ static int add_pending_commit(const char *arg, struct rev_info *revs, int flags)
 }
 
 static const char cherry_usage[] =
-"git cherry [-v] [<upstream> [<head> [<limit>]]]";
+"git cherry [-v] [--abbrev[=<n>]] [<upstream> [<head> [<limit>]]]";
 int cmd_cherry(int argc, const char **argv, const char *prefix)
 {
 	struct rev_info revs;
@@ -1142,9 +1142,23 @@ int cmd_cherry(int argc, const char **argv, const char *prefix)
 	const char *head = "HEAD";
 	const char *limit = NULL;
 	int verbose = 0;
+	int abbrev = 40;
+
+	while(argc > 1 && argv[1][0] == '-') {
+		if (!strcmp(argv[1], "-v")) {
+			verbose = 1;
+		} else if(!strcmp(argv[1], "--abbrev")) {
+			abbrev = DEFAULT_ABBREV;
+		} else if(!prefixcmp(argv[1], "--abbrev=")) {
+			abbrev = strtol(argv[1] + 9, NULL, 10);
+			if(abbrev < MINIMUM_ABBREV)
+				abbrev = MINIMUM_ABBREV;
+			else if(abbrev > 40)
+				abbrev = 40;
+		} else {
+			die("unrecognized argument: %s", argv[1]);
+		}
 
-	if (argc > 1 && !strcmp(argv[1], "-v")) {
-		verbose = 1;
 		argc--;
 		argv++;
 	}
@@ -1218,12 +1232,12 @@ int cmd_cherry(int argc, const char **argv, const char *prefix)
 			struct strbuf buf = STRBUF_INIT;
 			pretty_print_commit(CMIT_FMT_ONELINE, commit,
 			                    &buf, 0, NULL, NULL, 0, 0);
-			printf("%c %s %s\n", sign,
+			printf("%c %.*s %s\n", sign, abbrev,
 			       sha1_to_hex(commit->object.sha1), buf.buf);
 			strbuf_release(&buf);
 		}
 		else {
-			printf("%c %s\n", sign,
+			printf("%c %.*s\n", sign, abbrev,
 			       sha1_to_hex(commit->object.sha1));
 		}

-- 
1.5.4.3

Markus Heidelberg· May 30, 2009, 16:26 UTC · re: Jeff Epler · lore

Re: [PATCH v2] add --abbrev to 'git cherry'

Jeff Epler, 30.05.2009:

>  Documentation/git-cherry.txt |    5 ++++-
>  builtin-log.c                |   24 +++++++++++++++++++-----
>  2 files changed, 23 insertions(+), 6 deletions(-)

You could also add --abbrev= to the bash completion.

Show 10 quoted lines

> diff --git a/Documentation/git-cherry.txt b/Documentation/git-cherry.txt
> index 7deefda..5c03da0 100644
> --- a/Documentation/git-cherry.txt
> +++ b/Documentation/git-cherry.txt
> @@ -49,6 +49,9 @@ OPTIONS
>  -v::
>  	Verbose.
>  
> +--abbrev[=<n>]::
> +	Abbreviate commit ids to the given number of characters

The full stop is missing :) And you could add "The default value is 7." as in the git-branch docs. Or even copy the whole description from there for consistency, it also mentions that this sets the minimum length, the displayed SHA1 may be longer, but more about this below.

Show 18 quoted lines

> diff --git a/builtin-log.c b/builtin-log.c
> index f10cfeb..1f3093e 100644
> --- a/builtin-log.c
> +++ b/builtin-log.c
> @@ -1218,12 +1232,12 @@ int cmd_cherry(int argc, const char **argv, const char *prefix)
>  			struct strbuf buf = STRBUF_INIT;
>  			pretty_print_commit(CMIT_FMT_ONELINE, commit,
>  			                    &buf, 0, NULL, NULL, 0, 0);
> -			printf("%c %s %s\n", sign,
> +			printf("%c %.*s %s\n", sign, abbrev,
>  			       sha1_to_hex(commit->object.sha1), buf.buf);
>  			strbuf_release(&buf);
>  		}
>  		else {
> -			printf("%c %s\n", sign,
> +			printf("%c %.*s\n", sign, abbrev,
>  			       sha1_to_hex(commit->object.sha1));
>  		}

There is no test for unique ids. "git cherry --abbrev=4" always prints 4 chars per SHA1, so "git show" on these SHA1s mostly gives "error: short SHA1 xxxx is ambiguous." in git.git.

find_unique_abbrev() will help.

Markus

Jeff Epler· May 30, 2009, 16:53 UTC · re: Markus Heidelberg · lore

[PATCH v3] add --abbrev to 'git cherry'

Abbreviating ids makes 'git cherry -v' more useful, since you can see more
of the commit message summary:
    git cherry -v --abbrev | less -S

Signed-off-by: Jeff Epler <jepler@unpythonic.net>
---

Compared to the last patch, this adds to the bash completion, improves doc consistency, and uses find_unique_abbrev to shorten commit ids. Thanks to Markus Heidelberg for feedback.

 Documentation/git-cherry.txt           |    6 +++++-
 builtin-log.c                          |   24 +++++++++++++++++++-----
 contrib/completion/git-completion.bash |   10 +++++++++-
 3 files changed, 33 insertions(+), 7 deletions(-)

Show changes to 3 files +33 −7

Documentation/git-cherry.txt, builtin-log.c, contrib/completion/git-completion.bash

diff --git a/Documentation/git-cherry.txt b/Documentation/git-cherry.txt
index 7deefda..c8cbbcc 100644
--- a/Documentation/git-cherry.txt
+++ b/Documentation/git-cherry.txt
@@ -7,7 +7,7 @@ git-cherry - Find commits not merged upstream
 
 SYNOPSIS
 --------
-'git cherry' [-v] [<upstream> [<head> [<limit>]]]
+'git cherry' [-v] [--abbrev[=<n>]] [<upstream> [<head> [<limit>]]]
 
 DESCRIPTION
 -----------
@@ -49,6 +49,10 @@ OPTIONS
 -v::
 	Verbose.
 
+--abbrev[=<n>]::
+	Alter the sha1's minimum display length in the output listing.
+	The default value is 7.
+
 <upstream>::
 	Upstream branch to compare against.
 	Defaults to the first tracked remote branch, if available.
diff --git a/builtin-log.c b/builtin-log.c
index f10cfeb..c115a8e 100644
--- a/builtin-log.c
+++ b/builtin-log.c
@@ -1130,7 +1130,7 @@ static int add_pending_commit(const char *arg, struct rev_info *revs, int flags)
 }
 
 static const char cherry_usage[] =
-"git cherry [-v] [<upstream> [<head> [<limit>]]]";
+"git cherry [-v] [--abbrev[=<n>]] [<upstream> [<head> [<limit>]]]";
 int cmd_cherry(int argc, const char **argv, const char *prefix)
 {
 	struct rev_info revs;
@@ -1142,9 +1142,23 @@ int cmd_cherry(int argc, const char **argv, const char *prefix)
 	const char *head = "HEAD";
 	const char *limit = NULL;
 	int verbose = 0;
+	int abbrev = 40;
+
+	while(argc > 1 && argv[1][0] == '-') {
+		if (!strcmp(argv[1], "-v")) {
+			verbose = 1;
+		} else if(!strcmp(argv[1], "--abbrev")) {
+			abbrev = DEFAULT_ABBREV;
+		} else if(!prefixcmp(argv[1], "--abbrev=")) {
+			abbrev = strtol(argv[1] + 9, NULL, 10);
+			if(abbrev < MINIMUM_ABBREV)
+				abbrev = MINIMUM_ABBREV;
+			else if(abbrev > 40)
+				abbrev = 40;
+		} else {
+			die("unrecognized argument: %s", argv[1]);
+		}
 
-	if (argc > 1 && !strcmp(argv[1], "-v")) {
-		verbose = 1;
 		argc--;
 		argv++;
 	}
@@ -1219,12 +1233,12 @@ int cmd_cherry(int argc, const char **argv, const char *prefix)
 			pretty_print_commit(CMIT_FMT_ONELINE, commit,
 			                    &buf, 0, NULL, NULL, 0, 0);
 			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;
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index c84d765..536a769 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -804,7 +804,15 @@ _git_checkout ()
 
 _git_cherry ()
 {
-	__gitcomp "$(__git_refs)"
+	local cur="${COMP_WORDS[COMP_CWORD]}"
+	case "$cur" in
+	-*)
+		__gitcomp "-v --abbrev --abbrev="
+		;;
+	*)
+		__gitcomp "$(__git_refs)"
+		;;
+	esac
 }
 
 _git_cherry_pick ()

-- 
1.5.4.3

Stephen Boyd· May 30, 2009, 21:13 UTC · re: Jeff Epler · lore

Re: [PATCH v3] add --abbrev to 'git cherry'

On Sat, May 30, 2009 at 9:53 AM, Jeff Epler <jepler@unpythonic.net> wrote:

Show 21 quoted lines

> @@ -1142,9 +1142,23 @@ int cmd_cherry(int argc, const char **argv, const char *prefix)
>        const char *head = "HEAD";
>        const char *limit = NULL;
>        int verbose = 0;
> +       int abbrev = 40;
> +
> +       while(argc > 1 && argv[1][0] == '-') {
> +               if (!strcmp(argv[1], "-v")) {
> +                       verbose = 1;
> +               } else if(!strcmp(argv[1], "--abbrev")) {
> +                       abbrev = DEFAULT_ABBREV;
> +               } else if(!prefixcmp(argv[1], "--abbrev=")) {
> +                       abbrev = strtol(argv[1] + 9, NULL, 10);
> +                       if(abbrev < MINIMUM_ABBREV)
> +                               abbrev = MINIMUM_ABBREV;
> +                       else if(abbrev > 40)
> +                               abbrev = 40;
> +               } else {
> +                       die("unrecognized argument: %s", argv[1]);
> +               }
>

You might want to look at using the parse options API. It has options for verbose and abbrev builtin, so you don't have to do any extra work. Plus you get a nice usage message for free. See Documentation/technical/api-parse-options.txt for more info.

Show 18 quoted lines

> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> index c84d765..536a769 100755
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -804,7 +804,15 @@ _git_checkout ()
>
>  _git_cherry ()
>  {
> -       __gitcomp "$(__git_refs)"
> +       local cur="${COMP_WORDS[COMP_CWORD]}"
> +       case "$cur" in
> +       -*)
> +               __gitcomp "-v --abbrev --abbrev="
> +               ;;
> +       *)
> +               __gitcomp "$(__git_refs)"
> +               ;;
> +       esac

Completion doesn't include short options (-v). This also means that --* is used instead of -*

Finally, you'll want to Cc Shawn (Shawn O. Pearce <spearce@spearce.org>) on bash completion.

Junio C Hamano· May 30, 2009, 23:08 UTC · re: Stephen Boyd · lore

Re: [PATCH v3] add --abbrev to 'git cherry'

Stephen Boyd <bebarino@gmail.com> writes:

> You might want to look at using the parse options API. It has options
> for verbose and abbrev builtin, so you don't have to do any extra
> work....

Why do people even think a change like this to a _plumbing_ command is desirable?

Admittedly, there already is "verbose" option that adds redundant information to the output of this particular plumbing, which might arguably be equally wrong as what this patch does, but I think it is excusable. At least it lets the Porcelain script that uses the command avoid calling 'git cat-file commit' to find out the title of the commit.

But --abbrev does not even add any information. If implemented correctly (which earlier iteration did not even do), it may not lose information by choping the output too short to make it ambiguous, but as others pointed out about using grep in the calling Porcelain to filter (or more likely, sift the lines into "+" and "-" bins) to shoot down -d/-D options, I do not see the point of adding --abbrev to this plumbing command very much.

Markus Heidelberg· May 30, 2009, 23:44 UTC · re: Junio C Hamano · lore

Re: [PATCH v3] add --abbrev to 'git cherry'

Junio C Hamano, 31.05.2009:

> Why do people even think a change like this to a _plumbing_ command is
> desirable?

git-cherry is plumbing? In git(1) it is listed as porcelain. And the plumbings show-ref, ls-files and ls-tree support --abbrev.

Markus

Junio C Hamano· May 31, 2009, 00:16 UTC · re: Markus Heidelberg · lore

Re: [PATCH v3] add --abbrev to 'git cherry'

Markus Heidelberg <markus.heidelberg@web.de> writes:

Show 5 quoted lines

> Junio C Hamano, 31.05.2009:
>> Why do people even think a change like this to a _plumbing_ command is
>> desirable?
>
> git-cherry is plumbing? In git(1) it is listed as porcelain.

I'd say it is a miscategorization, but I do not care too deeply, as I never use it myself (even though my Porcelain scripts would).

Stephen Boyd· May 31, 2009, 04:53 UTC · re: Junio C Hamano · lore

Re: [PATCH v3] add --abbrev to 'git cherry'

Junio C Hamano wrote:

Show 20 quoted lines

> Stephen Boyd <bebarino@gmail.com> writes:
>> You might want to look at using the parse options API. It has options
>> for verbose and abbrev builtin, so you don't have to do any extra
>> work....
>
> Why do people even think a change like this to a _plumbing_ command is
> desirable?
>
> Admittedly, there already is "verbose" option that adds redundant
> information to the output of this particular plumbing, which might
> arguably be equally wrong as what this patch does, but I think it is
> excusable.  At least it lets the Porcelain script that uses the command
> avoid calling 'git cat-file commit' to find out the title of the commit.
>
> But --abbrev does not even add any information.  If implemented correctly
> (which earlier iteration did not even do), it may not lose information by
> choping the output too short to make it ambiguous, but as others pointed
> out about using grep in the calling Porcelain to filter (or more likely,
> sift the lines into "+" and "-" bins) to shoot down -d/-D options, I do
> not see the point of adding --abbrev to this plumbing command very much.

I was tempted to say the same thing, but I decided to leave it up to the maintainer ;-) Maybe if there was a compelling use case it would make more sense?

Or, would it make more sense to just use git-log? Right now you can do git log --oneline --cherry-pick <head>..<upstream> and get close. Maybe we can add a "--cherry" option to git-log which will act like git-cherry by finding unmerged commits?

Jeff Epler· Jun 1, 2009, 11:54 UTC · lore

Re: [PATCH v3] add --abbrev to 'git cherry'

On Sun, May 31, 2009 at 12:51:23PM -0700, Junio C Hamano wrote:

> Stopping here would be a good idea if "log --left-right --cherry-pick A...B"
> (perhaps with a custom --pretty option) covers what you originally wanted
> to do.

Yes, I now see that 'git log' can pretty much do what I want. Having learned of 'git cherry', it didn't cross my mind that 'git log' was set up to do all 'git cherry' did and more.

> But if the reason why you wanted --abbrev was because you wanted to use it
> in your scripted Porcelain, and if the reason why you have your scripted
> Porcelain is because you wanted to add some _other_ information that "log"
> does not give you easily, perhaps it would be a good idea to share _that_.

No, this is all about displaying directly to the user (me) in a pager, specifically about getting as much of the change summary in the first 80 columns as possible.

Jeff