-
Notifications
You must be signed in to change notification settings - Fork 141
[Outreachy] advice: revamp advise API #548
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
ec1654c
to
0dcab7b
Compare
/submit |
Submitted as [email protected] |
On the Git mailing list, Derrick Stolee wrote (reply to this):
|
On the Git mailing list, Junio C Hamano wrote (reply to this):
|
On the Git mailing list, Taylor Blau wrote (reply to this):
|
On the Git mailing list, Jeff King wrote (reply to this):
|
On the Git mailing list, Emily Shaffer wrote (reply to this):
|
On the Git mailing list, Junio C Hamano wrote (reply to this):
|
On the Git mailing list, Emily Shaffer wrote (reply to this):
|
On the Git mailing list, Heba Waly wrote (reply to this):
|
On the Git mailing list, Heba Waly wrote (reply to this):
|
On the Git mailing list, Heba Waly wrote (reply to this):
|
On the Git mailing list, Heba Waly wrote (reply to this):
|
On the Git mailing list, Heba Waly wrote (reply to this):
|
On the Git mailing list, Derrick Stolee wrote (reply to this):
|
On the Git mailing list, Junio C Hamano wrote (reply to this):
|
On the Git mailing list, Jeff King wrote (reply to this):
|
On the Git mailing list, Jeff King wrote (reply to this):
|
On the Git mailing list, Taylor Blau wrote (reply to this):
|
06124d1
to
3e4f52e
Compare
/submit |
Submitted as [email protected] |
On the Git mailing list, Junio C Hamano wrote (reply to this):
|
@@ -695,6 +695,7 @@ X = | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the Git mailing list, Junio C Hamano wrote (reply to this):
There seem to be a handful of lines with trailing whitespaces. I
think I've fixed them all on the receiving end, but please proofread
before you come up with the next round.
Thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the Git mailing list, Heba Waly wrote (reply to this):
On Mon, Feb 17, 2020 at 4:28 PM Junio C Hamano <[email protected]> wrote:
>
>And I see that now you are going all the way to discard the
>string-based keys to enumeration, I think this deserves to be called
>"revamp advise API".
Fair enough.
> There seem to be a handful of lines with trailing whitespaces. I
> think I've fixed them all on the receiving end, but please proofread
> before you come up with the next round.
>
Sorry about that, I fixed this setting in my IDE a while ago but looks
like it has a bug.
Will keep an eye on it in the future, thanks
> Thanks.
Heba
This branch is now known as |
Submitted as [email protected] |
@@ -695,6 +695,7 @@ X = | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the Git mailing list, Junio C Hamano wrote (reply to this):
"Heba Waly via GitGitGadget" <[email protected]> writes:
> +static struct {
> + const char *key;
> + int enabled;
> +} advice_setting[] = {
> + [ADVICE_ADD_EMBEDDED_REPO] = { "addEmbeddedRepo", 1 },
It would be nicer to future developers to flip the polarity, as we
do not have to write 1 all over the place, especially if we plan to
extend the structure over time and to use designated initializers
for only certain fields:
static struct {
const char *key;
int disabled;
} advice_setting[] = {
[ADDVICE_ADD_EMBEDDED_REPO] = { .key = "addEmbeddedRepo" },
> @@ -149,6 +218,13 @@ int git_default_advice_config(const char *var, const char *value)
> if (strcasecmp(k, advice_config[i].name))
> continue;
> *advice_config[i].preference = git_config_bool(var, value);
> + break;
> + }
> +
> + for (i = 0; i < ARRAY_SIZE(advice_setting); i++) {
> + if (strcasecmp(k, advice_setting[i].key))
> + continue;
> + advice_setting[i].enabled = git_config_bool(var, value);
> return 0;
Turning this into "break;" would make it similar to the loop before
this one, and will allow other people to add more code after this
loop later.
> +int cmd__advise_if_enabled(int argc, const char **argv)
> +{
> + if (!argv[1])
> + die("usage: %s <advice>", argv[0]);
> +
> + setup_git_directory();
> + git_config(git_default_config, NULL);
> +
> + /*
> + Any advice type can be used for testing, but NESTED_TAG was selected
> + here and in t0018 where this command is being executed.
> + */
Style (will fix up locally).
Thanks. I think this is reasonable with or without the suggested
fixes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the Git mailing list, Junio C Hamano wrote (reply to this):
Junio C Hamano <[email protected]> writes:
> "Heba Waly via GitGitGadget" <[email protected]> writes:
>
>> +static struct {
>> + const char *key;
>> + int enabled;
>> +} advice_setting[] = {
>> + [ADVICE_ADD_EMBEDDED_REPO] = { "addEmbeddedRepo", 1 },
>
> It would be nicer to future developers to flip the polarity, as we
> do not have to write 1 all over the place, especially if we plan to
> extend the structure over time and to use designated initializers
> for only certain fields...
Just to avoid needless churn, I think this does not matter in the
longer term, so .enabled is OK as-is. The reason I say so is
because, even though renaming to .disabled to allow initializers to
default it to 0 is nicer for those who write the initializers
manually, and it especially is true when we have more fields in the
struct (we may add descriptive text so that we can issue an on-line
help, for example), but I expect that would happen much later than
we start generating these parts of the source code in two places
(the initializer for advice_setting[] and the advice_type enum) from
a single source by mechanical process. And the auto-generation will
eliminate the burden of writing 1 manually.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the Git mailing list, Heba Waly wrote (reply to this):
On Wed, Mar 4, 2020 at 3:16 AM Junio C Hamano <[email protected]> wrote:
>
> Junio C Hamano <[email protected]> writes:
>
> > "Heba Waly via GitGitGadget" <[email protected]> writes:
> >
> >> +static struct {
> >> + const char *key;
> >> + int enabled;
> >> +} advice_setting[] = {
> >> + [ADVICE_ADD_EMBEDDED_REPO] = { "addEmbeddedRepo", 1 },
> >
> > It would be nicer to future developers to flip the polarity, as we
> > do not have to write 1 all over the place, especially if we plan to
> > extend the structure over time and to use designated initializers
> > for only certain fields...
>
> Just to avoid needless churn, I think this does not matter in the
> longer term, so .enabled is OK as-is. The reason I say so is
> because, even though renaming to .disabled to allow initializers to
> default it to 0 is nicer for those who write the initializers
> manually, and it especially is true when we have more fields in the
> struct (we may add descriptive text so that we can issue an on-line
> help, for example), but I expect that would happen much later than
> we start generating these parts of the source code in two places
> (the initializer for advice_setting[] and the advice_type enum) from
> a single source by mechanical process. And the auto-generation will
> eliminate the burden of writing 1 manually.
Agree.
> Turning this into "break;" would make it similar to the loop before
> this one, and will allow other people to add more code after this
> loop later.
The loop with break is redundant and will be removed in the next
patch, but the rest of the loops in this function end with return
which I think makes more sense assuming any new code/loop that will
need to be added in the future is expected to handle a different
configuration group.
> > + /*
> > + Any advice type can be used for testing, but NESTED_TAG was selected
> > + here and in t0018 where this command is being executed.
> > + */
>
> Style (will fix up locally).
>
> Thanks. I think this is reasonable with or without the suggested
> fixes.
Great, thanks.
Heba
This patch series was integrated into pu via git@20bc48f. |
This patch series was integrated into pu via git@f084d93. |
This patch series was integrated into pu via git@0ae1e05. |
This patch series was integrated into pu via git@b90e6e8. |
This patch series was integrated into pu via git@45f98be. |
This patch series was integrated into pu via git@eb833f4. |
This patch series was integrated into next via git@ea9e5a1. |
This patch series was integrated into pu via git@6159684. |
This patch series was integrated into pu via git@c332b88. |
This patch series was integrated into pu via git@1e6d758. |
This patch series was integrated into pu via git@0bd68b2. |
This patch series was integrated into pu via git@f30f916. |
This patch series was integrated into pu via git@ef65f35. |
This patch series was integrated into pu via git@51b547d. |
This patch series was integrated into pu via git@b65dd7e. |
This patch series was integrated into pu via git@0d75da2. |
This patch series was integrated into pu via git@820bc22. |
This patch series was integrated into pu via git@951f464. |
This patch series was integrated into pu via git@7297bb2. |
This patch series was integrated into pu via git@d625c35. |
This patch series was integrated into pu via git@953f4ff. |
This patch series was integrated into pu via git@c4a09cc. |
This patch series was integrated into next via git@c4a09cc. |
This patch series was integrated into master via git@c4a09cc. |
Closed via c4a09cc. |
V7:
the switch case in advice_enabled(), it is kept because
it'll be needed when handling other special cases e.g:
ADVICE_ GRAFT_FILE_DEPRECATED.
In V6:
Main changes in V4:
Changes in V3:
Main changes in V2:
The advice API is currently a little bit confusing to call.
quoting from [1]:
A new approach was suggested in [1] which this patch is based upon.
A new advise_if_enabled() is introduced to gradually replace advise()
advice_enabled() helper is also introduced to be used by those callers who:
if(advice_enabled(advice_type))
advise("some advice message");
To introduce a new advice message, the caller needs to:
advice_setting[]
Documentation/config/advice.txt
The reason a new list of configuration variables was added to the library is to be used by the
list_config_advices()
function instead ofadvice_config[]
.And we should get rid of
advice_config[]
once we migrate all the callers to use the new APIs instead of checking the global variables (which we'll get rid of as well).In the future, we can investigate generating the documentation from the list of config variables or vice versa to make introducing a new advice much easier, but this approach will do it for now.
V2 makes the process of introducing a new advice longer than V1 and almost as long as the original library, but having the advice library responsible for checking the message visibility is still an improvement and in my own opinion the new structure makes better sense and makes the library less confusing to use.
After this patch the plan is to change the advise() calls to advise_if_enabled() whenever possible, or at least replace the global variables checks by advise_enabled() when advise_if_enabled() is not suitable.
[1] https://public-inbox.org/git/[email protected]/