[PATCH] Remove various dead assignments and dead increments found by the clang static analyzer

[PATCH] Remove various dead assignments and dead increments found by the clang static analyzer

[Giuseppe Scrivano]

Hello,

I tried the clang static analyzer on the git source code, this patch
fixes the found dead assignments/increments.


Regards,
Giuseppe Scrivano



>From 88fe9b63d159ad1fd0579564558fbf0f900bd8e3 Mon Sep 17 00:00:00 2001
From: Giuseppe Scrivano <gscrivano@gnu.org>
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(-)

diff --git a/archive.c b/archive.c
index 73b8e8a..88feed7 100644
--- a/archive.c
+++ b/archive.c
@@ -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();
 
diff --git a/builtin-add.c b/builtin-add.c
index cb6e590..2788315 100644
--- a/builtin-add.c
+++ b/builtin-add.c
@@ -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)
diff --git a/builtin-bisect--helper.c b/builtin-bisect--helper.c
index 5b22639..f9c7695 100644
--- a/builtin-bisect--helper.c
+++ b/builtin-bisect--helper.c
@@ -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)
diff --git a/builtin-commit.c b/builtin-commit.c
index 200ffda..56b595f 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c
@@ -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 */
diff --git a/builtin-fetch--tool.c b/builtin-fetch--tool.c
index 3dbdf7a..c47469f 100644
--- a/builtin-fetch--tool.c
+++ b/builtin-fetch--tool.c
@@ -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" : "",
diff --git a/builtin-fetch-pack.c b/builtin-fetch-pack.c
index 629735f..583f4e3 100644
--- a/builtin-fetch-pack.c
+++ b/builtin-fetch-pack.c
@@ -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;
diff --git a/builtin-grep.c b/builtin-grep.c
index 761799d..d36b59e 100644
--- a/builtin-grep.c
+++ b/builtin-grep.c
@@ -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) {
diff --git a/builtin-help.c b/builtin-help.c
index e1eba77..76307fd 100644
--- a/builtin-help.c
+++ b/builtin-help.c
@@ -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) {
diff --git a/builtin-ls-files.c b/builtin-ls-files.c
index f473220..80212f4 100644
--- a/builtin-ls-files.c
+++ b/builtin-ls-files.c
@@ -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 ";
diff --git a/builtin-mktree.c b/builtin-mktree.c
index 098395f..36053cf 100644
--- a/builtin-mktree.c
+++ b/builtin-mktree.c
@@ -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) {
diff --git a/builtin-pack-objects.c b/builtin-pack-objects.c
index 02f9246..bea7141 100644
--- a/builtin-pack-objects.c
+++ b/builtin-pack-objects.c
@@ -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);
diff --git a/builtin-prune-packed.c b/builtin-prune-packed.c
index be99eb0..9a8fcfe 100644
--- a/builtin-prune-packed.c
+++ b/builtin-prune-packed.c
@@ -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);
diff --git a/builtin-receive-pack.c b/builtin-receive-pack.c
index b771fe9..957e7f0 100644
--- a/builtin-receive-pack.c
+++ b/builtin-receive-pack.c
@@ -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;
diff --git a/builtin-rev-parse.c b/builtin-rev-parse.c
index 45bead6..4a66ba4 100644
--- a/builtin-rev-parse.c
+++ b/builtin-rev-parse.c
@@ -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);
 
diff --git a/builtin-send-pack.c b/builtin-send-pack.c
index 37e528e..5afd542 100644
--- a/builtin-send-pack.c
+++ b/builtin-send-pack.c
@@ -55,7 +55,7 @@ static int pack_objects(int fd, struct ref *refs, struct extra_have_objects *ext
 	if (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;
diff --git a/builtin-show-branch.c b/builtin-show-branch.c
index 3510a86..e567eb5 100644
--- a/builtin-show-branch.c
+++ b/builtin-show-branch.c
@@ -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);
diff --git a/builtin-show-ref.c b/builtin-show-ref.c
index c46550c..8a0ae6c 100644
--- a/builtin-show-ref.c
+++ b/builtin-show-ref.c
@@ -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)
diff --git a/builtin-write-tree.c b/builtin-write-tree.c
index b223af4..848c3e4 100644
--- a/builtin-write-tree.c
+++ b/builtin-write-tree.c
@@ -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);
diff --git a/color.c b/color.c
index 62977f4..5b31588 100644
--- a/color.c
+++ b/color.c
@@ -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';
diff --git a/compat/mkstemps.c b/compat/mkstemps.c
index 14179c8..dbf916e 100644
--- a/compat/mkstemps.c
+++ b/compat/mkstemps.c
@@ -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)
diff --git a/connect.c b/connect.c
index 7945e38..da6c7c1 100644
--- a/connect.c
+++ b/connect.c
@@ -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 --git a/diff.c b/diff.c
index e1be189..e75f58e 100644
--- a/diff.c
+++ b/diff.c
@@ -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 --git a/http-fetch.c b/http-fetch.c
index e8f44ba..6879904 100644
--- a/http-fetch.c
+++ b/http-fetch.c
@@ -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);
 
diff --git a/transport.c b/transport.c
index 644a30a..8ec0df6 100644
--- a/transport.c
+++ b/transport.c
@@ -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));
diff --git a/upload-pack.c b/upload-pack.c
index 38ddac2..fb3436c 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -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;
-- 
1.6.3.3

Re: [PATCH] Remove various dead assignments and dead increments found by the clang static analyzer

[Johannes Schindelin]

Hi,

On Sat, 26 Sep 2009, Giuseppe Scrivano wrote:

> I tried the clang static analyzer on the git source code, this patch
> fixes the found dead assignments/increments.
> 
> diff --git a/archive.c b/archive.c
> index 73b8e8a..88feed7 100644
> --- a/archive.c
> +++ b/archive.c
> @@ -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

Re: [PATCH] Remove various dead assignments and dead increments found by the clang static analyzer

[Giuseppe Scrivano]

Hello,


Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> 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

Re: [PATCH] Remove various dead assignments and dead increments found by the clang static analyzer

[Sverre Rabbelier]

Heya,

On Sat, Sep 26, 2009 at 20:21, Giuseppe Scrivano <gscrivano@gnu.org> wrote:
> 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.

-- 
Cheers,

Sverre Rabbelier

Re: [PATCH] Remove various dead assignments and dead increments found by the clang static analyzer

[Giuseppe Scrivano]

Hello,

Sverre Rabbelier <srabbelier@gmail.com> writes:

> 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

Re: [PATCH] Remove various dead assignments and dead increments found by the clang static analyzer

[Giuseppe Scrivano]

Here is a cleaned patch.  I think these assignments can be removed
without any problem.

Cheers,
Giuseppe



>From 7501d82998132b15ad5cda78c0650f4f4a0b0e93 Mon Sep 17 00:00:00 2001
From: Giuseppe Scrivano <gscrivano@gnu.org>
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(-)

diff --git a/builtin-commit.c b/builtin-commit.c
index 200ffda..331d2a0 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c
@@ -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 */
diff --git a/builtin-fetch--tool.c b/builtin-fetch--tool.c
index 3dbdf7a..c47469f 100644
--- a/builtin-fetch--tool.c
+++ b/builtin-fetch--tool.c
@@ -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" : "",
diff --git a/builtin-fetch-pack.c b/builtin-fetch-pack.c
index 629735f..583f4e3 100644
--- a/builtin-fetch-pack.c
+++ b/builtin-fetch-pack.c
@@ -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;
diff --git a/builtin-grep.c b/builtin-grep.c
index 761799d..d36b59e 100644
--- a/builtin-grep.c
+++ b/builtin-grep.c
@@ -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) {
diff --git a/builtin-pack-objects.c b/builtin-pack-objects.c
index 02f9246..bea7141 100644
--- a/builtin-pack-objects.c
+++ b/builtin-pack-objects.c
@@ -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);
diff --git a/builtin-receive-pack.c b/builtin-receive-pack.c
index b771fe9..82d1564 100644
--- a/builtin-receive-pack.c
+++ b/builtin-receive-pack.c
@@ -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;
diff --git a/builtin-send-pack.c b/builtin-send-pack.c
index 37e528e..5afd542 100644
--- a/builtin-send-pack.c
+++ b/builtin-send-pack.c
@@ -55,7 +55,7 @@ static int pack_objects(int fd, struct ref *refs, struct extra_have_objects *ext
 	if (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;
diff --git a/builtin-show-branch.c b/builtin-show-branch.c
index 3510a86..e567eb5 100644
--- a/builtin-show-branch.c
+++ b/builtin-show-branch.c
@@ -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);
diff --git a/color.c b/color.c
index 62977f4..5b31588 100644
--- a/color.c
+++ b/color.c
@@ -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';
diff --git a/compat/mkstemps.c b/compat/mkstemps.c
index 14179c8..dbf916e 100644
--- a/compat/mkstemps.c
+++ b/compat/mkstemps.c
@@ -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)
diff --git a/connect.c b/connect.c
index 7945e38..da6c7c1 100644
--- a/connect.c
+++ b/connect.c
@@ -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 --git a/diff.c b/diff.c
index e1be189..e75f58e 100644
--- a/diff.c
+++ b/diff.c
@@ -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 --git a/http-fetch.c b/http-fetch.c
index e8f44ba..6879904 100644
--- a/http-fetch.c
+++ b/http-fetch.c
@@ -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);
 
diff --git a/transport.c b/transport.c
index 644a30a..8ec0df6 100644
--- a/transport.c
+++ b/transport.c
@@ -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));
diff --git a/upload-pack.c b/upload-pack.c
index 38ddac2..fb3436c 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -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;
-- 
1.6.3.3

Re: [PATCH] Remove various dead assignments and dead increments found by the clang static analyzer

[René Scharfe]

> diff --git a/diff.c b/diff.c
> index e1be189..e75f58e 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -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é

Re: [PATCH] Remove various dead assignments and dead increments found by the clang static analyzer

