Bug #84491 for Dist-Zilla-Plugin-Git: [Git::GatherDir] does not handle symlinks, contrary to documentation

Sun Apr 07 15:35:57 2013 ether [...] cpan.org - Ticket created

Subject:

[Git::GatherDir] does not handle symlinks, contrary to documentation

The documentation claims to use the 'follow_symlinks' config variable (just like [GatherDir]), but the code wholly overrides the gather_files sub, and doesn't ever check this option. The presence of a symlink causes the build to die, because Dist::Zilla::File::OnDisk::content tries to read from the target of the symlink (a directory) and returns undef.

Sun Apr 07 16:10:13 2013 cjm [...] cpan.org - Correspondence added

You're correct, but the solution is not obvious. What exactly should follow_symlinks = 1 mean to Git::GatherDir? Should it assume that the symlinked directory is also a Git repo? I think the best solution is to remove the follow_symlinks option entirely, and just make Git::GatherDir skip symlinks and emit a warning. To suppress the warning, add the symlink to exclude_filename (or exclude_match). That's the same as the current behavior, except you get a warning instead of an error. So, you can use the exclude_filename trick without waiting for a new release. To gather the symlinked files, add another instance of GatherDir or Git::GatherDir with appropriate root & prefix options.

Sun Apr 07 16:10:14 2013 The RT System itself - Status changed from 'new' to 'open'

Sun Apr 07 16:47:22 2013 cjm [...] cpan.org - Correspondence added

On second thought, I think an error might be better than a warning. It's too easy to overlook warnings.

Mon Apr 08 13:27:36 2013 ether [...] cpan.org - Correspondence added

Subject:	Re: [rt.cpan.org #84491] [Git::GatherDir] does not handle symlinks, contrary to documentation
Date:	Mon, 8 Apr 2013 10:27:21 -0700
To:	"Christopher J. Madsen via RT" <bug-Dist-Zilla-Plugin-Git [...] rt.cpan.org>
From:	Karen Etheridge <ether [...] cpan.org>

On Sun, Apr 07, 2013 at 04:10:14PM -0400, Christopher J. Madsen via RT wrote: Show quoted text

> <URL: https://rt.cpan.org/Ticket/Display.html?id=84491 > > > You're correct, but the solution is not obvious. What exactly should follow_symlinks = 1 mean to Git::GatherDir? Should it assume that the symlinked directory is also a Git repo?

No, it should assume it's something it should gather, just like everything else, I think. (this is assuming it doesn't point to outside the repository itself, which would be a surprising and unexpected thing, and *should* fail.) Show quoted text

> I think the best solution is to remove the follow_symlinks option entirely, and just make Git::GatherDir skip symlinks and emit a warning. To suppress the warning, add the symlink to exclude_filename (or exclude_match). > > To gather the symlinked files, add another instance of GatherDir or Git::GatherDir with appropriate root & prefix options.

Why should it be necessary to use a separate gather plugin to get symlinks? git can see them, so we should be able to gather them in the same plugin, shouldn't we?

Mon Apr 08 14:10:36 2013 cjm [...] cpan.org - Correspondence added

On Mon, Apr 8, 2013 12:27:36 PM, ETHER wrote: Show quoted text

> On Sun, Apr 07, 2013 at 04:10:14PM -0400, Christopher J. Madsen via RT > wrote:

> > You're correct, but the solution is not obvious. What exactly

> should follow_symlinks = 1 mean to Git::GatherDir? Should it > assume that the symlinked directory is also a Git repo? > > No, it should assume it's something it should gather, just like > everything else, I think. (this is assuming it doesn't point to > outside the repository itself, which would be a surprising and > unexpected thing, and *should* fail.)

Git::GatherDir gathers the Git-tracked files. There are no Git-tracked files in a symlinked directory; only the link itself is tracked. We can't gather an empty directory; dzil only works with files. If you want it to gather the files in the symlinked directory, which files and how? If you want it to gather all files, we'd either have to copy the code from GatherDir, or somehow call the base class implementation of gather_dir on that directory. If you want it to gather only Git-tracked files in the symlinked directory, that'd be some non-trivial code to figure out what files those are.

