Hacker News Board Hacker News, browsed like a classic message board
Live data from Hacker News

Some of us like "interdiff" code review

gist.github.com

Post reply on HN
Print view View original

211–220 of 224 posts

Re: Some of us like "interdiff" code review

#211

Post by jrochkind1 »

dmurray wrote: post #197
shambulatron wrote: post #152

Earlier quoted context omitted.

Have you come across `git rebase --update-refs`? This automatically moves your "intermediate" branches during a rebase and sounds like it could be useful in your situation.

There's also `git rebase --onto`, which effectively does the reverse - you tell git you've already rebased the part of this branch that overlapped with the "intermediate" branch, and it just needs to take care of the rest.

Wow, I knew of neither of these, thank you!

I'll have to take some time to experiment with both and make sure I understand howt hey work and any pitfalls!

I am definitely in the crowd that thinks git UX is pretty unintuitive and challenging, despite having used it for over a decade!

These sound like great plumbing for a github-style web UI that actually facilitates multi-step sequence PR's though...

Re: Some of us like "interdiff" code review

#212

Post by jrochkind1 »

dgfitz wrote: post #181
jrochkind1 wrote: post #118

Earlier quoted context omitted.

I realized reading the first part of this article that I often want to set up a sequence of PR's, for very similar reasons as in the begininng of OP. Say, a prefatory refactor, then the main work, then some data cleanup. When I do this, the problem with rebase is that it kind of breaks the additional "next in sequence" PRs "on top", or at least requires (confusing to me) cleanup in all of them when I rebase the base.

> a sequence of PR's, for very similar reasons as in the begininng of OP. Say, a prefatory refactor, then the main work, then some data cleanup. I would dislike this as a reviewer. Now I need to keep your stack of changes in my head, in order. Just make the change and push it, if people can’t review the diff you have different problems.

> Now I need to keep your stack of changes in my head, in order.

Agreed. In another comment in this thread, I said the problem with this was "And been frustrated that Github's interface does not make it very easy to make the sequence apparent or have good DX for the reviewers."

For this to work, we need better tooling support -- the reviewer having to keep track in their head is not good.

I'm not sure which of the realistic alternatives you are suggesting:

1. I could submit them all as one PR. But I _know_ it's too big to review well, I needed several steps to get there. Yes, this is the "different problem", but it's normal to sometimes need to take several reviewable steps to get to the outcome, right? The OP had a very common pattern for me: Refactor with no behavior change to set the stage and make the feature easily implementable; feature implementation; data migration.

2. I could submit the PR's one at a time, and not submit subsequent ones until the first are reviewed. But now I need to keep it all in my head, which my head isn't up to, and I not infrequentkly wind up messing up my branches. AND, sticking with that common case, the reviewer doesn't see what motivated the refactor, so I have to just explain it in words, and their like "Oh but I don't like this aspect of the refactor" and I have to be like "Yeah, okay, but if you could see how I used it to implmeent the feature you'd understand why"

