Skip to content

cred: change enum to git_credential_t and GIT_CREDENTIAL_*#5336

Merged
pks-t merged 1 commit into
masterfrom
ethomson/credtype
Jan 30, 2020
Merged

cred: change enum to git_credential_t and GIT_CREDENTIAL_*#5336
pks-t merged 1 commit into
masterfrom
ethomson/credtype

Conversation

@ethomson

@ethomson ethomson commented Dec 13, 2019 •
edited
Loading

Copy link
Copy Markdown
Member

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

ethomson commented Dec 13, 2019

Copy link
Copy Markdown
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

pks-t commented Dec 13, 2019

Copy link
Copy Markdown
Member

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

ethomson commented Jan 8, 2020

Copy link
Copy Markdown
Member Author

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

ethomson commented Jan 18, 2020

Copy link
Copy Markdown
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

ethomson commented Jan 18, 2020

Copy link
Copy Markdown
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
Comment thread include/git2/cred_helpers.h
/* These declarations have moved. */
#ifndef GIT_DEPRECATE_HARD
# include "git2/credential_helpers.h"
#endif

@pks-t pks-t Jan 24, 2020

Copy link
Copy Markdown
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"?

@ethomson ethomson Jan 24, 2020

Copy link
Copy Markdown
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.

Comment thread src/streams/openssl.c.bak Outdated
@@ -0,0 +1,795 @@
/*

@pks-t pks-t Jan 24, 2020

Copy link
Copy Markdown
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 :)

@ethomson ethomson Jan 24, 2020

Copy link
Copy Markdown
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

@pks-t pks-t left a comment

Copy link
Copy Markdown
Member

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!

Comment thread src/transports/credential.c
const char *username,
const char *password)
{
return git_credential_userpass_plaintext_new(out,username, password);

@pks-t pks-t Jan 30, 2020

Copy link
Copy Markdown
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