2,500 karma · joined April 9, 2014
I really wish that more places would just set up a private NNTP server without any peering as a forum. Do people just use NNTP readers, or is there a web frontend?
That's a good point.
> Commit messages are text, so you could have a similar line by line click-and-comment review interface as you already have for the code changes.
It would be nice if something like that was available in Github. The closest thing you could do would be to copy the commit title and body and paste it as quoted text in the text area and then comment on it inline.
You actually can comment on a commit itself. I'm in the habit on middle-clicking on the sha1 link of commits in a PR and looking at the commit itself. You can comment on lines in the commit, and there's a text area at the bottom where you can comment on the entire commit itself. I'll then follow up with making a comment on the PR linking the commit (pasting the sha1 link) and saying I made a few comments here.
> It's as if GitHub is being run by people who just don't know how Git is meant to be used.
Github wasn't really designed with code review in mind. A lot of the features they added over the years for review appear to be hacked on rather than fixing fundamental design issues (like being able to comment on commit messages without having to jump through a bunch of hoops).
Review systems like gerrit, phabricator, review board, or even email, do a much better job at exposing individual commits and their associated metadata like the commit message.
Companies do change bug trackers and ticketing systems and those links may no longer work years down the line.
> The bug tracker has entire discussions of what lead to the commit so it's not clear to me that a detailed commit message is a plus when the real detailed info is in the tracker. Yes it's indirect but there's no way I'm going to summarize the entire issue discussion.
But summarizing it can be one of the most valuable things you can do for a maintainer who has to make changes years after you've moved on. For one thing, the problem and discussion is fresh in your mind and you understand the context. In a few minutes, you could summarize the problem, the approach taken to fix it and alternatives that were considered but not used because the chosen solution clearly didn't have an issue/was more efficient, etc.
Even if you didn't want to do that, you could just copy and paste the entire discussion text at the end of the commit message so that even if the bug tracker is no longer in use in the future, the discussion itself was preserved in the commit history and accessible via git log or blame.
The real problem here is expecting people to be able bodied enough to deal with a lack of alternative exits when someone in the same building inevitably is careless enough to start a fire.
Not everyone is young and in shape to push couches or jump over them.
Ctrl-C does work in the GUI. That said, one thing I like about Linux is being able to highlight text using the mouse and then pasting it by middle-clicking. I don't have to interact with the keyboard at all to copy and paste text that way.
Except that newer products like Slack and email providers with their conversation view are conflating the two use cases.
Slack tries to implement features of a forum by allowing threaded conversations. Conversation view email removes the nested threads and makes the email thread look more like a chat as opposed to a forum discussion with multiple threads.
$ date --date="@2200000000"
Sun Sep 18 07:06:40 PM EDT 2039 $ date --date="@1800000000"
Fri Jan 15 03:00:00 AM EST 2027
$ date --date="@1900000000"
Sun Mar 17 01:46:40 PM EDT 2030
$ date --date="@2000000000"
Tue May 17 11:33:20 PM EDT 2033> This is really only viable if each intermediate commit on a development branch is intended to be bug free.
git rebase has an --exec option that allows you to run a command or set of commands for each commit in the branch. You could rebase your development branch before pushing it up for review and ensure each commit passes coffee linting and tests.
Pretty much every ISP provided an email address for each account. Some email clients supported the MIME multipart/partial subtype which allowed for large attachments to be split up amongst multiple emails.
You shouldn't have to revert a lot of tiny commits if the bug is due to just one of those commits.
> The result of a pull request should be the equivalent of having built a single patch to start with, just with better review tooling.
In my experience, features take more than one commit to implement. The merge commit provides a way to group those multiple commits so that you can see all commits that went into implementing a feature.
If you're trying to make pull requests that are like small commits, then a feature branch becomes
first-commit
first-pr-merge
second-commit
second-pr-merge
third-commit
third-pr-merge
...
nth-commit
nth-pr-merge
with commits and their associated merge commits for other features interspersed with your set of commits.Which basically introduces a lot of merge commits (effectively doubling the number of commits) where the first parent is the previous merge commit and the second parent is the single commit for that pull request. You have no way to really group related commits that were used to implement a feature since there's no merge commit that groups them all together. In that case, you could halve the number of commits by dispensing with merge commits entirely and just adding the #PR-number in the commit message to link to the PR discussion.
The problem with squash commits is that they tend to make changes to more files and make changes to more lines in a given file. This makes it harder to revert changes after more changes have been applied to the main branch. In contrast, organizing commits into a set of logical changes necessary to implement a feature in a given branch makes it much easier to revert one of those commits even after other feature branches have been merbed into the main branch because the commits affect one or just a few files and not many changes in a file).
The difference is a sequence of commits that implement a feature where each commit basically is a logical change that does a single thing (e.g., add a function and associated test, add calls to a new function, etc) versus a sequence of commits with additional commits that fix issues brought up during review. Those additional commits really should be amended to the corresponding original commit rather than being a completely separate commit.
Another way to look at it is submitting an assignment where you have one page per problem solution as opposed to submitting that and then several more pages at the end containing fixes to those problem solutions rather than incorporating those fixes in the original set of pages.
git am < saved_email_file.emlIt wasn't initially designed with code review in mind (unlike systems like phabricator, gerrit, review board, etc).
It is possible to comment on commits in github (you can do it by clicking on the sha1 of the commit and then making a comment on a line in the diff). But the comment won't show up in the main PR diff window.
> But I don't understand how a contributor can feel pressure to address all of the reviewers' comments in a single commit: They can commit as many times as they want, and only push when they feel it's ready.
Unfortunately, this leads to a lot of fixup commits in the branch that muddle up the history. A changeset consists of one or more commits where each makes one logical change where the what was done and why it was done that way are detailed in the commit message.
This is a misconception. git itself has commands (git-format-patch and git-send-email) that automate the creation of patches and sending changeset email threads to the mailing list. The only thing one needs to do is set the appropriate configuration settings in their git config (which is a one time operation like setting your name and email address).
The actual interaction on the mailing list (responding to those who review patches and changesets can be done in any email client of one's choosing (though it's helpful to use an email client that supports threading using the Message-Id, In-Reply-To and Reference headers rather than one that only handles conversation view style replies).
I haven't really used xrdp, but I have used xpra and have been able to attach to running instances of applications that I left open on the host machine remotely (much like I can reattach to a screen or tmux session remotely).
The interesting thing is that if you use a conventional email client that supports actual threading using the Message-Id and In-Reply-To and References header values, and you edit the email message to trim quoted content and reply inline, the conversation view implementation in their email client breaks and it shows a blank message.