Re: [PATCH v2 1/2] am: learn passing -b to mailinfo

[Thomas Rast <trast@student.ethz.ch>]

Junio C Hamano <gitster@pobox.com> writes:

> After re-reading the code that parses the command line options given to
> "am" and the previous invocation state we read from $dotest/*, however, I
> think the way this change uses $keep makes things somewhat inconsistent
> and harder to follow.
>
> Currently the variables are given abstract meaning (e.g. "are we told to
> record to utf8? yes or no") when we parse our command line options and
> read from the previous invocation state, and then based on that abstract
> meaning, a later code decides what exact option we throw at the git
> commands we invoke (e.g. "utf8=t" -> "-u").
>
> How about doing something like this instead at least for now?  It might be
> better to decide when we parse our options and $dotest/* immediately what
> options we give to the git commands we run (which your patch does but only
> to $keep option), but that kind of change (1) belongs to a separate topic
> and should be done consistently to all options, and (2) I am not convinced
> if it is necessarily a good change.

Yes, at second glance it's probably better to remain consistent.  I
didn't like it at first because it's layering complexity on it, but you
are right, the existing code follows the same pattern.

> diff --git a/git-am.sh b/git-am.sh
> index 6cdd591..8b755d9 100755
> --- a/git-am.sh
> +++ b/git-am.sh
> @@ -15,6 +15,7 @@ q,quiet         be quiet
>  s,signoff       add a Signed-off-by line to the commit message
>  u,utf8          recode into utf8 (default)
>  k,keep          pass -k flag to git-mailinfo
> +keep-non-patch  pass -b flag to git-mailinfo
>  keep-cr         pass --keep-cr flag to git-mailsplit for mbox format
>  no-keep-cr      do not pass --keep-cr flag to git-mailsplit independent of am.keepcr
>  c,scissors      strip everything before a scissors line
> @@ -345,6 +346,8 @@ do
>  		utf8= ;;
>  	-k|--keep)
>  		keep=t ;;
> +	--keep-non-patch)
> +		keep=b ;;
>  	-c|--scissors)
>  		scissors=t ;;
>  	--no-scissors)
> @@ -522,16 +525,25 @@ case "$resolved" in
>  	fi
>  esac
>  
> +# Now, decide what command line options we will give to the git
> +# commands we invoke, based on the result of parsing command line
> +# options and previous invocation state stored in $dotest/ files.
> +
>  if test "$(cat "$dotest/utf8")" = t
>  then
>  	utf8=-u
>  else
>  	utf8=-n
>  fi
> -if test "$(cat "$dotest/keep")" = t
> -then
> -	keep=-k
> -fi
> +keep=$(cat "$dotest/keep")
> +case "$keep" in
> +t)
> +	keep=-k ;;
> +b)
> +	keep=-b ;;
> +*)
> +	keep= ;;
> +esac
>  case "$(cat "$dotest/keepcr")" in
>  t)
>  	keepcr=--keep-cr ;;

-- 
Thomas Rast
trast@{inf,student}.ethz.ch