Re: [PATCHv3 4/4] am, rebase: teach quiet option

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 16, 2009, 05:57 UTC
Message-ID: <7vbpoog1py.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1245117905-19351-5-git-send-email-bebarino@gmail.com>

Stephen Boyd <bebarino@gmail.com> writes:

Show 7 quoted lines

> git-rebase and git-am are talkative scripts. This option will quiet
> them and allow them to speak only when they fail or experience errors.
>
> Note that git-am with 3way will output errors when applying, even though
> the 3way will usually be successfull. We suppress these errors from
> git-apply because they are not "true" errors until the 3way has been
> attempted.

Thanks.

Show 14 quoted lines

> @@ -498,11 +505,18 @@ do
> ...
>  	case "$resolved" in
>  	'')
> -		eval 'git apply '"$git_apply_opt"' --index "$dotest/patch"'
> +		if test "$threeway" = t && test -n "$GIT_QUIET"
> +		then
> +			eval 'git apply '"$git_apply_opt" \
> +			' --index "$dotest/patch" > /dev/null 2>&1'
> +		else
> +			eval 'git apply '"$git_apply_opt" \
> +			' --index "$dotest/patch"'
> +		fi
>  		apply_status=$?

Hmm, this long conditional body looks ugly, and I suspect it is harder to maintain than necessary. Can we do something about it?

	# When we are allowed to fall back to 3-way later, don't give
        # false errors during the initial attempt.
	squelch=
	if test "$threeway" = t && test -n "$GIT_QUIET"
	then
		squelch='>/dev/null 2>&1 '
	fi
        eval "git apply $squelch$git_apply_opt"' --index "$dotest/patch"'

Show 21 quoted lines

> @@ -72,11 +72,20 @@ continue_merge () {
>  			echo "directly, but instead do one of the following: "
>  			die "$RESOLVEMSG"
>  		fi
> -		printf "Committed: %0${prec}d " $msgnum
> +		if test -z "$GIT_QUIET"
> +		then
> +			printf "Committed: %0${prec}d " $msgnum
> +		fi
>  	else
> -		printf "Already applied: %0${prec}d " $msgnum
> +		if test -z "$GIT_QUIET"
> +		then
> +			printf "Already applied: %0${prec}d " $msgnum
> +		fi
> +	fi
> +	if test -z "$GIT_QUIET"
> +	then
> +		git rev-list --pretty=oneline -1 "$cmt" | sed -e 's/^[^ ]* //'
>  	fi
> -	git rev-list --pretty=oneline -1 "$cmt" | sed -e 's/^[^ ]* //'

This is very good, because a straightforward translation is what we want in this particular patch.

We however would want to update this using

	git show -s --oneline

or something, perhaps even with a custom --format='...', in a follow-up patch. This "rev-list --pretty=oneline" piped to sed looks very much antiquated.

Show 7 quoted lines

> @@ -445,15 +460,15 @@ then
>  	then
>  		# Lazily switch to the target branch if needed...
>  		test -z "$switch_to" || git checkout "$switch_to"
> -		echo >&2 "Current branch $branch_name is up to date."
> +		say >&2 "Current branch $branch_name is up to date."
>  		exit 0

Ah, I was blind.

While sending non-error messages to stderr is justifiable, I do not think this one is, because all the other progress-y message in this program we reviewed so far go to stdout. I think we should drop >&2 here.

Show 6 quoted lines

> @@ -471,7 +486,7 @@ fi
>  # we just fast forwarded.
>  if test "$mb" = "$branch"
>  then
> -	echo >&2 "Fast-forwarded $branch_name to $onto_name."
> +	say >&2 "Fast-forwarded $branch_name to $onto_name."

Ditto.

There is one more thing to think about in git-am, which I do not think you addressed. Consider this scenario.

    (1) Tell am to run quietly, feeding a four-patch series.

	$ git am -q -3 mbox

    (2) The first patch applies cleanly; the second one does not apply,
        even with -3, and leaves conflict (you did the right thing not to
        squelch the message when this happens).

    (3) You fix it up, and save the result from your editor, and tell it
        to continue.

	$ git am --resolved

Now, should the second invocation be also quiet, or talkative as default?

Note that the third and fourth patch are applied with -3 option in effect, even though you did not say -3 when you restarted "am" with --resolved (cf. ll.280-340 in git-am.sh).