Johannes Schindelin <johannes.schinde...@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.
