{"thread":{"id":"21065","subject":"[PATCH] Remove various dead assignments and dead increments found by the clang static analyzer","startedAt":"2009-09-26T14:46:00Z","lastAt":"2009-09-27T08:21:17Z","messageCount":17,"participants":["Giuseppe Scrivano","Johannes Schindelin","Sverre Rabbelier","René Scharfe","Jeff King","Reece Dunn","Nicolas Pitre"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"123845","messageId":"87ab0hepcn.fsf@master.homenet","threadId":"21065","inReplyTo":null,"subject":"[PATCH] Remove various dead assignments and dead increments found by the clang static analyzer","fromName":"Giuseppe Scrivano","fromEmail":"gscrivano@gnu.org","sentAt":"2009-09-26T14:46:00Z","receivedAt":"2009-09-26T14:46:00Z","isPatch":true,"sender":{"key":"gscrivano@gnu.org","avatar":"https://avatars.githubusercontent.com/u/67430?v=4"},"body":"Hello,\n\nI tried the clang static analyzer on the git source code, this patch\nfixes the found dead assignments/increments.\n\n\nRegards,\nGiuseppe Scrivano\n\n\n\n>From 88fe9b63d159ad1fd0579564558fbf0f900bd8e3 Mon Sep 17 00:00:00 2001\nFrom: Giuseppe Scrivano <gscrivano@gnu.org>\nDate: Sat, 26 Sep 2009 16:34:56 +0200\nSubject: [PATCH] Remove various dead assignments and dead increments found by the clang static analyzer\n\n---\n archive.c                |    2 +-\n builtin-add.c            |    2 +-\n builtin-bisect--helper.c |    2 +-\n builtin-commit.c         |    2 +-\n builtin-fetch--tool.c    |    2 +-\n builtin-fetch-pack.c     |    2 +-\n builtin-grep.c           |    2 --\n builtin-help.c           |    2 +-\n builtin-ls-files.c       |    2 +-\n builtin-mktree.c         |    2 +-\n builtin-pack-objects.c   |    2 +-\n builtin-prune-packed.c   |    2 +-\n builtin-receive-pack.c   |    6 +++---\n builtin-rev-parse.c      |    2 +-\n builtin-send-pack.c      |    2 +-\n builtin-show-branch.c    |    4 ++--\n builtin-show-ref.c       |    2 +-\n builtin-write-tree.c     |    2 +-\n color.c                  |    2 +-\n compat/mkstemps.c        |    2 +-\n connect.c                |    1 -\n diff.c                   |    2 +-\n http-fetch.c             |    3 +--\n transport.c              |    4 ++--\n upload-pack.c            |    2 +-\n 25 files changed, 27 insertions(+), 31 deletions(-)\n\ndiff --git a/archive.c b/archive.c\nindex 73b8e8a..88feed7 100644\n--- a/archive.c\n+++ b/archive.c\n@@ -357,7 +357,7 @@ int write_archive(int argc, const char **argv, const char *prefix,\n \tconst struct archiver *ar = NULL;\n \tstruct archiver_args args;\n \n-\targc = parse_archive_args(argc, argv, &ar, &args);\n+\tparse_archive_args(argc, argv, &ar, &args);\n \tif (setup_prefix && prefix == NULL)\n \t\tprefix = setup_git_directory();\n \ndiff --git a/builtin-add.c b/builtin-add.c\nindex cb6e590..2788315 100644\n--- a/builtin-add.c\n+++ b/builtin-add.c\n@@ -193,7 +193,7 @@ static int edit_patch(int argc, const char **argv, const char *prefix)\n \tinit_revisions(&rev, prefix);\n \trev.diffopt.context = 7;\n \n-\targc = setup_revisions(argc, argv, &rev, NULL);\n+\tsetup_revisions(argc, argv, &rev, NULL);\n \trev.diffopt.output_format = DIFF_FORMAT_PATCH;\n \tout = open(file, O_CREAT | O_WRONLY, 0644);\n \tif (out < 0)\ndiff --git a/builtin-bisect--helper.c b/builtin-bisect--helper.c\nindex 5b22639..f9c7695 100644\n--- a/builtin-bisect--helper.c\n+++ b/builtin-bisect--helper.c\n@@ -17,7 +17,7 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n \t\tOPT_END()\n \t};\n \n-\targc = parse_options(argc, argv, prefix, options,\n+\tparse_options(argc, argv, prefix, options,\n \t\t\t     git_bisect_helper_usage, 0);\n \n \tif (!next_all)\ndiff --git a/builtin-commit.c b/builtin-commit.c\nindex 200ffda..56b595f 100644\n--- a/builtin-commit.c\n+++ b/builtin-commit.c\n@@ -1035,7 +1035,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \t\t\tparents = reduce_heads(parents);\n \t} else {\n \t\treflog_msg = \"commit\";\n-\t\tpptr = &commit_list_insert(lookup_commit(head_sha1), pptr)->next;\n+\t\tcommit_list_insert(lookup_commit(head_sha1), pptr)->next;\n \t}\n \n \t/* Finally, get the commit message */\ndiff --git a/builtin-fetch--tool.c b/builtin-fetch--tool.c\nindex 3dbdf7a..c47469f 100644\n--- a/builtin-fetch--tool.c\n+++ b/builtin-fetch--tool.c\n@@ -169,7 +169,7 @@ static int append_fetch_head(FILE *fp,\n \t\t\tnote_len += sprintf(note + note_len, \"%s \", kind);\n \t\tnote_len += sprintf(note + note_len, \"'%s' of \", what);\n \t}\n-\tnote_len += sprintf(note + note_len, \"%.*s\", remote_len, remote);\n+\tsprintf(note + note_len, \"%.*s\", remote_len, remote);\n \tfprintf(fp, \"%s\\t%s\\t%s\\n\",\n \t\tsha1_to_hex(commit ? commit->object.sha1 : sha1),\n \t\tnot_for_merge ? \"not-for-merge\" : \"\",\ndiff --git a/builtin-fetch-pack.c b/builtin-fetch-pack.c\nindex 629735f..583f4e3 100644\n--- a/builtin-fetch-pack.c\n+++ b/builtin-fetch-pack.c\n@@ -555,7 +555,7 @@ static int get_pack(int xd[2], char **pack_lockfile)\n \t}\n \tif (*hdr_arg)\n \t\t*av++ = hdr_arg;\n-\t*av++ = NULL;\n+\t*av = NULL;\n \n \tcmd.in = demux.out;\n \tcmd.git_cmd = 1;\ndiff --git a/builtin-grep.c b/builtin-grep.c\nindex 761799d..d36b59e 100644\n--- a/builtin-grep.c\n+++ b/builtin-grep.c\n@@ -400,7 +400,6 @@ static int external_grep(struct grep_opt *opt, const char **paths, int cached)\n \t\t\t\tif (sizeof(randarg) <= len)\n \t\t\t\t\tdie(\"maximum length of args exceeded\");\n \t\t\t\tpush_arg(argptr);\n-\t\t\t\targptr += len;\n \t\t\t}\n \t\t}\n \t\telse {\n@@ -410,7 +409,6 @@ static int external_grep(struct grep_opt *opt, const char **paths, int cached)\n \t\t\tif (sizeof(randarg) <= len)\n \t\t\t\tdie(\"maximum length of args exceeded\");\n \t\t\tpush_arg(argptr);\n-\t\t\targptr += len;\n \t\t}\n \t}\n \tfor (p = opt->pattern_list; p; p = p->next) {\ndiff --git a/builtin-help.c b/builtin-help.c\nindex e1eba77..76307fd 100644\n--- a/builtin-help.c\n+++ b/builtin-help.c\n@@ -419,7 +419,7 @@ int cmd_help(int argc, const char **argv, const char *prefix)\n \tsetup_git_directory_gently(&nongit);\n \tgit_config(git_help_config, NULL);\n \n-\targc = parse_options(argc, argv, prefix, builtin_help_options,\n+\tparse_options(argc, argv, prefix, builtin_help_options,\n \t\t\tbuiltin_help_usage, 0);\n \n \tif (show_all) {\ndiff --git a/builtin-ls-files.c b/builtin-ls-files.c\nindex f473220..80212f4 100644\n--- a/builtin-ls-files.c\n+++ b/builtin-ls-files.c\n@@ -481,7 +481,7 @@ int cmd_ls_files(int argc, const char **argv, const char *prefix)\n \t\tprefix_offset = strlen(prefix);\n \tgit_config(git_default_config, NULL);\n \n-\targc = parse_options(argc, argv, prefix, builtin_ls_files_options,\n+\tparse_options(argc, argv, prefix, builtin_ls_files_options,\n \t\t\tls_files_usage, 0);\n \tif (show_tag || show_valid_bit) {\n \t\ttag_cached = \"H \";\ndiff --git a/builtin-mktree.c b/builtin-mktree.c\nindex 098395f..36053cf 100644\n--- a/builtin-mktree.c\n+++ b/builtin-mktree.c\n@@ -155,7 +155,7 @@ int cmd_mktree(int ac, const char **av, const char *prefix)\n \t\tOPT_END()\n \t};\n \n-\tac = parse_options(ac, av, prefix, option, mktree_usage, 0);\n+\tparse_options(ac, av, prefix, option, mktree_usage, 0);\n \n \twhile (!got_eof) {\n \t\twhile (1) {\ndiff --git a/builtin-pack-objects.c b/builtin-pack-objects.c\nindex 02f9246..bea7141 100644\n--- a/builtin-pack-objects.c\n+++ b/builtin-pack-objects.c\n@@ -2307,7 +2307,7 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n \t */\n \n \tif (!pack_to_stdout)\n-\t\tbase_name = argv[i++];\n+\t\tbase_name = argv[i];\n \n \tif (pack_to_stdout != !base_name)\n \t\tusage(pack_usage);\ndiff --git a/builtin-prune-packed.c b/builtin-prune-packed.c\nindex be99eb0..9a8fcfe 100644\n--- a/builtin-prune-packed.c\n+++ b/builtin-prune-packed.c\n@@ -78,7 +78,7 @@ int cmd_prune_packed(int argc, const char **argv, const char *prefix)\n \t\tOPT_END()\n \t};\n \n-\targc = parse_options(argc, argv, prefix, prune_packed_options,\n+\tparse_options(argc, argv, prefix, prune_packed_options,\n \t\t\t     prune_packed_usage, 0);\n \n \tprune_packed_objects(opts);\ndiff --git a/builtin-receive-pack.c b/builtin-receive-pack.c\nindex b771fe9..957e7f0 100644\n--- a/builtin-receive-pack.c\n+++ b/builtin-receive-pack.c\n@@ -391,7 +391,7 @@ static void run_update_post_hook(struct command *cmd)\n \t\targc++;\n \t}\n \targv[argc] = NULL;\n-\tstatus = run_command_v_opt(argv, RUN_COMMAND_NO_STDIN\n+\trun_command_v_opt(argv, RUN_COMMAND_NO_STDIN\n \t\t\t| RUN_COMMAND_STDOUT_TO_STDERR);\n }\n \n@@ -506,7 +506,7 @@ static const char *unpack(void)\n \t\tif (receive_fsck_objects)\n \t\t\tunpacker[i++] = \"--strict\";\n \t\tunpacker[i++] = hdr_arg;\n-\t\tunpacker[i++] = NULL;\n+\t\tunpacker[i] = NULL;\n \t\tcode = run_command_v_opt(unpacker, RUN_GIT_CMD);\n \t\tif (!code)\n \t\t\treturn NULL;\n@@ -528,7 +528,7 @@ static const char *unpack(void)\n \t\tkeeper[i++] = \"--fix-thin\";\n \t\tkeeper[i++] = hdr_arg;\n \t\tkeeper[i++] = keep_arg;\n-\t\tkeeper[i++] = NULL;\n+\t\tkeeper[i] = NULL;\n \t\tmemset(&ip, 0, sizeof(ip));\n \t\tip.argv = keeper;\n \t\tip.out = -1;\ndiff --git a/builtin-rev-parse.c b/builtin-rev-parse.c\nindex 45bead6..4a66ba4 100644\n--- a/builtin-rev-parse.c\n+++ b/builtin-rev-parse.c\n@@ -396,7 +396,7 @@ static int cmd_parseopt(int argc, const char **argv, const char *prefix)\n \t/* put an OPT_END() */\n \tALLOC_GROW(opts, onb + 1, osz);\n \tmemset(opts + onb, 0, sizeof(opts[onb]));\n-\targc = parse_options(argc, argv, prefix, opts, usage,\n+\tparse_options(argc, argv, prefix, opts, usage,\n \t\t\tkeep_dashdash ? PARSE_OPT_KEEP_DASHDASH : 0 |\n \t\t\tstop_at_non_option ? PARSE_OPT_STOP_AT_NON_OPTION : 0);\n \ndiff --git a/builtin-send-pack.c b/builtin-send-pack.c\nindex 37e528e..5afd542 100644\n--- a/builtin-send-pack.c\n+++ b/builtin-send-pack.c\n@@ -55,7 +55,7 @@ static int pack_objects(int fd, struct ref *refs, struct extra_have_objects *ext\n \tif (args->use_ofs_delta)\n \t\targv[i++] = \"--delta-base-offset\";\n \tif (args->quiet)\n-\t\targv[i++] = \"-q\";\n+\t\targv[i] = \"-q\";\n \tmemset(&po, 0, sizeof(po));\n \tpo.argv = argv;\n \tpo.in = -1;\ndiff --git a/builtin-show-branch.c b/builtin-show-branch.c\nindex 3510a86..e567eb5 100644\n--- a/builtin-show-branch.c\n+++ b/builtin-show-branch.c\n@@ -191,9 +191,9 @@ static void name_commits(struct commit_list *list,\n \t\t\t\t\tbreak;\n \t\t\t\t}\n \t\t\t\tif (nth == 1)\n-\t\t\t\t\ten += sprintf(en, \"^\");\n+\t\t\t\t\tsprintf(en, \"^\");\n \t\t\t\telse\n-\t\t\t\t\ten += sprintf(en, \"^%d\", nth);\n+\t\t\t\t\tsprintf(en, \"^%d\", nth);\n \t\t\t\tname_commit(p, xstrdup(newname), 0);\n \t\t\t\ti++;\n \t\t\t\tname_first_parent_chain(p);\ndiff --git a/builtin-show-ref.c b/builtin-show-ref.c\nindex c46550c..8a0ae6c 100644\n--- a/builtin-show-ref.c\n+++ b/builtin-show-ref.c\n@@ -201,7 +201,7 @@ static const struct option show_ref_options[] = {\n \n int cmd_show_ref(int argc, const char **argv, const char *prefix)\n {\n-\targc = parse_options(argc, argv, prefix, show_ref_options,\n+\tparse_options(argc, argv, prefix, show_ref_options,\n \t\t\t     show_ref_usage, PARSE_OPT_NO_INTERNAL_HELP);\n \n \tif (exclude_arg)\ndiff --git a/builtin-write-tree.c b/builtin-write-tree.c\nindex b223af4..848c3e4 100644\n--- a/builtin-write-tree.c\n+++ b/builtin-write-tree.c\n@@ -34,7 +34,7 @@ int cmd_write_tree(int argc, const char **argv, const char *unused_prefix)\n \t};\n \n \tgit_config(git_default_config, NULL);\n-\targc = parse_options(argc, argv, unused_prefix, write_tree_options,\n+\tparse_options(argc, argv, unused_prefix, write_tree_options,\n \t\t\t     write_tree_usage, 0);\n \n \tret = write_cache_as_tree(sha1, flags, prefix);\ndiff --git a/color.c b/color.c\nindex 62977f4..5b31588 100644\n--- a/color.c\n+++ b/color.c\n@@ -110,7 +110,7 @@ void color_parse_mem(const char *value, int value_len, const char *var,\n \t\t\t}\n \t\t}\n \t\tif (bg >= 0) {\n-\t\t\tif (sep++)\n+\t\t\tif (sep)\n \t\t\t\t*dst++ = ';';\n \t\t\tif (bg < 8) {\n \t\t\t\t*dst++ = '4';\ndiff --git a/compat/mkstemps.c b/compat/mkstemps.c\nindex 14179c8..dbf916e 100644\n--- a/compat/mkstemps.c\n+++ b/compat/mkstemps.c\n@@ -45,7 +45,7 @@ int gitmkstemps(char *pattern, int suffix_len)\n \t\ttemplate[2] = letters[v % num_letters]; v /= num_letters;\n \t\ttemplate[3] = letters[v % num_letters]; v /= num_letters;\n \t\ttemplate[4] = letters[v % num_letters]; v /= num_letters;\n-\t\ttemplate[5] = letters[v % num_letters]; v /= num_letters;\n+\t\ttemplate[5] = letters[v % num_letters];\n \n \t\tfd = open(pattern, O_CREAT | O_EXCL | O_RDWR, 0600);\n \t\tif (fd > 0)\ndiff --git a/connect.c b/connect.c\nindex 7945e38..da6c7c1 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -18,7 +18,6 @@ static int check_ref(const char *name, int len, unsigned int flags)\n \n \t/* Skip the \"refs/\" part */\n \tname += 5;\n-\tlen -= 5;\n \n \t/* REF_NORMAL means that we don't want the magic fake tag refs */\n \tif ((flags & REF_NORMAL) && check_ref_format(name) < 0)\ndiff --git a/diff.c b/diff.c\nindex e1be189..e75f58e 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -901,7 +901,7 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)\n \n \t/* Find the longest filename and max number of changes */\n \treset = diff_get_color_opt(options, DIFF_RESET);\n-\tset   = diff_get_color_opt(options, DIFF_PLAIN);\n+\tdiff_get_color_opt(options, DIFF_PLAIN);\n \tadd_c = diff_get_color_opt(options, DIFF_FILE_NEW);\n \tdel_c = diff_get_color_opt(options, DIFF_FILE_OLD);\n \ndiff --git a/http-fetch.c b/http-fetch.c\nindex e8f44ba..6879904 100644\n--- a/http-fetch.c\n+++ b/http-fetch.c\n@@ -3,7 +3,6 @@\n \n int main(int argc, const char **argv)\n {\n-\tconst char *prefix;\n \tstruct walker *walker;\n \tint commits_on_stdin = 0;\n \tint commits;\n@@ -19,7 +18,7 @@ int main(int argc, const char **argv)\n \tint get_verbosely = 0;\n \tint get_recover = 0;\n \n-\tprefix = setup_git_directory();\n+\tsetup_git_directory();\n \n \tgit_config(git_default_config, NULL);\n \ndiff --git a/transport.c b/transport.c\nindex 644a30a..8ec0df6 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -308,7 +308,7 @@ static int rsync_transport_push(struct transport *transport,\n \targs[i++] = \"info\";\n \targs[i++] = get_object_directory();\n \targs[i++] = buf.buf;\n-\targs[i++] = NULL;\n+\targs[i] = NULL;\n \n \tif (run_command(&rsync))\n \t\treturn error(\"Could not push objects to %s\",\n@@ -334,7 +334,7 @@ static int rsync_transport_push(struct transport *transport,\n \t\targs[i++] = \"--ignore-existing\";\n \targs[i++] = temp_dir.buf;\n \targs[i++] = rsync_url(transport->url);\n-\targs[i++] = NULL;\n+\targs[i] = NULL;\n \tif (run_command(&rsync))\n \t\tresult = error(\"Could not push to %s\",\n \t\t\t\trsync_url(transport->url));\ndiff --git a/upload-pack.c b/upload-pack.c\nindex 38ddac2..fb3436c 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -241,7 +241,7 @@ static void create_pack_file(void)\n \t\targv[arg++] = \"--delta-base-offset\";\n \tif (use_include_tag)\n \t\targv[arg++] = \"--include-tag\";\n-\targv[arg++] = NULL;\n+\targv[arg] = NULL;\n \n \tmemset(&pack_objects, 0, sizeof(pack_objects));\n \tpack_objects.in = shallow_nr ? rev_list.out : -1;\n-- \n1.6.3.3\n"},{"id":"123847","messageId":"alpine.DEB.1.00.0909261756510.4985@pacific.mpi-cbg.de","threadId":"21065","inReplyTo":"87ab0hepcn.fsf@master.homenet","subject":"Re: [PATCH] Remove various dead assignments and dead increments found by the clang static analyzer","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-09-26T15:58:43Z","receivedAt":"2009-09-26T15:58:43Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Sat, 26 Sep 2009, Giuseppe Scrivano wrote:\n\n> I tried the clang static analyzer on the git source code, this patch\n> fixes the found dead assignments/increments.\n> \n> diff --git a/archive.c b/archive.c\n> index 73b8e8a..88feed7 100644\n> --- a/archive.c\n> +++ b/archive.c\n> @@ -357,7 +357,7 @@ int write_archive(int argc, const char **argv, const char *prefix,\n>  \tconst struct archiver *ar = NULL;\n>  \tstruct archiver_args args;\n>  \n> -\targc = parse_archive_args(argc, argv, &ar, &args);\n> +\tparse_archive_args(argc, argv, &ar, &args);\n>  \tif (setup_prefix && prefix == NULL)\n>  \t\tprefix = setup_git_directory();\n\nI understand that clang complains when argc is not really used afterwards, \nbut do we really want to do this?  I mean, if somebody decides it'd be a \ngood idea to check the number of arguments after parsing the arguments, \nthey might be bitten by the fact that it is now actively wrong.\n\nCiao,\nDscho\n"},{"id":"123853","messageId":"871vltefdj.fsf@master.homenet","threadId":"21065","inReplyTo":"alpine.DEB.1.00.0909261756510.4985@pacific.mpi-cbg.de","subject":"Re: [PATCH] Remove various dead assignments and dead increments found by the clang static analyzer","fromName":"Giuseppe Scrivano","fromEmail":"gscrivano@gnu.org","sentAt":"2009-09-26T18:21:28Z","receivedAt":"2009-09-26T18:21:28Z","isPatch":true,"sender":{"key":"gscrivano@gnu.org","avatar":"https://avatars.githubusercontent.com/u/67430?v=4"},"body":"Hello,\n\n\nJohannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n\n> I understand that clang complains when argc is not really used afterwards, \n> but do we really want to do this?  I mean, if somebody decides it'd be a \n> good idea to check the number of arguments after parsing the arguments, \n> they might be bitten by the fact that it is now actively wrong.\n\nprobably this is not the only case to leave as it is.  I just cleaned\nanything clang reported.\n\nCheers,\nGiuseppe\n"},{"id":"123854","messageId":"fabb9a1e0909261134qd90dba1n9637fe4adc253fc1@mail.gmail.com","threadId":"21065","inReplyTo":"871vltefdj.fsf@master.homenet","subject":"Re: [PATCH] Remove various dead assignments and dead increments found by the clang static analyzer","fromName":"Sverre Rabbelier","fromEmail":"srabbelier@gmail.com","sentAt":"2009-09-26T18:34:22Z","receivedAt":"2009-09-26T18:34:22Z","isPatch":true,"sender":{"key":"srabbelier@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3098?v=4"},"body":"Heya,\n\nOn Sat, Sep 26, 2009 at 20:21, Giuseppe Scrivano <gscrivano@gnu.org> wrote:\n> probably this is not the only case to leave as it is.  I just cleaned\n> anything clang reported.\n\nThen it would probably have been better to say so by at least marking\nyour patch as RFC and including such a remark in the cover letter, no?\nAlso, now that this has been pointed out, you shouldn't expect it to\nbe included until someone either takes your patch and cleans it up (as\nin, checks all statements manually), or until you do so yourself.\n\n-- \nCheers,\n\nSverre Rabbelier\n"},{"id":"123855","messageId":"87ws3lczmy.fsf@master.homenet","threadId":"21065","inReplyTo":"fabb9a1e0909261134qd90dba1n9637fe4adc253fc1@mail.gmail.com","subject":"Re: [PATCH] Remove various dead assignments and dead increments found by the clang static analyzer","fromName":"Giuseppe Scrivano","fromEmail":"gscrivano@gnu.org","sentAt":"2009-09-26T18:46:45Z","receivedAt":"2009-09-26T18:46:45Z","isPatch":true,"sender":{"key":"gscrivano@gnu.org","avatar":"https://avatars.githubusercontent.com/u/67430?v=4"},"body":"Hello,\n\nSverre Rabbelier <srabbelier@gmail.com> writes:\n\n> Then it would probably have been better to say so by at least marking\n> your patch as RFC and including such a remark in the cover letter, no?\n> Also, now that this has been pointed out, you shouldn't expect it to\n> be included until someone either takes your patch and cleans it up (as\n> in, checks all statements manually), or until you do so yourself.\n\nI really had to include a RFC remark.  After what Johannes reported, I\nthink there is need only to restore assignments to argc while other ones\ncan be dropped without problems.  I'll post a cleaned patch later.\n\nCheers,\nGiuseppe\n"},{"id":"123856","messageId":"87ske9cya9.fsf@master.homenet","threadId":"21065","inReplyTo":"fabb9a1e0909261134qd90dba1n9637fe4adc253fc1@mail.gmail.com","subject":"Re: [PATCH] Remove various dead assignments and dead increments found by the clang static analyzer","fromName":"Giuseppe Scrivano","fromEmail":"gscrivano@gnu.org","sentAt":"2009-09-26T19:15:58Z","receivedAt":"2009-09-26T19:15:58Z","isPatch":true,"sender":{"key":"gscrivano@gnu.org","avatar":"https://avatars.githubusercontent.com/u/67430?v=4"},"body":"Here is a cleaned patch.  I think these assignments can be removed\nwithout any problem.\n\nCheers,\nGiuseppe\n\n\n\n>From 7501d82998132b15ad5cda78c0650f4f4a0b0e93 Mon Sep 17 00:00:00 2001\nFrom: Giuseppe Scrivano <gscrivano@gnu.org>\nDate: Sat, 26 Sep 2009 21:11:21 +0200\nSubject: [PATCH] Remove various dead assignments and dead increments found by the clang static analyzer\n\n---\n builtin-commit.c       |    2 +-\n builtin-fetch--tool.c  |    2 +-\n builtin-fetch-pack.c   |    2 +-\n builtin-grep.c         |    2 --\n builtin-pack-objects.c |    2 +-\n builtin-receive-pack.c |    8 ++++----\n builtin-send-pack.c    |    2 +-\n builtin-show-branch.c  |    4 ++--\n color.c                |    2 +-\n compat/mkstemps.c      |    2 +-\n connect.c              |    1 -\n diff.c                 |    2 +-\n http-fetch.c           |    3 +--\n transport.c            |    4 ++--\n upload-pack.c          |    2 +-\n 15 files changed, 18 insertions(+), 22 deletions(-)\n\ndiff --git a/builtin-commit.c b/builtin-commit.c\nindex 200ffda..331d2a0 100644\n--- a/builtin-commit.c\n+++ b/builtin-commit.c\n@@ -1035,7 +1035,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \t\t\tparents = reduce_heads(parents);\n \t} else {\n \t\treflog_msg = \"commit\";\n-\t\tpptr = &commit_list_insert(lookup_commit(head_sha1), pptr)->next;\n+\t\tcommit_list_insert(lookup_commit(head_sha1), pptr);\n \t}\n \n \t/* Finally, get the commit message */\ndiff --git a/builtin-fetch--tool.c b/builtin-fetch--tool.c\nindex 3dbdf7a..c47469f 100644\n--- a/builtin-fetch--tool.c\n+++ b/builtin-fetch--tool.c\n@@ -169,7 +169,7 @@ static int append_fetch_head(FILE *fp,\n \t\t\tnote_len += sprintf(note + note_len, \"%s \", kind);\n \t\tnote_len += sprintf(note + note_len, \"'%s' of \", what);\n \t}\n-\tnote_len += sprintf(note + note_len, \"%.*s\", remote_len, remote);\n+\tsprintf(note + note_len, \"%.*s\", remote_len, remote);\n \tfprintf(fp, \"%s\\t%s\\t%s\\n\",\n \t\tsha1_to_hex(commit ? commit->object.sha1 : sha1),\n \t\tnot_for_merge ? \"not-for-merge\" : \"\",\ndiff --git a/builtin-fetch-pack.c b/builtin-fetch-pack.c\nindex 629735f..583f4e3 100644\n--- a/builtin-fetch-pack.c\n+++ b/builtin-fetch-pack.c\n@@ -555,7 +555,7 @@ static int get_pack(int xd[2], char **pack_lockfile)\n \t}\n \tif (*hdr_arg)\n \t\t*av++ = hdr_arg;\n-\t*av++ = NULL;\n+\t*av = NULL;\n \n \tcmd.in = demux.out;\n \tcmd.git_cmd = 1;\ndiff --git a/builtin-grep.c b/builtin-grep.c\nindex 761799d..d36b59e 100644\n--- a/builtin-grep.c\n+++ b/builtin-grep.c\n@@ -400,7 +400,6 @@ static int external_grep(struct grep_opt *opt, const char **paths, int cached)\n \t\t\t\tif (sizeof(randarg) <= len)\n \t\t\t\t\tdie(\"maximum length of args exceeded\");\n \t\t\t\tpush_arg(argptr);\n-\t\t\t\targptr += len;\n \t\t\t}\n \t\t}\n \t\telse {\n@@ -410,7 +409,6 @@ static int external_grep(struct grep_opt *opt, const char **paths, int cached)\n \t\t\tif (sizeof(randarg) <= len)\n \t\t\t\tdie(\"maximum length of args exceeded\");\n \t\t\tpush_arg(argptr);\n-\t\t\targptr += len;\n \t\t}\n \t}\n \tfor (p = opt->pattern_list; p; p = p->next) {\ndiff --git a/builtin-pack-objects.c b/builtin-pack-objects.c\nindex 02f9246..bea7141 100644\n--- a/builtin-pack-objects.c\n+++ b/builtin-pack-objects.c\n@@ -2307,7 +2307,7 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n \t */\n \n \tif (!pack_to_stdout)\n-\t\tbase_name = argv[i++];\n+\t\tbase_name = argv[i];\n \n \tif (pack_to_stdout != !base_name)\n \t\tusage(pack_usage);\ndiff --git a/builtin-receive-pack.c b/builtin-receive-pack.c\nindex b771fe9..82d1564 100644\n--- a/builtin-receive-pack.c\n+++ b/builtin-receive-pack.c\n@@ -368,7 +368,7 @@ static char update_post_hook[] = \"hooks/post-update\";\n static void run_update_post_hook(struct command *cmd)\n {\n \tstruct command *cmd_p;\n-\tint argc, status;\n+\tint argc;\n \tconst char **argv;\n \n \tfor (argc = 0, cmd_p = cmd; cmd_p; cmd_p = cmd_p->next) {\n@@ -391,7 +391,7 @@ static void run_update_post_hook(struct command *cmd)\n \t\targc++;\n \t}\n \targv[argc] = NULL;\n-\tstatus = run_command_v_opt(argv, RUN_COMMAND_NO_STDIN\n+\trun_command_v_opt(argv, RUN_COMMAND_NO_STDIN\n \t\t\t| RUN_COMMAND_STDOUT_TO_STDERR);\n }\n \n@@ -506,7 +506,7 @@ static const char *unpack(void)\n \t\tif (receive_fsck_objects)\n \t\t\tunpacker[i++] = \"--strict\";\n \t\tunpacker[i++] = hdr_arg;\n-\t\tunpacker[i++] = NULL;\n+\t\tunpacker[i] = NULL;\n \t\tcode = run_command_v_opt(unpacker, RUN_GIT_CMD);\n \t\tif (!code)\n \t\t\treturn NULL;\n@@ -528,7 +528,7 @@ static const char *unpack(void)\n \t\tkeeper[i++] = \"--fix-thin\";\n \t\tkeeper[i++] = hdr_arg;\n \t\tkeeper[i++] = keep_arg;\n-\t\tkeeper[i++] = NULL;\n+\t\tkeeper[i] = NULL;\n \t\tmemset(&ip, 0, sizeof(ip));\n \t\tip.argv = keeper;\n \t\tip.out = -1;\ndiff --git a/builtin-send-pack.c b/builtin-send-pack.c\nindex 37e528e..5afd542 100644\n--- a/builtin-send-pack.c\n+++ b/builtin-send-pack.c\n@@ -55,7 +55,7 @@ static int pack_objects(int fd, struct ref *refs, struct extra_have_objects *ext\n \tif (args->use_ofs_delta)\n \t\targv[i++] = \"--delta-base-offset\";\n \tif (args->quiet)\n-\t\targv[i++] = \"-q\";\n+\t\targv[i] = \"-q\";\n \tmemset(&po, 0, sizeof(po));\n \tpo.argv = argv;\n \tpo.in = -1;\ndiff --git a/builtin-show-branch.c b/builtin-show-branch.c\nindex 3510a86..e567eb5 100644\n--- a/builtin-show-branch.c\n+++ b/builtin-show-branch.c\n@@ -191,9 +191,9 @@ static void name_commits(struct commit_list *list,\n \t\t\t\t\tbreak;\n \t\t\t\t}\n \t\t\t\tif (nth == 1)\n-\t\t\t\t\ten += sprintf(en, \"^\");\n+\t\t\t\t\tsprintf(en, \"^\");\n \t\t\t\telse\n-\t\t\t\t\ten += sprintf(en, \"^%d\", nth);\n+\t\t\t\t\tsprintf(en, \"^%d\", nth);\n \t\t\t\tname_commit(p, xstrdup(newname), 0);\n \t\t\t\ti++;\n \t\t\t\tname_first_parent_chain(p);\ndiff --git a/color.c b/color.c\nindex 62977f4..5b31588 100644\n--- a/color.c\n+++ b/color.c\n@@ -110,7 +110,7 @@ void color_parse_mem(const char *value, int value_len, const char *var,\n \t\t\t}\n \t\t}\n \t\tif (bg >= 0) {\n-\t\t\tif (sep++)\n+\t\t\tif (sep)\n \t\t\t\t*dst++ = ';';\n \t\t\tif (bg < 8) {\n \t\t\t\t*dst++ = '4';\ndiff --git a/compat/mkstemps.c b/compat/mkstemps.c\nindex 14179c8..dbf916e 100644\n--- a/compat/mkstemps.c\n+++ b/compat/mkstemps.c\n@@ -45,7 +45,7 @@ int gitmkstemps(char *pattern, int suffix_len)\n \t\ttemplate[2] = letters[v % num_letters]; v /= num_letters;\n \t\ttemplate[3] = letters[v % num_letters]; v /= num_letters;\n \t\ttemplate[4] = letters[v % num_letters]; v /= num_letters;\n-\t\ttemplate[5] = letters[v % num_letters]; v /= num_letters;\n+\t\ttemplate[5] = letters[v % num_letters];\n \n \t\tfd = open(pattern, O_CREAT | O_EXCL | O_RDWR, 0600);\n \t\tif (fd > 0)\ndiff --git a/connect.c b/connect.c\nindex 7945e38..da6c7c1 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -18,7 +18,6 @@ static int check_ref(const char *name, int len, unsigned int flags)\n \n \t/* Skip the \"refs/\" part */\n \tname += 5;\n-\tlen -= 5;\n \n \t/* REF_NORMAL means that we don't want the magic fake tag refs */\n \tif ((flags & REF_NORMAL) && check_ref_format(name) < 0)\ndiff --git a/diff.c b/diff.c\nindex e1be189..e75f58e 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -901,7 +901,7 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)\n \n \t/* Find the longest filename and max number of changes */\n \treset = diff_get_color_opt(options, DIFF_RESET);\n-\tset   = diff_get_color_opt(options, DIFF_PLAIN);\n+\tdiff_get_color_opt(options, DIFF_PLAIN);\n \tadd_c = diff_get_color_opt(options, DIFF_FILE_NEW);\n \tdel_c = diff_get_color_opt(options, DIFF_FILE_OLD);\n \ndiff --git a/http-fetch.c b/http-fetch.c\nindex e8f44ba..6879904 100644\n--- a/http-fetch.c\n+++ b/http-fetch.c\n@@ -3,7 +3,6 @@\n \n int main(int argc, const char **argv)\n {\n-\tconst char *prefix;\n \tstruct walker *walker;\n \tint commits_on_stdin = 0;\n \tint commits;\n@@ -19,7 +18,7 @@ int main(int argc, const char **argv)\n \tint get_verbosely = 0;\n \tint get_recover = 0;\n \n-\tprefix = setup_git_directory();\n+\tsetup_git_directory();\n \n \tgit_config(git_default_config, NULL);\n \ndiff --git a/transport.c b/transport.c\nindex 644a30a..8ec0df6 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -308,7 +308,7 @@ static int rsync_transport_push(struct transport *transport,\n \targs[i++] = \"info\";\n \targs[i++] = get_object_directory();\n \targs[i++] = buf.buf;\n-\targs[i++] = NULL;\n+\targs[i] = NULL;\n \n \tif (run_command(&rsync))\n \t\treturn error(\"Could not push objects to %s\",\n@@ -334,7 +334,7 @@ static int rsync_transport_push(struct transport *transport,\n \t\targs[i++] = \"--ignore-existing\";\n \targs[i++] = temp_dir.buf;\n \targs[i++] = rsync_url(transport->url);\n-\targs[i++] = NULL;\n+\targs[i] = NULL;\n \tif (run_command(&rsync))\n \t\tresult = error(\"Could not push to %s\",\n \t\t\t\trsync_url(transport->url));\ndiff --git a/upload-pack.c b/upload-pack.c\nindex 38ddac2..fb3436c 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -241,7 +241,7 @@ static void create_pack_file(void)\n \t\targv[arg++] = \"--delta-base-offset\";\n \tif (use_include_tag)\n \t\targv[arg++] = \"--include-tag\";\n-\targv[arg++] = NULL;\n+\targv[arg] = NULL;\n \n \tmemset(&pack_objects, 0, sizeof(pack_objects));\n \tpack_objects.in = shallow_nr ? rev_list.out : -1;\n-- \n1.6.3.3\n"},{"id":"123858","messageId":"4ABE6B6A.4000400@lsrfire.ath.cx","threadId":"21065","inReplyTo":"87ske9cya9.fsf@master.homenet","subject":"Re: [PATCH] Remove various dead assignments and dead increments found by the clang static analyzer","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2009-09-26T19:28:42Z","receivedAt":"2009-09-26T19:28:42Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"> diff --git a/diff.c b/diff.c\n> index e1be189..e75f58e 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -901,7 +901,7 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)\n>  \n>  \t/* Find the longest filename and max number of changes */\n>  \treset = diff_get_color_opt(options, DIFF_RESET);\n> -\tset   = diff_get_color_opt(options, DIFF_PLAIN);\n> +\tdiff_get_color_opt(options, DIFF_PLAIN);\n>  \tadd_c = diff_get_color_opt(options, DIFF_FILE_NEW);\n>  \tdel_c = diff_get_color_opt(options, DIFF_FILE_OLD);\n\ndiff_get_color_opt() has no side-effects; the changed line is a no-op.\n\nRené\n"},{"id":"123866","messageId":"alpine.DEB.1.00.0909262235010.4985@pacific.mpi-cbg.de","threadId":"21065","inReplyTo":"87ske9cya9.fsf@master.homenet","subject":"Re: [PATCH] Remove various dead assignments and dead increments found by the clang static analyzer","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-09-26T20:39:32Z","receivedAt":"2009-09-26T20:39:32Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Sat, 26 Sep 2009, Giuseppe Scrivano wrote:\n\n> diff --git a/builtin-commit.c b/builtin-commit.c\n> index 200ffda..331d2a0 100644\n> --- a/builtin-commit.c\n> +++ b/builtin-commit.c\n> @@ -1035,7 +1035,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n>  \t\t\tparents = reduce_heads(parents);\n>  \t} else {\n>  \t\treflog_msg = \"commit\";\n> -\t\tpptr = &commit_list_insert(lookup_commit(head_sha1), pptr)->next;\n> +\t\tcommit_list_insert(lookup_commit(head_sha1), pptr);\n>  \t}\n\nSorry, but from the context it seems as if the same remark I had for argc \napplies here, too.  There are exactly three other similar-looking \nassignments and it is too easy IMO to mess up when one want to rearrange \nthings there.\n\nIn other words, I deem the removal of this assignment worse than what we \nhave now -- at least in terms of how easy it is to modify the code safely.\n\nI just looked further 3 hunks and had exactly the same impression there, \nso I stopped looking.\n\nSorry,\nDscho\n"},{"id":"123867","messageId":"20090926204604.GA2960@coredump.intra.peff.net","threadId":"21065","inReplyTo":"87ske9cya9.fsf@master.homenet","subject":"Re: [PATCH] Remove various dead assignments and dead increments found by the clang static analyzer","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-09-26T20:46:04Z","receivedAt":"2009-09-26T20:46:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Sep 26, 2009 at 09:15:58PM +0200, Giuseppe Scrivano wrote:\n\n> Here is a cleaned patch.  I think these assignments can be removed\n> without any problem.\n\nI don't agree. For example:\n\n> --- a/builtin-fetch--tool.c\n> +++ b/builtin-fetch--tool.c\n> @@ -169,7 +169,7 @@ static int append_fetch_head(FILE *fp,\n>  \t\t\tnote_len += sprintf(note + note_len, \"%s \", kind);\n>  \t\tnote_len += sprintf(note + note_len, \"'%s' of \", what);\n>  \t}\n> -\tnote_len += sprintf(note + note_len, \"%.*s\", remote_len, remote);\n> +\tsprintf(note + note_len, \"%.*s\", remote_len, remote);\n\nThis is a very particular C idiom: you are building a string over\nseveral statements using a function that adds to the string and tells\nyou how much it added. The implicit invariant of the note_len variable\nis that it _always_ contains the current length, so each statement uses\nit as input and pushes it forward on output.\n\nAny experienced C programmer should look at that and be able to see\nexactly what's going on. And people adding more lines don't need to\nmunge the existing lines; the invariant property of note_len means they\njust need to add more, similar lines.\n\nBut your patch destroys that invariant. It makes it harder to see what's\ngoing on, because it breaks the idiom. And it makes it more likely for\nsomebody adding a line further on to make a mistake (and certainly it\nmakes their patch harder to read and review, as they have to munge\nunrelated lines).\n\nSo no, while there is no code _now_ that is relying on the invariant\nbeing kept after the last statement (which is what the static analyzer\nis finding out), the point is not for the compiler to realize that, but\nfor human programmers to see it.\n\nSo I think your version is less readable and maintainable. And it\ndoesn't even introduce any efficiency; any decent compiler should be\nable to optimize out the addition.\n\n> --- a/builtin-fetch-pack.c\n> +++ b/builtin-fetch-pack.c\n> @@ -555,7 +555,7 @@ static int get_pack(int xd[2], char **pack_lockfile)\n>  \t}\n>  \tif (*hdr_arg)\n>  \t\t*av++ = hdr_arg;\n> -\t*av++ = NULL;\n> +\t*av = NULL;\n\nI would argue a similar same idiom exists here, though given that NULL\nby definition is ending the av list it is somewhat less strong (i.e.,\nthere is already something to that statement that indicates that it\n_must_ be the last one).\n\n> --- a/builtin-pack-objects.c\n> +++ b/builtin-pack-objects.c\n> @@ -2307,7 +2307,7 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n>  \t */\n>  \n>  \tif (!pack_to_stdout)\n> -\t\tbase_name = argv[i++];\n> +\t\tbase_name = argv[i];\n\nAnd again here. Maintaining the invariant on 'i' is important to\nreadability and maintainability.\n\n> --- a/builtin-receive-pack.c\n> +++ b/builtin-receive-pack.c\n> @@ -368,7 +368,7 @@ static char update_post_hook[] = \"hooks/post-update\";\n>  static void run_update_post_hook(struct command *cmd)\n>  {\n>  \tstruct command *cmd_p;\n> -\tint argc, status;\n> +\tint argc;\n>  \tconst char **argv;\n>  \n>  \tfor (argc = 0, cmd_p = cmd; cmd_p; cmd_p = cmd_p->next) {\n> @@ -391,7 +391,7 @@ static void run_update_post_hook(struct command *cmd)\n>  \t\targc++;\n>  \t}\n>  \targv[argc] = NULL;\n> -\tstatus = run_command_v_opt(argv, RUN_COMMAND_NO_STDIN\n> +\trun_command_v_opt(argv, RUN_COMMAND_NO_STDIN\n>  \t\t\t| RUN_COMMAND_STDOUT_TO_STDERR);\n>  }\n\nNow this is one that I do think is sensible. The variable isn't used, so\ndon't even bother declaring it.\n\n> @@ -506,7 +506,7 @@ static const char *unpack(void)\n>  \t\tif (receive_fsck_objects)\n>  \t\t\tunpacker[i++] = \"--strict\";\n>  \t\tunpacker[i++] = hdr_arg;\n> -\t\tunpacker[i++] = NULL;\n> +\t\tunpacker[i] = NULL;\n>  \t\tcode = run_command_v_opt(unpacker, RUN_GIT_CMD);\n>  \t\tif (!code)\n>  \t\t\treturn NULL;\n\nAnother invariant on 'i', though it has the NULL argument as above.\n\n> @@ -528,7 +528,7 @@ static const char *unpack(void)\n>  \t\tkeeper[i++] = \"--fix-thin\";\n>  \t\tkeeper[i++] = hdr_arg;\n>  \t\tkeeper[i++] = keep_arg;\n> -\t\tkeeper[i++] = NULL;\n> +\t\tkeeper[i] = NULL;\n>  \t\tmemset(&ip, 0, sizeof(ip));\n>  \t\tip.argv = keeper;\n>  \t\tip.out = -1;\n\nDitto.\n\n> diff --git a/builtin-send-pack.c b/builtin-send-pack.c\n> index 37e528e..5afd542 100644\n> --- a/builtin-send-pack.c\n> +++ b/builtin-send-pack.c\n> @@ -55,7 +55,7 @@ static int pack_objects(int fd, struct ref *refs, struct extra_have_objects *ext\n>  \tif (args->use_ofs_delta)\n>  \t\targv[i++] = \"--delta-base-offset\";\n>  \tif (args->quiet)\n> -\t\targv[i++] = \"-q\";\n> +\t\targv[i] = \"-q\";\n>  \tmemset(&po, 0, sizeof(po));\n>  \tpo.argv = argv;\n>  \tpo.in = -1;\n\nInvariant on 'i'.\n\n> diff --git a/builtin-show-branch.c b/builtin-show-branch.c\n> index 3510a86..e567eb5 100644\n> --- a/builtin-show-branch.c\n> +++ b/builtin-show-branch.c\n> @@ -191,9 +191,9 @@ static void name_commits(struct commit_list *list,\n>  \t\t\t\t\tbreak;\n>  \t\t\t\t}\n>  \t\t\t\tif (nth == 1)\n> -\t\t\t\t\ten += sprintf(en, \"^\");\n> +\t\t\t\t\tsprintf(en, \"^\");\n>  \t\t\t\telse\n> -\t\t\t\t\ten += sprintf(en, \"^%d\", nth);\n> +\t\t\t\t\tsprintf(en, \"^%d\", nth);\n\nBuilding up string, invariant on 'en'.\n\n\nAnd there are more examples of each. I'm not going to bother labeling\nthem all. But I really think any time you're removing an increment that\nis meant to keep an invariant for future code, we should leave it as-is.\n\n-Peff\n"},{"id":"123868","messageId":"3f4fd2640909261403n78a7e45cm3d2cd48408b5ff52@mail.gmail.com","threadId":"21065","inReplyTo":"20090926204604.GA2960@coredump.intra.peff.net","subject":"Re: [PATCH] Remove various dead assignments and dead increments found by the clang static analyzer","fromName":"Reece Dunn","fromEmail":"msclrhd@googlemail.com","sentAt":"2009-09-26T21:03:27Z","receivedAt":"2009-09-26T21:03:27Z","isPatch":true,"sender":{"key":"msclrhd@googlemail.com","avatar":null},"body":"2009/9/26 Jeff King <peff@peff.net>:\n> On Sat, Sep 26, 2009 at 09:15:58PM +0200, Giuseppe Scrivano wrote:\n>\n>> Here is a cleaned patch.  I think these assignments can be removed\n>> without any problem.\n>\n>> --- a/builtin-receive-pack.c\n>> +++ b/builtin-receive-pack.c\n>> @@ -368,7 +368,7 @@ static char update_post_hook[] = \"hooks/post-update\";\n>>  static void run_update_post_hook(struct command *cmd)\n>>  {\n>>       struct command *cmd_p;\n>> -     int argc, status;\n>> +     int argc;\n>>       const char **argv;\n>>\n>>       for (argc = 0, cmd_p = cmd; cmd_p; cmd_p = cmd_p->next) {\n>> @@ -391,7 +391,7 @@ static void run_update_post_hook(struct command *cmd)\n>>               argc++;\n>>       }\n>>       argv[argc] = NULL;\n>> -     status = run_command_v_opt(argv, RUN_COMMAND_NO_STDIN\n>> +     run_command_v_opt(argv, RUN_COMMAND_NO_STDIN\n>>                       | RUN_COMMAND_STDOUT_TO_STDERR);\n>>  }\n>\n> Now this is one that I do think is sensible. The variable isn't used, so\n> don't even bother declaring it.\n\nThe status variable is removed in this patch.\n\nBut then shouldn't the status returned be checked and acted on? That\nis, are failures from run_command_v_opt being reported to the user, or\notherwise reacted to?\n\nIn this case, IIUC, the status should be returned by the\nrun_update_post_hook function. I.e.:\n\n-  static void run_update_post_hook(struct command *cmd)\n+  static int run_update_post_hook(struct command *cmd)\n  {\n      struct command *cmd_p;\n-     int argc, status;\n+     int argc;\n...\n-     status = run_command_v_opt(argv, RUN_COMMAND_NO_STDIN\n+     return run_command_v_opt(argv, RUN_COMMAND_NO_STDIN\n                      | RUN_COMMAND_STDOUT_TO_STDERR);\n   }\n\nThus having the same effect (removing the status variable). Callers of\nrun_update_post_hook should be checked as well, as should other\nrun_command_* calls.\n\n- Reece\n"},{"id":"123869","messageId":"20090926211220.GA3387@coredump.intra.peff.net","threadId":"21065","inReplyTo":"3f4fd2640909261403n78a7e45cm3d2cd48408b5ff52@mail.gmail.com","subject":"Re: [PATCH] Remove various dead assignments and dead increments found by the clang static analyzer","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-09-26T21:12:20Z","receivedAt":"2009-09-26T21:12:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Sep 26, 2009 at 10:03:27PM +0100, Reece Dunn wrote:\n\n> > Now this is one that I do think is sensible. The variable isn't used, so\n> > don't even bother declaring it.\n> \n> The status variable is removed in this patch.\n\nYes. Sorry if I wasn't clear, but what I meant was \"this does not fall\nunder the same idioms as the other ones, and it is a fine thing to be\nremoving\".\n\n> But then shouldn't the status returned be checked and acted on? That\n> is, are failures from run_command_v_opt being reported to the user, or\n> otherwise reacted to?\n\nPerhaps. This is the post-update hook, so at that point we have already\ncommitted any changes to the repository. Usually it is used for running\n\"git update-server-info\" for repositories available over dumb protocols.\n\nSo there is no useful action for receive-pack to do after seeing an\nerror. But I said \"perhaps\" above, because it might be useful to notify\nthe user over the stderr sideband that the hook failed. Even though we\nhave no action to take, the user might care or want to investigate a\npotential problem.\n\nI suspect nobody has cared about this before, though, because the stderr\nchannel for the hook is also directed to the user. So if\nupdate-server-info (or whatever) fails, presumably it is complaining to\nstderr and the user sees that. Adding an additional \"by the way, your\nhook failed\" is just going to be noise in most cases.\n\n> Thus having the same effect (removing the status variable). Callers of\n> run_update_post_hook should be checked as well, as should other\n> run_command_* calls.\n\nThere is exactly one caller, and it doesn't care about the return code\nfor the reasons mentioned above.\n\n-Peff\n"},{"id":"123870","messageId":"3f4fd2640909261420h2588df4cld8dd3e49f9654e9e@mail.gmail.com","threadId":"21065","inReplyTo":"20090926211220.GA3387@coredump.intra.peff.net","subject":"Re: [PATCH] Remove various dead assignments and dead increments found by the clang static analyzer","fromName":"Reece Dunn","fromEmail":"msclrhd@googlemail.com","sentAt":"2009-09-26T21:20:18Z","receivedAt":"2009-09-26T21:20:18Z","isPatch":true,"sender":{"key":"msclrhd@googlemail.com","avatar":null},"body":"2009/9/26 Jeff King <peff@peff.net>:\n> On Sat, Sep 26, 2009 at 10:03:27PM +0100, Reece Dunn wrote:\n>\n>> > Now this is one that I do think is sensible. The variable isn't used, so\n>> > don't even bother declaring it.\n>>\n>> The status variable is removed in this patch.\n>\n> Yes. Sorry if I wasn't clear, but what I meant was \"this does not fall\n> under the same idioms as the other ones, and it is a fine thing to be\n> removing\".\n\nSure.\n\n>> But then shouldn't the status returned be checked and acted on? That\n>> is, are failures from run_command_v_opt being reported to the user, or\n>> otherwise reacted to?\n>\n> Perhaps. This is the post-update hook, so at that point we have already\n> committed any changes to the repository. Usually it is used for running\n> \"git update-server-info\" for repositories available over dumb protocols.\n>\n> So there is no useful action for receive-pack to do after seeing an\n> error. But I said \"perhaps\" above, because it might be useful to notify\n> the user over the stderr sideband that the hook failed. Even though we\n> have no action to take, the user might care or want to investigate a\n> potential problem.\n>\n> I suspect nobody has cared about this before, though, because the stderr\n> channel for the hook is also directed to the user. So if\n> update-server-info (or whatever) fails, presumably it is complaining to\n> stderr and the user sees that. Adding an additional \"by the way, your\n> hook failed\" is just going to be noise in most cases.\n\nIt could be used to return an error status from main if it is used in\na chained command in a script. Other than that, I agree.\n\n>> Thus having the same effect (removing the status variable). Callers of\n>> run_update_post_hook should be checked as well, as should other\n>> run_command_* calls.\n>\n> There is exactly one caller, and it doesn't care about the return code\n> for the reasons mentioned above.\n\nIncluding being called from a script?\n\n- Reece\n"},{"id":"123871","messageId":"20090926213602.GA3756@coredump.intra.peff.net","threadId":"21065","inReplyTo":"3f4fd2640909261420h2588df4cld8dd3e49f9654e9e@mail.gmail.com","subject":"Re: [PATCH] Remove various dead assignments and dead increments found by the clang static analyzer","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-09-26T21:36:02Z","receivedAt":"2009-09-26T21:36:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Sep 26, 2009 at 10:20:18PM +0100, Reece Dunn wrote:\n\n> > I suspect nobody has cared about this before, though, because the stderr\n> > channel for the hook is also directed to the user. So if\n> > update-server-info (or whatever) fails, presumably it is complaining to\n> > stderr and the user sees that. Adding an additional \"by the way, your\n> > hook failed\" is just going to be noise in most cases.\n> \n> It could be used to return an error status from main if it is used in\n> a chained command in a script. Other than that, I agree.\n\nI'm not sure that's a good idea. Your push _did_ happen, and the remote\nrepo was updated. So you have no way of knowing from an error exit code\nthat changes were in fact made, and it was simply the post-update hook\nfailing.\n\nOf course, you can argue that the current behavior is similarly broken:\non success, you have no idea if the post-update hook failed or not. But\nI would argue that whether the push itself happened is more important\nthan whether the hook succeeded or not. If you really care, you should\neither:\n\n  1. Use some sort of side channel to report hook status.\n\n  2. Use the pre-receive hook, which can abort the push if it wants to.\n\nBut all of that is \"if we were designing this hook from scratch\". At\nthis point, it doesn't make sense to change the semantics. People may be\nrelying on the current behavior, and in fact it is documented (in\ngithooks(5)):\n\n  This hook is meant primarily for notification, and cannot\n  affect the outcome of git-receive-pack.\n\n> > There is exactly one caller, and it doesn't care about the return code\n> > for the reasons mentioned above.\n> \n> Including being called from a script?\n\nI suppose people can be scripting around \"git receive-pack\" itself,\nthough I find it pretty unlikely. I find it much more likely for them to\nscript around \"git push\", which calls receive-pack on the remote end,\nand may or may not get the actual status (without checking, I imagine\nthe exit code is lost anyway for git:// pushes, but probably passed back\nalong for pushes over ssh).\n\nAt any rate, even if we assume that people are scripting around it, and\nthat they can in fact see the exit status, I think we would want to keep\nit the same for compatibility reasons, as mentioned above.\n\n-Peff\n"},{"id":"123872","messageId":"87bpkxcrhy.fsf@master.homenet","threadId":"21065","inReplyTo":"3f4fd2640909261420h2588df4cld8dd3e49f9654e9e@mail.gmail.com","subject":"Re: [PATCH] Remove various dead assignments and dead increments found by the clang static analyzer","fromName":"Giuseppe Scrivano","fromEmail":"gscrivano@gnu.org","sentAt":"2009-09-26T21:42:33Z","receivedAt":"2009-09-26T21:42:33Z","isPatch":true,"sender":{"key":"gscrivano@gnu.org","avatar":"https://avatars.githubusercontent.com/u/67430?v=4"},"body":"Reece Dunn <msclrhd@googlemail.com> writes:\n\n>> There is exactly one caller, and it doesn't care about the return code\n>> for the reasons mentioned above.\n>\n> Including being called from a script?\n\nI agree, if something goes wrong then it should be reported.  The same\napplies to the `run_receive_hook' return code that is not checked in\n`cmd_receive_pack'.\n\nConsidering you want to keep the current source code invariants, and I\ndon't have any objection to it, probably the only assignment that can be\nremoved is the following one:\n\n>From f8dd14bf4c3f3e132f6a8e13bf3e2fc575a804b1 Mon Sep 17 00:00:00 2001\nFrom: Giuseppe Scrivano <gscrivano@gnu.org>\nDate: Sat, 26 Sep 2009 23:23:13 +0200\nSubject: [PATCH] Remove a dead assignment found by the clang static analyzer\n\n---\n http-fetch.c |    3 +--\n 1 files changed, 1 insertions(+), 2 deletions(-)\n\ndiff --git a/http-fetch.c b/http-fetch.c\nindex e8f44ba..6879904 100644\n--- a/http-fetch.c\n+++ b/http-fetch.c\n@@ -3,7 +3,6 @@\n \n int main(int argc, const char **argv)\n {\n-\tconst char *prefix;\n \tstruct walker *walker;\n \tint commits_on_stdin = 0;\n \tint commits;\n@@ -19,7 +18,7 @@ int main(int argc, const char **argv)\n \tint get_verbosely = 0;\n \tint get_recover = 0;\n \n-\tprefix = setup_git_directory();\n+\tsetup_git_directory();\n \n \tgit_config(git_default_config, NULL);\n \n-- \n1.6.3.3\n"},{"id":"123873","messageId":"3f4fd2640909261446t412d0c26mcee27535be2b8954@mail.gmail.com","threadId":"21065","inReplyTo":"20090926213602.GA3756@coredump.intra.peff.net","subject":"Re: [PATCH] Remove various dead assignments and dead increments found by the clang static analyzer","fromName":"Reece Dunn","fromEmail":"msclrhd@googlemail.com","sentAt":"2009-09-26T21:46:06Z","receivedAt":"2009-09-26T21:46:06Z","isPatch":true,"sender":{"key":"msclrhd@googlemail.com","avatar":null},"body":"2009/9/26 Jeff King <peff@peff.net>:\n> On Sat, Sep 26, 2009 at 10:20:18PM +0100, Reece Dunn wrote:\n>\n>> > I suspect nobody has cared about this before, though, because the stderr\n>> > channel for the hook is also directed to the user. So if\n>> > update-server-info (or whatever) fails, presumably it is complaining to\n>> > stderr and the user sees that. Adding an additional \"by the way, your\n>> > hook failed\" is just going to be noise in most cases.\n>>\n>> It could be used to return an error status from main if it is used in\n>> a chained command in a script. Other than that, I agree.\n>\n> I'm not sure that's a good idea. Your push _did_ happen, and the remote\n> repo was updated. So you have no way of knowing from an error exit code\n> that changes were in fact made, and it was simply the post-update hook\n> failing.\n\nOk.\n\n> Of course, you can argue that the current behavior is similarly broken:\n> on success, you have no idea if the post-update hook failed or not. But\n> I would argue that whether the push itself happened is more important\n> than whether the hook succeeded or not. If you really care, you should\n> either:\n>\n>  1. Use some sort of side channel to report hook status.\n>\n>  2. Use the pre-receive hook, which can abort the push if it wants to.\n>\n> But all of that is \"if we were designing this hook from scratch\". At\n> this point, it doesn't make sense to change the semantics. People may be\n> relying on the current behavior, and in fact it is documented (in\n> githooks(5)):\n>\n>  This hook is meant primarily for notification, and cannot\n>  affect the outcome of git-receive-pack.\n\nThat's fine. As long as the behaviour is documented (which as you\npointed out, it is).\n\n- Reece\n"},{"id":"123878","messageId":"alpine.LFD.2.00.0909262038470.4997@xanadu.home","threadId":"21065","inReplyTo":"20090926204604.GA2960@coredump.intra.peff.net","subject":"Re: [PATCH] Remove various dead assignments and dead increments found by the clang static analyzer","fromName":"Nicolas Pitre","fromEmail":"nico@fluxnic.net","sentAt":"2009-09-27T00:41:35Z","receivedAt":"2009-09-27T00:41:35Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Sat, 26 Sep 2009, Jeff King wrote:\n\n> On Sat, Sep 26, 2009 at 09:15:58PM +0200, Giuseppe Scrivano wrote:\n> \n> > Here is a cleaned patch.  I think these assignments can be removed\n> > without any problem.\n> \n> I don't agree. For example:\n> \n> > --- a/builtin-fetch--tool.c\n> > +++ b/builtin-fetch--tool.c\n> > @@ -169,7 +169,7 @@ static int append_fetch_head(FILE *fp,\n> >  \t\t\tnote_len += sprintf(note + note_len, \"%s \", kind);\n> >  \t\tnote_len += sprintf(note + note_len, \"'%s' of \", what);\n> >  \t}\n> > -\tnote_len += sprintf(note + note_len, \"%.*s\", remote_len, remote);\n> > +\tsprintf(note + note_len, \"%.*s\", remote_len, remote);\n> \n> This is a very particular C idiom: you are building a string over\n> several statements using a function that adds to the string and tells\n> you how much it added. The implicit invariant of the note_len variable\n> is that it _always_ contains the current length, so each statement uses\n> it as input and pushes it forward on output.\n> \n> Any experienced C programmer should look at that and be able to see\n> exactly what's going on. And people adding more lines don't need to\n> munge the existing lines; the invariant property of note_len means they\n> just need to add more, similar lines.\n> \n> But your patch destroys that invariant. It makes it harder to see what's\n> going on, because it breaks the idiom. And it makes it more likely for\n> somebody adding a line further on to make a mistake (and certainly it\n> makes their patch harder to read and review, as they have to munge\n> unrelated lines).\n> \n> So no, while there is no code _now_ that is relying on the invariant\n> being kept after the last statement (which is what the static analyzer\n> is finding out), the point is not for the compiler to realize that, but\n> for human programmers to see it.\n\nAnd the compiler (at least gcc) is indeed smart enough to realize that \nnothing uses the result from the last statement, and does optimize away \nthe code associated to it already.  So this patch is unlikely to change \nanything to the compiled result.\n\n\nNicolas\n"},{"id":"123889","messageId":"874oqokdc2.fsf@master.homenet","threadId":"21065","inReplyTo":"alpine.LFD.2.00.0909262038470.4997@xanadu.home","subject":"Re: [PATCH] Remove various dead assignments and dead increments found by the clang static analyzer","fromName":"Giuseppe Scrivano","fromEmail":"gscrivano@gnu.org","sentAt":"2009-09-27T08:21:17Z","receivedAt":"2009-09-27T08:21:17Z","isPatch":true,"sender":{"key":"gscrivano@gnu.org","avatar":"https://avatars.githubusercontent.com/u/67430?v=4"},"body":"Nicolas Pitre <nico@fluxnic.net> writes:\n\n> And the compiler (at least gcc) is indeed smart enough to realize that \n> nothing uses the result from the last statement, and does optimize away \n> the code associated to it already.  So this patch is unlikely to change \n> anything to the compiled result.\n\nRight, and gcc can do many other amazing things.  But still it is used a\nvariable that is never accessed, removing it can make the code slightly\nmore readable.\n\nCheers,\nGiuseppe\n"}]}