Re: [PATCH 1/2] git-am foreign patch support: format autodetection

From: Junio C Hamano <gitster@pobox.com>
Date: May 25, 2009, 22:23 UTC
Message-ID: <7vk544u8hx.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1243278846-14120-2-git-send-email-giuseppe.bilotta@gmail.com>

Giuseppe Bilotta <giuseppe.bilotta@gmail.com> writes:

Show 9 quoted lines

> This patch is the first step towards the introduction of a framework to
> allow git-am to import patches not in mailbox format.
>
> Currently detected formats are
>   * the mailbox format itself, which is assumed by default if input is
>     form stdin
>   * Mercurial's output from 'hg export'
>   * Stacked Git's output from 'stg export' with the default export
>     template; StGIT patch series are also detected and expanded.

I personally do not trust "autodetection" (especially done by others ;-), and prefer to have an explicit override by the users, but that aside...

Show 12 quoted lines

> +	# from stdin we only accept mboxes, because peeking at stdin
> +	# to detect the format is destructive
> +	case $# in
> +	0)
> +		patch_format=mbox
> +		;;
> +	1)
> +		if test x"$1" = x"-"
> +		then
> +			# stdin, so assume mbox
> +			patch_format=mbox
> +		else

	# have parseopt to set explicit patch_format before this part...

	if test -z "$patch_format" && {
           test $# = 0 || test "x$1" = x-
        }
        then
        	patch_format=mbox
	else
        	patch_format=$(guess_patch_format)
	fi

Having this extra logic inside the main codeflow makes it extremely harder to read; have it in a separate shell function.

Show 8 quoted lines

> +# a single non-stdin argument was passed, check if it's a StGit patch series
> +# index by checking if the first line begins with '# This series'
> +			{
> +				read l1
> +				case "$l1" in
> +				'# This series '*)
> +# replace the argument list with the files listed in the series index,
> +# prefixing them with the series index dirname, skipping comment lines

Can the "series-index-name" file begin with '-' (which would affect the way how 'set "@"' works in the loop below)? A standard trick would be to do something like this.

	series_index="$1"
	shift ;# discard
        set x
        while ...
	do
        	set "$@" another
	done
        shift ;# discard 'x' protection

> +	# (which is not stdin) to try to understand the format.
> +	if test $patch_format = none

I do not understand this duplication and inconsistency. Why have the detection in two places?

Show 10 quoted lines

> +	case "$patch_format" in
> +	mbox)
> +		git mailsplit -d"$prec" -o"$dotest" -b -- "$@" > "$dotest/last" ||  {
> +			rm -fr "$dotest"
> +			exit 1
> +		}
> +		;;
> +	*)
> +		echo "Patch format $patch_format is not currently handled, sorry"
>  		exit 1

No fixing broken "Subject:" line for your format here?