Re: [PATCH v3 4/4] tag: use pgp_verify_function in tag -v call

[Jeff King]

On Mon, Apr 04, 2016 at 02:24:48PM -0400, Santiago Torres wrote:

> > I think it can be part of this series, but doesn't have to be. As I
> > understand it, the current code is just handing the name to the `git
> > verify-tag` process, so if we continue to do so, that would be OK.
> 
> IIRC, the current code for git tag -v hands the hex-representation[1] of
> the sha1 to git verify-tag --- I believe that's related to the
> desamgibuation issue I've seen people discuss.  I think this behavior is
> lost unless we add this on top of the patch.

Oh, you're right. I didn't notice that. So yeah, we should make sure in
this series to hand the sha1 over to gpg_verify_tag().

> > Yes, though I'd generally advise against a function taking either a name or
> > a sha1, and ignoring the other option. That often leads to confusing
> > interfaces for the callers. Instead, perhaps just take the sha1, and let
> > the caller do the get_sha1() themselves. Or possibly provide two
> > functions, one of which is a convenience to translate the name to sha1
> > and then call the other.
> 
> I think the former sounds easier. I can replace the name argument and
> move the sha1-resolution code to in verify-tag. git tag -v already
> resolves the tagname to a sha1, so it is easier there.
> 
> Does this sound reasonable?

Yes, I think that is a good solution.

Thanks.

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 4/4] tag: use pgp_verify_function in tag -v call

[Santiago Torres]

On Mon, Apr 04, 2016 at 09:38:54AM -0400, Jeff King wrote:
> On Mon, Apr 04, 2016 at 12:12:04AM -0400, Santiago Torres wrote:
> 
> > > As a side note, it might actually be an improvement for pgp_verify_tag
> > > to take a sha1 (so that git-tag is sure that it is verifying the same
> > > object that it is printing), but that refactoring should probably come
> > > separately, I think.
> > 
> > Just to be sure, this refactoring is something we should still include
> > in this set of patches, right? I think that otherwise we'd lose the
> > desambigutaion that git tag -v does in this patch.
> 
> I think it can be part of this series, but doesn't have to be. As I
> understand it, the current code is just handing the name to the `git
> verify-tag` process, so if we continue to do so, that would be OK.

IIRC, the current code for git tag -v hands the hex-representation[1] of
the sha1 to git verify-tag --- I believe that's related to the
desamgibuation issue I've seen people discuss.  I think this behavior is
lost unless we add this on top of the patch.

> 
> > I also think that most of the rippling is gone if we use and adaptor as
> > you suggested. Should I add a patch on top of this to support a sha1 as
> > part for gpg_verify_tag()?
> 
> Yes, though I'd generally advise against a function taking either a name or
> a sha1, and ignoring the other option. That often leads to confusing
> interfaces for the callers. Instead, perhaps just take the sha1, and let
> the caller do the get_sha1() themselves. Or possibly provide two
> functions, one of which is a convenience to translate the name to sha1
> and then call the other.

I think the former sounds easier. I can replace the name argument and
move the sha1-resolution code to in verify-tag. git tag -v already
resolves the tagname to a sha1, so it is easier there.

Does this sound reasonable? 

Thanks!
-Santiago

[1] https://git.kernel.org/cgit/git/git.git/tree/builtin/tag.c#n109
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 4/4] tag: use pgp_verify_function in tag -v call

[Jeff King]

On Mon, Apr 04, 2016 at 12:12:04AM -0400, Santiago Torres wrote:

> > As a side note, it might actually be an improvement for pgp_verify_tag
> > to take a sha1 (so that git-tag is sure that it is verifying the same
> > object that it is printing), but that refactoring should probably come
> > separately, I think.
> 
> Just to be sure, this refactoring is something we should still include
> in this set of patches, right? I think that otherwise we'd lose the
> desambigutaion that git tag -v does in this patch.

