Skip to content

cred: change enum to git_credential_t and GIT_CREDENTIAL_* #5336

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
Merged
merged 1 commit into from
Jan 30, 2020
Merged

cred: change enum to git_credential_t and GIT_CREDENTIAL_* #5336

merged 1 commit into from
Jan 30, 2020

Conversation

ethomson
Copy link
Member

@ethomson ethomson commented Dec 13, 2019 •
edited
Loading

We have standardized on a trailing _t for enum types, instead of using "type" in the name. So git_credtype_t is more appropriately named git_credential_t.

Change the name, keeping the previous name and values as deprecated.

@ethomson ethomson force-pushed the ethomson/credtype branch 2 times, most recently from 8e409a4 to d40bfe3 Compare December 13, 2019 04:53
@ethomson
Copy link
Member Author

We've gone on a tear and removed abbreviations. Maybe git_credential_t and moving cred -> credential would be more helpful? It's more code churn but it feels more consistent.

@pks-t
Copy link
Member

pks-t commented Dec 13, 2019

Hm, I'm honestly not too sure about this. I mean we are discerning credential types here, right? There's definitely some inconsistencies in our tree:

  • git_merge_diff_type_t
  • git_rebase_type_t
  • git_object_t
  • git_reference_t

Well, seeing that we've already moved from git_otype_t to git_object_t and that object and reference types are probably one of the most commonly used types, let's go ahead and remove the "type" stuff. I'd love to do the same for git_merge_diff_type_t and git_rebase_type_t then, though. Do you want to tackle this or shall I do it?

And yes, if we're already dropping "type" then I'd vote for going with "git_credential_t".

@ethomson ethomson added the 1.0 label Jan 7, 2020
@ethomson
Copy link
Member Author

ethomson commented Jan 8, 2020

Do you want to tackle this or shall I do it?

I'll knock this out over the weekend.

@ethomson ethomson force-pushed the ethomson/credtype branch 2 times, most recently from edc46fe to 31afa04 Compare January 18, 2020 14:21
@ethomson
Copy link
Member Author

I'd love to do the same for git_merge_diff_type_t and git_rebase_type_t then, though. Do you want to tackle this or shall I do it?

I did this over in #5364

@ethomson ethomson force-pushed the ethomson/credtype branch 2 times, most recently from a6d305c to 17446ba Compare January 18, 2020 15:03
@ethomson
Copy link
Member Author

And yes, if we're already dropping "type" then I'd vote for going with "git_credential_t".

OK, I've refactored git_cred to be git_credential, git_credtype_t to be git_credential_t and GIT_CREDTYPE to be GIT_CREDENTIAL.

I also renamed the cred.[ch] files to be credential.[ch]. This was... a little odd - especially for cred_helpers.h, and sys/cred.h which we expect callers to include directly (instead of as part of git2.h).

@ethomson ethomson changed the title cred: change enum to git_cred_t and GIT_CRED_* cred: change enum to git_credential_t and GIT_CREDENTIAL_* Jan 18, 2020
pks-t
pks-t reviewed Jan 24, 2020
View reviewed changes
include/git2/cred_helpers.h
/* These declarations have moved. */
#ifndef GIT_DEPRECATE_HARD
# include "git2/credential_helpers.h"
#endif
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't we want to do the same header-forwarding for "git2/cred.h"?

Copy link
Member Author

Choose a reason for hiding this comment

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

We could, but our guidance has always been for people to include git2.h, which will bring in all the things except sys/ and - somewhat oddly - cred_helpers.h. So people should not have been including git2/cred.h directly.

This feels a little weird to me though. I almost feel like we should just include these functions as part of the overall git2.h experience.

src/streams/openssl.c.bak Outdated
@@ -0,0 +1,795 @@
/*
Copy link
Member

Choose a reason for hiding this comment

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

This file doesn't look like it belongs here :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Oops. Thanks - fixed!

@ethomson
credential: change git_cred to git_credential
3f54ba8 
We avoid abbreviations where possible; rename git_cred to
git_credential.

In addition, we have standardized on a trailing `_t` for enum types,
instead of using "type" in the name.  So `git_credtype_t` has become
`git_credential_t` and its members have become `GIT_CREDENTIAL` instead
of `GIT_CREDTYPE`.

Finally, the source and header files have been renamed to `credential`
instead of `cred`.

Keep previous name and values as deprecated, and include the new header
files from the previous ones.
pks-t
pks-t approved these changes Jan 30, 2020
View reviewed changes
Copy link
Member

@pks-t pks-t left a comment

Choose a reason for hiding this comment

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

Let's not hold this up because of that missing space, right? ;) Thanks a lot for your work, @ethomson!

src/transports/credential.c
const char *username,
const char *password)
{
return git_credential_userpass_plaintext_new(out,username, password);
Copy link
Member

Choose a reason for hiding this comment

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

Missing space

@pks-t pks-t merged commit aa4cd77 into master Jan 30, 2020
@ethomson ethomson deleted the ethomson/credtype branch February 2, 2020 13:06
@implausible implausible mentioned this pull request Feb 26, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pks-t pks-t pks-t approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ethomson @pks-t