Re: [PATCH v5 1/5] Move commit GPG signature verification to commit.c

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 30, 2013, 03:37 UTC
Message-ID: <7v38vdlfr1.fsf@alter.siamese.dyndns.org>
In-Reply-To: <51562E4E.7070605@physik.tu-berlin.de>

Sebastian Götte <jaseg@physik.tu-berlin.de> writes:

Show 10 quoted lines

> @@ -230,4 +231,12 @@ extern void print_commit_list(struct commit_list *list,
>  			      const char *format_cur,
>  			      const char *format_last);
>  
> +/*
> + * Check the signature of the given commit. The result of the check is stored in
> + * sig->check_result, 'G' for a good signature, 'B' for a bad signature and 'N'
> + * for no signature at all.
> + * This may allocate memory for sig->gpg_output, sig->gpg_status, sig->signer and sig->key.
> + */

How wide is your terminal? These lines are a tad wider than our standard. We tend to keep function decls in *.h files on a single long line (primarily to help people who grep them without using CTAGS/ETAGS), but everywhere else we try to keep most of our lines comfortably fit on 80-column terminals.

Show 15 quoted lines

> +extern void check_commit_signature(const struct commit* commit, struct signature_check *sigc);
> +
>  #endif /* COMMIT_H */
> diff --git a/gpg-interface.h b/gpg-interface.h
> index cf99021..44f70aa 100644
> --- a/gpg-interface.h
> +++ b/gpg-interface.h
> @@ -1,6 +1,14 @@
>  #ifndef GPG_INTERFACE_H
>  #define GPG_INTERFACE_H
>  
> +struct signature_check {
> +	char *gpg_output;
> +	char *gpg_status;
> +	char check_result; /* 0 (not checked), N (checked but no further result), G (good) or B (bad) */

Listing the possible values is a good idea, but not on a single line.

Also now the structure screams with its name that it is about checking, I do not see a reason for its field to repeat "check". Calling it "result" (or "result_code") would avoid stuttering when you use them, e.g.

	struct signature_check signature_check;

	switch (signature_check.check_result) {
        ...

would be less nice than

        switch (signature_check.result) {
	...

no?