[PATCH] bisect.c: remove unused includes

[PATCH] bisect.c: remove unused includes

[Garrit Franke]

Clean up includes no longer needed by bisect.c.

Signed-off-by: Garrit Franke <garrit@slashdev.space>
---
 bisect.c | 9 ---------
 1 file changed, 9 deletions(-)

diff --git a/bisect.c b/bisect.c
index 9e6a2b7f20..e07e2d215d 100644
--- a/bisect.c
+++ b/bisect.c
@@ -1,21 +1,12 @@
-#include "cache.h"
 #include "config.h"
-#include "commit.h"
-#include "diff.h"
-#include "revision.h"
 #include "refs.h"
 #include "list-objects.h"
 #include "quote.h"
-#include "hash-lookup.h"
 #include "run-command.h"
 #include "log-tree.h"
 #include "bisect.h"
-#include "oid-array.h"
-#include "strvec.h"
-#include "commit-slab.h"
 #include "commit-reach.h"
 #include "object-store.h"
-#include "dir.h"
 
 static struct oid_array good_revs;
 static struct oid_array skipped_revs;
-- 
2.35.1

Re: [PATCH] bisect.c: remove unused includes

[Junio C Hamano]

Garrit Franke <garrit@slashdev.space> writes:

> Clean up includes no longer needed by bisect.c.
>
> Signed-off-by: Garrit Franke <garrit@slashdev.space>
> ---
>  bisect.c | 9 ---------
>  1 file changed, 9 deletions(-)
>
> diff --git a/bisect.c b/bisect.c
> index 9e6a2b7f20..e07e2d215d 100644
> --- a/bisect.c
> +++ b/bisect.c
> @@ -1,21 +1,12 @@
> -#include "cache.h"

cf. Documentation/CodingGuidelines

The first #include must be <git-compat-util.h>, or <cache.h> or
<builtin.h>, which are well known to include <git-compat-util.h>
first.

Including <git-compat-util.h> indirectly by <config.h> ->
<hashmap.h> -> <hash.h> -> <git-compat-util.h> does not count.

>  #include "config.h"
> -#include "commit.h"

Other headers may indirectly include <commit.h> as their
implementation detail, but what matters is that *we* in this source
file use what <commit.h> gives us ourselves, like the concrete shape
of "struct commit_list".  This change is not wanted.

I'll stop here.  There may be truly leftover "unused" includes among
those removed by the remainder of this patch, but I suspect that
some are like <commit.h> above, i.e. we directly use it, and because
we do not want to be broken by some header file's implementation
detail changing, we MUST include it ourselves.

I think this should give us a useful guideline to sift through the
rest, and an updated patch to remove truly unused ones are very much
welcome.  We may actually find some we are not directly including
ourselves but we should (e.g. I do not see <string-list.h> included
by us, but we clearly use structures and functions declared there,
and probably is depending, wrongly, on some header file we include
happens to indirectly include it).

Thanks.

> -#include "diff.h"
> -#include "revision.h"
>  #include "refs.h"
>  #include "list-objects.h"
>  #include "quote.h"
> -#include "hash-lookup.h"
>  #include "run-command.h"
>  #include "log-tree.h"
>  #include "bisect.h"
> -#include "oid-array.h"
> -#include "strvec.h"
> -#include "commit-slab.h"
>  #include "commit-reach.h"
>  #include "object-store.h"
> -#include "dir.h"
>  
>  static struct oid_array good_revs;
>  static struct oid_array skipped_revs;

using iwyu (include-what-you-use) to analyze includes (was: [PATCH] bisect.c: remove unused includes)

[Ævar Arnfjörð Bjarmason]

On Thu, Mar 31 2022, Junio C Hamano wrote:

[Changed $subject to make this easier to find]