Of course I end up doing one of these three paths (with the sequence of PR's made all at once being one of them) when this situation arises, having no other options. They are all unsatisfactory, agreed.

Re: Some of us like "interdiff" code review

#213

Post by samatman »

travisb wrote: post #130
samatman wrote: post #127

Posts like this add fuel to the fire of my conviction that the future of revision control is Pijul, or something much like it. I can't completely justify the intuition here, but this seems like an artificial distinction forced by a snapshot model of a codebase. It's literally about the same patches, but arranged differently to give clarity to the history of the project. So it seems obvious on that level that a system…

While patches and snapshots are duals of each other, patches are generally less easy to reason about and work with, in no small part because compilers and humans work on the snapshot, not the patch. Having worked extensively with patches as the semantic unit via patch(1) and quilt and stgit, I'm very skeptical that a VCS based on patches is actually superior in most circumstances. In this particular case, perhaps it…

Roughly, patch(1) has about the same relation to Pijul as RCS has to git.

I don't think useful conclusions can be drawn from that experience, basically. As you say, they're duals: but the actual changes need to be modeled as patches to do anything non-trivial with a snapshot. One can build a linear history as a series of snapshots, but applying them to some other timeline can be done with a patch, or with? Nope, it's just patches.

My intuition is that architecting the VCS as patches, and making snapshots an interface question, will work better than the other way around. But Pijul is not yet advanced enough in UX terms to really cash out on that guess.

Re: Some of us like "interdiff" code review

#214

Post by codethief »

crabbone wrote: post #164
codethief wrote: post #69

Earlier quoted context omitted.

As mentioned elsewhere in this thread, this is also the approach that Sapling follows. As for GitContext, how do you keep track of commits across fixups, rebases, reordering, etc.?

What's the point of keeping track of commits? I honestly never understood people wanting that. Is this for some kind of weird accounting / social-score system where the number of commits decides your yearly bonus? It's useful to see how the system evolved (because you might want to go back a bit and redo the newer stuff), but it's pointless to see the mistakes made along the way, for example, unless you have some adm…

I think we're completely talking past each other. OP wrote:

> Reviewers should always see only the changes since their last review, no matter how many crazy git operations happen behind the scenes.

I merely wanted to know how they achieve that with git.

Re: Some of us like "interdiff" code review

#215

Post by crabbone »

codethief wrote: post #214
crabbone wrote: post #164

Earlier quoted context omitted.

What's the point of keeping track of commits? I honestly never understood people wanting that. Is this for some kind of weird accounting / social-score system where the number of commits decides your yearly bonus? It's useful to see how the system evolved (because you might want to go back a bit and redo the newer stuff), but it's pointless to see the mistakes made along the way, for example, unless you have some adm…

I think we're completely talking past each other. OP wrote: > Reviewers should always see only the changes since their last review, no matter how many crazy git operations happen behind the scenes. I merely wanted to know how they achieve that with git.

Not only do I think that's a pipe dream... I think it's technically impossible... I mean, diff has to show also what happened before whatever change took place. How do they expect not to see what was replaced? Or maybe I just don't understand what they mean by "changes since their last review".

Re: Some of us like "interdiff" code review

#216

Post by crabbone »

Izkata wrote: post #173
crabbone wrote: post #170

Earlier quoted context omitted.

This is solving the problem with the wrong tool. What you need is documentation. But people working on the project you have to maintain didn't write one. So, you are trying to use git blame, astrology and ouija board to guess what the project authors wanted. And, while doing so, you are arguing for never cleaning the house, keeping all the garbage where it falls. Which will only make your situation worse, because the…

> What you need is documentation. But people working on the project you have to maintain didn't write one. Oh there is documentation, but it's old and contradictory. Commits are self-organizing by history. > So, you are trying to use git blame, astrology and ouija board to guess what the project authors wanted. There's no guessing involved. Keep the original commits and it's right there in the commit message and orde…

> Oh there is documentation, but it's old and contradictory.

That's like saying "oh, but I have food to eat, except it's been eaten by mice and mold. I mean, if documentation doesn't work, then it's the same as "no documentation"...

> There's no guessing involved.

But that's where you are wrong. The more garbage isn't cleaned from the repository, the more guessing you have to do. What if a comment next to the code contradicts the code? -- It's garbage! But nobody cleaned it, and now you have to guess whether the comment or the code is correct. But this is just an easy-to-understand example. Essentially, any bug is like this: there was an intention to do X, but Y was done instead, and now you need to figure out whether Y was intended or X.

History needs to make sense, it doesn't need to document the mistakes that happened along the way. Not for the purpose of development anyways.

> Clean the code up as much as you want. The history however is completely hidden and not causing any clutter,

I'm not talking about refactoring code, I'm talking about cleaning the history. It's only hidden if you are completely hapless when it comes to working with Git. Any developer worth their salt work with the history regularly. It's not a secret, and it's definitely not hidden.

On the contrary, if you believe history to be "invisible" -- just use rsync. There's no need for complicated system that's designed to work with history... If you cannot make use of your history -- why bother keeping it?

Re: Some of us like "interdiff" code review

#217

Post by gloryjulio »

majormajor wrote: post #190
gloryjulio wrote: post #17

Earlier quoted context omitted.

When we publish a stack of commits, our ci ensures that every commit is build and tested individually. There is no consistency issue Squash and merge actually makes the above goal harder. With rebase + small commits, all we need to make sure is that every commit pass all the build signals and tests during ci

What is "publishing a stack of commits"? Is that putting it up for review? Or are you not doing a PR workflow at all, in which case this doesn't really relate to the article. Is the expectation that the developer either never commits stuff in a broken state during development or that they go back and rewrite or squash the sequence before pushing it for review?

> What is "publishing a stack of commits"?

Yes publishing for review.

> Is the expectation that the developer either never commits stuff in a broken state

That's exactly right. In a stack, every commit is built by ci, reviewed by the team. There will be no broken commits

Re: Some of us like "interdiff" code review

#219

Post by Piraty »

another-acct wrote: post #179
Piraty wrote: post #90

using github's UI as preferred way to interact with code in review is a bad idea, as it encourages lazy from-the-couch-just-yolo-approve-it-looks-alright style of review. this is where gerrit+mail based workflows shine as reviewer is more encouraged to apply the series, compile/run it in their env (which might differ from your's); here is an example [0]. here are some useful notes on how to have a purely branch centr…

> mail based workflows shine as reviewer is more encouraged to apply the series, compile/run it in their env not to mention: if the reviewer does this for every version of the posted series, on appropriately named (versioned) local branches, then they can trivially run git-range-diff between adjacent versions!

yes. git-worktrees are great: https://piraty.dev/txt/git-worktrees-multitasking.html

Re: Some of us like "interdiff" code review

#220

Post by Piraty »

tome wrote: post #133
Piraty wrote: post #88

Earlier quoted context omitted.

https://news.ycombinator.com/item?id=37086022

Wow, my jaw is on the floor. This is such a good idea! I already had the idea of tracking issues in the same repo as the code -- not that I actually use that idea -- but I didn't have the idea of doing PRs in the repo itself. Love it!

tracking issues in the repo would work well. for example, have a dir like ./issues/fix-this-weird-bug.md , and once fixed rename it to ./changelog/fix-this-weird-bug.md . if you adhere to some rules for the metadata, you can use this to base your per-release changelog off of it (some tools already do similar)
Post reply on HN