Merge lp:~nathanael-naeri/deja-dup/fix-1549776 into lp:deja-dup/34

[Naël]

Support $HOME/subdir and $XDG_SPECIAL_USER_DIRECTORY/subdir constructs in include-list and exclude-list DConf keys (fixes LP:1549776).

I propose minor changes to libdeja/DirHandling.vala in order to support constructs like $HOME/subdir and $DOCUMENTS/subdir in the include-list and exclude-list DConf keys (fixes LP:1549776). Current support is limited to the top directories ($HOME, $DOCUMENTS...) without any subdirectory indication.

The documentation for the include-list and exclude-list keys is changed accordingly in data/org.gnome.DejaDup.gschema.xml.in. Changes are limited to those two files. Please review the bug report and the changes in those files.

Testing the changes in this branch is made easier by the fixes to the test shell I propose in branch lp:~nathanael-naeri/deja-dup/fix-1660174-1660224-1660342, so please merge that branch first.

[Michael Terry (mterry)]

This looks fine, thank you so much!

Ideally there'd be a test exercising the new functionality. But I don't want to block the improvement on that.

And ideally this whole code path would be a bit more robust. Like we replace $USER, but do so even if the user uses $USERSTRING or some such nonsense. But that's a separate patch and perhaps this feature doesn't really need an industrial-strength parser.

review: Approve

[Michael Terry (mterry)]

One thing I notice is that we might have a path like $MUSIC/$USER/importantstuff

So I'm merging your branch, but dropping the "else" bit at the end of the list and unconditionally replacing $USER.

[Naël (nathanael-naeri)]

Thanks for the review and the merge Michael. I think dropping the else condition is a good idea indeed. However, it results in a metric ton of errors when I run "make check". I think it's just because what was in the else:

  result = dir.replace("$USER", Environment.get_user_name());

now overwrites "result" unconditionally with "dir.replace", so the benefit of all the previous if-conditions is lost. But we can't simply use:

  result = result.replace("$USER", Environment.get_user_name());

either because then "result" is "null" if none of the previous if-conditions is triggered.

Below is a possible fix to this issue (can be applied to current revision 1572 but beware of Launchpad line wrapping), but there are many others I think, for instance initializing "result" with "dir" instead of "null" and then using "result = result.replace" should work too.

----- BEGIN PATCH -----
=== modified file 'libdeja/DirHandling.vala'
--- libdeja/DirHandling.vala 2017-02-13 22:18:16 +0000
+++ libdeja/DirHandling.vala 2017-02-19 21:05:35 +0000
@@ -52,9 +52,11 @@
     result = dir.replace("$TRASH", get_trash_path());
   else if (dir.has_prefix("$VIDEOS"))
     result = dir.replace("$VIDEOS", Environment.get_user_special_dir(UserDirectory.VIDEOS));
+ else
+ result = dir;

   // Some variables can be placed anywhere in the path
- result = dir.replace("$USER", Environment.get_user_name());
+ result = result.replace("$USER", Environment.get_user_name());

   // Relative paths are relative to the user's home directory
   if (Uri.parse_scheme(result) == null && !Path.is_absolute(result))
----- END PATCH -----

[Naël (nathanael-naeri)]

Regarding an automated test for this new feature (support of $HOME/subdir and $XDG_SPECIAL_USER_DIRECTORY/subdir): I don't know how to write automated tests for Déjà-Dup, but I explained how I tested my patch in bug 1549776 comment 14. Perhaps we can use these explanations as the basis for a test.

I have tried to do so in the following file, adapted from libdeja/tests/scripts/excludes.test. This is only a draft, and it doesn't pass "make check": it looks like in tests, despite my previous patch about the test shell, $HOME is still the actual user's home directory, instead of a fake home directory in /tmp, and the XDG Special User Directories are not created in the /tmp/deja-dup-test-* directories, so it can't be tested if they are appropriately taken into account.

I'm afraid I don't have the knowledge to take it further.

