git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: Johannes Schindelin <Johannes.Schindelin@gmx.de>
To: Phillip Wood <phillip.wood@dunelm.org.uk>
Cc: Git Mailing List <git@vger.kernel.org>
Subject: Re: [PATCH 2/2] rebase --root -k: fix when root commit is empty
Date: Thu, 15 Mar 2018 11:28:10 +0100 (STD)	[thread overview]
Message-ID: <nycvar.QRO.7.76.6.1803151118490.20700@ZVAVAG-6OXH6DA.rhebcr.pbec.zvpebfbsg.pbz> (raw)
In-Reply-To: <20180314111127.14217-2-phillip.wood@talktalk.net>

Hi Phillip,

On Wed, 14 Mar 2018, Phillip Wood wrote:

> From: Phillip Wood <phillip.wood@dunelm.org.uk>
> 
> When the root commit was empty it was being pruned by the
> --cherry-pick option passed to rev-parse. This is because when --onto
> is omitted rebase creates an empty commit (which it later amends) for
> the new root commit.

This will have ramifications on the upcoming patches on top of my
--recreate-merges patches that introduce support for running -i --root
directly through the sequencer. But from your patch, it looks as if things
should Just Work.

FWIW the patch in question is currently here (will be rebased frequently):
https://github.com/dscho/git/commit/147e8f3c2bf5f07708d76efd9da51cbd0eb5958c

> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> index 4ea54fc1c4..3ad74fc57c 100644
> --- a/git-rebase--interactive.sh
> +++ b/git-rebase--interactive.sh
> @@ -894,7 +894,12 @@ then
>  	revisions=$upstream...$orig_head
>  	shortrevisions=$shortupstream..$shorthead
>  else
> -	revisions=$onto...$orig_head
> +	if test -n "$squash_onto"
> +	then
> +		revisions=$orig_head
> +	else
> +		revisions=$onto...$orig_head
> +	fi

Before anybody else can post the suggestion: this could be written more
compactly as

	revisions=${squash_onto:+$onto...}$orig_head

As it would not only be more compact, but also a lot less readable, I
would *also* like to point out that I prefer your version.

> diff --git a/git-rebase.sh b/git-rebase.sh
> index 40301756be..30b8eaf489 100755
> --- a/git-rebase.sh
> +++ b/git-rebase.sh
> @@ -61,6 +61,7 @@ $(gettext 'Resolve all conflicts manually, mark them as resolved with
>  You can instead skip this commit: run "git rebase --skip".
>  To abort and get back to the state before "git rebase", run "git rebase --abort".')
>  "
> +squash_onto=
>  unset onto
>  unset restrict_revision
>  cmd=

This is a bug fix. Maybe we can pull it out into its own commit? Commit
message would be something like this:

	rebase --root: stop assuming that squash_onto is unset

	When the user set the environment variable `squash_onto`, the
	`rebase` command would erroneously assume that the user passed the
	option `--root`.

> diff --git a/t/t3428-rebase-signoff.sh b/t/t3428-rebase-signoff.sh
> index 2ff7f534e3..90ca6636d5 100755
> --- a/t/t3428-rebase-signoff.sh
> +++ b/t/t3428-rebase-signoff.sh
> @@ -59,7 +59,7 @@ test_expect_success 'rebase --exec --signoff adds a sign-off line' '
>  	test_cmp expected-signed actual
>  '
>  
> -test_expect_failure 'rebase --root --signoff adds a sign-off line' '
> +test_expect_success 'rebase --root --signoff adds a sign-off line' '
>  	git commit --amend -m "first" &&
>  	git rebase --root --keep-empty --signoff &&
>  	git cat-file commit HEAD^ | sed -e "1,/^\$/d" >actual &&

Hehe... I guess this fixes that test case for a surprising reason: it is
not that all of a sudden, rebase --root respects --signoff. The reason it
works now is that an "empty" root commit is now rebased correctly.

The test case is simple enough to pass my "can a regression be debugged
easily, even by somebody else than the original author" test, so I do not
mind at all lumping the two issues (--root respects --signoff, and --root
respects empty root commits) into one test case, as that will make the
test suite run faster.

Please note that I do not feel all that strongly about my suggested
modifications to pull out changes from 1/2 and 2/2 into their own
respective commits. If you have the time, and if you feel that there is
merit to my suggestions, please do it. Otherwise, please feel free not to
do it ;-)

So: ACK from me.

Ciao,
Dscho

  reply	other threads:[~2018-03-15 10:28 UTC|newest]

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-03-14 11:11 [PATCH 1/2] rebase: support --signoff with implicit rebase Phillip Wood
2018-03-14 11:11 ` [PATCH 2/2] rebase --root -k: fix when root commit is empty Phillip Wood
2018-03-15 10:28   ` Johannes Schindelin [this message]
2018-03-15 10:59   ` Phillip Wood
2018-03-14 17:48 ` [PATCH 1/2] rebase: support --signoff with implicit rebase Junio C Hamano
2018-03-15 10:11   ` Johannes Schindelin
2018-03-15 10:18 ` Johannes Schindelin
2018-03-15 10:55   ` Phillip Wood
2018-03-16 12:36     ` Johannes Schindelin
2018-03-20 11:10 ` [PATCH v2 0/3] rebase: extend --signoff support Phillip Wood
2018-03-20 11:10   ` [PATCH v2 1/3] " Phillip Wood
2018-03-20 11:10   ` [PATCH v2 2/3] rebase -p: error out if --signoff is given Phillip Wood
2018-03-20 11:10   ` [PATCH v2 3/3] rebase --keep-empty: always use interactive rebase Phillip Wood
2018-03-20 16:08     ` Johannes Schindelin
2018-03-20 18:44       ` Phillip Wood

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=nycvar.QRO.7.76.6.1803151118490.20700@ZVAVAG-6OXH6DA.rhebcr.pbec.zvpebfbsg.pbz \
    --to=johannes.schindelin@gmx.de \
    --cc=git@vger.kernel.org \
    --cc=phillip.wood@dunelm.org.uk \
    --subject='Re: [PATCH 2/2] rebase --root -k: fix when root commit is empty' \
    /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

Code repositories for project(s) associated with this 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).