-
Notifications
You must be signed in to change notification settings - Fork 129
Sort commits topologically first instead of by author date #386
Comments
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?:
|
@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/
So, there is acknowledgement that the chosen sort method presents commits in the wrong 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.
The |
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 For people who do use |
+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:
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:
To literally implement 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. |
Yes, but almost. If i However,any form of topology-related ordering ( |
@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 |
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 |
Using rebase's 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 Edit: The network is also very helpful. You can find it at |
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. |
The Developer Support Team forwarded my request to the Product Team. |
Coming back to a pull request almost two years later (git-tfs/git-tfs#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... 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. |
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. |
Maybe it isn't one of the best places. That's all |
The problem of comments-on-PRs interleaved with commits-in-topological order sounds easily solvable. Afterall, GH does have a date it can use to correlate the two; the push date; it even displays that in PRs. And since you can't push commits out-of-order (i.e. descendants before ancestors), the push date is always consistent with any topological order. In short, a topological sort based on the tuple |
Last friday, on github entreprise, it seems that it was solved! I really hope it wasn't just a random thing, but I thinb the PR I did should have been out-of-order by wasn't. |
It's so sad that there is a need for whole tutorials of how to fix it... https://andrewlock.net/how-to-fix-the-order-of-commits-in-github-pull-requests/ @andrewlock @github |
As a workaround I do git rebase origin/master -i --exec "git commit --amend --no-edit --date=now" --exec "sleep 10" Posted if it might help someone. |
@LabhanshAgrawal you can do it 10x faster. I use |
if push comes to shove, you can do it even faster than that, with a |
@findepi well that was just an example, you can use whatever you find ok. I usually have only 4-5 commits per branch so it doesn't bother me. |
Please fix this annoyance. Thank you! |
@mxm and everyone else, Commenting here does nothing to get this fixed. Email to [email protected]. |
Done.
|
To follow @mxm's example and to add the suggestion on how to clarify topological ordering in the face of external things like PR comments, I also posted feedback to https://support.github.com/contact/feedback/: Currently, pull requests can be hard to review when they contain commits that have out-of-order dates; a situation that commonly occurs when rebasing or otherwise rewriting history. In particular, the commits are shown ordered by date, not in the order as committed, which can make diffs hard to read. Worse, if there are comments made in the pull-request, those too can appear out of order, possibly even appearing to comment on things that look like they happened later. In principle, if the author waits a long time betweeen committing and pushing, or if their clock is wrong, even without rebasing you can see weird and difficult to understand sequences of events. |
Fixed, apparently: https://github.blog/changelog/2020-07-14-pull-request-commits-now-ordered-chronologically/ (edit) I guess they went with "reverse chronological order", which is git's default. So it's not |
This appears to be chronological rather than topological |
This phrase is wrong (git ordering is topological, not chronological), but I assume that they meant topological instead of chronological… |
So, I just test this, and it's better... but still wrong. Seriously, how hard can it be :-/ |
At least the commits are now in toplological order: yay! However, PR comments are interleaved based on author date, not push date... that's weird. |
Maybe it's coincidence that github addressed this feedback so quickly, but just in case I sent more feedback: Recently, you improve the commit ordering, as announced here: https://github.blog/changelog/2020-07-14-pull-request-commits-now-ordered-chronologically/ - that's great, and makes PR's easier to review! (note to self: proofread before hitting send... oh well!) |
Well, they may be referring to the chronology of the git commit timestamp, which, if your clock is monotonic, would be consistent with a topological ordering. Even if your clock isn't monotonic, it's possible git pulls tricks to hide that (no idea) and in any case observing clock non-monotonicity is typically hard. In some sense "chronological" might be equivalent to "topological" - if you use the commit timestamp, and if the clock is "good" enough. |
I reordered the commits on a branch yesterday (without touching author date), and lo and behold, I think they did sort topologically now on the GitHub pull request page. |
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 cirosantilli/test-git-web-interface@ff86a7b, 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-23The text was updated successfully, but these errors were encountered: