Support for parameters with arguments

[rzikm]

Was playing around with gitu and wanted to refresh some of the Rust I know. This is a very rough sketch which allows me to add arguments to flags (such as git log -n256 or in the future for git log -S).

It's far from optimal, or production quality, just wanted to float it out there and see if you agree with the direction I took. We can iterate from there.

[altsem]

👍
I think it looks pretty good! Happy to see you pick this up, arguments with values is kind of a missing feature.

See if the comments make any sense.

[altsem]

@rzikm Code looks good I think. Just some clippy complaints, "feat: " missing, tests.

[rzikm]

Did some work on this, I am not happy with how the Arg structure turned out, the fact that we need the Arg::new* functions be complicates it a bit. Maybe if we make it into a get_items() instead, I could simplify Arg a lot.

Anyway, it turns out this change greatly speeds up log screen because we no longer try to read the entire history :D very noticeable on Windows on large repos.

I removed the -S parameter for now, and added -F instead so that it can be checked that the shape works for other arg types.

[rzikm]

Got nerdsniped by this, so I reworked it a bit to allow arbitrary values without need for further Arg changes (that is, unless we want to add parameter completion or list of choices). Still had to fight the Borrow checker on few occasions so there may be some unnecessary copies involved, which I am unable to get rid off.

Should be good for another review

[altsem]

@rzikm I've been busy with work recently, I'll try find some time soon to look at it.

[altsem]

I dug around a bit. I managed to remove the need to implement Clone! :) It's over at the branch altsem/value-arguments.

I had some idea that the tests could:

• Create some commits (there's a commit helper function).
• Limit log to a number less than these commits (We've tested that limiting somewhat works).

Same with grepping:

• Create two commits, e.g. "Commit 1", "Another commit"
• Grep for "Another"
• Only one should be in the log!

I think it would be nice to flatten a few structs maybe. Could dig around a lot. But at some point I'm all for merging this into master and take it from there, it doesn't have to be perfect initially :)

[altsem]

Perhaps they should be called init_args? Just to make it clear that it's creating them anew.

[rzikm]

Thanks a lot, now it looks reasonable to me,

I added some tests as asked.

[altsem]

I'm generally happy with the implementation.
Just found one problem: --grep when doing log other.

I tried to make src/tests/log.rs a bit more clear over at altsem/value-arguments. Also added a test for the above.

[altsem]

No big deal, but.

I would put these more unit-y tests in the same file as the implementation. (like in src/key_parser.rs)

The tests in src/tests used to be in tests/ as proper "integration tests".
However they were moved just to make it easier to inspect internals if really needed.

[rzikm]

Just found one problem: --grep when doing log other.

Right, this might be more tricky to fix, State.close_menu call seems to throw away the not-yet used arguments, I would have to parse the values from the [OsString] args, which I don't like.

Maybe we can keep the pending_menu a bit longer? What do you think?

[altsem]

I haven't exactly grasped what's going on. pending_menu (and state of the arguments) should exist until the "log other" rev prompt is done and "log other" callback has been executed I think.

edit: I do think getting rid of the args parameter in the callback would be nice.
I'm playing around a bit trying to do this.

Also found that grepping for something that does not match seems to cause a panic:

cargo run -- -k 'l-Fxxxxxxxxx<enter>l'

[altsem]

@rzikm Figured I'd share. After some digging I think I found an approach that might work.
The idea is to:

• Remove the args: [&OsString] parameter from prompt callbacks.
• Hide the menu when a prompt is active (you can still access its args).
• Call state.close_menu() explicitly in each action/prompt.

This way we can close the prompt where appropriate (some actions don't), and after the args have been assigned to commands.

My work so far: 99a75bb

Heading for vacay and might continue next week :)

[rzikm]

Thanks, I did not have time to spend on this last couple of days, hopefully I will get to it this week so we can finally draw this to finish line.