Hi Junio, On 04/12/17 11:09 AM, Junio C Hamano wrote: > Johannes Schindelin <Johannes.Schindelin@xxxxxx> 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. > Good point on saying this name change is going backwards. > 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). > You're right! We do want the name to reflect that we intend to change both halves and not only the command. > Calling it just transform_todo() would probably be more in line with > the reason why you wanted to rename it in the first place. > Good suggestion. Would transform_todos() work too? I'll send an update tomorrow. Thanks, Liam
