{"thread":{"id":"49622","subject":"[PATCH] builtin/receive-pack: dead initializer for retval in check_nonce","startedAt":"2018-10-20T07:09:29Z","lastAt":"2018-10-22T03:35:42Z","messageCount":4,"participants":["Carlo Marcelo Arenas Belón","Torsten Bögershausen","Carlo Arenas","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"360999","messageId":"20181020070859.48172-1-carenas@gmail.com","threadId":"49622","inReplyTo":null,"subject":"[PATCH] builtin/receive-pack: dead initializer for retval in check_nonce","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2018-10-20T07:08:59Z","receivedAt":"2018-10-20T07:09:29Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"NONCE_BAD is explicitly set when needed with the fallback\ninstead as NONCE_SLOP\n\nSigned-off-by: Carlo Marcelo Arenas Belón \n---\n builtin/receive-pack.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 95740f4f0e..ecce3d4043 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -507,7 +507,7 @@ static const char *check_nonce(const char *buf, size_t len)\n \tchar *nonce = find_header(buf, len, \"nonce\", NULL);\n \ttimestamp_t stamp, ostamp;\n \tchar *bohmac, *expect = NULL;\n-\tconst char *retval = NONCE_BAD;\n+\tconst char *retval;\n \n \tif (!nonce) {\n \t\tretval = NONCE_MISSING;\n-- \n2.19.1\n\n"},{"id":"361065","messageId":"20181020164526.GA1077@tor.lan","threadId":"49622","inReplyTo":"20181020070859.48172-1-carenas@gmail.com","subject":"Re: [PATCH] builtin/receive-pack: dead initializer for retval in check_nonce","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2018-10-20T16:45:26Z","receivedAt":"2018-10-20T16:53:30Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On Sat, Oct 20, 2018 at 12:08:59AM -0700, Carlo Marcelo Arenas Belón wrote:\n> NONCE_BAD is explicitly set when needed with the fallback\n> instead as NONCE_SLOP\n> \n> Signed-off-by: Carlo Marcelo Arenas Belón \n> ---\n> builtin/receive-pack.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n> \n> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\n> index 95740f4f0e..ecce3d4043 100644\n> --- a/builtin/receive-pack.c\n> +++ b/builtin/receive-pack.c\n> @@ -507,7 +507,7 @@ static const char *check_nonce(const char *buf, size_t len)\n> \tchar *nonce = find_header(buf, len, \"nonce\", NULL);\n> \ttimestamp_t stamp, ostamp;\n> \tchar *bohmac, *expect = NULL;\n> -\tconst char *retval = NONCE_BAD;\n> +\tconst char *retval;\n> \n> \tif (!nonce) {\n> \t\tretval = NONCE_MISSING;\n\n\nThanks for the patch.\nThe motivation feels a little bit weak, at least to me.\nInitializing a variable to \"BAD\" in the beginning can be a good thing\nfor two reasons:\n- There is a complex if-elseif chain, which should set retval\n in any case, this is at least what I expect taking a very quick look at the\n code:\n\tconst char *retval = NONCE_BAD;\n\n\tif (!nonce) {\n\t\tretval = NONCE_MISSING;\n\t\tgoto leave;\n\t} else if (!push_cert_nonce) {\n\t\tretval = NONCE_UNSOLICITED;\n\t\tgoto leave;\n\t} else if (!strcmp(push_cert_nonce, nonce)) {\n\t\tretval = NONCE_OK;\n\t\tgoto leave;\n\t}\n\t# And here I started to wonder if we should have an else or not.\n\t# Having retval NONCE_BAD set to NONCE:BAD in the beginning makes\n\t# it clear, that we are save without the else.\n\t# As an alternative, we could have coded like this:\n\t\n\tconst char *retval;\n\n\tif (!nonce) {\n\t\tretval = NONCE_MISSING;\n\t\tgoto leave;\n\t} else if (!push_cert_nonce) {\n\t\tretval = NONCE_UNSOLICITED;\n\t\tgoto leave;\n\t} else if (!strcmp(push_cert_nonce, nonce)) {\n\t\tretval = NONCE_OK;\n\t\tgoto leave;\n\t} else {\n\t\t/* Set to BAD, until we know better further down */\n\t\tretval = NONCE_BAD;\n\t}\n\n# The second reason is that some compilers don't understand this complex\n# stuff either, and through out a warning, like\n# \"retval may be uninitialized\" or something in that style.\n# This is very compiler dependent.\n\nSo yes, the current code may seem to be over-eager and ask for optimization,\nbut we don't gain more that a couple of nano-seconds or so.\nThe good thing is that we have the code a little bit more robust, when changes are done\nin the future.\n\n\n\n\n"},{"id":"361087","messageId":"CAPUEspjprhMSCz+tJEUhV8PbmGSuBEi71OCeq8aF=dh23_Zs-g@mail.gmail.com","threadId":"49622","inReplyTo":"20181020164526.GA1077@tor.lan","subject":"Re: [PATCH] builtin/receive-pack: dead initializer for retval in check_nonce","fromName":"Carlo Arenas","fromEmail":"carenas@gmail.com","sentAt":"2018-10-21T10:00:43Z","receivedAt":"2018-10-21T10:01:00Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Sat, Oct 20, 2018 at 9:45 AM Torsten Bögershausen wrote:\n>\n> The motivation feels a little bit weak, at least to me.\n\nI have to admit, I was sitting on this patch for a while for the same reason\nbut I should had made a more compelling commit message either way and\nwill definitely fix that with v2.\n\nthe point was that setting the variable to BAD originally seemed to be legacy\nfrom the original version of the code, specially considering that the newer\nversion was setting it to SLOB at the last \"else\".\n\nthe code was written in a way that would make all those assignments to BAD\nexplicit (even if it wasn't needed, since it was initialized to that\nvalue) and so\nit would seem better IMHO to use the compiler to remind us that this variable\nMUST be set to the right value than setting the most likely value by default.\n\n> Initializing a variable to \"BAD\" in the beginning can be a good thing\n> for two reasons:\n> - There is a complex if-elseif chain, which should set retval\n> in any case, this is at least what I expect taking a very quick look at the\n> code:\n> const char *retval = NONCE_BAD;\n>\n> if (!nonce) {\n> retval = NONCE_MISSING;\n> goto leave;\n> } else if (!push_cert_nonce) {\n> retval = NONCE_UNSOLICITED;\n> goto leave;\n> } else if (!strcmp(push_cert_nonce, nonce)) {\n> retval = NONCE_OK;\n> goto leave;\n> }\n> # And here I started to wonder if we should have an else or not.\n> # Having retval NONCE_BAD set to NONCE:BAD in the beginning makes\n> # it clear, that we are save without the else.\n> # As an alternative, we could have coded like this:\n>\n> const char *retval;\n>\n> if (!nonce) {\n> retval = NONCE_MISSING;\n> goto leave;\n> } else if (!push_cert_nonce) {\n> retval = NONCE_UNSOLICITED;\n> goto leave;\n> } else if (!strcmp(push_cert_nonce, nonce)) {\n> retval = NONCE_OK;\n> goto leave;\n> } else {\n> /* Set to BAD, until we know better further down */\n> retval = NONCE_BAD;\n> }\n>\n> # The second reason is that some compilers don't understand this complex\n> # stuff either, and through out a warning, like\n> # \"retval may be uninitialized\" or something in that style.\n> # This is very compiler dependent.\n\nFWIW I did test with gcc (from 4.9 to 8) and clang 7 (linux) and 10\n(macos) and didn't\ntrigger any warning.\n\n> So yes, the current code may seem to be over-eager and ask for optimization,\n> but we don't gain more that a couple of nano-seconds or so.\n> The good thing is that we have the code a little bit more robust, when changes are done\n> in the future.\n\non the other hand was able to trigger a warning if the code was changed so some\npath will leave retval uninitialized (after adding\n-Wmaybe-uninitialized to gcc and -Wsometimes-uninitialized to clang)\nsince there was no longer a default of BAD (probably incorrectly) that\nwould be returned in case setting retval to the\nright value was forgotten.\n\nCarlo\n"},{"id":"361128","messageId":"xmqqzhv6wt4n.fsf@gitster-ct.c.googlers.com","threadId":"49622","inReplyTo":"20181020164526.GA1077@tor.lan","subject":"Re: [PATCH] builtin/receive-pack: dead initializer for retval in check_nonce","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-10-22T03:35:36Z","receivedAt":"2018-10-22T03:35:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Torsten Bögershausen writes:\n\n> Initializing a variable to \"BAD\" in the beginning can be a good thing\n> for two reasons:\n> - There is a complex if-elseif chain, which should set retval\n> in any case, this is at least what I expect taking a very quick look at the\n> code:\n> ...\n> # The second reason is that some compilers don't understand this complex\n> # stuff either, and through out a warning, like\n> # \"retval may be uninitialized\" or something in that style.\n> # This is very compiler dependent.\n\nAnd to help humans that unless some if/else chain explicitly says it\nis OK, the caller receives BAD by default. In other words, it is\nbeing defensive.\n\nAt least that was the reasoning behind the original code that did\nnot support SLOP.\n\n> So yes, the current code may seem to be over-eager and ask for\n> optimization, but we don't gain more that a couple of nano-seconds\n> or so. The good thing is that we have the code a little bit more\n> robust, when changes are done in the future.\n\nTrue.\n\n"}]}