Open cirosantilli opened 9 years ago
Standard from Ivan today.
I just noticed this in the list of commits on one of my PRs. They're not in the order I expected and I wondered if I screwed up my rebase, until I checked. Does this mean that GitHub said that's their intended behavior?:
Standard from Ivan today.
@jmm hey, I cryptically meant that Ivan gave a standard reply that says nothing except acknowledge my email :-) So I don't know what they think about it.
@cirosantilli ah, gotcha.
from https://help.github.com/articles/why-are-my-commits-in-the-wrong-order/
If you rewrite your commit history via git rebase or a force push, you may notice that your commit sequence is out of order ...
So, there is acknowledgement that the chosen sort method presents commits in the wrong order.
GitHub emphasizes Pull Requests as a space for discussion. All aspects of it ... are represented in a chronological order.
It's not clear to me how only showing things in the wrong order (where the 'right' order is the way the committer ordered them during rebase) fosters better discussion. If anything, I've experienced the opposite - that it confuses both the pull request submitters and reviewers, leading to wasted time and brain cycles better spent on creating and reviewing actual content.
If you always want to see commits in order, we recommend not using
git rebase
.
The rebase
command is really useful for cleaning up commit histories by reordering, squashing, splitting, and rewording commits, making them easier for reviewers to understand. Would it be better if everyone just committed the right code in the right order with the right organization and messages the first time? Absolutely, but that is a fantasy world. Rather than give up a tool that enables better commit organization, it seems more sensible to display commits in the committer's intended order.
Fixing this would not involve a huge paradigm shift. GitHub currently sorts the commits in a PR by author date (not sure if this was always the case, but I just checked). There is no reason they couldn’t simply use the commit date, rather than the author date, as the commit’s insertion point in the same chronological stream. Ties in the commit date ordering would be broken by another monotonic measure, such as the number of ancestor commits.
For people who follow GitHub’s current advice and never use git rebase
or similar tools, the commit date will be equal to the author date and nothing will change.
For people who do use git rebase
, the commit date is a much better indicator of what Git users expect “chronological” to mean. It’s when work on the commit was finished, rather than when it was started. When rewriting a commit, Git bumps its commit date to the current time; then, when replaying its descendants on top of it, Git bumps their commit dates as well. Because the replays may happen within 1 second, the resulting commit dates may be equal—hence the need for the tie breaker—but they will never go backwards (barring clock skew across machines or deliberate manual manipulation).
+1, sorting by commit date, rather than author date, is definitely the correct solution here (I just came to this issue with the intent of posting what @andersk wrote).
@timabbott note that I'm proposing topological sort, not commit date. If you want that, consider opening a separate issue.
@cirosantilli Your original proposal is unclear on a couple of points:
git log
does by default”—but the default for git log
is reverse chronological order, not --topo-order
. On any sufficiently complicated repository (I tried git.git itself), git log
, git log --topo-order
, git log --date-order
, and git log --author-date-order
produce four different orders. In fact, the default git log
misorders a handful of commits with respect to their parents, while the other three do extra work to avoid that. But the default is otherwise closest to --date-order
.I’m only trying to refine your proposal with a plausible solution that correctly orders commits with respect to their parents in all realistic cases, while allowing an implementation consistent with GitHub’s requirements, which they explained to me as follows:
Date: Fri, 12 Apr 2013 15:00:15 -0700 From: Yossef Mendelssohn support@github.com (GitHub Staff) To: andersk@mit.edu Subject: Re: Commits shown misordered on pull request page
Anders,
We view a Pull Request as an ongoing discussion, and so the main view ("discussion" tab) shows all the activity in chronological order. This includes comments and references as well as the commits. With that in mind, we show the commits in that same order, whether or not that matches the graph order that Git has them stored in.
This isn't perfect, especially in cases like barnowl/barnowl#130 where there is no extra discussion. We're working to improve the experience, as we run into this same sort of confusion from time to time if we rebase or otherwise alter the commit history.
Yossef
To literally implement --topo-order
GitHub would almost certainly have to drastically modify the pull request UI, e.g. by separating comments from commits. To implement my refined proposal they need only make a small non-disruptive tweak to the sorting function.
Keeping in mind that this is an unofficial issue tracker, you’re of course welcome to propose other solutions. Just remember that if GitHub ever looks at this issue, they’re only likely to change anything if it’s easy and it won’t break anything for other users.
@andersk agreed on first two points.
I'm not sure if it would be that hard to implement topo order on PRs, GitHub could just use the server's timestamp + topo order to order the magic commit comments.
@cirosantilli We’re talking about pull request comments, not commit comments. Pull request comments are not related to the Git graph and do not have a “topo order”.
Ahh, thank you for posting that email, @andersk. So, the real problem here is that GH is conflating two different concepts - commit history & code review (PR comments/code comments).
It's reasonable and in some cases desirable for long-term readability and organization to re-order and re-write your commit history.
It's entirely unreasonable and confusing to re-order or re-write your code review history.
It doesn't make much sense to tie the ordering of something with un-editable history with something with editable history. I wonder why it was done that way... It's good to hear that github is aware of the conflict and is trying to design a better interface, though.
@toejough That may be a problem, but it is not this problem. Even if GitHub were to remember all previous commits when the commit history of a PR is rewritten, the result would make no sense if sorted by author date the way commits are sorted today, while it would work almost perfectly if sorted by commit date like I proposed.
it would work almost perfectly if sorted by commit date like I proposed.
Yes, but almost. If i rebase
to reoder my commits (of apply a fixup to one), all commits will have equal commit time (http://stackoverflow.com/a/28238046/65458 says we have precision of 1 s here), so one cannot sort over that.
However,any form of topology-related ordering (git log
default git log --topo-order
..) is going to make me happy, as far as PR are concerned.
@findepi I addressed that point above: “Ties in the commit date ordering would be broken by another monotonic measure, such as the number of ancestor commits.”.
And again, the git log
default is not topology-related.
I can't believe this is an issue and I can't see why would someone ever prefer time-based sorting of commits in a PR over topological sort. I hope this gets fixed one day.
It really would be nice to finally fix this issue. It's annoying to do git rebase -i origin/master --exec "sleep 1"
every time before publishing PR.
Using rebase's --ignore-date
seems to work for me as it forces the author date to be the same as commit date.
We shouldn't need to do this though, I'm not sure that ordering by author date provides any purpose.
Yeah, the thing that's super frustrating about this bug is that it displays commits in an order that is confusing and incorrect for everyone; there's essentially no application for which GitHub's ordering is better. And it should be trivial to fix.
In the Zulip project, we regularly hear from developers who get confused because of this issue.
It would be helpful if the user could select how the view is sorted then it would be a personal preference. I expect everyone would select view by commit date. It is a really annoying issue.
Is there any way to view a sequence of git commits in github as they actually exist, not in the order they arrived conceptually in github? I'm used to caring strongly about how the sequence of commits tells a story, especially in terms of TDD-approach adding tests, then adding small commits that gradually make those tests pass, and not being able to visualize/review that story makes it awfully hard to use github PRs for code review.
@nickurak As a workaround, I use the "compare" feature a lot because it seems to show the commits in topological order. For example, if you have a PR from a branch called my-branch
into master
, then you can go to https://github.com/MyUserName/MyRepo/compare/master...my-branch
to see a comparison between the two branches. You can also get there by clicking "New Pull Request" on the repo's home page and then selecting the base and compare branches from the drop-down menus.
Edit: The network is also very helpful. You can find it at https://github.com/MyUserName/MyRepo/network
.
Curious.
Does anyone at Github actually care about ever fixing this or is it more important to make every developer's life more miserable for no obvious reason?
I think it is git default so nobody just shouted loud enough for somebody to notice.
@hramrach No, this is not Git’s fault. There are no options you can pass to git log
or similar to get an ordering as broken as the one GitHub uses on the PR page. GitHub is doing the sort themselves—they have to, in order to interleave PR comments into the stream. They’re just sorting by the wrong date.
Please switch the sorting key from author date
to commit date
: it will change nothing for people who do not rebase and it will fix wrong ordering of rebased commits.
Nobody cares about chronological order.
I'm trully amazed that this has been open for so long.
I just had to manually alter my branches 25 commits to force them in order again for review. I'd really appreciate if that was not needed and Github just respected the same order Git does during Rebase and Visual Studio does when checking the history.
I see no justification for not adding at least an option too see the commits in topological order.
Cc: @mislav
Please please do something about this. It is so confusing and unproductive the way you show commits right now. It should be parent->child.
I contacted github support by email regarding this issue. This is their response:
It doesn't look like we have anything planned in the next year to change the current sort order but we'll pass your request onto the team to consider. I can't promise if or when we'd add this but we'll make sure the request is in the right hands.
Great. Probably emailing GitHub support is what other folks who come across this thread should do, since GitHub staff don't actually use this issue tracker.
In the Zulip project, I probably deal with a new contributor who's confused about why their commits are out of order every week. I've talked GitHub product management about this at in-person events every year since I started running a large open source project on GitHub, but at this point I've basically basically given up on the idea that they'll ever fix this.
FWIW, git filter-branch
can do the trick.
git filter-branch --env-filter 'GIT_AUTHOR_DATE=`date +%s`' --msg-filter '
sleep 1 && cat
' master..HEAD
And it's a bit annoying that I have to do this every time I made my PR and commits easier to review (reordering, changing message, etc. which could cause author date and commit date mismatch). At least, we should have an option to sort by author date.
Issue #386 is more than 4 years old 🎂🎈
We missed the birthday celebration :(
Those teams that do not reorder commits would never notice any difference should GitHub switch to topological order. Those who know how to use rebase and cherry-pick perhaps want the commits to be presented in a specific order and would certainly benefit from such switch.
Moreover, the way GitHub currently depicts commit series in pull requests visually suggests topological order:
-o- Commit message
|
-o- Another commit message
|
-o- One more commit message
which is confusingly not the case. Isn't there a consistent way to properly support pull requests that attempt to keep commit history clean and git blame
-able?
Oh hey! This issue is still open... why?
I was so confused when I discovered that this issue even exists…
Hiding the real topological structure of the commits seems to dumb down the view offered to users.
This totally makes sense for occasional or naive users, but giving up on offering the mere opportunity to really see what's going on negates the possibility to learn to those who could.
Not a good choice for the ecosystem.
This issue seems to be fixed now, doesn't it? Anyway, this is not an official issue tracker for GitHub.com (see here).
@sergefdrv doesn't look like it unless that page is outdated. I did not test in a new PR.
@sergefdrv doesn't look like it unless that page is outdated. I did not test in a new PR.
If I was not confused, commits were shown in the topological order (both in "Conversation" and "Commits") when I updated a PR with reordered commits recently.
@sergefdrv indeed looks like this is now addressed 🥳: https://github.com/agirault/test-github-commit-ordering/pull/2
I did not test with comments in the pull request, but this looks pretty good! 🎉
My concern is commit order in pull requests. Because that is what we review before merging the pull request.
I did a test just now, and the order is still by author date.
I sent this to support@github.com.
Hello,
Commits viewed in pull requests are sorted by author date. In order to
be able to review pull requests with multiple commits, it's vital that
the commits are in topological order, not author date.
The suggested workaround as per the URL below, "If you always want to
see commits in order, we recommend not using git rebase." is not an
option. Rebasing is widely used to rework and clean up pull requests to
shape them up before merging.
https://help.github.com/en/github/committing-changes-to-your-project/why-are-my-commits-in-the-wrong-order
It would be nice if the user had a user setting where the user could decide whether they viewed the commits in author date or topologically. Then GitHub would be able to see the folly of their ways because I bet the majority of users would select a topological order.
Author date order makes no sense to me because it fails to portray the logical relationship of the commits making it hard to track the logical changes.
Perhaps a health warning could be added to the PR web page saying "Warning: Commits may be presented in an a different order to branch order. This is a feature and not a bug.".
The Developer Support Team forwarded my request to the Product Team.
Coming back to a pull request almost two years later (https://github.com/git-tfs/git-tfs/pull/1214), rebased, cleaned up and force-pushed... and very sad to see commits are (still) out of any sane order (where they're actually ordered pretty carefully to make review as easy as possible).
It's beyond me how one of the best places for project (exposure and) collaboration can keep failing so much in regards to one of the most important aspects of the very same collaboration (being pull request review process)... :(
I understand rebasing is not the only way, but being the one (and only?) which in fact does care about clean project history (which in turn does make collaboration easier), could we please get a benefit of choice, at least...? So even if left as-is by default, users who prefer to see pull request commits as they actually appear in history (even after rewriting), can do so.
One workaround is to rebase and update the dates. There are several ways to do that...
Manually with edit
and commit ammend --date
along the way
Or automatic using git rebase --ignore-date
Another nuance is that what I really care about is the review order (when clicking next/prev), and not necessarily the display order of commits in a list. Maybe fixing just the review order is less intrusive in implementation?
Although having both ordered right will be nicer.
One workaround is to rebase and update the dates. There are several ways to do that... Manually with
edit
andcommit ammend --date
along the way Or automatic usinggit rebase --ignore-date
Yeah, I know, and I did use it before in desperation, but especially for these specific changes it's kind of sad to throw away the fact they've been written almost two years ago... :( Not the most important thing in the world, but it still could provide valuable context in some cases, drilling through history at later time.
Like
git log --topo-order
does by default. E.g.: https://github.com/cirosantilli/test-log-order/commits/master , screenshot 2015-05-01.Generated with:
Tree:
Actual log:
Expected log:
The actual tree structure is more important than the timestamp, which is just an arbitrary value that can be controlled by users.
For example, if an old commit gets merged later, I'd expect to see it on the top of the log as the merge date is what matters most.
Not to mention my evil desire to annoy projects by making a future max commit https://github.com/cirosantilli/test/commit/ff86a7bc45e3c28d7838cb0ac785720880481976, create a fake account, find a typo on some famous project with
aspell
and make a pull request. I bet it would pass, and when the project admins notice it, they would likely be forced to force push it away. MUAHAHAHA. But I won't do it :-)Maybe this was mentioned at: https://help.github.com/articles/why-are-my-commits-in-the-wrong-order/
IMHO the best option is
--topo-order
fromman git-log
, as it shows the most "logical" topo sort possible. libgit2 even has it already: https://libgit2.github.com/libgit2/ex/HEAD/log.html#section-23