Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

More accurate changed path skipping. Fixes #3544240 #2

Closed
wants to merge 9 commits into from

Conversation

wmbaum
Copy link
Contributor

@wmbaum wmbaum commented Jul 19, 2012

As described in sourceforge bug #3544240, the "Skip this entry?" block in log.c was not skipping changed paths that started with the prefix but were actually in a different path in the repository. Additions and modifications were getting filtered out later in the process, but deletes were coming through, making the dumps invalid and unloadable.

A delete to /foobar/something would come through as if it had been a delete to /foo/bar/something in a dump of /foo/.

Revision argument parsing for HEAD is now case insensitive as is common in svn tools.

Tweaks for building on windows against current win32svn (1.7.5).

@jgehring
Copy link
Owner

William,

Thanks for contributing and fixing this bug! I tested your changes with a custom test script and everything is working fine.

It is great that you fixed some other things as well! However, I'd prefer having separate pull requests for them (the actual bugfix, the HEAD argument parsing and the Windows tweaks). Could you please prepare branches for the respective features that include the relevant commits?

Additionally, I've added comments for some minor things to the diff of the pull request.

Thanks,
Jonas

@@ -68,6 +68,7 @@ static svn_error_t *log_receiver(void *baton, apr_hash_t *changed_paths, svn_rev
{
apr_hash_index_t *hi;
log_receiver_baton_t *data = (log_receiver_baton_t *)baton;
unsigned short int prefixlen = (unsigned short int)strlen(data->session->prefix);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use size_t instead of unsigned short int here. There's no difference in performance, and trading a few bytes for clarity is ok.

Replace stricmp() with more portable strcasecmp().

* src/main.c
  parse_revnum()
…ctually in the same path. Don't confuse /foobar/ with /foo/

* src/log.c
  log_receiver(): If the key matches the prefix, skip it if the following character isn't a path separator ('/').  Store length of prefix for repeated use.
* src/log.c
  log_receiver(): Change prefixlen to size_t. Avoid reference past end of string (key).
@wmbaum
Copy link
Contributor Author

wmbaum commented Jul 21, 2012

Hi Jonas,

First, thanks for getting back to me so quickly. Sorry I wasn't able to reciprocate..

I created three distinct branches, but then I had the brilliant idea to rebase two of the branches so they started after the windows tweaks (so they would build locally). This had the undesirable effect of having the windows_tweaks showing up in all 3 pull requests. As soon as I can figure out how to get proper looking pull requests, I'll fire them off to you.

--Bill

@wmbaum
Copy link
Contributor Author

wmbaum commented Jul 22, 2012

Hi Jonas, I've incorporated your suggestions and split these changes into three branches and created three new pull requests. Please let me know if anything isn't as expected. I'm guessing the etiquette is for me to close this one. (?)

@wmbaum wmbaum closed this Jul 22, 2012
@jgehring
Copy link
Owner

Thanks for your extra work and for a smooth merge!

@wmbaum
Copy link
Contributor Author

wmbaum commented Jul 23, 2012

My pleasure. And thanks for your help. This was my first experience with github, pull requests, etc..

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants