ref-filter: add new atom "signature" atom
PATCH 1/1 ref-filter: add new atom "signature" atom
From: Nsengiyumva Wilberforce <[email protected]>
This only works for commits. Add "signature" atom with grade, signer, key, fingerprint, primarykeyfingerprint, trustlevel as arguments. This code and it's documentation are inspired by how the %GG, %G?, %GS, %GK, %GF, %GP, and %GT pretty formats were implemented.
---
ref-filter: add new atom "signature" atom
This only works for commits. Add "signature" atom with grade, signer, key, fingerprint, primarykeyfingerprint, trustlevel as arguments. This code and it's documentation are inspired by how the %GG, %G?, %GS, %GK, %GF, %GP, and %GT pretty formats were implemented.
Jaydeep Das [email protected] Mentored-by: Christian Couder [email protected] Mentored-by: Hariom Verma [email protected]
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1452%2Fnsengiyumva-wilberforce%2Fsignature6-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1452/nsengiyumva-wilberforce/signature6-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1452
Documentation/git-for-each-ref.txt | 27 +++++++ ref-filter.c | 94 +++++++++++++++++++++++ t/t6300-for-each-ref.sh | 116 +++++++++++++++++++++++++++++ 3 files changed, 237 insertions(+)
Documentation/git-for-each-ref.txt +27 -0
@@ -212,6 +212,33 @@ symref:: `:lstrip` and `:rstrip` options in the same way as `refname` above. +signature:: + The GPG signature of a commit. + +signature:grade:: + Show "G" for a good (valid) signature, "B" for a bad + signature, "U" for a good signature with unknown validity, "X" + for a good signature that has expired, "Y" for a good + signature made by an expired key, "R" for a good signature + made by a revoked key, "E" if the signature cannot be + checked (e.g. missing key) and "N" for no signature. + +signature:signer:: + The signer of the GPG signature of a commit. + +signature:key:: + The key of the GPG signature of a commit. + +signature:fingerprint:: + The fingerprint of the GPG signature of a commit. + +signature:primarykeyfingerprint:: + The Primary Key fingerprint of the GPG signature of a commit. + +signature:trustlevel:: + The Trust level of the GPG signature of a commit. Possible + outputs are `ultimate`, `fully`, `marginal`, `never` and `undefined`. + worktreepath:: The absolute path to the worktree in which the ref is checked out, if it is checked out in any linked worktree. Empty string
ref-filter.c +94 -0
130 lines not on this page · raw message
t/t6300-for-each-ref.sh +116 -0
126 lines not on this page · raw message
base-commit: c48035d29b4e524aed3a32f0403676f0d9128863
Signature · 1 line
gitgitgadget
Quoted 7 lines · "nsengaw4c via GitGitGadget" <[email protected]> writes:
From: Nsengiyumva Wilberforce <[email protected]>
This only works for commits. Add "signature" atom with
grade,signer,key,fingerprint,primarykeyfingerprint,trustlevelas arguments. This code and it's documentation are inspired by how the %GG, %G?, %GS, %GK, %GF, %GP, and %GT pretty formats were implemented.
Lacking motivation. Without explaining why somebody may want to have the feature and what it would be used for, "only works for commits" would invite a "so what? does it even have to work?" as a response, so start with a brief descrioption "with the current set of atoms, $this_useful_thing cannot easily be achieved" before describing its limitation.
Having said that, wouldn't it be natural to expect that the same code can deal with signed tags? After all we use the same signature verification machinery at the lowest level in the callchain.
Documentation/git-for-each-ref.txt symref:: · line 219
Quoted 13 lines
Documentation/git-for-each-ref.txt +5
-0@@ -212,6 +212,33 @@ symref:: `:lstrip` and `:rstrip` options in the same way as `refname` above. +signature:: +... +signature:trustlevel:: + The Trust level of the GPG signature of a commit. Possible + outputs are `ultimate`, `fully`, `marginal`, `never` and `undefined`.
A good list. How do these work for signature made with a tool other than GPG (in other words, when "gpg.format" is set to something other than "openpgp")?
Quoted 17 lines
Documentation/git-for-each-ref.txt +13
-0@@ -378,6 +383,30 @@ static int subject_atom_parser(struct ref_format *format, struct used_atom *atom return 0; } +static int signature_atom_parser(struct ref_format *format, struct used_atom *atom, + const char *arg, struct strbuf *err) +{ + if (arg) { + if (!strcmp(arg, "signer")) + atom->u.signature.option = S_SIGNER; + else if (!strcmp(arg, "grade")) + atom->u.signature.option = S_GRADE; + else if (!strcmp(arg, "key")) + atom->u.signature.option = S_KEY; + else if (!strcmp(arg, "fingerprint")) + atom->u.signature.option = S_FINGERPRINT; + else if (!strcmp(arg, "primarykeyfingerprint"))
Documentation/git-for-each-ref.txt static int subject_atom_parser(struct ref_format *format, struct used_atom *atom · line 408
Quoted 10 lines
Documentation/git-for-each-ref.txt +10
-0+ atom->u.signature.option = S_PRI_KEY_FP; + else if (!strcmp(arg, "trustlevel")) + atom->u.signature.option = S_TRUST_LEVEL; + else + return strbuf_addf_ret(err, -1, _("unknown %%(signature) argument: %s"), arg); + } + else + atom->u.signature.option = S_BARE; + return 0; +}
Handing the !arg case first will make the if/else if/... cascade easier to follow, no? Also the body of the function may want to become a separate function that returns one of these S_FOO constants.
static enum signatore_option signature_atom_parser(...)
{
enum signature_option opt = parse_signature_option(arg);
if (opt < 0)
return strbuf_addf_ret(err, opt, _("unknown ..."), arg);
return opt;
}where parse_signature_option() would look like
static enum signature_option parse_signature_option(const char *arg)
{
if (!arg)
return S_BARE;
else if (!strcmp(arg, "signer"))
return S_SIGNER;
...
else
return -1;
}or something like that?
Documentation/git-for-each-ref.txt static void grab_person(const char *who, struct atom_value *val, int deref, void · line 1377
Quoted 5 lines
Documentation/git-for-each-ref.txt +1
-0@@ -1344,6 +1374,69 @@ static void grab_person(const char *who, struct atom_value *val, int deref, void } } +static void grab_signature(struct atom_value *val, int deref, struct object *obj)
To be considerate for future developers, perhaps rename this to grab_commit_signature(), so that they can add grab_tag_signature() when they lift the limitation of this implementaiton?
Documentation/git-for-each-ref.txt static void grab_person(const char *who, struct atom_value *val, int deref, void · line 1380
Quoted 3 lines
Documentation/git-for-each-ref.txt +3
-0+{ + int i; + struct commit *commit = (struct commit *) obj;
Style? No SP between cast and value?
Quoted 9 lines
Documentation/git-for-each-ref.txt +9
-0+ + for (i = 0; i < used_atom_cnt; i++) { + struct used_atom *atom = &used_atom[i]; + const char *name = atom->name; + struct atom_value *v = &val[i]; + struct signature_check sigc = { 0 }; + + if (!!deref != (*name == '*')) + continue;
Documentation/git-for-each-ref.txt static void grab_person(const char *who, struct atom_value *val, int deref, void · line 1399
Quoted 10 lines
Documentation/git-for-each-ref.txt +10
-0+ if (deref) + name++; + if (strcmp(name, "signature") && + strcmp(name, "signature:signer") && + strcmp(name, "signature:grade") && + strcmp(name, "signature:key") && + strcmp(name, "signature:fingerprint") && + strcmp(name, "signature:primarykeyfingerprint") && + strcmp(name, "signature:trustlevel")) + continue;
And with the helper above, we can avoid the repetition here that can go out of sync with the parser function.
Documentation/git-for-each-ref.txt static void grab_person(const char *who, struct atom_value *val, int deref, void · line 1400
Quoted 1 line
Documentation/git-for-each-ref.txt +1
-0+ check_commit_signature(commit, &sigc);
If a format asks for signature:signer and signature:key, we shouldn't be running GPG twice. First check used_atom[] to see if we even need to do _any_ signature processing (and leave if there is not), populate the sigc just once and then enter the loop, perhaps?
In adddition, a call to check_commit_signature() should have a matching call to signature_check_clear(); otherwise all the resources held by sigc would leak, wouldn't it?
Hi
Quoted 9 lines
Quoted 8 lines
From: Nsengiyumva Wilberforce <[email protected]>
This only works for commits. Add "signature" atom with
grade,signer,key,fingerprint,primarykeyfingerprint,trustlevelas arguments. This code and it's documentation are inspired by how the %GG, %G?, %GS, %GK, %GF, %GP, and %GT pretty formats were implemented.
Quoted 6 lines
Lacking motivation. Without explaining why somebody may want to have the feature and what it would be used for, "only works for commits" would invite a "so what? does it even have to work?" as a response, so start with a brief descrioption "with the current set of atoms, $this_useful_thing cannot easily be achieved" before describing its limitation.
Ok, I will edit the commit message. Thanks
Quoted 9 lines
Quoted 9 lines
Documentation/git-for-each-ref.txt +0
-0@@ -212,6 +212,33 @@ symref:: `:lstrip` and `:rstrip` options in the same way as `refname` above.
Quoted 9 lines
Documentation/git-for-each-ref.txt symref:: · line 240
Quoted 5 lines
Documentation/git-for-each-ref.txt +5
-0+signature:: +... +signature:trustlevel:: + The Trust level of the GPG signature of a commit. Possible + outputs are `ultimate`, `fully`, `marginal`, `never` and `undefined`.A good list. How do these work for signature made with a tool other than GPG (in other words, when "gpg.format" is set to something other than "openpgp")?
You mean ssh and X509, right? honestly, I did not check the behavior. I am going to check
Quoted 3 lines
Having said that, wouldn't it be natural to expect that the same code can deal with signed tags? After all we use the same signature verification machinery at the lowest level in the callchain.
Very right, it works for signed tags too.
Quoted 26 lines
Handing the !arg case first will make the if/else if/... cascade easier to follow, no? Also the body of the function may want to become a separate function that returns one of these
S_FOOconstants.static enum signatore_option signature_atom_parser(...) { enum signature_option opt = parse_signature_option(arg); if (opt < 0) return strbuf_addf_ret(err, opt, _("unknown ..."), arg); return opt; }where
parse_signature_option()would look likestatic enum signature_option parse_signature_option(const char *arg) { if (!arg) return S_BARE; else if (!strcmp(arg, "signer")) return S_SIGNER; ... else return -1; }
Quoted 1 line
or something like that?
It makes more sense
Quoted 6 lines
Documentation/git-for-each-ref.txt symref:: · line 222
Quoted 4 lines
Documentation/git-for-each-ref.txt +3
-0+{ + int i; + struct commit *commit = (struct commit *) obj;Style? No SP between cast and value?
ok, noted
Quoted 15 lines
Quoted 15 lines
Documentation/git-for-each-ref.txt +14
-0+ + for (i = 0; i < used_atom_cnt; i++) { + struct used_atom *atom = &used_atom[i]; + const char *name = atom->name; + struct atom_value *v = &val[i]; + struct signature_check sigc = { 0 }; + + if (!!deref != (*name == '*')) + continue; + if (deref) + name++; + if (strcmp(name, "signature") && + strcmp(name, "signature:signer") && + strcmp(name, "signature:grade") &&
Quoted 8 lines
Documentation/git-for-each-ref.txt symref:: · line 241
Quoted 5 lines
Documentation/git-for-each-ref.txt +5
-0+ strcmp(name, "signature:key") && + strcmp(name, "signature:fingerprint") && + strcmp(name, "signature:primarykeyfingerprint") && + strcmp(name, "signature:trustlevel")) + continue;And with the helper above, we can avoid the repetition here that can go out of sync with the parser function.
I am not sure I have understood this, which helper?
Quoted 6 lines
Documentation/git-for-each-ref.txt symref:: · line 242
Quoted 1 line
Documentation/git-for-each-ref.txt +1
-0+ check_commit_signature(commit, &sigc);If a format asks for signature:signer and signature:key, we shouldn't be running GPG twice. First check used_atom[] to see if we even need to do _any_ signature processing (and leave if there is not), populate the sigc just once and then enter the loop, perhaps?
Yeah, I think it was not right calling check_commit_signature() in the loop. Populating sigc at once looks more good to me
Quoted 6 lines
In adddition, a call to
check_commit_signature()should have amatching call to
signature_check_clear(); otherwise all theresources held by sigc would leak, wouldn't it?
Yeah, it would.
Quoted 144 lines · Junio C Hamano
Quoted 7 lines · "nsengaw4c via GitGitGadget" <[email protected]> writes:
From: Nsengiyumva Wilberforce <[email protected]>
This only works for commits. Add "signature" atom with
grade,signer,key,fingerprint,primarykeyfingerprint,trustlevelas arguments. This code and it's documentation are inspired by how the %GG, %G?, %GS, %GK, %GF, %GP, and %GT pretty formats were implemented.Lacking motivation. Without explaining why somebody may want to have the feature and what it would be used for, "only works for commits" would invite a "so what? does it even have to work?" as a response, so start with a brief descrioption "with the current set of atoms, $this_useful_thing cannot easily be achieved" before describing its limitation.
Having said that, wouldn't it be natural to expect that the same code can deal with signed tags? After all we use the same signature verification machinery at the lowest level in the callchain.
Documentation/git-for-each-ref.txt symref:: · line 219
Quoted 13 lines
Documentation/git-for-each-ref.txt +5
-0@@ -212,6 +212,33 @@ symref:: `:lstrip` and `:rstrip` options in the same way as `refname` above. +signature:: +... +signature:trustlevel:: + The Trust level of the GPG signature of a commit. Possible + outputs are `ultimate`, `fully`, `marginal`, `never` and `undefined`.A good list. How do these work for signature made with a tool other than GPG (in other words, when "gpg.format" is set to something other than "openpgp")?
Quoted 17 lines
Documentation/git-for-each-ref.txt +13
-0@@ -378,6 +383,30 @@ static int subject_atom_parser(struct ref_format *format, struct used_atom *atom return 0; } +static int signature_atom_parser(struct ref_format *format, struct used_atom *atom, + const char *arg, struct strbuf *err) +{ + if (arg) { + if (!strcmp(arg, "signer")) + atom->u.signature.option = S_SIGNER; + else if (!strcmp(arg, "grade")) + atom->u.signature.option = S_GRADE; + else if (!strcmp(arg, "key")) + atom->u.signature.option = S_KEY; + else if (!strcmp(arg, "fingerprint")) + atom->u.signature.option = S_FINGERPRINT; + else if (!strcmp(arg, "primarykeyfingerprint"))Documentation/git-for-each-ref.txt static int subject_atom_parser(struct ref_format *format, struct used_atom *atom · line 408
Quoted 10 lines
Documentation/git-for-each-ref.txt +10
-0+ atom->u.signature.option = S_PRI_KEY_FP; + else if (!strcmp(arg, "trustlevel")) + atom->u.signature.option = S_TRUST_LEVEL; + else + return strbuf_addf_ret(err, -1, _("unknown %%(signature) argument: %s"), arg); + } + else + atom->u.signature.option = S_BARE; + return 0; +}Handing the !arg case first will make the if/else if/... cascade easier to follow, no? Also the body of the function may want to become a separate function that returns one of these
S_FOOconstants.static enum signatore_option signature_atom_parser(...) { enum signature_option opt = parse_signature_option(arg); if (opt < 0) return strbuf_addf_ret(err, opt, _("unknown ..."), arg); return opt; }where
parse_signature_option()would look likestatic enum signature_option parse_signature_option(const char *arg) { if (!arg) return S_BARE; else if (!strcmp(arg, "signer")) return S_SIGNER; ... else return -1; }or something like that?
Documentation/git-for-each-ref.txt static void grab_person(const char *who, struct atom_value *val, int deref, void · line 1377
Quoted 5 lines
Documentation/git-for-each-ref.txt +1
-0@@ -1344,6 +1374,69 @@ static void grab_person(const char *who, struct atom_value *val, int deref, void } } +static void grab_signature(struct atom_value *val, int deref, struct object *obj)To be considerate for future developers, perhaps rename this to
grab_commit_signature(), so that they can addgrab_tag_signature()when they lift the limitation of this implementaiton?Documentation/git-for-each-ref.txt static void grab_person(const char *who, struct atom_value *val, int deref, void · line 1380
Quoted 3 lines
Documentation/git-for-each-ref.txt +3
-0+{ + int i; + struct commit *commit = (struct commit *) obj;Style? No SP between cast and value?
Quoted 9 lines
Documentation/git-for-each-ref.txt +9
-0+ + for (i = 0; i < used_atom_cnt; i++) { + struct used_atom *atom = &used_atom[i]; + const char *name = atom->name; + struct atom_value *v = &val[i]; + struct signature_check sigc = { 0 }; + + if (!!deref != (*name == '*')) + continue;Documentation/git-for-each-ref.txt static void grab_person(const char *who, struct atom_value *val, int deref, void · line 1399
Quoted 10 lines
Documentation/git-for-each-ref.txt +10
-0+ if (deref) + name++; + if (strcmp(name, "signature") && + strcmp(name, "signature:signer") && + strcmp(name, "signature:grade") && + strcmp(name, "signature:key") && + strcmp(name, "signature:fingerprint") && + strcmp(name, "signature:primarykeyfingerprint") && + strcmp(name, "signature:trustlevel")) + continue;And with the helper above, we can avoid the repetition here that can go out of sync with the parser function.
Documentation/git-for-each-ref.txt static void grab_person(const char *who, struct atom_value *val, int deref, void · line 1400
Quoted 1 line
Documentation/git-for-each-ref.txt +1
-0+ check_commit_signature(commit, &sigc);If a format asks for signature:signer and signature:key, we shouldn't be running GPG twice. First check used_atom[] to see if we even need to do _any_ signature processing (and leave if there is not), populate the sigc just once and then enter the loop, perhaps?
In adddition, a call to
check_commit_signature()should have a matching call tosignature_check_clear(); otherwise all the resources held by sigc would leak, wouldn't it?
Quoted 26 lines · NSENGIYUMVA WILBERFORCE
Quoted 26 lines
Handing the !arg case first will make the if/else if/... cascade easier to follow, no? Also the body of the function may want to become a separate function that returns one of these
S_FOOconstants.static enum signatore_option signature_atom_parser(...) { enum signature_option opt = parse_signature_option(arg); if (opt < 0) return strbuf_addf_ret(err, opt, _("unknown ..."), arg); return opt; }where
parse_signature_option()would look likestatic enum signature_option parse_signature_option(const char *arg) { if (!arg) return S_BARE; else if (!strcmp(arg, "signer")) return S_SIGNER; ... else return -1; }or something like that?
[...]
Quoted 9 lines
Quoted 9 lines
Quoted 8 lines
ref-filter.c +8
-0+if (strcmp (name,"signature" ) && +strcmp (name,"signature:signer" ) && +strcmp (name,"signature:grade" ) && +strcmp (name,"signature:key" ) && +strcmp (name,"signature:fingerprint" ) && +strcmp (name,"signature:primarykeyfingerprint" ) && +strcmp (name,"signature:trustlevel" )) +continue ;
Quoted 4 lines
Quoted 2 lines
And with the helper above, we can avoid the repetition here that can go out of sync with the parser function.
I am not sure I have understood this, which helper?
I think Junio is talking about the following function:
static enum signature_option parse_signature_option(const char *arg)
he suggested above.
With this function the above code could be just something like:
if (parse_signature_option(name) < 0)
continue;
Quoted 7 lines · Christian Couder
Quoted 1 line
I am not sure I have understood this, which helper?
I think Junio is talking about the following function:
static enum
signature_optionparse_signature_option(const char *arg)he suggested above.
Correct.
Quoted 4 lines
With this function the above code could be just something like:
if (
parse_signature_option(name)< 0)continue;
More or less so, but the first "if" in the helper I wrote in the message above is broken. It should be
static enum signature_option parse_signature_option(const char *arg)
{
if (!*arg)
return S_BARE;
else if (!strcmp(arg, "signer"))
return S_SIGNER;
...
else
return -1;
}and then the code equivalent to the bunch of strcmp() would be
if (skip_prefix(name, "signature", &name) &&
(!*name || *name++ == ':') &&
(0 <= parse_signature_option(name)))
; /* we have "signature"-related atom */
else
continue; /* not a "signature" atom */or something like that.
A message that is not in the archive (sent off-list or not yet received): CA+PPyiGd0-AiwhPa5e+fDdA9RybS+c5XeOYm5yycCZco3VHAxg@mail.gmail.com
Quoted 18 lines
Quoted 7 lines
Quoted 1 line
I am not sure I have understood this, which helper?
I think Junio is talking about the following function:
static enum
signature_optionparse_signature_option(const char *arg)he suggested above.
Correct.
Quoted 4 lines
With this function the above code could be just something like:
if (
parse_signature_option(name)< 0)continue;More or less so, but the first "if" in the helper I wrote in the message above is broken. It should be
Quoted 10 lines
static enum signature_option parse_signature_option(const char *arg)
{
if (!*arg)
return S_BARE;
else if (!strcmp(arg, "signer"))
return S_SIGNER;
...
else
return -1;
}This way, running git for-each-ref refs/heads/signature8 --format="%(signature)" raises a seg fault, I looked for the bug using gdb when I check the contents of *arg like this:p *arg, I get this:
Cannot access memory at 0x0.
However, others like signature:key, signature:signer, etc are ok. Leaving it as arg makes everything fine. So I decided to leave it as you had suggested first.
I had actually forgotten to add the test for "%(signature)", so this scenario reminded me to do so.
Quoted 273 lines · NSENGIYUMVA WILBERFORCE
Hi
Quoted 9 lines
Quoted 8 lines
From: Nsengiyumva Wilberforce <[email protected]>
This only works for commits. Add "signature" atom with
grade,signer,key,fingerprint,primarykeyfingerprint,trustlevelas arguments. This code and it's documentation are inspired by how the %GG, %G?, %GS, %GK, %GF, %GP, and %GT pretty formats were implemented.Quoted 6 lines
Lacking motivation. Without explaining why somebody may want to have the feature and what it would be used for, "only works for commits" would invite a "so what? does it even have to work?" as a response, so start with a brief descrioption "with the current set of atoms, $this_useful_thing cannot easily be achieved" before describing its limitation.
Ok, I will edit the commit message. Thanks
Quoted 9 lines
Quoted 9 lines
Documentation/git-for-each-ref.txt +0
-0@@ -212,6 +212,33 @@ symref:: `:lstrip` and `:rstrip` options in the same way as `refname` above.Quoted 9 lines
Documentation/git-for-each-ref.txt symref:: · line 240
Quoted 5 lines
Documentation/git-for-each-ref.txt +5
-0+signature:: +... +signature:trustlevel:: + The Trust level of the GPG signature of a commit. Possible + outputs are `ultimate`, `fully`, `marginal`, `never` and `undefined`.A good list. How do these work for signature made with a tool other than GPG (in other words, when "gpg.format" is set to something other than "openpgp")?
You mean ssh and X509, right? honestly, I did not check the behavior. I am going to check
Quoted 3 lines
Having said that, wouldn't it be natural to expect that the same code can deal with signed tags? After all we use the same signature verification machinery at the lowest level in the callchain.
Very right, it works for signed tags too.
Quoted 26 lines
Handing the !arg case first will make the if/else if/... cascade easier to follow, no? Also the body of the function may want to become a separate function that returns one of these
S_FOOconstants.static enum signatore_option signature_atom_parser(...) { enum signature_option opt = parse_signature_option(arg); if (opt < 0) return strbuf_addf_ret(err, opt, _("unknown ..."), arg); return opt; }where
parse_signature_option()would look likestatic enum signature_option parse_signature_option(const char *arg) { if (!arg) return S_BARE; else if (!strcmp(arg, "signer")) return S_SIGNER; ... else return -1; }Quoted 1 line
or something like that?
It makes more sense
Quoted 6 lines
Documentation/git-for-each-ref.txt symref:: · line 222
Quoted 4 lines
Documentation/git-for-each-ref.txt +3
-0+{ + int i; + struct commit *commit = (struct commit *) obj;Style? No SP between cast and value?
ok, noted
Quoted 15 lines
Quoted 15 lines
Documentation/git-for-each-ref.txt +14
-0+ + for (i = 0; i < used_atom_cnt; i++) { + struct used_atom *atom = &used_atom[i]; + const char *name = atom->name; + struct atom_value *v = &val[i]; + struct signature_check sigc = { 0 }; + + if (!!deref != (*name == '*')) + continue; + if (deref) + name++; + if (strcmp(name, "signature") && + strcmp(name, "signature:signer") && + strcmp(name, "signature:grade") &&Quoted 8 lines
Documentation/git-for-each-ref.txt symref:: · line 241
Quoted 5 lines
Documentation/git-for-each-ref.txt +5
-0+ strcmp(name, "signature:key") && + strcmp(name, "signature:fingerprint") && + strcmp(name, "signature:primarykeyfingerprint") && + strcmp(name, "signature:trustlevel")) + continue;And with the helper above, we can avoid the repetition here that can go out of sync with the parser function.
I am not sure I have understood this, which helper?
Quoted 6 lines
Documentation/git-for-each-ref.txt symref:: · line 242
Quoted 1 line
Documentation/git-for-each-ref.txt +1
-0+ check_commit_signature(commit, &sigc);If a format asks for signature:signer and signature:key, we shouldn't be running GPG twice. First check used_atom[] to see if we even need to do _any_ signature processing (and leave if there is not), populate the sigc just once and then enter the loop, perhaps?
Yeah, I think it was not right calling
check_commit_signature()in the loop. Populating sigc at once looks more good to meQuoted 6 lines
In adddition, a call to
check_commit_signature()should have amatching call to
signature_check_clear(); otherwise all theresources held by sigc would leak, wouldn't it?
Yeah, it would.
Quoted 144 lines · Junio C Hamano
Quoted 7 lines · "nsengaw4c via GitGitGadget" <[email protected]> writes:
From: Nsengiyumva Wilberforce <[email protected]>
This only works for commits. Add "signature" atom with
grade,signer,key,fingerprint,primarykeyfingerprint,trustlevelas arguments. This code and it's documentation are inspired by how the %GG, %G?, %GS, %GK, %GF, %GP, and %GT pretty formats were implemented.Lacking motivation. Without explaining why somebody may want to have the feature and what it would be used for, "only works for commits" would invite a "so what? does it even have to work?" as a response, so start with a brief descrioption "with the current set of atoms, $this_useful_thing cannot easily be achieved" before describing its limitation.
Having said that, wouldn't it be natural to expect that the same code can deal with signed tags? After all we use the same signature verification machinery at the lowest level in the callchain.
Documentation/git-for-each-ref.txt symref:: · line 219
Quoted 13 lines
Documentation/git-for-each-ref.txt +5
-0@@ -212,6 +212,33 @@ symref:: `:lstrip` and `:rstrip` options in the same way as `refname` above. +signature:: +... +signature:trustlevel:: + The Trust level of the GPG signature of a commit. Possible + outputs are `ultimate`, `fully`, `marginal`, `never` and `undefined`.A good list. How do these work for signature made with a tool other than GPG (in other words, when "gpg.format" is set to something other than "openpgp")?
Quoted 17 lines
Documentation/git-for-each-ref.txt +13
-0@@ -378,6 +383,30 @@ static int subject_atom_parser(struct ref_format *format, struct used_atom *atom return 0; } +static int signature_atom_parser(struct ref_format *format, struct used_atom *atom, + const char *arg, struct strbuf *err) +{ + if (arg) { + if (!strcmp(arg, "signer")) + atom->u.signature.option = S_SIGNER; + else if (!strcmp(arg, "grade")) + atom->u.signature.option = S_GRADE; + else if (!strcmp(arg, "key")) + atom->u.signature.option = S_KEY; + else if (!strcmp(arg, "fingerprint")) + atom->u.signature.option = S_FINGERPRINT; + else if (!strcmp(arg, "primarykeyfingerprint"))Documentation/git-for-each-ref.txt static int subject_atom_parser(struct ref_format *format, struct used_atom *atom · line 408
Quoted 10 lines
Documentation/git-for-each-ref.txt +10
-0+ atom->u.signature.option = S_PRI_KEY_FP; + else if (!strcmp(arg, "trustlevel")) + atom->u.signature.option = S_TRUST_LEVEL; + else + return strbuf_addf_ret(err, -1, _("unknown %%(signature) argument: %s"), arg); + } + else + atom->u.signature.option = S_BARE; + return 0; +}Handing the !arg case first will make the if/else if/... cascade easier to follow, no? Also the body of the function may want to become a separate function that returns one of these
S_FOOconstants.static enum signatore_option signature_atom_parser(...) { enum signature_option opt = parse_signature_option(arg); if (opt < 0) return strbuf_addf_ret(err, opt, _("unknown ..."), arg); return opt; }where
parse_signature_option()would look likestatic enum signature_option parse_signature_option(const char *arg) { if (!arg) return S_BARE; else if (!strcmp(arg, "signer")) return S_SIGNER; ... else return -1; }or something like that?
Documentation/git-for-each-ref.txt static void grab_person(const char *who, struct atom_value *val, int deref, void · line 1377
Quoted 5 lines
Documentation/git-for-each-ref.txt +1
-0@@ -1344,6 +1374,69 @@ static void grab_person(const char *who, struct atom_value *val, int deref, void } } +static void grab_signature(struct atom_value *val, int deref, struct object *obj)To be considerate for future developers, perhaps rename this to
grab_commit_signature(), so that they can addgrab_tag_signature()when they lift the limitation of this implementaiton?Documentation/git-for-each-ref.txt static void grab_person(const char *who, struct atom_value *val, int deref, void · line 1380
Quoted 3 lines
Documentation/git-for-each-ref.txt +3
-0+{ + int i; + struct commit *commit = (struct commit *) obj;Style? No SP between cast and value?
Quoted 9 lines
Documentation/git-for-each-ref.txt +9
-0+ + for (i = 0; i < used_atom_cnt; i++) { + struct used_atom *atom = &used_atom[i]; + const char *name = atom->name; + struct atom_value *v = &val[i]; + struct signature_check sigc = { 0 }; + + if (!!deref != (*name == '*')) + continue;Documentation/git-for-each-ref.txt static void grab_person(const char *who, struct atom_value *val, int deref, void · line 1399
Quoted 10 lines
Documentation/git-for-each-ref.txt +10
-0+ if (deref) + name++; + if (strcmp(name, "signature") && + strcmp(name, "signature:signer") && + strcmp(name, "signature:grade") && + strcmp(name, "signature:key") && + strcmp(name, "signature:fingerprint") && + strcmp(name, "signature:primarykeyfingerprint") && + strcmp(name, "signature:trustlevel")) + continue;And with the helper above, we can avoid the repetition here that can go out of sync with the parser function.
Documentation/git-for-each-ref.txt static void grab_person(const char *who, struct atom_value *val, int deref, void · line 1400
Quoted 1 line
Documentation/git-for-each-ref.txt +1
-0+ check_commit_signature(commit, &sigc);If a format asks for signature:signer and signature:key, we shouldn't be running GPG twice. First check used_atom[] to see if we even need to do _any_ signature processing (and leave if there is not), populate the sigc just once and then enter the loop, perhaps?
In adddition, a call to
check_commit_signature()should have a matching call tosignature_check_clear(); otherwise all the resources held by sigc would leak, wouldn't it?
Quoted 5 lines · On Tue, Dec 27, 2022 at 12:55:23AM +0000, nsengaw4c via GitGitGadget wrote:
This only works for commits. Add "signature" atom with
grade,signer,key,fingerprint,primarykeyfingerprint,trustlevelas arguments. This code and it's documentation are inspired by how the %GG, %G?, %GS, %GK, %GF, %GP, and %GT pretty formats were implemented.
I don't have a real review for you, but rather two small requests since I was working in this area recently.
Review static int subject_atom_parser(struct ref_format *format, struct used_atom *atom · line 387
Quoted 6 lines
(quoted diff) +2
-0@@ -378,6 +383,30 @@ static int subject_atom_parser(struct ref_format *format, struct used_atom *atom return 0; } +static int signature_atom_parser(struct ref_format *format, struct used_atom *atom, + const char *arg, struct strbuf *err)
Can you squash in an annotation for the unused parameter, like this:
ref-filter.c +3 -2
@@ -405,8 +405,9 @@ static int subject_atom_parser(struct ref_format *format UNUSED,return 0 ; } -static int signature_atom_parser (struct ref_format *format,struct used_atom *atom, -const char *arg,struct strbuf *err) +static int signature_atom_parser(struct ref_format *format UNUSED, +struct used_atom *atom, +const char *arg,struct strbuf *err) {if (arg) {if (!strcmp (arg,"signer" ))
This will eventually be necessary once we turn on -Wunused-parameter. I'm preparing a patch to convert all of the other parsers that need it, and I don't want to create a dependency between the two patches (it's OK for you to add the UNUSED now, it's just not enforced yet).
I can also circle back after your patch is merged and add it, but it's a bit easier to do it up front.
Quoted 7 lines
(quoted diff) +7
-0+{ + if (arg) { + if (!strcmp(arg, "signer")) + atom->u.signature.option = S_SIGNER; + else if (!strcmp(arg, "grade")) + atom->u.signature.option = S_GRADE; + else if (!strcmp(arg, "key"))
Review static int subject_atom_parser(struct ref_format *format, struct used_atom *atom · line 404
Quoted 10 lines
(quoted diff) +10
-0+ atom->u.signature.option = S_KEY; + else if (!strcmp(arg, "fingerprint")) + atom->u.signature.option = S_FINGERPRINT; + else if (!strcmp(arg, "primarykeyfingerprint")) + atom->u.signature.option = S_PRI_KEY_FP; + else if (!strcmp(arg, "trustlevel")) + atom->u.signature.option = S_TRUST_LEVEL; + else + return strbuf_addf_ret(err, -1, _("unknown %%(signature) argument: %s"), arg); + }
The ref-filter code recently got a helper function to report this kind of argument error consistently, via dda4fc1a84 (ref-filter: factor out "unrecognized %(foo) arg" errors, 2022-12-14). If you rebase the patch on the current master, you can just do:
return err_bad_arg(err, "signature", arg);
which will make the error message match the others (which in turn saves work for translators).
-Peff
Quoted 18 lines
ref-filter.c +3
-2@@ -405,8 +405,9 @@ static int subject_atom_parser(struct ref_format *format UNUSED,return 0 ; } -static int signature_atom_parser (struct ref_format *format,struct used_atom *atom, -const char *arg,struct strbuf *err) +static int signature_atom_parser(struct ref_format *format UNUSED, +struct used_atom *atom, +const char *arg,struct strbuf *err) {if (arg) {if (!strcmp (arg,"signer" ))
Quoted 7 lines
This will eventually be necessary once we turn on -Wunused-parameter. I'm preparing a patch to convert all of the other parsers that need it, and I don't want to create a dependency between the two patches (it's OK for you to add the UNUSED now, it's just not enforced yet).
I can also circle back after your patch is merged and add it, but it's a bit easier to do it up front.
Thanks, worked on it
Quoted 7 lines
ref-filter.c +7
-0+{ +if (arg) { +if (!strcmp (arg,"signer" )) + atom->u.signature.option =S_SIGNER ; +else if (!strcmp (arg,"grade" )) + atom->u.signature.option =S_GRADE ; +else if (!strcmp (arg,"key" ))
ref-filter.c
Quoted 10 lines
ref-filter.c +10
-0+ atom->u.signature.option =S_KEY ; + else if (!strcmp(arg, "fingerprint")) + atom->u.signature.option =S_FINGERPRINT ; + else if (!strcmp(arg, "primarykeyfingerprint")) + atom->u.signature.option =S_PRI_KEY_FP ; + else if (!strcmp(arg, "trustlevel")) + atom->u.signature.option =S_TRUST_LEVEL ; + else + return strbuf_addf_ret(err, -1, _("unknown %%(signature) argument: %s"), arg); + }
The ref-filter code recently got a helper function to report this kind of argument error consistently, via dda4fc1a84 (ref-filter: factor out "unrecognized %(foo) arg" errors, 2022-12-14). If you rebase the patch on the current master, you can just do:
return err_bad_arg(err, "signature", arg);
which will make the error message match the others (which in turn saves
Quoted 2 lines
work for translators).
Thanks for this, I have seen it too
Quoted 75 lines · Jeff King
Quoted 5 lines · On Tue, Dec 27, 2022 at 12:55:23AM +0000, nsengaw4c via GitGitGadget wrote:
This only works for commits. Add "signature" atom with
grade,signer,key,fingerprint,primarykeyfingerprint,trustlevelas arguments. This code and it's documentation are inspired by how the %GG, %G?, %GS, %GK, %GF, %GP, and %GT pretty formats were implemented.I don't have a real review for you, but rather two small requests since I was working in this area recently.
ref-filter.c static int subject_atom_parser(struct ref_format *format, struct used_atom *atom · line 387
Quoted 6 lines
ref-filter.c +2
-0@@ -378,6 +383,30 @@ static int subject_atom_parser(struct ref_format *format, struct used_atom *atomreturn 0 ; } +static int signature_atom_parser (struct ref_format *format,struct used_atom *atom, +const char *arg,struct strbuf *err)Can you squash in an annotation for the unused parameter, like this:
ref-filter.c +3
-2@@ -405,8 +405,9 @@ static int subject_atom_parser(struct ref_format *format UNUSED,return 0 ; } -static int signature_atom_parser (struct ref_format *format,struct used_atom *atom, -const char *arg,struct strbuf *err) +static int signature_atom_parser(struct ref_format *format UNUSED, +struct used_atom *atom, +const char *arg,struct strbuf *err) {if (arg) {if (!strcmp (arg,"signer" ))This will eventually be necessary once we turn on -Wunused-parameter. I'm preparing a patch to convert all of the other parsers that need it, and I don't want to create a dependency between the two patches (it's OK for you to add the UNUSED now, it's just not enforced yet).
I can also circle back after your patch is merged and add it, but it's a bit easier to do it up front.
Quoted 7 lines
(quoted diff) +7
-0+{ + if (arg) { + if (!strcmp(arg, "signer")) + atom->u.signature.option = S_SIGNER; + else if (!strcmp(arg, "grade")) + atom->u.signature.option = S_GRADE; + else if (!strcmp(arg, "key"))Review static int subject_atom_parser(struct ref_format *format, struct used_atom *atom · line 404
Quoted 10 lines
(quoted diff) +10
-0+ atom->u.signature.option = S_KEY; + else if (!strcmp(arg, "fingerprint")) + atom->u.signature.option = S_FINGERPRINT; + else if (!strcmp(arg, "primarykeyfingerprint")) + atom->u.signature.option = S_PRI_KEY_FP; + else if (!strcmp(arg, "trustlevel")) + atom->u.signature.option = S_TRUST_LEVEL; + else + return strbuf_addf_ret(err, -1, _("unknown %%(signature) argument: %s"), arg); + }The ref-filter code recently got a helper function to report this kind of argument error consistently, via
dda4fc1a84(ref-filter: factor out "unrecognized %(foo) arg" errors, 2022-12-14). If you rebase the patch on the current master, you can just do:return err_bad_arg(err, "signature", arg);which will make the error message match the others (which in turn saves work for translators).
-Peff
v3 cover ref-filter: add new "signature" atom
In reply to nsengaw4c via GitGitGadgetThis commit duplicates the code for signature atom from pretty.c to ref-filter.c. This feature will help to get rid of current duplicate implementation of signature atom when unifying implementations by using ref-filter logic everywhere when ref-filter can do everything pretty is doing.
Add "signature" atom with grade, signer, key, fingerprint, primarykeyfingerprint, trustlevel as arguments. This code and its documentation are inspired by how the %GG, %G?, %GS, %GK, %GF, %GP, and %GT pretty formats were implemented.
Nsengiyumva Wilberforce (1):
ref-filter: add new "signature" atom
Documentation/git-for-each-ref.txt | 27 +++++++ ref-filter.c | 101 +++++++++++++++++++++++++ t/t6300-for-each-ref.sh | 116 +++++++++++++++++++++++++++++ 3 files changed, 244 insertions(+)
Range-diff against v2:
1: ce51d8e79e = 1: ce51d8e79e ref-filter: add new "signature" atom
Signature · 1 line
2.39.0.138.gb334f1a8b9
PATCH v3 1/1 ref-filter: add new "signature" atom
This commit duplicates the code for signature atom from pretty.c to ref-filter.c. This feature will help to get rid of current duplicate implementation of signature atom when unifying implementations by using ref-filter logic everywhere when ref-filter can do everything pretty is doing.
Add "signature" atom with grade, signer, key, fingerprint, primarykeyfingerprint, trustlevel as arguments. This code and its documentation are inspired by how the %GG, %G?, %GS, %GK, %GF, %GP, and %GT pretty formats were implemented.
--- Documentation/git-for-each-ref.txt | 27 +++++++ ref-filter.c | 101 +++++++++++++++++++++++++ t/t6300-for-each-ref.sh | 116 +++++++++++++++++++++++++++++ 3 files changed, 244 insertions(+)
Documentation/git-for-each-ref.txt +27 -0
@@ -212,6 +212,33 @@ symref:: `:lstrip` and `:rstrip` options in the same way as `refname` above. +signature:: + The GPG signature of a commit. + +signature:grade:: + Show "G" for a good (valid) signature, "B" for a bad + signature, "U" for a good signature with unknown validity, "X" + for a good signature that has expired, "Y" for a good + signature made by an expired key, "R" for a good signature + made by a revoked key, "E" if the signature cannot be + checked (e.g. missing key) and "N" for no signature. + +signature:signer:: + The signer of the GPG signature of a commit. + +signature:key:: + The key of the GPG signature of a commit. + +signature:fingerprint:: + The fingerprint of the GPG signature of a commit. + +signature:primarykeyfingerprint:: + The Primary Key fingerprint of the GPG signature of a commit. + +signature:trustlevel:: + The Trust level of the GPG signature of a commit. Possible + outputs are `ultimate`, `fully`, `marginal`, `never` and `undefined`. + worktreepath:: The absolute path to the worktree in which the ref is checked out, if it is checked out in any linked worktree. Empty string
ref-filter.c +101 -0
137 lines not on this page · raw message
t/t6300-for-each-ref.sh +116 -0
126 lines not on this page · raw message
Signature · 1 line
2.39.0.138.gb334f1a8b9
v4 cover ref-filter: add new "signature" atom
This patch is not different from previous version(3), this is because the previous version(3) could not show the difference from version(2) of the patch. I will explain every change from the first version
<https://public-inbox.org/git/[email protected]/>
to version 3
<https://public-inbox.org/git/[email protected]/>
because I first had a big trouble in transitioning from gitgitgadget to using git send-mail
***THE FOLLOWING ARE THE CHANGES***
a) From the first version to version 2
<https://public-inbox.org/git/[email protected]/>
Version 2 addresses all Junio's comments for version 1, the comments are here.
<https://public-inbox.org/git/[email protected]/>
->summary of the changes
i) I changed the commit message to detail more about the feature I am introducing.
ii) Introduced a new helper function in ref-filter.c called parse_signature_option() and handled !arg case first.
iii) Used the above helper function to eliminate the repetition that was in grab_signature() for checking different signature option. iv) I also moved check_commit_signature(commit, &sigc) out of the to avoid running GPG twice.still this change is in grab_signature() in ref-filter.c.
v) add a new test in t6300 to test bare signature atom(%(signature)) since I had missed it
NB: I did not change the parser function name as he suggested, I think my commit message was misleading.
b) from version 2 to version 3
<https://public-inbox.org/git/[email protected]/>
->summary of changes
i) Got rid of test for bare signature atom. This is because the test was passing for some CI tests(different machines) and some others were failing.
Best Regards,
Wilberforce
Nsengiyumva Wilberforce (1):
ref-filter: add new "signature" atom
Documentation/git-for-each-ref.txt | 27 +++++++ ref-filter.c | 101 +++++++++++++++++++++++++ t/t6300-for-each-ref.sh | 116 +++++++++++++++++++++++++++++ 3 files changed, 244 insertions(+)
Range-diff against v3:
1: ce51d8e79e = 1: ce51d8e79e ref-filter: add new "signature" atom
Signature · 1 line
2.39.0.138.gb334f1a8b9
PATCH v4 1/1 ref-filter: add new "signature" atom
This commit duplicates the code for signature atom from pretty.c to ref-filter.c. This feature will help to get rid of current duplicate implementation of signature atom when unifying implementations by using ref-filter logic everywhere when ref-filter can do everything pretty is doing.
Add "signature" atom with grade, signer, key, fingerprint, primarykeyfingerprint, trustlevel as arguments. This code and its documentation are inspired by how the %GG, %G?, %GS, %GK, %GF, %GP, and %GT pretty formats were implemented.
--- Documentation/git-for-each-ref.txt | 27 +++++++ ref-filter.c | 101 +++++++++++++++++++++++++ t/t6300-for-each-ref.sh | 116 +++++++++++++++++++++++++++++ 3 files changed, 244 insertions(+)
Documentation/git-for-each-ref.txt +27 -0
@@ -212,6 +212,33 @@ symref:: `:lstrip` and `:rstrip` options in the same way as `refname` above. +signature:: + The GPG signature of a commit. + +signature:grade:: + Show "G" for a good (valid) signature, "B" for a bad + signature, "U" for a good signature with unknown validity, "X" + for a good signature that has expired, "Y" for a good + signature made by an expired key, "R" for a good signature + made by a revoked key, "E" if the signature cannot be + checked (e.g. missing key) and "N" for no signature. + +signature:signer:: + The signer of the GPG signature of a commit. + +signature:key:: + The key of the GPG signature of a commit. + +signature:fingerprint:: + The fingerprint of the GPG signature of a commit. + +signature:primarykeyfingerprint:: + The Primary Key fingerprint of the GPG signature of a commit. + +signature:trustlevel:: + The Trust level of the GPG signature of a commit. Possible + outputs are `ultimate`, `fully`, `marginal`, `never` and `undefined`. + worktreepath:: The absolute path to the worktree in which the ref is checked out, if it is checked out in any linked worktree. Empty string
ref-filter.c +101 -0
137 lines not on this page · raw message
t/t6300-for-each-ref.sh +116 -0
126 lines not on this page · raw message
Signature · 1 line
2.39.0.138.gb334f1a8b9
v5 cover ref-filter: add new "signature" atom
This patch addresses Junio's comment here<https://public-inbox.org/git/[email protected]/>. I introduced an integer flag called signature_checked to cater for this.
Also, the fact that "test bare signature atom" test was failing on different machines(CI on github) due to trustdb, I introduced a hack to cater for this.Something like this,grep -v "checking the trustdb" out_orig >out &&
Nsengiyumva Wilberforce (1):
ref-filter: add new "signature" atom
Documentation/git-for-each-ref.txt | 27 ++++++ ref-filter.c | 106 ++++++++++++++++++++++++ t/t6300-for-each-ref.sh | 128 +++++++++++++++++++++++++++++ 3 files changed, 261 insertions(+)
Range-diff against v4:
1: d9ce65f411 = 1: d9ce65f411 ref-filter: add new "signature" atom
Signature · 1 line
2.39.GIT
PATCH v5 1/1 ref-filter: add new "signature" atom
This commit duplicates the code for signature atom from pretty.c to ref-filter.c. This feature will help to get rid of current duplicate implementation of signature atom when unifying implementations by using ref-filter logic everywhere when ref-filter can do everything pretty is doing.
Add "signature" atom with grade, signer, key, fingerprint, primarykeyfingerprint, trustlevel as arguments. This code and its documentation are inspired by how the %GG, %G?, %GS, %GK, %GF, %GP, and %GT pretty formats were implemented.
--- Documentation/git-for-each-ref.txt | 27 ++++++ ref-filter.c | 106 ++++++++++++++++++++++++ t/t6300-for-each-ref.sh | 128 +++++++++++++++++++++++++++++ 3 files changed, 261 insertions(+)
Documentation/git-for-each-ref.txt +27 -0
@@ -212,6 +212,33 @@ symref:: `:lstrip` and `:rstrip` options in the same way as `refname` above. +signature:: + The GPG signature of a commit. + +signature:grade:: + Show "G" for a good (valid) signature, "B" for a bad + signature, "U" for a good signature with unknown validity, "X" + for a good signature that has expired, "Y" for a good + signature made by an expired key, "R" for a good signature + made by a revoked key, "E" if the signature cannot be + checked (e.g. missing key) and "N" for no signature. + +signature:signer:: + The signer of the GPG signature of a commit. + +signature:key:: + The key of the GPG signature of a commit. + +signature:fingerprint:: + The fingerprint of the GPG signature of a commit. + +signature:primarykeyfingerprint:: + The Primary Key fingerprint of the GPG signature of a commit. + +signature:trustlevel:: + The Trust level of the GPG signature of a commit. Possible + outputs are `ultimate`, `fully`, `marginal`, `never` and `undefined`. + worktreepath:: The absolute path to the worktree in which the ref is checked out, if it is checked out in any linked worktree. Empty string
ref-filter.c +106 -0
142 lines not on this page · raw message
t/t6300-for-each-ref.sh +128 -0
138 lines not on this page · raw message
Signature · 1 line
2.39.GIT
Re: [PATCH v5 1/1] ref-filter: add new "signature" atom
Quoted 5 lines · Nsengiyumva Wilberforce
Co-authored-by Hariom Verma <[email protected]>Co-authored-by Jaydeep Das <[email protected]>Mentored-by Christian Couder <[email protected]>Mentored-by Hariom Verma <[email protected]>Signed-off-by Nsengiyumva Wilberforce <[email protected]>
https://github.com/git/git/actions/runs/4410308954/jobs/7727652857#step:6:1898
+ git for-each-ref refs/heads/signed --format=%(signature)
+ test_cmp expected actual
+ test 2 -ne 2
+ eval diff -u "$@"
+ diff -u expected actual
--- expected2023-03-13 23:08:51.495987174 +0000
+++ actual2023-03-13 23:08:51.519986953 +0000
@@ -1,5 +1,4 @@
gpg: Signature made Mon Mar 13 23:08:51 2023 UTC
gpg: using DSA key 13B6F51ECDDE430D
-gpg: 3 marginal(s) needed, 1 complete(s) needed, PGP trust model
gpg: Good signature from "C O Mitter <[email protected]>"
error: last command exited with $?=1
not ok 342 - test bare signature atom
+ git for-each-ref refs/heads/signed --format=%(signature)
+ test_cmp expected actual
+ test 2 -ne 2
+ eval diff -u "$@"
+ diff -u expected actual
--- expected
+++ actual
@@ -1,5 +1,4 @@
gpg: Signature made Mon Mar 13 23:08:51 2023 UTC
gpg: using DSA key 13B6F51ECDDE430D
-gpg: 3 marginal(s) needed, 1 complete(s) needed, PGP trust model
gpg: Good signature from "C O Mitter <[email protected]>"
error: last command exited with $?=1
not ok 342 - test bare signature atom
Are there differences between versions of GPG that, for the purpose of the testing, we do not care?
Re: [PATCH v5 1/1] ref-filter: add new "signature" atom
In reply to Nsengiyumva WilberforceQuoted 1 line · Nsengiyumva Wilberforce
+test_expect_success GPG 'test bare signature atom' '
I think this test is failing on CI because, as Junio said, there are different versions of gpg coming into play here. In particular, this test is failing on (according to the logs) linux32 (daald/ubuntu32:xenial). The version of GPG that xenial can use is at a maximum v1.4.20 (this is evident here https://packages.ubuntu.com/xenial/allpackages). But according the code in lib-gpg.sh, we should be able to handle any GPG version, except for v1.0.6.
Looking at the logs that Junio posted, I think that the cuplrit is
t/t6300-for-each-ref.sh sig_crlf="$(printf "%s" "$sig" | append_cr; echo dummy)" · line 1478
Quoted 3 lines
t/t6300-for-each-ref.sh +3
-0+ grep -v "checking the trustdb" out_orig >out && + head -3 out >expected && + tail -1 out >>expected &&
but I'm not really sure. So I would really be grateful if you could explain this hack to me, the one you mentioned in the cover-letter.
Thanks
Re: [PATCH v5 1/1] ref-filter: add new "signature" atom
Quoted 9 lines · Kousik Sanagavarapu
Quoted 1 line
+test_expect_success GPG 'test bare signature atom' '
I think this test is failing on CI because, as Junio said, there are different versions of gpg coming into play here. In particular, this test is failing on (according to the logs) linux32 (daald/ubuntu32:xenial). The version of GPG that xenial can use is at a maximum v1.4.20 (this is evident here https://packages.ubuntu.com/xenial/allpackages). But according the code in
lib-gpg.sh, we should be able to handle any GPG version, except for v1.0.6.
After digging a little bit more into this, I found that when checking the trustdb, the output is different for GPG v1 and GPG v2. So, I understand now why you did this
Quoted 1 line
t/t6300-for-each-ref.sh sig_crlf="$(printf "%s" "$sig" | append_cr; echo dummy)" · line 1476
Quoted 1 line
t/t6300-for-each-ref.sh +1
-0+ grep -v "checking the trustdb" out_orig >out &&
The test is still failing because this is not the only line in the output that is different, but also the line following it, which conveys information about the key trust levels
gpg: 3 marginal(s) needed, 1 complete(s) needed, PGP trust model
This is also evident from the diff between "expected" and "actual" in the logs[1].
[1] https://github.com/git/git/actions/runs/4410308954/jobs/7727652857#step:6:1898
Thanks
Re: [PATCH v4 0/1] ref-filter: add new "signature" atom
In reply to Nsengiyumva WilberforceQuoted 3 lines · Nsengiyumva Wilberforce
iv) I also moved
check_commit_signature(commit, &sigc)out of the to avoid running GPG twice.still this change is ingrab_signature()inref-filter.c.
Is there some code to avoid calling check_commit_signature() or grab_signature() itself when no %(signature) atoms is used? When nobody asked to use signature atom at all, calling GPG once is already once too many.
Thanks.
Re: [PATCH v3 0/1] ref-filter: add new "signature" atom
In reply to Nsengiyumva WilberforceQuoted 7 lines · Nsengiyumva Wilberforce
Documentation/git-for-each-ref.txt | 27 +++++++ ref-filter.c | 101 +++++++++++++++++++++++++ t/t6300-for-each-ref.sh | 116 +++++++++++++++++++++++++++++ 3 files changed, 244 insertions(+)Range-diff against v2:
1:ce51d8e79e= 1:ce51d8e79eref-filter: add new "signature" atom
It's strange that it's saying nothing changed compared to v2. I guess the v2 it's talking about is not actually the v2 you sent to the list, right?
Anyway it's better if you can describe a bit with your own words in the cover letter what changed, and sometimes even what didn't change, since the previous version you sent.
For example, here you can say that you decided to actually remove the test that checked the %(signature) format and explain a bit the reason why you did that.
You can also send links in the cover letter to the branch(es) (on GitHub, GitLab or other such platforms) that you used for this and previous versions of the patch (or patch series). It can help people try it and look at the changes on their own system.
Thanks.