[PATCH] xdiff: avoid accidental redefinition of LFS feature in OpenIndiana

[PATCH] xdiff: avoid accidental redefinition of LFS feature in OpenIndiana

[Carlo Marcelo Arenas Belón]

after b46054b374 ("xdiff: use git-compat-util", 2019-04-11), two system
headers added in 2012 to xutils where no longer needed and could conflict
as shown below:

In file included from xdiff/xinclude.h:26:0,
                 from xdiff/xutils.c:25:
./git-compat-util.h:4:0: warning: "_FILE_OFFSET_BITS" redefined
 #define _FILE_OFFSET_BITS 64

In file included from /usr/include/limits.h:37:0,
                 from xdiff/xutils.c:23:
/usr/include/sys/feature_tests.h:231:0: note: this is the location of the previous definition
 #define _FILE_OFFSET_BITS 32

make sure git-compat-util.h is the first header (through xinclude.h)

Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
---
 xdiff/xutils.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/xdiff/xutils.c b/xdiff/xutils.c
index 963e1c58b9..cfa6e2220f 100644
--- a/xdiff/xutils.c
+++ b/xdiff/xutils.c
@@ -20,13 +20,9 @@
  *
  */
 
-#include <limits.h>
-#include <assert.h>
 #include "xinclude.h"
 
 
