Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(smartlog): Support for rendering a sparse smartlog #619

Merged
merged 3 commits into from
Nov 9, 2022

Conversation

claytonrcarter
Copy link
Collaborator

This description is still TBD, but I've cleaned up my local hacks. (A bit ... still more to do.) This version aims to target the sparse smartlog only at cases where the user has a requested a specific revset, eg by calling git smartlog foo() vs just git smartlog or via the internal calls thereof.

The main ideas is that there are different use cases for the smartlog. Sometimes you just want the default set, but if you ask for a revset maybe you don't want the ancestors filled in.

This expands the usage of the vertical ellipsis to indicate non-immediate ancestor links in the smartlog, as well as "false heads", where a commit has visible descendants, but none of them are included in the smartlog.

This includes the fix for the undo issues I mentioned in chat, but it still seems to have problems with being too aggressive about excluding rewritten/obsolete commits.

Example:

❯ cargo run -- smartlog
⋮
◇ b5c32ba 2d (master) fix(rewrite): don't warn about bug with missing reworded commits
┣━┓
┃ ◯ 4c30dc2 2d refactor(rebase): Store rewritten OIDs in a map
┃ ┃
┃ ◯ 9d19e9d 2d test: Add functions to facilitate comparing changes within a file
┃ ┃
┃ ◯ ffcdec7 2d feat(repo): Add `AmendFastOptions::FromCommit`
┃ ┃
┃ ◯ 40b1f2b 1d refactor(rebase): Allow `RebaseCommand::Pick` to apply multiple commits
┃ ┃
┃ ◯ ced7caa 1d test: Add more assertions to `current()` revset test
┃ ┃
┃ ◯ c8bd0b7 1d feat(rebase): Update `RebaseCommand::Pick` to support squashing commits
┃ ┃
┃ ◯ c293c9c 1d feat(move): Add `--fixup` to squash moved commits into the destination
┃ ┃
┃ ◯ 6defa13 1d fix(move --fixup): Try to get on disk fixups working
┃ ┃
┃ ◯ 554a8e4 1d (move-fixup) fix: Don't print scary messages during on disk fixups
┃ ┃
┃ ◯ e30bb27 1d temp: buncha crude debugging
┃
◯ 117ccf2 7m refactor(smartlog): Move definition of default Revset
┃
◯ 063e083 7m feat(smartlog): Allow user configurable default revset
┃
◯ cb1a415 7m feat(smartlog): Only HEAD and main should be mandatory in the smartlog
┃
● 8718514 7m (ᐅ sparse-smartlog) feat(smartlog): Support for rendering a sparse smartlog


❯ cargo run -- smartlog 'stack()'
⋮
◇ b5c32ba 2d (master) fix(rewrite): don't warn about bug with missing reworded commits
┃
◯ 117ccf2 7m refactor(smartlog): Move definition of default Revset
┃
◯ 063e083 7m feat(smartlog): Allow user configurable default revset
┃
◯ cb1a415 7m feat(smartlog): Only HEAD and main should be mandatory in the smartlog
┃
● 8718514 7m (ᐅ sparse-smartlog) feat(smartlog): Support for rendering a sparse smartlog


❯ cargo run -- smartlog @~~
⋮
◇ b5c32ba 2d (master) fix(rewrite): don't warn about bug with missing reworded commits
⋮
◯ 063e083 7m feat(smartlog): Allow user configurable default revset
⋮
● 8718514 7m (ᐅ sparse-smartlog) feat(smartlog): Support for rendering a sparse smartlog


❯ cargo run -- smartlog '@~~ + c293c9c..move-fixup'
⋮
◇ b5c32ba 2d (master) fix(rewrite): don't warn about bug with missing reworded commits
┣━┓
⋮ ⋮
⋮ ◯ 6defa13 1d fix(move --fixup): Try to get on disk fixups working
⋮ ┃
⋮ ◯ 554a8e4 1d (move-fixup) fix: Don't print scary messages during on disk fixups
⋮ ⋮
⋮
◯ 063e083 7m feat(smartlog): Allow user configurable default revset
⋮
● 8718514 7m (ᐅ sparse-smartlog) feat(smartlog): Support for rendering a sparse smartlog

@claytonrcarter claytonrcarter marked this pull request as draft November 4, 2022 02:39
@claytonrcarter claytonrcarter force-pushed the sparse-smartlog branch 2 times, most recently from 8718514 to eaa874c Compare November 5, 2022 17:41
@claytonrcarter
Copy link
Collaborator Author

Just updated and rebased onto master after #620 was merged. Still just a proof of concept; I didn't look at tests yet, nor really dig too far into what changes to the original implementation could be made based on the new changes from #620. The output seems to match what I was getting before. Now that the hidden commits have been dealt with though, and there's general sense that this change desired, I'll work on updating tests and such.

@claytonrcarter claytonrcarter force-pushed the sparse-smartlog branch 3 times, most recently from dd4eac6 to 08134fa Compare November 6, 2022 13:27
git-branchless/src/commands/smartlog.rs Outdated Show resolved Hide resolved
git-branchless/src/opts.rs Outdated Show resolved Hide resolved
git-branchless/src/commands/smartlog.rs Outdated Show resolved Hide resolved
git-branchless/tests/command/test_smartlog.rs Show resolved Hide resolved
git-branchless/src/commands/smartlog.rs Outdated Show resolved Hide resolved
@claytonrcarter claytonrcarter force-pushed the sparse-smartlog branch 2 times, most recently from 3cac073 to 8d3d175 Compare November 7, 2022 04:12
@claytonrcarter claytonrcarter marked this pull request as ready for review November 7, 2022 04:13
@claytonrcarter
Copy link
Collaborator Author

