git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: "Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com>
To: git@vger.kernel.org
Cc: Phillip Wood <phillip.wood123@gmail.com>,
	Phillip Wood <phillip.wood@dunelm.org.uk>
Subject: [PATCH v2 0/4] rebase -i: fix ORIG_HEAD handling
Date: Wed, 04 Nov 2020 15:29:36 +0000	[thread overview]
Message-ID: <pull.773.v2.git.1604503780.gitgitgadget@gmail.com> (raw)
In-Reply-To: <pull.773.git.1603807337.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

  parent reply	other threads:[~2020-11-04 15:29 UTC|newest]

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-10-27 14:02 [PATCH " Phillip Wood via GitGitGadget
2020-10-27 14:02 ` [PATCH 1/4] rebase -i: stop overwriting ORIG_HEAD buffer Phillip Wood via GitGitGadget
2020-10-27 21:10   ` Junio C Hamano
2020-10-31 10:55     ` Phillip Wood
2020-11-02 19:40     ` herr.kaste
2020-11-03  0:21       ` Junio C Hamano
2020-11-03 11:02         ` herr.kaste
2020-10-27 14:02 ` [PATCH 2/4] rebase -i: use struct object_id rather than looking up commit Phillip Wood via GitGitGadget
2020-10-27 14:02 ` [PATCH 3/4] rebase -i: use struct object_id when writing state Phillip Wood via GitGitGadget
2020-10-27 14:02 ` [PATCH 4/4] rebase -i: simplify get_revision_ranges() Phillip Wood via GitGitGadget
2020-11-04 15:29 ` Phillip Wood via GitGitGadget [this message]
2020-11-04 15:29   ` [PATCH v2 1/4] rebase -i: stop overwriting ORIG_HEAD buffer Phillip Wood via GitGitGadget
2020-11-04 15:29   ` [PATCH v2 2/4] rebase -i: use struct object_id rather than looking up commit Phillip Wood via GitGitGadget
2020-11-04 15:29   ` [PATCH v2 3/4] rebase -i: use struct object_id when writing state Phillip Wood via GitGitGadget
2020-11-04 15:29   ` [PATCH v2 4/4] rebase -i: simplify get_revision_ranges() Phillip Wood via GitGitGadget

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=pull.773.v2.git.1604503780.gitgitgadget@gmail.com \
    --to=gitgitgadget@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=phillip.wood123@gmail.com \
    --cc=phillip.wood@dunelm.org.uk \
    --subject='Re: [PATCH v2 0/4] rebase -i: fix ORIG_HEAD handling' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).