5 ms·

Oh that’s super annoying. We use that to draw attention to important things that changed in certain commits, for instance we attach human readable diffs to lock

by donatj 4y ago

Oh that’s super annoying. We use that to draw attention to important things that changed in certain commits, for instance we attach human readable diffs to lock file change commits. Beyond that sometimes it’s super helpful to add comments as a human directly to a commit noting details about that specific change set that don’t make sense in the commit itself. This is one of the most boneheaded moves I have seen GitHub make.

rwmj 4y ago
It does seem very strange. If I'm looking at a PR, especially a historical one which has been merged already, I want to see all the comments which were made about the commit in one place. If it was confusing to have these comments turn up "out of context" then maybe that's something that github could have improved instead, by visualising the context better.
- eurasiantiger 4y ago
  They seem to be removing heavy features from often-used pages. I would imagine this change creates quadratic savings in data fetching for the timeline.
  - rwmj 4y ago
    This could be so, but wouldn't a better solution be to prerender popular pages or cache generated HTML (a la memcached etc)? (Having said that I have no idea how modern web apps work, nor what technology stack github is using.)
    - FeepingCreature 4y ago
      Depends how long their tail is, I guess.
  - untitaker_ 4y ago
    Honestly, if that was the reason ("we need to make our mysql faster"), I would strongly prefer if they'd just say that. Certainly more understandable (if not relatable) than this sort of communication.
  - 00kevn 4y ago
    Maybe they'll bring them back in a little while as a premium feature
  - julianlam 4y ago
    I'm still not understanding why this is a justifiable reason. It sounds like something like this is a prime candidate for caching.
    - yunohn 4y ago
      Caching what? All possible comments everywhere? That’s just another db, not a cache. And having infinite caches is not feasible, the point is to store widely and frequently accessed data, not anyone’s comments on any commit in the repo.
  - eloisius 4y ago
    They've been in a roll nerfing stuff like this ever since MS bought them. See "load more comments" ajax links when there are more than 10 comments in a PR review, or "load diff" when a file has changed more than a few lines. All of these degrade the ability to actually look at changes and review them, which is kind of the whole point of pull requests. But alas, the enterprise must enterprise.
ColanR 4y ago
I'd predict they have a plan to replace it with some GitHub-specific metadata. That would enhance lock-in and follow the usual Microsoft strategy.
- donatj 4y ago
  Commit comments already are GitHub specific metadata.
  - IggleSniggle 4y ago
    I see I wasn’t the only one to conflate “commit comment” with “commit message.” For a good 5 minutes I thought this was saying that GitHub would no longer show commit message content in the pull request timeline, which does seem like a crazy vendor lock-in scheme.
    - kadoban 4y ago
      Oh we're not talking about commit messages? Wtf are commit comments then?
      - msbarnett 4y ago
        When you view a commit on GitHub, you can attach a comment to any given line of it. It looks basically like a comment left on a line of code in a PR, except tied to a commit instead of a PR.
        kadoban 4y ago
        Oh. Thanks. If they're inside of PRs are they still commit comments? I'm thinking not, in which case maybe I never noticed those were a possibility.
        avar 4y ago
        A comment <-> PR is a many-many relationship. For a lot of workflows it's practically a one-to-one, as people would only comment on a commit starting from a PR, but you can just comment on any given commit you find. E.g. here's git.git's first commit, with a lot of random (mostly garbage) comments: https://github.com/git/git/commit/e83c5163316 https://github.com/git/git/commit/e83c5163316 That wasn't part of a PR (GitHub didn't even exist then), and if it was it could be a part of many different PRs. I really don't have a full overview of this GitHub change, but this general area is something other hosting providers have definitely struggled with. I.e. how and when to treat a PR/MR as some holistic vertical component, v.s. being mostly incidental metadata about a "push" (or "potential push"), with the commits (and any comments) being the important way to view or think about individual changes, and anything in-between.
hulitu 4y ago
"Real Programmers don't comment their code. If it was hard to write, it should be hard to understand."

by donatj 4y ago

rwmj 4y ago

It does seem very strange. If I'm looking at a PR, especially a historical one which has been merged already, I want to see all the comments which were made about the commit in one place. If it was confusing to have these comments turn up "out of context" then maybe that's something that github could have improved instead, by visualising the context better.

eurasiantiger 4y ago
They seem to be removing heavy features from often-used pages. I would imagine this change creates quadratic savings in data fetching for the timeline.
- rwmj 4y ago
  This could be so, but wouldn't a better solution be to prerender popular pages or cache generated HTML (a la memcached etc)? (Having said that I have no idea how modern web apps work, nor what technology stack github is using.)
  - FeepingCreature 4y ago
    Depends how long their tail is, I guess.
- untitaker_ 4y ago
  Honestly, if that was the reason ("we need to make our mysql faster"), I would strongly prefer if they'd just say that. Certainly more understandable (if not relatable) than this sort of communication.
- 00kevn 4y ago
  Maybe they'll bring them back in a little while as a premium feature
- julianlam 4y ago
  I'm still not understanding why this is a justifiable reason. It sounds like something like this is a prime candidate for caching.
  - yunohn 4y ago
    Caching what? All possible comments everywhere? That’s just another db, not a cache. And having infinite caches is not feasible, the point is to store widely and frequently accessed data, not anyone’s comments on any commit in the repo.
- eloisius 4y ago
  They've been in a roll nerfing stuff like this ever since MS bought them. See "load more comments" ajax links when there are more than 10 comments in a PR review, or "load diff" when a file has changed more than a few lines. All of these degrade the ability to actually look at changes and review them, which is kind of the whole point of pull requests. But alas, the enterprise must enterprise.

ColanR 4y ago

I'd predict they have a plan to replace it with some GitHub-specific metadata. That would enhance lock-in and follow the usual Microsoft strategy.

donatj 4y ago
Commit comments already are GitHub specific metadata.
- IggleSniggle 4y ago
  I see I wasn’t the only one to conflate “commit comment” with “commit message.” For a good 5 minutes I thought this was saying that GitHub would no longer show commit message content in the pull request timeline, which does seem like a crazy vendor lock-in scheme.
  - kadoban 4y ago
    Oh we're not talking about commit messages? Wtf are commit comments then?
    - msbarnett 4y ago
      When you view a commit on GitHub, you can attach a comment to any given line of it. It looks basically like a comment left on a line of code in a PR, except tied to a commit instead of a PR.
      - kadoban 4y ago
        Oh. Thanks. If they're inside of PRs are they still commit comments? I'm thinking not, in which case maybe I never noticed those were a possibility.
        avar 4y ago
        A comment <-> PR is a many-many relationship. For a lot of workflows it's practically a one-to-one, as people would only comment on a commit starting from a PR, but you can just comment on any given commit you find. E.g. here's git.git's first commit, with a lot of random (mostly garbage) comments: https://github.com/git/git/commit/e83c5163316 https://github.com/git/git/commit/e83c5163316 That wasn't part of a PR (GitHub didn't even exist then), and if it was it could be a part of many different PRs. I really don't have a full overview of this GitHub change, but this general area is something other hosting providers have definitely struggled with. I.e. how and when to treat a PR/MR as some holistic vertical component, v.s. being mostly incidental metadata about a "push" (or "potential push"), with the commits (and any comments) being the important way to view or think about individual changes, and anything in-between.

hulitu 4y ago

"Real Programmers don't comment their code. If it was hard to write, it should be hard to understand."