On Mon, Nov 24, 2008 at 12:04:59PM +0100, Johannes Schindelin wrote: > 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@xxxxxxxxx> > > --- > > 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. > > 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@xxxxxxxxx> > > + * Copyright (c) 2008 Deskin Miller <deskinm@xxxxxxxxx> > > 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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
