[prev in list] [next in list] [prev in thread] [next in thread] 

List:       git
Subject:    Re: [PATCH 2/2] rebase: turn on progress option by default for format-patch
From:       Jeff King <peff () peff ! net>
Date:       2017-05-31 22:11:51
Message-ID: 20170531221151.pxdikxgxy37g7zub () sigill ! intra ! peff ! net
[Download RAW message or body]

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).

> 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
[prev in list] [next in list] [prev in thread] [next in thread]