gpush/gpick: make rebase mode remember the base of the previous push

avoids that one needs to specify the base each time when re-pushing a
series.
added the --rebase option as a convenient way to reset the base.

this has the "side effect" that gpick is better equipped to follow
structural changes to series which are based on other pending series.

Change-Id: I8ee028def7245deda9fd15f5c1c6c7a9b48847cc
Reviewed-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>

3 files changed, 124 insertions, 40 deletions

diff --git a/bin/git-gpick b/bin/git-gpick
index 73a8473..ac578c5 100755
--- a/bin/git-gpick
+++ b/bin/git-gpick
@@ -972,6 +972,23 @@ sub drop_merged_patchsets($$)
     }
 }
 
+# Traverse a series until we hit the upstream. On the way back,
+# set each visited commit's push base to the found merge base.
+sub set_base_deduced($);
+
+sub set_base_deduced($)
+{
+    my ($sha1) = @_;
+
+    my $commit = $commit_by_id{$sha1};
+    return $sha1 if (!$commit);
+    my $base = $$commit{base};
+    return $base if (defined($base));
+    $base = set_base_deduced(get_1st_parent($commit));
+    $$commit{base} = $base;
+    return $base;
+}
+
 sub analyze_patchset($$)
 {
     my ($ginfo, $cat) = @_;
@@ -987,6 +1004,7 @@ sub analyze_patchset($$)
     wfail("PatchSet $$rev{ps} of Change $$ginfo{key} is apparently upstream?!\n")
         if (!$commit);
     $$ginfo{"${cat}_commit"} = $commit;
+    set_base_deduced($rev_id);
 }
 
 # Extract additional information from the fetched PatchSets.
@@ -1044,9 +1062,9 @@ sub attach_series($$$)
     }
 }
 
-sub assemble_series($$$$@)
+sub assemble_series($$$$$@)
 {
-    my ($changeid, $sha1, $seen, $callback, @args) = @_;
+    my ($changeid, $sha1, $base, $seen, $callback, @args) = @_;
 
     my $commit = $commit_by_id{$sha1};
     if (!$commit) {
@@ -1068,11 +1086,16 @@ sub assemble_series($$$$@)
         unshift @series, $commit;
 
         $sha1 = get_1st_parent($commit);
+        if (defined($base) && ($sha1 eq $base)) {
+            print "Hit base.\n" if ($debug);
+            $commit = undef;
+            last;
+        }
         if (!($commit = $commit_by_id{$sha1})) {
             print "Hit upstream.\n" if ($debug);
             last;
         }
-        my ($nsha1, $stop) = $callback->($commit, @args);
+        my ($nsha1, $stop) = $callback->($commit, \$base, @args);
         if ($stop) {
             $commit = undef;
             last;
@@ -1089,9 +1112,9 @@ sub assemble_series($$$$@)
     return (\@series, $commit);
 }
 
-sub advance_pushed_series($@)
+sub advance_pushed_series($$@)
 {
-    my ($commit, $stamp, $pmap, $missing, $fails) = @_;
+    my ($commit, $base, $stamp, $pmap, $bmap, $missing, $fails) = @_;
 
     my ($sha1, $changeid) = ($$commit{id}, $$commit{changeid});
     my $ginfo = $gerrit_info_by_sha1{$sha1};
@@ -1113,6 +1136,8 @@ sub advance_pushed_series($@)
         }
         # The ancestors must be younger. See below.
         $$stamp = $$rev{ts};
+        my $nbase = $$bmap{$changeid};
+        $$base = $nbase if (defined($nbase));
         return $nsha1;
     }
     #print "$changeid ($sha1) is only deduced.\n" if ($debug);
@@ -1174,17 +1199,18 @@ sub complete_pushed_series($$)
             next;
         }
         my $stamp = $$rev{ts};
+        my $base = $$change{base};
         my ($series, $anchor) = assemble_series(
-                $changeid, $sha1, \%seen, \&advance_pushed_series,
-                \$stamp, $$spec{pmap}, $missing, \$fails);
+                $changeid, $sha1, $base, \%seen, \&advance_pushed_series,
+                \$stamp, $$spec{pmap}, $$spec{bmap}, $missing, \$fails);
         attach_series($series, $anchor, \%result) if (!$fails);
     }
     $$spec{pushed_changes} = \%result if (!$fails);
 }
 
