Misc tab refactoring

[th1000s]

Just internal refactoring, except for the new silent limit of 64 for --tabs which might affect very eccentric user configs. However I think arbitrary lengths can be enabled again once I add the option to allow user configurable tab replacement chars.

And related, in my opinion the default tab size should be changed from 4 to 8, as that is what most editors default to (not to be confused with the default indent width of e.g. 4 in Rust). What do you think @wescande @nickelc @phillipwood?

[dandavison]

Is there an argument for the fold over

        for c in line_bytes.iter() {
            if *c == b'\t' {
                expanded.extend_from_slice(SPACES[..tab_width].as_bytes());
            } else {
                expanded.push(*c);
            }
        }

here?

[th1000s]

Less mut and thus more idiomatic? And maybe the fold one vectorizes better (well, I checked on godbolt, neither do currently...)

[wescande]

I may be missing something obvious but why not a simple split + join ?

line.split('\t').collect::<Vec<_>>().join(&SPACES[..tab_width])

Or do we want to improve expansion and make it expand as tab expand in editor (aka not a fixed length but they extend up to a modulo

Something like this:

let (expanded, _len): (String, _) = line.chars().fold(("".into(), 0), | (s, len), e| {
        if e != '\t' {
            (s + &e.to_string(), len + 1)
        } else {
            let tab_len = tab_width - (len % tab_width)
        ;
            (s + &SPACES[..tab_len], len + tab_len)
        }
    });

[th1000s]

The .collect() creates a temporary vector, however itertools::join can use the split-iterator directly, so that's what I use now.

And indeed, using fold is from when the original idea was to be tabstop aware -- but this clashes with the raw lines which are also passed through this function. But I will look at that next.

[wescande]

Why as_bytes() and not chars().
Using chars from the beginning may allow the code not to check again std::str::from_utf8(&expanded) at the end of the expansion.
If it was valid before expansion, it will still be after it.

[th1000s]

I was using bytes directly because I only needed ascii, and not using chars() might be faster if I also do the unsafe bytes -> str conversion. But I don't want to use unsafe unless a benchmark points to this as a bottleneck which is very unlikely. So yes, I could have used chars() right from the beginning.

[wescande]

Can you add a test that try to expand a utf8 char that has the tab ascii (0x9)code in it?

like this for example:
क

U+0915

[th1000s]

Well, that's the beauty of the utf-8 representation of unicode codepoints, all non-ascii bytes have the highest/0x80 bit set and a tab does not, so no such utf-8 char exists (unlike in utf-16).

[th1000s]

Thanks for the feedback!

Also, the silent limit of 64 is gone now, and a new commit which sets the tab limit to 8 has been added.

[phillipwood]

And related, in my opinion the default tab size should be changed from 4 to 8, as that is what most editors default to (not to be confused with the default indent width of e.g. 4 in Rust). What do you think @wescande @nickelc @phillipwood?

I'd support changing the default tab width to 8. In principle delta could use libgit2 to read the tabwidth attribute for each file but I suspect that would be quite a bit more work as it would need to check the attributes and fall back to config.

[dandavison]

Great, thanks everyone! (I didn't have a strong reason for 4; I think I thought that was a common default.)

[th1000s]

I just read up on git's core.whitespace settings since at least the global config for the tab width could easily be considered. However it seems this is mostly used for whitespace error detection, so quite unlikely that someone has this set. git diff etc. will always output a literal tab when there is one, then it is the pager's job to display it properly.