| Message ID | 20211026013452.1372122-4-eschwartz@archlinux.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add some more options to the pretty-formats | expand |
On Mon, Oct 25, 2021 at 9:36 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. > > 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. Inconsistent mix of `<N>` and `<n>`. > 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; Same question I asked while reviewing the other patch regarding checking `arglen` in both conditionals: `if (... && arglen)` vs. `if (!arglen)`
On 2021-10-25 21:34:52-0400, 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. > > 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; Other than the question pointed out by Eric, with DEVELOPER=1, -Werror=declaration-after-statement We'll need this change squashed in: ------- 8< ----- diff --git a/pretty.c b/pretty.c index 289b5456c8..85d4ab008b 100644 --- a/pretty.c +++ b/pretty.c @@ -1249,9 +1249,9 @@ 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) { + char* endptr; if (!arglen) return 0; - char* endptr; strtol(argval, &endptr, 10); if (endptr - argval != arglen) return 0; ------- >8 ----- > + strtol(argval, &endptr, 10); > + if (endptr - argval != arglen) > + return 0; > + strvec_pushf(args, "--%s=%.*s", option[i].name, (int)arglen, argval); > + found = 1; > + } > + break; > case OPT_STRING: > if (match_placeholder_arg_value(arg, option[i].name, &arg, > &argval, &arglen) && arglen) { > diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh > index d4acf8882f..35eef4c865 100755 > --- a/t/t4205-log-pretty-formats.sh > +++ b/t/t4205-log-pretty-formats.sh > @@ -1010,4 +1010,12 @@ test_expect_success '%(describe:tags) vs git describe --tags' ' > test_cmp expect actual > ' > > +test_expect_success '%(describe:abbrev=...) vs git describe --abbrev=...' ' > + test_when_finished "git tag -d tagname" && > + git tag -a -m tagged tagname && > + git describe --abbrev=15 >expect && > + git log -1 --format="%(describe:abbrev=15)" >actual && > + test_cmp expect actual > +' > + > test_done > -- > 2.33.1 >
On Tue, Oct 26, 2021 at 8:07 AM Đoàn Trần Công Danh <congdanhqx@gmail.com> wrote: > 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; This highlights a style nit, as well; should be: 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: 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 Wed, Oct 27, 2021 at 12:23 AM Eli Schwartz <eschwartz@archlinux.org> 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: you can always avoid breaking your build by using: DEVOPTS=no-error Carlo
Hi Eli, 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. Stay tuned, Johannes
Hi Eli, On Thu, 4 Nov 2021, Johannes Schindelin wrote: > 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. FYI here is the patch: https://lore.kernel.org/git/pull.1072.v2.git.1635998463474.gitgitgadget@gmail.com/ Ciao, Johannes
On 11/3/21 7:20 PM, Johannes Schindelin wrote: > 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, FWIW this was present in the official version of glibc released in August... the problem is finding an official version of a distro that ships it. :D Thanks for the heads up.
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; + strtol(argval, &endptr, 10); + if (endptr - argval != arglen) + return 0; + strvec_pushf(args, "--%s=%.*s", option[i].name, (int)arglen, argval); + found = 1; + } + break; case OPT_STRING: if (match_placeholder_arg_value(arg, option[i].name, &arg, &argval, &arglen) && arglen) { diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh index d4acf8882f..35eef4c865 100755 --- a/t/t4205-log-pretty-formats.sh +++ b/t/t4205-log-pretty-formats.sh @@ -1010,4 +1010,12 @@ test_expect_success '%(describe:tags) vs git describe --tags' ' test_cmp expect actual ' +test_expect_success '%(describe:abbrev=...) vs git describe --abbrev=...' ' + test_when_finished "git tag -d tagname" && + git tag -a -m tagged tagname && + git describe --abbrev=15 >expect && + git log -1 --format="%(describe:abbrev=15)" >actual && + test_cmp expect actual +' + test_done
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(-)