OSC-1717 draft spec

[stefanhaller]

This was created as part of the work that happens in #5732; broken out into its own PR to make it easier to comment on.

[stefanhaller]

Input from @scottchiefbaker from #5732:

1. In this new metadata mode lazygit (or whoever) emits an environment variable EMIT_OSC1717_METADATA = V1[,V2,…] when it's in select hunk/line mode.

Yes.

2. If a pager sees this environment variable it needs to output a "handshake" as the first part of it's output indicating what version of OSC1717 it understands? "\e]1717;1\e\"

Not what version it understands, but what version it actually uses in this output, based on what the env var requested and what the pager itself supports. Section 3 of the spec is very clear on that.

Note that my goal is to never need a v2 of the spec, so the handshake will hopefully stay a simple "I'm requesting v1 -> ok, I'll emit v1" forever.

3. Will each line of context (the lines that are NOT changing) need a OSC1717 record? In the case of d-s-f which does line-by-line output, every output line will have a record?

Yes. This is important because if you use lazygit with a very large context size, you might be looking at an entire page of context without any change line in sight; you still want to be able to select those and hit e on them, for example.

4. From section 6.1 "One record per region, at the region's start".

In a line-by-line pager like d-s-f, this means one record per output line, so in your example that would be 19. A side-by-side pager will combine two original diff lines into one output line, so this one line will carry two records, and the word "region" refers to sections within that one output line that render one or the other original diff line.

In section 4.3 it says "context, new line 10" but I think it would be more clear as "context, line 10" because it's not new.

"Old line" and "new line" refer to line numbers in the old file (before applying the diff) and in the new file (after applying the diff). In the git project they often call these pre-image and post-image. A context line can have different old line and new line numbers, so it is important to say that records for context lines carry the new line number. We were actually considering whether we should require them to carry both, but lazygit doesn't need the old line number, so we thought it might be simpler for the pager not to have to include the old one.

[dandavison]

How about simplifying the env var name to OSC1717_METADATA?

I think leading with the important bit ("OSC1717") will be helpful for people scanning env output etc, and having the name be an imperative verb is a bit unusual among env var names related to development environments; they're most often nouns. I think it's OK to leave it implicit that if its set, supporting pagers should honor it.

[stefanhaller]

This makes a lot of sense, I pushed a fixup to change this.

[dandavison]

I'd like to question the use of the word "pager" to describe these applications. delta and diff-so-fancy are basically classic unix filters: read stdin, transform it, output to stdout. One could call them "diff renderers" perhaps? (Or "terminal diff renderers") They achieve paging functionality by piping their output to a pager (less). However, when their output is consumed by lazygit, no pager is involved (they only pipe to the pager when the output is going to a tty and therefore is presumably being consumed directly by a human).

[stefanhaller]

Ok, this is interesting. Lazygit has always called these diff renderers "pagers" (see the docs, and the lazygit user config is also called "pagers"), and I have never questioned the term. You are right of course that a pager is something like less that allows users to see content page by page (hence the name); I suppose lazygit uses the term because of the way it is implemented (by setting the GIT_PAGER env var); it's also common to set git's core.pager config to delta.

I pushed a fixup (821546b) to use "diff renderer" instead of pager, and a short paragraph explaining that you'll see the term "pager" in discussions about this, to avoid confusion.

[dandavison]

I think (but am not sure) that it might be clearer to fold this section ( 3. Negotiation handshake) into 2. Design at a glance so that it is clear that it is just informal initial description.

Then describe it formallly below in 4.4 The handshake record.

However, is it confusing that 4.4 comes after 4.1 despite the initial version-only handshake record being emitted before any full record?

[stefanhaller]

I think keeping section 3 separate is fine. It's also not just informal, but it only describes one half of the handshake (the format of the env var); the other half is specified in 4.4. I added 063b64b to add a reference to that section to make this clearer.

I could see reordering the 4. subsections (move 4.4 first), but I'm not sure it matters too much. Right now it is ordered by importance (the actual metadata records first), which I think is fine too.