Mon Apr 08 14:28:34 2013 ether [...] cpan.org - Correspondence added

Subject:	Re: [rt.cpan.org #84491] [Git::GatherDir] does not handle symlinks, contrary to documentation
Date:	Mon, 8 Apr 2013 11:28:19 -0700
To:	"Christopher J. Madsen via RT" <bug-Dist-Zilla-Plugin-Git [...] rt.cpan.org>
From:	Karen Etheridge <ether [...] cpan.org>

On Mon, Apr 08, 2013 at 02:10:37PM -0400, Christopher J. Madsen via RT wrote: Show quoted text

> <URL: https://rt.cpan.org/Ticket/Display.html?id=84491 > > > On Mon, Apr 8, 2013 12:27:36 PM, ETHER wrote:

> > On Sun, Apr 07, 2013 at 04:10:14PM -0400, Christopher J. Madsen via RT > > wrote:

> > > You're correct, but the solution is not obvious. What exactly

> > should follow_symlinks = 1 mean to Git::GatherDir? Should it > > assume that the symlinked directory is also a Git repo? > > > > No, it should assume it's something it should gather, just like > > everything else, I think. (this is assuming it doesn't point to > > outside the repository itself, which would be a surprising and > > unexpected thing, and *should* fail.)

> > Git::GatherDir gathers the Git-tracked files. There are no Git-tracked files in a symlinked directory; only the link itself is tracked. We can't gather an empty directory; dzil only works with files.

The symlink itself should be gathered. If the target of the symlink isn't under git control, then [Git::GatherDir] wouldn't pick it up; if it was, it would.

Mon Apr 08 14:50:51 2013 cjm [...] cpan.org - Correspondence added

On Mon, Apr 8, 2013 1:28:34 PM, ETHER wrote: Show quoted text

> The symlink itself should be gathered.

I don't know what you mean by that. Dzil doesn't deal with symlinks; they're not recommended for CPAN modules because Windows doesn't have them. It can only gather files.

Mon Apr 08 15:05:54 2013 ether [...] cpan.org - Correspondence added

Subject:	Re: [rt.cpan.org #84491] [Git::GatherDir] does not handle symlinks, contrary to documentation
Date:	Mon, 8 Apr 2013 12:05:34 -0700
To:	"Christopher J. Madsen via RT" <bug-Dist-Zilla-Plugin-Git [...] rt.cpan.org>
From:	Karen Etheridge <ether [...] cpan.org>

On Mon, Apr 08, 2013 at 02:50:52PM -0400, Christopher J. Madsen via RT wrote: Show quoted text

> > The symlink itself should be gathered.

> > I don't know what you mean by that. Dzil doesn't deal with symlinks; they're not recommended for CPAN modules because Windows doesn't have them. It can only gather files.

Oh! I see where the disconnect happened (all my fault) -- I was labouring under the mistaken conception that symlinks made it into the built distribution. I've been using symlinks all this time in a darkpan-constructed minting profile distribution (with a 'default' symlink pointing to one of the other profile directories), but just noticed that the wrapper bash script I've been using actually did specify a profile name, rather than using the default... because, of course, that default symlink never made it into the built dist. I thought it did! so my bad. In this light, I agree the right thing to do is to drop mention of the follow_symlinks option in the documentation, as it has no meaning here.

Sat Apr 13 19:23:09 2013 cjm [...] cpan.org - Fixed in 2.013 added

Sat Apr 13 19:23:09 2013 cjm [...] cpan.org - Correspondence added

I decided to just make it skip directory symlinks with a warning, and updated the documentation to explain that. Released to CPAN as 2.013.

Sat Apr 13 19:23:10 2013 cjm [...] cpan.org - Status changed from 'open' to 'resolved'