patchxdiff: avoid accidental redefinition of LFS feature in OpenIndiana

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(-)

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:

s/where/are/

Otherwise the patch looks fine to me, thanks, Johannes

On Mon, Jun 17, 2019 at 11:45:37PM -0700, Carlo Marcelo Arenas Belón 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.

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

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(-)

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(-)

On Tue, Jun 18, 2019 at 8:53 AM Jeff King <peff@peff.net> wrote:

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.

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?

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

That's be fine with me. Thanks.

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

Jeff King <peff@peff.net> writes:

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.

On Wed, Jul 3, 2019 at 11:49 AM Junio C Hamano <gitster@pobox.com> wrote:

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

Carlo

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(-)

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(-)

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(-)

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(-)

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(-)

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(-)

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

Thanks, these all look good to me.

-Peff

Jeff King <peff@peff.net> writes:

Thanks, both.  They look good to me too.