Blame line numbers

[th1000s]

Adds --blame-line-number-suffix "off|{}|{block}|{every-N}", (and, technically ""):

--blame-line-number-suffix <blame-line-number-suffix>
  Separator between the commit metadata and code sections of a line of git blame output. Contains the line
  number by default. Possible values are "off" to disable line numbers or a format string which may contain
  exactly one placeholder. These are: "{}" to display the line number on every line; "{block}" to only show
  the line number when a new blame block starts; or "{every-N}" to show the line number with every block and
  every N-th (modulo) line [default: │{:^4}│]

I really want my blame line numbers (and I want them center-right aligned :) - though the usefulness of every-N is debatable I admit.

[dandavison]

I'm thinking about how to increase consistency with other options (I think that with so many options, consistency is valuable if we are to retain some sanity). How about

• Call this blame_separator_format. All the other options that have {placeholder}s have a name like *-format.
• Support {n} to mean: replace this placeholder with the current line number. This gives consistency with line-numbers-{left,right}-format, sort of -- those are {nm} and {np} to distinguish the minus and plus line numbers.
• Is it natural that empty placeholder {} means insert line number, or should we not support that?
• Now, the way I'm thinking of it, {block} and {every-N} are "magical" in that they are not placeholders in the sense that it's used elsewhere, but rather directives saying when line numbers should be inserted. I'm wondering whether{n:block} and {n:every-N} are even more justifiable, meaning "insert line numbers, but subject to a certain policy".

I note that hyperlinks-file-link-format uses {line} to mean current line number. Possibly that should be changed to {n}, although the breaking change is probably not worth it. Maybe at least that one should have {n} support added and maybe {line} dropped from docs or mentioned as deprecated.

[dandavison]

Also

• Do you think we need the off special case? It's the same as │ I believe. Perhaps, users who care are going to be just as capable of looking up the default and copying the │ character, as they are of learning the off special case?

[th1000s]

Good idea, now using {n}, {n:block} and {n:every-N} and extended the format regex to support it. Still need to work around StructOpt displaying {n} as a newline. An empty {} could also be supported, but that would make the help text more complicated.

And I think just writing off is easier than copy/pasting a unicode │, plus it prevents mistaking it for a plain ascii |pipe and wondering why the output doesn't look as good as with line numbers.

[dandavison]

And I think just writing off is easier than copy/pasting a unicode │, plus it prevents mistaking it for a plain ascii | pipe and wondering why the output doesn't look as good as with line numbers.

OK, I'm convinced. What do you think about using none instead of off? That would be consistent with --syntax-theme=none.

[dandavison]

This is great, thanks for doing it. I realised after doing the first version of blame support that line numbers were a bit of a glaring omission in the blame output but hadn't done anything about it.

[th1000s]

Line numbers are now disabled with none. Also the help text must use {n:} because clap uses {n} for newlines and it seems no verbatim_doc_comment or other option can disable that. Probably there for backwards compatibility, as it is undocumented.

The type format specifier can now start with a _, e.g. {n:^4_block}.

[dandavison]

Great, let's get this merged. I've rebased it against master: #916

However, I'm seeing an exciting error locally when trying to run the tests on this branch (whether the rebased version or not):

cargo test
   Compiling git-delta v0.11.3 (/Users/ddavison/src/delta)
error: internal compiler error: compiler/rustc_typeck/src/collect.rs:1089:14: impossible case reached

thread 'rustc' panicked at 'Box<dyn Any>', compiler/rustc_errors/src/lib.rs:1169:9
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

note: the compiler unexpectedly panicked. this is a bug.

note: we would appreciate a bug report: https://github.com/rust-lang/rust/issues/new?labels=C-bug%2C+I-ICE%2C+T-compiler&template=ice.md

note: rustc 1.58.0 (02072b482 2022-01-11) running on x86_64-apple-darwin

note: compiler flags: -C embed-bitcode=no -C split-debuginfo=unpacked -C debuginfo=2 -C incremental

note: some of the compiler flags provided by cargo are hidden

query stack during panic:
#0 [adt_def] computing ADT definition for `format::FormatStringPlaceholderData`
#1 [typeck_item_bodies] type-checking all item bodies
end of query stack
error: could not compile `git-delta`

I also note that you have disabled CI on this branch which feels...non-coincidental? :) Are you also seeing that error or is it something about my local environment? I did do rustup update yesterday for an unrelated reason.

[dandavison]

I've rebased this branch, and also reverted the change to the GitHub actions CI config.

Just in case I made a mistake in rebasing, the original branch commits are in branch blame_line_numbers_before_rebase.

I have no idea yet about my rust compiler crash, but that appears to occur in my local development environment only.

There is one test failing on the 32 bit machines:

error: literal out of range for `usize`
Error:    --> src/format.rs:310:14
    |
310 |         dbg!(9876654321_usize.width_for_center_right());
    |              ^^^^^^^^^^^^^^^^
    |
    = note: `#[deny(overflowing_literals)]` on by default
    = note: the literal `9876654321_usize` does not fit into the type `usize` whose range is `0..=4294967295`

[th1000s]

I disabled the tests to not waste compute time since the "Center Align numbers right-ish" commit breaks just about everything involving line numbers. Is the new center-right alignment for numbers Ok with you? If so I can get to fixing these and add actual asserts to the new ones.

I noticed the compiler crash as well, seem to be due to incremental compilation again so it does not affect release. I can't reproduce it though, but it was already reported. The fix is to just remove the debug objects: rm -r target/debug/ and rebuild (cargo clean seem to remove the unaffected release/ subdir as well, despite not using --release).

[dandavison]

Is the new center-right alignment for numbers Ok with you?

Oh, I'm sorry to have held that up. Yes, I agree the "center-aligned" integers look better rightish.

[th1000s]

Ok, push one more commit to re-enable nojobs disabled CI pipelines, I hope I remember that next time :)

[dandavison]

Excellent, merged. Fixes #928