Enabling line-numbers breaks -/+ prefix stripping and staging individual hunks

The-Compiler commented 4 years ago

Hey! I'm relatively new to emacs and found this package (and delta itself). Thanks for all your work, it looks amazing! :star_struck:

However, it looks like enabling the line-numbers feature in delta breaks various functionality in magit-delta:

The + and - prefixes aren't stripped anymore
I can't stage individual hunks anymore from the magit status buffer - when I try, I get "corrupt patch at line xy" from git

Screenshot:

This is on Archlinux, with emacs 27.1, Doom v2.0.9. I haven't found out what magit-delta version I'm running - Doom Emacs uses straight, so pkg-info-package-version doesn't know about magit-delta, and straight-freeze-version somehow errors out. Happy to provide more information if you tell me how to get it :slightly_smiling_face:

dandavison commented 4 years ago

Yes, there's a little bit of a mess here that needs sorting out.

TL;DR: line-numbers cannot be used with magit-delta (see below). If you want to be free to use line-numbers with delta on the command line (and why not!) then (EDIT) see the suggestion below. the best workaround I can suggest currently is to add --no-gitconfig to your magit-delta-delta-args. That will make your magit-delta config independent of your main delta config. But then of course you'll have to also add to magit-delta-delta-args all the other delta command line args that you want delta to be using when it is invoked by magit-delta (e.g. colors, syntax-highlighting theme, etc).

Here's the situation:

line-numbers fundamentally cannot be used with magit-delta. This is because the string that delta gives to magit must be a valid git patch (because magit assumes whatever is in its diff buffer is a valid patch and sends it to git apply to stage changes). Similarly (and perhaps more obviously), side-by-side cannot be used. Nor can any of delta's "decorations" (the boxes and lines, etc)
This is the motivation for --color-only being included in the emacs variable magit-delta-delta-args. In fact, magit-delta puts that back in the args if you remove it.
However, --color-only is also the recommended way to use delta with git add -p (see example gitconfig in delta README.
The semantics of color-only are intended to be "just add colors to the git output, but do not make any structural changes to be made to the git output".
But, people want to use line-numbers with git add -p. So currently color-only does not actively disable line-numbers if the user has set it.

dandavison commented 4 years ago

Actually, I think there's a better solution involving using delta "features". Try setting your magit-delta-delta-args something like this

(setq magit-delta-delta-args
  '("--24-bit-color" "always"
    "--features" "magit-delta"
    "--color-only"))

Then magit-delta will not use the line-numbers and decorations features you've declared in your gitconfig. And you can add magit-delta -specific config in a section in gitconfig like

[delta "magit-delta"]
    whitespace-error-style = green reverse  # or whatever

wyuenho commented 3 years ago

@dandavison I have this config and magit-delta-delta-args set to only '("--features" "magit-delta"), but line numbers is still showing in my magit diff and break hunk staging.

[delta]
line-numbers = true

