Re: [PATCH/RFC] git svn: optionally trim imported log messages

From: Robert Luberda <robert@debian.org>
Date: Aug 24, 2012, 22:38 UTC
Message-ID: <5038027B.4090208@debian.org>
In-Reply-To: <7vsjbio2ky.fsf@alter.siamese.dyndns.org>

Junio C Hamano writes:

Hi,

Junio, for some reason I don't get mails from you, I've just discovered your e-mails on gmane news list. Anyway many thanks for your comments, I'll fix them and send updated patch next week.

Show 7 quoted lines

>> +When committing to svn from git (as part of 'commit-diff', 'set-tree'
>> +or 'dcommit') and '--add-author-from' is in effect, a new line character
>> +is not added before the `From: ` line if the log message ends with
>> +a pseudo-headers section.
> 
> I think it would be saner to call them "trailers" to avoid
> confusion.

Thanks, I haven't got any idea how to call them, especially because existing git documentation refers to them just by using the word `line', e.g.:

 git-am.txt:     Add a `Signed-off-by:` line to the commit message,
 git-cherry-pick.txt:    Add Signed-off-by line at the end of the

(The "trailer" keyword appears once in SubmittingPatches and - in a bit different meaning in technical/pack-format.txt)

> 
> I've seen S-o-b, Cc and Change-Id there, but does anybody actually
> put "From: " there?

Yes, `git svn dcommit --add-author-from' adds such a trailer to the svn log message, and then resets or rebases the git index against svn, so that the message with the trailer appears in git.

> 
> There needs an explanation to the reader why this is an optional
> feature.

OK, I'll add some explanation. Basically it is optional, per Eric request, for backward compatibility to make it possible to work on a centralized clone of svn repository by people using both old and new versions of git svn.

Show 7 quoted lines

> 
> The prerequisite mechanism is to allow some tests in an environment
> where they cannot be run (e.g. no SVN available on the platform);
> any code that uses set_prereq unconditionally looks extremely
> strange.  It is OK while the patch is in RFC stage under active
> debugging, but they would want to go away before the polishing is
> done.

OK, I used it merely for checking that the tests work on older version of git svn, and I didn't break the backward compatibility by accident. Will be dropped from next version of the patch.

Show 8 quoted lines

> 
> What does En-El stand for?  We often see (but not in Git where we
> prefer LF for that purpose)
> 
> 	NL='
>         ' ;# newline
> 
> and using $NL to mean "empty" may be misleading to the readers.

NL means newline. The new line characters implicitly added after each commit message line, that's why the value is empty. But, yes, this can be misleading. I'd prefer to keep it short, so would EL (i.e. `empty-line') be an acceptable name?

>> +	N=$((N + 1)) &&
>

Sorry, it was a typo, I meant to use $(($N + 1)). Thanks for catching this.

Show 7 quoted lines

> 
> 	next_N () {
> 		N=$(($N + 1)) &&
>                 ...
> 	}
> 
> (the above also has two style fixes).

Just to be sure: shall the `...' line start a new level of indentation or is it a typo? (I guess that the two style fixes are just after the function name.)

Thanks a lot, robert