Re: [PATCH v4 0/5] git_config callers rewritten with the new config cache API
On 30/07/14 17:45, Matthieu Moy wrote: Tanay Abhra tanay...@gmail.com writes: Also, any thoughts on what to do with git_default_config()? We can, 1 make a new function git_load_default_config(), use it for the rewrites. That seems the most sensible option. It could be called it git.c before the command-dependant part, so that any call to git loads this. I didn't check if it was correct (e.g. do some command rely on the fact that the default config is not loaded?) Hmm, here be dragons ... :-P I don't know that there has actually been any kind of policy regarding the reading of config files/variables in git (or if there is a different policy for plumbing vs porcelain), but it has always seemed to be somewhat ad-hoc; each command decides for itself what it wants to read. However, with increased use of common code which _uses_ certain config variables for correct operation, the 'choice' is much harder to make (and may change after the fact!). For example, about a year ago I submitted a couple of patches which added a call to git_config(git_default_config, NULL) to both 'git pack-refs' and 'git show-refs'. This was as a result of the 'mh/ref-races' branch which introduced a 'stat_validity' api for checking if the packed-refs file had changed on the filesystem since last you looked. This re-used some of the same code used to handle index updates that used config variables like core.checkstat and core.trustctime. These config variables can affect the correctness and/or the efficiency of the code on some platforms (e.g. cygwin, mingw). [Note: 'stat_validity' has since been re-used again (why not?) in some shallow clone code, so similar comments may apply ... I haven't looked.] However, those patches were dropped, because it resulted in an (unwanted) change in behaviour. In particular, 'git show-refs' changed behaviour because it now 'listened' to core.abbrev! I started to look at splitting the 'core config variables' into two groups; essential variables that _all_ git commands should read for correct/efficient/consistent behaviour and everything else (mainly UI related variables). However, something else came up ... Just an FYI. ATB, Ramsay Jones -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 0/5] git_config callers rewritten with the new config cache API
Tanay Abhra tanay...@gmail.com writes: [PATCH v4]: Tiny style nits corrected. Patch 2/5 has been totally reworked. One thing to check is if the config variables I changed in the series are single valued or multi valued. Though I have tried to ascertain if the variable was single valued or not by reading the docs and code, mistakes may creep in. [PATCH v3]: Most of Eric's suggestions has been implemented. See [2] for discussion. Also, new helpers introduced in v7 of the config-set API series have been used. This series builds on the top of 4c715ebb in pu (ta/config-set). I think it semantically needs your other WIP series, that makes git_config_get_* die in case of error. Otherwise, there's an unexpected behavior change in case of error. -- Matthieu Moy http://www-verimag.imag.fr/~moy/ -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 0/5] git_config callers rewritten with the new config cache API
On 7/30/2014 7:16 PM, Matthieu Moy wrote: Tanay Abhra tanay...@gmail.com writes: [PATCH v4]: Tiny style nits corrected. Patch 2/5 has been totally reworked. One thing to check is if the config variables I changed in the series are single valued or multi valued. Though I have tried to ascertain if the variable was single valued or not by reading the docs and code, mistakes may creep in. [PATCH v3]: Most of Eric's suggestions has been implemented. See [2] for discussion. Also, new helpers introduced in v7 of the config-set API series have been used. This series builds on the top of 4c715ebb in pu (ta/config-set). I think it semantically needs your other WIP series, that makes git_config_get_* die in case of error. Otherwise, there's an unexpected behavior change in case of error. Yes you are right, there is a call to git_die_config() also in the series. I will add the info in the next reroll. Also, any thoughts on what to do with git_default_config()? We can, 1 make a new function git_load_default_config(), use it for the rewrites. or 2 git_default_config() is rewritten to be loaded only once even if it is called again and again during the process run, so that we use the same function in callbacks and in the new API using rewrites. Thanks. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 0/5] git_config callers rewritten with the new config cache API
Tanay Abhra tanay...@gmail.com writes: Yes you are right, there is a call to git_die_config() also in the series. I will add the info in the next reroll. If unsure, rebase your branch locally on the commit on which it is meant to apply, and check that it works for you. Also, any thoughts on what to do with git_default_config()? We can, 1 make a new function git_load_default_config(), use it for the rewrites. That seems the most sensible option. It could be called it git.c before the command-dependant part, so that any call to git loads this. I didn't check if it was correct (e.g. do some command rely on the fact that the default config is not loaded?) 2 git_default_config() is rewritten to be loaded only once even if it is called again and again during the process run, so that we use the same function in callbacks and in the new API using rewrites. Seems like a workaround, and to me the point of your GSoC is to make the code cleaner, hence avoid workaraounds ... -- Matthieu Moy http://www-verimag.imag.fr/~moy/ -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
