Re: [PATCH] cleans up builtin-mv

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Aug 18, 2006, 09:51 UTC
Message-ID: <Pine.LNX.4.63.0608181137000.28360@wbgn013.biozentrum.uni-wuerzburg.de>
In-Reply-To: <Pine.LNX.4.63.0608172301520.25827@chino.corp.google.com>

Hi,

On Thu, 17 Aug 2006, David Rientjes wrote:

Show 8 quoted lines

> On Thu, 17 Aug 2006, David Rientjes wrote:
> 
> > Cleans up builtin-mv by removing a needless check of source's length, 
> > redefinition of source's length, and misuse of strlen call that was 
> > already assigned.
> > 
> 
> I'm not sure when this command had been added to the tree

Tip of the day: "git log builtin-mv.c"

> because it definitely was not included six months ago in a git tree I 
> use everyday.  It seems to me like this would more appropriately be 
> handled by a simple shell script

Nooooooo!

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.

> that would be much simpler to implement and could not possibly be slower 
> than this implementation.

Not slower? I beg to differ, admitting it is only a few percent. But your statement is obviously uncorrect.

As for the "simpler to implement": "harder to port" comes into mind. And do not try to argue that everybody and his dog could switch to Linux.

> This patch is a small fraction of what could be changed in this 
> implementation and I don't doubt it will undergo a complete rewrite in 
> the future.

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?

Show 6 quoted lines

> I think the problems with it have compounded on top of itself over time 
> which doesn't make a lot of sense since it appears to be a relatively 
> new addition.
> 
> 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

		!strncmp(destination[i], source[i], (length = strlen(source[i])))

but even I find that ugly.

> was _completely_ unnecessary since the previous instruction was a call to 
> lstat(source[i], ...) which would return ENOENT if source[i] was empty.

Clarified enough?

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

> This code is _utterly_ unsatisfactory.

I disagree.

What is unsatisfactory to me is that I expected some performance improvements from one of your earlier mails, and I see patches which rearrange working code. This _can_ result in performance improvements, but in these cases, no, it doesn't.

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.

Ciao, Dscho