Automatically clean up book metadata (#976)

nik9000 · web-flow · commit 93abb123ae20 · 2019-07-10T09:21:28.000-04:00
We keep information about which branches contribute to which books but before this change we didn't automatically remove the metadata when a branch is no longer involved in building a book we never cleared that metadata. For the most part this just left lines in a YAML file that aren't used except when we remove a branch from a book and then re-add it. In that case since we only use the branch data to figure out if we should rebuild a book we *wouldn't* start to build the book when it is added back unless the branch that it is built from changes. This happened to us recently so it makes sense to clean up these files automatically so it never happens again. Closes #973
diff --git a/build_docs.pl b/build_docs.pl
@@ -220,7 +220,8 @@ sub build_all {
 
     say "Updating repositories";
     my $target_repo = init_target_repo( $repos_dir, $temp_dir, $reference_dir );
-    init_repos( $repos_dir, $temp_dir, $reference_dir, $target_repo );
+    my $tracker = init_repos(
+            $repos_dir, $temp_dir, $reference_dir, $target_repo );
 
     my $build_dir = $Conf->{paths}{build}
         or die "Missing <paths.build> in config";
@@ -259,6 +260,8 @@ sub build_all {
         say "Checking links";
         check_links($build_dir);
     }
+    $tracker->prune_out_of_date( @$contents );
+    $tracker->write;
     push_changes( $build_dir, $target_repo ) if $Opts->{push};
     serve_and_open_browser( $build_dir, $redirects ) if $Opts->{open};
 
@@ -553,7 +556,7 @@ sub init_repos {
         say "Removing old repo <" . $dir->basename . ">";
         $dir->rmtree;
     }
-    return $target_repo;
+    return $tracker;
 }
 
 
diff --git a/integtest/spec/all_books_change_detection_spec.rb b/integtest/spec/all_books_change_detection_spec.rb
@@ -285,6 +285,58 @@ def self.build_one_book_then_two_books(
           let(:new_text) { 'Some text.' }
           include_examples 'second build is not a noop'
         end
+        context 'because we add a branch to the book' do
+          build_one_book_out_of_one_repo_twice(
+            before_second_build: lambda do |src, _config|
+              repo = src.repo 'repo'
+              repo.switch_to_new_branch 'foo'
+              book = src.book 'Test'
+              book.branches.push 'foo'
+            end
+          )
+          let(:latest_revision) { 'init' }
+          let(:new_text) { 'Some text.' }
+          context 'the second build' do
+            let(:out) { outputs[1] }
+            include_examples 'commits changes'
+          end
+          file_context 'html/branches.yaml' do
+            it 'includes the original branch' do
+              expect(contents).to include('Test/index.asciidoc/master')
+            end
+            it 'includes the added branch' do
+              expect(contents).to include('Test/index.asciidoc/foo')
+            end
+          end
+        end
+        context 'because we remove a branch from the book' do
+          build_one_book_out_of_one_repo_twice(
+            before_first_build: lambda do |src, _config|
+              repo = src.repo 'repo'
+              repo.switch_to_new_branch 'foo'
+              book = src.book 'Test'
+              book.branches.push 'foo'
+            end,
+            before_second_build: lambda do |src, _config|
+              book = src.book 'Test'
+              book.branches.delete 'foo'
+            end
+          )
+          let(:latest_revision) { 'init' }
+          let(:new_text) { 'Some text.' }
+          context 'the second build' do
+            let(:out) { outputs[1] }
+            include_examples 'commits changes'
+          end
+          file_context 'html/branches.yaml' do
+            it 'includes the built branch' do
+              expect(contents).to include('Test/index.asciidoc/master')
+            end
+            it "doesn't include the removed branch" do
+              expect(contents).not_to include('Test/index.asciidoc/foo')
+            end
+          end
+        end
       end
     end
 
diff --git a/integtest/spec/helper/book.rb b/integtest/spec/helper/book.rb
@@ -12,12 +12,17 @@ class Book
   # Should this book build with asciidoctor (true) or asciidoc (false).
   attr_accessor :asciidoctor
 
+  ##
+  # The list of branches to build
+  attr_accessor :branches
+
   def initialize(title, prefix)
     @title = title
     @prefix = prefix
     @index = 'index.asciidoc'
     @asciidoctor = true
     @sources = []
+    @branches = ['master']
   end
 
   ##
@@ -47,7 +52,7 @@ def conf
       title:      #{@title}
       prefix:     #{@prefix}
       current:    master
-      branches:   [ master ]
+      branches:   [ #{@branches.join ', '} ]
       index:      #{@index}
       tags:       test tag
       subject:    Test
@@ -61,7 +66,9 @@ def conf
   # The html for a link to a particular branch of this book.
   def link_to(branch)
     url = "#{@prefix}/#{branch}/index.html"
-    %(<a class="ulink" href="#{url}" target="_top">#{@title}</a>)
+    decoration = ''
+    decoration = ' [master]' unless @branches.length == 1
+    %(<a class="ulink" href="#{url}" target="_top">#{@title}#{decoration}</a>)
   end
 
   private
diff --git a/lib/ES/BranchTracker.pm b/lib/ES/BranchTracker.pm
@@ -4,7 +4,7 @@ use strict;
 use warnings;
 use v5.10;
 
-use Path::Class();
+use Path::Class qw(dir);
 use ES::Util qw(run sha_for);
 use YAML qw(Dump Load);
 use Storable qw(dclone);
@@ -15,27 +15,19 @@ my %Repos;
 sub new {
 #===================================
     my ( $class, $file, @repos ) = @_;
-    my $old  = {};
-    my $yaml = '';
+    my %shas;
 
     if ( -e $file ) {
         my $yaml = $file->slurp( iomode => '<:utf8' );
-        $old = Load($yaml);
-    }
-
-    my %new;
-    for (@repos) {
-        $new{$_} = $old->{$_} || {};
+        %shas = %{ Load($yaml) };
     }
 
     my $self = bless {
         file => $file,
-        shas => \%new,
-        yaml => $yaml,
+        shas => \%shas,
     }, $class;
-    $self->write;
-    return $self;
 
+    return $self;
 }
 
 #===================================
@@ -56,39 +48,69 @@ sub sha_for_branch {
 sub set_sha_for_branch {
 #===================================
     my ( $self, $repo, $branch, $sha ) = @_;
+
     $self->shas->{$repo}{$branch} = $sha;
-    $self->write;
 }
 
 #===================================
-sub delete_branch {
+sub prune_out_of_date {
 #===================================
-    my ( $self, $repo, $branch ) = @_;
-    my $shas = $self->shas;
-    delete $shas->{$repo}{$branch} || return;
-    unless ( keys %{ $shas->{$repo} } ) {
-        delete $shas->{$repo};
+# Prunes tracker entries for books that are no longer built.
+#===================================
+    my ( $self, @entries ) = @_;
+    my %allowed;
+    _allowed_entries_from_books( \%allowed, @entries );
+
+    while ( my ($repo, $branches) = each %{ $self->{shas} } ) {
+        my $allowed_for_repo = $allowed{$repo} || '';
+        unless ($allowed_for_repo) {
+            delete $self->{shas}->{$repo};
+            next;
+        }
+        foreach my $branch ( keys %{ $branches } ) {
+            # We can't clear the link check information at this point safely
+            # because we need it for PR builds and we don't have a good way
+            # tell if it'll be needed again. It is a problem, but not a big one
+            # right now.
+            unless ($allowed_for_repo->{$branch} || $branch =~ /^link-check/) {
+                delete $branches->{$branch};
+            }
+        }
+        # Empty can show up because there is a new book that were not
+        # building at this time and we don't want that to force a commit so we
+        # clean them up while we're purging here.
+        delete $self->{shas}->{$repo} unless keys %{ $branches };
     }
-    $self->write;
 }
 
 #===================================
-sub write {
+sub _allowed_entries_from_books {
 #===================================
-    my $self = shift;
-    my $to_save = dclone( $self->shas );
-    # Empty hashes are caused by new repos that are unused which shouldn't
-    # force a commit.
-    while (my ($repo, $branches) = each %{ $to_save } ) {
-        unless ( keys %{ $branches } ) {
-            delete $to_save->{$repo};
+    my ( $allowed, @entries ) = @_;
+
+    foreach my $book ( @entries ) {
+        my $title = $book->{title};
+        foreach ( @{ $book->{branches} } ) {
+            my ( $branch, $branch_title ) = ref $_ eq 'HASH' ? (%$_) : ( $_, $_ );
+            foreach my $source ( @{ $book->{sources} } ) {
+                my $repo = $source->{repo};
+                my $path = dir('.')->subdir( $source->{path} )->relative('.');
+                my $mapped_branch = $source->{map_branches}{$branch} || $branch;
+                $allowed->{$repo}{"$title/$path/$mapped_branch"} = 1;
+            }
+        }
+        if (exists $book->{sections}) {
+            _allowed_entries_from_books( $allowed, @{ $book->{sections} } );
         }
     }
-    my $new  = Dump( $to_save );
-    return if $new eq $self->{yaml};
+}
+
+#===================================
+sub write {
+#===================================
+    my $self = shift;
     $self->file->parent->mkpath;
-    $self->file->spew( iomode => '>:utf8', $new );
-    $self->{yaml} = $new;
+    $self->file->spew( iomode => '>:utf8', Dump( $self->{shas} ) );
 }
 
 #===================================
