[PATCH] Remove useless assignments

[PATCH] Remove useless assignments

[Дилян Палаузов]

Signed-off-by: Дилян Палаузов <git-dpa@aegee.org>

diff --git a/archive.c b/archive.c
index 60b889198..e2534d186 100644
--- a/archive.c
+++ b/archive.c
@@ -503,7 +503,7 @@ int write_archive(int argc, const char **argv, const char *prefix,
 	init_tar_archiver();
 	init_zip_archiver();
 
-	argc = parse_archive_args(argc, argv, &ar, &args, name_hint, remote);
+	parse_archive_args(argc, argv, &ar, &args, name_hint, remote);
 	if (!startup_info->have_repository) {
 		/*
 		 * We know this will die() with an error, so we could just
diff --git a/builtin/add.c b/builtin/add.c
index 9f53f020d..fc1481273 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -211,7 +211,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;
 	rev.diffopt.use_color = 0;
 	DIFF_OPT_SET(&rev.diffopt, IGNORE_DIRTY_SUBMODULES);
diff --git a/builtin/help.c b/builtin/help.c
index 49f7a07f8..3d24b084e 100644
--- a/builtin/help.c
+++ b/builtin/help.c
@@ -453,7 +453,7 @@ int cmd_help(int argc, const char **argv, const char *prefix)
 	int nongit;
 	enum help_format parsed_help_format;
 
-	argc = parse_options(argc, argv, prefix, builtin_help_options,
+	parse_options(argc, argv, prefix, builtin_help_options,
 			builtin_help_usage, 0);
 	parsed_help_format = help_format;
 
diff --git a/builtin/merge.c b/builtin/merge.c
index 703827f00..337efef6f 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -795,7 +795,7 @@ static int merge_trivial(struct commit *head, struct commit_list *remoteheads)
 	write_tree_trivial(&result_tree);
 	printf(_("Wonderful.\n"));
 	pptr = commit_list_append(head, pptr);
-	pptr = commit_list_append(remoteheads->item, pptr);
+	commit_list_append(remoteheads->item, pptr);
 	prepare_to_commit(remoteheads);
 	if (commit_tree(merge_msg.buf, merge_msg.len, result_tree.hash, parents,
 			result_commit.hash, NULL, sign_commit))
diff --git a/builtin/notes.c b/builtin/notes.c
index 7b891471c..6ec753383 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -1015,7 +1015,7 @@ int cmd_notes(int argc, const char **argv, const char *prefix)
 	else if (!strcmp(argv[0], "get-ref"))
 		result = get_ref(argc, argv, prefix);
 	else {
-		result = error(_("unknown subcommand: %s"), argv[0]);
+		error(_("unknown subcommand: %s"), argv[0]);
 		usage_with_options(git_notes_usage, options);
 	}
 
diff --git a/builtin/prune-packed.c b/builtin/prune-packed.c
index c026299e7..73f424d9f 100644
--- a/builtin/prune-packed.c
+++ b/builtin/prune-packed.c
@@ -59,7 +59,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 0bb36d584..0fa779103 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -498,7 +498,7 @@ static const char *check_nonce(const char *buf, size_t len)
 	char *nonce = find_header(buf, len, "nonce");
 	timestamp_t stamp, ostamp;
 	char *bohmac, *expect = NULL;
-	const char *retval = NONCE_BAD;
+	const char *retval;
 
 	if (!nonce) {
 		retval = NONCE_MISSING;
diff --git a/builtin/reset.c b/builtin/reset.c
index fc3b906c4..f547efd8d 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -291,7 +291,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
 
 	git_config(git_default_config, NULL);
 
-	argc = parse_options(argc, argv, prefix, options, git_reset_usage,
+	parse_options(argc, argv, prefix, options, git_reset_usage,
 						PARSE_OPT_KEEP_DASHDASH);
 	parse_args(&pathspec, argv, prefix, patch_mode, &rev);
 
diff --git a/diffcore-rename.c b/diffcore-rename.c
index f7444c86b..632e19089 100644
--- a/diffcore-rename.c
+++ b/diffcore-rename.c
@@ -580,7 +580,7 @@ void diffcore_rename(struct diff_options *options)
 
 	rename_count += find_renames(mx, dst_cnt, minimum_score, 0);
 	if (detect_rename == DIFF_DETECT_COPY)
-		rename_count += find_renames(mx, dst_cnt, minimum_score, 1);
+		find_renames(mx, dst_cnt, minimum_score, 1);
 	free(mx);
 
  cleanup:
diff --git a/fast-import.c b/fast-import.c
index cf58f875b..0369363b2 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -2054,7 +2054,7 @@ static int validate_raw_date(const char *src, struct strbuf *result)
 
 	errno = 0;
 
-	num = strtoul(src, &endp, 10);
+	strtoul(src, &endp, 10);
 	/* NEEDSWORK: perhaps check for reasonable values? */
 	if (errno || endp == src || *endp != ' ')
 		return -1;
diff --git a/fsck.c b/fsck.c
index d589341cd..35d421c08 100644
--- a/fsck.c
+++ b/fsck.c
@@ -703,7 +703,6 @@ static int fsck_ident(const char **ident, struct object *obj, struct fsck_option
 	    !isdigit(p[4]) ||
 	    (p[5] != '\n'))
 		return report(options, obj, FSCK_MSG_BAD_TIMEZONE, "invalid author/committer line - bad time zone");
-	p += 6;
 	return 0;
 }
 
diff --git a/ref-filter.c b/ref-filter.c
index 1fc5e9970..2d06b98ce 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1634,10 +1634,6 @@ static int match_name_as_path(const struct ref_filter *filter, const char *refna
 {
 	const char **pattern = filter->name_patterns;
 	int namelen = strlen(refname);
-	unsigned flags = WM_PATHNAME;
-
-	if (filter->ignore_case)
-		flags |= WM_CASEFOLD;
 
 	for (; *pattern; pattern++) {
 		const char *p = *pattern;
diff --git a/t/helper/test-lazy-init-name-hash.c b/t/helper/test-lazy-init-name-hash.c
index 6368a8934..29a16a61b 100644
--- a/t/helper/test-lazy-init-name-hash.c
+++ b/t/helper/test-lazy-init-name-hash.c
@@ -209,7 +209,7 @@ int cmd_main(int argc, const char **argv)
 
 	prefix = setup_git_directory();
 
-	argc = parse_options(argc, argv, prefix, options, usage, 0);
+	parse_options(argc, argv, prefix, options, usage, 0);
 
 	/*
 	 * istate->dir_hash is only created when ignore_case is set.
diff --git a/wrapper.c b/wrapper.c
index d83741770..ad7b63a0f 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -485,7 +485,7 @@ int git_mkstemps_mode(char *pattern, int suffix_len, int mode)
 		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, mode);
 		if (fd >= 0)
-- 
2.12.3

Re: [PATCH] Remove useless assignments

[Jeff King]

On Sat, May 20, 2017 at 05:52:16AM +0000, Дилян Палаузов wrote:

> Signed-off-by: Дилян Палаузов <git-dpa@aegee.org>

I assume this is just going through the results of clang's static
analyzer and quieting it. I think most of these are not the right fix,
though, as I discussed in

  http://public-inbox.org/git/20170225223146.ixubnwqkfol5q2gn@sigill.intra.peff.net/

For instance:

> diff --git a/archive.c b/archive.c
> index 60b889198..e2534d186 100644
> --- a/archive.c
> +++ b/archive.c
> @@ -503,7 +503,7 @@ int write_archive(int argc, const char **argv, const char *prefix,
>  	init_tar_archiver();
>  	init_zip_archiver();
>  
> -	argc = parse_archive_args(argc, argv, &ar, &args, name_hint, remote);
> +	parse_archive_args(argc, argv, &ar, &args, name_hint, remote);
>  	if (!startup_info->have_repository) {
>  		/*
>  		 * We know this will die() with an error, so we could just

It's true that nobody accesses argc after this, so the assignment is
dead. But if we _don't_ assign to argc, we're leaving a maintenance trap
for somebody who later does want to access it. Its value must match that
what is in argv, and after your patch that invariant no longer holds.

Ditto for all of the argc hunks in this patch.

> diff --git a/builtin/merge.c b/builtin/merge.c
> index 703827f00..337efef6f 100644
> --- a/builtin/merge.c
> +++ b/builtin/merge.c
> @@ -795,7 +795,7 @@ static int merge_trivial(struct commit *head, struct commit_list *remoteheads)
>  	write_tree_trivial(&result_tree);
>  	printf(_("Wonderful.\n"));
>  	pptr = commit_list_append(head, pptr);
> -	pptr = commit_list_append(remoteheads->item, pptr);
> +	commit_list_append(remoteheads->item, pptr);
>  	prepare_to_commit(remoteheads);
>  	if (commit_tree(merge_msg.buf, merge_msg.len, result_tree.hash, parents,
>  			result_commit.hash, NULL, sign_commit))

This one is similar. The return value of the append function updates the
tail pointer. Nobody appends to the list, so we never look at the tail
pointer again. But it has violated the tail-pointer invariant, and is
setting a trap for somebody who tries to append to the list later.

> diff --git a/builtin/notes.c b/builtin/notes.c
> index 7b891471c..6ec753383 100644
> --- a/builtin/notes.c
> +++ b/builtin/notes.c
> @@ -1015,7 +1015,7 @@ int cmd_notes(int argc, const char **argv, const char *prefix)
>  	else if (!strcmp(argv[0], "get-ref"))
>  		result = get_ref(argc, argv, prefix);
>  	else {
> -		result = error(_("unknown subcommand: %s"), argv[0]);
> +		error(_("unknown subcommand: %s"), argv[0]);
>  		usage_with_options(git_notes_usage, options);
>  	}
>  

This one actually seems reasonable (usage_with_options exits, so we
don't need to bother setting result).

> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
> index 0bb36d584..0fa779103 100644
> --- a/builtin/receive-pack.c
> +++ b/builtin/receive-pack.c
> @@ -498,7 +498,7 @@ static const char *check_nonce(const char *buf, size_t len)
>  	char *nonce = find_header(buf, len, "nonce");
>  	timestamp_t stamp, ostamp;
>  	char *bohmac, *expect = NULL;
> -	const char *retval = NONCE_BAD;
> +	const char *retval;
>  
>  	if (!nonce) {
>  		retval = NONCE_MISSING;

I have mixed feeling on this one. It's true that the NONCE_BAD value is
never used. But it's a defensive programming measure to set the default
in case we add code paths that do not set retval. If we could trust the
compiler to tell us when the uninitialized value was used, that would
let us avoid that situation. But the maybe-uninitialized warnings have
historically been one of the least trustworthy warnings we've seen (most
of the compiler-warning workarounds we've done have been for that).

> diff --git a/fsck.c b/fsck.c
> index d589341cd..35d421c08 100644
> --- a/fsck.c
> +++ b/fsck.c
> @@ -703,7 +703,6 @@ static int fsck_ident(const char **ident, struct object *obj, struct fsck_option
>  	    !isdigit(p[4]) ||
>  	    (p[5] != '\n'))
>  		return report(options, obj, FSCK_MSG_BAD_TIMEZONE, "invalid author/committer line - bad time zone");
> -	p += 6;
>  	return 0;
>  }

Another "invariant" one. Anybody adding more fsck checks would be
surprised when "p" is not updated past the last check.

> diff --git a/ref-filter.c b/ref-filter.c
> index 1fc5e9970..2d06b98ce 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -1634,10 +1634,6 @@ static int match_name_as_path(const struct ref_filter *filter, const char *refna
>  {
>  	const char **pattern = filter->name_patterns;
>  	int namelen = strlen(refname);
> -	unsigned flags = WM_PATHNAME;
> -
> -	if (filter->ignore_case)
> -		flags |= WM_CASEFOLD;
>  
>  	for (; *pattern; pattern++) {
>  		const char *p = *pattern;

I think this one is a real bug, but your fix is going in the wrong
direction. Those lines were added by 3bb16a8bf (tag, branch,
for-each-ref: add --ignore-case for sorting and filtering, 2016-12-04),
and it was probably a mistake that it did not update the call to
wildmatch(). IOW, the fix is probably:

diff --git a/ref-filter.c b/ref-filter.c
index 8f48e0dc4..f4306793b 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1533,7 +1533,7 @@ static int match_name_as_path(const struct ref_filter *filter, const char *refna
 		     refname[plen] == '/' ||
 		     p[plen-1] == '/'))
 			return 1;
-		if (!wildmatch(p, refname, WM_PATHNAME, NULL))
+		if (!wildmatch(p, refname, flags, NULL))
 			return 1;
 	}
 	return 0;

-Peff