bots: Add github-push tool

[mvollmer]

No description provided.

[mvollmer]

Pull request has been rewritten.
Old commits | Diff

diff --git a/bots/github-push b/bots/github-push
index 18299dbc9..ad34c6a0b 100755
--- a/bots/github-push
+++ b/bots/github-push
@@ -106,9 +106,12 @@ def main():
 
     if diff == "":
         old_statuses = api.statuses(remote)
+        copy_count = 0
         for key in old_statuses:
             if old_statuses[key]["state"] != "pending":
                 api.post("statuses/" + local, old_statuses[key])
+                copy_count += 1
+        comment += "\n%s test results have been copied.\n" % copy_count
     else:
         comment += " | [Diff](%s)" % diff_link
         if diff.count("\n") < 50:

[mvollmer]

Pull request has been rewritten.
Old commits | Diff

[mvollmer]

Pull request has been rewritten.
Old commits
0 test results have been copied.

[mvollmer]

Commit history has been rewritten
Old commits
26 test results have been copied.

[mvollmer]

Pull request has been rewritten
Old commits | Diff

diff --git a/bots/github-push b/bots/github-push
index b5e11e2f8..4e4d4698d 100755
--- a/bots/github-push
+++ b/bots/github-push
@@ -109,7 +109,7 @@ def main():
             if old_statuses[key]["state"] != "pending":
                 api.post("statuses/" + local, old_statuses[key])
                 copy_count += 1
-        comment = "Commit history has been rewritten\n[Old commits](%s)" % old_commits_link
+        comment = "Commit history has been rewritten.\n[Old commits](%s)" % old_commits_link
         comment += "\n%s test results have been copied.\n" % copy_count
     else:
         comment = "Pull request has been rewritten\n[Old commits](%s)" % old_commits_link

[mvollmer]

Pull request has been rewritten
Old commits | Diff

diff --git a/bots/github-push b/bots/github-push
index 4e4d4698d..9047a2a11 100755
--- a/bots/github-push
+++ b/bots/github-push
@@ -22,10 +22,9 @@
 # The idea is that by using this tool, most (if not all) downsides of
 # force pushing to a PR go away.
 #
-# TODO - In this version, the tool expects that the local branch has a
-# upstream tracking branch configured, and that this tracking
-# branch corresponds to the branch of the pull request.  We can relax
-# that.
+# TODO - The tool expects that the local branch has a upstream
+# tracking branch configured, and that this tracking branch
+# corresponds to the branch of the pull request.  We can relax that.
 
 import subprocess
 import argparse

[mvollmer]

Commit history has been rewritten.
Old commits
25 test results have been copied.

[mvollmer]

Pull request has been rewritten
Old commits | Diff

diff --git a/bots/github-push b/bots/github-push
index fa700ee96..7c15fb375 100755
--- a/bots/github-push
+++ b/bots/github-push
@@ -19,12 +19,38 @@
 # - In case of a non fast-forward push, this tool adds the right
 #   --force-with-lease option.
 #
-# The idea is that by using this tool, most (if not all) downsides of
-# force pushing to a PR go away.
-#
 # The tool finds the pull request number by looking through all open
 # pull requests on GitHub.  Once it has been found, the number is
 # cached in the Git config file to speed up subsequent invocations.
+#
+# NOTE: By using this tool, force pushes to a pull request get a
+# little bit more visible and a little bit less confusing.  But only a
+# little bit.  Use force pushes rarely, and only when you know that
+# the other participants in the pull request are expecting them.  You
+# should still follow the workflow rules that tell you when it is
+# appropriate to use this tool.
+#
+# When writing this tool, I had two review processes in mind:
+#
+# 1) During review, no rewriting is done whatsoever and review
+# comments are addressed by putting new FIXUP comments into the
+# history.  GitHub will show theses FIXUP comments nicely interleaved
+# with the comments.  When the pull request is approved and the tests
+# are green, the person who is going to merge it will squash all the
+# FIXUPS, force push the result, and then immediately merge from the
+# GitHub UI.  [ We might have issues here with approving the result of
+# the squashing if the original auther does the merging.  This issue
+# probably goes away if we run this via a webhook. ]
+#
+# 2) During review, the author produces new versions of the pull
+# request by rewriting it all the time.  No FIXUP commits that need
+# squashing are ever pushed to GitHub.  This tool will show the
+# changes from one rewrite to the next and the reviewers will look at
+# those comments instead of at FIXUP commits.  When the PR is approved
+# and green, it can be merged as is from the GitHub UI.
+#
+# You shouldn't attempt to do both 1) and 2) in the same PR.  That'll
+# be very confusing, I think.
 
 import subprocess
 import argparse

