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

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

[Alexander 'z33ky' Hirsch]

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(+)

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

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

[Junio C Hamano]

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?

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

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

[Alexander 'z33ky' Hirsch]

On Wed, May 18, 2016 at 09:04:24AM -0700, Junio C Hamano wrote:
> > 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?

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

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

> 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

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

[Junio C Hamano]

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.