On Wed, Apr 23, 2025 at 02:25:40AM +0200, Remo Senekowitsch wrote: > On Wed Apr 23, 2025 at 12:23 AM CEST, Nico Williams wrote: > > GitLab seems to figure it out -- an existence proof that it can be done. > > So maybe Junio and Theodore are quite right that similarity checks > > should be enough. > > I haven't used GitLab in a while so I had to test it. I found the > feature you describe and it's nice, but it falls short in important and > somewhat predicable ways. True. > Firstly, it only happens when you make a comment on the whole diff. > Then it shows you an interdiff between the version you commented on and > the immediate next version. (I didn't find a way to make it show the > interdiff between the commented-on and the _latest_ version, but that > seems doable implementation wise.) I suspect that's a UI design issue: how cluttered do you want the UI to be? GitLab's UI is already quite cluttered. Often I struggle to find the thing I want, and I use it often. > But that's not really what we're talking about here. That's taking the > interdiff between two versions _of the entire branch_ and selecting a > relevant _hunk_ from it. Neat, but completely unrelated. I think GitHub and Gitlab both can handle ... commit ranges, can they not? The problem is that then you have to find refs (or commit hashes) for each version you want to compare, and once again UI complexity gets ugly. This is why I often do code reviews in the terminal, using `git diff` and `git log --patch` as needed. The problem then is that actually leaving a comment on the CR requires going back to the browser, navigating to the CR/MR/PR/WhateverR, navigating to the file and diff, finding the relevant hunk, and finally authoring my comment -- this is very painful. In principle I want to be able to do everything in the browser because I don't see how to design a TUI that lets me do all of this, but I'm really a shell and TUI type of user, so if we could design a TUI I'd rather use that than the browser. > If you make a comment on a _specific commit_, the feature doesn't kick > in at all. GitLab just tells you that this comment was made "on an > outdated change in commit xyz". I suppose GL could give you a link to a diff view of the same commit in different versions. The point is that GL demonstrates that these things can be done. And I don't see how a change ID would have helped GL much except in cases where one re-does all the commits with different subject lines etc, but leaves the actual patches mostly the same. Now it does happen that I split and squash commits, but it's rare that I completely redo them. So I think I'm coming back to my original view, that change IDs are useful for forward- and back-porting, and not much else, and that existing conventions are probably good enough for forward- and back-porting anyways. Though I don't object to a change ID header, mind you. > A truly patch-based review UI would show the interdiff between the > previous version of the specific commit that was commented on and the > next or latest version of _that specific patch_. I suspect most users are not pedantic like me and don't really go to that sort of length, know they could, would want to, or would if they knew they can. > So, while I must say GitLab is doing pretty well here, claiming that > they "figured out" patch-based review on top of git without change-ids > is not accurate. They've demonstrated that this is possible now and without change IDs -- that is very relevant here since the assertion has been made that change IDs would help do better than most CR tools do w/o them. GL probably have done what they think their users want them to do. GL might yet add more of this functionality. Nico --
