Support workflow with editing branch history

[MikhailGolubtsov]

Problem:
After a series of code reviews there are many 'changes of changes' commits in a branch, which make sense only during a code review phase. After code review is finished, one possible solution would be to squash a whole branch into one commit when merging. But often it's not desirable especially for bigger features. To solve this it is possible to create a new branch with some suffix, e.g. "-reviewed", and squash all secondary commits into primary ones (if --fixup commits were used initially it is really easy with rebase --autosquash). Then this branch with clean history is pushed and merged normally.

Proposal:
Can Zappr recognize, than a new branch with a suffix is introducing exactly the same diff as the old one, which is approved already? So that Zappr would allow to merge.

EDIT:
Can Zappr recognize, than the force pushed branch or a new branch with a suffix is introducing exactly the same diff as the old one, which is approved already? So that Zappr would allow to merge.

[prayerslayer]

I actually like your idea of approving a tuple (base commit id, diff) instead of the PR head commit. It would also solve another issue: Currently we rely on knowing when a commit was pushed to the remote in order to decide which approval comments to count. This is a separate web hook and database table we could then remove.

However from my perspective this change would require a couple of days work and only benefit a small fraction of our users (I suppose — don't have data on this). So... I doubt I'll have time for it, but I'm happy to assist anyone who wants to tackle this issue.

As a workaround one could rebase and force push without another -reviewed branch. Or create this branch, but push it with an already approved commit id.

[ePaul]

@prayerslayer After rebasing (to fix the history) you get new commit IDs. One could possibly get a commit with the same tree ID?

Though the commit comments maybe also belong to the review?

[prayerslayer]

@ePaul (Without understanding your point/proposal)

Or create this branch, but push it with an already approved commit id.

That was wrong and probably confusing. I suggested the following:

Workaround A:

1. hack hack hack
2. review - request changes
3. fix review comments
4. review - no changes requested
5. rebase, force push
6. approve
7. merge

Workaround B: As A, but 5. is rebasing onto new branch (*-reviewed). So basically it's the same, but with B you don't have force pushes if that bugs you somehow.

[ePaul]

Yes, this workaround works. (I don't see a problem with rebasing and force-pushing on a branch which is not yet merged – any problems which can happen here would also happen if you rebase this branch with a new name and merge it to master.)

About the confusion: If I understand it right, this feature proposal says this:

If a review approved some pull request (consisting of a branch and some commits), don't merge it, and later we get a new branch/pull request with exactly the same net changes, but possibly arranged differently into commits (and possibly with some meta data pointing to the other PR/branch), the approval should also be valid for the second pull request. (Of course, just one of them should be merged.)

First, an implementation idea: Just compare the (file) tree hashes of the latest commit of both PRs. If that is same, then (most likely) the content of both file trees is same. (Of course this doesn't work if the base commit of the branch was changed, i.e. we actually rebased something in the literal sense.)

Second, a possible objection: A review could possibly also be having a look at the commit comments (and the distribution of code changes to individual commits), not just the end result. In that case a reviewer might not automatically want to approve a reviewed version. (Not sure how relevant this is, though.)

[prayerslayer]

About the confusion: If I understand it right, this feature proposal says this:

I got the same impression, so yes.

Second, a possible objection

With "commit comment" do you mean (some part of) the commit message or a comment on the diff in Github?

In any case I don't think we can make everybody happy. What we can do is add behavior and introduce more config options later, if necessary. I like the idea because it solves an unrelated problem better than the current solution 😁

[MikhailGolubtsov]

Workaround A:

1. hack hack hack

2. review - request changes
   

3. fix review comments
   

4. review - no changes requested
   

5. rebase, force push
   

6. approve
   

7. merge
   

@prayerslayer my idea was that after a branch was approved by a reviewer, it is possible to rebase & merge, so 6 should be before 5. If by 6 you mean approved by Zappr then it's fine.
If this feature works the same for push -f, then it's great.

Second, a possible objection

@ePaul Most of the time people care about code, not git metadata. But then they squash whole stuff in one commit or merge a branch with all garbage commits as they are. But people, who rebase, do care. So I think, we should additionally check not only that diff is the same, but also that original comments retain (except fixup commits). That would be not very difficult. So if some rewording happens then it requires new approval. Of course, that might be an option... Speaking about checking of changes distribution between commits - seems to difficult to check and unlikely to change after a review without rewording.

[prayerslayer]

If by 6 you mean approved by Zappr then it's fine.

I do.

If this feature works the same for push -f, then it's great.

Currently you can force-push how much you want and Zappr will work as you expect. So I guess it does.

=> Does this mean I can close this issue because your desired workflow is currently supported?

[MikhailGolubtsov]

Wait a minute, how is it possible now? If I change the branch ref to another commit, i.e. I force pushed, does Zappr still count previous approvals?

[prayerslayer]

If I change the branch ref to another commit, i.e. I force pushed, does Zappr still count previous approvals?

No, but you could approve afterwards as well? I agree it's a bit of a chicken-egg problem though 😁 Without approvals you don't want to rebase+forcepush, and without rebased commits your colleagues won't approve. You would need to find some "ok to rebase" codeword...

[MikhailGolubtsov]

This is exactly why I'm proposing - don't ask for approval twice, when an automated check is possible. Moreover it is boring for approvers to check the second time same code. And they SHOULD check all the lines! What if not only commits arrangement has changed, but also additional code changes are there? That is not nice to reviewers.

If not rewrite-it-all case, the proper way of making amendments to your previous work is to do fixup commits. Then it is really clear to reviewers what has changed since the last time. But after code review those commits don't make sense anymore.

UPD: Ok, we can say, that we can trust our colleagues, that they changes only commits metadata and nothing else. Then we just approve twice without examination. Though I believe, purpose of Zappr is to prevent situations, when something not checked gets merged, not to provoke them.

Also, a diligent reviewer checks out code locally, runs tests, runs an application and does some manual testing. Not every kind of testing is always automated and run with CI. So that again, asking for an approval more often, than is really required, is not nice.

[prayerslayer]

Okay, so we're back at the beginning, except we now know that there is no workaround for you, as far as I understood.

To reiterate: I think it's a good idea, but am pretty sure I won't have time for it. I'll leave the issue open and anybody who feels like tackling it is free to do so. In which case I will give my best to assist.

[MikhailGolubtsov]

@prayerslayer Fine for me.