-sub advance_remote_series($@)
+sub advance_remote_series($$@)
 {
-    my ($commit, $stamp, $missing, $fails) = @_;
+    my ($commit, $base, $stamp, $bmap, $missing, $fails) = @_;
 
     my ($sha1, $changeid) = ($$commit{id}, $$commit{changeid});
     my $ginfo = $gerrit_info_by_sha1{$sha1};
@@ -1202,20 +1228,22 @@ sub advance_remote_series($@)
         return undef;
     }
     my $rev = $$ginfo{revs}[$$ginfo{pick_idx}];
+    my $nbase = $$bmap{$changeid};
+    $$base = $nbase if (defined($nbase));
     return $$rev{id};
 }
 
 # Assemble a remote series from its tip commit. On the way record required
 # Changes that we didn't query yet, and fail the operation if there are any.
-sub assemble_remote_series($$$$$)
+sub assemble_remote_series($$$$$$$)
 {
-    my ($ginfo, $stamp, $seen, $missing, $fails) = @_;
+    my ($ginfo, $base, $stamp, $bmap, $seen, $missing, $fails) = @_;
 
     my $rev = $$ginfo{revs}[$$ginfo{pick_idx}];
     my $sha1 = $$rev{id};
     my ($series, $anchor) = assemble_series(
-            $$ginfo{id}, $sha1, $seen, \&advance_remote_series,
-            $stamp, $missing, $fails);
+            $$ginfo{id}, $sha1, $base, $seen, \&advance_remote_series,
+            $stamp, $bmap, $missing, $fails);
     return ($series, $anchor);
 }
 
@@ -1238,7 +1266,7 @@ sub complete_remote_series($$)
             next;
         }
         my ($series, $anchor) = assemble_remote_series(
-                $ginfo, $$spec{stamp}, \%seen, $missing, \$fails);
+                $ginfo, $$change{base}, $$spec{stamp}, $$spec{bmap}, \%seen, $missing, \$fails);
         attach_series($series, $anchor, \%result) if (!$fails);
     }
     $$spec{changes} = \%result if (!$fails);
@@ -1257,7 +1285,7 @@ sub resolve_insertion_spec($$)
     # First assemble the entire series.
     my $fails = 0;
     my ($series, undef) = assemble_remote_series(
-            $tip, $stamp, undef, $missing, \$fails);
+            $tip, undef, $stamp, {}, undef, $missing, \$fails);
     wfail("Range $$spec{orig} is empty.\n") if (!@$series);
     return if ($fails);
 
@@ -1534,15 +1562,18 @@ sub complete_spec_tails($)
     foreach my $spec (@$specs) {
         next if ($$spec{action} != UPDATE);
 
-        my %pmap;
+        my (%bmap, %pmap);
         foreach my $change (@{$$spec{range}}) {
             my $changeid = $$change{id};
             next if ($ignore_struct);
+            my $base = $$change{base};
+            $bmap{$changeid} = $base if (defined($base));
             next if ($force_struct);
             my $pushed = $$change{pushed};
             $pmap{$changeid} = $pushed if (defined($pushed));
         }
         next if ($ignore_struct);
+        $$spec{bmap} = \%bmap;
         next if ($force_struct);
         $$spec{pmap} = \%pmap;
     }
@@ -2117,6 +2148,11 @@ sub verify_update($$$$$$$$)
             ($pfx, $annot) = ("Keep      ",
                               "PS$pushed_ps ".format_parts($rmt_parts)." (IGNORED)")
                 if (!$quiet);
+            # Do not update the base when ignoring. The presumption
+            # is that the push was wholly bogus and should be reverted
+            # including the base, so inter-diffs can skip over it. And
+            # if we are actually seeing a bona fide conflict, the base
+            # was probably not modified anyway.
             # INFO, as the source is "applied" even if the update is ignored.
             return (UPD_PUSHED | UPD_INFO, NO_PARTS, $pfx, $annot);
         }
@@ -2328,6 +2364,10 @@ sub do_adjust_changes($)
             if ($upd & UPD_PUSHED) {
                 $$change{pushed} = $$rmt_commit{id};
             }
+            # Generally, the base should match the latest PatchSet even if
+            # we push over it in the end, to optimize inter-diffs.
+            $$change{base} = $$rmt_commit{base}
+                if (($upd & UPD_META) || ($ginfo && !defined($$change{base})));
             # If {pushed} was already set, branch tracking already made this a no-op.
             $$change{tgt} = $$ginfo{branch}
                 if (($upd & UPD_META) || ($ginfo && !defined($$change{tgt})));
diff --git a/bin/git-gpush b/bin/git-gpush
index 87d9a09..2e14503 100755
--- a/bin/git-gpush
+++ b/bin/git-gpush
@@ -52,10 +52,11 @@ Description:
     them onto a new base. This has the advantage that you can keep many
     unrelated "series" in your local branch without creating spurious
     dependencies on Gerrit, effectively pretending that you have a separate
-    branch for every series. Furthermore, you can keep the base of
-    subsequent pushes of the same series constant, which means that you
-    can closely track the upstream branch without pushing needless rebases
-    to Gerrit (which would obfuscate inter-PatchSet diffs).
+    branch for every series. Furthermore, gpush will keep the base of
+    subsequent pushes of the same series constant (unless told otherwise),
+    which means that you can closely track the upstream branch without
+    pushing needless rebases to Gerrit (which would obfuscate inter-PatchSet
+    diffs).
 
     'From' may be specified as either a ref, a SHA1, or a Gerrit Change-Id,
     possibly abbreviated. Git rev-spec suffixes like '~2' are allowed; if
@@ -92,10 +93,14 @@ Options:
         Specify the commit to rebase the pushed series onto. It is possible
         to specify arbitrary other commits already known to Gerrit, including
         still pending PatchSets. Use 'ROOT' to orphan the series.
-        If no base is specified, gpush will use the head of the target branch
-        on the gerrit remote. That means that you can set a new default base
-        by fetching your gerrit remote, which you should do much less often
-        than fetching the origin remote.
+        Except for merge commits and their children, once chosen, the base
+        will persist through subsequent pushes until overridden, even when
+        the series grows.
+        By default, the local branch's base is used.
+
+    --rebase
+        Reset the base of a previously pushed series to the local branch's
+        base. Typically used after a conflicted pull.
 
     -l, --list
         Report all Changes that would be pushed, then quit.
@@ -158,13 +163,14 @@ Configuration:
 
 Examples:
     git gpush .. +alex
-        Push HEAD and its ancestors for the upstream branch of HEAD,
-        rebased on top of the current tip of that branch, and add 'alex'
-        as a reviewer.
+        Push HEAD and its ancestors for the upstream branch of HEAD, and
+        add 'alex' as a reviewer. No rebasing is initially done, but note
+        that the remote base will stick even if you subsequently rebase
+        the local branch.
 
     git gpush I2c9ccbc26..I9434d28fc
         Push the range identified by Gerrit Change-Ids for the upstream
-        branch of HEAD, rebased on top of the current tip of that branch.
+        branch of HEAD, rebased on top of the local branch's base.
 
     git gpush ~1:3 -b 5.4 -o 85af7f4538b
         Push the range HEAD~4..HEAD~1 for branch 5.4, rebased on top of
