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]

@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.

[rzikm]

Thanks a lot, now it looks reasonable to me,

I added some tests as asked.

[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: https://github.com/altsem/gitu/commit/99a75bbe69cb49342510c2a6d2d7b215679464fa

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.

[rzikm]

I am sorry for dragging this, I was busy last couple of weeks, I hope to get to this soon

[altsem]

That's alright, no need to stress.

[altsem]

I continued on the branch: altsem/value-arguments

Made a test for grepping for something that doesn't exist. It seems that we get a crash when showing a screen with no lines.

Otherwise I think I solved the other issues :)

[rzikm]

I fixed the failing test by ensuring there is always at least one item in the log screen, I did not want to start rewriting the screen logic to allow for empty item list.

Let me know if there is anything more missing.

[codecov[bot]]

Codecov Report

Attention: Patch coverage is 92.73927% with 44 lines in your changes missing coverage. Please review.

Project coverage is 87.47%. Comparing base (378f832) to head (2ce391d).

Files	Patch %	Lines
src/ops/commit.rs	0.00%	20 Missing :warning:
src/ops/editor.rs	88.23%	6 Missing :warning:
src/ops/rebase.rs	82.60%	4 Missing :warning:
src/menu/arg.rs	97.87%	3 Missing :warning:
src/items.rs	90.47%	2 Missing :warning:
src/menu.rs	88.23%	2 Missing :warning:
src/ops/revert.rs	88.23%	2 Missing :warning:
src/ops/unstage.rs	50.00%	2 Missing :warning:
src/ops/copy_hash.rs	0.00%	1 Missing :warning:
src/ops/show.rs	50.00%	1 Missing :warning:
... and 1 more

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #159      +/-   ##
==========================================
+ Coverage   86.97%   87.47%   +0.49%     
==========================================
  Files          58       59       +1     
  Lines        4514     4981     +467     
==========================================
+ Hits         3926     4357     +431     
- Misses        588      624      +36     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[altsem]

I think we're ready. Good job! :)

I did a rebase onto master, and will do a hack to add some entries to the changelog. Will make a release soon :raised_hands:

[altsem]

Pushed here with a rebase instead / some empty commits just for changelog entries purposes: https://github.com/altsem/gitu/pull/195