pivotal-legacy / homebrew-tap

33 stars 49 forks source link

git-author.rb: add instructions to follow README.md in caveats #130

Closed amilkh closed 5 years ago

amilkh commented 5 years ago

Co-authored-by: Oliver Albertini oalbertini@pivotal.io Co-authored-by: Amil Khanzada akhanzada@pivotal.io

amilkh commented 5 years ago

@professor @xinzweb can you please take a look?

w/ @oliverralbertini

professor commented 5 years ago

I had an IRL chat with @oliverralbertini -- we do not need to alias git to git-together. We should improve the git-author documentation to make this more clear.

If there are other specific steps missing, I'd suggest we call them out or automate them here. It might be too easy to miss this output.

xinzweb commented 5 years ago

I believe the setup for the git-author is complete. People can use git author without aliasing git.

@amilkh What are the scenarios in your mind? Thanks.

professor commented 5 years ago

Here's what I learned from the IRL communication....

For me the authorship is recorded in the git commit message. The commit message is what github uses to display authorship. The team was still looking at the git commit's author field and wanting it to rotate. If you are coming from git-duet, this is a natural tendency. Once you've used git-author for awhile, you'll stop looking there and just focus on the commit message.

oliverralbertini commented 5 years ago

Here's a little workflow I made to demonstrate the alternation issue:

  |2.5.1| coyote-hill in ~/workspace/fake-repo
○ → git init
Initialized empty Git repository in /home/pivotal/workspace/fake-repo/.git/

  |2.5.1| coyote-hill in ~/workspace/fake-repo
± |master ✓| → git author oa ak
Co-authored-by: Oliver Albertini <oalbertini@pivotal.io>
Co-authored-by: Amil Khanzada <akhanzada@pivotal.io>

  |2.5.1| coyote-hill in ~/workspace/fake-repo
± |master ✓| → touch afile && git add afile

  |2.5.1| coyote-hill in ~/workspace/fake-repo
± |master S:1 ✗| → git ci
hint: Waiting for your editor to close the file... error: There was a problem with the editor '/usr/bin/vim'.
Please supply the message using either -m or -F option.

  |2.5.1| coyote-hill in ~/workspace/fake-repo
± |master S:1 ✗| → which \git
/usr/local/bin/git

  |2.5.1| coyote-hill in ~/workspace/fake-repo
± |master S:1 ✗| → \git ci
[master (root-commit) f65a748] test1
 1 file changed, 0 insertions(+), 0 deletions(-)
 create mode 100644 afile

  |2.5.1| coyote-hill in ~/workspace/fake-repo
± |master ✓| → echo hi > afile && git add afile

  |2.5.1| coyote-hill in ~/workspace/fake-repo
± |master S:1 ✗| → \git ci
[master 6070c10] test2
 1 file changed, 1 insertion(+)

  |2.5.1| coyote-hill in ~/workspace/fake-repo
± |master ✓| → type git
git is aliased to `git-together'

  |2.5.1| coyote-hill in ~/workspace/fake-repo
± |master ✓| → echo hello > afile && git add afile

  |2.5.1| coyote-hill in ~/workspace/fake-repo
± |master S:1 ✗| → git ci # using alias git=git-together
[master f2f1641] test3
 Author: Oliver Albertini <oalbertini@pivotal.io>
 1 file changed, 1 insertion(+), 1 deletion(-)

  |2.5.1| coyote-hill in ~/workspace/fake-repo
± |master ✓| → echo hola > afile && git add afile

  |2.5.1| coyote-hill in ~/workspace/fake-repo
± |master S:1 ✗| → git ci
[master c6656b5] test4
 Author: Amil Khanzada <akhanzada@pivotal.io>
 1 file changed, 1 insertion(+), 1 deletion(-)

  |2.5.1| coyote-hill in ~/workspace/fake-repo
± |master ✓| → git log
commit c6656b553d08b5df4a2f5303ba920eae30a77f16 (HEAD -> master)
Author: Amil Khanzada <akhanzada@pivotal.io>
Date:   Thu Mar 7 12:22:04 2019 -0800

    test4

    Co-authored-by: Oliver Albertini <oalbertini@pivotal.io>
    Co-authored-by: Amil Khanzada <akhanzada@pivotal.io>

commit f2f16411f911728c645421126575ee3e11b6ad0c
Author: Oliver Albertini <oalbertini@pivotal.io>
Date:   Thu Mar 7 12:21:44 2019 -0800

    test3

    Co-authored-by: Oliver Albertini <oalbertini@pivotal.io>
    Co-authored-by: Amil Khanzada <akhanzada@pivotal.io>

commit 6070c107e98995a1a97d9e4e1fa0b4f0e27b5c9f
Author: Oliver Albertini <oalbertini@pivotal.io>
Date:   Thu Mar 7 12:19:12 2019 -0800

    test2

    Co-authored-by: Oliver Albertini <oalbertini@pivotal.io>
    Co-authored-by: Amil Khanzada <akhanzada@pivotal.io>

commit f65a74896af837af155de63a27fa4d62acf0469b
Author: Oliver Albertini <oalbertini@pivotal.io>
Date:   Thu Mar 7 12:18:38 2019 -0800

    test1

    Co-authored-by: Oliver Albertini <oalbertini@pivotal.io>
    Co-authored-by: Amil Khanzada <akhanzada@pivotal.io>

On github.com: image

oliverralbertini commented 5 years ago

Given the above, it looks like we don't have to worry about alternation of author, but from a local perspective it may throw someone off. I think we should update the README to say something about this. There will be a PR coming soon about that.

xinzweb commented 5 years ago

I see what you mean. Yes, without the aliasing, you won't get alternation to pick a person as the primary author.

Git-author doesn't replace git-together, and it doesn't do the git-together setup. Maybe the aliasing should be done as part of the git-together formula instead of git-author formula?

The git-author is only focused on getting the co-authored-by when supporting multiple authors.

Thoughts?

amilkh commented 5 years ago

+1!!

2019年3月7日(木) 午後2:33 Xin Zhang notifications@github.com:

@xinzweb requested changes on this pull request.

In git-author.rb https://github.com/pivotal-legacy/homebrew-tap/pull/130#discussion_r263597704 :

@@ -14,6 +14,9 @@ def install def caveats; <<-EOS export GIT_TOGETHER_NO_SIGNOFF=1 to ~/.bash_profile to disable --signoff added by git-together commit +

  • Please note the setup is incomplete, you must follow instructions in

I will probably just replace the entire caveats (line 15 - 19) with:

Please follow https://github.com/pivotal/git-author#setup for further setup instructions.

Then, we can enhance the README.md overthere. Thoughts?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/pivotal-legacy/homebrew-tap/pull/130#pullrequestreview-212057106, or mute the thread https://github.com/notifications/unsubscribe-auth/ABdaft1A830bRzD0jfHwsO7XisPtbC3hks5vUZOpgaJpZM4bj1Yf .

xinzweb commented 5 years ago

Close this PR, since the new PR #131 has the caveat change. Please comments over there. Thanks.