On Fri, 18 Aug 2006, Johannes Schindelin wrote: > First, it _was_ a perl script, which you probably could find out by > checking your old git. > > Second, it was rewritten to use Git.pm, and because _that_ did not work, > git-mv was rewritten as a builtin. > It shouldn't have ever been a perl script, it should have been /bin/sh. Any shell implementation of this would be significantly faster than the current implementation. > Not slower? I beg to differ, admitting it is only a few percent. But your > statement is obviously uncorrect. > It _is_ slower since it takes considerably more time to do its job than any corresponding shell script. > Well, the patch has an improvement factor of almost none. I actually read > the patch, and asked myself: why would anybody fix a non-problem? > Because it's _wrong_. Secondly, it's WRONG. > > For example: > > (length = strlen(source[i])) >= 0 > > Yes. Taken out of context, this sure sounds silly. > > What you cleverly did not mention: It was inside a > > if (!bad && > (length = strlen(source[i])) >= 0 && > !strncmp(destination[i], source[i], length) && > (destination[i][length] == 0 || destination[i][length] == '/')) > > construct. So, we assign the "length" variable only if we have to. And the > ">= 0" trick is a common one. I could have done > This is not a plausible justification _at all_. The idea that "length" is assigned only on the condition that lstat(path, ...) failed does not justify its comparison to >= 0 since this comparison is always true, nor does it justify the assignment of char *dir = source[i]; int len = strlen(dir); later. > > strlen(source[i]) was assigned to a variable later in the function, this > > time called "len" instead. > > Only if source[i] is a directory. So again, we only do it when we need to. > You're completely ignoring the point, and more importantly, ignoring the code path. Your implementation would have _always_ assigned strlen(source[i]) to "length" if lstat returned 0. So at this point in the code, "length" is always equal to strlen(source[i]). But your code introduces another call to strlen, another variable, and another assignment. > Having said that, I do not have anything against the patch being applied, > but if I see more of these i-would-like-the-cupboard-here-not-there > patches, I will just not review them any more. > My patch is correct and improves your code. Any criticism for such a patch has purely personal motives, and not technical motives, assigned to it. David - To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html