I think it can be part of this series, but doesn't have to be. As I
understand it, the current code is just handing the name to the `git
verify-tag` process, so if we continue to do so, that would be OK.

> I also think that most of the rippling is gone if we use and adaptor as
> you suggested. Should I add a patch on top of this to support a sha1 as
> part for gpg_verify_tag()?

Yes, though I'd generally advise against a function taking either a name or
a sha1, and ignoring the other option. That often leads to confusing
interfaces for the callers. Instead, perhaps just take the sha1, and let
the caller do the get_sha1() themselves. Or possibly provide two
functions, one of which is a convenience to translate the name to sha1
and then call the other.

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 4/4] tag: use pgp_verify_function in tag -v call

[Santiago Torres]

> > diff --git a/builtin/verify-tag.c b/builtin/verify-tag.c
> > index f776778..8abc357 100644
> > --- a/builtin/verify-tag.c
> > +++ b/builtin/verify-tag.c
> > @@ -30,6 +30,8 @@ int cmd_verify_tag(int argc, const char **argv, const 
> > char *prefix)
> >  {
> > int i = 1, verbose = 0, had_error = 0;
> > unsigned flags = 0;
> > +   unsigned char sha1[20];
> > +   const char *name;
> > const struct option verify_tag_options[] = {
> > OPT__VERBOSE(, N_("print tag contents")),
> > OPT_BIT(0, "raw", , N_("print raw gpg status output"), 
> > GPG_VERIFY_RAW),
> > @@ -46,8 +48,16 @@ int cmd_verify_tag(int argc, const char **argv, const 
> > char *prefix)
> > if (verbose)
> > flags |= GPG_VERIFY_VERBOSE;
> >  
> > -   while (i < argc)
> > -   if (pgp_verify_tag(argv[i++], flags))
> > +   while (i < argc) {
> > +   name = argv[i++];
> > +   if (get_sha1(name, sha1)) {
> > +   error("tag '%s' not found.", name);
> > had_error = 1;
> > +   }
> > +
> > +   if (pgp_verify_tag(name, NULL, sha1, flags))
> > +   had_error = 1;
> > +
> > +   }
> 
> So this is a good example of the rippling I mentioned earlier.
> 
> As a side note, it might actually be an improvement for pgp_verify_tag
> to take a sha1 (so that git-tag is sure that it is verifying the same
> object that it is printing), but that refactoring should probably come
> separately, I think.
> 
> -Peff

Just to be sure, this refactoring is something we should still include
in this set of patches, right? I think that otherwise we'd lose the
desambigutaion that git tag -v does in this patch.

I also think that most of the rippling is gone if we use and adaptor as
you suggested. Should I add a patch on top of this to support a sha1 as
part for gpg_verify_tag()?

Thanks!
-Santiago.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 4/4] tag: use pgp_verify_function in tag -v call

[Santiago Torres]

> > if (cmdmode == 'v')
> > -   return for_each_tag_name(argv, verify_tag);
> > +   return for_each_tag_name(argv, pgp_verify_tag,
> > +   GPG_VERIFY_VERBOSE);
> 
> but I think that is coupling too closely. What happens later when the
> public, multi-file pgp_verify_tag function changes its interface? Or we
> want to change our interface here, and it no longer matches
> pgp_verify_tag? The results ripple a lot further than they should.
> 
> I think you probably want to keep a simple adapter callback in this
> file, like:
> 
>   int verify_tag(const char *name, const char *ref, const unsigned char *sha1)
>   {
>   return pgp_verify_tag(name, GPG_VERIFY_VERBOSE));
>   }

Yes, agreed. I'll give this a go

Thanks!
-Santiago

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 4/4] tag: use pgp_verify_function in tag -v call

[Jeff King]

On Sat, Apr 02, 2016 at 07:16:15PM -0400, santi...@nyu.edu wrote:

