Re: [PATCH v2 02/10] sequencer: introduce new commands to reset the revision

> On Mon, Jan 29, 2018 at 2:54 PM, Johannes Schindelin
> <johannes.schindelin@gmx.de> wrote:
> > @@ -116,6 +118,13 @@ static GIT_PATH_FUNC(rebase_path_stopped_sha, "rebase-merge/stopped-sha")
> >  static GIT_PATH_FUNC(rebase_path_rewritten_list, "rebase-merge/rewritten-list")
> >  static GIT_PATH_FUNC(rebase_path_rewritten_pending,
> >         "rebase-merge/rewritten-pending")
> > +
> > +/*
> > + * The path of the file listing refs that need to be deleted after the rebase
> > + * finishes. This is used by the `merge` command.
> > + */

> which is appended in do_label, it uses refs to store the commits in
> refs/rewritten.  We do not have to worry about the contents of that file
> getting too long, or label re-use, because the directory containing all
> these helper files will be deleted upon successful rebase in
> `sequencer_remove_state()`.

> > +static int do_reset(const char *name, int len)
> > +{
> > +       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 opts;
> > +       int ret = 0, i;
> > +
> > +       if (hold_locked_index(&lock, LOCK_REPORT_ON_ERROR) < 0)
> > +               return -1;
> > +
> > +       /* Determine the length of the label */
> > +       for (i = 0; i < len; i++)
> > +               if (isspace(name[i]))
> > +                       len = i;
> > +
> > +       strbuf_addf(&ref_name, "refs/rewritten/%.*s", len, name);
> > +       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(&opts, 0, sizeof(opts));
> > +       opts.head_idx = 1;
> > +       opts.src_index = &the_index;
> > +       opts.dst_index = &the_index;
> > +       opts.fn = oneway_merge;
> > +       opts.merge = 1;
> > +       opts.update = 1;
> > +       opts.reset = 1;
> > +
> > +       read_cache_unmerged();
> 
> In read-tree.c merge.c and pull.c we guard this conditionally
> and use die_resolve_conflict to bail out. In am.c we do not.
> 
> I think we'd want to guard it here, too?

> Constructing an instruction sheet that produces a merge
> conflict just before the reset is a bit artificial, but still:
> 
>     label onto
>     pick abc
>     exec false # run "git merge out-of-rebase-merge"
>         # manually to produce a conflict

> > +       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, &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);
> 
> For most newer structs we have a {release, clear, free}_X,
> but for tree_desc's this seems to be the convention to avoid memleaks.