Skip to content

Conversation

dscho
Copy link
Member

@dscho dscho commented Aug 25, 2018

As nice as the speed-ups are, the patches in question are still in flux, and they are not battle-tested at all.

Let's use the scripted commands by default, and offer the new, fast, experimental builtins as options.

This gives us the best of both worlds: users who want the raw speed improvement we got through three Google Summer of Code projects working in parallel can have that, while others who are reluctant to play guinea pig by running only well-tested code can stay on the safe side.

Plus, it gives us a chance of battle-testing those patches :-)

Note: the code added in this PR will only kick in if bundling a Git built including the latest patches of git-for-windows/git#1800.

Copy link
Contributor

@jamill jamill left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the idea of exposing these options to people who might be interested in trying them out. I haven't run the code, but the code changes look reasonable to me (on inspection).

My concern is that we are adding more options to a setup that already exposes a lot of options to users. If I wasn't already familiar with the options, I am not sure how I would make an informed decision about what option I should choose based on the information presented in this dialog.

@dscho
Copy link
Member Author

dscho commented Aug 27, 2018

My concern is that we are adding more options to a setup that already exposes a lot of options to users. If I wasn't already familiar with the options, I am not sure how I would make an informed decision about what option I should choose based on the information presented in this dialog.

Any suggestions how to word it?

These are opt-ins, for now.

This gives us the best of both worlds: users who want the raw speed
improvement we got through three Google Summer of Code projects working
in parallel can have that, while others who are reluctant to play guinea
pig by running only well-tested code can stay on the safe side.

Signed-off-by: Johannes Schindelin <[email protected]>
@dscho dscho force-pushed the offer-builtin-rebase-and-stash branch from 57c6dd6 to 263f643 Compare August 28, 2018 14:37
@dscho
Copy link
Member Author

dscho commented Aug 28, 2018

Screenshot:

image

@jamill jamill merged commit 3dfa130 into git-for-windows:master Aug 28, 2018
dscho added a commit that referenced this pull request Aug 28, 2018
There are now *fast*, built-in versions of `git
stash` and `git rebase`, [available as experimental
options](#203).

Signed-off-by: Johannes Schindelin <[email protected]>
@dscho dscho deleted the offer-builtin-rebase-and-stash branch August 28, 2018 21:35
@bviktor
Copy link

bviktor commented Sep 13, 2018

Great work. How do we enable this via command line?

@bviktor
Copy link

bviktor commented Sep 13, 2018

Nevermind :)

git config --global rebase.useBuiltin true
git config --global stash.useBuiltin true

@Qartar
Copy link

Qartar commented Sep 14, 2018

Is it too late to change the wording on the setup page? "90% faster" is incredibly ambiguous.

@shiftkey
Copy link
Contributor

@Qartar the source for the wording is here if you'd like to submit some better wording:

#ifdef WITH_EXPERIMENTAL_BUILTIN_REBASE
// 2nd option
RdbExperimentalOptions[GP_BuiltinRebase]:=CreateCheckBox(ExperimentalOptionsPage,'Enable experimental, built-in rebase','<RED>(NEW!)</RED> Use the experimental built-in rebase (about 70% faster, but only'+#13+'lightly tested).',TabOrder,Top,Left);
// Restore the settings chosen during a previous install
RdbExperimentalOptions[GP_BuiltinRebase].Checked:=ReplayChoice('Enable Builtin Rebase','Auto')='Disabled';
#endif
#ifdef WITH_EXPERIMENTAL_BUILTIN_STASH
// 3rd option
RdbExperimentalOptions[GP_BuiltinStash]:=CreateCheckBox(ExperimentalOptionsPage,'Enable experimental, built-in stash','<RED>(NEW!)</RED> Use the experimental built-in stash (about 90% faster, but only'+#13+'lightly tested).',TabOrder,Top,Left);
// Restore the settings chosen during a previous install
RdbExperimentalOptions[GP_BuiltinStash].Checked:=ReplayChoice('Enable Builtin Stash','Auto')='Disabled';
#endif

@Qartar
Copy link

Qartar commented Sep 14, 2018

Thanks, but I'm not sure what the intended value is, e.g. is it almost twice as fast or is it ten times faster?

@dscho
Copy link
Member Author

dscho commented Sep 27, 2018

@Qartar I guess you're right. Have you tested the performance? What I meant was a 10x speed-up. But now that you mention it, I would like to have a bit more confirmation on those numbers from other people.

@randomascii
Copy link

Uggh. Is it too late to change the "90% faster" text to say "10x as fast"?

90% faster is hopelessly ambiguous, and it hugely undersells the value of the optimizations if they really are supposed to be 10x as fast.

@PhilipOakley
Copy link
Contributor

Is it too late to change the "90% faster" text to say "10x as fast"?

No. Pull requests welcome. (though it won't show until the next build... but it will still be valuable)

@palver123
Copy link

I agree with @jamill , the installer is already a bit jam-packed with technical decisions. Even as a software developer I am somewhat daunted when I see it.

I am frequently asked by non-IT colleagues to help with the installation of Git on their (Windows) computer because they are afraid of this installer and the questions asked in it.

@coachmullen22
Copy link

My concern is that we are adding more options to a setup that already exposes a lot of options to users. If I wasn't already familiar with the options, I am not sure how I would make an informed decision about what option I should choose based on the information presented in this dialog.

Any suggestions how to word it?

I am someone who doesn't know which "bleeding edge features" I would like to enable. It's frustrating to slog through setting up my computer so I can learn to code, only to come across two options that may not mean much to me yet...but I still want to make a semi-informed choice. What do they do differently from one another except for running 70% faster or 90% faster than a completely undefined processing speed?

I think I prefer the word 'stash' over 'rebase'. Besides it's (1.9x-1.7x) faster than just x.

Seriously though, I know you're all working on a level I can only aspire to reach someday. Just keep us newbies in mind...

@Rutix
Copy link

Rutix commented Nov 22, 2018

@coachmullen22 'stash' is a git feature though (https://git-scm.com/docs/git-stash) so 'stash' and 'rebase' are different things. So you don't have to choose between them, but it's that both of them received an experimental option.

@dscho
Copy link
Member Author

dscho commented Nov 22, 2018

Well, the entire discussion is moot now, as v2.19.2 no longer marks those built-in commands as experimental.

@Sarkie
Copy link

Sarkie commented Dec 17, 2018

@dscho Just to be clear

git config --global rebase.useBuiltin true
git config --global stash.useBuiltin true

Are enabled by default as of 2.19.2, so not needed to do manually run the commands?

@dscho
Copy link
Member Author

dscho commented Dec 17, 2018

@Sarkie it is even "worse": those config settings' default value has changed. Read: if you do not configure stash.useBuiltin at all, it will use the built-in command.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.