| Message ID | patch-1.1-e1d8c842c70-20210428T161817Z-avarab@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | repo-settings.c: simplify the setup | expand |
On 4/28/2021 12:26 PM, Ævar Arnfjörð Bjarmason wrote: > Simplify the setup code in repo-settings.c in various ways, making the > code shorter, easier to read, and requiring fewer hacks to do the same > thing as it did before: This patch is interesting, and I'll review it when I have some more time. Probably tomorrow. But I thought that I would point out that this pattern of adding a patch within the thread of a larger series makes it very difficult to separate the two. I use an email client that groups messages by thread in order to help parse meaningful discussion from the list which otherwise looks like a fire hose of noise. Now, this patch is linked to the FS Monitor thread and feedback to either will trigger the thread as having unread messages. I find it very difficult to track multiple patch series that are being juggled in the same thread. It is mentally taxing enough that I have avoided reviewing code presented this way to save myself the effort of tracking which patches go with what topic in what order. Since I've committed to reviewing the FS Monitor code, I'd prefer if this patch (or maybe its v2, since this is here already) be sent as a top-level message so it can be discussed independently. Thanks, -Stolee
On Wed, Apr 28 2021, Derrick Stolee wrote: > On 4/28/2021 12:26 PM, Ævar Arnfjörð Bjarmason wrote: >> Simplify the setup code in repo-settings.c in various ways, making the >> code shorter, easier to read, and requiring fewer hacks to do the same >> thing as it did before: > > This patch is interesting, and I'll review it when I have some more > time. Probably tomorrow. > > But I thought that I would point out that this pattern of adding a > patch within the thread of a larger series makes it very difficult > to separate the two. I use an email client that groups messages by > thread in order to help parse meaningful discussion from the list > which otherwise looks like a fire hose of noise. Now, this patch is > linked to the FS Monitor thread and feedback to either will trigger > the thread as having unread messages. > > I find it very difficult to track multiple patch series that are > being juggled in the same thread. It is mentally taxing enough that > I have avoided reviewing code presented this way to save myself the > effort of tracking which patches go with what topic in what order. > > Since I've committed to reviewing the FS Monitor code, I'd prefer if > this patch (or maybe its v2, since this is here already) be sent as > a top-level message so it can be discussed independently. As a practical matter I think any effort I make to accommodate your request will be dwarfed by your own starting of a sub-thread on E-Mail/MUA nuances :) When [1] was brought up the other day (showing that I'm probably not the best person to ask about on-list In-Reply-To semantics) I was surprised to find that we don't have much (if any) explicit documentation about In-Reply-To best practices. There's a passing mention in Documentation/MyFirstContribution.txt, but as far as I can tell from a cursory glance that's it. Personally I draw the line at "this random unrelated thing occurred to me while reading X" v.s. "this is directly in reply to X". Reading the upthread I don't really see a good point at which to start breaking the reply chain and not make things harder for others reading along with clients that aren't yours (which, looking at your headers seems to be Thunderbird 78). I.e. the one feedback on the patch idea is your upthread "waiting until such a change". With threading you can see the context, but without you'd need to get it via some not-MUA side-channel (presumably lore.kernel.org link). Sending a v2 (if any) without threading would break the chain again. 1. https://lore.kernel.org/git/nycvar.QRO.7.76.6.2103191540330.57@tvgsbejvaqbjf.bet/
Derrick Stolee <stolee@gmail.com> writes: > On 4/28/2021 12:26 PM, Ævar Arnfjörð Bjarmason wrote: >> Simplify the setup code in repo-settings.c in various ways, making the >> code shorter, easier to read, and requiring fewer hacks to do the same >> thing as it did before: > > This patch is interesting, and I'll review it when I have some more > time. Probably tomorrow. > > But I thought that I would point out that this pattern of adding a > patch within the thread of a larger series makes it very difficult > to separate the two. I use an email client that groups messages by > thread in order to help parse meaningful discussion from the list > which otherwise looks like a fire hose of noise. Now, this patch is > linked to the FS Monitor thread and feedback to either will trigger > the thread as having unread messages. > > I find it very difficult to track multiple patch series that are > being juggled in the same thread. It is mentally taxing enough that > I have avoided reviewing code presented this way to save myself the > effort of tracking which patches go with what topic in what order. I do find it distracting to have a full "ah, I just thought of something while discussing this unrelated series" patch fairly irritating for the same reason. It however is unavoidable human nature that we come up with ideas while thinking about something not necessarily related. So it largely is a presentation issue. I really appreciate the way some people (Peff is a stellar example, but there are others who are as good at this) handle these tangents, where the message sent to an existing thread is limited to only give an outline of the idea (possibly with "something like this?" patch for illustration) and then they quickly get out of the way of the discussion by starting a separate thread, while back-referencing "So here is a proper patch based on the idea I interjected in the discussion of that other topic." And the discussion on the tangent will be done on its own thread.
On Thu, Apr 29 2021, Junio C Hamano wrote: > Derrick Stolee <stolee@gmail.com> writes: > >> On 4/28/2021 12:26 PM, Ævar Arnfjörð Bjarmason wrote: >>> Simplify the setup code in repo-settings.c in various ways, making the >>> code shorter, easier to read, and requiring fewer hacks to do the same >>> thing as it did before: >> >> This patch is interesting, and I'll review it when I have some more >> time. Probably tomorrow. >> >> But I thought that I would point out that this pattern of adding a >> patch within the thread of a larger series makes it very difficult >> to separate the two. I use an email client that groups messages by >> thread in order to help parse meaningful discussion from the list >> which otherwise looks like a fire hose of noise. Now, this patch is >> linked to the FS Monitor thread and feedback to either will trigger >> the thread as having unread messages. >> >> I find it very difficult to track multiple patch series that are >> being juggled in the same thread. It is mentally taxing enough that >> I have avoided reviewing code presented this way to save myself the >> effort of tracking which patches go with what topic in what order. > > I do find it distracting to have a full "ah, I just thought of > something while discussing this unrelated series" patch fairly > irritating for the same reason. It however is unavoidable human > nature that we come up with ideas while thinking about something not > necessarily related. So it largely is a presentation issue. > > I really appreciate the way some people (Peff is a stellar example, > but there are others who are as good at this) handle these tangents, > where the message sent to an existing thread is limited to only give > an outline of the idea (possibly with "something like this?" patch > for illustration) and then they quickly get out of the way of the > discussion by starting a separate thread, while back-referencing "So > here is a proper patch based on the idea I interjected in the > discussion of that other topic." And the discussion on the tangent > will be done on its own thread. In RFC 822 terms. Are you talking about the In-Reply-To[1] or References[2] headers, or both/neither? I'm happy to go along with whatever the convention is, but as noted think it's valuable to come to some explicit decision to document the convention. Threading isn't a concept that exists in E-Mail protocols per-se. Just In-Reply-To and References. The References header can reference N messages most would think about as a separate "thread", and "thread" is ultimately some fuzzy MUA-specific concept on top of these (and others). E.g. in my client right now I'm looking at just 4 messages in this "thread", it doesn't descend down the whole In-Reply-To, others would act differently. Some (such as GMail) have their own ad-hoc concept of "thread" separate from anything in RFCs (which includes some fuzzy group-by-subject). In GMail's web UI everything as of my "upthread" <patch-1.1-e1d8c842c70-20210428T161817Z-avarab@gmail.com> is presented as its own thread. The ML read as it happens, but it's also a collectively maintained datastructure. It seems to me to be better to veer on the side of using standard fields for their intended purpose for archiving / future use. I.e. making "a reference" universally machine-readable, as opposed to a lore.kernel.org link, or a free-form "in a recent thread" blurb. ML Archive Formats Matter[3] :) But yes, maybe MUAs in the wild these days mostly render things one way or another, so catering to them would be a good trade-off. I'm writing this from within an Emacs MUA, so I don't have much of a feel for common MUA conventions these days. I'm prodding to see if we can define the problem exactly, because e.g. maybe "References: <break@threading.hack> [actual <references>]" is something that would achieve both aims, i.e. make the references machine-readable, but break up threading in common in-the-wild clients. We could then patch format-patch etc. to support such "detached" threading. 1. https://tools.ietf.org/html/rfc822#section-4.6.2 2. https://tools.ietf.org/html/rfc822#section-4.6.3 3. https://keithp.com/blogs/Repository_Formats_Matter/
On Thu, Apr 29, 2021 at 02:14:52PM +0200, Ævar Arnfjörð Bjarmason wrote: > > I really appreciate the way some people (Peff is a stellar example, > > but there are others who are as good at this) handle these tangents, > > where the message sent to an existing thread is limited to only give > > an outline of the idea (possibly with "something like this?" patch > > for illustration) and then they quickly get out of the way of the > > discussion by starting a separate thread, while back-referencing "So > > here is a proper patch based on the idea I interjected in the > > discussion of that other topic." And the discussion on the tangent > > will be done on its own thread. > > In RFC 822 terms. Are you talking about the In-Reply-To[1] or > References[2] headers, or both/neither? Since I got listed as an example, I can tell you what I do: I start a totally new thread with no in-reply-to or references to the old thread. And the subject is new (usually "[PATCH 0/N] foo..."), so no clever group-by-subject heuristics will link them. It's usually a good idea to reference the message-id/lore link in at least one direction, though (usually I'd do it in the new thread, saying "this is a followup to ...", but you could also follow-up in the original to say "I've spun this off into its own series here..."). Which is really _sort of_ like putting it into "References", except that it's not machine readable. Which is a good thing, because it's a weaker form and doesn't tell mail clients to group it all into one thread. > Threading isn't a concept that exists in E-Mail protocols per-se. Just > In-Reply-To and References. The References header can reference N > messages most would think about as a separate "thread", and "thread" is > ultimately some fuzzy MUA-specific concept on top of these (and others). > > E.g. in my client right now I'm looking at just 4 messages in this > "thread", it doesn't descend down the whole In-Reply-To, others would > act differently. Interesting. Mutt (and notmuch, and public-inbox) definitely view these as part of a larger thread. It looks like you're using mu4e; I'm surprised it doesn't, too (of course some clients will give a partial view of a thread if you've already marked the older messages as read and moved them into an archival folder). > It seems to me to be better to veer on the side of using standard fields > for their intended purpose for archiving / future use. I.e. making "a > reference" universally machine-readable, as opposed to a lore.kernel.org > link, or a free-form "in a recent thread" blurb. I'd disagree here. There's a long history of intentionally breaking the thread in mailing lists and newsgroups exactly because the topic is sufficiently different that you want to make it easy for people to treat it as a separate unit. I admit there's a bit of an art form to deciding when that is appropriate and when not. -Peff
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > In RFC 822 terms. Are you talking about the In-Reply-To[1] or > References[2] headers, or both/neither? Neither (I think Peff explained why it is a good idea to defer to verbal communication not to confuse tools better than I could).
Hi Ævar, On Thu, 29 Apr 2021, Ævar Arnfjörð Bjarmason wrote: > On Wed, Apr 28 2021, Derrick Stolee wrote: > > > On 4/28/2021 12:26 PM, Ævar Arnfjörð Bjarmason wrote: > >> Simplify the setup code in repo-settings.c in various ways, making the > >> code shorter, easier to read, and requiring fewer hacks to do the same > >> thing as it did before: > > > > [...] > > Since I've committed to reviewing the FS Monitor code, I'd prefer if > > this patch (or maybe its v2, since this is here already) be sent as > > a top-level message so it can be discussed independently. > > As a practical matter I think any effort I make to accommodate your > request will be dwarfed by your own starting of a sub-thread on > E-Mail/MUA nuances :) > > When [1] was brought up the other day (showing that I'm probably not the > best person to ask about on-list In-Reply-To semantics) I was surprised > to find that we don't have much (if any) explicit documentation about > In-Reply-To best practices. [...] > > 1. https://lore.kernel.org/git/nycvar.QRO.7.76.6.2103191540330.57@tvgsbejvaqbjf.bet/ I find it a bit disingenous to reference my complaint about your disconnected cover letter (which _definitely_ belongs with the patches for which it covers) with the practice of hiding patches or patch series deep in a thread discussion an (lengthy!) patch series, _especially_ if it threatens to totally conflict with that patch series and thereby disrupt the flow. Couldn't you hold off with your patch for a while, instead help FSMonitor get over the finish line, and _then_ submit that simplification of repo-settings? That would be constructive, from my perspective. Ciao, Dscho
diff --git a/cache.h b/cache.h index 148d9ab5f18..7ea0feb3462 100644 --- a/cache.h +++ b/cache.h @@ -1684,13 +1684,6 @@ int update_server_info(int); const char *get_log_output_encoding(void); const char *get_commit_output_encoding(void); -/* - * This is a hack for test programs like test-dump-untracked-cache to - * ensure that they do not modify the untracked cache when reading it. - * Do not use it otherwise! - */ -extern int ignore_untracked_cache_config; - int committer_ident_sufficiently_given(void); int author_ident_sufficiently_given(void); diff --git a/environment.c b/environment.c index 2f27008424a..bc825cc7e05 100644 --- a/environment.c +++ b/environment.c @@ -96,13 +96,6 @@ int auto_comment_line_char; /* Parallel index stat data preload? */ int core_preload_index = 1; -/* - * This is a hack for test programs like test-dump-untracked-cache to - * ensure that they do not modify the untracked cache when reading it. - * Do not use it otherwise! - */ -int ignore_untracked_cache_config; - /* This is set by setup_git_dir_gently() and/or git_default_config() */ char *git_work_tree_cfg; diff --git a/fetch-negotiator.c b/fetch-negotiator.c index 57ed5784e14..c7c0eda7e21 100644 --- a/fetch-negotiator.c +++ b/fetch-negotiator.c @@ -8,8 +8,11 @@ void fetch_negotiator_init(struct repository *r, struct fetch_negotiator *negotiator) { + enum fetch_negotiation_setting setting; prepare_repo_settings(r); - switch(r->settings.fetch_negotiation_algorithm) { + setting = r->settings.fetch_negotiation_algorithm; + + switch (setting) { case FETCH_NEGOTIATION_SKIPPING: skipping_negotiator_init(negotiator); return; @@ -19,7 +22,6 @@ void fetch_negotiator_init(struct repository *r, return; case FETCH_NEGOTIATION_DEFAULT: - default: default_negotiator_init(negotiator); return; } diff --git a/read-cache.c b/read-cache.c index 5a907af2fb5..1aefe4a5c23 100644 --- a/read-cache.c +++ b/read-cache.c @@ -1889,16 +1889,23 @@ static void check_ce_order(struct index_state *istate) static void tweak_untracked_cache(struct index_state *istate) { struct repository *r = the_repository; + enum untracked_cache_setting setting; prepare_repo_settings(r); + setting = r->settings.core_untracked_cache; - if (r->settings.core_untracked_cache == UNTRACKED_CACHE_REMOVE) { + switch (setting) { + case UNTRACKED_CACHE_REMOVE: remove_untracked_cache(istate); - return; - } - - if (r->settings.core_untracked_cache == UNTRACKED_CACHE_WRITE) + break; + case UNTRACKED_CACHE_WRITE: add_untracked_cache(istate); + break; + case UNTRACKED_CACHE_UNSET: + /* This includes core.untrackedCache=keep */ + break; + } + return; } static void tweak_split_index(struct index_state *istate) diff --git a/repo-settings.c b/repo-settings.c index f7fff0f5ab8..2be242fde1d 100644 --- a/repo-settings.c +++ b/repo-settings.c @@ -3,40 +3,84 @@ #include "repository.h" #include "midx.h" -#define UPDATE_DEFAULT_BOOL(s,v) do { if (s == -1) { s = v; } } while(0) +static void repo_config_get_bool_or(struct repository *r, const char *key, + int *dest, int def) +{ + if (repo_config_get_bool(r, key, dest)) + *dest = def; +} void prepare_repo_settings(struct repository *r) { - int value; + int experimental; + int intval; char *strval; + int manyfiles; if (r->settings.initialized) return; /* Defaults */ - memset(&r->settings, -1, sizeof(r->settings)); + r->settings.index_version = -1; + r->settings.core_untracked_cache = UNTRACKED_CACHE_UNSET; + r->settings.fetch_negotiation_algorithm = FETCH_NEGOTIATION_DEFAULT; + + /* Booleans config or default, cascades to other settings */ + repo_config_get_bool_or(r, "feature.manyfiles", &manyfiles, 0); + repo_config_get_bool_or(r, "feature.experimental", &experimental, 0); + + /* Defaults modified by feature.* */ + if (experimental) { + r->settings.fetch_negotiation_algorithm = FETCH_NEGOTIATION_SKIPPING; + } + if (manyfiles) { + r->settings.index_version = 4; + r->settings.core_untracked_cache = UNTRACKED_CACHE_WRITE; + } - if (!repo_config_get_bool(r, "core.commitgraph", &value)) - r->settings.core_commit_graph = value; - if (!repo_config_get_bool(r, "commitgraph.readchangedpaths", &value)) - r->settings.commit_graph_read_changed_paths = value; - if (!repo_config_get_bool(r, "gc.writecommitgraph", &value)) - r->settings.gc_write_commit_graph = value; - UPDATE_DEFAULT_BOOL(r->settings.core_commit_graph, 1); - UPDATE_DEFAULT_BOOL(r->settings.commit_graph_read_changed_paths, 1); - UPDATE_DEFAULT_BOOL(r->settings.gc_write_commit_graph, 1); + /* Boolean config or default, does not cascade (simple) */ + repo_config_get_bool_or(r, "core.commitgraph", + &r->settings.core_commit_graph, 1); + repo_config_get_bool_or(r, "commitgraph.readchangedpaths", + &r->settings.commit_graph_read_changed_paths, 1); + repo_config_get_bool_or(r, "gc.writecommitgraph", + &r->settings.gc_write_commit_graph, 1); + repo_config_get_bool_or(r, "fetch.writecommitgraph", + &r->settings.fetch_write_commit_graph, 0); + repo_config_get_bool_or(r, "pack.usesparse", + &r->settings.pack_use_sparse, 1); + repo_config_get_bool_or(r, "core.multipackindex", + &r->settings.core_multi_pack_index, 1); - if (!repo_config_get_int(r, "index.version", &value)) - r->settings.index_version = value; - if (!repo_config_get_maybe_bool(r, "core.untrackedcache", &value)) { - if (value == 0) - r->settings.core_untracked_cache = UNTRACKED_CACHE_REMOVE; - else - r->settings.core_untracked_cache = UNTRACKED_CACHE_WRITE; - } else if (!repo_config_get_string(r, "core.untrackedcache", &strval)) { - if (!strcasecmp(strval, "keep")) - r->settings.core_untracked_cache = UNTRACKED_CACHE_KEEP; + /* + * The GIT_TEST_MULTI_PACK_INDEX variable is special in that + * either it *or* the config sets + * r->settings.core_multi_pack_index if true. We don't take + * the environment variable if it exists (even if false) over + * any config, as in other cases. + */ + if (git_env_bool(GIT_TEST_MULTI_PACK_INDEX, 0)) + r->settings.core_multi_pack_index = 1; + /* + * Non-boolean config + */ + if (!repo_config_get_int(r, "index.version", &intval)) + r->settings.index_version = intval; + + if (!repo_config_get_string(r, "core.untrackedcache", &strval)) { + int maybe_bool = git_parse_maybe_bool(strval); + if (maybe_bool == -1) { + /* + * Set to "keep", or some other non-boolean + * value. In either case we do nothing but + * keep UNTRACKED_CACHE_UNSET. + */ + } else { + r->settings.core_untracked_cache = maybe_bool + ? UNTRACKED_CACHE_WRITE + : UNTRACKED_CACHE_REMOVE; + } free(strval); } @@ -45,36 +89,5 @@ void prepare_repo_settings(struct repository *r) r->settings.fetch_negotiation_algorithm = FETCH_NEGOTIATION_SKIPPING; else if (!strcasecmp(strval, "noop")) r->settings.fetch_negotiation_algorithm = FETCH_NEGOTIATION_NOOP; - else - r->settings.fetch_negotiation_algorithm = FETCH_NEGOTIATION_DEFAULT; } - - if (!repo_config_get_bool(r, "pack.usesparse", &value)) - r->settings.pack_use_sparse = value; - UPDATE_DEFAULT_BOOL(r->settings.pack_use_sparse, 1); - - value = git_env_bool(GIT_TEST_MULTI_PACK_INDEX, 0); - if (value || !repo_config_get_bool(r, "core.multipackindex", &value)) - r->settings.core_multi_pack_index = value; - UPDATE_DEFAULT_BOOL(r->settings.core_multi_pack_index, 1); - - if (!repo_config_get_bool(r, "feature.manyfiles", &value) && value) { - UPDATE_DEFAULT_BOOL(r->settings.index_version, 4); - UPDATE_DEFAULT_BOOL(r->settings.core_untracked_cache, UNTRACKED_CACHE_WRITE); - } - - if (!repo_config_get_bool(r, "fetch.writecommitgraph", &value)) - r->settings.fetch_write_commit_graph = value; - UPDATE_DEFAULT_BOOL(r->settings.fetch_write_commit_graph, 0); - - if (!repo_config_get_bool(r, "feature.experimental", &value) && value) - UPDATE_DEFAULT_BOOL(r->settings.fetch_negotiation_algorithm, FETCH_NEGOTIATION_SKIPPING); - - /* Hack for test programs like test-dump-untracked-cache */ - if (ignore_untracked_cache_config) - r->settings.core_untracked_cache = UNTRACKED_CACHE_KEEP; - else - UPDATE_DEFAULT_BOOL(r->settings.core_untracked_cache, UNTRACKED_CACHE_KEEP); - - UPDATE_DEFAULT_BOOL(r->settings.fetch_negotiation_algorithm, FETCH_NEGOTIATION_DEFAULT); } diff --git a/repository.h b/repository.h index b385ca3c94b..9345423c5ba 100644 --- a/repository.h +++ b/repository.h @@ -12,18 +12,15 @@ struct raw_object_store; struct submodule_cache; enum untracked_cache_setting { - UNTRACKED_CACHE_UNSET = -1, - UNTRACKED_CACHE_REMOVE = 0, - UNTRACKED_CACHE_KEEP = 1, - UNTRACKED_CACHE_WRITE = 2 + UNTRACKED_CACHE_UNSET, + UNTRACKED_CACHE_REMOVE, + UNTRACKED_CACHE_WRITE, }; enum fetch_negotiation_setting { - FETCH_NEGOTIATION_UNSET = -1, - FETCH_NEGOTIATION_NONE = 0, - FETCH_NEGOTIATION_DEFAULT = 1, - FETCH_NEGOTIATION_SKIPPING = 2, - FETCH_NEGOTIATION_NOOP = 3, + FETCH_NEGOTIATION_DEFAULT, + FETCH_NEGOTIATION_SKIPPING, + FETCH_NEGOTIATION_NOOP, }; struct repo_settings { diff --git a/t/helper/test-dump-untracked-cache.c b/t/helper/test-dump-untracked-cache.c index cf0f2c7228e..8b73a2f8bc3 100644 --- a/t/helper/test-dump-untracked-cache.c +++ b/t/helper/test-dump-untracked-cache.c @@ -45,8 +45,10 @@ int cmd__dump_untracked_cache(int ac, const char **av) struct untracked_cache *uc; struct strbuf base = STRBUF_INIT; - /* Hack to avoid modifying the untracked cache when we read it */ - ignore_untracked_cache_config = 1; + /* Set core.untrackedCache=keep before setup_git_directory() */ + setenv("GIT_CONFIG_COUNT", "1", 1); + setenv("GIT_CONFIG_KEY_0", "core.untrackedCache", 1); + setenv("GIT_CONFIG_VALUE_0", "keep", 1); setup_git_directory(); if (read_cache() < 0)