Double From:s in StGIT's patch email template

StGIT's default patch email template has both "From: %(maintainer)s"
in the e-mail header, and "From: %(authname)s <%(authemail)s>" in the
e-mail body. Why?

-- 
Karl Hasselström, kha@treskal.com

On 2006-11-11 23:02:04 +0000, Catalin Marinas wrote:

On 11/11/06, Karl Hasselström [off-list ref] wrote:

quoted

Is there any particular reason to have the author and committer
names in ~/.stgitrc? Simply taking them from the same place git
does would probably be a usability enhancement (unless they're
specified on the command line, of course).

At the time I added these to .stgitrc, the only place git was taking
them from was the environment variables and I wanted to put them in a
single place. I also didn't like the idea of having the committer
e-mail address be some username@local-machine as I don't think the
name of the machine where I create patches is relevant. I also define
the committer/author per repository in the .git/stgitrc file (i.e. I
use @arm.com for Linux patches and @gmail.com for StGIT).

Well, this should all be sorted out now; git has both per-repository
and per-user config files.

I use StGIT almost exclusively, even in "maintainer" mode and I
would like not to spread the configuration options over many files.
It is on my todo list to use the same configuration file as git
(with a [stgit] section) since it has a format that should be
understood by the Python config module.

The last patch in this series deprecates name and email config in
stgitrc by not mentioning them in the example stgitrc, because
teaching newbies to use yet another layer of identity configuration on
top of what git already provides is madness. Old-timers may continue
using stgitrc for that purpose for now (but as you say, integrating
the configuration with git is on the TODO list).

-- 
Karl Hasselström, kha@treskal.com

From: Karl Hasselström <redacted>

There are two distinct disadvantages to having author and committer
names in stgitrc:

  * They override GIT_AUTHOR_NAME and friends, not the other way
    around as one might imagine.

  * They cause stgit and plain git to use different names (unless one
    manages to keep them in sync manually), which can't possibly be a
    good idea.

This patch removes the description of these variables in the example
stgitrc, so that new users aren't tempted to use them. They will still
continue to function, however.

Signed-off-by: Karl Hasselström <redacted>
---

 examples/stgitrc |    5 -----
 stgit/git.py     |    3 +++
 2 files changed, 3 insertions(+), 5 deletions(-)

diff --git a/examples/stgitrc b/examples/stgitrc
index 45a629b..9cd95e1 100644
--- a/examples/stgitrc
+++ b/examples/stgitrc

@@ -3,11 +3,6 @@
 # the former.
 
 [stgit]
-# Default author/committer details
-#authname:  Your Name
-#authemail: your.name@yourcompany.com
-#commname:  Your Name
-#commemail: your.name@yourcompany.com
 
 # Automatically Bcc the address below
 #autobcc: your.name@yourcompany.com

diff --git a/stgit/git.py b/stgit/git.py
index a6e1a63..f654cde 100644
--- a/stgit/git.py
+++ b/stgit/git.py

@@ -452,6 +452,9 @@ def __git_person(p):
 
 __cached_stgit_persons = {}
 def __stgit_person(p, name_key, email_key):
+    """Calls __git_person to get the details for a person, but allows
+    the deprecated author and committer variables in stgitrc override
+    the result."""
     if not p in __cached_stgit_persons:
         person = __git_person(p)

On 2006-11-11 23:02:04 +0000, Catalin Marinas wrote:

I use StGIT almost exclusively, even in "maintainer" mode

It used to be the case that I did almost all my comitting with stgit
(while still using other git tools, such as gitk, reset, bisect, fetch
etc.). But then I discovered the extremely handy git emacs modes, and
started using them to commit stuff. Thus the need for "stg
assimilate". :-)

-- 
Karl Hasselström, kha@treskal.com

On 2006-11-11 21:26:31 +0100, Robin Rosenberg wrote:

lördag 11 november 2006 15:57 skrev Karl Hasselström:

quoted

