Bug #82373 for Git-Repository: Command silently filters string overloaded objects such as Path::Class.

Thu Jan 03 00:37:18 2013 mschwern [...] cpan.org - Ticket created

Subject:

Command silently filters string overloaded objects such as Path::Class.

use Path::Class;
use Git::Repository;

my $dir = dir("/tmp");
print Git::Repository->run( init => $dir ), "\n";
my $repo = Git::Repository->new( work_tree => $dir );
__END__
Reinitialized existing Git repository in /Users/schwern/src/Git-Repository/.git/
fatal: Not a git repository (or any of the parent directories): .git at - line 6.

Git::Repository::Command->new silently filters out all references from the list of commands. This means any string overloaded objects, such as Path::Class::Dir, will silently be thrown out. This makes using Path::Class with Git::Repository problematic and dangerous, without a work directory the git object might try to work in the current directory. I already had a project get a bogus commit because of this.

Possible solutions: remove the silent filter, make the filter throw an exception, make the filter check if the thing is an object and is string overloaded. Personally I'd just remove the filter.

Thu Jan 03 01:40:33 2013 mschwern [...] cpan.org - Correspondence added

See pull request at https://github.com/book/Git-Repository/pull/9

The feature tested in t/20-simple.t which ignores extra Git::Repository objects seems like a bad idea. Nothing but that test triggered that behavior. Should it not be an error?

Thu Jan 03 01:40:34 2013 mschwern [...] cpan.org - Status changed from 'new' to 'open'

Thu Jan 03 10:49:50 2013 book [...] cpan.org - Correspondence added

On Thu Jan 03 01:40:33 2013, MSCHWERN wrote: Show quoted text

> See pull request at https://github.com/book/Git-Repository/pull/9 > > The feature tested in t/20-simple.t which ignores extra > Git::Repository objects > seems like a bad idea. Nothing but that test triggered that behavior. > Should it > not be an error?

You mean die if we happen to have more than one Git::Repository object in @cmd? Probably. I suppose my idea at the time was to be able to build command-lines from a wide array of possibilities, including having more than one Git::Repository object passed. But if I really wanted to enable that, I should have picked the *last* such item in the list, instead of the first. So it does not seem of any use indeed.

Thu Jan 03 10:49:51 2013 book [...] cpan.org - Correspondence added

On Thu Jan 03 01:40:33 2013, MSCHWERN wrote: Show quoted text

> See pull request at https://github.com/book/Git-Repository/pull/9 > > The feature tested in t/20-simple.t which ignores extra > Git::Repository objects > seems like a bad idea. Nothing but that test triggered that behavior. > Should it > not be an error?

You mean die if we happen to have more than one Git::Repository object in @cmd? Probably. I suppose my idea at the time was to be able to build command-lines from a wide array of possibilities, including having more than one Git::Repository object passed. But if I really wanted to enable that, I should have picked the *last* such item in the list, instead of the first. So it does not seem of any use indeed.

Thu Jan 03 19:10:48 2013 schwern [...] pobox.com - Correspondence added

Subject:	Re: [rt.cpan.org #82373] Command silently filters string overloaded objects such as Path::Class.
Date:	Thu, 03 Jan 2013 16:10:38 -0800
To:	bug-Git-Repository [...] rt.cpan.org
From:	"Michael G. Schwern" <schwern [...] pobox.com>

On 1/3/13 7:49 AM, Philippe 'BooK' Bruhat via RT wrote: Show quoted text

> You mean die if we happen to have more than one Git::Repository object > in @cmd? Probably. > > I suppose my idea at the time was to be able to build command-lines > from a wide array of possibilities, including having more than one > Git::Repository object passed. But if I really wanted to enable that, > I should have picked the *last* such item in the list, instead of > the first. > > So it does not seem of any use indeed.

The nice thing about making something a half-thought out idea an exception is you can always make it *not* an exception later. The other way is hard. This feature appears to be ill documented and tested, so doubt there will be harm in making it an exception. I'll happily tear it out. Shall I?

Thu Jan 03 20:14:26 2013 book [...] cpan.org - Correspondence added

Subject:	Re: [rt.cpan.org #82373] Command silently filters string overloaded objects such as Path::Class.
Date:	Fri, 4 Jan 2013 02:14:15 +0100
To:	Michael G Schwern via RT <bug-Git-Repository [...] rt.cpan.org>
From:	"Philippe Bruhat (BooK)" <book [...] cpan.org>

On Thu, Jan 03, 2013 at 07:10:49PM -0500, Michael G Schwern via RT wrote: Show quoted text

> Queue: Git-Repository > Ticket <URL: https://rt.cpan.org/Ticket/Display.html?id=82373 > > > On 1/3/13 7:49 AM, Philippe 'BooK' Bruhat via RT wrote:

> > You mean die if we happen to have more than one Git::Repository object > > in @cmd? Probably. > > > > I suppose my idea at the time was to be able to build command-lines > > from a wide array of possibilities, including having more than one > > Git::Repository object passed. But if I really wanted to enable that, > > I should have picked the *last* such item in the list, instead of > > the first. > > > > So it does not seem of any use indeed.

> > The nice thing about making something a half-thought out idea an > exception is you can always make it *not* an exception later. The other > way is hard. > > This feature appears to be ill documented and tested, so doubt there > will be harm in making it an exception. I'll happily tear it out. > > Shall I? >

I already removed it and another similarly half-thought default, and pushed the commits. Thanks for making me think these through. :-) -- Philippe Bruhat (BooK) Law is the best deterrent to Justice. (Moral from Groo The Wanderer #90 (Epic))

Tue Jan 08 03:20:52 2013 book [...] cpan.org - Status changed from 'open' to 'resolved'

Tue Jan 08 03:20:53 2013 book [...] cpan.org - Fixed in 1.300 added