Fix Path Replacements

[Vampire]

Path replacements did not work properly

• if --empty-dirs is used, old files were not cleaned out but survived
• if --svn-ignore is used and the path_from has svn:ignore, then the target path did not get a .gitignore file

This fixes #43 and is an alternative approach to #62 which tries to solve the first point, but at an earlier --empty-dirs-independent place.
I think this fix is the better one as it fixes the actually broken --empty-dirs implementation.

[kfsone]

Suggested change

	else if (is_dir && (change->change_kind == svn_fs_path_change_add || change->change_kind == svn_fs_path_change_replace || change->change_kind == svn_fs_path_change_modify)
	else if (is_dir && (change->change_kind == svn_fs_path_change_add || change->change_kind == svn_fs_path_change_modify || change->change_kind == svn_fs_path_change_replace)

[kfsone]

I'm not clear on how this patch set fixes #43 though

[Vampire]

Why the suggested change?
Just because modify happens more likely than replace or is there a different reason?

About how it fixes #43, as indicated by the fact that the bug only happens when --empty-dirs is used, the bug is in the implementation of that option, not in the normal implementation which you fixed, but actually works fine.

If you look at the three commits individually it is maybe easier to grasp.

commit 1:

• addGitIgnore without content as done in line 924 tests whether the directory is empty
• if the directory is not empty it returns with EXIT_FAILURE
• if the directory is empty it adds an empty .gitignore (that's what --empty-dirs should do) and return EXIT_SUCCESS

So in my use-case and also in the use-case of #43 which essentially is the same, the directory was not empty and, so EXIT_FAILURE was returned and thus ignoreSet was set to true, which then causes a few lines down the deleteFile not to be done, which causes the files to be left-over.

commit 2:
After fixing the problem in #43 with commit 1, I observed that you still have a problem if the directory actually is empty thus an empty .gitignore is generated and EXIT_SUCCESS returned, because then old line 925 returned from the method.
As addGitIgnore only returns EXIT_SUCCESS when there are no files in the folder, it should be save to assume that no further work is necessary and the method can be left, but this also omits the deleteFile call and thus in that case again all files would be left over and just an empty .gitignore added.
But as now with the removed else branch the deletion was not dependent on the result of the addGitIgnore I was able to simply move the deleteFile before it, so that the delete is always done, then the generation of the empty .gitignore if necessary and then the recursive dump if necessary.

commit 3:
This finally is not directly related to #43 anymore, just during my tests and code-reading I have observed that the porting over of svn:ignore properties with the --svn-ignore switch is also faulty. It did not port over the svn:ignore value when doing a replace, so I added the change->change_kind == svn_fs_path_change_replace to the respective if-clauses, which then caused the same problem as #43 had, namely that if a .gitignore file was generated, ignoreSet was set to true which prevented the delete and thus had left-over files. As long as only prop_mod and add were considered this was not a problem as there was nothing to delete. But with replace added, the same problem was present, so I moved the deleteFile before the --svn-ignore if and ignoreSet is only used to check whether to try to add and empty .gitignore for --empty-dirs.

[kfsone]

The change was just to maintain the original ordering -- there's no functional impact above -O1 https://gcc.godbolt.org/z/OQGNiY

Commit #1 looks like it was the one that foiled me, it didn't step thru that when I was running it thru ddd.

[Vampire]

I don't know where exactly you mean it didn't step through.
Do you mean it didn't step through the else?
I would really wonder if it didn't, but I don't know for sure, as I didn't use a debugger, just some debug output lines and at first I also fixed the wrong location until I found that it depends on the --empty-dirs switch which made me to start over.

[kfsone]

On 10/22/2018 4:01 PM, Björn Kautler wrote: ***@***.**** commented on this pull request. ------------------------------------------------------------------------ In src/svn.cpp <#63 (comment)>: > @@ -640,7 +640,7 @@ int SvnRevision::exportEntry(const char *key, const svn_fs_path_change2_t *chang //qDebug() << "Adding directory:" << key; } // svn:ignore-properties - else if (is_dir && (change->change_kind == svn_fs_path_change_add || change->change_kind == svn_fs_path_change_modify) + else if (is_dir && (change->change_kind == svn_fs_path_change_add || change->change_kind == svn_fs_path_change_replace || change->change_kind == svn_fs_path_change_modify) I don't know where exactly you mean it didn't step through. Do you mean it didn't step through the |else|? I would really wonder if it didn't, but I don't know for sure, as I didn't use a debugger, just some debug output lines and at first I also fixed the wrong location until I found that it depends on the |--empty-dirs| switch which made me to start over.

After reading through the code fairly quickly, I ran through the debugger on a small test repo to see if it did the things I expected it to, but it didn't go anywhere near the --empty-dirs code, and that could either be because of optimization, clumsiness with gdb (I've had to do more in lldb/xcode/msvc the last year) or missing passing it the --empty-dirs. But you've resolved my confusion already

…

-Oliver