9 ms·

Hmm, I don't get it: > If you want to have your code reviewed, you first have to branch master, then commit to that branch, then push it remotely, then create

Hmm, I don't get it: > If you want to have your code reviewed, you first have to branch master, then commit to that branch, then push it remotely, then create a PR. When I want to create a PR, I always create a feature/WIP branch locally and push that. Branches are cheap in Git. I find when the unit of scrutiny is an individual commit, like when you contribute to the Linux kernel, I spend way to much time fabricating nice atomic commits. Like maybe 50%, with 30% actual development time and 20% normal cleanup. The only thing I miss with PRs is the ability to rewrite my branch after showing it. I think it is not supported to do `rebase -i` and then `push --force` to a PR, is it? It would be really useful, especially for the cases where you have multiple commits that cancel each other out (e.g. add debugging output, remove debugging output).

je42 6y ago
you can rewrite it and force push it. it is part of the rebase-based work-flow ( always rebase PR to the head of main/master)
- pritambaral 6y ago
  Sure you can force push it, but the reviewer has to review the whole branch all over again. No diff between versions of the branch.
  - throwaway525142 6y ago
    Github shows the force push, and from which commit to which commit it happened. You can then manually put the into the diff view of Github and see what changed. I agree that it should have a UI though.
    - bonzini 6y ago
      The diff view is unusable if master has moved in the meanwhile. What you really need is a UI for "git range-diff".
      - NicoJuicy 6y ago
        You can do that in azure devops. By default, it shows all commits and then you can go to the commits tab and select the ones you want to view ( multiple or single)
        bonzini 6y ago
        Can it show a diff-of-diffs, and detect patches that were added, removed or reordered? Something like https://patchew.org/QEMU/20210414134112.25620-1-iii@linux.ibm.com/diff/20210415130305.28640-1-iii@linux.ibm.com/ https://patchew.org/QEMU/20210414134112.25620-1-iii@linux.ib....
    - pritambaral 6y ago
      > You can then manually put the into the diff view of Github and see what changed. GitHub throws up 404 when I do this. Same with simply trying to view the old commit directly using the `.../commit/<hash>` link.
  - brandmeyer 6y ago
    Yuck. That sounds like a github bug to me. bitbucket has its flaws, but this isn't one of them. We rebase review branches on a regular basis without difficulty and have a useable linear history as a result.
  - wcarss 6y ago
    Actually, the words 'force pushed' in the force push message on the PR (or very nearby to that), are a link to a diff between the pre and post push states of the branch. You can also construct such a diff yourself by going to something like /compare/<old commit hash>..<new commit hash>, and those hashes can also be tags or branch names, and should even allow relative stuff like HEAD^. (I haven't checked the URL just now, but I use both often. It's a little annoyingly hidden, but it is there.)
  - karmakaze 6y ago
    I do something in between. After the initial set of commits and review, I'll add a new commit 'Changes from PR review'. If there are additional changes from further review, I do usually amend and force push this commit. If there are extensive changes from a follow-up review it gets its own commit and specific commit message.
  - anaerobicover 6y ago
    This is something that Gerrit is really good at. The model is a little weird, because it tracks all pushes to a code review essentially as separate commits. But it makes the workflow really really easy for reviewers: you can diff any two versions of the proposed patch. Github has no equivalent that I've found. You can look at a subset of individual commits in the PR, but they're not diffed against each other, which makes it harder to check that the one or two things you were concerned about have been addressed.
    - piotrkaminski 6y ago
      <plug>You might want to take a look at https://reviewable.io https://reviewable.io -- we can diff arbitrary commits without Gerrit's weird model, and a whole lot more besides. </plug>
      - pritambaral 6y ago
        You're still missing the point: diff of diffs. When GitHub compares two commits, it diffs the whole repo as of one commit against the other (like `git diff commitA commitB); what is requested is diffing the change contained in one commit against the other (like `diff <(git show commitA) <(git show commitB)`). PS: I checked out your demo. I can't see how to diff arbitrary commits, only revisions of a PR.
        piotrkaminski 6y ago
        IIUC, you're looking for true diffs between revisions with different bases. If so, Reviewable deals with that automatically, running 4 separate diffs in the background and integrating the results so that base-only diffs are hidden by default. As for diffing commits, you can switch a review into "review each commit" mode so that it will create one revision per commit. You can make this the default for a repo but it's not in the demo as it's not the most common way to work. The demo does really need updating, though!
  - rnestler 6y ago
    In GitLab you can diff the states between force pushes of a branch, which is a feature I love and really miss when working on GitHub.
nemetroid 6y ago
What you describe is standard merge request practice in Gitlab (it is possible to view the diff between different force pushed ”versions” of the branch as well). Don’t know about Github.
- captainmuon 6y ago
  Good to know, we use GitLab and occasionally would use that. I tried it a couple of years ago on GitHub, and it seemed to break the UI. I read it got a bit better now.
- pritambaral 6y ago
  GitHub is hostile to rebased PRs, going as far as erasing commits not present in the latest version of a PR. It does record a "... was force pushed from commit xxxxxx to xxxxx" in the PR's chronology though; but if you click on the first commit in that message it shows a 404.