But I haven't gotten the impression that specifying them in
~/.stgitrc is deprecated. The example stgitrc has a section with
author name and committer name, for example.

The only docs I know of that mentions stgitrc also states that it
isn't required, so why use it unless you have to (or for some reason
want to)? Just because there are many ways, doesn't mean all but one
have to be deprecated.

No, but having a config option for something that git already provides
several ways to specify can't possibly be a good idea, especially
usability-wise. The only use I can think of is if you _want_ to have
different identities for git and stgit in the same repository, and
that's just mad.

I'm preparing a patch to fix this. Stay tuned. :-)

-- 
Karl Hasselström, kha@treskal.com

On 2006-11-11 15:31:15 +0100, Robin Rosenberg wrote:

lördag 11 november 2006 15:15 skrev Karl Hasselström:

quoted

Is there any particular reason to have the author and committer
names in ~/.stgitrc? Simply taking them from the same place git
does would probably be a usability enhancement (unless they're
specified on the command line, of course).

AFAIK StGit already does that, at least if you (like me) do not have
a .stgitrc.

But I haven't gotten the impression that specifying them in ~/.stgitrc
is deprecated. The example stgitrc has a section with author name and
committer name, for example.

-- 
Karl Hasselström, kha@treskal.com

From: Karl Hasselström <redacted>

Consistently do the following to get hold of default user and
committer:

  1. Use the value specified on the command line, if any.

  1. Otherwise, use the value from stgitrc, if available.

  2. Otherwise, ask git for the value. git will produce the value from
     on of its config files, from environment variables, or make it
     up. It might be asking the spirits of the dead for all we care.

Signed-off-by: Karl Hasselström <redacted>
---

 stgit/commands/mail.py |   19 ++------------
 stgit/git.py           |   63 ++++++++++++++++++++++++++++++++++++++++++++++++
 stgit/stack.py         |   32 ++++--------------------
 3 files changed, 70 insertions(+), 44 deletions(-)

diff --git a/stgit/commands/mail.py b/stgit/commands/mail.py
index 154df9c..78abfa4 100644
--- a/stgit/commands/mail.py
+++ b/stgit/commands/mail.py

@@ -127,17 +127,6 @@ options = [make_option('-a', '--all',
                        action = 'store_true')]
 
 
-def __get_maintainer():
-    """Return the 'authname <authemail>' string as read from the
-    configuration file
-    """
-    if config.has_option('stgit', 'authname') \
-           and config.has_option('stgit', 'authemail'):
-        return '%s <%s>' % (config.get('stgit', 'authname'),
-                            config.get('stgit', 'authemail'))
-    else:
-        return None
-
 def __parse_addresses(addresses):
     """Return a two elements tuple: (from, [to])
     """

@@ -301,9 +290,7 @@ def edit_message(msg):
 def __build_cover(tmpl, total_nr, msg_id, options):
     """Build the cover message (series description) to be sent via SMTP
     """
-    maintainer = __get_maintainer()
-    if not maintainer:
-        maintainer = ''
+    maintainer = git.user()
 
     if options.version:
         version_str = ' %s' % options.version

