Re: [PATCH] cleans up builtin-mv

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

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;

 - You muck with dest_dir by calling add_slash(dest_dir) but
   call prefix_path() with dst_len you computed earlier;
   prefix_path() may know what to do, but is this intended?