Re: [PATCH 1/2] branch.c: remove explicit reference to the_repository
On Wed, Aug 15, 2018 at 7:38 PM Junio C Hamano wrote: > > Duy Nguyen 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
Re: [PATCH 1/2] branch.c: remove explicit reference to the_repository
Duy Nguyen 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
On Wed, Aug 15, 2018 at 7:20 PM Junio C Hamano 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 ". sequencer.c as its new home then? -- Duy
Re: [PATCH 1/2] branch.c: remove explicit reference to the_repository
Duy Nguyen writes: > On Wed, Aug 15, 2018 at 6:48 PM Elijah Newren wrote: >> >> On Wed, Aug 15, 2018 at 9:24 AM Nguyễn Thái Ngọc Duy >> 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 ".
Re: [PATCH 1/2] branch.c: remove explicit reference to the_repository
On Wed, Aug 15, 2018 at 6:58 PM Stefan Beller wrote: > > On Wed, Aug 15, 2018 at 9:53 AM Duy Nguyen wrote: > > > > On Wed, Aug 15, 2018 at 6:48 PM Elijah Newren wrote: > > > > > > On Wed, Aug 15, 2018 at 9:24 AM Nguyễn Thái Ngọc Duy > > > 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 1/2] branch.c: remove explicit reference to the_repository
On Wed, Aug 15, 2018 at 9:53 AM Duy Nguyen wrote: > > On Wed, Aug 15, 2018 at 6:48 PM Elijah Newren wrote: > > > > On Wed, Aug 15, 2018 at 9:24 AM Nguyễn Thái Ngọc Duy > > 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 1/2] branch.c: remove explicit reference to the_repository
On Wed, Aug 15, 2018 at 6:48 PM Elijah Newren wrote: > > On Wed, Aug 15, 2018 at 9:24 AM Nguyễn Thái Ngọc Duy > 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
On Wed, Aug 15, 2018 at 9:24 AM Nguyễn Thái Ngọc Duy 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
> ---
> 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, )) == NULL)
> + commit = lookup_commit_reference(repo, );
> + if (!commit)
> die(_("Not a valid branch point: '%s'."), start_name);
> oidcpy(, >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."));
>
> -
