Re: [PATCH] Makefile: make NO_ICONV really mean "no iconv"

2018-06-18 Thread Junio C Hamano
Eric Sunshine writes: > On Sun, Jun 17, 2018 at 1:32 PM Kaartic Sivaraam > wrote: >> On Friday 15 June 2018 01:13 PM, Eric Sunshine wrote: >> > On Fri, Jun 15, 2018 at 2:58 AM Simon Ruderich wrote: >> >> Should we put the part about MacOS's make into the commit >> >> message? Seems like

Re: Doc/SubmittingPatches: re-phrashing a sentence about alternate solutions (was Re: [PATCH] Makefile: make NO_ICONV really mean "no iconv")

2018-06-17 Thread Jeff King
On Sun, Jun 17, 2018 at 11:55:33PM +0530, Kaartic Sivaraam wrote: > On Sun, 2018-06-17 at 14:00 -0400, Eric Sunshine wrote: > > Whether or not to talk about alternate solutions in the commit message > > is a judgment call. Same for deciding what belongs in the commit > > message proper and what

Doc/SubmittingPatches: re-phrashing a sentence about alternate solutions (was Re: [PATCH] Makefile: make NO_ICONV really mean "no iconv")

2018-06-17 Thread Kaartic Sivaraam
On Sun, 2018-06-17 at 14:00 -0400, Eric Sunshine wrote: > Whether or not to talk about alternate solutions in the commit message > is a judgment call. Same for deciding what belongs in the commit > message proper and what belongs in the "commentary" section of a > patch. A patch author should

Re: [PATCH] Makefile: make NO_ICONV really mean "no iconv"

2018-06-17 Thread Eric Sunshine
On Sun, Jun 17, 2018 at 1:32 PM Kaartic Sivaraam wrote: > On Friday 15 June 2018 01:13 PM, Eric Sunshine wrote: > > On Fri, Jun 15, 2018 at 2:58 AM Simon Ruderich wrote: > >> Should we put the part about MacOS's make into the commit > >> message? Seems like relevant information for future

Re: [PATCH] Makefile: make NO_ICONV really mean "no iconv"

2018-06-17 Thread Kaartic Sivaraam
On Friday 15 June 2018 01:13 PM, Eric Sunshine wrote: > On Fri, Jun 15, 2018 at 2:58 AM Simon Ruderich wrote: >> On Thu, Jun 14, 2018 at 10:25:03PM -0400, Eric Sunshine wrote: >>> This patch is extra noisy due to the indentation change. Viewing it with >>> "git diff -w" helps. An alternative to

Re: [PATCH] Makefile: make NO_ICONV really mean "no iconv"

2018-06-15 Thread Mahmoud Al-Qudsi
On Thu, Jun 14, 2018 at 9:25 PM, Eric Sunshine wrote: > The Makefile tweak NO_ICONV is meant to allow Git to be built without > iconv in case iconv is not installed or is otherwise dysfunctional. > However, NO_ICONV's disabling of iconv is incomplete and can incorrectly > allow "-liconv" to slip

Re: [PATCH] Makefile: make NO_ICONV really mean "no iconv"

2018-06-15 Thread Eric Sunshine
On Fri, Jun 15, 2018 at 2:58 AM Simon Ruderich wrote: > On Thu, Jun 14, 2018 at 10:25:03PM -0400, Eric Sunshine wrote: > > This patch is extra noisy due to the indentation change. Viewing it with > > "git diff -w" helps. An alternative to re-indenting would have been to > > "undefine

Re: [PATCH] Makefile: make NO_ICONV really mean "no iconv"

2018-06-15 Thread Simon Ruderich
On Thu, Jun 14, 2018 at 10:25:03PM -0400, Eric Sunshine wrote: > The Makefile tweak NO_ICONV is meant to allow Git to be built without > iconv in case iconv is not installed or is otherwise dysfunctional. > However, NO_ICONV's disabling of iconv is incomplete and can incorrectly > allow "-liconv"

Re: [PATCH] Makefile: make NO_ICONV really mean "no iconv"

2018-06-15 Thread Jeff King
On Fri, Jun 15, 2018 at 02:30:43AM -0400, Eric Sunshine wrote: > On Fri, Jun 15, 2018 at 12:20 AM Jeff King wrote: > > We have OLD_ICONV, too, which should probably do nothing if NO_ICONV is > > set. I think that works OK. We end up setting -DOLD_ICONV on the command > > line, but that's only

Re: [PATCH] Makefile: make NO_ICONV really mean "no iconv"

2018-06-15 Thread Eric Sunshine
On Fri, Jun 15, 2018 at 12:20 AM Jeff King wrote: > We have OLD_ICONV, too, which should probably do nothing if NO_ICONV is > set. I think that works OK. We end up setting -DOLD_ICONV on the command > line, but that's only consider inside "#ifndef NO_ICONV" within the > code. Right, that was my

Re: [PATCH] Makefile: make NO_ICONV really mean "no iconv"

2018-06-14 Thread Jeff King
On Thu, Jun 14, 2018 at 10:25:03PM -0400, Eric Sunshine wrote: > The Makefile tweak NO_ICONV is meant to allow Git to be built without > iconv in case iconv is not installed or is otherwise dysfunctional. > However, NO_ICONV's disabling of iconv is incomplete and can incorrectly > allow "-liconv"

[PATCH] Makefile: make NO_ICONV really mean "no iconv"

2018-06-14 Thread Eric Sunshine
The Makefile tweak NO_ICONV is meant to allow Git to be built without iconv in case iconv is not installed or is otherwise dysfunctional. However, NO_ICONV's disabling of iconv is incomplete and can incorrectly allow "-liconv" to slip into the linker flags when NEEDS_LIBICONV is defined, which