* [PATCH RFC] cherry: support --abbrev option
@ 2010-03-15 16:03 Erik Faye-Lund
2010-03-15 17:08 ` René Scharfe
2010-03-15 17:13 ` Junio C Hamano
0 siblings, 2 replies; 10+ messages in thread
From: Erik Faye-Lund @ 2010-03-15 16:03 UTC (permalink / raw)
To: git
Switch to parse-options API while we're at it.
Signed-off-by: Erik Faye-Lund <kusmabite@gmail.com>
---
I sometimes find it useful to look at the commit-subject together with
the SHA-1s. Using --abbrev increases the chance that the lines fits on
an 80 character wide terminal, making the output easier to read.
builtin/log.c | 40 ++++++++++++++++++++++------------------
1 files changed, 22 insertions(+), 18 deletions(-)
diff --git a/builtin/log.c b/builtin/log.c
index b70d0f7..020d618 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,26 @@ 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 = 40;
- 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,
+ PARSE_OPT_KEEP_UNKNOWN);
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 +1330,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 +1383,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.416.g84492
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH RFC] cherry: support --abbrev option
2010-03-15 16:03 [PATCH RFC] cherry: support --abbrev option Erik Faye-Lund
@ 2010-03-15 17:08 ` René Scharfe
2010-03-15 22:30 ` Erik Faye-Lund
2010-03-15 17:13 ` Junio C Hamano
1 sibling, 1 reply; 10+ messages in thread
From: René Scharfe @ 2010-03-15 17:08 UTC (permalink / raw)
To: Erik Faye-Lund; +Cc: git
Am 15.03.2010 17:03, schrieb Erik Faye-Lund:
> Switch to parse-options API while we're at it.
>
> Signed-off-by: Erik Faye-Lund <kusmabite@gmail.com>
> ---
>
> I sometimes find it useful to look at the commit-subject together with
> the SHA-1s. Using --abbrev increases the chance that the lines fits on
> an 80 character wide terminal, making the output easier to read.
>
> builtin/log.c | 40 ++++++++++++++++++++++------------------
> 1 files changed, 22 insertions(+), 18 deletions(-)
>
> diff --git a/builtin/log.c b/builtin/log.c
> index b70d0f7..020d618 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,26 @@ 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 = 40;
>
> - if (argc > 1 && !strcmp(argv[1], "-v")) {
> - verbose = 1;
> - argc--;
> - argv++;
> - }
> + struct option options[] = {
> + OPT__ABBREV(&abbrev),
> + OPT__VERBOSE(&verbose),
> + OPT_END()
> + };
If I use --no-abbrev, do I get 0 or 40 hash chars? I didn't actually
test it, but I suspect an "if (!abbrev) abbrev = 40;" is needed somewhere.
> - if (argc > 1 && !strcmp(argv[1], "-h"))
> - usage(cherry_usage);
> + argc = parse_options(argc, argv, prefix, options, cherry_usage,
> + PARSE_OPT_KEEP_UNKNOWN);
Why do you use PARSE_OPT_KEEP_UNKNOWN here? I think the old option
parsing code lazily relied on invalid options being found out by
add_pending_commit() later. We can be lazy again by leaving invalid
option handling to parse_options(); sure, it changes the error message,
but for the better.
René
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH RFC] cherry: support --abbrev option
2010-03-15 17:08 ` René Scharfe
@ 2010-03-15 22:30 ` Erik Faye-Lund
2010-03-15 23:16 ` René Scharfe
0 siblings, 1 reply; 10+ messages in thread
From: Erik Faye-Lund @ 2010-03-15 22:30 UTC (permalink / raw)
To: René Scharfe; +Cc: git
On Mon, Mar 15, 2010 at 6:08 PM, René Scharfe
<rene.scharfe@lsrfire.ath.cx> wrote:
> Am 15.03.2010 17:03, schrieb Erik Faye-Lund:
>> Switch to parse-options API while we're at it.
>>
>> Signed-off-by: Erik Faye-Lund <kusmabite@gmail.com>
>> ---
>>
>> I sometimes find it useful to look at the commit-subject together with
>> the SHA-1s. Using --abbrev increases the chance that the lines fits on
>> an 80 character wide terminal, making the output easier to read.
>>
>> builtin/log.c | 40 ++++++++++++++++++++++------------------
>> 1 files changed, 22 insertions(+), 18 deletions(-)
>>
>> diff --git a/builtin/log.c b/builtin/log.c
>> index b70d0f7..020d618 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,26 @@ 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 = 40;
>>
>> - if (argc > 1 && !strcmp(argv[1], "-v")) {
>> - verbose = 1;
>> - argc--;
>> - argv++;
>> - }
>> + struct option options[] = {
>> + OPT__ABBREV(&abbrev),
>> + OPT__VERBOSE(&verbose),
>> + OPT_END()
>> + };
>
> If I use --no-abbrev, do I get 0 or 40 hash chars? I didn't actually
> test it, but I suspect an "if (!abbrev) abbrev = 40;" is needed somewhere.
>
"abbrev" is initialized to 40 when declared, so you get the same
behavior as before by default.
>> - if (argc > 1 && !strcmp(argv[1], "-h"))
>> - usage(cherry_usage);
>> + argc = parse_options(argc, argv, prefix, options, cherry_usage,
>> + PARSE_OPT_KEEP_UNKNOWN);
>
> Why do you use PARSE_OPT_KEEP_UNKNOWN here? I think the old option
> parsing code lazily relied on invalid options being found out by
> add_pending_commit() later. We can be lazy again by leaving invalid
> option handling to parse_options(); sure, it changes the error message,
> but for the better.
Actually, It's a mistake, thanks for pointing it out :)
Yeah, I think your suggestion is better. Corrected.
--
Erik "kusma" Faye-Lund
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH RFC] cherry: support --abbrev option
2010-03-15 22:30 ` Erik Faye-Lund
@ 2010-03-15 23:16 ` René Scharfe
2010-03-15 23:50 ` Erik Faye-Lund
0 siblings, 1 reply; 10+ messages in thread
From: René Scharfe @ 2010-03-15 23:16 UTC (permalink / raw)
To: kusmabite; +Cc: Erik Faye-Lund, git
Am 15.03.2010 23:30, schrieb Erik Faye-Lund:
> On Mon, Mar 15, 2010 at 6:08 PM, René Scharfe
>> If I use --no-abbrev, do I get 0 or 40 hash chars? I didn't actually
>> test it, but I suspect an "if (!abbrev) abbrev = 40;" is needed somewhere.
>
> "abbrev" is initialized to 40 when declared, so you get the same
> behavior as before by default.
Yes, but --no-abbrev sets it to zero. Which is OK, though, as I found
out after actually testing your patch this time. A closer look at
find_unique_abbrev() in sha1_name.c reveals that the function returns
the full hash if len is either 40 or 0.
So you could initialize abbrev to zero and avoid the magic constant 40
there altogether. (Is this still nitpicking or already bikeshedding? ;)
René
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH RFC] cherry: support --abbrev option
2010-03-15 23:16 ` René Scharfe
@ 2010-03-15 23:50 ` Erik Faye-Lund
2010-03-16 0:46 ` René Scharfe
2010-03-16 1:44 ` Junio C Hamano
0 siblings, 2 replies; 10+ messages in thread
From: Erik Faye-Lund @ 2010-03-15 23:50 UTC (permalink / raw)
To: René Scharfe; +Cc: git
On Tue, Mar 16, 2010 at 12:16 AM, René Scharfe
<rene.scharfe@lsrfire.ath.cx> wrote:
> Am 15.03.2010 23:30, schrieb Erik Faye-Lund:
>> On Mon, Mar 15, 2010 at 6:08 PM, René Scharfe
>>> If I use --no-abbrev, do I get 0 or 40 hash chars? I didn't actually
>>> test it, but I suspect an "if (!abbrev) abbrev = 40;" is needed somewhere.
>>
>> "abbrev" is initialized to 40 when declared, so you get the same
>> behavior as before by default.
>
> Yes, but --no-abbrev sets it to zero. Which is OK, though, as I found
> out after actually testing your patch this time. A closer look at
> find_unique_abbrev() in sha1_name.c reveals that the function returns
> the full hash if len is either 40 or 0.
>
Ah yes, sorry. I misread your initial comment. That's what I get for
answering e-mail after coming home from a pub-quiz ;)
> So you could initialize abbrev to zero and avoid the magic constant 40
> there altogether. (Is this still nitpicking or already bikeshedding? ;)
>
It's still just nitpicking, and I appreciate the feed-back. I'm a
little bit hesitant here though, for the following reasons:
- All other users of OPT__ABBREV (with the exception of ls-files,
ls-tree and show-ref) initialize abbrev to it's default value (but
they all use DEFAULT_ABBREV).
- ls-files and ls-tree (but not show-ref) both does the following,
when using abbrev: "abbrev ? find_unique_abbrev(ce->sha1,abbrev) :
sha1_to_hex(ce->sha1)".
- ls-files, ls-tree and show-ref, all seems to default "abbrev" to
zero (by making it a static global).
- I want to be consistent with the existing code.
So, basically, ls-files and ls-tree seems to think
find_unique_abbrev() does not correctly (for this purpose) handle
abbrev=0. However, show-ref does seem to assume so. Looking at the
implementation of find_unique_abbrev(), it is clear that it does. But
as I said: I want to be consistent, and the variation from show-ref
(basically what you're suggesting) is the least common one.
So I guess I can either:
1) Change the code to be consistent with show-ref, and submit an
additional patch to make ls-files and ls-tree consistent with this.
This might have a performance-impact though, since
find_unique_abbrev() does some extra work (checking the sha1 for
existence and an extra buffer-copy).
2) Change the code as you suggest, and not care so much about consistency.
3) Leave the code to be functionally consistent with those who
initialize abbrev to DEFAULT_ABBREV (but with a different default,
which in itself is slightly inconsistent).
I'm leaning towards 3) for now, but I don't have any strong feelings.
--
Erik "kusma" Faye-Lund
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH RFC] cherry: support --abbrev option
2010-03-15 23:50 ` Erik Faye-Lund
@ 2010-03-16 0:46 ` René Scharfe
2010-03-16 0:59 ` Erik Faye-Lund
2010-03-16 1:44 ` Junio C Hamano
1 sibling, 1 reply; 10+ messages in thread
From: René Scharfe @ 2010-03-16 0:46 UTC (permalink / raw)
To: kusmabite; +Cc: Erik Faye-Lund, git
Am 16.03.2010 00:50, schrieb Erik Faye-Lund:
> It's still just nitpicking, and I appreciate the feed-back. I'm a
> little bit hesitant here though, for the following reasons:
> - All other users of OPT__ABBREV (with the exception of ls-files,
> ls-tree and show-ref) initialize abbrev to it's default value (but
> they all use DEFAULT_ABBREV).
> - ls-files and ls-tree (but not show-ref) both does the following,
> when using abbrev: "abbrev ? find_unique_abbrev(ce->sha1,abbrev) :
> sha1_to_hex(ce->sha1)".
> - ls-files, ls-tree and show-ref, all seems to default "abbrev" to
> zero (by making it a static global).
> - I want to be consistent with the existing code.
>
> So, basically, ls-files and ls-tree seems to think
> find_unique_abbrev() does not correctly (for this purpose) handle
> abbrev=0. However, show-ref does seem to assume so. Looking at the
> implementation of find_unique_abbrev(), it is clear that it does. But
> as I said: I want to be consistent, and the variation from show-ref
> (basically what you're suggesting) is the least common one.
>
> So I guess I can either:
> 1) Change the code to be consistent with show-ref, and submit an
> additional patch to make ls-files and ls-tree consistent with this.
> This might have a performance-impact though, since
> find_unique_abbrev() does some extra work (checking the sha1 for
> existence and an extra buffer-copy).
> 2) Change the code as you suggest, and not care so much about consistency.
> 3) Leave the code to be functionally consistent with those who
> initialize abbrev to DEFAULT_ABBREV (but with a different default,
> which in itself is slightly inconsistent).
>
> I'm leaning towards 3) for now, but I don't have any strong feelings.
If you do 2) then it stands 2:2 (ls-files, ls-tree vs. show-ref, cherry). :)
find_unique_abbrev() could be streamlined to degrade to sha1_to_hex()
early on if len is 0 or >= 40, without any existence check or copy..
René
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH RFC] cherry: support --abbrev option
2010-03-16 0:46 ` René Scharfe
@ 2010-03-16 0:59 ` Erik Faye-Lund
0 siblings, 0 replies; 10+ messages in thread
From: Erik Faye-Lund @ 2010-03-16 0:59 UTC (permalink / raw)
To: René Scharfe; +Cc: git
On Tue, Mar 16, 2010 at 1:46 AM, René Scharfe
<rene.scharfe@lsrfire.ath.cx> wrote:
> Am 16.03.2010 00:50, schrieb Erik Faye-Lund:
>> It's still just nitpicking, and I appreciate the feed-back. I'm a
>> little bit hesitant here though, for the following reasons:
>> - All other users of OPT__ABBREV (with the exception of ls-files,
>> ls-tree and show-ref) initialize abbrev to it's default value (but
>> they all use DEFAULT_ABBREV).
>> - ls-files and ls-tree (but not show-ref) both does the following,
>> when using abbrev: "abbrev ? find_unique_abbrev(ce->sha1,abbrev) :
>> sha1_to_hex(ce->sha1)".
>> - ls-files, ls-tree and show-ref, all seems to default "abbrev" to
>> zero (by making it a static global).
>> - I want to be consistent with the existing code.
>>
>> So, basically, ls-files and ls-tree seems to think
>> find_unique_abbrev() does not correctly (for this purpose) handle
>> abbrev=0. However, show-ref does seem to assume so. Looking at the
>> implementation of find_unique_abbrev(), it is clear that it does. But
>> as I said: I want to be consistent, and the variation from show-ref
>> (basically what you're suggesting) is the least common one.
>>
>> So I guess I can either:
>> 1) Change the code to be consistent with show-ref, and submit an
>> additional patch to make ls-files and ls-tree consistent with this.
>> This might have a performance-impact though, since
>> find_unique_abbrev() does some extra work (checking the sha1 for
>> existence and an extra buffer-copy).
>> 2) Change the code as you suggest, and not care so much about consistency.
>> 3) Leave the code to be functionally consistent with those who
>> initialize abbrev to DEFAULT_ABBREV (but with a different default,
>> which in itself is slightly inconsistent).
>>
>> I'm leaning towards 3) for now, but I don't have any strong feelings.
>
> If you do 2) then it stands 2:2 (ls-files, ls-tree vs. show-ref, cherry). :)
>
True
> find_unique_abbrev() could be streamlined to degrade to sha1_to_hex()
> early on if len is 0 or >= 40, without any existence check or copy..
>
True indeed, and as an added bonus, we could get rid of duplicate
logic. I don't think the function-call overhead is big enough to care
about for ls-files and ls-tree.
I might go for 2), with the intention of sending some follow-up
patches addressing this...
Luckily, now is sleepy sleepy time, and I can consider this tomorrow ;)
--
Erik "kusma" Faye-Lund
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH RFC] cherry: support --abbrev option
2010-03-15 23:50 ` Erik Faye-Lund
2010-03-16 0:46 ` René Scharfe
@ 2010-03-16 1:44 ` Junio C Hamano
1 sibling, 0 replies; 10+ messages in thread
From: Junio C Hamano @ 2010-03-16 1:44 UTC (permalink / raw)
To: kusmabite; +Cc: René Scharfe, git
Erik Faye-Lund <kusmabite@googlemail.com> writes:
> So, basically, ls-files and ls-tree seems to think
> find_unique_abbrev() does not correctly (for this purpose) handle
> abbrev=0. However, show-ref does seem to assume so.
I think this is merely historical; the former two were written way back
before corner case behaviour, namely length=0 case, was cast in stone, and
hence they call f-u-a unnecessarily defefnsively.
.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH RFC] cherry: support --abbrev option
2010-03-15 16:03 [PATCH RFC] cherry: support --abbrev option Erik Faye-Lund
2010-03-15 17:08 ` René Scharfe
@ 2010-03-15 17:13 ` Junio C Hamano
2010-03-15 22:39 ` Erik Faye-Lund
1 sibling, 1 reply; 10+ messages in thread
From: Junio C Hamano @ 2010-03-15 17:13 UTC (permalink / raw)
To: Erik Faye-Lund; +Cc: git
Erik Faye-Lund <kusmabite@googlemail.com> writes:
> Switch to parse-options API while we're at it.
>
> Signed-off-by: Erik Faye-Lund <kusmabite@gmail.com>
> ---
>
> I sometimes find it useful to look at the commit-subject together with
> the SHA-1s. Using --abbrev increases the chance that the lines fits on
> an 80 character wide terminal, making the output easier to read.
Even though "cherry" historically was meant to be a low-level helper for
Porcelain scripts, I've seen people use it as the top-level UI, and I
think what the patch tries to do makes sense.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH RFC] cherry: support --abbrev option
2010-03-15 17:13 ` Junio C Hamano
@ 2010-03-15 22:39 ` Erik Faye-Lund
0 siblings, 0 replies; 10+ messages in thread
From: Erik Faye-Lund @ 2010-03-15 22:39 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
On Mon, Mar 15, 2010 at 6:13 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Erik Faye-Lund <kusmabite@googlemail.com> writes:
>
>> Switch to parse-options API while we're at it.
>>
>> Signed-off-by: Erik Faye-Lund <kusmabite@gmail.com>
>> ---
>>
>> I sometimes find it useful to look at the commit-subject together with
>> the SHA-1s. Using --abbrev increases the chance that the lines fits on
>> an 80 character wide terminal, making the output easier to read.
>
> Even though "cherry" historically was meant to be a low-level helper for
> Porcelain scripts, I've seen people use it as the top-level UI, and I
> think what the patch tries to do makes sense.
>
I find it very useful. In fact, even the documentation seems to
suggest to use it to find out if a patch is merged upstream, which
doesn't sound very scriptish to me. So I think it has somehow exceeded
it's original goal :)
--
Erik "kusma" Faye-Lund
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2010-03-16 1:44 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2010-03-15 16:03 [PATCH RFC] cherry: support --abbrev option Erik Faye-Lund
2010-03-15 17:08 ` René Scharfe
2010-03-15 22:30 ` Erik Faye-Lund
2010-03-15 23:16 ` René Scharfe
2010-03-15 23:50 ` Erik Faye-Lund
2010-03-16 0:46 ` René Scharfe
2010-03-16 0:59 ` Erik Faye-Lund
2010-03-16 1:44 ` Junio C Hamano
2010-03-15 17:13 ` Junio C Hamano
2010-03-15 22:39 ` Erik Faye-Lund
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).