Hi Samuel
On 13/10/2020 11:51, Samuel Čavoj wrote:
Hi Phillip,
thank you very much for the feedback.
On 13.10.2020 11:02, Phillip Wood wrote:
Hi Samuel
On 13/10/2020 00:49, Samuel Čavoj wrote:
From: Johannes Schindelin <Johannes.Schindelin@xxxxxx>
When performing a rebase with --rebase-merges using either a custom
strategy specified with -s or an octopus merge, and at the same time
having gpgsign enabled (either rebase -S or config commit.gpgsign), the
operation would fail on making the merge commit. 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.
Fix the issue and add a test case as suggested by Johannes Schindelin.
Signed-off-by: Samuel Čavoj <samuel@xxxxxxxxx>
---
changed v1 -> v2:
added test case
---
sequencer.c | 2 +-
t/t3435-rebase-gpg-sign.sh | 6 ++++++
2 files changed, 7 insertions(+), 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)
diff --git a/t/t3435-rebase-gpg-sign.sh b/t/t3435-rebase-gpg-sign.sh
index b47c59c190..f70b280f5c 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
+'
Unfortunately I've just noticed that the test above runs
git config commit.gpgsign true
So this test would pass anyway if merge picks up that config setting.
That's predicated on my misunderstanding of the behavior without your
patch but it would be good to fix the test anyway
The
previous test needs to be changed to
test_config commit.gpgsign true
Should I do that now, maybe as a separate part of the patch series? Or
just override the config with a `test_config commit.gpgsign false` in
the test I added?
As you've found another instance that needs changing it would probably
be better as a separate patch
There is another usage of `git config` in the file, in the
test_rebase_gpg_sign function.
Well spotted, that should be changed as well
so that the config setting is cleared when that test finishes. That previous
test would then be a good template for testing `rebase -r --no-gpg-sign` if
you wanted to add a test for that to the next patch as Junio suggested.
Yes, I will definitely do that in v3.
Thanks
Also, the previous test is expect_failure,
that means its a known bug?
Yes, `rebase -p` is deprecated and it looks like it was skipped by the
recent fix for `rebase --no-gpg-sign` in c241371c04 ("rebase.c: honour
--no-gpg-sign", 2020-04-03)
Best Wishes
Phillip
Regards,
Samuel
Best Wishes
Phillip
test_done
