Re: [updated PATCH] Same default as cvsimport when using --use-log-author

From: Eric Wong <normalperson@yhbt.net>
Date: Apr 29, 2008, 06:18 UTC
Message-ID: <20080429061823.GE24171@muzzle>
In-Reply-To: <7vbq3vf2k4.fsf@gitster.siamese.dyndns.org>

Junio C Hamano <gitster@pobox.com> wrote:

Show 43 quoted lines

> "Stephen R. van den Berg" <srb@cuci.nl> writes:
> 
> > git-svn supports an experimental option --use-log-author which currently
> > results in:
> >
> > Author: foobaruser <unknown>
> 
> I have a question about this.  Is the "<unknown> coming from...
> 
> > This patches harmonises the result with cvsimport, and makes
> > git-svn --use-log-author produce:
> >
> > Author: foobaruser <foobaruser>
> > ...
> > diff --git a/git-svn.perl b/git-svn.perl
> > index b151049..846e739 100755
> > --- a/git-svn.perl
> > +++ b/git-svn.perl
> > @@ -2434,6 +2434,9 @@ sub make_log_entry {
> >  		} else {
> >  			($name, $email) = ($name_field, 'unknown');
> >  		}
> 
> ... this 'unknown' we see here?
> 
> > +	        if (!defined $email) {
> > +		    $email = $name;
> > +	        }
> >  	}
> 
> I would think not -- if that is the case, the codepath you added as a fix
> would not trigger.  Which means in some other cases, the 'unknown' we see
> above in the context also still happens.  Is it a good thing?  Maybe we
> would also want to make it consistently do "somebody <somebody>" instead,
> by doing...
> 
> 	} else {
> 		$name = $name_field;
> 	}
>         if (!defined $email) {
> 	    $email = $name;
>         }
>

I don't think Stephen's patch ever gets triggered, either.

This section of code was done by Andy, so I can't tell his motivations for using 'unknown' the way he did.

$email does appear to get set correctly for the first two elsifs cases here in the existing code:

		if (!defined $name_field) {
			#
		} elsif ($name_field =~ /(.*?)\s+<(.*)>/) {
			($name, $email) = ($1, $2);
		} elsif ($name_field =~ /(.*)@/) {
			($name, $email) = ($1, $name_field);
		} else {
			($name, $email) = ($name_field, $name_field);

So I propose the following one-line change instead of Stephen's:

diff --git a/git-svn.perl b/git-svn.perl
index b151049..301a5b4 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -2432,7 +2432,7 @@ sub make_log_entry {
 		} elsif ($name_field =~ /(.*)@/) {
 			($name, $email) = ($1, $name_field);
 		} else {
-			($name, $email) = ($name_field, 'unknown');
+			($name, $email) = ($name_field, $name_field);
 		}
 	}
 	if (defined $headrev && $self->use_svm_props) {

-- 
Eric Wong