Bug #87172 for Module-Rename: Request to handle 'git mv' in Module::Rename

Mon Jul 22 23:51:17 2013 jim.cochrane [...] gmail.com - Ticket created

Subject:	Request to handle 'git mv' in Module::Rename
Date:	Mon, 22 Jul 2013 22:51:05 -0500
To:	bug-Module-Rename [...] rt.cpan.org
From:	Jim Cochrane <jim.cochrane [...] gmail.com>

Hi. What I'm reporting here is not really a bug, but a feature request. I didn't find a path to report a feature request - apologies if I missed it. What I'm requesting is an extension to Module::Rename to use 'git-mv' instead of 'mv' to perform the file moves, so that git can be kept in the loop, thus saving extra admin work. I actually grabbed the code from github and made a small change, which worked fine in a simple test case. I can submit this modification to you if you like - just let me know how you prefer I do that - e.g., pull request on github or whatever. I see you've not made any changes to this module for a few years, but (I find) it's a very useful module/script and since the change is small and (IMO) useful, hopefully you'll find it practical and worthwhile to incorporate it. (I'm aware that you may need to add further changes or throw away my changes and implement your own - that's fine.) Thanks! Jim Cochrane

Sat Jul 27 07:35:43 2013 jim.cochrane [...] gmail.com - Correspondence added

From:

jim.cochrane [...] gmail.com

I've not gotten a response yet, so I put my modified version on github: https://github.com/jjttcc/module-rename It essentially used the 'mv' command by default (as the original does) and adds a -g option to do a 'git mv' instead.

Sat Jul 27 23:16:50 2013 MSCHILLI [...] cpan.org - Correspondence added

On Sat Jul 27 07:35:43 2013, jim.cochrane@gmail.com wrote: Show quoted text

> I've not gotten a response yet, so I put my modified version > on github:

Sorry for the delay. I've applied most of your changes: https://github.com/mschilli/module-rename-perl/commit/005014267394e9dc993ea9c73829d6d5b13bd27d And added a couple of fixes: https://github.com/mschilli/module-rename-perl/commit/aae967e6408a217e0eaaee41c1a607d2a4c06c70 And released 0.04 to CPAN. Going forward, if you want to make it easy for me apply your changes, please do the following: * press the fork button on github * apply your changes to the fork * send me a pull request Also, don't forget to add test cases, they keep the bugs out! Thanks for your contribution!

Sat Jul 27 23:16:50 2013 The RT System itself - Status changed from 'new' to 'open'

Sat Jul 27 23:16:50 2013 MSCHILLI [...] cpan.org - Status changed from 'open' to 'resolved'

Tue Jul 30 16:56:01 2013 jim.cochrane [...] gmail.com - Correspondence added

