8 ms·

Git Reflow

109 points by jaybosamiya 10y ago

teen 10y ago
This seems super useful- it matches my team's workflow, I'll def suggest we try it. Squash merge ftw
- DannyBee 10y ago
  I'm really curious about it. One of the purported reasons - "it makes git blame more useful", is pretty silly, unless you never have anyone fixing typos or reformatting code or whatever. git blame is already an approximation (because git, and well, everything, does not record what you did for real, only the smallest set of binary delta instructions you must execute to produce file version 2 from file version 1). So this is essentially is "this one git command sucks, so we are going to destroy all of history to make it's output slightly better in a few cases", instead of "hey, we are going to produce a version of blame that identifies the kind of info we care about"
  - juped 10y ago
    >yfw you type `git blame --first-parent`
juped 10y ago
Another "I like to deliberately lose information to no benefit because I'm bad at git" 'workflow' hits Hacker News. Something is deeply wrong with the ecosystem when people want to do things like this!
- throwanem 10y ago
  Where is information lost? Delivered branches aren't deleted, so full history information is available - it just doesn't clutter trunk by merging in every commit.
  - aeontech 10y ago
    This workflow explicitly deletes delivered branches. If your workflow looks like this: - Create a feature branch - Write great code - Create a pull request against master - Get 'lgtm' through a code review - Squash merge to master - *Delete the feature branch* As far as I'm concerned, commits should be rebased and squashed into logical units on the feature branch before merge, that's the responsibility of the dev. Squashing them all into one monster commit feels like a terrible idea.
    - throwanem 10y ago
      Whoops, yeah, you're right. I was looking at the screencast and didn't see an explicit deletion, and mistook that for there not being one at all.
- kiallmacinnes 10y ago
  Some information is worth loosing. Personally, I get exactly zero value from "Fixed a typo", "Fixed that errant semicolon", "fixed tests broken 3 commits ago" commit messages that come hand in hand with a hard and fast "never rewrite history" policy. If that information is valuable to you, great! This is why we have several ways to do things. The trend you're seeing simply seems to indicate (mildly indicate, at best) that a larger percentage of HN readers prefer to squash and merge. Each to their own.
  - npsimons 10y ago
    Very much this. I keep saying: I. Don't. Care. About. Every. Little. Sneeze. A. Developer. Had. On. The. Way. To. Closing. A. Ticket. See how annoying that is? That's what it feels like to me to read non-squashed commits.
    - aeontech 10y ago
      Why does everyone seem to insist that there's only two options? option A) SQUASH ALL THE THINGS option B) HISTORY IS SACRED AND HOLY We just make sure that the developer rebases and squashes the meandering micro-commits into parent logical units before merging. This gives us both sensible logical commits, and avoids monster commits. I spend enough time spelunking through history that I dread seeing something like this when I need to track down the context for a particular change client/something.js | 114 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ critical/something.rb | 41 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--------------------------------------------------------------------------- gulpfile.js | 7 +++++++ lib/stats.erl | 24 ++++++++++++------------ api/somethingelse.js | 114 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ critical_api/another_thing.rb | 41 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--------------------------------------------------------------------------- webpack.js | 7 +++++-- lib/mapreduce.exs | 24 ++++++++++++------------ client/user.js | 114 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ critical/security.rb | 41 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++---------------------------------------------------------------------------
- jerf 10y ago
  "bisect". Bisect becomes useless if not every commit compiles (or local equivalent). Bisect is a critical feature, even if I don't necessarily use it often, because when I need it, I need it. As long as I can bisect, I don't much care about the details. But this is definitely not compatible with three dozen commits mostly consisting of "oops didn't compile" and "forgot comma" and "fix syntax erorr". You've gotta do something about that. The way some people talk, the only acceptable history is an asciinema [1] recording of the development process with a microphone recording the developer's mutterings as it goes. The question isn't about what information you "lose" but about what you keep, because you must discard the vast bulk of it. We're arguing here about whether we chuck 99.97% or 99.99% of it. [1]: https://asciinema.org/ https://asciinema.org/
  - _ikke_ 10y ago
    Bisect also becomes useless because of large commits. Even if you find the commit, the change it introduces can be so large you don't git much improvement. Git allows you to clean up your feature branches to prevent these kind of fix commits. Look at git.git. They don't require people to squash all their commits into a single patch, but still every patch should be compilable.
    - jerf 10y ago
      "As long as I can bisect, I don't much care about the details." If I can't bisect, I don't approve. So, by simple logic based on the premise you supply, I also disapprove of large commits. My point may not be what you expected prior to reading.
      - juped 10y ago
        Then your comment makes no sense in context, because you replied to my complaint that people lose information by doing these giant squash merges and having no nontrivial graph structure in their main project history.
        jerf 10y ago
