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

[Elijah Newren <newren@gmail.com>]

Hi Dscho,

On Sat, Jun 9, 2018 at 2:45 PM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> On Thu, 7 Jun 2018, Elijah Newren wrote:
>
>> 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.

:-)

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

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