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.

Sign up for GitHub

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

Jump to bottom

Show added git status when staged file is modified #363

Merged
merged 2 commits into from
Mar 9, 2018
Merged

Show added git status when staged file is modified #363

merged 2 commits into from
Mar 9, 2018

Conversation

salmanulfarzy
Copy link
Member

Description

Now both modified and added indicators are shown when a staged file is modified in working copy or index.

Screenshot

Please, attach a screenshot, if possible.

spaceship-git-status

Fixes : #355

@salmanulfarzy
Show added git status when staged file is modified
4372a88 
Now both modified and added indicators are shown when a staged file in
modified in working copy or index.

Fixes : spaceship-prompt#355
@salmanulfarzy salmanulfarzy added the improvement A PR that make small changes for improving UX, performance, readability, etc label Feb 1, 2018
@salmanulfarzy salmanulfarzy self-assigned this Feb 1, 2018
@denysdovhan
Copy link
Member

@maximbaz have any doubts?

@maximbaz
Copy link
Contributor

maximbaz commented Feb 1, 2018 •
edited
Loading

I think it's in line with the current behavior, however there is the same issue for "removed" file - the x will be shown for both "deleted not in index" and "deleted in index".

To keep it consistent, "deleted in index" should also become x+.

@maximbaz
Copy link
Contributor

maximbaz commented Feb 1, 2018

As soon as you fix the status of "removed files in index", I think you should merge.

The text below should not hold the merge, but I just want to use this opportunity to explain why I find the current behavior of git_status confusing comparing to #359:

I want to look at the git_status part of the prompt and understand the state of the repo. Suppose the status is x!+, with current git_status behavior it can mean any of these things:

  1. x!+ - something is staged, there are unstaged modified files and unstaged removed files
  2. x!+ - something is staged, there are no unstaged modified files, but there are unstaged removed files
  3. x!+ - something is staged, there are unstaged modified files, but there are no unstaged removed files
  4. x!+ - everything is staged, there are no unstaged modified files and no unstaged removed files

In other words, by looking at x!+ I can't know if I already ran git add on modified and/or deleted files, I have to run git status manually to verify this.

On the other hand, with #359 any status can only mean one state of the repo, there is no trickery. The same example as above will be:

  • x!+ - something is staged, there are unstaged modified files and unstaged removed files
  • x+ - something is staged, there are no unstaged modified files, but there are unstaged removed files
  • !+ - something is staged, there are unstaged modified files, but there are no unstaged removed files
  • + - everything is staged, there are no unstaged modified files and no unstaged removed files

@salmanulfarzy
Copy link
Member Author

salmanulfarzy commented Feb 2, 2018 •
edited
Loading

"deleted in index" should also become x+

Thanks for the heads up, Let me look into that.

Agree that these indicators are not based on index/working tree statuses. Because git-status itself shows both the status for working tree and index, Users might be expecting simliar behaviour from their prompt indicators.

These reasonings confuse me and I'm unable to reproduce those. They are behaving as you described in the second part. I might have completely misunderstood the cases you were referring, Please correct me.

x!+ - something is staged, there are no unstaged modified files, but there are unstaged removed files

If there are no unstaged modified files, Current behaviour doesn't show modified indicator.

case-1

x!+ - something is staged, there are unstaged modified files, but there are no unstaged removed files

There are no removed indicators for this usecase.

case-2

x!+ - everything is staged, there are no unstaged modified files and no unstaged removed files

case-3

@salmanulfarzy
Copy link
Member Author

salmanulfarzy commented Feb 2, 2018 •
edited
Loading

For reference, Here are the regular expressions now being used,

grouped regex

@maximbaz
Copy link
Contributor

maximbaz commented Feb 2, 2018

Oh, sorry, I completely misunderstood the desired behavior of this PR, @salmanulfarzy thanks for your examples, now it makes more sense!

The origin of my misunderstanding was that I though you made !+ mean "modified and then staged", now I see that it really means "staged and then modified". In this case, my suggestion with "removed and then staged" is also incorrect, it should not become x+, instead it should become just +.

@maximbaz
Copy link
Contributor

maximbaz commented Feb 2, 2018

Wait... if you made "modified and then staged" show up as +, and if you will make "removed and then staged" also show up as +, then this section becomes almost identical to #359 😄

@salmanulfarzy
Copy link
Member Author

Does the order of status indicators matter in prompt ? It's not exactly as the changes being made, Just being in the order they are being parsed in the script.

my suggestion with "removed and then staged" is also incorrect, it should not become x+, instead it should become just +

I have conflicted opinion on this. From your argument it makes sense to just show + indicator. But deletion is not like modification, User have to be aware about he is going to commit removal of a file. So I would prefer x+ as indicator.

@maximbaz
Copy link
Contributor

maximbaz commented Feb 2, 2018 •
edited
Loading

The order didn't matter until now, but if you make x+ as a single indication of "removed and then staged", the order begins to matter. Not true, the order doesn't matter indeed.

I'm all for consistency, if you make "removed=x" and "removed and staged=x+", then this should also hold true: "modified=!" and "modified and staged=!+". This is actually what I thought you already did, all my notes in that comment were for this approach.

@salmanulfarzy
Copy link
Member Author

my suggestion with "removed and then staged" is also incorrect, it should not become x+, instead it should become just +

I'll make the necessary changes to reflect this behavior.

@maximbaz
Copy link
Contributor

maximbaz commented Feb 2, 2018

After you do this, I encourage you to check out #359 again. Just try to look at it from a different perspective, I have a picture there, compare the status icons of your PR vs that one.

You will notice that + becomes the indication of "any staged change", and other icons only reflect specific unstaged changes.

The only inconsistency left in this PR is the "renamed" indicator - everything staged (new file, modification, removal) is marked as +, but "renamed staged" is instead marked with ». I know you like the renamed icon, but this is very unnatural, why one particular staged change has a different icon comparing to all the rest. I argue that everything staged should be marked as +.

@salmanulfarzy
Copy link
Member Author

Ping !

@denysdovhan
Copy link
Member

I know you like the renamed icon, but this is very unnatural, why one particular staged change has a different icon comparing to all the rest.

Why the git shows the renamed files in status and marks then as renamed?

Honestly, I don't understand the problem. I live with current behavior and haven't ever noticed any confusions yet.

@maximbaz
Copy link
Contributor

It's not a problem, it's inconsistency - staged created files are marked with +, staged deleted files are marked with +, staged modified files are marked with +, why staged renamed files are marked with »?

But as I said in the very beginning, do not hold the PR from being merged if you are okay with that 🙂

@salmanulfarzy
Copy link
Member Author

Resolved conflicts. If merging, Please squash.

@denysdovhan denysdovhan merged commit 4248740 into spaceship-prompt:master Mar 9, 2018
@salmanulfarzy salmanulfarzy deleted the git-status branch March 9, 2018 18:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@denysdovhan denysdovhan denysdovhan approved these changes

Assignees

@salmanulfarzy salmanulfarzy

Labels
improvement A PR that make small changes for improving UX, performance, readability, etc
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@salmanulfarzy @denysdovhan @maximbaz