New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

crazy proposal: parse diffs to figure out when re-review is required #95

Closed
jeffkaufman opened this Issue Feb 1, 2019 · 6 comments

Comments

Projects
None yet
2 participants
@jeffkaufman
Copy link
Owner

jeffkaufman commented Feb 1, 2019

Right now we can avoid re-reviews in the situation where git can automatically handle the merge conflicts, which we handle by simply not merging master into the PR. But sometimes we need to merge master in or else the PR won't pass Travis: consider #93 where I needed the PR to run on python 3.6 or #67 where the branch in the PR was looking for approvals from people who were no longer players. Both needed re-review after merging in master, even though the PR diff didn't change at all.

If this gets annoying we should be able to make something that allows approvals from previous commits to still stand under very specific restrictions. Something like, if the diff "approved commit vs master at the time of approval" is identical to "current commit vs master now".

@tnelling

This comment has been minimized.

Copy link
Collaborator

tnelling commented Feb 1, 2019

I can imagine situations where we wouldn't want this behavior. Not so much because of what merging master does to the PR, but because (perhaps) something having been merged into master changes the higher-level context in which the PR is being evaluated. Take the example of the discussion about whether named bonuses should have the values in the file contents or file names. The PRs did in fact step on each other, but it wouldn't be hard to imagine a similar situation where they didn't, but nonetheless, once one of those two PRs went in, the other should not.

That all having been said, maintaining current approvals wouldn't necessarily cause such a PR to go in anyway, and in the vast majority of cases what you're suggesting makes sense.

@jeffkaufman

This comment has been minimized.

Copy link
Owner Author

jeffkaufman commented Feb 1, 2019

@tnelling sure, but we have that problem now. For example, #47 and #68/#69 don't have any merge conflicts and github would have allowed #68/#69 and then #47 to merge. But unless #47 were synced after #68/#69 were merged (which I intentionally did) then #47 would have added the two players back into the game.

@tnelling

This comment has been minimized.

Copy link
Collaborator

tnelling commented Feb 1, 2019

True. You could even argue we should go the complete other direction - all approvals are wiped after anything is merged into master. I think we may have already discussed that a bit, actually, as a hypothetical opening for sneaky behavior. I probably wouldn't even mind having to re-approve, but I'd be concerned about what happens when other players don't do so.

@jeffkaufman

This comment has been minimized.

Copy link
Owner Author

jeffkaufman commented Feb 1, 2019

That's how we had things when we very first started playing, but we turned it off because it was too annoying: #18

I don't think it's worth it.

@jeffkaufman

This comment has been minimized.

Copy link
Owner Author

jeffkaufman commented Feb 7, 2019

Here's how this could work: an approval at commit N is invalidated by pushing a new commit M unless the PR diff is exactly the same at commits N and M. In other words, merging from master would reset approvals if the files in question had been touched (added, removed, or edited) on master, because that would affect the left side of the diff.

Automated PR checks would still need to pass, of course.

@jeffkaufman

This comment has been minimized.

Copy link
Owner Author

jeffkaufman commented Feb 7, 2019

(this is significantly less crazy than I had been imagining)

jeffkaufman added a commit that referenced this issue Feb 8, 2019

Allow old approvals to persist when it's safe to do so.
Consider #111.  It was approved at 250d3a6 but then I needed to merge from
master to get up to date.  The delta from 250d3a6 to master is identical to
the diff from the new HEAD, 98b5a3a, to master.  Both are exactly:

```
diff --git a/validate.py b/validate.py
index 4bdd054..4debe90 100644
--- a/validate.py
+++ b/validate.py
@@ -1,6 +1,7 @@
 import os
 import runpy
 import copy
+import traceback

 import util
 import pull_request
@@ -67,7 +68,7 @@ def determine_if_mergeable(pr):

   # Go through rules sorted by priority, with ties broken by the filename.
   for rule_priority, rule_full_fname, rule_name, is_allow in sorted(rules):
-    print('Running rule %s' % rule_full_fname)
+    print('\nRunning rule %s' % rule_full_fname)

     pr_copy = copy.deepcopy(pr)

@@ -82,6 +83,7 @@ def determine_if_mergeable(pr):
           print('\nPASS: %s' % rule_name)
           return
       except Exception as e:
+        traceback.print_exc()
         print('  %s: %s' % (rule_full_fname, e))
     else:
       # Raises an exception to indicate blocking, anything else for no
```

Since the diff didn't change with merging in master, it's still safe to count
these approvals at the earlier commit as valid.  They represent approving the
same change.

If on master any of the files involved had been added, removed, or edited then
the diff would not be exactly the same and we'd need to get new approvals.

Fixes #95
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment