Re: Cleaning up elkscmd and adding help text - Questions

[Jody Bruchon <jody@jodybruchon.com>]

On 3/27/2015 4:19 AM, Nils Stec wrote:
> I wrote a patch for cat which shows the user some usage-information.
> Jody Bruchon used write() for this purpose, other tools are using
> fprintf(stderr, ....).
> I don't know which i should prefer. In my opinion i would rather use
> fprintf.
> STDERR should be the output for operations, right?

Some of the programs already used write() and write() avoids dragging 
the code in for the printf family of functions (all ELKS programs are 
statically linked), so if there is NO usage of the printf family of 
functions in the program, it may make sense to use write() to minimize 
total program size. That being said, even a trivial usage message should 
mention the program being run in the usage, and it's not a good idea to 
assume that i.e. 'ls' is ALWAYS run as 'ls' since someone could symlink 
or alias it in the future. Because of this,

fprintf(stderr, "usage: %s args\n", argv[0]);

is a thousand times better than:

write(STDERR_FILENO, "usage: ", 7);
write(STDERR_FILENO, argv[0], strlen(argv[0]));
write(STDERR_FILENO, "\n", 1);

This results in three calls to write() and a call to strlen() when we 
could have just used one fprintf() to do all the work. In the future, I 
think many calls to write() will be eliminated, since even one call to a 
printf family function brings in the core code for all printf functions 
anyway.

> Is this the right way to write code for this project? It's the first
> time I'm doing this in here and i don't wan't to do things wrong...

The cat patch should use fprintf(stderr, ...) since I've already brought 
in the printf function family with my error_read: code and write() is 
bad for the reasons outlined above.

A few other comments:

+#define NOT_OVERWRITING		0xABCD
...snip...
+			return NOT_OVERWRITING;

If the user asks you to not overwrite (clobber), you should silently not 
clobber files and not consider it an error. If any program succeeds in 
doing what the user asked for, it should return success (0) with no 
messages. If you haven't read Eric S. Raymond's rules of the Unix 
philosophy, take a look at them; I find them to be very helpful 
guidelines when making program design decisions.