32bitkid 10y ago
The "squash and merge" trend with git bothers me, and perhaps I'm "doing it wrong" but it just doesn't capture what I need a commit history/git-blame for. Usually, I don't care what feature a line code was for. I want to know why a developer thought that was the right change. And to get that visibility I tend to make lots of commits, treating commits almost as out-of-band comments that don't clutter the file/repo. When I think I'm done with a feature is exactly the time that metadata becomes relevant! Why would I want to lose it? If anything, I wish ides would integrate git-blame more into my visualisation of a file But there are so many people into it, that I feel I must be missing something obvious and it bothers me.
- pcl 10y ago
  I'm with you. History is mostly useful after the fact to understand the details, not the big picture. I wish git had a first-class model of a milestone-ish block of commits, so that the detailed commits and the feature milestones are disambiguated. I try to do this with merge points, but it doesn't seem to always work out, and since it's not native, it depends entirely on convention.
  - sergiosgc 10y ago
    I get close to the milestone-ish commit by opening feature branches and then merging with no fast forward. All commits in the main branch are merges, and all of those are features. The feature incremental commits go in the branch. It's ok, I'd just like to be able to apply this structure to stuff like bisect or blame.
    - pcl 10y ago
      Yeah exactly -- if git knew about a milestone point, then "git bisect" could tie into it. That in and of itself would be fantastic.
    - bennofs 10y ago
      Doesn't `git blame --first-parent` work for that? From my quick tests, it seems like it shows the merge commit if you have such a structure (`--first-parent` also works for `git log` etc)
andrewchilds 10y ago
From the README: $ git reflow setup Please enter your GitHub username: nhance Please enter your GitHub password (we do NOT store this): Your GitHub account was successfully setup! That implies that the username is actually stored somewhere. Is it stored locally or on some reenhanced.com server? The README should be very clear about what exactly gitreflow stores and where.
- bennofs 10y ago
  Perhaps they only use the password to retrieve an OAuth access token which they store? At least that's how the `hub` command line tool handles it afaik.
- zimbatm 10y ago
  It probably uses the password to create a token. The token is then stored on your computer. This allows the app to get access to github but also makes it easy for you to revoke the token. See https://github.com/settings/tokens https://github.com/settings/tokens
- codenamev 10y ago
  From the README: "On your first install, you'll need to setup your Github credentials. These are used only to get an oauth token that's stored in your global git config. We use the Github credentials so we can create pull requests from the command line."
Dru89 10y ago
I'm unhappy with the approval process being a simple search for "LGTM". I wish GitHub pull requests had actual support for a review process, e.g.: * open issues to address * review state, such as "changes requested" or "approved" (along with users that are in each state). We've been using Phabricator's[1] Differential tool for code reviews and it feels superior to this process, but it would certainly be nice to have an all-encompassing solution for this. [1] http://phabricator.org http://phabricator.org
- deathanatos 10y ago
  An actual flag wouldn't be bad, but I know that I've sometimes given out conditional approvals in a code review (i.e., "if you change this, then I approve; if you don't agree, then we should talk"). The idea being to remove a round-trip that would otherwise waste the reviewee's time. (This of course implies a certain degree of trust that the reviewee makes the change as you desire, but in practice I find this isn't a problem.)
- cyphar 10y ago
  Whenever I'm working on a big patchset, I always make sure I add a checklist. Unfortunately nothing forces me to do this, so some people make huge patchsets and there's no history of what was changed or fixed in that patchset. This makes review and maintainence quite frustrating.
codenamev 10y ago
As one of the maintainers of git-reflow, I understand the controversy over squash-merges. Personally, on all the projects I've worked on with this workflow, I have yet to find any drawbacks when needing to use git-blame; although is not to say that there is no value in maintaining a full history nor that it is our way or the highway. We like to keep all changes in context of a feature. We have worked in environments that promote rebasing of feature branches, and while that may work well, it can lead to holes in the history of the review process due to the need to force-push. That said, we are nearing a stabilized core API and have plans to allow for more flexibility in the process. If you are interested in following our ideas behind this, feel free to follow the issue we have open: https://github.com/reenhanced/gitreflow/issues/53 https://github.com/reenhanced/gitreflow/issues/53
tjbiddle 10y ago
Created something in-house very similar for a job a few years ago; We could accept a ticket, it would create a feature branch, update the ticket and comment on it that it was being worked on, then when we submit, it would update the ticket status - assign it to a reviewer, squash commits, create a pull request and comment with the link. This project seems to be done much cleaner though and in a more abstract and reusable manner. Well done!
bsimpson 10y ago
So...it's Gerrit's workflow for GitHub?

