Re: [PATCH v4 5/8] git-rebase, sequencer: extend --quiet option for the interactive machinery

Hi Elijah,

On Mon, 21 Jan 2019, Elijah Newren wrote:

On Mon, Jan 21, 2019 at 8:10 AM Johannes Schindelin
[off-list ref] wrote:

quoted

On Tue, 11 Dec 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.  The rewrite of interactive rebase in C
added a quiet option, though it only turns stats off.  Since we want to
make the interactive machinery also take over for git-rebase--merge, it
should fully implement the --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 "Successfully rebased..."

Also, for simplicity, remove the differences in how quiet and verbose
options were recorded.  Having one be signalled by the presence of a
"verbose" file in the state_dir, while the other was signalled by the
contents of a "quiet" file was just weirdly inconsistent.  (This
inconsistency pre-dated the rewrite into C.)  Make them consistent by
having them both key off the presence of the file.

Signed-off-by: Elijah Newren <redacted>

This is convincing. I would like to point out, though...

quoted

---
 builtin/rebase.c      |  5 +----
 git-legacy-rebase.sh  |  2 +-
 git-rebase--common.sh |  2 +-
 sequencer.c           | 23 +++++++++++++----------
 sequencer.h           |  1 +
 5 files changed, 17 insertions(+), 16 deletions(-)

diff --git a/builtin/rebase.c b/builtin/rebase.c
index 78e982298f..ec2e5fbf23 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c

@@ -185,10 +185,7 @@ static int read_basic_state(struct rebase_options *opts)
      if (get_oid(buf.buf, &opts->orig_head))
              return error(_("invalid orig-head: '%s'"), buf.buf);

-     strbuf_reset(&buf);
-     if (read_one(state_dir_path("quiet", opts), &buf))
-             return -1;
-     if (buf.len)
+     if (file_exists(state_dir_path("quiet", opts)))

This changes the behavior. AFAIR the `quiet` file was always written, but
contained `t` in quiet mode. I have to interrupt my review here, and will
continue later, but maybe you will beat me to looking into that.

Ciao,
Dscho

You are certainly consistent;

You flatter me!

you commented on this exact same issue in both v1 and v2 (you didn't
have time to review v3). In v2, your comment was[1]:

"
I am slightly concerned that some creative power user could have written
scripts that rely on this behavior.

But only *slightly* concerned.

The patch looks correct.
"

Also, I have a fuzzy memory of discussing a very similar case with
some rebase-oriented option and its on-disk representation, where the
concern was more about users upgrading git versions during an
incomplete rebase rather than power users looking at internal file
contents.  And I think either Phillip or Junio made some statement
about considering these internal details and that they felt the worry
about upgrade mid-rebase was overly worrying.  But I can't find the
emails right now, and it's been so long (at least half a year) that I
might be imagining things.

Indeed, it was one of Phillip's patch series that fixed an incorrect
quoting in the author script. And we eventually all agreed that it would
not be worth the trouble to take care of upgrade-spanning rebases (both
Phillip and I spent some time to make it so, IIRC).

But from first principles, in this case even if you're worried about
power users reaching into internal files of rebase or about users
upgrade mid-rebase, at *worst* people will get or lose a few fluffy
progress-update messages.  To me, that's an innocuous level of change
that's certainly worth the risk to allow us to get rid of the annoying
differences in implementation of handling of different options.  But,
if you strongly feel that's too big a risk, we can remove the part of
the patch making verbose and quiet be handled consistently; it's not
critical to the rest of the series.  I just thought it was a good
cleanup while I was touching the area.

I think you are correct. It is much easier in C to check whether a file
exists than to read it, parse it and determine whether its Boolean value
should be `false` or `true`.

So color me convinced that your clean-up is worth it. Maybe, just maybe,
if you submit another iteration (and only if), could you add a little
paragraph for future Dscho to understand that this clean-up was slipped
in?

Thanks,
Dscho