[mvollmer]

Pull request has been rewritten
Old commits | Diff

diff --git a/bots/github-push b/bots/github-push
index e6b4703a3..9f9559b01 100755
--- a/bots/github-push
+++ b/bots/github-push
@@ -55,6 +55,7 @@
 import subprocess
 import argparse
 import sys
+import time
 
 from task import github
 

[mvollmer]

New commits. Diff

diff --git a/bots/github-push b/bots/github-push
index 9f9559b01..a78ea9f7f 100755
--- a/bots/github-push
+++ b/bots/github-push
@@ -97,10 +97,6 @@ def main():
     push_refspecs = local + ":" + tracking_ref
     push_lease = tracking_ref + ":" + remote
 
-    if git("merge-base", local, remote).strip() == remote:
-        git("push", push_dest, push_refspecs)
-        return 0
-
     pull_number = None
     pull_config = git_config("pull.%s.%s" % (tracking_remote, tracking_ref))
     if not pull_config:
@@ -136,7 +132,12 @@ def main():
             print("(new commits not yet in the GiHub API...please stand by. *beep*)")
         time.sleep(1)
 
-    if diff == "":
+    if git("merge-base", local, remote).strip() == remote:
+        comment = "New commits. [Diff](%s)" % diff_link
+        if diff.count("\n") < 50:
+            comment += "\n```diff\n" + diff + "```"
+
+    elif diff == "":
         old_statuses = api.statuses(remote)
         copy_count = 0
         for key in old_statuses:

[mvollmer]

New commits. Diff

[mvollmer]

New commits. Diff

Changes

diff --git a/bots/github-push b/bots/github-push
index 114250249..a79623b97 100755
--- a/bots/github-push
+++ b/bots/github-push
@@ -79,7 +79,7 @@ def main():
     old_commits_link = "https://github.com/" + opts.repo + "/commits/" + remote
     diff_link = "https://github.com/" + opts.repo + "/compare/" + remote + ".." + local
     diff_text = ""
-    if diff.count("\n") < 50:
+    if diff.count("\n") < 200:
         diff_text = "\n<details><summary>Changes</summary>\n\n```diff\n" + diff + "```\n</details>"
 
     git("push", "--force-with-lease=" + push_lease, push_dest, push_refspecs)

[mvollmer]

New commits.