Git Reflow

109 points by jaybosamiya 10y ago

teen 10y ago

This seems super useful- it matches my team's workflow, I'll def suggest we try it. Squash merge ftw

DannyBee 10y ago
I'm really curious about it. One of the purported reasons - "it makes git blame more useful", is pretty silly, unless you never have anyone fixing typos or reformatting code or whatever. git blame is already an approximation (because git, and well, everything, does not record what you did for real, only the smallest set of binary delta instructions you must execute to produce file version 2 from file version 1). So this is essentially is "this one git command sucks, so we are going to destroy all of history to make it's output slightly better in a few cases", instead of "hey, we are going to produce a version of blame that identifies the kind of info we care about"
- juped 10y ago
  >yfw you type `git blame --first-parent`

juped 10y ago

Another "I like to deliberately lose information to no benefit because I'm bad at git" 'workflow' hits Hacker News. Something is deeply wrong with the ecosystem when people want to do things like this!

throwanem 10y ago
Where is information lost? Delivered branches aren't deleted, so full history information is available - it just doesn't clutter trunk by merging in every commit.
- aeontech 10y ago
  This workflow explicitly deletes delivered branches. If your workflow looks like this: - Create a feature branch - Write great code - Create a pull request against master - Get 'lgtm' through a code review - Squash merge to master - *Delete the feature branch* As far as I'm concerned, commits should be rebased and squashed into logical units on the feature branch before merge, that's the responsibility of the dev. Squashing them all into one monster commit feels like a terrible idea.
  - throwanem 10y ago
    Whoops, yeah, you're right. I was looking at the screencast and didn't see an explicit deletion, and mistook that for there not being one at all.
kiallmacinnes 10y ago
Some information is worth loosing. Personally, I get exactly zero value from "Fixed a typo", "Fixed that errant semicolon", "fixed tests broken 3 commits ago" commit messages that come hand in hand with a hard and fast "never rewrite history" policy. If that information is valuable to you, great! This is why we have several ways to do things. The trend you're seeing simply seems to indicate (mildly indicate, at best) that a larger percentage of HN readers prefer to squash and merge. Each to their own.
- npsimons 10y ago
  Very much this. I keep saying: I. Don't. Care. About. Every. Little. Sneeze. A. Developer. Had. On. The. Way. To. Closing. A. Ticket. See how annoying that is? That's what it feels like to me to read non-squashed commits.
  - aeontech 10y ago
    Why does everyone seem to insist that there's only two options? option A) SQUASH ALL THE THINGS option B) HISTORY IS SACRED AND HOLY We just make sure that the developer rebases and squashes the meandering micro-commits into parent logical units before merging. This gives us both sensible logical commits, and avoids monster commits. I spend enough time spelunking through history that I dread seeing something like this when I need to track down the context for a particular change client/something.js | 114 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ critical/something.rb | 41 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--------------------------------------------------------------------------- gulpfile.js | 7 +++++++ lib/stats.erl | 24 ++++++++++++------------ api/somethingelse.js | 114 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ critical_api/another_thing.rb | 41 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--------------------------------------------------------------------------- webpack.js | 7 +++++-- lib/mapreduce.exs | 24 ++++++++++++------------ client/user.js | 114 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ critical/security.rb | 41 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++---------------------------------------------------------------------------
jerf 10y ago
"bisect". Bisect becomes useless if not every commit compiles (or local equivalent). Bisect is a critical feature, even if I don't necessarily use it often, because when I need it, I need it. As long as I can bisect, I don't much care about the details. But this is definitely not compatible with three dozen commits mostly consisting of "oops didn't compile" and "forgot comma" and "fix syntax erorr". You've gotta do something about that. The way some people talk, the only acceptable history is an asciinema [1] recording of the development process with a microphone recording the developer's mutterings as it goes. The question isn't about what information you "lose" but about what you keep, because you must discard the vast bulk of it. We're arguing here about whether we chuck 99.97% or 99.99% of it. [1]: https://asciinema.org/ https://asciinema.org/
- _ikke_ 10y ago
  Bisect also becomes useless because of large commits. Even if you find the commit, the change it introduces can be so large you don't git much improvement. Git allows you to clean up your feature branches to prevent these kind of fix commits. Look at git.git. They don't require people to squash all their commits into a single patch, but still every patch should be compilable.
  - jerf 10y ago
    "As long as I can bisect, I don't much care about the details." If I can't bisect, I don't approve. So, by simple logic based on the premise you supply, I also disapprove of large commits. My point may not be what you expected prior to reading.
    - juped 10y ago
      Then your comment makes no sense in context, because you replied to my complaint that people lose information by doing these giant squash merges and having no nontrivial graph structure in their main project history.
      - jerf 10y ago

