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?
> 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.
> +
> + 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?
> +
> + 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.
> + }
> + 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);
> + rollback_lock_file(&lock);
> + strbuf_release(&ref_name);
> + return -1;
> + }