> Garrit Franke <garrit@slashdev.space> writes:
>
>> Clean up includes no longer needed by bisect.c.
>>
>> Signed-off-by: Garrit Franke <garrit@slashdev.space>
>> ---
>>  bisect.c | 9 ---------
>>  1 file changed, 9 deletions(-)
>>
>> diff --git a/bisect.c b/bisect.c
>> index 9e6a2b7f20..e07e2d215d 100644
>> --- a/bisect.c
>> +++ b/bisect.c
>> @@ -1,21 +1,12 @@
>> -#include "cache.h"
>
> cf. Documentation/CodingGuidelines
>
> The first #include must be <git-compat-util.h>, or <cache.h> or
> <builtin.h>, which are well known to include <git-compat-util.h>
> first.
>
> Including <git-compat-util.h> indirectly by <config.h> ->
> <hashmap.h> -> <hash.h> -> <git-compat-util.h> does not count.

Also: Some built-ins don't include builtin.h as they should, a fix (or
even basic CI check) for that would be most welcome.

	git grep -C2 -n -F -e builtin.h -e cache.h -e git-compat-util.h -- builtin

I.e. we have this saying a lot of those are redundant:
	
	Documentation/CodingGuidelines- - The first #include in C files, except in platform specific compat/
	Documentation/CodingGuidelines-   implementations, must be either "git-compat-util.h", "cache.h" or
	Documentation/CodingGuidelines:   "builtin.h".  You do not have to include more than one of these.

But maybe it's not worth it, anyway...

>>  #include "config.h"
>> -#include "commit.h"
>
> Other headers may indirectly include <commit.h> as their
> implementation detail, but what matters is that *we* in this source
> file use what <commit.h> gives us ourselves, like the concrete shape
> of "struct commit_list".  This change is not wanted.
>
> I'll stop here.  There may be truly leftover "unused" includes among
> those removed by the remainder of this patch, but I suspect that
> some are like <commit.h> above, i.e. we directly use it, and because
> we do not want to be broken by some header file's implementation
> detail changing, we MUST include it ourselves.
>
> I think this should give us a useful guideline to sift through the
> rest, and an updated patch to remove truly unused ones are very much
> welcome.  We may actually find some we are not directly including
> ourselves but we should (e.g. I do not see <string-list.h> included
> by us, but we clearly use structures and functions declared there,
> and probably is depending, wrongly, on some header file we include
> happens to indirectly include it).

... For anyone interested in pursuing this, I think using the excellent
include-what-you-use tool would be a nice start.

We could even eventually add it to our CI if the false positive rate
isn't bad (I haven't checked much):
https://github.com/include-what-you-use/include-what-you-use

