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

From: Junio C Hamano <[email-masked]>
Date: Fri, 16 Dec 2016 11:38:50 -0800

Johannes Schindelin <[email-masked]> 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.  

> +
> +	fprintf(out, "%s\n", oid_to_hex(oid));
> +	fclose(out);
> +
> +	if (!is_fixup(next_command))
> +		flush_rewritten_pending();
> +}
> +
>  static int do_pick_commit(enum todo_command command, struct commit *commit,
>  		struct replay_opts *opts, int final_fixup)
>  {
> @@ -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".  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".