Re: [PATCH] cleans up builtin-mv

From: David Rientjes <rientjes@google.com>
Date: Aug 18, 2006, 17:07 UTC
Message-ID: <Pine.LNX.4.63.0608180956100.29405@chino.corp.google.com>
In-Reply-To: <Pine.LNX.4.63.0608181137000.28360@wbgn013.biozentrum.uni-wuerzburg.de>

On Fri, 18 Aug 2006, Johannes Schindelin wrote:

Show 6 quoted lines

> 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.

Show 15 quoted lines

> > 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.

Show 5 quoted lines

> > 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