patch, 3 partspretty.c: rename describe options variable to more descriptive name

> It contains option arguments, not options. We would like to add option
> support here too.
>
> Signed-off-by: Eli Schwartz <eschwartz@archlinux.org>
> ---
>  pretty.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/pretty.c b/pretty.c
> index 73b5ead509..9db2c65538 100644
> --- a/pretty.c
> +++ b/pretty.c
> @@ -1216,7 +1216,7 @@ int format_set_trailers_options(struct process_trailer_options *opts,
>  
>  static size_t parse_describe_args(const char *start, struct strvec *args)
>  {
> -	const char *options[] = { "match", "exclude" };
> +	const char *option_arguments[] = { "match", "exclude" };

>  	const char *arg = start;
>  
>  	for (;;) {
> @@ -1225,10 +1225,10 @@ static size_t parse_describe_args(const char *start, struct strvec *args)
>  		size_t arglen = 0;
>  		int i;
>  
> -		for (i = 0; i < ARRAY_SIZE(options); i++) {
> -			if (match_placeholder_arg_value(arg, options[i], &arg,
> +		for (i = 0; i < ARRAY_SIZE(option_arguments); i++) {
> +			if (match_placeholder_arg_value(arg, option_arguments[i], &arg,
>  							&argval, &arglen)) {
> -				matched = options[i];
> +				matched = option_arguments[i];
>  				break;
>  			}
>  		}

> Eli Schwartz <eschwartz@archlinux.org> writes:
> 
>> It contains option arguments, not options. We would like to add option
>> support here too.
>>
>> Signed-off-by: Eli Schwartz <eschwartz@archlinux.org>
>> ---
>>  pretty.c | 8 ++++----
>>  1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/pretty.c b/pretty.c
>> index 73b5ead509..9db2c65538 100644
>> --- a/pretty.c
>> +++ b/pretty.c
>> @@ -1216,7 +1216,7 @@ int format_set_trailers_options(struct process_trailer_options *opts,
>>  
>>  static size_t parse_describe_args(const char *start, struct strvec *args)
>>  {
>> -	const char *options[] = { "match", "exclude" };
>> +	const char *option_arguments[] = { "match", "exclude" };
> 
> This renaming is more or less "Meh" without the other change in the
> series that may (or may not) be helped with this step, but because I
> haven't seen these later steps yet, I may sound too dismissive of
> the change in this step.
> 
> Anyway, at least call that option_args[] to match the way the
> function calls itself, not option_arguments[] that is a mouthful for
> a mere implementation detail of local variable for a file-private
> helper function.

> changing its length because the repository grew in size. This could
> cause the output of git-archive to change.
>
> Add the --abbrev option to `git describe` to the placeholder interface
> in order to provide tools to the user for fine-tuning project defaults
> and ensure reproducible archives.

> Eli Schwartz <eschwartz@archlinux.org> writes:
> 
>> The %(describe) placeholder by default, like `git describe`, uses a
>> seven-character abbreviated commit hash. This may not be sufficient to
> 
> "hash" -> "object name".
> 
>> fully describe all git repos, resulting in a placeholder replacement
> 
> "all git repos" -> "all commits in a given repository" (there may be
> other valid way to clarify, but the point is that 'describe' does
> not describe 'git repos' in the sense that my repository gets
> description X while your repository gets description Y).

>> changing its length because the repository grew in size. This could
>> cause the output of git-archive to change.
>>
>> Add the --abbrev option to `git describe` to the placeholder interface
>> in order to provide tools to the user for fine-tuning project defaults
>> and ensure reproducible archives.
> 
> Note that it is sad that --abbrev=<n> does not necessarily ensure
> reproducibility.  To be more precise, I do not think it sacrifices
> uniqueness to make the output reproducible.  You can get more than N
> hex-digits in the output if N is too small to ensure uniquness.
> 
> So it indeed is that this line of thought ...
> 
>> One alternative would be to just always specify --abbrev=40 but this may
>> be a bit too biased...
> 
> ... to use --abbrev=999 (because 40 is not the length of a full
> object name in the SHA-2 world) is the only reasonable way, if what
> you care about is the reproducibility.

>     Side note.  I think "git describe --no-abbrev" is buggy in that
>     it does not give a full object name; I didn't check the code,
>     but it appears to be behaving the same way as "git describe
>     --abbrev=0" (show no hexdigits).  Fixing this bug may possibly
>     be a low-hanging fruit.

> But even if the feature cannot be used to guarantee a full
> reproducibility, it is a good thing that we can now add this feature
> with minimum effort thanks to the previous two steps.
> 
> The refactoring I suggested in my review for the previous step will
> shine, if we want to do a good job parsing the --abbrev=<n> option,
> since such a code organization would make it a fairly easy addition
> to introduce "integer" type that calls match_placeholder_arg_value()
> to read the option value (like "string" does) and validate that the
> value is indeed an integer.
> 
> Would we want to support "--contains" as another boolean type?  How
> about "--all" and "--long"?  All three sound plausible candidates.

> The %(describe) placeholder by default, like `git describe`, only
> supports annotated tags. However, some people do use lightweight tags
> for releases, and would like to describe those anyway. The command line
> tool has an option to support this.
>
> Teach the placeholder to support this as well.
>
> Signed-off-by: Eli Schwartz <eschwartz@archlinux.org>
> ---
>  Documentation/pretty-formats.txt | 11 ++++++-----
>  pretty.c                         | 23 +++++++++++++++++++----
>  t/t4205-log-pretty-formats.sh    |  8 ++++++++
>  3 files changed, 33 insertions(+), 9 deletions(-)
>
> diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
> index ef6bd420ae..14107ac191 100644
> --- a/Documentation/pretty-formats.txt
> +++ b/Documentation/pretty-formats.txt
> @@ -220,6 +220,7 @@ The placeholders are:
>  			  inconsistent when tags are added or removed at
>  			  the same time.
>  +
> +** 'tags[=<BOOL>]': Also consider lightweight tags.
>  ** 'match=<pattern>': Only consider tags matching the given
>     `glob(7)` pattern, excluding the "refs/tags/" prefix.
>  ** 'exclude=<pattern>': Do not consider tags matching the given

> @@ -273,11 +274,6 @@ endif::git-rev-list[]
>  			  If any option is provided multiple times the
>  			  last occurrence wins.
>  +
> -The boolean options accept an optional value `[=<BOOL>]`. The values
> -`true`, `false`, `on`, `off` etc. are all accepted. See the "boolean"
> -sub-section in "EXAMPLES" in linkgit:git-config[1]. If a boolean
> -option is given with no value, it's enabled.
> -+
>  ** 'key=<K>': only show trailers with specified key. Matching is done
>     case-insensitively and trailing colon is optional. If option is
>     given multiple times trailer lines matching any of the keys are
> @@ -313,6 +309,11 @@ insert an empty string unless we are traversing reflog entries (e.g., by
>  decoration format if `--decorate` was not already provided on the command
>  line.
>  
> +The boolean options accept an optional value `[=<BOOL>]`. The values
> +`true`, `false`, `on`, `off` etc. are all accepted. See the "boolean"
> +sub-section in "EXAMPLES" in linkgit:git-config[1]. If a boolean
> +option is given with no value, it's enabled.
> +

> diff --git a/pretty.c b/pretty.c
> index 9db2c65538..3a41bedf1a 100644
> --- a/pretty.c
> +++ b/pretty.c
> @@ -1216,28 +1216,43 @@ int format_set_trailers_options(struct process_trailer_options *opts,
>  
>  static size_t parse_describe_args(const char *start, struct strvec *args)
>  {
> +	const char *options[] = { "tags" };
>  	const char *option_arguments[] = { "match", "exclude" };
>  	const char *arg = start;
>  
>  	for (;;) {
>  		const char *matched = NULL;
> -		const char *argval;
> +		const char *argval = NULL;
>  		size_t arglen = 0;
> +		int optval = 0;
>  		int i;
>  
>  		for (i = 0; i < ARRAY_SIZE(option_arguments); i++) {
>  			if (match_placeholder_arg_value(arg, option_arguments[i], &arg,
>  							&argval, &arglen)) {
>  				matched = option_arguments[i];
> +				if (!arglen)
> +					return 0;
>  				break;
>  			}
>  		}
> +		if (!matched)
> +			for (i = 0; i < ARRAY_SIZE(options); i++) {
> +				if (match_placeholder_bool_arg(arg, options[i], &arg,
> +								&optval)) {
> +					matched = options[i];
> +					break;
> +				}
> +			}
>  		if (!matched)
>  			break;
>  

> -		if (!arglen)
> -			return 0;
> -		strvec_pushf(args, "--%s=%.*s", matched, (int)arglen, argval);
> +
> +		if (argval) {
> +			strvec_pushf(args, "--%s=%.*s", matched, (int)arglen, argval);
> +		} else if (optval) {
> +			strvec_pushf(args, "--%s", matched);
> +		}
>  	}
>  	return arg - start;
>  }
> diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
> index 5865daa8f8..d4acf8882f 100755
> --- a/t/t4205-log-pretty-formats.sh
> +++ b/t/t4205-log-pretty-formats.sh
> @@ -1002,4 +1002,12 @@ test_expect_success '%(describe:exclude=...) vs git describe --exclude ...' '
>  	test_cmp expect actual
>  '
>  
> +test_expect_success '%(describe:tags) vs git describe --tags' '
> +	test_when_finished "git tag -d tagname" &&
> +	git tag tagname &&
> +	git describe --tags >expect &&
> +	git log -1 --format="%(describe:tags)" >actual &&
> +	test_cmp expect actual
> +'

> Eli Schwartz <eschwartz@archlinux.org> writes:
> 
>> The %(describe) placeholder by default, like `git describe`, only
>> supports annotated tags. However, some people do use lightweight tags
>> for releases, and would like to describe those anyway. The command line
>> tool has an option to support this.
>>
>> Teach the placeholder to support this as well.
>>
>> Signed-off-by: Eli Schwartz <eschwartz@archlinux.org>
>> ---
>>  Documentation/pretty-formats.txt | 11 ++++++-----
>>  pretty.c                         | 23 +++++++++++++++++++----
>>  t/t4205-log-pretty-formats.sh    |  8 ++++++++
>>  3 files changed, 33 insertions(+), 9 deletions(-)
>>
>> diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
>> index ef6bd420ae..14107ac191 100644
>> --- a/Documentation/pretty-formats.txt
>> +++ b/Documentation/pretty-formats.txt
>> @@ -220,6 +220,7 @@ The placeholders are:
>>  			  inconsistent when tags are added or removed at
>>  			  the same time.
>>  +
>> +** 'tags[=<BOOL>]': Also consider lightweight tags.
>>  ** 'match=<pattern>': Only consider tags matching the given
>>     `glob(7)` pattern, excluding the "refs/tags/" prefix.
>>  ** 'exclude=<pattern>': Do not consider tags matching the given
> 
> What is the guiding principle used in this patch to decide where the
> new entry should go?  
> 
> The existing 'match' and 'exclude' are the opposites to each other,
> and it makes sense to keep them together, and between them, 'match'
> is the positive variant while 'exclude' is the negative one, so the
> current order makes sense.  I wonder why the new "also consider"
> should come before them, instead of after.
> 
> I am not saying you should change the order, and I would be most
> unhappy if you did so without explanation in an updated patch.
> Rather, I would like to hear the reasoning behind the decision,
> preferrably in the proposed log message.

>> @@ -273,11 +274,6 @@ endif::git-rev-list[]
>>  			  If any option is provided multiple times the
>>  			  last occurrence wins.
>>  +
>> -The boolean options accept an optional value `[=<BOOL>]`. The values
>> -`true`, `false`, `on`, `off` etc. are all accepted. See the "boolean"
>> -sub-section in "EXAMPLES" in linkgit:git-config[1]. If a boolean
>> -option is given with no value, it's enabled.
>> -+
>>  ** 'key=<K>': only show trailers with specified key. Matching is done
>>     case-insensitively and trailing colon is optional. If option is
>>     given multiple times trailer lines matching any of the keys are
>> @@ -313,6 +309,11 @@ insert an empty string unless we are traversing reflog entries (e.g., by
>>  decoration format if `--decorate` was not already provided on the command
>>  line.
>>  
>> +The boolean options accept an optional value `[=<BOOL>]`. The values
>> +`true`, `false`, `on`, `off` etc. are all accepted. See the "boolean"
>> +sub-section in "EXAMPLES" in linkgit:git-config[1]. If a boolean
>> +option is given with no value, it's enabled.
>> +
> 
> This paragraph used to be inside the description of %(trailers:...),
> but that was only because %(trailers) was the only one that took a
> boolean value for its options, and not because it was the only one
> that had special treatment for its boolean options.  Because the
> existing rule for an option that takes a boolean value equally
> applies to the %(describe:...), and more importantly, because we
> expect that any other pretty-format placeholder that would acquire
> an option with boolean value would follow the same rule, it makes
> sense to move it here, together with other rules like %+ and %- that
> apply to any placeholders.
> 
> Makes sense.  I very much appreciate this extra attention to the
> detail.

>> diff --git a/pretty.c b/pretty.c
>> index 9db2c65538..3a41bedf1a 100644
>> --- a/pretty.c
>> +++ b/pretty.c
>> @@ -1216,28 +1216,43 @@ int format_set_trailers_options(struct process_trailer_options *opts,
>>  
>>  static size_t parse_describe_args(const char *start, struct strvec *args)
>>  {
>> +	const char *options[] = { "tags" };
>>  	const char *option_arguments[] = { "match", "exclude" };
>>  	const char *arg = start;
>>  
>>  	for (;;) {
>>  		const char *matched = NULL;
>> -		const char *argval;
>> +		const char *argval = NULL;
>>  		size_t arglen = 0;
>> +		int optval = 0;
>>  		int i;
>>  
>>  		for (i = 0; i < ARRAY_SIZE(option_arguments); i++) {
>>  			if (match_placeholder_arg_value(arg, option_arguments[i], &arg,
>>  							&argval, &arglen)) {
>>  				matched = option_arguments[i];
>> +				if (!arglen)
>> +					return 0;
>>  				break;
>>  			}
>>  		}
>> +		if (!matched)
>> +			for (i = 0; i < ARRAY_SIZE(options); i++) {
>> +				if (match_placeholder_bool_arg(arg, options[i], &arg,
>> +								&optval)) {
>> +					matched = options[i];
>> +					break;
>> +				}
>> +			}
>>  		if (!matched)
>>  			break;
>>  
> 
> I find this new structure of the code somewhat dubious.  Shouldn't
> we be rather starting with an array of struct that describes the
> token to match and how the token should be handled?  Something like
> 
>     struct {
> 	const char *name;
> 	enum { OPT_STRING, OPT_BOOL } type;
>     } option[] = {
> 	{ "exclude", OPT_STRING },
>         { "match", OPT_STRING },
> 	{ "tags", OPT_BOOL },
>     };
> 
>     for (;;) {
> 	int i;
> 	int found = 0;
> 	...
>         for (i = 0; !found && i < ARRAY_SIZE(option); i++) {
>             switch (option.type) {
>             case OPT_STRING:
>                 if (match_placeholder_arg_value(...)) {
>                     strvec_pushf(args, "--%s=%.*s", ...);
> 		    found = 1;
> 		}
>                 break;
>             case OPT_BOOL:
>                 if (match_placeholder_bool_arg(...)) {
> 		    found = 1;
> 		    if (optval)
> 			strvec_pushf(args, "--%s", option.name);
> 		    else
> 			strvec_pushf(args, "--no-%s", option.name);
> 		}
> 		break;
> 	    }
> 	}
>     }
> 
> And instead of the option -> option_arguments rename, the 1/3 of the
> series can be to introduce the above structure, without introducing
> OPT_BOOL and "tags" element to the option[] array.

>> -		if (!arglen)
>> -			return 0;
>> -		strvec_pushf(args, "--%s=%.*s", matched, (int)arglen, argval);
>> +
>> +		if (argval) {
>> +			strvec_pushf(args, "--%s=%.*s", matched, (int)arglen, argval);
>> +		} else if (optval) {
>> +			strvec_pushf(args, "--%s", matched);
>> +		}
>>  	}
>>  	return arg - start;
>>  }
>> diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
>> index 5865daa8f8..d4acf8882f 100755
>> --- a/t/t4205-log-pretty-formats.sh
>> +++ b/t/t4205-log-pretty-formats.sh
>> @@ -1002,4 +1002,12 @@ test_expect_success '%(describe:exclude=...) vs git describe --exclude ...' '
>>  	test_cmp expect actual
>>  '
>>  
>> +test_expect_success '%(describe:tags) vs git describe --tags' '
>> +	test_when_finished "git tag -d tagname" &&
>> +	git tag tagname &&
>> +	git describe --tags >expect &&
>> +	git log -1 --format="%(describe:tags)" >actual &&
>> +	test_cmp expect actual
>> +'
> 
> Nice.
> 
> I like how the end-user visible part of this addition is designed to
> look very much.  With a cleaned up implementation it would be great.
> 
> Thanks.
> 

> It contains option arguments only, not options. We would like to add
> option support here too, but to do that we need to distinguish between
> different types of options.
>
> Lay out the groundwork for distinguishing between bools, strings, etc.
> and move the central logic (validating values and pushing new arguments
> to *args) into the successful match, because that will be fairly
> conditional on what type of argument is being parsed.
>
> Signed-off-by: Eli Schwartz <eschwartz@archlinux.org>
> ---
> diff --git a/pretty.c b/pretty.c
> @@ -1216,28 +1216,37 @@ int format_set_trailers_options(struct process_trailer_options *opts,
>  static size_t parse_describe_args(const char *start, struct strvec *args)
>  {
> +       struct {
> +               char *name;
> +               enum { OPT_STRING } type;
> +       }  option[] = {
> +               { "exclude", OPT_STRING },
> +               { "match", OPT_STRING },
> +       };
>         const char *arg = start;
>
>         for (;;) {
> +               int found = 0;
>                 const char *argval;
>                 size_t arglen = 0;
>                 int i;
>
> +               for (i = 0; !found && i < ARRAY_SIZE(option); i++) {
> +                       switch(option[i].type) {
> +                       case OPT_STRING:
> +                               if (match_placeholder_arg_value(arg, option[i].name, &arg,
> +                                                               &argval, &arglen) && arglen) {
> +                                       if (!arglen)
> +                                               return 0;

> +                                       strvec_pushf(args, "--%s=%.*s", option[i].name, (int)arglen, argval);
> +                                       found = 1;
> +                               }
>                                 break;
>                         }
>                 }
> +               if (!found)
>                         break;

> On Mon, Oct 25, 2021 at 9:36 PM Eli Schwartz <eschwartz@archlinux.org> wrote:
>> It contains option arguments only, not options. We would like to add
>> option support here too, but to do that we need to distinguish between
>> different types of options.
>>
>> Lay out the groundwork for distinguishing between bools, strings, etc.
>> and move the central logic (validating values and pushing new arguments
>> to *args) into the successful match, because that will be fairly
>> conditional on what type of argument is being parsed.
>>
>> Signed-off-by: Eli Schwartz <eschwartz@archlinux.org>
>> ---
>> diff --git a/pretty.c b/pretty.c
>> @@ -1216,28 +1216,37 @@ int format_set_trailers_options(struct process_trailer_options *opts,
>>  static size_t parse_describe_args(const char *start, struct strvec *args)
>>  {
>> +       struct {
>> +               char *name;
>> +               enum { OPT_STRING } type;
>> +       }  option[] = {
>> +               { "exclude", OPT_STRING },
>> +               { "match", OPT_STRING },
>> +       };
>>         const char *arg = start;
>>
>>         for (;;) {
>> +               int found = 0;
>>                 const char *argval;
>>                 size_t arglen = 0;
>>                 int i;
>>
>> +               for (i = 0; !found && i < ARRAY_SIZE(option); i++) {
>> +                       switch(option[i].type) {
>> +                       case OPT_STRING:
>> +                               if (match_placeholder_arg_value(arg, option[i].name, &arg,
>> +                                                               &argval, &arglen) && arglen) {
>> +                                       if (!arglen)
>> +                                               return 0;
> 
> I may be missing something obvious, but how will it be possible for:
> 
>     if (!arglen)
>         return 0;
> 
> to trigger if the `if` immediately above it:
> 
>     if (... && arglen) {
> 
>  has already asserted that `arglen` is not 0?

>> +                                       strvec_pushf(args, "--%s=%.*s", option[i].name, (int)arglen, argval);
>> +                                       found = 1;
>> +                               }
>>                                 break;
>>                         }
>>                 }
>> +               if (!found)
>>                         break;
> 
> The use of `found` to break out of a loop from within a `switch` seems
> a bit clunky. An alternative would be to `goto` a label...
> 
>>         }
>>         return arg - start;
> 
> ... which could be introduced just before the `return`. Of course,
> this is highly subjective, so not necessarily worth changing.

> The %(describe) placeholder by default, like `git describe`, only
> supports annotated tags. However, some people do use lightweight tags
> for releases, and would like to describe those anyway. The command line
> tool has an option to support this.
>
> Teach the placeholder to support this as well.
>
> Signed-off-by: Eli Schwartz <eschwartz@archlinux.org>
> ---
> diff --git a/pretty.c b/pretty.c
> @@ -1229,10 +1230,21 @@ static size_t parse_describe_args(const char *start, struct strvec *args)
>                 for (i = 0; !found && i < ARRAY_SIZE(option); i++) {
>                         switch(option[i].type) {
> +                       case OPT_BOOL:
> +                               if(match_placeholder_bool_arg(arg, option[i].name, &arg, &optval)) {

> +                                       if (optval) {
> +                                               strvec_pushf(args, "--%s", option[i].name);
> +                                       } else {
> +                                               strvec_pushf(args, "--no-%s", option[i].name);
> +                                       }

> On Mon, Oct 25, 2021 at 9:36 PM Eli Schwartz <eschwartz@archlinux.org> wrote:
>> The %(describe) placeholder by default, like `git describe`, only
>> supports annotated tags. However, some people do use lightweight tags
>> for releases, and would like to describe those anyway. The command line
>> tool has an option to support this.
>>
>> Teach the placeholder to support this as well.
>>
>> Signed-off-by: Eli Schwartz <eschwartz@archlinux.org>
>> ---
>> diff --git a/pretty.c b/pretty.c
>> @@ -1229,10 +1230,21 @@ static size_t parse_describe_args(const char *start, struct strvec *args)
>>                 for (i = 0; !found && i < ARRAY_SIZE(option); i++) {
>>                         switch(option[i].type) {
>> +                       case OPT_BOOL:
>> +                               if(match_placeholder_bool_arg(arg, option[i].name, &arg, &optval)) {
> 
> Style nit: add space after `if`

>> +                                       if (optval) {
>> +                                               strvec_pushf(args, "--%s", option[i].name);
>> +                                       } else {
>> +                                               strvec_pushf(args, "--no-%s", option[i].name);
>> +                                       }
> 
> We would normally omit the braces for this simple `if`:
> 
>     if (optval)
>         strvec_pushf(...);
>     else
>         strvec_pushf(...);
> 
> ... or maybe even use the ternary operator:
> 
>     strvec_pushf(args, "--%s%s", optval ? "" : "no-", option[i].name);
> 
> but it's highly subjective whether or not that's more readable.

> The %(describe) placeholder by default, like `git describe`, uses a
> seven-character abbreviated commit object name. This may not be
> sufficient to fully describe all commits in a given repository,
> resulting in a placeholder replacement changing its length because the
> repository grew in size.  This could cause the output of git-archive to
> change.
>
> Add the --abbrev option to `git describe` to the placeholder interface
> in order to provide tools to the user for fine-tuning project defaults
> and ensure reproducible archives.
>
> One alternative would be to just always specify --abbrev=40 but this may
> be a bit too biased...
>
> Signed-off-by: Eli Schwartz <eschwartz@archlinux.org>
> ---
> diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
> @@ -222,6 +222,10 @@ The placeholders are:
> +** 'abbrev=<N>': Instead of using the default number of hexadecimal digits
> +   (which will vary according to the number of objects in the repository with a
> +   default of 7) of the abbreviated object name, use <n> digits, or as many digits
> +   as needed to form a unique object name.

> diff --git a/pretty.c b/pretty.c
> @@ -1245,6 +1246,19 @@ static size_t parse_describe_args(const char *start, struct strvec *args)
> +                       case OPT_INTEGER:
> +                               if (match_placeholder_arg_value(arg, option[i].name, &arg,
> +                                                               &argval, &arglen) && arglen) {
> +                                       if (!arglen)
> +                                               return 0;

> The %(describe) placeholder by default, like `git describe`, uses a
> seven-character abbreviated commit object name. This may not be
> sufficient to fully describe all commits in a given repository,
> resulting in a placeholder replacement changing its length because the
> repository grew in size.  This could cause the output of git-archive to
> change.
> 
> Add the --abbrev option to `git describe` to the placeholder interface
> in order to provide tools to the user for fine-tuning project defaults
> and ensure reproducible archives.
> 
> One alternative would be to just always specify --abbrev=40 but this may
> be a bit too biased...
> 
> Signed-off-by: Eli Schwartz <eschwartz@archlinux.org>
> ---
> 
> Notes:
>     With regard to validating that an integer is passed, I attempt to parse the
>     result using the same mechanism git-describe itself does in the abbrev
>     callback, just with slightly different validation of what we have at the end...
>     because of course here argval is the entire rest of the format string,
>     including the ")".
>     
>     While testing that this actually does what it's supposed to do, I noticed that
>     it doesn't validate junk like leading whitespace or plus signs... this is a
>     problem for `git describe --abbrev='    +15'` too so I guess it's not my
>     problem to fix...
> 
>  Documentation/pretty-formats.txt |  4 ++++
>  pretty.c                         | 16 +++++++++++++++-
>  t/t4205-log-pretty-formats.sh    |  8 ++++++++
>  3 files changed, 27 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
> index 86ed801aad..57fd84f579 100644
> --- a/Documentation/pretty-formats.txt
> +++ b/Documentation/pretty-formats.txt
> @@ -222,6 +222,10 @@ The placeholders are:
>  +
>  ** 'tags[=<BOOL>]': Instead of only considering annotated tags,
>     consider lightweight tags as well.
> +** 'abbrev=<N>': Instead of using the default number of hexadecimal digits
> +   (which will vary according to the number of objects in the repository with a
> +   default of 7) of the abbreviated object name, use <n> digits, or as many digits
> +   as needed to form a unique object name.
>  ** 'match=<pattern>': Only consider tags matching the given
>     `glob(7)` pattern, excluding the "refs/tags/" prefix.
>  ** 'exclude=<pattern>': Do not consider tags matching the given
> diff --git a/pretty.c b/pretty.c
> index 16b5366fed..44bfc49b38 100644
> --- a/pretty.c
> +++ b/pretty.c
> @@ -1218,9 +1218,10 @@ static size_t parse_describe_args(const char *start, struct strvec *args)
>  {
>  	struct {
>  		char *name;
> -		enum { OPT_BOOL, OPT_STRING, } type;
> +		enum { OPT_BOOL, OPT_INTEGER, OPT_STRING, } type;
>  	}  option[] = {
>  		{ "tags", OPT_BOOL},
> +		{ "abbrev", OPT_INTEGER },
>  		{ "exclude", OPT_STRING },
>  		{ "match", OPT_STRING },
>  	};
> @@ -1245,6 +1246,19 @@ static size_t parse_describe_args(const char *start, struct strvec *args)
>  					found = 1;
>  				}
>  				break;
> +			case OPT_INTEGER:
> +				if (match_placeholder_arg_value(arg, option[i].name, &arg,
> +								&argval, &arglen) && arglen) {
> +					if (!arglen)
> +						return 0;
> +					char* endptr;

> On 2021-10-25 21:34:52-0400, Eli Schwartz <eschwartz@archlinux.org> wrote:
> > +                                     if (!arglen)
> > +                                             return 0;
> > +                                     char* endptr;
>
> Other than the question pointed out by Eric,
>
> with DEVELOPER=1, -Werror=declaration-after-statement
> We'll need this change squashed in:
>
> +                                       char* endptr;
>                                         if (!arglen)
>                                                 return 0;
> -                                       char* endptr;

>
> On 10/26/21 8:06 AM, Đoàn Trần Công Danh wrote:
> > Other than the question pointed out by Eric,
> >
> > with DEVELOPER=1, -Werror=declaration-after-statement
> > We'll need this change squashed in:
>
> Thanks for the advice. In v1 of this patchset I attempted to do a
> developer build but failed due to preexisting errors:

> On 10/26/21 8:06 AM, Đoàn Trần Công Danh wrote:
> > Other than the question pointed out by Eric,
> >
> > with DEVELOPER=1, -Werror=declaration-after-statement
> > We'll need this change squashed in:
>
>
> Thanks for the advice. In v1 of this patchset I attempted to do a
> developer build but failed due to preexisting errors:
>
>
>     CC run-command.o
> run-command.c: In function ‘async_die_is_recursing’:
> run-command.c:1102:9: error: ‘pthread_setspecific’ expecting 1 byte in a
> region of size 0 [-Werror=stringop-overread]
>  1102 |         pthread_setspecific(async_die_counter, (void *)1);
>       |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> In file included from /usr/include/openssl/crypto.h:415,
>                  from /usr/include/openssl/comp.h:16,
>                  from /usr/include/openssl/ssl.h:17,
>                  from git-compat-util.h:309,
>                  from cache.h:4,
>                  from run-command.c:1:
> /usr/include/pthread.h:1308:12: note: in a call to function
> ‘pthread_setspecific’ declared with attribute ‘access (none, 2)’
>  1308 | extern int pthread_setspecific (pthread_key_t __key,
>       |            ^~~~~~~~~~~~~~~~~~~
> cc1: all warnings being treated as errors
>
>
>
> My system has a custom compiled glibc from git roughly around the 2.34
> release (a similar environment could be obtained by using Fedora rawhide
> I guess), and this commit looks mighty suspicious:
> https://sourceware.org/git/?p=glibc.git;a=commitdiff;h=a1561c3bbe8e72c6e44280d1eb5e529d2da4ecd0
>
> For this reason, I did not bother to try testing v2 under a developer
> build, leading to my overlooking this issue. ;)

> On Tue, 26 Oct 2021, Eli Schwartz wrote:
>
> > On 10/26/21 8:06 AM, Đoàn Trần Công Danh wrote:
> > > Other than the question pointed out by Eric,
> > >
> > > with DEVELOPER=1, -Werror=declaration-after-statement
> > > We'll need this change squashed in:
> >
> >
> > Thanks for the advice. In v1 of this patchset I attempted to do a
> > developer build but failed due to preexisting errors:
> >
> >
> >     CC run-command.o
> > run-command.c: In function ‘async_die_is_recursing’:
> > run-command.c:1102:9: error: ‘pthread_setspecific’ expecting 1 byte in a
> > region of size 0 [-Werror=stringop-overread]
> >  1102 |         pthread_setspecific(async_die_counter, (void *)1);
> >       |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > In file included from /usr/include/openssl/crypto.h:415,
> >                  from /usr/include/openssl/comp.h:16,
> >                  from /usr/include/openssl/ssl.h:17,
> >                  from git-compat-util.h:309,
> >                  from cache.h:4,
> >                  from run-command.c:1:
> > /usr/include/pthread.h:1308:12: note: in a call to function
> > ‘pthread_setspecific’ declared with attribute ‘access (none, 2)’
> >  1308 | extern int pthread_setspecific (pthread_key_t __key,
> >       |            ^~~~~~~~~~~~~~~~~~~
> > cc1: all warnings being treated as errors
> >
> >
> >
> > My system has a custom compiled glibc from git roughly around the 2.34
> > release (a similar environment could be obtained by using Fedora rawhide
> > I guess), and this commit looks mighty suspicious:
> > https://sourceware.org/git/?p=glibc.git;a=commitdiff;h=a1561c3bbe8e72c6e44280d1eb5e529d2da4ecd0
> >
> > For this reason, I did not bother to try testing v2 under a developer
> > build, leading to my overlooking this issue. ;)
>
> It seems that this issue now hit an official version. As I explained in
> https://lore.kernel.org/git/nycvar.QRO.7.76.6.2111040007170.56@tvgsbejvaqbjf.bet/T/#u,
> my colleague Victoria Dye will send a fix for this later.

> Hi Eli,
> 
> On Tue, 26 Oct 2021, Eli Schwartz wrote:
> 
>> My system has a custom compiled glibc from git roughly around the 2.34
>> release (a similar environment could be obtained by using Fedora rawhide
>> I guess), and this commit looks mighty suspicious:
>> https://sourceware.org/git/?p=glibc.git;a=commitdiff;h=a1561c3bbe8e72c6e44280d1eb5e529d2da4ecd0
>>
>> For this reason, I did not bother to try testing v2 under a developer
>> build, leading to my overlooking this issue. ;)
> 
> It seems that this issue now hit an official version. As I explained in
> https://lore.kernel.org/git/nycvar.QRO.7.76.6.2111040007170.56@tvgsbejvaqbjf.bet/T/#u,
> my colleague Victoria Dye will send a fix for this later.
> 
> Stay tuned,

> Eli Schwartz <eschwartz@archlinux.org> writes:
> 
>>  +
>> +** 'tags[=<BOOL>]': Instead of only considering annotated tags,
>> +   consider lightweight tags as well.
> 
> This part contradicts what Jean-Noël's df34a41f is trying to
> achieve, which can be seen in these hunks from it:
>
> [...]
> 
> So, let's instead use
> 
>     tags[=<bool-value>]: Instead of only considering ...
> 
> i.e. lowercase, with -value suffix.

> Eli Schwartz <eschwartz@archlinux.org> writes:
>
>>  +
>> +** 'tags[=<BOOL>]': Instead of only considering annotated tags,
>> +   consider lightweight tags as well.
>
> This part contradicts what Jean-Noël's df34a41f is trying to
> achieve, which can be seen in these hunks from it:
> ...
> So, let's instead use
>
>     tags[=<bool-value>]: Instead of only considering ...
>
> i.e. lowercase, with -value suffix.

> Junio C Hamano <gitster@pobox.com> writes:
> 
>> Eli Schwartz <eschwartz@archlinux.org> writes:
>>
>>>  +
>>> +** 'tags[=<BOOL>]': Instead of only considering annotated tags,
>>> +   consider lightweight tags as well.
>>
>> This part contradicts what Jean-Noël's df34a41f is trying to
>> achieve, which can be seen in these hunks from it:
>> ...
>> So, let's instead use
>>
>>     tags[=<bool-value>]: Instead of only considering ...
>>
>> i.e. lowercase, with -value suffix.
> 
> The other topic merges earlier to 'seen' before your topic, and FYI,
> the diff between the tip of 'seen' before and after your topic gets
> merged looks like this, with my semantic conflict resolution.
> 
> Notice the way placeholders are spelled in lowercase and generally
> have more descriptive names.
> 
> Thanks.
> 
> diff --git c/Documentation/pretty-formats.txt w/Documentation/pretty-formats.txt
> index d465cd59dd..25cfffab38 100644
> --- c/Documentation/pretty-formats.txt
> +++ w/Documentation/pretty-formats.txt
> @@ -220,6 +220,12 @@ The placeholders are:
>  			  inconsistent when tags are added or removed at
>  			  the same time.
>  +
> +** 'tags[=<bool-value>]': Instead of only considering annotated tags,
> +   consider lightweight tags as well.
> +** 'abbrev=<number>': Instead of using the default number of hexadecimal digits

> The %(describe) placeholder by default, like `git describe`, uses a
> seven-character abbreviated commit object name. This may not be
> sufficient to fully describe all commits in a given repository,
> resulting in a placeholder replacement changing its length because the
> repository grew in size.  This could cause the output of git-archive to
> change.
>
> Add the --abbrev option to `git describe` to the placeholder interface
> in order to provide tools to the user for fine-tuning project defaults
> and ensure reproducible archives.
> [...]
> Signed-off-by: Eli Schwartz <eschwartz@archlinux.org>
> ---
> diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
> @@ -222,6 +222,10 @@ The placeholders are:
> +** 'abbrev=<N>': Instead of using the default number of hexadecimal digits
> +   (which will vary according to the number of objects in the repository with a
> +   default of 7) of the abbreviated object name, use <n> digits, or as many digits
> +   as needed to form a unique object name.

> On Fri, Oct 29, 2021 at 2:45 PM Eli Schwartz <eschwartz@archlinux.org> wrote:
>> The %(describe) placeholder by default, like `git describe`, uses a
>> seven-character abbreviated commit object name. This may not be
>> sufficient to fully describe all commits in a given repository,
>> resulting in a placeholder replacement changing its length because the
>> repository grew in size.  This could cause the output of git-archive to
>> change.
>>
>> Add the --abbrev option to `git describe` to the placeholder interface
>> in order to provide tools to the user for fine-tuning project defaults
>> and ensure reproducible archives.
>> [...]
>> Signed-off-by: Eli Schwartz <eschwartz@archlinux.org>
>> ---
>> diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
>> @@ -222,6 +222,10 @@ The placeholders are:
>> +** 'abbrev=<N>': Instead of using the default number of hexadecimal digits
>> +   (which will vary according to the number of objects in the repository with a
>> +   default of 7) of the abbreviated object name, use <n> digits, or as many digits
>> +   as needed to form a unique object name.
> 
> There's still an inconsistent mix of `<N>` and `<n>` here (mentioned
> in my earlier review). Is that intentional or just a simple oversight?

> +	struct {
> +		char *name;
> +		enum { OPT_STRING } type;
> +	}  option[] = {
> +		{ "exclude", OPT_STRING },
> +		{ "match", OPT_STRING },
> +	};

> Eli Schwartz <eschwartz@archlinux.org> writes:
> 
>> +	struct {
>> +		char *name;
>> +		enum { OPT_STRING } type;
>> +	}  option[] = {
>> +		{ "exclude", OPT_STRING },
>> +		{ "match", OPT_STRING },
>> +	};
> 
> I floated OPT_<TYPE> in my earlier illustration as "something like
> this", not "literally use these tokens".  We have CPP macros of the
> same name in parse-options.h API---we may not see troubles from the
> name clashes today, but let's not leave it to chances.
> 
> Perhaps call it like DESCRBE_ARG_STRING or something that ensures
> uniqueness like that?

> On 10/29/21 4:11 PM, Junio C Hamano wrote:
>> Eli Schwartz <eschwartz@archlinux.org> writes:
>> 
>>> +	struct {
>>> +		char *name;
>>> +		enum { OPT_STRING } type;
>>> +	}  option[] = {
>>> +		{ "exclude", OPT_STRING },
>>> +		{ "match", OPT_STRING },
>>> +	};
>> 
>> I floated OPT_<TYPE> in my earlier illustration as "something like
>> this", not "literally use these tokens".  We have CPP macros of the
>> same name in parse-options.h API---we may not see troubles from the
>> name clashes today, but let's not leave it to chances.
>> 
>> Perhaps call it like DESCRBE_ARG_STRING or something that ensures
>> uniqueness like that?
>
>
> Ah. That alternative seems a bit long though. :( Without breaking enum
> type into one per line, it will quickly overflow line lengths... though
> maybe it should be one per line anyway?

> The %(describe) placeholder by default, like `git describe`, only
> supports annotated tags. However, some people do use lightweight tags
> for releases, and would like to describe those anyway. The command line
> tool has an option to support this.
>
> Teach the placeholder to support this as well.
>
> Signed-off-by: Eli Schwartz <eschwartz@archlinux.org>
> ---
>
> I use lowercase "bool" here not "boolean-value" because I don't see
> utility in the word "value" here.

> Eli Schwartz <eschwartz@archlinux.org> writes:
> 
>> The %(describe) placeholder by default, like `git describe`, only
>> supports annotated tags. However, some people do use lightweight tags
>> for releases, and would like to describe those anyway. The command line
>> tool has an option to support this.
>>
>> Teach the placeholder to support this as well.
>>
>> Signed-off-by: Eli Schwartz <eschwartz@archlinux.org>
>> ---
>>
>> I use lowercase "bool" here not "boolean-value" because I don't see
>> utility in the word "value" here.
> 
> Such a comment is much more useful if it is sent as a review to the
> patch that touches the same area as your patch does, namely,
> 
> https://lore.kernel.org/git/984b6d687a2e779c775de6ea80536afe6ecc0aaf.1635438124.git.gitgitgadget@gmail.com/
> 
> not here.