[PATCH 1/2] branch.c: remove explicit reference to the_repository

Signed-off-by: Nguyễn Thái Ngọc Duy <redacted>
---
 branch.c           | 22 ++++++++++++----------
 branch.h           |  7 +++++--
 builtin/am.c       |  2 +-
 builtin/branch.c   |  6 ++++--
 builtin/checkout.c |  5 +++--
 builtin/reset.c    |  2 +-
 6 files changed, 26 insertions(+), 18 deletions(-)

diff --git a/branch.c b/branch.c
index ecd710d730..0baa1f6877 100644
--- a/branch.c
+++ b/branch.c

@@ -244,7 +244,8 @@ N_("\n"
 "will track its remote counterpart, you may want to use\n"
 "\"git push -u\" to set the upstream config as you push.");
 
-void create_branch(const char *name, const char *start_name,
+void create_branch(struct repository *repo,
+		   const char *name, const char *start_name,
 		   int force, int clobber_head_ok, int reflog,
 		   int quiet, enum branch_track track)
 {

@@ -302,7 +303,8 @@ void create_branch(const char *name, const char *start_name,
 		break;
 	}
 
-	if ((commit = lookup_commit_reference(the_repository, &oid)) == NULL)
+	commit = lookup_commit_reference(repo, &oid);
+	if (!commit)
 		die(_("Not a valid branch point: '%s'."), start_name);
 	oidcpy(&oid, &commit->object.oid);
 

@@ -338,15 +340,15 @@ void create_branch(const char *name, const char *start_name,
 	free(real_ref);
 }
 
-void remove_branch_state(void)
+void remove_branch_state(struct repository *repo)
 {
-	unlink(git_path_cherry_pick_head(the_repository));
-	unlink(git_path_revert_head(the_repository));
-	unlink(git_path_merge_head(the_repository));
-	unlink(git_path_merge_rr(the_repository));
-	unlink(git_path_merge_msg(the_repository));
-	unlink(git_path_merge_mode(the_repository));
-	unlink(git_path_squash_msg(the_repository));
+	unlink(git_path_cherry_pick_head(repo));
+	unlink(git_path_revert_head(repo));
+	unlink(git_path_merge_head(repo));
+	unlink(git_path_merge_rr(repo));
+	unlink(git_path_merge_msg(repo));
+	unlink(git_path_merge_mode(repo));
+	unlink(git_path_squash_msg(repo));
 }
 
 void die_if_checked_out(const char *branch, int ignore_current_worktree)

diff --git a/branch.h b/branch.h
index 473d0a93e9..14d8282927 100644
--- a/branch.h
+++ b/branch.h

@@ -3,6 +3,8 @@
 
 /* Functions for acting on the information about branches. */
 
+struct repository;
+
 /*
  * Creates a new branch, where:
  *

@@ -24,7 +26,8 @@
  *     that start_name is a tracking branch for (if any).
  *
  */
-void create_branch(const char *name, const char *start_name,
+void create_branch(struct repository *repo,
+		   const char *name, const char *start_name,
 		   int force, int clobber_head_ok,
 		   int reflog, int quiet, enum branch_track track);
 

@@ -47,7 +50,7 @@ extern int validate_new_branchname(const char *name, struct strbuf *ref, int for
  * Remove information about the state of working on the current
  * branch. (E.g., MERGE_HEAD)
  */
-void remove_branch_state(void);
+void remove_branch_state(struct repository *);
 
 /*
  * Configure local branch "local" as downstream to branch "remote"

diff --git a/builtin/am.c b/builtin/am.c
index 2c19e69f58..7abe39939e 100644
--- a/builtin/am.c
+++ b/builtin/am.c

@@ -2017,7 +2017,7 @@ static int clean_index(const struct object_id *head, const struct object_id *rem
 	if (merge_tree(remote_tree))
 		return -1;
 
-	remove_branch_state();
+	remove_branch_state(the_repository);
 
 	return 0;
 }

diff --git a/builtin/branch.c b/builtin/branch.c
index 4fc55c3508..e04d528ce1 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c

@@ -795,7 +795,8 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 		 * create_branch takes care of setting up the tracking
 		 * info and making sure new_upstream is correct
 		 */
-		create_branch(branch->name, new_upstream, 0, 0, 0, quiet, BRANCH_TRACK_OVERRIDE);
+		create_branch(the_repository, branch->name, new_upstream,
+			      0, 0, 0, quiet, BRANCH_TRACK_OVERRIDE);
 	} else if (unset_upstream) {
 		struct branch *branch = branch_get(argv[0]);
 		struct strbuf buf = STRBUF_INIT;

@@ -831,7 +832,8 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 		if (track == BRANCH_TRACK_OVERRIDE)
 			die(_("the '--set-upstream' option is no longer supported. Please use '--track' or '--set-upstream-to' instead."));
 
-		create_branch(argv[0], (argc == 2) ? argv[1] : head,
+		create_branch(the_repository,
+			      argv[0], (argc == 2) ? argv[1] : head,
 			      force, 0, reflog, quiet, track);
 
 	} else

diff --git a/builtin/checkout.c b/builtin/checkout.c
index 516136a23a..4756018272 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c

@@ -653,7 +653,8 @@ static void update_refs_for_switch(const struct checkout_opts *opts,
 			free(refname);
 		}
 		else
-			create_branch(opts->new_branch, new_branch_info->name,
+			create_branch(the_repository,
+				      opts->new_branch, new_branch_info->name,
 				      opts->new_branch_force ? 1 : 0,
 				      opts->new_branch_force ? 1 : 0,
 				      opts->new_branch_log,

@@ -711,7 +712,7 @@ static void update_refs_for_switch(const struct checkout_opts *opts,
 				delete_reflog(old_branch_info->path);
 		}
 	}
-	remove_branch_state();
+	remove_branch_state(the_repository);
 	strbuf_release(&msg);
 	if (!opts->quiet &&
 	    (new_branch_info->path || (!opts->force_detach && !strcmp(new_branch_info->name, "HEAD"))))

diff --git a/builtin/reset.c b/builtin/reset.c
index 11cd0dcb8c..d90ccdb839 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c

@@ -399,7 +399,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
 			print_new_head_line(lookup_commit_reference(the_repository, &oid));
 	}
 	if (!pathspec.nr)
-		remove_branch_state();
+		remove_branch_state(the_repository);
 
 	return update_ref_status;
 }

-- 
2.18.0.1004.g6639190530

--quit is supposed to be --abort but without restoring HEAD. Leaving
CHERRY_PICK_HEAD behind could make other commands mistake that
cherry-pick is still ongoing (e.g. "git commit --amend" will refuse to
work). Clean it too.

For abort, this job of deleting CHERRY_PICK_HEAD is on "git reset" so
we don't need to do anything else.

Signed-off-by: Nguyễn Thái Ngọc Duy <redacted>
---
 builtin/revert.c                | 9 +++++++--
 t/t3510-cherry-pick-sequence.sh | 3 ++-
 2 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/builtin/revert.c b/builtin/revert.c
index 76f0a35b07..e94a4ead2b 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c

@@ -7,6 +7,7 @@
 #include "rerere.h"
 #include "dir.h"
 #include "sequencer.h"
+#include "branch.h"
 
 /*
  * This implements the builtins revert and cherry-pick.

@@ -191,8 +192,12 @@ static int run_sequencer(int argc, const char **argv, struct replay_opts *opts)
 	opts->gpg_sign = xstrdup_or_null(opts->gpg_sign);
 	opts->strategy = xstrdup_or_null(opts->strategy);
 
-	if (cmd == 'q')
-		return sequencer_remove_state(opts);
+	if (cmd == 'q') {
+		int ret = sequencer_remove_state(opts);
+		if (!ret)
+			remove_branch_state(the_repository);
+		return ret;
+	}
 	if (cmd == 'c')
 		return sequencer_continue(opts);
 	if (cmd == 'a')

diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh
index 3505b6aa14..9d121f8ce6 100755
--- a/t/t3510-cherry-pick-sequence.sh
+++ b/t/t3510-cherry-pick-sequence.sh

@@ -103,7 +103,8 @@ test_expect_success '--quit cleans up sequencer state' '
 	pristine_detach initial &&
 	test_expect_code 1 git cherry-pick base..picked &&
 	git cherry-pick --quit &&
-	test_path_is_missing .git/sequencer
+	test_path_is_missing .git/sequencer &&
+	test_path_is_missing .git/CHERRY_PICK_HEAD
 '
 
 test_expect_success '--quit keeps HEAD and conflicted index intact' '

-- 
2.18.0.1004.g6639190530

On Wed, Aug 15, 2018 at 9:24 AM Nguyễn Thái Ngọc Duy [off-list ref] wrote:

The patch looks good, but since this touches multiple .c files, I
think I'd s/branch.c/branch/ in the subject line.

On Wed, Aug 15, 2018 at 6:48 PM Elijah Newren [off-list ref] wrote:

On Wed, Aug 15, 2018 at 9:24 AM Nguyễn Thái Ngọc Duy [off-list ref] wrote:

The patch looks good, but since this touches multiple .c files, I
think I'd s/branch.c/branch/ in the subject line.

It is about removing the_repository from branch.c though. As much as I
want to completely erase the_repository, that would take a lot more
work.
-- 
Duy

On Wed, Aug 15, 2018 at 9:53 AM Duy Nguyen [off-list ref] wrote:

On Wed, Aug 15, 2018 at 6:48 PM Elijah Newren [off-list ref] wrote:

quoted

On Wed, Aug 15, 2018 at 9:24 AM Nguyễn Thái Ngọc Duy [off-list ref] wrote:

The patch looks good, but since this touches multiple .c files, I
think I'd s/branch.c/branch/ in the subject line.

It is about removing the_repository from branch.c though. As much as I
want to completely erase the_repository, that would take a lot more
work.

What is your envisioned end state?

1) IMHO we'd first want to put the_repository in place where needed,
2) then start replacing s/the_repository/a repository/ in /
3) builtin/ is not critical, but we'd want to do that later
4) eventually (in the very long run), we'd change the signature of
  all commands from cmd_foo(int argc, char argv, char *prefix)
  to cmd_foo(int argc, char argv, struct repository *repo)

you seem to be interested in removing the_repository from branch.c,
but not as much from bultin/ for now, which is roughly step 2 in that plan?

On Wed, Aug 15, 2018 at 9:23 AM Nguyễn Thái Ngọc Duy [off-list ref] wrote:

Technically you would not need patch 1 in this series, as you could call
remove_branch_state(void) as before that patch to do the same thing here.
I guess that patch 1 is more of a drive by cleanup, then?

It looks a bit interesting as sequencer_remove_state actually
takes no arguments and assumes the_repository, but I guess that is fine.

I wondered if we need to have this patch for 'a' as well, and it looks like
which does a sequencer_rollback, which is just some logic before
attempting sequencer_remove_state. So I'd think remove_branch_state
could be done in sequencer_remove_state as well?

Or are there functions that would definitely not want sequencer_remove_state
after sequencer_remove_state?

Going down on that I just realize sequencer_remove_state could also
be returning void, as of now it always returns 0, so the condition here
with !ret is just confusing the reader?

Thanks,
Stefan

On Wed, Aug 15, 2018 at 6:58 PM Stefan Beller [off-list ref] wrote:

On Wed, Aug 15, 2018 at 9:53 AM Duy Nguyen [off-list ref] wrote:

quoted

On Wed, Aug 15, 2018 at 6:48 PM Elijah Newren [off-list ref] wrote:

quoted

On Wed, Aug 15, 2018 at 9:24 AM Nguyễn Thái Ngọc Duy [off-list ref] wrote:

The patch looks good, but since this touches multiple .c files, I
think I'd s/branch.c/branch/ in the subject line.

It is about removing the_repository from branch.c though. As much as I
want to completely erase the_repository, that would take a lot more
work.

What is your envisioned end state?

1) IMHO we'd first want to put the_repository in place where needed,
2) then start replacing s/the_repository/a repository/ in /
3) builtin/ is not critical, but we'd want to do that later
4) eventually (in the very long run), we'd change the signature of
  all commands from cmd_foo(int argc, char argv, char *prefix)
  to cmd_foo(int argc, char argv, struct repository *repo)

you seem to be interested in removing the_repository from branch.c,
but not as much from bultin/ for now, which is roughly step 2 in that plan?

Yes. Though I think step 4 is to make setup_git_directory() and
enter_repo() return a 'struct repository *'. This means if you have
not called either function and not take the repo as an argument, you
do not have access to any repository.

I've been trying to make setup_git_directory() not touch any global
variables, which would be the first step towards that. Unfortunately
I'm currently stopped at the one exception called "git init".
-- 
Duy

On Wed, Aug 15, 2018 at 7:09 PM Stefan Beller [off-list ref] wrote:

Technically you would not need patch 1 in this series, as you could call
remove_branch_state(void) as before that patch to do the same thing here.
I guess that patch 1 is more of a drive by cleanup, then?

Yes.

It looks a bit interesting as sequencer_remove_state actually
takes no arguments and assumes the_repository, but I guess that is fine.

Don't worry. My effort to kill the_index will make sequencer.c take
'struct repository *' (its operations are so wide that passing just
struct index_state * does not make sense).

I wondered if we need to have this patch for 'a' as well, and it looks like
which does a sequencer_rollback, which is just some logic before
attempting sequencer_remove_state. So I'd think remove_branch_state
could be done in sequencer_remove_state as well?

sequencer_rollback does not need this remove_branch_state() line
because it calls reset_for_rollback() which does this deletion. Patch
1/1 kinda hints at that because it touches all remove_branch_state()
;-)

Another part of the reason I did not put remove_branch_state() in
sequencer_remove_state() is I was not sure if it could be used in a
different way (I did not study all of its call sites and am not very
familiar with sequencer code).

Or are there functions that would definitely not want sequencer_remove_state
after sequencer_remove_state?

Going down on that I just realize sequencer_remove_state could also
be returning void, as of now it always returns 0, so the condition here
with !ret is just confusing the reader?

-- 
Duy

On Wed, Aug 15, 2018 at 10:18 AM Duy Nguyen [off-list ref] wrote:

On Wed, Aug 15, 2018 at 7:09 PM Stefan Beller [off-list ref] wrote:

quoted

Technically you would not need patch 1 in this series, as you could call
remove_branch_state(void) as before that patch to do the same thing here.
I guess that patch 1 is more of a drive by cleanup, then?

Yes.

quoted

It looks a bit interesting as sequencer_remove_state actually
takes no arguments and assumes the_repository, but I guess that is fine.

Don't worry. My effort to kill the_index will make sequencer.c take
'struct repository *' (its operations are so wide that passing just
struct index_state * does not make sense).

Cool! I'll give that series a read, then! Thanks for killing the_index!

quoted

I wondered if we need to have this patch for 'a' as well, and it looks like
which does a sequencer_rollback, which is just some logic before
attempting sequencer_remove_state. So I'd think remove_branch_state
could be done in sequencer_remove_state as well?

sequencer_rollback does not need this remove_branch_state() line
because it calls reset_for_rollback() which does this deletion. Patch
1/1 kinda hints at that because it touches all remove_branch_state()
;-)

Gah! I am being slow.