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

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