Re: [PATCH v2 3/4] sequencer: refactor the code to detach HEAD to checkout.c

2018-07-03 Thread Junio C Hamano
Pratik Karki  writes:

> The motivation behind this commit is to extract the core part of
> do_reset() from sequencer.c and move it to a new detach_head_to()
> function in checkout.c.
>
> Here the index only gets locked after performing the first part of
> `do_reset()` rather than before which essentially derives the `oid`
> from the specified label/name passed to the `do_reset()` function.
> It also fixes two bugs: there were two `return error()` statements in
> the `[new root]` case that would have failed to unlock the index.
>
> The new function will be used in the next commit by the builtin rebase,
> to perform the initial checkout.

I like the split of do_reset() into this "detach-to" part that is
less specific to replaying an existing commit and the rest, which
as you said incidentally corrects the locking issue.

It is not clear to me why checkout.[ch] is a better place to house
this function, though.


[PATCH v2 3/4] sequencer: refactor the code to detach HEAD to checkout.c

2018-07-02 Thread Pratik Karki
The motivation behind this commit is to extract the core part of
do_reset() from sequencer.c and move it to a new detach_head_to()
function in checkout.c.

Here the index only gets locked after performing the first part of
`do_reset()` rather than before which essentially derives the `oid`
from the specified label/name passed to the `do_reset()` function.
It also fixes two bugs: there were two `return error()` statements in
the `[new root]` case that would have failed to unlock the index.

The new function will be used in the next commit by the builtin rebase,
to perform the initial checkout.

Signed-off-by: Pratik Karki 
---
 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 bdefc888b..da68915fd 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 998071117..6ce46cf4e 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 5354d4d51..106d518f4 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_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, ) &&
get_oid(ref_name.buf + strlen("refs/rewritten/"), )) {
error(_("could not read '%s'"), ref_name.buf);
-   rollback_lock_file();
strbuf_release(_name);
return -1;
}
}
 
-   memset(_tree_opts, 0, sizeof(unpack_tree_opts));
-