RFC patchcherry: support --abbrev option

> 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()
> +	};

> 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.

> 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.

> 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.
>

> 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.

> 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). :)
>

> 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.

> 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.
>