E.g. in this case (I manually omitted the rest of the output, there's
probably a iwyu option to omit it, but I didn't see how do that from
skimming the docs):

	$ sudo apt install iwyu # YMMV
	$ make bisect.o CC=include-what-you-use CFLAGS="-Xiwyu --verbose=1" 2>&1 | grep -v -E -e '^#include <' -e '^#include "(cache|git-compat-util|gettext)\.h"'
	    CC bisect.o
	
	(bisect.h has correct #includes/fwd-decls)
	
	bisect.c should add these lines:
	#include "hash.h"              // for oideq, object_id, oidcmp, oidcpy, GIT_...
	#include "object.h"            // for object, repo_clear_commit_marks
	#include "path.h"              // for GIT_PATH_FUNC, git_pathdup
	#include "pretty.h"            // for CMIT_FMT_UNSPECIFIED, format_commit_me...
	#include "repository.h"        // for repository (ptr only), the_repository
	#include "strbuf.h"            // for strbuf_release, strbuf, strbuf_getline_lf
	#include "string-list.h"       // for string_list_append, string_list_clear
	
	bisect.c should remove these lines:
	- #include "hash-lookup.h"  // lines 9-9
	- struct commit_weight;  // lines 76-76

Then if I patch it as:
	
	diff --git a/bisect.c b/bisect.c
	index 9e6a2b7f201..512430e3cc8 100644
	--- a/bisect.c
	+++ b/bisect.c
	@@ -6,7 +6,6 @@
	 #include "refs.h"
	 #include "list-objects.h"
	 #include "quote.h"
	-#include "hash-lookup.h"
	 #include "run-command.h"
	 #include "log-tree.h"
	 #include "bisect.h"
	@@ -16,6 +15,13 @@
	 #include "commit-reach.h"
	 #include "object-store.h"
	 #include "dir.h"
	+#include "hash.h"
	+#include "object.h"
	+#include "path.h"
	+#include "pretty.h"
	+#include "repository.h"
	+#include "strbuf.h"
	+#include "string-list.h"
	 
	 static struct oid_array good_revs;
	 static struct oid_array skipped_revs;

It's happier, but probably needs to be told to ignore define_commit_slab() somehow:

	$ make bisect.o CC=include-what-you-use CFLAGS="-Xiwyu --verbose=1" 2>&1 | grep -v -E -e '^#include <' -e '^#include "(cache|git-compat-util|gettext)\.h"'
	    CC bisect.o
	
	(bisect.h has correct #includes/fwd-decls)
	
	bisect.c should add these lines:
	
	bisect.c should remove these lines:
	- struct commit_weight;  // lines 82-82

That still needs to be massaged a bit, e.g. we should probably omit
hash.h and anything else in cache.h and git-compat-util.h.

Or maybe not & we should make those headers even lighter. It is rather
annoying that changing some of those things leads to a complete
re-build, but there's a trade-off there where we probably want things
like gettext.h and other used-almost-everywhere headers in included by
those.

So take all the above with a huge grain of salt. I haven't used iwyu
much, but it seems to be something that'll help us go in the direction
Junio noted above.

I think starting with:

	make -k git-objs <the CC etc. params above>

And tackling the "should remove these lines" issues first would be a
good start, e.g. for serve.c it says:
	
	serve.c should remove these lines:
	- #include "cache.h"  // lines 1-1
	- #include "strvec.h"  // lines 6-6

We don't want that first one, but it's right about the second one. It's
been orphaned since f0a35c9ce52 (serve: drop "keys" strvec, 2021-09-15),
I skimmed some of the rist and they all seem like good
suggestions. E.g. lockfile.h for builtin/apply.c, which isn't needed
since 6d058c88264 (apply: move lockfile into `apply_state`, 2017-10-05).

[PATCH v2 0/4] various: use iwyu (include-what-you-use) to analyze includes

[Garrit Franke]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=ANSI_X3.4-1968, Size: 1764 bytes --]

I'm sorry about the half-baked patch of mine. I initially noticed that
the "hash-lookup.h" wasn't needed (which was a correct assumption), and
then went overboard when trying to clean up the other includes.

This is truly a rookie mistake that only wastes the time of everyone
involved. This won't happen again!

On 01.04.22 10:07, Ævar Arnfjörð Bjarmason wrote:

> ... For anyone interested in pursuing this, I think using the excellent
> include-what-you-use tool would be a nice start.
>
> We could even eventually add it to our CI if the false positive rate
> isn't bad (I haven't checked much):
> https://github.com/include-what-you-use/include-what-you-use

This seems to be a really nice tool indeed. I wouldn't be comfortable
adding it to the CI just yet, but it did make it considerably easier to
spot includes that could safely be removed.

I think we could try battle-testing this tool in the codebase to get a
sense of how it behaves. To start, I added your reference-command to a
script under "contrib/iwyu" and ran it against the files you noted.
Before breaking a bulk of the files, I wanted to make sure that this
undertaking is headed in the right direction.

Feedback is of course welcomed!

Garrit Franke (4):
  contrib: add iwyu script
  bisect.c: remove unnecessary include
  serve.c: remove unnecessary include
  apply.c: remove unnecessary include

 bisect.c             |  1 -
 builtin/apply.c      |  1 -
 contrib/iwyu/README  | 33 +++++++++++++++++++++++++++++++++
 contrib/iwyu/iwyu.sh |  2 ++
 serve.c              |  1 -
 5 files changed, 35 insertions(+), 3 deletions(-)
 create mode 100644 contrib/iwyu/README
 create mode 100755 contrib/iwyu/iwyu.sh