[Johannes Schindelin]

Hi,

On Sat, 26 Sep 2009, Giuseppe Scrivano wrote:

> diff --git a/builtin-commit.c b/builtin-commit.c
> index 200ffda..331d2a0 100644
> --- a/builtin-commit.c
> +++ b/builtin-commit.c
> @@ -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

Re: [PATCH] Remove various dead assignments and dead increments found by the clang static analyzer

[Jeff King]

On Sat, Sep 26, 2009 at 09:15:58PM +0200, Giuseppe Scrivano wrote:

> Here is a cleaned patch.  I think these assignments can be removed
> without any problem.

I don't agree. For example:

> --- a/builtin-fetch--tool.c
> +++ b/builtin-fetch--tool.c
> @@ -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.

> --- a/builtin-fetch-pack.c
> +++ b/builtin-fetch-pack.c
> @@ -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).

> --- a/builtin-pack-objects.c
> +++ b/builtin-pack-objects.c
> @@ -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.

> --- a/builtin-receive-pack.c
> +++ b/builtin-receive-pack.c
> @@ -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);
>  }

Now this is one that I do think is sensible. The variable isn't used, so
don't even bother declaring it.

> @@ -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.

> @@ -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.

> diff --git a/builtin-send-pack.c b/builtin-send-pack.c
> index 37e528e..5afd542 100644
> --- a/builtin-send-pack.c
> +++ b/builtin-send-pack.c
> @@ -55,7 +55,7 @@ static int pack_objects(int fd, struct ref *refs, struct extra_have_objects *ext
>  	if (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'.

> diff --git a/builtin-show-branch.c b/builtin-show-branch.c
> index 3510a86..e567eb5 100644
> --- a/builtin-show-branch.c
> +++ b/builtin-show-branch.c
> @@ -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

Re: [PATCH] Remove various dead assignments and dead increments found by the clang static analyzer

[Reece Dunn]

2009/9/26 Jeff King <peff@peff.net>:
> On Sat, Sep 26, 2009 at 09:15:58PM +0200, Giuseppe Scrivano wrote:
>
>> Here is a cleaned patch.  I think these assignments can be removed
>> without any problem.
>
>> --- a/builtin-receive-pack.c
>> +++ b/builtin-receive-pack.c
>> @@ -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);
>>  }
>
> 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

Re: [PATCH] Remove various dead assignments and dead increments found by the clang static analyzer

[Jeff King]

On Sat, Sep 26, 2009 at 10:03:27PM +0100, Reece Dunn wrote:

> > 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".

> 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?

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.

> 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.

There is exactly one caller, and it doesn't care about the return code
for the reasons mentioned above.

-Peff

Re: [PATCH] Remove various dead assignments and dead increments found by the clang static analyzer

[Reece Dunn]

2009/9/26 Jeff King <peff@peff.net>:
> On Sat, Sep 26, 2009 at 10:03:27PM +0100, Reece Dunn wrote:
>
>> > 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.

>> 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?
>
> 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.

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.

>> 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.
>
> 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

Re: [PATCH] Remove various dead assignments and dead increments found by the clang static analyzer

[Jeff King]

On Sat, Sep 26, 2009 at 10:20:18PM +0100, Reece Dunn wrote:

> > 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.

> > 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

Re: [PATCH] Remove various dead assignments and dead increments found by the clang static analyzer

[Giuseppe Scrivano]

Reece Dunn <msclrhd@googlemail.com> writes:

>> 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:

>From f8dd14bf4c3f3e132f6a8e13bf3e2fc575a804b1 Mon Sep 17 00:00:00 2001
From: Giuseppe Scrivano <gscrivano@gnu.org>
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(-)

diff --git a/http-fetch.c b/http-fetch.c
index e8f44ba..6879904 100644
--- a/http-fetch.c
+++ b/http-fetch.c
@@ -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);
 
-- 
1.6.3.3

Re: [PATCH] Remove various dead assignments and dead increments found by the clang static analyzer

[Reece Dunn]

2009/9/26 Jeff King <peff@peff.net>:
> On Sat, Sep 26, 2009 at 10:20:18PM +0100, Reece Dunn wrote:
>
>> > 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.

Ok.

> 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.

That's fine. As long as the behaviour is documented (which as you
pointed out, it is).

- Reece

Re: [PATCH] Remove various dead assignments and dead increments found by the clang static analyzer

[Nicolas Pitre]

On Sat, 26 Sep 2009, Jeff King wrote:

> On Sat, Sep 26, 2009 at 09:15:58PM +0200, Giuseppe Scrivano wrote:
> 
> > Here is a cleaned patch.  I think these assignments can be removed
> > without any problem.
> 
> I don't agree. For example:
> 
> > --- a/builtin-fetch--tool.c
> > +++ b/builtin-fetch--tool.c
> > @@ -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.

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

Re: [PATCH] Remove various dead assignments and dead increments found by the clang static analyzer

[Giuseppe Scrivano]

Nicolas Pitre <nico@fluxnic.net> writes:

> 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