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

"Co-authored-by" trailer #57

Merged
merged 9 commits into from Mar 21, 2018

Conversation

Projects
None yet
3 participants
@ansd
Copy link
Contributor

ansd commented Feb 1, 2018

This is a proposal of how the co-authored-by trailer could be implemented. It is configurable via the GIT_DUET_CO_AUTHORED_BY environment variable.
Basically, the last commit gets amended by appending the co-authored-by trailer for each contributor. Not sure if there are better ways to do it?

"Co-authored-by" trailer
If the environment variable GIT_DUET_CO_AUTHORED_BY is set to 1,
there will be a "Co-authored-by" trailer added for each co-author
instead of a "Signed-off-by" trailer for the committer.

This gives every contributor attribution in Github's contribution graph.

Resolves #56.
@jszwedko

This comment has been minimized.

Copy link
Member

jszwedko commented Feb 12, 2018

Thanks for taking a crack at this @ansd !

I think what you have here is reasonable, but I'm wondering if we might be able to take this as an opportunity to simplify git-duet.

It would be a bit of a divergence from the current techniques the tool uses that utilize the committer field of commit messages, but we could opt have git duet / git solo actually just manage a commit-msg hook that would inject the Co-Authored by fields. This would have the advantage of eliminating the need to have the git duet-commit, git duet-merge, and other subcommands, allowing users to use their normal git commit workflows.

The downsides are:

  • that it's a little more opaque to the end user as we move from explicit to implicit injection of the co-authored trailers
  • this would remove support for setting the second author as committer when GIT_DUET_CO_AUTHORED_BY is set

While I'm actually OK with the second one as I always viewed usage of the committer field as a hack for lack of a better approach, but is definitely a divergence to drop it.

@ansd

This comment has been minimized.

Copy link
Contributor Author

ansd commented Feb 14, 2018

@jszwedko, thanks for your feedback.

I like the idea of having the Co-authored-by trailer(s) being added as part of a prepare-commit-msg hook. I tried it out here.

Open questions:

  • How does the prepare-commit-msg hook get added?
    • Is it the user executing some git duet-install-prepare-commit-msg-hook like in git-duet-install-hook?
    • If GIT_DUET_GLOBAL is set, the install script would probably do some global commit hook?
    • Does the user place it manually in the (global) hooks folder?
    • Is it an environment variable GIT_DUET_CO_AUTHORED_BY which lets the git duet command install the hook?
  • Is the whole feature optional?
    • If it is not, it would indeed be a simplification because, as you wrote, the duet-commit, duet-revert, duet-merge commands and rotating author / committer support can be deleted?
    • If it is, I think the solution with the commit hook is not necessarily simpler (from a developer perspective) compared to amending the last commit as in this PR? Or, do you mean it's simpler (from a user perspective) because the user doesn't have to type the duet subcommands anymore?
@jszwedko

This comment has been minimized.

Copy link
Member

jszwedko commented Feb 18, 2018

Thanks for putting together that example subcommand! That is very much along the lines of what I was thinking. With regards to your open questions:

  • I was thinking we could automatically install the hook when the user runs git duet or git solo with the GIT_DUET_CO_AUTHORED_BY variable set. Perhaps outputting a message to the user to this effect if the installed file changed. I agree that the GIT_DUET_GLOBAL configuration should cause it to manipulate the global hooks.
  • I think we should make the whole feature optional initially, but aim to move towards making it the default, and only, behavior in the future unless we find evidence to continue supporting the author/committer flow. I feel this new flow would be simpler from the user perspective in that they wouldn't have to use the git-duet-commit, git-duet-merge commands, but could simply git commit and git merge. I think it would also be simpler and less error-prone than our current manipulations of the committer field which have some rough edges and unsupported behavior (e.g. git-duet-rebase).

Thanks again for engaging on this.

@rafecolton

This comment has been minimized.

Copy link
Member

rafecolton commented Feb 20, 2018

@jszwedko It would be good to keep supporting the author/committer workflow until the trailer is officially supported in git, assuming there continues to be progress on the issue, so we can maintain the author & committer info in real metadata and not just the commit message.

