git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Cc: git@vger.kernel.org, Junio C Hamano <gitster@pobox.com>,
	Phillip Wood <phillip.wood@dunelm.org.uk>
Subject: Re: [PATCH v2] rebase: remove the rebase.useBuiltin setting
Date: Thu, 14 Mar 2019 16:27:06 +0100	[thread overview]
Message-ID: <87ef79bho5.fsf@evledraar.gmail.com> (raw)
In-Reply-To: <nycvar.QRO.7.76.6.1903141544110.41@tvgsbejvaqbjf.bet>


On Thu, Mar 14 2019, Johannes Schindelin wrote:

> Hi Ævar,
>
> On Thu, 14 Mar 2019, Ævar Arnfjörð Bjarmason wrote:
>
>> Remove the rebase.useBuiltin setting, which was added as an escape
>> hatch to disable the builtin version of rebase first released with Git
>> 2.20.
>>
>> See [1] for the initial implementation of rebase.useBuiltin, and [2]
>> and [3] for the documentation and corresponding
>> GIT_TEST_REBASE_USE_BUILTIN option.
>>
>> Carrying the legacy version is a maintenance burden as seen in
>> 7e097e27d3 ("legacy-rebase: backport -C<n> and --whitespace=<option>
>> checks", 2018-11-20) and 9aea5e9286 ("rebase: fix regression in
>> rebase.useBuiltin=false test mode", 2019-02-13). Since the built-in
>> version has been shown to be stable enough let's remove the legacy
>> version.
>
> I agree with that reasoning. Elsewhere, a wish cropped up for the `git
> stash` command to optionally ignore unmatched globs, and if we go about to
> implement this, we will have to implement it in the scripted and the
> built-in version. If we can at least avoid that for the `rebase` command,
> I think it would make things a bit easier over here.
>
>> diff --git a/Documentation/config/rebase.txt b/Documentation/config/rebase.txt
>> index 331d250e04..c747452983 100644
>> --- a/Documentation/config/rebase.txt
>> +++ b/Documentation/config/rebase.txt
>> @@ -1,16 +1,9 @@
>>  rebase.useBuiltin::
>> -	Set to `false` to use the legacy shellscript implementation of
>> -	linkgit:git-rebase[1]. Is `true` by default, which means use
>> -	the built-in rewrite of it in C.
>> -+
>> -The C rewrite is first included with Git version 2.20. This option
>> -serves an an escape hatch to re-enable the legacy version in case any
>> -bugs are found in the rewrite. This option and the shellscript version
>> -of linkgit:git-rebase[1] will be removed in some future release.
>> -+
>> -If you find some reason to set this option to `false` other than
>> -one-off testing you should report the behavior difference as a bug in
>> -git.
>> +	Unused configuration variable. Used between Git version 2.20
>> +	and 2.21 as an escape hatch to enable the legacy shellscript
>> +	implementation of rebase. Now the built-in rewrite of it in C
>> +	is always used. Setting this will emit a warning, to alert any
>> +	remaining users that setting this now does nothing.
>
> Do we really need to document this? Why not just remove the entire entry
> wholesale; the warning if `rebase.useBuiltin=false` is set will be
> informative enough.

I don't have a super-strong preference in this case, but in general I
think it makes sense to have docs for this too.

Individual versions of git tend to be around for a while due to distro
packaging timelines, so e.g. if we're "lucky" a given version like 2.21
might be installed on say OSX for half a decade.

That'll mean some people probably setting this in config, and then when
they later wonder if it's needed they can Google search the config
option name or check it in git-config, less of a stretch than needing to
know to run git-rebase with the option...

>> diff --git a/builtin/rebase.c b/builtin/rebase.c
>> index 52114cbf0d..829897a8fe 100644
>> --- a/builtin/rebase.c
>> +++ b/builtin/rebase.c
>> @@ -1143,21 +1143,8 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>>  	};
>>  	int i;
>>
>> -	/*
>> -	 * NEEDSWORK: Once the builtin rebase has been tested enough
>> -	 * and git-legacy-rebase.sh is retired to contrib/, this preamble
>> -	 * can be removed.
>> -	 */
>> -
>> -	if (!use_builtin_rebase()) {
>> -		const char *path = mkpath("%s/git-legacy-rebase",
>> -					  git_exec_path());
>> -
>> -		if (sane_execvp(path, (char **)argv) < 0)
>> -			die_errno(_("could not exec %s"), path);
>> -		else
>> -			BUG("sane_execvp() returned???");
>> -	}
>> +	if (!use_builtin_rebase())
>> +		warning(_("The rebase.useBuiltin support has been removed!"));
>
> A couple of thoughts about this:
>
> - `use_builtin_rebase()` spawns a `git config`. This is a pretty expensive
>   operation on Windows (even if it might not matter in the big scheme of
>   things, as the couple of milliseconds are probably a mere drop on a hot
>   stone compared to the I/O incurred by the recursive merge), and it was
>   only done in that way to allow for spawning the legacy rebase without
>   having touched any global state (such as setting `GIT_*` environment
>   variables when a Git directory was discovered).
>
>   Couldn't we rather move this warning into `rebase_config()`?
>
> - The warning should start with a lower-case letter (why don't we have any
>   automated linter for this? This is a totally automatable thing that
>   could run as part of `make` when `DEVELOPER` is set, maybe just on the
>   `git diff HEAD --` part, and maybe even generating a patch that can be
>   applied; No human should *ever* need to spend time on such issues).

Both of these make sense. Will have that in a v3 pending further
feedback. Didn't notice that strange use_builtin_rebase()
implementation.

> - That warning should probably talk more specifically about the scripted
>   version having been removed, not only the option (which was actually not
>   removed, otherwise the user would not see that warning ;-)).