-- 
2.35.1

[PATCH v2 1/4] contrib: add iwyu script

[Garrit Franke]

add include-what-you-use helper.

Signed-off-by: Garrit Franke <garrit@slashdev.space>
---
 contrib/iwyu/README  | 33 +++++++++++++++++++++++++++++++++
 contrib/iwyu/iwyu.sh |  2 ++
 2 files changed, 35 insertions(+)
 create mode 100644 contrib/iwyu/README
 create mode 100755 contrib/iwyu/iwyu.sh

diff --git a/contrib/iwyu/README b/contrib/iwyu/README
new file mode 100644
index 0000000000..5e2d218602
--- /dev/null
+++ b/contrib/iwyu/README
@@ -0,0 +1,33 @@
+Include What You Use
+====================
+
+Include what you use (iwyu) [1] is a tool that points out which headers a file
+should include. Moreover, it can point out includes that are not used by a file,
+which makes it especially handy for cleanup tasks.
+
+To run this script, you will need iwyu to be installed on your system.
+
+The "iwyu.sh" script runs iwyu on a given object and omits mandatory headers
+defined in "Documentation/CodingGuidelines".
+
+Example usage:
+
+    ./contrib/iwyu/iwyu.sh diff.o
+
+This yields:
+
+    diff.c should remove these lines:
+    - #include "attr.h"  // lines 13-13
+    - #include "submodule-config.h"  // lines 18-18
+
+In its current form, this script should not be used to auto-generate patches,
+since there are still some false-positives that only a human can resolve. It is
+meant as a starting point for further cleanups. It could be nice to integrate
+this as a step in our CI, but we're not quite there yet.
+
+The inspiration for this script came from this [2] email-thread.
+
+Garrit Franke <garrit@slashdev.space>
+
+[1]: https://github.com/include-what-you-use/include-what-you-use
+[2]: https://lore.kernel.org/all/220401.8635ixp3f4.gmgdl@evledraar.gmail.com/#t
\ No newline at end of file
diff --git a/contrib/iwyu/iwyu.sh b/contrib/iwyu/iwyu.sh
new file mode 100755
index 0000000000..3ef8639eae
--- /dev/null
+++ b/contrib/iwyu/iwyu.sh
@@ -0,0 +1,2 @@
+make $1 CC=include-what-you-use CFLAGS="-Xiwyu --verbose=1" 2>&1 \
+| grep -v -E -e '^#include <' -e '^#include "(cache|git-compat-util|gettext)\.h"' 
\ No newline at end of file
-- 
2.35.1

[PATCH v2 2/4] bisect.c: remove unnecessary include

[Garrit Franke]

Remove include "hash-lookup.h".

Signed-off-by: Garrit Franke <garrit@slashdev.space>
---
 bisect.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/bisect.c b/bisect.c
index 9e6a2b7f20..1aced76257 100644
--- a/bisect.c
+++ b/bisect.c
@@ -6,7 +6,6 @@
 #include "refs.h"
 #include "list-objects.h"
 #include "quote.h"
-#include "hash-lookup.h"
 #include "run-command.h"
 #include "log-tree.h"
 #include "bisect.h"
-- 
2.35.1

[PATCH v2 3/4] serve.c: remove unnecessary include

[Garrit Franke]

Remove include "strvec.h" from serve.c, which is orphaned since
f0a35c9ce52 (serve: drop "keys" strvec, 2021-09-15)

Signed-off-by: Garrit Franke <garrit@slashdev.space>
---
 serve.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/serve.c b/serve.c
index b3fe9b5126..733347f602 100644
--- a/serve.c
+++ b/serve.c
@@ -3,7 +3,6 @@
 #include "config.h"
 #include "pkt-line.h"
 #include "version.h"
-#include "strvec.h"
 #include "ls-refs.h"
 #include "protocol-caps.h"
 #include "serve.h"
-- 
2.35.1

[PATCH v2 4/4] apply.c: remove unnecessary include

[Garrit Franke]

