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