On 29.07.21 02:58, Junio C Hamano wrote:
Jonathan Tan <jonathantanmy@xxxxxxxxxx> writes:
- fmt = get_format_by_sig(signature);
- if (!fmt)
- BUG("bad signature '%s'", signature);
Here is the difference in functionality that I spotted. Here, lack of
fmt is fatal...
+ fmt = get_format_by_sig(signature);
+ if (!fmt)
+ return error(_("bad/incompatible signature '%s'"), signature);
...but here it is not.
While I was reviewing this step, I was assumign that the callers
would respond to this error return appropriately. If it is not the
case, then we do have to fix that.
The original's use of BUG() is wrong in any case, I woud think. The
"signature" there is an external input, so we were reporting a data
error (it should have been die()), not a program logic error.
Thanks.
My intention was to actually change this behavior (i should have made
that clear in the commit message). When the current git version
encounters an unknown signature format it will BUG() and leave the user
with a coredump.
I assume that some repos will have multiple different signature formats
in their history in the future and the effect i would have liked was to
only mark the commits with unknown signatures as bad/unknown when using
git log --show-signature for example.
I have checked all the calls to check_signature and unfortunately the
result check is really inconsistent. Some ignore it completely, others
check but still then dereference fields from the sigcheck struct.
So for now a die() is probably the correct way to go.
In the future we might want to differentiate between verifying a single
commit (in which case we can die()) and a list of commits. Or fix all
the calls to check_signature to check the return code.
Thanks
