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

Git-plugin: Don't truncate commit number #41

Closed
wants to merge 1 commit into from
Closed

Git-plugin: Don't truncate commit number #41

wants to merge 1 commit into from

Conversation

ColinHebert
Copy link
Contributor

A branch name shouldn't be a commit number, and a commit number should always be in the long format.
Themes can reduce the size of the commit id, and they should be able to choose between the branch name or the commit id.

A branch name shouldn't be a commit number, and a commit number should always be in the long format.
Themes can reduce the size of the commit id, and they should be able to choose between the branch name or the commit id
@sorin-ionescu
Copy link
Owner

On commit length, I wanted to make theming easier because many themes in OMZ Robby have issues. When are you ever going to be able to fit the full commit in a prompt?

I think you're right on branch should never be a commit when the branch is missing. vcs_info always sets the branch even when you are not on a branch. I think that's misleading, thus wrong.

However, I do not like branch_is_set and the inflexible ternary operator (%n(x.true.false)).

@sorin-ionescu
Copy link
Owner

At one time, I had a commit style where you could specify if %c meant long or short, and short was always 7, which is standard, but I removed it since realistically I do not see anybody fitting the full commit in a prompt.

@ColinHebert
Copy link
Contributor Author

Regarding the commit length, it's clearly a formatting issue, and therefore should be handled during the formatting part (even if it's said that by default only 7 chars are selected, anyone should be able to have the full commit name (if you have a multi lines prompt that could be feasible to use the full name).

Regarding the branch_is_set and the ternary operator, I haven't figured out how to format with conditions, this was the best way I could come with. If there is something better I would be glad to see that :)

@sorin-ionescu
Copy link
Owner

Well, I used %C for clean but %C could mean full commit id, and we can find synonyms for clean/dirty. I used %C and %D for clean and dirty to avoid the ternary operator. :(

@ColinHebert
Copy link
Contributor Author

I don't really like the ternary operator as it doesn't handle strings and in the end it's just a "hack", but multiplying variables for every possible case isn't a nice solution either.

In my current theme I have this system to print the current position:

If we have a branch, we print it
Otherwise if we have a relative name ( see my other commit ), we print it
Else we print the commit number

But handling this with new variables for each case starts to be a nightmare.

The same thing applies with other "conditions". For example, when my current branch has a remote, and that remote in neither behind or ahead I want to print "=". Doing that condition without ternary is just awful.

Plus, I think that the theme should do all the formatting, it shouldn't be handled at all by the plugin. The 'rule' of one variable, one value helps to keep the code clean.

I would really like to see a better solution, but unless zsh formatting system has other options this is somehow the best solution I can think of.

@sorin-ionescu
Copy link
Owner

git-info could be changed to assign to any number of variables, not just git_prompt_info and git_rprompt_info, and those variables can have just %b and %c. In prompt_sorin_precmd, you could set a variable with either the branch or the commit to add to PROMPT/RPROMPT in prompt_sorin_setup. This is the kind of stuff you have to do with vcs_info that I'm trying to avoid. I want git-info to be simple.

@ColinHebert
Copy link
Contributor Author

I would rather use the current system even if there is more variables for the size of some strings. It's clearer and easier to format.

Being stuck with ternary operator instead of having a huge script in the theme file is, I think, a good trade-off

@sorin-ionescu
Copy link
Owner

Yeah, but what letter do you use for the size of strings? An easy pattern would be for %b to be the branch and %B to be the size. Unfortunately, I had to use some uppercase letters.

@sorin-ionescu
Copy link
Owner

Maybe, we can fake our own ternary. It's ugly and slower, but it's better than polluting the code with a tonne of length variables.

function fake-ternary() {
  if [[ -n "$argv[1]" ]]; then
    print "$argv[1]"
    return 0
  elif [[ -n "$argv[2]" ]]; then
    print "$argv[2]"
    return 0
  else
    return 1
  fi
}

zstyle ':omz:plugin:git:prompt' prompt ' %F{blue}git%f$(fake-ternary "%b" "%c")%s'

@sorin-ionescu
Copy link
Owner

That's not really a ternary since we are not passing it a condition, more like select-non-empty-string.

@sorin-ionescu
Copy link
Owner

By the way, Zsh has a math ternary print $((1 ? 1 : 0)).

@ColinHebert
Copy link
Contributor Author

Yes, it was more an elvis operator than a ternary.
I tried with the math ternary (I thought about that too, but no luck with that, here is the result printed:

$((1?(89f321d...):temp_master))

It seems that you can't just run code when formatting.

Regarding the letter I just chose a random one, but it would have been indeed way easier to have uppercase = size of string.

@ColinHebert
Copy link
Contributor Author

It also seems that the math ternary is only "math" (surprise), it's not possible to do print $((1 ? "true" : "false"))

@sorin-ionescu
Copy link
Owner

I wasn't thinking of running code when formatting with what you call the elvis operator (fake-ternary). I was thinking that the formatter will generate $(fake-ternary "mybranch" "89f321d"), which will be executed when the prompt is shown and print "mybranch".

@ColinHebert
Copy link
Contributor Author

It's maybe more work, but rearranging the letters to have the uppercase/lowercase system seems to be the best solution (It's more clean)

@sorin-ionescu
Copy link
Owner

I'm going to try to see if I can get a function to work.

function not-empty() {
  for arg in $argv; do
    print "$arg"
    return 0
  done
  return 1
}

not-empty "" "" "" "mybranch" "89f321d"

It will print the first non-empty argument.

@ColinHebert
Copy link
Contributor Author

I was thinking about a Coalesce too, but I'm still not convinced that having to write script is the best solution

@sorin-ionescu
Copy link
Owner

No, that function would go into helper.zsh.

@ColinHebert
Copy link
Contributor Author

Yes, but if you want to create your prompt, you will have to write a script (using this function) which is slower than a simple format.

@sorin-ionescu
Copy link
Owner

You mean you'll have to execute a sub shell. It's not that slow. You're executing a function in a sub shell, not an external script.

@sorin-ionescu
Copy link
Owner

Tried

zstyle ':omz:plugin:git:prompt' prompt ' %F{blue}git%f:$(not-empty "%b" "%c")%s'

Got

oh-my-zsh git:$(not-empty "" "09b1249"):rebase-i ❯.

I need to figure out how to make Zsh execute it. I've been playing with quotes with no success.

@ColinHebert
Copy link
Contributor Author

I'm not sure it worth doing that much work. Here are the values right now:

"s:$action_formatted"    : String
"a:$added_formatted"     : int
"A:$ahead_formatted"     : int
"B:$behind_formatted"    : int
"b:$branch_formatted"    : String
"C:$clean_formatted"     : boolean (should be removed)
"c:$commit_formatted"    : String
"d:$deleted_formatted"   : int
"D:$dirty_formatted"     : boolean (should be an int)
"m:$modified_formatted"  : int
"R:$remote_formatted"    : String
"r:$renamed_formatted"   : int
"S:$stashed_formatted"   : int
"U:$unmerged_formatted"  : int
"u:$untracked_formatted" : int

With my patches I add a new value, describe_formated, so it means that at most we will need a grand total of 5 new variables (action_is_set, branch_is_set, commit_is_set?!, remote_is_set and describe_is_set)
And I would even throw another one, remote_is_sync which would be $ahead + $behind, allowing to know if we have a remote and if the remote is synchronised with the current branch.

So it's 6 new characters. It isn't that much and if one day the ternary operator is fixed to allow strings (using the length as a mathematical expression) we will be able to remove most of these chars.

I have nothing against the ternary operator, it's the idea of using new chars for length that bothers me, but if we don't need that many new variables, I think it's better to create these.

@sorin-ionescu
Copy link
Owner

While the counts may be integers, the formats are all strings, not integers nor booleans.

I do not want to remove clean_formatted. There may be people who want to display something instead of nothing when the repository is clean.

Making that function execute, if possible, is simpler than changing all those formats and frees letters for more formats.

@ColinHebert
Copy link
Contributor Author

Indeed, I forgot that it was the formated value, not the count itself.

Regarding clean_formatted, having a dirty_count allows to use a ternary operator to display something if the repo is dirty or not.

@sorin-ionescu
Copy link
Owner

Yes, but if you have formatted dirty, the ternary will fail.

@ColinHebert
Copy link
Contributor Author

zstyle ':omz:plugin:git:prompt' dirty '(%(D.ø.ð))'

If it hasn't changed you get ø, otherwise you get ð

@sorin-ionescu
Copy link
Owner

I value consistency over ternary tricks, but yes, that can be used. I'll consider it.

@sorin-ionescu sorin-ionescu reopened this Mar 22, 2012
@sorin-ionescu
Copy link
Owner

I didn't mean to close the issue. It seems that pushing to a branch other master closes GitHub issues as well.

I have a solution to this. It's not pretty, but it's better than plastering git-info with a ton of variables. It shows up under your name because I amended your commit. See cdb95f8.

@ColinHebert
Copy link
Contributor Author

I saw that on #zsh.

As you said, it's not pretty, but it could be useful later if someone wants to add more "custom code".

It works also with my remote_is_sync, by using $(not-empty "%A%B" "="), so it's a nice way to deal with that (nice but not pretty still :P ).

@sorin-ionescu
Copy link
Owner

I have looked at zstyle -e before. But, somehow I missed reply. Either my skimming skills suck, or the man page is written Scottish. :p

@sorin-ionescu
Copy link
Owner

Can you think of a better name than not-empty? Are you fine with committing this as is? I don't think it's too much of a performance impact.

@sorin-ionescu sorin-ionescu reopened this Mar 22, 2012
@ColinHebert
Copy link
Contributor Author

As it mostly acts like a COALESCE (http://www.postgresql.org/docs/9.1/static/functions-conditional.html#FUNCTIONS-COALESCE-NVL-IFNULL) coalesce could be a good name.

I'm fine with that commit.

And for what it worths, I've read the documentation on zstyle countless time I never actually noticed that -e could be a nice way out of this problem.

@sorin-ionescu
Copy link
Owner

Yes, you have mentioned that before, but I have looked at Apple's dictionary, which defines the word as 'come together to form one mass or whole', which makes it seem more like a synonym for catenation.

@ColinHebert
Copy link
Contributor Author

I just tried to get in headless, here is what I got:

.dotfilesgit: ❯

The spacing issue has been fixed, but I don't get a commit id.

@ColinHebert
Copy link
Contributor Author

After investigation, (I changed temporarily the result of coalesce in print "$arg $#arg"), the size of the string is 2. I think that zstyle -e does the eval before the format, thus the size of "%b" won't be the size of the branch name but the actual size of %b (which is 2).

@sorin-ionescu
Copy link
Owner

I see .tilde git::rebase-i ❯❯❯ as well. This is annoying, but it makes sense. I need it to format then eavaluate. This is why I was looking for parameter expansion when I started my conversation with Mikachu.

@sorin-ionescu
Copy link
Owner

There is another problem. The branch will never be empty due to formatting. Coalesce might need a regex.

@sorin-ionescu
Copy link
Owner

Maybe ${(e)git_prompt_info} will do the trick of zstyle -e.

@sorin-ionescu
Copy link
Owner

Try 5e5a780.

@ColinHebert
Copy link
Contributor Author

Yes, this patch works for me, and it's even better as you dont have to set the reply.

@sorin-ionescu
Copy link
Owner

I had my sight on something like this before Mikachu distracted me with zstyle -e. I have looked again at man zshexpn and found it.

Now, I can style the commit in a different colour from the branch; before, it was not possible. Though, I do not believe there was much confusion. Who would name a branch with alphanumeric gibberish?

However, all colours seem to be taken. I should probably convert it to 256 colours, but right now I am lacking inspiration.

lildude pushed a commit to lildude/prezto that referenced this pull request Jan 12, 2014
lildude pushed a commit to lildude/prezto that referenced this pull request Jan 9, 2016
kodelint pushed a commit to kodelint/prezto that referenced this pull request Nov 15, 2016
kodelint pushed a commit to kodelint/prezto that referenced this pull request Nov 15, 2016
gpanders pushed a commit to gpanders/prezto that referenced this pull request Mar 5, 2017
mjwestcott added a commit to mjwestcott/prezto that referenced this pull request Mar 18, 2017
* 'master' of https://github.com/zsh-users/prezto: (53 commits)
  Allow setting ZPREZTODIR to make prezto easier to integrate with (sorin-ionescu#54)
  documenting PR approval process defined in sorin-ionescu#41 (sorin-ionescu#45)
  Revert "Restore the parser status to PS2"
  Fix typos and link usernames
  Clearly define who are the project leaders (sorin-ionescu#26)
  Restore the parser status to PS2
  Revert "resolve race condition between git completion and coalesce function"
  Fix links in README
  Do not call `editor-info` in `zle-line-finish`
  Bump license years and tidy up readme
  Provide upgrade instructions + link to new fork
  ssh: update ps|grep ssh-agent check
  Don't load pfunctions from vi swap files
  Add module 'encode64' from oh-my-zsh (renamed to 'base64')
  Add module 'fancy-ctrl-z' from oh-my-zsh
  Update all submodules
  Discover and reuse existing ssh-agent if possible
  added maven plugin from oh-my-zsh
  Add basic set of aliases for npm
  Revert "use non breaking space" see sorin-ionescu#14
  ...
jetm pushed a commit to jetm/prezto that referenced this pull request Mar 23, 2017
RIT80 pushed a commit to RIT80/prezto that referenced this pull request Jan 25, 2022
RIT80 added a commit to RIT80/prezto that referenced this pull request Jan 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants