{"thread":{"id":"18300","subject":"[PATCH] Remove unused assignments","startedAt":"2009-03-13T12:51:33Z","lastAt":"2009-03-14T20:57:13Z","messageCount":3,"participants":["Benjamin Kramer","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"107940","messageId":"49BA56D5.5050807@googlemail.com","threadId":"18300","inReplyTo":null,"subject":"[PATCH] Remove unused assignments","fromName":"Benjamin Kramer","fromEmail":"benny.kra@googlemail.com","sentAt":"2009-03-13T12:51:33Z","receivedAt":"2009-03-13T12:51:33Z","isPatch":true,"sender":{"key":"benny.kra@googlemail.com","avatar":"https://avatars.githubusercontent.com/u/16542?v=4"},"body":"These variables were always overwritten or the assigned\nvalue was unused:\n\nbuiltin-diff-tree.c::cmd_diff_tree(): nr_sha1\nbuiltin-for-each-ref.c::opt_parse_sort(): sort_tail\nbuiltin-mailinfo.c::decode_header_bq(): in\nbuiltin-shortlog.c::insert_one_record(): len\nconnect.c::git_connect(): path\nimap-send.c::v_issue_imap_cmd(): n\npretty.c::pp_user_info(): filler\nremote::parse_refspec_internal(): llen\n\nSigned-off-by: Benjamin Kramer <benny.kra@googlemail.com>\n---\n builtin-diff-tree.c    |    1 -\n builtin-for-each-ref.c |    1 -\n builtin-mailinfo.c     |    1 -\n builtin-shortlog.c     |    1 -\n connect.c              |    2 +-\n imap-send.c            |    2 +-\n pretty.c               |    1 -\n remote.c               |    2 +-\n 8 files changed, 3 insertions(+), 8 deletions(-)\n\ndiff --git a/builtin-diff-tree.c b/builtin-diff-tree.c\nindex 8ecefd4..79cedb7 100644\n--- a/builtin-diff-tree.c\n+++ b/builtin-diff-tree.c\n@@ -102,7 +102,6 @@ int cmd_diff_tree(int argc, const char **argv, const char *prefix)\n \n \tinit_revisions(opt, prefix);\n \tgit_config(git_diff_basic_config, NULL); /* no \"diff\" UI options */\n-\tnr_sha1 = 0;\n \topt->abbrev = 0;\n \topt->diff = 1;\n \targc = setup_revisions(argc, argv, opt, NULL);\ndiff --git a/builtin-for-each-ref.c b/builtin-for-each-ref.c\nindex e46b7ad..5cbb4b0 100644\n--- a/builtin-for-each-ref.c\n+++ b/builtin-for-each-ref.c\n@@ -943,7 +943,6 @@ static int opt_parse_sort(const struct option *opt, const char *arg, int unset)\n \t\treturn -1;\n \n \t*sort_tail = s = xcalloc(1, sizeof(*s));\n-\tsort_tail = &s->next;\n \n \tif (*arg == '-') {\n \t\ts->reverse = 1;\ndiff --git a/builtin-mailinfo.c b/builtin-mailinfo.c\nindex 2789ccd..1eeeb4d 100644\n--- a/builtin-mailinfo.c\n+++ b/builtin-mailinfo.c\n@@ -537,7 +537,6 @@ static int decode_header_bq(struct strbuf *it)\n \t\t\t\t */\n \t\t\t\tstrbuf_add(&outbuf, in, ep - in);\n \t\t\t}\n-\t\t\tin = ep;\n \t\t}\n \t\t/* E.g.\n \t\t * ep : \"=?iso-2022-jp?B?GyR...?= foo\"\ndiff --git a/builtin-shortlog.c b/builtin-shortlog.c\nindex badd912..b28091b 100644\n--- a/builtin-shortlog.c\n+++ b/builtin-shortlog.c\n@@ -101,7 +101,6 @@ static void insert_one_record(struct shortlog *log,\n \t}\n \twhile (*oneline && isspace(*oneline) && *oneline != '\\n')\n \t\toneline++;\n-\tlen = eol - oneline;\n \tformat_subject(&subject, oneline, \" \");\n \tbuffer = strbuf_detach(&subject, NULL);\n \ndiff --git a/connect.c b/connect.c\nindex 0a35cc1..7636bf9 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -504,7 +504,7 @@ struct child_process *git_connect(int fd[2], const char *url_orig,\n \t\t\t\t  const char *prog, int flags)\n {\n \tchar *url = xstrdup(url_orig);\n-\tchar *host, *path = url;\n+\tchar *host, *path;\n \tchar *end;\n \tint c;\n \tstruct child_process *conn;\ndiff --git a/imap-send.c b/imap-send.c\nindex cb518eb..8154cb2 100644\n--- a/imap-send.c\n+++ b/imap-send.c\n@@ -579,7 +579,7 @@ static struct imap_cmd *v_issue_imap_cmd(struct imap_store *ctx,\n \t\t\tn = socket_write(&imap->buf.sock, cmd->cb.data, cmd->cb.dlen);\n \t\t\tfree(cmd->cb.data);\n \t\t\tif (n != cmd->cb.dlen ||\n-\t\t\t    (n = socket_write(&imap->buf.sock, \"\\r\\n\", 2)) != 2) {\n+\t\t\t    socket_write(&imap->buf.sock, \"\\r\\n\", 2) != 2) {\n \t\t\t\tfree(cmd->cmd);\n \t\t\t\tfree(cmd);\n \t\t\t\treturn NULL;\ndiff --git a/pretty.c b/pretty.c\nindex c018408..3a24cd5 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -154,7 +154,6 @@ void pp_user_info(const char *what, enum cmit_fmt fmt, struct strbuf *sb,\n \t\twhile (line < name_tail && isspace(name_tail[-1]))\n \t\t\tname_tail--;\n \t\tdisplay_name_length = name_tail - line;\n-\t\tfiller = \"\";\n \t\tstrbuf_addstr(sb, \"From: \");\n \t\tadd_rfc2047(sb, line, display_name_length, encoding);\n \t\tstrbuf_add(sb, name_tail, namelen - display_name_length);\ndiff --git a/remote.c b/remote.c\nindex d7079c6..7efaa02 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -495,7 +495,7 @@ static struct refspec *parse_refspec_internal(int nr_refspec, const char **refsp\n \t\tint is_glob;\n \t\tconst char *lhs, *rhs;\n \n-\t\tllen = is_glob = 0;\n+\t\tis_glob = 0;\n \n \t\tlhs = refspec[i];\n \t\tif (*lhs == '+') {\n-- \n1.6.2.169.g92418\n"},{"id":"108017","messageId":"7v7i2rc0zp.fsf@gitster.siamese.dyndns.org","threadId":"18300","inReplyTo":"49BA56D5.5050807@googlemail.com","subject":"Re: [PATCH] Remove unused assignments","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-03-14T20:18:02Z","receivedAt":"2009-03-14T20:18:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Benjamin Kramer <benny.kra@googlemail.com> writes:\n\n> These variables were always overwritten or the assigned\n> value was unused:\n>\n> builtin-diff-tree.c::cmd_diff_tree(): nr_sha1\n> builtin-for-each-ref.c::opt_parse_sort(): sort_tail\n> builtin-mailinfo.c::decode_header_bq(): in\n> builtin-shortlog.c::insert_one_record(): len\n> connect.c::git_connect(): path\n> imap-send.c::v_issue_imap_cmd(): n\n> pretty.c::pp_user_info(): filler\n> remote::parse_refspec_internal(): llen\n>\n> Signed-off-by: Benjamin Kramer <benny.kra@googlemail.com>\n\nThanks.  I eyeballed all of them and they look safe, but this patch made\nme wonder...\n\nDid you use some dataflow analysis tool to spot these?\n\nIt will never scale if a human has to sanity check output from a\nmechanical process like this patch, especially when the human is already a\nchokepoint of the whole process (i.e. the maintainer).\n\nWe'd need some process improvements in the longer term to handle patches\nlike this, but I do not think of a good one offhand.\n\nI do not think we want to trust tool output blindly; it will not solve the\nissue to give me the recipe for the mechanical process you used, to have\nme run it and to make me trust the result without checking.  We would want\nto keep some human whose judgement we can trust in the loop to always\ncheck the changes mechanical analysis tools may produce.\n\nI think the only viable long term solution would be to keep doing the\nmanual verification I did like this round until somebody like you\naccumulate enough \"trust points\" in the community for consistently sending\ngood patches like this one to allow me to apply patches from these people\nblindly, trusting the sender's judgement.\n\nBut please disregard all of the above if you spotted these by manual\ninspection.\n\nI think the patch to pp_user_info() can go one step further.  The filler\nis used at only one place after this patch, as a strong given to one of\nthe placeholders in strbuf_addf().  We can simply feed a constant to the\nfunction instead, like this:\n\ndiff --git a/pretty.c b/pretty.c\nindex 3a24cd5..efa7024 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -135,7 +135,6 @@ void pp_user_info(const char *what, enum cmit_fmt fmt, struct strbuf *sb,\n \tint namelen;\n \tunsigned long time;\n \tint tz;\n-\tconst char *filler = \"    \";\n \n \tif (fmt == CMIT_FMT_ONELINE)\n \t\treturn;\n@@ -161,7 +160,7 @@ void pp_user_info(const char *what, enum cmit_fmt fmt, struct strbuf *sb,\n \t} else {\n \t\tstrbuf_addf(sb, \"%s: %.*s%.*s\\n\", what,\n \t\t\t      (fmt == CMIT_FMT_FULLER) ? 4 : 0,\n-\t\t\t      filler, namelen, line);\n+\t\t\t      \"    \", namelen, line);\n \t}\n \tswitch (fmt) {\n \tcase CMIT_FMT_MEDIUM:\n"},{"id":"108025","messageId":"49BC1A29.60503@googlemail.com","threadId":"18300","inReplyTo":"7v7i2rc0zp.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] Remove unused assignments","fromName":"Benjamin Kramer","fromEmail":"benny.kra@googlemail.com","sentAt":"2009-03-14T20:57:13Z","receivedAt":"2009-03-14T20:57:13Z","isPatch":true,"sender":{"key":"benny.kra@googlemail.com","avatar":"https://avatars.githubusercontent.com/u/16542?v=4"},"body":"Junio C Hamano wrote:\n> \n> Thanks.  I eyeballed all of them and they look safe, but this patch made\n> me wonder...\n> \n> Did you use some dataflow analysis tool to spot these?\n> \n> It will never scale if a human has to sanity check output from a\n> mechanical process like this patch, especially when the human is already a\n> chokepoint of the whole process (i.e. the maintainer).\n\nYep, they were found with a little help of the clang static analyzer\n\nhttp://clang.llvm.org/StaticAnalysis.html\n\nIt is in early stages of development so it may report false positives and\nit chokes on some files. Here is the latest output I have, with my patch\napplied:\n\nhttp://doktorz.mooltied.de/stuff/scan-build-2009-03-13-2/\n\nI've looked briefly at the \"Logic Errors\" and they all seem to be false\npositives. I did not have enough time to look into all the remaining \"Dead\nStores\" though.\n\n-- Benjamin\n"}]}