Hi Eric,
On Tue, 30 Jan 2018, Eric Sunshine wrote:
> On Mon, Jan 29, 2018 at 5:54 PM, Johannes Schindelin
> <johannes.schindelin@xxxxxx> wrote:
> > [...]
> > This commit implements the commands to label, and to reset to, given
> > revisions. The syntax is:
> >
> > label <name>
> > reset <name>
> > [...]
> >
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@xxxxxx>
> > ---
> > diff --git a/sequencer.c b/sequencer.c
> > @@ -1253,7 +1266,8 @@ static int parse_insn_line(struct todo_item *item, const char *bol, char *eol)
> > if (skip_prefix(bol, todo_command_info[i].str, &bol)) {
> > item->command = i;
> > break;
> > - } else if (bol[1] == ' ' && *bol == todo_command_info[i].c) {
> > + } else if ((bol + 1 == eol || bol[1] == ' ') &&
> > + *bol == todo_command_info[i].c) {
>
> This adds support for commands which have no arguments, however, now
> that the "bud" command has been retired, this can go away too, right?
Good point. Fixed.
> > bol++;
> > item->command = i;
> > break;
> > @@ -1919,6 +1934,144 @@ static int do_exec(const char *command_line)
> > +static int safe_append(const char *filename, const char *fmt, ...)
> > +{
> > + va_list ap;
> > + struct lock_file lock = LOCK_INIT;
> > + int fd = hold_lock_file_for_update(&lock, filename, 0);
> > + struct strbuf buf = STRBUF_INIT;
> > +
> > + if (fd < 0)
> > + return error_errno(_("could not lock '%s'"), filename);
>
> Minor: unable_to_lock_message() can provide a more detailed
> explanation of the failure.
That is true. Due to its awkward signature (returning void, using a
strbuf), it would add a whopping 4 lines, too.
There is a better solution, though, adding only one line: passing
LOCK_REPORT_ON_ERROR as flag to hold_lock_file_for_update().
> > +
> > + if (strbuf_read_file(&buf, filename, 0) < 0 && errno != ENOENT)
> > + return error_errno(_("could not read '%s'"), filename);
> > + strbuf_complete(&buf, '\n');
> > + va_start(ap, fmt);
> > + strbuf_vaddf(&buf, fmt, ap);
> > + va_end(ap);
>
> Would it make sense to also
>
> strbuf_complete(&buf, '\n')
>
> here, as well, to be a bit more robust against lazy callers?
I'd rather not make that assumption. It *may* be true that the current
sole user wants the last line of the file to end in a newline. I try to
design my code for maximum reusability, though. And who is to say whether
my next use case for the safe_append() function wants the semantics you
suggest, if it wants to append less than entire lines at a time, maybe?
Let's not optimize prematurely, okay?
> > +
> > + if (write_in_full(fd, buf.buf, buf.len) < 0) {
> > + rollback_lock_file(&lock);
> > + return error_errno(_("could not write to '%s'"), filename);
>
> Reading lockfile.h & tempfile.c, I see that rollback_lock_file()
> clobbers write_in_full()'s errno before error_errno() is called.
True. Fixed.
I also fixed the code from where I copy-edited this pattern (increasing
the patch series by yet another patch).
> > + }
> > + if (commit_lock_file(&lock) < 0) {
> > + rollback_lock_file(&lock);
> > + return error(_("failed to finalize '%s'"), filename);
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int do_reset(const char *name, int len)
> > +{
> > + [...]
> > + 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);
>
> Checking my understanding: The two get_oid() calls allow the argument
> to 'reset' to be a label created with the 'label' command or any other
> way to name an object, right? If so, then I wonder if the error
> invocation should instead be:
>
> error(_("could not read '%.*s'"), len, name);
I would rather give the preferred form: refs/rewritten/<label>.
The main reason this code falls back to getting the OID of `<label>`
directly is to support the `no-rebase-cousins` code: in that mode, topic
branches may be based on commits other than the one labeled `onto`, but
the original, unchanged one. In this case, we have no way of labeling the
base commit, and therefore use a unique abbreviation of that base commit's
OID.
But this is really a very special use case, and the more common use case
should be the one using refs/rewritten/<label>.
Ciao,
Dscho
