Prototype: Fold the staging functionality into the main view

[stefanhaller]

This is a prototype of a major change to how staging works:

• there is no longer a separate staging (or patch-building) panel, this is now done directly from the main view
• for this purpose, the main view gets a selection when focused, which works the same way as the one in the former staging view
• this works even when a pager is used, as long as the pager supports the new OSC protocol described in https://github.com/stefanhaller/diff-line-metadata-spec
• many other things that I forgot to list here.

In order to illustrate this better I made a video to demo it. Unfortunately it got a little longer than I had planned, I'm not experienced in producing content like this.

Prototype branches for the three mentioned pagers are here: delta, diff-so-fancy, difftastic.

For feedback on the OSC spec, please comment on stefanhaller/diff-line-metadata-spec#1, not here.

[scottchiefbaker]

I'm about 8 minutes into your demo video and I'm already very excited. Conceptually it seems simple enough. I just need to add some "hidden" escape codes into the d-s-f output to mark changed lines?

[scottchiefbaker]

9. Where feedback is most wanted

1. The OSC number, 1717....

I don't know enough about OSC codes to have a strong opinion on this. But 1717 seems fine to me.

2. The env-var name and grammar (EMIT_OSC1717_METADATA=V1,…).

This seems fine to me I think.

3. The token-vs-line mismatch (§8) — should there be an m type, or is host-side inference the right home for it?

Not sure I have enough information to say on this.

4. Can your pager actually produce all four fields per region?

I believe so. I'd definitely like to work closer with you to beta-test this feature as there are a lot of technical details I don't 100% follow quite yet.

5. Content-lines-only scope (§5.5) — is anything lost for your pager?

I don't think so. d-s-f parses line by line, and retains header information so I should be able to call back to the last header when I encounter a line change.

[scottchiefbaker]

Having read the spec document and watched your video can I get clarification on a couple of things?

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.
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\"
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?
4. From section 6.1 "One record per region, at the region's start". In this hypothetical diff which contains: a line add, a line change, and a li ne delete, how many "regions" would that be? 3? 4? Or is region another way of saying "line" in which case there would be 20?

diff --git a/LICENSE b/LICENSE
index eb26539..028c6af 100644
--- a/LICENSE
+++ b/LICENSE
@@ -5,17 +5,18 @@ Copyright (c) 2016 So Fancy team
 Permission is hereby granted, free of charge, to any person obtaining a copy
 of this software and associated documentation files (the "Software"), to deal
 in the Software without restriction, including without limitation the rights
-to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
 copies of the Software, and to permit persons to whom the Software is
 furnished to do so, subject to the following conditions:

 The above copyright notice and this permission notice shall be included in
 all copies or substantial portions of the Software.

+FOOBAR
 THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
 IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
 FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
 AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
-LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM;
 OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
 THE SOFTWARE.

Help me understand the fields for the deleted line. Would it be: "\e]1717;1;d;;8;b/LICENSE\e\"? I don't fully understand old line vs new line.

For the added line. Would it be: "\e]1717;1;a;15;;b/LICENSE\e\"?

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.

[scottchiefbaker]

Possible minor hiccup? While attempting to implement the basics of this I came across a minor problem:

# Works fine
perl -E 'print "\e]1717;1\e\\"'

# Outputs `\`
perl -E 'print "\e]1717;1\e\\"' | less

The default mode with d-s-f it to pipe through less as a pager, and it appears less doesn't handle OSC escape sequences super cleanly.

[stefanhaller]

While attempting to implement the basics of this ...

I must not have made that clear enough: you don't have to do that, I did already. I just didn't post a (draft) PR yet because I first wanted to get feedback on the spec; but the changes are ready (apart from the necessary changes to the docs, if any, and to the changelog etc.). My branch is here if you want to try it.

it appears less doesn't handle OSC escape sequences super cleanly.

That's too bad, but it's not a problem, because d-s-f will only emit the sequences when it is running inside lazygit.

I'm taking your other questions over to #5731, that's where the spec should be discussed.

[scottchiefbaker]

Oh man... you didn't tell me you had most of the work done in d-s-f. You should have led with that. Your code looks pretty sane to me. Well done.

[nullromo]

Just want to say that this prototype looks very exciting and I would be ecstatic if these changes landed in an official Lazygit release.

[jesseduffield]

This looks amazing. This would be a serious game-changer for my workflow and I imagine many others as well.

[stefanhaller]

@dandavison @scottchiefbaker I want to make progress on this. I'm happy with the prototype and with the spec, and I want to start implementing it soon. Anything that is still worth iterating on or discussing before I productionize it?

My plan is to start working on it after the next lazygit release, which is scheduled for August 2. My rough estimate is that it will take between one and two weeks to get it all in; we then have a bit of time to test it thoroughly, and hopefully release it with the September release if all goes well. Which means we'd have to ship delta and d-s-f updates some time in late August so that they are available in time. Does that sound like it would work for you?

[dandavison]

That's fine with me @stefanhaller. I'm running your delta branch as my delta locally (with gitu) and will of course report any issues I encounter. From delta's point of view, I'll initially consider OSC 1717 support to be experimental in the sense that delta won't complain if there are some backwards incompatible changes in the way the feature works. Though delta might of course need help with PRs for evolving the support.

[scottchiefbaker]

@stefanhaller I'm fine with all the decisions we've made so far. I support moving forward with this. Will you be updated your diff-so-fancy branch with these proposed changes?

[stefanhaller]

Will you be updated your diff-so-fancy branch with these proposed changes?

@scottchiefbaker The PR is already up to date, it is ready to be merged once the fixups are squashed. I'll do that right now. (Please don't merge it just yet though.)

[stefanhaller]

This work was redone in non-prototype quality, merged to master, and released with v0.66.0. Closing.