Hi Stefan,
On Tue, 30 Jan 2018, Stefan Beller wrote:
> On Mon, Jan 29, 2018 at 2:54 PM, Johannes Schindelin
> <johannes.schindelin@xxxxxx> 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.
> > + */
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.
> > +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.
> > + 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
