threads / patch / 42366

patchpull: warn on --verify-signatures with --rebase

Subject: [PATCH] pull: warn on --verify-signatures with --rebase

## tl;dr

4 messages between May 18, 2016 and May 19, 2016. Diffs are folded; open one to read it.

replies: 3people: 2as markdown or json

Alexander 'z33ky' Hirsch· May 18, 2016, 10:18 UTC · lore

Previously git-pull silently ignored the --verify-signatures option for --rebase.

Signed-off-by: Alexander 'z33ky' Hirsch <1zeeky@gmail.com>
---

Sorry it took so long for the update. I made git-pull warn instead or error and explained why "the --verify-signatures option does not work for --rebase" in the message.

 builtin/pull.c  |  2 ++
 t/t5520-pull.sh | 16 ++++++++++++++++
 2 files changed, 18 insertions(+)

Show changes to 2 files +18 −0

builtin/pull.c, t/t5520-pull.sh

diff --git a/builtin/pull.c b/builtin/pull.c
index 1d7333c..0eafae7 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -815,6 +815,8 @@ static int run_rebase(const unsigned char *curr_head,
 		argv_array_push(&args, "--no-autostash");
 	else if (opt_autostash == 1)
 		argv_array_push(&args, "--autostash");
+	if (opt_verify_signatures && strcmp(opt_verify_signatures, "--verify-signatures") == 0)
+		warning(_("git-rebase does not support --verify-signatures"));
 
 	argv_array_push(&args, "--onto");
 	argv_array_push(&args, sha1_to_hex(merge_head));
diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
index 739c089..d605450 100755
--- a/t/t5520-pull.sh
+++ b/t/t5520-pull.sh
@@ -341,6 +341,22 @@ test_expect_success 'branch.to-rebase.rebase should override pull.rebase' '
 	test new = "$(git show HEAD:file2)"
 '
 
+test_expect_success "pull --rebase warns on --verify-signatures" '
+	git reset --hard before-rebase &&
+	git pull --rebase --verify-signatures . copy 2>err &&
+	test "$(git rev-parse HEAD^)" = "$(git rev-parse copy)" &&
+	test new = "$(git show HEAD:file2)" &&
+	test_i18ngrep "git-rebase does not support --verify-signatures" err
+'
+
+test_expect_success "pull --rebase does not warn on --no-verify-signatures" '
+	git reset --hard before-rebase &&
+	git pull --rebase --no-verify-signatures . copy 2>err &&
+	test "$(git rev-parse HEAD^)" = "$(git rev-parse copy)" &&
+	test new = "$(git show HEAD:file2)" &&
+	test_i18ngrep ! "verify-signatures" err
+'
+
 # add a feature branch, keep-merge, that is merged into master, so the
 # test can try preserving the merge commit (or not) with various
 # --rebase flags/pull.rebase settings.

-- 
2.8.2

Junio C Hamano· May 18, 2016, 16:04 UTC · re: Alexander 'z33ky' Hirsch · lore

Re: [PATCH] pull: warn on --verify-signatures with --rebase

Alexander 'z33ky' Hirsch <1zeeky@gmail.com> writes:

> Previously git-pull silently ignored the --verify-signatures option for
> --rebase.

Missing pieces information that would have made the patch more complete are answers to these questions:

 - Is that a bad thing?  Why?

 - Assuming it is a bad thing, what is the solution this patch
   presents us?  Teach rebase about the option?  Error out the
   request?  What is the reason why "warn" was chosen as the best
   way forward?

Show 13 quoted lines

>  builtin/pull.c  |  2 ++
>  t/t5520-pull.sh | 16 ++++++++++++++++
>  2 files changed, 18 insertions(+)
>
> diff --git a/builtin/pull.c b/builtin/pull.c
> index 1d7333c..0eafae7 100644
> --- a/builtin/pull.c
> +++ b/builtin/pull.c
> @@ -815,6 +815,8 @@ static int run_rebase(const unsigned char *curr_head,
>  		argv_array_push(&args, "--no-autostash");
>  	else if (opt_autostash == 1)
>  		argv_array_push(&args, "--autostash");
> +	if (opt_verify_signatures && strcmp(opt_verify_signatures, "--verify-signatures") == 0)

The logic looks OK. I would have written that long line as two lines, e.g.

	if (opt_verify_signatures &&
            !strcmp(opt_verify_signatures, "--verify-signatures")

though.

> +		warning(_("git-rebase does not support --verify-signatures"));

Is this a good warning message?

As a casual reader, my reaction to this warning would be "Does not support? Then what did it do instead? Did it refuse to integrate my changes on top of what happened on the remote?"

Something like

    warning(_("ignored --verify-signatures as it is meaningless in rebase"));

may convey what is going on better, in that it makes it clear that we are not failing "rebase" and instead we are ignoring "verify".

It is way too long for the final version, though. A more concise way to say the same thing needs to be found.

Thanks.

Alexander 'z33ky' Hirsch· May 19, 2016, 10:02 UTC · re: Junio C Hamano · lore

Re: [PATCH] pull: warn on --verify-signatures with --rebase

On Wed, May 18, 2016 at 09:04:24AM -0700, Junio C Hamano wrote:

Show 13 quoted lines

> > Previously git-pull silently ignored the --verify-signatures option for
> > --rebase.
> 
> Missing pieces information that would have made the patch more
> complete are answers to these questions:
> 
>  - Is that a bad thing?  Why?
> 
>  - Assuming it is a bad thing, what is the solution this patch
>    presents us?  Teach rebase about the option?  Error out the
>    request?  What is the reason why "warn" was chosen as the best
>    way forward?
>

Is the warning a solution "for now" and might this become an error should a valid usecase not be found after a while?

Show 22 quoted lines

> >  builtin/pull.c  |  2 ++
> >  t/t5520-pull.sh | 16 ++++++++++++++++
> >  2 files changed, 18 insertions(+)
> >
> > diff --git a/builtin/pull.c b/builtin/pull.c
> > index 1d7333c..0eafae7 100644
> > --- a/builtin/pull.c
> > +++ b/builtin/pull.c
> > @@ -815,6 +815,8 @@ static int run_rebase(const unsigned char *curr_head,
> >  		argv_array_push(&args, "--no-autostash");
> >  	else if (opt_autostash == 1)
> >  		argv_array_push(&args, "--autostash");
> > +	if (opt_verify_signatures && strcmp(opt_verify_signatures, "--verify-signatures") == 0)
> 
> The logic looks OK.  I would have written that long line as two
> lines, e.g.
> 
> 	if (opt_verify_signatures &&
>             !strcmp(opt_verify_signatures, "--verify-signatures")
> 
> though.
>

I shall format it as per your suggestion in the next submission.

Show 8 quoted lines

> > +		warning(_("git-rebase does not support --verify-signatures"));
> 
> Is this a good warning message?
> 
> As a casual reader, my reaction to this warning would be "Does not
> support?  Then what did it do instead?  Did it refuse to integrate
> my changes on top of what happened on the remote?"
>

Indeed.

Show 10 quoted lines

> Something like
> 
>     warning(_("ignored --verify-signatures as it is meaningless in rebase"));
> 
> may convey what is going on better, in that it makes it clear that
> we are not failing "rebase" and instead we are ignoring "verify".
> 
> It is way too long for the final version, though.  A more concise
> way to say the same thing needs to be found.
>

Would "ignoring --verify-signatures for rebase" be sufficient? It does not describe why it is ignored though.

With Regards, Alexander Hirsch

Junio C Hamano· May 19, 2016, 15:46 UTC · re: Alexander 'z33ky' Hirsch · lore

Re: [PATCH] pull: warn on --verify-signatures with --rebase

Alexander 'z33ky' Hirsch <1zeeky@gmail.com> writes:

> Would "ignoring --verify-signatures for rebase" be sufficient? It does
> not describe why it is ignored though.

Yeah, I agree that that would be sufficient.