On Fri, Jul 30, 2021 at 1:00 AM Kaartic Sivaraam <kaartic.sivaraam@xxxxxxxxx> wrote: > > On 29/07/21 11:05 pm, Atharva Raykar wrote: > > (apologies for the reflowed text, seems to only happen when replying to > > this message?? Won't affect this response much though) > > > > In case you're using thunderbird then you could see if the following helps: > > http://kb.mozillazine.org/Plain_text_e-mail_%28Thunderbird%29#Flowed_format Yeah, I have pretty much been following the setup that is in the git-format-patch [1] documentation. It worked fine until the last couple of days. The mailing list is now rejecting all my mails. My guess is because Thunderbird is forcing a 'Content-Transfer-Encoding: 7-bit' which I read causes problems with this list [2]. Strangely, so far, this header is added only when I send mails to git@xxxxxxxxxxxxxxx, not elsewhere. (sending this from GMail for now) Here's the error message: ------8<------8<------8<------ <git@xxxxxxxxxxxxxxx>: host 23.128.96.18[23.128.96.18] said: 550 5.7.1 Content-Policy reject msg: Wrong MIME labeling on 8-bit character texts. BF:<H 0>; S229739AbhG2RfR (in reply to end of DATA command) ------8<------8<------8<------ I'll try fixing my mail situation today, and if I still have problems, I'll bring it up on a separate thread. [1] https://git-scm.com/docs/git-format-patch#_approach_1_add_on [2] https://lore.kernel.org/git/alpine.DEB.2.20.1611031554100.3108@virtualbox/ > > On 29/07/21 01:21, Kaartic Sivaraam wrote: > >> Hi Atharva, > >> > >> On 28/07/21 5:23 pm, Atharva Raykar wrote: > >>> Add a new "add-config" subcommand to `git submodule--helper` with the > >>> goal of converting part of the shell code in git-submodule.sh related to > >>> `git submodule add` into C code. This new subcommand sets the > >>> configuration variables of a newly added submodule, by registering the > >>> url in local git config, as well as the submodule name and path in the > >>> .gitmodules file. It also sets 'submodule.<name>.active' to "true" if > >>> the submodule path has not already been covered by any pathspec > >>> specified in 'submodule.active'. > >>> > >>> This is meant to be a faithful conversion from shell to C, with only one > >>> minor change: A warning is emitted if no value is specified in > >>> 'submodule.active', ie, the config looks like: "[submodule] active\n", > >>> because it is an invalid configuration. It would be helpful to let the > >>> user know that the pathspec is unset, and the value of > >>> 'submodule.<name>.active' might be set to 'true' so that they can > >>> rectify their configuration and prevent future surprises (especially > >>> given that the latter variable has a higher priority than the former). > >>> > >> > >> v2 doesn't have the warning that this paragraph describes. So, this could > >> be dropped. > > > > My bad, looks like I forgot to edit the commit message. > > > >>> [ snip ] > >>> > >>> A comment has been > >>> added to explain that only one value of 'submodule.active' is obtained > >>> to check if we need to call is_submodule_active() at all. > >>> > >> > >> This could be me likely not understanding this properly. Anyways, where > >> is this comment in the code? I only see a comment about how > >> 'is_submodule_active' > >> iterates over all values. I couldn't find any "one value" reference in it. > > > > Looks like my comment does not explain it clearly. It would have made > > more sense to start the comment with "If there is no value found for > > submodule.active", but I think instead of modifying that comment (which > > is clear enough as it is), I'll make the commit message better, by > > removing the mention of the "we check one value". > > > > It seems like the line: > > > > if (git_config_get_string("submodule.active", &val) > > > > makes it clear that a single string is being queried first. The larger > > point was about why that conditional was needed, if we were going to > > call 'is_submodule_active()' to retrieve the value anyway. > > > > Ah. Now I get the idea. A rephrasing might indeed make this clear. > > >>> + if (config_submodule_in_gitmodules(add_data->sm_name, "path", add_data->sm_path) || > >>> + config_submodule_in_gitmodules(add_data->sm_name, "url", add_data->repo)) > >>> + die(_("Failed to register submodule '%s'"), add_data->sm_path); > >>> + > >>> + if (add_data->branch) > >>> + if (config_submodule_in_gitmodules(add_data->sm_name, > >>> + "branch", add_data->branch)) > >>> + die(_("Failed to register submodule '%s'"), add_data->sm_path); > >>> + > >>> + add_gitmodules.git_cmd = 1; > >>> + strvec_pushl(&add_gitmodules.args, > >>> + "add", "--force", "--", ".gitmodules", NULL); > >>> + > >>> + if (run_command(&add_gitmodules)) > >>> + die(_("Failed to register submodule '%s'"), add_data->sm_path); > >>> + > >> > >> We could restructure this portion like so ... > >> > >> -- 8< -- > >> add_gitmodules.git_cmd = 1; > >> strvec_pushl(&add_gitmodules.args, > >> "add", "--force", "--", ".gitmodules", NULL); > >>> > >> if (config_submodule_in_gitmodules(add_data->sm_name, "path", add_data->sm_path) || > >> config_submodule_in_gitmodules(add_data->sm_name, "url", add_data->repo) || > >> (add_data->branch && config_submodule_in_gitmodules(add_data->sm_name, > >> "branch", add_data->branch)) || > >> run_command(&add_gitmodules)) > >> die(_("Failed to register submodule '%s'"), > >> add_data->sm_path); > >> -- >8 -- > >> > >> .. to avoid the redundant "Failed to register submodule ..." error message. > >> Whether the restructured version has poor readability or not is debatable, though. > > > > Yeah, I felt the redundancy in this case was okay, I find that big > > conditional rather hard to read. > > > > I tried to make it as easy to read as possible but its a really long one > indeed. So, I could understand. But the redundancy bothered me a bit ;-) > > >>> + /* > >>> + * NEEDSWORK: In a multi-working-tree world this needs to be > >>> + * set in the per-worktree config. > >>> + * > >> > >> It might be a good idea to differentiate the NEEDSWORK comment from an > >> informative comment about the code snippet. > > > > Okay. I suppose you mean give this part it's own closing delimiter and > > start the next line with a new multiline comment. > > > > Yeah. I did mean this. > > > If you meant something else, do let me know. > > > >> Also, you could add another NEEDSWORK/TODO comment regarding the change > >> to 'is_submodule_active' which you mention before[1]. > >> > >> [1]: https://public-inbox.org/git/a6de518a-d4a2-5a2b-28e2-ca8b62f2c85b@xxxxxxxxx/ > > > > Good point. I'll add it. > > > >>> + * If submodule.active does not exist, or if the pathspec was unset, > >>> + * we will activate this module unconditionally. > >>> + * > >>> + * Otherwise, we ask is_submodule_active(), which iterates > >>> + * through all the values of 'submodule.active' to determine > >>> + * if this module is already active. > >>> + */ > >>> + if (git_config_get_string("submodule.active", &val) || > >>> + !is_submodule_active(the_repository, add_data->sm_path)) { > >>> + key = xstrfmt("submodule.%s.active", add_data->sm_name); > >>> + git_config_set_gently(key, "true"); > >>> + free(key); > >>> + } > >> > >> It might be a good idea to expand this condition similar to the scripted version, > >> to retain the following comment which seems like a useful one to keep. > > > > I felt that this version had less redundant code, and hence seemed more > > readable than the expanded conditional in shell. > > > > For comparison this is the same code imitating the shell version: > > > > if (!git_config_get_string("submodule.active", &var) && var) { > > > > /* > > * If the submodule being added isn't already covered by the > > * current configured pathspec, set the submodule's active flag > > */ > > if (!is_submodule_active(the_repository, info->sm_path)) { > > key = xstrfmt("submodule.%s.active", info->sm_name); > > git_config_set_gently(key, "true"); > > free(key); > > } > > > > } else { > > key = xstrfmt("submodule.%s.active", info->sm_name); > > git_config_set_gently(key, "true"); > > free(key); > > } > > > > It repeats the string allocation and freeing, and also is a lot more > > code to parse mentally while reading. The shorter version that I used > > does not feel more "clever" to me than this either. > > > > As for the comment, I felt that the new one I introduced (Otherwise, we > > ask ...) covers the same ground. > > > > I think the comment you introduced only mentions that 'is_submodule_active' > iterates over configs to determine that a submodule is active. It doesn't mention > that we set the submodule's active flag if the submodule is not covered by the > current configured pathspec, which is what the original tries to convey. > Correct me if I missed anything. > > > I am open to reverting to the expanded conditional, but it would be nice > > if you could help me understand the motivation behind why it should be done. > > > > I'm not against short-circuiting the conditional. I suggested expanding the conditional > so that we get a structure similar to the scripted version. That way we could keep the > original comment close to the inside conditional where it felt relevant :) Ah okay, so the reason is so that we could keep the structure similar to retain the comment? Okay, I'll change that. > >>> [ snip ] > >>> > >>> - if git config --get submodule.active >/dev/null > >>> - then > >>> - # If the submodule being adding isn't already covered by the > >>> - # current configured pathspec, set the submodule's active flag > >>> - if ! git submodule--helper is-active "$sm_path" > >>> - then > >>> - git config submodule."$sm_name".active "true" > >>> - fi > >>> - else > >>> - git config submodule."$sm_name".active "true" > >>> - fi > >>> + git submodule--helper add-config ${force:+--force} > >>> ${branch:+--branch "$branch"} --url "$repo" --resolved-url "$realrepo" > >>> --path "$sm_path" --name "$sm_name" > >>> } > >>> # > >>> > >> > > > > > -- > Sivaraam
