From: Junio C Hamano <gitster@pobox•com>
To: santiago@nyu•edu
Cc: git@vger•kernel.org, Jeff King <peff@peff•net>,
Eric Sunshine <sunshine@sunshineco•com>
Subject: Re: [PATCH v5 4/6] builtin/verify-tag: replace name argument with sha1
Date: Wed, 06 Apr 2016 09:27:55 -0700 [thread overview]
Message-ID: <xmqqa8l6zoqc.fsf@gitster.mtv.corp.google.com> (raw)
In-Reply-To: <1459872449-7537-5-git-send-email-santiago@nyu.edu> (santiago@nyu.edu's message of "Tue, 5 Apr 2016 12:07:27 -0400")
santiago@nyu•edu writes:
> From: Santiago Torres <santiago@nyu•edu>
>
> This change is meant to prepare verify_tag for libification. Many
> existing modules/commands already do the refname to sha1 resolution, so
> should avoid resolving the refname twice. To avoid breaking
> builtin/verify-tag, we move the refname resolution outside of the
> verify_tag() call.
>
> Signed-off-by: Santiago Torres <santiago@nyu•edu>
> ---
> builtin/verify-tag.c | 25 ++++++++++++++++---------
> 1 file changed, 16 insertions(+), 9 deletions(-)
>
> diff --git a/builtin/verify-tag.c b/builtin/verify-tag.c
> index 1ca9a05..7a7c376 100644
> --- a/builtin/verify-tag.c
> +++ b/builtin/verify-tag.c
> @@ -42,25 +42,23 @@ static int run_gpg_verify(const char *buf, unsigned long size, unsigned flags)
> return ret;
> }
>
> -static int verify_tag(const char *name, unsigned flags)
> +static int verify_tag(const unsigned char *sha1, unsigned flags)
> {
> enum object_type type;
> - unsigned char sha1[20];
> char *buf;
> + char *hex_sha1;
> unsigned long size;
> int ret;
>
> - if (get_sha1(name, sha1))
> - return error("tag '%s' not found.", name);
> -
> + hex_sha1 = sha1_to_hex(sha1);
> type = sha1_object_info(sha1, NULL);
> if (type != OBJ_TAG)
> return error("%s: cannot verify a non-tag object of type %s.",
> - name, typename(type));
> + hex_sha1, typename(type));
So, if I said
$ git verify-tag master
the code used to take "master" from argv[], fed it to verify_tag()
as parameter 'name', turned it to the object name of the commit,
noticed that it is not a tag, and complained that "master: cannot verify".
With this rewrite, the same invocation would cause "master" to be
turned into the object name, which is passed to verify_tag() and the
complaint is an overlong
76bece327f490cb344b95ae8f869cbeb89a4d20b: cannot verify a non-tag object of type commit
That does not sound like a good change at all.
If you want to support a future caller of a libified version of
verify_tag() that has a raw object name but not the original name,
I'd suggest to make this function keep parameter 'name' while adding
the new parameter 'sha1'. Then, the error reporting may become:
return error("%s: cannot verify a non-tag object of type '%s'",
name ? name : sha1_to_hex(sha1), typename(type));
and the output would still be useful. Further improvements may be
- rename 'name' to 'report_name' to clarify that the parameter is
only used for reporting, and that the tag object to verify is
always identified by the new 'sha1' parameter.
- use find_unique_abbrev() to shorten the fallback name displayed
in the error message.
next prev parent reply other threads:[~2016-04-06 16:28 UTC|newest]
Thread overview: 28+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-04-05 16:07 [PATCH v5 0/6] tag: move PGP verification code to tag.c santiago
2016-04-05 16:07 ` [PATCH v5 1/6] builtin/verify-tag.c: Ignore SIGPIPE on gpg-interface santiago
2016-04-06 16:43 ` Junio C Hamano
2016-04-05 16:07 ` [PATCH v5 2/6] t7030-verify-tag: Adds validation for multiple tags santiago
2016-04-06 6:21 ` Eric Sunshine
2016-04-17 17:31 ` Santiago Torres
2016-04-17 18:19 ` Eric Sunshine
2016-04-17 18:38 ` Santiago Torres
2016-04-17 18:53 ` Eric Sunshine
2016-04-06 16:45 ` Junio C Hamano
2016-04-05 16:07 ` [PATCH v5 3/6] builtin/verify-tag: change variable name for readability santiago
2016-04-06 16:46 ` Junio C Hamano
2016-04-05 16:07 ` [PATCH v5 4/6] builtin/verify-tag: replace name argument with sha1 santiago
2016-04-06 6:56 ` Eric Sunshine
2016-04-06 16:27 ` Junio C Hamano [this message]
2016-04-07 3:38 ` Santiago Torres
2016-04-05 16:07 ` [PATCH v5 5/6] builtin/verify-tag: move verification code to tag.c santiago
2016-04-06 16:33 ` Junio C Hamano
2016-04-05 16:07 ` [PATCH v5 6/6] tag: use gpg_verify_function in tag -v call santiago
2016-04-06 7:09 ` Eric Sunshine
2016-04-05 16:44 ` [PATCH v5 0/6] tag: move PGP verification code to tag.c Santiago Torres
2016-04-06 7:18 ` Eric Sunshine
2016-04-07 3:40 ` Santiago Torres
2016-04-07 16:19 ` Eric Sunshine
2016-04-17 17:34 ` Santiago Torres
2016-04-17 18:14 ` Eric Sunshine
2016-04-06 16:39 ` Junio C Hamano
2016-04-07 3:33 ` Santiago Torres
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=xmqqa8l6zoqc.fsf@gitster.mtv.corp.google.com \
--to=gitster@pobox$(echo .)com \
--cc=git@vger$(echo .)kernel.org \
--cc=peff@peff$(echo .)net \
--cc=santiago@nyu$(echo .)edu \
--cc=sunshine@sunshineco$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox