Hi Phillip,

On Fri, 19 Jan 2018, Phillip Wood wrote:

On 18/01/18 15:35, Johannes Schindelin wrote:

quoted

This idea was developed in Git for Windows' Git garden shears (that
are used to maintain the "thicket of branches" on top of upstream
Git), and this patch is part of the effort to make it available to a
wider audience, as well as to make the entire process more robust (by
implementing it in a safe and portable language rather than a Unix
shell script).

This commit implements the commands to label, and to reset to, given
revisions. The syntax is:

	label <name>
	reset <name>

If I've understood the code below correctly then reset will clobber
untracked files, this is the opposite behaviour to what happens when
tries to checkout <onto> at the start of a rebase - then it will fail if
untracked files would be overwritten.

This would be completely unintentional, I will verify that untracked files
are not clobbered.

However, in practice this should not happen because the intended use case
is for revisions to be labeled *before* checking them out at a later
stage. Therefore, the files that would be clobbered would already have
been tracked in the revision when it was labeled, and I do not quite see
how those files could become untracked without playing sloppy exec games
in between.

quoted

Internally, the `label <name>` command creates the ref
`refs/rewritten/<name>`. This makes it possible to work with the labeled
revisions interactively, or in a scripted fashion (e.g. via the todo
list command `exec`).

If a user has two work trees and runs a rebase in each with the same
label name, they'll clobber each other. I'd suggest storing them under
refs/rewritten/<branch-name or detached HEAD SHA> instead. If the user
tries to rebase a second worktree with the same detached HEAD as an
existing rebase then refuse to start.

That is why a later patch marks those refs/rewritten/ refs as
worktree-local.

quoted

+static int do_label(const char *name, int len)
+{
+	struct ref_store *refs = get_main_ref_store();
+	struct ref_transaction *transaction;
+	struct strbuf ref_name = STRBUF_INIT, err = STRBUF_INIT;
+	struct strbuf msg = STRBUF_INIT;
+	int ret = 0;
+	struct object_id head_oid;
+
+	strbuf_addf(&ref_name, "refs/rewritten/%.*s", len, name);
+	strbuf_addf(&msg, "label '%.*s'", len, name);

The other reflog messages below have a (rebase -i) prefix

Good point. I changed it to "rebase -i (label)".

quoted

+	transaction = ref_store_transaction_begin(refs, &err);
+	if (!transaction ||
+	    get_oid("HEAD", &head_oid) ||
+	    ref_transaction_update(transaction, ref_name.buf, &head_oid, NULL,
+				   0, msg.buf, &err) < 0 ||
+	    ref_transaction_commit(transaction, &err)) {
+		error("%s", err.buf);

if get_oid() fails then err is empty so there wont be an message after
the 'error: '

Yep, that would be nasty. Fixed.

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;
+
+	for (i = 0; i < len; i++)
+		if (isspace(name[i]))
+			len = i;

If name starts with any white space then I think this effectively
truncates name to a bunch of white space which doesn't sound right. I'm
not sure how this is being called, but it might be better to clean up
name when the to-do list is parsed instead.

The left-trimming of the name was already performed as part of the todo
list parsing.

And we are not really right-trimming here. We are splitting a line of the
form

	reset <label> <oneline>

In fact, after reflecting about it, I changed the code so that it would
now even read:

	reset <label> # <oneline>

So the code really is doing the intended thing here.

Ciao,
Dscho