Remove include "lockfile.h" from builtin/apply.c, which is orphaned
since 6d058c88264 (apply: move lockfile into `apply_state`, 2017-10-05)

Signed-off-by: Garrit Franke <garrit@slashdev.space>
---
 builtin/apply.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/builtin/apply.c b/builtin/apply.c
index 3f099b9605..555219de40 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -1,7 +1,6 @@
 #include "cache.h"
 #include "builtin.h"
 #include "parse-options.h"
-#include "lockfile.h"
 #include "apply.h"
 
 static const char * const apply_usage[] = {
-- 
2.35.1

Re: [PATCH v2 1/4] contrib: add iwyu script

[Ævar Arnfjörð Bjarmason]

On Tue, Apr 05 2022, Garrit Franke wrote:

> add include-what-you-use helper.
>
> Signed-off-by: Garrit Franke <garrit@slashdev.space>
> ---
>  contrib/iwyu/README  | 33 +++++++++++++++++++++++++++++++++
>  contrib/iwyu/iwyu.sh |  2 ++
>  2 files changed, 35 insertions(+)
>  create mode 100644 contrib/iwyu/README
>  create mode 100755 contrib/iwyu/iwyu.sh
>
> diff --git a/contrib/iwyu/README b/contrib/iwyu/README
> new file mode 100644
> index 0000000000..5e2d218602
> --- /dev/null
> +++ b/contrib/iwyu/README
> @@ -0,0 +1,33 @@
> +Include What You Use
> +====================
> +
> +Include what you use (iwyu) [1] is a tool that points out which headers a file
> +should include. Moreover, it can point out includes that are not used by a file,
> +which makes it especially handy for cleanup tasks.
> +
> +To run this script, you will need iwyu to be installed on your system.
> +
> +The "iwyu.sh" script runs iwyu on a given object and omits mandatory headers
> +defined in "Documentation/CodingGuidelines".
> +
> +Example usage:
> +
> +    ./contrib/iwyu/iwyu.sh diff.o
> +
> +This yields:
> +
> +    diff.c should remove these lines:
> +    - #include "attr.h"  // lines 13-13
> +    - #include "submodule-config.h"  // lines 18-18
> +
> +In its current form, this script should not be used to auto-generate patches,
> +since there are still some false-positives that only a human can resolve. It is
> +meant as a starting point for further cleanups. It could be nice to integrate
> +this as a step in our CI, but we're not quite there yet.
> +
> +The inspiration for this script came from this [2] email-thread.
> +
> +Garrit Franke <garrit@slashdev.space>
> +
> +[1]: https://github.com/include-what-you-use/include-what-you-use
> +[2]: https://lore.kernel.org/all/220401.8635ixp3f4.gmgdl@evledraar.gmail.com/#t
> \ No newline at end of file

Note the "No newline at end of file" warnings, new files should have \n
at the end.

> diff --git a/contrib/iwyu/iwyu.sh b/contrib/iwyu/iwyu.sh
> new file mode 100755
> index 0000000000..3ef8639eae
> --- /dev/null
> +++ b/contrib/iwyu/iwyu.sh
> @@ -0,0 +1,2 @@
> +make $1 CC=include-what-you-use CFLAGS="-Xiwyu --verbose=1" 2>&1 \
> +| grep -v -E -e '^#include <' -e '^#include "(cache|git-compat-util|gettext)\.h"' 
> \ No newline at end of file

So that's just my one-line hack as-is (well, bisect.co replaced with $1)
:)

I think if we integrate some IWYU spport support that this is a rather
dead end to pursue.

A much better way would be to just do ("REAL_CC" is already understood
by sparse):

	REAL_CC=gcc make CC=contrib/iwyu.sh 

I.e. we shouldn't run make and then parse its output, but to have make
run a CC which wraps our REAL_CC.

Then you could either have it "really" compile, as e.g. the "sparse"
wrapper can do (note: note our sparse target but cgcc), or run whatever
"make" target, and then only having to parse the output of iwyu, not
iwyu+make.

