"-b --suffix '' --delete --backup-dir /path/" combination does not act as expected

[jw schultz]

On Mon, Jul 28, 2003 at 05:19:23PM -0400, Bradley M. Kuhn wrote:
> If you use rsync with the following options:
> 
>     -b --suffix '' --delete --backup-dir /path/to/mybackupdir
> 
> You will find that it works properly with one exception: the deleted files
> are not properly put into /path/to/mybackupdir.  (Modified files are
> handled properly.)
> 
> I have traced the problem down to one line in receiver.c, which appears to
> already sport the question:
> 
>   /* Hi Andrew, do we really need to play with backup_suffix here? */
> 
> I think the answer is "yes", to avoid the brain-dead case of trying to
> back the file back up to the same name.
> 
> I added a check to receiver.c (patch attached) that checks for the
> specific case of having a backup-dir and an empty backup suffix.  I did
> not make the case any more general, so that it does not risk changing
> other behavior that people rely on.
> 
> I have a hard time believing that people might rely on the misfeature of
> "I don't want files that are scheduled for deletion in my backup dir".
> This is the only behavior I believe that my patch corrects.  I believe
> this change is a big help to people who use rsync to do incremental
> backups.
> 
> This patch is done against 2.5.6, but it appears to apply cleanly to a CVS
> checkout I did a few minutes ago.
> 
> I hereby put this patch into the public domain, and this message is
> GPG-signed.
> 
>    -- bkuhn

Any other developer's thoughts?  The logic may be good but
i don't like the patch.

The comment block belongs in the changelog, not here.

I hate the formatting: the whitespace is wrong 
and i HATE trailing operators.

Finally, the conditional is too convoluted.  It may be
slightly less efficient but i think it needs breaking up or
something.

For illustration I've reduced the indent a bit to keep from
wrapping and used indentation to reflect nesting.  This is
too ugly for words.

	if (make_backups
	    && ( ((k <= 0) || (strcmp(f+k, backup_suffix) != 0))
		|| ((backup_dir != NULL) && (strlen(backup_dir) > 0)
		    && (strlen(backup_suffix) == 0)
		)
	    )) {
		(void) make_backup(f);
	} else {
		deletion_count++;
		delete_one(local_file_list->files[i]);
	}

This has the conditional broken up.

The logic for the added test (bkdir_no_suffix) moved outside
the loops because it only looks at globals that are
unchanged since we parsed the command-line.

The test to see if f is suffixed i split out into another
(tiny) function.  I'd inline it but we don't have inlines
anywhere else.  It could be moved back but k should only be
bothered with if make_backups is set.


--- receiver.c.orig	Mon Jul 28 15:41:08 2003
+++ receiver.c.new	Mon Jul 28 15:46:43 2003
@@ -37,6 +37,7 @@
 extern int make_backups;
 extern int do_progress;
 extern char *backup_suffix;
+extern char *backup_dir;
 
 static struct delete_list {
 	DEV64_T dev;
@@ -99,8 +100,12 @@
 	}
 }
 
-
-
+int
+not_suffixed(char *f)
+{
+	int k = strlen(f) - strlen(backup_suffix);
+	return ((k <= 0) || (strcmp(f+k,backup_suffix) != 0)) ? 1 : 0;
+}
 
 /* this deletes any files on the receiving side that are not present
    on the sending side. For version 1.6.4 I have changed the behaviour
@@ -115,6 +120,11 @@
 	extern int max_delete;
 	static int deletion_count;
 
+	int bkdir_no_suffix = (backup_dir != NULL)
+		&& (strlen(backup_dir) > 0)
+		&& (strlen(backup_suffix) == 0);
+
+
 	if (cvs_exclude)
 		add_cvs_excludes();
 
@@ -148,10 +158,8 @@
 				add_delete_entry(local_file_list->files[i]);
 			if (-1 == flist_find(flist,local_file_list->files[i])) {
 				char *f = f_name(local_file_list->files[i]);
-				int k = strlen(f) - strlen(backup_suffix);
-/* Hi Andrew, do we really need to play with backup_suffix here? */
-				if (make_backups && ((k <= 0) ||
-					    (strcmp(f+k,backup_suffix) != 0))) {
+				if (make_backups
+				    && (not_suffixed(f) || bkdir_no_suffix)) {
 					(void) make_backup(f);
 				} else {
 					deletion_count++;