tybit 6y ago
The author is describing creating a local branch too, but that branch needs to be created from a commit, which will typically be creating your local branch from the most recent master commit. e.g git checkout origin/master -b my-work
epage 6y ago
To make it easiest on review, I do fixup commits (commit --fixup <sha>) and then clean them up at the end. If the conflicts are too messy, I don't rewrite. I have a check to prevent merging with fixups present. Id love it if that was built into the merge itself. This is one problem with the "don't rewrite history" comments people make: the advise isn't clear enough, People advise against it once "push"ed or "public", but that isn't right. Its when collaborating, like on a long lived branch. PR branches are 99% of the time single contributor.

je42 6y ago

you can rewrite it and force push it. it is part of the rebase-based work-flow ( always rebase PR to the head of main/master)

pritambaral 6y ago
Sure you can force push it, but the reviewer has to review the whole branch all over again. No diff between versions of the branch.
- throwaway525142 6y ago
  Github shows the force push, and from which commit to which commit it happened. You can then manually put the into the diff view of Github and see what changed. I agree that it should have a UI though.
  - bonzini 6y ago
    The diff view is unusable if master has moved in the meanwhile. What you really need is a UI for "git range-diff".
    - NicoJuicy 6y ago
      You can do that in azure devops. By default, it shows all commits and then you can go to the commits tab and select the ones you want to view ( multiple or single)
      - bonzini 6y ago
        Can it show a diff-of-diffs, and detect patches that were added, removed or reordered? Something like https://patchew.org/QEMU/20210414134112.25620-1-iii@linux.ibm.com/diff/20210415130305.28640-1-iii@linux.ibm.com/ https://patchew.org/QEMU/20210414134112.25620-1-iii@linux.ib....
  - pritambaral 6y ago
    > You can then manually put the into the diff view of Github and see what changed. GitHub throws up 404 when I do this. Same with simply trying to view the old commit directly using the `.../commit/<hash>` link.
- brandmeyer 6y ago
  Yuck. That sounds like a github bug to me. bitbucket has its flaws, but this isn't one of them. We rebase review branches on a regular basis without difficulty and have a useable linear history as a result.
- wcarss 6y ago
  Actually, the words 'force pushed' in the force push message on the PR (or very nearby to that), are a link to a diff between the pre and post push states of the branch. You can also construct such a diff yourself by going to something like /compare/<old commit hash>..<new commit hash>, and those hashes can also be tags or branch names, and should even allow relative stuff like HEAD^. (I haven't checked the URL just now, but I use both often. It's a little annoyingly hidden, but it is there.)
- karmakaze 6y ago
  I do something in between. After the initial set of commits and review, I'll add a new commit 'Changes from PR review'. If there are additional changes from further review, I do usually amend and force push this commit. If there are extensive changes from a follow-up review it gets its own commit and specific commit message.
- anaerobicover 6y ago
  This is something that Gerrit is really good at. The model is a little weird, because it tracks all pushes to a code review essentially as separate commits. But it makes the workflow really really easy for reviewers: you can diff any two versions of the proposed patch. Github has no equivalent that I've found. You can look at a subset of individual commits in the PR, but they're not diffed against each other, which makes it harder to check that the one or two things you were concerned about have been addressed.
  - piotrkaminski 6y ago
    <plug>You might want to take a look at https://reviewable.io https://reviewable.io -- we can diff arbitrary commits without Gerrit's weird model, and a whole lot more besides. </plug>
    - pritambaral 6y ago
      You're still missing the point: diff of diffs. When GitHub compares two commits, it diffs the whole repo as of one commit against the other (like `git diff commitA commitB); what is requested is diffing the change contained in one commit against the other (like `diff <(git show commitA) <(git show commitB)`). PS: I checked out your demo. I can't see how to diff arbitrary commits, only revisions of a PR.
      - piotrkaminski 6y ago
        IIUC, you're looking for true diffs between revisions with different bases. If so, Reviewable deals with that automatically, running 4 separate diffs in the background and integrating the results so that base-only diffs are hidden by default. As for diffing commits, you can switch a review into "review each commit" mode so that it will create one revision per commit. You can make this the default for a repo but it's not in the demo as it's not the most common way to work. The demo does really need updating, though!
- rnestler 6y ago
  In GitLab you can diff the states between force pushes of a branch, which is a feature I love and really miss when working on GitHub.

nemetroid 6y ago

What you describe is standard merge request practice in Gitlab (it is possible to view the diff between different force pushed ”versions” of the branch as well). Don’t know about Github.

captainmuon 6y ago
Good to know, we use GitLab and occasionally would use that. I tried it a couple of years ago on GitHub, and it seemed to break the UI. I read it got a bit better now.
pritambaral 6y ago
GitHub is hostile to rebased PRs, going as far as erasing commits not present in the latest version of a PR. It does record a "... was force pushed from commit xxxxxx to xxxxx" in the PR's chronology though; but if you click on the first commit in that message it shows a 404.

tybit 6y ago

The author is describing creating a local branch too, but that branch needs to be created from a commit, which will typically be creating your local branch from the most recent master commit. e.g git checkout origin/master -b my-work

epage 6y ago

To make it easiest on review, I do fixup commits (commit --fixup <sha>) and then clean them up at the end. If the conflicts are too messy, I don't rewrite. I have a check to prevent merging with fixups present. Id love it if that was built into the merge itself. This is one problem with the "don't rewrite history" comments people make: the advise isn't clear enough, People advise against it once "push"ed or "public", but that isn't right. Its when collaborating, like on a long lived branch. PR branches are 99% of the time single contributor.