Also, it looks like iwyu is a nicely API'd python library, so trying to
parse its output is probably a dead end too, can't we just ship a
trivial API-using script for it that gets all this as nicely
machine-readable data?

Or, if you look at iwyu.git its own source tree has a fix_includes.py
which seems to do that, *and* be able to "fix" the includes for you.

All of that is just stuff I didn't have time to poke at when posting a
quick reply in the original thread, but hopefully someone turning this
into a series of patches will :)
	
	$ python3 fix_includes.py -h
	Usage: fix_includes.py [options] [filename] ... < <output from include-what-you-use script>
	    OR fix_includes.py -s [other options] <filename> ...
	
	fix_includes.py reads the output from the include-what-you-use
	script on stdin -- run with --v=1 (default) verbose or above -- and,
	unless --sort_only or --dry_run is specified,
	modifies the files mentioned in the output, removing their old
	#include lines and replacing them with the lines given by the
	include_what_you_use script.  It also sorts the #include and
	forward-declare lines.
	

Re: [PATCH v2 2/4] bisect.c: remove unnecessary include

[Ævar Arnfjörð Bjarmason]

On Tue, Apr 05 2022, Garrit Franke wrote:

> Remove include "hash-lookup.h".

Like your 3/4 it would be nice to have a "orphaned since xyz", or was it
never used etc?

In these cases unlike most such C changes the compiler isn't of much use
for validation (we might be including this implicitly), so an
explanation saying why is helpful.

Re: [PATCH v2 0/4] various: use iwyu (include-what-you-use) to analyze includes

[Ævar Arnfjörð Bjarmason]

On Tue, Apr 05 2022, Garrit Franke wrote:

> On 01.04.22 10:07, Ævar Arnfjörð Bjarmason wrote:

Aside: I don't think I've ever seen encoded quoted-printable go quite so
bad so fast. That went from =C3=86var to =C3=83=C6=92=C3=A2=E2=82=AC in
one reply. Whatever your E-Mail is doing with encodings seems to be
taking multiple passes through misencodings :)

Don't worry about getting the name "right" or whatever, I'm amused by
the encoding issue...

>> ... For anyone interested in pursuing this, I think using the excellent
>> include-what-you-use tool would be a nice start.
>>
>> We could even eventually add it to our CI if the false positive rate
>> isn't bad (I haven't checked much):
>> https://github.com/include-what-you-use/include-what-you-use
>
> This seems to be a really nice tool indeed. I wouldn't be comfortable
> adding it to the CI just yet, but it did make it considerably easier to
> spot includes that could safely be removed.

Re the reply I had on 1/4 I think it's probably best to drop that in its
current form, but the fixes themselves (perhaps with a re-roll for nits
I posted in reply) seem good.

I was really hoping though that if someone wanted to pursue this a bit
more we'd get to the point of being able to run "make all test" on a
source tree that iwyu would munge with all its suggestions, and then see
if it outright failed to compile, or whether it would e.g. have faster
compilation speed (or not..).

> I think we could try battle-testing this tool in the codebase to get a
> sense of how it behaves. To start, I added your reference-command to a
> script under "contrib/iwyu" and ran it against the files you noted.
> Before breaking a bulk of the files, I wanted to make sure that this
> undertaking is headed in the right direction.

Even if the patches aren't sent in making the actual changes a one-off
script to e.g. wrap the fix_includes.py script I mentioned would be very
interesting.

We could then even run that in CI with relatively little setup,
i.e. checkout <rev>, do munging, then compile.

Re: [PATCH v2 2/4] bisect.c: remove unnecessary include

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> On Tue, Apr 05 2022, Garrit Franke wrote:
>
>> Remove include "hash-lookup.h".
>
> Like your 3/4 it would be nice to have a "orphaned since xyz", or was it
> never used etc?
>
> In these cases unlike most such C changes the compiler isn't of much use
> for validation (we might be including this implicitly), so an
> explanation saying why is helpful.

Thanks for pointing it out.  I found the lack of explanation in some
but not all of the changes disturbing.