gpush: revamp initialize_get_changes()

try to extract the branch from the $from revspec prior to trying to
resolve it. this has roughly the same cost in case of expected failure
(that is, when the revspec uses a Change-Id), but it is a lot cleaner,
as we need only a little more code in determine_local_branch() (and by
extension branch_for_commit()), while parse_local_rev_id() disappears,
which enables a massive consolidation of parse_local_rev().

Change-Id: If864848c302809e3ba444be9f4ac2a4133e26111
Reviewed-by: Alexandru Croitor <alexandru.croitor@qt.io>

2 files changed, 61 insertions, 104 deletions

diff --git a/bin/git-gpush b/bin/git-gpush
index 5be39d3..d7e28bc 100755
--- a/bin/git-gpush
+++ b/bin/git-gpush
@@ -844,14 +844,15 @@ use constant {
 
 # Find _the_ branch the specified commit lives on. This can be the current
 # branch (and other branches are ignored), or _one_ other branch.
-sub branch_for_commit($)
+sub branch_for_commit($$)
 {
-    my ($commit) = @_;
+    my ($commit, $flags) = @_;
 
     my $src_type = SRC_FLOATING;
     my $curbranch;
     my @otherbranches;
-    my $branches = open_cmd_pipe(0, "git", "branch", "--contains", $commit);
+    my $branches = open_process($flags | USE_STDOUT,
+                                "git", "branch", "--contains", $commit);
     while (read_process($branches)) {
         if (/^\* \(/) {
             $src_type = SRC_HEAD;
@@ -873,6 +874,7 @@ sub branch_for_commit($)
         }
     }
     close_process($branches);
+    return if ($?);
     if (!defined($curbranch)) {
         # The commit is not on the current branch.
         if (@otherbranches == 1) {
@@ -909,18 +911,18 @@ sub branch_for_commit($)
 #   It is the tip, as there are no references. There is no name.
 # Note that commits specified by SHA1 which live on exactly one named
 # branch fall into the first case.
-sub determine_local_branch($)
+sub determine_local_branch($$$)
 {
-    my ($source) = @_;
+    my ($source, $core_source, $flags) = @_;
 
     # First, try to extract a branch name directly.
-    $local_branch = ($source =~ s/[~^].*$//r);
-    $local_branch = resolve_head($local_branch);
+    $local_branch = resolve_head($core_source);
     $local_branch =~ s,^refs/heads/,,;
     my $tip = $local_refs{$local_branch};
     if (!defined($tip)) {
         # Next, try to deduce a branch from the commit.
-        my ($branch, $src_type) = branch_for_commit($source);
+        my ($branch, $src_type) = branch_for_commit($source, $flags);
+        return if (!defined($src_type));
         # Cannot use the branch's tip mid-rebase, because the
         # referenced commits may have been rewritten already.
         if ($src_type == SRC_BRANCH) {
@@ -932,9 +934,9 @@ sub determine_local_branch($)
         }
         $local_branch = $branch;
     }
-    setup_remotes($source);
-    set_gerrit_config($remote) if (!$mail_mode);
-    source_map_validate();
+    printf("Extracted tip %s (%s) from %s.\n", $tip,
+           $local_branch ? "branch '$local_branch'" : "no branch", $source)
+        if ($debug);
     return $tip;
 }
 
@@ -1165,35 +1167,33 @@ sub visit_pushed_changes($)
 
 sub initialize_get_changes()
 {
-    my $commit = parse_local_rev($from, SPEC_TIP);
-    if (!defined($commit)) {
-        # The revspec might refer to a Change-Id.
+    # Get the pool of Changes to push from, and possibly search in
+    # (by Change-Id).
+    # We don't limit the range and even overshoot if possible; the
+    # performance impact of that should be negligible. A corner-case
+    # benefit of this is a better error message when 1) the range base
+    # is specified by Change-Id, 2) it is invalid by being a descendant
+    # of the tip, and 3) the tip is NOT specified by Change-Id. More
+    # importantly, reverse traversal in do_determine_series() also
+    # needs the full range.
+    my ($source, $core_source) = ($from, $from =~ s/^([^~^]*+).*$/$1/r);
+    # Try to derive the branch from the given tip.
+    my $tip = determine_local_branch($source, $core_source, SOFT_FAIL);
+    if (!defined($tip)) {
+        # That didn't work, so maybe the rev-spec refers to a Change-Id.
         # We need a place to start from. We try only the current local branch -
         # trying multiple branches would be expensive, potentially ambiguous
         # (as the same Change-Id can exist multiple times), and probably not
         # very useful to start with.
-        my $tip = determine_local_branch('HEAD');
-        # Get the pool of Changes to search in, and later to push from.
-        analyze_local_branch($tip) or
-            fail("No local Changes (from HEAD).\n");
-        # Now try again.
-        $commit = parse_local_rev_id($from, SPEC_TIP);
-    } else {
-        # We did not need to visit the current branch to find the tip,
-        # so determine the branch from the tip now.
-        my $tip = determine_local_branch($from);
-        # Get the pool of Changes to push from.
-        # We don't limit the range and even overshoot if possible; the
-        # performance impact of that should be negligible. A corner-case
-        # benefit of this is a better error message when 1) the range base
-        # is specified by change-id, 2) it is invalid by being a descendant
-        # of the tip, and 3) the tip is NOT specified by change-id. More
-        # importantly, reverse traversal in do_determine_series() also
-        # needs the full range.
-        analyze_local_branch($tip) or
-            fail("No local Changes (from $from).\n");
-    }
-    return $commit;
+        $source = $core_source = 'HEAD';
+        $tip = determine_local_branch($source, $core_source, FWD_STDERR);
+    }
+    setup_remotes($source);
+    set_gerrit_config($remote) if (!$mail_mode);
+    source_map_validate();
+    analyze_local_branch($tip) or
+        fail("No local Changes (from $core_source).\n");
+    return parse_local_rev($from, SPEC_TIP);
 }
 
 sub finalize_get_changes($$)
diff --git a/bin/git_gpush.pm b/bin/git_gpush.pm
index 4d6ceaf..52ff9f1 100644
--- a/bin/git_gpush.pm
+++ b/bin/git_gpush.pm
@@ -1399,7 +1399,6 @@ sub visit_local_commits($;$)
     return visit_commits($tips, \@upstream_excludes, $cid_opt);
 }
 
-my $analyzed_local_branch = 0;
 # SHA1 of the local branch's merge base with upstream.
 our $local_base;
 # Mapping of Change-Ids to commits on the local branch.
@@ -1415,8 +1414,6 @@ sub analyze_local_branch($)
 {
     my ($tip) = @_;
 
-    $analyzed_local_branch = 1;
-
     # Get the revs ...
     print "Enumerating local Changes ...\n" if ($debug);
     my $raw_commits = visit_local_commits([ $tip ]);
@@ -1546,78 +1543,38 @@ use constant {
     SPEC_PARENT => 2
 };
 
-sub _finalize_parse_local_rev($$$)
-{
-    my ($rev, $out, $scope) = @_;
-
-    # There is no point in restricting the base here - subsequent
-    # use will find out soon enough if it is not reachable.
-    return $out if ($scope == SPEC_BASE);
-    return $out if (!$analyzed_local_branch || $commit_by_id{$out});
-    fail("$rev is outside the local branch.\n");
-}
-
-sub _parse_local_rev_sym($$)
+# Parse a revision specification referring to a commit in the local branch.
+sub parse_local_rev($$)
 {
     my ($rev, $scope) = @_;
 
+    if ($rev eq 'ROOT') {
+        fail("ROOT is not valid for tip revspecs.\n")
+            if ($scope == SPEC_TIP);
+        return $rev;
+    } elsif (($rev eq '@{u}') || ($rev eq '@{upstream}')) {
+        fail("\@{upstream} is not valid for tip revspecs.\n")
+            if ($scope == SPEC_TIP);
+        return "";
+    }
+    fail("$rev is not a valid rev-spec.\n")
+        if ($rev !~ /^([^~^]*+)(.*)$/);
+    my ($core, $rest) = (length($1) ? $1 : "HEAD", $2);
     my $out;
-    if ($rev eq 'HEAD') {
-        fail("HEAD is not valid for base revspecs.\n") if ($scope == SPEC_BASE);
+    if ($core eq 'HEAD') {
         $out = $head_commit;
-    } elsif ($rev eq 'ROOT') {
-        fail("ROOT is not valid for tip revspecs.\n") if ($scope == SPEC_TIP);
-        return ($rev, 1);
-    } elsif (($rev eq '@{u}') || ($rev eq '@{upstream}')) {
-        fail("\@{upstream} is not valid for tip revspecs.\n") if ($scope == SPEC_TIP);
-        return ("", 1);
     } else {
-        return (undef, undef);
+        $out = _sha1_for_partial_local_id($core);
     }
-    return (_finalize_parse_local_rev($rev, $out, $scope), 1);
-}
-
-sub _parse_local_rev_id_only($$)
-{
-    my ($rev, $scope) = @_;
-
-    fail("$rev is not a valid revspec.\n") if ($rev !~ /^(\w+)(.*)$/);
-
-    # This looks like a valid revspec, but git failed to parse it.
-    # Try to parse it as a Change-Id instead.
-    my ($id, $rest) = ($1, $2);
-    my $sha1 = _sha1_for_partial_local_id($id);
-    wfail("$rev does not refer to a Change on the local branch.\n") if (!$sha1);
-    return $sha1 if (!$rest);
-
-    my $out = read_cmd_line(SOFT_FAIL, 'git', 'rev-parse', '--verify', '-q', $sha1.$rest);
-    fail("$rev is not a valid revspec.\n") if (!$out);
-    return _finalize_parse_local_rev($rev, $out, $scope);
-}
-
-sub parse_local_rev_id($$)
-{
-    my ($rev, $scope) = @_;
-
-    my ($sout, $done) = _parse_local_rev_sym($rev, $scope);
-    return $sout if ($done);
-    return _parse_local_rev_id_only($rev, $scope);
-}
-
-# Parse a revision specification referring to a commit in the local branch.
-# This will return undef for @{u}, HEAD, and Change-Ids if the local branch
-# was not visited yet; call parse_local_rev_id() after doing so.
-sub parse_local_rev($$)
-{
-    my ($rev, $scope) = @_;
-
-    $rev = "HEAD".$rev if ($rev =~ /^[~^]/);
-    my ($sout, $done) = _parse_local_rev_sym($rev, $scope);
-    return $sout if ($done);
-    my $out = read_cmd_line(SOFT_FAIL, 'git', 'rev-parse', '--verify', '-q', $rev);
-    return _finalize_parse_local_rev($rev, $out, $scope) if ($out);
-    return undef if (!$analyzed_local_branch);  # Come back later!
-    return _parse_local_rev_id_only($rev, $scope);
+    $out = read_cmd_line(SOFT_FAIL, 'git', 'rev-parse', '--verify', '-q',
+                                    ($out // $core).$rest."^{commit}")
+        if (!defined($out) || length($rest));
+    fail("$rev is not a valid rev-spec.\n") if (!defined($out));
+    # There is no point in restricting the base here - subsequent
+    # use will find out soon enough if it is not reachable.
+    return $out if ($scope == SPEC_BASE);
+    return $out if ($commit_by_id{$out});
+    fail("$rev is outside the local branch.\n");
 }
 
 ################