[PATCH 0/4] fix make sparse warning

[PATCH 0/4] fix make sparse warning

[Đoàn Trần Công Danh]

It's happened that I tried to run make check on project that support autoconf.
Git's Makefile told me to run `make sparse` instead.

I /think/ if we have a rule in Makefile, we should adhere to it.
I also fix another change in ds/blame-on-bloom, which I think it's worth to
fix, see: <20200423133937.GA1984@danh.dev>

This series is developed from latest master.

It's conflicted when merge down next and pu.

For next, please ignore the conflict with fmt-merge-msg.c and
apply [5/4] instead.

For pu, reftable.c has too much warnings, I'm too lazy to look into it.

Đoàn Trần Công Danh (4):
  C: s/0/NULL/ for pointer type
  compat/regex: silence `make sparse` warning
  graph.c: limit linkage of internal variable
  progress.c: silence cgcc suggestion about internal linkage

 add-interactive.c                   | 2 +-
 builtin/fmt-merge-msg.c             | 2 +-
 compat/regex/regex.c                | 1 +
 compat/regex/regex_internal.c       | 2 +-
 compat/regex/regex_internal.h       | 5 ++---
 graph.c                             | 2 +-
 log-tree.c                          | 4 ++--
 progress.c                          | 2 ++
 range-diff.c                        | 2 +-
 t/helper/test-parse-pathspec-file.c | 6 +++---
 10 files changed, 15 insertions(+), 13 deletions(-)

-- 
2.26.2.384.g435bf60bd5

[PATCH 1/4] C: s/0/NULL/ for pointer type

[Đoàn Trần Công Danh]

Fix warning from  `make sparse`.

Signed-off-by: Đoàn Trần Công Danh <congdanhqx@gmail.com>
---
 add-interactive.c                   | 2 +-
 builtin/fmt-merge-msg.c             | 2 +-
 log-tree.c                          | 4 ++--
 range-diff.c                        | 2 +-
 t/helper/test-parse-pathspec-file.c | 6 +++---
 5 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/add-interactive.c b/add-interactive.c