@@ -370,9 +357,7 @@ def __build_message(tmpl, patch, patch_n
     short_descr = descr_lines[0].rstrip()
     long_descr = '\n'.join(descr_lines[1:]).lstrip()
 
-    maintainer = __get_maintainer()
-    if not maintainer:
-        maintainer = '%s <%s>' % (p.get_commname(), p.get_commemail())
+    maintainer = git.user()
 
     if options.version:
         version_str = ' %s' % options.version

diff --git a/stgit/git.py b/stgit/git.py
index 8d88769..a6e1a63 100644
--- a/stgit/git.py
+++ b/stgit/git.py

@@ -33,6 +33,35 @@ class GitException(Exception):
 #
 # Classes
 #
+
+class Person:
+    """An author, committer, etc."""
+    def __init__(self, name = None, email = None, date = None,
+                 desc = None):
+        if name or email or date:
+            assert not desc
+            self.name = name
+            self.email = email
+            self.date = date
+        elif desc:
+            assert not (name or email or date)
+            def parse_desc(s):
+                m = re.match(r'^(.+)<(.+)>(.*)$', s)
+                assert m
+                return [x.strip() or None for x in m.groups()]
+            self.name, self.email, self.date = parse_desc(desc)
+    def set_name(self, val):
+        if val:
+            self.name = val
+    def set_email(self, val):
+        if val:
+            self.email = val
+    def __str__(self):
+        if self.name and self.email:
+            return '%s <%s>' % (self.name, self.email)
+        else:
+            raise Exception, 'not enough identity data'
+
 class Commit:
     """Handle the commit objects
     """

@@ -402,6 +431,40 @@ def rm(files, force = False):
         if files:
             __run('git-update-index --force-remove --', files)
 
+def var(key):
+    """Ask git-var for the value of a variable."""
+    return _output_one_line(['git-var', key])
+
+def repo_config(key):
+    """Ask git-repo-config for the value of a variable."""
+    return _output_one_line(['git-repo-config', key])
+
+__cached_git_persons = {}
+def __git_person(p):
+    if not p in __cached_git_persons:
+        __cached_git_persons[p] = {
+            'author': lambda: Person(desc = var('GIT_AUTHOR_IDENT')),
+            'committer': lambda: Person(desc = var('GIT_COMMITTER_IDENT')),
+            'user': lambda: Person(repo_config('user.name'),
+                                   repo_config('user.email')),
+            }[p]()
+    return __cached_git_persons[p]
+
+__cached_stgit_persons = {}
+def __stgit_person(p, name_key, email_key):
+    if not p in __cached_stgit_persons:
+        person = __git_person(p)
+        if config.has_option('stgit', name_key):
+            person.set_name(config.get('stgit', name_key))
+        if config.has_option('stgit', email_key):
+            person.set_email(config.get('stgit', email_key))
+        __cached_stgit_persons[p] = person
+    return __cached_stgit_persons[p]
+
+def author(): return __stgit_person('author', 'authname', 'authemail')
+def committer(): return __stgit_person('committer', 'commname', 'commemail')
+def user(): return __stgit_person('user', 'authname', 'authemail')
+
 def update_cache(files = None, force = False):
     """Update the cache information for the given files
     """

diff --git a/stgit/stack.py b/stgit/stack.py
index a477e7d..fee1316 100644
--- a/stgit/stack.py
+++ b/stgit/stack.py

@@ -238,53 +238,31 @@ class Patch:
         return self.__get_field('authname')
 
     def set_authname(self, name):
-        if not name:
-            if config.has_option('stgit', 'authname'):
-                name = config.get('stgit', 'authname')
-            elif 'GIT_AUTHOR_NAME' in os.environ:
-                name = os.environ['GIT_AUTHOR_NAME']
-        self.__set_field('authname', name)
+        self.__set_field('authname', name or git.author().name)
 
     def get_authemail(self):
         return self.__get_field('authemail')
 
     def set_authemail(self, address):
-        if not address:
-            if config.has_option('stgit', 'authemail'):
-                address = config.get('stgit', 'authemail')
-            elif 'GIT_AUTHOR_EMAIL' in os.environ:
-                address = os.environ['GIT_AUTHOR_EMAIL']
-        self.__set_field('authemail', address)
+        self.__set_field('authemail', address or git.author().email)
 
     def get_authdate(self):
         return self.__get_field('authdate')
 
     def set_authdate(self, date):
-        if not date and 'GIT_AUTHOR_DATE' in os.environ:
-            date = os.environ['GIT_AUTHOR_DATE']
-        self.__set_field('authdate', date)
+        self.__set_field('authdate', date or git.author().date)
 
     def get_commname(self):
         return self.__get_field('commname')
 
     def set_commname(self, name):
-        if not name:
-            if config.has_option('stgit', 'commname'):
-                name = config.get('stgit', 'commname')
-            elif 'GIT_COMMITTER_NAME' in os.environ:
-                name = os.environ['GIT_COMMITTER_NAME']
-        self.__set_field('commname', name)
+        self.__set_field('commname', name or git.committer().name)
 
     def get_commemail(self):
         return self.__get_field('commemail')
 
     def set_commemail(self, address):
-        if not address:
-            if config.has_option('stgit', 'commemail'):
-                address = config.get('stgit', 'commemail')
-            elif 'GIT_COMMITTER_EMAIL' in os.environ:
-                address = os.environ['GIT_COMMITTER_EMAIL']
-        self.__set_field('commemail', address)
+        self.__set_field('commemail', address or git.committer().email)
 
     def get_log(self):

On 11/11/06, Karl Hasselström [off-list ref] wrote:

Consistently do the following to get hold of default user and
committer:

  1. Use the value specified on the command line, if any.

  1. Otherwise, use the value from stgitrc, if available.

  2. Otherwise, ask git for the value. git will produce the value from
     on of its config files, from environment variables, or make it
     up. It might be asking the spirits of the dead for all we care.

I haven't forgot about this patch. I switched StGIT to the gitconfig
files and there won't be a need to invoke "git repo-config". I'll try
to modify your patch.

-- 

lördag 11 november 2006 15:15 skrev Karl Hasselström:

Is there any particular reason to have the author and committer names
in ~/.stgitrc? Simply taking them from the same place git does would
probably be a usability enhancement (unless they're specified on the
command line, of course).

AFAIK StGit already does that, at least if you (like me) do not have 
a .stgitrc.

From: Karl Hasselström <redacted>

%(endofheaders)s and %(date)s are deprecated and expand to the empty
string, so don't mention them in the help text.

Signed-off-by: Karl Hasselström <redacted>
---

 stgit/commands/mail.py |   13 ++++---------
 1 files changed, 4 insertions(+), 9 deletions(-)

diff --git a/stgit/commands/mail.py b/stgit/commands/mail.py
index 78abfa4..d4fc293 100644
--- a/stgit/commands/mail.py
+++ b/stgit/commands/mail.py

@@ -52,19 +52,14 @@ SMTP authentication is also possible wit
 '--smtp-password' options, also available as configuration settings:
 'smtpuser' and 'smtppassword'.
 
-The template e-mail headers and body must be separated by
-'%(endofheaders)s' variable, which is replaced by StGIT with
-additional headers and a blank line. The patch e-mail template accepts
-the following variables:
+The patch e-mail template accepts the following variables:
 
   %(patch)s        - patch name
   %(maintainer)s   - 'authname <authemail>' as read from the config file
   %(shortdescr)s   - the first line of the patch description
   %(longdescr)s    - the rest of the patch description, after the first line
-  %(endofheaders)s - delimiter between e-mail headers and body
   %(diff)s         - unified diff of the patch
   %(diffstat)s     - diff statistics
-  %(date)s         - current date/time
   %(version)s      - ' version' string passed on the command line (or empty)
   %(prefix)s       - 'prefix ' string passed on the command line
   %(patchnr)s      - patch number

@@ -76,9 +71,9 @@ the following variables:
   %(commname)s     - committer's name
   %(commemail)s    - committer's e-mail
 
-For the preamble e-mail template, only the %(maintainer)s, %(date)s,
-%(endofheaders)s, %(version)s, %(patchnr)s, %(totalnr)s and %(number)s
-variables are supported."""
+For the preamble e-mail template, only the %(maintainer)s,
+%(version)s, %(patchnr)s, %(totalnr)s and %(number)s variables are
+supported."""
 
 options = [make_option('-a', '--all',

Is there any particular reason to have the author and committer names
in ~/.stgitrc? Simply taking them from the same place git does would
probably be a usability enhancement (unless they're specified on the
command line, of course).

-- 
Karl Hasselström, kha@treskal.com

On 2006-11-11 12:35:53 +0100, Karl Hasselström wrote:

StGIT's default patch email template has both "From: %(maintainer)s"
in the e-mail header, and "From: %(authname)s <%(authemail)s>" in
the e-mail body. Why?

Ah, I just figured it out. "maintainer" is the person sending the
mail, and "author" is the person who wrote the patch. In general, they
are not the same.

Hmm. It would be nice to omit the second From: in case they really are
the same.

-- 
Karl Hasselström, kha@treskal.com

On 2006-11-12 00:30:46 +0100, Karl Hasselström wrote:

  1. Use the value specified on the command line, if any.

  1. Otherwise, use the value from stgitrc, if available.

  2. Otherwise, ask git for the value. git will produce the value
     from on of its config files, from environment variables, or
     make it up. It might be asking the spirits of the dead for all
     we care.

Oops. Feel free to renumber these points as you see fit. :-)

-- 
Karl Hasselström, kha@treskal.com

On 05/12/06, Karl Hasselström [off-list ref] wrote:

On 2006-12-04 22:24:52 +0000, Catalin Marinas wrote:

quoted

I haven't forgot about this patch. I switched StGIT to the gitconfig
files and there won't be a need to invoke "git repo-config". I'll
try to modify your patch.

Ah, nice. But will you still honor the git author and committer
environment variables that way?

Yes. My understanding is that the environment variables would override
the config options. I would also keep the [stgit] user information for
backward compatibility (there are people who use StGIT exclusively and
don't want to dig into GIT configs).

-- 

On 2006-12-04 22:24:52 +0000, Catalin Marinas wrote:

On 11/11/06, Karl Hasselström [off-list ref] wrote:

quoted

Consistently do the following to get hold of default user and
committer:

  1. Use the value specified on the command line, if any.

  1. Otherwise, use the value from stgitrc, if available.

  2. Otherwise, ask git for the value. git will produce the value
     from on of its config files, from environment variables, or
     make it up. It might be asking the spirits of the dead for
     all we care.

I haven't forgot about this patch. I switched StGIT to the gitconfig
files and there won't be a need to invoke "git repo-config". I'll
try to modify your patch.

Ah, nice. But will you still honor the git author and committer
environment variables that way?

-- 
Karl Hasselström, kha@treskal.com

On 11/11/06, Karl Hasselström [off-list ref] wrote:

Is there any particular reason to have the author and committer names
in ~/.stgitrc? Simply taking them from the same place git does would
probably be a usability enhancement (unless they're specified on the
command line, of course).

At the time I added these to .stgitrc, the only place git was taking
them from was the environment variables and I wanted to put them in a
single place. I also didn't like the idea of having the committer
e-mail address be some username@local-machine as I don't think the
name of the machine where I create patches is relevant. I also define
the committer/author per repository in the .git/stgitrc file (i.e. I
use @arm.com for Linux patches and @gmail.com for StGIT).

I use StGIT almost exclusively, even in "maintainer" mode and I would
like not to spread the configuration options over many files. It is on
my todo list to use the same configuration file as git (with a [stgit]
section) since it has a format that should be understood by the Python
config module.

-- 

lördag 11 november 2006 15:57 skrev Karl Hasselström:

On 2006-11-11 15:31:15 +0100, Robin Rosenberg wrote:

quoted

lördag 11 november 2006 15:15 skrev Karl Hasselström:

quoted

Is there any particular reason to have the author and committer
names in ~/.stgitrc? Simply taking them from the same place git
does would probably be a usability enhancement (unless they're
specified on the command line, of course).

AFAIK StGit already does that, at least if you (like me) do not have
a .stgitrc.

But I haven't gotten the impression that specifying them in ~/.stgitrc
is deprecated. The example stgitrc has a section with author name and
committer name, for example.

The only docs I know of that mentions stgitrc also states that it isn't 
required, so why use it unless you have to (or for some reason want to)? Just 
because there are many ways, doesn't mean all but one have to be deprecated.