> diff --git a/builtin/tag.c b/builtin/tag.c
> index 1705c94..3dffdff 100644
> --- a/builtin/tag.c
> +++ b/builtin/tag.c
> @@ -65,9 +65,10 @@ static int list_tags(struct ref_filter *filter, struct 
> ref_sorting *sorting, con
>  }
>  
>  typedef int (*each_tag_name_fn)(const char *name, const char *ref,
> - const unsigned char *sha1);
> + const unsigned char *sha1, unsigned flags);

I'm not sure it's a good idea to add a flags field here; most of the
callbacks don't use it, and as you probably noticed, it makes the patch
a lot noisier. It does let you directly use pgp_verify_tag like this:

>   if (cmdmode == 'v')
> - return for_each_tag_name(argv, verify_tag);
> + return for_each_tag_name(argv, pgp_verify_tag,
> + GPG_VERIFY_VERBOSE);

but I think that is coupling too closely. What happens later when the
public, multi-file pgp_verify_tag function changes its interface? Or we
want to change our interface here, and it no longer matches
pgp_verify_tag? The results ripple a lot further than they should.

I think you probably want to keep a simple adapter callback in this
file, like:

  int verify_tag(const char *name, const char *ref, const unsigned char *sha1)
  {
return pgp_verify_tag(name, GPG_VERIFY_VERBOSE));
  }

> diff --git a/builtin/verify-tag.c b/builtin/verify-tag.c
> index f776778..8abc357 100644
> --- a/builtin/verify-tag.c
> +++ b/builtin/verify-tag.c
> @@ -30,6 +30,8 @@ int cmd_verify_tag(int argc, const char **argv, const char 
> *prefix)
>  {
>   int i = 1, verbose = 0, had_error = 0;
>   unsigned flags = 0;
> + unsigned char sha1[20];
> + const char *name;
>   const struct option verify_tag_options[] = {
>   OPT__VERBOSE(, N_("print tag contents")),
>   OPT_BIT(0, "raw", , N_("print raw gpg status output"), 
> GPG_VERIFY_RAW),
> @@ -46,8 +48,16 @@ int cmd_verify_tag(int argc, const char **argv, const char 
> *prefix)
>   if (verbose)
>   flags |= GPG_VERIFY_VERBOSE;
>  
> - while (i < argc)
> - if (pgp_verify_tag(argv[i++], flags))
> + while (i < argc) {
> + name = argv[i++];
> + if (get_sha1(name, sha1)) {
> + error("tag '%s' not found.", name);
>   had_error = 1;
> + }
> +
> + if (pgp_verify_tag(name, NULL, sha1, flags))
> + had_error = 1;
> +
> + }

So this is a good example of the rippling I mentioned earlier.

As a side note, it might actually be an improvement for pgp_verify_tag
to take a sha1 (so that git-tag is sure that it is verifying the same
object that it is printing), but that refactoring should probably come
separately, I think.

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 4/4] tag: use pgp_verify_function in tag -v call

[santiago]

From: Santiago Torres 

Instead of running the verify-tag plumbing command, we use the
pgp_verify_tag(). This avoids the usage of an extra fork call. To do
this, we extend the number of parameters that tag.c takes, and
verify-tag passes. Redundant calls done in the pgp_verify_tag function
are removed.

Signed-off-by: Santiago Torres 
---
 Notes:
 - In this version I fixed the issues with the brackets (the old patch
   doesn't work in this case due to the new test.

 builtin/tag.c| 28 +---
 builtin/verify-tag.c | 14 --
 tag.c|  7 ++-
 tag.h|  3 ++-
 4 files changed, 25 insertions(+), 27 deletions(-)

diff --git a/builtin/tag.c b/builtin/tag.c
index 1705c94..3dffdff 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -65,9 +65,10 @@ static int list_tags(struct ref_filter *filter, struct 
ref_sorting *sorting, con
 }
 
 typedef int (*each_tag_name_fn)(const char *name, const char *ref,
-   const unsigned char *sha1);
+   const unsigned char *sha1, unsigned flags);
 
-static int for_each_tag_name(const char **argv, each_tag_name_fn fn)
+static int for_each_tag_name(const char **argv, each_tag_name_fn fn,
+   unsigned flags)
 {
const char **p;
char ref[PATH_MAX];
@@ -86,33 +87,21 @@ static int for_each_tag_name(const char **argv, 
each_tag_name_fn fn)
had_error = 1;
continue;
}
-   if (fn(*p, ref, sha1))
+   if (fn(*p, ref, sha1, flags))
had_error = 1;
}
return had_error;
 }
 
 static int delete_tag(const char *name, const char *ref,
-   const unsigned char *sha1)
+   const unsigned char *sha1, unsigned flags)
 {
-   if (delete_ref(ref, sha1, 0))
+   if (delete_ref(ref, sha1, flags))
return 1;
printf(_("Deleted tag '%s' (was %s)\n"), name, find_unique_abbrev(sha1, 
DEFAULT_ABBREV));
return 0;
 }
 
-static int verify_tag(const char *name, const char *ref,
-   const unsigned char *sha1)
-{
-   const char *argv_verify_tag[] = {"verify-tag",
-   "-v", "SHA1_HEX", NULL};
-   argv_verify_tag[2] = sha1_to_hex(sha1);
-
-   if (run_command_v_opt(argv_verify_tag, RUN_GIT_CMD))
-   return error(_("could not verify the tag '%s'"), name);
-   return 0;
-}
-
 static int do_sign(struct strbuf *buffer)
 {
return sign_buffer(buffer, buffer, get_signing_key());
@@ -424,9 +413,10 @@ int cmd_tag(int argc, const char **argv, const char 
*prefix)
if (filter.merge_commit)
die(_("--merged and --no-merged option are only allowed with 
-l"));
if (cmdmode == 'd')
-   return for_each_tag_name(argv, delete_tag);
+   return for_each_tag_name(argv, delete_tag, 0);
if (cmdmode == 'v')
-   return for_each_tag_name(argv, verify_tag);
+   return for_each_tag_name(argv, pgp_verify_tag,
+   GPG_VERIFY_VERBOSE);
 
if (msg.given || msgfile) {
if (msg.given && msgfile)
diff --git a/builtin/verify-tag.c b/builtin/verify-tag.c
index f776778..8abc357 100644
--- a/builtin/verify-tag.c
+++ b/builtin/verify-tag.c
@@ -30,6 +30,8 @@ int cmd_verify_tag(int argc, const char **argv, const char 
*prefix)
 {
int i = 1, verbose = 0, had_error = 0;
unsigned flags = 0;
+   unsigned char sha1[20];
+   const char *name;
const struct option verify_tag_options[] = {
OPT__VERBOSE(, N_("print tag contents")),
OPT_BIT(0, "raw", , N_("print raw gpg status output"), 
GPG_VERIFY_RAW),
@@ -46,8 +48,16 @@ int cmd_verify_tag(int argc, const char **argv, const char 
*prefix)
if (verbose)
flags |= GPG_VERIFY_VERBOSE;
 
-   while (i < argc)
-   if (pgp_verify_tag(argv[i++], flags))
+   while (i < argc) {
+   name = argv[i++];
+   if (get_sha1(name, sha1)) {
+   error("tag '%s' not found.", name);
had_error = 1;
+   }
+
+   if (pgp_verify_tag(name, NULL, sha1, flags))
+   had_error = 1;
+
+   }
return had_error;
 }
diff --git a/tag.c b/tag.c
index 918ae39..2a0b24c 100644
--- a/tag.c
+++ b/tag.c
@@ -29,18 +29,15 @@ static int run_gpg_verify(const char *buf, unsigned long 
size, unsigned flags)
return ret;
 }
 
-int pgp_verify_tag(const char *name, unsigned flags)
+int pgp_verify_tag(const char *name, const char *ref,
+   const unsigned char *sha1, unsigned flags)
 {
 
enum object_type type;
unsigned long size;
-   unsigned char sha1[20];
char* buf;