It would also be handy to keep existing functionality until use of Co-Authored-By is fully supported in all major hosted Git providers (i.e. GitLab and maybe Bitbucket) and not just GitHub. As GitLab provides unlimited private repos, we use it for 95% of our projects. It would be too bad for myself and other GitLab users to be locked to an old version of git-duet.

ansd added some commits Feb 14, 2018

Prepare commit message by including Co-authored-by trailer
Requires a prepare-commit-msg hook:

#!/usr/bin/env bash
exec git duet-prepare-commit-msg "$1"
@ansd

This comment has been minimized.

Copy link
Contributor Author

ansd commented Feb 23, 2018

@jszwedko, thanks for your answer. I gave it a try.

The prepare-commit-msg hook file gets now installed when running git duet with the GIT_DUET_CO_AUTHORED_BY variable set.

Questions:

  • git solo doesn't manage any hooks, right? Currently, after running git solo the hook file gets invoked when running normal git commands, but no co-author-trailer will be appended.
  • Currently, the are no git-duet commands for deleting the hook files. I guess that's okay? I was first thinking of having git solo delete the hook file. But if GIT_DUET_GLOBAL is set, it would delete the global hook template and not all the repository local hooks. So, not sure if that would be of much value?
@jszwedko

This comment has been minimized.

Copy link
Member

jszwedko commented Feb 25, 2018

Good notes, @rafecolton, thanks for checking me. I agree that we should continue supporting the committer workflow until, at the very least, until the major hosted git providers support it. This can stay as an optional feature until then and we can re-discuss in the future.

@jszwedko
Copy link
Member

jszwedko left a comment

Overall this is looking great, thanks for all of your work on this @ansd

I think the remaining items are:

  • Default GIT_DUET_SET_GIT_USER_CONFIG when GIT_DUET_CO_AUTHORED_BY is set
  • Think a bit more about the various configuration settings users may have had set before setting GIT_DUET_CO_AUTHORED_BY
  • Some manual testing to make sure we aren't missing any cases in the automated tests you added

As this is such a divergence from the normal workflow, I'm also thinking we could mark this feature as experimental in the README to set expectations and encourage people to report any issues they find (both bugs and usability).

Thanks again! 🎉 🎊

