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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



"Stefan-W. Hahn" <stefan.hahn@xxxxxxxxx> 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.

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

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

> @@ -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?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]