Skip to content

Conversation

sbeckeriv
Copy link
Contributor

Dearest Reviewer

This is to support environment variables for name and email more like
git. This closes #1213 . I did look in to using libgit2 but I did not
see a clear way to get the author ident. This just moves from or_else
chaining to map_filter with env checks and getting the nth(0) result.

The docs I found
https://git-scm.com/book/en/v2/Git-Internals-Environment-Variables#Committing
do not really indicate an order of preference to the variables. I am
more then happy to rearrange the arrays in anyway.

I also did not add in a new test for the positive case because the
test for the current variables cover that path.

Thanks for reviewing,
Becker

@rust-highfive
Copy link

r? @alexcrichton

(rust_highfive has picked a reviewer for you, use r? to override)

@alexcrichton
Copy link
Member

Thanks! Maybe this functionality could be refactored a bit as well? (e.g. get an env var from a list of possible keys).

Also, could you add a test for one of the GIT_* variables too?

@sbeckeriv sbeckeriv force-pushed the an-author-by-another-name-1213 branch from 0f9207c to ab22961 Compare May 16, 2016 20:22
@sbeckeriv
Copy link
Contributor Author

Moved to a function and added test for name and email.

Copy link
Member

Choose a reason for hiding this comment

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

&Vec<T> is typically more idiomatically written as &[T] in this case (you can avoid the vec! as well)

Copy link
Member

Choose a reason for hiding this comment

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

Could you add a space after this > as well?

@sbeckeriv sbeckeriv force-pushed the an-author-by-another-name-1213 branch from ab22961 to 91e4065 Compare May 16, 2016 23:50
Dearest Reviewer

This is to support environment variables for name and email more like
git. This closes rust-lang#1213 . I did look in to using libgit2 but I did not
see a clear way to get the author ident. This just moves from or_else
chaining to map_filter with env checks and getting the nth(0) result.

The docs I found
https://git-scm.com/book/en/v2/Git-Internals-Environment-Variables#Committing
do not really indicate an order of preference to the variables. I am
more then happy to rearrange the arrays in anyway.

Thanks for reviewing,
Becker
@sbeckeriv
Copy link
Contributor Author

Thanks for the comments. They are all addressed. I also was trying to figure out how to write the array signature. Thanks for that!

Becker

@alexcrichton
Copy link
Member

@bors: r+ 91e4065

@bors
Copy link
Contributor

bors commented May 17, 2016

⌛ Testing commit 91e4065 with merge 4e009a6...

bors added a commit that referenced this pull request May 17, 2016
…xcrichton

An author by another name

Dearest Reviewer

This is to support environment variables for name and email more like
git. This closes #1213 . I did look in to using libgit2 but I did not
see a clear way to get the author ident. This just moves from or_else
chaining to map_filter with env checks and getting the nth(0) result.

The docs I found
https://git-scm.com/book/en/v2/Git-Internals-Environment-Variables#Committing
do not really indicate an order of preference to the variables. I am
more then happy to rearrange the arrays in anyway.

I also did not add in a new test for the positive case because the
test for the current variables cover that path.

Thanks for reviewing,
Becker
@bors
Copy link
Contributor

bors commented May 17, 2016

@bors bors merged commit 91e4065 into rust-lang:master May 17, 2016
@joshtriplett
Copy link
Member

Thanks for working on this!

@sbeckeriv wrote:

The docs I found
https://git-scm.com/book/en/v2/Git-Internals-Environment-Variables#Committing
do not really indicate an order of preference to the variables.

"man git-commit-tree" provides the full precedence:

       author and
       committer information is taken from the following environment
       variables, if set:

           GIT_AUTHOR_NAME
           GIT_AUTHOR_EMAIL
           GIT_AUTHOR_DATE
           GIT_COMMITTER_NAME
           GIT_COMMITTER_EMAIL
           GIT_COMMITTER_DATE

       (nb "<", ">" and "\n"s are stripped)

       In case (some of) these environment variables are not set, the
       information is taken from the configuration items user.name and
       user.email, or, if not present, the environment variable EMAIL, or, if
       that is not set, system user name and the hostname used for outgoing
       mail (taken from /etc/mailname and falling back to the fully qualified
       hostname when that file does not exist).

I don't think the last fallback (to the username, mailname, and FQDN) makes sense anymore. So the git precedence for name is $GIT_AUTHOR_NAME or $GIT_COMMITTER_NAME, then user.name; the git precedence for email is $GIT_AUTHOR_EMAIL or $GIT_COMMITTER_EMAIL, then user.email, then $EMAIL.

Adding a name fallback to the generic $NAME makes sense, but it should come after the git-specific environment variables and then user.name. Using $USER or $USERNAME doesn't make sense, because those only contain a login name, which doesn't make sense in the author field. As for AUTHOR versus COMMITTER, I'd tend to favor the former as you did, but it shouldn't matter. Finally, you might want to add a $CARGO_NAME and $CARGO_EMAIL, on the off chance someone wants to customize this. (For instance, someone might use a specific email address for their Rust work.)

So, I'd suggest the following:

Name: $CARGO_NAME, $GIT_AUTHOR_NAME, $GIT_COMMITTER_NAME, user.name, $NAME

Email: $CARGO_EMAIL, $GIT_AUTHOR_EMAIL, $GIT_COMMITTER_EMAIL, user.email, $EMAIL.

In both cases, the presence of the config item in the middle of the list makes it necessary to split the array, or introduce an enum, or go back to and_then.

@sbeckeriv sbeckeriv deleted the an-author-by-another-name-1213 branch May 17, 2016 01:33
sbeckeriv added a commit to sbeckeriv/cargo that referenced this pull request May 17, 2016
Dearest Reviewer,

PR rust-lang#2696 added some new checks for environment variables for author and
email. After the pull request was merged a comment was left about gits
ordering for looking at the variables. This pull request closes rust-lang#2705 by
reordering the look up and also addeds CARGO_NAME and CARGO_EMAIL to the
top of the order.

Thanks
Becker
@sbeckeriv sbeckeriv mentioned this pull request May 17, 2016
bors added a commit that referenced this pull request May 18, 2016
Correct author order

Dearest Reviewer,

PR #2696 added some new checks for environment variables for author and
email. After the pull request was merged a comment was left about gits
ordering for looking at the variables. This pull request closes #2705 by
reordering the look up and also addeds CARGO_NAME and CARGO_EMAIL to the
top of the order.

Thanks
Becker
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.

cargo new does not detect author name and email for author field
5 participants