-
-
 long xdl_bogosqrt(long n) {
 	long i;
 
-- 
2.22.0

Re: [PATCH] xdiff: avoid accidental redefinition of LFS feature in OpenIndiana

[Johannes Schindelin]

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

Hi Carlo,

in the Git ecosystem, the abbreviation "LFS" does not mean what you think
it means... Let's not use the abbreviation for anything else.

On Mon, 17 Jun 2019, Carlo Marcelo Arenas Belón wrote:

> after b46054b374 ("xdiff: use git-compat-util", 2019-04-11), two system
> headers added in 2012 to xutils where no longer needed and could conflict

s/where/are/

Otherwise the patch looks fine to me, thanks,
Johannes

> as shown below:
>
> In file included from xdiff/xinclude.h:26:0,
>                  from xdiff/xutils.c:25:
> ./git-compat-util.h:4:0: warning: "_FILE_OFFSET_BITS" redefined
>  #define _FILE_OFFSET_BITS 64
>
> In file included from /usr/include/limits.h:37:0,
>                  from xdiff/xutils.c:23:
> /usr/include/sys/feature_tests.h:231:0: note: this is the location of the previous definition
>  #define _FILE_OFFSET_BITS 32
>
> make sure git-compat-util.h is the first header (through xinclude.h)
>
> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
> ---
>  xdiff/xutils.c | 4 ----
>  1 file changed, 4 deletions(-)
>
> diff --git a/xdiff/xutils.c b/xdiff/xutils.c
> index 963e1c58b9..cfa6e2220f 100644
> --- a/xdiff/xutils.c
> +++ b/xdiff/xutils.c
> @@ -20,13 +20,9 @@
>   *
>   */
>
> -#include <limits.h>
> -#include <assert.h>
>  #include "xinclude.h"
>
>
> -
> -
>  long xdl_bogosqrt(long n) {
>  	long i;
>
> --
> 2.22.0
>
>

[PATCH 0/2] a few more redundant system include cleanups

[Jeff King]

On Mon, Jun 17, 2019 at 11:45:37PM -0700, Carlo Marcelo Arenas Belón wrote:

> after b46054b374 ("xdiff: use git-compat-util", 2019-04-11), two system
> headers added in 2012 to xutils where no longer needed and could conflict
> as shown below:
> 
> In file included from xdiff/xinclude.h:26:0,
>                  from xdiff/xutils.c:25:
> ./git-compat-util.h:4:0: warning: "_FILE_OFFSET_BITS" redefined
>  #define _FILE_OFFSET_BITS 64
> 
> In file included from /usr/include/limits.h:37:0,
>                  from xdiff/xutils.c:23:
> /usr/include/sys/feature_tests.h:231:0: note: this is the location of the previous definition
>  #define _FILE_OFFSET_BITS 32
> 
> make sure git-compat-util.h is the first header (through xinclude.h)

Thanks, this looks good to me. I fixed the system headers in xinclude.h,
but didn't think to check for individual C files including them.

I did a quick grep for similar cases, and didn't find any that I think
would be problematic. There were a few cleanups, below.

There are still some other system includes, but they are mostly one-offs
that we wouldn't expect git-compat-util to cover (e.g., threading stuff
in thread-utils.[ch], openssl stuff in hash.h). So I think with your
patch, plus the patches below, there's nothing left to clean up.

  [1/2]: verify-tag: drop signal.h include
  [2/2]: wt-status.h: drop stdio.h include

 builtin/verify-commit.c | 1 -
 builtin/verify-tag.c    | 1 -
 wt-status.h             | 1 -
 3 files changed, 3 deletions(-)

-Peff

[PATCH 1/2] verify-tag: drop signal.h include

[Jeff King]

There's no reason verify-tag.c needs to include signal.h. It's already
in git-compat-util.h, which we properly include as the first header.
And there doesn't seem to be a particular reason for this include; it's
just an artifact from the file creation in 2ae68fcb78 (Make verify-tag a
builtin., 2007-07-27).

Likewise verify-commit.c has the same issue, probably because it was
created using verify-tag as a template in d07b00b7f3 (verify-commit:
scriptable commit signature verification, 2014-06-23).

These includes are probably just redundant, and not hurting anything by
circumventing the order that git-compat-util.h tries to impose, since
we'll always have loaded git-compat-util by the time we get to these. So
this is just a cleanup, and shouldn't fix or break any platforms.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/verify-commit.c | 1 -
 builtin/verify-tag.c    | 1 -
 2 files changed, 2 deletions(-)

diff --git a/builtin/verify-commit.c b/builtin/verify-commit.c
index 4b9e823f8f..40c69a0bed 100644
--- a/builtin/verify-commit.c
+++ b/builtin/verify-commit.c
@@ -12,7 +12,6 @@
 #include "repository.h"
 #include "commit.h"
 #include "run-command.h"
-#include <signal.h>
 #include "parse-options.h"
 #include "gpg-interface.h"
 
diff --git a/builtin/verify-tag.c b/builtin/verify-tag.c
index 6fa04b751a..f45136a06b 100644
--- a/builtin/verify-tag.c
+++ b/builtin/verify-tag.c
@@ -10,7 +10,6 @@
 #include "builtin.h"
 #include "tag.h"
 #include "run-command.h"
-#include <signal.h>
 #include "parse-options.h"
 #include "gpg-interface.h"
 #include "ref-filter.h"
-- 
2.22.0.rc3.685.g5185838c9a

[PATCH 2/2] wt-status.h: drop stdio.h include

[Jeff King]

We started including stdio.h to pick up the declaration of "FILE" in
f26a001226 (Enable wt-status output to a given FILE pointer.,
2007-09-17). But there's no need, since headers can assume that
git-compat-util.h has been included, which covers stdio.

This should just be redundant, and not hurting anything (like pulling in
includes out of order) because C files are supposed to always include
git-compat-util.h first. But it's worth cleaning up to model good
behavior.

Signed-off-by: Jeff King <peff@peff.net>
---
 wt-status.h | 1 -
 1 file changed, 1 deletion(-)

diff --git a/wt-status.h b/wt-status.h
index 64f1ddc9fd..8849768e92 100644
--- a/wt-status.h
+++ b/wt-status.h
@@ -1,7 +1,6 @@
 #ifndef STATUS_H
 #define STATUS_H
 
-#include <stdio.h>
 #include "string-list.h"
 #include "color.h"
 #include "pathspec.h"
-- 
2.22.0.rc3.685.g5185838c9a

Re: [PATCH 0/2] a few more redundant system include cleanups

[Carlo Arenas]

On Tue, Jun 18, 2019 at 8:53 AM Jeff King <peff@peff.net> wrote:
>
> Thanks, this looks good to me. I fixed the system headers in xinclude.h,
> but didn't think to check for individual C files including them.

also noticed that after the refactoring some files will include a couple
of headers twice (not an issue, though thanks to the header guards)
but didn't send that patch to reduce the churn and avoid confusion.

> I did a quick grep for similar cases, and didn't find any that I think
> would be problematic. There were a few cleanups, below.

would you mind if I add your 2 patches to a series and include that
missing one?, that way I'll also have a chance to write a better commit
message from my original change, including the other feedback I got
as well

Carlo

PS. is there a recommendation on how to version a patch that move
into a series that would be preferred?

Re: [PATCH 0/2] a few more redundant system include cleanups

[Jeff King]

On Wed, Jun 19, 2019 at 02:12:30AM -0700, Carlo Arenas wrote:

> > I did a quick grep for similar cases, and didn't find any that I think
> > would be problematic. There were a few cleanups, below.
> 
> would you mind if I add your 2 patches to a series and include that
> missing one?, that way I'll also have a chance to write a better commit
> message from my original change, including the other feedback I got
> as well

That's be fine with me. Thanks.

> PS. is there a recommendation on how to version a patch that move
> into a series that would be preferred?

I think you can just say "v2" on the whole thing, include all three
patches, and then write a note that the series supersedes what I sent.

-Peff

Re: [PATCH 0/2] a few more redundant system include cleanups

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Wed, Jun 19, 2019 at 02:12:30AM -0700, Carlo Arenas wrote:
>
>> > I did a quick grep for similar cases, and didn't find any that I think
>> > would be problematic. There were a few cleanups, below.
>> 
>> would you mind if I add your 2 patches to a series and include that
>> missing one?, that way I'll also have a chance to write a better commit
>> message from my original change, including the other feedback I got
>> as well
>
> That's be fine with me. Thanks.
>
>> PS. is there a recommendation on how to version a patch that move
>> into a series that would be preferred?
>
> I think you can just say "v2" on the whole thing, include all three
> patches, and then write a note that the series supersedes what I sent.

Carlo, no need to rush, but has this happened already?  I do not
recall seeing an update, and I am wondering if I missed one.

Thanks.

Re: [PATCH 0/2] a few more redundant system include cleanups

[Carlo Arenas]

On Wed, Jul 3, 2019 at 11:49 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Carlo, no need to rush, but has this happened already?  I do not
> recall seeing an update, and I am wondering if I missed one.

Junio; had yet to send the update, so nothing was missed

Carlo

[PATCH v2 0/5] system header cleanup

[Carlo Marcelo Arenas Belón]

This series is a reroll of jk/no-system-includes-in-dot-c with
cb/xdiff-no-system-includes-in-dot-c applied on top with minor
fixes for the commit message based on feedback from Johannes
and the example put forward by Peff with his own patches.

The changes proposed shouldn't affect any systems (except for
the 3rd one) and that has since shown to also be needed
in Alpine Linux (because of _XOPEN_SOURCE redefinition).