[dandavison]

I'm sorry, I think I made a bad suggestion. "metadata" refers to the payload in the emitted OSC1717 records. But the value of the env var is versions, so if we are naming it as a noun it would be OSC1717_VERSIONS.

[stefanhaller]

To me, the main point about the env var is not to specify which version(s) are accepted, but to enable the emission of OSC-1717 in the first place; hence the original name EMIT_OSC1717_METADATA, as a request to the renderer. The fact that the value of the env var specifies the versions the host accepts is only a secondary aspect of this. I don't know -- "metadata" is a bit redundant, that's what OSC-1717 is about, so I could go with any of ENABLE_OSC1717, EMIT_OSC1717, or just OSC1717. I don't have strong preferences here.

[dandavison]

I agree with you that the versions are not the main point and that it's OK to just slip them in as the value of the env var.

I orginally wrote a longer version of my first comment but deleted most of it because it was unclear. I'm finding it a bit hard to think what the right name is for this env var, but I do think it's a relatively important part of the spec.

I think what this env var is is a way for TUIs / terminal emulators to inform diff renderers that the terminal/TUI in which they are running has a certain capability: the ability to handle OSC1717 messages. So it's perhaps analogous to the COLORTERM env var (see e.g. alacritty/alacritty#1526 (comment)).

So, we've mentioned two different semantics

1. Request that the diff renderer emit OSC1717 messages
2. Inform the diff renderer that the TUI they are speaking to will do something useful if they emit OSC1717 messages

I believe that (2) is the applicable semantics for the env var in this situation.

Does this help the naming choice? I'm not sure! It argues against the imperative verb (EMIT_XXX). But what does it suggest in its place? OSC1717=v1,v2? OSC1717_DIFF_CAPABILITY=v1,v2? DIFF_CAPABILITY=OSC1717,v1,v2?

[stefanhaller]

It's getting a bit philosophical here, but I actually think it's more (1) than (2). The COLORTERM example is not a good analogy for me: it informs the TUI what capabilities are available, so that the program can make decisions based on that and render its output differently. That's not the case here; the OSC1717 env only requests that the renderer emit OSC1717 sequences; what the host then does with those is of no interest to the renderer.

But after this discussion I feel that OSC1717=v1,v2 is probably the best middle ground, given how neutral it is.

[dandavison]

Sounds good. I don't have a better suggestion than that at this stage. I haven't got to a point where I have a clear preference on what the name should be which probably means that I'm not understanding it properly / seeing the situation as I should.

[dandavison]

I'm pretty excited about this -- it's giving me and lots of other people something that we've wanted for years.

What I'm doing to test out the spec is getting claude to implement it in https://github.com/altsem/gitu (I was a magit user for so many years that I think I just have to stick to that for my interactive git UI; but in any case, I think involving a second git TUI is healthy for testing the spec).

So, that seemed to go well and didn't take long. What I realised on doing this is that I want gitu to render the hunk and file headers in delta's style, so that the experience is even more like having delta's rendering, with gitu's interactive functionality and outer chrome. Is this desire relevant to the research you've done on "f/h types", e.g. bd1a858?

I don't want to slow down the introduction of the feature into lazygit. Just wanted to point out that this is something that I think I'm going to want, so I'd like to make make things forward-compatible in that direction if possible. However, I have not fully understood the constraints here and what is / is not required of the spec to achieve what I want. I.e. is this something that I can "hack in" to the TUI, or does it really need spec support to make this robust for all user's delta configurations? I think the latter, right?

[dandavison]

So, getting claude to extend your delta PR with file/header messages and add to gitu support for external diff renderers (e.g. delta) and OSC1717 support, yields a gitu as well as a lazygit with proper delta support.

For example, here, gitu with delta in side-by-side mode, with interactive staging fully functional (AFAICS). It's been possible to run delta in emacs magit for a long time (and edamagit post-LLMs), but only ever --color-only, not with full freedom to render the diff as desired, and not in the terminal. That seems to be one of the most exciting developments in delta's history; thanks very much @stefanhaller! cc @th1000s and @altsem (hi and thanks for gitu!)

