[PATCH 0/3] Fix code issues spotted by clang

This series fixes some of the code issues raised by clang. This leaves
the following warnings that I haven't addressed:

    revision.c:766:25: warning: implicit truncation from 'unsigned int' to bitfield changes value from 4294967279 to 134217711
          [-Wconstant-conversion]
                    p->item->object.flags &= ~TMP_MARK;
                                          ^  ~~~~~~~~~
    revision.c:768:25: warning: implicit truncation from 'unsigned int' to bitfield changes value from 4294967279 to 134217711
          [-Wconstant-conversion]
                    p->item->object.flags &= ~TMP_MARK;
                                          ^  ~~~~~~~~~
    revision.c:1875:25: warning: implicit truncation from 'unsigned int' to bitfield changes value from 4294967279 to 134217711
          [-Wconstant-conversion]
                    p->item->object.flags &= ~TMP_MARK;
                                          ^  ~~~~~~~~~
    revision.c:2202:25: warning: implicit truncation from 'unsigned int' to bitfield changes value from 4294967158 to 134217590
          [-Wconstant-conversion]
                            commit->object.flags &= ~(ADDED | SEEN | SHOWN);
                                                 ^  ~~~~~~~~~~~~~~~~~~~~~~~
    upload-pack.c:115:12: warning: implicit truncation from 'unsigned int' to bitfield changes value from 4294967293 to 134217725
          [-Wconstant-conversion]
                    o->flags &= ~UNINTERESTING;
                             ^  ~~~~~~~~~~~~~~
    upload-pack.c:689:19: warning: implicit truncation from 'unsigned int' to bitfield changes value from 4294705151 to 133955583
          [-Wconstant-conversion]
                                    object->flags &= ~CLIENT_SHALLOW;
                                                  ^  ~~~~~~~~~~~~~~~
    builtin/checkout.c:676:16: warning: implicit truncation from 'unsigned int' to bitfield changes value from 4294967293 to 134217725
          [-Wconstant-conversion]
            object->flags &= ~UNINTERESTING;
                          ^  ~~~~~~~~~~~~~~
    builtin/reflog.c:173:32: warning: implicit truncation from 'unsigned int' to bitfield changes value from 4294965247 to 134215679
          [-Wconstant-conversion]
                    found.objects[i].item->flags &= ~STUDYING;
                                                 ^  ~~~~~~~~~
    builtin/reflog.c:232:31: warning: implicit truncation from 'unsigned int' to bitfield changes value from 4294963199 to 134213631
          [-Wconstant-conversion]
                    pending->item->object.flags &= ~REACHABLE;
                                                ^  ~~~~~~~~~~
    bisect.c:66:24: warning: implicit truncation from 'unsigned int' to bitfield changes value from 4294901759 to 134152191
          [-Wconstant-conversion]
                    commit->object.flags &= ~COUNTED;
                                         ^  ~~~~~~~~
    
Ævar Arnfjörð Bjarmason (3):
  apply: get rid of useless x < 0 comparison on a size_t type
  diff/apply: cast variable in call to free()
  grep: get rid of useless x < 0 comparison on an enum member

 builtin/apply.c |    3 ---
 builtin/diff.c  |    2 +-
 grep.c          |    2 +-
 submodule.c     |    2 +-
 4 files changed, 3 insertions(+), 6 deletions(-)

-- 
1.7.6.3

Both of these free() calls are freeing a "const unsigned char (*)[20]"
type while free() expects a "void *". This results in the following
warning under clang 2.9:

    builtin/diff.c:185:7: warning: passing 'const unsigned char (*)[20]' to parameter of type 'void *' discards qualifiers
            free(parent);
                 ^~~~~~

    submodule.c:394:7: warning: passing 'const unsigned char (*)[20]' to parameter of type 'void *' discards qualifiers
            free(parents);
                 ^~~~~~~

This free()-ing without a cast was added by Jim Meyering to
builtin/diff.c in v1.7.6-rc3~4 and later by Fredrik Gustafsson in
submodule.c in v1.7.7-rc1~25^2.

Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
 builtin/diff.c |    2 +-
 submodule.c    |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/builtin/diff.c b/builtin/diff.c
index 1118689..0fe638f 100644
--- a/builtin/diff.c
+++ b/builtin/diff.c

@@ -182,7 +182,7 @@ static int builtin_diff_combined(struct rev_info *revs,
 		hashcpy((unsigned char *)(parent + i), ent[i].item->sha1);
 	diff_tree_combined(parent[0], parent + 1, ents - 1,
 			   revs->dense_combined_merges, revs);
-	free(parent);
+	free((void *)parent);
 	return 0;
 }
 

diff --git a/submodule.c b/submodule.c
index 0fd10a0..52cdcc6 100644
--- a/submodule.c
+++ b/submodule.c

@@ -391,7 +391,7 @@ static void commit_need_pushing(struct commit *commit, struct commit_list *paren
 	rev.diffopt.format_callback_data = needs_pushing;
 	diff_tree_combined(commit->object.sha1, parents, n, 1, &rev);
 
-	free(parents);
+	free((void *)parents);
 }
 
 int check_submodule_needs_pushing(unsigned char new_sha1[20], const char *remotes_name)

-- 
1.7.6.3

According to the C standard size_t is always unsigned, therefore the
comparison "n1 < 0 || n2 < 0" when n1 and n2 are size_t will always be
false.

This was raised by clang 2.9 which throws this warning when compiling
apply.c:

    builtin/apply.c:253:9: warning: comparison of unsigned expression < 0 is always false [-Wtautological-compare]
            if (n1 < 0 || n2 < 0)
                ~~ ^ ~
    builtin/apply.c:253:19: warning: comparison of unsigned expression < 0 is always false [-Wtautological-compare]
            if (n1 < 0 || n2 < 0)
                          ~~ ^ ~

This check was originally added in v1.6.5-rc0~53^2 by Giuseppe Bilotta
while adding an option to git-apply to ignore whitespace differences.

Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
 builtin/apply.c |    3 ---
 1 files changed, 0 insertions(+), 3 deletions(-)

diff --git a/builtin/apply.c b/builtin/apply.c
index 84a8a0b..b3b59db 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c

@@ -250,9 +250,6 @@ static int fuzzy_matchlines(const char *s1, size_t n1,
 	const char *last2 = s2 + n2 - 1;
 	int result = 0;
 
-	if (n1 < 0 || n2 < 0)
-		return 0;
-
 	/* ignore line endings */
 	while ((*last1 == '\r') || (*last1 == '\n'))
 		last1--;

-- 
1.7.6.3

Remove an "p->field < 0" comparison in grep.c that'll always be
false. In this case "p" is a "grep_pat" where "field" is defined as:

	enum grep_header_field field;

And grep_header_field is in turn defined as:

    enum grep_header_field {
    	GREP_HEADER_AUTHOR = 0,
    	GREP_HEADER_COMMITTER
    };

Meaning that this comparison will always be false. This was spotted by
clang 2.9 which produced the following warning while compiling grep.c:

    grep.c:330:16: warning: comparison of unsigned enum expression < 0 is always false [-Wtautological-compare]
                    if (p->field < 0 || GREP_HEADER_FIELD_MAX <= p->field)
                        ~~~~~~~~ ^ ~

Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
 grep.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/grep.c b/grep.c
index b29d09c..025d3f8 100644
--- a/grep.c
+++ b/grep.c

@@ -327,7 +327,7 @@ static struct grep_expr *prep_header_patterns(struct grep_opt *opt)
 	for (p = opt->header_list; p; p = p->next) {
 		if (p->token != GREP_PATTERN_HEAD)
 			die("bug: a non-header pattern in grep header list.");
-		if (p->field < 0 || GREP_HEADER_FIELD_MAX <= p->field)
+		if (GREP_HEADER_FIELD_MAX <= p->field)
 			die("bug: unknown header field %d", p->field);
 		compile_regexp(p, opt);
 	}

-- 
1.7.6.3

On Sun, Nov 6, 2011 at 13:06, Ævar Arnfjörð Bjarmason [off-list ref] wrote:

This series fixes some of the code issues raised by clang.

Out of curiosity I also compiled git on "Sun Studio 12 Update 1" on
ee6dfb2d83ba1b057943e705f707fa27e34e47f9 with this patch series
applied and it emits the following warnings, quoted below along with
my comments:

    "pack-write.c", line 76: warning: name redefined by pragma
redefine_extname declared static: tmpfile

Presumably this means that we've imported the function tmpfile() and
Sun's CC doesn't like that we're creating a variable with that name.

    "read-cache.c", line 761: warning: statement not reached

SunCC's brain is melting on this particularly clever piece of code:

    	goto inside;
    	for (;;) {
    		if (!c)
    			return 1;
    		if (is_dir_sep(c)) {
    inside:

    "sha1_file.c", line 2453: warning: name redefined by pragma
redefine_extname declared static: tmpfile

same tempfile issue.

    "compat/../git-compat-util.h", line 4: warning: macro redefined:
_FILE_OFFSET_BITS
    "compat/../git-compat-util.h", line 4: warning: macro redefined:
_FILE_OFFSET_BITS

Seems we're clashing with this:

    $ grep -r '#define.*_FILE_OFFSET_BITS' /usr/include/
    /usr/include/sys/feature_tests.h:#define   _FILE_OFFSET_BITS   64
    /usr/include/sys/feature_tests.h:#define   _FILE_OFFSET_BITS   32

    "xdiff/xutils.c", line 194: warning: statement not reached

This was added in b97e911643341cb31e6b97029b9ffd96fc675b1d as a
workaround on systems using glibc, should this even be run on Sun
libc? Martin?

Another clever bit of code blowing SunCC's brain:

	if (flags & XDF_IGNORE_WHITESPACE) {
		goto skip_ws;
		while (i1 < s1 && i2 < s2) {
			if (l1[i1++] != l2[i2++])
				return 0;
		skip_ws:
			while (i1 < s1 && XDL_ISSPACE(l1[i1]))
				i1++;
			while (i2 < s2 && XDL_ISSPACE(l2[i2]))
				i2++;
		}
	}

    "fast-import.c", line 858: warning: name redefined by pragma
redefine_extname declared static: tmpfile

ditto tempfile issue.

    "builtin/fast-export.c", line 54: warning: enum type mismatch: op "="

This seems to me to be a legitimate issue we introduced in
2d8ad46919213ebbd7bb72eb5b56cca8cc3ae07f, Elijah?

We're defining an enum like this:

    static enum { ABORT, VERBATIM, WARN, STRIP } signed_tag_mode = ABORT;
    static enum { ERROR, DROP, REWRITE } tag_of_filtered_mode = ABORT;

And then doing:

	if (unset || !strcmp(arg, "abort"))
		tag_of_filtered_mode = ABORT; <-- Line 54
	else if (!strcmp(arg, "drop"))
		tag_of_filtered_mode = DROP;
	else if (!strcmp(arg, "rewrite"))
		tag_of_filtered_mode = REWRITE;

Presumably that assignment should be "= ERROR".

    "builtin/index-pack.c", line 175: warning: name redefined by
pragma redefine_extname declared static: tmpfile

ditto tempfile.

    "vcs-svn/string_pool.c", line 11: warning: initializer will be
sign-extended: -1
    "vcs-svn/string_pool.c", line 81: warning: initializer will be
sign-extended: -1
    "vcs-svn/repo_tree.c", line 112: warning: initializer will be
sign-extended: -1
    "vcs-svn/repo_tree.c", line 112: warning: initializer will be
sign-extended: -1
    "test-treap.c", line 34: warning: initializer will be sign-extended: -1

All of these come down to assigning ~0 to an uint32_t variable, e.g.:

	uint32_t token = ~0;

Hi,

I imagine the following would be less controversial.

-- >8 --
From: Ævar Arnfjörð Bjarmason <redacted>
Subject: grep: get rid of bounds check on "enum grep_header_field" value

The grep_header_field enum is defined as:

    enum grep_header_field {
    	GREP_HEADER_AUTHOR = 0,
    	GREP_HEADER_COMMITTER
    };

Git's grep code sanity-checks the value of p->field before using it in
order to avoid reading and writing past the end of an array.
Unfortunately that test trips up misguided static analyzers like
"gcc -Wtype-limits" that do not recognize that C allows this enum to
be a signed integer type and an "x < 0" comparison really could fire
if someone passed in an uninitialized value.

Let's just drop the check.  Luckily git always does initialize
p->field correctly, and if it were to stop doing so, then hopefully
running the test suite with valgrind would catch it.

Noticed with clang -Wtautological-compare.

[jn: drop the GREP_HEADER_FIELD_MAX <= p->field check, too,
 for symmetry]

Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
Signed-off-by: Jonathan Nieder <redacted>
---
 grep.c |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)

diff --git a/grep.c b/grep.c
index b29d09c7..424c46cd 100644
--- a/grep.c
+++ b/grep.c

@@ -327,8 +327,6 @@ static struct grep_expr *prep_header_patterns(struct grep_opt *opt)
 	for (p = opt->header_list; p; p = p->next) {
 		if (p->token != GREP_PATTERN_HEAD)
 			die("bug: a non-header pattern in grep header list.");
-		if (p->field < 0 || GREP_HEADER_FIELD_MAX <= p->field)
-			die("bug: unknown header field %d", p->field);
 		compile_regexp(p, opt);
 	}
 

-- 
1.7.8.rc0

On Sun, Nov 6, 2011 at 5:33 AM, Ævar Arnfjörð Bjarmason
[off-list ref] wrote:

   "builtin/fast-export.c", line 54: warning: enum type mismatch: op "="

This seems to me to be a legitimate issue we introduced in
2d8ad46919213ebbd7bb72eb5b56cca8cc3ae07f, Elijah?

We're defining an enum like this:

   static enum { ABORT, VERBATIM, WARN, STRIP } signed_tag_mode = ABORT;
   static enum { ERROR, DROP, REWRITE } tag_of_filtered_mode = ABORT;

And then doing:

       if (unset || !strcmp(arg, "abort"))
               tag_of_filtered_mode = ABORT; <-- Line 54
       else if (!strcmp(arg, "drop"))
               tag_of_filtered_mode = DROP;
       else if (!strcmp(arg, "rewrite"))
               tag_of_filtered_mode = REWRITE;

Presumably that assignment should be "= ERROR".

Looking back at that patch, "abort" was the word that made the most
sense in the documentation and I was trying to make the code match.
Unfortunately, that would result in "ABORT" being redefined (to the
same value) so to squelch the compiler error I just went with the
quick hack of changing "ABORT" in the enum definition to "ERROR", but
didn't change "ABORT" later since "it had the same value anyway".
Yeah, not the safest programming practice.

I probably should have just used FILTERED_TAG_ABORT and changed the
signed_tag_mode enum to use SIGNED_TAG_ABORT (and then changed all
references to "ABORT" to match one of those two)...or maybe somehow
come up with a combined enum used for both (though WARN would only be
useful fo signed_tag_mode, REWRITE would only be useful for
tag_of_filtered_mode, etc.).