git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Pratik Karki <predatoramigo@gmail.com>
Cc: git@vger.kernel.org, christian.couder@gmail.com,
	Johannes.Schindelin@gmx.de, sbeller@google.com,
	alban.gruin@gmail.com
Subject: Re: [PATCH v4 3/4] sequencer: refactor the code to detach HEAD to checkout.c
Date: Mon, 09 Jul 2018 09:42:35 -0700	[thread overview]
Message-ID: <xmqq8t6kfjic.fsf@gitster-ct.c.googlers.com> (raw)
In-Reply-To: <20180708180104.17921-4-predatoramigo@gmail.com> (Pratik Karki's message of "Sun, 8 Jul 2018 23:46:03 +0545")

Pratik Karki <predatoramigo@gmail.com> writes:

> In the upcoming builtin rebase, we will have to start by detaching
> the HEAD, just like shell script version does. Essentially, we have
> to do the same thing as `git checkout -q <revision>^0 --`, in pure C.
>
> The aforementioned functionality was already present in `sequencer.c`
> in `do_reset()` function. But `do_reset()` performs more than detaching
> the HEAD, and performs action specific to `sequencer.c`.
>
> So this commit refactors out that part from `do_reset()`, and moves it
> to a new function called `detach_head_to()`. As this function has
> nothing to do with the sequencer, and everything to do with what `git
> checkout -q <revision>^0 --` does, we move that function to checkout.c.
>
> This refactoring actually introduces a slight change in behavior:
> previously, the index was locked before parsing the argument to the
> todo command `reset`, while it now gets locked *after* that, in the
> `detach_head_to()` function.
>
> It does not make a huge difference, and the upside is that this closes
> a few (unlikely) code paths where the index would not be unlocked upon
> error.
>
> Signed-off-by: Pratik Karki <predatoramigo@gmail.com>
> ---

Here is a place to say "unchanged since v3" for a change like this
one that changes neither the proposed log message above nor the
patch below to help reviewers who have seen the previous round (they
can stop reading here).  Hopefully there are more reviewers than you
who write new code, so spending a bit more time to help them spend
less would be an overall win for the project.

Thanks.

>  checkout.c  | 64 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  checkout.h  |  3 +++
>  sequencer.c | 58 +++++-------------------------------------------
>  3 files changed, 72 insertions(+), 53 deletions(-)
>
> diff --git a/checkout.c b/checkout.c
> index bdefc888ba..da68915fd7 100644
> --- a/checkout.c
> +++ b/checkout.c
> @@ -2,6 +2,11 @@
>  #include "remote.h"
>  #include "refspec.h"
>  #include "checkout.h"
> +#include "unpack-trees.h"
> +#include "lockfile.h"
> +#include "refs.h"
> +#include "tree.h"
> +#include "cache-tree.h"
>  
>  struct tracking_name_data {
>  	/* const */ char *src_ref;
> @@ -42,3 +47,62 @@ const char *unique_tracking_name(const char *name, struct object_id *oid)
>  	free(cb_data.dst_ref);
>  	return NULL;
>  }
> +
> +int detach_head_to(struct object_id *oid, const char *action,
> +		   const char *reflog_message)
> +{
> +	struct strbuf ref_name = STRBUF_INIT;
> +	struct tree_desc desc;
> +	struct lock_file lock = LOCK_INIT;
> +	struct unpack_trees_options unpack_tree_opts;
> +	struct tree *tree;
> +	int ret = 0;
> +
> +	if (hold_locked_index(&lock, LOCK_REPORT_ON_ERROR) < 0)
> +		return -1;
> +
> +	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_index;
> +	unpack_tree_opts.dst_index = &the_index;
> +	unpack_tree_opts.fn = oneway_merge;
> +	unpack_tree_opts.merge = 1;
> +	unpack_tree_opts.update = 1;
> +
> +	if (read_cache_unmerged()) {
> +		rollback_lock_file(&lock);
> +		strbuf_release(&ref_name);
> +		return error_resolve_conflict(_(action));
> +	}
> +
> +	if (!fill_tree_descriptor(&desc, oid)) {
> +		error(_("failed to find tree of %s"), oid_to_hex(oid));
> +		rollback_lock_file(&lock);
> +		free((void *)desc.buffer);
> +		strbuf_release(&ref_name);
> +		return -1;
> +	}
> +
> +	if (unpack_trees(1, &desc, &unpack_tree_opts)) {
> +		rollback_lock_file(&lock);
> +		free((void *)desc.buffer);
> +		strbuf_release(&ref_name);
> +		return -1;
> +	}
> +
> +	tree = parse_tree_indirect(oid);
> +	prime_cache_tree(&the_index, tree);
> +
> +	if (write_locked_index(&the_index, &lock, COMMIT_LOCK) < 0)
> +		ret = error(_("could not write index"));
> +	free((void *)desc.buffer);
> +
> +	if (!ret)
> +		ret = update_ref(reflog_message, "HEAD", oid,
> +				 NULL, 0, UPDATE_REFS_MSG_ON_ERR);
> +
> +	strbuf_release(&ref_name);
> +	return ret;
> +
> +}
> diff --git a/checkout.h b/checkout.h
> index 9980711179..6ce46cf4e8 100644
> --- a/checkout.h
> +++ b/checkout.h
> @@ -10,4 +10,7 @@
>   */
>  extern const char *unique_tracking_name(const char *name, struct object_id *oid);
>  
> +int detach_head_to(struct object_id *oid, const char *action,
> +		   const char *reflog_message);
> +
>  #endif /* CHECKOUT_H */
> diff --git a/sequencer.c b/sequencer.c
> index 5354d4d51e..106d518f4d 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -29,6 +29,7 @@
>  #include "oidset.h"
>  #include "commit-slab.h"
>  #include "alias.h"
> +#include "checkout.h"
>  
>  #define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION"
>  
> @@ -2756,14 +2757,7 @@ static int do_reset(const char *name, int len, struct replay_opts *opts)
>  {
>  	struct strbuf ref_name = STRBUF_INIT;
>  	struct object_id oid;
> -	struct lock_file lock = LOCK_INIT;
> -	struct tree_desc desc;
> -	struct tree *tree;
> -	struct unpack_trees_options unpack_tree_opts;
> -	int ret = 0, i;
> -
> -	if (hold_locked_index(&lock, LOCK_REPORT_ON_ERROR) < 0)
> -		return -1;
> +	int i;
>  
>  	if (len == 10 && !strncmp("[new root]", name, len)) {
>  		if (!opts->have_squash_onto) {
> @@ -2789,56 +2783,14 @@ static int do_reset(const char *name, int len, struct replay_opts *opts)
>  		if (get_oid(ref_name.buf, &oid) &&
>  		    get_oid(ref_name.buf + strlen("refs/rewritten/"), &oid)) {
>  			error(_("could not read '%s'"), ref_name.buf);
> -			rollback_lock_file(&lock);
>  			strbuf_release(&ref_name);
>  			return -1;
>  		}
>  	}
>  
> -	memset(&unpack_tree_opts, 0, sizeof(unpack_tree_opts));
> -	setup_unpack_trees_porcelain(&unpack_tree_opts, "reset");
> -	unpack_tree_opts.head_idx = 1;
> -	unpack_tree_opts.src_index = &the_index;
> -	unpack_tree_opts.dst_index = &the_index;
> -	unpack_tree_opts.fn = oneway_merge;
> -	unpack_tree_opts.merge = 1;
> -	unpack_tree_opts.update = 1;
> -
> -	if (read_cache_unmerged()) {
> -		rollback_lock_file(&lock);
> -		strbuf_release(&ref_name);
> -		return error_resolve_conflict(_(action_name(opts)));
> -	}
> -
> -	if (!fill_tree_descriptor(&desc, &oid)) {
> -		error(_("failed to find tree of %s"), oid_to_hex(&oid));
> -		rollback_lock_file(&lock);
> -		free((void *)desc.buffer);
> -		strbuf_release(&ref_name);
> -		return -1;
> -	}
> -
> -	if (unpack_trees(1, &desc, &unpack_tree_opts)) {
> -		rollback_lock_file(&lock);
> -		free((void *)desc.buffer);
> -		strbuf_release(&ref_name);
> -		return -1;
> -	}
> -
> -	tree = parse_tree_indirect(&oid);
> -	prime_cache_tree(&the_index, tree);
> -
> -	if (write_locked_index(&the_index, &lock, COMMIT_LOCK) < 0)
> -		ret = error(_("could not write index"));
> -	free((void *)desc.buffer);
> -
> -	if (!ret)
> -		ret = update_ref(reflog_message(opts, "reset", "'%.*s'",
> -						len, name), "HEAD", &oid,
> -				 NULL, 0, UPDATE_REFS_MSG_ON_ERR);
> -
> -	strbuf_release(&ref_name);
> -	return ret;
> +	return detach_head_to(&oid, action_name(opts),
> +			      reflog_message(opts, "reset", "'%.*s'",
> +					     len, name));
>  }
>  
>  static int do_merge(struct commit *commit, const char *arg, int arg_len,

  parent reply	other threads:[~2018-07-09 16:42 UTC|newest]

Thread overview: 61+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-06-28  7:46 [GSoC] [PATCH 0/5] rebase: rewrite rebase in C Pratik Karki
2018-06-28  7:46 ` [PATCH 1/5] Start TODO-rebase.sh Pratik Karki
2018-06-28  9:17   ` Pratik Karki
2018-06-28  7:46 ` [PATCH 2/5] rebase: start implementing it as a builtin Pratik Karki
2018-06-28  8:08   ` Christian Couder
2018-06-28 18:49   ` Stefan Beller
2018-06-28  7:46 ` [PATCH 3/5] rebase: refactor common shell functions into their own file Pratik Karki
2018-06-28  8:02   ` Christian Couder
2018-06-28 19:17   ` Stefan Beller
2018-06-28  7:46 ` [PATCH 4/5] sequencer: refactor the code to detach HEAD to checkout.c Pratik Karki
2018-06-28  8:14   ` Christian Couder
2018-06-28 21:19   ` Stefan Beller
2018-06-28  7:46 ` [PATCH 5/5] builtin/rebase: support running "git rebase <upstream>" Pratik Karki
2018-06-28 21:58   ` Stefan Beller
2018-07-02  9:15 ` [GSoC] [PATCH v2 0/4] rebase: rewrite rebase in C Pratik Karki
2018-07-02  9:15   ` [PATCH v2 1/4] rebase: start implementing it as a builtin Pratik Karki
2018-07-03 21:09     ` Junio C Hamano
2018-07-02  9:15   ` [PATCH v2 2/4] rebase: refactor common shell functions into their own file Pratik Karki
2018-07-03 21:13     ` Junio C Hamano
2018-07-02  9:15   ` [PATCH v2 3/4] sequencer: refactor the code to detach HEAD to checkout.c Pratik Karki
2018-07-03 21:26     ` Junio C Hamano
2018-07-02  9:15   ` [PATCH v2 4/4] builtin/rebase: support running "git rebase <upstream>" Pratik Karki
2018-07-03 21:42     ` Junio C Hamano
2018-07-06 12:08 ` [GSoC] [PATCH v3 0/4] rebase: rewrite rebase in C Pratik Karki
2018-07-06 12:08   ` [PATCH v3 1/4] rebase: start implementing it as a builtin Pratik Karki
2018-07-06 21:09     ` Junio C Hamano
2018-07-06 12:08   ` [PATCH v3 2/4] rebase: refactor common shell functions into their own file Pratik Karki
2018-07-06 12:36     ` Johannes Schindelin
2018-07-06 12:08   ` [PATCH v3 3/4] sequencer: refactor the code to detach HEAD to checkout.c Pratik Karki
2018-07-06 12:08   ` [PATCH v3 4/4] builtin/rebase: support running "git rebase <upstream>" Pratik Karki
2018-07-06 21:30     ` Junio C Hamano
2018-07-07  6:45     ` Christian Couder
2018-07-07 11:59       ` Johannes Schindelin
2018-07-07 16:24       ` Junio C Hamano
2018-07-17 21:49     ` Beat Bolli
2018-07-17 21:55       ` Beat Bolli
2018-07-08 18:01 ` [GSoC] [PATCH v4 0/4] rebase: rewrite rebase in C Pratik Karki
2018-07-08 18:01   ` [PATCH v4 1/4] rebase: start implementing it as a builtin Pratik Karki
2018-07-09  7:59     ` Andrei Rybak
2018-07-09  8:36       ` Eric Sunshine
2018-07-09 17:18         ` Pratik Karki
2018-07-22  9:14     ` Duy Nguyen
2018-07-08 18:01   ` [PATCH v4 2/4] rebase: refactor common shell functions into their own file Pratik Karki
2018-07-08 18:01   ` [PATCH v4 3/4] sequencer: refactor the code to detach HEAD to checkout.c Pratik Karki
2018-07-08 21:31     ` Johannes Schindelin
2018-07-09 17:23       ` Pratik Karki
2018-07-09 16:42     ` Junio C Hamano [this message]
2018-07-09 17:20       ` Pratik Karki
2018-07-08 18:01   ` [PATCH v4 4/4] builtin/rebase: support running "git rebase <upstream>" Pratik Karki
2018-07-08 21:44     ` Johannes Schindelin
2018-07-08 21:14   ` [GSoC] [PATCH v4 0/4] rebase: rewrite rebase in C Johannes Schindelin
2018-07-30 16:29   ` [GSoC] [PATCH v5 0/3] " Pratik Karki
2018-07-30 16:29     ` [PATCH v5 1/3] rebase: start implementing it as a builtin Pratik Karki
2018-07-30 16:29     ` [PATCH v5 2/3] rebase: refactor common shell functions into their own file Pratik Karki
2018-07-30 16:29     ` [PATCH v5 3/3] builtin/rebase: support running "git rebase <upstream>" Pratik Karki
2018-08-01  0:17     ` [GSoC] [PATCH v5 0/3] rebase: rewrite rebase in C Pratik Karki
2018-08-06 19:31     ` [GSoC] [PATCH v6 " Pratik Karki
2018-08-06 19:31       ` [GSoC] [PATCH v6 1/3] rebase: start implementing it as a builtin Pratik Karki
2018-08-06 19:31       ` [GSoC] [PATCH v6 2/3] rebase: refactor common shell functions into their own file Pratik Karki
2018-08-06 19:31       ` [GSoC] [PATCH v6 3/3] builtin/rebase: support running "git rebase <upstream>" Pratik Karki
2018-08-16 21:43         ` Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqq8t6kfjic.fsf@gitster-ct.c.googlers.com \
    --to=gitster@pobox.com \
    --cc=Johannes.Schindelin@gmx.de \
    --cc=alban.gruin@gmail.com \
    --cc=christian.couder@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=predatoramigo@gmail.com \
    --cc=sbeller@google.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).