Remove various dead assignments and dead increments found by the clang static analyzer
PATCH 1/1 Remove various dead assignments and dead increments found by the clang static analyzer
Hello,
I tried the clang static analyzer on the git source code, this patch fixes the found dead assignments/increments.
Regards,
Giuseppe Scrivano
Quoted 1 line
From
88fe9b63d159ad1fd0579564558fbf0f900bd8e3Mon Sep 17 00:00:00 2001
From: Giuseppe Scrivano <[email protected]>
Date: Sat, 26 Sep 2009 16:34:56 +0200
Subject: [PATCH] Remove various dead assignments and dead increments found by the clang static analyzer
--- archive.c | 2 +- builtin-add.c | 2 +- builtin-bisect--helper.c | 2 +- builtin-commit.c | 2 +- builtin-fetch--tool.c | 2 +- builtin-fetch-pack.c | 2 +- builtin-grep.c | 2 -- builtin-help.c | 2 +- builtin-ls-files.c | 2 +- builtin-mktree.c | 2 +- builtin-pack-objects.c | 2 +- builtin-prune-packed.c | 2 +- builtin-receive-pack.c | 6 +++--- builtin-rev-parse.c | 2 +- builtin-send-pack.c | 2 +- builtin-show-branch.c | 4 ++-- builtin-show-ref.c | 2 +- builtin-write-tree.c | 2 +- color.c | 2 +- compat/mkstemps.c | 2 +- connect.c | 1 - diff.c | 2 +- http-fetch.c | 3 +-- transport.c | 4 ++-- upload-pack.c | 2 +- 25 files changed, 27 insertions(+), 31 deletions(-)
archive.c +1 -1
@@ -357,7 +357,7 @@ int write_archive(int argc, const char **argv, const char *prefix,const struct archiver *ar =NULL ;struct archiver_args args; - argc =parse_archive_args (argc, argv, &ar, &args); +parse_archive_args (argc, argv, &ar, &args);if (setup_prefix && prefix ==NULL ) prefix =setup_git_directory ();
builtin-add.c +1 -1
@@ -193,7 +193,7 @@ static int edit_patch(int argc, const char **argv, const char *prefix)init_revisions (&rev, prefix); rev.diffopt.context =7 ; - argc =setup_revisions (argc, argv, &rev,NULL ); +setup_revisions (argc, argv, &rev,NULL ); rev.diffopt.output_format =DIFF_FORMAT_PATCH ; out =open (file,O_CREAT |O_WRONLY ,0644 );if (out <0 )
builtin-bisect--helper.c +1 -1
@@ -17,7 +17,7 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix) OPT_END() }; - argc =parse_options (argc, argv, prefix, options, +parse_options (argc, argv, prefix, options, git_bisect_helper_usage,0 );if (!next_all)
builtin-commit.c +1 -1
@@ -1035,7 +1035,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix) parents =reduce_heads (parents); }else { reflog_msg ="commit" ; - pptr = &commit_list_insert (lookup_commit (head_sha1), pptr)->next; +commit_list_insert (lookup_commit (head_sha1), pptr)->next; }/* Finally, get the commit message */
builtin-fetch--tool.c +1 -1
@@ -169,7 +169,7 @@ static int append_fetch_head(FILE *fp, note_len +=sprintf (note + note_len,"%s " , kind); note_len +=sprintf (note + note_len,"'%s' of " , what); } - note_len +=sprintf (note + note_len,"%.*s" , remote_len, remote); +sprintf (note + note_len,"%.*s" , remote_len, remote); fprintf(fp, "%s\t%s\t%s\n", sha1_to_hex(commit ? commit->object.sha1 : sha1), not_for_merge ? "not-for-merge" : "",
builtin-fetch-pack.c +1 -1
@@ -555,7 +555,7 @@ static int get_pack(int xd[2], char **pack_lockfile) }if (*hdr_arg) *av++ = hdr_arg; - *av++ =NULL ; + *av =NULL ; cmd.in = demux.out; cmd.git_cmd =1 ;
builtin-grep.c +0 -2
@@ -400,7 +400,6 @@ static int external_grep(struct grep_opt *opt, const char **paths, int cached)if (sizeof (randarg) <= len)die ("maximum length of args exceeded" );push_arg (argptr); - argptr += len; } }else {
@@ -410,7 +409,6 @@ static int external_grep(struct grep_opt *opt, const char **paths, int cached)if (sizeof (randarg) <= len)die ("maximum length of args exceeded" );push_arg (argptr); - argptr += len; } }for (p = opt->pattern_list; p; p = p->next) {
builtin-help.c +1 -1
@@ -419,7 +419,7 @@ int cmd_help(int argc, const char **argv, const char *prefix)setup_git_directory_gently (&nongit);git_config (git_help_config,NULL ); - argc =parse_options (argc, argv, prefix, builtin_help_options, +parse_options (argc, argv, prefix, builtin_help_options, builtin_help_usage,0 );if (show_all) {
builtin-ls-files.c +1 -1
@@ -481,7 +481,7 @@ int cmd_ls_files(int argc, const char **argv, const char *prefix) prefix_offset =strlen (prefix);git_config (git_default_config,NULL ); - argc =parse_options (argc, argv, prefix, builtin_ls_files_options, +parse_options (argc, argv, prefix, builtin_ls_files_options, ls_files_usage,0 );if (show_tag || show_valid_bit) { tag_cached ="H " ;
builtin-mktree.c +1 -1
@@ -155,7 +155,7 @@ int cmd_mktree(int ac, const char **av, const char *prefix) OPT_END() }; - ac =parse_options (ac, av, prefix, option, mktree_usage,0 ); +parse_options (ac, av, prefix, option, mktree_usage,0 );while (!got_eof) {while (1 ) {
builtin-pack-objects.c +1 -1
@@ -2307,7 +2307,7 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)*/ if (!pack_to_stdout) - base_name = argv[i++]; + base_name = argv[i];if (pack_to_stdout != !base_name)usage (pack_usage);
builtin-prune-packed.c +1 -1
@@ -78,7 +78,7 @@ int cmd_prune_packed(int argc, const char **argv, const char *prefix) OPT_END() }; - argc =parse_options (argc, argv, prefix, prune_packed_options, +parse_options (argc, argv, prefix, prune_packed_options, prune_packed_usage,0 );prune_packed_objects (opts);
builtin-receive-pack.c +3 -3
@@ -391,7 +391,7 @@ static void run_update_post_hook(struct command *cmd) argc++; } argv[argc] =NULL ; - status =run_command_v_opt (argv,RUN_COMMAND_NO_STDIN +run_command_v_opt (argv,RUN_COMMAND_NO_STDIN |RUN_COMMAND_STDOUT_TO_STDERR ); }
@@ -506,7 +506,7 @@ static const char *unpack(void)if (receive_fsck_objects) unpacker[i++] ="--strict" ; unpacker[i++] = hdr_arg; - unpacker[i++] =NULL ; + unpacker[i] =NULL ; code =run_command_v_opt (unpacker,RUN_GIT_CMD );if (!code)return NULL ;
@@ -528,7 +528,7 @@ static const char *unpack(void) keeper[i++] ="--fix-thin" ; keeper[i++] = hdr_arg; keeper[i++] = keep_arg; - keeper[i++] =NULL ; + keeper[i] =NULL ;memset (&ip,0 ,sizeof (ip)); ip.argv = keeper; ip.out =-1 ;
builtin-rev-parse.c +1 -1
@@ -396,7 +396,7 @@ static int cmd_parseopt(int argc, const char **argv, const char *prefix)/* put an OPT_END() */ ALLOC_GROW (opts, onb +1 , osz);memset (opts + onb,0 ,sizeof (opts[onb])); - argc =parse_options (argc, argv, prefix, opts, usage, +parse_options (argc, argv, prefix, opts, usage, keep_dashdash ?PARSE_OPT_KEEP_DASHDASH :0 | stop_at_non_option ?PARSE_OPT_STOP_AT_NON_OPTION :0 );
builtin-send-pack.c +1 -1
@@ -55,7 +55,7 @@ static int pack_objects(int fd, struct ref *refs, struct extra_have_objects *extif (args->use_ofs_delta) argv[i++] ="--delta-base-offset" ;if (args->quiet) - argv[i++] ="-q" ; + argv[i] ="-q" ;memset (&po,0 ,sizeof (po)); po.argv = argv; po.in =-1 ;
builtin-show-branch.c +2 -2
@@ -191,9 +191,9 @@ static void name_commits(struct commit_list *list,break ; }if (nth ==1 ) - en +=sprintf (en,"^" ); +sprintf (en,"^" );else - en +=sprintf (en,"^%d" , nth); +sprintf (en,"^%d" , nth);name_commit (p,xstrdup (newname),0 ); i++;name_first_parent_chain (p);
builtin-show-ref.c +1 -1
@@ -201,7 +201,7 @@ static const struct option show_ref_options[] = {int cmd_show_ref (int argc,const char **argv,const char *prefix) { - argc =parse_options (argc, argv, prefix, show_ref_options, +parse_options (argc, argv, prefix, show_ref_options, show_ref_usage,PARSE_OPT_NO_INTERNAL_HELP );if (exclude_arg)
builtin-write-tree.c +1 -1
@@ -34,7 +34,7 @@ int cmd_write_tree(int argc, const char **argv, const char *unused_prefix) };git_config (git_default_config,NULL ); - argc =parse_options (argc, argv, unused_prefix, write_tree_options, +parse_options (argc, argv, unused_prefix, write_tree_options, write_tree_usage,0 ); ret =write_cache_as_tree (sha1, flags, prefix);
color.c +1 -1
@@ -110,7 +110,7 @@ void color_parse_mem(const char *value, int value_len, const char *var, } }if (bg >=0 ) { -if (sep++) +if (sep) *dst++ =';' ;if (bg <8 ) { *dst++ ='4' ;
compat/mkstemps.c +1 -1
@@ -45,7 +45,7 @@ int gitmkstemps(char *pattern, int suffix_len) template[2 ] = letters[v % num_letters]; v /= num_letters; template[3 ] = letters[v % num_letters]; v /= num_letters; template[4 ] = letters[v % num_letters]; v /= num_letters; - template[5 ] = letters[v % num_letters]; v /= num_letters; + template[5 ] = letters[v % num_letters]; fd =open (pattern,O_CREAT |O_EXCL |O_RDWR ,0600 );if (fd >0 )
connect.c +0 -1
@@ -18,7 +18,6 @@ static int check_ref(const char *name, int len, unsigned int flags)/* Skip the "refs/" part */ name +=5 ; - len -=5 ;/* REF_NORMAL means that we don't want the magic fake tag refs */ if ((flags &REF_NORMAL ) &&check_ref_format (name) <0 )
diff.c +1 -1
@@ -901,7 +901,7 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)/* Find the longest filename and max number of changes */ reset =diff_get_color_opt (options,DIFF_RESET ); - set =diff_get_color_opt (options,DIFF_PLAIN ); +diff_get_color_opt (options,DIFF_PLAIN ); add_c =diff_get_color_opt (options,DIFF_FILE_NEW ); del_c =diff_get_color_opt (options,DIFF_FILE_OLD );
http-fetch.c +1 -2
@@ -3,7 +3,6 @@int main (int argc,const char **argv) { -const char *prefix;struct walker *walker;int commits_on_stdin =0 ;int commits;
@@ -19,7 +18,7 @@ int main(int argc, const char **argv)int get_verbosely =0 ;int get_recover =0 ; - prefix =setup_git_directory (); +setup_git_directory ();git_config (git_default_config,NULL );
transport.c +2 -2
@@ -308,7 +308,7 @@ static int rsync_transport_push(struct transport *transport, args[i++] ="info" ; args[i++] =get_object_directory (); args[i++] = buf.buf; - args[i++] =NULL ; + args[i] =NULL ;if (run_command (&rsync)) return error("Could not push objects to %s",
@@ -334,7 +334,7 @@ static int rsync_transport_push(struct transport *transport, args[i++] ="--ignore-existing" ; args[i++] = temp_dir.buf; args[i++] =rsync_url (transport->url); - args[i++] =NULL ; + args[i] =NULL ;if (run_command (&rsync)) result =error ("Could not push to %s" ,rsync_url (transport->url));
upload-pack.c +1 -1
@@ -241,7 +241,7 @@ static void create_pack_file(void) argv[arg++] ="--delta-base-offset" ;if (use_include_tag) argv[arg++] ="--include-tag" ; - argv[arg++] =NULL ; + argv[arg] =NULL ;memset (&pack_objects,0 ,sizeof (pack_objects)); pack_objects.in = shallow_nr ? rev_list.out :-1 ;
Signature · 1 line
1.6.3.3
Hi,
Quoted 8 lines · Giuseppe Scrivano
I tried the clang static analyzer on the git source code, this patch fixes the found dead assignments/increments.
archive.c +0
-0
archive.c int write_archive(int argc, const char **argv, const char *prefix, · line 362
Quoted 7 lines
archive.c +1
-1@@ -357,7 +357,7 @@ int write_archive(int argc, const char **argv, const char *prefix,const struct archiver *ar =NULL ;struct archiver_args args; - argc =parse_archive_args (argc, argv, &ar, &args); +parse_archive_args (argc, argv, &ar, &args);if (setup_prefix && prefix ==NULL ) prefix =setup_git_directory ();
I understand that clang complains when argc is not really used afterwards, but do we really want to do this? I mean, if somebody decides it'd be a good idea to check the number of arguments after parsing the arguments, they might be bitten by the fact that it is now actively wrong.
Ciao,
Dscho
Hello,
Quoted 4 lines · Johannes Schindelin
I understand that clang complains when argc is not really used afterwards, but do we really want to do this? I mean, if somebody decides it'd be a good idea to check the number of arguments after parsing the arguments, they might be bitten by the fact that it is now actively wrong.
probably this is not the only case to leave as it is. I just cleaned anything clang reported.
Cheers,
Giuseppe
Heya,
Quoted 2 lines · Giuseppe Scrivano
probably this is not the only case to leave as it is. I just cleaned anything clang reported.
Then it would probably have been better to say so by at least marking your patch as RFC and including such a remark in the cover letter, no? Also, now that this has been pointed out, you shouldn't expect it to be included until someone either takes your patch and cleans it up (as in, checks all statements manually), or until you do so yourself.
Signature · 3 lines
Cheers, Sverre Rabbelier
Here is a cleaned patch. I think these assignments can be removed without any problem.
Cheers,
Giuseppe
Quoted 1 line
From
7501d82998132b15ad5cda78c0650f4f4a0b0e93Mon Sep 17 00:00:00 2001
From: Giuseppe Scrivano <[email protected]>
Date: Sat, 26 Sep 2009 21:11:21 +0200
Subject: [PATCH] Remove various dead assignments and dead increments found by the clang static analyzer
--- builtin-commit.c | 2 +- builtin-fetch--tool.c | 2 +- builtin-fetch-pack.c | 2 +- builtin-grep.c | 2 -- builtin-pack-objects.c | 2 +- builtin-receive-pack.c | 8 ++++---- builtin-send-pack.c | 2 +- builtin-show-branch.c | 4 ++-- color.c | 2 +- compat/mkstemps.c | 2 +- connect.c | 1 - diff.c | 2 +- http-fetch.c | 3 +-- transport.c | 4 ++-- upload-pack.c | 2 +- 15 files changed, 18 insertions(+), 22 deletions(-)
builtin-commit.c +1 -1
@@ -1035,7 +1035,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix) parents =reduce_heads (parents); }else { reflog_msg ="commit" ; - pptr = &commit_list_insert (lookup_commit (head_sha1), pptr)->next; +commit_list_insert (lookup_commit (head_sha1), pptr); }/* Finally, get the commit message */
builtin-fetch--tool.c +1 -1
@@ -169,7 +169,7 @@ static int append_fetch_head(FILE *fp, note_len +=sprintf (note + note_len,"%s " , kind); note_len +=sprintf (note + note_len,"'%s' of " , what); } - note_len +=sprintf (note + note_len,"%.*s" , remote_len, remote); +sprintf (note + note_len,"%.*s" , remote_len, remote); fprintf(fp, "%s\t%s\t%s\n", sha1_to_hex(commit ? commit->object.sha1 : sha1), not_for_merge ? "not-for-merge" : "",
builtin-fetch-pack.c +1 -1
@@ -555,7 +555,7 @@ static int get_pack(int xd[2], char **pack_lockfile) }if (*hdr_arg) *av++ = hdr_arg; - *av++ =NULL ; + *av =NULL ; cmd.in = demux.out; cmd.git_cmd =1 ;
builtin-grep.c +0 -2
@@ -400,7 +400,6 @@ static int external_grep(struct grep_opt *opt, const char **paths, int cached)if (sizeof (randarg) <= len)die ("maximum length of args exceeded" );push_arg (argptr); - argptr += len; } }else {
@@ -410,7 +409,6 @@ static int external_grep(struct grep_opt *opt, const char **paths, int cached)if (sizeof (randarg) <= len)die ("maximum length of args exceeded" );push_arg (argptr); - argptr += len; } }for (p = opt->pattern_list; p; p = p->next) {
builtin-pack-objects.c +1 -1
@@ -2307,7 +2307,7 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)*/ if (!pack_to_stdout) - base_name = argv[i++]; + base_name = argv[i];if (pack_to_stdout != !base_name)usage (pack_usage);
builtin-receive-pack.c +4 -4
@@ -368,7 +368,7 @@ static char update_post_hook[] = "hooks/post-update";static void run_update_post_hook (struct command *cmd) {struct command *cmd_p; -int argc, status; +int argc;const char **argv;for (argc =0 , cmd_p = cmd; cmd_p; cmd_p = cmd_p->next) {
@@ -391,7 +391,7 @@ static void run_update_post_hook(struct command *cmd) argc++; } argv[argc] =NULL ; - status =run_command_v_opt (argv,RUN_COMMAND_NO_STDIN +run_command_v_opt (argv,RUN_COMMAND_NO_STDIN |RUN_COMMAND_STDOUT_TO_STDERR ); }
@@ -506,7 +506,7 @@ static const char *unpack(void)if (receive_fsck_objects) unpacker[i++] ="--strict" ; unpacker[i++] = hdr_arg; - unpacker[i++] =NULL ; + unpacker[i] =NULL ; code =run_command_v_opt (unpacker,RUN_GIT_CMD );if (!code)return NULL ;
@@ -528,7 +528,7 @@ static const char *unpack(void) keeper[i++] ="--fix-thin" ; keeper[i++] = hdr_arg; keeper[i++] = keep_arg; - keeper[i++] =NULL ; + keeper[i] =NULL ;memset (&ip,0 ,sizeof (ip)); ip.argv = keeper; ip.out =-1 ;
builtin-send-pack.c +1 -1
@@ -55,7 +55,7 @@ static int pack_objects(int fd, struct ref *refs, struct extra_have_objects *extif (args->use_ofs_delta) argv[i++] ="--delta-base-offset" ;if (args->quiet) - argv[i++] ="-q" ; + argv[i] ="-q" ;memset (&po,0 ,sizeof (po)); po.argv = argv; po.in =-1 ;
builtin-show-branch.c +2 -2
@@ -191,9 +191,9 @@ static void name_commits(struct commit_list *list,break ; }if (nth ==1 ) - en +=sprintf (en,"^" ); +sprintf (en,"^" );else - en +=sprintf (en,"^%d" , nth); +sprintf (en,"^%d" , nth);name_commit (p,xstrdup (newname),0 ); i++;name_first_parent_chain (p);
color.c +1 -1
@@ -110,7 +110,7 @@ void color_parse_mem(const char *value, int value_len, const char *var, } }if (bg >=0 ) { -if (sep++) +if (sep) *dst++ =';' ;if (bg <8 ) { *dst++ ='4' ;
compat/mkstemps.c +1 -1
@@ -45,7 +45,7 @@ int gitmkstemps(char *pattern, int suffix_len) template[2 ] = letters[v % num_letters]; v /= num_letters; template[3 ] = letters[v % num_letters]; v /= num_letters; template[4 ] = letters[v % num_letters]; v /= num_letters; - template[5 ] = letters[v % num_letters]; v /= num_letters; + template[5 ] = letters[v % num_letters]; fd =open (pattern,O_CREAT |O_EXCL |O_RDWR ,0600 );if (fd >0 )
connect.c +0 -1
@@ -18,7 +18,6 @@ static int check_ref(const char *name, int len, unsigned int flags)/* Skip the "refs/" part */ name +=5 ; - len -=5 ;/* REF_NORMAL means that we don't want the magic fake tag refs */ if ((flags &REF_NORMAL ) &&check_ref_format (name) <0 )
diff.c +1 -1
@@ -901,7 +901,7 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)/* Find the longest filename and max number of changes */ reset =diff_get_color_opt (options,DIFF_RESET ); - set =diff_get_color_opt (options,DIFF_PLAIN ); +diff_get_color_opt (options,DIFF_PLAIN ); add_c =diff_get_color_opt (options,DIFF_FILE_NEW ); del_c =diff_get_color_opt (options,DIFF_FILE_OLD );
http-fetch.c +1 -2
@@ -3,7 +3,6 @@int main (int argc,const char **argv) { -const char *prefix;struct walker *walker;int commits_on_stdin =0 ;int commits;
@@ -19,7 +18,7 @@ int main(int argc, const char **argv)int get_verbosely =0 ;int get_recover =0 ; - prefix =setup_git_directory (); +setup_git_directory ();git_config (git_default_config,NULL );
transport.c +2 -2
@@ -308,7 +308,7 @@ static int rsync_transport_push(struct transport *transport, args[i++] ="info" ; args[i++] =get_object_directory (); args[i++] = buf.buf; - args[i++] =NULL ; + args[i] =NULL ;if (run_command (&rsync)) return error("Could not push objects to %s",
@@ -334,7 +334,7 @@ static int rsync_transport_push(struct transport *transport, args[i++] ="--ignore-existing" ; args[i++] = temp_dir.buf; args[i++] =rsync_url (transport->url); - args[i++] =NULL ; + args[i] =NULL ;if (run_command (&rsync)) result =error ("Could not push to %s" ,rsync_url (transport->url));
upload-pack.c +1 -1
@@ -241,7 +241,7 @@ static void create_pack_file(void) argv[arg++] ="--delta-base-offset" ;if (use_include_tag) argv[arg++] ="--include-tag" ; - argv[arg++] =NULL ; + argv[arg] =NULL ;memset (&pack_objects,0 ,sizeof (pack_objects)); pack_objects.in = shallow_nr ? rev_list.out :-1 ;
Signature · 1 line
1.6.3.3
Quoted 2 lines · Giuseppe Scrivano
Here is a cleaned patch. I think these assignments can be removed without any problem.
I don't agree. For example:
builtin-fetch--tool.c static int append_fetch_head(FILE *fp, · line 172
Quoted 8 lines
builtin-fetch--tool.c +1
-1@@ -169,7 +169,7 @@ static int append_fetch_head(FILE *fp, note_len +=sprintf (note + note_len,"%s " , kind); note_len +=sprintf (note + note_len,"'%s' of " , what); } - note_len +=sprintf (note + note_len,"%.*s" , remote_len, remote); +sprintf (note + note_len,"%.*s" , remote_len, remote);
This is a very particular C idiom: you are building a string over several statements using a function that adds to the string and tells you how much it added. The implicit invariant of the note_len variable is that it _always_ contains the current length, so each statement uses it as input and pushes it forward on output.
Any experienced C programmer should look at that and be able to see exactly what's going on. And people adding more lines don't need to munge the existing lines; the invariant property of note_len means they just need to add more, similar lines.
But your patch destroys that invariant. It makes it harder to see what's going on, because it breaks the idiom. And it makes it more likely for somebody adding a line further on to make a mistake (and certainly it makes their patch harder to read and review, as they have to munge unrelated lines).
So no, while there is no code _now_ that is relying on the invariant being kept after the last statement (which is what the static analyzer is finding out), the point is not for the compiler to realize that, but for human programmers to see it.
So I think your version is less readable and maintainable. And it doesn't even introduce any efficiency; any decent compiler should be able to optimize out the addition.
builtin-fetch-pack.c static int get_pack(int xd[2], char **pack_lockfile) · line 558
Quoted 8 lines
builtin-fetch-pack.c +1
-1@@ -555,7 +555,7 @@ static int get_pack(int xd[2], char **pack_lockfile) }if (*hdr_arg) *av++ = hdr_arg; - *av++ =NULL ; + *av =NULL ;
I would argue a similar same idiom exists here, though given that NULL by definition is ending the av list it is somewhat less strong (i.e., there is already something to that statement that indicates that it _must_ be the last one).
builtin-pack-objects.c int cmd_pack_objects(int argc, const char **argv, const char *prefix) · line 2310
Quoted 8 lines
builtin-pack-objects.c +1
-1@@ -2307,7 +2307,7 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)*/ if (!pack_to_stdout) - base_name = argv[i++]; + base_name = argv[i];
And again here. Maintaining the invariant on 'i' is important to readability and maintainability.
Quoted 12 lines
builtin-receive-pack.c +1
-1@@ -368,7 +368,7 @@ static char update_post_hook[] = "hooks/post-update";static void run_update_post_hook (struct command *cmd) {struct command *cmd_p; -int argc, status; +int argc;const char **argv;for (argc =0 , cmd_p = cmd; cmd_p; cmd_p = cmd_p->next) {
builtin-receive-pack.c static void run_update_post_hook(struct command *cmd) · line 396
Quoted 7 lines
builtin-receive-pack.c +1
-1@@ -391,7 +391,7 @@ static void run_update_post_hook(struct command *cmd) argc++; } argv[argc] =NULL ; - status =run_command_v_opt (argv,RUN_COMMAND_NO_STDIN +run_command_v_opt (argv,RUN_COMMAND_NO_STDIN |RUN_COMMAND_STDOUT_TO_STDERR ); }
Now this is one that I do think is sensible. The variable isn't used, so don't even bother declaring it.
builtin-receive-pack.c static const char *unpack(void) · line 512
Quoted 9 lines
builtin-receive-pack.c +1
-1@@ -506,7 +506,7 @@ static const char *unpack(void)if (receive_fsck_objects) unpacker[i++] ="--strict" ; unpacker[i++] = hdr_arg; - unpacker[i++] =NULL ; + unpacker[i] =NULL ; code =run_command_v_opt (unpacker,RUN_GIT_CMD );if (!code)return NULL ;
Another invariant on 'i', though it has the NULL argument as above.
builtin-receive-pack.c static const char *unpack(void) · line 534
Quoted 9 lines
builtin-receive-pack.c +1
-1@@ -528,7 +528,7 @@ static const char *unpack(void) keeper[i++] ="--fix-thin" ; keeper[i++] = hdr_arg; keeper[i++] = keep_arg; - keeper[i++] =NULL ; + keeper[i] =NULL ;memset (&ip,0 ,sizeof (ip)); ip.argv = keeper; ip.out =-1 ;
Ditto.
builtin-send-pack.c static int pack_objects(int fd, struct ref *refs, struct extra_have_objects *ext · line 61
Quoted 13 lines
builtin-send-pack.c +1
-1@@ -55,7 +55,7 @@ static int pack_objects(int fd, struct ref *refs, struct extra_have_objects *extif (args->use_ofs_delta) argv[i++] ="--delta-base-offset" ;if (args->quiet) - argv[i++] ="-q" ; + argv[i] ="-q" ;memset (&po,0 ,sizeof (po)); po.argv = argv; po.in =-1 ;
Invariant on 'i'.
builtin-show-branch.c static void name_commits(struct commit_list *list, · line 196
Quoted 13 lines
builtin-show-branch.c +2
-2@@ -191,9 +191,9 @@ static void name_commits(struct commit_list *list,break ; }if (nth ==1 ) - en +=sprintf (en,"^" ); +sprintf (en,"^" );else - en +=sprintf (en,"^%d" , nth); +sprintf (en,"^%d" , nth);
Building up string, invariant on 'en'.
And there are more examples of each. I'm not going to bother labeling them all. But I really think any time you're removing an increment that is meant to keep an invariant for future code, we should leave it as-is.
-Peff
2009/9/26 Jeff King <[email protected]>:
Quoted 15 lines · Giuseppe Scrivano
Quoted 14 lines
Here is a cleaned patch. I think these assignments can be removed without any problem.
builtin-receive-pack.c +1
-1@@ -368,7 +368,7 @@ static char update_post_hook[] = "hooks/post-update"; static void run_update_post_hook(struct command *cmd) { struct command *cmd_p; - int argc, status; + int argc; const char **argv;
Quoted 12 lines
builtin-receive-pack.c static void run_update_post_hook(struct command *cmd) · line 396
Quoted 9 lines
builtin-receive-pack.c +1
-1for (argc = 0, cmd_p = cmd; cmd_p; cmd_p = cmd_p->next) {@@ -391,7 +391,7 @@ static void run_update_post_hook(struct command *cmd) argc++; } argv[argc] = NULL; - status = run_command_v_opt(argv, RUN_COMMAND_NO_STDIN + run_command_v_opt(argv, RUN_COMMAND_NO_STDIN | RUN_COMMAND_STDOUT_TO_STDERR); }Now this is one that I do think is sensible. The variable isn't used, so don't even bother declaring it.
The status variable is removed in this patch.
But then shouldn't the status returned be checked and acted on? That is, are failures from run_command_v_opt being reported to the user, or otherwise reacted to?
In this case, IIUC, the status should be returned by the run_update_post_hook function. I.e.:
- static void run_update_post_hook(struct command *cmd)
+ static int run_update_post_hook(struct command *cmd)
{
struct command *cmd_p;- int argc, status;
+ int argc;
...
- status = run_command_v_opt(argv, RUN_COMMAND_NO_STDIN
+ return run_command_v_opt(argv, RUN_COMMAND_NO_STDIN
| RUN_COMMAND_STDOUT_TO_STDERR);
}
Thus having the same effect (removing the status variable). Callers of run_update_post_hook should be checked as well, as should other run_command_* calls.
- Reece
Quoted 4 lines · Reece Dunn
Quoted 2 lines
Now this is one that I do think is sensible. The variable isn't used, so don't even bother declaring it.
The status variable is removed in this patch.
Yes. Sorry if I wasn't clear, but what I meant was "this does not fall under the same idioms as the other ones, and it is a fine thing to be removing".
Quoted 3 lines
But then shouldn't the status returned be checked and acted on? That is, are failures from
run_command_v_optbeing reported to the user, or otherwise reacted to?
Perhaps. This is the post-update hook, so at that point we have already committed any changes to the repository. Usually it is used for running "git update-server-info" for repositories available over dumb protocols.
So there is no useful action for receive-pack to do after seeing an error. But I said "perhaps" above, because it might be useful to notify the user over the stderr sideband that the hook failed. Even though we have no action to take, the user might care or want to investigate a potential problem.
I suspect nobody has cared about this before, though, because the stderr channel for the hook is also directed to the user. So if update-server-info (or whatever) fails, presumably it is complaining to stderr and the user sees that. Adding an additional "by the way, your hook failed" is just going to be noise in most cases.
Quoted 3 lines
Thus having the same effect (removing the status variable). Callers of
run_update_post_hookshould be checked as well, as should other run_command_* calls.
There is exactly one caller, and it doesn't care about the return code for the reasons mentioned above.
-Peff
2009/9/26 Jeff King <[email protected]>:
Quoted 10 lines · Reece Dunn
Quoted 5 lines
Quoted 3 lines
Now this is one that I do think is sensible. The variable isn't used, so don't even bother declaring it.
The status variable is removed in this patch.
Yes. Sorry if I wasn't clear, but what I meant was "this does not fall under the same idioms as the other ones, and it is a fine thing to be removing".
Sure.
Quoted 14 lines
Quoted 3 lines
But then shouldn't the status returned be checked and acted on? That is, are failures from
run_command_v_optbeing reported to the user, or otherwise reacted to?Perhaps. This is the post-update hook, so at that point we have already committed any changes to the repository. Usually it is used for running "git update-server-info" for repositories available over dumb protocols.
So there is no useful action for receive-pack to do after seeing an error. But I said "perhaps" above, because it might be useful to notify the user over the stderr sideband that the hook failed. Even though we have no action to take, the user might care or want to investigate a potential problem.
Quoted 5 lines
I suspect nobody has cared about this before, though, because the stderr channel for the hook is also directed to the user. So if update-server-info (or whatever) fails, presumably it is complaining to stderr and the user sees that. Adding an additional "by the way, your hook failed" is just going to be noise in most cases.
It could be used to return an error status from main if it is used in a chained command in a script. Other than that, I agree.
Quoted 6 lines
Quoted 3 lines
Thus having the same effect (removing the status variable). Callers of
run_update_post_hookshould be checked as well, as should other run_command_* calls.There is exactly one caller, and it doesn't care about the return code for the reasons mentioned above.
Including being called from a script?
- Reece
Quoted 8 lines · Reece Dunn
Quoted 5 lines
I suspect nobody has cared about this before, though, because the stderr channel for the hook is also directed to the user. So if update-server-info (or whatever) fails, presumably it is complaining to stderr and the user sees that. Adding an additional "by the way, your hook failed" is just going to be noise in most cases.
It could be used to return an error status from main if it is used in a chained command in a script. Other than that, I agree.
I'm not sure that's a good idea. Your push _did_ happen, and the remote repo was updated. So you have no way of knowing from an error exit code that changes were in fact made, and it was simply the post-update hook failing.
Of course, you can argue that the current behavior is similarly broken:
on success, you have no idea if the post-update hook failed or not. But I would argue that whether the push itself happened is more important than whether the hook succeeded or not. If you really care, you should either:
1. Use some sort of side channel to report hook status.
2. Use the pre-receive hook, which can abort the push if it wants to.
But all of that is "if we were designing this hook from scratch". At this point, it doesn't make sense to change the semantics. People may be relying on the current behavior, and in fact it is documented (in githooks(5)):
This hook is meant primarily for notification, and cannot affect the outcome of git-receive-pack.
Quoted 4 lines
Quoted 2 lines
There is exactly one caller, and it doesn't care about the return code for the reasons mentioned above.
Including being called from a script?
I suppose people can be scripting around "git receive-pack" itself, though I find it pretty unlikely. I find it much more likely for them to script around "git push", which calls receive-pack on the remote end, and may or may not get the actual status (without checking, I imagine the exit code is lost anyway for git:// pushes, but probably passed back along for pushes over ssh).
At any rate, even if we assume that people are scripting around it, and that they can in fact see the exit status, I think we would want to keep it the same for compatibility reasons, as mentioned above.
-Peff
2009/9/26 Jeff King <[email protected]>:
Quoted 8 lines · Reece Dunn
Quoted 7 lines
Quoted 6 lines
I suspect nobody has cared about this before, though, because the stderr channel for the hook is also directed to the user. So if update-server-info (or whatever) fails, presumably it is complaining to stderr and the user sees that. Adding an additional "by the way, your hook failed" is just going to be noise in most cases.
Quoted 7 lines
Quoted 2 lines
It could be used to return an error status from main if it is used in a chained command in a script. Other than that, I agree.
I'm not sure that's a good idea. Your push _did_ happen, and the remote repo was updated. So you have no way of knowing from an error exit code that changes were in fact made, and it was simply the post-update hook failing.
Ok.
Quoted 10 lines
Of course, you can argue that the current behavior is similarly broken:
on success, you have no idea if the post-update hook failed or not. But I would argue that whether the push itself happened is more important than whether the hook succeeded or not. If you really care, you should either:1. Use some sort of side channel to report hook status.
2. Use the pre-receive hook, which can abort the push if it wants to.
Quoted 7 lines
But all of that is "if we were designing this hook from scratch". At this point, it doesn't make sense to change the semantics. People may be relying on the current behavior, and in fact it is documented (in githooks(5)):
This hook is meant primarily for notification, and cannot affect the outcome of git-receive-pack.
That's fine. As long as the behaviour is documented (which as you pointed out, it is).
- Reece
Quoted 4 lines · Reece Dunn
Quoted 2 lines
There is exactly one caller, and it doesn't care about the return code for the reasons mentioned above.
Including being called from a script?
I agree, if something goes wrong then it should be reported. The same applies to the run_receive_hook' return code that is not checked in cmd_receive_pack'.
Considering you want to keep the current source code invariants, and I don't have any objection to it, probably the only assignment that can be removed is the following one:
Quoted 1 line
From
f8dd14bf4c3f3e132f6a8e13bf3e2fc575a804b1Mon Sep 17 00:00:00 2001
From: Giuseppe Scrivano <[email protected]>
Date: Sat, 26 Sep 2009 23:23:13 +0200
Subject: [PATCH] Remove a dead assignment found by the clang static analyzer
--- http-fetch.c | 3 +-- 1 files changed, 1 insertions(+), 2 deletions(-)
http-fetch.c +1 -2
@@ -3,7 +3,6 @@int main (int argc,const char **argv) { -const char *prefix;struct walker *walker;int commits_on_stdin =0 ;int commits;
@@ -19,7 +18,7 @@ int main(int argc, const char **argv)int get_verbosely =0 ;int get_recover =0 ; - prefix =setup_git_directory (); +setup_git_directory ();git_config (git_default_config,NULL );
Signature · 1 line
1.6.3.3
Quoted 33 lines · Jeff King
Quoted 2 lines · Giuseppe Scrivano
Here is a cleaned patch. I think these assignments can be removed without any problem.
I don't agree. For example:
builtin-fetch--tool.c static int append_fetch_head(FILE *fp, · line 172
Quoted 8 lines
builtin-fetch--tool.c +1
-1@@ -169,7 +169,7 @@ static int append_fetch_head(FILE *fp, note_len +=sprintf (note + note_len,"%s " , kind); note_len +=sprintf (note + note_len,"'%s' of " , what); } - note_len +=sprintf (note + note_len,"%.*s" , remote_len, remote); +sprintf (note + note_len,"%.*s" , remote_len, remote);This is a very particular C idiom: you are building a string over several statements using a function that adds to the string and tells you how much it added. The implicit invariant of the
note_lenvariable is that it _always_ contains the current length, so each statement uses it as input and pushes it forward on output.Any experienced C programmer should look at that and be able to see exactly what's going on. And people adding more lines don't need to munge the existing lines; the invariant property of
note_lenmeans they just need to add more, similar lines.But your patch destroys that invariant. It makes it harder to see what's going on, because it breaks the idiom. And it makes it more likely for somebody adding a line further on to make a mistake (and certainly it makes their patch harder to read and review, as they have to munge unrelated lines).
Quoted 4 lines
So no, while there is no code _now_ that is relying on the invariant being kept after the last statement (which is what the static analyzer is finding out), the point is not for the compiler to realize that, but for human programmers to see it.
And the compiler (at least gcc) is indeed smart enough to realize that nothing uses the result from the last statement, and does optimize away the code associated to it already. So this patch is unlikely to change anything to the compiled result.
Nicolas
Quoted 4 lines · Nicolas Pitre
And the compiler (at least gcc) is indeed smart enough to realize that nothing uses the result from the last statement, and does optimize away the code associated to it already. So this patch is unlikely to change anything to the compiled result.
Right, and gcc can do many other amazing things. But still it is used a variable that is never accessed, removing it can make the code slightly more readable.
Cheers,
Giuseppe
diff.c static void show_stats(struct diffstat_t *data, struct diff_options *options) · line 906
Quoted 12 lines
diff.c +1
-1@@ -901,7 +901,7 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)/* Find the longest filename and max number of changes */ reset =diff_get_color_opt (options,DIFF_RESET ); - set =diff_get_color_opt (options,DIFF_PLAIN ); +diff_get_color_opt (options,DIFF_PLAIN ); add_c =diff_get_color_opt (options,DIFF_FILE_NEW ); del_c =diff_get_color_opt (options,DIFF_FILE_OLD );
diff_get_color_opt() has no side-effects; the changed line is a no-op.
René
Hi,
builtin-commit.c int cmd_commit(int argc, const char **argv, const char *prefix) · line 1039
Quoted 11 lines · Giuseppe Scrivano
builtin-commit.c +1
-1@@ -1035,7 +1035,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix) parents =reduce_heads (parents); }else { reflog_msg ="commit" ; - pptr = &commit_list_insert (lookup_commit (head_sha1), pptr)->next; +commit_list_insert (lookup_commit (head_sha1), pptr); }
Sorry, but from the context it seems as if the same remark I had for argc applies here, too. There are exactly three other similar-looking assignments and it is too easy IMO to mess up when one want to rearrange things there.
In other words, I deem the removal of this assignment worse than what we have now -- at least in terms of how easy it is to modify the code safely.
I just looked further 3 hunks and had exactly the same impression there, so I stopped looking.
Sorry,
Dscho
Hello,
Quoted 5 lines · Sverre Rabbelier
Then it would probably have been better to say so by at least marking your patch as RFC and including such a remark in the cover letter, no? Also, now that this has been pointed out, you shouldn't expect it to be included until someone either takes your patch and cleans it up (as in, checks all statements manually), or until you do so yourself.
I really had to include a RFC remark. After what Johannes reported, I think there is need only to restore assignments to argc while other ones can be dropped without problems. I'll post a cleaned patch later.
Cheers,
Giuseppe