+	if (stat(srcname, &statbuf1) < 0) {	// src nonexistent

You can't write comments using // unless you're using a C99 compiler. 
bcc is a K&R C compiler that is not even guaranteed to be C89 compliant, 
so no // comments. Use /* comment */ instead. Source comments are nice 
to have as long as they're not excessive or explaining something that is 
very obvious.

+static int option_no_overwrite;
+static int option_verbose;
+static int option_interactive;

Depending on how many options you add, you may want to use one global 
static int called 'flags' or similar, and use bit flags instead of a 
bunch of ints. From the program 'fdupes' in one of my GitHub repos, 
snipped up for brevity:

#define ISFLAG(a,b) ((a & b) == b)
#define SETFLAG(a,b) (a |= b)
#define F_RECURSE 0x0001
#define F_HIDEPROGRESS 0x0002
etc. etc.
...
switch (opt) {
case 'q':
   SETFLAG(flags, F_HIDEPROGRESS);
   break;
...
if (ISFLAG(flags, F_RECURSEAFTER)) <code here>...
if (!ISFLAG(flags, F_HIDEPROGRESS)) <code here>...

I don't necessarily recommend using this code, it's just an example and 
it's written to be usable with more than one variable that holds a set 
of flags. I'd rewrite it like this for ELKS programs to make it cleaner:

static int flags;
#define ISFLAG(a) ((flags & a) == a)
#define SETFLAG(a) (flags |= a)
#define F_QUIET 0x0001
etc.
...
if (strcmp(argv[i], "-q")) SETFLAG(F_QUIET);
if (!ISFLAG(F_QUIET)) fprintf(stderr, "copied %d files\n", count);

---
+	if(option_verbose) {
+		printf("copied '%s' -> '%s'\n", srcname, destname);
+	}

Firstly, the output from GNU 'cp' looks like this:

$ cp -v foo/* bar/
'foo/1' -> 'bar/1'
'foo/2' -> 'bar/2'
'foo/3' -> 'bar/3'

I'd also prefer one-liners such as this with no multi-line 'else' clause 
to be collapsed together (it makes the code easier to read and avoids 
'brace hell'). I also prefer that if, for, and while statements have a 
space between the word and the (condition) part for the sake of code 
readability. Write it like this instead:

if (option_verbose) printf("'%s' -> '%s'\n", srcname, destname);

Next:

+	// set defaults for options //
+	option_no_overwrite = 0;
+	option_verbose = 0;
+	option_interactive = 0;

The comment is unnecessary since it's obvious that "options are being 
initialized to zero" by the code itself. This is self-commenting code. 
I'd also prefer to see the variable names shortened a bit ('o_' or 
'opt_' instead of 'option_' for example) and as mentioned previously, 
it's probably better to use the bit flags method mentioned previously if 
you're only using the variables as a yes/no flag.

+	arg_counter = 1;	// start at argv[1]
+	options_counter = 0;
+	while(arg_counter < argc) {
+		if(!strcmp(argv[arg_counter], "-n")) {
+			options_counter++;
+			option_no_overwrite = 1;
+		}
+		if(!strcmp(argv[arg_counter], "-v")) {
+			options_counter++;
+			option_verbose = 1;
+		}
+		if(!strcmp(argv[arg_counter], "-i")) {
+			options_counter++;
+			option_interactive = 1;
+		}
+		if(!strcmp(argv[arg_counter], "-h")) goto usage;
+		arg_counter++;
+	}

-	if ((argc > 3) && !dirflag) {
-		fprintf(stderr, "%s: not a directory\n", lastarg);
+	if((argc > (3+options_counter)) && !dirflag) { 	// can't copy more 
than one src-files into one dst-file

Ouch. Rather than try to explain the problems in detail, let me rewrite it.

	arg_cnt = 1;	// start at argv[1]
	opt_cnt = 0;
	while(arg_cnt < argc) {
		if (!strcmp(argv[arg_cnt], "-n")) {
			opt_noclobber = 1;
		} else if (!strcmp(argv[arg_cnt], "-v")) {
			opt_verbose = 1;
		} else if (!strcmp(argv[arg_cnt], "-i")) {
			opt_interactive = 1;
		} else if (!strcmp(argv[arg_cnt], "-h")) goto usage;
		else break;
		opt_cnt++;
		arg_cnt++;
	}

	/* If multiple files specified, last one must be a directory */
	if ((argc > (opt_cnt + 3)) && !dirflag) {

This rewrite combines all the option count increments into one instance 
which reduces code size and stops parsing options at the first 
non-option string. The comparison on the last line is more readable and 
it is clearer that it is "comparing total argument count to the number 
of options specified plus 3." The 'if-elseif-else' tree stops processing 
once a valid option is hit and "falls through" once a non-option is hit. 
It could (and probably should) be improved by handling the combined 
option case such as 'cp -iv src dest' but this is sufficient for now as 
long as the usage text implies that option flags must be specified 
separately.

+	copy_count = 0;
+	file_count = 0;
+	while (argc-- > (2+options_counter)) {
+		srcname = argv[options_counter+1+file_count];

I've spent a lot of time cleaning these compressed mathematical 
expressions out of the elkscmd code; let's not introduce more of them 
(also shorten variable names while retaining their meaning, this is a 
good example for why to use shorter names):

+	copy_cnt = 0;
+	file_cnt = 0;
+	while (argc-- > (opt_cnt + 2)) {
+		srcname = argv[opt_cnt + 1 + file_cnt];

Next,

-		if (dirflag) destname = buildname(destname, srcname);
+		if(dirflag) destname = buildname(destname, srcname);

Don't do this, if no other reason than the fact that 'if' is not a 
function call. I understand that it comes down to individual coding 
style and the compiler doesn't care if you write

if((something+(q/2))==argv[x+2+y*z[aaa]])usage();

but it is hard to read and readability is very important. I believe that 
'if', 'while', and 'for' shouldn't look like function calls in code 
since they are actually C control statements. Using a space makes those 
control statements immediately obvious, whereas if() and while() and 
for() look like function calls when skimming the code.

-		if (!copyfile(*++argv, destname, 0)) goto error_copy;
+		retval = copyfile(srcname, destname, 0);
+		if((!retval) && (!(retval == NOT_OVERWRITING))) goto error_copy;

Remember the rule about silent success mentioned earlier? This change 
can be eliminated because of it. Even if it wasn't, though, this code 
could be simplified and made more readable in one shot:

+		retval = copyfile(srcname, destname, 0);
+		if (retval != 1) goto error_copy;

I'd actually recommend reversing the return values in copyfile(), using 
nonzero as an error return instead (most C library calls seem to use 
this convention and consistency with that is rarely a bad thing) so you 
could just go:

+		if (retval) goto error_copy;

Next,

+	if(option_verbose) printf("copy-count = %d\n", copy_count);

This shouldn't be done. Here's GNU coreutils 'cp' in action:

file_utils $ cp -v foo/* bar/
'foo/COPYING' -> 'bar/COPYING'
'foo/COPYING.sash' -> 'bar/COPYING.sash'
'foo/Makefile' -> 'bar/Makefile'
'foo/README' -> 'bar/README'
'foo/WARRANTY' -> 'bar/WARRANTY'
...snip...

If someone is interested in knowing a "copy-count" for some reason, 
they'll almost certainly do this instead:

file_utils $ cp -v foo/* bar/ | wc -l
46

Your helpful message will throw this off and make it not work as 
expected. A shell script author will have to introduce an incompatible 
kludge to their script to make up for 'cp' misbehaving, i.e.

CNT=$(cp -v $A/* $B/ | wc -l)
CNT=$((CNT - 1))  # This shouldn't be necessary...

Another problem is that you've declared a static global 'copy_count' 
variable and you're introducing a local 'copy_count' variable as well. 
It needs to be called something else for the sake of avoiding programmer 
confusion. However, assuming you eliminate the "copy-count" message I 
just talked about along with its local variable counterpart, the global 
'static unsigned int copy_count' is set (copy_count++) but not used 
throughout the entire program and can be discarded.

If you ever want an "official" reference for how a particular elkscmd 
program "should" behave, take a look at the POSIX standards:

http://pubs.opengroup.org/onlinepubs/9699919799/

Whew, that took me a long time to write. I hope it's helpful to you and 
everyone else on the list. Thanks for your impressive work so far! I'm 
looking forward to your finalized [PATCH] submissions. :-)

-Jody