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

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

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 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

[PATCH 2/2] cherry-pick: fix --quit not deleting CHERRY_PICK_HEAD

[Nguyễn Thái Ngọc Duy]

--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 <pclouds@gmail.com>
---
 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

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

[Elijah Newren]

On Wed, Aug 15, 2018 at 9:24 AM Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:

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

>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  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
>

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

[Duy Nguyen]

On Wed, Aug 15, 2018 at 6:48 PM Elijah Newren <newren@gmail.com> wrote:
>
> On Wed, Aug 15, 2018 at 9:24 AM Nguyễn Thái Ngọc Duy <pclouds@gmail.com> 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

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

[Stefan Beller]

On Wed, Aug 15, 2018 at 9:53 AM Duy Nguyen <pclouds@gmail.com> wrote:
>
> On Wed, Aug 15, 2018 at 6:48 PM Elijah Newren <newren@gmail.com> wrote:
> >
> > On Wed, Aug 15, 2018 at 9:24 AM Nguyễn Thái Ngọc Duy <pclouds@gmail.com> 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?

Re: [PATCH 2/2] cherry-pick: fix --quit not deleting CHERRY_PICK_HEAD

[Stefan Beller]

On Wed, Aug 15, 2018 at 9:23 AM Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:
>
> --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 <pclouds@gmail.com>
> ---
>  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);

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

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

[Duy Nguyen]

On Wed, Aug 15, 2018 at 6:58 PM Stefan Beller <sbeller@google.com> wrote:
>
> On Wed, Aug 15, 2018 at 9:53 AM Duy Nguyen <pclouds@gmail.com> wrote:
> >
> > On Wed, Aug 15, 2018 at 6:48 PM Elijah Newren <newren@gmail.com> wrote:
> > >
> > > On Wed, Aug 15, 2018 at 9:24 AM Nguyễn Thái Ngọc Duy <pclouds@gmail.com> 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

Re: [PATCH 2/2] cherry-pick: fix --quit not deleting CHERRY_PICK_HEAD

[Duy Nguyen]

On Wed, Aug 15, 2018 at 7:09 PM Stefan Beller <sbeller@google.com> 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

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

[Junio C Hamano]

Duy Nguyen <pclouds@gmail.com> writes:

> On Wed, Aug 15, 2018 at 6:48 PM Elijah Newren <newren@gmail.com> wrote:
>>
>> On Wed, Aug 15, 2018 at 9:24 AM Nguyễn Thái Ngọc Duy <pclouds@gmail.com> 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.

I do not think this is about removing the_repository from branch.c;
it is primarily about allowing create_branch() to work on an
arbitrary repository instance.

I also do not think remove_branch_state() function belongs to
branch.c in the first place.  The state it is clearing is not even
about a "branch".  It is state left by the last command that stopped
in the middle; its only callers are "reset", "am --abort/--skip" and
"checkout <another-branch>".

Re: [PATCH 2/2] cherry-pick: fix --quit not deleting CHERRY_PICK_HEAD

[Stefan Beller]

On Wed, Aug 15, 2018 at 10:18 AM Duy Nguyen <pclouds@gmail.com> wrote:
>
> On Wed, Aug 15, 2018 at 7:09 PM Stefan Beller <sbeller@google.com> 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).

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

> > 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.

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

[Duy Nguyen]

On Wed, Aug 15, 2018 at 7:20 PM Junio C Hamano <gitster@pobox.com> wrote:
> I also do not think remove_branch_state() function belongs to
> branch.c in the first place.  The state it is clearing is not even
> about a "branch".  It is state left by the last command that stopped
> in the middle; its only callers are "reset", "am --abort/--skip" and
> "checkout <another-branch>".

sequencer.c as its new home then?
-- 
Duy

Re: [PATCH 2/2] cherry-pick: fix --quit not deleting CHERRY_PICK_HEAD

[Junio C Hamano]

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

> --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 <pclouds@gmail.com>
> ---

Please do not hide this bugfix behind 1/2 that is likely to require
longer to cook than the fix itself.   And more importantly, it makes
it impossible to merge down the fix to the maintenance track, as I
do not think we'd want to merge 1/2 there.

Thanks.

>  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' '

Re: [PATCH 2/2] cherry-pick: fix --quit not deleting CHERRY_PICK_HEAD

[Duy Nguyen]

On Wed, Aug 15, 2018 at 7:26 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
>
> > --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 <pclouds@gmail.com>
> > ---
>
> Please do not hide this bugfix behind 1/2 that is likely to require
> longer to cook than the fix itself.   And more importantly, it makes
> it impossible to merge down the fix to the maintenance track, as I
> do not think we'd want to merge 1/2 there.

Oh sorry I did not think about that. Will drop 1/2 and send 2/2 as
standalone. But for the record, I think this has been a bug since the
introduction of --quit in this command (back when it was still called
--reset).
-- 
Duy

Re: [PATCH 2/2] cherry-pick: fix --quit not deleting CHERRY_PICK_HEAD

[Junio C Hamano]

Duy Nguyen <pclouds@gmail.com> writes:

> On Wed, Aug 15, 2018 at 7:26 PM Junio C Hamano <gitster@pobox.com> wrote:
>>
>> Please do not hide this bugfix behind 1/2 that is likely to require
>> longer to cook than the fix itself.   And more importantly, it makes
>> it impossible to merge down the fix to the maintenance track, as I
>> do not think we'd want to merge 1/2 there.
>
> Oh sorry I did not think about that. Will drop 1/2 and send 2/2 as
> standalone. But for the record, I think this has been a bug since the
> introduction of --quit in this command (back when it was still called
> --reset).

