git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Elijah Newren <newren@gmail.com>
To: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Cc: Git Mailing List <git@vger.kernel.org>,
	Junio C Hamano <gitster@pobox.com>,
	Alban Gruin <alban.gruin@gmail.com>
Subject: Re: [RFC PATCH 2/3] rebase: Implement --merge via git-rebase--interactive
Date: Sat, 9 Jun 2018 18:14:48 -0700	[thread overview]
Message-ID: <CABPp-BG6WFCBm0u+=Jxi2PGYgKHPifEcxpSkrPBoM39-49dKDw@mail.gmail.com> (raw)
In-Reply-To: <nycvar.QRO.7.76.6.1806092348390.77@tvgsbejvaqbjf.bet>

Hi Dscho,

On Sat, Jun 9, 2018 at 3:04 PM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> On Thu, 7 Jun 2018, Elijah Newren wrote:
>
>> diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
>> index 451252c173..28d1658d7a 100644
>> --- a/Documentation/git-rebase.txt
>> +++ b/Documentation/git-rebase.txt
>> @@ -275,6 +275,10 @@ branch on top of the <upstream> branch.  Because of this, when a merge
>>  conflict happens, the side reported as 'ours' is the so-far rebased
>>  series, starting with <upstream>, and 'theirs' is the working branch.  In
>>  other words, the sides are swapped.
>> ++
>> +This uses the `--interactive` machinery internally, and as such,
>> +anything that is incompatible with --interactive is incompatible
>> +with this option.
>
> I am not sure I like this change, as it describes an implementation detail
> that users should neither know, nor care about, nor rely on.

Let me back up for just a second to see if I can point out the real
problem I'm trying to address here, which you may have a better
solution for.  What should happen if a user runs
   git rebase --merge --whitespace=fix
?

git currently silently ignores the --whitepsace=fix argument, leaving
the whitespace damage present at the end of the rebase.  Same goes for
--interactive combined with any am-specific options  (Fix proposed at
https://public-inbox.org/git/20180607050654.19663-2-newren@gmail.com/).
This silent ignoring of some options depending on which other options
were specified has caused me problems in the past.

So, while I totally agree with you that users shouldn't need to know
implementation details, they do need to know how to use commands and
which options go well together and which are mutually incompatible.
Do you have any suggestions on alternate wording that would convey the
sets of mutually incompatible options without talking about
implementation details?  Would you rather only address that in the
code and not mention it in the documentation?

See also https://public-inbox.org/git/20180607050654.19663-1-newren@gmail.com/,
which proposes testcases for these incompatibility sets.


>> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
>> index 1f2401f702..dcc4a26a78 100644
>> --- a/git-rebase--interactive.sh
>> +++ b/git-rebase--interactive.sh
>> @@ -885,7 +885,10 @@ init_basic_state () {
>>       mkdir -p "$state_dir" || die "$(eval_gettext "Could not create temporary \$state_dir")"
>>       rm -f "$(git rev-parse --git-path REBASE_HEAD)"
>>
>> -     : > "$state_dir"/interactive || die "$(gettext "Could not mark as interactive")"
>> +     if test -n "$actually_interactive"
>> +     then
>> +             : > "$state_dir"/interactive || die "$(gettext "Could not mark as interactive")"
>> +     fi
>
> Do we really care at this stage? IOW what breaks if we still write that
> file, even in --merge mode?

Two things will change if we do that:
  * bash prompt will be different for those using git-prompt.sh
(REBASE-m vs. REBASE-i).
  * git-status output is no longer the same ('rebase in progress' vs.
'interactive rebase in progress...last command done:...pick 0dea123 my
wonderful commit')

I don't think the prompt is a big deal.  The status output might not
be either, but showing the "last command done" may be weird to someone
that never saw or edited a list of commands.  (Then again, that same
argument could be made for users of --exec, --rebase-merges, or
--keep-empty without an explicit --interactive)

Opinions on whether these two difference matter?  If others don't
think these differences are significant, I'm happy to update any
necessary testcases and just unconditionally create the
$state_dir/interactive file.


>> @@ -501,17 +502,11 @@ fi
>>
>>  if test -n "$git_am_opt"; then
>>       incompatible_opts=`echo "$git_am_opt" | sed -e 's/ -q//'`
>> -     if test -n "$interactive_rebase"
>> +     if test -n "$incompatible_opts"
>
> Did you not mean to turn this into a test for actually_interactve ||
> do_merge?
>
>>       then
>> -             if test -n "$incompatible_opts"
>> +             if test -n "$actually_interactive" || test "$do_merge"
>
> This could now be combined with the previous if (and the `-n` could be
> added to the latter test):
>
>         if test -n "$actually_interactive" -o -n "$do_merge" &&
>                 test -n "$incompatible_opts"
>
> The indentation would change, but this hunk is already confusing to read,
> anyway, so...

I'm happy to switch the order of the nesting as you suggest and agree
that it would make it easier to read.  However, I hesitate to combine
the two if-lines.  When I read your combined suggested line, I parsed
it as follows (using invalid pseduo-syntax just to convey grouping):

  ( -n "$actually_interactive ) || ( -n "$do_merge" && -n "$incompatible_opts" )

Granted, I parsed it wrong, putting the parentheses in the wrong
places, and bash parses it as you intended.  While you may have
precedence and left- vs. right- associativity rules down pat, I
clearly didn't.  If we combine the lines, I'll probably mis-read them
again when I see them in a year or more.

>> @@ -704,6 +699,22 @@ then
>>       GIT_PAGER='' git diff --stat --summary "$mb" "$onto"
>>  fi
>>
>> +if test -z "$actually_interactive"
>> +then
>> +     # If the $onto is a proper descendant of the tip of the branch, then
>> +     # we just fast-forwarded.
>> +     if test "$mb" = "$orig_head"
>> +     then
>
> Likewise, this would be easier to read as
>
>         if test -z "$actually_interactive" && test "$mb" = "$orig_head"

Good point, I'll fix that up.

> Also: how certain are we that "$mb" does not start with a dash? We may
> have to use the `test a"$mb" = a"$orig_head"` trick here... But I guess
> this is moved code, so if it is buggy, it was buggy before.

From earlier in the file,
mb=$(git merge-base ...)

So, unless we're expecting the output of git-merge-base to change in
the future to include leading dashes, we shouldn't hit any issues.  I
can make the change you suggest if you're worried, though.

>> diff --git a/t/t3406-rebase-message.sh b/t/t3406-rebase-message.sh
>> index 0392e36d23..04d6c71899 100755
>> --- a/t/t3406-rebase-message.sh
>> +++ b/t/t3406-rebase-message.sh
>> @@ -17,14 +17,9 @@ test_expect_success 'setup' '
>>       git tag start
>>  '
>>
>> -cat >expect <<\EOF
>> -Already applied: 0001 A
>> -Already applied: 0002 B
>> -Committed: 0003 Z
>> -EOF
>> -
>>  test_expect_success 'rebase -m' '
>>       git rebase -m master >report &&
>> +     >expect &&
>>       sed -n -e "/^Already applied: /p" \
>>               -e "/^Committed: /p" report >actual &&
>>       test_cmp expect actual
>
> This might be called a regression... I don't know any user of `git rebase
> -m`, but I guess if I was one, I would like to keep seeing those messages.
>
> It *should* be relatively easy to tell the sequencer.c to issue these
> messages, I think. But it would be more work.

You may well be right.  Here's where my thinking came from...

am-based, interactive-based, and merge-based rebases have lots of
little ways in which they have differed, this being just one of them.
It was sometimes hard making a judgement call when writing this series
about whether any given deviation was a difference I wanted to smooth
over or a difference I wanted to perpetuate between various flags.
Further, if it was a difference I wanted to smooth over, then I had to
decide which of the current behaviors was 'correct'.

In this particular case, I basically went off perceived usage.
am-based rebases have lots of special flags relevant to it only
(--whitespace, -C, etc.) and is the default, so it clearly has lots of
users.  interactive-based rebases are pretty prominent too, and have
very specific special capabilities the other modes don't.  In
contrast, merge-based rebases can't do a single thing that interactive
can't; and unless you're using a special merge strategy, for the last
10 years merge-based rebases haven't been able to do anything a normal
am-based rebase couldn't.  merge-based rebases were added in mid-2006
to handle renames, but am-based rebases gained that ability at the end
of 2008.  Basically, rebase -m was dormant and useless...until
directory rename detection became a thing this cycle.  (Also, in
config options and documentation merge tends to be overlooked; just a
single example is that pull.rebase can be set to interactive, but not
to merge.)

Anyway, with this in mind, I didn't think those extra messages were
all that important.  If others disagree I can look into adding them --
that'd also make the --quiet mode more useful for interactive, since
there'd be more messages for it to strip out.

>>  test_expect_success "rebase -p is no-op in non-linear history" "
>> diff --git a/t/t5407-post-rewrite-hook.sh b/t/t5407-post-rewrite-hook.sh
>> index 9b2a274c71..145c61251d 100755
>> --- a/t/t5407-post-rewrite-hook.sh
>> +++ b/t/t5407-post-rewrite-hook.sh
>> @@ -120,6 +120,7 @@ test_expect_success 'git rebase -m --skip' '
>>       git rebase --continue &&
>>       echo rebase >expected.args &&
>>       cat >expected.data <<-EOF &&
>> +     $(git rev-parse C) $(git rev-parse HEAD^)
>>       $(git rev-parse D) $(git rev-parse HEAD)
>>       EOF
>>       verify_hook_input
>
> This is a bit sad, because it seems to suggest that we now do more
> unnecessary work here, or is my reading incorrect?

I agree that it's a bit sad.  I spent a while looking at this testcase
and puzzling over what it meant, and my commit message pointed out
that I wasn't quite sure where it came from:

      t5407: different rebase types varied slightly in how many times checkout
             or commit or equivalents were called based on a quick comparison
             of this tests and previous ones which covered different rebase
             flavors.  I think this is just attributable to this difference.

It would be nice to avoid the extra work, but I'm worried tackling
that might cause me to step on toes of folks doing the rewrite of
interactive rebases from shell to C.  Maybe I should just add a TODO
note in the testcase, similar to the one in t3425 near a few lines I
touched in this patch?


Thanks for the detailed feedback and suggestions!

Elijah

  reply	other threads:[~2018-06-10  1:15 UTC|newest]

Thread overview: 130+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-06-07  4:58 RFC: rebase inconsistency in 2.18 -- course of action? Elijah Newren
2018-06-07  5:04 ` [PATCH] t3401: add directory rename testcases for rebase and am Elijah Newren
2018-06-25 16:17   ` Elijah Newren
2018-06-07  5:05 ` [PATCH] apply: fix grammar error in comment Elijah Newren
2018-06-07  5:05 ` [PATCH] t5407: fix test to cover intended arguments Elijah Newren
2018-06-07  5:06 ` [PATCH] git-rebase--merge: modernize "git-$cmd" to "git $cmd" Elijah Newren
2018-06-07  5:24   ` Elijah Newren
2018-06-27  7:46   ` [PATCH v2] " Elijah Newren
2018-07-12 15:49     ` Johannes Schindelin
2018-07-13 16:13       ` Elijah Newren
2018-07-14 22:20         ` Johannes Schindelin
2018-06-07  5:06 ` [PATCH 1/2] t3422: new testcases for checking when incompatible options passed Elijah Newren
2018-06-07  5:06   ` [PATCH 2/2] git-rebase: error out " Elijah Newren
2018-06-10 19:40     ` Phillip Wood
2018-06-11 15:19       ` Elijah Newren
2018-06-11 15:59         ` Phillip Wood
2018-06-11 15:49       ` Elijah Newren
2018-06-12  9:45         ` Phillip Wood
2018-06-17  5:58   ` [RFC PATCH v2 0/7] Document/fix/warn about rebase incompatibilities and inconsistences Elijah Newren
2018-06-17  5:58     ` [RFC PATCH v2 1/7] git-rebase.txt: document incompatible options Elijah Newren
2018-06-17 15:38       ` Phillip Wood
2018-06-20 17:09         ` Elijah Newren
2018-06-17 17:15       ` Eric Sunshine
2018-06-20 17:10         ` Elijah Newren
2018-06-17  5:58     ` [RFC PATCH v2 2/7] git-rebase.sh: update help messages a bit Elijah Newren
2018-06-17  5:58     ` [RFC PATCH v2 3/7] t3422: new testcases for checking when incompatible options passed Elijah Newren
2018-06-17  5:58     ` [RFC PATCH v2 4/7] git-rebase: error out " Elijah Newren
2018-06-17  5:58     ` [RFC PATCH v2 5/7] git-rebase.txt: document behavioral inconsistencies between modes Elijah Newren
2018-06-17  5:58     ` [RFC PATCH v2 6/7] git-rebase.txt: address confusion between --no-ff vs --force-rebase Elijah Newren
2018-06-17  5:58     ` [RFC PATCH v2 7/7] git-rebase: make --allow-empty-message the default Elijah Newren
2018-06-17 15:30       ` Phillip Wood
2018-06-20 16:47         ` Elijah Newren
2018-06-17 15:41     ` [RFC PATCH v2 0/7] Document/fix/warn about rebase incompatibilities and inconsistences Phillip Wood
2018-06-21 15:00     ` [PATCH v3 0/7] Document/fix/warn about rebase incompatibilities and inconsistencies Elijah Newren
2018-06-21 15:00       ` [PATCH v3 1/7] git-rebase.txt: document incompatible options Elijah Newren
2018-06-21 19:47         ` Junio C Hamano
2018-06-21 20:33           ` Elijah Newren
2018-06-22  8:32         ` SZEDER Gábor
2018-06-21 15:00       ` [PATCH v3 2/7] git-rebase.sh: update help messages a bit Elijah Newren
2018-06-21 19:58         ` Junio C Hamano
2018-06-21 20:34           ` Elijah Newren
2018-06-21 15:00       ` [PATCH v3 3/7] t3422: new testcases for checking when incompatible options passed Elijah Newren
2018-06-21 20:04         ` Junio C Hamano
2018-06-21 20:37           ` Elijah Newren
2018-06-21 21:18           ` Eric Sunshine
2018-06-21 15:00       ` [PATCH v3 4/7] git-rebase: error out " Elijah Newren
2018-06-21 20:29         ` Junio C Hamano
2018-06-21 20:41           ` Elijah Newren
2018-06-21 15:00       ` [PATCH v3 5/7] git-rebase.txt: document behavioral inconsistencies between modes Elijah Newren
2018-06-21 20:44         ` Junio C Hamano
2018-06-21 21:25           ` Elijah Newren
2018-06-21 15:00       ` [PATCH v3 6/7] git-rebase.txt: address confusion between --no-ff vs --force-rebase Elijah Newren
2018-06-21 20:46         ` Junio C Hamano
2018-06-21 21:22           ` Elijah Newren
2018-06-21 15:00       ` [RFC PATCH v3 7/7] git-rebase: make --allow-empty-message the default Elijah Newren
2018-06-25 16:12       ` [PATCH v4 0/9] Document/fix/warn about rebase incompatibilities and inconsistencies Elijah Newren
2018-06-25 16:12         ` [PATCH v4 1/9] git-rebase.txt: document incompatible options Elijah Newren
2018-06-25 16:12         ` [PATCH v4 2/9] git-rebase.sh: update help messages a bit Elijah Newren
2018-06-25 16:12         ` [PATCH v4 3/9] t3422: new testcases for checking when incompatible options passed Elijah Newren
2018-06-26 18:17           ` Junio C Hamano
2018-06-27  6:50             ` Elijah Newren
2018-06-25 16:12         ` [PATCH v4 4/9] git-rebase: error out " Elijah Newren
2018-06-25 16:12         ` [PATCH v4 5/9] git-rebase.txt: address confusion between --no-ff vs --force-rebase Elijah Newren
2018-06-25 16:12         ` [PATCH v4 6/9] directory-rename-detection.txt: technical docs on abilities and limitations Elijah Newren
2018-06-25 16:12         ` [PATCH v4 7/9] git-rebase.txt: document behavioral differences between modes Elijah Newren
2018-06-25 16:12         ` [PATCH v4 8/9] t3401: add directory rename testcases for rebase and am Elijah Newren
2018-06-25 16:13         ` [RFC PATCH v4 9/9] git-rebase: make --allow-empty-message the default Elijah Newren
2018-06-27  7:23         ` [PATCH v5 0/9] Document/fix/warn about rebase incompatibilities and inconsistencies Elijah Newren
2018-06-27  7:23           ` [PATCH v5 1/9] git-rebase.txt: document incompatible options Elijah Newren
2018-06-27  7:23           ` [PATCH v5 2/9] git-rebase.sh: update help messages a bit Elijah Newren
2018-06-27  7:23           ` [PATCH v5 3/9] t3422: new testcases for checking when incompatible options passed Elijah Newren
2018-06-27  7:23           ` [PATCH v5 4/9] git-rebase: error out " Elijah Newren
2018-06-27  7:23           ` [PATCH v5 5/9] git-rebase.txt: address confusion between --no-ff vs --force-rebase Elijah Newren
2018-06-27  7:23           ` [PATCH v5 6/9] directory-rename-detection.txt: technical docs on abilities and limitations Elijah Newren
2018-06-27  7:23           ` [PATCH v5 7/9] git-rebase.txt: document behavioral differences between modes Elijah Newren
2018-06-27  7:23           ` [PATCH v5 8/9] t3401: add directory rename testcases for rebase and am Elijah Newren
2018-06-27  7:23           ` [RFC PATCH v5 9/9] git-rebase: make --allow-empty-message the default Elijah Newren
2018-09-12  2:42             ` 2.19.0 regression: leaving editor with empty commit message doesn't stop rebase [was: Re: [RFC PATCH v5 9/9] git-rebase: make --allow-empty-message the default] SZEDER Gábor
2018-09-12  6:21               ` Elijah Newren
2018-09-12 21:18               ` [PATCH] sequencer: fix --allow-empty-message behavior, make it smarter Elijah Newren
2018-09-13 20:24                 ` Junio C Hamano
2018-06-29 13:20           ` [PATCH v5 0/9] Document/fix/warn about rebase incompatibilities and inconsistencies Phillip Wood
2018-06-07  5:07 ` [PATCH] git-rebase.sh: handle keep-empty like all other options Elijah Newren
2018-06-10 19:26   ` Phillip Wood
2018-06-11 14:42     ` Elijah Newren
2018-06-11 15:16       ` Phillip Wood
2018-06-11 16:08         ` Elijah Newren
2018-06-12  9:53           ` Phillip Wood
2018-06-07  5:08 ` [PATCH 1/2] t3418: add testcase showing problems with rebase -i and strategy options Elijah Newren
2018-06-07  5:08   ` [PATCH 2/2] Fix use of strategy options with interactive rebases Elijah Newren
2018-06-27  7:36   ` [PATCH v2 0/2] " Elijah Newren
2018-06-27  7:36     ` [PATCH v2 1/2] t3418: add testcase showing problems with rebase -i and strategy options Elijah Newren
2018-06-27  7:45       ` Eric Sunshine
2018-06-27  7:49         ` Elijah Newren
2018-06-27 16:54           ` Junio C Hamano
2018-06-27 17:27             ` Elijah Newren
2018-06-27 18:17               ` Johannes Sixt
2018-06-27 18:24                 ` Eric Sunshine
2018-06-27 18:34                 ` [PATCH v2 1/2] t3418: add testcase showing problems with rebase SZEDER Gábor
2018-06-27 19:20                 ` [PATCH v2 1/2] t3418: add testcase showing problems with rebase -i and strategy options Junio C Hamano
2018-06-27 19:23               ` Junio C Hamano
2018-06-27  7:36     ` [PATCH v2 2/2] Fix use of strategy options with interactive rebases Elijah Newren
2018-06-27 15:48     ` [PATCH v3 0/2] " Elijah Newren
2018-06-27 15:48       ` [PATCH v3 1/2] t3418: add testcase showing problems with rebase -i and strategy options Elijah Newren
2018-06-27 18:28         ` SZEDER Gábor
2018-07-11 10:56           ` SZEDER Gábor
2018-07-11 19:12             ` Elijah Newren
2018-06-27 15:48       ` [PATCH v3 2/2] Fix use of strategy options with interactive rebases Elijah Newren
2018-07-12 15:41         ` Johannes Schindelin
2018-07-13 15:51           ` Elijah Newren
2018-06-07 17:13 ` [RFC PATCH 0/3] Send more rebases through interactive machinery Elijah Newren
2018-06-07 17:13   ` [RFC PATCH 1/3] git-rebase, sequencer: add a --quiet option for the " Elijah Newren
2018-06-09 21:45     ` Johannes Schindelin
2018-06-09 23:05       ` Elijah Newren
2018-06-07 17:13   ` [RFC PATCH 2/3] rebase: Implement --merge via git-rebase--interactive Elijah Newren
2018-06-09 22:04     ` Johannes Schindelin
2018-06-10  1:14       ` Elijah Newren [this message]
2018-06-11  9:54         ` Phillip Wood
2018-06-07 17:13   ` [RFC PATCH 3/3] git-rebase.sh: make git-rebase--interactive the default Elijah Newren
2018-06-09 22:11     ` Johannes Schindelin
2018-06-10  1:31       ` Elijah Newren
2018-06-17 21:44         ` Johannes Schindelin
2018-06-20 16:27           ` Elijah Newren
2018-06-21 10:57             ` Johannes Schindelin
2018-06-21 15:36               ` Elijah Newren
2019-01-21 15:55                 ` Johannes Schindelin
2019-01-21 19:02                   ` Elijah Newren
2019-01-22 15:37                     ` Johannes Schindelin
2018-06-11 17:44 ` RFC: rebase inconsistency in 2.18 -- course of action? Junio C Hamano
2018-06-11 20:17   ` Elijah Newren

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CABPp-BG6WFCBm0u+=Jxi2PGYgKHPifEcxpSkrPBoM39-49dKDw@mail.gmail.com' \
    --to=newren@gmail.com \
    --cc=Johannes.Schindelin@gmx.de \
    --cc=alban.gruin@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).