Re: [PATCH v3 07/25] sequencer: completely revamp the "todo" script parsing
- From
Johannes Schindelin <johannes.schindelin@gmx.de>
- Date
- Oct 11, 2016, 10:20 UTC
- Message-ID
- <alpine.DEB.2.20.1610111212090.35196@virtualbox>
- In-Reply-To
- <xmqqlgxvdf90.fsf@gitster.mtv.corp.google.com>
Hi Junio,
On Mon, 10 Oct 2016, Junio C Hamano wrote:
Show 19 quoted lines
> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
>
> > 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.Urgh. Thanks for pointing this out to me, though. Will be fixed in the next iteration.
Show 22 quoted lines
> > + 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.Right, I actually looked at this place when I tried to decide where I could use that function. Somehow I thought I'd not break up the flow here.
But since you asked so nicely, I'll squash this in (I personally find it uglier, and longer, but it does use strbuf_add_unique_abbrev() now):
-- snipsnap --
@@ -906,11 +904,13 @@ static int walk_revs_populate_todo(struct todo_list *todo_list, item->command = command; item->commit = commit; item->offset_in_buf = todo_list->buf.len; + strbuf_addstr(&todo_list->buf, command); + strbuf_addch(&todo_list->buf, ' '); + strbuf_add_unique_abbrev(&todo_list->buf, + commit->object.oid.hash, + DEFAULT_ABBREV); 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); + strbuf_add(&todo_list->buf, subject, subject_len); unuse_commit_buffer(commit, commit_buffer); } return 0;