Re: [PATCH 2/2] rebase: turn on progress option by default for format-patch

From: Jeff King <peff@peff.net>
Date: May 31, 2017, 22:11 UTC
Message-ID: <20170531221151.pxdikxgxy37g7zub@sigill.intra.peff.net>
In-Reply-To: <20170531150427.7820-3-kewillf@microsoft.com>

On Wed, May 31, 2017 at 08:04:27AM -0700, Kevin Willford wrote:

> This change passes the progress option of format-patch by
> default and passes the -q --quiet option through to the
> format-patch call so that it is respected as well.

That makes sense. Is it a bug that we aren't propagating "-q" already?

I think the answer is "no", because that option only claims to silence the printing of the filenames that format-patch writes. But since we're using --stdout, it wouldn't write those anyway.

Come to think of it, I'm not sure that "-q" silencing "--progress" in your patch 1 actually makes sense. If you do:

  git format-patch -o out/

you don't need progress, because we're already writing the filenames to stdout. So it's only if you did:

  git format-patch -o out/ -q

that you'd actually want progress. But "-q" would override any --progress option! So I actually think we may be better off keeping the two as distinct features (especially if we follow the "no progress unless --progress is given" rule I mentioned in the earlier message).

Show 15 quoted lines

> diff --git a/git-rebase--am.sh b/git-rebase--am.sh
> index 375239341f..ab2be30abf 100644
> --- a/git-rebase--am.sh
> +++ b/git-rebase--am.sh
> @@ -51,8 +51,9 @@ then
>  else
>  	rm -f "$GIT_DIR/rebased-patches"
>  
> -	git format-patch -k --stdout --full-index --cherry-pick --right-only \
> -		--src-prefix=a/ --dst-prefix=b/ --no-renames --no-cover-letter \
> +	git format-patch $git_format_patch_opt -k --stdout --full-index \
> +		--cherry-pick --right-only --src-prefix=a/ --dst-prefix=b/ \
> +		--no-renames --no-cover-letter --progress \
>  		"$revisions" ${restrict_revision+^$restrict_revision} \
>  		>"$GIT_DIR/rebased-patches"

Here we pass --progress unconditionally, which tells format-patch to output progress information. Shouldn't we be checking whether stderr is a tty before making that decision? And that we're not in --quiet mode?

That explains why you want to pass "-q" in the other hunk; to countermand the explicit --progress here. But if we separate the two as I mentioned above, you'd want logic more like:

  if test -t 2 && test "$GIT_QUIET" != "t"
	git_format_patch_opt="$git_format_patch_opt --progress"
  fi

-Peff