remove tabled

[benmkw]

follow up from #1292

I did another profiling run on gitoxide main with cargo build --release --timings --no-default-features and was wondering if tabled and its (derive) dependencies are really needed.

It turns out there is really just one place where its used and while I don't yet understand the exact output semantics (I feel like a manual implementation could be more clear here) it seems that with clever use of the rust format specifiers one could probably go a long way.

tabled usage:

impl Tabled for Record {
    const LENGTH: usize = 3;

    fn fields(&self) -> Vec<Cow<'_, str>> {
        let mut tokens = self.config.split('.');
        let mut buf = vec![{
            let name = tokens.next().expect("present");
            if name == "gitoxide" {
                name.bold().green()
            } else {
                name.bold()
            }
            .to_string()
        }];
        buf.extend(tokens.map(ToOwned::to_owned));

        vec![
            Cow::Borrowed(self.usage.icon()),
            buf.join(".").into(),
            self.usage.to_string().into(),
        ]
    }

    fn headers() -> Vec<Cow<'static, str>> {
        vec!["icon".into(), "key".into(), "info".into()]
    }
}


let mut table = tabled::Table::new(sorted);
    let table = table.with(Style::blank()).with(Extract::rows(1..));
    println!(
        "{}",
        if let Some((terminal_size::Width(w), _)) = terminal_size::terminal_size() {
            table.with(Width::wrap(w as usize).keep_words().priority::<PriorityMax>())
        } else {
            table
        }
    );

[Byron]

Thanks for researching this!

If removing the derive in favour of a manual implementation works while really only adding the code demonstrated above, I am all for it.

But also to provide some guidance, to me compile time optimisations should be focussed on gix (the library crate), and personally I consider gix (the binary) to be a development tool where anything goes as long as it facilitates or accelerates development. That doesn't mean to be wasteful, but generally I wouldn't optimise here.

With all that said, if you do find an opportunity that seems worth it, I'd always want to bring it in.

[benmkw]

works while really only adding the code demonstrated above, I am all for it

As far as I can see the new (or more or less your old) implementation does only lack the line wrapping (noticeable for smaller terminals). Because this seems to only be used when running config-tree this does not seem like a large limitation.

Unfortunately I could not get the config name column to be padded to 50 characters (: <50 does not change the output for me, not sure why, this works just fine). Maybe something about terminal escapes (?).

Given that this removes 14 packages it seems somewhat worth it to me.

I consider gix (the binary) to be a development tool where anything goes as long as it facilitates or accelerates development. That doesn't mean to be wasteful, but generally I wouldn't optimise here.

That makes sense. I can only add to that that while I had to recompile some times to tweak the format strings the compile times sure do add up, but thats also just rust dev in general :D

[Byron]

Unfortunately I could not get the config name column to be padded to 50 characters (: <50 does not change the output for me, not sure why, this works just fine). Maybe something about terminal escapes (?).

All this formatting can get complicated and sometimes it feels a little like luck if it actually looks like it should 😅.

Given that this removes 14 packages it seems somewhat worth it to me.

I wasn't aware, but yes, that sounds valuable especially if that removal also reduces compile time meaningfully.

That makes sense. I can only add to that that while I had to recompile some times to tweak the format strings the compile times sure do add up, but thats also just rust dev in general :D

Agreed, and it's particularly bad when working with gix (the binary), and it won't get better as code just accumulates there. Even gix, the library, takes some time to compile.

I suppose that makes the split into plumbing crates even more valuable, as these typically compile very fast and make testing simpler. Thus far I am quite happy with the overall performance though, and think it can be good for another 3 years or so (without having to upgrade hardware).

[benmkw]

