Re: [PATCH 2/3] git-am: Add command line parameter `--keep-cr` passing it to git-mailsplit.

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 22, 2010, 21:10 UTC
Message-ID: <7v635p9dss.fsf@alter.siamese.dyndns.org>
In-Reply-To: <20100213171127.GB14754@scotty.home>

"Stefan-W. Hahn" <stefan.hahn@s-hahn.de> writes:

> The behaviour of git-mailsplit, which is called from git-am for
> patches in mbox format, has been changed in commit c2ca1d79. The new
> default behaviour will remove `\r` from line endings with `\r\n`.

This might offend people who caused c2ca1d7 (Allow mailsplit (and hence git-am) to handle mails with CRLF line-endings, 2009-08-04) to come into existence in the first place, as their argument was that "git am" not reading from the output from their MUA's save-as (Thunderbird I think it was but I may be mistaken) was a _bug_. I personally didn't like that bugfix very much and we could have added --strip-cr to help them back then, but that is not what happened.

Perhaps this would be a more agreeable description of the backstory?

    c2ca1d7 (Allow mailsplit (and hence git-am) to handle mails with CRLF
    line-endings, 2009-08-04) fixed "git mailsplit" to help people with
    MUA whose output from save-as command uses CRLF as line terminators by
    stripping CR at the end of lines.

    However, when you know you are feeding output from "git format-patch"
    directly to "git am", and especially when your contents have CR at the
    end of line, such stripping is undesirable.  To help such a use case,
    teach --keep-cr option to "git am" and pass that to "git mailinfo".

> This patch adds the command line parameter `--keep-cr` for git-am and
> the configuration `am.keepcr`.

If one sets am.keepcr (because he regularly runs format-patch piped to am by hand), but occasionally wants to apply an e-mailed patch out of his MUA that happens to write things out with CRLF, how would one do so, without touching the configuration (and not forgetting to revert the change after doing so)?

As a general rule, if you introduce a new configuration, you need to make sure that the configuration can be overriden per invocation if necessary, and it is usually done from the command line, i.e. "--no-keep-cr".

I have to warn you that it would be a lot more work that needs careful thinking than adding a command line option alone, so you may want to split this [PATCH 2/3] into two, one to add command line option, and the other to add configuration.

Show 12 quoted lines

> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 4c36aa9..aa452f3 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -550,6 +550,12 @@ it will be treated as a shell command.  For example, defining
>  executed from the top-level directory of a repository, which may
>  not necessarily be the current directory.
>  
> +am.keepcr::
> +	If true, git-am will call git-mailsplit for patches in mbox format 
> +	with parameter '--keep-cr'. In this case git-mailsplit will
> +	not remove `\r` from lines ending with `\r\n`.

Hence you would need something like:

    s/$/  Can be overriden by giving --no-keep-cr from the command line./

Also Documentation/git-am.txt would need something like:

--keep-cr::
--no-keep-cr::
	With --keep-cr, call git-mailsplit with the same option, to
        prevent it from stripping CR at the end of lines.  `am.keepcr`
        configuration variable can be used to specify the default
	behaviour.  --no-keep-cr is useful to override `am.keepcr`.

Show 13 quoted lines

> diff --git a/git-am.sh b/git-am.sh
> index c8b9cbb..3057a83 100755
> --- a/git-am.sh
> +++ b/git-am.sh
> @@ -347,6 +353,8 @@ do
>  		allow_rerere_autoupdate="$1" ;;
>  	-q|--quiet)
>  		GIT_QUIET=t ;;
> +	--keep-cr)
> +		keepcr=t ;;
>  	--)
>  		shift; break ;;
>  	*)

And you obviously need to have "--no-keep-cr" here...

Show 16 quoted lines

> @@ -452,6 +460,7 @@ else
>  	echo "$sign" >"$dotest/sign"
>  	echo "$utf8" >"$dotest/utf8"
>  	echo "$keep" >"$dotest/keep"
> +	echo "$keepcr" >"$dotest/keepcr"
>  	echo "$scissors" >"$dotest/scissors"
>  	echo "$no_inbody_headers" >"$dotest/no_inbody_headers"
>  	echo "$GIT_QUIET" >"$dotest/quiet"
> @@ -495,6 +504,10 @@ if test "$(cat "$dotest/keep")" = t
>  then
>  	keep=-k
>  fi
> +if test "$(cat "$dotest/keepcr")" = t
> +then
> +	keepcr=--keep-cr
> +fi

Also you may have to set keepcr to --no-keep-cr or something (I won't do the necessary thinking for you while writing this message), to deal with a case where:

 - The user has am.keepcr set to true;

 - This particular invocation was made with --no-keep-cr from the command
   line;

 - It stopped due to unappliable patch in the series and $dotest/keepcr
   became empty;

 - The user dealt with the stoppage and restarted the command; we read
   empty from $dotest/keepcr.

If I am reading your patch correctly, I think the restarted command will use keepcr=t that was set by reading from the configuration at the beginning?