On Tue, Apr 22, 2025 at 6:23 PM Nico Williams <nico@xxxxxxxxxxxxxxxx> wrote:
>
> On Tue, Apr 22, 2025 at 04:17:03PM -0400, D. Ben Knoble wrote:
> > On Wed, Apr 16, 2025 at 7:36 AM Remo Senekowitsch <remo@xxxxxxxxxxx> wrote:
> > > On Mon Apr 14, 2025 at 9:54 PM CEST, D. Ben Knoble wrote:
>
> [Responding out of order.]
>
> > > I think it's the other way around: Building a code review UI built on
> > > git and centered around interdiffs today is _hard_, that's why we don't
> > > have it yet. Adding change-ids to commits will make it much easier,
> > > paving the way for these tools to be implemented.
> >
> > Fair point, although GitHub's detection of force-pushes makes me think
> > it could split a PR into versions at that point, cross-link backwards
> > and forwards by one version (from the force-push detection), show
> > range-diffs between versions based on the target branch of the merge,
> > and even follow the cross-links to show an overall sequence of
> > versions.
>
> GitLab does something like this for review comments where the author
> updates the commented you get a link you can click on that shows you the
> differences between version n-1 and version n. GitLab does this without
> change IDs.
Does GitLab's output resemble that of git-range-diff? If not, it's not
quite what I had in mind ;)
GitHub's "compare versions" button (after a force-push) is more like
"git diff" on the 2 trees, which is only a fraction of the relevant
information.
[…]
> > But the treatment below is something I agree with and failed to
> > convey, I think: namely, URIs seem to encode too much
> > "unportable"/"specific" information in Git. I feel like the current
> > design is not really "tool-agnostic" as much as "built on a universal
> > core." That seems valuable and prone to more longevity.
>
> On the other hand URIs can easily be dereferenced, as long as they are
> not rotted.
Rot happens, though… your point is well-taken! Most of Git seems to be
valuable in the presence of linkrot, though, and I don't see how
adding URIs keeps that principle alive.
> > > > Which begs another question: what/who applies the initial change ID to
> > > > a commit and when?
> > >
> > > These are all great questions, which the originally proposed format
> > > (fixed-width reverse-hex) has answers to. I think a URI would be
> > > strictly worse.
> >
> > Well, I think we still missed "what/who applies the initial change ID
> > to a commit and when."
>
> Some possible answers:
>
> - The user constructs the change ID from other things like
> bugzilla/jira/... ticket IDs.
>
> This is OK, but probably not what OP had in mind.
>
> If the user needs additional sub-IDs just add a -{N} suffix.
>
> It's on the user and utilities to keep usage consistent. CR tooling
> could refuse to allow reuse of change IDs after a CR is merged.
Yep; I think trailers are the common version of this today. And ofc,
see linkrot.
> - CR tooling edits your history to add that change ID when you create a
> CR
>
> This feels wrong.
Yep.
> - CR tooling creates an 'empty' CR just to acquire a change ID that the
> user can then add to their commits, either manually or with utility
> that does it for them (but which they invoke).
>
> This is alright, though a bit annoying. If the user fails to set the
> change ID, then it's no worse than today.
>
> - The user adds it when they update an existing CR by manually editing
> their history or invoking a utility that does it for them. The
> change ID comes from the CR.
>
> This is alright, though a bit annoying. If the user fails to set the
> change ID, then it's no worse than today.
>
> - The CR tooling adds an empty commit at the head just to hold the
> change ID and any additional metadata for each of the commits in the
> series.
>
> The user has to remember to fetch this commit, unless it's generated
> locally, but they're going to fetch anyways so this is ok.
>
> IMO empty commits are annoying. (That includes merge commits, but
> then I'm a rebase workflow / linear history zealot, so merge commits
> are also annoying because merge workflows are annoying.) But I would
> think that empty commits that include metadata about code review are
> reasonable and tolerable, especially if their subject lines are
> descriptive, like "CR: {cr title here}" or "CR {ID}: {title}", say.
>
> I think I'd only really like the first and last of the above.
>
> Nico
> --
Thanks for thinking out some concrete options on where IDs come from!
I think I've gotten even more confused on how they are supposed to be
used (which should probably inform the implementation), but hopefully
we're getting somewhere :)
--
D. Ben Knoble
