rebase -i: fix ORIG_HEAD handling

The buffer containing the oid for ORIG_HEAD is overwritten before ORIG_HEAD is created. This series fixes that bug and then converts the code to use struct object_id rather than passing around strings.

Thanks to Caspar for reporting the bug and providing a reproducible example

Changes since v1:

* Updated the commit message to patch 1 as suggested by Junio
* Moved the conversion from find_unique_abbrev() to oid_to_hex() from patch 1 to patch 4
* Fixed a compilation error in patch 3 (the required change was in patch 4 by a mistake)

Cc: Johannes Schindelin [email protected] [[email protected]], Caspar Duregger [email protected] [[email protected]]

Phillip Wood (4):
rebase -i: stop overwriting ORIG_HEAD buffer
rebase -i: use struct object_id rather than looking up commit rebase -i: use struct object_id when writing state rebase -i: simplify get_revision_ranges()

 builtin/rebase.c              | 20 ++++++++++----------
 sequencer.c                   | 15 ++++++---------
 sequencer.h                   |  7 ++++---
 t/t3404-rebase-interactive.sh | 11 +++++++++++
 4 files changed, 31 insertions(+), 22 deletions(-)

base-commit: 2e673356aefa8ed19be3c878f966ad6189ecb510
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-773%2Fphillipwood%2Fwip%2Frebase-fix-orig_head-handling-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-773/phillipwood/wip/rebase-fix-orig_head-handling-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/773

Range-diff vs v1:

1: 24f2c4a623 ! 1: da05958c58 rebase -i: stop overwriting ORIG_HEAD buffer
@@ Metadata
## Commit message ##
rebase -i: stop overwriting ORIG_HEAD buffer

- After rebasing ORIG_HEAD is supposed to point to the old HEAD of the
- rebased branch. Unfortunately the buffer storing the oid was
- overwritten with a new oid before ORIG_HEAD was created. The buffer is
- also used when writing .git/rebase-merge/orig-head which is used by
- rebase --abort to restore the previous head. Luckily that file is
- written before the buffer is overwritten. As we want the full oid
- find_unique_abbrev() is replaced with oid_to_hex_r() rather than
- find_unique_abbrev_r().
+ After rebasing, ORIG_HEAD is supposed to point to the old HEAD of the
+ rebased branch. The code used find_unique_abbrev() to obtain the
+ object name of the old HEAD and wrote to both
+ .git/rebase-merge/orig-head (used by rebase --abort to go back to
+ the previous state) and to ORIG_HEAD. The buffer find_unique_abbrev()
+ gives back is volatile, unfortunately, and was overwritten after the
+ former file is written but before ORIG_FILE is written, leaving an
+ incorrect object name in it.
+
+ Avoid relying on the volatile buffer of find_unique_abbrev(), and
+ instead supply our own buffer to keep the object name.

I think that all of the users of head_hash should actually be using opts->orig_head instead as passing a string rather than a struct object_id around is a hang over from the scripted implementation. This patch just fixes the immediate bug and adds a regression test based on
- Caspar's reproduction example. The users will be converted to use
+ Caspar's reproduction example[1]. The users will be converted to use
struct object_id and head_hash removed in the next few commits.

+ [1] https://lore.kernel.org/git/CAFzd1+7PDg2PZgKw7U0kdepdYuoML9wSN4kofmB_-8NHrbbrHg@mail.gmail.com
+

@@ builtin/rebase.c: static int edit_todo_file(unsigned flags)
const char *shorthead;

- *head_hash = find_unique_abbrev(orig_head, GIT_MAX_HEXSZ);
-+ oid_to_hex_r(head_hash, orig_head);
++ find_unique_abbrev_r(head_hash, orig_head, GIT_MAX_HEXSZ);
*revisions = xstrfmt("%s...%s", oid_to_hex(&base_rev->object.oid),
- *head_hash);
+ head_hash);

 2:  589aed08f1 = 2:  da499f1e4c rebase -i: use struct object_id rather than looking up commit
 3:  11494a7b79 ! 3:  076d212915 rebase -i: use struct object_id when writing state

@@ builtin/rebase.c: static int get_revision_ranges(struct commit *upstream, struct
{
FILE *interactive;

+@@ builtin/rebase.c: static int do_interactive_rebase(struct rebase_options *opts, unsigned flags) +
+ if (init_basic_state(&replay,
+ opts->head_name ? opts->head_name : "detached HEAD",
+- opts->onto, head_hash)) {
++ opts->onto, &opts->orig_head)) {
+ free(revisions);
+ free(shortrevisions);
+

## sequencer.c ##
@@ sequencer.c: static void write_strategy_opts(struct replay_opts *opts)
4: ed78f8628a ! 4: faae3ccff5 rebase -i: simplify get_revision_ranges()

     @@ Commit message
          use a opts->orig_head instead we can stop returning head_hash from
          get_revision_ranges().
      
     +    Because we want to pass the full object names back to the caller in
     +    `revisions` the find_unique_abbrev_r() call that was used to initialize
     +    `head_hash` is replaced with oid_to_hex().
     +
          Signed-off-by: Phillip Wood <[email protected]>
      
       ## builtin/rebase.c ##
     @@ builtin/rebase.c: static int edit_todo_file(unsigned flags)
       	struct commit *base_rev = upstream ? upstream : onto;
       	const char *shorthead;
       
     --	oid_to_hex_r(head_hash, orig_head);
     +-	find_unique_abbrev_r(head_hash, orig_head, GIT_MAX_HEXSZ);
       	*revisions = xstrfmt("%s...%s", oid_to_hex(&base_rev->object.oid),
      -						   head_hash);
      +			     oid_to_hex(orig_head));
     @@ builtin/rebase.c: static int do_interactive_rebase(struct rebase_options *opts,
       		return -1;
       
       	if (init_basic_state(&replay,
     - 			     opts->head_name ? opts->head_name : "detached HEAD",
     --			     opts->onto, head_hash)) {
     -+			     opts->onto, &opts->orig_head)) {
     - 		free(revisions);
     - 		free(shortrevisions);
     - 

gitgitgadget