[RFC,01/11] sequencer: always discard index after checkout

Commit Message

Phillip Wood March 19, 2019, 7:03 p.m. UTC

From: Phillip Wood <[e-mail]>

As the checkout runs in a separate process our index will be out of date
so it should be discarded. The existing callers are not doing this
consistently so do it here to avoid the callers having to worry about
it.

This fixes some test failures that happen if do_interactive_rebase() is
called without forking rebase--interactive which we will implement
shortly. Running

  git rebase -i master topic

starting on master created empty todo lists because all the commits in
topic were marked as cherry-picks. After topic was checked out in
prepare_branch_to_be_rebased() the working tree contained the contents
from topic but the index contained master and the cache entries were
still valid. This meant that diff_populate_filespec() which loads the
blobs when calculating patch-id's ended up reading the contents for
master from the working tree which actually contained topic.

Signed-off-by: Phillip Wood <[e-mail]>
---

Notes:
    It would perhaps be better to pass around the_index rather than
    the_repository

 builtin/rebase--interactive.c |  2 +-
 sequencer.c                   | 27 +++++++++++++++++----------
 sequencer.h                   |  3 ++-
 3 files changed, 20 insertions(+), 12 deletions(-)

Comments

Duy Nguyen March 20, 2019, 1:50 a.m. UTC | #1

On Wed, Mar 20, 2019 at 2:04 AM Phillip Wood <[e-mail]> wrote:
>     It would perhaps be better to pass around the_index rather than
>     the_repository

Not by a large margin. For sequencer.c most operations require more
than just the index and passing 'struct repository *' around has been
the norm. And as soon as you need to load the index back (not sure if
you should do it here btw, after discard_index, since we have the
index loaded before) you need 'struct repository' not 'struct
index_state'.

>  builtin/rebase--interactive.c |  2 +-
>  sequencer.c                   | 27 +++++++++++++++++----------
>  sequencer.h                   |  3 ++-
>  3 files changed, 20 insertions(+), 12 deletions(-)

Phillip Wood March 21, 2019, 2:35 p.m. UTC | #2

On 20/03/2019 01:50, Duy Nguyen wrote:
> On Wed, Mar 20, 2019 at 2:04 AM Phillip Wood <[e-mail]> wrote:
>>      It would perhaps be better to pass around the_index rather than
>>      the_repository
> 
> Not by a large margin. For sequencer.c most operations require more
> than just the index and passing 'struct repository *' around has been
> the norm. And as soon as you need to load the index back (not sure if
> you should do it here btw, after discard_index, since we have the
> index loaded before) you need 'struct repository' not 'struct
> index_state'.

Thanks, I think I'll leave it as it is then. After we checkout the new 
base we reload the index in the loop that picks the commits. For 'rebase 
<upstream> <branch>' after we checkout <branch> we create the todo-list 
which involves a revision walk and then checkout the new base. I'm not 
entirely sure if it needs reloading before we create the todo list but I 
think it probably not as I don't think rebase--interactive.c loads the 
index (which would explain why this only becomes an issue when we stop 
forking rebase--interactive from rebase because then rebase.c has loaded 
the index to check there are no uncommitted changes)

Best Wishes

Phillip



> 
>>   builtin/rebase--interactive.c |  2 +-
>>   sequencer.c                   | 27 +++++++++++++++++----------
>>   sequencer.h                   |  3 ++-
>>   3 files changed, 20 insertions(+), 12 deletions(-)

Patch