Subject:	Thanks, and found new bug [Re: [rt.cpan.org #87172] Request to handle 'git mv' in Module::Rename]
Date:	Tue, 30 Jul 2013 15:55:48 -0500
To:	bug-Module-Rename [...] rt.cpan.org
From:	Jim Cochrane <jim.cochrane [...] gmail.com>

Hi - sorry for the late reply, and thanks very much for your fixes and notification. I've found (what I consider to be) a bug (which is present in both the old and new versions of module-rename) in how the old-name pattern is used to determine which modules to rename - a partial match is accepted, so that, for example, module-rename Bar::Foo Bar::Boo would consider Bar::Food a match and apply the change on it. If you'd like a simple concrete example of how this produces unwanted behavior, let me know. I've found that adding one line in Rename.pm fixes the problem, at least for the simple test case I'm using: --- Rename.pm.orig 2013-07-27 21:59:50.000000000 -0500 +++ Rename.pm 2013-07-29 16:13:53.440512721 -0500 @@ -36,6 +36,7 @@ push @{ $self->{dir_exclude} }, ".git"; } + $self->{name_old} = '\b' . $self->{name_old} . '\b'; $self->{dir_exclude_hash} = { map { $_ => 1 } @{$self->{dir_exclude}} }; $self->{dir_ignore_hash} = { map { $_ => 1 } @{$self->{dir_ignore}} }; If you agree with this solution, let me know if you'd like me to follow the fork/pull-request procedure on github for this change. Thanks! Jim On Sat, Jul 27, 2013 at 10:16 PM, Michael_Schilli via RT < bug-Module-Rename@rt.cpan.org> wrote: Show quoted text

> <URL: https://rt.cpan.org/Ticket/Display.html?id=87172 > > > On Sat Jul 27 07:35:43 2013, jim.cochrane@gmail.com wrote:

> > I've not gotten a response yet, so I put my modified version > > on github:

> > Sorry for the delay. > > I've applied most of your changes: > > > https://github.com/mschilli/module-rename-perl/commit/005014267394e9dc993ea9c73829d6d5b13bd27d > > And added a couple of fixes: > > > https://github.com/mschilli/module-rename-perl/commit/aae967e6408a217e0eaaee41c1a607d2a4c06c70 > > And released 0.04 to CPAN. > > Going forward, if you want to make it easy for me apply your changes, > please do the following: > > * press the fork button on github > * apply your changes to the fork > * send me a pull request > > Also, don't forget to add test cases, they keep the bugs out! > > Thanks for your contribution! > > > > > > > >

Wed Jul 31 02:22:41 2013 MSCHILLI [...] cpan.org - Correspondence added

On Tue Jul 30 16:56:01 2013, jim.cochrane@gmail.com wrote: Show quoted text

> determine which modules to rename - a partial match is accepted, so > that, > for example, module-rename Bar::Foo Bar::Boo would > consider Bar::Food

Interesting, are you referring to file names or file content? If the latter, it seems that s/($self->{name_old})\b... should take care of it, no?

Wed Jul 31 17:13:24 2013 jim.cochrane [...] gmail.com - Correspondence added

Subject:	Re: [rt.cpan.org #87172] Request to handle 'git mv' in Module::Rename
Date:	Wed, 31 Jul 2013 16:13:08 -0500
To:	bug-Module-Rename [...] rt.cpan.org
From:	Jim Cochrane <jim.cochrane [...] gmail.com>

The content is OK; the file/directory names are what get messed up. I think what happens is that the first search - for file names that match the old pattern - finds more than it should; but when \b is applied in file_process, no overly-optimistic matches are found, so the bug does not occur when changing file contents. But the bug does occur when figuring out what 'git mv' commands (and, probably, when -g is not specified, what 'mv' commands) to generate. I'm attaching a small example, with original git repo, cloned repo with buggy version applied, and cloned repo with non-buggy version (my fix) applied, which I think gives a simple demo of the problem. Let me know if anything's unclear or you have questions. If the mail server we're using doesn't allow attachments, please let me know the best way to get this tar file to you. Thanks. On Wed, Jul 31, 2013 at 1:22 AM, Michael_Schilli via RT < bug-Module-Rename@rt.cpan.org> wrote: Show quoted text

> <URL: https://rt.cpan.org/Ticket/Display.html?id=87172 > > > On Tue Jul 30 16:56:01 2013, jim.cochrane@gmail.com wrote:

> > determine which modules to rename - a partial match is accepted, so > > that, > > for example, module-rename Bar::Foo Bar::Boo would > > consider Bar::Food

> > Interesting, are you referring to file names or file content? If the > latter, it seems that > > s/($self->{name_old})\b... > > should take care of it, no? > > >

Download namebug.tar
application/x-tar 180k

Message body not shown because it is not plain text.

Fri Aug 02 08:54:59 2013 jim.cochrane [...] gmail.com - Correspondence added

Subject:	Re: [rt.cpan.org #87172] Request to handle 'git mv' in Module::Rename
Date:	Fri, 2 Aug 2013 07:54:45 -0500
To:	bug-Module-Rename [...] rt.cpan.org
From:	Jim Cochrane <jim.cochrane [...] gmail.com>

Hi Michael. I've not heard back and I'm not sure if my last message got to you - I included an attachment and I suspect the message may have been discarded because your mail server does not allow attachments. So I'm sending the same message again (below) minus the attachment. Let me know if you want me to send you the file I tried to attach (tar file with evidence of the bug) by some other means. Also, here is a listing of the *.pm files in the tar file I sent, from which I think the problem is evident: ./renamed_without_bug/lib/NowOrLater/Core/Tasks/TaskState.pm ./renamed_without_bug/lib/NowOrLater/Core/Tasks/Task.pm ./renamed_without_bug/lib/NowOrLater.pm ./renamed_with_bug/lib/NowOrLater/Core/Tasks/Task.pm ./renamed_with_bug/lib/NowOrLater/Core/Tasks/Tasks/TaskState.pm ./renamed_with_bug/lib/NowOrLater.pm ./orig/lib/NowOrLater/Core/TaskState.pm ./orig/lib/NowOrLater/Core/Task.pm ./orig/lib/NowOrLater.pm Evidence of the bug shows up in the listing: ./renamed_with_bug/lib/NowOrLater/Core/Tasks/Tasks/TaskState.pm Without the bug, it should be: ./renamed_with_bug/lib/NowOrLater/Core/Tasks/TaskState.pm Two rename commands were run: module-rename -g NowOrLater::Core::TaskState NowOrLater::Core::Tasks::TaskState lib/ module-rename -g NowOrLater::Core::Task NowOrLater::Core::Tasks::Task lib/ As always, let me know if you have further questions. Thanks! On Wed, Jul 31, 2013 at 4:13 PM, Jim Cochrane <jim.cochrane@gmail.com>wrote: Show quoted text

> The content is OK; the file/directory names are what get messed up. I > think what happens is that the first search - for file names that match the > old pattern - finds more than it should; but when \b is applied in > file_process, no overly-optimistic matches are found, so the bug does not > occur when changing file contents. But the bug does occur when figuring > out what 'git mv' commands (and, probably, when -g is not specified, what > 'mv' commands) to generate. > > I'm attaching a small example, with original git repo, cloned repo with > buggy version applied, and cloned repo with non-buggy version (my fix) > applied, which I think gives a simple demo of the problem. Let me know if > anything's unclear or you have questions. > > If the mail server we're using doesn't allow attachments, please let me > know the best way to get this tar file to you. > > Thanks. > > > > On Wed, Jul 31, 2013 at 1:22 AM, Michael_Schilli via RT < > bug-Module-Rename@rt.cpan.org> wrote: >

>> <URL: https://rt.cpan.org/Ticket/Display.html?id=87172 > >> >> On Tue Jul 30 16:56:01 2013, jim.cochrane@gmail.com wrote:

>> > determine which modules to rename - a partial match is accepted, so >> > that, >> > for example, module-rename Bar::Foo Bar::Boo would >> > consider Bar::Food

>> >> Interesting, are you referring to file names or file content? If the >> latter, it seems that >> >> s/($self->{name_old})\b... >> >> should take care of it, no? >> >> >>

Sat Aug 10 23:31:34 2013 MSCHILLI [...] cpan.org - Correspondence added

Hi Jim, sorry again for the delay. I only work on my modules in my spare time, and there's been very little of that lately :). I've looked over your patch, but I think the \b...\b regex might actually trigger some other bugs (and the test suite broke with it in my env), so I went ahead and created a test case for the bug you reported: https://github.com/mschilli/module-rename-perl/commit/4b3d1c1801036afe49ca4dff4ab67a27b8559298 And a fix for it: https://github.com/mschilli/module-rename-perl/commit/b6a7d38daa6f5187e540c7baaec022843003fd22 Let me know if that works for you as well! -- Mike

Mon Aug 19 03:44:10 2013 jim.cochrane [...] gmail.com - Correspondence added

Subject:	Re: [rt.cpan.org #87172] Request to handle 'git mv' in Module::Rename
Date:	Mon, 19 Aug 2013 02:43:52 -0500
To:	bug-Module-Rename [...] rt.cpan.org
From:	Jim Cochrane <jim.cochrane [...] gmail.com>

Hi Mike. No problem with the delay - I had assumed you were busy with your day job that you actually get paid for. :-) Thanks for the fix and notification - and apologies for responding so late. It took me a while, but I finally got around to testing the new version and I found a couple fairly minor (but still important, IMO) problems. I think the basic logic of your changes is fine, but: - You appear to be missing 'use Cwd;' in Rename.pm (causes undefined subroutine error [getcwd] for my test). - The line 'INFO "mv $file $newfile"' causes a redundant report when the trial_run option is enabled, and with -g, the report of 'mv xxx yyy' and 'git mv xxx yyy' can cause confusion for the user (e.g., "What? It would have both done a 'mv' and a 'git mv' - that's no good!). (I think that line wasn't triggered before, but I think your change of 'level' to INFO put it back in. That's fine, I think, but I think this line should not be invoked when -t is used. And to be really picky, with -g [no -t], perhaps the output should be "git mv xxx yyy".) I'll attach a patch with my suggested fixes for this. Also, a couple even more minor issues (but I'm hoping they can be addressed if it's practical to do so): - commands ('mv' and 'git mv') are output to stderr, but diffs are output to stdout. (Can cause problems when redirecting output to a file - and if >2&1 is used, you might get other, unwanted, error messages in the file.) - The output "tapping git mv File1.pm A/B/File1.pm (skipped - dry run)" is more verbose than necessary, IMO. Better would be: git mv File1.pm A/B/File1.pm Since the user knows he typed module-rename -t ..., he knows it's a trial (dry) run and doesn't need to be told that; and "tapping" refers to the implementation, which the user doesn't care about. I suspect there might not a practical way to make this change with Sysadm::Install, so I'll leave it up to you whether this is doable. I have a little more testing to do - I'll let you know if I uncover anything else. Thanks! Jim On Sat, Aug 10, 2013 at 10:31 PM, Michael_Schilli via RT < bug-Module-Rename@rt.cpan.org> wrote: Show quoted text

> ...

Download renpatch
application/octet-stream 694b

Message body not shown because it is not plain text.

Mon Aug 19 22:58:39 2013 MSCHILLI [...] cpan.org - Correspondence added

On Mon Aug 19 03:44:10 2013, jim.cochrane@gmail.com wrote: Show quoted text

> - You appear to be missing 'use Cwd;' in Rename.pm (causes undefined > subroutine error [getcwd] for my test).

Good catch, I've added a test case for that. Show quoted text

> - The line 'INFO "mv $file $newfile"' causes a redundant report

Yep, but I think we should just delete it entirely, because in case of a real run, Sysadm::Install's mv() is going to do the logging. Show quoted text

> - commands ('mv' and 'git mv') are output to stderr, but diffs are > output to stdout.

Yep, I've changed the print commands to Log4perl statements, now everything goes to STDERR (or STDOUT, whatever you configure via Log4perl). Show quoted text

> - The output "tapping git mv File1.pm A/B/File1.pm (skipped - dry run)" > is more verbose than necessary, IMO. Better would be: > git mv File1.pm A/B/File1.pm > Since the user knows he typed module-rename -t

Actually, I like the extra confirmation. I know I typed -t, but do I know that the script is doing the right thing? The "skipped" message lets me sleep calmly at night :). Here's (hopefully) all of the above: https://github.com/mschilli/module-rename-perl/commit/a04a0e7e394f38b6c8529a09bcaa35ddc8a46e97 Would be great if you could run your tests on it one more time, thanks! -- Mike

Mon Aug 19 23:09:23 2013 jim.cochrane [...] gmail.com - Correspondence added

Subject:	Re: [rt.cpan.org #87172] Request to handle 'git mv' in Module::Rename
Date:	Mon, 19 Aug 2013 22:09:10 -0500
To:	bug-Module-Rename [...] rt.cpan.org
From:	Jim Cochrane <jim.cochrane [...] gmail.com>

On Mon, Aug 19, 2013 at 9:58 PM, Michael_Schilli via RT <bug-Module-Rename@rt.cpan.org> wrote: Show quoted text

> > <URL: https://rt.cpan.org/Ticket/Display.html?id=87172 > > > On Mon Aug 19 03:44:10 2013, jim.cochrane@gmail.com wrote: >

> > - You appear to be missing 'use Cwd;' in Rename.pm (causes undefined > > subroutine error [getcwd] for my test).

> > Good catch, I've added a test case for that. >

> > - The line 'INFO "mv $file $newfile"' causes a redundant report

> > Yep, but I think we should just delete it entirely, because in case of a real run, Sysadm::Install's mv() is going to do the logging.

That's fine with me. Show quoted text

> >

> > - commands ('mv' and 'git mv') are output to stderr, but diffs are > > output to stdout.

> > Yep, I've changed the print commands to Log4perl statements, now everything goes to STDERR (or STDOUT, whatever you configure via Log4perl). >

Good. Thanks. Show quoted text

> > - The output "tapping git mv File1.pm A/B/File1.pm (skipped - dry run)" > > is more verbose than necessary, IMO. Better would be: > > git mv File1.pm A/B/File1.pm > > Since the user knows he typed module-rename -t

> > Actually, I like the extra confirmation. I know I typed -t, but do I know that the script is doing the right thing? The "skipped" message lets me sleep calmly at night :). >

OK, I'll accept that. Show quoted text

> > Here's (hopefully) all of the above: > > https://github.com/mschilli/module-rename-perl/commit/a04a0e7e394f38b6c8529a09bcaa35ddc8a46e97 > > Would be great if you could run your tests on it one more time, thanks! > > -- Mike

I'll try to get to it tonight and let you know the result. Jim