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

Properly handle GPG signatures in log, fix git version comparisons. #70

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Conversation

jaakristioja
Copy link

This fixes #66 and incorrect git version comparisons in src/git.cpp.

Although qgit requires git version GIT_VERSION_REQUIRED or later, git has
introduced new features and behavior since then. Hence qgit might need to
distinguish between different git versions for running some commands.
This is a workaround for not yet having proper support for parsing GPG
signatures.

Closes #66.
Lexicographical comparison of version QStrings just doesn't work properly in all
cases.
@tibirna
Copy link
Owner

tibirna commented May 26, 2019

Commit ae562871fcb08 :

  • Error message when git version can't be parsed by the regex should read "Unexpected" instead of "Incorrect"
  • gitVersionCompare should not trigger overall program failure as if git version was incompatible when versions can't be parsed with defined regex, because git developers can decide unexpectedly to change their version numbering scheme in the future (or locally modified copies of git can be used, with non-compliant version numbering). A fallback should be added, that simply compares string equality or lexicographical order (care must be paid in this later case, to avoid issuing incorrect return, e.g. when comparing lexicographically 3.1.0 and 3.1.0-dev -- such cases can be signaled as incompatibilities, though)
  • instead of -1, 0, 1 returns from gitVersionCompare, prefer adding an enum {INCOMPATIBLE = 0, COMPATIBLE) and use that as return code(this will greatly increase code readability)

@tibirna
Copy link
Owner

tibirna commented May 26, 2019

Commit d315fd45afd6 :

  • qgit should display a message in the console (and perhaps in its status bar) when it ignores gpg signatures.

@tibirna
Copy link
Owner

tibirna commented May 26, 2019

Thank you very much for having taken the time to address #66.

I apologize for the long time I took for revising this. Please consider my comments and let me know. I will process your pull request immediately after.

@jaakristioja
Copy link
Author

Sorry for the very long delay. To be honest, I stopped using qgit because I've learnt that the git command-line interface is actually sufficient for things I previously used qgit for (most importantly for visual graphs, e.g. git log --oneline --decorate --graph --no-show-signature --date-order --remotes --branches --tags). This means I'm no longer interested in pursuing this. I am sorry. I release my code in this pull request as public domain, no attribution required.

Note that commit ae562871fcb08 is not strictly required to fix #66, but is a different issue. Given that the git developers might not follow a strict versioning scheme, ae562871fcb08 might indeed not be enough.

As for gitVersionCompare() returning -1, 0 or 1, I tried to follow the logic of strcmp() and similar functions (including the <=> operator in C++20). If gitVersionCompare() is only to be used to compare against GIT_VERSION_REQUIRED, then perhaps it would make sense to refactor it into a bool isGitVersionCompatible(QString const &) function instead.

Regarding commit d315fd45afd6, I think that displaying a message for this would not provide any extra value, because you are already ignoring them if log.showSignature is not true in the git configuration. As opposed to --no-verify-signatures (as for git pull), passing --no-show-signature to git is just part of some background processing.

I hope this helps.

@tibirna
Copy link
Owner

tibirna commented Jun 6, 2020

Thank you very much for the detailed and thoroughly helpful rundown of your contribution. It is wholeheartedly appreciated.

With your accord, I will take at some point your branch, adapt it based on my observations and your clarifications, and included in the base qgit code.

Thanks again and great success in your future endeavors.

@jaakristioja
Copy link
Author

Thank you very much! Same to you! 😃

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.

QGit fails doesnt work if log.showSignature = true in git config
2 participants