On Mon, May 22, 2017 at 2:17 AM, Junio C Hamano <gitster@xxxxxxxxx> wrote: > Samuel Lijin <sxlijin@xxxxxxxxx> writes: > >>> By the way, instead of putting NULL, it may be easier to follow if >>> you used two pointers, src and dst, into dir.entries[], just like >>> you did in your latest version of [PATCH 4/6]. That way, you do not >>> have to change anything in the later loop that walks over elements >>> in the dir.entries[] array. It would also help the logic easier to >>> follow if the above loop were its own helper function. >> >> Agreed on the helper function. On the src-dst thing: I considered it, >> but I figured another O(n) set of array moves was unnecessary. I guess >> this is one of those cases where premature optimization doesn't make >> sense? > > I actually did not mean to give the variables more descriptive names > and preserve the original 'main loop' (namely, not adding the "skip > if NULL" which would never happen in normal case where "-d" is not > used without "-x") as "optimization", whether it is premature or > not. My suggestions were purely from "wouldn't the resulting code > easier to follow and understand, leading to fewer bugs in the > future?" point of view. > > As I said, I am undecided if the result is easier to follow than > your version ;-) I think I'll defer to your patch: I do agree that your version is easier to follow and understand. Should I reroll just this patch and its commit message, or would you prefer to handle that in the queuing yourself?