[PATCH v2 0/4] rebase -i: fix ORIG_HEAD handling

["Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com>]

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 Johannes.Schindelin@gmx.de
[Johannes.Schindelin@gmx.de], Caspar Duregger herr.kaste@gmail.com
[herr.kaste@gmail.com]

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
     +
          Reported-by: Caspar Duregger <herr.kaste@gmail.com>
          Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
      
     @@ 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 <phillip.wood@dunelm.org.uk>
      
       ## 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