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

[Robert Luberda <robert@debian.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.


>> +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.

> 
> 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.

> 
> 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.

> 
> 	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