[scottchiefbaker]

As we start to implement this in our various tools, I think it would be helpful to have some sort simple validation or visual display to ensure that what we're emitting is correctly parsed by Lazygit (or whatever tool). Maybe some simple unit tests? Maybe a sample tool that reads and parses OSC1717 to show how it's parsed. I'm open to suggestion on this would look. Since this will ultimately affect multiple tools having a validation library/script/whatever would be very helpful.

[dandavison]

I think it would be helpful to have some sort simple validation or visual display to ensure that what we're emitting is correctly

I think that's a good suggestion @scottchiefbaker. My first thought is maybe something like this?

$ osc1717-checker --generate-diff --example 7 \
  | OSC1717=v1 {diff-so-fancy,delta,difftastic} --custom-options ... \
  | osc1717-checker --check-output --example 7

[stefanhaller]

@dandavison Your experiments with gitu are very interesting. I agree that it's very valuable to test the concepts against another host.

About f/h records: I'm really undecided about these. Right now lazygit gets by without them, using heuristics; for example, when pressing n to navigate to the next file it scans forward for the next content line whose file is different, and from there it scans backwards and skips over lines that have no record at all. After that, the line it ends up with is assumed to be the first line of the new file. This is not always true, sometimes it's off by one line (when that line is really a blank line of padding at the end of the previous file, and it can't tell the difference). I considered this good enough, but maybe it's not.

Similar for hunks: lazygit doesn't care about hunks (in the git sense) at all; when you ask it to navigate to the next "hunk" it actually scans for the next block-of-adjacent-change-lines-separated-by-context (there may be more than one of these in an actual git hunk), which I think is more useful. But other hosts may want to do something with actual git hunks, and then need to use a heuristic again (which shouldn't be hard; just search for lines that have no record at all, these must be hunk headers).

So, I'm open to adding f/h records if it helps. We tried this during prototyping, and the commit you cited (bd1a858) explains what was tricky about it. I also have implementations of this both for delta and for difftastic (we didn't make one for diff-so-fancy, I think). I didn't check how my delta patch differs from yours; you might have a look if you're curious.

My biggest concern was: the code that is needed to support f/h headers seems to be substantial, judging just from the amount of changed lines; this felt like too much of a burden and could make adoption harder for other diff renderers. And it doesn't make sense to make these optional, as the prototype doc explains. On the other hand, I also want to avoid having to do a v2 in the future, so if we think we might need them at some point, we should add them now.

My biggest question is whether the line number should be mandatory or optional for "f" records. In delta it is difficult to obtain the line number of the first hunk of the file at the point where the file header is written, so it leaves it empty in my patch (concretely this means that if you press e in lazygit it opens the file but doesn't jump to any line, leaving the cursor where it is. Probably fine.) In difftastic the situtation is different: difftastic doesn't have file headers at all, only hunk headers, so we decided to emit an "f" record rather than "h" for the first hunk in each file. And in difftastic it does carry a line number, because it was easy to do and because all hunk headers have one.

So, should we leave this to the renderer to decide? It will influence how the host behaves (although only in minor ways). We could also say that "f" headers never carry a line number, I think that could also be ok.

[scottchiefbaker]

@dandavison I like that example. Ultimately this is something we'd probably all want to roll into our unit tests. Something simple and recreateable would be ideal.

[dandavison]

@stefanhaller thanks for your reply about f/h records. I need to acknowledge that my day job doesn't leave much time for other software and so I am going to be very slow to participate here. On a positive note, it does appear that in my LLM-generated solutions, the f/h support was added with relatively few lines of code:

• delta f/h emission: https://github.com/stefanhaller/delta/compare/osc-1717-metadata...dandavison:osc-1717-metadata-extensions?expand=1
• gitu f/h handling: altsem/gitu@0758ae0?w=1