{"thread":{"id":"55265","subject":"[PATCH] fix: added new BANNED_EXPL macro for better error messages, new parameter","startedAt":"2021-03-06T00:52:44Z","lastAt":"2021-03-08T18:41:52Z","messageCount":5,"participants":["HG King via GitGitGadget","Junio C Hamano","Taylor Blau","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"418360","messageId":"pull.896.git.1614991897210.gitgitgadget@gmail.com","threadId":"55265","inReplyTo":null,"subject":"[PATCH] fix: added new BANNED_EXPL macro for better error messages, new parameter","fromName":"HG King via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-03-06T00:51:36Z","receivedAt":"2021-03-06T00:52:44Z","isPatch":true,"sender":{"key":"name:HG King","avatar":null},"body":"From: HGimself \n\nSigned-off-by: HGimself \n---\n fix: added new BANNED_EXPL macro for better error messages, has a par…\n \n \n Extend Banned Function Error Messages\n =====================================\n \n AS A NEWER USER, I WANT TO BE ABLE TO UNDERSTAND WHY CERTAIN FUNCTIONS\n ARE BANNED AS WELL AS KNOW WHAT FUNCTIONS TO USE INSTEAD.\n \n \n Changes\n =======\n \n * Added new macro named BANNED_EXPL(func, expl) which added the expl\n onto the error message\n * Used strcpy as an example\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-896%2FHGHimself%2Ffix%2Fadd-new-ban-macro-with-explanation-parameter-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-896/HGHimself/fix/add-new-ban-macro-with-explanation-parameter-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/896\n\n banned.h | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/banned.h b/banned.h\nindex 7ab4f2e49219..a19f0afeda79 100644\n--- a/banned.h\n+++ b/banned.h\n@@ -9,9 +9,10 @@\n */\n \n #define BANNED(func) sorry_##func##_is_a_banned_function\n+#define BANNED_EXPL(func, expl) sorry_##func##_is_a_banned_funcion_because_##expl##.\n \n #undef strcpy\n-#define strcpy(x,y) BANNED(strcpy)\n+#define strcpy(x,y) BANNED_EXPL(strcpy, buffer_overflow_risk)\n #undef strcat\n #define strcat(x,y) BANNED(strcat)\n #undef strncpy\n\nbase-commit: be7935ed8bff19f481b033d0d242c5d5f239ed50\n-- \ngitgitgadget\n"},{"id":"418421","messageId":"xmqq7dmi8zym.fsf@gitster.c.googlers.com","threadId":"55265","inReplyTo":"pull.896.git.1614991897210.gitgitgadget@gmail.com","subject":"Re: [PATCH] fix: added new BANNED_EXPL macro for better error messages, new parameter","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-03-07T20:34:57Z","receivedAt":"2021-03-07T20:35:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"HG King via GitGitGadget\" writes:\n\n> #undef strcpy\n> -#define strcpy(x,y) BANNED(strcpy)\n> +#define strcpy(x,y) BANNED_EXPL(strcpy, buffer_overflow_risk)\n\nThat does not help programmers that much (the above does not say\nwhat to use instead, for example), and the mechanism inherently\ndoes not give you sufficient space to give helpful guidance.\n\nAdding a comment around each of these definition may be OK. Upon\nseeing foo_is_a_banned_function, somebody new to the codebase would\nlook for where it is banned, and find the above, so that is a good\nplace to give guidance.\n"},{"id":"418486","messageId":"YEZnaeJVt8Rk6duv@nand.local","threadId":"55265","inReplyTo":"xmqq7dmi8zym.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] fix: added new BANNED_EXPL macro for better error messages, new parameter","fromName":"Taylor Blau","fromEmail":"ttaylorr@github.com","sentAt":"2021-03-08T18:05:46Z","receivedAt":"2021-03-08T18:06:44Z","isPatch":true,"sender":{"key":"ttaylorr@github.com","avatar":"https://gravatar.com/avatar/d5f3476f26b6f99cbb6b467e7ed7482f5762c8157bc73f569196e428bdcbea25?d=mp&s=160"},"body":"On Sun, Mar 07, 2021 at 12:34:57PM -0800, Junio C Hamano wrote:\n> \"HG King via GitGitGadget\" writes:\n>\n> > #undef strcpy\n> > -#define strcpy(x,y) BANNED(strcpy)\n> > +#define strcpy(x,y) BANNED_EXPL(strcpy, buffer_overflow_risk)\n>\n> That does not help programmers that much (the above does not say\n> what to use instead, for example), and the mechanism inherently\n> does not give you sufficient space to give helpful guidance.\n\nTrying to cram information like \"why is this function unsafe?\" and \"what\nfunction should I use instead?\" seems ill-fitted to trying to a macro\nwhich is supposed to have a field for each.\n\nI'm certainly not opposed to making these banned functions clearer, but\nI do not think that this is the way to do it.\n\n> Adding a comment around each of these definition may be OK. Upon\n> seeing foo_is_a_banned_function, somebody new to the codebase would\n> look for where it is banned, and find the above, so that is a good\n> place to give guidance.\n\nPerhaps, but all of this information is already covered accurately in\nthe patches that introduced each banned function. So I'm not sure that I\neven agree that this information is difficult to discover to begin with,\nbut I may be biased.\n\nThanks,\nTaylor\n"},{"id":"418492","messageId":"YEZtKHS7c19N967O@coredump.intra.peff.net","threadId":"55265","inReplyTo":"YEZnaeJVt8Rk6duv@nand.local","subject":"Re: [PATCH] fix: added new BANNED_EXPL macro for better error messages, new parameter","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-03-08T18:30:00Z","receivedAt":"2021-03-08T18:31:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Mar 08, 2021 at 01:05:46PM -0500, Taylor Blau wrote:\n\n> > Adding a comment around each of these definition may be OK. Upon\n> > seeing foo_is_a_banned_function, somebody new to the codebase would\n> > look for where it is banned, and find the above, so that is a good\n> > place to give guidance.\n> \n> Perhaps, but all of this information is already covered accurately in\n> the patches that introduced each banned function. So I'm not sure that I\n> even agree that this information is difficult to discover to begin with,\n> but I may be biased.\n\nYeah, certainly my intent when I introduced banned.h was that people\nwould get the full reasoning from the commit log. I figured Git\ndevelopers could run \"git log -S\" or \"git blame\".\n\nI'm not opposed to comments if somebody wants to write them, but it's\nnot clear to whether people who are actively writing patches for Git\nhave actually run into this situation and been confused, or if this is\nbikeshedding from the recent posting of banned.h on Hacker News. (Even\nif it is the latter, I am OK taking a patch that adds comments; I just\ndoubt that it's a good use of anybody's time).\n\n-Peff\n"},{"id":"418501","messageId":"xmqqo8ft5vzz.fsf@gitster.c.googlers.com","threadId":"55265","inReplyTo":"YEZnaeJVt8Rk6duv@nand.local","subject":"Re: [PATCH] fix: added new BANNED_EXPL macro for better error messages, new parameter","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-03-08T18:41:04Z","receivedAt":"2021-03-08T18:41:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau writes:\n\n>> Adding a comment around each of these definition may be OK. Upon\n>> seeing foo_is_a_banned_function, somebody new to the codebase would\n>> look for where it is banned, and find the above, so that is a good\n>> place to give guidance.\n>\n> Perhaps, but all of this information is already covered accurately in\n> the patches that introduced each banned function. So I'm not sure that I\n> even agree that this information is difficult to discover to begin with,\n> but I may be biased.\n\nTo help those who are not yet familiar with this codebase but are\nwilling to learn, I tend to agree with you that it is a good idea\nnot to miss opportunities to encourage them to run \"git blame\" to\nfind out about the project history in the parts of the system that\npique their interest. They will make more motivated contributors\nwho are invested in the project, if they stick around.\n\nIt would discourage and filter out \"drive-by\" contributors, though.\n\nSomebody may not yet be interested enough to \"git clone\" us, but\nhappens to have an extract of distro source tarball and enough\nmotivation to try peeking and tweaking around. To such a curious\ndeveloper, the \"blame\" information and log messages with the change\nare not readily available, so there probably is some balance to be\nstruck.\n"}]}