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