[delta "magit-delta]
line-numbers = false

dandavison commented 3 years ago

Hi @wyuenho, instead of setting the line-numbers boolean directly in the main delta stanza, can you try activating it via its feature:

[delta]
    features = line-numbers

[delta "magit-delta"]
    line-numbers = false

I just tested this and it seemed to fix it. I'm not claiming this is obvious! I definitely admit that delta config when using all the various possibilities has some subtleties and complexity, but it is at least pretty configurable. There are probably improvements to be made to make it more consistent.

(There's a missing closing " in the gitconfig you posted but I think that's just a typo here not in your actual gitconfig.)

wyuenho commented 3 years ago

Does that mean I can only really have one set of config?

dandavison commented 3 years ago

Does that means I can only really have one set of configs anyway?

No, pretty much anything is achievable I think -- I'd be happy to try to provide a config implementing any requested behavior. The config I posted above results in line-numbers being active for delta in normal usage, but inactive when used by magit-delta. The features list can contain any number of features. There is an environment variable DELTA_FEATURES that can be used to modify active features on-the-fly (e.g. in shell-aliases etc).

For the adventurous, a feature can have its own feature list, thus yielding a tree of sets of key-value pairs, in which any conflicts are resolved according to a tree traversal order which should hopefully be intuitive but is documented in the code (here and here).

wyuenho commented 3 years ago

Your config isn't equivalent to mine. I don't want the main config to reference to magit-delta config unless I explicitly tells delta to in the command line.

I don't understand what's happening here. What's the relationship between [delta], [delta "feature"] and delta's defaults? Delta doesn't turn on line-numbers by default, but omitting it from the main section does? Does a feature section override the delta section? The answer seems to be no in both cases. What I really need is, when I tell delta to use a feature, delta will use that feature's config alone and merge with the defaults, but not any other sections.

dandavison commented 3 years ago

What's the relationship between [delta], [delta "feature"] and delta's defaults?

Does this help?

[delta]
    # settings here will always be applied unless the user supplies --no-gitconfig on the command line
    # These settings will override delta's defaults

[delta "my-feature"]
    # This is just a named collection of key-value pairs.
    # It has no effect at all unless the user has added "my-feature" to their "features" list.
    # They could do that either
    # (a) on the command line via `--features`, or
    # (b) in the main `[delta]` section via `features = my-feature some-other-feature`
    # (c) by setting the env var: `DELTA_FEATURES="my-feature some-other-feature"`

I don't want the main config to reference to magit-delta config unless I explicitly tells delta to in the command line.

That's right, that's how it works. The [delta "magit-delta"] settings are just sitting there doing nothing, until you pass --features=magit-delta on the command line, or otherwise add magit-delta to the list of active features.

wyuenho commented 3 years ago

The crux of the matter is, the same key in a feature section does not override the main section, because anything in the main section always applies, that's why line numbers are still showing up in my config when invoking delta --features=magit-delta. This is undesirable, either feature config should override or replace an ancestor section, there's no way to set the custom defaults in the main section now as they always override any features. That's not what defaults are suppose to do.

dandavison commented 3 years ago

Ah I'm sorry, I should have linked this issue: https://github.com/dandavison/delta/issues/446

the same key in a feature section does not override the main section, because anything in the main section always applies,

I think that's a bug specific to line-numbers (https://github.com/dandavison/delta/issues/446 ), or to delta's "builtin features". I'd like that to be fixed soon. But it's not generally true: feature config does override the main section in general.

dandavison commented 3 years ago

So my suggestion

[delta]
    features = line-numbers

[delta "magit-delta"]
    line-numbers = false

is a workaround for that bug. What you did definitely should have worked.

DrWaleedAYousef commented 3 years ago

Sorry for jumping in. I arrived here because I searched for the side-by-side option from withing emacs using magit. I found [this reply above] that confirms it does not work with magit. However, the reply says as well line-number does not work with magit. It works with me! Is this an update to a recent version?

kaushalmodi commented 3 years ago

@dandavison The suggestion in https://github.com/dandavison/magit-delta/issues/13#issuecomment-754803416 does not work for me.

I started using delta a few days back and I love it! The I learned that you also developed magit-delta, so I tried that out, but the diff folding (M-2, M-4 bindings) in magit-status breaks for me even after I applied your suggestion above.

Diffs polluting the magit status

Diffs show/hide using `M-2`/`M-4` if I remove `features = line-numbers`

Issue

As I see, if I leave my .gitconfig as shown in the second screenshot, I cannot use delta from the terminal when running git diff, etc. there.

Do you have any suggestions that work on the latest delta version?

Thanks!

Versions:

delta: 0.8.2
magit: Latest version from melpa as of today (2021-10-22)
emacs: Latest emacs-28 branch build as of today (GNU Emacs 28.0.60 (build 2, x86_64-pc-linux-gnu, GTK+ Version 3.22.30, cairo version 1.15.12) of 2021-10-22, built using commit 06c944cff1a8a348b9c01a92891bd12576c0896d)

kaushalmodi commented 3 years ago

Quick update!

It looks like this config works and makes both terminal-side and magit-side delta invocation happy.

# In .gitconfig
[delta]
    # https://github.com/dandavison/magit-delta/issues/13
    # line-numbers = true    # Don't do this.. messes up diffs in magit
    #
    side-by-side = true      # Display a side-by-side diff view instead of the traditional view

I simply removed line-numbers = true from the config and that makes magit-delta work.
I still have side-by-side = true in there, and that doesn't seem to affect magit-delta!
But because side-by-side is enabled, the line numbers also seem to be auto-enabled when I used delta from the terminal.

Again, this would work only for folks who like to enable side-by-side.

brc commented 1 year ago

Using @dandavison's suggestion worked. :+1:

But I could only assign magit-delta-delta-args in Spacemacs via customize; trying to use (add-to-list) or (setq-default) in various Spacemacs places (such as (dotspacemacs/user-config)) wouldn't actually change the value. Then again, I use Spacemacs because I don't know Emacs well enough (and subsequently, I don't know Emacs well enough because I use Spacemacs)!

At one point, I had read about the relationship between (setq) and customizable variables, and what you should or shouldn't do, but I don't recall any details (...pretty much ever).

Thank you, @dandavison.

dandavison / magit-delta