Hi Dscho,
On Sat, Jun 9, 2018 at 2:45 PM, Johannes Schindelin
<Johannes.Schindelin@xxxxxx> 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
