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

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 16, 2016, 19:38 UTC
Message-ID: <xmqqvaujr7ed.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <a2ea8bd57461fe0969f3df09a0374005f12ac6c7.1481642927.git.johannes.schindelin@gmx.de>

Johannes Schindelin <johannes.schindelin@gmx.de> writes:

Show 8 quoted lines

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

Show 11 quoted lines

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

Show 22 quoted lines

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