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

Address headers encoding #34

Merged
merged 3 commits into from Aug 8, 2013

Conversation

rhertzog
Copy link
Contributor

@rhertzog rhertzog commented Aug 7, 2013

Please merge this branch to fix issue #33 with invalid encoding of email addresses.

@mhagger
Copy link
Contributor

mhagger commented Aug 8, 2013

Cool, thanks for the patches!

I was looking into a fix for the encoding problem myself so I was able to give pretty detailed comments. If you fix the line-length problem then I will definitely accept patches 1 and 3. Regarding patch 2, I am willing to be convinced. Is patch 2 necessary for the other two patches? If not, please rebase it to the end of the patch series so that it doesn't hold the other two patches hostage.

Please also sign off your patches in the Git style, as I have to sign them off when I submit the change to Git and I have to be more careful to do things by the book :-)

@rhertzog
Copy link
Contributor Author

rhertzog commented Aug 8, 2013

Thanks for the thorough review, I'll update my branch. The second patch was "only needed" to be able to update the test data, I can certainly move it at the end.

@rhertzog
Copy link
Contributor Author

rhertzog commented Aug 8, 2013

I think that I have taken care of all your comments. I left the "git log using user config" issue aside and just fixed the test suite to work in a clean environment (mimicking what t/test-env was already doing).

RFC 2231 requires us to encode non-ascii characters but we must not encode
the email part of email addresses, only the real name part. Do this
properly now.

Signed-off-by: Raphaël Hertzog <hertzog@debian.org>
This ensures that the code that encodes the real names
works properly, both for a single address and multiple addresses.

Signed-off-by: Raphaël Hertzog <hertzog@debian.org>
$HOME/.gitconfig or /etc/gitconfig could lead to differing behaviour
of some commands used by git-multimail. In my case, it was "git log"
that gained an implicit --decorate.

Signed-off-by: Raphaël Hertzog <hertzog@debian.org>
@rhertzog
Copy link
Contributor Author

rhertzog commented Aug 8, 2013

Here's another update taking care of your latest comments.

@mhagger mhagger merged commit 4ff9baf into git-multimail:master Aug 8, 2013
@mhagger
Copy link
Contributor

mhagger commented Aug 8, 2013

I just merged and pushed your changes. Thanks!

@moy
Copy link
Contributor

moy commented Aug 27, 2013

Some more thoughts about the "use plumbing" Vs "disable config" question: After thinking a bit more about it, I think it makes sense to run "git log" (not "git rev-list") to generate the body of messages and possibly to disable the user's configuration in this case.

Using plumbing is the right thing when the output is meant for machine consumption. But in our case, the output of "git log" is not parsed, but only used to create a message that is meant to be read by the user. Using "git log" is what makes it simple to have multimailhook.logOpts, which accepts exactly what "git log" accepts. Arguably, git-multimail taking into account the repository's configuration (log.decorate) is a good thing. It's not a good idea to follow the user's log.decorate for a shared repository though.

@mhagger
Copy link
Contributor

mhagger commented Sep 11, 2013

Your comments about using "git log" rather than "git rev-list" sound reasonable.

ISTM that there are a few distinct ways of using git-multimail that we should try to keep in mind when considering configuration questions:

  • Central, server-side repo controlled by one human user (e.g., pull request model) or one unix user (e.g., gitolite's setup, where the repo is always accessed by a uid "git"). In such a case the administrator of the repository can set the configuration to get the git-multimail output that he/she wants.
  • Central repo accessed via multiple uids (e.g., an SSH setup which each user accesses using his/her own login). In this case the git config depends on which user happens to have done the push, because the user's ~/.gitconfig comes into play.
  • Local repo for which the user wants to generate emails to him/herself (@moy, this is your use case, right?). In this case, the user can configure git the way he/she wants, but the config doesn't just affect git-multimail (and therefore the user does not necessarily want to optimize his/her config for git-multimail).

Maybe we should make it easier for the user to configure (via the hook script or within the repo configuration) which other Git configs should be read/ignored by git-multimail. This could currently be done by creating a mock HOME directory and setting it in the hook script via $HOME, and setting $GIT_CONFIG_NOSYSTEM to avoid reading /etc/gitconfig, but that's kindof cumbersome.

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.

None yet

3 participants