Skip to content

LibGit2 miscellaneous fixes to credential callback#20751

Merged
tkelman merged 2 commits intomasterfrom
cv/libgit2-cred-misc
Mar 1, 2017
Merged

LibGit2 miscellaneous fixes to credential callback#20751
tkelman merged 2 commits intomasterfrom
cv/libgit2-cred-misc

Conversation

@omus
Copy link
Copy Markdown
Member

@omus omus commented Feb 23, 2017

Part of #20725.

  • Removes unnecessary nothing checks against Strings.
  • Corrects a type instability with the err variable.
  • Uses @static to avoid runtime calls to is_windows

@omus omus added the libgit2 The libgit2 library or the LibGit2 stdlib module label Feb 23, 2017
@tkelman
Copy link
Copy Markdown
Contributor

tkelman commented Feb 23, 2017

where's the type instability here?

is doing is_windows at runtime really making a material difference?

@omus
Copy link
Copy Markdown
Member Author

omus commented Feb 24, 2017

Type instability was that err = 0 is an Int64 and later it is set to a Int32 (Cint) by authenticate_ssh. Also note that err == 0 && return Cint(0) at the end of authenticate_ssh makes that function return either false or Cint(0)

@omus
Copy link
Copy Markdown
Member Author

omus commented Feb 24, 2017

is doing is_windows at runtime really making a material difference?

Probably not, I didn't do performance tests between the two. Is there an issue with adding in the @static macro?

@tkelman
Copy link
Copy Markdown
Contributor

tkelman commented Feb 24, 2017

same as other kinds of conditional compilation, you could have invalid code in the inactive branch and possibly not notice until you send things through appveyor

@omus
Copy link
Copy Markdown
Member Author

omus commented Feb 24, 2017

I didn't consider that. Since we always test with appveyor though the invalid code will eventually get caught. I personally would still leave in the @static macro. I'd be okay with dropping that commit if that is your preference.

@tkelman
Copy link
Copy Markdown
Contributor

tkelman commented Feb 28, 2017

appveyor doesn't help people testing locally, I'd rather use @static as sparingly as possible, only where absolutely necessary for performance or failure to build without it

@yuyichao
Copy link
Copy Markdown
Contributor

What's the kind of error you want to catch by removing @static?

@ararslan
Copy link
Copy Markdown
Member

I understand your point and agree in principle, @tkelman, but it seems pretty harmless in this case. (At least it seems that way to me.) I have no strong feelings either way I suppose, but the rest of this PR looks good to me.

@tkelman
Copy link
Copy Markdown
Contributor

tkelman commented Feb 28, 2017

Deprecations, as one example

julia> @static if false
           ASCIIString("foo")
       end

julia> if false
           ASCIIString("foo")
       end
WARNING: Base.ASCIIString is deprecated, use String instead.
  likely near no file:0

Unless you're actually looking at the appveyor log every time, that stuff can slip through until someone who actually builds on windows (usually me) notices and fixes it. If @static isn't necessary, I'd rather have that failing your local builds first (if it's running in --depwarn=error anyway), even on other operating systems before you submit a PR and waste a bunch of CI time.

@omus omus force-pushed the cv/libgit2-cred-misc branch from 6d3e691 to 35321e7 Compare February 28, 2017 22:15
@omus
Copy link
Copy Markdown
Member Author

omus commented Feb 28, 2017

I've reverted the @static change.

@tkelman tkelman merged commit 46c5ba1 into master Mar 1, 2017
@tkelman tkelman deleted the cv/libgit2-cred-misc branch March 1, 2017 17:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

libgit2 The libgit2 library or the LibGit2 stdlib module

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants