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

File stage checker not working as expected #418

Closed
xaviml opened this issue Sep 9, 2021 · 5 comments · Fixed by #420
Closed

File stage checker not working as expected #418

xaviml opened this issue Sep 9, 2021 · 5 comments · Fixed by #420

Comments

@xaviml
Copy link
Contributor

xaviml commented Sep 9, 2021

Description

If we have changed files not staged, and not staged files, then I don't get the "No files added to staging!".

Steps to reproduce

Assuming you have a project with commitizen installed and the project is tracked by git, then we can do:

  1. Edit an existing file
  2. Run cz commit

Current behavior

The questionnaire will pop up without erroring out with No files added to staging! since no files are yet added to stage.

Desired behavior

The desired behaviour should be what the error says ("No files added to staging!"), so if no files are added to the stage, then the error should pop.

Environment

  • commitizen version: 2.17.11
  • python version: Python 3.7.9
  • operating system: Linux

This is an application bug, so it happens with any Python version and OS.

Extra

This issue is related to #45 (fixed in #49), which was the original issue related to this problem, but in my opinion, was never fixed the right way. I am not randomly opening this issue, I am doing it because I bumped into this frustrating problem all the time where:

  • I run cz commit
  • Fill the questionnaire (~1 minute)
  • Pre-commit runs (~1 minute)
  • Get an error because the stage is empty
  • Then, I have to stage the files, and run cz commit --retry

It seems that this was deliberately done in the code since just this command is needed:

git diff --no-ext-diff --cached --name-only

The no-cached one will pick up files that are unstaged. In fact, in the more recent versions of git one can use --staged flag which is an alias for --cached.

I understand if this was picked up from here in cz-cli, but it has the same problem in there, and this PR is a proposal for the fix in that repo (not yet merged).

If this is a recognized bug, and we are willing to fix it, I offer myself to open the PR if needed.

Thank you for the great work and this amazing tool!

Regards,
Xavi M.

@woile
Copy link
Member

woile commented Sep 10, 2021

Hello @xaviml , thanks for reporting this issue.

Given the need to propagate many different flags to git commit, one idea we have is to use --, something like this:

cz commit -- -a -s 
# this will stage and sign the commit

do you think this could work out for you?
@Lee-W what do you think?

@woile
Copy link
Member

woile commented Sep 10, 2021

We could also show a warning before starting with the questionnaire

@Lee-W
Copy link
Member

Lee-W commented Sep 11, 2021

I just test out and confirm this error. Will changing git diff --no-ext-diff --cached --name-only to what's fixed in commitizen/cz-cli#818 breaks anything? If not so, I'll say solve the issue from the root might be a better idea.

@xaviml
Copy link
Contributor Author

xaviml commented Sep 11, 2021

Hi both,

Given the need to propagate many different flags to git commit, one idea we have is to use --, something like this:

This is not a bad idea. This is what commitizen/cz-cli already has, and the fix for commitizen/cz-cli#818 will:

  • Check with git diff --no-ext-diff --cached --name-only if there are staged files
  • Check with git diff --no-ext-diff --cached --name-only and git diff --no-ext-diff --name-only if the -a is passed for git args like cz commit -- -a as you suggested.

Considering this commitizen does not have the option for git args, the solution to this problem is removing the check git diff --no-ext-diff --name-only and leave only the --cached one:

def is_staging_clean() -> bool:
    """Check if staing is clean."""
    c = cmd.run("git diff --no-ext-diff --cached --name-only")
    return not bool(c.out)

Does this make sense to you both?

Regards,
Xavi M.

@xaviml
Copy link
Contributor Author

xaviml commented Sep 11, 2021

I created #420, we can review changes in there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants