* [PATCH v2] rebase -i: Fix possibly wrong onto hash in todo
@ 2020-08-13 17:42 Antti Keränen
0 siblings, 0 replies; 5+ messages in thread
From: Antti Keränen @ 2020-08-13 17:42 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
'todo_list_write_to_file' may overwrite the static buffer, originating
from 'find_unique_abbrev', that was used to store the short commit hash
'c' for "# Rebase a..b onto c" message in the todo editor. This is
because the buffer that is returned from 'find_unique_abbrev' is valid
until 4 more calls to `find_unique_abbrev` are made.
As 'todo_list_write_to_file' calls 'find_unique_abbrev' for each rebased
commit, the hash for 'c' is overwritten if there are 4 or more commits
in the rebase. This behavior has been broken since its introduction.
Fix by storing the short onto commit hash in a different buffer that
remains valid, before calling 'todo_list_write_to_file'.
Found-by: Jussi Keränen <jussike@gmail.com>
Signed-off-by: Antti Keränen <detegr@rbx.email>
Acked-by: Alban Gruin <alban.gruin@gmail.com>
---
sequencer.c | 5 +++--
t/t3404-rebase-interactive.sh | 6 ++++++
2 files changed, 9 insertions(+), 2 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index fd7701c88a..e2007dbb8c 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -5178,13 +5178,14 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla
struct string_list *commands, unsigned autosquash,
struct todo_list *todo_list)
{
- const char *shortonto, *todo_file = rebase_path_todo();
+ char shortonto[GIT_MAX_HEXSZ + 1];
+ const char *todo_file = rebase_path_todo();
struct todo_list new_todo = TODO_LIST_INIT;
struct strbuf *buf = &todo_list->buf, buf2 = STRBUF_INIT;
struct object_id oid = onto->object.oid;
int res;
- shortonto = find_unique_abbrev(&oid, DEFAULT_ABBREV);
+ find_unique_abbrev_r(shortonto, &oid, DEFAULT_ABBREV);
if (buf->len == 0) {
struct todo_item *item = append_new_todo(todo_list);
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 4a7d21f898..1b4fa0843e 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -1760,6 +1760,12 @@ test_expect_success 'correct error message for commit --amend after empty pick'
test_i18ngrep "middle of a rebase -- cannot amend." err
'
+test_expect_success 'todo has correct onto hash' '
+ GIT_SEQUENCE_EDITOR=cat git rebase -i no-conflict-branch~4 no-conflict-branch >actual &&
+ onto=$(git rev-parse --short HEAD~4) &&
+ test_i18ngrep "^# Rebase ..* onto $onto" actual
+'
+
# This must be the last test in this file
test_expect_success '$EDITOR and friends are unchanged' '
test_editor_unchanged
--
2.27.0.395.g84249cff14
^ permalink raw reply related [flat|nested] 5+ messages in thread* [PATCH v2] rebase -i: Fix possibly wrong onto hash in todo
@ 2020-08-12 18:33 Antti Keränen
2020-08-12 20:46 ` Junio C Hamano
2020-08-13 10:41 ` Alban Gruin
0 siblings, 2 replies; 5+ messages in thread
From: Antti Keränen @ 2020-08-12 18:33 UTC (permalink / raw)
To: git
Cc: Taylor Blau, Antti Keränen, Jussi Keränen,
Junio C Hamano, Phillip Wood, Alban Gruin, Johannes Schindelin
'todo_list_write_to_file' may overwrite the static buffer, originating
from 'find_unique_abbrev', that was used to store the short commit hash
'c' for "# Rebase a..b onto c" message in the todo editor. This is
because the buffer that is returned from 'find_unique_abbrev' is valid
until 4 more calls to `find_unique_abbrev` are made.
As 'todo_list_write_to_file' calls 'find_unique_abbrev' for each rebased
commit, the hash for 'c' is overwritten if there are 4 or more commits
in the rebase. This behavior has been broken since its introduction.
Fix by storing the short onto commit hash in a different buffer that
remains valid, before calling 'todo_list_write_to_file'.
Found-by: Jussi Keränen <jussike@gmail.com>
Signed-off-by: Antti Keränen <detegr@rbx.email>
---
sequencer.c | 5 +++--
t/t3404-rebase-interactive.sh | 6 ++++++
2 files changed, 9 insertions(+), 2 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index fd7701c88a..e2007dbb8c 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -5178,13 +5178,14 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla
struct string_list *commands, unsigned autosquash,
struct todo_list *todo_list)
{
- const char *shortonto, *todo_file = rebase_path_todo();
+ char shortonto[GIT_MAX_HEXSZ + 1];
+ const char *todo_file = rebase_path_todo();
struct todo_list new_todo = TODO_LIST_INIT;
struct strbuf *buf = &todo_list->buf, buf2 = STRBUF_INIT;
struct object_id oid = onto->object.oid;
int res;
- shortonto = find_unique_abbrev(&oid, DEFAULT_ABBREV);
+ find_unique_abbrev_r(shortonto, &oid, DEFAULT_ABBREV);
if (buf->len == 0) {
struct todo_item *item = append_new_todo(todo_list);
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 4a7d21f898..1b4fa0843e 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -1760,6 +1760,12 @@ test_expect_success 'correct error message for commit --amend after empty pick'
test_i18ngrep "middle of a rebase -- cannot amend." err
'
+test_expect_success 'todo has correct onto hash' '
+ GIT_SEQUENCE_EDITOR=cat git rebase -i no-conflict-branch~4 no-conflict-branch >actual &&
+ onto=$(git rev-parse --short HEAD~4) &&
+ test_i18ngrep "^# Rebase ..* onto $onto" actual
+'
+
# This must be the last test in this file
test_expect_success '$EDITOR and friends are unchanged' '
test_editor_unchanged
--
2.27.0.395.g84249cff14
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH v2] rebase -i: Fix possibly wrong onto hash in todo
2020-08-12 18:33 Antti Keränen
@ 2020-08-12 20:46 ` Junio C Hamano
2020-08-13 10:41 ` Alban Gruin
1 sibling, 0 replies; 5+ messages in thread
From: Junio C Hamano @ 2020-08-12 20:46 UTC (permalink / raw)
To: Antti Keränen
Cc: git, Taylor Blau, Jussi Keränen, Phillip Wood, Alban Gruin,
Johannes Schindelin
Antti Keränen <detegr@rbx.email> writes:
> 'todo_list_write_to_file' may overwrite the static buffer, originating
> from 'find_unique_abbrev', that was used to store the short commit hash
> 'c' for "# Rebase a..b onto c" message in the todo editor. This is
> because the buffer that is returned from 'find_unique_abbrev' is valid
> until 4 more calls to `find_unique_abbrev` are made.
>
> As 'todo_list_write_to_file' calls 'find_unique_abbrev' for each rebased
> commit, the hash for 'c' is overwritten if there are 4 or more commits
> in the rebase. This behavior has been broken since its introduction.
>
> Fix by storing the short onto commit hash in a different buffer that
> remains valid, before calling 'todo_list_write_to_file'.
>
> Found-by: Jussi Keränen <jussike@gmail.com>
> Signed-off-by: Antti Keränen <detegr@rbx.email>
> ---
Looking good.
> sequencer.c | 5 +++--
> t/t3404-rebase-interactive.sh | 6 ++++++
> 2 files changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/sequencer.c b/sequencer.c
> index fd7701c88a..e2007dbb8c 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -5178,13 +5178,14 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla
> struct string_list *commands, unsigned autosquash,
> struct todo_list *todo_list)
> {
> - const char *shortonto, *todo_file = rebase_path_todo();
> + char shortonto[GIT_MAX_HEXSZ + 1];
> + const char *todo_file = rebase_path_todo();
> struct todo_list new_todo = TODO_LIST_INIT;
> struct strbuf *buf = &todo_list->buf, buf2 = STRBUF_INIT;
> struct object_id oid = onto->object.oid;
> int res;
>
> - shortonto = find_unique_abbrev(&oid, DEFAULT_ABBREV);
> + find_unique_abbrev_r(shortonto, &oid, DEFAULT_ABBREV);
>
> if (buf->len == 0) {
> struct todo_item *item = append_new_todo(todo_list);
> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> index 4a7d21f898..1b4fa0843e 100755
> --- a/t/t3404-rebase-interactive.sh
> +++ b/t/t3404-rebase-interactive.sh
> @@ -1760,6 +1760,12 @@ test_expect_success 'correct error message for commit --amend after empty pick'
> test_i18ngrep "middle of a rebase -- cannot amend." err
> '
>
> +test_expect_success 'todo has correct onto hash' '
> + GIT_SEQUENCE_EDITOR=cat git rebase -i no-conflict-branch~4 no-conflict-branch >actual &&
> + onto=$(git rev-parse --short HEAD~4) &&
> + test_i18ngrep "^# Rebase ..* onto $onto" actual
> +'
> +
> # This must be the last test in this file
> test_expect_success '$EDITOR and friends are unchanged' '
> test_editor_unchanged
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH v2] rebase -i: Fix possibly wrong onto hash in todo
2020-08-12 18:33 Antti Keränen
2020-08-12 20:46 ` Junio C Hamano
@ 2020-08-13 10:41 ` Alban Gruin
2020-08-13 14:38 ` Phillip Wood
1 sibling, 1 reply; 5+ messages in thread
From: Alban Gruin @ 2020-08-13 10:41 UTC (permalink / raw)
To: Antti Keränen, git
Cc: Taylor Blau, Jussi Keränen, Junio C Hamano, Phillip Wood,
Johannes Schindelin
Hi Antti,
Le 12/08/2020 à 20:33, Antti Keränen a écrit :
> 'todo_list_write_to_file' may overwrite the static buffer, originating
> from 'find_unique_abbrev', that was used to store the short commit hash
> 'c' for "# Rebase a..b onto c" message in the todo editor. This is
> because the buffer that is returned from 'find_unique_abbrev' is valid
> until 4 more calls to `find_unique_abbrev` are made.
>
> As 'todo_list_write_to_file' calls 'find_unique_abbrev' for each rebased
> commit, the hash for 'c' is overwritten if there are 4 or more commits
> in the rebase. This behavior has been broken since its introduction.
>
> Fix by storing the short onto commit hash in a different buffer that
> remains valid, before calling 'todo_list_write_to_file'.
>
> Found-by: Jussi Keränen <jussike@gmail.com>
> Signed-off-by: Antti Keränen <detegr@rbx.email>
> ---
> sequencer.c | 5 +++--
> t/t3404-rebase-interactive.sh | 6 ++++++
> 2 files changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/sequencer.c b/sequencer.c
> index fd7701c88a..e2007dbb8c 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -5178,13 +5178,14 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla
> struct string_list *commands, unsigned autosquash,
> struct todo_list *todo_list)
> {
> - const char *shortonto, *todo_file = rebase_path_todo();
> + char shortonto[GIT_MAX_HEXSZ + 1];
> + const char *todo_file = rebase_path_todo();
> struct todo_list new_todo = TODO_LIST_INIT;
> struct strbuf *buf = &todo_list->buf, buf2 = STRBUF_INIT;
> struct object_id oid = onto->object.oid;
> int res;
>
> - shortonto = find_unique_abbrev(&oid, DEFAULT_ABBREV);
> + find_unique_abbrev_r(shortonto, &oid, DEFAULT_ABBREV);
>
> if (buf->len == 0) {
> struct todo_item *item = append_new_todo(todo_list);
> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> index 4a7d21f898..1b4fa0843e 100755
> --- a/t/t3404-rebase-interactive.sh
> +++ b/t/t3404-rebase-interactive.sh
> @@ -1760,6 +1760,12 @@ test_expect_success 'correct error message for commit --amend after empty pick'
> test_i18ngrep "middle of a rebase -- cannot amend." err
> '
>
> +test_expect_success 'todo has correct onto hash' '
> + GIT_SEQUENCE_EDITOR=cat git rebase -i no-conflict-branch~4 no-conflict-branch >actual &&
> + onto=$(git rev-parse --short HEAD~4) &&
> + test_i18ngrep "^# Rebase ..* onto $onto" actual
> +'
> +
> # This must be the last test in this file
> test_expect_success '$EDITOR and friends are unchanged' '
> test_editor_unchanged
>
Looks good to me.
Acked-by: Alban Gruin <alban.gruin@gmail.com>
This makes me wonder if it's worth to do the same change in
todo_list_to_strbuf(). #leftoverbits, perhaps?
Cheers,
Alban
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH v2] rebase -i: Fix possibly wrong onto hash in todo
2020-08-13 10:41 ` Alban Gruin
@ 2020-08-13 14:38 ` Phillip Wood
0 siblings, 0 replies; 5+ messages in thread
From: Phillip Wood @ 2020-08-13 14:38 UTC (permalink / raw)
To: Alban Gruin, Antti Keränen, git
Cc: Taylor Blau, Jussi Keränen, Junio C Hamano, Phillip Wood,
Johannes Schindelin
Hi Antti & Alban
On 13/08/2020 11:41, Alban Gruin wrote:
> Hi Antti,
>
> Le 12/08/2020 à 20:33, Antti Keränen a écrit :
>> 'todo_list_write_to_file' may overwrite the static buffer, originating
>> from 'find_unique_abbrev', that was used to store the short commit hash
>> 'c' for "# Rebase a..b onto c" message in the todo editor. This is
>> because the buffer that is returned from 'find_unique_abbrev' is valid
>> until 4 more calls to `find_unique_abbrev` are made.
>>
>> As 'todo_list_write_to_file' calls 'find_unique_abbrev' for each rebased
>> commit, the hash for 'c' is overwritten if there are 4 or more commits
>> in the rebase. This behavior has been broken since its introduction.
>>
>> Fix by storing the short onto commit hash in a different buffer that
>> remains valid, before calling 'todo_list_write_to_file'.
>>
>> Found-by: Jussi Keränen <jussike@gmail.com>
>> Signed-off-by: Antti Keränen <detegr@rbx.email>
>> ---
>> sequencer.c | 5 +++--
>> t/t3404-rebase-interactive.sh | 6 ++++++
>> 2 files changed, 9 insertions(+), 2 deletions(-)
>>
>> diff --git a/sequencer.c b/sequencer.c
>> index fd7701c88a..e2007dbb8c 100644
>> --- a/sequencer.c
>> +++ b/sequencer.c
>> @@ -5178,13 +5178,14 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla
>> struct string_list *commands, unsigned autosquash,
>> struct todo_list *todo_list)
>> {
>> - const char *shortonto, *todo_file = rebase_path_todo();
>> + char shortonto[GIT_MAX_HEXSZ + 1];
>> + const char *todo_file = rebase_path_todo();
>> struct todo_list new_todo = TODO_LIST_INIT;
>> struct strbuf *buf = &todo_list->buf, buf2 = STRBUF_INIT;
>> struct object_id oid = onto->object.oid;
>> int res;
>>
>> - shortonto = find_unique_abbrev(&oid, DEFAULT_ABBREV);
>> + find_unique_abbrev_r(shortonto, &oid, DEFAULT_ABBREV);
>>
>> if (buf->len == 0) {
>> struct todo_item *item = append_new_todo(todo_list);
>> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
>> index 4a7d21f898..1b4fa0843e 100755
>> --- a/t/t3404-rebase-interactive.sh
>> +++ b/t/t3404-rebase-interactive.sh
>> @@ -1760,6 +1760,12 @@ test_expect_success 'correct error message for commit --amend after empty pick'
>> test_i18ngrep "middle of a rebase -- cannot amend." err
>> '
>>
>> +test_expect_success 'todo has correct onto hash' '
>> + GIT_SEQUENCE_EDITOR=cat git rebase -i no-conflict-branch~4 no-conflict-branch >actual &&
>> + onto=$(git rev-parse --short HEAD~4) &&
>> + test_i18ngrep "^# Rebase ..* onto $onto" actual
>> +'
>> +
>> # This must be the last test in this file
>> test_expect_success '$EDITOR and friends are unchanged' '
>> test_editor_unchanged
>>
>
> Looks good to me.
It looks good to me too, thanks Antti
> Acked-by: Alban Gruin <alban.gruin@gmail.com>
>
> This makes me wonder if it's worth to do the same change in
> todo_list_to_strbuf(). #leftoverbits, perhaps?
In todo_list_to_strbuf() we append the short oid to an strbuf before we
call find_unique_abbrev() again so I don't think it should be a problem
there
Best Wishes
Phillip
> Cheers,
> Alban
>
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-08-13 17:43 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-08-13 17:42 [PATCH v2] rebase -i: Fix possibly wrong onto hash in todo Antti Keränen
-- strict thread matches above, loose matches on Subject: below --
2020-08-12 18:33 Antti Keränen
2020-08-12 20:46 ` Junio C Hamano
2020-08-13 10:41 ` Alban Gruin
2020-08-13 14:38 ` Phillip Wood
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.