If this bug has been there longer, it is a reason why we may want to
ensure that the fix applies to even older maintenance tracks.

Thanks.

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

[Junio C Hamano]

Duy Nguyen <pclouds@gmail.com> writes:

>> 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.

That part is understandable if somewhat hand-wavy, but it is not
clear how you can lose the prefix and still keep things like
OPT_FILENAME() working correctly.

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

[Duy Nguyen]

On Wed, Aug 15, 2018 at 7:38 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Duy Nguyen <pclouds@gmail.com> writes:
>
> >> 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.
>
> That part is understandable if somewhat hand-wavy, but it is not
> clear how you can lose the prefix and still keep things like
> OPT_FILENAME() working correctly.

I haven't worked it all out yet, but I think setup_git_directory()
could still return it either as a separate argument or inside 'struct
repository'. Then parse_options() still takes the prefix like now, or
take the struct repository (the latter may be better because there's
also other option callbacks that need struct repo).
-- 
Duy

[PATCH v2] cherry-pick: fix --quit not deleting CHERRY_PICK_HEAD

[Nguyễn Thái Ngọc Duy]

--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. But let's add extra checks in
--abort tests to confirm.

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

diff --git a/builtin/revert.c b/builtin/revert.c
index 76f0a35b07..9a66720cfc 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();
+		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 b42cd66d3a..68b8c14e27 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' '
@@ -132,6 +133,7 @@ test_expect_success '--abort to cancel multiple cherry-pick' '
 	test_expect_code 1 git cherry-pick base..anotherpick &&
 	git cherry-pick --abort &&
 	test_path_is_missing .git/sequencer &&
+	test_path_is_missing .git/CHERRY_PICK_HEAD &&
 	test_cmp_rev initial HEAD &&
 	git update-index --refresh &&
 	git diff-index --exit-code HEAD
@@ -142,6 +144,7 @@ test_expect_success '--abort to cancel single cherry-pick' '
 	test_expect_code 1 git cherry-pick picked &&
 	git cherry-pick --abort &&
 	test_path_is_missing .git/sequencer &&
+	test_path_is_missing .git/CHERRY_PICK_HEAD &&
 	test_cmp_rev initial HEAD &&
 	git update-index --refresh &&
 	git diff-index --exit-code HEAD
@@ -162,6 +165,7 @@ test_expect_success 'cherry-pick --abort to cancel multiple revert' '
 	test_expect_code 1 git revert base..picked &&
 	git cherry-pick --abort &&
 	test_path_is_missing .git/sequencer &&
+	test_path_is_missing .git/CHERRY_PICK_HEAD &&
 	test_cmp_rev anotherpick HEAD &&
 	git update-index --refresh &&
 	git diff-index --exit-code HEAD
@@ -239,6 +243,7 @@ test_expect_success '--abort after last commit in sequence' '
 	test_expect_code 1 git cherry-pick base..picked &&
 	git cherry-pick --abort &&
 	test_path_is_missing .git/sequencer &&
+	test_path_is_missing .git/CHERRY_PICK_HEAD &&
 	test_cmp_rev initial HEAD &&
 	git update-index --refresh &&
 	git diff-index --exit-code HEAD
-- 
2.18.0.1004.g6639190530

Re: [PATCH v2] cherry-pick: fix --quit not deleting CHERRY_PICK_HEAD

[Junio C Hamano]

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

> --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. But let's add extra checks in
> --abort tests to confirm.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---

Thanks, this makes sense.

>  builtin/revert.c                | 9 +++++++--
>  t/t3510-cherry-pick-sequence.sh | 7 ++++++-
>  2 files changed, 13 insertions(+), 3 deletions(-)
>
> diff --git a/builtin/revert.c b/builtin/revert.c
> index 76f0a35b07..9a66720cfc 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();
> +		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 b42cd66d3a..68b8c14e27 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' '
> @@ -132,6 +133,7 @@ test_expect_success '--abort to cancel multiple cherry-pick' '
>  	test_expect_code 1 git cherry-pick base..anotherpick &&
>  	git cherry-pick --abort &&
>  	test_path_is_missing .git/sequencer &&
> +	test_path_is_missing .git/CHERRY_PICK_HEAD &&
>  	test_cmp_rev initial HEAD &&
>  	git update-index --refresh &&
>  	git diff-index --exit-code HEAD
> @@ -142,6 +144,7 @@ test_expect_success '--abort to cancel single cherry-pick' '
>  	test_expect_code 1 git cherry-pick picked &&
>  	git cherry-pick --abort &&
>  	test_path_is_missing .git/sequencer &&
> +	test_path_is_missing .git/CHERRY_PICK_HEAD &&
>  	test_cmp_rev initial HEAD &&
>  	git update-index --refresh &&
>  	git diff-index --exit-code HEAD
> @@ -162,6 +165,7 @@ test_expect_success 'cherry-pick --abort to cancel multiple revert' '
>  	test_expect_code 1 git revert base..picked &&
>  	git cherry-pick --abort &&
>  	test_path_is_missing .git/sequencer &&
> +	test_path_is_missing .git/CHERRY_PICK_HEAD &&
>  	test_cmp_rev anotherpick HEAD &&
>  	git update-index --refresh &&
>  	git diff-index --exit-code HEAD
> @@ -239,6 +243,7 @@ test_expect_success '--abort after last commit in sequence' '
>  	test_expect_code 1 git cherry-pick base..picked &&
>  	git cherry-pick --abort &&
>  	test_path_is_missing .git/sequencer &&
> +	test_path_is_missing .git/CHERRY_PICK_HEAD &&
>  	test_cmp_rev initial HEAD &&
>  	git update-index --refresh &&
>  	git diff-index --exit-code HEAD