Let the builtin rebase call the git am command directly
cover Let the builtin rebase call the git am command directly
Especially on Windows, where Unix shell scripting is a foreign endeavor, and an expensive one at that, we really want to avoid running through the Bash.
This not only makes everything faster, but also more robust, as the Bash we use on Windows relies on a derivative of the Cygwin runtime, which in turn has to jump through a couple of hoops that are sometimes a little too tricky to make things work. Read: the less we rely on Unix shell scripting, the more likely Windows users will be able to enjoy our software.
Johannes Schindelin (4):
rebase: move reset_head() into a better spot
rebase: avoid double reflog entry when switching branches
rebase: teach reset_head() to optionally skip the worktree built-in rebase: call git am directly
builtin/rebase.c | 428 ++++++++++++++++++++++++++++++++++------------- 1 file changed, 309 insertions(+), 119 deletions(-)
base-commit: b21ebb671bb7dea8d342225f0d66c41f4e54d5ca
Published-As: https://github.com/gitgitgadget/git/releases/tags/pr-24%2Fdscho%2Fbuiltin-rebase--am-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-24/dscho/builtin-rebase--am-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/24
Signature · 1 line
gitgitgadget
PATCH 2/4 rebase: avoid double reflog entry when switching branches
From: Johannes Schindelin <[email protected]>
When switching a branch *and* updating said branch to a different revision, let's avoid a double entry by first updating the branch and then adjusting the symbolic ref HEAD.
--- builtin/rebase.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)
builtin/rebase.c +4 -3
@@ -438,10 +438,11 @@ static int reset_head(struct object_id *oid, const char *action, detach_head ?REF_NO_DEREF :0 , UPDATE_REFS_MSG_ON_ERR); else { - ret =create_symref ("HEAD" , switch_to_branch, msg.buf); + ret =update_ref (reflog_orig_head, switch_to_branch, oid, +NULL ,0 ,UPDATE_REFS_MSG_ON_ERR );if (!ret) - ret =update_ref (reflog_head,"HEAD" , oid,NULL ,0 , -UPDATE_REFS_MSG_ON_ERR ); + ret =create_symref ("HEAD" , switch_to_branch, + reflog_head); } leave_reset_head:
Signature · 1 line
gitgitgadget
PATCH 1/4 rebase: move `reset_head()` into a better spot
In reply to Johannes Schindelin via GitGitGadgetFrom: Johannes Schindelin <[email protected]>
Over the next commits, we want to make use of it in run_am() (i.e. running the --am backend directly, without detouring to Unix shell script code) which in turn will be called from run_specific_rebase().
So let's move it before that latter function.
This commit is best viewed using --color-moved.
--- builtin/rebase.c | 238 +++++++++++++++++++++++------------------------ 1 file changed, 119 insertions(+), 119 deletions(-)
builtin/rebase.c +119 -119
@@ -333,6 +333,125 @@ static void add_var(struct strbuf *buf, const char *name, const char *value) } } +#define GIT_REFLOG_ACTION_ENVIRONMENT "GIT_REFLOG_ACTION" + +#define RESET_HEAD_DETACH (1<<0) +#define RESET_HEAD_HARD (1<<1) + +static int reset_head (struct object_id *oid,const char *action, +const char *switch_to_branch,unsigned flags, +const char *reflog_orig_head,const char *reflog_head) +{ +unsigned detach_head = flags &RESET_HEAD_DETACH ; +unsigned reset_hard = flags &RESET_HEAD_HARD ; +struct object_id head_oid; +struct tree_desc desc[2 ] = { {NULL }, {NULL } }; +struct lock_file lock =LOCK_INIT ; +struct unpack_trees_options unpack_tree_opts; +struct tree *tree; +const char *reflog_action; +struct strbuf msg =STRBUF_INIT ; +size_t prefix_len; +struct object_id *orig =NULL , oid_orig, + *old_orig =NULL , oid_old_orig; +int ret =0 , nr =0 ; + +if (switch_to_branch && !starts_with (switch_to_branch,"refs/" )) +BUG ("Not a fully qualified branch: '%s'" , switch_to_branch); + +if (hold_locked_index (&lock,LOCK_REPORT_ON_ERROR ) <0 ) { + ret =-1 ; + goto leave_reset_head; + } + +if ((!oid || !reset_hard) &&get_oid ("HEAD" , &head_oid)) { + ret =error (_ ("could not determine HEAD revision" )); + goto leave_reset_head; + } + +if (!oid) + oid = &head_oid; + +memset (&unpack_tree_opts,0 ,sizeof (unpack_tree_opts)); +setup_unpack_trees_porcelain (&unpack_tree_opts, action); + unpack_tree_opts.head_idx =1 ; + unpack_tree_opts.src_index = the_repository->index; + unpack_tree_opts.dst_index = the_repository->index; + unpack_tree_opts.fn = reset_hard ? oneway_merge : twoway_merge; + unpack_tree_opts.update =1 ; + unpack_tree_opts.merge =1 ; +if (!detach_head) + unpack_tree_opts.reset =1 ; + +if (read_index_unmerged (the_repository->index) <0 ) { + ret =error (_ ("could not read index" )); + goto leave_reset_head; + } + +if (!reset_hard && !fill_tree_descriptor (&desc[nr++], &head_oid)) { + ret =error (_ ("failed to find tree of %s" ), +oid_to_hex (&head_oid)); + goto leave_reset_head; + } + +if (!fill_tree_descriptor (&desc[nr++], oid)) { + ret =error (_ ("failed to find tree of %s" ),oid_to_hex (oid)); + goto leave_reset_head; + } + +if (unpack_trees (nr, desc, &unpack_tree_opts)) { + ret =-1 ; + goto leave_reset_head; + } + + tree =parse_tree_indirect (oid); +prime_cache_tree (the_repository->index, tree); + +if (write_locked_index (the_repository->index, &lock,COMMIT_LOCK ) <0 ) { + ret =error (_ ("could not write index" )); + goto leave_reset_head; + } + + reflog_action =getenv (GIT_REFLOG_ACTION_ENVIRONMENT ); +strbuf_addf (&msg,"%s: " , reflog_action ? reflog_action :"rebase" ); + prefix_len = msg.len; + +if (!get_oid ("ORIG_HEAD" , &oid_old_orig)) + old_orig = &oid_old_orig; +if (!get_oid ("HEAD" , &oid_orig)) { + orig = &oid_orig; +if (!reflog_orig_head) { +strbuf_addstr (&msg,"updating ORIG_HEAD" ); + reflog_orig_head = msg.buf; + } +update_ref (reflog_orig_head,"ORIG_HEAD" , orig, old_orig,0 , +UPDATE_REFS_MSG_ON_ERR ); + }else if (old_orig) +delete_ref (NULL ,"ORIG_HEAD" , old_orig,0 ); +if (!reflog_head) { +strbuf_setlen (&msg, prefix_len); +strbuf_addstr (&msg,"updating HEAD" ); + reflog_head = msg.buf; + } +if (!switch_to_branch) + ret =update_ref (reflog_head,"HEAD" , oid, orig, + detach_head ?REF_NO_DEREF :0 , +UPDATE_REFS_MSG_ON_ERR ); +else { + ret =create_symref ("HEAD" , switch_to_branch, msg.buf); +if (!ret) + ret =update_ref (reflog_head,"HEAD" , oid,NULL ,0 , +UPDATE_REFS_MSG_ON_ERR ); + } + +leave_reset_head: +strbuf_release (&msg); +rollback_lock_file (&lock); +while (nr) +free ((void *)desc[--nr].buffer); +return ret; +} +static const char *resolvemsg =N_ ("Resolve all conflicts manually, mark them as resolved with\n" "\"git add/rm <conflicted_files>\", then run \"git rebase --continue\".\n"
@@ -526,125 +645,6 @@ static int run_specific_rebase(struct rebase_options *opts)return status ?-1 :0 ; } -#define GIT_REFLOG_ACTION_ENVIRONMENT "GIT_REFLOG_ACTION" - -#define RESET_HEAD_DETACH (1<<0) -#define RESET_HEAD_HARD (1<<1) - -static int reset_head (struct object_id *oid,const char *action, -const char *switch_to_branch,unsigned flags, -const char *reflog_orig_head,const char *reflog_head) -{ -unsigned detach_head = flags &RESET_HEAD_DETACH ; -unsigned reset_hard = flags &RESET_HEAD_HARD ; -struct object_id head_oid; -struct tree_desc desc[2 ] = { {NULL }, {NULL } }; -struct lock_file lock =LOCK_INIT ; -struct unpack_trees_options unpack_tree_opts; -struct tree *tree; -const char *reflog_action; -struct strbuf msg =STRBUF_INIT ; -size_t prefix_len; -struct object_id *orig =NULL , oid_orig, - *old_orig =NULL , oid_old_orig; -int ret =0 , nr =0 ; - -if (switch_to_branch && !starts_with (switch_to_branch,"refs/" )) -BUG ("Not a fully qualified branch: '%s'" , switch_to_branch); - -if (hold_locked_index (&lock,LOCK_REPORT_ON_ERROR ) <0 ) { - ret =-1 ; - goto leave_reset_head; - } - -if ((!oid || !reset_hard) &&get_oid ("HEAD" , &head_oid)) { - ret =error (_ ("could not determine HEAD revision" )); - goto leave_reset_head; - } - -if (!oid) - oid = &head_oid; - -memset (&unpack_tree_opts,0 ,sizeof (unpack_tree_opts)); -setup_unpack_trees_porcelain (&unpack_tree_opts, action); - unpack_tree_opts.head_idx =1 ; - unpack_tree_opts.src_index = the_repository->index; - unpack_tree_opts.dst_index = the_repository->index; - unpack_tree_opts.fn = reset_hard ? oneway_merge : twoway_merge; - unpack_tree_opts.update =1 ; - unpack_tree_opts.merge =1 ; -if (!detach_head) - unpack_tree_opts.reset =1 ; - -if (read_index_unmerged (the_repository->index) <0 ) { - ret =error (_ ("could not read index" )); - goto leave_reset_head; - } - -if (!reset_hard && !fill_tree_descriptor (&desc[nr++], &head_oid)) { - ret =error (_ ("failed to find tree of %s" ), -oid_to_hex (&head_oid)); - goto leave_reset_head; - } - -if (!fill_tree_descriptor (&desc[nr++], oid)) { - ret =error (_ ("failed to find tree of %s" ),oid_to_hex (oid)); - goto leave_reset_head; - } - -if (unpack_trees (nr, desc, &unpack_tree_opts)) { - ret =-1 ; - goto leave_reset_head; - } - - tree =parse_tree_indirect (oid); -prime_cache_tree (the_repository->index, tree); - -if (write_locked_index (the_repository->index, &lock,COMMIT_LOCK ) <0 ) { - ret =error (_ ("could not write index" )); - goto leave_reset_head; - } - - reflog_action =getenv (GIT_REFLOG_ACTION_ENVIRONMENT ); -strbuf_addf (&msg,"%s: " , reflog_action ? reflog_action :"rebase" ); - prefix_len = msg.len; - -if (!get_oid ("ORIG_HEAD" , &oid_old_orig)) - old_orig = &oid_old_orig; -if (!get_oid ("HEAD" , &oid_orig)) { - orig = &oid_orig; -if (!reflog_orig_head) { -strbuf_addstr (&msg,"updating ORIG_HEAD" ); - reflog_orig_head = msg.buf; - } -update_ref (reflog_orig_head,"ORIG_HEAD" , orig, old_orig,0 , -UPDATE_REFS_MSG_ON_ERR ); - }else if (old_orig) -delete_ref (NULL ,"ORIG_HEAD" , old_orig,0 ); -if (!reflog_head) { -strbuf_setlen (&msg, prefix_len); -strbuf_addstr (&msg,"updating HEAD" ); - reflog_head = msg.buf; - } -if (!switch_to_branch) - ret =update_ref (reflog_head,"HEAD" , oid, orig, - detach_head ?REF_NO_DEREF :0 , -UPDATE_REFS_MSG_ON_ERR ); -else { - ret =create_symref ("HEAD" , switch_to_branch, msg.buf); -if (!ret) - ret =update_ref (reflog_head,"HEAD" , oid,NULL ,0 , -UPDATE_REFS_MSG_ON_ERR ); - } - -leave_reset_head: -strbuf_release (&msg); -rollback_lock_file (&lock); -while (nr) -free ((void *)desc[--nr].buffer); -return ret; -} -static int rebase_config (const char *var,const char *value,void *data) {struct rebase_options *opts = data;
Signature · 1 line
gitgitgadget
PATCH 3/4 rebase: teach `reset_head()` to optionally skip the worktree
In reply to Johannes Schindelin via GitGitGadgetFrom: Johannes Schindelin <[email protected]>
This is what the legacy (scripted) rebase does in move_to_original_branch, and we will need this functionality in the next commit.
--- builtin/rebase.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)
builtin/rebase.c +7 -1
@@ -337,6 +337,7 @@ static void add_var(struct strbuf *buf, const char *name, const char *value)#define RESET_HEAD_DETACH (1<<0)#define RESET_HEAD_HARD (1<<1) +#define RESET_HEAD_REFS_ONLY (1<<2) static int reset_head(struct object_id *oid, const char *action, const char *switch_to_branch, unsigned flags,
@@ -344,6 +345,7 @@ static int reset_head(struct object_id *oid, const char *action, {unsigned detach_head = flags &RESET_HEAD_DETACH ;unsigned reset_hard = flags &RESET_HEAD_HARD ; +unsigned refs_only = flags &RESET_HEAD_REFS_ONLY ;struct object_id head_oid;struct tree_desc desc[2 ] = { {NULL }, {NULL } };struct lock_file lock =LOCK_INIT ;
@@ -359,7 +361,7 @@ static int reset_head(struct object_id *oid, const char *action,if (switch_to_branch && !starts_with (switch_to_branch,"refs/" ))BUG ("Not a fully qualified branch: '%s'" , switch_to_branch); -if (hold_locked_index (&lock,LOCK_REPORT_ON_ERROR ) <0 ) { +if (!refs_only &&hold_locked_index (&lock,LOCK_REPORT_ON_ERROR ) <0 ) { ret =-1 ; goto leave_reset_head; }
@@ -372,6 +374,9 @@ static int reset_head(struct object_id *oid, const char *action,if (!oid) oid = &head_oid; +if (flags &RESET_HEAD_REFS_ONLY ) + goto reset_head_refs; +memset (&unpack_tree_opts,0 ,sizeof (unpack_tree_opts));setup_unpack_trees_porcelain (&unpack_tree_opts, action); unpack_tree_opts.head_idx =1 ;
@@ -412,6 +417,7 @@ static int reset_head(struct object_id *oid, const char *action, goto leave_reset_head; } +reset_head_refs: reflog_action =getenv (GIT_REFLOG_ACTION_ENVIRONMENT );strbuf_addf (&msg,"%s: " , reflog_action ? reflog_action :"rebase" ); prefix_len = msg.len;
Signature · 1 line
gitgitgadget
PATCH 4/4 built-in rebase: call `git am` directly
In reply to Johannes Schindelin via GitGitGadgetFrom: Johannes Schindelin <[email protected]>
While the scripted git rebase still has to rely on the git-rebase--am.sh script to implement the glue between the rebase and the am commands, we can go a more direct route in the built-in rebase and avoid using a shell script altogether.
This patch represents a straight-forward port of git-rebase--am.sh to C, along with the glue code to call it directly from within builtin/rebase.c.
This reduces the chances of Git for Windows running into trouble due to problems with the POSIX emulation layer (known as "MSYS2 runtime", itself a derivative of the Cygwin runtime): when no shell script is called, the POSIX emulation layer is avoided altogether.
--- builtin/rebase.c | 183 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 183 insertions(+)
builtin/rebase.c +183 -0
@@ -246,6 +246,37 @@ static int read_basic_state(struct rebase_options *opts)return 0 ; } +static int write_basic_state (struct rebase_options *opts) +{ +write_file (state_dir_path ("head-name" , opts),"%s" , + opts->head_name ? opts->head_name :"detached HEAD" ); +write_file (state_dir_path ("onto" , opts),"%s" , + opts->onto ?oid_to_hex (&opts->onto->object.oid) :"" ); +write_file (state_dir_path ("orig-head" , opts),"%s" , +oid_to_hex (&opts->orig_head)); +write_file (state_dir_path ("quiet" , opts),"%s" , + opts->flags &REBASE_NO_QUIET ?"" :"t" ); +if (opts->flags &REBASE_VERBOSE ) +write_file (state_dir_path ("verbose" , opts),"%s" ,"" ); +if (opts->strategy) +write_file (state_dir_path ("strategy" , opts),"%s" , + opts->strategy); +if (opts->strategy_opts) +write_file (state_dir_path ("strategy_opts" , opts),"%s" , + opts->strategy_opts); +if (opts->allow_rerere_autoupdate >=0 ) +write_file (state_dir_path ("allow_rerere_autoupdate" , opts), +"-%s-rerere-autoupdate" , + opts->allow_rerere_autoupdate ?"" :"-no" ); +if (opts->gpg_sign_opt) +write_file (state_dir_path ("gpg_sign_opt" , opts),"%s" , + opts->gpg_sign_opt); +if (opts->signoff) +write_file (state_dir_path ("strategy" , opts),"--signoff" ); + +return 0 ; +} +static int apply_autostash (struct rebase_options *opts) {const char *path =state_dir_path ("autostash" , opts);
@@ -459,6 +490,30 @@ static int reset_head(struct object_id *oid, const char *action,return ret; } +static int move_to_original_branch (struct rebase_options *opts) +{ +struct strbuf orig_head_reflog =STRBUF_INIT , head_reflog =STRBUF_INIT ; +int ret; + +if (!opts->head_name) +return 0 ;/* nothing to move back to */ + +if (!opts->onto) +BUG ("move_to_original_branch without onto" ); + +strbuf_addf (&orig_head_reflog,"rebase finished: %s onto %s" , + opts->head_name,oid_to_hex (&opts->onto->object.oid)); +strbuf_addf (&head_reflog,"rebase finished: returning to %s" , + opts->head_name); + ret =reset_head (NULL ,"checkout" , opts->head_name, +RESET_HEAD_REFS_ONLY , + orig_head_reflog.buf, head_reflog.buf); + +strbuf_release (&orig_head_reflog); +strbuf_release (&head_reflog); +return ret; +} +static const char *resolvemsg =N_ ("Resolve all conflicts manually, mark them as resolved with\n" "\"git add/rm <conflicted_files>\", then run \"git rebase --continue\".\n"
@@ -466,6 +521,129 @@ N_("Resolve all conflicts manually, mark them as resolved with\n""To abort and get back to the state before \"git rebase\", run " "\"git rebase --abort\"."); +static int run_am (struct rebase_options *opts) +{ +struct child_process am =CHILD_PROCESS_INIT ; +struct child_process format_patch =CHILD_PROCESS_INIT ; +struct strbuf revisions =STRBUF_INIT ; +int status; +char *rebased_patches; + + am.git_cmd =1 ; +argv_array_push (&am.args,"am" ); + +if (opts->action && !strcmp ("continue" , opts->action)) { +argv_array_push (&am.args,"--resolved" ); +argv_array_pushf (&am.args,"--resolvemsg=%s" , resolvemsg); +if (opts->gpg_sign_opt) +argv_array_push (&am.args, opts->gpg_sign_opt); + status =run_command (&am); +if (status) +return status; + +discard_cache (); +return move_to_original_branch (opts); + } +if (opts->action && !strcmp ("skip" , opts->action)) { +argv_array_push (&am.args,"--skip" ); +argv_array_pushf (&am.args,"--resolvemsg=%s" , resolvemsg); + status =run_command (&am); +if (status) +return status; + +discard_cache (); +return move_to_original_branch (opts); + } +if (opts->action && !strcmp ("show-current-patch" , opts->action)) { +argv_array_push (&am.args,"--show-current-patch" ); +return run_command (&am); + } + +strbuf_addf (&revisions,"%s...%s" , +oid_to_hex (opts->root ? +/* this is now equivalent to ! -z "$upstream" */ + &opts->onto->object.oid : + &opts->upstream->object.oid), +oid_to_hex (&opts->orig_head)); + + rebased_patches =xstrdup (git_path ("rebased-patches" )); + format_patch.out =open (rebased_patches, +O_WRONLY |O_CREAT |O_TRUNC ,0666 ); +if (format_patch.out <0 ) { + status =error_errno (_ ("could not write '%s'" ), + rebased_patches); +free (rebased_patches); +argv_array_clear (&am.args); +return status; + } + + format_patch.git_cmd =1 ; +argv_array_pushl (&format_patch.args,"format-patch" ,"-k" ,"--stdout" , +"--full-index" ,"--cherry-pick" ,"--right-only" , +"--src-prefix=a/" ,"--dst-prefix=b/" ,"--no-renames" , +"--no-cover-letter" ,"--pretty=mboxrd" ,NULL ); +if (opts->git_format_patch_opt.len) +argv_array_split (&format_patch.args, + opts->git_format_patch_opt.buf); +argv_array_push (&format_patch.args, revisions.buf); +if (opts->restrict_revision) +argv_array_pushf (&format_patch.args,"^%s" , +oid_to_hex (&opts->restrict_revision->object.oid)); + + status =run_command (&format_patch); +if (status) { +unlink (rebased_patches); +free (rebased_patches); +argv_array_clear (&am.args); + +reset_head (&opts->orig_head,"checkout" , opts->head_name,0 , +"HEAD" ,NULL ); +error (_ ("\ngit encountered an error while preparing the " +"patches to replay\n" +"these revisions:\n" +"\n %s\n\n" +"As a result, git cannot rebase them." ), + opts->revisions); + +strbuf_release (&revisions); +return status; + } +strbuf_release (&revisions); + + am.in =open (rebased_patches,O_RDONLY ); +if (am.in <0 ) { + status =error_errno (_ ("could not read '%s'" ), + rebased_patches); +free (rebased_patches); +argv_array_clear (&am.args); +return status; + } + +argv_array_pushv (&am.args, opts->git_am_opts.argv); +argv_array_push (&am.args,"--rebasing" ); +argv_array_pushf (&am.args,"--resolvemsg=%s" , resolvemsg); +argv_array_push (&am.args,"--patch-format=mboxrd" ); +if (opts->allow_rerere_autoupdate >0 ) +argv_array_push (&am.args,"--rerere-autoupdate" ); +else if (opts->allow_rerere_autoupdate ==0 ) +argv_array_push (&am.args,"--no-rerere-autoupdate" ); +if (opts->gpg_sign_opt) +argv_array_push (&am.args, opts->gpg_sign_opt); + status =run_command (&am); +unlink (rebased_patches); +free (rebased_patches); + +if (!status) { +discard_cache (); +return move_to_original_branch (opts); + } + +if (is_directory (opts->state_dir)) +write_basic_state (opts); + +return status; +} +static int run_specific_rebase (struct rebase_options *opts) {const char *argv[] = {NULL ,NULL };
@@ -546,6 +724,11 @@ static int run_specific_rebase(struct rebase_options *opts) goto finished_rebase; } +if (opts->type ==REBASE_AM ) { + status =run_am (opts); + goto finished_rebase; + } +add_var (&script_snippet,"GIT_DIR" ,absolute_path (get_git_dir ()));add_var (&script_snippet,"state_dir" , opts->state_dir);
Signature · 1 line
gitgitgadget
Re: [PATCH 4/4] built-in rebase: call `git am` directly
"Johannes Schindelin via GitGitGadget" <[email protected]>
Quoted 12 lines · writes:
builtin/rebase.c +12
-0+static int write_basic_state (struct rebase_options *opts) +{ +write_file (state_dir_path ("head-name" , opts),"%s" , + opts->head_name ? opts->head_name :"detached HEAD" ); +write_file (state_dir_path ("onto" , opts),"%s" , + opts->onto ?oid_to_hex (&opts->onto->object.oid) :"" ); +write_file (state_dir_path ("orig-head" , opts),"%s" , +oid_to_hex (&opts->orig_head)); +write_file (state_dir_path ("quiet" , opts),"%s" , + opts->flags &REBASE_NO_QUIET ?"" :"t" ); +if (opts->flags &REBASE_VERBOSE ) +write_file (state_dir_path ("verbose" , opts),"%s" ,"" );
builtin/rebase.c static int read_basic_state(struct rebase_options *opts) · line 270
Quoted 10 lines
builtin/rebase.c +10
-0+if (opts->strategy) +write_file (state_dir_path ("strategy" , opts),"%s" , + opts->strategy); +if (opts->strategy_opts) +write_file (state_dir_path ("strategy_opts" , opts),"%s" , + opts->strategy_opts); +if (opts->allow_rerere_autoupdate >=0 ) +write_file (state_dir_path ("allow_rerere_autoupdate" , opts), +"-%s-rerere-autoupdate" , + opts->allow_rerere_autoupdate ?"" :"-no" );
Inside rebase, allow-rerere-autoupdate can be -1 (unspecified), 0 (declined) or 1 (requested), and this code is being consistent with that convention.
The "--[no-]rerere-autoupdate" option that is parsed via OPT_RERERE_AUTOUPDATE (used in builtin/rebase--interactive.c among other built-in commands) on the other hand is tertially that uses 0 (unspecified), 1 (requested) and 2 (declined). This might be a ticking timebomb to confuse us in the future that may be worth fixing but probably outside this series.
builtin/rebase.c static int read_basic_state(struct rebase_options *opts) · line 278
Quoted 8 lines
builtin/rebase.c +8
-0+if (opts->gpg_sign_opt) +write_file (state_dir_path ("gpg_sign_opt" , opts),"%s" , + opts->gpg_sign_opt); +if (opts->signoff) +write_file (state_dir_path ("strategy" , opts),"--signoff" ); + +return 0 ; +}
The above looks like a literal translation of a helper function with the same name in git-rebase--common.sh. Good.
Quoted 4 lines
builtin/rebase.c +0
-0@@ -459,6 +490,30 @@ static int reset_head(struct object_id *oid, const char *action,return ret; }
builtin/rebase.c static int reset_head(struct object_id *oid, const char *action, · line 502
Quoted 10 lines
builtin/rebase.c +10
-0+static int move_to_original_branch (struct rebase_options *opts) +{ +struct strbuf orig_head_reflog =STRBUF_INIT , head_reflog =STRBUF_INIT ; +int ret; + +if (!opts->head_name) +return 0 ;/* nothing to move back to */ + +if (!opts->onto) +BUG ("move_to_original_branch without onto" );
This check is absent in the scripted version, but from the message we generate here, it is clear that the caller must not call this when there is no "onto" commit. Good.
builtin/rebase.c static int reset_head(struct object_id *oid, const char *action, · line 510
Quoted 7 lines
builtin/rebase.c +7
-0+strbuf_addf (&orig_head_reflog,"rebase finished: %s onto %s" , + opts->head_name,oid_to_hex (&opts->onto->object.oid)); +strbuf_addf (&head_reflog,"rebase finished: returning to %s" , + opts->head_name); + ret =reset_head (NULL ,"checkout" , opts->head_name, +RESET_HEAD_REFS_ONLY , + orig_head_reflog.buf, head_reflog.buf);
The *action given to reset_head() here is "checkout". Makes me wonder about two things:
- The only real use of the parameter in the callee is to prepare the error and advice messages from the unpack_trees machinery, but because we are using it in REFS_ONLY mode, it does not matter. In fact it might even be misleading; perhaps pass NULL or something, so that a mistaken update to reset_head() later that lets REFS_ONLY request to go to unpack_trees machinery will catch it as a bug?
- Another topic in flight wants to make sure that the post-checkout hook gets called when the RESET_HEAD_RUN_POST_CHECKOUT_HOOK flag is given by the caller, and IIRC, the use of the flag is strongly correlated to *action being "checkout". Do we want to pass REFS_ONLY and RUN_POST_CHECKOUT_HOOK flag for this call, or do we rather keep it silent? As the original scripted version did not use "checkout" here and never triggered post-checkout hook, I am inclined to say that we should not pass that other bit. That then leads me to suspect that we do not want *action to be "checkout" here.
builtin/rebase.c static int reset_head(struct object_id *oid, const char *action, · line 515
Quoted 4 lines
builtin/rebase.c +4
-0+strbuf_release (&orig_head_reflog); +strbuf_release (&head_reflog); +return ret; +}
Unlike the scripted version, this does not die() upon failure, so the caller needs to be careful about the returned status.
Quoted 16 lines
builtin/rebase.c +12
-0@@ -466,6 +521,129 @@ N_("Resolve all conflicts manually, mark them as resolved with\n""To abort and get back to the state before \"git rebase\", run " "\"git rebase --abort\"."); +static int run_am (struct rebase_options *opts) +{ +struct child_process am =CHILD_PROCESS_INIT ; +struct child_process format_patch =CHILD_PROCESS_INIT ; +struct strbuf revisions =STRBUF_INIT ; +int status; +char *rebased_patches; + + am.git_cmd =1 ; +argv_array_push (&am.args,"am" ); + +if (opts->action && !strcmp ("continue" , opts->action)) {
builtin/rebase.c N_("Resolve all conflicts manually, mark them as resolved with\n" · line 545
Quoted 10 lines
builtin/rebase.c +10
-0+argv_array_push (&am.args,"--resolved" ); +argv_array_pushf (&am.args,"--resolvemsg=%s" , resolvemsg); +if (opts->gpg_sign_opt) +argv_array_push (&am.args, opts->gpg_sign_opt); + status =run_command (&am); +if (status) +return status; + +discard_cache (); +return move_to_original_branch (opts);
It is curious why discard_cache() is placed exacly here, as if we want to preserve the contents of the in-core index when run_command() failed. But I do not think we care about the in-core index as the only thing that happen after "return status" is to return the control to run_specific_rebase(), let it jump to finished_rebase label to clean things up and rturn control to cmd_rebase() and exit based on the status value.
It's not like move_to_original_branch() wants to call read_cache() and get the result from the "am" that run_command() executed, either.
Puzzled. Care to explain a bit more in the in-code comment?
builtin/rebase.c N_("Resolve all conflicts manually, mark them as resolved with\n" · line 555
Quoted 10 lines
builtin/rebase.c +10
-0+ } +if (opts->action && !strcmp ("skip" , opts->action)) { +argv_array_push (&am.args,"--skip" ); +argv_array_pushf (&am.args,"--resolvemsg=%s" , resolvemsg); + status =run_command (&am); +if (status) +return status; + +discard_cache (); +return move_to_original_branch (opts);
Ditto.
builtin/rebase.c N_("Resolve all conflicts manually, mark them as resolved with\n" · line 560
Quoted 5 lines
builtin/rebase.c +5
-0+ } +if (opts->action && !strcmp ("show-current-patch" , opts->action)) { +argv_array_push (&am.args,"--show-current-patch" ); +return run_command (&am); + }
Up to this point, it is a faithful conversion of the first case/esac statement. Good.
builtin/rebase.c N_("Resolve all conflicts manually, mark them as resolved with\n" · line 564
Quoted 3 lines
builtin/rebase.c +3
-0+ strbuf_addf(&revisions, "%s...%s", + oid_to_hex(opts->root ? +/* this is now equivalent to ! -z "$upstream" */
Does "this" refer to the "opts->root being true" check?
Because you are flipping the polarity of the test from scripted version, shouldn't the comment be updated to "-z $upstream"?
builtin/rebase.c N_("Resolve all conflicts manually, mark them as resolved with\n" · line 571
Quoted 7 lines
builtin/rebase.c +6
-0+ &opts->onto->object.oid : + &opts->upstream->object.oid), + oid_to_hex(&opts->orig_head)); + rebased_patches =xstrdup (git_path ("rebased-patches" )); + format_patch.out =open (rebased_patches, +O_WRONLY |O_CREAT |O_TRUNC ,0666 );
Unlike scripted version, we do not remove a (possibly) existing file. We give CREAT in case there is no existing one, and TRUNC in case there is an existing one. Makes sense. A more faithful translation would have unlink(2)ed a (possibly) existing one, and then because we can afford to, passed O_EXCL to avoid stomping on somebody else racing with us, but I do not think it is worth it.
builtin/rebase.c N_("Resolve all conflicts manually, mark them as resolved with\n" · line 574
Quoted 3 lines
builtin/rebase.c +3
-0+if (format_patch.out <0 ) { + status =error_errno (_ ("could not write '%s'" ), + rebased_patches);
s/write '%s'/open '%s' for writing/? I dunno.
Quoted 7 lines
builtin/rebase.c +7
-0+free (rebased_patches); +argv_array_clear (&am.args); +return status; + } + + format_patch.git_cmd =1 ; + argv_array_pushl(&format_patch.args, "format-patch", "-k", "--stdout",
builtin/rebase.c N_("Resolve all conflicts manually, mark them as resolved with\n" · line 591
Quoted 10 lines
builtin/rebase.c +10
-0+"--full-index" ,"--cherry-pick" ,"--right-only" , +"--src-prefix=a/" ,"--dst-prefix=b/" ,"--no-renames" , + "--no-cover-letter", "--pretty=mboxrd", NULL); +if (opts->git_format_patch_opt.len) +argv_array_split (&format_patch.args, + opts->git_format_patch_opt.buf); +argv_array_push (&format_patch.args, revisions.buf); +if (opts->restrict_revision) +argv_array_pushf (&format_patch.args,"^%s" , +oid_to_hex (&opts->restrict_revision->object.oid));
It is kinda surprising to see that we have learned quite a lot of fringe "configurations" we need to explicitly override like this.
Looks like a quite faithful conversion, anyway.
builtin/rebase.c N_("Resolve all conflicts manually, mark them as resolved with\n" · line 600
Quoted 8 lines
builtin/rebase.c +8
-0+ status =run_command (&format_patch); +if (status) { +unlink (rebased_patches); +free (rebased_patches); +argv_array_clear (&am.args); + +reset_head (&opts->orig_head,"checkout" , opts->head_name,0 , +"HEAD" ,NULL );
This one may need to trigger post-checkout hook. The scripted version does two different things depending on the value of $head_name, but we can just use the same code without conditional?
Quoted 6 lines
builtin/rebase.c +6
-0+error (_ ("\ngit encountered an error while preparing the " +"patches to replay\n" +"these revisions:\n" +"\n %s\n\n" +"As a result, git cannot rebase them." ), + opts->revisions);
builtin/rebase.c N_("Resolve all conflicts manually, mark them as resolved with\n" · line 616
Quoted 10 lines
builtin/rebase.c +10
-0+ +strbuf_release (&revisions); +return status; + } +strbuf_release (&revisions); + + am.in =open (rebased_patches,O_RDONLY ); +if (am.in <0 ) { + status =error_errno (_ ("could not read '%s'" ), + rebased_patches);
s/write '%s'/open '%s' for reading/? I dunno.
Quoted 34 lines
builtin/rebase.c +30
-0+free (rebased_patches); +argv_array_clear (&am.args); +return status; + } + +argv_array_pushv (&am.args, opts->git_am_opts.argv); +argv_array_push (&am.args,"--rebasing" ); +argv_array_pushf (&am.args,"--resolvemsg=%s" , resolvemsg); +argv_array_push (&am.args,"--patch-format=mboxrd" ); +if (opts->allow_rerere_autoupdate >0 ) +argv_array_push (&am.args,"--rerere-autoupdate" ); +else if (opts->allow_rerere_autoupdate ==0 ) +argv_array_push (&am.args,"--no-rerere-autoupdate" ); +if (opts->gpg_sign_opt) +argv_array_push (&am.args, opts->gpg_sign_opt); + status =run_command (&am); +unlink (rebased_patches); +free (rebased_patches); + +if (!status) { +discard_cache (); +return move_to_original_branch (opts); + } + +if (is_directory (opts->state_dir)) +write_basic_state (opts); + +return status; +} +static int run_specific_rebase (struct rebase_options *opts) {const char *argv[] = {NULL ,NULL };
builtin/rebase.c static int run_specific_rebase(struct rebase_options *opts) · line 733
Quoted 10 lines
builtin/rebase.c +5
-0@@ -546,6 +724,11 @@ static int run_specific_rebase(struct rebase_options *opts) goto finished_rebase; } +if (opts->type ==REBASE_AM ) { + status =run_am (opts); + goto finished_rebase; + } +add_var (&script_snippet,"GIT_DIR" ,absolute_path (get_git_dir ()));add_var (&script_snippet,"state_dir" , opts->state_dir);
Overall, this was quite a pleasant read and a well constructed series. Other than two minor points (i.e. interaction with the 'post-checkout hook' topic, and discard_cache() before calling move_to_original_branch) I did not quite understand, looks good to me.
When merged to 'pu', I seem to be getting failure from t3425.5, .8 and .11, by the way. I haven't dug into the actual breakages any further than that.
Thanks.
Re: [PATCH 3/4] rebase: teach `reset_head()` to optionally skip the worktree
In reply to Johannes Schindelin via GitGitGadget"Johannes Schindelin via GitGitGadget" <[email protected]>
Quoted 5 lines · writes:
From: Johannes Schindelin <[email protected]>
This is what the legacy (scripted) rebase does in
move_to_original_branch, and we will need this functionality in the next commit.
The move-to-original-branch helper does:
- point $head_name to the commit pointed at by HEAD
- point HEAD symref to $head_name
without touching the index or the working tree files. It's not exactly "reset --soft" but more like "switch-branch --soft" ;-)
Quoted 10 lines
Signed-off-by Johannes Schindelin <[email protected]>--- builtin/rebase.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)builtin/rebase.c +0
-0
builtin/rebase.c static void add_var(struct strbuf *buf, const char *name, const char *value) · line 340
Quoted 4 lines
builtin/rebase.c +1
-0@@ -337,6 +337,7 @@ static void add_var(struct strbuf *buf, const char *name, const char *value)#define RESET_HEAD_DETACH (1<<0)#define RESET_HEAD_HARD (1<<1) +#define RESET_HEAD_REFS_ONLY (1<<2)
In the future codebase in 'pu', we have 1<<2 already taken by another topic, so I'll tell my rerere database that the bit assignment needs to be adjusted.
Quoted 11 lines
builtin/rebase.c +1
-0static int reset_head(struct object_id *oid, const char *action, const char *switch_to_branch, unsigned flags,@@ -344,6 +345,7 @@ static int reset_head(struct object_id *oid, const char *action, {unsigned detach_head = flags &RESET_HEAD_DETACH ;unsigned reset_hard = flags &RESET_HEAD_HARD ; +unsigned refs_only = flags &RESET_HEAD_REFS_ONLY ;struct object_id head_oid;struct tree_desc desc[2 ] = { {NULL }, {NULL } };struct lock_file lock =LOCK_INIT ;
builtin/rebase.c static int reset_head(struct object_id *oid, const char *action, · line 367
Quoted 8 lines
builtin/rebase.c +1
-1@@ -359,7 +361,7 @@ static int reset_head(struct object_id *oid, const char *action,if (switch_to_branch && !starts_with (switch_to_branch,"refs/" ))BUG ("Not a fully qualified branch: '%s'" , switch_to_branch); -if (hold_locked_index (&lock,LOCK_REPORT_ON_ERROR ) <0 ) { +if (!refs_only &&hold_locked_index (&lock,LOCK_REPORT_ON_ERROR ) <0 ) { ret =-1 ; goto leave_reset_head; }
Not touching the index, so no need to lock the index. Makes sense.
builtin/rebase.c static int reset_head(struct object_id *oid, const char *action, · line 379
Quoted 7 lines
builtin/rebase.c +3
-0@@ -372,6 +374,9 @@ static int reset_head(struct object_id *oid, const char *action,if (!oid) oid = &head_oid; +if (flags &RESET_HEAD_REFS_ONLY ) + goto reset_head_refs; +
Why not "refs_only" that we already prepared above??? Are we munging that secondary variable before control comes here?
In any case, not touching the index nor the working tree, so no need to call into the unpack_trees machinery. Makes sense.
builtin/rebase.c static int reset_head(struct object_id *oid, const char *action, · line 421
Quoted 9 lines
builtin/rebase.c +1
-0memset (&unpack_tree_opts,0 ,sizeof (unpack_tree_opts));setup_unpack_trees_porcelain (&unpack_tree_opts, action); unpack_tree_opts.head_idx =1 ;@@ -412,6 +417,7 @@ static int reset_head(struct object_id *oid, const char *action, goto leave_reset_head; } +reset_head_refs: reflog_action =getenv (GIT_REFLOG_ACTION_ENVIRONMENT );
And the control continues from the point we update the reflog. Makes sense.
builtin/rebase.c static int reset_head(struct object_id *oid, const char *action, · line 423
Quoted 2 lines
builtin/rebase.c +0
-0strbuf_addf (&msg,"%s: " , reflog_action ? reflog_action :"rebase" ); prefix_len = msg.len;
This helper is touched by two other topics in flight, and that was one of the reason why it took a bit longer than usual for me to merge this topic. Please sanity-check the result of the conflict resolution at the tip of 'pu' branch.
Thanks.
Re: [PATCH 2/4] rebase: avoid double reflog entry when switching branches
In reply to Johannes Schindelin via GitGitGadget"Johannes Schindelin via GitGitGadget" <[email protected]>
Quoted 5 lines · writes:
From: Johannes Schindelin <[email protected]>
When switching a branch *and* updating said branch to a different revision, let's avoid a double entry by first updating the branch and then adjusting the symbolic ref HEAD.
Ah, in the original sequence, HEAD is updated twice, leaving two reflog entries for HEAD (and one for the underlying "switch_to" branch by virtue of REF_UPDATE_VIA_HEAD). In the new sequence, update_ref() updates the underlying "switch_to" and then HEAD, so we'd get one reflog entry for each of them.
Makes sense. s/let's avoid a double entry/& in HEAD's reflog/ would have avoided wasting reader's time who needlessly wondered where that redundancy came from, though.
Quoted 25 lines
Signed-off-by Johannes Schindelin <[email protected]>--- builtin/rebase.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)builtin/rebase.c +4
-3@@ -438,10 +438,11 @@ static int reset_head(struct object_id *oid, const char *action, detach_head ?REF_NO_DEREF :0 , UPDATE_REFS_MSG_ON_ERR); else { - ret =create_symref ("HEAD" , switch_to_branch, msg.buf); + ret =update_ref (reflog_orig_head, switch_to_branch, oid, +NULL ,0 ,UPDATE_REFS_MSG_ON_ERR );if (!ret) - ret =update_ref (reflog_head,"HEAD" , oid,NULL ,0 , -UPDATE_REFS_MSG_ON_ERR ); + ret =create_symref ("HEAD" , switch_to_branch, + reflog_head); } leave_reset_head:
Re: [PATCH 1/4] rebase: move `reset_head()` into a better spot
In reply to Johannes Schindelin via GitGitGadget"Johannes Schindelin via GitGitGadget" <[email protected]>
Quoted 7 lines · writes:
From: Johannes Schindelin <[email protected]>
Over the next commits, we want to make use of it in
run_am()(i.e. running the--ambackend directly, without detouring to Unix shell script code) which in turn will be called fromrun_specific_rebase().So let's move it before that latter function.
OK.
Re: [PATCH 3/4] rebase: teach `reset_head()` to optionally skip the worktree
In reply to Junio C HamanoQuoted 85 lines · Junio C Hamano
"Johannes Schindelin via GitGitGadget" <[email protected]>
Quoted 5 lines · writes:
From: Johannes Schindelin <[email protected]>
This is what the legacy (scripted) rebase does in
move_to_original_branch, and we will need this functionality in the next commit.The move-to-original-branch helper does:
- point $head_name to the commit pointed at by HEAD
- point HEAD symref to $head_namewithout touching the index or the working tree files. It's not exactly "reset --soft" but more like "switch-branch --soft" ;-)
Quoted 10 lines
Signed-off-by Johannes Schindelin <[email protected]>--- builtin/rebase.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)builtin/rebase.c +0
-0builtin/rebase.c static void add_var(struct strbuf *buf, const char *name, const char *value) · line 340
Quoted 4 lines
builtin/rebase.c +1
-0@@ -337,6 +337,7 @@ static void add_var(struct strbuf *buf, const char *name, const char *value)#define RESET_HEAD_DETACH (1<<0)#define RESET_HEAD_HARD (1<<1) +#define RESET_HEAD_REFS_ONLY (1<<2)In the future codebase in 'pu', we have 1<<2 already taken by another topic, so I'll tell my rerere database that the bit assignment needs to be adjusted.
Quoted 11 lines
builtin/rebase.c +1
-0static int reset_head(struct object_id *oid, const char *action, const char *switch_to_branch, unsigned flags,@@ -344,6 +345,7 @@ static int reset_head(struct object_id *oid, const char *action, {unsigned detach_head = flags &RESET_HEAD_DETACH ;unsigned reset_hard = flags &RESET_HEAD_HARD ; +unsigned refs_only = flags &RESET_HEAD_REFS_ONLY ;struct object_id head_oid;struct tree_desc desc[2 ] = { {NULL }, {NULL } };struct lock_file lock =LOCK_INIT ;builtin/rebase.c static int reset_head(struct object_id *oid, const char *action, · line 367
Quoted 8 lines
builtin/rebase.c +1
-1@@ -359,7 +361,7 @@ static int reset_head(struct object_id *oid, const char *action,if (switch_to_branch && !starts_with (switch_to_branch,"refs/" ))BUG ("Not a fully qualified branch: '%s'" , switch_to_branch); -if (hold_locked_index (&lock,LOCK_REPORT_ON_ERROR ) <0 ) { +if (!refs_only &&hold_locked_index (&lock,LOCK_REPORT_ON_ERROR ) <0 ) { ret =-1 ; goto leave_reset_head; }Not touching the index, so no need to lock the index. Makes sense.
builtin/rebase.c static int reset_head(struct object_id *oid, const char *action, · line 379
Quoted 7 lines
builtin/rebase.c +3
-0@@ -372,6 +374,9 @@ static int reset_head(struct object_id *oid, const char *action,if (!oid) oid = &head_oid; +if (flags &RESET_HEAD_REFS_ONLY ) + goto reset_head_refs; +Why not "
refs_only" that we already prepared above??? Are we munging that secondary variable before control comes here?In any case, not touching the index nor the working tree, so no need to call into the
unpack_treesmachinery. Makes sense.builtin/rebase.c static int reset_head(struct object_id *oid, const char *action, · line 421
Quoted 9 lines
builtin/rebase.c +1
-0memset (&unpack_tree_opts,0 ,sizeof (unpack_tree_opts));setup_unpack_trees_porcelain (&unpack_tree_opts, action); unpack_tree_opts.head_idx =1 ;@@ -412,6 +417,7 @@ static int reset_head(struct object_id *oid, const char *action, goto leave_reset_head; } +reset_head_refs: reflog_action =getenv (GIT_REFLOG_ACTION_ENVIRONMENT );And the control continues from the point we update the reflog. Makes sense.
Quoted 7 lines
builtin/rebase.c static int reset_head(struct object_id *oid, const char *action, · line 423
Quoted 2 lines
builtin/rebase.c +0
-0strbuf_addf (&msg,"%s: " , reflog_action ? reflog_action :"rebase" ); prefix_len = msg.len;This helper is touched by two other topics in flight, and that was one of the reason why it took a bit longer than usual for me to merge this topic. Please sanity-check the result of the conflict resolution at the tip of 'pu' branch.
The topics are os/rebase-runs-post-checkout-hook and nd/backup-log IIRC.
Re: [PATCH 4/4] built-in rebase: call `git am` directly
In reply to Johannes Schindelin via GitGitGadget"Johannes Schindelin via GitGitGadget" <[email protected]>
Quoted 4 lines · writes:
While the scripted
git rebasestill has to rely on thegit-rebase--am.shscript to implement the glue between therebaseand theamcommands, we can go a more direct route in the built-in rebase and avoid using a shell script altogether.
...
Quoted 1 line
builtin/rebase.c | 183 +++++++++++++++++++++++++++++++++++++++++++++++
Now at some point as a follow-up change, we'd need to remove the git-rebase--am.sh that is no longer used, together with the reference to it in the Makefile, no?
Re: [PATCH 4/4] built-in rebase: call `git am` directly
Quoted 8 lines · Junio C Hamano
"Johannes Schindelin via GitGitGadget" <[email protected]>
Quoted 4 lines · writes:
While the scripted
git rebasestill has to rely on thegit-rebase--am.shscript to implement the glue between therebaseand theamcommands, we can go a more direct route in the built-in rebase and avoid using a shell script altogether.
Quoted 7 lines
...
Quoted 1 line
builtin/rebase.c | 183 +++++++++++++++++++++++++++++++++++++++++++++++Now at some point as a follow-up change, we'd need to remove the
git-rebase--am.shthat is no longer used, together with the reference to it in theMakefile, no?
Answering to myself. That needs to wait until the legacy-rebase is retired, so it's not like "immediately after this series is done" kind of thing.
Anyway, thanks for working on this.
Re: [PATCH 4/4] built-in rebase: call `git am` directly
In reply to Junio C HamanoHi Junio,
Quoted 30 lines · Junio C Hamano
"Johannes Schindelin via GitGitGadget" <[email protected]>
Quoted 12 lines · writes:
builtin/rebase.c +12
-0+static int write_basic_state (struct rebase_options *opts) +{ +write_file (state_dir_path ("head-name" , opts),"%s" , + opts->head_name ? opts->head_name :"detached HEAD" ); +write_file (state_dir_path ("onto" , opts),"%s" , + opts->onto ?oid_to_hex (&opts->onto->object.oid) :"" ); +write_file (state_dir_path ("orig-head" , opts),"%s" , +oid_to_hex (&opts->orig_head)); +write_file (state_dir_path ("quiet" , opts),"%s" , + opts->flags &REBASE_NO_QUIET ?"" :"t" ); +if (opts->flags &REBASE_VERBOSE ) +write_file (state_dir_path ("verbose" , opts),"%s" ,"" );builtin/rebase.c static int read_basic_state(struct rebase_options *opts) · line 270
Quoted 10 lines
builtin/rebase.c +10
-0+if (opts->strategy) +write_file (state_dir_path ("strategy" , opts),"%s" , + opts->strategy); +if (opts->strategy_opts) +write_file (state_dir_path ("strategy_opts" , opts),"%s" , + opts->strategy_opts); +if (opts->allow_rerere_autoupdate >=0 ) +write_file (state_dir_path ("allow_rerere_autoupdate" , opts), +"-%s-rerere-autoupdate" , + opts->allow_rerere_autoupdate ?"" :"-no" );Inside rebase, allow-rerere-autoupdate can be -1 (unspecified), 0 (declined) or 1 (requested), and this code is being consistent with that convention.
Quoted 6 lines
The "--[no-]rerere-autoupdate" option that is parsed via
OPT_RERERE_AUTOUPDATE(used inbuiltin/rebase--interactive.camong other built-in commands) on the other hand is tertially that uses 0 (unspecified), 1 (requested) and 2 (declined). This might be a ticking timebomb to confuse us in the future that may be worth fixing but probably outside this series.
Good point. We use -1 for unspecified in so many places, I think OPT_RERERE_AUTOUPDATE needs to be fixed. But yes, I'll leave this as #leftoverbits here.
Quoted 38 lines
Quoted 4 lines
builtin/rebase.c +0
-0@@ -459,6 +490,30 @@ static int reset_head(struct object_id *oid, const char *action,return ret; }builtin/rebase.c static int reset_head(struct object_id *oid, const char *action, · line 502
Quoted 10 lines
builtin/rebase.c +10
-0+static int move_to_original_branch (struct rebase_options *opts) +{ +struct strbuf orig_head_reflog =STRBUF_INIT , head_reflog =STRBUF_INIT ; +int ret; + +if (!opts->head_name) +return 0 ;/* nothing to move back to */ + +if (!opts->onto) +BUG ("move_to_original_branch without onto" );This check is absent in the scripted version, but from the message we generate here, it is clear that the caller must not call this when there is no "onto" commit. Good.
builtin/rebase.c static int reset_head(struct object_id *oid, const char *action, · line 510
Quoted 7 lines
builtin/rebase.c +7
-0+strbuf_addf (&orig_head_reflog,"rebase finished: %s onto %s" , + opts->head_name,oid_to_hex (&opts->onto->object.oid)); +strbuf_addf (&head_reflog,"rebase finished: returning to %s" , + opts->head_name); + ret =reset_head (NULL ,"checkout" , opts->head_name, +RESET_HEAD_REFS_ONLY , + orig_head_reflog.buf, head_reflog.buf);The *action given to
reset_head()here is "checkout". Makes me wonder about two things:- The only real use of the parameter in the callee is to prepare the error and advice messages from the
unpack_treesmachinery, but because we are using it inREFS_ONLYmode, it does not matter. In fact it might even be misleading; perhaps pass NULL or something, so that a mistaken update toreset_head()later that letsREFS_ONLYrequest to go tounpack_treesmachinery will catch it as a bug?
Quoted 10 lines
- Another topic in flight wants to make sure that the post-checkout hook gets called when the
RESET_HEAD_RUN_POST_CHECKOUT_HOOKflag is given by the caller, and IIRC, the use of the flag is strongly correlated to *action being "checkout". Do we want to passREFS_ONLYandRUN_POST_CHECKOUT_HOOKflag for this call, or do we rather keep it silent? As the original scripted version did not use "checkout" here and never triggered post-checkout hook, I am inclined to say that we should not pass that other bit. That then leads me to suspect that we do not want *action to be "checkout" here.
The only thing for which that the action is used, though, is the call to setup_unpack_trees_porcelain(), which does not accept a NULL. I guess I could replace it by the empty string. Will do that.
Quoted 8 lines
builtin/rebase.c static int reset_head(struct object_id *oid, const char *action, · line 515
Quoted 5 lines
builtin/rebase.c +4
-0+strbuf_release (&orig_head_reflog); +strbuf_release (&head_reflog); +return ret; +}Unlike the scripted version, this does not
die()upon failure, so the caller needs to be careful about the returned status.
Indeed. That function is only called from run_am(), and returns the status in every instance. The caller of run_am(), run_specific_rebase() also handles it correctly.
Quoted 36 lines
Quoted 17 lines
builtin/rebase.c +12
-0@@ -466,6 +521,129 @@ N_("Resolve all conflicts manually, mark them as resolved with\n""To abort and get back to the state before \"git rebase\", run " "\"git rebase --abort\"."); +static int run_am (struct rebase_options *opts) +{ +struct child_process am =CHILD_PROCESS_INIT ; +struct child_process format_patch =CHILD_PROCESS_INIT ; +struct strbuf revisions =STRBUF_INIT ; +int status; +char *rebased_patches; + + am.git_cmd =1 ; +argv_array_push (&am.args,"am" ); + +if (opts->action && !strcmp ("continue" , opts->action)) {builtin/rebase.c N_("Resolve all conflicts manually, mark them as resolved with\n" · line 545
Quoted 10 lines
builtin/rebase.c +10
-0+argv_array_push (&am.args,"--resolved" ); +argv_array_pushf (&am.args,"--resolvemsg=%s" , resolvemsg); +if (opts->gpg_sign_opt) +argv_array_push (&am.args, opts->gpg_sign_opt); + status =run_command (&am); +if (status) +return status; + +discard_cache (); +return move_to_original_branch (opts);It is curious why
discard_cache()is placed exacly here, as if we want to preserve the contents of the in-core index whenrun_command()failed. But I do not think we care about the in-core index as the only thing that happen after "return status" is to return the control torun_specific_rebase(), let it jump tofinished_rebaselabel to clean things up and rturn control tocmd_rebase()and exit based on the status value.
Quoted 5 lines
It's not like
move_to_original_branch()wants to callread_cache()and get the result from the "am" thatrun_command()executed, either.Puzzled. Care to explain a bit more in the in-code comment?
I think that this call is just a left-over from a previous version that did not have the REFS_ONLY flag to pass to move_to_original_branch() (and it caused havoc before that flag was passed). Let me double-check whether the discard_cache() even makes sense any longer.
*clicketyclick* indeed that is the case. Will remove all three discard_cache() calls.
Quoted 23 lines
builtin/rebase.c N_("Resolve all conflicts manually, mark them as resolved with\n" · line 555
Quoted 11 lines
builtin/rebase.c +10
-0+ } +if (opts->action && !strcmp ("skip" , opts->action)) { +argv_array_push (&am.args,"--skip" ); +argv_array_pushf (&am.args,"--resolvemsg=%s" , resolvemsg); + status =run_command (&am); +if (status) +return status; + +discard_cache (); +return move_to_original_branch (opts);Ditto.
builtin/rebase.c N_("Resolve all conflicts manually, mark them as resolved with\n" · line 560
Quoted 5 lines
builtin/rebase.c +5
-0+ } +if (opts->action && !strcmp ("show-current-patch" , opts->action)) { +argv_array_push (&am.args,"--show-current-patch" ); +return run_command (&am); + }Up to this point, it is a faithful conversion of the first case/esac statement. Good.
Quoted 8 lines
builtin/rebase.c N_("Resolve all conflicts manually, mark them as resolved with\n" · line 564
Quoted 3 lines
builtin/rebase.c +3
-0+ strbuf_addf(&revisions, "%s...%s", + oid_to_hex(opts->root ? +/* this is now equivalent to ! -z "$upstream" */ Does "this" refer to the "
opts->rootbeing true" check?Because you are flipping the polarity of the test from scripted version, shouldn't the comment be updated to "-z $upstream"?
It did flip the polarity, you are right, this comment is incorrect. It is even more incorrect, though, as it talks about a shell construct that is no longer applicable. Will fix.
Quoted 5 lines
Quoted 4 lines
builtin/rebase.c +3
-0+ &opts->onto->object.oid : + &opts->upstream->object.oid), + oid_to_hex(&opts->orig_head));
Quoted 10 lines
builtin/rebase.c N_("Resolve all conflicts manually, mark them as resolved with\n" · line 571
Quoted 3 lines
builtin/rebase.c +3
-0+ rebased_patches =xstrdup (git_path ("rebased-patches" )); + format_patch.out =open (rebased_patches, +O_WRONLY |O_CREAT |O_TRUNC ,0666 );Unlike scripted version, we do not remove a (possibly) existing file. We give CREAT in case there is no existing one, and TRUNC in case there is an existing one. Makes sense. A more faithful translation would have unlink(2)ed a (possibly) existing one, and then because we can afford to, passed
O_EXCLto avoid stomping on somebody else racing with us, but I do not think it is worth it.
Okay.
Quoted 5 lines
builtin/rebase.c N_("Resolve all conflicts manually, mark them as resolved with\n" · line 574
Quoted 3 lines
builtin/rebase.c +3
-0+if (format_patch.out <0 ) { + status =error_errno (_ ("could not write '%s'" ), + rebased_patches);s/write '%s'/open '%s' for writing/? I dunno.
Yep, of course! Will fix.
Quoted 23 lines
Quoted 7 lines
builtin/rebase.c +7
-0+free (rebased_patches); +argv_array_clear (&am.args); +return status; + } + + format_patch.git_cmd =1 ; + argv_array_pushl(&format_patch.args, "format-patch", "-k", "--stdout",builtin/rebase.c N_("Resolve all conflicts manually, mark them as resolved with\n" · line 591
Quoted 10 lines
builtin/rebase.c +10
-0+"--full-index" ,"--cherry-pick" ,"--right-only" , +"--src-prefix=a/" ,"--dst-prefix=b/" ,"--no-renames" , + "--no-cover-letter", "--pretty=mboxrd", NULL); +if (opts->git_format_patch_opt.len) +argv_array_split (&format_patch.args, + opts->git_format_patch_opt.buf); +argv_array_push (&format_patch.args, revisions.buf); +if (opts->restrict_revision) +argv_array_pushf (&format_patch.args,"^%s" , +oid_to_hex (&opts->restrict_revision->object.oid));It is kinda surprising to see that we have learned quite a lot of fringe "configurations" we need to explicitly override like this.
Looks like a quite faithful conversion, anyway.
Quoted 12 lines
builtin/rebase.c N_("Resolve all conflicts manually, mark them as resolved with\n" · line 600
Quoted 8 lines
builtin/rebase.c +8
-0+ status =run_command (&format_patch); +if (status) { +unlink (rebased_patches); +free (rebased_patches); +argv_array_clear (&am.args); + +reset_head (&opts->orig_head,"checkout" , opts->head_name,0 , +"HEAD" ,NULL );This one may need to trigger post-checkout hook. The scripted version does two different things depending on the value of $head_name, but we can just use the same code without conditional?
Yes, because opts->head_name is NULL in one case, and not NULL in the other, and the reset_head() function performs the desired operation in each case.
Quoted 10 lines
Quoted 10 lines
builtin/rebase.c +10
-0+error (_ ("\ngit encountered an error while preparing the " +"patches to replay\n" +"these revisions:\n" +"\n %s\n\n" +"As a result, git cannot rebase them." ), + opts->revisions); + +strbuf_release (&revisions); +return status; + }
Quoted 8 lines
builtin/rebase.c N_("Resolve all conflicts manually, mark them as resolved with\n" · line 616
Quoted 6 lines
builtin/rebase.c +6
-0+strbuf_release (&revisions); + + am.in =open (rebased_patches,O_RDONLY ); +if (am.in <0 ) { + status =error_errno (_ ("could not read '%s'" ), + rebased_patches);s/write '%s'/open '%s' for reading/? I dunno.
Yep, will fix.
Quoted 53 lines
Quoted 35 lines
builtin/rebase.c +30
-0+free (rebased_patches); +argv_array_clear (&am.args); +return status; + } + +argv_array_pushv (&am.args, opts->git_am_opts.argv); +argv_array_push (&am.args,"--rebasing" ); +argv_array_pushf (&am.args,"--resolvemsg=%s" , resolvemsg); +argv_array_push (&am.args,"--patch-format=mboxrd" ); +if (opts->allow_rerere_autoupdate >0 ) +argv_array_push (&am.args,"--rerere-autoupdate" ); +else if (opts->allow_rerere_autoupdate ==0 ) +argv_array_push (&am.args,"--no-rerere-autoupdate" ); +if (opts->gpg_sign_opt) +argv_array_push (&am.args, opts->gpg_sign_opt); + status =run_command (&am); +unlink (rebased_patches); +free (rebased_patches); + +if (!status) { +discard_cache (); +return move_to_original_branch (opts); + } + +if (is_directory (opts->state_dir)) +write_basic_state (opts); + +return status; +} +static int run_specific_rebase (struct rebase_options *opts) {const char *argv[] = {NULL ,NULL };builtin/rebase.c static int run_specific_rebase(struct rebase_options *opts) · line 733
Quoted 10 lines
builtin/rebase.c +5
-0@@ -546,6 +724,11 @@ static int run_specific_rebase(struct rebase_options *opts) goto finished_rebase; } +if (opts->type ==REBASE_AM ) { + status =run_am (opts); + goto finished_rebase; + } +add_var (&script_snippet,"GIT_DIR" ,absolute_path (get_git_dir ()));add_var (&script_snippet,"state_dir" , opts->state_dir);Overall, this was quite a pleasant read and a well constructed series. Other than two minor points (i.e. interaction with the 'post-checkout hook' topic, and
discard_cache()before callingmove_to_original_branch) I did not quite understand, looks good to me.
Quoted 3 lines
When merged to 'pu', I seem to be getting failure from t3425.5, .8 and .11, by the way. I haven't dug into the actual breakages any further than that.
Sorry for the trouble, and for my silence (I was heads-down into the Azure Pipelines support).
I did not see any breakage in pu lately, hopefully things resolved themselves?
Ciao,
Dscho
Re: [PATCH 3/4] rebase: teach `reset_head()` to optionally skip the worktree
In reply to Junio C HamanoHi Junio,
Quoted 9 lines · Junio C Hamano
"Johannes Schindelin via GitGitGadget" <[email protected]>
Quoted 5 lines · writes:
From: Johannes Schindelin <[email protected]>
This is what the legacy (scripted) rebase does in
move_to_original_branch, and we will need this functionality in the next commit.
Quoted 7 lines
The move-to-original-branch helper does:
- point $head_name to the commit pointed at by HEAD
- point HEAD symref to $head_namewithout touching the index or the working tree files. It's not exactly "reset --soft" but more like "switch-branch --soft" ;-)
Right, but it does reset the HEAD softly.
Quoted 11 lines
Quoted 11 lines
Signed-off-by Johannes Schindelin <[email protected]>--- builtin/rebase.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)builtin/rebase.c +0
-0@@ -337,6 +337,7 @@ static void add_var(struct strbuf *buf, const char *name, const char *value)
Quoted 7 lines
builtin/rebase.c static void add_var(struct strbuf *buf, const char *name, const char *value) · line 340
Quoted 3 lines
builtin/rebase.c +1
-0#define RESET_HEAD_DETACH (1<<0)#define RESET_HEAD_HARD (1<<1) +#define RESET_HEAD_REFS_ONLY (1<<2)In the future codebase in 'pu', we have 1<<2 already taken by another topic, so I'll tell my rerere database that the bit assignment needs to be adjusted.
Okay.
Quoted 26 lines
Quoted 11 lines
builtin/rebase.c +1
-0static int reset_head(struct object_id *oid, const char *action, const char *switch_to_branch, unsigned flags,@@ -344,6 +345,7 @@ static int reset_head(struct object_id *oid, const char *action, {unsigned detach_head = flags &RESET_HEAD_DETACH ;unsigned reset_hard = flags &RESET_HEAD_HARD ; +unsigned refs_only = flags &RESET_HEAD_REFS_ONLY ;struct object_id head_oid;struct tree_desc desc[2 ] = { {NULL }, {NULL } };struct lock_file lock =LOCK_INIT ;builtin/rebase.c static int reset_head(struct object_id *oid, const char *action, · line 367
Quoted 8 lines
builtin/rebase.c +1
-1@@ -359,7 +361,7 @@ static int reset_head(struct object_id *oid, const char *action,if (switch_to_branch && !starts_with (switch_to_branch,"refs/" ))BUG ("Not a fully qualified branch: '%s'" , switch_to_branch); -if (hold_locked_index (&lock,LOCK_REPORT_ON_ERROR ) <0 ) { +if (!refs_only &&hold_locked_index (&lock,LOCK_REPORT_ON_ERROR ) <0 ) { ret =-1 ; goto leave_reset_head; }Not touching the index, so no need to lock the index. Makes sense.
Quoted 4 lines
builtin/rebase.c +0
-0@@ -372,6 +374,9 @@ static int reset_head(struct object_id *oid, const char *action,if (!oid) oid = &head_oid;
Quoted 6 lines
builtin/rebase.c static int reset_head(struct object_id *oid, const char *action, · line 379
Quoted 3 lines
builtin/rebase.c +3
-0+if (flags &RESET_HEAD_REFS_ONLY ) + goto reset_head_refs; +Why not "
refs_only" that we already prepared above??? Are we munging that secondary variable before control comes here?
No, just an oversight. Will fix.
Quoted 16 lines
In any case, not touching the index nor the working tree, so no need to call into the
unpack_treesmachinery. Makes sense.builtin/rebase.c static int reset_head(struct object_id *oid, const char *action, · line 421
Quoted 9 lines
builtin/rebase.c +1
-0memset (&unpack_tree_opts,0 ,sizeof (unpack_tree_opts));setup_unpack_trees_porcelain (&unpack_tree_opts, action); unpack_tree_opts.head_idx =1 ;@@ -412,6 +417,7 @@ static int reset_head(struct object_id *oid, const char *action, goto leave_reset_head; } +reset_head_refs: reflog_action =getenv (GIT_REFLOG_ACTION_ENVIRONMENT );And the control continues from the point we update the reflog. Makes sense.
Quoted 7 lines
builtin/rebase.c static int reset_head(struct object_id *oid, const char *action, · line 423
Quoted 2 lines
builtin/rebase.c +0
-0strbuf_addf (&msg,"%s: " , reflog_action ? reflog_action :"rebase" ); prefix_len = msg.len;This helper is touched by two other topics in flight, and that was one of the reason why it took a bit longer than usual for me to merge this topic. Please sanity-check the result of the conflict resolution at the tip of 'pu' branch.
Sorry, I *just now* had time to take care of this patch series. I will gladly check once you integrate the new patch series iteration.
Thanks,
Dscho
Re: [PATCH 2/4] rebase: avoid double reflog entry when switching branches
In reply to Junio C HamanoHi Junio,
Quoted 15 lines · Junio C Hamano
"Johannes Schindelin via GitGitGadget" <[email protected]>
Quoted 5 lines · writes:
From: Johannes Schindelin <[email protected]>
When switching a branch *and* updating said branch to a different revision, let's avoid a double entry by first updating the branch and then adjusting the symbolic ref HEAD.
Ah, in the original sequence, HEAD is updated twice, leaving two reflog entries for HEAD (and one for the underlying "
switch_to" branch by virtue ofREF_UPDATE_VIA_HEAD). In the new sequence,update_ref()updates the underlying "switch_to" and then HEAD, so we'd get one reflog entry for each of them.
Quoted 3 lines
Makes sense. s/let's avoid a double entry/& in HEAD's reflog/ would have avoided wasting reader's time who needlessly wondered where that redundancy came from, though.
Will fix the commit message.
Thanks!
Dscho
Quoted 27 lines
Quoted 26 lines
Signed-off-by Johannes Schindelin <[email protected]>--- builtin/rebase.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)builtin/rebase.c +4
-3@@ -438,10 +438,11 @@ static int reset_head(struct object_id *oid, const char *action, detach_head ?REF_NO_DEREF :0 , UPDATE_REFS_MSG_ON_ERR); else { - ret =create_symref ("HEAD" , switch_to_branch, msg.buf); + ret =update_ref (reflog_orig_head, switch_to_branch, oid, +NULL ,0 ,UPDATE_REFS_MSG_ON_ERR );if (!ret) - ret =update_ref (reflog_head,"HEAD" , oid,NULL ,0 , -UPDATE_REFS_MSG_ON_ERR ); + ret =create_symref ("HEAD" , switch_to_branch, + reflog_head); } leave_reset_head:
v2 cover Let the builtin rebase call the git am command directly
In reply to Johannes Schindelin via GitGitGadgetEspecially on Windows, where Unix shell scripting is a foreign endeavor, and an expensive one at that, we really want to avoid running through the Bash.
This not only makes everything faster, but also more robust, as the Bash we use on Windows relies on a derivative of the Cygwin runtime, which in turn has to jump through a couple of hoops that are sometimes a little too tricky to make things work. Read: the less we rely on Unix shell scripting, the more likely Windows users will be able to enjoy our software.
Changes since v1:
* Rebased on top of master to avoid merge conflicts.
* Adjusted the commit message talking about double entries, to clarify that it talks about HEAD's reflog.
* Replaced a misleading action parameter "checkout" for the reset_head() function by the empty string: we do not check out here, we just update the refs, and certainly do not want any checkout functionality (such as hooks) to be involved.
* Reused a just-prepared refs_only variable instead of repeating the value assigned to it.
* Fixed a stale comment about the shell variable "$upstream" (which should have been negated to begin with).
* Fixed error messages when files could not be opened.
Johannes Schindelin (4):
rebase: move reset_head() into a better spot
rebase: avoid double reflog entry when switching branches
rebase: teach reset_head() to optionally skip the worktree built-in rebase: call git am directly
builtin/rebase.c | 424 ++++++++++++++++++++++++++++++++++------------- 1 file changed, 305 insertions(+), 119 deletions(-)
base-commit: 77556354bb7ac50450e3b28999e3576969869068
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-24%2Fdscho%2Fbuiltin-rebase--am-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-24/dscho/builtin-rebase--am-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/24
Range-diff vs v1:
1: 175dc7d29a ! 1: e886ae3de5 rebase: move reset_head() into a better spot
@@ -91,7 +91,7 @@
+ }
+
+ tree = parse_tree_indirect(oid);
-+ prime_cache_tree(the_repository->index, tree);
++ prime_cache_tree(the_repository, the_repository->index, tree);
+
+ if (write_locked_index(the_repository->index, &lock, COMMIT_LOCK) < 0) {
+ ret = error(_("could not write index"));
@@ -217,7 +217,7 @@
- }
-
- tree = parse_tree_indirect(oid);
-- prime_cache_tree(the_repository->index, tree);
+- prime_cache_tree(the_repository, the_repository->index, tree);
-
- if (write_locked_index(the_repository->index, &lock, COMMIT_LOCK) < 0) {
- ret = error(_("could not write index")); 2: 4c5f87b9dc ! 2: 3a68f1c509 rebase: avoid double reflog entry when switching branches
@@ -3,8 +3,8 @@
rebase: avoid double reflog entry when switching branches
When switching a branch *and* updating said branch to a different
- revision, let's avoid a double entry by first updating the branch and
- then adjusting the symbolic ref HEAD.
+ revision, let's avoid a double entry in HEAD's reflog by first updating
+ the branch and then adjusting the symbolic ref HEAD.
3: 21939140e0 ! 3: 387071dcd7 rebase: teach reset_head() to optionally skip the worktree
@@ -40,7 +40,7 @@
if (!oid)
oid = &head_oid;
-+ if (flags & RESET_HEAD_REFS_ONLY)
++ if (refs_only)
+ goto reset_head_refs;
+
memset(&unpack_tree_opts, 0, sizeof(unpack_tree_opts));
4: 2b5ece8263 ! 4: 3c40318682 built-in rebase: call `git am` directly
@@ -16,6 +16,10 @@
itself a derivative of the Cygwin runtime): when no shell script is
called, the POSIX emulation layer is avoided altogether.
+ Note: we pass an empty action to `reset_head()` here when moving back to
+ the original branch, as no other action is applicable, really. This
+ parameter is used to initialize `unpack_trees()`' messages.
+
Signed-off-by: Johannes Schindelin <[email protected]>
diff --git a/builtin/rebase.c b/builtin/rebase.c
@@ -78,8 +82,7 @@
+ opts->head_name, oid_to_hex(&opts->onto->object.oid));
+ strbuf_addf(&head_reflog, "rebase finished: returning to %s",
+ opts->head_name);
-+ ret = reset_head(NULL, "checkout", opts->head_name,
-+ RESET_HEAD_REFS_ONLY,
++ ret = reset_head(NULL, "", opts->head_name, RESET_HEAD_REFS_ONLY,
+ orig_head_reflog.buf, head_reflog.buf);
+
+ strbuf_release(&orig_head_reflog);
@@ -114,7 +117,6 @@
+ if (status)
+ return status;
+
-+ discard_cache();
+ return move_to_original_branch(opts);
+ }
+ if (opts->action && !strcmp("skip", opts->action)) {
@@ -124,7 +126,6 @@
+ if (status)
+ return status;
+
-+ discard_cache();
+ return move_to_original_branch(opts);
+ }
+ if (opts->action && !strcmp("show-current-patch", opts->action)) {
@@ -134,7 +135,7 @@
+
+ strbuf_addf(&revisions, "%s...%s",
+ oid_to_hex(opts->root ?
-+ /* this is now equivalent to ! -z "$upstream" */
++ /* this is now equivalent to !opts->upstream */
+ &opts->onto->object.oid :
+ &opts->upstream->object.oid),
+ oid_to_hex(&opts->orig_head));
@@ -143,7 +144,7 @@
+ format_patch.out = open(rebased_patches,
+ O_WRONLY | O_CREAT | O_TRUNC, 0666);
+ if (format_patch.out < 0) {
-+ status = error_errno(_("could not write '%s'"),
++ status = error_errno(_("could not open '%s' for writing"),
+ rebased_patches);
+ free(rebased_patches);
+ argv_array_clear(&am.args);
@@ -185,7 +186,7 @@
+
+ am.in = open(rebased_patches, O_RDONLY);
+ if (am.in < 0) {
-+ status = error_errno(_("could not read '%s'"),
++ status = error_errno(_("could not open '%s' for reading"),
+ rebased_patches);
+ free(rebased_patches);
+ argv_array_clear(&am.args);
@@ -207,7 +208,6 @@
+ free(rebased_patches);
+
+ if (!status) {
-+ discard_cache();
+ return move_to_original_branch(opts);
+ }
+Signature · 1 line
gitgitgadget
PATCH v2 1/4 rebase: move `reset_head()` into a better spot
From: Johannes Schindelin <[email protected]>
Over the next commits, we want to make use of it in run_am() (i.e. running the --am backend directly, without detouring to Unix shell script code) which in turn will be called from run_specific_rebase().
So let's move it before that latter function.
This commit is best viewed using --color-moved.
--- builtin/rebase.c | 238 +++++++++++++++++++++++------------------------ 1 file changed, 119 insertions(+), 119 deletions(-)
builtin/rebase.c +119 -119
@@ -333,6 +333,125 @@ static void add_var(struct strbuf *buf, const char *name, const char *value) } } +#define GIT_REFLOG_ACTION_ENVIRONMENT "GIT_REFLOG_ACTION" + +#define RESET_HEAD_DETACH (1<<0) +#define RESET_HEAD_HARD (1<<1) + +static int reset_head (struct object_id *oid,const char *action, +const char *switch_to_branch,unsigned flags, +const char *reflog_orig_head,const char *reflog_head) +{ +unsigned detach_head = flags &RESET_HEAD_DETACH ; +unsigned reset_hard = flags &RESET_HEAD_HARD ; +struct object_id head_oid; +struct tree_desc desc[2 ] = { {NULL }, {NULL } }; +struct lock_file lock =LOCK_INIT ; +struct unpack_trees_options unpack_tree_opts; +struct tree *tree; +const char *reflog_action; +struct strbuf msg =STRBUF_INIT ; +size_t prefix_len; +struct object_id *orig =NULL , oid_orig, + *old_orig =NULL , oid_old_orig; +int ret =0 , nr =0 ; + +if (switch_to_branch && !starts_with (switch_to_branch,"refs/" )) +BUG ("Not a fully qualified branch: '%s'" , switch_to_branch); + +if (hold_locked_index (&lock,LOCK_REPORT_ON_ERROR ) <0 ) { + ret =-1 ; + goto leave_reset_head; + } + +if ((!oid || !reset_hard) &&get_oid ("HEAD" , &head_oid)) { + ret =error (_ ("could not determine HEAD revision" )); + goto leave_reset_head; + } + +if (!oid) + oid = &head_oid; + +memset (&unpack_tree_opts,0 ,sizeof (unpack_tree_opts)); +setup_unpack_trees_porcelain (&unpack_tree_opts, action); + unpack_tree_opts.head_idx =1 ; + unpack_tree_opts.src_index = the_repository->index; + unpack_tree_opts.dst_index = the_repository->index; + unpack_tree_opts.fn = reset_hard ? oneway_merge : twoway_merge; + unpack_tree_opts.update =1 ; + unpack_tree_opts.merge =1 ; +if (!detach_head) + unpack_tree_opts.reset =1 ; + +if (read_index_unmerged (the_repository->index) <0 ) { + ret =error (_ ("could not read index" )); + goto leave_reset_head; + } + +if (!reset_hard && !fill_tree_descriptor (&desc[nr++], &head_oid)) { + ret =error (_ ("failed to find tree of %s" ), +oid_to_hex (&head_oid)); + goto leave_reset_head; + } + +if (!fill_tree_descriptor (&desc[nr++], oid)) { + ret =error (_ ("failed to find tree of %s" ),oid_to_hex (oid)); + goto leave_reset_head; + } + +if (unpack_trees (nr, desc, &unpack_tree_opts)) { + ret =-1 ; + goto leave_reset_head; + } + + tree =parse_tree_indirect (oid); +prime_cache_tree (the_repository, the_repository->index, tree); + +if (write_locked_index (the_repository->index, &lock,COMMIT_LOCK ) <0 ) { + ret =error (_ ("could not write index" )); + goto leave_reset_head; + } + + reflog_action =getenv (GIT_REFLOG_ACTION_ENVIRONMENT ); +strbuf_addf (&msg,"%s: " , reflog_action ? reflog_action :"rebase" ); + prefix_len = msg.len; + +if (!get_oid ("ORIG_HEAD" , &oid_old_orig)) + old_orig = &oid_old_orig; +if (!get_oid ("HEAD" , &oid_orig)) { + orig = &oid_orig; +if (!reflog_orig_head) { +strbuf_addstr (&msg,"updating ORIG_HEAD" ); + reflog_orig_head = msg.buf; + } +update_ref (reflog_orig_head,"ORIG_HEAD" , orig, old_orig,0 , +UPDATE_REFS_MSG_ON_ERR ); + }else if (old_orig) +delete_ref (NULL ,"ORIG_HEAD" , old_orig,0 ); +if (!reflog_head) { +strbuf_setlen (&msg, prefix_len); +strbuf_addstr (&msg,"updating HEAD" ); + reflog_head = msg.buf; + } +if (!switch_to_branch) + ret =update_ref (reflog_head,"HEAD" , oid, orig, + detach_head ?REF_NO_DEREF :0 , +UPDATE_REFS_MSG_ON_ERR ); +else { + ret =create_symref ("HEAD" , switch_to_branch, msg.buf); +if (!ret) + ret =update_ref (reflog_head,"HEAD" , oid,NULL ,0 , +UPDATE_REFS_MSG_ON_ERR ); + } + +leave_reset_head: +strbuf_release (&msg); +rollback_lock_file (&lock); +while (nr) +free ((void *)desc[--nr].buffer); +return ret; +} +static const char *resolvemsg =N_ ("Resolve all conflicts manually, mark them as resolved with\n" "\"git add/rm <conflicted_files>\", then run \"git rebase --continue\".\n"
@@ -526,125 +645,6 @@ static int run_specific_rebase(struct rebase_options *opts)return status ?-1 :0 ; } -#define GIT_REFLOG_ACTION_ENVIRONMENT "GIT_REFLOG_ACTION" - -#define RESET_HEAD_DETACH (1<<0) -#define RESET_HEAD_HARD (1<<1) - -static int reset_head (struct object_id *oid,const char *action, -const char *switch_to_branch,unsigned flags, -const char *reflog_orig_head,const char *reflog_head) -{ -unsigned detach_head = flags &RESET_HEAD_DETACH ; -unsigned reset_hard = flags &RESET_HEAD_HARD ; -struct object_id head_oid; -struct tree_desc desc[2 ] = { {NULL }, {NULL } }; -struct lock_file lock =LOCK_INIT ; -struct unpack_trees_options unpack_tree_opts; -struct tree *tree; -const char *reflog_action; -struct strbuf msg =STRBUF_INIT ; -size_t prefix_len; -struct object_id *orig =NULL , oid_orig, - *old_orig =NULL , oid_old_orig; -int ret =0 , nr =0 ; - -if (switch_to_branch && !starts_with (switch_to_branch,"refs/" )) -BUG ("Not a fully qualified branch: '%s'" , switch_to_branch); - -if (hold_locked_index (&lock,LOCK_REPORT_ON_ERROR ) <0 ) { - ret =-1 ; - goto leave_reset_head; - } - -if ((!oid || !reset_hard) &&get_oid ("HEAD" , &head_oid)) { - ret =error (_ ("could not determine HEAD revision" )); - goto leave_reset_head; - } - -if (!oid) - oid = &head_oid; - -memset (&unpack_tree_opts,0 ,sizeof (unpack_tree_opts)); -setup_unpack_trees_porcelain (&unpack_tree_opts, action); - unpack_tree_opts.head_idx =1 ; - unpack_tree_opts.src_index = the_repository->index; - unpack_tree_opts.dst_index = the_repository->index; - unpack_tree_opts.fn = reset_hard ? oneway_merge : twoway_merge; - unpack_tree_opts.update =1 ; - unpack_tree_opts.merge =1 ; -if (!detach_head) - unpack_tree_opts.reset =1 ; - -if (read_index_unmerged (the_repository->index) <0 ) { - ret =error (_ ("could not read index" )); - goto leave_reset_head; - } - -if (!reset_hard && !fill_tree_descriptor (&desc[nr++], &head_oid)) { - ret =error (_ ("failed to find tree of %s" ), -oid_to_hex (&head_oid)); - goto leave_reset_head; - } - -if (!fill_tree_descriptor (&desc[nr++], oid)) { - ret =error (_ ("failed to find tree of %s" ),oid_to_hex (oid)); - goto leave_reset_head; - } - -if (unpack_trees (nr, desc, &unpack_tree_opts)) { - ret =-1 ; - goto leave_reset_head; - } - - tree =parse_tree_indirect (oid); -prime_cache_tree (the_repository, the_repository->index, tree); - -if (write_locked_index (the_repository->index, &lock,COMMIT_LOCK ) <0 ) { - ret =error (_ ("could not write index" )); - goto leave_reset_head; - } - - reflog_action =getenv (GIT_REFLOG_ACTION_ENVIRONMENT ); -strbuf_addf (&msg,"%s: " , reflog_action ? reflog_action :"rebase" ); - prefix_len = msg.len; - -if (!get_oid ("ORIG_HEAD" , &oid_old_orig)) - old_orig = &oid_old_orig; -if (!get_oid ("HEAD" , &oid_orig)) { - orig = &oid_orig; -if (!reflog_orig_head) { -strbuf_addstr (&msg,"updating ORIG_HEAD" ); - reflog_orig_head = msg.buf; - } -update_ref (reflog_orig_head,"ORIG_HEAD" , orig, old_orig,0 , -UPDATE_REFS_MSG_ON_ERR ); - }else if (old_orig) -delete_ref (NULL ,"ORIG_HEAD" , old_orig,0 ); -if (!reflog_head) { -strbuf_setlen (&msg, prefix_len); -strbuf_addstr (&msg,"updating HEAD" ); - reflog_head = msg.buf; - } -if (!switch_to_branch) - ret =update_ref (reflog_head,"HEAD" , oid, orig, - detach_head ?REF_NO_DEREF :0 , -UPDATE_REFS_MSG_ON_ERR ); -else { - ret =create_symref ("HEAD" , switch_to_branch, msg.buf); -if (!ret) - ret =update_ref (reflog_head,"HEAD" , oid,NULL ,0 , -UPDATE_REFS_MSG_ON_ERR ); - } - -leave_reset_head: -strbuf_release (&msg); -rollback_lock_file (&lock); -while (nr) -free ((void *)desc[--nr].buffer); -return ret; -} -static int rebase_config (const char *var,const char *value,void *data) {struct rebase_options *opts = data;
Signature · 1 line
gitgitgadget
PATCH v2 2/4 rebase: avoid double reflog entry when switching branches
In reply to Johannes Schindelin via GitGitGadgetFrom: Johannes Schindelin <[email protected]>
When switching a branch *and* updating said branch to a different revision, let's avoid a double entry in HEAD's reflog by first updating the branch and then adjusting the symbolic ref HEAD.
--- builtin/rebase.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)
builtin/rebase.c +4 -3
@@ -438,10 +438,11 @@ static int reset_head(struct object_id *oid, const char *action, detach_head ?REF_NO_DEREF :0 , UPDATE_REFS_MSG_ON_ERR); else { - ret =create_symref ("HEAD" , switch_to_branch, msg.buf); + ret =update_ref (reflog_orig_head, switch_to_branch, oid, +NULL ,0 ,UPDATE_REFS_MSG_ON_ERR );if (!ret) - ret =update_ref (reflog_head,"HEAD" , oid,NULL ,0 , -UPDATE_REFS_MSG_ON_ERR ); + ret =create_symref ("HEAD" , switch_to_branch, + reflog_head); } leave_reset_head:
Signature · 1 line
gitgitgadget
PATCH v2 3/4 rebase: teach `reset_head()` to optionally skip the worktree
In reply to Johannes Schindelin via GitGitGadgetFrom: Johannes Schindelin <[email protected]>
This is what the legacy (scripted) rebase does in move_to_original_branch, and we will need this functionality in the next commit.
--- builtin/rebase.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)
builtin/rebase.c +7 -1
@@ -337,6 +337,7 @@ static void add_var(struct strbuf *buf, const char *name, const char *value)#define RESET_HEAD_DETACH (1<<0)#define RESET_HEAD_HARD (1<<1) +#define RESET_HEAD_REFS_ONLY (1<<2) static int reset_head(struct object_id *oid, const char *action, const char *switch_to_branch, unsigned flags,
@@ -344,6 +345,7 @@ static int reset_head(struct object_id *oid, const char *action, {unsigned detach_head = flags &RESET_HEAD_DETACH ;unsigned reset_hard = flags &RESET_HEAD_HARD ; +unsigned refs_only = flags &RESET_HEAD_REFS_ONLY ;struct object_id head_oid;struct tree_desc desc[2 ] = { {NULL }, {NULL } };struct lock_file lock =LOCK_INIT ;
@@ -359,7 +361,7 @@ static int reset_head(struct object_id *oid, const char *action,if (switch_to_branch && !starts_with (switch_to_branch,"refs/" ))BUG ("Not a fully qualified branch: '%s'" , switch_to_branch); -if (hold_locked_index (&lock,LOCK_REPORT_ON_ERROR ) <0 ) { +if (!refs_only &&hold_locked_index (&lock,LOCK_REPORT_ON_ERROR ) <0 ) { ret =-1 ; goto leave_reset_head; }
@@ -372,6 +374,9 @@ static int reset_head(struct object_id *oid, const char *action,if (!oid) oid = &head_oid; +if (refs_only) + goto reset_head_refs; +memset (&unpack_tree_opts,0 ,sizeof (unpack_tree_opts));setup_unpack_trees_porcelain (&unpack_tree_opts, action); unpack_tree_opts.head_idx =1 ;
@@ -412,6 +417,7 @@ static int reset_head(struct object_id *oid, const char *action, goto leave_reset_head; } +reset_head_refs: reflog_action =getenv (GIT_REFLOG_ACTION_ENVIRONMENT );strbuf_addf (&msg,"%s: " , reflog_action ? reflog_action :"rebase" ); prefix_len = msg.len;
Signature · 1 line
gitgitgadget
PATCH v2 4/4 built-in rebase: call `git am` directly
In reply to Johannes Schindelin via GitGitGadgetFrom: Johannes Schindelin <[email protected]>
While the scripted git rebase still has to rely on the git-rebase--am.sh script to implement the glue between the rebase and the am commands, we can go a more direct route in the built-in rebase and avoid using a shell script altogether.
This patch represents a straight-forward port of git-rebase--am.sh to C, along with the glue code to call it directly from within builtin/rebase.c.
This reduces the chances of Git for Windows running into trouble due to problems with the POSIX emulation layer (known as "MSYS2 runtime", itself a derivative of the Cygwin runtime): when no shell script is called, the POSIX emulation layer is avoided altogether.
Note: we pass an empty action to reset_head() here when moving back to the original branch, as no other action is applicable, really. This parameter is used to initialize unpack_trees()' messages.
--- builtin/rebase.c | 179 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 179 insertions(+)
builtin/rebase.c +179 -0
@@ -246,6 +246,37 @@ static int read_basic_state(struct rebase_options *opts)return 0 ; } +static int write_basic_state (struct rebase_options *opts) +{ +write_file (state_dir_path ("head-name" , opts),"%s" , + opts->head_name ? opts->head_name :"detached HEAD" ); +write_file (state_dir_path ("onto" , opts),"%s" , + opts->onto ?oid_to_hex (&opts->onto->object.oid) :"" ); +write_file (state_dir_path ("orig-head" , opts),"%s" , +oid_to_hex (&opts->orig_head)); +write_file (state_dir_path ("quiet" , opts),"%s" , + opts->flags &REBASE_NO_QUIET ?"" :"t" ); +if (opts->flags &REBASE_VERBOSE ) +write_file (state_dir_path ("verbose" , opts),"%s" ,"" ); +if (opts->strategy) +write_file (state_dir_path ("strategy" , opts),"%s" , + opts->strategy); +if (opts->strategy_opts) +write_file (state_dir_path ("strategy_opts" , opts),"%s" , + opts->strategy_opts); +if (opts->allow_rerere_autoupdate >=0 ) +write_file (state_dir_path ("allow_rerere_autoupdate" , opts), +"-%s-rerere-autoupdate" , + opts->allow_rerere_autoupdate ?"" :"-no" ); +if (opts->gpg_sign_opt) +write_file (state_dir_path ("gpg_sign_opt" , opts),"%s" , + opts->gpg_sign_opt); +if (opts->signoff) +write_file (state_dir_path ("strategy" , opts),"--signoff" ); + +return 0 ; +} +static int apply_autostash (struct rebase_options *opts) {const char *path =state_dir_path ("autostash" , opts);
@@ -459,6 +490,29 @@ static int reset_head(struct object_id *oid, const char *action,return ret; } +static int move_to_original_branch (struct rebase_options *opts) +{ +struct strbuf orig_head_reflog =STRBUF_INIT , head_reflog =STRBUF_INIT ; +int ret; + +if (!opts->head_name) +return 0 ;/* nothing to move back to */ + +if (!opts->onto) +BUG ("move_to_original_branch without onto" ); + +strbuf_addf (&orig_head_reflog,"rebase finished: %s onto %s" , + opts->head_name,oid_to_hex (&opts->onto->object.oid)); +strbuf_addf (&head_reflog,"rebase finished: returning to %s" , + opts->head_name); + ret =reset_head (NULL ,"" , opts->head_name,RESET_HEAD_REFS_ONLY , + orig_head_reflog.buf, head_reflog.buf); + +strbuf_release (&orig_head_reflog); +strbuf_release (&head_reflog); +return ret; +} +static const char *resolvemsg =N_ ("Resolve all conflicts manually, mark them as resolved with\n" "\"git add/rm <conflicted_files>\", then run \"git rebase --continue\".\n"
@@ -466,6 +520,126 @@ N_("Resolve all conflicts manually, mark them as resolved with\n""To abort and get back to the state before \"git rebase\", run " "\"git rebase --abort\"."); +static int run_am (struct rebase_options *opts) +{ +struct child_process am =CHILD_PROCESS_INIT ; +struct child_process format_patch =CHILD_PROCESS_INIT ; +struct strbuf revisions =STRBUF_INIT ; +int status; +char *rebased_patches; + + am.git_cmd =1 ; +argv_array_push (&am.args,"am" ); + +if (opts->action && !strcmp ("continue" , opts->action)) { +argv_array_push (&am.args,"--resolved" ); +argv_array_pushf (&am.args,"--resolvemsg=%s" , resolvemsg); +if (opts->gpg_sign_opt) +argv_array_push (&am.args, opts->gpg_sign_opt); + status =run_command (&am); +if (status) +return status; + +return move_to_original_branch (opts); + } +if (opts->action && !strcmp ("skip" , opts->action)) { +argv_array_push (&am.args,"--skip" ); +argv_array_pushf (&am.args,"--resolvemsg=%s" , resolvemsg); + status =run_command (&am); +if (status) +return status; + +return move_to_original_branch (opts); + } +if (opts->action && !strcmp ("show-current-patch" , opts->action)) { +argv_array_push (&am.args,"--show-current-patch" ); +return run_command (&am); + } + +strbuf_addf (&revisions,"%s...%s" , +oid_to_hex (opts->root ? +/* this is now equivalent to !opts->upstream */ + &opts->onto->object.oid : + &opts->upstream->object.oid), +oid_to_hex (&opts->orig_head)); + + rebased_patches =xstrdup (git_path ("rebased-patches" )); + format_patch.out =open (rebased_patches, +O_WRONLY |O_CREAT |O_TRUNC ,0666 ); +if (format_patch.out <0 ) { + status =error_errno (_ ("could not open '%s' for writing" ), + rebased_patches); +free (rebased_patches); +argv_array_clear (&am.args); +return status; + } + + format_patch.git_cmd =1 ; +argv_array_pushl (&format_patch.args,"format-patch" ,"-k" ,"--stdout" , +"--full-index" ,"--cherry-pick" ,"--right-only" , +"--src-prefix=a/" ,"--dst-prefix=b/" ,"--no-renames" , +"--no-cover-letter" ,"--pretty=mboxrd" ,NULL ); +if (opts->git_format_patch_opt.len) +argv_array_split (&format_patch.args, + opts->git_format_patch_opt.buf); +argv_array_push (&format_patch.args, revisions.buf); +if (opts->restrict_revision) +argv_array_pushf (&format_patch.args,"^%s" , +oid_to_hex (&opts->restrict_revision->object.oid)); + + status =run_command (&format_patch); +if (status) { +unlink (rebased_patches); +free (rebased_patches); +argv_array_clear (&am.args); + +reset_head (&opts->orig_head,"checkout" , opts->head_name,0 , +"HEAD" ,NULL ); +error (_ ("\ngit encountered an error while preparing the " +"patches to replay\n" +"these revisions:\n" +"\n %s\n\n" +"As a result, git cannot rebase them." ), + opts->revisions); + +strbuf_release (&revisions); +return status; + } +strbuf_release (&revisions); + + am.in =open (rebased_patches,O_RDONLY ); +if (am.in <0 ) { + status =error_errno (_ ("could not open '%s' for reading" ), + rebased_patches); +free (rebased_patches); +argv_array_clear (&am.args); +return status; + } + +argv_array_pushv (&am.args, opts->git_am_opts.argv); +argv_array_push (&am.args,"--rebasing" ); +argv_array_pushf (&am.args,"--resolvemsg=%s" , resolvemsg); +argv_array_push (&am.args,"--patch-format=mboxrd" ); +if (opts->allow_rerere_autoupdate >0 ) +argv_array_push (&am.args,"--rerere-autoupdate" ); +else if (opts->allow_rerere_autoupdate ==0 ) +argv_array_push (&am.args,"--no-rerere-autoupdate" ); +if (opts->gpg_sign_opt) +argv_array_push (&am.args, opts->gpg_sign_opt); + status =run_command (&am); +unlink (rebased_patches); +free (rebased_patches); + +if (!status) { +return move_to_original_branch (opts); + } + +if (is_directory (opts->state_dir)) +write_basic_state (opts); + +return status; +} +static int run_specific_rebase (struct rebase_options *opts) {const char *argv[] = {NULL ,NULL };
@@ -546,6 +720,11 @@ static int run_specific_rebase(struct rebase_options *opts) goto finished_rebase; } +if (opts->type ==REBASE_AM ) { + status =run_am (opts); + goto finished_rebase; + } +add_var (&script_snippet,"GIT_DIR" ,absolute_path (get_git_dir ()));add_var (&script_snippet,"state_dir" , opts->state_dir);
Signature · 1 line
gitgitgadget
Re: [PATCH 4/4] built-in rebase: call `git am` directly
In reply to Johannes SchindelinQuoted 10 lines · Johannes Schindelin
Quoted 9 lines
Overall, this was quite a pleasant read and a well constructed series. Other than two minor points (i.e. interaction with the 'post-checkout hook' topic, and
discard_cache()before callingmove_to_original_branch) I did not quite understand, looks good to me.When merged to 'pu', I seem to be getting failure from t3425.5, .8 and .11, by the way. I haven't dug into the actual breakages any further than that.
Quoted 5 lines
Sorry for the trouble, and for my silence (I was heads-down into the Azure Pipelines support).
I did not see any breakage in
pulately, hopefully things resolved themselves?
The (semantic) conflict resolution can be seen in
$ git show 'origin/pu^{/^Merge branch .js/rebase-am. into}'which is recorded in my rerere database ;-)
"Johannes Schindelin via GitGitGadget" <[email protected]>
Quoted 9 lines · writes:
Especially on Windows, where Unix shell scripting is a foreign endeavor, and an expensive one at that, we really want to avoid running through the Bash.
This not only makes everything faster, but also more robust, as the Bash we use on Windows relies on a derivative of the Cygwin runtime, which in turn has to jump through a couple of hoops that are sometimes a little too tricky to make things work. Read: the less we rely on Unix shell scripting, the more likely Windows users will be able to enjoy our software.
Quoted 3 lines
Changes since v1:
* Rebased on top of master to avoid merge conflicts.
I do not appreciate this very much.
We already have a good resolution prepared when merging this to either 'next' or 'master', which also resolves conflict with the other topic that requires this topic to add "--topo-order" in its call. Rebasing series means invalidating the previous work recorded in rerere.
side note. The rerere database entry can be recovered from master..pu with contrib/rerere-train.sh).
Quoted 2 lines
* Adjusted the commit message talking about double entries, to clarify that it talks about HEAD's reflog.
Good.
Quoted 4 lines
* Replaced a misleading action parameter "checkout" for the
reset_head()function by the empty string: we do not check out here, we just update the refs, and certainly do not want any checkout functionality (such as hooks) to be involved.
OK.
Quoted 2 lines
* Reused a just-prepared
refs_onlyvariable instead of repeating the value assigned to it.
OK.
Quoted 3 lines
* Fixed a stale comment about the shell variable "$upstream" (which should have been negated to begin with).
* Fixed error messages when files could not be opened.
Good.
Will take a look. Thanks.
Re: [PATCH 4/4] built-in rebase: call `git am` directly
In reply to Junio C HamanoHi Junio,
Quoted 15 lines · Junio C Hamano
Quoted 13 lines · Johannes Schindelin
Quoted 9 lines
Overall, this was quite a pleasant read and a well constructed series. Other than two minor points (i.e. interaction with the 'post-checkout hook' topic, and
discard_cache()before callingmove_to_original_branch) I did not quite understand, looks good to me.When merged to 'pu', I seem to be getting failure from t3425.5, .8 and .11, by the way. I haven't dug into the actual breakages any further than that.
Sorry for the trouble, and for my silence (I was heads-down into the Azure Pipelines support).
Quoted 8 lines
Quoted 2 lines
I did not see any breakage in
pulately, hopefully things resolved themselves?The (semantic) conflict resolution can be seen in
$ git show 'origin/pu^{/^Merge branch .js/rebase-am. into}'which is recorded in my rerere database ;-)
That looks all good to me, with the minor nit that you did not wrap at <= 80 columns/row:
- "--no-cover-letter", "--pretty=mboxrd", NULL);
++ "--no-cover-letter", "--pretty=mboxrd", "--topo-order", NULL);
I am not a stickler to this line length rule, though, as I long switched away from the 80x25 screen and can easily resize my text windows.
Ciao,
Dscho
Quoted 18 lines · Junio C Hamano
"Johannes Schindelin via GitGitGadget" <[email protected]>
Quoted 9 lines · writes:
Especially on Windows, where Unix shell scripting is a foreign endeavor, and an expensive one at that, we really want to avoid running through the Bash.
This not only makes everything faster, but also more robust, as the Bash we use on Windows relies on a derivative of the Cygwin runtime, which in turn has to jump through a couple of hoops that are sometimes a little too tricky to make things work. Read: the less we rely on Unix shell scripting, the more likely Windows users will be able to enjoy our software.
Quoted 3 lines
Changes since v1:
* Rebased on top of master to avoid merge conflicts.
I do not appreciate this very much.
Quoted 8 lines
We already have a good resolution prepared when merging this to either 'next' or 'master', which also resolves conflict with the other topic that requires this topic to add "--topo-order" in its call. Rebasing series means invalidating the previous work recorded in rerere.
side note. The rerere database entry can be recovered from master..pu with
contrib/rerere-train.sh).
I prepared a set of patches rebased back on top of the previous base and applied them on top of the previous base (call the result X).
Applying the posted patches directly on top of master, and merging X into master, produces the same tree.
So I'll keep the X (i.e. these patches backward-rebased to the previous base) and replace js/rebase-am with it. That is safer when recreating 'pu' and merging it to 'next', which I hope we can do soonish.
Quoted 1 line
Will take a look. Thanks.
They were pleasant and smooth read. Crafted nicely.
Thanks.
Hi Junio,
Quoted 16 lines · Junio C Hamano
"Johannes Schindelin via GitGitGadget" <[email protected]>
Quoted 9 lines · writes:
Especially on Windows, where Unix shell scripting is a foreign endeavor, and an expensive one at that, we really want to avoid running through the Bash.
This not only makes everything faster, but also more robust, as the Bash we use on Windows relies on a derivative of the Cygwin runtime, which in turn has to jump through a couple of hoops that are sometimes a little too tricky to make things work. Read: the less we rely on Unix shell scripting, the more likely Windows users will be able to enjoy our software.
Quoted 3 lines
Changes since v1:
* Rebased on top of master to avoid merge conflicts.
Quoted 7 lines
I do not appreciate this very much.
We already have a good resolution prepared when merging this to either 'next' or 'master', which also resolves conflict with the other topic that requires this topic to add "--topo-order" in its call. Rebasing series means invalidating the previous work recorded in rerere.
You misunderstand. The PR had merge conflicts with master. As such, the Azure Pipeline that tests it on Windows, macOS and Linux could not give me enough confidence. So I *had* to rebase.
Besides, I think it is a terrible idea to leave the resolution of merge conflicts to you. You are not in the best position to judge how to resolve them, and as a consequence you spend more time and effort on this than necessary. Of course, it's your call if you want to do it, as a contributor I'd rather be certain that *I* resolved the conflicts.
Ciao,
Dscho
Quoted 29 lines
side note. The rerere database entry can be recovered from master..pu with
contrib/rerere-train.sh).Quoted 2 lines
* Adjusted the commit message talking about double entries, to clarify that it talks about HEAD's reflog.
Good.
Quoted 4 lines
* Replaced a misleading action parameter "checkout" for the
reset_head()function by the empty string: we do not check out here, we just update the refs, and certainly do not want any checkout functionality (such as hooks) to be involved.OK.
Quoted 2 lines
* Reused a just-prepared
refs_onlyvariable instead of repeating the value assigned to it.OK.
Quoted 3 lines
* Fixed a stale comment about the shell variable "$upstream" (which should have been negated to begin with).
* Fixed error messages when files could not be opened.Good.
Will take a look. Thanks.
Re: [PATCH v2 4/4] built-in rebase: call `git am` directly
In reply to Johannes Schindelin via GitGitGadgetHi Johannes,
Le 18/01/2019 à 16:09, Johannes Schindelin via GitGitGadget a écrit :
Quoted 5 lines
-%<-
builtin/rebase.c +3
-1+static int run_am (struct rebase_options *opts) +{ -%<- +
builtin/rebase.c N_("Resolve all conflicts manually, mark them as resolved with\n" · line 642
Quoted 10 lines
builtin/rebase.c +10
-0+if (!status) { +return move_to_original_branch (opts); + } + +if (is_directory (opts->state_dir)) +write_basic_state (opts); + +return status; +} +
This means that the state directory will be created only if there is a problem (ie. a conflict), not if the user cancels the rebase by hitting ^C. In this case, the user will be left in a corrupted state where the builtin rebase cannot abort the rebase properly.
-- Alban
Re: [PATCH v2 4/4] built-in rebase: call `git am` directly
Hi Alban,
Quoted 13 lines · Alban Gruin
Le 18/01/2019 à 16:09, Johannes Schindelin via GitGitGadget a écrit :
Quoted 12 lines
-%<-
builtin/rebase.c +10
-1+static int run_am (struct rebase_options *opts) +{ -%<- + +if (!status) { +return move_to_original_branch (opts); + } + +if (is_directory (opts->state_dir)) +write_basic_state (opts); +
Quoted 8 lines
builtin/rebase.c N_("Resolve all conflicts manually, mark them as resolved with\n" · line 551
Quoted 3 lines
builtin/rebase.c +3
-0+return status; +} +This means that the state directory will be created only if there is a problem (ie. a conflict), not if the user cancels the rebase by hitting ^C. In this case, the user will be left in a corrupted state where the builtin rebase cannot abort the rebase properly.
This is an almost literal translation of
https://github.com/git/git/blob/v2.20.1/git-rebase--am.sh#L77-L83:
```sh
if test 0 != $ret
then
test -d "$state_dir" && write_basic_state
return $ret
fi
move_to_original_branch
```All I did was to switch the order to handle the simple (and common) case first.
So I do not think that this changes the behavior of the shell-scripted am backend.
Looking at the code, for a moment I thought that I had introduced a bug where the state_dir is not cleaned up after move_to_original_branch() is called, but I call run_am() in run_specific_rebase() and skip directly to the finished_rebase label after that, which does take care of calling finish_rebase() (which removes the state_dir, among other things such as applying the autostash).
Ciao,
Dscho