Re: [PATCH v4 3/4] sequencer: refactor the code to detach HEAD to checkout.c
Hi,
On Mon, Jul 9, 2018 at 3:16 AM Johannes Schindelin
wrote:
>
> Hi Pratik,
>
> On Sun, 8 Jul 2018, Pratik Karki wrote:
>
> > 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_REPORT_ON_ERROR) < 0)
> > + return -1;
> > +
> > + memset(_tree_opts, 0, sizeof(unpack_tree_opts));
> > + setup_unpack_trees_porcelain(_tree_opts, action);
> > + unpack_tree_opts.head_idx = 1;
> > + unpack_tree_opts.src_index = _index;
> > + unpack_tree_opts.dst_index = _index;
> > + unpack_tree_opts.fn = oneway_merge;
> > + unpack_tree_opts.merge = 1;
> > + unpack_tree_opts.update = 1;
> > +
> > + if (read_cache_unmerged()) {
> > + rollback_lock_file();
> > + strbuf_release(_name);
> > + return error_resolve_conflict(_(action));
> > + }
> > +
> > + if (!fill_tree_descriptor(, oid)) {
> > + error(_("failed to find tree of %s"), oid_to_hex(oid));
> > + rollback_lock_file();
> > + free((void *)desc.buffer);
> > + strbuf_release(_name);
> > + return -1;
> > + }
> > +
> > + if (unpack_trees(1, , _tree_opts)) {
> > + rollback_lock_file();
> > + free((void *)desc.buffer);
> > + strbuf_release(_name);
> > + return -1;
> > + }
> > +
> > + tree = parse_tree_indirect(oid);
> > + prime_cache_tree(_index, tree);
> > +
> > + if (write_locked_index(_index, , 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);
>
> I noticed that this does not actually detach the HEAD. That is my fault,
> of course, as I should have not only suggested refactoring the
> `do_reset()` function from `sequencer.c`, but I should also have
> remembered that that function has the benefit of *always* acting on a
> detached HEAD (because it runs during an interactive rebase), and
> therefore does not need to detach it explicitly.
>
> In light of the `reset_hard()` function that you added in a `wip` (see
> https://github.com/git/git/pull/505/files#diff-c7361e406139e8cd3a300b80b8f8cc8dR296),
> I could imagine that it might be better, after all, to leave `do_reset()`
> alone and implement a `reset_hard()` function that also optionally
> detaches the `HEAD` (I *think* that the flag `REF_NO_DEREF` would do that
> for you).
Yes. I think this will be better. Thanks.
> Alternatively, just update the code in `do_reset()` to use that flag
> first, and only *then* extract the code to `checkout.c`.
>
> (I could not resist, and made this quick change on top of your
> `wip-rebase`, and together with a couple more, obvious fixups, this lets
> t3403 pass. It still needs some things that you have not yet sent to the
> mailing list, such as support for `--skip`.)
Thank you for taking the time to review.
Re: [PATCH v4 3/4] sequencer: refactor the code to detach HEAD to checkout.c
On Mon, Jul 9, 2018 at 10:27 PM Junio C Hamano wrote: > > Pratik Karki 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 ^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 ^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 > > --- > > 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 for the review. I'll keep this in mind, the next time.
Re: [PATCH v4 3/4] sequencer: refactor the code to detach HEAD to checkout.c
Pratik Karki 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 ^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 ^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
> ---
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_REPORT_ON_ERROR) < 0)
> + return -1;
> +
> + memset(_tree_opts, 0, sizeof(unpack_tree_opts));
> + setup_unpack_trees_porcelain(_tree_opts, action);
> + unpack_tree_opts.head_idx = 1;
> + unpack_tree_opts.src_index = _index;
> + unpack_tree_opts.dst_index = _index;
> + unpack_tree_opts.fn = oneway_merge;
> + unpack_tree_opts.merge = 1;
> + unpack_tree_opts.update = 1;
> +
> + if (read_cache_unmerged()) {
> + rollback_lock_file();
> + strbuf_release(_name);
> + return error_resolve_conflict(_(action));
> + }
> +
> + if (!fill_tree_descriptor(, oid)) {
> + error(_("failed to find tree of %s"), oid_to_hex(oid));
> + rollback_lock_file();
> + free((void *)desc.buffer);
> + strbuf_release(_name);
> + return -1;
> + }
> +
> + if (unpack_trees(1, , _tree_opts)) {
> + rollback_lock_file();
> + free((void *)desc.buffer);
> + strbuf_release(_name);
> + return -1;
> + }
> +
> + tree = parse_tree_indirect(oid);
> + prime_cache_tree(_index, tree);
> +
> + if (write_locked_index(_index, , 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(_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
>
Re: [PATCH v4 3/4] sequencer: refactor the code to detach HEAD to checkout.c
Hi Pratik,
On Sun, 8 Jul 2018, Pratik Karki wrote:
> 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_REPORT_ON_ERROR) < 0)
> + return -1;
> +
> + memset(_tree_opts, 0, sizeof(unpack_tree_opts));
> + setup_unpack_trees_porcelain(_tree_opts, action);
> + unpack_tree_opts.head_idx = 1;
> + unpack_tree_opts.src_index = _index;
> + unpack_tree_opts.dst_index = _index;
> + unpack_tree_opts.fn = oneway_merge;
> + unpack_tree_opts.merge = 1;
> + unpack_tree_opts.update = 1;
> +
> + if (read_cache_unmerged()) {
> + rollback_lock_file();
> + strbuf_release(_name);
> + return error_resolve_conflict(_(action));
> + }
> +
> + if (!fill_tree_descriptor(, oid)) {
> + error(_("failed to find tree of %s"), oid_to_hex(oid));
> + rollback_lock_file();
> + free((void *)desc.buffer);
> + strbuf_release(_name);
> + return -1;
> + }
> +
> + if (unpack_trees(1, , _tree_opts)) {
> + rollback_lock_file();
> + free((void *)desc.buffer);
> + strbuf_release(_name);
> + return -1;
> + }
> +
> + tree = parse_tree_indirect(oid);
> + prime_cache_tree(_index, tree);
> +
> + if (write_locked_index(_index, , 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);
I noticed that this does not actually detach the HEAD. That is my fault,
of course, as I should have not only suggested refactoring the
`do_reset()` function from `sequencer.c`, but I should also have
remembered that that function has the benefit of *always* acting on a
detached HEAD (because it runs during an interactive rebase), and
therefore does not need to detach it explicitly.
In light of the `reset_hard()` function that you added in a `wip` (see
https://github.com/git/git/pull/505/files#diff-c7361e406139e8cd3a300b80b8f8cc8dR296),
I could imagine that it might be better, after all, to leave `do_reset()`
alone and implement a `reset_hard()` function that also optionally
detaches the `HEAD` (I *think* that the flag `REF_NO_DEREF` would do that
for you).
Alternatively, just update the code in `do_reset()` to use that flag
first, and only *then* extract the code to `checkout.c`.
(I could not resist, and made this quick change on top of your
`wip-rebase`, and together with a couple more, obvious fixups, this lets
t3403 pass. It still needs some things that you have not yet sent to the
mailing list, such as support for `--skip`.)
Ciao,
Dscho
