Re: [PATCH 2/2] git-am: add --message-id/--no-message-id

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 25, 2014, 23:34 UTC
Message-ID: <xmqqbnnusvmd.fsf@gitster.dls.corp.google.com>
In-Reply-To: <1416924056-29993-3-git-send-email-bonzini@gnu.org>

Paolo Bonzini <bonzini@gnu.org> writes:

Show 6 quoted lines

> @@ -371,13 +372,18 @@ split_patches () {
>  prec=4
>  dotest="$GIT_DIR/rebase-apply"
>  sign= utf8=t keep= keepcr= skip= interactive= resolved= rebasing= abort=
> -resolvemsg= resume= scissors= no_inbody_headers=
> +messageid= resolvemsg= resume= scissors= no_inbody_headers=

It is somewhat irritating to read a diff that adds new things to the beginning of anything (a line, or a block of lines) that lists things in no particular order, as it just adds cognitive burden.

Show 22 quoted lines

>  git_apply_opt=
>  committer_date_is_author_date=
>  ignore_date=
>  allow_rerere_autoupdate=
>  gpg_sign_opt=
>  
> +if test "$(git config --bool --get am.messageid)" = true
> +then
> +    messageid=t
> +fi
> +
>  if test "$(git config --bool --get am.keepcr)" = true
>  then
>      keepcr=t
> @@ -400,6 +406,10 @@ it will be removed. Please do not use it anymore."
>  		utf8=t ;; # this is now default
>  	--no-utf8)
>  		utf8= ;;
> +	-m|--message-id)
> +		messageid=t ;;
> +	--no-message-id)
> +		messageid=f ;;

This one, taken together with these two hunks ...

Show 21 quoted lines

>  	-k|--keep)
>  		keep=t ;;
>  	--keep-non-patch)
> @@ -567,6 +577,7 @@ Use \"git am --abort\" to remove it.")"
>  	echo "$sign" >"$dotest/sign"
>  	echo "$utf8" >"$dotest/utf8"
>  	echo "$keep" >"$dotest/keep"
> +	echo "$messageid" >"$dotest/messageid"
>  	echo "$scissors" >"$dotest/scissors"
>  	echo "$no_inbody_headers" >"$dotest/no_inbody_headers"
>  	echo "$GIT_QUIET" >"$dotest/quiet"
> @@ -621,6 +632,12 @@ b)
>  *)
>  	keep= ;;
>  esac
> +case "$(cat "$dotest/messageid")" in
> +t)
> +	messageid=-m ;;
> +f)
> +	messageid= ;;
> +esac

... makes the result look questionable. The variable is initialized to empty; when it is written out to $dotest/messageid and later read back here, that empty value is not covered by this case statement.

Perhaps clearing messageid= upon seeing "--no-message-id" and using "'t' or empty" makes the code a bit easier to follow? I dunno.