diff --git a/builtin/rebase--interactive.c b/builtin/rebase--interactive.c
index 4535523bf5..d1a4ac1b84 100644
--- a/builtin/rebase--interactive.c
+++ b/builtin/rebase--interactive.c
@@ -171,7 +171,7 @@  static int do_interactive_rebase(struct replay_opts *opts, unsigned flags,
 	struct argv_array make_script_args = ARGV_ARRAY_INIT;
 	struct todo_list todo_list = TODO_LIST_INIT;
 
-	if (prepare_branch_to_be_rebased(opts, switch_to))
+	if (prepare_branch_to_be_rebased(the_repository, opts, switch_to))
 		return -1;
 
 	if (get_revision_ranges(upstream, onto, &head_hash,
diff --git a/sequencer.c b/sequencer.c
index 281a8ade19..ccc0160800 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -3418,10 +3418,11 @@  static const char *reflog_message(struct replay_opts *opts,
 	return buf.buf;
 }
 
-static int run_git_checkout(struct replay_opts *opts, const char *commit,
-			    const char *action)
+static int run_git_checkout(struct repository *r, struct replay_opts *opts,
+			    const char *commit, const char *action)
 {
 	struct child_process cmd = CHILD_PROCESS_INIT;
+	int ret;
 
 	cmd.git_cmd = 1;
 
@@ -3430,25 +3431,31 @@  static int run_git_checkout(struct replay_opts *opts, const char *commit,
 	argv_array_pushf(&cmd.env_array, GIT_REFLOG_ACTION "=%s", action);
 
 	if (opts->verbose)
-		return run_command(&cmd);
+		ret = run_command(&cmd);
 	else
-		return run_command_silent_on_success(&cmd);
+		ret = run_command_silent_on_success(&cmd);
+
+	if (!ret)
+		discard_index(r->index);
+
+	return ret;
 }
 
-int prepare_branch_to_be_rebased(struct replay_opts *opts, const char *commit)
+int prepare_branch_to_be_rebased(struct repository *r, struct replay_opts *opts,
+				 const char *commit)
 {
 	const char *action;
 
 	if (commit && *commit) {
 		action = reflog_message(opts, "start", "checkout %s", commit);
-		if (run_git_checkout(opts, commit, action))
+		if (run_git_checkout(r, opts, commit, action))
 			return error(_("could not checkout %s"), commit);
 	}
 
 	return 0;
 }
 
-static int checkout_onto(struct replay_opts *opts,
+static int checkout_onto(struct repository *r, struct replay_opts *opts,
 			 const char *onto_name, const char *onto,
 			 const char *orig_head)
 {
@@ -3458,7 +3465,7 @@  static int checkout_onto(struct replay_opts *opts,
 	if (get_oid(orig_head, &oid))
 		return error(_("%s: not a valid OID"), orig_head);
 
-	if (run_git_checkout(opts, onto, action)) {
+	if (run_git_checkout(r, opts, onto, action)) {
 		apply_autostash(opts);
 		sequencer_remove_state(opts);
 		return error(_("could not detach HEAD"));
@@ -4786,7 +4793,7 @@  int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla
 	if (todo_list_parse_insn_buffer(r, new_todo.buf.buf, &new_todo) ||
 	    todo_list_check(todo_list, &new_todo)) {
 		fprintf(stderr, _(edit_todo_list_advice));
-		checkout_onto(opts, onto_name, onto, orig_head);
+		checkout_onto(r, opts, onto_name, onto, orig_head);
 		todo_list_release(&new_todo);
 
 		return -1;
@@ -4805,7 +4812,7 @@  int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla
 
 	todo_list_release(&new_todo);
 
-	if (checkout_onto(opts, onto_name, oid_to_hex(&oid), orig_head))
+	if (checkout_onto(r, opts, onto_name, oid_to_hex(&oid), orig_head))
 		return -1;
 
 	if (require_clean_work_tree(r, "rebase", "", 1, 1))
diff --git a/sequencer.h b/sequencer.h
index a515ee4457..6c55aa4200 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -175,7 +175,8 @@  void commit_post_rewrite(struct repository *r,
 			 const struct commit *current_head,
 			 const struct object_id *new_head);
 
-int prepare_branch_to_be_rebased(struct replay_opts *opts, const char *commit);
+int prepare_branch_to_be_rebased(struct repository *r, struct replay_opts *opts,
+				 const char *commit);
 
 #define SUMMARY_INITIAL_COMMIT   (1 << 0)
 #define SUMMARY_SHOW_AUTHOR_DATE (1 << 1)