Re: [RFC PATCH 1/3] git-rebase, sequencer: add a --quiet option for the interactive machinery

Hi Dscho,

On Sat, Jun 9, 2018 at 2:45 PM, Johannes Schindelin
[off-list ref] wrote:

On Thu, 7 Jun 2018, Elijah Newren wrote:

quoted

While 'quiet' and 'interactive' may sound like antonyms, the interactive
machinery actually has logic that implements several
interactive_rebase=implied cases (--exec, --keep-empty, --rebase-merges)
which won't pop up an editor.  Further, we want to make the interactive
machinery also take over for git-rebase--merge and become the default
merge strategy, so it makes sense for these other cases to have a quiet
option.

git-rebase--interactive was already somewhat quieter than
git-rebase--merge and git-rebase--am, possibly because cherry-pick has
just traditionally been quieter.  As such, we only drop a few
informational messages -- "Rebasing (n/m)" and "Succesfully rebased..."

Makes sense. As long as it is coordinated with Alban and Pratik, as both
of their GSoC projects are affected by this.

I had Alban cc'ed, and had looked briefly at the GSoC projects but
somehow missed that Pratik was also working in the area.  Adding him
to cc.

In particular Pratik's project, I think, would actually *benefit* from
your work, as it might even make it possible to turn all modes but
--preserve-merges into pure builtin code, which would be awesome.

:-)

quoted

@@ -713,6 +717,7 @@ Commit or stash your changes, and then run
              "$hook" rebase < "$rewritten_list"
              true # we don't care if this hook failed
      fi &&
+             test -z "$GIT_QUIET" &&
              warn "$(eval_gettext "Successfully rebased and updated \$head_name.")"

In general, I tried the statements to return success at all times. That
means that

                test -n "$GIT_QUIET" ||

would be better in this case.

Good point, I'll switch it over.

quoted

diff --git a/git-rebase.sh b/git-rebase.sh
index 7d1612b31b..b639c0d4fe 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh

@@ -136,7 +136,7 @@ write_basic_state () {
      echo "$head_name" > "$state_dir"/head-name &&
      echo "$onto" > "$state_dir"/onto &&
      echo "$orig_head" > "$state_dir"/orig-head &&
-     echo "$GIT_QUIET" > "$state_dir"/quiet &&
+     test t = "$GIT_QUIET" && : > "$state_dir"/quiet

Maybe it would be better to `echo t` into that file? That way, scripts
that used the value in that file would continue to work. (But maybe there
were no scripts that could use it, as only the interactive rebase allows
scripting, and it did not handle that flag before?)

Right, I don't think we had users before, and I'd rather make the code
a little more self-consistent.  In particular, since
$state_dir/verbose work based off the presence of the file rather than
the contents, I'd rather we just did the same for $state_dir/quiet.

However, there is one bug here; in order to make it like verbose, I
need to also make the following change:

diff --git a/git-rebase.sh b/git-rebase.sh
index c8c3d0d05a..8f0c7a4738 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh

@@ -119,7 +119,7 @@ read_basic_state () {
        else
                orig_head=$(cat "$state_dir"/head)
        fi &&
-       GIT_QUIET=$(cat "$state_dir"/quiet) &&
+       test -f "$state_dir"/quiet && GIT_QUIET=t
        test -f "$state_dir"/verbose && verbose=t
        test -f "$state_dir"/strategy && strategy="$(cat "$state_dir"/strategy)"
        test -f "$state_dir"/strategy_opts &&

The rest looks obviously good.

Thanks!
Elijah