[PATCH] sequencer: assign only free()able strings to gpg_sign

[PATCH] sequencer: assign only free()able strings to gpg_sign

[Johannes Schindelin]

The gpg_sign member of the replay_opts structure is of type `char *`,
meaning that the sequencer deems the string to which gpg_sign points to
be under its custody, i.e. it needs to be free()d by the sequencer.

Therefore, let's only assign malloc()ed buffers to it.

Reported-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---

	Phillip, if you want to squash these changes into your patches,
	I'd totally fine with that.

Based-On: pw/sequencer-in-process-commit at https://github.com/dscho/git
Fetch-Base-Via: git fetch https://github.com/dscho/git pw/sequencer-in-process-commit
Published-As: https://github.com/dscho/git/releases/tag/sequencer-owns-gpg-sign-v1
Fetch-It-Via: git fetch https://github.com/dscho/git sequencer-owns-gpg-sign-v1
 sequencer.c                   |  2 +-
 t/t3404-rebase-interactive.sh | 10 ++++++++++
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/sequencer.c b/sequencer.c
index 7051b20b762..1b2599668f5 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -160,7 +160,7 @@ static int git_sequencer_config(const char *k, const char *v, void *cb)
 	}
 
 	if (!strcmp(k, "commit.gpgsign")) {
-		opts->gpg_sign = git_config_bool(k, v) ? "" : NULL;
+		opts->gpg_sign = git_config_bool(k, v) ? xstrdup("") : NULL;
 		return 0;
 	}
 
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 9ed0a244e6c..040ef1a4dbc 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -1318,6 +1318,16 @@ test_expect_success 'editor saves as CR/LF' '
 
 SQ="'"
 test_expect_success 'rebase -i --gpg-sign=<key-id>' '
+	test_when_finished "test_might_fail git rebase --abort" &&
+	set_fake_editor &&
+	FAKE_LINES="edit 1" git rebase -i --gpg-sign="\"S I Gner\"" HEAD^ \
+		>out 2>err &&
+	test_i18ngrep "$SQ-S\"S I Gner\"$SQ" err
+'
+
+test_expect_success 'rebase -i --gpg-sign=<key-id> overrides commit.gpgSign' '
+	test_when_finished "test_might_fail git rebase --abort" &&
+	test_config commit.gpgsign true &&
 	set_fake_editor &&
 	FAKE_LINES="edit 1" git rebase -i --gpg-sign="\"S I Gner\"" HEAD^ \
 		>out 2>err &&

base-commit: 28d6daed4f119940ace31e523b3b272d3d153d04
-- 
2.15.1.windows.2

Re: [PATCH] sequencer: assign only free()able strings to gpg_sign

[phillip.wood]

>----Original Message----
>From: johannes.schindelin@gmx.de
>Date: 22/12/2017 11:50 
>To: <git@vger.kernel.org>
>Cc: "Junio C Hamano"<gitster@pobox.com>, "Phillip Wood"<phillip.
wood@dunelm.org.uk>, "Kaartic Sivaraam"<kaartic.sivaraam@gmail.com>
>Subj: [PATCH] sequencer: assign only free()able strings to gpg_sign
>
>The gpg_sign member of the replay_opts structure is of type `char *`,
>meaning that the sequencer deems the string to which gpg_sign points 
to
>be under its custody, i.e. it needs to be free()d by the sequencer.
>
>Therefore, let's only assign malloc()ed buffers to it.
>
>Reported-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>
>Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
>---
>
>	Phillip, if you want to squash these changes into your patches,
>	I'd totally fine with that.
>

Hi Johannes, thanks for putting this together, the patch it fixes is 
already in next so I think it'd be best to leave this one separate. I 
wonder if it would be worth adding another test, see below.

Best Wishes


Phillip

>Based-On: pw/sequencer-in-process-commit at https://github.com/dscho/git

>Fetch-Base-Via: git fetch https://github.com/dscho/git pw/sequencer-
in-process-commit
>Published-As: https://github.com/dscho/git/releases/tag/sequencer-owns-gpg-sign-v1

