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

Prevent no branches #1074

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

Conversation

rienafairefr
Copy link

as discussed in #1073, this seems to fix it.

returns "not supported" ENOTSUP when a change would result in no branches.
incidentally, the code for branches from_string was already raising/erroring out in case of setting completely empty branches, that's fixed. I've added unit test cases to test all that.

On my system, the -fsanitize=undefined option in the compiler added some problems that seem to be fixed by also adding the -fsanitize=undefined flag on linking, not sure why, that's why the change is in there,

@lgtm-com
Copy link

lgtm-com bot commented Oct 9, 2022

This pull request fixes 1 alert when merging f2bb649 into 5d3c500 - view on LGTM.com

fixed alerts:

  • 1 for Comparison result is always the same

@rienafairefr
Copy link
Author

Sorry I think I broke the CI with é in my last name 😅

@trapexit
Copy link
Owner

trapexit commented Oct 9, 2022

Yeah. Perhaps my script could be setup to decode utf8.

Try changing .decode() in git2debcl to .decode('utf-8')

src/branches.cpp Outdated
@@ -269,6 +272,10 @@ namespace l
int
erase_begin(Branches::Impl *branches_)
{
if (branches_->size() <= 1) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you don't mind updating the code to use a more GNU style for braces and such. And I'm fine with something like

if(branches_->size() <= 1)
  return -ENOTSUP;

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, updated the code to better match the style

@lgtm-com
Copy link

lgtm-com bot commented Oct 9, 2022

This pull request fixes 1 alert when merging ad6c93f into 5d3c500 - view on LGTM.com

fixed alerts:

  • 1 for Comparison result is always the same

@lgtm-com
Copy link

lgtm-com bot commented Oct 9, 2022

This pull request fixes 1 alert when merging 5d61a5b into 5d3c500 - view on LGTM.com

fixed alerts:

  • 1 for Comparison result is always the same

@lgtm-com
Copy link

lgtm-com bot commented Oct 9, 2022

This pull request fixes 1 alert when merging 80e0752 into 5d3c500 - view on LGTM.com

fixed alerts:

  • 1 for Comparison result is always the same

@rienafairefr rienafairefr force-pushed the prevent-no-branches branch 2 times, most recently from f90ab5f to 694b819 Compare October 10, 2022 18:47
@lgtm-com
Copy link

lgtm-com bot commented Oct 10, 2022

This pull request fixes 1 alert when merging 694b819 into 5d3c500 - view on LGTM.com

fixed alerts:

  • 1 for Comparison result is always the same

@lgtm-com
Copy link

lgtm-com bot commented Oct 10, 2022

This pull request fixes 1 alert when merging f283cba into 5d3c500 - view on LGTM.com

fixed alerts:

  • 1 for Comparison result is always the same

@trapexit
Copy link
Owner

Ugh. Sorry. Python is a major pita regarding char encoding. I can try to fix it and then you can submit the PR for the original issue. Or if you want to see it through feel free.

@saltydk
Copy link

saltydk commented Mar 29, 2023

Can't you just set the ENV to UTF-8 encoding? Either through PYTHONIOENCODING="UTF-8" or the LANG/LANGUAGE/LC_ALL?

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.

3 participants