Only push to new target branches if there changes (#899)

Before this commit if you add `--target_branch` and the branch is *new*
then we pushed to it regardless of if there were changes. This felt nice
and consistent but we're going to end up with a lot of branches that
way.

Author: nik9000

build_docs.pl

@@ -555,20 +555,11 @@ sub push_changes {
     $build_dir->file('revision.txt')
         ->spew( iomode => '>:utf8', ES::Repo->all_repo_branches );
 
-    # Check if there are changes before committing and save that because if
-    # there are changes we'll commit them. Once we've committed them checking
-    # if there are changes will say "no" because there aren't changes
-    # *any more*. Thus we build $has_changes and $should_push here and not
-    # in the `if` statements below.
-    my $has_changes = $target_repo->outstanding_changes;
-    my $should_push = $has_changes || $target_repo->new_branch;
-    if ( $has_changes ) {
+    if ( $target_repo->outstanding_changes ) {
         say 'Preparing commit';
         build_sitemap($build_dir);
         say "Commiting changes";
         $target_repo->commit;
-    }
-    if ( $should_push ) {
         say "Pushing changes";
         $target_repo->push_changes;
     } else {

integtest/spec/all_books_change_detection_spec.rb

@@ -6,9 +6,11 @@
 RSpec.describe 'building all books' do
   class Config
     attr_accessor :target_branch
+    attr_accessor :checkout_branch
 
     def initialize
       @target_branch = nil
+      @checkout_branch = nil
     end
   end
   describe 'change detection' do
@@ -31,7 +33,8 @@ def self.build_twice(
         dest.convert_all src.conf, target_branch: config.target_branch
 
         # Checkout the files so we can assert about them.
-        dest.checkout_conversion branch: config.target_branch
+        checkout = config.checkout_branch || config.target_branch
+        dest.checkout_conversion branch: checkout
       end
       include_context 'build one book twice'
     end
@@ -129,26 +132,13 @@ def self.build_one_book_out_of_two_repos_twice(
           include_examples 'second build is noop'
         end
         context 'even when there is a new target branch' do
-          # Adding a new target branch will cause us to fork it from the
-          # master branch which so we won't have to rebuild the book *but*
-          # we push anyway so the new target branch is available.
           build_one_book_out_of_one_repo_twice(
             before_second_build: lambda do |_src, config|
               config.target_branch = 'new_target'
+              config.checkout_branch = 'master'
             end
           )
-          context 'the second build' do
-            let(:out) { outputs[1] }
-            it "doesn't print that it is building any books" do
-              expect(out).not_to include(': Building ')
-            end
-            it "doesn't print that it is commiting changes" do
-              expect(out).not_to include('Commiting changes')
-            end
-            it 'prints that it is pushing changes' do
-              expect(out).to include('Pushing changes')
-            end
-          end
+          include_examples 'second build is noop'
         end
       end
       context "when the second build isn't a noop" do

lib/ES/TargetRepo.pm

@@ -75,7 +75,6 @@ sub checkout_minimal {
         printf(" - %20s: Forking <%s> from master\n",
             'target_repo', $self->{branch});
         run qw(git checkout -b), $self->{branch};
-        $self->{new_branch} = 1;
         1;
     } or die "Error checking out repo <target_repo>: $@";
     chdir $original_pwd;
@@ -140,13 +139,6 @@ sub push_changes {
     run qw(git push origin), $self->{branch};
 }
 
-#===================================
-# Is this a new branch?
-#===================================
-sub new_branch {
-    return shift->{new_branch};
-}
-
 #===================================
 # Write a sparse checkout config for the repo.
 #===================================