Re: [PATCH 1/8] sequencer: introduce new commands to resettherevision

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi Phillip,

On Fri, 19 Jan 2018, Phillip Wood wrote:

> 
> On 18/01/18 15:35, Johannes Schindelin wrote:
> 
> > 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.

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

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

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

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