Skip to content

Conversation

@mimre25
Copy link
Contributor

@mimre25 mimre25 commented Jan 3, 2025

Proposed implementation of #79.

@keis let me know what you think 🙂

This is a small quality of life improvement, but comes naturally when
allowing to sort by time.
@mimre25
Copy link
Contributor Author

mimre25 commented Jan 3, 2025

I would like to add those options to be configurable in the .gitconfig file, but I don't know how that exactly works - do you have any pointer for this?

@keis
Copy link
Owner

keis commented Jan 3, 2025

Hey, thanks for looking into it.

I wonder if the new options are really needed or if the default (and only) behaviour could be changed to always show date, always sort in descending order. I doubt anyone would really opt for the random order that happens now. My concern here is trying to keep the explosion of different options down where possible and some changes in output is probably better.

The only thing that I absolutely don't want to break is the tab completion scripts and custom menu commands which rely on the hash being the first word of the output. So the date needs to shuffle to the end.

@mimre25
Copy link
Contributor Author

mimre25 commented Jan 3, 2025

I've now made the time come second in the line to be neatly below one another (I considered the EoL but that is a bit messy as commit message length influences that).

This is how it looks now with the default picker:

sort & reverse

(git-fixup) ➜  git-fixup git:(add-sort-by-time) ✗ ./git-fixup -AS
1) c64094fea2f3f4d7d2a62e9937469ef9b796313a [F] fixup! feat: Allow displaying the commit time of fixup candidates <_email_>
2) 217c498dc613058e0bb873c2b495f610be96cecf [F] WIP <_email_>

(git-fixup) ➜  git-fixup git:(add-sort-by-time) ✗ ./git-fixup -ASr
1) 217c498dc613058e0bb873c2b495f610be96cecf [F] WIP <_email_>
2) c64094fea2f3f4d7d2a62e9937469ef9b796313a [F] fixup! feat: Allow displaying the commit time of fixup candidates <_email_>

with time

(git-fixup) ➜  git-fixup git:(add-sort-by-time) ✗ ./git-fixup -ASt
1) c64094fea2f3f4d7d2a62e9937469ef9b796313a 2025-01-03 12:36:27 +0100 [F] fixup! feat: Allow displaying the commit time of fixup candidates <_email_>
2) 217c498dc613058e0bb873c2b495f610be96cecf 2025-01-03 12:48:14 +0100 [F] WIP <_email_>

(git-fixup) ➜  git-fixup git:(add-sort-by-time) ✗ ./git-fixup -AStr
1) 217c498dc613058e0bb873c2b495f610be96cecf 2025-01-03 12:48:14 +0100 [F] WIP <_email_>
2) c64094fea2f3f4d7d2a62e9937469ef9b796313a 2025-01-03 12:36:27 +0100 [F] fixup! feat: Allow displaying the commit time of fixup candidates <_email_>

I've also tested it out with fzf as my menu and noticed fzf automatically reverses the order, ie, in fzf the first element is at the bottom, whereas in the default menu the first element is at the top.
Probably can configure fzf to not do that.

I agree that the -S flag doesn't make much sense, but I think in keeping at least the -r flag makes does.

What do you think?

Also, how could I add those to the .gitconfig file, what would I need to add to the code?

@keis
Copy link
Owner

keis commented Jan 3, 2025

That output looks good to me.

I'm more inclined to do something like #58 to allow customisation of the output rather than a "show time" flag, lets just change the default to include time. Sure, the reverse flag makes some sense. The "cut time" operation I don't really see the need of

git config is being loaded like this https://github.com/keis/git-fixup/blob/master/git-fixup#L169

op=${GITFIXUPACTION:-$(git config --default=fixup fixup.action)}

Maybe something like fixup.sortflags that has a default of -k2 -k3 -k4, then the -r can extend that by appending a literal -r. Again controlling more with one setting rather than many small ones. Would also work well with customised output (Mabye this is finally the year I get around to it 😅)

@mimre25
Copy link
Contributor Author

mimre25 commented Jan 4, 2025

I'm more inclined to do something like #58 to allow customisation of the output rather than a "show time" flag,[...]

I think this is a great idea.
But that could easily break the constraint of having the SHA as the first first word for the completion scripts?
How about also adding a flag a la --no-customization to get the (current) default output, that the completion scripts can use?
That would only leave the issue with the menu relying on the SHA as first field.
I guess we could just mention that in the documentation of the customization so that nobody misses it.

And in terms of custom output format, how about just taking whatever config is passed and passing it straight to git log in print_sha?
That seems the most straight forward and avoids a ton of documentation about how to format the output, because everything is already documented in the man page of git log.

On a related note: With displaying the time now, the output gets quite long - should we maybe also change the default to show the short hash instead of the full hash?

The "cut time" operation I don't really see the need of

That was mainly because I'm concerned about backwards compatibility.

git config is being loaded like this https://github.com/keis/git-fixup/blob/master/git-fixup#L169

op=${GITFIXUPACTION:-$(git config --default=fixup fixup.action)}

Thanks, I'll take a look at adding this, once we figured out what we want to add exactly.
Or should we scope this PR to just adding the time column per default and allowing the sort?

Maybe something like fixup.sortflags that has a default of -k2 -k3 -k4, then the -r can extend that by appending a literal -r. Again controlling more with one setting rather than many small ones. Would also work well with customised output (Mabye this is finally the year I get around to it 😅)

Sounds like a good idea.

@keis
Copy link
Owner

keis commented Jan 4, 2025

The way I'm using this script is by the completion script and I actually do want the full input. The completion scripts work by showing the entire line by but only using the first word as the value.

Making the config be git log format sounds good. the use of $type complicates things a bit.

I like the idea of switching to short sha by default.

But, yeah, lets keep the scope here to the sorting, make showing time and sorting the default.

This is now the default - for discussion check
keis#80.
This is mainly to reduce the overcrowding of the output due to the time
being added in the previous commit.
@mimre25
Copy link
Contributor Author

mimre25 commented Jan 5, 2025

Screenshots taken on a 27" 2560x1440 screen with a half-screen terminal (full screen tmux with 1 vertical split).

Long hash:
image
image

Short hash:
image
image

I've added the change to short hash as well.

Let me know what you think 🙂

@keis
Copy link
Owner

keis commented Jan 6, 2025

Looks good to me!

@keis keis merged commit d37826e into keis:master Jan 6, 2025
1 check passed
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