The last 2 patches are new to the series and just cleanup
the dependency list in xdiff.

Carlo Marcelo Arenas Belón (3):
  xdiff: drop system includes in xutils.c
  xdiff: remove duplicate headers from xhistogram.c
  xdiff: remove duplicate headers from xpatience.c

Jeff King (2):
  verify-tag: drop signal.h include
  wt-status.h: drop stdio.h include

 builtin/verify-commit.c | 1 -
 builtin/verify-tag.c    | 1 -
 wt-status.h             | 1 -
 xdiff/xhistogram.c      | 2 --
 xdiff/xpatience.c       | 2 --
 xdiff/xutils.c          | 4 ----
 6 files changed, 11 deletions(-)

-- 
2.22.0

[PATCH v2 1/5] verify-tag: drop signal.h include

[Carlo Marcelo Arenas Belón]

From: Jeff King <peff@peff.net>

There's no reason verify-tag.c needs to include signal.h. It's already
in git-compat-util.h, which we properly include as the first header.
And there doesn't seem to be a particular reason for this include; it's
just an artifact from the file creation in 2ae68fcb78 (Make verify-tag a
builtin., 2007-07-27).

Likewise verify-commit.c has the same issue, probably because it was
created using verify-tag as a template in d07b00b7f3 (verify-commit:
scriptable commit signature verification, 2014-06-23).

These includes are probably just redundant, and not hurting anything by
circumventing the order that git-compat-util.h tries to impose, since
we'll always have loaded git-compat-util by the time we get to these. So
this is just a cleanup, and shouldn't fix or break any platforms.

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/verify-commit.c | 1 -
 builtin/verify-tag.c    | 1 -
 2 files changed, 2 deletions(-)

diff --git a/builtin/verify-commit.c b/builtin/verify-commit.c
index 7772c07ed7..4e93914e59 100644
--- a/builtin/verify-commit.c
+++ b/builtin/verify-commit.c
@@ -12,7 +12,6 @@
 #include "repository.h"
 #include "commit.h"
 #include "run-command.h"
-#include <signal.h>
 #include "parse-options.h"
 #include "gpg-interface.h"
 
diff --git a/builtin/verify-tag.c b/builtin/verify-tag.c
index 6fa04b751a..f45136a06b 100644
--- a/builtin/verify-tag.c
+++ b/builtin/verify-tag.c
@@ -10,7 +10,6 @@
 #include "builtin.h"
 #include "tag.h"
 #include "run-command.h"
-#include <signal.h>
 #include "parse-options.h"
 #include "gpg-interface.h"
 #include "ref-filter.h"
-- 
2.22.0

[PATCH v2 2/5] wt-status.h: drop stdio.h include

[Carlo Marcelo Arenas Belón]

From: Jeff King <peff@peff.net>

We started including stdio.h to pick up the declaration of "FILE" in
f26a001226 (Enable wt-status output to a given FILE pointer.,
2007-09-17). But there's no need, since headers can assume that
git-compat-util.h has been included, which covers stdio.

This should just be redundant, and not hurting anything (like pulling in
includes out of order) because C files are supposed to always include
git-compat-util.h first. But it's worth cleaning up to model good
behavior.

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 wt-status.h | 1 -
 1 file changed, 1 deletion(-)

diff --git a/wt-status.h b/wt-status.h
index 64f1ddc9fd..8849768e92 100644
--- a/wt-status.h
+++ b/wt-status.h
@@ -1,7 +1,6 @@
 #ifndef STATUS_H
 #define STATUS_H
 
-#include <stdio.h>
 #include "string-list.h"
 #include "color.h"
 #include "pathspec.h"
-- 
2.22.0

[PATCH v2 3/5] xdiff: drop system includes in xutils.c

[Carlo Marcelo Arenas Belón]

After b46054b374 ("xdiff: use git-compat-util", 2019-04-11), two system
headers added with 6942efcfa9 ("xdiff: load full words in the inner loop
of xdl_hash_record", 2012-04-06) to xutils.c are no longer needed and
could conflict as shown below from an OpenIndiana build:

In file included from xdiff/xinclude.h:26:0,
                 from xdiff/xutils.c:25:
./git-compat-util.h:4:0: warning: "_FILE_OFFSET_BITS" redefined
 #define _FILE_OFFSET_BITS 64

In file included from /usr/include/limits.h:37:0,
                 from xdiff/xutils.c:23:
/usr/include/sys/feature_tests.h:231:0: note: this is the location of the previous definition
 #define _FILE_OFFSET_BITS 32

Make sure git-compat-util.h is the first header (through xinclude.h)

Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
V2: reword commit with feedback from Johannes

 xdiff/xutils.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/xdiff/xutils.c b/xdiff/xutils.c
index 963e1c58b9..cfa6e2220f 100644
--- a/xdiff/xutils.c
+++ b/xdiff/xutils.c
@@ -20,13 +20,9 @@
  *
  */
 
-#include <limits.h>
-#include <assert.h>
 #include "xinclude.h"
 
 
-
-
 long xdl_bogosqrt(long n) {
 	long i;
 
-- 
2.22.0

[PATCH v2 4/5] xdiff: remove duplicate headers from xhistogram.c

[Carlo Marcelo Arenas Belón]

8c912eea94 ("teach --histogram to diff", 2011-07-12) included them, but
were already part of xinclude.h

Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
---
 xdiff/xhistogram.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/xdiff/xhistogram.c b/xdiff/xhistogram.c
index ec85f5992b..c7b35a9667 100644
--- a/xdiff/xhistogram.c
+++ b/xdiff/xhistogram.c
@@ -42,8 +42,6 @@
  */
 
 #include "xinclude.h"
-#include "xtypes.h"
-#include "xdiff.h"
 
 #define MAX_PTR	UINT_MAX
 #define MAX_CNT	UINT_MAX
-- 
2.22.0

[PATCH v2 5/5] xdiff: remove duplicate headers from xpatience.c

[Carlo Marcelo Arenas Belón]

92b7de93fb (Implement the patience diff algorithm, 2009-01-07) added them
but were already part of xinclude.h

Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
---
 xdiff/xpatience.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/xdiff/xpatience.c b/xdiff/xpatience.c
index f3573d9f00..3c5601b602 100644
--- a/xdiff/xpatience.c
+++ b/xdiff/xpatience.c
@@ -20,8 +20,6 @@
  *
  */
 #include "xinclude.h"
-#include "xtypes.h"
-#include "xdiff.h"
 
 /*
  * The basic idea of patience diff is to find lines that are unique in
-- 
2.22.0

Re: [PATCH v2 0/5] system header cleanup

[Jeff King]

On Sun, Jul 28, 2019 at 01:07:19PM -0700, Carlo Marcelo Arenas Belón wrote:

> This series is a reroll of jk/no-system-includes-in-dot-c with
> cb/xdiff-no-system-includes-in-dot-c applied on top with minor
> fixes for the commit message based on feedback from Johannes
> and the example put forward by Peff with his own patches.
> 
> The changes proposed shouldn't affect any systems (except for
> the 3rd one) and that has since shown to also be needed
> in Alpine Linux (because of _XOPEN_SOURCE redefinition).
> 
> The last 2 patches are new to the series and just cleanup
> the dependency list in xdiff.

Thanks, these all look good to me.

-Peff

Re: [PATCH v2 0/5] system header cleanup

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Sun, Jul 28, 2019 at 01:07:19PM -0700, Carlo Marcelo Arenas Belón wrote:
>
>> This series is a reroll of jk/no-system-includes-in-dot-c with
>> cb/xdiff-no-system-includes-in-dot-c applied on top with minor
>> fixes for the commit message based on feedback from Johannes
>> and the example put forward by Peff with his own patches.
>> 
>> The changes proposed shouldn't affect any systems (except for
>> the 3rd one) and that has since shown to also be needed
>> in Alpine Linux (because of _XOPEN_SOURCE redefinition).
>> 
>> The last 2 patches are new to the series and just cleanup
>> the dependency list in xdiff.
>
> Thanks, these all look good to me.

Thanks, both.  They look good to me too.