index 29cd2fe020..b8983838b9 100644
--- a/add-interactive.c
+++ b/add-interactive.c
@@ -526,7 +526,7 @@ static int get_modified_files(struct repository *r,
 
 	for (i = 0; i < 2; i++) {
 		struct rev_info rev;
-		struct setup_revision_opt opt = { 0 };
+		struct setup_revision_opt opt = { NULL };
 
 		if (filter == INDEX_ONLY)
 			s.mode = (i == 0) ? FROM_INDEX : FROM_WORKTREE;
diff --git a/builtin/fmt-merge-msg.c b/builtin/fmt-merge-msg.c
index 172dfbd852..f4376bccef 100644
--- a/builtin/fmt-merge-msg.c
+++ b/builtin/fmt-merge-msg.c
@@ -494,7 +494,7 @@ static void fmt_merge_msg_sigs(struct strbuf *out)
 		enum object_type type;
 		unsigned long size, len;
 		char *buf = read_object_file(oid, &type, &size);
-		struct signature_check sigc = { 0 };
+		struct signature_check sigc = { NULL };
 		struct strbuf sig = STRBUF_INIT;
 
 		if (!buf || type != OBJ_TAG)
diff --git a/log-tree.c b/log-tree.c
index 0064788b25..ca721150d4 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -449,7 +449,7 @@ static void show_signature(struct rev_info *opt, struct commit *commit)
 {
 	struct strbuf payload = STRBUF_INIT;
 	struct strbuf signature = STRBUF_INIT;
-	struct signature_check sigc = { 0 };
+	struct signature_check sigc = { NULL };
 	int status;
 
 	if (parse_signed_commit(commit, &payload, &signature) <= 0)
@@ -496,7 +496,7 @@ static int show_one_mergetag(struct commit *commit,
 	struct object_id oid;
 	struct tag *tag;
 	struct strbuf verify_message;
-	struct signature_check sigc = { 0 };
+	struct signature_check sigc = { NULL };
 	int status, nth;
 	size_t payload_size;
 
diff --git a/range-diff.c b/range-diff.c
index f745567cf6..71dcd947c5 100644
--- a/range-diff.c
+++ b/range-diff.c
@@ -107,7 +107,7 @@ static int read_patches(const char *range, struct string_list *list,
 		}
 
 		if (starts_with(line, "diff --git")) {
-			struct patch patch = { 0 };
+			struct patch patch = { NULL };
 			struct strbuf root = STRBUF_INIT;
 			int linenr = 0;
 
diff --git a/t/helper/test-parse-pathspec-file.c b/t/helper/test-parse-pathspec-file.c
index 02f4ccfd2a..b3e08cef4b 100644
--- a/t/helper/test-parse-pathspec-file.c
+++ b/t/helper/test-parse-pathspec-file.c
@@ -6,7 +6,7 @@
 int cmd__parse_pathspec_file(int argc, const char **argv)
 {
 	struct pathspec pathspec;
-	const char *pathspec_from_file = 0;
+	const char *pathspec_from_file = NULL;
 	int pathspec_file_nul = 0, i;
 
 	static const char *const usage[] = {
@@ -20,9 +20,9 @@ int cmd__parse_pathspec_file(int argc, const char **argv)
 		OPT_END()
 	};
 
-	parse_options(argc, argv, 0, options, usage, 0);
+	parse_options(argc, argv, NULL, options, usage, 0);
 
-	parse_pathspec_file(&pathspec, 0, 0, 0, pathspec_from_file,
+	parse_pathspec_file(&pathspec, 0, 0, NULL, pathspec_from_file,
 			    pathspec_file_nul);
 
 	for (i = 0; i < pathspec.nr; i++)
-- 
2.26.2.384.g435bf60bd5

[PATCH 2/4] compat/regex: silence `make sparse` warning

[Đoàn Trần Công Danh]

* alloca: somewhere later in the code, we indirectly include alloca.h
which will define alloca again, include it prior to define alloca in
order to not define it against.

* Copy all attributes from the header to source file, and move the
  attributes prior to function name. cgcc is very picky on the position
  of attribute.

Signed-off-by: Đoàn Trần Công Danh <congdanhqx@gmail.com>
---
 compat/regex/regex.c          | 1 +
 compat/regex/regex_internal.c | 2 +-
 compat/regex/regex_internal.h | 5 ++---
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/compat/regex/regex.c b/compat/regex/regex.c
index f3e03a9eab..4bef75a716 100644
--- a/compat/regex/regex.c
+++ b/compat/regex/regex.c
@@ -62,6 +62,7 @@
 #include <stdint.h>
 
 #ifdef GAWK
+#include <alloca.h>
 #undef alloca
 #define alloca alloca_is_bad_you_should_never_use_it
 #endif
diff --git a/compat/regex/regex_internal.c b/compat/regex/regex_internal.c
index ec51cf3446..58504f795b 100644
--- a/compat/regex/regex_internal.c
+++ b/compat/regex/regex_internal.c
@@ -921,7 +921,7 @@ re_string_destruct (re_string_t *pstr)
 /* Return the context at IDX in INPUT.  */
 
 static unsigned int
-internal_function
+internal_function __attribute ((pure))
 re_string_context_at (const re_string_t *input, int idx, int eflags)
 {
   int c;
diff --git a/compat/regex/regex_internal.h b/compat/regex/regex_internal.h
index 3ee8aae59d..c61a1e4971 100644
--- a/compat/regex/regex_internal.h
+++ b/compat/regex/regex_internal.h
@@ -430,9 +430,8 @@ static reg_errcode_t build_wcs_upper_buffer (re_string_t *pstr)
 # endif /* RE_ENABLE_I18N */
 static void build_upper_buffer (re_string_t *pstr) internal_function;
 static void re_string_translate_buffer (re_string_t *pstr) internal_function;
-static unsigned int re_string_context_at (const re_string_t *input, int idx,
-					  int eflags)
-     internal_function __attribute ((pure));
+static internal_function __attribute ((pure))
+unsigned int re_string_context_at (const re_string_t *input, int idx, int eflags);
 #endif
 #define re_string_peek_byte(pstr, offset) \
   ((pstr)->mbs[(pstr)->cur_idx + offset])
-- 
2.26.2.384.g435bf60bd5

[PATCH 3/4] graph.c: limit linkage of internal variable

[Đoàn Trần Công Danh]

Signed-off-by: Đoàn Trần Công Danh <congdanhqx@gmail.com>
---
 graph.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/graph.c b/graph.c
index 4fb25ad464..4cd9915075 100644
--- a/graph.c
+++ b/graph.c
@@ -1055,7 +1055,7 @@ static void graph_output_commit_line(struct git_graph *graph, struct graph_line
 		graph_update_state(graph, GRAPH_COLLAPSING);
 }
 
-const char merge_chars[] = {'/', '|', '\\'};
+static const char merge_chars[] = {'/', '|', '\\'};
 
 static void graph_output_post_merge_line(struct git_graph *graph, struct graph_line *line)
 {
-- 
2.26.2.384.g435bf60bd5

[PATCH 4/4] progress.c: silence cgcc suggestion about internal linkage

[Đoàn Trần Công Danh]

Signed-off-by: Đoàn Trần Công Danh <congdanhqx@gmail.com>
---
 progress.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/progress.c b/progress.c
index 19805ac646..fb53a2ec0c 100644
--- a/progress.c
+++ b/progress.c
@@ -50,7 +50,9 @@ static volatile sig_atomic_t progress_update;
  * These are only intended for testing the progress output, i.e. exclusively
  * for 'test-tool progress'.
  */
+extern int progress_testing; /* to silence sparse: internal linkage */
 int progress_testing;
+extern uint64_t progress_test_ns; /* to silence sparse: internal linkage */
 uint64_t progress_test_ns = 0;
 void progress_test_force_update(void); /* To silence -Wmissing-prototypes */
 void progress_test_force_update(void)
-- 
2.26.2.384.g435bf60bd5

[PATCH 5/4] fmt-merge-msg.c: fix `make sparse` on next

[Đoàn Trần Công Danh]

Probably, un-necessary, since this is merged down to master.

Signed-off-by: Đoàn Trần Công Danh <congdanhqx@gmail.com>
---
 fmt-merge-msg.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fmt-merge-msg.c b/fmt-merge-msg.c
index 08022ed2e4..72d32bd73b 100644
--- a/fmt-merge-msg.c
+++ b/fmt-merge-msg.c
@@ -484,7 +484,7 @@ static void fmt_merge_msg_sigs(struct strbuf *out)
 		enum object_type type;
 		unsigned long size, len;
 		char *buf = read_object_file(oid, &type, &size);
-		struct signature_check sigc = { 0 };
+		struct signature_check sigc = { NULL };
 		struct strbuf sig = STRBUF_INIT;
 
 		if (!buf || type != OBJ_TAG)
-- 
2.26.2.384.g435bf60bd5

Re: [PATCH 0/4] fix make sparse warning

[Ramsay Jones]

On 23/04/2020 14:47, Đoàn Trần Công Danh wrote:
> It's happened that I tried to run make check on project that support autoconf.

I try to ignore autoconf as much as possible, so I don't know why people
who use it regularly seem to expect a 'make check' target (is that anything
to do with ./config.status --recheck?).

> Git's Makefile told me to run `make sparse` instead.

In which case, you must have had sparse installed, otherwise you would
have been directed to 'make test'. [See commit 912f9980d2 ("Makefile: help
people who run 'make check' by mistake", 2008-11-11)]

The 'check' target was introduced in commit 44c9e8594e ("Fix up header file
dependencies and add sparse checking rules", 2005-07-03). As you can see,
that target has nothing to do with autoconf/configure.

Exactly a year later, support for autoconf tools was added in commit
556677144b ("autoconf: Use autoconf to write installation directories to config.mak.autogen", 2006-07-03). This provides an 'alternative' to just
using the Makefile (which is the _primary_ method used to build git).

I suspect that the majority of git developers don't use the autoconf
tools (I have no numbers, just gut feeling).

I added the 'sparse' target in commit 0bcd9ae85d ("sparse: Fix errors due to missing target-specific variables", 2011-04-21). After nine years, we could
perhaps just drop the 'check' target altogether? dunno.

> I /think/ if we have a rule in Makefile, we should adhere to it.

Hmm, do you mean that 'make check' should be _reserved_ for autoconf use?
[fun fact: the test target in the sparse Makefile is 'check' :-D ]

> I also fix another change in ds/blame-on-bloom, which I think it's worth to
> fix, see: <20200423133937.GA1984@danh.dev>

Yes, looks good.

> This series is developed from latest master.

I have some comments on the patches (I'm running out of time here,
so I may not get to them till tomorrow).

> It's conflicted when merge down next and pu.
> 
> For next, please ignore the conflict with fmt-merge-msg.c and
> apply [5/4] instead.
> 
> For pu, reftable.c has too much warnings, I'm too lazy to look into it.

For those interested, the 'too much warnings' on the 'pu' branch looks
like (for pu@faf094bf11):

  $ make sparse >psp-out 2>&1
  $ diff nsp-out psp-out
  154a155
  >     SP refs/reftable-backend.c
  406a408
  >     SP t/helper/test-proc-receive.c
  447a450,511
  >     SP reftable/basics.c
  > reftable/basics.c:157:6: warning: symbol 'reftable_malloc_ptr' was not declared. Should it be static?
  > reftable/basics.c:158:6: warning: symbol 'reftable_realloc_ptr' was not declared. Should it be static?
  > reftable/basics.c:159:6: warning: symbol 'reftable_free_ptr' was not declared. Should it be static?
  >     SP reftable/block.c
  >     SP reftable/bytes.c
  >     SP reftable/file.c
  > reftable/file.c:57:37: warning: symbol 'file_vtable' was not declared. Should it be static?
  >     SP reftable/iter.c
  > reftable/iter.c:32:33: warning: symbol 'empty_vtable' was not declared. Should it be static?
  > reftable/iter.c:61:31: warning: Using plain integer as NULL pointer
  > reftable/iter.c:69:31: warning: Using plain integer as NULL pointer
  > reftable/iter.c:96:57: warning: Using plain integer as NULL pointer
  > reftable/iter.c:124:33: warning: symbol 'filtering_ref_iterator_vtable' was not declared. Should it be static?
  > reftable/iter.c:224:33: warning: symbol 'indexed_table_ref_iter_vtable' was not declared. Should it be static?
  >     SP reftable/merged.c
  > reftable/merged.c:141:33: warning: symbol 'merged_iter_vtable' was not declared. Should it be static?
  > reftable/merged.c:283:31: warning: Using plain integer as NULL pointer
  > reftable/merged.c:296:31: warning: Using plain integer as NULL pointer
  >     SP reftable/pq.c
  >     SP reftable/reader.c
  > reftable/reader.c:180:42: warning: Using plain integer as NULL pointer
  > reftable/reader.c:181:42: warning: Using plain integer as NULL pointer
  > reftable/reader.c:284:41: warning: Using plain integer as NULL pointer
  > reftable/reader.c:360:44: warning: Using plain integer as NULL pointer
  > reftable/reader.c:396:33: warning: symbol 'table_iter_vtable' was not declared. Should it be static?
  > reftable/reader.c:450:36: warning: Using plain integer as NULL pointer
  > reftable/reader.c:498:42: warning: Using plain integer as NULL pointer
  > reftable/reader.c:500:44: warning: Using plain integer as NULL pointer
  > reftable/reader.c:501:42: warning: Using plain integer as NULL pointer
  > reftable/reader.c:502:36: warning: Using plain integer as NULL pointer
  > reftable/reader.c:565:34: warning: Using plain integer as NULL pointer
  > reftable/reader.c:610:31: warning: Using plain integer as NULL pointer
  > reftable/reader.c:623:31: warning: Using plain integer as NULL pointer
  > reftable/reader.c:669:36: warning: Using plain integer as NULL pointer
  > reftable/reader.c:670:42: warning: Using plain integer as NULL pointer
  > reftable/reader.c:671:35: warning: Using plain integer as NULL pointer
  > reftable/reader.c:672:35: warning: Using plain integer as NULL pointer
  >     SP reftable/record.c
  > reftable/record.c:556:22: warning: symbol 'obj_record_vtable' was not declared. Should it be static?
  > reftable/record.c:839:22: warning: symbol 'reftable_log_record_vtable' was not declared. Should it be static?
  > reftable/record.c:961:22: warning: symbol 'index_record_vtable' was not declared. Should it be static?
  >     SP reftable/slice.c
  > reftable/slice.c:180:37: warning: symbol 'slice_vtable' was not declared. Should it be static?
  > reftable/slice.c:200:37: warning: symbol 'malloc_vtable' was not declared. Should it be static?
  > reftable/slice.c:204:30: warning: symbol 'malloc_block_source_instance' was not declared. Should it be static?
  >     SP reftable/stack.c
  > reftable/stack.c:157:62: warning: Using plain integer as NULL pointer
  > reftable/stack.c:667:41: warning: Using plain integer as NULL pointer
  > reftable/stack.c:668:44: warning: Using plain integer as NULL pointer
  > reftable/stack.c:669:44: warning: Using plain integer as NULL pointer
  > reftable/stack.c:1085:41: warning: Using plain integer as NULL pointer
  > reftable/stack.c:1111:41: warning: Using plain integer as NULL pointer
  >     SP reftable/tree.c
  >     SP reftable/writer.c
  > reftable/writer.c:231:31: warning: Using plain integer as NULL pointer
  > reftable/writer.c:299:39: warning: Using plain integer as NULL pointer
  > reftable/writer.c:347:47: warning: Using plain integer as NULL pointer
  > reftable/writer.c:426:31: warning: Using plain integer as NULL pointer
  > reftable/writer.c:468:45: warning: Using plain integer as NULL pointer
  > reftable/writer.c:584:11: warning: symbol 'debug' was not declared. Should it be static?
  >     SP reftable/zlib-compat.c

Also, my static-check perl script says the following symbols are not
used outside the file which defines them (so they could be marked static):

  $ ./static-check.pl >psc
  $ diff nsc psc
  71a72,136
  > reftable/basics.o	- reftable_free_ptr
  > reftable/basics.o	- reftable_malloc_ptr
  > reftable/basics.o	- reftable_realloc_ptr
  > reftable/basics.o	- reftable_set_alloc
  > reftable/block.o	- block_reader_seek
  > reftable/block.o	- block_writer_register_restart
  > reftable/file.o	- file_vtable
  > reftable/iter.o	- empty_vtable
  > reftable/iter.o	- filtering_ref_iterator_vtable
  > reftable/iter.o	- indexed_table_ref_iter_vtable
  > reftable/merged.o	- merged_iter_vtable
  > reftable/merged.o	- reftable_merged_table_max_update_index
  > reftable/merged.o	- reftable_merged_table_min_update_index
  > reftable/merged.o	- reftable_merged_table_seek_log_at
  > reftable/pq.o	- merged_iter_pqueue_check
  > reftable/pq.o	- pq_less
  > reftable/reader.o	- block_source_close
  > reftable/reader.o	- block_source_read_block
  > reftable/reader.o	- block_source_size
  > reftable/reader.o	- init_reader
  > reftable/reader.o	- reftable_reader_hash_id
  > reftable/reader.o	- reftable_reader_refs_for
  > reftable/reader.o	- reftable_reader_seek_log
  > reftable/reader.o	- reftable_reader_seek_log_at
  > reftable/reader.o	- table_iter_vtable
  > reftable/record.o	- get_var_int
  > reftable/record.o	- index_record_vtable
  > reftable/record.o	- obj_record_vtable
  > reftable/record.o	- put_var_int
  > reftable/record.o	- record_as_log
  > reftable/record.o	- record_as_ref
  > reftable/record.o	- reftable_log_record_equal
  > reftable/record.o	- reftable_log_record_print
  > reftable/record.o	- reftable_log_record_vtable
  > reftable/record.o	- reftable_ref_record_equal
  > reftable/record.o	- reftable_ref_record_print
  > reftable/record.o	- reftable_ref_record_vtable
  > reftable/slice.o	- block_source_from_slice
  > reftable/slice.o	- malloc_block_source_instance
  > reftable/slice.o	- malloc_vtable
  > reftable/slice.o	- slice_equal
  > reftable/slice.o	- slice_vtable
  > reftable/slice.o	- slice_write
  > reftable/slice.o	- slice_write_void
  > reftable/slice.o	- slice_yield
  > reftable/stack.o	- fastlog2
  > reftable/stack.o	- read_lines
  > reftable/stack.o	- reftable_addition_add
  > reftable/stack.o	- reftable_addition_close
  > reftable/stack.o	- reftable_addition_commit
  > reftable/stack.o	- reftable_stack_auto_compact
  > reftable/stack.o	- reftable_stack_compaction_stats
  > reftable/stack.o	- reftable_stack_destroy
  > reftable/stack.o	- reftable_stack_new_addition
  > reftable/stack.o	- reftable_stack_read_log
  > reftable/stack.o	- reftable_stack_reload
  > reftable/stack.o	- sizes_to_segments
  > reftable/stack.o	- stack_try_add
  > reftable/stack.o	- stack_write_compact
  > reftable/stack.o	- suggest_compaction_segment
  > reftable/writer.o	- debug
  > reftable/writer.o	- writer_clear_index
  > reftable/writer.o	- writer_finish_public_section
  > reftable/writer.o	- writer_flush_block
  > reftable/writer.o	- writer_stats

Note: I have not looked at any of the reftable patches/files.

ATB,
Ramsay Jones

Re: [PATCH 0/4] fix make sparse warning

[Danh Doan]

On 2020-04-24 00:10:25+0100, Ramsay Jones <ramsay@ramsayjones.plus.com> wrote:
> 
> 
> On 23/04/2020 14:47, Đoàn Trần Công Danh wrote:
> > It's happened that I tried to run make check on project that support autoconf.
> 
> I try to ignore autoconf as much as possible, so I don't know why people

For some reason, people refer to autotools as autohell

> who use it regularly seem to expect a 'make check' target (is that anything
> to do with ./config.status --recheck?).

Because it's autotools's recommendation.
https://www.gnu.org/savannah-checkouts/gnu/autoconf/manual/autoconf-2.69/html_node/Basic-Installation.html#Basic-Installation
https://www.gnu.org/software/automake/manual/automake.html#Standard-Targets

Yes, Linux kernel, and Git don't like GNU Coding Standards (and me
neither, because of their weird bracket position). But, it's
reasonable expectation if autotools is used.

> > Git's Makefile told me to run `make sparse` instead.
> 
> In which case, you must have had sparse installed, otherwise you would
> have been directed to 'make test'. [See commit 912f9980d2 ("Makefile: help
> people who run 'make check' by mistake", 2008-11-11)]
> 
> The 'check' target was introduced in commit 44c9e8594e ("Fix up header file
> dependencies and add sparse checking rules", 2005-07-03). As you can see,
> that target has nothing to do with autoconf/configure.
> 
> Exactly a year later, support for autoconf tools was added in commit
> 556677144b ("autoconf: Use autoconf to write installation
> directories to config.mak.autogen", 2006-07-03). This provides an
> 'alternative' to just
> using the Makefile (which is the _primary_ method used to build git).
> 
> I suspect that the majority of git developers don't use the autoconf
> tools (I have no numbers, just gut feeling).

I also think so.
 
> I added the 'sparse' target in commit 0bcd9ae85d ("sparse: Fix
> errors due to missing target-specific variables", 2011-04-21). After
> nine years, we could
> perhaps just drop the 'check' target altogether? dunno.
> 
> > I /think/ if we have a rule in Makefile, we should adhere to it.
> 
> Hmm, do you mean that 'make check' should be _reserved_ for autoconf use?
> [fun fact: the test target in the sparse Makefile is 'check' :-D ]

No, I didn't mean that, I just want to fix sparse warning.

I don't care much about the target's name,
in fact, I prefer `make test` over `make check`.

If we don't really care about sparse warning.
Let's remove this sparse target.

But, I think we do care about some sparse warning that modern compiler
stopped caring about, like the one with final_new_line.

> > I also fix another change in ds/blame-on-bloom, which I think it's worth to
> > fix, see: <20200423133937.GA1984@danh.dev>
> 
> Yes, looks good.
> 
> > This series is developed from latest master.
> 
> I have some comments on the patches (I'm running out of time here,
> so I may not get to them till tomorrow).
> 
> > It's conflicted when merge down next and pu.
> > 
> > For next, please ignore the conflict with fmt-merge-msg.c and
> > apply [5/4] instead.
> > 
> > For pu, reftable.c has too much warnings, I'm too lazy to look into it.
> 
> For those interested, the 'too much warnings' on the 'pu' branch looks
> like (for pu@faf094bf11):
> 
>   $ make sparse >psp-out 2>&1
>   $ diff nsp-out psp-out
>   154a155
>   >     SP refs/reftable-backend.c
>   406a408
>   >     SP t/helper/test-proc-receive.c
>   447a450,511
>   >     SP reftable/basics.c
>   > reftable/basics.c:157:6: warning: symbol 'reftable_malloc_ptr' was not declared. Should it be static?
>   > reftable/basics.c:158:6: warning: symbol 'reftable_realloc_ptr' was not declared. Should it be static?
>   > reftable/basics.c:159:6: warning: symbol 'reftable_free_ptr' was not declared. Should it be static?
>   >     SP reftable/block.c
>   >     SP reftable/bytes.c
>   >     SP reftable/file.c
>   > reftable/file.c:57:37: warning: symbol 'file_vtable' was not declared. Should it be static?
>   >     SP reftable/iter.c
>   > reftable/iter.c:32:33: warning: symbol 'empty_vtable' was not declared. Should it be static?
>   > reftable/iter.c:61:31: warning: Using plain integer as NULL pointer
>   > reftable/iter.c:69:31: warning: Using plain integer as NULL pointer
>   > reftable/iter.c:96:57: warning: Using plain integer as NULL pointer
>   > reftable/iter.c:124:33: warning: symbol 'filtering_ref_iterator_vtable' was not declared. Should it be static?
>   > reftable/iter.c:224:33: warning: symbol 'indexed_table_ref_iter_vtable' was not declared. Should it be static?
>   >     SP reftable/merged.c
>   > reftable/merged.c:141:33: warning: symbol 'merged_iter_vtable' was not declared. Should it be static?
>   > reftable/merged.c:283:31: warning: Using plain integer as NULL pointer
>   > reftable/merged.c:296:31: warning: Using plain integer as NULL pointer
>   >     SP reftable/pq.c
>   >     SP reftable/reader.c
>   > reftable/reader.c:180:42: warning: Using plain integer as NULL pointer
>   > reftable/reader.c:181:42: warning: Using plain integer as NULL pointer
>   > reftable/reader.c:284:41: warning: Using plain integer as NULL pointer
>   > reftable/reader.c:360:44: warning: Using plain integer as NULL pointer
>   > reftable/reader.c:396:33: warning: symbol 'table_iter_vtable' was not declared. Should it be static?
>   > reftable/reader.c:450:36: warning: Using plain integer as NULL pointer
>   > reftable/reader.c:498:42: warning: Using plain integer as NULL pointer
>   > reftable/reader.c:500:44: warning: Using plain integer as NULL pointer
>   > reftable/reader.c:501:42: warning: Using plain integer as NULL pointer
>   > reftable/reader.c:502:36: warning: Using plain integer as NULL pointer
>   > reftable/reader.c:565:34: warning: Using plain integer as NULL pointer
>   > reftable/reader.c:610:31: warning: Using plain integer as NULL pointer
>   > reftable/reader.c:623:31: warning: Using plain integer as NULL pointer
>   > reftable/reader.c:669:36: warning: Using plain integer as NULL pointer
>   > reftable/reader.c:670:42: warning: Using plain integer as NULL pointer
>   > reftable/reader.c:671:35: warning: Using plain integer as NULL pointer
>   > reftable/reader.c:672:35: warning: Using plain integer as NULL pointer
>   >     SP reftable/record.c
>   > reftable/record.c:556:22: warning: symbol 'obj_record_vtable' was not declared. Should it be static?
>   > reftable/record.c:839:22: warning: symbol 'reftable_log_record_vtable' was not declared. Should it be static?
>   > reftable/record.c:961:22: warning: symbol 'index_record_vtable' was not declared. Should it be static?
>   >     SP reftable/slice.c
>   > reftable/slice.c:180:37: warning: symbol 'slice_vtable' was not declared. Should it be static?
>   > reftable/slice.c:200:37: warning: symbol 'malloc_vtable' was not declared. Should it be static?
>   > reftable/slice.c:204:30: warning: symbol 'malloc_block_source_instance' was not declared. Should it be static?
>   >     SP reftable/stack.c
>   > reftable/stack.c:157:62: warning: Using plain integer as NULL pointer
>   > reftable/stack.c:667:41: warning: Using plain integer as NULL pointer
>   > reftable/stack.c:668:44: warning: Using plain integer as NULL pointer
>   > reftable/stack.c:669:44: warning: Using plain integer as NULL pointer
>   > reftable/stack.c:1085:41: warning: Using plain integer as NULL pointer
>   > reftable/stack.c:1111:41: warning: Using plain integer as NULL pointer
>   >     SP reftable/tree.c
>   >     SP reftable/writer.c
>   > reftable/writer.c:231:31: warning: Using plain integer as NULL pointer
>   > reftable/writer.c:299:39: warning: Using plain integer as NULL pointer
>   > reftable/writer.c:347:47: warning: Using plain integer as NULL pointer
>   > reftable/writer.c:426:31: warning: Using plain integer as NULL pointer
>   > reftable/writer.c:468:45: warning: Using plain integer as NULL pointer
>   > reftable/writer.c:584:11: warning: symbol 'debug' was not declared. Should it be static?
>   >     SP reftable/zlib-compat.c
> 
> Also, my static-check perl script says the following symbols are not
> used outside the file which defines them (so they could be marked static):

All of them are in reftable, and they're catched by `make sparse`,
too.

>   $ ./static-check.pl >psc
>   $ diff nsc psc
>   71a72,136
>   > reftable/basics.o	- reftable_free_ptr
>   > reftable/basics.o	- reftable_malloc_ptr
>   > reftable/basics.o	- reftable_realloc_ptr
>   > reftable/basics.o	- reftable_set_alloc
>   > reftable/block.o	- block_reader_seek
>   > reftable/block.o	- block_writer_register_restart
>   > reftable/file.o	- file_vtable
>   > reftable/iter.o	- empty_vtable
>   > reftable/iter.o	- filtering_ref_iterator_vtable
>   > reftable/iter.o	- indexed_table_ref_iter_vtable
>   > reftable/merged.o	- merged_iter_vtable
>   > reftable/merged.o	- reftable_merged_table_max_update_index
>   > reftable/merged.o	- reftable_merged_table_min_update_index
>   > reftable/merged.o	- reftable_merged_table_seek_log_at
>   > reftable/pq.o	- merged_iter_pqueue_check
>   > reftable/pq.o	- pq_less
>   > reftable/reader.o	- block_source_close
>   > reftable/reader.o	- block_source_read_block
>   > reftable/reader.o	- block_source_size
>   > reftable/reader.o	- init_reader
>   > reftable/reader.o	- reftable_reader_hash_id
>   > reftable/reader.o	- reftable_reader_refs_for
>   > reftable/reader.o	- reftable_reader_seek_log
>   > reftable/reader.o	- reftable_reader_seek_log_at
>   > reftable/reader.o	- table_iter_vtable
>   > reftable/record.o	- get_var_int
>   > reftable/record.o	- index_record_vtable
>   > reftable/record.o	- obj_record_vtable
>   > reftable/record.o	- put_var_int
>   > reftable/record.o	- record_as_log
>   > reftable/record.o	- record_as_ref
>   > reftable/record.o	- reftable_log_record_equal
>   > reftable/record.o	- reftable_log_record_print
>   > reftable/record.o	- reftable_log_record_vtable
>   > reftable/record.o	- reftable_ref_record_equal
>   > reftable/record.o	- reftable_ref_record_print
>   > reftable/record.o	- reftable_ref_record_vtable
>   > reftable/slice.o	- block_source_from_slice
>   > reftable/slice.o	- malloc_block_source_instance
>   > reftable/slice.o	- malloc_vtable
>   > reftable/slice.o	- slice_equal
>   > reftable/slice.o	- slice_vtable
>   > reftable/slice.o	- slice_write
>   > reftable/slice.o	- slice_write_void
>   > reftable/slice.o	- slice_yield
>   > reftable/stack.o	- fastlog2
>   > reftable/stack.o	- read_lines
>   > reftable/stack.o	- reftable_addition_add
>   > reftable/stack.o	- reftable_addition_close
>   > reftable/stack.o	- reftable_addition_commit
>   > reftable/stack.o	- reftable_stack_auto_compact
>   > reftable/stack.o	- reftable_stack_compaction_stats
>   > reftable/stack.o	- reftable_stack_destroy
>   > reftable/stack.o	- reftable_stack_new_addition
>   > reftable/stack.o	- reftable_stack_read_log
>   > reftable/stack.o	- reftable_stack_reload
>   > reftable/stack.o	- sizes_to_segments
>   > reftable/stack.o	- stack_try_add
>   > reftable/stack.o	- stack_write_compact
>   > reftable/stack.o	- suggest_compaction_segment
>   > reftable/writer.o	- debug
>   > reftable/writer.o	- writer_clear_index
>   > reftable/writer.o	- writer_finish_public_section
>   > reftable/writer.o	- writer_flush_block
>   > reftable/writer.o	- writer_stats
> 
> Note: I have not looked at any of the reftable patches/files.

Me neither.

-- 
Danh

Re: [PATCH 1/4] C: s/0/NULL/ for pointer type

[Ramsay Jones]

On 23/04/2020 14:47, Đoàn Trần Công Danh wrote:
> Fix warning from  `make sparse`.

You may want to split the changes to 't/helper/test-parse-pathspec-file.c'
into its own patch, since those changes are not potentially controversial.

The remainder of this patch follows a pattern that I used in a patch that
was rejected; see https://public-inbox.org/git/e6796c60-a870-e761-3b07-b680f934c537@ramsayjones.plus.com/, et seq.

Actually, I have a patch somewhere which suppressed the sparse warning
for the '= { 0 }' token sequence used in these initializations. However,
I don't seem to be able to find them at the moment! :(

[Luc, this topic came up on the sparse and kernel mailing-lists at one
point, but I didn't get around to posting my patch to the list - something
came up. Hopefully, I will find some time to find it and post it soon.]

ATB,
Ramsay Jones

> 
> Signed-off-by: Đoàn Trần Công Danh <congdanhqx@gmail.com>
> ---
>  add-interactive.c                   | 2 +-
>  builtin/fmt-merge-msg.c             | 2 +-
>  log-tree.c                          | 4 ++--
>  range-diff.c                        | 2 +-
>  t/helper/test-parse-pathspec-file.c | 6 +++---
>  5 files changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/add-interactive.c b/add-interactive.c
> index 29cd2fe020..b8983838b9 100644
> --- a/add-interactive.c
> +++ b/add-interactive.c
> @@ -526,7 +526,7 @@ static int get_modified_files(struct repository *r,
>  
>  	for (i = 0; i < 2; i++) {
>  		struct rev_info rev;
> -		struct setup_revision_opt opt = { 0 };
> +		struct setup_revision_opt opt = { NULL };
>  
>  		if (filter == INDEX_ONLY)
>  			s.mode = (i == 0) ? FROM_INDEX : FROM_WORKTREE;
> diff --git a/builtin/fmt-merge-msg.c b/builtin/fmt-merge-msg.c
> index 172dfbd852..f4376bccef 100644
> --- a/builtin/fmt-merge-msg.c
> +++ b/builtin/fmt-merge-msg.c
> @@ -494,7 +494,7 @@ static void fmt_merge_msg_sigs(struct strbuf *out)
>  		enum object_type type;
>  		unsigned long size, len;
>  		char *buf = read_object_file(oid, &type, &size);
> -		struct signature_check sigc = { 0 };
> +		struct signature_check sigc = { NULL };
>  		struct strbuf sig = STRBUF_INIT;
>  
>  		if (!buf || type != OBJ_TAG)
> diff --git a/log-tree.c b/log-tree.c
> index 0064788b25..ca721150d4 100644
> --- a/log-tree.c
> +++ b/log-tree.c
> @@ -449,7 +449,7 @@ static void show_signature(struct rev_info *opt, struct commit *commit)
>  {
>  	struct strbuf payload = STRBUF_INIT;
>  	struct strbuf signature = STRBUF_INIT;
> -	struct signature_check sigc = { 0 };
> +	struct signature_check sigc = { NULL };
>  	int status;
>  
>  	if (parse_signed_commit(commit, &payload, &signature) <= 0)
> @@ -496,7 +496,7 @@ static int show_one_mergetag(struct commit *commit,
>  	struct object_id oid;
>  	struct tag *tag;
>  	struct strbuf verify_message;
> -	struct signature_check sigc = { 0 };
> +	struct signature_check sigc = { NULL };
>  	int status, nth;
>  	size_t payload_size;
>  
> diff --git a/range-diff.c b/range-diff.c
> index f745567cf6..71dcd947c5 100644
> --- a/range-diff.c
> +++ b/range-diff.c
> @@ -107,7 +107,7 @@ static int read_patches(const char *range, struct string_list *list,
>  		}
>  
>  		if (starts_with(line, "diff --git")) {
> -			struct patch patch = { 0 };
> +			struct patch patch = { NULL };
>  			struct strbuf root = STRBUF_INIT;
>  			int linenr = 0;
>  
> diff --git a/t/helper/test-parse-pathspec-file.c b/t/helper/test-parse-pathspec-file.c
> index 02f4ccfd2a..b3e08cef4b 100644
> --- a/t/helper/test-parse-pathspec-file.c
> +++ b/t/helper/test-parse-pathspec-file.c
> @@ -6,7 +6,7 @@
>  int cmd__parse_pathspec_file(int argc, const char **argv)
>  {
>  	struct pathspec pathspec;
> -	const char *pathspec_from_file = 0;
> +	const char *pathspec_from_file = NULL;
>  	int pathspec_file_nul = 0, i;
>  
>  	static const char *const usage[] = {
> @@ -20,9 +20,9 @@ int cmd__parse_pathspec_file(int argc, const char **argv)
>  		OPT_END()
>  	};
>  
> -	parse_options(argc, argv, 0, options, usage, 0);
> +	parse_options(argc, argv, NULL, options, usage, 0);
>  
> -	parse_pathspec_file(&pathspec, 0, 0, 0, pathspec_from_file,
> +	parse_pathspec_file(&pathspec, 0, 0, NULL, pathspec_from_file,
>  			    pathspec_file_nul);
>  
>  	for (i = 0; i < pathspec.nr; i++)
> 

Re: [PATCH 2/4] compat/regex: silence `make sparse` warning

[Ramsay Jones]

On 23/04/2020 14:47, Đoàn Trần Công Danh wrote:
> * alloca: somewhere later in the code, we indirectly include alloca.h
> which will define alloca again, include it prior to define alloca in
> order to not define it against.
> 
> * Copy all attributes from the header to source file, and move the
>   attributes prior to function name. cgcc is very picky on the position
>   of attribute.

This patch is no longer needed (iff you use a bleeding edge version of
sparse), since this was fixed in commit 172f6a98 ("let function definition inherit prototype attributes", 2019-11-19).

  $ git describe 172f6a98
  v0.6.1-37-g172f6a98
  $ 

Ah, so this fix is not in any released version of sparse (v0.6.1 is the
current release - I always build/run the tip of the 'master' branch from
the sparse git repo).

I starred at this warning for years (on cygwin, I don't build with NO_REGEX
on Linux), but I didn't send a patch for it because it was a sparse fault.

ATB,
Ramsay Jones

> 
> Signed-off-by: Đoàn Trần Công Danh <congdanhqx@gmail.com>
> ---
>  compat/regex/regex.c          | 1 +
>  compat/regex/regex_internal.c | 2 +-
>  compat/regex/regex_internal.h | 5 ++---
>  3 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/compat/regex/regex.c b/compat/regex/regex.c
> index f3e03a9eab..4bef75a716 100644
> --- a/compat/regex/regex.c
> +++ b/compat/regex/regex.c
> @@ -62,6 +62,7 @@
>  #include <stdint.h>
>  
>  #ifdef GAWK
> +#include <alloca.h>
>  #undef alloca
>  #define alloca alloca_is_bad_you_should_never_use_it
>  #endif
> diff --git a/compat/regex/regex_internal.c b/compat/regex/regex_internal.c
> index ec51cf3446..58504f795b 100644
> --- a/compat/regex/regex_internal.c
> +++ b/compat/regex/regex_internal.c
> @@ -921,7 +921,7 @@ re_string_destruct (re_string_t *pstr)
>  /* Return the context at IDX in INPUT.  */
>  
>  static unsigned int
> -internal_function
> +internal_function __attribute ((pure))
>  re_string_context_at (const re_string_t *input, int idx, int eflags)
>  {
>    int c;
> diff --git a/compat/regex/regex_internal.h b/compat/regex/regex_internal.h
> index 3ee8aae59d..c61a1e4971 100644
> --- a/compat/regex/regex_internal.h
> +++ b/compat/regex/regex_internal.h
> @@ -430,9 +430,8 @@ static reg_errcode_t build_wcs_upper_buffer (re_string_t *pstr)
>  # endif /* RE_ENABLE_I18N */
>  static void build_upper_buffer (re_string_t *pstr) internal_function;
>  static void re_string_translate_buffer (re_string_t *pstr) internal_function;
> -static unsigned int re_string_context_at (const re_string_t *input, int idx,
> -					  int eflags)
> -     internal_function __attribute ((pure));
> +static internal_function __attribute ((pure))
> +unsigned int re_string_context_at (const re_string_t *input, int idx, int eflags);
>  #endif
>  #define re_string_peek_byte(pstr, offset) \
>    ((pstr)->mbs[(pstr)->cur_idx + offset])
> 

Re: [PATCH 3/4] graph.c: limit linkage of internal variable

[Ramsay Jones]

On 23/04/2020 14:47, Đoàn Trần Công Danh wrote:
> Signed-off-by: Đoàn Trần Công Danh <congdanhqx@gmail.com>
> ---
>  graph.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/graph.c b/graph.c
> index 4fb25ad464..4cd9915075 100644
> --- a/graph.c
> +++ b/graph.c
> @@ -1055,7 +1055,7 @@ static void graph_output_commit_line(struct git_graph *graph, struct graph_line
>  		graph_update_state(graph, GRAPH_COLLAPSING);
>  }
>  
> -const char merge_chars[] = {'/', '|', '\\'};
> +static const char merge_chars[] = {'/', '|', '\\'};
>  
>  static void graph_output_post_merge_line(struct git_graph *graph, struct graph_line *line)
>  {
> 

Yep, LGTM.

ATB,
Ramsay Jones

Re: [PATCH 1/4] C: s/0/NULL/ for pointer type

[Junio C Hamano]

Ramsay Jones <ramsay@ramsayjones.plus.com> writes:

> Actually, I have a patch somewhere which suppressed the sparse warning
> for the '= { 0 }' token sequence used in these initializations. However,
> I don't seem to be able to find them at the moment! :(

AHHHHHhhhhhhhh.  

Thanks for reminding.  Yeah, I recall that the concensus of those
who were vocal in that old discussion [*1*] was that "= { 0 }"
should be taken as an idiom and should not be subject to s/0/NULL/
conversion.

> [Luc, this topic came up on the sparse and kernel mailing-lists at one
> point, but I didn't get around to posting my patch to the list - something
> came up. Hopefully, I will find some time to find it and post it soon.]


[References]

*1*

https://lore.kernel.org/git/e6796c60-a870-e761-3b07-b680f934c537@ramsayjones.plus.com/

Re: [PATCH 4/4] progress.c: silence cgcc suggestion about internal linkage

[Ramsay Jones]

On 23/04/2020 14:47, Đoàn Trần Công Danh wrote:
> Signed-off-by: Đoàn Trần Công Danh <congdanhqx@gmail.com>
> ---
>  progress.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/progress.c b/progress.c
> index 19805ac646..fb53a2ec0c 100644
> --- a/progress.c
> +++ b/progress.c
> @@ -50,7 +50,9 @@ static volatile sig_atomic_t progress_update;
>   * These are only intended for testing the progress output, i.e. exclusively
>   * for 'test-tool progress'.
>   */
> +extern int progress_testing; /* to silence sparse: internal linkage */
>  int progress_testing;
> +extern uint64_t progress_test_ns; /* to silence sparse: internal linkage */
>  uint64_t progress_test_ns = 0;
>  void progress_test_force_update(void); /* To silence -Wmissing-prototypes */
>  void progress_test_force_update(void)

My preference would be to add these extern declarations to the
progress.h header file, with a note that they are only used by
the 't/helper/test-progress.c' test helper. (Also, remove the
extern declarations from test-progress.c, of course).

But this works as well. :D

ATB,
Ramsay Jones

Re: [PATCH 2/4] compat/regex: silence `make sparse` warning

[Danh Doan]

On 2020-04-24 01:51:40+0100, Ramsay Jones <ramsay@ramsayjones.plus.com> wrote:
> 
> 
> On 23/04/2020 14:47, Đoàn Trần Công Danh wrote:
> > * alloca: somewhere later in the code, we indirectly include alloca.h
> > which will define alloca again, include it prior to define alloca in
> > order to not define it against.
> > 
> > * Copy all attributes from the header to source file, and move the
> >   attributes prior to function name. cgcc is very picky on the position
> >   of attribute.
> 
> This patch is no longer needed (iff you use a bleeding edge version of
> sparse), since this was fixed in commit 172f6a98 ("let function definition inherit prototype attributes", 2019-11-19).
> 
>   $ git describe 172f6a98
>   v0.6.1-37-g172f6a98
>   $ 
> 
> Ah, so this fix is not in any released version of sparse (v0.6.1 is the
> current release - I always build/run the tip of the 'master' branch from
> the sparse git repo).

Correct, when I saw your bleeding edge, I check my sparse version. It
says 0.6.1

> I starred at this warning for years (on cygwin, I don't build with NO_REGEX
> on Linux), but I didn't send a patch for it because it was a sparse fault.

It's sparse' fault, of course, I talked about its pickiness in the
commit message after all.

But, we still need the alloca part of this patch.

-- 
Danh

Re: [PATCH 1/4] C: s/0/NULL/ for pointer type

[Danh Doan]

On 2020-04-23 17:54:30-0700, Junio C Hamano <gitster@pobox.com> wrote:
> Ramsay Jones <ramsay@ramsayjones.plus.com> writes:
> 
> > Actually, I have a patch somewhere which suppressed the sparse warning
> > for the '= { 0 }' token sequence used in these initializations. However,
> > I don't seem to be able to find them at the moment! :(
> 
> AHHHHHhhhhhhhh.  
> 
> Thanks for reminding.  Yeah, I recall that the concensus of those
> who were vocal in that old discussion [*1*] was that "= { 0 }"
> should be taken as an idiom and should not be subject to s/0/NULL/
> conversion.

So, to follow that idiom, this patch should be changed, too?
https://lore.kernel.org/git/nycvar.QRO.7.76.6.2004101604210.46@tvgsbejvaqbjf.bet/

+Cc: Dscho and Han-Wen

> > [Luc, this topic came up on the sparse and kernel mailing-lists at one
> > point, but I didn't get around to posting my patch to the list - something
> > came up. Hopefully, I will find some time to find it and post it soon.]
> 
> 
> [References]
> 
> *1*
> 
> https://lore.kernel.org/git/e6796c60-a870-e761-3b07-b680f934c537@ramsayjones.plus.com/

-- 
Danh

Re: [PATCH 1/4] C: s/0/NULL/ for pointer type

[Junio C Hamano]

Danh Doan <congdanhqx@gmail.com> writes:

> So, to follow that idiom, this patch should be changed, too?
> https://lore.kernel.org/git/nycvar.QRO.7.76.6.2004101604210.46@tvgsbejvaqbjf.bet/

I didn't look at that patch carefully, but it seems that it changes

	struct foo variable_to_be_initialized = {};

into various forms, e.g.

	struct foo variable_to_be_initialized = { NULL };
	struct foo variable_to_be_initialized = { { NULL } };

depending on the actual shape of 'foo', and that old thread explains
that it is sufficient to consistently write "= { 0 };". 

Having said that, if I recall correctly, Dscho suggested even larger
style fixes to the code, so if that were to happen, the above initializer
fix may just be lost in the noise ;-)

Thanks.

> +Cc: Dscho and Han-Wen
>
>> > [Luc, this topic came up on the sparse and kernel mailing-lists at one
>> > point, but I didn't get around to posting my patch to the list - something
>> > came up. Hopefully, I will find some time to find it and post it soon.]
>> 
>> 
>> [References]
>> 
>> *1*
>> 
>> https://lore.kernel.org/git/e6796c60-a870-e761-3b07-b680f934c537@ramsayjones.plus.com/

Re: [PATCH 4/4] progress.c: silence cgcc suggestion about internal linkage

[Jeff King]

On Fri, Apr 24, 2020 at 01:58:51AM +0100, Ramsay Jones wrote:

> > +extern int progress_testing; /* to silence sparse: internal linkage */
> >  int progress_testing;
> > +extern uint64_t progress_test_ns; /* to silence sparse: internal linkage */
> >  uint64_t progress_test_ns = 0;
> >  void progress_test_force_update(void); /* To silence -Wmissing-prototypes */
> >  void progress_test_force_update(void)
> 
> My preference would be to add these extern declarations to the
> progress.h header file, with a note that they are only used by
> the 't/helper/test-progress.c' test helper. (Also, remove the
> extern declarations from test-progress.c, of course).

I came here to say the same thing.

As a bonus, it would catch any changes to the types in the future
(rather than just segfaulting when test-progress.c attempts to use an
out-of-date type).

-Peff

[PATCH v2 0/4] Fix Sparse Warning

[Đoàn Trần Công Danh]

Change in v2 from v1:
* = { 0 } is an idiom to zero the structure, it shouldn't be changed
* change in [4/4] for prototype
* ignore diffent function prototype

Đoàn Trần Công Danh (4):
  test-parse-pathspec-file.c: s/0/NULL/ for pointer type
  compat/regex: include alloca.h before undef it
  graph.c: limit linkage of internal variable
  progress.c: silence cgcc suggestion about internal linkage

 compat/regex/regex.c                | 1 +
 graph.c                             | 2 +-
 progress.c                          | 2 +-
 progress.h                          | 8 ++++++++
 t/helper/test-parse-pathspec-file.c | 6 +++---
 t/helper/test-progress.c            | 9 +--------
 6 files changed, 15 insertions(+), 13 deletions(-)

-- 
2.26.2.384.g435bf60bd5

[PATCH v2 1/4] test-parse-pathspec-file.c: s/0/NULL/ for pointer type

[Đoàn Trần Công Danh]

Signed-off-by: Đoàn Trần Công Danh <congdanhqx@gmail.com>
---
 t/helper/test-parse-pathspec-file.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/t/helper/test-parse-pathspec-file.c b/t/helper/test-parse-pathspec-file.c
index 02f4ccfd2a..b3e08cef4b 100644
--- a/t/helper/test-parse-pathspec-file.c
+++ b/t/helper/test-parse-pathspec-file.c
@@ -6,7 +6,7 @@
 int cmd__parse_pathspec_file(int argc, const char **argv)
 {
 	struct pathspec pathspec;
-	const char *pathspec_from_file = 0;
+	const char *pathspec_from_file = NULL;
 	int pathspec_file_nul = 0, i;
 
 	static const char *const usage[] = {
@@ -20,9 +20,9 @@ int cmd__parse_pathspec_file(int argc, const char **argv)
 		OPT_END()
 	};
 
-	parse_options(argc, argv, 0, options, usage, 0);
+	parse_options(argc, argv, NULL, options, usage, 0);
 
-	parse_pathspec_file(&pathspec, 0, 0, 0, pathspec_from_file,
+	parse_pathspec_file(&pathspec, 0, 0, NULL, pathspec_from_file,
 			    pathspec_file_nul);
 
 	for (i = 0; i < pathspec.nr; i++)
-- 
2.26.2.384.g435bf60bd5

[PATCH v2 2/4] compat/regex: include alloca.h before undef it

[Đoàn Trần Công Danh]

Somewhere later in the code, we indirectly include alloca.h
which will define alloca again, thus create a warning about
redefinition of a preprocessor.

Include it prior to define alloca in order to not define it again.

Signed-off-by: Đoàn Trần Công Danh <congdanhqx@gmail.com>
---
 compat/regex/regex.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/compat/regex/regex.c b/compat/regex/regex.c
index f3e03a9eab..4bef75a716 100644
--- a/compat/regex/regex.c
+++ b/compat/regex/regex.c
@@ -62,6 +62,7 @@
 #include <stdint.h>
 
 #ifdef GAWK
+#include <alloca.h>
 #undef alloca
 #define alloca alloca_is_bad_you_should_never_use_it
 #endif
-- 
2.26.2.384.g435bf60bd5

[PATCH v2 3/4] graph.c: limit linkage of internal variable

[Đoàn Trần Công Danh]

Signed-off-by: Đoàn Trần Công Danh <congdanhqx@gmail.com>
---
 graph.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/graph.c b/graph.c
index 4fb25ad464..4cd9915075 100644
--- a/graph.c
+++ b/graph.c
@@ -1055,7 +1055,7 @@ static void graph_output_commit_line(struct git_graph *graph, struct graph_line
 		graph_update_state(graph, GRAPH_COLLAPSING);
 }
 
-const char merge_chars[] = {'/', '|', '\\'};
+static const char merge_chars[] = {'/', '|', '\\'};
 
 static void graph_output_post_merge_line(struct git_graph *graph, struct graph_line *line)
 {
-- 
2.26.2.384.g435bf60bd5

[PATCH v2 4/4] progress.c: silence cgcc suggestion about internal linkage

[Đoàn Trần Công Danh]

Signed-off-by: Đoàn Trần Công Danh <congdanhqx@gmail.com>
---
 progress.c               | 2 +-
 progress.h               | 8 ++++++++
 t/helper/test-progress.c | 9 +--------
 3 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/progress.c b/progress.c
index 19805ac646..75633e9c5e 100644
--- a/progress.c
+++ b/progress.c
@@ -8,6 +8,7 @@
  * published by the Free Software Foundation.
  */
 
+#define GIT_TEST_PROGRESS_ONLY
 #include "cache.h"
 #include "gettext.h"
 #include "progress.h"
@@ -52,7 +53,6 @@ static volatile sig_atomic_t progress_update;
  */
 int progress_testing;
 uint64_t progress_test_ns = 0;
-void progress_test_force_update(void); /* To silence -Wmissing-prototypes */
 void progress_test_force_update(void)
 {
 	progress_update = 1;
diff --git a/progress.h b/progress.h
index 847338911f..f1913acf73 100644
--- a/progress.h
+++ b/progress.h
@@ -3,6 +3,14 @@
 
 struct progress;
 
+#ifdef GIT_TEST_PROGRESS_ONLY
+
+extern int progress_testing;
+extern uint64_t progress_test_ns;
+void progress_test_force_update(void);
+
+#endif
+
 void display_throughput(struct progress *progress, uint64_t total);
 void display_progress(struct progress *progress, uint64_t n);
 struct progress *start_progress(const char *title, uint64_t total);
diff --git a/t/helper/test-progress.c b/t/helper/test-progress.c
index 42b96cb103..5d05cbe789 100644
--- a/t/helper/test-progress.c
+++ b/t/helper/test-progress.c
@@ -13,20 +13,13 @@
  *
  * See 't0500-progress-display.sh' for examples.
  */
+#define GIT_TEST_PROGRESS_ONLY
 #include "test-tool.h"
 #include "gettext.h"
 #include "parse-options.h"
 #include "progress.h"
 #include "strbuf.h"
 
-/*
- * These are defined in 'progress.c', but are not exposed in 'progress.h',
- * because they are exclusively for testing.
- */
-extern int progress_testing;
-extern uint64_t progress_test_ns;
-void progress_test_force_update(void);
-
 int cmd__progress(int argc, const char **argv)
 {
 	int total = 0;
-- 
2.26.2.384.g435bf60bd5

Re: [PATCH 0/4] fix make sparse warning

[Ramsay Jones]

On 24/04/2020 00:58, Danh Doan wrote:
> On 2020-04-24 00:10:25+0100, Ramsay Jones <ramsay@ramsayjones.plus.com> wrote:
>> On 23/04/2020 14:47, Đoàn Trần Công Danh wrote:
>>> It's happened that I tried to run make check on project that support autoconf.
>>
>> I try to ignore autoconf as much as possible, so I don't know why people
> 
> For some reason, people refer to autotools as autohell
> 
>> who use it regularly seem to expect a 'make check' target (is that anything
>> to do with ./config.status --recheck?).
> 
> Because it's autotools's recommendation.
> https://www.gnu.org/savannah-checkouts/gnu/autoconf/manual/autoconf-2.69/html_node/Basic-Installation.html#Basic-Installation
> https://www.gnu.org/software/automake/manual/automake.html#Standard-Targets

Ah, OK, that explains alot! :D

I have never seen that documented before, so thanks for the reference.
Having said that, most all 'autotools' projects I have seen have had
an 'make test' target and _not_ 'make check'! (Well, thinking about
it, none of those were actual GNU projects - maybe that's the difference).

[snip]
>> For those interested, the 'too much warnings' on the 'pu' branch looks
>> like (for pu@faf094bf11):
>>
>>   $ make sparse >psp-out 2>&1
>>   $ diff nsp-out psp-out
>>   154a155
>>   >     SP refs/reftable-backend.c
>>   406a408
>>   >     SP t/helper/test-proc-receive.c
>>   447a450,511
>>   >     SP reftable/basics.c
>>   > reftable/basics.c:157:6: warning: symbol 'reftable_malloc_ptr' was not declared. Should it be static?
>>   > reftable/basics.c:158:6: warning: symbol 'reftable_realloc_ptr' was not declared. Should it be static?
>>   > reftable/basics.c:159:6: warning: symbol 'reftable_free_ptr' was not declared. Should it be static?

[snip]

>>   >     SP reftable/writer.c
>>   > reftable/writer.c:231:31: warning: Using plain integer as NULL pointer
>>   > reftable/writer.c:299:39: warning: Using plain integer as NULL pointer
>>   > reftable/writer.c:347:47: warning: Using plain integer as NULL pointer
>>   > reftable/writer.c:426:31: warning: Using plain integer as NULL pointer
>>   > reftable/writer.c:468:45: warning: Using plain integer as NULL pointer
>>   > reftable/writer.c:584:11: warning: symbol 'debug' was not declared. Should it be static?
>>   >     SP reftable/zlib-compat.c
>>
>> Also, my static-check perl script says the following symbols are not
>> used outside the file which defines them (so they could be marked static):
> 
> All of them are in reftable, and they're catched by `make sparse`,
> too.

Yeah, I didn't even look at the output. ;-)

However, the output from static-check.pl is usually a super-set
of the those from sparse - because sparse is really only looking
at the declarations (or lack thereof) in one compilation unit.
In contrast, static-check.pl looks at all object files to tie up
definitions with use (so across compilation units). (which is also
why you have to be careful not to leave stale '*.o' files laying
around when switching branches).

ATB,
Ramsay Jones

Re: [PATCH v2 0/4] Fix Sparse Warning

[Ramsay Jones]

On 24/04/2020 16:12, Đoàn Trần Công Danh wrote:
> Change in v2 from v1:
> * = { 0 } is an idiom to zero the structure, it shouldn't be changed
> * change in [4/4] for prototype
> * ignore diffent function prototype

Apart from the regex patch [2/4], these all look good to me.

Thanks!

ATB,
Ramsay Jones

> 
> Đoàn Trần Công Danh (4):
>   test-parse-pathspec-file.c: s/0/NULL/ for pointer type
>   compat/regex: include alloca.h before undef it
>   graph.c: limit linkage of internal variable
>   progress.c: silence cgcc suggestion about internal linkage
> 
>  compat/regex/regex.c                | 1 +
>  graph.c                             | 2 +-
>  progress.c                          | 2 +-
>  progress.h                          | 8 ++++++++
>  t/helper/test-parse-pathspec-file.c | 6 +++---
>  t/helper/test-progress.c            | 9 +--------
>  6 files changed, 15 insertions(+), 13 deletions(-)
> 

Re: [PATCH v2 2/4] compat/regex: include alloca.h before undef it

[Ramsay Jones]

On 24/04/2020 16:12, Đoàn Trần Công Danh wrote:
> Somewhere later in the code, we indirectly include alloca.h
> which will define alloca again, thus create a warning about
> redefinition of a preprocessor.
> 
> Include it prior to define alloca in order to not define it again.

So, on cygwin, this patch is not required. ie. I don't see any sparse
errors/warnings for compat/regex/regex.c.

Since cygwin uses a different c library (new-lib rather than glibc),
I did a quick test on Linux, thus:

  $ sparse --version
  v0.6.1-191-gc51a0382
  $ 
  $ git checkout master
  Switched to branch 'master'
  Your branch is up-to-date with 'origin/master'.
  $ 
  $ make clean
  GIT_VERSION = 2.26.2.266.ge870325ee8
  ...
  $ 
  $ make NO_REGEX=1 sparse >sp-out1 2>&1
  $ 
  $ diff sp-out sp-out1
  0a1,2
  > GIT_VERSION = 2.26.2.266.ge870325ee8
  >     * new build flags
  12a15
  >     * new prefix flags
  72a76
  >     GEN command-list.h
  226a231
  >     SP compat/regex/regex.c
  $ 
  $ make V=1 NO_REGEX=1 compat/regex/regex.sp
  cgcc -no-compile -Werror -Wall -Wdeclaration-after-statement -Wformat-security -Wold-style-definition -Woverflow -Wpointer-arith -Wstrict-prototypes -Wunused -Wvla -DENABLE_SHA256 -Wextra -Wmissing-prototypes -Wno-empty-body -Wno-missing-field-initializers -Wno-sign-compare -Wno-unused-parameter  -g -O2 -Wall -I. -DHAVE_SYSINFO -DGIT_HOST_CPU="\"x86_64\"" -DHAVE_ALLOCA_H -DUSE_CURL_FOR_IMAP_SEND -DSHA1_DC -DSHA1DC_NO_STANDARD_INCLUDES -DSHA1DC_INIT_SAFE_HASH_DEFAULT=0 -DSHA1DC_CUSTOM_INCLUDE_SHA1_C="\"cache.h\"" -DSHA1DC_CUSTOM_INCLUDE_UBC_CHECK_C="\"git-compat-util.h\"" -DSHA256_BLK  -DHAVE_PATHS_H -DHAVE_DEV_TTY -DHAVE_CLOCK_GETTIME -DHAVE_CLOCK_MONOTONIC -DHAVE_GETDELIM '-DPROCFS_EXECUTABLE_PATH="/proc/self/exe"'  -DFREAD_READS_DIRECTORIES -DNO_STRLCPY -Icompat/regex -DSHELL_PATH='"/bin/sh"' -DPAGER_ENV='"LESS=FRX LV=-c"' -DGAWK -DNO_MBSUPPORT \
  	  compat/regex/regex.c
  $ 
   
So, again I don't see a problem. I guess it is possible that the
version of sparse I am using (see above) has _also_ fixed this
problem, in addition to the prototype attribute placement fix.

Another option is that the version of glibc also matters. (I am
on Linux Mint, which is based on Ubuntu 18.04 LTS) It would not
be the first time that I have seen errors in system header files
change from one release to the next ...

[Hmm, I have a fedora 31 system I could try - much more up-to-date! :D ]

ATB,
Ramsay Jones

> Signed-off-by: Đoàn Trần Công Danh <congdanhqx@gmail.com>
> ---
>  compat/regex/regex.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/compat/regex/regex.c b/compat/regex/regex.c
> index f3e03a9eab..4bef75a716 100644
> --- a/compat/regex/regex.c
> +++ b/compat/regex/regex.c
> @@ -62,6 +62,7 @@
>  #include <stdint.h>
>  
>  #ifdef GAWK
> +#include <alloca.h>
>  #undef alloca
>  #define alloca alloca_is_bad_you_should_never_use_it
>  #endif
> 

Re: [PATCH v2 2/4] compat/regex: include alloca.h before undef it

[Danh Doan]

On 2020-04-24 17:56:46+0100, Ramsay Jones <ramsay@ramsayjones.plus.com> wrote:
> 
> 
> On 24/04/2020 16:12, Đoàn Trần Công Danh wrote:
> > Somewhere later in the code, we indirectly include alloca.h
> > which will define alloca again, thus create a warning about
> > redefinition of a preprocessor.
> > 
> > Include it prior to define alloca in order to not define it again.
> 
> So, on cygwin, this patch is not required. ie. I don't see any sparse
> errors/warnings for compat/regex/regex.c.
> 
> Since cygwin uses a different c library (new-lib rather than glibc),
> I did a quick test on Linux, thus:
> 
>   $ sparse --version
>   v0.6.1-191-gc51a0382
>   $ 
>   $ git checkout master
>   Switched to branch 'master'
>   Your branch is up-to-date with 'origin/master'.
>   $ 
>   $ make clean
>   GIT_VERSION = 2.26.2.266.ge870325ee8
>   ...
>   $ 
>   $ make NO_REGEX=1 sparse >sp-out1 2>&1
>   $ 
>   $ diff sp-out sp-out1
>   0a1,2
>   > GIT_VERSION = 2.26.2.266.ge870325ee8
>   >     * new build flags
>   12a15
>   >     * new prefix flags
>   72a76
>   >     GEN command-list.h
>   226a231
>   >     SP compat/regex/regex.c
>   $ 
>   $ make V=1 NO_REGEX=1 compat/regex/regex.sp
>   cgcc -no-compile -Werror -Wall -Wdeclaration-after-statement -Wformat-security -Wold-style-definition -Woverflow -Wpointer-arith -Wstrict-prototypes -Wunused -Wvla -DENABLE_SHA256 -Wextra -Wmissing-prototypes -Wno-empty-body -Wno-missing-field-initializers -Wno-sign-compare -Wno-unused-parameter  -g -O2 -Wall -I. -DHAVE_SYSINFO -DGIT_HOST_CPU="\"x86_64\"" -DHAVE_ALLOCA_H -DUSE_CURL_FOR_IMAP_SEND -DSHA1_DC -DSHA1DC_NO_STANDARD_INCLUDES -DSHA1DC_INIT_SAFE_HASH_DEFAULT=0 -DSHA1DC_CUSTOM_INCLUDE_SHA1_C="\"cache.h\"" -DSHA1DC_CUSTOM_INCLUDE_UBC_CHECK_C="\"git-compat-util.h\"" -DSHA256_BLK  -DHAVE_PATHS_H -DHAVE_DEV_TTY -DHAVE_CLOCK_GETTIME -DHAVE_CLOCK_MONOTONIC -DHAVE_GETDELIM '-DPROCFS_EXECUTABLE_PATH="/proc/self/exe"'  -DFREAD_READS_DIRECTORIES -DNO_STRLCPY -Icompat/regex -DSHELL_PATH='"/bin/sh"' -DPAGER_ENV='"LESS=FRX LV=-c"' -DGAWK -DNO_MBSUPPORT \
>   	  compat/regex/regex.c
>   $ 
>    
> So, again I don't see a problem. I guess it is possible that the
> version of sparse I am using (see above) has _also_ fixed this
> problem, in addition to the prototype attribute placement fix.
> 
> Another option is that the version of glibc also matters. (I am
> on Linux Mint, which is based on Ubuntu 18.04 LTS) It would not
> be the first time that I have seen errors in system header files
> change from one release to the next ...

I'm using a Linux distro with musl libc.

I guess it's the main culprit?
I have another box with glibc, but it's mostly in Windows 10,
because my sister is its main user.

I'll take a look if it make that warning when my sister agree to leave
that box to me.

-- 
Danh

Re: [PATCH v2 2/4] compat/regex: include alloca.h before undef it

[Ramsay Jones]

On 24/04/2020 18:09, Danh Doan wrote:
> On 2020-04-24 17:56:46+0100, Ramsay Jones <ramsay@ramsayjones.plus.com> wrote:
[snip]

>> So, again I don't see a problem. I guess it is possible that the
>> version of sparse I am using (see above) has _also_ fixed this
>> problem, in addition to the prototype attribute placement fix.
>>
>> Another option is that the version of glibc also matters. (I am
>> on Linux Mint, which is based on Ubuntu 18.04 LTS) It would not
>> be the first time that I have seen errors in system header files
>> change from one release to the next ...
> 
> I'm using a Linux distro with musl libc.

Ah, OK.

I just tried re-building v0.6.1 to see if any '<alloca.h>' related
errors/warnings show up for me, but they don't:

  $ sparse --version
  v0.6.1
  $ 
  $ git checkout master
  Switched to branch 'master'
  Your branch is up-to-date with 'origin/master'.
  $ 
  $ make clean
  GIT_VERSION = 2.26.2.266.ge870325ee8
  ...
  $ 
  $ make NO_REGEX=1 compat/regex/regex.sp
      SP compat/regex/regex.c
  compat/regex/regex_internal.c:925:1: error: symbol 're_string_context_at' redeclared with different type (originally declared at compat/regex/regex_internal.h:433) - different modifiers
  $ 

> I guess it's the main culprit?

Quite possible, I guess. What do the errors/warnings look like?

> I have another box with glibc, but it's mostly in Windows 10,
> because my sister is its main user.
> 
> I'll take a look if it make that warning when my sister agree to leave
> that box to me.

Sounds good.

ATB,
Ramsay Jones

Re: [PATCH v2 2/4] compat/regex: include alloca.h before undef it

[Danh Doan]

On 2020-04-24 19:29:31+0100, Ramsay Jones <ramsay@ramsayjones.plus.com> wrote:
> 
> 
> On 24/04/2020 18:09, Danh Doan wrote:
> > On 2020-04-24 17:56:46+0100, Ramsay Jones <ramsay@ramsayjones.plus.com> wrote:
> [snip]
> 
> >> So, again I don't see a problem. I guess it is possible that the
> >> version of sparse I am using (see above) has _also_ fixed this
> >> problem, in addition to the prototype attribute placement fix.
> >>
> >> Another option is that the version of glibc also matters. (I am
> >> on Linux Mint, which is based on Ubuntu 18.04 LTS) It would not
> >> be the first time that I have seen errors in system header files
> >> change from one release to the next ...
> > 
> > I'm using a Linux distro with musl libc.
> 
> Ah, OK.
> 
> I just tried re-building v0.6.1 to see if any '<alloca.h>' related
> errors/warnings show up for me, but they don't:
> 
>   $ sparse --version
>   v0.6.1
>   $ 
>   $ git checkout master
>   Switched to branch 'master'
>   Your branch is up-to-date with 'origin/master'.
>   $ 
>   $ make clean
>   GIT_VERSION = 2.26.2.266.ge870325ee8
>   ...
>   $ 
>   $ make NO_REGEX=1 compat/regex/regex.sp
>       SP compat/regex/regex.c
>   compat/regex/regex_internal.c:925:1: error: symbol 're_string_context_at' redeclared with different type (originally declared at compat/regex/regex_internal.h:433) - different modifiers
>   $ 
> 
> > I guess it's the main culprit?
> 
> Quite possible, I guess. What do the errors/warnings look like?

OK, I've tried with my glibc box, it doesn't have that warning.
On musl, it warns:

	$ make compat/regex/regex.sp
	GIT_VERSION = 2.26.2
	    * new build flags
	    SP compat/regex/regex.c
	/usr/include/alloca.h:14:9: warning: preprocessor token alloca redefined
	compat/regex/regex.c:66:9: this was the original definition
	compat/regex/regex_internal.c:925:1: error: symbol 're_string_context_at' redeclared with different type (originally declared at compat/regex/regex_internal.h:433) - different modifiers


-- 
Danh

Re: [PATCH 0/4] fix make sparse warning

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 778 bytes --]

Hi Danh,

On Thu, 23 Apr 2020, Đoàn Trần Công Danh wrote:

> It's happened that I tried to run make check on project that support autoconf.
> Git's Makefile told me to run `make sparse` instead.
>
> I /think/ if we have a rule in Makefile, we should adhere to it.
> I also fix another change in ds/blame-on-bloom, which I think it's worth to
> fix, see: <20200423133937.GA1984@danh.dev>

FWIW there is still an Azure Pipeline building `sparse` for Ubuntu every
two weeks: https://dev.azure.com/git/git/_build?definitionId=10&_a=summary

I created this Pipeline in order to support the proposed project at
https://github.com/gitgitgadget/git/issues/345 which is: teach our CI
builds to run `make sparse`.

Maybe it is time to tackle that?

Ciao,
Dscho

Re: [PATCH v2 2/4] compat/regex: include alloca.h before undef it

[Ramsay Jones]

On 24/04/2020 23:34, Danh Doan wrote:
[snip]

> OK, I've tried with my glibc box, it doesn't have that warning.
> On musl, it warns:
> 
> 	$ make compat/regex/regex.sp
> 	GIT_VERSION = 2.26.2
> 	    * new build flags
> 	    SP compat/regex/regex.c
> 	/usr/include/alloca.h:14:9: warning: preprocessor token alloca redefined
> 	compat/regex/regex.c:66:9: this was the original definition
> 	compat/regex/regex_internal.c:925:1: error: symbol 're_string_context_at' redeclared with different type (originally declared at compat/regex/regex_internal.h:433) - different modifiers
> 
> 

OK, I had a quick look at the <alloca.h> header file on a glibc
system (linux) and new-lib system (cygwin) and they both do
(more or less) the same thing: first #undef alloca, and then
if being compiled by gcc, define alloca(size) to be __builtin_alloca(size).

So, even if <alloca.h> is #included after regex.c:66, it wouldn't
be a problem. Since I don't have access to a musl based system,
I don't know what that system header is doing.

However, I said *even if* above, because I don't see why it is trying
to #include <alloca.h> in the first place! ;-)

Note that the three calls to alloca in compat/regex:

  $ git grep -n '\<alloca\>' -- compat/regex
  compat/regex/regex.c:65:#undef alloca
  compat/regex/regex.c:66:#define alloca alloca_is_bad_you_should_never_use_it
  compat/regex/regex_internal.h:460:#   include <alloca.h>
  compat/regex/regex_internal.h:468:/* alloca is implemented with malloc, so just use malloc.  */
  compat/regex/regexec.c:1428:    prev_idx_match = (regmatch_t *) alloca (nmatch * sizeof (regmatch_t));
  compat/regex/regexec.c:3338:    dests_alloc = (struct dests_alloc *) alloca (sizeof (struct dests_alloc));
  compat/regex/regexec.c:3385:      alloca (ndests * 3 * sizeof (re_dfastate_t *));
  $ 

... in compat/regex/regexec.c are all protected by '#ifdef HAVE_ALLOCA', as
indeed is the #include of the header file in compat/regex/regex_internal.h
at line 460. Since HAVE_ALLOCA should not be defined, where is that file
being included?

Note that HAVE_ALLOCA
 
  $ git grep -n HAVE_ALLOCA -- compat
  compat/regex/regex_internal.h:455:# if HAVE_ALLOCA
  compat/regex/regexec.c:1426:#ifdef HAVE_ALLOCA
  compat/regex/regexec.c:3336:#ifdef HAVE_ALLOCA
  compat/regex/regexec.c:3381:#ifdef HAVE_ALLOCA
  $ 

... is not the same as HAVE_ALLOCA_H

  $ git grep -n HAVE_ALLOCA
  Makefile:45:# Define HAVE_ALLOCA_H if you have working alloca(3) defined in that header.
  Makefile:1349:ifdef HAVE_ALLOCA_H
  Makefile:1350:  BASIC_CFLAGS += -DHAVE_ALLOCA_H
  compat/regex/regex_internal.h:455:# if HAVE_ALLOCA
  compat/regex/regexec.c:1426:#ifdef HAVE_ALLOCA
  compat/regex/regexec.c:3336:#ifdef HAVE_ALLOCA
  compat/regex/regexec.c:3381:#ifdef HAVE_ALLOCA
  config.mak.uname:47:    HAVE_ALLOCA_H = YesPlease
  config.mak.uname:63:    HAVE_ALLOCA_H = YesPlease
  config.mak.uname:147:   HAVE_ALLOCA_H = YesPlease
  config.mak.uname:206:   HAVE_ALLOCA_H = YesPlease
  config.mak.uname:310:   HAVE_ALLOCA_H = YesPlease
  config.mak.uname:395:   HAVE_ALLOCA_H = YesPlease
  config.mak.uname:586:   HAVE_ALLOCA_H = YesPlease
  configure.ac:320:# Define HAVE_ALLOCA_H if you have working alloca(3) defined in that header.
  configure.ac:323:    yes)    HAVE_ALLOCA_H=YesPlease;;
  configure.ac:324:    *)      HAVE_ALLOCA_H='';;
  configure.ac:326:GIT_CONF_SUBST([HAVE_ALLOCA_H])
  git-compat-util.h:840:#ifdef HAVE_ALLOCA_H
  $ 

... used by the rest of git, outside of the compat/regex directory.

Once again, you can see that HAVE_ALLOCA

  $ make V=1 NO_REGEX=1 compat/regex/regex.sp
  cgcc -no-compile -Werror -Wall -Wdeclaration-after-statement -Wformat-security -Wold-style-definition -Woverflow -Wpointer-arith -Wstrict-prototypes -Wunused -Wvla -DENABLE_SHA256 -Wextra -Wmissing-prototypes -Wno-empty-body -Wno-missing-field-initializers -Wno-sign-compare -Wno-unused-parameter  -g -O2 -Wall -I. -DHAVE_SYSINFO -DGIT_HOST_CPU="\"x86_64\"" -DHAVE_ALLOCA_H  -DUSE_CURL_FOR_IMAP_SEND -DSHA1_DC -DSHA1DC_NO_STANDARD_INCLUDES -DSHA1DC_INIT_SAFE_HASH_DEFAULT=0 -DSHA1DC_CUSTOM_INCLUDE_SHA1_C="\"cache.h\"" -DSHA1DC_CUSTOM_INCLUDE_UBC_CHECK_C="\"git-compat-util.h\"" -DSHA256_BLK  -DHAVE_PATHS_H -DHAVE_DEV_TTY -DHAVE_CLOCK_GETTIME -DHAVE_CLOCK_MONOTONIC -DHAVE_GETDELIM '-DPROCFS_EXECUTABLE_PATH="/proc/self/exe"'  -DFREAD_READS_DIRECTORIES -DNO_STRLCPY -Icompat/regex -DSHELL_PATH='"/bin/sh"' -DPAGER_ENV='"LESS=FRX LV=-c"' -DGAWK -DNO_MBSUPPORT \
  	  compat/regex/regex.c
  $ 
  
... is not being set on the command-line.

Hmm, do you have this set in config.mak, config.mak.autogen, or some other
source? puzzled! ;-)

BTW, why are you compiling with NO_REGEX set anyway?


ATB,
Ramsay Jones

Re: [PATCH v2 2/4] compat/regex: include alloca.h before undef it

[Danh Doan]

On 2020-04-25 21:28:05+0100, Ramsay Jones <ramsay@ramsayjones.plus.com> wrote:
> 
> 
> On 24/04/2020 23:34, Danh Doan wrote:
> [snip]
> 
> > OK, I've tried with my glibc box, it doesn't have that warning.
> > On musl, it warns:
> > 
> > 	$ make compat/regex/regex.sp
> > 	GIT_VERSION = 2.26.2
> > 	    * new build flags
> > 	    SP compat/regex/regex.c
> > 	/usr/include/alloca.h:14:9: warning: preprocessor token alloca redefined
> > 	compat/regex/regex.c:66:9: this was the original definition
> > 	compat/regex/regex_internal.c:925:1: error: symbol 're_string_context_at' redeclared with different type (originally declared at compat/regex/regex_internal.h:433) - different modifiers
> > 
> > 
> 
> OK, I had a quick look at the <alloca.h> header file on a glibc
> system (linux) and new-lib system (cygwin) and they both do
> (more or less) the same thing: first #undef alloca, and then
> if being compiled by gcc, define alloca(size) to be __builtin_alloca(size).

musl people don't do that.
They just go ahead define it, if any other header file requires
alloca, they will include alloca.h

> So, even if <alloca.h> is #included after regex.c:66, it wouldn't
> be a problem. Since I don't have access to a musl based system,
> I don't know what that system header is doing.

musl's alloca.h is available here:

https://git.musl-libc.org/cgit/musl/tree/include/alloca.h

> However, I said *even if* above, because I don't see why it is trying
> to #include <alloca.h> in the first place! ;-)

I looked into my system again. The inclusion chain is:

compat/regex/regex.c:77
`-> compat/regex/regex_internal.h:26
    `-> /usr/include/stdlib.h:138 [*1*]

[*1*]: https://git.musl-libc.org/cgit/musl/tree/include/stdlib.h#n137

I don't know why _GNU_SOURCE and/or _BSD_SOURCE is defined.

>   $ make V=1 NO_REGEX=1 compat/regex/regex.sp
>   cgcc -no-compile -Werror -Wall -Wdeclaration-after-statement -Wformat-security -Wold-style-definition -Woverflow -Wpointer-arith -Wstrict-prototypes -Wunused -Wvla -DENABLE_SHA256 -Wextra -Wmissing-prototypes -Wno-empty-body -Wno-missing-field-initializers -Wno-sign-compare -Wno-unused-parameter  -g -O2 -Wall -I. -DHAVE_SYSINFO -DGIT_HOST_CPU="\"x86_64\"" -DHAVE_ALLOCA_H  -DUSE_CURL_FOR_IMAP_SEND -DSHA1_DC -DSHA1DC_NO_STANDARD_INCLUDES -DSHA1DC_INIT_SAFE_HASH_DEFAULT=0 -DSHA1DC_CUSTOM_INCLUDE_SHA1_C="\"cache.h\"" -DSHA1DC_CUSTOM_INCLUDE_UBC_CHECK_C="\"git-compat-util.h\"" -DSHA256_BLK  -DHAVE_PATHS_H -DHAVE_DEV_TTY -DHAVE_CLOCK_GETTIME -DHAVE_CLOCK_MONOTONIC -DHAVE_GETDELIM '-DPROCFS_EXECUTABLE_PATH="/proc/self/exe"'  -DFREAD_READS_DIRECTORIES -DNO_STRLCPY -Icompat/regex -DSHELL_PATH='"/bin/sh"' -DPAGER_ENV='"LESS=FRX LV=-c"' -DGAWK -DNO_MBSUPPORT \
>   	  compat/regex/regex.c
>   $ 
>   
> ... is not being set on the command-line.

Here's the invocation of cc and cgcc:

	$ make V=1  compat/regex/regex.o
	cc -o compat/regex/regex.o -c -MF compat/regex/.depend/regex.o.d -MQ compat/regex/regex.o -MMD -MP -Werror -Wall -Wdeclaration-after-statement -Wformat-security -Wold-style-definition -Woverflow -Wpointer-arith -Wstrict-prototypes -Wunused -Wvla -DENABLE_SHA256 -Wextra -Wmissing-prototypes -Wno-empty-body -Wno-missing-field-initializers -Wno-sign-compare -Wno-unused-parameter  -g -O2 -Wall -I. -DHAVE_SYSINFO -DGIT_HOST_CPU="\"x86_64\"" -DUSE_LIBPCRE2 -DHAVE_ALLOCA_H -DUSE_CURL_FOR_IMAP_SEND -DICONV_OMITS_BOM -DSHA1_DC -DSHA1DC_NO_STANDARD_INCLUDES -DSHA1DC_INIT_SAFE_HASH_DEFAULT=0 -DSHA1DC_CUSTOM_INCLUDE_SHA1_C="\"cache.h\"" -DSHA1DC_CUSTOM_INCLUDE_UBC_CHECK_C="\"git-compat-util.h\"" -DSHA256_BLK  -DHAVE_PATHS_H -DHAVE_DEV_TTY -DHAVE_CLOCK_GETTIME -DHAVE_CLOCK_MONOTONIC -DHAVE_GETDELIM '-DPROCFS_EXECUTABLE_PATH="/proc/self/exe"' -DFREAD_READS_DIRECTORIES -DNO_STRLCPY -Icompat/regex -DSHELL_PATH='"/bin/sh"' -DPAGER_ENV='"LESS=FRX LV=-c"' -DGAWK -DNO_MBSUPPORT compat/regex/regex.c
	$ make V=1  compat/regex/regex.sp
	cgcc -no-compile -Werror -Wall -Wdeclaration-after-statement -Wformat-security -Wold-style-definition -Woverflow -Wpointer-arith -Wstrict-prototypes -Wunused -Wvla -DENABLE_SHA256 -Wextra -Wmissing-prototypes -Wno-empty-body -Wno-missing-field-initializers -Wno-sign-compare -Wno-unused-parameter  -g -O2 -Wall -I. -DHAVE_SYSINFO -DGIT_HOST_CPU="\"x86_64\"" -DUSE_LIBPCRE2 -DHAVE_ALLOCA_H -DUSE_CURL_FOR_IMAP_SEND -DICONV_OMITS_BOM -DSHA1_DC -DSHA1DC_NO_STANDARD_INCLUDES -DSHA1DC_INIT_SAFE_HASH_DEFAULT=0 -DSHA1DC_CUSTOM_INCLUDE_SHA1_C="\"cache.h\"" -DSHA1DC_CUSTOM_INCLUDE_UBC_CHECK_C="\"git-compat-util.h\"" -DSHA256_BLK  -DHAVE_PATHS_H -DHAVE_DEV_TTY -DHAVE_CLOCK_GETTIME -DHAVE_CLOCK_MONOTONIC -DHAVE_GETDELIM '-DPROCFS_EXECUTABLE_PATH="/proc/self/exe"' -DFREAD_READS_DIRECTORIES -DNO_STRLCPY -Icompat/regex -DSHELL_PATH='"/bin/sh"' -DPAGER_ENV='"LESS=FRX LV=-c"' -DGAWK -DNO_MBSUPPORT \
	          compat/regex/regex.c

> Hmm, do you have this set in config.mak, config.mak.autogen, or some other
> source? puzzled! ;-)

I don't have `config.make.autogen`,
Here is config.mak

	$ cat config.mak
	USE_ASCIIDOCTOR=Yes
	DEVELOPER=1
	DEFAULT_TEST_TARGET=prove
	prefix = /home/danh/.local
	USE_LIBPCRE2=YesPlease
	ICONV_OMITS_BOM=Yes
	NO_REGEX=YesPlease

> BTW, why are you compiling with NO_REGEX set anyway?

Because I use musl-libc, and musl-libc doesn't have StartEnd

-- 
Danh

Re: [PATCH v2 2/4] compat/regex: include alloca.h before undef it

[Danh Doan]

On 2020-04-26 07:54:51+0700, Danh Doan <congdanhqx@gmail.com> wrote:
> On 2020-04-25 21:28:05+0100, Ramsay Jones <ramsay@ramsayjones.plus.com> wrote:
> > 
> > 
> > On 24/04/2020 23:34, Danh Doan wrote:
> > [snip]
> > 
> > > OK, I've tried with my glibc box, it doesn't have that warning.
> > > On musl, it warns:
> > > 
> > > 	$ make compat/regex/regex.sp
> > > 	GIT_VERSION = 2.26.2
> > > 	    * new build flags
> > > 	    SP compat/regex/regex.c
> > > 	/usr/include/alloca.h:14:9: warning: preprocessor token alloca redefined
> > > 	compat/regex/regex.c:66:9: this was the original definition
> > > 	compat/regex/regex_internal.c:925:1: error: symbol 're_string_context_at' redeclared with different type (originally declared at compat/regex/regex_internal.h:433) - different modifiers
> > > 
> > > 
> > 
> > OK, I had a quick look at the <alloca.h> header file on a glibc
> > system (linux) and new-lib system (cygwin) and they both do
> > (more or less) the same thing: first #undef alloca, and then
> > if being compiled by gcc, define alloca(size) to be __builtin_alloca(size).
> 
> musl people don't do that.
> They just go ahead define it, if any other header file requires
> alloca, they will include alloca.h
> 
> > So, even if <alloca.h> is #included after regex.c:66, it wouldn't
> > be a problem. Since I don't have access to a musl based system,
> > I don't know what that system header is doing.
> 
> musl's alloca.h is available here:
> 
> https://git.musl-libc.org/cgit/musl/tree/include/alloca.h
> 
> > However, I said *even if* above, because I don't see why it is trying
> > to #include <alloca.h> in the first place! ;-)
> 
> I looked into my system again. The inclusion chain is:
> 
> compat/regex/regex.c:77
> `-> compat/regex/regex_internal.h:26
>     `-> /usr/include/stdlib.h:138 [*1*]
>         `-> /usr/include/alloca.h
> 
> [*1*]: https://git.musl-libc.org/cgit/musl/tree/include/stdlib.h#n137

Sorry for the noise, but I should link to a specific tree instead of
their master for future refererence.

https://git.musl-libc.org/cgit/musl/tree/include/stdlib.h?id=8e452abae67db445fb6c3e37cd566c4788c2e8f3#n137

> 
> I don't know why _GNU_SOURCE and/or _BSD_SOURCE is defined.
> 
> >   $ make V=1 NO_REGEX=1 compat/regex/regex.sp
> >   cgcc -no-compile -Werror -Wall -Wdeclaration-after-statement -Wformat-security -Wold-style-definition -Woverflow -Wpointer-arith -Wstrict-prototypes -Wunused -Wvla -DENABLE_SHA256 -Wextra -Wmissing-prototypes -Wno-empty-body -Wno-missing-field-initializers -Wno-sign-compare -Wno-unused-parameter  -g -O2 -Wall -I. -DHAVE_SYSINFO -DGIT_HOST_CPU="\"x86_64\"" -DHAVE_ALLOCA_H  -DUSE_CURL_FOR_IMAP_SEND -DSHA1_DC -DSHA1DC_NO_STANDARD_INCLUDES -DSHA1DC_INIT_SAFE_HASH_DEFAULT=0 -DSHA1DC_CUSTOM_INCLUDE_SHA1_C="\"cache.h\"" -DSHA1DC_CUSTOM_INCLUDE_UBC_CHECK_C="\"git-compat-util.h\"" -DSHA256_BLK  -DHAVE_PATHS_H -DHAVE_DEV_TTY -DHAVE_CLOCK_GETTIME -DHAVE_CLOCK_MONOTONIC -DHAVE_GETDELIM '-DPROCFS_EXECUTABLE_PATH="/proc/self/exe"'  -DFREAD_READS_DIRECTORIES -DNO_STRLCPY -Icompat/regex -DSHELL_PATH='"/bin/sh"' -DPAGER_ENV='"LESS=FRX LV=-c"' -DGAWK -DNO_MBSUPPORT \
> >   	  compat/regex/regex.c
> >   $ 
> >   
> > ... is not being set on the command-line.
> 
> Here's the invocation of cc and cgcc:
> 
> 	$ make V=1  compat/regex/regex.o
> 	cc -o compat/regex/regex.o -c -MF compat/regex/.depend/regex.o.d -MQ compat/regex/regex.o -MMD -MP -Werror -Wall -Wdeclaration-after-statement -Wformat-security -Wold-style-definition -Woverflow -Wpointer-arith -Wstrict-prototypes -Wunused -Wvla -DENABLE_SHA256 -Wextra -Wmissing-prototypes -Wno-empty-body -Wno-missing-field-initializers -Wno-sign-compare -Wno-unused-parameter  -g -O2 -Wall -I. -DHAVE_SYSINFO -DGIT_HOST_CPU="\"x86_64\"" -DUSE_LIBPCRE2 -DHAVE_ALLOCA_H -DUSE_CURL_FOR_IMAP_SEND -DICONV_OMITS_BOM -DSHA1_DC -DSHA1DC_NO_STANDARD_INCLUDES -DSHA1DC_INIT_SAFE_HASH_DEFAULT=0 -DSHA1DC_CUSTOM_INCLUDE_SHA1_C="\"cache.h\"" -DSHA1DC_CUSTOM_INCLUDE_UBC_CHECK_C="\"git-compat-util.h\"" -DSHA256_BLK  -DHAVE_PATHS_H -DHAVE_DEV_TTY -DHAVE_CLOCK_GETTIME -DHAVE_CLOCK_MONOTONIC -DHAVE_GETDELIM '-DPROCFS_EXECUTABLE_PATH="/proc/self/exe"' -DFREAD_READS_DIRECTORIES -DNO_STRLCPY -Icompat/regex -DSHELL_PATH='"/bin/sh"' -DPAGER_ENV='"LESS=FRX LV=-c"' -DGAWK -DNO_MBSUPPORT compat/regex/regex.c
> 	$ make V=1  compat/regex/regex.sp
> 	cgcc -no-compile -Werror -Wall -Wdeclaration-after-statement -Wformat-security -Wold-style-definition -Woverflow -Wpointer-arith -Wstrict-prototypes -Wunused -Wvla -DENABLE_SHA256 -Wextra -Wmissing-prototypes -Wno-empty-body -Wno-missing-field-initializers -Wno-sign-compare -Wno-unused-parameter  -g -O2 -Wall -I. -DHAVE_SYSINFO -DGIT_HOST_CPU="\"x86_64\"" -DUSE_LIBPCRE2 -DHAVE_ALLOCA_H -DUSE_CURL_FOR_IMAP_SEND -DICONV_OMITS_BOM -DSHA1_DC -DSHA1DC_NO_STANDARD_INCLUDES -DSHA1DC_INIT_SAFE_HASH_DEFAULT=0 -DSHA1DC_CUSTOM_INCLUDE_SHA1_C="\"cache.h\"" -DSHA1DC_CUSTOM_INCLUDE_UBC_CHECK_C="\"git-compat-util.h\"" -DSHA256_BLK  -DHAVE_PATHS_H -DHAVE_DEV_TTY -DHAVE_CLOCK_GETTIME -DHAVE_CLOCK_MONOTONIC -DHAVE_GETDELIM '-DPROCFS_EXECUTABLE_PATH="/proc/self/exe"' -DFREAD_READS_DIRECTORIES -DNO_STRLCPY -Icompat/regex -DSHELL_PATH='"/bin/sh"' -DPAGER_ENV='"LESS=FRX LV=-c"' -DGAWK -DNO_MBSUPPORT \
> 	          compat/regex/regex.c
> 
> > Hmm, do you have this set in config.mak, config.mak.autogen, or some other
> > source? puzzled! ;-)
> 
> I don't have `config.make.autogen`,
> Here is config.mak
> 
> 	$ cat config.mak
> 	USE_ASCIIDOCTOR=Yes
> 	DEVELOPER=1
> 	DEFAULT_TEST_TARGET=prove
> 	prefix = /home/danh/.local
> 	USE_LIBPCRE2=YesPlease
> 	ICONV_OMITS_BOM=Yes
> 	NO_REGEX=YesPlease
> 
> > BTW, why are you compiling with NO_REGEX set anyway?
> 
> Because I use musl-libc, and musl-libc doesn't have StartEnd
> 
> -- 
> Danh

-- 
Danh

Re: [PATCH 0/4] fix make sparse warning

[Danh Doan]

On 2020-04-25 15:13:53+0200, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> Hi Danh,
> 
> On Thu, 23 Apr 2020, Đoàn Trần Công Danh wrote:
> 
> > It's happened that I tried to run make check on project that support autoconf.
> > Git's Makefile told me to run `make sparse` instead.
> >
> > I /think/ if we have a rule in Makefile, we should adhere to it.
> > I also fix another change in ds/blame-on-bloom, which I think it's worth to
> > fix, see: <20200423133937.GA1984@danh.dev>
> 
> FWIW there is still an Azure Pipeline building `sparse` for Ubuntu every
> two weeks: https://dev.azure.com/git/git/_build?definitionId=10&_a=summary
> 
> I created this Pipeline in order to support the proposed project at
> https://github.com/gitgitgadget/git/issues/345 which is: teach our CI
> builds to run `make sparse`.
> 
> Maybe it is time to tackle that?

I don't think it's ready, yet!

May be it's.

But we'll need

	make sparse |
	grep -v -e 'plain integer as NULL pointer' \
		-e 'redeclared with different type'

The first one for:

	struct foo val = { 0 };

to zero structure.

The second one for different in declaration and definition (which is
fixed in sparse's master).


-- 
Danh

Re: [PATCH v2 2/4] compat/regex: include alloca.h before undef it

[Ramsay Jones]

On 26/04/2020 01:54, Danh Doan wrote:
> On 2020-04-25 21:28:05+0100, Ramsay Jones <ramsay@ramsayjones.plus.com> wrote:
[snip]
>> OK, I had a quick look at the <alloca.h> header file on a glibc
>> system (linux) and new-lib system (cygwin) and they both do
>> (more or less) the same thing: first #undef alloca, and then
>> if being compiled by gcc, define alloca(size) to be __builtin_alloca(size).
> 
> musl people don't do that.
> They just go ahead define it, if any other header file requires
> alloca, they will include alloca.h
> 
>> So, even if <alloca.h> is #included after regex.c:66, it wouldn't
>> be a problem. Since I don't have access to a musl based system,
>> I don't know what that system header is doing.
> 
> musl's alloca.h is available here:
> 
> https://git.musl-libc.org/cgit/musl/tree/include/alloca.h

Hmm, OK, so that partly explains the problem. I wonder if the
musl guys would accept a bug report?

>> However, I said *even if* above, because I don't see why it is trying
>> to #include <alloca.h> in the first place! ;-)
> 
> I looked into my system again. The inclusion chain is:
> 
> compat/regex/regex.c:77
> `-> compat/regex/regex_internal.h:26
>     `-> /usr/include/stdlib.h:138 [*1*]
> 
> [*1*]: https://git.musl-libc.org/cgit/musl/tree/include/stdlib.h#n137
> 
> I don't know why _GNU_SOURCE and/or _BSD_SOURCE is defined.

... and this explains the main cause. Hmm, as you say, why are
one (or both) of those pp variables set. :(

[snip] 
>> BTW, why are you compiling with NO_REGEX set anyway?
> 
> Because I use musl-libc, and musl-libc doesn't have StartEnd

Ah, OK. ;-)

Well, even if the musl project accepted a PR and provided a fix, that
will not help you in the short term! :D

Hmm, whatever patch you decide to send (even the original one) I think
you need to add an explanation of the problem to the commit message,
including why the patch provides a solution. (You don't have to type
a novel - see commit bd8f005583 :-P ).

I haven't thought about this too much, but some options:

  - iff the musl library sets some kind of identifying pp variable
    (_MUSL_LIBC_ or somesuch - I haven't looked), then you could
    make the '#include <alloca.h>' conditional on that variable.
    This has the benefit of making it obvious to people reading the
    code that this is specific to musl-libc.

  - you could simply remove that '#ifdef GAWK' block completely (Lines
    64->67). We set GAWK and NO_MBSUPPORT  unconditionally in the Makefile
    so that it compiles (see commit a997bf423d), but these particular
    lines simply reflect the gawk projects dislike of alloca (along with
    the desire to catch any attempts to add new calls which are not protected
    by HAVE_ALLOCA). Given that we are very unlikely to add new calls ...

  - change the conditional on this block to (totally untested, just typing
    into my email client) '#if defined(GAWK) && defined(HAVE_ALLOCA)'.
    This should work, but it does disable the 'catch any attempts to add
    new _unintentional_ calls' aspect of that block; so you may as well
    remove it ...

Just some 'off the top of my head ideas', ... ;-)

ATB,
Ramsay Jones

Re: [PATCH 0/4] fix make sparse warning

[Ramsay Jones]

On 26/04/2020 04:32, Danh Doan wrote:
> On 2020-04-25 15:13:53+0200, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
>> Hi Danh,
>>
>> On Thu, 23 Apr 2020, Đoàn Trần Công Danh wrote:
>>
>>> It's happened that I tried to run make check on project that support autoconf.
>>> Git's Makefile told me to run `make sparse` instead.
>>>
>>> I /think/ if we have a rule in Makefile, we should adhere to it.
>>> I also fix another change in ds/blame-on-bloom, which I think it's worth to
>>> fix, see: <20200423133937.GA1984@danh.dev>
>>
>> FWIW there is still an Azure Pipeline building `sparse` for Ubuntu every
>> two weeks: https://dev.azure.com/git/git/_build?definitionId=10&_a=summary
>>
>> I created this Pipeline in order to support the proposed project at
>> https://github.com/gitgitgadget/git/issues/345 which is: teach our CI
>> builds to run `make sparse`.
>>
>> Maybe it is time to tackle that?
> 
> I don't think it's ready, yet!

Heh, not too long ago, the 'master' and 'next' branches were 'sparse clean'.
(on non NO_REGEX builds anyway, and even on those for some of us. ;-) )
The 'pu' branch was frequently 'unclean', of course.

ATB,
Ramsay Jones

> 
> May be it's.
> 
> But we'll need
> 
> 	make sparse |
> 	grep -v -e 'plain integer as NULL pointer' \
> 		-e 'redeclared with different type'
> 
> The first one for:
> 
> 	struct foo val = { 0 };
> 
> to zero structure.
> 
> The second one for different in declaration and definition (which is
> fixed in sparse's master).
> 
> 

Re: [PATCH v2 2/4] compat/regex: include alloca.h before undef it

[Ramsay Jones]

On 26/04/2020 17:17, Ramsay Jones wrote:
[snip]

> Hmm, whatever patch you decide to send (even the original one) I think
> you need to add an explanation of the problem to the commit message,
> including why the patch provides a solution. (You don't have to type
> a novel - see commit bd8f005583 :-P ).
> 
> I haven't thought about this too much, but some options:
> 
>   - iff the musl library sets some kind of identifying pp variable
>     (_MUSL_LIBC_ or somesuch - I haven't looked), then you could
>     make the '#include <alloca.h>' conditional on that variable.
>     This has the benefit of making it obvious to people reading the
>     code that this is specific to musl-libc.
> 
>   - you could simply remove that '#ifdef GAWK' block completely (Lines
>     64->67). We set GAWK and NO_MBSUPPORT  unconditionally in the Makefile
>     so that it compiles (see commit a997bf423d), but these particular
>     lines simply reflect the gawk projects dislike of alloca (along with
>     the desire to catch any attempts to add new calls which are not protected
>     by HAVE_ALLOCA). Given that we are very unlikely to add new calls ...
> 
>   - change the conditional on this block to (totally untested, just typing
>     into my email client) '#if defined(GAWK) && defined(HAVE_ALLOCA)'.
>     This should work, but it does disable the 'catch any attempts to add
>     new _unintentional_ calls' aspect of that block; so you may as well
>     remove it ...
> 
> Just some 'off the top of my head ideas', ... ;-)

Another option I thought about, but didn't mention above, is given by
the patch below. I didn't mention it because it has the potential to
cause problems on non musl-libc systems (and I was feeling too lazy at
the time to go and test ...). Again, see commit bd8f005583.

So, I have now tested this patch on (glibc) linux and it seems to work
just fine; compile, sparse, test-suite passed. It also compiles clean
(and sparse clean) on cygwin (new-lib) - I didn't run the test-suite
on cygwin, since it takes about 3.5 to 4 hours to run.

I don't have any other systems to test this on, so I can't say that it
won't cause problems somewhere. In practice, I think the chances of that
are rather low, but don't quote me on that! :-P

ATB,
Ramsay Jones

-- >8 --
Subject: [PATCH] regex: fix up musl-libc builds

---
 compat/regex/regex.c          | 1 +
 compat/regex/regex_internal.h | 1 -
 2 files changed, 1 insertion(+), 1 deletion(-)

diff --git a/compat/regex/regex.c b/compat/regex/regex.c
index f3e03a9eab..e6f4a5d177 100644
--- a/compat/regex/regex.c
+++ b/compat/regex/regex.c
@@ -60,6 +60,7 @@
    #undefs RE_DUP_MAX and sets it to the right value.  */
 #include <limits.h>
 #include <stdint.h>
+#include <stdlib.h>
 
 #ifdef GAWK
 #undef alloca
diff --git a/compat/regex/regex_internal.h b/compat/regex/regex_internal.h
index 3ee8aae59d..0bad8b841e 100644
--- a/compat/regex/regex_internal.h
+++ b/compat/regex/regex_internal.h
@@ -23,7 +23,6 @@
 #include <assert.h>
 #include <ctype.h>
 #include <stdio.h>
-#include <stdlib.h>
 #include <string.h>
 
 #if defined HAVE_LANGINFO_H || defined HAVE_LANGINFO_CODESET || defined _LIBC
-- 
2.26.2

Re: [PATCH v2 2/4] compat/regex: include alloca.h before undef it

[Junio C Hamano]

Ramsay Jones <ramsay@ramsayjones.plus.com> writes:

> I don't have any other systems to test this on, so I can't say that it
> won't cause problems somewhere. In practice, I think the chances of that
> are rather low, but don't quote me on that! :-P

This does smell like a right approach to me.  If we can get it
tested widely, that would be good.

Thanks.

> -- >8 --
> Subject: [PATCH] regex: fix up musl-libc builds
>
> ---
>  compat/regex/regex.c          | 1 +
>  compat/regex/regex_internal.h | 1 -
>  2 files changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/compat/regex/regex.c b/compat/regex/regex.c
> index f3e03a9eab..e6f4a5d177 100644
> --- a/compat/regex/regex.c
> +++ b/compat/regex/regex.c
> @@ -60,6 +60,7 @@
>     #undefs RE_DUP_MAX and sets it to the right value.  */
>  #include <limits.h>
>  #include <stdint.h>
> +#include <stdlib.h>
>  
>  #ifdef GAWK
>  #undef alloca
> diff --git a/compat/regex/regex_internal.h b/compat/regex/regex_internal.h
> index 3ee8aae59d..0bad8b841e 100644
> --- a/compat/regex/regex_internal.h
> +++ b/compat/regex/regex_internal.h
> @@ -23,7 +23,6 @@
>  #include <assert.h>
>  #include <ctype.h>
>  #include <stdio.h>
> -#include <stdlib.h>
>  #include <string.h>
>  
>  #if defined HAVE_LANGINFO_H || defined HAVE_LANGINFO_CODESET || defined _LIBC

Re: [PATCH v2 2/4] compat/regex: include alloca.h before undef it

[Danh Doan]

On 2020-04-26 17:17:56+0100, Ramsay Jones <ramsay@ramsayjones.plus.com> wrote:
> 
> 
> On 26/04/2020 01:54, Danh Doan wrote:
> > On 2020-04-25 21:28:05+0100, Ramsay Jones <ramsay@ramsayjones.plus.com> wrote:
> [snip]
> >> OK, I had a quick look at the <alloca.h> header file on a glibc
> >> system (linux) and new-lib system (cygwin) and they both do
> >> (more or less) the same thing: first #undef alloca, and then
> >> if being compiled by gcc, define alloca(size) to be __builtin_alloca(size).
> > 
> > musl people don't do that.
> > They just go ahead define it, if any other header file requires
> > alloca, they will include alloca.h
> > 
> >> So, even if <alloca.h> is #included after regex.c:66, it wouldn't
> >> be a problem. Since I don't have access to a musl based system,
> >> I don't know what that system header is doing.
> > 
> > musl's alloca.h is available here:
> > 
> > https://git.musl-libc.org/cgit/musl/tree/include/alloca.h
> 
> Hmm, OK, so that partly explains the problem. I wonder if the
> musl guys would accept a bug report?

I don't think they have a policy of no `#undef` whatsoever.
But, I think they're picky when come to C-correctly and
POSIX-correctly.
Does C or POSIX define alloca(3) at all?

And, I /think/ they'll likely ignore this one, [musl-faq] says:

	Assuming that including one header will cause symbols from
	another unrelated header to be exposed. This is an application
	bug, and fixing it is as simple as adding the missing #include
	directives. 

I guess they meant, if we have `alloca` defined, we must have included
`alloca.h` somewhere.

> >> However, I said *even if* above, because I don't see why it is trying
> >> to #include <alloca.h> in the first place! ;-)
> > 
> > I looked into my system again. The inclusion chain is:
> > 
> > compat/regex/regex.c:77
> > `-> compat/regex/regex_internal.h:26
> >     `-> /usr/include/stdlib.h:138 [*1*]
> > 
> > [*1*]: https://git.musl-libc.org/cgit/musl/tree/include/stdlib.h#n137
> > 
> > I don't know why _GNU_SOURCE and/or _BSD_SOURCE is defined.
> 
> ... and this explains the main cause. Hmm, as you say, why are
> one (or both) of those pp variables set. :(

Okay, I tracked it down.

compat/regex/regex.c:63
`-> /usr/include/limits.h:4
   `-> /usr/include/features.h:15

https://git.musl-libc.org/cgit/musl/tree/include/features.h?id=8e452abae67db445fb6c3e37cd566c4788c2e8f3#n14
musl defined `_BSD_SOURCE` if none of those below macro was defined:

- _BSD_SOURCE
- _POSIX_SOURCE
- _XOPEN_SOURCE
- _GNU_SOURCE
- __STRICT_ANSI__

I don't think we have any business to define which one of those macros
should be defined in the compat code.

> 
> [snip] 
> >> BTW, why are you compiling with NO_REGEX set anyway?
> > 
> > Because I use musl-libc, and musl-libc doesn't have StartEnd
> 
> Ah, OK. ;-)
> 
> Well, even if the musl project accepted a PR and provided a fix, that
> will not help you in the short term! :D
> 
> Hmm, whatever patch you decide to send (even the original one) I think
> you need to add an explanation of the problem to the commit message,
> including why the patch provides a solution. (You don't have to type
> a novel - see commit bd8f005583 :-P ).
> 
> I haven't thought about this too much, but some options:
> 
>   - iff the musl library sets some kind of identifying pp variable
>     (_MUSL_LIBC_ or somesuch - I haven't looked), then you could
>     make the '#include <alloca.h>' conditional on that variable.
>     This has the benefit of making it obvious to people reading the
>     code that this is specific to musl-libc.

musl's wiki's faq [musl-wiki-faq]:

	Q: Why is there no __MUSL__ macro?

>   - you could simply remove that '#ifdef GAWK' block completely (Lines
>     64->67). We set GAWK and NO_MBSUPPORT  unconditionally in the Makefile
>     so that it compiles (see commit a997bf423d), but these particular
>     lines simply reflect the gawk projects dislike of alloca (along with
>     the desire to catch any attempts to add new calls which are not protected
>     by HAVE_ALLOCA). Given that we are very unlikely to add new calls ...
> 
>   - change the conditional on this block to (totally untested, just typing
>     into my email client) '#if defined(GAWK) && defined(HAVE_ALLOCA)'.
>     This should work, but it does disable the 'catch any attempts to add
>     new _unintentional_ calls' aspect of that block; so you may as well
>     remove it ...

I'll go with your patch in the next email.
<1a0c2b25-e283-9936-1fa2-ce51df1404dc@ramsayjones.plus.com>

[musl-faq]: https://www.musl-libc.org/faq.html
[musl-wiki-faq]: https://wiki.musl-libc.org/faq.html

-- 
Danh

[PATCH v3 0/4] Partial fix `make sparse`

[Đoàn Trần Công Danh]

Change in v3 from v2:
* rewrite [2/4] based on Ramsay's suggestion

Change in v2 from v1:
* = { 0 } is an idiom to zero the structure, it shouldn't be changed
* change in [4/4] for prototype
* ignore diffent function prototype


Đoàn Trần Công Danh (4):
  test-parse-pathspec-file.c: s/0/NULL/ for pointer type
  compat/regex: move stdlib.h up in inclusion chain
  graph.c: limit linkage of internal variable
  progress.c: silence cgcc suggestion about internal linkage

 compat/regex/regex.c                | 1 +
 compat/regex/regex_internal.h       | 1 -
 graph.c                             | 2 +-
 progress.c                          | 2 +-
 progress.h                          | 8 ++++++++
 t/helper/test-parse-pathspec-file.c | 6 +++---
 t/helper/test-progress.c            | 9 +--------
 7 files changed, 15 insertions(+), 14 deletions(-)

Range-diff against v2:
1:  c9a1812abf = 1:  c9a1812abf test-parse-pathspec-file.c: s/0/NULL/ for pointer type
2:  290ba923b5 < -:  ---------- compat/regex: include alloca.h before undef it
-:  ---------- > 2:  8d18c53bc8 compat/regex: move stdlib.h up in inclusion chain
3:  39f8d85c2f = 3:  4e7580e1d1 graph.c: limit linkage of internal variable
4:  41eecf18ed = 4:  d66d9aa677 progress.c: silence cgcc suggestion about internal linkage
-- 
2.26.2.526.g744177e7f7

[PATCH v3 1/4] test-parse-pathspec-file.c: s/0/NULL/ for pointer type

[Đoàn Trần Công Danh]

Signed-off-by: Đoàn Trần Công Danh <congdanhqx@gmail.com>
---

Notes:
    	struct foo val = { 0 };
    
    should be taken as idiom for zero-ing structured data.

    I think this should be written to CodingGuideline.
    I'll prepare a patch for this addition

 t/helper/test-parse-pathspec-file.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/t/helper/test-parse-pathspec-file.c b/t/helper/test-parse-pathspec-file.c
index 02f4ccfd2a..b3e08cef4b 100644
--- a/t/helper/test-parse-pathspec-file.c
+++ b/t/helper/test-parse-pathspec-file.c
@@ -6,7 +6,7 @@
 int cmd__parse_pathspec_file(int argc, const char **argv)
 {
 	struct pathspec pathspec;
-	const char *pathspec_from_file = 0;
+	const char *pathspec_from_file = NULL;
 	int pathspec_file_nul = 0, i;
 
 	static const char *const usage[] = {
@@ -20,9 +20,9 @@ int cmd__parse_pathspec_file(int argc, const char **argv)
 		OPT_END()
 	};
 
-	parse_options(argc, argv, 0, options, usage, 0);
+	parse_options(argc, argv, NULL, options, usage, 0);
 
-	parse_pathspec_file(&pathspec, 0, 0, 0, pathspec_from_file,
+	parse_pathspec_file(&pathspec, 0, 0, NULL, pathspec_from_file,
 			    pathspec_file_nul);
 
 	for (i = 0; i < pathspec.nr; i++)
-- 
2.26.2.526.g744177e7f7

[PATCH v3 2/4] compat/regex: move stdlib.h up in inclusion chain

[Đoàn Trần Công Danh]

In Linux with musl libc, we have this inclusion chain:

compat/regex/regex.c:69
`-> compat/regex/regex_internal.h
   `-> /usr/include/stdlib.h
      `-> /usr/include/features.h
      `-> /usr/include/alloca.h

In that inclusion chain, `<features.h>` claims it's _BSD_SOURCE
compatible when it's NOT asked to be either
{_POSIX,_GNU,_XOPEN,_BSD}_SOURCE, or __STRICT_ANSI__.
And, `<stdlib.h>` will include `<alloca.h>` to be compatible with
software written for GNU and BSD. Thus, redefine `alloca` macro,
which was defined before at compat/regex/regex.c:66.

Considering this is only compat code, we've taken from other project,
it's not our business to decide which source should we adhere to.

Include `<stdlib.h>` early to prevent the redefinition of alloca.
This also remove a potential warning about alloca not defined on:
	#undef alloca

Helped-by: Ramsay Jones <ramsay@ramsayjones.plus.com>
Signed-off-by: Đoàn Trần Công Danh <congdanhqx@gmail.com>
---

Notes:
    There is another warning about un-match declaration and definition of
    re_string_context_at.
    
    It's arguably, a bug in sparse itself.
    Consider that there's a fix for it in their development version already,
    and, we expect they'll fix the zero-ing pattern in 1/? before enable it again.
    
    There're no point to change that pair of declaration/definition.

    This patch is technically Ramsay's work.
    Since <alloca.h> is likely unportable, my patch will be likely unusable.
    I took Ramsay's work and write the commit message instead.

    I would love to see Ramsay's SoB on reply to this patch.

 compat/regex/regex.c          | 1 +
 compat/regex/regex_internal.h | 1 -
 2 files changed, 1 insertion(+), 1 deletion(-)

diff --git a/compat/regex/regex.c b/compat/regex/regex.c
index f3e03a9eab..e6f4a5d177 100644
--- a/compat/regex/regex.c
+++ b/compat/regex/regex.c
@@ -60,6 +60,7 @@
    #undefs RE_DUP_MAX and sets it to the right value.  */
 #include <limits.h>
 #include <stdint.h>
+#include <stdlib.h>
 
 #ifdef GAWK
 #undef alloca
diff --git a/compat/regex/regex_internal.h b/compat/regex/regex_internal.h
index 3ee8aae59d..0bad8b841e 100644
--- a/compat/regex/regex_internal.h
+++ b/compat/regex/regex_internal.h
@@ -23,7 +23,6 @@
 #include <assert.h>
 #include <ctype.h>
 #include <stdio.h>
-#include <stdlib.h>
 #include <string.h>
 
 #if defined HAVE_LANGINFO_H || defined HAVE_LANGINFO_CODESET || defined _LIBC
-- 
2.26.2.526.g744177e7f7

[PATCH v3 3/4] graph.c: limit linkage of internal variable

[Đoàn Trần Công Danh]

Signed-off-by: Đoàn Trần Công Danh <congdanhqx@gmail.com>
---
 graph.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/graph.c b/graph.c
index 4fb25ad464..4cd9915075 100644
--- a/graph.c
+++ b/graph.c
@@ -1055,7 +1055,7 @@ static void graph_output_commit_line(struct git_graph *graph, struct graph_line
 		graph_update_state(graph, GRAPH_COLLAPSING);
 }
 
-const char merge_chars[] = {'/', '|', '\\'};
+static const char merge_chars[] = {'/', '|', '\\'};
 
 static void graph_output_post_merge_line(struct git_graph *graph, struct graph_line *line)
 {
-- 
2.26.2.526.g744177e7f7

[PATCH v3 4/4] progress.c: silence cgcc suggestion about internal linkage

[Đoàn Trần Công Danh]

Signed-off-by: Đoàn Trần Công Danh <congdanhqx@gmail.com>
---

Notes:
    Unify all declaration together and keep them under an ifdef.
    
    With the hope that no other static analysis tools complain.

 progress.c               | 2 +-
 progress.h               | 8 ++++++++
 t/helper/test-progress.c | 9 +--------
 3 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/progress.c b/progress.c
index 19805ac646..75633e9c5e 100644
--- a/progress.c
+++ b/progress.c
@@ -8,6 +8,7 @@
  * published by the Free Software Foundation.
  */
 
+#define GIT_TEST_PROGRESS_ONLY
 #include "cache.h"
 #include "gettext.h"
 #include "progress.h"
@@ -52,7 +53,6 @@ static volatile sig_atomic_t progress_update;
  */
 int progress_testing;
 uint64_t progress_test_ns = 0;
-void progress_test_force_update(void); /* To silence -Wmissing-prototypes */
 void progress_test_force_update(void)
 {
 	progress_update = 1;
diff --git a/progress.h b/progress.h
index 847338911f..f1913acf73 100644
--- a/progress.h
+++ b/progress.h
@@ -3,6 +3,14 @@
 
 struct progress;
 
+#ifdef GIT_TEST_PROGRESS_ONLY
+
+extern int progress_testing;
+extern uint64_t progress_test_ns;
+void progress_test_force_update(void);
+
+#endif
+
 void display_throughput(struct progress *progress, uint64_t total);
 void display_progress(struct progress *progress, uint64_t n);
 struct progress *start_progress(const char *title, uint64_t total);
diff --git a/t/helper/test-progress.c b/t/helper/test-progress.c
index 42b96cb103..5d05cbe789 100644
--- a/t/helper/test-progress.c
+++ b/t/helper/test-progress.c
@@ -13,20 +13,13 @@
  *
  * See 't0500-progress-display.sh' for examples.
  */
+#define GIT_TEST_PROGRESS_ONLY
 #include "test-tool.h"
 #include "gettext.h"
 #include "parse-options.h"
 #include "progress.h"
 #include "strbuf.h"
 
-/*
- * These are defined in 'progress.c', but are not exposed in 'progress.h',
- * because they are exclusively for testing.
- */
-extern int progress_testing;
-extern uint64_t progress_test_ns;
-void progress_test_force_update(void);
-
 int cmd__progress(int argc, const char **argv)
 {
 	int total = 0;
-- 
2.26.2.526.g744177e7f7

Re: [PATCH v2 2/4] compat/regex: include alloca.h before undef it

[Ramsay Jones]

On 27/04/2020 02:08, Danh Doan wrote:
[snip]

>>> musl's alloca.h is available here:
>>>
>>> https://git.musl-libc.org/cgit/musl/tree/include/alloca.h
>>
>> Hmm, OK, so that partly explains the problem. I wonder if the
>> musl guys would accept a bug report?
> 
> I don't think they have a policy of no `#undef` whatsoever.

That's fair enough.

> But, I think they're picky when come to C-correctly and
> POSIX-correctly.
> Does C or POSIX define alloca(3) at all?

No alloca() is not in either the POSIX or C standard(s).
This was an extension from the early days of BSD Unix.

For some reason, I thought you had to explicitly '#include <alloca.h>'
to use it, but it appears that (by default) you get a bonus include
from the <stdlib.h> header, unless you restrict the headers using the
various macros and/or compiler command-line options.

As it happens, even on glibc systems, the <alloca.h> header is included
by the <stdlib.h> header, unless you take steps to suppress it. So, we
would have had the same issue, if it wasn't for the aforementioned
'#undef alloca' the the glibc header file.

When I need to look at pp output, while debugging things like this,
I cherry-pick a patch to the Makefile:

  $ git diff
  diff --git a/Makefile b/Makefile
  index 6d5cee270c..cd8753bf54 100644
  --- a/Makefile
  +++ b/Makefile
  @@ -2423,6 +2423,9 @@ $(ASM_OBJ): %.o: %.S GIT-CFLAGS $(missing_dep_dirs)
   %.s: %.c GIT-CFLAGS FORCE
          $(QUIET_CC)$(CC) -o $@ -S $(ALL_CFLAGS) $(EXTRA_CPPFLAGS) $<
   
  +%.i: %.c GIT-CFLAGS FORCE
  +       $(QUIET_CC)$(CC) $(ALL_CFLAGS) $(EXTRA_CPPFLAGS) -E $< >$*.i
  +
   ifdef USE_COMPUTED_HEADER_DEPENDENCIES
   # Take advantage of gcc's on-the-fly dependency generation
   # See <http://gcc.gnu.org/gcc-3.0/features.html>.
  @@ -2474,7 +2477,7 @@ http-walker.sp http-walker.s http-walker.o: EXTRA_CPPFLAGS = -DNO_EXPAT
   endif
   
   ifdef NO_REGEX
  -compat/regex/regex.sp compat/regex/regex.o: EXTRA_CPPFLAGS = \
  +compat/regex/regex.i compat/regex/regex.sp compat/regex/regex.o: EXTRA_CPPFLAGS = \
          -DGAWK -DNO_MBSUPPORT
   endif
   
  $ 

[The second hunk above is not actually part of the cherry-picked patch,
but I needed it in this instance to get GAWK and NO_MBSUPPORT passed to
the compiler!]

  $ make NO_REGEX=1 compat/regex/regex.i
      CC compat/regex/regex.i
  $ vim compat/regex/regex.i
  $ 

... which shows <alloca.h> is indeed being #included from <stdlib.h>.
[it is protected by a __USE_MISC pp variable, but I didn't bother to
track it down! ;-)]

ATB,
Ramsay Jones

  

Re: [PATCH v3 0/4] Partial fix `make sparse`

[Ramsay Jones]

On 27/04/2020 15:22, Đoàn Trần Công Danh wrote:
> Change in v3 from v2:
> * rewrite [2/4] based on Ramsay's suggestion

Yep, these all look good to me. Thanks!

ATB,
Ramsay Jones

> 
> Change in v2 from v1:
> * = { 0 } is an idiom to zero the structure, it shouldn't be changed
> * change in [4/4] for prototype
> * ignore diffent function prototype
> 
> 
> Đoàn Trần Công Danh (4):
>   test-parse-pathspec-file.c: s/0/NULL/ for pointer type
>   compat/regex: move stdlib.h up in inclusion chain
>   graph.c: limit linkage of internal variable
>   progress.c: silence cgcc suggestion about internal linkage
> 
>  compat/regex/regex.c                | 1 +
>  compat/regex/regex_internal.h       | 1 -
>  graph.c                             | 2 +-
>  progress.c                          | 2 +-
>  progress.h                          | 8 ++++++++
>  t/helper/test-parse-pathspec-file.c | 6 +++---
>  t/helper/test-progress.c            | 9 +--------
>  7 files changed, 15 insertions(+), 14 deletions(-)
> 
> Range-diff against v2:
> 1:  c9a1812abf = 1:  c9a1812abf test-parse-pathspec-file.c: s/0/NULL/ for pointer type
> 2:  290ba923b5 < -:  ---------- compat/regex: include alloca.h before undef it
> -:  ---------- > 2:  8d18c53bc8 compat/regex: move stdlib.h up in inclusion chain
> 3:  39f8d85c2f = 3:  4e7580e1d1 graph.c: limit linkage of internal variable
> 4:  41eecf18ed = 4:  d66d9aa677 progress.c: silence cgcc suggestion about internal linkage
> 

Re: [PATCH v3 2/4] compat/regex: move stdlib.h up in inclusion chain

[Ramsay Jones]

On 27/04/2020 15:22, Đoàn Trần Công Danh wrote:
> In Linux with musl libc, we have this inclusion chain:
> 
> compat/regex/regex.c:69
> `-> compat/regex/regex_internal.h
>    `-> /usr/include/stdlib.h
>       `-> /usr/include/features.h
>       `-> /usr/include/alloca.h
> 
> In that inclusion chain, `<features.h>` claims it's _BSD_SOURCE
> compatible when it's NOT asked to be either
> {_POSIX,_GNU,_XOPEN,_BSD}_SOURCE, or __STRICT_ANSI__.
> And, `<stdlib.h>` will include `<alloca.h>` to be compatible with
> software written for GNU and BSD. Thus, redefine `alloca` macro,
> which was defined before at compat/regex/regex.c:66.
> 
> Considering this is only compat code, we've taken from other project,
> it's not our business to decide which source should we adhere to.
> 
> Include `<stdlib.h>` early to prevent the redefinition of alloca.
> This also remove a potential warning about alloca not defined on:
> 	#undef alloca
> 
> Helped-by: Ramsay Jones <ramsay@ramsayjones.plus.com>
> Signed-off-by: Đoàn Trần Công Danh <congdanhqx@gmail.com>
> ---
> 
> Notes:
>     There is another warning about un-match declaration and definition of
>     re_string_context_at.
>     
>     It's arguably, a bug in sparse itself.
>     Consider that there's a fix for it in their development version already,
>     and, we expect they'll fix the zero-ing pattern in 1/? before enable it again.

Ahem, yes I still haven't found time to find that patch yet.
Sorry about that. :(

>     There're no point to change that pair of declaration/definition.
> 
>     This patch is technically Ramsay's work.
>     Since <alloca.h> is likely unportable, my patch will be likely unusable.
>     I took Ramsay's work and write the commit message instead.
> 
>     I would love to see Ramsay's SoB on reply to this patch.

I think 'Helped-by:' is all that is necessary (you did the hard work,
including testing on musl-libc), but if you prefer:

Signed-off-by: Ramsay Jones <ramsay@ramsayjones.plus.com>

ATB,
Ramsay Jones

> 
>  compat/regex/regex.c          | 1 +
>  compat/regex/regex_internal.h | 1 -
>  2 files changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/compat/regex/regex.c b/compat/regex/regex.c
> index f3e03a9eab..e6f4a5d177 100644
> --- a/compat/regex/regex.c
> +++ b/compat/regex/regex.c
> @@ -60,6 +60,7 @@
>     #undefs RE_DUP_MAX and sets it to the right value.  */
>  #include <limits.h>
>  #include <stdint.h>
> +#include <stdlib.h>
>  
>  #ifdef GAWK
>  #undef alloca
> diff --git a/compat/regex/regex_internal.h b/compat/regex/regex_internal.h
> index 3ee8aae59d..0bad8b841e 100644
> --- a/compat/regex/regex_internal.h
> +++ b/compat/regex/regex_internal.h
> @@ -23,7 +23,6 @@
>  #include <assert.h>
>  #include <ctype.h>
>  #include <stdio.h>
> -#include <stdlib.h>
>  #include <string.h>
>  
>  #if defined HAVE_LANGINFO_H || defined HAVE_LANGINFO_CODESET || defined _LIBC
> 

Re: [PATCH v2 2/4] compat/regex: include alloca.h before undef it

[Danh Doan]

On 2020-04-27 17:28:03+0100, Ramsay Jones <ramsay@ramsayjones.plus.com> wrote:
> No alloca() is not in either the POSIX or C standard(s).
> This was an extension from the early days of BSD Unix.

Okay, I saw it was mentioned in GNU manpage for alloca(3)

 
> For some reason, I thought you had to explicitly '#include <alloca.h>'
> to use it, but it appears that (by default) you get a bonus include
> from the <stdlib.h> header, unless you restrict the headers using the
> various macros and/or compiler command-line options.

It looks like <alloca.h> is GNU's invention.

*BSD defined it in <stdlib.h>:
https://www.freebsd.org/cgi/man.cgi?alloca
https://man.openbsd.org/alloca
https://netbsd.gw.com/cgi-bin/man-cgi?alloca+3+NetBSD-current

Even GNU's alloca(3) says:

       Normally, gcc(1) translates calls to alloca() with inlined code.  This
       is not done when either the -ansi, -std=c89, -std=c99, or the -std=c11
       option is given and the header <alloca.h> is not included.  Otherwise,
       (without an -ansi or -std=c* option) the glibc version of <stdlib.h>
       includes <alloca.h> 

> As it happens, even on glibc systems, the <alloca.h> header is included
> by the <stdlib.h> header, unless you take steps to suppress it. So, we
> would have had the same issue, if it wasn't for the aforementioned
> '#undef alloca' the the glibc header file.

From above information, I think it's fine to include <stdlib.h> first.
It's AT&T Unix's invention and everyone seems to follow it (except Windows,
but the lack of complains from our Windows friends may signify that
their alloca is fine already).

I've sent it already for v3.

 
> When I need to look at pp output, while debugging things like this,
> I cherry-pick a patch to the Makefile:
> 
>   $ git diff
>   diff --git a/Makefile b/Makefile
>   index 6d5cee270c..cd8753bf54 100644
>   --- a/Makefile
>   +++ b/Makefile
>   @@ -2423,6 +2423,9 @@ $(ASM_OBJ): %.o: %.S GIT-CFLAGS $(missing_dep_dirs)
>    %.s: %.c GIT-CFLAGS FORCE
>           $(QUIET_CC)$(CC) -o $@ -S $(ALL_CFLAGS) $(EXTRA_CPPFLAGS) $<
>    
>   +%.i: %.c GIT-CFLAGS FORCE
>   +       $(QUIET_CC)$(CC) $(ALL_CFLAGS) $(EXTRA_CPPFLAGS) -E $< >$*.i
>   +
>    ifdef USE_COMPUTED_HEADER_DEPENDENCIES
>    # Take advantage of gcc's on-the-fly dependency generation
>    # See <http://gcc.gnu.org/gcc-3.0/features.html>.
>   @@ -2474,7 +2477,7 @@ http-walker.sp http-walker.s http-walker.o: EXTRA_CPPFLAGS = -DNO_EXPAT
>    endif
>    
>    ifdef NO_REGEX
>   -compat/regex/regex.sp compat/regex/regex.o: EXTRA_CPPFLAGS = \
>   +compat/regex/regex.i compat/regex/regex.sp compat/regex/regex.o: EXTRA_CPPFLAGS = \
>           -DGAWK -DNO_MBSUPPORT
>    endif
>    
>   $ 

I think it's worth to have this included. `.s` rules is there, anyway.
Personally, I've run this to trace the inclusion:

	make V=1 CFLAGS=-E compat/regex/regex.o compat/regex/regex.sp


-- 
Danh

Re: [PATCH v2 2/4] compat/regex: include alloca.h before undef it

[Ramsay Jones]

On 27/04/2020 17:46, Danh Doan wrote:
[snip]

> It looks like <alloca.h> is GNU's invention.
> 
> *BSD defined it in <stdlib.h>:
> https://www.freebsd.org/cgi/man.cgi?alloca
> https://man.openbsd.org/alloca
> https://netbsd.gw.com/cgi-bin/man-cgi?alloca+3+NetBSD-current

Yeah, that would make sense. It's been about 25 years since I used
a BSD based system (Hmm, Irix changed its base from AT&T to BSD at
one point; or was it the other way round - I forget!)

>> As it happens, even on glibc systems, the <alloca.h> header is included
>> by the <stdlib.h> header, unless you take steps to suppress it. So, we
>> would have had the same issue, if it wasn't for the aforementioned
>> '#undef alloca' the the glibc header file.
> 
>>From above information, I think it's fine to include <stdlib.h> first.
> It's AT&T Unix's invention and everyone seems to follow it (except Windows,
> but the lack of complains from our Windows friends may signify that
> their alloca is fine already).
> 
> I've sent it already for v3.

Yep, looks good.

>> When I need to look at pp output, while debugging things like this,
>> I cherry-pick a patch to the Makefile:
>>
>>   $ git diff
>>   diff --git a/Makefile b/Makefile
>>   index 6d5cee270c..cd8753bf54 100644
>>   --- a/Makefile
>>   +++ b/Makefile
>>   @@ -2423,6 +2423,9 @@ $(ASM_OBJ): %.o: %.S GIT-CFLAGS $(missing_dep_dirs)
>>    %.s: %.c GIT-CFLAGS FORCE
>>           $(QUIET_CC)$(CC) -o $@ -S $(ALL_CFLAGS) $(EXTRA_CPPFLAGS) $<
>>    
>>   +%.i: %.c GIT-CFLAGS FORCE
>>   +       $(QUIET_CC)$(CC) $(ALL_CFLAGS) $(EXTRA_CPPFLAGS) -E $< >$*.i
>>   +
>>    ifdef USE_COMPUTED_HEADER_DEPENDENCIES
>>    # Take advantage of gcc's on-the-fly dependency generation
>>    # See <http://gcc.gnu.org/gcc-3.0/features.html>.
>>   @@ -2474,7 +2477,7 @@ http-walker.sp http-walker.s http-walker.o: EXTRA_CPPFLAGS = -DNO_EXPAT
>>    endif
>>    
>>    ifdef NO_REGEX
>>   -compat/regex/regex.sp compat/regex/regex.o: EXTRA_CPPFLAGS = \
>>   +compat/regex/regex.i compat/regex/regex.sp compat/regex/regex.o: EXTRA_CPPFLAGS = \
>>           -DGAWK -DNO_MBSUPPORT
>>    endif
>>    
>>   $ 
> 
> I think it's worth to have this included. `.s` rules is there, anyway.

Hmm, I can't remember if I ever actually submitted a patch; I've had
this patch floating in my git repo for about 10 years or so! ;-)
I don't use it very often, but it's very useful when needed.

ATB,
Ramsay Jones

Re: [PATCH 0/4] fix make sparse warning

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 1982 bytes --]

Hi,

On Sun, 26 Apr 2020, Ramsay Jones wrote:

> On 26/04/2020 04:32, Danh Doan wrote:
> > On 2020-04-25 15:13:53+0200, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> >> Hi Danh,
> >>
> >> On Thu, 23 Apr 2020, Đoàn Trần Công Danh wrote:
> >>
> >>> It's happened that I tried to run make check on project that support autoconf.
> >>> Git's Makefile told me to run `make sparse` instead.
> >>>
> >>> I /think/ if we have a rule in Makefile, we should adhere to it.
> >>> I also fix another change in ds/blame-on-bloom, which I think it's worth to
> >>> fix, see: <20200423133937.GA1984@danh.dev>
> >>
> >> FWIW there is still an Azure Pipeline building `sparse` for Ubuntu every
> >> two weeks: https://dev.azure.com/git/git/_build?definitionId=10&_a=summary
> >>
> >> I created this Pipeline in order to support the proposed project at
> >> https://github.com/gitgitgadget/git/issues/345 which is: teach our CI
> >> builds to run `make sparse`.
> >>
> >> Maybe it is time to tackle that?
> >
> > I don't think it's ready, yet!
>
> Heh, not too long ago, the 'master' and 'next' branches were 'sparse clean'.
> (on non NO_REGEX builds anyway, and even on those for some of us. ;-) )

Maybe we should spend the time to make it "sparse clean" again.

Of course, that would mean that reftables would have a ton of work in
front of it (not that it does already...).

> The 'pu' branch was frequently 'unclean', of course.

But mostly because there was no automated job to tell contributors about
it.

Ciao,
Dscho

>
> ATB,
> Ramsay Jones
>
> >
> > May be it's.
> >
> > But we'll need
> >
> > 	make sparse |
> > 	grep -v -e 'plain integer as NULL pointer' \
> > 		-e 'redeclared with different type'
> >
> > The first one for:
> >
> > 	struct foo val = { 0 };
> >
> > to zero structure.
> >
> > The second one for different in declaration and definition (which is
> > fixed in sparse's master).
> >
> >
>

Re: [PATCH v3 4/4] progress.c: silence cgcc suggestion about internal linkage

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 1129 bytes --]

Hi,

On Mon, 27 Apr 2020, Đoàn Trần Công Danh wrote:

> diff --git a/progress.h b/progress.h
> index 847338911f..f1913acf73 100644
> --- a/progress.h
> +++ b/progress.h
> @@ -3,6 +3,14 @@
>
>  struct progress;
>
> +#ifdef GIT_TEST_PROGRESS_ONLY
> +
> +extern int progress_testing;
> +extern uint64_t progress_test_ns;
> +void progress_test_force_update(void);
> +
> +#endif
> +
>  void display_throughput(struct progress *progress, uint64_t total);
>  void display_progress(struct progress *progress, uint64_t n);
>  struct progress *start_progress(const char *title, uint64_t total);

A slightly cleaner strategy would be to add a separate header file, say,
`progress-internal.h` or `internal/progress.h` to declare the variables
and the function.

I guess at some stage we really need to define a proper API of libgit.a.
Just because we refuse to declare such an API doesn't mean that people
don't use libgit.a in that way (e.g. cgit, cinnabar). We might just as
well face reality and split up `cache.h` and put the API part into header
files in a subdirectory `include/`.

Ciao,
Dscho

Re: [PATCH 1/4] C: s/0/NULL/ for pointer type

[Luc Van Oostenryck]

On Thu, Apr 23, 2020 at 05:54:30PM -0700, Junio C Hamano wrote:
> Ramsay Jones <ramsay@ramsayjones.plus.com> writes:
> 
> > Actually, I have a patch somewhere which suppressed the sparse warning
> > for the '= { 0 }' token sequence used in these initializations. However,
> > I don't seem to be able to find them at the moment! :(
> 
> AHHHHHhhhhhhhh.  
> 
> Thanks for reminding.  Yeah, I recall that the concensus of those
> who were vocal in that old discussion [*1*] was that "= { 0 }"
> should be taken as an idiom and should not be subject to s/0/NULL/
> conversion.
> 
> > [Luc, this topic came up on the sparse and kernel mailing-lists at one
> > point, but I didn't get around to posting my patch to the list - something
> > came up. Hopefully, I will find some time to find it and post it soon.]
> 
> 
> [References]
> 
> *1*
> 
> https://lore.kernel.org/git/e6796c60-a870-e761-3b07-b680f934c537@ramsayjones.plus.com/

Sorry for the late reply.

I hadn't seen this old discussion but I vaguely remember 2 emails about
this problem, probaly on lkml, without a real discussion, but where the
opinion was the opposite.

Personally, I prefer '= { }' but yes it's not legit and it seems that
some compilers don't like it. I'll be glad to add an option to Sparse
to shut up the warnings now given by '{ 0 }'.

-- Luc