@@ -198,6 +204,7 @@ my @CCs;
 
 sub parse_arguments(@)
 {
+    my $rebase = 0;
     while (scalar @_) {
         my $arg = shift @_;
 
@@ -226,6 +233,8 @@ sub parse_arguments(@)
         } elsif ($arg eq "-o" || $arg eq "--onto" || $arg eq "--base") {
             fail("--onto needs an argument.\n") if (!@_ || ($_[0] =~ /^-/));
             $ref_base = shift @_;
+        } elsif ($arg eq "--rebase") {
+            $rebase = 1;
         } elsif ($arg eq "-l" || $arg eq "--list") {
             $list_only = 1;
         } elsif ($arg eq "-ll" || $arg eq "--list-online") {
@@ -273,6 +282,10 @@ sub parse_arguments(@)
     fail("--quiet and --verbose/--debug are mutually exclusive.\n")
         if ($quiet && $verbose);
 
+    fail("--base and --rebase are mutually exclusive.\n")
+        if (defined($ref_base) && $rebase);
+    $ref_base = "" if ($rebase);
+
     if (!defined($from)) {
         $from = "HEAD";
     } else {
@@ -749,15 +762,38 @@ sub determine_base($)
     confirm_ref_base();
     my $base = $ref_base;
     if (!defined($base)) {
-        # We base this on the target remote under the assumption that it will
-        # be updated only when really necessary, unlike the origin remote.
-        my $refs = $remote_refs{$remote};
-        my $to = $$group{branch};
-        $base = $refs && $$refs{$to};
-        wfail("Remote '$remote' does not appear to have branch '$to'."
-                ." Maybe fetch it?\n")
-            if (!defined($base));
-        print "Using upstream $base as base.\n" if ($verbose);
+        # We have no base yet, so deduce it from the previous push(es).
+        # The simple approach would be using the merge-base(s) of the
+        # previous push(es) and upstream. However, that would not work
+        # if we based the series on another pending series. To address
+        # this, we could stop the ancestor traversal at the first Change
+        # which is not part of this series. But then Changes dropped from
+        # the bottom of the series would appear to be bases. That means
+        # that we need to record the base of each push explicitly.
+        #
+        # Note that gpick'ing (or re-syncing after manual picking) records
+        # the base of the Change's series as seen on Gerrit. That implies
+        # that if a series was picked only partially, re-pushing it with
+        # gpush will "rebase up" these Changes, thus making their order
+        # relative to the omitted Changes ambiguous on Gerrit. This may
+        # be surprising at first, but it makes sense, as a partial pick
+        # is functionally equivalent to gpick-ing the entire series and
+        # subsequently dropping some Changes locally.
+        $base = aggregate_property($group, 'base',
+                                   sub { $_[0]->{base} },
+                                   sub { '  @'.substr($_[0], 0, 8) });
+        # Technically, we should re-validate the base before each push,
+        # because pending PatchSets (or entire Changes) can be deleted,
+        # and upstream commits can disappear due to forced pushes.
+        # While querying base PatchSets along with the pushed Changes
+        # would be cheap and easy, we'd still need a fallback for direct-
+        # pushed base commits, and we'd need to fetch the target remote
+        # if we wanted to catch forced pushes reliably.
+        # This all doesn't appear to be worth it ...
+    }
+    if (!length($base)) {
+        $base = $local_base;
+        print "Basing series on local branch's base.\n" if ($verbose);
     }
     $$group{base} = $base;
 }
@@ -1039,7 +1075,7 @@ sub print_errors($)
 {
     my ($group) = @_;
 
-    fail_push($group, "Please specify a different base.\n")
+    fail_push($group, "Please use --rebase or specify a different base.\n")
         if ($have_conflicts);
     fail_push($group, "Giving up - push is going to be rejected.\n")
         if ($have_rejected);
@@ -1163,7 +1199,7 @@ sub update_state($)
 {
     my ($group) = @_;
 
-    my ($branch, $tpc) = ($$group{branch}, $$group{topic});
+    my ($branch, $tpc, $base) = ($$group{branch}, $$group{topic}, $$group{base});
     # Setting an empty topic clears the previous topic from the server.
     $tpc = undef if (defined($tpc) && !length($tpc));
     foreach my $change (@{$$group{changes}}) {
@@ -1173,6 +1209,12 @@ sub update_state($)
             $$change{topic} = $tpc;
         }
         $$change{tgt} = $branch;
+        my $commit = $$change{local};
+        # --rebase would be required after redoing a merge with a different
+        # 1st parent anyway, so just don't save the base for merges.
+        # Regular commits on top of merges are auto-rebased as well.
+        $base = undef if (@{$$commit{parents}} > 1);
+        $$change{base} = $base;
     }
 }
 
diff --git a/bin/git_gpush.pm b/bin/git_gpush.pm
index 410a3d4..abf66b6 100644
--- a/bin/git_gpush.pm
+++ b/bin/git_gpush.pm
@@ -645,6 +645,8 @@ our %gerrit_infos_by_id;
 # - topic: Gerrit topic. Persisted only as a cache.
 # - pushed: SHA1 of the commit this Change was pushed as last time
 #   from this repository.
+# - base: SHA1 of commit on top of which the entire series which this
+#   Change is part of was pushed.
 
 my $next_key = 10000;
 # All known Gerrit Changes for the current repository.
@@ -692,7 +694,7 @@ sub save_state(;$$)
 
     print "Saving ".($new ? "new " : "")."state".($dry ? " [DRY]" : "")." ...\n" if ($debug);
     my (@lines, @updates);
-    my @fkeys = ('key', 'id', 'src', 'tgt', 'topic');
+    my @fkeys = ('key', 'id', 'src', 'tgt', 'topic', 'base');
     my @rkeys = ('pushed');
     if ($new) {
         push @lines, "verify $new", "updater $state_updater";