Re: [PATCH v2 4/9] rebase -i: refactor transform_todo_ids

[Junio C Hamano <gitster@pobox.com>]

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

> On Sun, 3 Dec 2017, Liam Beguin wrote:
>
>> The transform_todo_ids function is a little hard to read. Lets try
>> to make it easier by using more of the strbuf API. Also, since we'll
>> soon be adding command abbreviations, let's rename the function so
>> it's name reflects that change.
>
> I am not really a fan of the new name, and would prefer the old one, but
> that's only a nit, not a reason to reject the patch.

FWIW, I do think the new name goes backwards.  The command uses to
remember what operations are to be carried out in which order using
a thing, and the name of that thing "todo list".  We also called it
the "instruction sheet", and "insn" was a good term to call one item
on that sheet among other items.

But recent commits in the area are pushing us to call it "todo list"
consistently.  An element in that list should be called "todo".

A "todo" consists of two parts, "what operation is done" part and
"using what commit object" part.  The original implementation of
this function affected only the latter part, and in that light, the
original name transform_todo_ids() is understandable.  Now you are
planning to make it modify both parts, not just "ids", so it is
understandable that you would want to rename it.  But I do not think
"insn" matches the recent trend.  It even risks misunderstanding
(i.e. xfrm_todo_ids() is about modifying "using what commit" part,
so perhaps xfrm_todo_insns() is about modifying "what operation is
done" part---but that is different from what you want to do, which
is to update _both_ halves).

Calling it just transform_todo() would probably be more in line with
the reason why you wanted to rename it in the first place.