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

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 19, 2012, 23:59 UTC
Message-ID: <7vsjbio2ky.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1345413170-2519-1-git-send-email-robert@debian.org>

Robert Luberda <robert@debian.org> writes:

Show 21 quoted lines

> Introduce a `--trim-svn-log' option and svn.trimsvnlog boolean
> configuration key for git svn commands.
>
> When enabled while retrieving commits from svn, git svn will strip any
> white spaces from beginnings and endings of SVN commit messages and will
> skip adding an extra new line character before `git-svn-id:' line in case
> the commit message already ends with a pseudo-headers section (a section
> starting with an empty line followed by one or more pseudo-headers like
> `From: ', `Signed-off-by: ', or `Change-Id: ').
>
> Additionally, while creating new commits in svn when the new option is
> enabled and `--add-author-from' is in effect, git svn will not add
> a new line character before the `From: ' pseudo-header if the commit
> message already ends with a pseudo-headers section.
>
> The new option allows one to use gerrit code review system on
> git-svn-managed repositories. gerrit expects its `Change-Id:' header
> to appear in the last paragraph of commit message and the standard
> behaviour of preceding `git-svn-id:' line with a new line character
> was breaking this expectation.
> ---

Sign-off?

Show 28 quoted lines

>  Documentation/git-svn.txt          |   16 ++
>  git-svn.perl                       |    8 +-
>  perl/Git/SVN.pm                    |   19 +-
>  t/t9165-git-svn-import-messages.sh |  387 ++++++++++++++++++++++++++++++++++++
>  4 files changed, 427 insertions(+), 3 deletions(-)
>  create mode 100755 t/t9165-git-svn-import-messages.sh
>
> diff --git a/Documentation/git-svn.txt b/Documentation/git-svn.txt
> index cfe8d2b..79c21ee 100644
> --- a/Documentation/git-svn.txt
> +++ b/Documentation/git-svn.txt
> @@ -603,6 +603,22 @@ creating the branch or tag.
>  	git commit's author string.  If you use this, then `--use-log-author`
>  	will retrieve a valid author string for all commits.
>  
> +--trim-svn-log::
> +	When retrieving svn commits into git (as part of 'fetch', 'rebase',
> +	or 'dcommit') trim the whitespaces from beginnings and endings
> +	of the svn log messages and avoid preceding `git-svn-id:` line with
> +	a new line character in case the log message already ends with a
> +	pseudo-headers section (i.e. section starting with an empty line
> +	followed by one or more lines like `Signed-off-by: `, `From: `,
> +	or `Change-Id: `).
> ++
> +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.

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

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

Show 29 quoted lines

> --- /dev/null
> +++ b/t/t9165-git-svn-import-messages.sh
> @@ -0,0 +1,387 @@
> +#!/bin/sh
> +#
> +# Copyright (c) 2012 Robert Luberda
> +#
> +
> +test_description='checks behaviour of --trim-svn-log option of git svn'
> +. ./lib-git-svn.sh
> +
> +# The test cases in this file check if the --trim-svn-log option
> +# of git svn works as expected.
> +#
> +# Basically the test cases use two git repositories:
> +# * work-TRIM.git, created by git svn clone with the option enabled,
> +# * work-NOTR.git, created with the option disabled.
> +#
> +# Each test case (except for the first two) does the following:
> +# 1. commit a change to svn with either svn commit, or git svn dcommit
> +#    (what is important is the commit log message, not the changed file)
> +# 2. git svn rebase the  work-NOTR.git repository and check its most recent
> +#    log message against some expected output
> +# 3. git svn rebase the work-TRIM.git repository and similarly check the
> +#    latest log message
> +#
> +# The following two prerequisites are defined mostly for debugging purposes
> +# to make it possible to skip test cases or parts of the test cases that
> +# operate on one of the two git repositories. For example to ignore checking

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.

Show 10 quoted lines

> +# of log messages imported when --trim-svn-log is enabled, one needs to comment
> +# out the PRQ_TRIM pre-requisite (this makes it possible to run the test with
> +# a version of git svn that does not support the option yet). Similarly
> +# commenting out the PRQ_NOTR pre-requisite will check only the effects of the
> +# svn log trimming option.
> +# Please note that a whole test case must be marked as requiring one of
> +# those pre-requisites if and only if it uses `git svn dcommit' for committing
> +# changes to svn.
> +test_set_prereq PRQ_TRIM
> +test_set_prereq PRQ_NOTR

> +N=0
> +NL=""	# for better readability only, e.g.:
> +	# "$NL" "   " "$NL" is cleaner than "" "   " ""

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.

Show 5 quoted lines

> +next_N()
> +{
> +	N=$((N + 1)) &&
> +	echo "N is $N"
> +}

Most shells tolerate writing a bare N inside arith substitution, but for better portability, please spell this as

	next_N () {
		N=$(($N + 1)) &&
                ...
	}

(the above also has two style fixes).

Show 8 quoted lines

> +
> +# An utility function used for writing log messages to files
> +get_file_contents()
> +{
> +	for line in "$@"; do
> +		echo "$line"
> +	done
> +}

(Style)
	get_file_contents () {
                for line
                do
                        ...
                done
	}