branch.c: remove explicit reference to the_repository
PATCH 1/2 branch.c: remove explicit reference to the_repository
--- 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(-)
branch.c +12 -10
@@ -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)
branch.h +5 -2
@@ -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"
builtin/am.c +1 -1
@@ -2017,7 +2017,7 @@ static int clean_index(const struct object_id *head, const struct object_id *remif (merge_tree (remote_tree))return -1 ; -remove_branch_state (); +remove_branch_state (the_repository);return 0 ; }
builtin/branch.c +4 -2
@@ -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
builtin/checkout.c +3 -2
@@ -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" ))))
builtin/reset.c +1 -1
@@ -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; }
Signature · 1 line
2.18.0.1004.g6639190530
PATCH 2/2 cherry-pick: fix --quit not deleting CHERRY_PICK_HEAD
--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.
--- builtin/revert.c | 9 +++++++-- t/t3510-cherry-pick-sequence.sh | 3 ++- 2 files changed, 9 insertions(+), 3 deletions(-)
builtin/revert.c +7 -2
@@ -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' )
t/t3510-cherry-pick-sequence.sh +2 -1
@@ -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' '
Signature · 1 line
2.18.0.1004.g6639190530
Re: [PATCH 2/2] cherry-pick: fix --quit not deleting CHERRY_PICK_HEAD
Quoted 28 lines · Nguyễn Thái Ngọc Duy
--quit is supposed to be --abort but without restoring HEAD. Leaving
CHERRY_PICK_HEADbehind 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_HEADis on "git reset" so we don't need to do anything else.Signed-off-by Nguyễn Thái Ngọc Duy <[email protected]>--- builtin/revert.c | 9 +++++++-- t/t3510-cherry-pick-sequence.sh | 3 ++- 2 files changed, 9 insertions(+), 3 deletions(-)builtin/revert.c +1
-0@@ -7,6 +7,7 @@#include "rerere.h" #include "dir.h" #include "sequencer.h" +#include "branch.h" /* * This implements the builtins revert and cherry-pick.
builtin/revert.c static int run_sequencer(int argc, const char **argv, struct replay_opts *opts) · line 198
Quoted 9 lines
builtin/revert.c +4
-2@@ -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 2/2] cherry-pick: fix --quit not deleting CHERRY_PICK_HEAD
Quoted 3 lines · Stefan Beller
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 2 lines
It looks a bit interesting as
sequencer_remove_stateactually takes no arguments and assumesthe_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).
Quoted 4 lines
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 attemptingsequencer_remove_state. So I'd thinkremove_branch_statecould be done insequencer_remove_stateas 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).
Quoted 6 lines
Or are there functions that would definitely not want
sequencer_remove_stateaftersequencer_remove_state?Going down on that I just realize
sequencer_remove_statecould also be returning void, as of now it always returns 0, so the condition here with !ret is just confusing the reader?
Signature · 1 line
Duy
Re: [PATCH 2/2] cherry-pick: fix --quit not deleting CHERRY_PICK_HEAD
Quoted 6 lines · Duy Nguyen
Quoted 3 lines · Stefan Beller
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?
Quoted 8 lines
Yes.
Quoted 2 lines
It looks a bit interesting as
sequencer_remove_stateactually takes no arguments and assumesthe_repository, but I guess that is fine.Don't worry. My effort to kill
the_indexwill makesequencer.ctake 'struct repository *' (its operations are so wide that passing just structindex_state* does not make sense).
Cool! I'll give that series a read, then! Thanks for killing the_index!
Quoted 9 lines
Quoted 4 lines
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 attemptingsequencer_remove_state. So I'd thinkremove_branch_statecould be done insequencer_remove_stateas well?
sequencer_rollbackdoes not need thisremove_branch_state()line because it callsreset_for_rollback()which does this deletion. Patch 1/1 kinda hints at that because it touches allremove_branch_state();-)
Gah! I am being slow.
Re: [PATCH 2/2] cherry-pick: fix --quit not deleting CHERRY_PICK_HEAD
In reply to Nguyễn Thái Ngọc DuyQuoted 10 lines · Nguyễn Thái Ngọc Duy
--quit is supposed to be --abort but without restoring HEAD. Leaving
CHERRY_PICK_HEADbehind 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_HEADis on "git reset" so we don't need to do anything else.Signed-off-by Nguyễn Thái Ngọc Duy <[email protected]>---
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.
Quoted 45 lines
builtin/revert.c | 9 +++++++-- t/t3510-cherry-pick-sequence.sh | 3 ++- 2 files changed, 9 insertions(+), 3 deletions(-)builtin/revert.c +7
-2@@ -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' )t/t3510-cherry-pick-sequence.sh +2
-1@@ -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
Quoted 11 lines · Junio C Hamano
Quoted 8 lines · Nguyễn Thái Ngọc Duy
--quit is supposed to be --abort but without restoring HEAD. Leaving
CHERRY_PICK_HEADbehind 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_HEADis on "git reset" so we don't need to do anything else.
Quoted 7 lines
Quoted 2 lines
Signed-off-by Nguyễn Thái Ngọc Duy <[email protected]>---
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).
Signature · 1 line
Duy
Re: [PATCH 2/2] cherry-pick: fix --quit not deleting CHERRY_PICK_HEAD
Quoted 11 lines · Duy Nguyen
Quoted 5 lines · Junio C Hamano
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.
PATCH v2 1/1 cherry-pick: fix --quit not deleting CHERRY_PICK_HEAD
In reply to Duy Nguyen--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.
--- builtin/revert.c | 9 +++++++-- t/t3510-cherry-pick-sequence.sh | 7 ++++++- 2 files changed, 13 insertions(+), 3 deletions(-)
builtin/revert.c +7 -2
@@ -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' )
t/t3510-cherry-pick-sequence.sh +6 -1
@@ -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
Signature · 1 line
2.18.0.1004.g6639190530
Re: [PATCH v2] cherry-pick: fix --quit not deleting CHERRY_PICK_HEAD
Quoted 11 lines · Nguyễn Thái Ngọc Duy
--quit is supposed to be --abort but without restoring HEAD. Leaving
CHERRY_PICK_HEADbehind 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_HEADis 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 <[email protected]>---
Thanks, this makes sense.
Quoted 77 lines
builtin/revert.c | 9 +++++++-- t/t3510-cherry-pick-sequence.sh | 7 ++++++- 2 files changed, 13 insertions(+), 3 deletions(-)builtin/revert.c +7
-2@@ -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' )t/t3510-cherry-pick-sequence.sh +6
-1@@ -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
On Wed, Aug 15, 2018 at 9:24 AM Nguyễn Thái Ngọc Duy <[email protected]> wrote:
The patch looks good, but since this touches multiple .c files, I think I'd s/branch.c/branch/ in the subject line.
Quoted 167 lines
Signed-off-by Nguyễn Thái Ngọc Duy <[email protected]>--- 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(-)branch.c +12
-10@@ -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)branch.h +5
-2@@ -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" builtin/am.c +1
-1@@ -2017,7 +2017,7 @@ static int clean_index(const struct object_id *head, const struct object_id *remif (merge_tree (remote_tree))return -1 ; -remove_branch_state (); +remove_branch_state (the_repository);return 0 ; }builtin/branch.c +4
-2@@ -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 builtin/checkout.c +3
-2@@ -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" ))))builtin/reset.c +1
-2@@ -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
Quoted 5 lines · Elijah Newren
On Wed, Aug 15, 2018 at 9:24 AM Nguyễn Thái Ngọc Duy <[email protected]> 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.
Signature · 1 line
Duy
Quoted 11 lines · Duy Nguyen
Quoted 5 lines · Elijah Newren
On Wed, Aug 15, 2018 at 9:24 AM Nguyễn Thái Ngọc Duy <[email protected]> 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_repositoryfrombranch.cthough. As much as I want to completely erasethe_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?
Quoted 23 lines · Stefan Beller
Quoted 11 lines · Duy Nguyen
Quoted 5 lines · Elijah Newren
On Wed, Aug 15, 2018 at 9:24 AM Nguyễn Thái Ngọc Duy <[email protected]> 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_repositoryfrombranch.cthough. As much as I want to completely erasethe_repository, that would take a lot more work.What is your envisioned end state?
1) IMHO we'd first want to put
the_repositoryin 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 fromcmd_foo(int argc, char argv, char *prefix)tocmd_foo(int argc, char argv, struct repository *repo)
Quoted 2 lines
you seem to be interested in removing
the_repositoryfrombranch.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".
Signature · 1 line
Duy
Quoted 11 lines · Duy Nguyen
Quoted 6 lines
4) eventually (in the very long run), we'd change the signature of all commands from
cmd_foo(int argc, char argv, char *prefix)tocmd_foo(int argc, char argv, struct repository *repo)you seem to be interested in removing
the_repositoryfrombranch.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()andenter_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.
Quoted 10 lines · Junio C Hamano
Quoted 7 lines · Duy Nguyen
Quoted 6 lines
4) eventually (in the very long run), we'd change the signature of all commands from
cmd_foo(int argc, char argv, char *prefix)tocmd_foo(int argc, char argv, struct repository *repo)you seem to be interested in removing
the_repositoryfrombranch.c, but not as much from bultin/ for now, which is roughly step 2 in that plan?
Quoted 8 lines
Quoted 4 lines
Yes. Though I think step 4 is to make
setup_git_directory()andenter_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).
Signature · 1 line
Duy
Quoted 10 lines · Duy Nguyen
Quoted 5 lines · Elijah Newren
On Wed, Aug 15, 2018 at 9:24 AM Nguyễn Thái Ngọc Duy <[email protected]> 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_repositoryfrombranch.cthough. As much as I want to completely erasethe_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>".
Quoted 5 lines · Junio C Hamano
I also do not think
remove_branch_state()function belongs tobranch.cin 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?
Signature · 1 line
Duy