os.Exit(1)
}
if committers == nil || len(committers) == 0 {
os.Exit(0)

This comment has been minimized.

Copy link
@jszwedko

jszwedko Feb 25, 2018

Member

This appears to be assuming GIT_DUET_CO_AUTHORED_BY implies GIT_DUET_SET_GIT_USER_CONFIG. I agree with this decision, but I think we'll need update to duet.Configuration to codify this (and maybe include this in the README).

This comment has been minimized.

Copy link
@jszwedko

jszwedko Feb 25, 2018

Member

This also makes me realize we'll need to consider the various paths users might take opting in to this feature, e.g.:

git solo js # without GIT_DUET_SET_GIT_USER_CONFIG, so only sets `git-duet` specific configuration
export GIT_DUET_CO_AUTHORED_BY=1
git commit -m 'test' # will not use the author set by `git solo js`

I'll think about this a bit more.

This comment has been minimized.

Copy link
@ansd

ansd Mar 1, 2018

Author Contributor

I think there is just a single path how users can opt in, namely by running git duet with GIT_DUET_CO_AUTHORED_BY set.

I think in your above example, it's okay that the commit doesn't set js as author since the user didn't run git solo with either GIT_DUET_SET_GIT_USER_CONFIG or GIT_DUET_CO_AUTHORED_BY (the latter implicitly setting GIT_DUET_SET_GIT_USER_CONFIG).

This comment has been minimized.

Copy link
@jszwedko

jszwedko Mar 10, 2018

Member

Right, I'm just thinking that people might have an expectation, in my example, that GIT_DUET_CO_AUTHORED_BY would pick up the previous git solo even without GIT_DUET_SET_USER_CONFIG. I'm willing to concede that it feels likely that users will set that variable and then call git duet though.


contents := strings.TrimSpace(string(b))
if contents != "" {
if hook == preCommitHook && contents != strings.TrimSpace(preCommitHook) ||

This comment has been minimized.

Copy link
@jszwedko

jszwedko Feb 25, 2018

Member

I'm concerned that this doesn't allow for future versions of git-duet to overwrite with updated hook contents.

This comment has been minimized.

Copy link
@jszwedko

jszwedko Feb 25, 2018

Member

However, this should be rare and I think we can address it if it comes up by including an index of previous hook contents for comparison (or hashes) so I'm OK with this as-is.

@ansd

This comment has been minimized.

Copy link
Contributor Author

ansd commented Mar 1, 2018

@jszwedko, concerning the remaining items:

  • GIT_DUET_CO_AUTHORED_BY now implicitly defaults GIT_DUET_SET_GIT_USER_CONFIG. It's documented in the README
  • I included some documentation about the configuration settings in the README, e.g. that GIT_DUET_ROTATE_AUTHOR doesn't apply anymore in this scenario. GIT_DUET_GLOBAL and GIT_DUET_SECONDS_AGO_STALE should actually work.
  • I did some manual tests and added some automated tests for merging, reverting, and rebasing. Rebasing in non-interactive mode works as intended by neither touching the author field nor modifying the commit message, but only resetting the committer. Rebasing in interactive mode also preserves the author field but seems to call the prepare-commit-msg hook which appends another co-authored-by trailer to the end of the commit message. I'm not happy about this latter behaviour but don't see a good solution currently.
  • This feature is marked as experimental in the README

Thank you for reviewing the code :)

@@ -67,6 +67,10 @@ func main() {
printNextComitter(committers)
if configuration.CoAuthoredBy {
installPrepareCommitMsgHook()
if err = gitConfig.SetAuthor(author); err != nil {

This comment has been minimized.

Copy link
@jszwedko

jszwedko Mar 10, 2018

Member

Can you clarify why this is needed? It wasn't immediately apparent to me (maybe a comment here would help).

This comment has been minimized.

Copy link
@ansd

ansd Mar 13, 2018

Author Contributor

This is needed for the case that the user runs GIT_DUET_CO_AUTHORED_BY=1 git duet without args (author initials) and without having set GIT_DUET_SET_GIT_USER_CONFIG in previous git duet commands. If the above line is missing, this test will fail. I will add a comment for this.

The basic question is whether running git duet without args (author initials) should install the hook (and set the author like above) or should it be just a "getter" like it always has been which merely prints the current author and committer?

@jszwedko

This comment has been minimized.

Copy link
Member

jszwedko commented Mar 10, 2018

Fantastic, thanks @ansd . I appreciate you adding the additional tests and changes.

I think that, for the git rebase --interactive case, that the precommit hook could look for existing co-author trailers and avoid adding duplicate ones. Do you think that'd be feasible?

ansd added some commits Mar 15, 2018

Avoid duplicate Co-authored-by trailers
When interactively rebasing or cherry-picking, the prepare-commit-msg
hook gets called. Previously, a Co-authored-by trailer has been
appended. This change checks first if there is already a trailer, and if
there is one, no further trailer will be added since rebasing and
cherry-picking doesn't change authorship.

Current drawbacks:
- if the commit being rebased doesn't have a trailer, a new trailer
will still be appended
- git commit --amend will not append new trailers if there is already a
trailer
Put trailers block before the start of the comments
Before this commit, "git commit --verbose" didn't append any trailers
because the trailers were added after the comments and diff.
The right place for the trailers block is before the start of the comments.
This is easily achieved with the "git interpret-trailers" command.
@ansd

This comment has been minimized.

Copy link
Contributor Author

ansd commented Mar 15, 2018

@jszwedko, sure, that's a good idea. See changes above.

Just to summarise:

The goal for rebasing and cherry-picking is to not add any Co-authored-by trailers at all. Since the hook doesn't know whether it is invoked as part of rebasing or cherry-picking, as a workaround, it currently checks if there are already trailers in its commit message, and if there are any, it doesn't append a new trailer.

So, the only drawback for now is that a new Co-authored-by trailer will be appended in interactive rebasing or cherry-picking if there is no trailer already in that commit message. I guess we can live with that?

Also, git commit --amend now works the way expected, by adding a new trailer for a new co-author, but not adding duplicate trailers (i.e. trailers with the same co-author).

as part of rebasing or cherry-picking, at the very least, it checks for existing trailers,
and if there is one, no new trailers will be appended.
Trailers will still be appended for "git commit --amend" in which case the
commitMsgSource's value is "commit". */

This comment has been minimized.

Copy link
@jszwedko
err = ioutil.WriteFile(commitMsgFile, []byte(fmt.Sprintf("%s\n%s", string(commitMsg), coAuthorsTrailer)), 0644)
for _, c := range committers {
trailer := "Co-authored-by: " + c.Name + " <" + c.Email + ">"
cmd := exec.Command("git", "interpret-trailers", "--in-place", "--trailer", trailer, commitMsgFile)

This comment has been minimized.

Copy link
@jszwedko

jszwedko Mar 17, 2018

Member

TIL this command exists.

This comment has been minimized.

Copy link
@ansd

ansd Mar 19, 2018

Author Contributor

I also just saw it by coincidence looking at the prepare-commit-msg.sample file :)

}

// override the default "addIfDifferentNeighbor" so that no duplicate trailers will get appended
err = gitConfig.SetUnnamespacedKey("trailer.ifexists", "addIfDifferent")

This comment has been minimized.

Copy link
@jszwedko

jszwedko Mar 17, 2018

Member

I think setting this un-namespaced variable here might cause some slight confusion to people already depending on a different behavior. I admit it's probably pretty rare, but what do you think, rather than setting this here, we just document that you can set this value to avoid duplicate Co-authored-by:s for the same author?

This comment has been minimized.

Copy link
@ansd

ansd Mar 19, 2018

Author Contributor

Right, makes sense. I removed this part from the hook and wrote it into the README so that the user can optionally set it.

return err
}
return nil
}

func (gc *GitConfig) getKey(key string) (value string, err error) {
func (gc *GitConfig) SetUnnamespacedKey(key, value string) (err error) {

This comment has been minimized.

Copy link
@jszwedko

jszwedko Mar 17, 2018

Member

I think, if we decide to keep the setting of the trailer config above, I'd prefer to see that exposed as a new method that abstracts away how it is setting the value (e.g. what you have for SetInitTemplateDir) rather than exposing this method.

This comment has been minimized.

Copy link
@ansd

ansd Mar 19, 2018

Author Contributor

True, that would have been better! I reverted the changes to this file to the previous commit since we decide to not set trailer.ifexists in the hook.

@jszwedko

This comment has been minimized.

Copy link
Member

jszwedko commented Mar 17, 2018

Hi @ansd,

Thanks for your continued work on this! I really appreciate it.

I left a couple of comments in-line. I really like the change to using git-interpret-trailers to add the trailers.

🎆

Prepare-commit-msg hook does not set git config trailer.ifexists
Rather than having the prepare-commit-msg hook set this
variable's value to `addIfDifferent`, let the user set it manually
(just in case they already depend on different behaviour).
@ansd

This comment has been minimized.

Copy link
Contributor Author

ansd commented Mar 19, 2018

Hi @jszwedko, thanks for your comments. I removed setting the git config trailer.ifexists from the hook. Please, let me know if there are other improvements to make :)

README.md Outdated
When amending a commit and the co-author has changed, a new `Co-authored-by` trailer will get appended for
that co-author. In order to avoid duplicate `Co-authored-by` trailers (i.e. trailers with the same co-author),
set `git config [--global] trailer.ifexists addIfDifferent` to override the default value `addIfDifferentNeighbor`.

This comment has been minimized.

Copy link
@jszwedko
@jszwedko

This comment has been minimized.

Copy link
Member

jszwedko commented Mar 21, 2018

Looks great! Thanks again for all of your hard work @ansd . I think this is ready to merge.

@jszwedko jszwedko merged commit 3c83bbc into git-duet:master Mar 21, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jszwedko

This comment has been minimized.

Copy link
Member

jszwedko commented Mar 21, 2018

Released as 0.6.0 🎉 🎺

@ansd ansd deleted the ansd:co-authored-by-trailer branch Mar 21, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.