| Message ID | 20181017164021.15204-3-peartben@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v1,1/2] reset: don't compute unstaged changes after reset when --quiet | expand |
On Wed, Oct 17, 2018 at 12:40 PM Ben Peart <peartben@gmail.com> wrote: > Add a reset.quietDefault config setting that sets the default value of the > --quiet flag when running the reset command. This enables users to change > the default behavior to take advantage of the performance advantages of > avoiding the scan for unstaged changes after reset. Defaults to false. As with the previous patch, my knee-jerk reaction is that this really feels wrong being tied to --quiet. It's particularly unintuitive. What I _could_ see, and what would feel more natural is if you add a new option (say, --optimize) which is more general, incorporating whatever optimizations become available in the future, not just this one special-case. A side-effect of --optimize is that it implies --quiet, and that is something which can and should be documented. > Signed-off-by: Ben Peart <benpeart@microsoft.com>
On Wed, Oct 17, 2018 at 02:19:59PM -0400, Eric Sunshine wrote: > On Wed, Oct 17, 2018 at 12:40 PM Ben Peart <peartben@gmail.com> wrote: > > Add a reset.quietDefault config setting that sets the default value of the > > --quiet flag when running the reset command. This enables users to change > > the default behavior to take advantage of the performance advantages of > > avoiding the scan for unstaged changes after reset. Defaults to false. > > As with the previous patch, my knee-jerk reaction is that this really > feels wrong being tied to --quiet. It's particularly unintuitive. > > What I _could_ see, and what would feel more natural is if you add a > new option (say, --optimize) which is more general, incorporating > whatever optimizations become available in the future, not just this > one special-case. A side-effect of --optimize is that it implies > --quiet, and that is something which can and should be documented. Heh, I just wrote something very similar elsewhere in the thread. I'm still not sure if it's a dumb idea, but at least we can be dumb together. -Peff
On Wed, Oct 17 2018, Jeff King wrote: > On Wed, Oct 17, 2018 at 02:19:59PM -0400, Eric Sunshine wrote: > >> On Wed, Oct 17, 2018 at 12:40 PM Ben Peart <peartben@gmail.com> wrote: >> > Add a reset.quietDefault config setting that sets the default value of the >> > --quiet flag when running the reset command. This enables users to change >> > the default behavior to take advantage of the performance advantages of >> > avoiding the scan for unstaged changes after reset. Defaults to false. >> >> As with the previous patch, my knee-jerk reaction is that this really >> feels wrong being tied to --quiet. It's particularly unintuitive. >> >> What I _could_ see, and what would feel more natural is if you add a >> new option (say, --optimize) which is more general, incorporating >> whatever optimizations become available in the future, not just this >> one special-case. A side-effect of --optimize is that it implies >> --quiet, and that is something which can and should be documented. > > Heh, I just wrote something very similar elsewhere in the thread. I'm > still not sure if it's a dumb idea, but at least we can be dumb > together. Same here. I'm in general if favor of having the ability to configure porcelain command-line options, but in this case it seems like it would be more logical to head for something like: core.uiMessaging=[default,exhaustive,lossyButFaster,quiet] Where default would be our current "exhaustive", and this --quiet case would be covered by lossyButFaster, but also things like the "--no-ahead-behind" flag for git-status. Just on this implementation: The usual idiom for flags as config is command.flag=xyz, not command.flagDefault=xyz, so this should be reset.quiet.
On 10/23/2018 5:13 AM, Ævar Arnfjörð Bjarmason wrote: > > On Wed, Oct 17 2018, Jeff King wrote: > >> On Wed, Oct 17, 2018 at 02:19:59PM -0400, Eric Sunshine wrote: >> >>> On Wed, Oct 17, 2018 at 12:40 PM Ben Peart <peartben@gmail.com> wrote: >>>> Add a reset.quietDefault config setting that sets the default value of the >>>> --quiet flag when running the reset command. This enables users to change >>>> the default behavior to take advantage of the performance advantages of >>>> avoiding the scan for unstaged changes after reset. Defaults to false. >>> >>> As with the previous patch, my knee-jerk reaction is that this really >>> feels wrong being tied to --quiet. It's particularly unintuitive. >>> >>> What I _could_ see, and what would feel more natural is if you add a >>> new option (say, --optimize) which is more general, incorporating >>> whatever optimizations become available in the future, not just this >>> one special-case. A side-effect of --optimize is that it implies >>> --quiet, and that is something which can and should be documented. >> >> Heh, I just wrote something very similar elsewhere in the thread. I'm >> still not sure if it's a dumb idea, but at least we can be dumb >> together. > > Same here. I'm in general if favor of having the ability to configure > porcelain command-line options, but in this case it seems like it would > be more logical to head for something like: > > core.uiMessaging=[default,exhaustive,lossyButFaster,quiet] > > Where default would be our current "exhaustive", and this --quiet case > would be covered by lossyButFaster, but also things like the > "--no-ahead-behind" flag for git-status. > This sounds like an easy way to choose a set of default values that we think make sense to get bundled together. That could be a way for users to quickly choose a set of good defaults but I still think you would want find grained control over the individual settings. Coming up with the set of values to bundle together, figuring out the hierarchy of precedence for this new global config->individual config->individual command line, updating the code to make it all work is outside the scope of this particular patch series. > Just on this implementation: The usual idiom for flags as config is > command.flag=xyz, not command.flagDefault=xyz, so this should be > reset.quiet. > Thanks, I agree and fixed that in later iterations.
On Tue, Oct 23, 2018 at 02:11:01PM -0400, Ben Peart wrote: > This sounds like an easy way to choose a set of default values that we think > make sense to get bundled together. That could be a way for users to quickly > choose a set of good defaults but I still think you would want find grained > control over the individual settings. > > Coming up with the set of values to bundle together, figuring out the > hierarchy of precedence for this new global config->individual > config->individual command line, updating the code to make it all work is > outside the scope of this particular patch series. True, it probably does make sense to give individual defaults. Having a unifying option may help with the discoverability issue you were thinking of elsewhere, though. -Peff
On Tue, Oct 23 2018, Ben Peart wrote: > On 10/23/2018 5:13 AM, Ævar Arnfjörð Bjarmason wrote: >> >> On Wed, Oct 17 2018, Jeff King wrote: >> >>> On Wed, Oct 17, 2018 at 02:19:59PM -0400, Eric Sunshine wrote: >>> >>>> On Wed, Oct 17, 2018 at 12:40 PM Ben Peart <peartben@gmail.com> wrote: >>>>> Add a reset.quietDefault config setting that sets the default value of the >>>>> --quiet flag when running the reset command. This enables users to change >>>>> the default behavior to take advantage of the performance advantages of >>>>> avoiding the scan for unstaged changes after reset. Defaults to false. >>>> >>>> As with the previous patch, my knee-jerk reaction is that this really >>>> feels wrong being tied to --quiet. It's particularly unintuitive. >>>> >>>> What I _could_ see, and what would feel more natural is if you add a >>>> new option (say, --optimize) which is more general, incorporating >>>> whatever optimizations become available in the future, not just this >>>> one special-case. A side-effect of --optimize is that it implies >>>> --quiet, and that is something which can and should be documented. >>> >>> Heh, I just wrote something very similar elsewhere in the thread. I'm >>> still not sure if it's a dumb idea, but at least we can be dumb >>> together. >> >> Same here. I'm in general if favor of having the ability to configure >> porcelain command-line options, but in this case it seems like it would >> be more logical to head for something like: >> >> core.uiMessaging=[default,exhaustive,lossyButFaster,quiet] >> >> Where default would be our current "exhaustive", and this --quiet case >> would be covered by lossyButFaster, but also things like the >> "--no-ahead-behind" flag for git-status. >> > > This sounds like an easy way to choose a set of default values that we > think make sense to get bundled together. That could be a way for > users to quickly choose a set of good defaults but I still think you > would want find grained control over the individual settings. Would you? It seems wanting to configure reset's --quiet in particular is purely a proxy goal for wanting to toggle off slow things in the UI. Otherwise why focus on it, and not the plethora of other --quiet options we have? # Including (but probably not limited to): $ git grep -e OPT__QUIET -e '(OPT|option).*"quiet"' -- '*.[ch]' | wc -l 34 > Coming up with the set of values to bundle together, figuring out the > hierarchy of precedence for this new global config->individual > config->individual command line[...] If we'd still want reset.quiet & whatever the global "turn off slow stuff" UI option is then this part is easy and e.g. {transfer,fetch,receive}.fsckObjects can be used as a template for how to do it. https://github.com/git/git/blob/v2.19.0/fetch-pack.c#L1432-L1443 https://github.com/git/git/blob/v2.19.0/fetch-pack.c#L859-L863 I.e. the more specific option always overrides the less specific one. > [...]updating the code to make it all work is outside the scope of > this particular patch series. Is that a Jedi mind trick to get out of patch review? :) I understand that it's not the patch you wrote, but sometimes feedback is "maybe we shouldn't do this, but this other thing". The --ahead-behind config setting stalled on-list before: https://public-inbox.org/git/36e3a9c3-f7e2-4100-1bfc-647b809a09d0@jeffhostetler.com/ Now we have this similarly themed thing. I think we need to be mindful of how changes like this can add up to very confusing UI. I.e. in this case I can see a "how take make git fast on large repos" post on stackoverflow in our future where the answer is setting a bunch of seemingly irrelevant config options like reset.quiet and status.aheadbehind=false etc. So maybe we should take a step back and consider if the real thing we want is just some way for the user to tell git "don't work so hard at coming up with these values". That can also be smart, e.g. some "auto" setting that tweaks it based on estimated repo size so even with the same config your tiny dotfiles.git will get "ahead/behind" reporting, but not when you cd into windows.git. >> Just on this implementation: The usual idiom for flags as config is >> command.flag=xyz, not command.flagDefault=xyz, so this should be >> reset.quiet. >> > > Thanks, I agree and fixed that in later iterations.
On 10/23/2018 4:03 PM, Ævar Arnfjörð Bjarmason wrote: > [snip] > The --ahead-behind config setting stalled on-list before: > https://public-inbox.org/git/36e3a9c3-f7e2-4100-1bfc-647b809a09d0@jeffhostetler.com/ > > Now we have this similarly themed thing. > > I think we need to be mindful of how changes like this can add up to > very confusing UI. I.e. in this case I can see a "how take make git fast > on large repos" post on stackoverflow in our future where the answer is > setting a bunch of seemingly irrelevant config options like reset.quiet > and status.aheadbehind=false etc. > > So maybe we should take a step back and consider if the real thing we > want is just some way for the user to tell git "don't work so hard at > coming up with these values". > > That can also be smart, e.g. some "auto" setting that tweaks it based on > estimated repo size so even with the same config your tiny dotfiles.git > will get "ahead/behind" reporting, but not when you cd into windows.git. Generally, there are a lot of config settings that are likely in the "if you have a big repo, then you should use this" category. However, there is rarely a one-size-fits-all solution to these problems, just like there are different ways a repo can be "big" (working directory? number of commits? submodules?). I would typically expect that users with _really_ big repos have the resources to have an expert tweak the settings that are best for that data shape and share those settings with all the users. In VFS for Git, we turn certain config settings on by default when mounting the repo [1], but others are either signaled through warning messages (like the status.aheadBehind config setting [2]). We never upstreamed the status.aheadBehind config setting [2], but we _did_ upstream the command-line option as fd9b544 "status: add --[no-]ahead-behind to status and commit for V2 format". We didn't want to change the expected output permanently, so we didn't add the config setting to our list of "required" settings, but instead created a list of optional settings [3]; these settings don't override the existing settings so users can opt-out. (Now that we have the commit-graph enabled and kept up-to-date, it may be time to revisit the importance of this setting.) All of this is to say: it is probably a good idea to have some "recommended configuration" for big repos, but there will always be power users who want to tweak each and every one of these settings. I'm open to design ideas of how to store a list of recommended configurations and how to set a group of config settings with one command (say, a "git recommended-config [small|large|submodules]" builtin that fills the local config with the important settings). Thanks, -Stolee [1] https://github.com/Microsoft/VFSForGit/blob/7daa9f1133764a4e4bd87014833fc2091e6702c1/GVFS/GVFS/CommandLine/GVFSVerb.cs#L79-L104 Code in VFS for Git that enables "required" config settings. [2] https://github.com/Microsoft/git/commit/0cbe9e6b23e4d9008d4a1676e1dd6a87bdcd6ed5 status: add status.aheadbehind setting [3] https://github.com/Microsoft/VFSForGit/blob/7daa9f1133764a4e4bd87014833fc2091e6702c1/GVFS/GVFS/CommandLine/GVFSVerb.cs#L120-L123 Code in VFS for Git that enables "optional" config settings.
On Wed, Oct 24, 2018 at 11:48:20AM -0400, Derrick Stolee wrote: > Generally, there are a lot of config settings that are likely in the "if you > have a big repo, then you should use this" category. However, there is > rarely a one-size-fits-all solution to these problems, just like there are > different ways a repo can be "big" (working directory? number of commits? > submodules?). > [...] > All of this is to say: it is probably a good idea to have some "recommended > configuration" for big repos, but there will always be power users who want > to tweak each and every one of these settings. I'm open to design ideas of > how to store a list of recommended configurations and how to set a group of > config settings with one command (say, a "git recommended-config > [small|large|submodules]" builtin that fills the local config with the > important settings). Maybe it would be useful to teach git-sizer[1] to recommend particular settings based on the actual definitions of "big" that it measures. I do hope that some options will just be no-brainers to enable always, though (e.g., I think in the long run commit-graph should just default to "on"; it's cheap to keep up to date and helps proportionally to the repo size). [1] https://github.com/github/git-sizer
Jeff King <peff@peff.net> writes: > I do hope that some options will just be no-brainers to enable always, > though (e.g., I think in the long run commit-graph should just default > to "on"; it's cheap to keep up to date and helps proportionally to the > repo size). Same here. We should strive to make any feature to optimize for repositories with a particular trait not to hurt too much to repositories without that trait, so that we can start such a feature as opt-in but later can make it the default for everybody. Sometimes it may not be possible, but my gut feeling is that features aiming for optimizing big repositories should fundamentally need only very small overhead when enabled in a small repository. So I view them not as a set of million "if your repository matches this criterion, turn it on" knobs. Rather, they are "we haven't tested it fully, but you can opt into the experiment a new way to do the same operation, which is designed to optimize for repositories with this trait. Enabling it even when your repository does not have that trait and reporting regression is also very welcome, as it is a good indication that the new way has rough edges at its corners". Thanks.
diff --git a/Documentation/config.txt b/Documentation/config.txt index f6f4c21a54..a5cf4c019b 100644 --- a/Documentation/config.txt +++ b/Documentation/config.txt @@ -2728,6 +2728,12 @@ rerere.enabled:: `$GIT_DIR`, e.g. if "rerere" was previously used in the repository. +reset.quietDefault:: + Sets the default value of the "quiet" option for the reset command. + Choosing "quiet" can optimize the performance of the reset command + by avoiding the scan of all files in the repo looking for additional + unstaged changes. Defaults to false. + include::sendemail-config.txt[] sequence.editor:: diff --git a/builtin/reset.c b/builtin/reset.c index 0822798616..7d151d48a0 100644 --- a/builtin/reset.c +++ b/builtin/reset.c @@ -306,6 +306,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix) }; git_config(git_reset_config, NULL); + git_config_get_bool("reset.quietDefault", &quiet); argc = parse_options(argc, argv, prefix, options, git_reset_usage, PARSE_OPT_KEEP_DASHDASH);