----- BEGIN FILE: libdeja/tests/scripts/xdg-special-user-dirs.test -----
[Operation]
Type=backup
Settings=include-list=['$HOME/included', '$DOWNLOAD/included', '$MUSIC'];exclude-list=['$HOME', '$HOME/included/excluded', '$DOWNLOAD/included/excluded', '$TRASH'];
Script=mkdir -p $HOME/included/excluded $DOWNLOAD/included/excluded; touch $HOME/included/in.txt $HOME/included/excluded/ex.txt $DOWNLOADS/included/in.jpg $DOWNLOAD/included/excluded/ex.jpg

[Duplicity]
Runs=status;dry;backup;status-restore;list;verify;

[Duplicity status]
IncludeArgs='--exclude=$HOME/included/excluded' '--include=$HOME/included' '--exclude=$HOME' '--exclude=$DOWNLOAD/included/excluded' '--include=$DOWNLOAD/included' '--include=$MUSIC' '--exclude=$TRASH'

[Duplicity dry]
IncludeArgs='--exclude=$HOME/included/excluded' '--include=$HOME/included' '--exclude=$HOME' '--exclude=$DOWNLOAD/included/excluded' '--include=$DOWNLOAD/included' '--include=$MUSIC' '--exclude=$TRASH'

[Duplicity backup]
IncludeArgs='--exclude=$HOME/included/excluded' '--include=$HOME/included' '--exclude=$HOME' '--exclude=$DOWNLOAD/included/excluded' '--include=$DOWNLOAD/included' '--include=$MUSIC' '--exclude=$TRASH'

----- END FILE: libdeja/tests/scripts/xdg-special-user-dirs.test -----

[Michael Terry (mterry)]

Oh geeze, thanks for that catch. Lazy last minute patch edit by me. I've committed a fixed version, cheers!

=== modified file 'data/org.gnome.DejaDup.gschema.xml.in'
--- data/org.gnome.DejaDup.gschema.xml.in	2016-12-15 18:33:08 +0000
+++ data/org.gnome.DejaDup.gschema.xml.in	2017-02-12 02:37:21 +0000
@@ -4,12 +4,12 @@
     <key name="include-list" type="as">
       <default>[ '$HOME' ]</default>
       <_summary>Folders to save</_summary>
-      <_description>This list of directories will be backed up.  Reserved values $HOME, $DESKTOP, $DOCUMENTS, $DOWNLOAD, $MUSIC, $PICTURES, $PUBLIC_SHARE, $TEMPLATES, $TRASH, and $VIDEOS are recognized as the user’s special directories.  Relative entries are relative to the user’s home directory.</_description>
+      <_description>This list of directories will be backed up.  Reserved values $HOME, $DESKTOP, $DOCUMENTS, $DOWNLOAD, $MUSIC, $PICTURES, $PUBLIC_SHARE, $TEMPLATES, $TRASH, and $VIDEOS are recognized as the user’s special directories and can be at the start of a longer path ($HOME/subdir).  Reserved value $USER is replaced by the user’s username and can be anywhere in the path.  Relative entries are relative to the user’s home directory.</_description>
     </key>
     <key name="exclude-list" type="as">
       <default>[ '$TRASH', '$DOWNLOAD' ]</default>
       <_summary>Folders to ignore</_summary>
-      <_description>This list of directories will not be backed up.  Reserved values $HOME, $DESKTOP, $DOCUMENTS, $DOWNLOAD, $MUSIC, $PICTURES, $PUBLIC_SHARE, $TEMPLATES, $TRASH, and $VIDEOS are recognized as the user’s special directories.  Relative entries are relative to the user’s home directory.</_description>
+      <_description>This list of directories will not be backed up.  Reserved values $HOME, $DESKTOP, $DOCUMENTS, $DOWNLOAD, $MUSIC, $PICTURES, $PUBLIC_SHARE, $TEMPLATES, $TRASH, and $VIDEOS are recognized as the user’s special directories and can be at the start of a longer path ($HOME/subdir).  Reserved value $USER is replaced by the user’s username and can be anywhere in the path.  Relative entries are relative to the user’s home directory.</_description>
     </key>
     <key name="root-prompt" type="b">
       <default>true</default>

