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

From: Deskin Miller <deskinm@umich.edu>
Date: Nov 28, 2008, 00:18 UTC
Message-ID: <20081128001847.GB29662@euler>
In-Reply-To: <alpine.DEB.1.00.0811241147490.30769@pacific.mpi-cbg.de>

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

Show 20 quoted lines

> Hi,
> 
> On Sun, 23 Nov 2008, Deskin Miller wrote:
> 
> > 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 <deskinm@umich.edu>
> > ---
> >  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.

Show 35 quoted lines

> >  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.
> 
> >   *
> > - * 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.

Show 15 quoted lines

> 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.

Show 13 quoted lines

> 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