[Diff](https://github.com/cockpit-project/cockpit/compare/43f6b196f8042d953ac52305598906aa21562e7a..c7aaddca3298ce151e5a363d831476891a695ee4)

diff --git a/bots/github-push b/bots/github-push
index a79623b97..ae21ff427 100755
--- a/bots/github-push
+++ b/bots/github-push
@@ -75,12 +75,21 @@ def main():
         pull_number=int(pull_config)
 
     diff = git("diff",remote, local)
-
     old_commits_link = "https://github.com/" + opts.repo + "/commits/" + remote
     diff_link = "https://github.com/" + opts.repo + "/compare/" + remote + ".." + local
-    diff_text = ""
-    if diff.count("\n") < 200:
-        diff_text = "\n<details><summary>Changes</summary>\n\n```diff\n" + diff + "```\n</details>"
+
+    def make_comment(title, include_old_commits=False):
+        diff_lines = diff.count("\n")
+        if include_old_commits:
+            first_line = "[Old commits](%s) | [Diff](%s)" % (old_commits_link, diff_link)
+        else:
+            first_line = "[Diff](%s)" % (diff_link)
+        if diff_lines < 500:
+            diff_open = "open" if diff_lines < 50 else ""
+            return ("<details %s><summary>%s</summary>\n%s\n\n```diff\n%s```\n</details>\n"
+                    % (diff_open, title, first_line, diff))
+        else:
+            return "%s\n%s\n" % (title, first_line)
 
     git("push", "--force-with-lease=" + push_lease, push_dest, push_refspecs)
 
@@ -95,8 +104,7 @@ def main():
         time.sleep(1)
 
     if git("merge-base", local, remote).strip() == remote:
-        comment = "New commits. [Diff](%s)" % diff_link
-        comment += diff_text
+        comment = make_comment("New commits.")
 
     elif diff == "":
         old_statuses = api.statuses(remote)
@@ -105,14 +113,12 @@ def main():
             if old_statuses[key]["state"] != "pending":
                 api.post("statuses/" + local, old_statuses[key])
                 copy_count += 1
-        comment = "Commit history has been rewritten.\n[Old commits](%s)" % old_commits_link
+
+        comment = make_comment("Commit history has been rewritten.", include_old_commits=True)
         comment += "\n%s test results have been copied.\n" % copy_count
     else:
-        comment = "Pull request has been rewritten\n[Old commits](%s)" % old_commits_link
-        comment += " | [Diff](%s)" % diff_link
-        comment += diff_text
+        comment = make_comment("Pull request has been rewritten.")
 
-    comment += "\n"
     api.post("issues/{0}/comments".format(pull_number), { "body": comment })
 
 if __name__ == '__main__':

[mvollmer]

New commits.

Diff

diff --git a/bots/github-push b/bots/github-push
index ae21ff427..427213da1 100755
--- a/bots/github-push
+++ b/bots/github-push
@@ -86,7 +86,7 @@ def main():
             first_line = "[Diff](%s)" % (diff_link)
         if diff_lines < 500:
             diff_open = "open" if diff_lines < 50 else ""
-            return ("<details %s><summary>%s</summary>\n%s\n\n```diff\n%s```\n</details>\n"
+            return ("<details %s><summary>%s</summary>\n\n%s\n```diff\n%s```\n</details>\n"
                     % (diff_open, title, first_line, diff))
         else:
             return "%s\n%s\n" % (title, first_line)

[martinpitt]

This looks really cool! I have some questions/suggestions.

[martinpitt]

This seems rather expensive. The API allows you to filter by branch name, which seems appropriate here? It's pretty well the only thing that remains constant after a rebase, and IMHO it's also an appropriate identity.

Possibly this is then also fast enough to not have to cache the PR in local github. IMHO this is also a bit dangerous: I'm quite sure that branch names do get re-used occasionally. E. g. I'm sure that I created a "test-fixes" or "fix-rhel-x" branch several times, they just get deleted after merging.

[mvollmer]

The API allows you to filter by branch name,

Yeah, but you also need to know the user upfront, no? I'll experiment with this to figure that out.

[martinpitt]

How do yo mean? A remote name always uniquely maps to a user, so if I push to a remote like "my" or "cockpituous", .git/config maps that to an URL like [email protected]:martinpitt/cockpit.git which has the user name.

[martinpitt]

It seems worth moving the determination of the pull number into a function, with some comments.

[mvollmer]

The code has been refactored quite a lot now. How does it look now?

[martinpitt]

Is there still any possibility to get larger diffs? It sounds like the diff_link above should already provide that? If so, why can't we just always use that? It seems more useful to me to go to a GitHub-provided diff page, as you can then also inline-comment on it, rather than a static comment.

[mvollmer]

Is there still any possibility to get larger diffs? It sounds like the diff_link above should already provide that?

Yes.

If so, why can't we just always use that? It seems more useful to me to go to a GitHub-provided diff page, as you can then also inline-comment on it, rather than a static comment.

That's a good point. I thought inline diffs are nice because you don't need to jump between pages, you can just read them as you scroll down the page. But maybe they are distracting...

What about moving the link out of the expander and keeping the expander always closed initially?

[martinpitt]

That seems fine. I also don't want to dwell on this too much -- we can always change it later if it becomes to clumsy or so; I was mostly curious how it works, and that it also supports large diffs.

[martinpitt]

Please no spaces around the = in default arguments.

[martinpitt]

Also, I think this PR should remove the push-rewrite tool, as it completely replaces it AFAICS? We shouldn't establish two new workflows, just one.

[mvollmer]

New commits.

Diff

diff --git a/bots/github-push b/bots/github-push
index b1b66f160..80f7a1dc4 100755
--- a/bots/github-push
+++ b/bots/github-push
@@ -87,7 +87,7 @@ def push_and_comment(api, repo, pull_number, remote, local, push_dest, push_ref)
         comment = make_comment("Commit history has been rewritten.", include_old_commits=True)
         comment += "\n%s test results have been copied.\n" % copy_count
     else:
-        comment = make_comment("Pull request has been rewritten.")
+        comment = make_comment("Pull request has been rewritten.", include_old_commits=True)
 
     api.post("issues/{0}/comments".format(pull_number), { "body": comment })
 

[mvollmer]

Commit history has been rewritten.
Old commits | Diff

24 test results have been copied.

[mvollmer]

New commits.

Diff

diff --git a/bots/github-push b/bots/github-push
index 80f7a1dc4..bfae6227d 100755
--- a/bots/github-push
+++ b/bots/github-push
@@ -51,14 +51,15 @@ def push_and_comment(api, repo, pull_number, remote, local, push_dest, push_ref)
 
     def make_comment(title, include_old_commits=False):
         diff_lines = diff.count("\n")
+        first_line_parts = [ ]
         if include_old_commits:
-            first_line = "[Old commits](%s) | [Diff](%s)" % (old_commits_link, diff_link)
-        else:
-            first_line = "[Diff](%s)" % (diff_link)
+            first_line_parts.append("[Old commits](%s)" % old_commits_link)
+        if diff_lines > 0:
+            first_line_parts.append("[Diff](%s)" % (diff_link))
+        first_line = " | ".join(first_line_parts)
         if diff_lines > 0 and diff_lines < 500:
-            diff_open = "open" if diff_lines < 50 else ""
-            return ("<details %s><summary>%s</summary>\n\n%s\n```diff\n%s```\n</details>\n"
-                    % (diff_open, title, first_line, diff))
+            return ("<details><summary>%s</summary>\n\n%s\n```diff\n%s```\n</details>\n"
+                    % (title, first_line, diff))
         else:
             return "%s\n%s\n" % (title, first_line)
 

[mvollmer]

Commit history has been rewritten.
Old commits

29 test results have been copied.

[mvollmer]

Pull request has been rewritten.

Old commits | Diff

diff --git a/bots/push-rewrite b/bots/push-rewrite
deleted file mode 100755
index 7be09b03b..000000000
--- a/bots/push-rewrite
+++ /dev/null
@@ -1,75 +0,0 @@
-#! /usr/bin/python3
-
-# push-rewrite -- Force push to a PR after rewriting history, with benefits.
-#
-# This tool force pushes your local changes to origin but only if that
-# doesn't change any files.  If that is the case, the tool will also
-# copy the test results over.
-#
-# The idea is that you use this tool after squashing fixups in a pull
-# request or after other similar last minute rewriting activity before
-# merging a pull request.  Then you can merge the PR using the GitHub
-# UI and get a nice "merged" label for it, and the tests will still be
-# green.
-
-import subprocess
-import argparse
-import sys
-import time
-
-from task import github
-
-def execute(*args):
-    try:
-        sys.stderr.write("+ " + " ".join(args) + "\n")
-        output = subprocess.check_output(args, universal_newlines=True)
-    except subprocess.CalledProcessError as ex:
-        sys.exit(ex.returncode)
-    return output
-
-def git(*args):
-    return execute("git", *args).strip()
-
-def main():
-    parser = argparse.ArgumentParser(description='Force push after a rewrite')
-    parser.add_argument('--repo', help="The GitHub repository to work with", default="cockpit-project/cockpit")
-    parser.add_argument('remote', help='The remote to push to')
-    parser.add_argument('branch', nargs='?',  help='The branch to push')
-    opts = parser.parse_args()
-
-    if not opts.branch:
-        opts.branch = git("rev-parse", "--abbrev-ref", "HEAD")
-
-    local = git("rev-parse", opts.branch)
-    remote = git("rev-parse", opts.remote + "/" + opts.branch)
-
-    if local == remote:
-        sys.stderr.write("Nothing to push.\n")
-        return 0
-
-    if git("diff", "--stat", local, remote) != "":
-        sys.stderr.write("You have local changes, aborting.\n")
-        return 1
-
-    api = github.GitHub(repo=opts.repo)
-    old_statuses = api.statuses(remote)
-
-    git("push", "--force-with-lease=" + opts.branch + ":" + remote, opts.remote, opts.branch + ":" + opts.branch)
-
-    for n in range(100,0,-1):
-        try:
-            if api.get("commits/%s" % local, verbose=False):
-                break
-        except RuntimeError as e:
-            if not "Unprocessable Entity" in str(e) or n <= 1:
-                raise
-            print("(new commits not yet in the GiHub API...please stand by. *beep*)")
-        time.sleep(1)
-
-    for key in old_statuses:
-        if old_statuses[key]["state"] != "pending":
-            print("Copying results for %s" % old_statuses[key]["context"])
-            api.post("statuses/" + local, old_statuses[key])
-
-if __name__ == '__main__':
-    sys.exit(main())

[martinpitt]

I've used github-push for the first time in PR #10511, with my normal branch setup. The first push just changed the commit message and copied the test results, the next push changed the code and triggered new tests. The corresponding comments/diffs look correct. This is awesome, thanks @mvollmer! Will review the code in a bit, but as this is new, we don't need to be overly picky.

changes done after my previous review

[martinpitt]

This looks good enough to land now IMHO. I just have one question about the "XXX" (not sure how potentially dangerous it can be).

[martinpitt]

IMHO 500 is quite an enormous comment; the /compare link should suffice for most use cases and doesn't clobber the comments then. But this is not a veto, we can try this and change this in the future to our liking.

[martinpitt]

The existence of this github API is a really nice little gem! So the most important thing on rewriting push requests is to record all the HEADs somewhere.

[martinpitt]

copy_count could just be replaced with len(old_statuses). But nitpicking..

[martinpitt]

Is this something which should be addressed before landing this?

[martinpitt]

Meh, it bitrotted a bit, now needs rebasing. @mvollmer, can we settle the question about how severe the "TODO" is, and land this?

[mvollmer]

Regarding the TODO.

If you have never fetched the current tip of the remote branch, the tool can not produce the diff and do other parts of its work. So currently the tool makes sure that the remote branch tip is in the local repo.

However, if you have never fetched that tip, you are very likely going to overwrite work that you have never seen. So it would be better to just fail and require the user to do the fetch.

This only applies in the "no tracking branch" case.

[martinpitt]

Sometimes coincidences in life are really curious -- about a week ago, GitHub changed their UI to now generate messages like

Gundersanne force-pushed the Gundersanne:cockpit-289 branch from c05a535 to e6d9fbd 22 hours ago

And "force-pushed" is a link to the /compare page. This seems to be pretty much exactly what github-push does on top of push-rewrite, isn't it?

[martinpitt]

@mvollmer: See the previous comment - do you still want to go through with this, or just keep the current push-rewrite?

[martinpitt]

I force-pushed a rebase to unblock our CI.

[marusak]

Needs to rebase to master since #12367 changed tests names

[mvollmer]

I am afraid I lost interest in this...