Hi Stefan,

On Tue, 30 Jan 2018, Stefan Beller wrote:

On Mon, Jan 29, 2018 at 2:54 PM, Johannes Schindelin
[off-list ref] wrote:

quoted

@@ -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.
+ */

Whoops. The comment "This is used by the `merge` command`" is completely
wrong. Will fix.

So this file contains (label -> commit),

Only `label`. No `commit`.

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()`.

Yes.

quoted

+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?

Yes.

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

This would fail already, as `exec` tests for a clean index after the
operation ran.

    reset onto # we want to stop here telling the user to fix it.

But you are absolutely right that we still need to fix it.

quoted

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

Yep, this code is just copy-edited from elsewhere. It seemed to be
different enough from the (very generic) use case in builtin/reset.c that
I did not think refactoring this into a convenience function in
unpack-trees.[ch] would make sense.

Ciao,
Dscho