Subject: Re: [PATCH v3 07/25] sequencer: completely revamp the "todo" script parsing

From: Junio C Hamano <[email-masked]>
Date: Mon, 10 Oct 2016 15:13:31 -0700

Johannes Schindelin <[email-masked]> writes:

> Let's just bite the bullet and rewrite the entire parser; the code now
> ...
> In particular, we choose to maintain the list of commands in an array
> instead of a linked list: this is flexible enough to allow us later on to
> even implement rebase -i's reordering of fixup!/squash! commits very
> easily (and with a very nice speed bonus, at least on Windows).
>
> While at it, do not stop at the first problem, but list *all* of the
> problems. This will help the user when the sequencer will do `rebase
> -i`'s work by allowing to address all issues in one go rather than going
> back and forth until the todo list is valid.

All sounds sensible.

>  	if (parent && parse_commit(parent) < 0)
> -		/* TRANSLATORS: The first %s will be "revert" or
> -		   "cherry-pick", the second %s a SHA1 */
> +		/*
> +		 * TRANSLATORS: The first %s will be a "todo" command like
> +		 * "revert" or "pick", the second %s a SHA1.
> +		 */

You may want to double check this with i18n folks; IIRC the tool
that extracts TRANSLATORS: comment was somewhat particular about
where that magic "TRANSLATORS:" token resides on a comment line and
that is why we have this multi-line comment formatted in an unusual
way.

Ahh, no you do not have to bug i18n folks.  47fbfded53 ("i18n: only
extract comments marked with "TRANSLATORS:"", 2014-04-17) is an
example of such an adjustment.

I just found it in CodingGuidelines, cbcfd4e3ea ("i18n: mention
"TRANSLATORS:" marker in Documentation/CodingGuidelines",
2014-04-18).

> +	while ((commit = get_revision(opts->revs))) {
> +		struct todo_item *item = append_new_todo(todo_list);
> +		const char *commit_buffer = get_commit_buffer(commit, NULL);
> +		const char *subject;
> +		int subject_len;
> +
> +		item->command = command;
> +		item->commit = commit;
> +		item->offset_in_buf = todo_list->buf.len;
> +		subject_len = find_commit_subject(commit_buffer, &subject);
> +		strbuf_addf(&todo_list->buf, "%s %s %.*s\n", command_string,
> +			find_unique_abbrev(commit->object.oid.hash,
> +				DEFAULT_ABBREV),
> +			subject_len, subject);

I am personally fine with this line; two things come to mind:

 - This would work just fine as-is with Linus's change to turn
   DEFAULT_ABBREV to -1.

 - It appears that it is more fashionable to use
   strbuf_add_unique_abbrev() these days.

Thanks.