👋 Thanks for the review. I've marked this a ready, though I always do so w/ some trepidation on these substantial PRs, for fear that I've forgotten to take care of some expedient unwrap() or expect() or just forgotten to dot some i's, etc. If you see anything, just let me know and I'll take care of it.

This seems to be working just fine in my experimentation. If you have any add'l tests you'd like me to add, I'll be happy to.

I also took care of a few performance issues that were making sparse lots significantly slower then connected logs. From my casual benchmarks, on a repo w/ ~12 branches and probably 80 draft commits, the sparse log is ever so slightly faster than a connected log.

Note too the commit message for the first commit in this stack, regarding clap and the default value for revset. Removing the default revset from the clap help text is maybe a regression, but on the other hand, it's not actually the revset being displayed: it's that revset connected back to main. So I figured that that wasn't too big of a deal.

Finally, I think this will get really interesting when/if we give users that ability to set a custom revset. For example, I'm been playing with stack() | branches() to just see my current work + a quick topology of my other branches.

@claytonrcarter claytonrcarter marked this pull request as draft November 7, 2022 22:21
@claytonrcarter
Copy link
Collaborator Author

Marking as draft again. Just encountered this locally:

❯ git sw -
⠠ Examining local history (1.3s)
  ⠤ Walking commits (1.2s)                                                                                   
The application panicked (crashed).
Message:  called `Option::unwrap()` on a `None` value
Location: git-branchless/src/commands/smartlog.rs:260

Backtrace omitted. Run with RUST_BACKTRACE=1 environment variable to display it.
Run with RUST_BACKTRACE=full to include source snippets.

@claytonrcarter claytonrcarter marked this pull request as ready for review November 8, 2022 01:11
@arxanas
Copy link
Owner

arxanas commented Nov 8, 2022

Do we need SmartlogVariant at all? It seems like the connected case should be a subset of the sparse case, as long as we can craft a revset that includes all of the commits we would want to see in the smartlog. Something like this seems to work:

         ((draft() | branches() | @) % main())
| parents((draft() | branches() | @) % main())
| branches() | @

(Jujutsu also has a weird revset for its default log output.)

@arxanas
Copy link
Owner

arxanas commented Nov 9, 2022

I posted https://github.com/claytonrcarter/git-branchless/pull/25/files with my changes; let me know what you think.

@claytonrcarter
Copy link
Collaborator Author

I posted https://github.com/claytonrcarter/git-branchless/pull/25/files with my changes; let me know what you think.

You beat me to it! I have a very similar changeset locally, though mine is stuck on the test_switch_pty tests. I'll pull in your changes and push it all up here in a bit.

Do we need SmartlogVariant at all?

As you figured out, the answer is "no not at all". I had also been wondering about replacing the "connected" part w/ the default revset, but didn't pursue it. Turns out: revsets are amazing!

... replacement revset ...

Yes, that works! 🎉 In fact, I think we can even simplify it by removing the parents(...) clause b/c the smartlog code is already finding and adding the merge bases.

@claytonrcarter claytonrcarter force-pushed the sparse-smartlog branch 2 times, most recently from 4fadf9a to 326e768 Compare November 9, 2022 03:50
@claytonrcarter
Copy link
Collaborator Author

OK, I've updated my changes and incorporated yours. 🔥

I did make some slight edits to your changes, mostly to remove some (I think unneeded) extra braces and what now. I also removed the parents(...) part of the default revset. I updated the commit message, too, but please feel free to change it back or edit it further. Sorry if making those edits is too forward of me; I'll be happy to revert them.

Oh, final note, this PR also includes the single commit from #632. If that's merged first, I can remove it from this, or if it's small enough and acceptable, it can be merged as part of this, which ever you prefer. (There are some small conflicts, so it's non-trivial to push them independently.)

Copy link
Owner

@arxanas arxanas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉

claytonrcarter and others added 3 commits November 9, 2022 03:16
This doesn't do much for us now, but will soon be used to help us determine
what type of smartlog we should be rendering.

Unfortunately, this prevents clap from rendering the default revset in
the help message, but I believe this is the only way (via the derive API)
to detect if the user has or has not not supplied an arg for a particular
value.

See clap-rs/clap#3846 fmi on getting the
value_source of an arg via the derive API.
This should only happen when the user has requested to render a specific
revset, as opposed to calling `smartlog` without any arguments, or when
any of the other commands trigger a call to `smartlog`.

Note that HEAD and the head of the main branch are always displayed,
regardless of the revset provided by the user.
@arxanas arxanas enabled auto-merge (rebase) November 9, 2022 08:16
@arxanas
Copy link
Owner

arxanas commented Nov 9, 2022

Oh, final note, this PR also includes the single commit from #632. If that's merged first, I can remove it from this, or if it's small enough and acceptable, it can be merged as part of this, which ever you prefer. (There are some small conflicts, so it's non-trivial to push them independently.)

I merged that commit and rebased this PR for you (and enabled auto-merge). If you like, you can push the branches to this repository and stack the PRs. You would set this PR's "base branch" to the branch containing the other commit. Then, once the other PR is merged, this PR's base branch will be updated to master.

@arxanas arxanas merged commit 3ab9b9e into arxanas:master Nov 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants