Re: [PATCH] cleans up builtin-mv

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > 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.
> 
> I usually side with you but on this I can't.
> 
> There are 2 ways to generate branch instructions in C.
> 
>  - compound statements specifically designed for expressing
>    control structure: if () ... else ..., for (), while (),
>    switch (), etc.
> 
>  - expressions using conditional operators or logical operators
>    that short circuit: ... ? ... : ..., ... && ... || ...
> 
> The latter form may still be readable even with simple side
> effects inside its terms, but "(l = strlen(s)) >= 0" is done
> solely for the side effect, and its computed value does not have
> anything to do with the logical operation &&.
> 
> THIS IS UGLY.  And do not want to live in a world where this
> ugliness is a "common one", as you put it.

> And this avoiding one call to strlen(source[i]) is unnecessary
> even as an optimization -- you end up calling strlen() on it
> later in the code anyway, as David points out.
> 
> I think this part is far easier to read if you did it like this:
>  
> 		length = strlen(source[i]);
> 		if (lstat(source[i], &st) < 0)
> 			bad = "bad source";
> 		else if (!strncmp(destination[i], source[i], length) &&
> 			 (destination[i][length] == 0 ||
> 			  destination[i][length] == '/'))
> 			bad = "can not move directory into itself";
> 
> 		if (S_ISDIR(st.st_mode)) {
> 			...
> 
> Note that the above is an absolute minimum rewrite.  Other
> things I noticed are:
> 
>  - source[i] and destination[i] are referenced all the time; the
>    code would be easer to read if you had something like this
>    upfront:
> 
>                 /* Checking */
>                 for (i = 0; i < count; i++) {
>                         const char *bad = NULL;
> 			const char *src = source[i];
>                         const char *dst = destination[i];
>                         int srclen = strlen(src);
>                         int dstlen = strlen(dst);
> 
>    You might end up not using dstlen in some cases, but I think
>    this would be far easier to read.  Micro-optimizing by saying
>    "this is used only in this branch of this later if()
>    statement but in that case it is always set in that branch of
>    that earlier if() statement" makes unmaintainably confusing
>    code.
> 
>  - I do not think you need "const char *dir, *dest_dir" inside
>    the "source is directory" branch; I would just use src and dst
>    consistently;