Re: [PATCH 3/4] rebase: teach `reset_head()` to optionally skip the worktree

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 4, 2019, 18:38 UTC
Message-ID: <xmqqpntcclot.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <21939140e00d96cf6f76e4c994638fecd3a95639.1545398254.git.gitgitgadget@gmail.com>

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 5 quoted lines

> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> 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" ;-)

Show 14 quoted lines

> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  builtin/rebase.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index 768bea0da8..303175fdf1 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -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.

Show 19 quoted lines

>  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;
>  	}

Not touching the index, so no need to lock the index.  Makes sense.

Show 7 quoted lines

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

Show 9 quoted lines

>  	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);

And the control continues from the point we update the reflog. Makes sense.

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