32bitkid 10y ago

The "squash and merge" trend with git bothers me, and perhaps I'm "doing it wrong" but it just doesn't capture what I need a commit history/git-blame for. Usually, I don't care what feature a line code was for. I want to know why a developer thought that was the right change. And to get that visibility I tend to make lots of commits, treating commits almost as out-of-band comments that don't clutter the file/repo. When I think I'm done with a feature is exactly the time that metadata becomes relevant! Why would I want to lose it? If anything, I wish ides would integrate git-blame more into my visualisation of a file But there are so many people into it, that I feel I must be missing something obvious and it bothers me.

pcl 10y ago
I'm with you. History is mostly useful after the fact to understand the details, not the big picture. I wish git had a first-class model of a milestone-ish block of commits, so that the detailed commits and the feature milestones are disambiguated. I try to do this with merge points, but it doesn't seem to always work out, and since it's not native, it depends entirely on convention.
- sergiosgc 10y ago
  I get close to the milestone-ish commit by opening feature branches and then merging with no fast forward. All commits in the main branch are merges, and all of those are features. The feature incremental commits go in the branch. It's ok, I'd just like to be able to apply this structure to stuff like bisect or blame.
  - pcl 10y ago
    Yeah exactly -- if git knew about a milestone point, then "git bisect" could tie into it. That in and of itself would be fantastic.
  - bennofs 10y ago
    Doesn't `git blame --first-parent` work for that? From my quick tests, it seems like it shows the merge commit if you have such a structure (`--first-parent` also works for `git log` etc)

andrewchilds 10y ago

From the README: $ git reflow setup Please enter your GitHub username: nhance Please enter your GitHub password (we do NOT store this): Your GitHub account was successfully setup! That implies that the username is actually stored somewhere. Is it stored locally or on some reenhanced.com server? The README should be very clear about what exactly gitreflow stores and where.

bennofs 10y ago
Perhaps they only use the password to retrieve an OAuth access token which they store? At least that's how the `hub` command line tool handles it afaik.
zimbatm 10y ago
It probably uses the password to create a token. The token is then stored on your computer. This allows the app to get access to github but also makes it easy for you to revoke the token. See https://github.com/settings/tokens https://github.com/settings/tokens
codenamev 10y ago
From the README: "On your first install, you'll need to setup your Github credentials. These are used only to get an oauth token that's stored in your global git config. We use the Github credentials so we can create pull requests from the command line."

Dru89 10y ago

I'm unhappy with the approval process being a simple search for "LGTM". I wish GitHub pull requests had actual support for a review process, e.g.: * open issues to address * review state, such as "changes requested" or "approved" (along with users that are in each state). We've been using Phabricator's[1] Differential tool for code reviews and it feels superior to this process, but it would certainly be nice to have an all-encompassing solution for this. [1] http://phabricator.org http://phabricator.org

deathanatos 10y ago
An actual flag wouldn't be bad, but I know that I've sometimes given out conditional approvals in a code review (i.e., "if you change this, then I approve; if you don't agree, then we should talk"). The idea being to remove a round-trip that would otherwise waste the reviewee's time. (This of course implies a certain degree of trust that the reviewee makes the change as you desire, but in practice I find this isn't a problem.)
cyphar 10y ago
Whenever I'm working on a big patchset, I always make sure I add a checklist. Unfortunately nothing forces me to do this, so some people make huge patchsets and there's no history of what was changed or fixed in that patchset. This makes review and maintainence quite frustrating.

codenamev 10y ago

As one of the maintainers of git-reflow, I understand the controversy over squash-merges. Personally, on all the projects I've worked on with this workflow, I have yet to find any drawbacks when needing to use git-blame; although is not to say that there is no value in maintaining a full history nor that it is our way or the highway. We like to keep all changes in context of a feature. We have worked in environments that promote rebasing of feature branches, and while that may work well, it can lead to holes in the history of the review process due to the need to force-push. That said, we are nearing a stabilized core API and have plans to allow for more flexibility in the process. If you are interested in following our ideas behind this, feel free to follow the issue we have open: https://github.com/reenhanced/gitreflow/issues/53 https://github.com/reenhanced/gitreflow/issues/53

tjbiddle 10y ago

Created something in-house very similar for a job a few years ago; We could accept a ticket, it would create a feature branch, update the ticket and comment on it that it was being worked on, then when we submit, it would update the ticket status - assign it to a reviewer, squash commits, create a pull request and comment with the link. This project seems to be done much cleaner though and in a more abstract and reusable manner. Well done!

bsimpson 10y ago

So...it's Gerrit's workflow for GitHub?