{"thread":{"id":"27445","subject":"Simple dead assignment","startedAt":"2011-05-24T21:07:58Z","lastAt":"2011-05-25T19:45:05Z","messageCount":10,"participants":["Chris Wilson","Matthieu Moy","Michael Schubert","Junio C Hamano","Jeff King"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"168597","messageId":"20110524210758.GH16052@localhost","threadId":"27445","inReplyTo":null,"subject":"Simple dead assignment","fromName":"Chris Wilson","fromEmail":"cwilson@vigilantsw.com","sentAt":"2011-05-24T21:07:58Z","receivedAt":"2011-05-24T21:07:58Z","isPatch":false,"sender":{"key":"cwilson@vigilantsw.com","avatar":null},"body":"Hi Folks,\n\nSentry picked up this dead assignment commited yesterday in ba67aaf.\nI've provided a patch to remove it. It might also be a good idea to\nask the author if that value was supposed to be used for something\nin particular before pulling it out.\n\nThanks,\nChris\n\n-- \nChris Wilson\nhttp://vigilantsw.com/\nVigilant Software, LLC\n\n\ndiff --git a/sh-i18n--envsubst.c b/sh-i18n--envsubst.c\nindex 7125093..5829463 100644\n--- a/sh-i18n--envsubst.c\n+++ b/sh-i18n--envsubst.c\n@@ -67,9 +67,6 @@ static void subst_from_stdin (void);\n int\n main (int argc, char *argv[])\n {\n-  /* Default values for command line options.  */\n-  unsigned short int show_variables = 0;\n-\n   switch (argc)\n \t{\n \tcase 1:\n@@ -88,7 +85,6 @@ main (int argc, char *argv[])\n \t  /* git sh-i18n--envsubst --variables '$foo and $bar' */\n \t  if (strcmp(argv[1], \"--variables\"))\n \t\terror (\"first argument must be --variables when two are given\");\n-\t  show_variables = 1;\n       print_variables (argv[2]);\n \t  break;\n \tdefault:\n"},{"id":"168607","messageId":"20110524224525.GI16052@localhost","threadId":"27445","inReplyTo":"20110524210758.GH16052@localhost","subject":"[PATCH] Simple dead assignment","fromName":"Chris Wilson","fromEmail":"cwilson@vigilantsw.com","sentAt":"2011-05-24T22:45:25Z","receivedAt":"2011-05-24T22:45:25Z","isPatch":true,"sender":{"key":"cwilson@vigilantsw.com","avatar":null},"body":"On Tue, May 24, 2011 at 05:07:58PM -0400, Chris Wilson wrote:\n> Sentry picked up this dead assignment commited yesterday in ba67aaf.\n> I've provided a patch to remove it. It might also be a good idea to\n> ask the author if that value was supposed to be used for something\n> in particular before pulling it out.\n\nOops, I see others putting the patches inline. Here you go.\n\nChris\n\n-- \nChris Wilson\nhttp://vigilantsw.com/\nVigilant Software, LLC\n\ndiff --git a/sh-i18n--envsubst.c b/sh-i18n--envsubst.c\nindex 7125093..5829463 100644\n--- a/sh-i18n--envsubst.c\n+++ b/sh-i18n--envsubst.c\n@@ -67,9 +67,6 @@ static void subst_from_stdin (void);\n int\n main (int argc, char *argv[])\n {\n-  /* Default values for command line options.  */\n-  unsigned short int show_variables = 0;\n-\n   switch (argc)\n \t{\n \tcase 1:\n@@ -88,7 +85,6 @@ main (int argc, char *argv[])\n \t  /* git sh-i18n--envsubst --variables '$foo and $bar' */\n \t  if (strcmp(argv[1], \"--variables\"))\n \t\terror (\"first argument must be --variables when two are given\");\n-\t  show_variables = 1;\n       print_variables (argv[2]);\n \t  break;\n \tdefault:\n"},{"id":"168630","messageId":"vpqfwo3ush3.fsf@bauges.imag.fr","threadId":"27445","inReplyTo":"20110524224525.GI16052@localhost","subject":"Re: [PATCH] Simple dead assignment","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2011-05-25T07:52:56Z","receivedAt":"2011-05-25T07:52:56Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Chris Wilson <cwilson@vigilantsw.com> writes:\n\n> Oops, I see others putting the patches inline. Here you go.\n\nPlease, read Documentation/SubmittingPatches. Especially read about\nsigned-off-by and the way patches should be formatted (git send-email\nwould help).\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"168664","messageId":"20110525150631.GA29161@localhost","threadId":"27445","inReplyTo":"vpqfwo3ush3.fsf@bauges.imag.fr","subject":"[PATCH] Remove a dead assignment","fromName":"Chris Wilson","fromEmail":"cwilson@vigilantsw.com","sentAt":"2011-05-25T15:06:32Z","receivedAt":"2011-05-25T15:06:32Z","isPatch":true,"sender":{"key":"cwilson@vigilantsw.com","avatar":null},"body":"On Wed, May 25, 2011 at 09:52:56AM +0200, Matthieu Moy wrote:\n> Chris Wilson <cwilson@vigilantsw.com> writes:\n> \n> > Oops, I see others putting the patches inline. Here you go.\n> \n> Please, read Documentation/SubmittingPatches. Especially read about\n> signed-off-by and the way patches should be formatted (git send-email\n> would help).\n\nThanks, trying this again. Like I said before, the author should\ninvestigate if this variable should have been used before removing it.\n\nSigned-off-by: Chris Wilson <cwilson@vigilantsw.com>\n---\n sh-i18n--envsubst.c |    4 ----\n 1 files changed, 0 insertions(+), 4 deletions(-)\n\ndiff --git a/sh-i18n--envsubst.c b/sh-i18n--envsubst.c\nindex 7125093..5829463 100644\n--- a/sh-i18n--envsubst.c\n+++ b/sh-i18n--envsubst.c\n@@ -67,9 +67,6 @@ static void subst_from_stdin (void);\n int\n main (int argc, char *argv[])\n {\n-  /* Default values for command line options.  */\n-  unsigned short int show_variables = 0;\n-\n   switch (argc)\n        {\n        case 1:\n@@ -88,7 +85,6 @@ main (int argc, char *argv[])\n          /* git sh-i18n--envsubst --variables '$foo and $bar' */\n          if (strcmp(argv[1], \"--variables\"))\n                error (\"first argument must be --variables when two are given\");\n-         show_variables = 1;\n       print_variables (argv[2]);\n          break;\n        default:\n--\n1.7.5.2.354.g19aea\n"},{"id":"168673","messageId":"vpqwrhevkxz.fsf@bauges.imag.fr","threadId":"27445","inReplyTo":"20110525150631.GA29161@localhost","subject":"Re: [PATCH] Remove a dead assignment","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2011-05-25T15:50:16Z","receivedAt":"2011-05-25T15:50:16Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Chris Wilson <cwilson@vigilantsw.com> writes:\n\n> On Wed, May 25, 2011 at 09:52:56AM +0200, Matthieu Moy wrote:\n>> Chris Wilson <cwilson@vigilantsw.com> writes:\n>> \n>> > Oops, I see others putting the patches inline. Here you go.\n>> \n>> Please, read Documentation/SubmittingPatches. Especially read about\n>> signed-off-by and the way patches should be formatted (git send-email\n>> would help).\n>\n> Thanks, trying this again.\n\nStill not right ;-). The text here (above ---) will become the commit\nmessage when applied, and you don't want the commit message to mention\nthis discussion. This discussion could have been added below ...\n\n> Like I said before, the author should investigate if this variable\n> should have been used before removing it.\n>\n> Signed-off-by: Chris Wilson <cwilson@vigilantsw.com>\n> ---\n\n=> this is the place for informal discussions.\n\n>  sh-i18n--envsubst.c |    4 ----\n>  1 files changed, 0 insertions(+), 4 deletions(-)\n\nIf you don't follow this, then the maintainer has to do this manually,\nand we all prefer his time to be invested in better activities ;-).\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"168687","messageId":"4DDD3A01.6040407@elegosoft.com","threadId":"27445","inReplyTo":"20110525150631.GA29161@localhost","subject":"Re: [PATCH] Remove a dead assignment","fromName":"Michael Schubert","fromEmail":"mschub@elegosoft.com","sentAt":"2011-05-25T17:18:57Z","receivedAt":"2011-05-25T17:18:57Z","isPatch":true,"sender":{"key":"mschub@elegosoft.com","avatar":null},"body":"There already is a patch on its way:\n\nhttp://article.gmane.org/gmane.comp.version-control.git/174378\n"},{"id":"168700","messageId":"20110525184514.GA20005@localhost","threadId":"27445","inReplyTo":"4DDD3A01.6040407@elegosoft.com","subject":"[PATCH] Remove a dead assignment","fromName":"Chris Wilson","fromEmail":"cwilson@vigilantsw.com","sentAt":"2011-05-25T18:45:14Z","receivedAt":"2011-05-25T18:45:14Z","isPatch":true,"sender":{"key":"cwilson@vigilantsw.com","avatar":null},"body":"The assignment to fmt is dead and is also useless.\n\nSigned-off-by: Chris Wilson <cwilson@vigilantsw.com>\n---\n\nOn Wed, May 25, 2011 at 07:18:57PM +0200, Michael Schubert wrote:\n> There already is a patch on its way:\n> \n> http://article.gmane.org/gmane.comp.version-control.git/174378\n\nThanks! Well, I wasn't going to report this dead assignment since\nit wasn't done recently, but now I want to figure out how to properly\nsubmit a patch. :) Am I there yet? and thanks for the help.\n\nThanks,\nCHris\n\n pretty.c |    1 -\n 1 files changed, 0 insertions(+), 1 deletions(-)\n\ndiff --git a/pretty.c b/pretty.c\nindex dff5c8d..5667c7f 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -1082,7 +1082,6 @@ void userformat_find_requirements(const char *fmt, struct userformat_\n        if (!fmt) {\n                if (!user_format)\n                        return;\n-               fmt = user_format;\n        }\n        strbuf_expand(&dummy, user_format, userformat_want_item, w);\n        strbuf_release(&dummy);\n-- \n1.7.5.2.354.g19aea\n"},{"id":"168702","messageId":"7v4o4i4mte.fsf@alter.siamese.dyndns.org","threadId":"27445","inReplyTo":"20110525184514.GA20005@localhost","subject":"Re: [PATCH] Remove a dead assignment","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-05-25T19:11:57Z","receivedAt":"2011-05-25T19:11:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Chris Wilson <cwilson@vigilantsw.com> writes:\n\n> Thanks! Well, I wasn't going to report this dead assignment since\n> it wasn't done recently, but now I want to figure out how to properly\n> submit a patch. :) Am I there yet? and thanks for the help.\n\nThe compiler does not understand the meaning of the code, so after seeing\nsuch a \"set but unused\" statement, you should wonder why such a seemingly\nuseless statement is there, before sending a mechanical patch to remove it\nwithout thinking things through.\n\n>  pretty.c |    1 -\n>  1 files changed, 0 insertions(+), 1 deletions(-)\n>\n> diff --git a/pretty.c b/pretty.c\n> index dff5c8d..5667c7f 100644\n> --- a/pretty.c\n> +++ b/pretty.c\n> @@ -1082,7 +1082,6 @@ void userformat_find_requirements(const char *fmt, struct userformat_\n>         if (!fmt) {\n>                 if (!user_format)\n>                         return;\n> -               fmt = user_format;\n>         }\n>         strbuf_expand(&dummy, user_format, userformat_want_item, w);\n>         strbuf_release(&dummy);\n\nThe if statement says \"we might be passing NULL in fmt and in that case\nplease fall back to user_format\" to human readers, but the compiler is too\nstupid to infer such an intention, so you have to help it with your brain.\nI have to wonder if the strbuf_expand() should be passing fmt instead of\nuser_format.  \"git blame -L1082,+7 pretty.c\" points at 5b16360 (pretty:\nInitialize notes if %N is used, 2010-04-13).\n\nThe only callsite that is introduced by that patch passes NULL to fmt, so\na better fix might be to do something like this instead.\n\n builtin/log.c |    3 +--\n commit.h      |    2 +-\n pretty.c      |   10 ++++------\n 3 files changed, 6 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex f621990..9e05d46 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -110,8 +110,7 @@ static void cmd_log_init_finish(int argc, const char **argv, const char *prefix,\n \tif (argc > 1)\n \t\tdie(\"unrecognized argument: %s\", argv[1]);\n \n-\tmemset(&w, 0, sizeof(w));\n-\tuserformat_find_requirements(NULL, &w);\n+\tuserformat_find_requirements(&w);\n \n \tif (!rev->show_notes_given && (!rev->pretty_given || w.notes))\n \t\trev->show_notes = 1;\ndiff --git a/commit.h b/commit.h\nindex 3114bd1..b652c22 100644\n--- a/commit.h\n+++ b/commit.h\n@@ -92,7 +92,7 @@ extern char *reencode_commit_message(const struct commit *commit,\n extern void get_commit_format(const char *arg, struct rev_info *);\n extern const char *format_subject(struct strbuf *sb, const char *msg,\n \t\t\t\t  const char *line_separator);\n-extern void userformat_find_requirements(const char *fmt, struct userformat_want *w);\n+extern void userformat_find_requirements(struct userformat_want *w);\n extern void format_commit_message(const struct commit *commit,\n \t\t\t\t  const char *format, struct strbuf *sb,\n \t\t\t\t  const struct pretty_print_context *context);\ndiff --git a/pretty.c b/pretty.c\nindex dff5c8d..ca24925 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -1075,15 +1075,13 @@ static size_t userformat_want_item(struct strbuf *sb, const char *placeholder,\n \treturn 0;\n }\n \n-void userformat_find_requirements(const char *fmt, struct userformat_want *w)\n+void userformat_find_requirements(struct userformat_want *w)\n {\n \tstruct strbuf dummy = STRBUF_INIT;\n \n-\tif (!fmt) {\n-\t\tif (!user_format)\n-\t\t\treturn;\n-\t\tfmt = user_format;\n-\t}\n+\tmemset(w, 0, sizeof(*w));\n+\tif (!user_format)\n+\t\treturn;\n \tstrbuf_expand(&dummy, user_format, userformat_want_item, w);\n \tstrbuf_release(&dummy);\n }\n"},{"id":"168703","messageId":"7vzkma37pb.fsf@alter.siamese.dyndns.org","threadId":"27445","inReplyTo":"7v4o4i4mte.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] Remove a dead assignment","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-05-25T19:23:44Z","receivedAt":"2011-05-25T19:23:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> The if statement says \"we might be passing NULL in fmt and in that case\n> please fall back to user_format\" to human readers, but the compiler is too\n> stupid to infer such an intention, so you have to help it with your brain.\n> I have to wonder if the strbuf_expand() should be passing fmt instead of\n> user_format.  \"git blame -L1082,+7 pretty.c\" points at 5b16360 (pretty:\n> Initialize notes if %N is used, 2010-04-13).\n>\n> The only callsite that is introduced by that patch passes NULL to fmt, so\n> a better fix might be to do something like this instead.\n\nIf somebody cares about the reusability of the code for other callsites\nadded in the future, we could do this instead.\n\nI think this is what Johannes wanted to do from the beginning, and is a\nbetter fix than my previous one to remove the fmt parameter altogether.\n\n-- >8 --\nSubject: userformat_find_requirements(): find requirement for the correct format\n\nThis function was introduced in 5b16360 (pretty: Initialize notes if %N is\nused, 2010-04-13) to check what kind of information the \"log --format=...\"\nuser format string wants. The function can be passed a NULL instead of a\nformat string to ask it to check user_format variable kept by an earlier\ncall to save_user_format().\n\nBut it unconditionally checked user_format and not the string it was\ngiven.  The only caller introduced by the change passes NULL, which\nkept the bug unnoticed, until a new GCC noticed that there is an\nassignment to fmt that is never used.\n\nNoticed-by: Chris Wilson's compiler\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n pretty.c |    2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/pretty.c b/pretty.c\nindex dff5c8d..52174fd 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -1084,7 +1084,7 @@ void userformat_find_requirements(const char *fmt, struct userformat_want *w)\n \t\t\treturn;\n \t\tfmt = user_format;\n \t}\n-\tstrbuf_expand(&dummy, user_format, userformat_want_item, w);\n+\tstrbuf_expand(&dummy, fmt, userformat_want_item, w);\n \tstrbuf_release(&dummy);\n }\n \n"},{"id":"168708","messageId":"20110525194505.GB27260@sigill.intra.peff.net","threadId":"27445","inReplyTo":"7vzkma37pb.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] Remove a dead assignment","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-05-25T19:45:05Z","receivedAt":"2011-05-25T19:45:05Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, May 25, 2011 at 12:23:44PM -0700, Junio C Hamano wrote:\n\n> Junio C Hamano <gitster@pobox.com> writes:\n> \n> > The if statement says \"we might be passing NULL in fmt and in that case\n> > please fall back to user_format\" to human readers, but the compiler is too\n> > stupid to infer such an intention, so you have to help it with your brain.\n> > I have to wonder if the strbuf_expand() should be passing fmt instead of\n> > user_format.  \"git blame -L1082,+7 pretty.c\" points at 5b16360 (pretty:\n> > Initialize notes if %N is used, 2010-04-13).\n> >\n> > The only callsite that is introduced by that patch passes NULL to fmt, so\n> > a better fix might be to do something like this instead.\n> \n> If somebody cares about the reusability of the code for other callsites\n> added in the future, we could do this instead.\n> \n> I think this is what Johannes wanted to do from the beginning, and is a\n> better fix than my previous one to remove the fmt parameter altogether.\n\nActually, I am to blame for this interface, see:\n\n  http://article.gmane.org/gmane.comp.version-control.git/144650\n\nand my response:\n\n  http://article.gmane.org/gmane.comp.version-control.git/144715\n\nThe resulting patch definitely has a bug (albeit one that could not be\ntriggered by current users), and should have had this:\n\n> -\tstrbuf_expand(&dummy, user_format, userformat_want_item, w);\n> +\tstrbuf_expand(&dummy, fmt, userformat_want_item, w);\n\nall along.\n\nI'm also OK with just tightening the interface to always use\nuser_format, as no callers who wanted the extra parameter have come up\nin the past year.\n\n> Subject: userformat_find_requirements(): find requirement for the correct format\n> \n> This function was introduced in 5b16360 (pretty: Initialize notes if %N is\n> used, 2010-04-13) to check what kind of information the \"log --format=...\"\n> user format string wants. The function can be passed a NULL instead of a\n> format string to ask it to check user_format variable kept by an earlier\n> call to save_user_format().\n> \n> But it unconditionally checked user_format and not the string it was\n> given.  The only caller introduced by the change passes NULL, which\n> kept the bug unnoticed, until a new GCC noticed that there is an\n> assignment to fmt that is never used.\n\nAcked-by: Jeff King <peff@peff.net>\n\n> Noticed-by: Chris Wilson's compiler\n\nHeh.\n\n-Peff\n"}]}