=== modified file 'libdeja/DirHandling.vala'
--- libdeja/DirHandling.vala	2013-01-27 21:50:59 +0000
+++ libdeja/DirHandling.vala	2017-02-12 02:37:21 +0000
@@ -29,30 +29,34 @@
 public string? parse_keywords(string dir)
 {
   string result = null;
-  if (dir == "$HOME")
-    result = Environment.get_home_dir();
-  else if (dir == "$DESKTOP")
-    result = Environment.get_user_special_dir(UserDirectory.DESKTOP);
-  else if (dir == "$DOCUMENTS")
-    result = Environment.get_user_special_dir(UserDirectory.DOCUMENTS);
-  else if (dir == "$DOWNLOAD")
-    result = Environment.get_user_special_dir(UserDirectory.DOWNLOAD);
-  else if (dir == "$MUSIC")
-    result = Environment.get_user_special_dir(UserDirectory.MUSIC);
-  else if (dir == "$PICTURES")
-    result = Environment.get_user_special_dir(UserDirectory.PICTURES);
-  else if (dir == "$PUBLIC_SHARE")
-    result = Environment.get_user_special_dir(UserDirectory.PUBLIC_SHARE);
-  else if (dir == "$TEMPLATES")
-    result = Environment.get_user_special_dir(UserDirectory.TEMPLATES);
-  else if (dir == "$TRASH")
-    result = get_trash_path();
-  else if (dir == "$VIDEOS")
-    result = Environment.get_user_special_dir(UserDirectory.VIDEOS);
+
+  // Replace special variables when they are at the start of a larger path
+  // The resulting string is an absolute path
+  if (dir.has_prefix("$HOME"))
+    result = dir.replace("$HOME", Environment.get_home_dir());
+  else if (dir.has_prefix("$DESKTOP"))
+    result = dir.replace("$DESKTOP", Environment.get_user_special_dir(UserDirectory.DESKTOP));
+  else if (dir.has_prefix("$DOCUMENTS"))
+    result = dir.replace("$DOCUMENTS", Environment.get_user_special_dir(UserDirectory.DOCUMENTS));
+  else if (dir.has_prefix("$DOWNLOAD"))
+    result = dir.replace("$DOWNLOAD", Environment.get_user_special_dir(UserDirectory.DOWNLOAD));
+  else if (dir.has_prefix("$MUSIC"))
+    result = dir.replace("$MUSIC", Environment.get_user_special_dir(UserDirectory.MUSIC));
+  else if (dir.has_prefix("$PICTURES"))
+    result = dir.replace("$PICTURES", Environment.get_user_special_dir(UserDirectory.PICTURES));
+  else if (dir.has_prefix("$PUBLIC_SHARE"))
+    result = dir.replace("$PUBLIC_SHARE", Environment.get_user_special_dir(UserDirectory.PUBLIC_SHARE));
+  else if (dir.has_prefix("$TEMPLATES"))
+    result = dir.replace("$TEMPLATES", Environment.get_user_special_dir(UserDirectory.TEMPLATES));
+  else if (dir.has_prefix("$TRASH"))
+    result = dir.replace("$TRASH", get_trash_path());
+  else if (dir.has_prefix("$VIDEOS"))
+    result = dir.replace("$VIDEOS", Environment.get_user_special_dir(UserDirectory.VIDEOS));
   else {
-    // Some variables can be placed inside a larger path, so replace those
+    // Some variables can be placed anywhere in the path
     result = dir.replace("$USER", Environment.get_user_name());
 
+    // Relative paths are relative to the user's home directory
     if (Uri.parse_scheme(result) == null && !Path.is_absolute(result))
       result = Path.build_filename(Environment.get_home_dir(), result);
   }