Re: [RFC PATCH 1/4] Refactor builtin-verify-tag.c

On Mon, Nov 24, 2008 at 12:04:59PM +0100, Johannes Schindelin wrote:

Hi,

On Sun, 23 Nov 2008, Deskin Miller wrote:

quoted

builtin-verify-tag.c didn't expose any of its functionality to be used
internally.  Refactor some of it into new verify-tag.c and expose
verify_tag_sha1 able to be called from elsewhere in git.

Signed-off-by: Deskin Miller <redacted>
---
 Makefile             |    2 +
 builtin-verify-tag.c |   61 ++-------------------------------------
 verify-tag.c         |   77 ++++++++++++++++++++++++++++++++++++++++++++++++++
 verify-tag.h         |   10 ++++++
 4 files changed, 93 insertions(+), 57 deletions(-)
 create mode 100644 verify-tag.c
 create mode 100644 verify-tag.h

I'll comment on the output of "format-patch -n -C -C" instead, as that 
makes it much easier to see what you actually did:

Didn't realise -C -C was the magic incantation; I'll remember it for the
future.

quoted

 Makefile                             |    2 +
 builtin-verify-tag.c                 |   61 ++-------------------------------
 builtin-verify-tag.c => verify-tag.c |   48 ++++-----------------------
 verify-tag.h                         |   10 +++++
 4 files changed, 23 insertions(+), 98 deletions(-)
 copy builtin-verify-tag.c => verify-tag.c (56%)
 create mode 100644 verify-tag.h

[...] 

diff --git a/builtin-verify-tag.c b/verify-tag.c
similarity index 56%
copy from builtin-verify-tag.c
copy to verify-tag.c
index 729a159..c9be331 100644
--- a/builtin-verify-tag.c
+++ b/verify-tag.c

@@ -1,18 +1,12 @@
 /*
- * Builtin "git verify-tag"
+ * Internals for "git verify-tag"

Agree.

quoted

  *
- * Copyright (c) 2007 Carlos Rica <jasampler@gmail.com>
+ * Copyright (c) 2008 Deskin Miller <deskinm@umich.edu>

Disagree.

Even if Carlos seemed to stop his work on Git entirely, which I find 
disappointing, you are _not_ free to pretend his work is yours.  And given 
this diff:
[...] 
I think pretty much all you did was deleting (and thereby you do not gain 
any copyright).

I realised my mistake in altering the copyright information just after
sending out these patches.  I think I'd written the header first in
verify-tag.c before copying the code in; though I couldn't say what I
thought I'd be writing that would end up protected by copyright.  At any
rate, it was an honest mistake, and I apologise, Carlos, for my
unintended plagarism; I'll be sure to restore the proper copyright
notice for any subsequent versions.

Except for one change: why on earth did you think it a good idea to 
suppress telling the user the _name_ of the tag when an error occurs?

I, for one, would find it way less than helpful to read

	Cannot verify a non-tag object of type blob.

than to read

	refs/tags/dscho-key: cannot verify a non-tag object of type blob.

Besides, I do not see where you warn that "tag <name> not found."  Changes 
like this one need to be justified (by saying in the commit message where 
the warning is already issued, and not letting the reviewer/reader leave 
wondering).

The verify_tag_sha1 function is newly exposed to the rest of git, and
has a different signature from verify_tag, which could take a ref while
verify_tag_sha1 takes a sha1.  verify_tag still includes both the checks
you refer to before calling verify_tag_sha1, so the error output is
identical in all cases before and after applying this patch.

The OBJ_TAG check, however, is duplicated so that internal git calls to
verify_tag_sha1 can't pass in e.g. a blob sha1 which just happens to
contain the same contents as a signed tag.

Actually, I initially did not leave the OBJ_TAG check in verify_tag, but
relied on it checking the return value of verify_tag_sha1 to see if an
error occurred, and printing 'Failed to verify <name>' in that case, for
precisely the reason you point out, that the ref name is very useful in
this failure case.  However, I ultimately decided to duplicate the check
so that the error output would match up exactly.
 

Please, next time you submit a patch like this, do the -C -C yourself.  
Letting all the reviewers do it looks lousy on the overall time balance 
sheet, and it may also lead to a potential reviewer preferring to do 
something else instead.

Will do; thanks for reviewing in spite of my shortcomings.

Now, Junio already said that he is not (yet) convinced that this change 
should be in Git proper, rather than a hook, so it is up to you to decide 
if you deem it important enough to try harder to convince people.

I, for one, would think that it may be a good change: AFAIK only hard-core 
gits use hooks, everybody else avoids them.  So if we deem verifying 
signatures important enough, we might want to have better support for it 
than some example hooks.

So color me half-convinced.

Ciao,
Dscho

Deskin Miller