Looking around more I also found itertools as a dependency which is only used for join and tuple_windows in a total of 3 places and already has the izip macro vendored (although that's only used once so far) (I think this really should be in std, I have had the need for that many times as well). I don't want to drive this in a direction which creates more noise but thought I'd mention it briefly.

One noticeable benefit of these changes in btoi I think is the opportunity for better naming as you did/ more domain specific adaptions. In the tabled case I think the shorter code is actually easier to read.

[Byron]

Oh my! Itertools with about 10 seconds! I actually never really want to use it by feel its convenience forces my hand. Indeed, maybe it's feasible to inline the few things that are used here, but I have a feeling it's going to be harder and might add large swaths of code.

I don't want to drive this in a direction which creates more noise but thought I'd mention it briefly.

Yes, let's take one step at a time while keeping other opportunities in mind as well.

[benmkw]

Yes, let's take one step at a time while keeping other opportunities in mind as well.

The main issue with this PR from my POV is that the alignment does not yet work, but I'm not yet sure how to best debug this. The blinking is not the reason at least (tried without it).

but I have a feeling it's going to be harder and might add large swaths of code.

I would try, I have a padded iterator inlined here which was not much code https://github.com/benmkw/p2r/blob/eae398a8388b7ce6b1d1663b5499fdcf7fe68cb6/p2r/src/util.rs e.g..

For the two uses of join one can probably collect in a vec and join afterwards cause its not in any hot code.

The one usage of tuple_windows could be replaced with a regular for loop and a prev variable instead of a fold.

[Byron]

The main issue with this PR from my POV is that the alignment does not yet work, but I'm not yet sure how to best debug this. The blinking is not the reason at least (tried without it).

It's surprising that the formatting is affected at all given that the only think I thought would change is the addition of the manual implementation of what the tabled derive does. Would that mean it does something special that's not reproduced? And if so, could expanding the macro help seeing what that is?

Regarding the removal of itertools, everything you said sounds like a net-win to me, and I'd love to use the opportunity to stop the proliferation of itertools in its tracks - when it's already there, I will use it, if not, I will think twice.

[benmkw]

The output now looks like:

👌️ gitoxide.objects.cacheLimit                   ❗️If unset or 0, there is no object cache. overridden by 'GIX_OBJECT_CACHE_MEMORY'❗️
👌️ gitoxide.objects.replaceRefBase               ❗️overridden by 'GIT_REPLACE_REF_BASE'❗️
👌️ gitoxide.pathspec.glob                        ❗️pathspec wildcards don't match the slash character, then needing '**' to get past them. overridden by 'GIT_GLOB_PATHSPECS'❗️
👌️ gitoxide.pathspec.icase                       ❗️Compare string in a case-insensitive manner. overridden by 'GIT_ICASE_PATHSPECS'❗️
👌️ gitoxide.pathspec.inheritIgnoreCase           ❗️Inherit `core.ignoreCase` for defaults in pathspecs❗️
👌️ gitoxide.pathspec.literal                     ❗️Make the entire spec used verbatim, the only way to get ':()name' verbatim for instance. overridden by 'GIT_LITERAL_PATHSPECS'❗️
👌️ gitoxide.pathspec.noglob                      ❗️Enable literal matching for glob patterns, effectively disabling globbing. overridden by 'GIT_NOGLOB_PATHSPECS'❗️
👌️ gitoxide.ssh.commandWithoutShellFallback      ❗️is always executed without shell and treated as fallback. overridden by 'GIT_SSH'❗️
👌️ gitoxide.tracePacket                          ❗️overridden by 'GIT_TRACE_PACKET'❗️
👌️ gitoxide.user.emailFallback                   ❗️overridden by 'EMAIL'❗️
👌️ gitoxide.userAgent                            ❗️The user agent presented on the git protocol layer, serving as fallback for when no `http.userAgent` is set❗️
🕒 http.<url>.*                                                 planned ℹ definitely needed for correctness, testing against baseline is a must ℹ
🤔 http.cookieFile                                              not planned ℹ on demand ℹ
🤔 http.curloptResolve                                          not planned ℹ on demand ℹ
🤔 http.delegation                                              not planned ℹ on demand ℹ
🤔 http.emptyAuth                                               not planned ℹ on demand ℹ
👌️ http.extraHeader                                             ❗️fails on illformed UTF-8, without leniency❗️
✅ http.followRedirects                                         
👌️ http.lowSpeedLimit                                           ❗️fails on negative values❗️

(gitoxide is still green)

One issue could be that the rust format specifiers don't understand terminal escapes and thus calculate the length differently https://stackoverflow.com/a/69611481 so this was one reason why it was pulling in so many crates.

The question then is how to best replicate that behaviour in a simple way, if desired.

[benmkw]

The question then is how to best replicate that behaviour in a simple way, if desired.

• removed green and bold formatting from the config string, any fancy colors in the description can stay as they are just appended

❓ fetch.fsck.skipList                               : ❓
❓ fetch.fsckObjects                                 : ❓
✅ fetch.negotiationAlgorithm                        : 
🤔 fetch.output                                      : not planned ℹ 'gix' might support it, but there is no intention on copying the 'git' CLI ℹ
🕒 fetch.parallel                                    : planned
🕒 fetch.prune                                       : planned
🕒 fetch.pruneTags                                   : planned
🕒 fetch.recurseSubmodules                           : planned ℹ Seems useful for cargo as well ℹ
❌ fetch.showForcedUpdates                           : not applicable: we don't support advices
🕒 fetch.unpackLimit                                 : planned
🕒 fetch.writeCommitGraph                            : planned
👌️ gitoxide.allow.protocolFromUser                   : ❗️overridden by 'GIT_PROTOCOL_FROM_USER'❗️
👌️ gitoxide.author.emailFallback                     : ❗️overridden by 'GIT_AUTHOR_EMAIL'❗️
👌️ gitoxide.author.nameFallback                      : ❗️overridden by 'GIT_AUTHOR_NAME'❗️
👌️ gitoxide.committer.emailFallback                  : ❗️overridden by 'GIT_COMMITTER_EMAIL'❗️
👌️ gitoxide.committer.nameFallback                   : ❗️overridden by 'GIT_COMMITTER_NAME'❗️
👌️ gitoxide.core.defaultPackCacheMemoryLimit         : ❗️If unset, we default to 96MB memory cap for the default 64 slot LRU cache for object deltas.❗️
👌️ gitoxide.core.externalCommandStderr               : ❗️overridden by 'GIX_EXTERNAL_COMMAND_STDERR'❗️
✅ gitoxide.core.filterProcessDelay                  : 

[Byron]

Thanks for the great work - I am amazed how close both versions are with 14 dependencies less and overall, much less and simpler code.

It's quite amazing to see what tabled actually does in this case, and I might say that it's certainly fine to drop it. Colors aren't too important.

But what I find important is the linebreak it makes for the second column. If terminal-size was re-added, it should be possible to compute linebreaks by hand without too much effort, presumably.

Is this something you think is possible on top?

[benmkw]

But what I find important is the linebreak it makes for the second column. If terminal-size was re-added, it should be possible to compute linebreaks by hand without too much effort, presumably.

sure I understand, terminal size is fairly small and I would not have removed it for the sake of codesize so it would be no problem to add it back.

The problem is the calculation of the displayed size of the usage section. I'll have to look into how to best do this/ if this is easily possible.

I think the tabled solution was a bit overkill because in this case the usage strings are fixed and the calculation does not have to be 100% robust but it should of course be at least reasonable.

Another way would be to basically record the size of the strings before adding escape codes and also remembering to number of emojis per string but yeah .. I'll have to see if there is a clean solution to that or if its easier to basically re-parse the escape codes and that way calculate the lenght. owo-colors does not seem to have a way to say "give me the underlying string without the terminal codes" as far as I could see which is a bit unfortunate for this use case (being tied to the API of a dependency is sometimes a real bummer), but maybe there is a way.

[Byron]

Right, I see what you mean - trying to adjust the right-hand size and split it needs to consider terminal escape codes, which makes this a whole lot harder than it otherwise would be.

Maybe limiting this to just unicode will be good enough, so no ansicodes have to be dealt with. On the right-hand side this seems to be just the blinking and bold text.

There is an excellent crate to help with all of that: anstyle: https://github.com/rust-cli/anstyle - it has a parse crate that can probably help with that.

But honestly, I am OK cutting back on that complexity as I am sure once simple line-breaks of simple unicode strings work, it will look good enough. I am not even a fan of the blinking which is now shown since I switched from Alacritty to Wezterm.

[benmkw]

I added manual line wrapping but decided to remove the ansi highlighting as it seems the interface conveys all the information just as well without it (e.g. making text white doesn't do much, on the other hand on a white background its probably not very readable).

The Records are also simplified a bit because one struct field could be used without a name and the None values can just as well be empty strings as there does not really seem to be a need to distinguish the states for this relatively simple task at hand (no Some("") values).

All in all this makes it a negative diff for the dependencies but also a negative diff for the code itself.

[Byron]

That's absolutely wonderful, so much better than before, and way more readable.

I really had to use tabled I guess just to not implement the wordwrap 😅.