Bug #76703 for Dist-Zilla-Plugin-Git: [Git::NextVersion] should look only at tags on the current branch

Thu Apr 19 13:21:45 2012 ether [...] cpan.org - Ticket created

Subject:

[Git::NextVersion] should look only at tags on the current branch

When determining the next version to use, [Git::NextVersion] should only n++ from *the last tag on the current branch*, not the most recent tag in the entire repository. This allows one to follow a branching model like http://nvie.com/posts/a-successful-git-branching-model/ (or any, really; this is just a nice illustration) where one switches to a different branch to do particular releases. This is the scenario I just ran into: - my 'stable' branch has v1.000000 as its most recent release - 'master' has v1.000000 in its history, but has since also seen a v1.001000 and v1.002000 as "dev" releases - I needed to do a hotfix against v1.000000, so I switched to 'stable', cherry-picked my changes, and went to do a release, wanting a release version of v1.000001. Instead I got v1.002001, and had to manually override. - If the most recent tag on the current branch was used, I would have gotten v1.000001 as desired. rjbs suggests that this behaviour might be controllable with a config var; I agree, but IMHO the behaviour I described should be the default. Also, one would need to check that the selected version hasn't been duplicated by a release already - so all tags in the repository still need to be scanned, to check for such dupes. (probably in that case it would be fair to bail out and request the user manually override the version with V=, as it would be difficult to calculate the next version automatically.)

Wed Sep 12 23:35:05 2012 cjm [...] cpan.org - Taken

Wed Sep 12 23:44:11 2012 cjm [...] cpan.org - Correspondence added

I'm working on this at https://github.com/madsen/dist-zilla-plugin-git/tree/version-by-branch I didn't make it the default; you have to set "version_by_branch = 1". I agree that it would be nice to make it the default, but finding tags on a branch is a noticeably more expensive operation than just listing the tags. It can take several seconds (depending on your history and what's in your filesystem cache). So, I think it's better if only the people who actually need it have to pay for it. Any comments?

Wed Sep 12 23:44:12 2012 The RT System itself - Status changed from 'new' to 'open'

Wed Sep 12 23:44:12 2012 cjm [...] cpan.org - Status changed from 'open' to 'patched'

Wed Sep 12 23:48:16 2012 cjm [...] cpan.org - Correspondence added

Note: it does not currently check for duplicated versions. I'm not sure if that's necessary. Git::NextVersion isn't really practical without Git::Tag, and that will detect duplicate tags at release time. If you have Git::Tag include a trial release indicator, then it would be possible to have a stable release and a trial release with the same version number. (PAUSE wouldn't complain either, because it's the filename that has to be unique.) I'm not sure that's a significant problem, though.

Wed Sep 12 23:48:16 2012 The RT System itself - Status changed from 'patched' to 'open'

Mon Sep 17 18:06:33 2012 cjm [...] cpan.org - Correspondence added

I've pushed some updates to my version-by-branch branch. It now caches the results for the HEAD revision, so it only has to do the history search once per revision, instead of every time dzil runs. I've also added tests.

Thu Oct 04 00:23:49 2012 rsrchboy [...] cpan.org - Correspondence added

On Mon Sep 17 18:06:33 2012, CJM wrote: Show quoted text

> I've pushed some updates to my version-by-branch branch. It now caches > the results for the HEAD revision, so it only has to do the history > search once per revision, instead of every time dzil runs. I've also > added tests.

Unfortunately this has completely broken Dist::Zilla::Plugin::Git::CheckFor::Fixups :( I'm going to pull the appropriate code over, but we really ought to figure out a way to share this information in a way less... fragile.

Thu Oct 04 00:43:45 2012 cjm [...] cpan.org - Correspondence added

RT-Send-CC:

rsrchboy [...] cpan.org

On Wed, Oct 3, 2012 11:23:49 PM, RSRCHBOY wrote: Show quoted text

> Unfortunately this has completely broken > Dist::Zilla::Plugin::Git::CheckFor::Fixups

Sorry about that, but that's what happens when you rely on undocumented methods. I hadn't realized Git::CheckFor::Fixups used that, and even if I'd run its tests, it does't have any. I think the solution here is to put last_version back in Git::NextVersion and document it, so it won't go away again. So instead of fixing Git::CheckFor::Fixups, how about writing some tests for it so I can tell if I break it again?

Thu Oct 04 01:18:26 2012 cjm [...] cpan.org - Correspondence added

On Wed, Oct 3, 2012 11:43:45 PM, CJM wrote: Show quoted text

> I think the solution here is to put last_version back in > Git::NextVersion and document it, so it won't go away again.

Actually, now that I look more closely at what CheckFor::Fixups is doing, last_version isn't really even the information you want. The raw tag would be better. And I'm not sure CheckFor::Fixups even works properly if you have something more complicated than a linear history. I think maybe what I should do is restore last_version as an undocumented attribute for now, and then we can think about a better solution. Please try this patch and make sure it fixes CheckFor::Fixups. If it does, I'll release it.

Subject:

last_version.patch.txt

diff --git a/lib/Dist/Zilla/Plugin/Git/NextVersion.pm b/lib/Dist/Zilla/Plugin/Git/NextVersion.pm index d715632..f933e18 100644 --- a/lib/Dist/Zilla/Plugin/Git/NextVersion.pm +++ b/lib/Dist/Zilla/Plugin/Git/NextVersion.pm @@ -56,7 +56,16 @@ sub _max_version { return undef; } # end _max_version -sub _last_version { +# Dist::Zilla::Plugin::Git::CheckFor::Fixups uses last_version, but +# that's not really what it needs; the tag from the last version would +# be better. Please don't use this attribute. +# TODO: Come up with a better way to handle this. +has last_version => ( + is => 'ro', isa=>'Maybe[Str]', + init_arg => undef, lazy => 1, builder => '_build_last_version' +); + +sub _build_last_version { my ($self) = @_; my $last_ver; @@ -123,7 +132,7 @@ sub provide_version { # override (or maybe needed to initialize) return $ENV{V} if exists $ENV{V}; - my $last_ver = $self->_last_version; + my $last_ver = $self->last_version; return $self->first_version unless defined $last_ver;

Thu Oct 04 10:40:24 2012 rsrchboy [...] cpan.org - Correspondence added

On Thu Oct 04 01:18:26 2012, CJM wrote: Show quoted text

> On Wed, Oct 3, 2012 11:43:45 PM, CJM wrote: >

> > I think the solution here is to put last_version back in > > Git::NextVersion and document it, so it won't go away again.

> > Actually, now that I look more closely at what CheckFor::Fixups is > doing, last_version isn't really even the information you want. The raw > tag would be better. And I'm not sure CheckFor::Fixups even works > properly if you have something more complicated than a linear history.

I'd written the removed attribute to try to keep from ending up in a situation where chunks of code and configuration were duplicated between these and the checkfor plugins. It was a temporary measure, but worked well enough that I could let it be for a while. Unfortunately, depending on this elsewhere raised the bar a bit for changing it as well, so I'm OK with having the old code in CheckFor while we resolve this. Remember that 'A..B' in git ranges (essentially) means "all commits reachable from B except those reachable from A". The history doesn't need to be linear, but the previous tag must be reachable somehow. Show quoted text

> I think maybe what I should do is restore last_version as an > undocumented attribute for now, and then we can think about a better > solution. Please try this patch and make sure it fixes > CheckFor::Fixups. If it does, I'll release it.

I'd already released a new CheckFor last night, unfortunately, so I think it's ok there for the meantime... That'll also give me the flexibility to go for what I really need from the tag deduction code. This is useful information, however, and we'd probably be well served by either creating a stash to store this and other common information away in, or moving the tag/etc logic into, say, the repository role.

Thu Oct 04 12:27:52 2012 ether [...] cpan.org - Correspondence added

Subject:	Re: [rt.cpan.org #76703] [Git::NextVersion] should look only at tags on the current branch
Date:	Thu, 4 Oct 2012 09:27:40 -0700
To:	Chris Weyl via RT <bug-Dist-Zilla-Plugin-Git [...] rt.cpan.org>
From:	Karen Etheridge <ether [...] cpan.org>

On Thu, Oct 04, 2012 at 10:40:26AM -0400, Chris Weyl via RT wrote: Show quoted text

> This is useful information, however, and we'd probably be well served by > either creating a stash to store this and other common information away > in, or moving the tag/etc logic into, say, the repository role.

Yes, we really need to talk about formalizing a stash to be shared by multiple git plugins - for storing configs (e.g. tag format, branches to release from..) and the results of expensive calculations.

Thu Oct 04 13:23:01 2012 cjm [...] cpan.org - Correspondence added

RT-Send-CC:

rsrchboy [...] cpan.org

On Thu, Oct 4, 2012 11:27:52 AM, ETHER wrote: Show quoted text

> > Yes, we really need to talk about formalizing a stash to be shared by > multiple git plugins - for storing configs (e.g. tag format, branches to > release from..) and the results of expensive calculations.

I agree, but this bug isn't the place to do it. I've opened https://rt.cpan.org/Ticket/Display.html?id=80010 and am closing this bug. Please take the discussion there.

Thu Oct 04 13:23:23 2012 cjm [...] cpan.org - Status changed from 'open' to 'resolved'

Thu Oct 04 13:23:23 2012 cjm [...] cpan.org - Fixed in 1.122770 added