Re: [PATCH v2 20/34] sequencer (rebase -i): copy commit notes at end

Johannes Schindelin [off-list ref] writes:

+static void flush_rewritten_pending(void) {
+	struct strbuf buf = STRBUF_INIT;
+	unsigned char newsha1[20];
+	FILE *out;
+
+	if (strbuf_read_file(&buf, rebase_path_rewritten_pending(), 82) > 0 &&
+			!get_sha1("HEAD", newsha1) &&
+			(out = fopen(rebase_path_rewritten_list(), "a"))) {

An error in fopen() here ...

+ ...
+	}
+	strbuf_release(&buf);
+}
+
+static void record_in_rewritten(struct object_id *oid,
+		enum todo_command next_command) {
+	FILE *out = fopen(rebase_path_rewritten_pending(), "a");
+
+	if (!out)
+		return;

... and here are ignored as an insignificant error in the scripted
version, and this one does the same.  

Makes me wonder, after having commented on 07/34 regarding the fact
that in the end you would end up having three variants of no-op
(i.e. NOOP, DROP and COMMENT), what definition of a "command" this
function uses to return its result, when asked to "peek".  I suspect
that this will be updated in a later patch to do "< TODO_NOOP"
instead?  If so, then that answers one question in my comment on
07/34, i.e.

    If a check for "is it one of the no-op commands?" appears only
    here, a single liner comment may be sufficient (but necessary)
    to help readers.  Otherwise a single-liner helper function
    (similar to is_fixup() you have) with a descriptive name would
    be better than a single liner comment.

The answer is "no, it is not just there" hence the conclusion is "we
want a helper with a descriptive name".

Hi Junio,

On Fri, 16 Dec 2016, Junio C Hamano wrote:

Johannes Schindelin [off-list ref] writes:

quoted

@@ -1750,6 +1797,17 @@ static int is_final_fixup(struct todo_list *todo_list)
 	return 1;
 }
 
+static enum todo_command peek_command(struct todo_list *todo_list, int offset)
+{
+	int i;
+
+	for (i = todo_list->current + offset; i < todo_list->nr; i++)
+		if (todo_list->items[i].command != TODO_NOOP)
+			return todo_list->items[i].command;

Makes me wonder, after having commented on 07/34 regarding the fact
that in the end you would end up having three variants of no-op
(i.e. NOOP, DROP and COMMENT), what definition of a "command" this
function uses to return its result, when asked to "peek".

Well, it uses the todo_command idea of a "command"... ;-)

The only thing we do with this for now is to look whether the next command
is a fixup/squash (so that the user gets to edit the commit message just
once, for example, and also to record rewritten commits properly).

I suspect that this will be updated in a later patch to do "< TODO_NOOP"
instead?

Actually, no. I introduced a new function is_noop() and that is used now.

Ciao,
Dscho