>Fetch-It-Via: git fetch https://github.com/dscho/git sequencer-owns-
gpg-sign-v1
> sequencer.c                   |  2 +-
> t/t3404-rebase-interactive.sh | 10 ++++++++++
> 2 files changed, 11 insertions(+), 1 deletion(-)
>
>diff --git a/sequencer.c b/sequencer.c
>index 7051b20b762..1b2599668f5 100644
>--- a/sequencer.c
>+++ b/sequencer.c
>@@ -160,7 +160,7 @@ static int git_sequencer_config(const char *k, 
const char *v, void *cb)
> 	}
> 
> 	if (!strcmp(k, "commit.gpgsign")) {
>-		opts->gpg_sign = git_config_bool(k, v) ? "" : NULL;
>+		opts->gpg_sign = git_config_bool(k, v) ? xstrdup("") : NULL;
> 		return 0;
> 	}
> 
>diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-
interactive.sh
>index 9ed0a244e6c..040ef1a4dbc 100755
>--- a/t/t3404-rebase-interactive.sh
>+++ b/t/t3404-rebase-interactive.sh
>@@ -1318,6 +1318,16 @@ test_expect_success 'editor saves as CR/LF' '
> 
> SQ="'"
> test_expect_success 'rebase -i --gpg-sign=<key-id>' '
>+	test_when_finished "test_might_fail git rebase --abort" &&
>+	set_fake_editor &&
>+	FAKE_LINES="edit 1" git rebase -i --gpg-sign="\"S I Gner\"" HEAD^ \
>+		>out 2>err &&
>+	test_i18ngrep "$SQ-S\"S I Gner\"$SQ" err
>+'
>+
>+test_expect_success 'rebase -i --gpg-sign=<key-id> overrides commit.
gpgSign' '
>+	test_when_finished "test_might_fail git rebase --abort" &&
>+	test_config commit.gpgsign true &&
> 	set_fake_editor &&
> 	FAKE_LINES="edit 1" git rebase -i --gpg-sign="\"S I Gner\"" HEAD^ \
> 		>out 2>err &&


I thought the bug could be triggered when commit.gpgsign was true and 
it was not overriden on the commandline, is it worth adding a test for 
that?


>base-commit: 28d6daed4f119940ace31e523b3b272d3d153d04
>-- 
>2.15.1.windows.2
>

Re: [PATCH] sequencer: assign only free()able strings to gpg_sign

[Johannes Schindelin]

Hi Phillip,

On Fri, 22 Dec 2017, phillip.wood@talktalk.net wrote:

> I thought the bug could be triggered when commit.gpgsign was true and 
> it was not overriden on the commandline, is it worth adding a test for 
> that?

Everybody working with extensive test suites seems to write/blog these
days that you have to be careful to test meaningfully, i.e. you need to
avoid making running the test suite so expensive that developers start to
avoid running it.

In that light, what do we want to test? If we want to verify that the
gpg_sign is correctly allocated before it is free()d, then the test case I
added *already* covers it, and another test case would only increase the
runtime of the test suite (which, as I hinted above, I deem a bad thing).

So I'm really in favor of keeping the tests concise.

Ciao,
Dscho

Re: [PATCH] sequencer: assign only free()able strings to gpg_sign

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

>> I thought the bug could be triggered when commit.gpgsign was true and 
>> it was not overriden on the commandline, is it worth adding a test for 
>> that?
>
> ... If we want to verify that the
> gpg_sign is correctly allocated before it is free()d, then the test case I
> added *already* covers it,...

It depends on what we are testing, how we anticipate this code will
be broken by others in the future and how we want to futureproof the
code.  We can say "already covers it" if we know the implementation
(especially, the code calls free() when it replaces opts->gpg_sign
always, so the other side you are choosing not to test will die the
same way) and assume it won't be broken (i.e. the attitude is OK for
whitebox testing).

I'd think that this particular case it is sufficient to test just
one; I doubt that adding another will increse the test load
measurably, though.

Re: [PATCH] sequencer: assign only free()able strings to gpg_sign

[Junio C Hamano]

"phillip.wood@talktalk.net" <phillip.wood@talktalk.net> writes:

>>----Original Message----
>>From: johannes.schindelin@gmx.de
>>Date: 22/12/2017 11:50 
>>To: <git@vger.kernel.org>
>>Cc: "Junio C Hamano"<gitster@pobox.com>, "Phillip Wood"<phillip.
> wood@dunelm.org.uk>, "Kaartic Sivaraam"<kaartic.sivaraam@gmail.com>
>>Subj: [PATCH] sequencer: assign only free()able strings to gpg_sign
>>
>>The gpg_sign member of the replay_opts structure is of type `char *`,
>>meaning that the sequencer deems the string to which gpg_sign points 
> to
>>be under its custody, i.e. it needs to be free()d by the sequencer.
>>
>>Therefore, let's only assign malloc()ed buffers to it.
>>
>>Reported-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>
>>Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
>>---
>>
>>	Phillip, if you want to squash these changes into your patches,
>>	I'd totally fine with that.
>>
>
> Hi Johannes, thanks for putting this together, the patch it fixes is 
> already in next so I think it'd be best to leave this one separate. I 
> wonder if it would be worth adding another test, see below.

Thanks, both.  Let's queue this on top as a fix-up.