...or just change it to briefly refer to the git-config docs where all
of this will be explained :)

>> diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
>> index 3e73f7584c..0a88eed1db 100755
>> --- a/t/t3400-rebase.sh
>> +++ b/t/t3400-rebase.sh
>> @@ -311,4 +311,10 @@ test_expect_success 'rebase--merge.sh and --show-current-patch' '
>>  	)
>>  '
>>
>> +test_expect_success 'rebase -c rebase.useBuiltin=false warning' '
>> +	test_must_fail env GIT_TEST_REBASE_USE_BUILTIN= \
>
> Good attention to detail! I would have forgotten to unset that environment
> variable.
>
>> +		git -c rebase.useBuiltin=false rebase 2>err &&
>> +	test_i18ngrep "rebase.useBuiltin support has been removed" err
>> +'
>> +
>>  test_done
>> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
>> index b60b11f9f2..1723e1a858 100755
>> --- a/t/t3404-rebase-interactive.sh
>> +++ b/t/t3404-rebase-interactive.sh
>> @@ -149,12 +149,10 @@ test_expect_success 'rebase -i with the exec command checks tree cleanness' '
>>
>>  test_expect_success 'rebase -x with empty command fails' '
>>  	test_when_finished "git rebase --abort ||:" &&
>> -	test_must_fail env GIT_TEST_REBASE_USE_BUILTIN=true \
>> -		git rebase -x "" @ 2>actual &&
>> +	test_must_fail env git rebase -x "" @ 2>actual &&
>>  	test_write_lines "error: empty exec command" >expected &&
>>  	test_i18ncmp expected actual &&
>> -	test_must_fail env GIT_TEST_REBASE_USE_BUILTIN=true \
>> -		git rebase -x " " @ 2>actual &&
>> +	test_must_fail env git rebase -x " " @ 2>actual &&
>>  	test_i18ncmp expected actual
>>  '
>>
>> @@ -162,8 +160,7 @@ LF='
>>  '
>>  test_expect_success 'rebase -x with newline in command fails' '
>>  	test_when_finished "git rebase --abort ||:" &&
>> -	test_must_fail env GIT_TEST_REBASE_USE_BUILTIN=true \
>> -		git rebase -x "a${LF}b" @ 2>actual &&
>> +	test_must_fail env git rebase -x "a${LF}b" @ 2>actual &&
>
> Not a terribly big deal, but I would have structured the patch (series) by
> leaving this change to t3404 as a 2/2, as it is not technically necessary
> to include those changes in 1/2 (if your goal is, as mine usually is, to
> "go from working state to working state" between commits).

I run the test suite on various git versions, including for bisecting
purposes and with various GIT_TEST_* options on.

I'll probably never bump into *this* particular commit for this option,
but in general I think it makes more sense to not break the test suite
under existing GIT_TEST_* flags, unless it's a breakage where e.g. we
say "this isn't supported anymore".

By splitting this up as you suggest the 1/2 of those would be a head
scratching breakage under GIT_TEST_REBASE_USE_BUILTIN=false, and only
under 2/2 would we show via the warning why it was failing.

  reply	other threads:[~2019-03-14 15:27 UTC|newest]

Thread overview: 31+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-01-28 10:26 [PATCH] rebase -x: sanity check command Phillip Wood
2019-01-28 18:23 ` Junio C Hamano
2019-01-28 21:56   ` Johannes Schindelin
2019-01-29 11:40     ` Phillip Wood
2019-01-29 15:35       ` Johannes Schindelin
2019-01-28 22:03 ` Johannes Schindelin
2019-01-29 11:34   ` Phillip Wood
2019-01-29 15:32     ` Johannes Schindelin
2019-01-29 18:43       ` [PATCH v2] " Phillip Wood
2019-01-29 21:53         ` Junio C Hamano
2019-01-30 12:25         ` Johannes Schindelin
2019-02-13 13:31         ` Ævar Arnfjörð Bjarmason
2019-02-13 14:22           ` [PATCH] rebase: remove the rebase.useBuiltin setting Ævar Arnfjörð Bjarmason
2019-02-13 16:25             ` Johannes Schindelin
2019-02-13 20:46             ` Junio C Hamano
2019-02-13 21:49               ` [PATCH] rebase: fix regression in rebase.useBuiltin=false test mode Ævar Arnfjörð Bjarmason
2019-02-13 23:21                 ` Junio C Hamano
2019-02-14 16:12                 ` Phillip Wood
2019-03-14 13:24             ` [PATCH v2] rebase: remove the rebase.useBuiltin setting Ævar Arnfjörð Bjarmason
2019-03-14 14:58               ` Johannes Schindelin
2019-03-14 15:27                 ` Ævar Arnfjörð Bjarmason [this message]
2019-03-15 13:45                   ` [PATCH v3] " Ævar Arnfjörð Bjarmason
2019-03-15 15:44                     ` Johannes Schindelin
2019-03-15 16:11                       ` Ævar Arnfjörð Bjarmason
2019-03-18  6:06                         ` Junio C Hamano
2019-03-18 10:19                     ` Phillip Wood
2019-03-18 11:01                       ` [PATCH v4] " Ævar Arnfjörð Bjarmason
2019-03-19 10:21                         ` Phillip Wood
2021-03-23 15:23                         ` [PATCH] rebase: remove transitory rebase.useBuiltin setting & env Ævar Arnfjörð Bjarmason
2021-03-23 20:42                           ` Junio C Hamano
2021-03-23 20:52                           ` Johannes Schindelin

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=87ef79bho5.fsf@evledraar.gmail.com \
    --to=avarab@gmail.com \
    --cc=Johannes.Schindelin@gmx.de \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=phillip.wood@dunelm.org.uk \
    /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).