Re: [PATCH v2 02/10] sequencer: introduce new commands to reset the revision

Hi Eric,

On Tue, 30 Jan 2018, Eric Sunshine wrote:

On Mon, Jan 29, 2018 at 5:54 PM, Johannes Schindelin
[off-list ref] wrote:

quoted

[...]
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 <redacted>
---

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.

quoted

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

quoted

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

quoted

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

quoted

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