| Message ID | 20201011224804.722607-1-samuel@cavoj.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | sequencer: fix gpg option passed to octopus merge | expand |
On 2020-10-11 at 22:48:04, Samuel Čavoj wrote: > When performing octopus merges with interactive rebase with gpgsign > enabled (either using rebase -S or config commit.gpgsign), the operation > would fail on the merge. Instead of "-S%s" with the key id substituted, > only the bare key id would get passed to the underlying merge command, > which tried to interpret it as a ref. > > Signed-off-by: Samuel Čavoj <samuel@cavoj.net> > --- > It is unclear to me whether I should have based this off of maint or > master, master made more sense to me. I apologize if maint was the > correct one, please tell and I will resubmit. > --- > sequencer.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/sequencer.c b/sequencer.c > index 00acb12496..88ccff4838 100644 > --- a/sequencer.c > +++ b/sequencer.c > @@ -3677,7 +3677,7 @@ static int do_merge(struct repository *r, > strvec_push(&cmd.args, "-F"); > strvec_push(&cmd.args, git_path_merge_msg(r)); > if (opts->gpg_sign) > - strvec_push(&cmd.args, opts->gpg_sign); > + strvec_pushf(&cmd.args, "-S%s", opts->gpg_sign); Yeah, this seems obviously correct, and it's very similar to what we do elsewhere in the file. This will also handle the case where the option is empty (because we want to do autodetection of the key to sign) correctly as well.
Hi, On Sun, 11 Oct 2020, brian m. carlson wrote: > On 2020-10-11 at 22:48:04, Samuel Čavoj wrote: > > When performing octopus merges with interactive rebase with gpgsign > > enabled (either using rebase -S or config commit.gpgsign), the operation > > would fail on the merge. Instead of "-S%s" with the key id substituted, > > only the bare key id would get passed to the underlying merge command, > > which tried to interpret it as a ref. > > > > Signed-off-by: Samuel Čavoj <samuel@cavoj.net> > > --- > > It is unclear to me whether I should have based this off of maint or > > master, master made more sense to me. I apologize if maint was the > > correct one, please tell and I will resubmit. > > --- > > sequencer.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/sequencer.c b/sequencer.c > > index 00acb12496..88ccff4838 100644 > > --- a/sequencer.c > > +++ b/sequencer.c > > @@ -3677,7 +3677,7 @@ static int do_merge(struct repository *r, > > strvec_push(&cmd.args, "-F"); > > strvec_push(&cmd.args, git_path_merge_msg(r)); > > if (opts->gpg_sign) > > - strvec_push(&cmd.args, opts->gpg_sign); > > + strvec_pushf(&cmd.args, "-S%s", opts->gpg_sign); > > Yeah, this seems obviously correct, and it's very similar to what we do > elsewhere in the file. This will also handle the case where the option > is empty (because we want to do autodetection of the key to sign) > correctly as well. ACK It is unclear to me whether we want to bother introducing a test case for this; Octopus merges are somewhat rare... Ciao, Dscho
Thanks for the patch Samuel On 12/10/2020 11:34, Johannes Schindelin wrote: > Hi, > > On Sun, 11 Oct 2020, brian m. carlson wrote: > >> On 2020-10-11 at 22:48:04, Samuel Čavoj wrote: >>> When performing octopus merges with interactive rebase with gpgsign >>> enabled (either using rebase -S or config commit.gpgsign), the operation >>> would fail on the merge. Instead of "-S%s" with the key id substituted, >>> only the bare key id would get passed to the underlying merge command, >>> which tried to interpret it as a ref. >>> >>> Signed-off-by: Samuel Čavoj <samuel@cavoj.net> >>> --- >>> It is unclear to me whether I should have based this off of maint or >>> master, master made more sense to me. I apologize if maint was the >>> correct one, please tell and I will resubmit. >>> --- >>> sequencer.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/sequencer.c b/sequencer.c >>> index 00acb12496..88ccff4838 100644 >>> --- a/sequencer.c >>> +++ b/sequencer.c >>> @@ -3677,7 +3677,7 @@ static int do_merge(struct repository *r, >>> strvec_push(&cmd.args, "-F"); >>> strvec_push(&cmd.args, git_path_merge_msg(r)); >>> if (opts->gpg_sign) >>> - strvec_push(&cmd.args, opts->gpg_sign); >>> + strvec_pushf(&cmd.args, "-S%s", opts->gpg_sign); >> >> Yeah, this seems obviously correct, and it's very similar to what we do >> elsewhere in the file. In run_git_commit() we do if (opts->gpg_sign) strvec_pushf(&cmd.args, "-S%s", opts->gpg_sign); else strvec_push(&cmd.args, "--no-gpg-sign"); I'm not immediately clear why we pass --no-gpg-sign when opts->gpg_sign isn't set but it makes me wonder if we should be doing that here as well > This will also handle the case where the option >> is empty (because we want to do autodetection of the key to sign) >> correctly as well. > > ACK > > It is unclear to me whether we want to bother introducing a test case for > this; Octopus merges are somewhat rare... This code path is used whenever the user specifies a merge strategy other than "recursive" or they pass merge strategy options to any merge strategy including "recursive" so while octopus merges may be rare the union of everything other than a plain recursive merge may not be. Best Wishes Phillip > > Ciao, > Dscho >
Phillip Wood <phillip.wood123@gmail.com> writes: > In run_git_commit() we do > > if (opts->gpg_sign) > strvec_pushf(&cmd.args, "-S%s", opts->gpg_sign); > else > strvec_push(&cmd.args, "--no-gpg-sign"); > > I'm not immediately clear why we pass --no-gpg-sign when > opts->gpg_sign isn't set ... Isn't it because there is a configuration that the &cmd may honor that forces gpg signing all the time? > but it makes me wonder if we should be doing > that here as well Possibly.
On 12/10/2020 17:56, Junio C Hamano wrote: > Phillip Wood <phillip.wood123@gmail.com> writes: > >> In run_git_commit() we do >> >> if (opts->gpg_sign) >> strvec_pushf(&cmd.args, "-S%s", opts->gpg_sign); >> else >> strvec_push(&cmd.args, "--no-gpg-sign"); >> >> I'm not immediately clear why we pass --no-gpg-sign when >> opts->gpg_sign isn't set ... > > Isn't it because there is a configuration that the &cmd may honor > that forces gpg signing all the time? Yes you're right, so we should be passing --no-gpg-sign here if opts->gpg_sign == NULL, otherwise `git merge` will still sign commits when it is run by `git rebase --no-gpg-sign` Best Wishes Phillip > >> but it makes me wonder if we should be doing >> that here as well > > Possibly. >
Junio C Hamano <gitster@pobox.com> writes: > Phillip Wood <phillip.wood123@gmail.com> writes: > >> In run_git_commit() we do >> >> if (opts->gpg_sign) >> strvec_pushf(&cmd.args, "-S%s", opts->gpg_sign); >> else >> strvec_push(&cmd.args, "--no-gpg-sign"); >> >> I'm not immediately clear why we pass --no-gpg-sign when >> opts->gpg_sign isn't set ... > > Isn't it because there is a configuration that the &cmd may honor > that forces gpg signing all the time? > >> but it makes me wonder if we should be doing >> that here as well I was reacting only based on what I saw in these message, but it turns out that cmd above is an internal invocation of "git merge"; as the command does honor the "commit.gpgsign" option, if "rebase" or whatever command that invoked the sequencer turned off the signing by setting opts->gpg_sign to false, I agree that the part touched by the patch should have the else clause to explicitly override the configuration option.
Samuel Čavoj <samuel@cavoj.net> writes: > Subject: Re: [PATCH] sequencer: fix gpg option passed to octopus merge Puzzling. Why do you single out octopus merge like this? sequencer.c::do_merge() is called from pick_commits() whenever we see a "merge" insn, and not limited to an octopus merge. Can we have a test to demonstrate the existing failure, so that we can notice if this fix is broken in the future? > When performing octopus merges with interactive rebase with gpgsign > enabled (either using rebase -S or config commit.gpgsign), the operation > would fail on the merge. Instead of "-S%s" with the key id substituted, > only the bare key id would get passed to the underlying merge command, > which tried to interpret it as a ref. > > Signed-off-by: Samuel Čavoj <samuel@cavoj.net> > --- > It is unclear to me whether I should have based this off of maint or > master, master made more sense to me. I apologize if maint was the > correct one, please tell and I will resubmit. > --- > sequencer.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/sequencer.c b/sequencer.c > index 00acb12496..88ccff4838 100644 > --- a/sequencer.c > +++ b/sequencer.c > @@ -3677,7 +3677,7 @@ static int do_merge(struct repository *r, > strvec_push(&cmd.args, "-F"); > strvec_push(&cmd.args, git_path_merge_msg(r)); > if (opts->gpg_sign) > - strvec_push(&cmd.args, opts->gpg_sign); > + strvec_pushf(&cmd.args, "-S%s", opts->gpg_sign); > > /* Add the tips to be merged */ > for (j = to_merge; j; j = j->next)
Junio C Hamano <gitster@pobox.com> writes: > Junio C Hamano <gitster@pobox.com> writes: > >> Phillip Wood <phillip.wood123@gmail.com> writes: >> >>> In run_git_commit() we do >>> >>> if (opts->gpg_sign) >>> strvec_pushf(&cmd.args, "-S%s", opts->gpg_sign); >>> else >>> strvec_push(&cmd.args, "--no-gpg-sign"); >>> >>> I'm not immediately clear why we pass --no-gpg-sign when >>> opts->gpg_sign isn't set ... >> >> Isn't it because there is a configuration that the &cmd may honor >> that forces gpg signing all the time? >> >>> but it makes me wonder if we should be doing >>> that here as well > > I was reacting only based on what I saw in these message, but it > turns out that cmd above is an internal invocation of "git merge"; > as the command does honor the "commit.gpgsign" option, if "rebase" > or whatever command that invoked the sequencer turned off the > signing by setting opts->gpg_sign to false, I agree that the part > touched by the patch should have the else clause to explicitly > override the configuration option. Forgot to say that it is perfectly fine to leave it outside Samuel's patch. It "fixes" the "intended" behaviour of the current design where it somehow was deemed a good idea to disallow overriding commit.gpgsign variable. Fixing that design so that we can override configured default from the command line can be left for a follow-up patch. Thanks.
Hi, On Mon, 12 Oct 2020, Junio C Hamano wrote: > Samuel Čavoj <samuel@cavoj.net> writes: > > > Subject: Re: [PATCH] sequencer: fix gpg option passed to octopus merge > > Puzzling. Why do you single out octopus merge like this? > > sequencer.c::do_merge() is called from pick_commits() whenever we > see a "merge" insn, and not limited to an octopus merge. > > Can we have a test to demonstrate the existing failure, so that we > can notice if this fix is broken in the future? Yes, now that I understand that not only octopus merges are affected, I would be very much in favor of adding a test case. If I may suggest to add a new test case to t3435 based on t3430's '--rebase-merges with strategies' and t3404's 'rebase -i --gpg-sign=<key-id>'? Something along these lines: -- snipsnap -- diff --git a/sequencer.c b/sequencer.c index 00acb124962..88ccff4838d 100644 --- a/sequencer.c +++ b/sequencer.c @@ -3677,7 +3677,7 @@ static int do_merge(struct repository *r, strvec_push(&cmd.args, "-F"); strvec_push(&cmd.args, git_path_merge_msg(r)); if (opts->gpg_sign) - strvec_push(&cmd.args, opts->gpg_sign); + strvec_pushf(&cmd.args, "-S%s", opts->gpg_sign); /* Add the tips to be merged */ for (j = to_merge; j; j = j->next) diff --git a/t/t3435-rebase-gpg-sign.sh b/t/t3435-rebase-gpg-sign.sh index b47c59c190b..f70b280f5c1 100755 --- a/t/t3435-rebase-gpg-sign.sh +++ b/t/t3435-rebase-gpg-sign.sh @@ -68,4 +68,10 @@ test_expect_failure 'rebase -p --no-gpg-sign override commit.gpgsign' ' test_must_fail git verify-commit HEAD ' +test_expect_success 'rebase -r, GPG and merge strategies' ' + git reset --hard merged && + git rebase -fr --gpg-sign -s resolve --root && + git verify-commit HEAD +' + test_done
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: >> Puzzling. Why do you single out octopus merge like this? >> >> sequencer.c::do_merge() is called from pick_commits() whenever we >> see a "merge" insn, and not limited to an octopus merge. >> >> Can we have a test to demonstrate the existing failure, so that we >> can notice if this fix is broken in the future? > > Yes, now that I understand that not only octopus merges are affected, I > would be very much in favor of adding a test case. Ah, I see why the initial description was focused on octopus. The code special cases the two-parent merge using the recursive strategy and uses completely separate codepath for it, which does not have this problem but the codepath that handles octopus merges and merges with the non-default strategy have the problem. > If I may suggest to add a new test case to t3435 based on t3430's > '--rebase-merges with strategies' and t3404's 'rebase -i > --gpg-sign=<key-id>'? Something along these lines: In addition, a test that passes --no-gpg-sign from the command line, because commit.gpgsign is set in the repository used in these tests, would be necessary---we want to make sure the command line overrides the configured default (which is the topic of [patch 2/1]). > > -- snipsnap -- > diff --git a/sequencer.c b/sequencer.c > index 00acb124962..88ccff4838d 100644 > --- a/sequencer.c > +++ b/sequencer.c > @@ -3677,7 +3677,7 @@ static int do_merge(struct repository *r, > strvec_push(&cmd.args, "-F"); > strvec_push(&cmd.args, git_path_merge_msg(r)); > if (opts->gpg_sign) > - strvec_push(&cmd.args, opts->gpg_sign); > + strvec_pushf(&cmd.args, "-S%s", opts->gpg_sign); > > /* Add the tips to be merged */ > for (j = to_merge; j; j = j->next) > diff --git a/t/t3435-rebase-gpg-sign.sh b/t/t3435-rebase-gpg-sign.sh > index b47c59c190b..f70b280f5c1 100755 > --- a/t/t3435-rebase-gpg-sign.sh > +++ b/t/t3435-rebase-gpg-sign.sh > @@ -68,4 +68,10 @@ test_expect_failure 'rebase -p --no-gpg-sign override commit.gpgsign' ' > test_must_fail git verify-commit HEAD > ' > > +test_expect_success 'rebase -r, GPG and merge strategies' ' > + git reset --hard merged && > + git rebase -fr --gpg-sign -s resolve --root && > + git verify-commit HEAD > +' > + > test_done
diff --git a/sequencer.c b/sequencer.c index 00acb12496..88ccff4838 100644 --- a/sequencer.c +++ b/sequencer.c @@ -3677,7 +3677,7 @@ static int do_merge(struct repository *r, strvec_push(&cmd.args, "-F"); strvec_push(&cmd.args, git_path_merge_msg(r)); if (opts->gpg_sign) - strvec_push(&cmd.args, opts->gpg_sign); + strvec_pushf(&cmd.args, "-S%s", opts->gpg_sign); /* Add the tips to be merged */ for (j = to_merge; j; j = j->next)
When performing octopus merges with interactive rebase with gpgsign enabled (either using rebase -S or config commit.gpgsign), the operation would fail on the merge. Instead of "-S%s" with the key id substituted, only the bare key id would get passed to the underlying merge command, which tried to interpret it as a ref. Signed-off-by: Samuel Čavoj <samuel@cavoj.net> --- It is unclear to me whether I should have based this off of maint or master, master made more sense to me. I apologize if maint was the correct one, please tell and I will resubmit. --- sequencer.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)