* Is NO_ICONV misnamed or is it broken? @ 2018-06-14 22:47 Mahmoud Al-Qudsi 2018-06-15 2:25 ` [PATCH] Makefile: make NO_ICONV really mean "no iconv" Eric Sunshine 2018-06-17 2:57 ` Is NO_ICONV misnamed or is it broken? Christian Couder 0 siblings, 2 replies; 15+ messages in thread From: Mahmoud Al-Qudsi @ 2018-06-14 22:47 UTC (permalink / raw) To: git Hello list, With regards to the Makefile define/variable `NO_ICONV` - the Makefile comments imply that it should be used if "your libc doesn't properly support iconv," which could mean anything from "a patch will be applied" to "iconv won't be used." Based off the name of the varibale, the assumption is that iconv is an optional dependency that can be omitted if compiled with NO_ICONV. However, in practice attempting to compile git with `make ... NO_ICONV=1` and libiconv not installed results in linker errors as follows: ``` ~> make clean # omitted ~> make NO_ICONV=1 # ommitted LINK git-credential-store /usr/bin/ld: cannot find -liconv cc: error: linker command failed with exit code 1 (use -v to see invocation) gmake: *** [Makefile:2327: git-credential-store] Error 1 ``` Am I misunderstanding the intended behavior when NO_ICONV is defined (i.e. it does not remove the dependency on libiconv) or is this a bug and iconv should not, in fact, be required? Many thanks, Mahmoud Al-Qudsi NeoSmart Technologies ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH] Makefile: make NO_ICONV really mean "no iconv" 2018-06-14 22:47 Is NO_ICONV misnamed or is it broken? Mahmoud Al-Qudsi @ 2018-06-15 2:25 ` Eric Sunshine 2018-06-15 4:20 ` Jeff King ` (2 more replies) 2018-06-17 2:57 ` Is NO_ICONV misnamed or is it broken? Christian Couder 1 sibling, 3 replies; 15+ messages in thread From: Eric Sunshine @ 2018-06-15 2:25 UTC (permalink / raw) To: git; +Cc: Mahmoud Al-Qudsi, 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 breaks the build when iconv is not installed. On some platforms, iconv lives directly in libc, whereas, on others it resides in libiconv. For the latter case, NEEDS_LIBICONV instructs the Makefile to add "-liconv" to the linker flags. config.mak.uname automatically defines NEEDS_LIBICONV for platforms which require it. The adding of "-liconv" is done unconditionally, despite NO_ICONV. Work around this problem by making NO_ICONV take precedence over NEEDS_LIBICONV. Reported by: Mahmoud Al-Qudsi <mqudsi@neosmart.net> Signed-off-by: Eric Sunshine <sunshine@sunshineco.com> --- 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 NEEDS_LIBICONV", however, 'undefine' was added to GNU make in 3.82 but MacOS is stuck on 3.81 (from 2006) so 'undefine' was avoided. Reported here: https://public-inbox.org/git/CACcTrKePbgyCbXneN5NZ+cS-tiDyYe_dkdwttXpP0CUeEicvHw@mail.gmail.com/T/#u Makefile | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/Makefile b/Makefile index 1d27f36365..e4b503d259 100644 --- a/Makefile +++ b/Makefile @@ -1351,17 +1351,19 @@ ifdef APPLE_COMMON_CRYPTO LIB_4_CRYPTO += -framework Security -framework CoreFoundation endif endif -ifdef NEEDS_LIBICONV - ifdef ICONVDIR - BASIC_CFLAGS += -I$(ICONVDIR)/include - ICONV_LINK = -L$(ICONVDIR)/$(lib) $(CC_LD_DYNPATH)$(ICONVDIR)/$(lib) - else - ICONV_LINK = - endif - ifdef NEEDS_LIBINTL_BEFORE_LIBICONV - ICONV_LINK += -lintl +ifndef NO_ICONV + ifdef NEEDS_LIBICONV + ifdef ICONVDIR + BASIC_CFLAGS += -I$(ICONVDIR)/include + ICONV_LINK = -L$(ICONVDIR)/$(lib) $(CC_LD_DYNPATH)$(ICONVDIR)/$(lib) + else + ICONV_LINK = + endif + ifdef NEEDS_LIBINTL_BEFORE_LIBICONV + ICONV_LINK += -lintl + endif + EXTLIBS += $(ICONV_LINK) -liconv endif - EXTLIBS += $(ICONV_LINK) -liconv endif ifdef NEEDS_LIBGEN EXTLIBS += -lgen -- 2.18.0.rc1.256.g331a1db143 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH] Makefile: make NO_ICONV really mean "no iconv" 2018-06-15 2:25 ` [PATCH] Makefile: make NO_ICONV really mean "no iconv" Eric Sunshine @ 2018-06-15 4:20 ` Jeff King 2018-06-15 6:30 ` Eric Sunshine 2018-06-15 6:58 ` Simon Ruderich 2018-06-15 8:15 ` Mahmoud Al-Qudsi 2 siblings, 1 reply; 15+ messages in thread From: Jeff King @ 2018-06-15 4:20 UTC (permalink / raw) To: Eric Sunshine; +Cc: git, Mahmoud Al-Qudsi 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" to slip into the linker flags when NEEDS_LIBICONV is > defined, which breaks the build when iconv is not installed. > > On some platforms, iconv lives directly in libc, whereas, on others it > resides in libiconv. For the latter case, NEEDS_LIBICONV instructs the > Makefile to add "-liconv" to the linker flags. config.mak.uname > automatically defines NEEDS_LIBICONV for platforms which require it. > The adding of "-liconv" is done unconditionally, despite NO_ICONV. > > Work around this problem by making NO_ICONV take precedence over > NEEDS_LIBICONV. Nicely explained. 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. > 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 NEEDS_LIBICONV", however, 'undefine' was added to GNU make in > 3.82 but MacOS is stuck on 3.81 (from 2006) so 'undefine' was avoided. Yeah, with "-w" it looks pretty obviously correct. -Peff ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] Makefile: make NO_ICONV really mean "no iconv" 2018-06-15 4:20 ` Jeff King @ 2018-06-15 6:30 ` Eric Sunshine 2018-06-15 6:39 ` Jeff King 0 siblings, 1 reply; 15+ messages in thread From: Eric Sunshine @ 2018-06-15 6:30 UTC (permalink / raw) To: Jeff King; +Cc: Git List, Mahmoud Al-Qudsi On Fri, Jun 15, 2018 at 12:20 AM Jeff King <peff@peff.net> 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 conclusion, as well. Since it works as is, I'm not sure suppressing -DOLD_ICONV in Makefile is worth the extra patch noise. I can re-roll with that change too, if someone thinks it's worthwhile, though. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] Makefile: make NO_ICONV really mean "no iconv" 2018-06-15 6:30 ` Eric Sunshine @ 2018-06-15 6:39 ` Jeff King 0 siblings, 0 replies; 15+ messages in thread From: Jeff King @ 2018-06-15 6:39 UTC (permalink / raw) To: Eric Sunshine; +Cc: Git List, Mahmoud Al-Qudsi On Fri, Jun 15, 2018 at 02:30:43AM -0400, Eric Sunshine wrote: > On Fri, Jun 15, 2018 at 12:20 AM Jeff King <peff@peff.net> 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 conclusion, as well. Since it works as is, I'm not > sure suppressing -DOLD_ICONV in Makefile is worth the extra patch > noise. I can re-roll with that change too, if someone thinks it's > worthwhile, though. Nah, I was just thinking out loud. I don't think it's worth changing the Makefile (and I wouldn't be surprised if there are other "dependent" cases like this that work just fine because of the #ifdefs in the code). -Peff ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] Makefile: make NO_ICONV really mean "no iconv" 2018-06-15 2:25 ` [PATCH] Makefile: make NO_ICONV really mean "no iconv" Eric Sunshine 2018-06-15 4:20 ` Jeff King @ 2018-06-15 6:58 ` Simon Ruderich 2018-06-15 7:43 ` Eric Sunshine 2018-06-15 8:15 ` Mahmoud Al-Qudsi 2 siblings, 1 reply; 15+ messages in thread From: Simon Ruderich @ 2018-06-15 6:58 UTC (permalink / raw) To: Eric Sunshine; +Cc: git, Mahmoud Al-Qudsi 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" to slip into the linker flags when NEEDS_LIBICONV is > defined, which breaks the build when iconv is not installed. > > On some platforms, iconv lives directly in libc, whereas, on others it > resides in libiconv. For the latter case, NEEDS_LIBICONV instructs the > Makefile to add "-liconv" to the linker flags. config.mak.uname > automatically defines NEEDS_LIBICONV for platforms which require it. > The adding of "-liconv" is done unconditionally, despite NO_ICONV. > > Work around this problem by making NO_ICONV take precedence over > NEEDS_LIBICONV. > > Reported by: Mahmoud Al-Qudsi <mqudsi@neosmart.net> > Signed-off-by: Eric Sunshine <sunshine@sunshineco.com> > --- > > 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 NEEDS_LIBICONV", however, 'undefine' was added to GNU make in > 3.82 but MacOS is stuck on 3.81 (from 2006) so 'undefine' was avoided. Should we put the part about MacOS's make into the commit message? Seems like relevant information for future readers. Regards Simon -- + privacy is necessary + using gnupg http://gnupg.org + public key id: 0x92FEFDB7E44C32F9 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] Makefile: make NO_ICONV really mean "no iconv" 2018-06-15 6:58 ` Simon Ruderich @ 2018-06-15 7:43 ` Eric Sunshine 2018-06-17 17:32 ` Kaartic Sivaraam 0 siblings, 1 reply; 15+ messages in thread From: Eric Sunshine @ 2018-06-15 7:43 UTC (permalink / raw) To: simon; +Cc: Git List, Mahmoud Al-Qudsi On Fri, Jun 15, 2018 at 2:58 AM Simon Ruderich <simon@ruderich.org> 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 NEEDS_LIBICONV", however, 'undefine' was added to GNU make in > > 3.82 but MacOS is stuck on 3.81 (from 2006) so 'undefine' was avoided. > > Should we put the part about MacOS's make into the commit > message? Seems like relevant information for future readers. No. The bit of commentary mentioning MacOS's very old 'make' was just talking about a possible alternate way of implementing the change. That alternative was not chosen, so talking about old 'make' in the commit message would be confusing for readers. More importantly, although that alternative would have made a less noisy patch, the actual result would have made the Makefile itself noisier and uglier, particularly for people just reading the Makefile in the future, people who did not read the patch. Specifically, these alternatives were considered: ifdef NO_ICONV undefine NEEDS_LIBICONV endif ifdef NEEDS_LIBICONV ...as before... endif and: ifdef NO_ICONV NEEDS_LIBICONV= endif ifeq ($(NEEDS_LIBICONV),) ...as before... endif Both of which are uglier for a future reader of Makefile than the end-result actually implemented by this patch: ifndef NO_ICONV ifdef NEEDS_LIBICONV ...as before... endif endif ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] Makefile: make NO_ICONV really mean "no iconv" 2018-06-15 7:43 ` Eric Sunshine @ 2018-06-17 17:32 ` Kaartic Sivaraam 2018-06-17 18:00 ` Eric Sunshine 0 siblings, 1 reply; 15+ messages in thread From: Kaartic Sivaraam @ 2018-06-17 17:32 UTC (permalink / raw) To: Eric Sunshine, simon; +Cc: Git List, Mahmoud Al-Qudsi [-- Attachment #1.1: Type: text/plain, Size: 2296 bytes --] On Friday 15 June 2018 01:13 PM, Eric Sunshine wrote: > On Fri, Jun 15, 2018 at 2:58 AM Simon Ruderich <simon@ruderich.org> 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 NEEDS_LIBICONV", however, 'undefine' was added to GNU make in >>> 3.82 but MacOS is stuck on 3.81 (from 2006) so 'undefine' was avoided. >> >> Should we put the part about MacOS's make into the commit >> message? Seems like relevant information for future readers. > > No. The bit of commentary mentioning MacOS's very old 'make' was just > talking about a possible alternate way of implementing the change. > That alternative was not chosen, so talking about old 'make' in the > commit message would be confusing for readers. Interesting. Documentation/SubmittinPatches reads: The body should provide a meaningful commit message, which: ... <snip> ... . alternate solutions considered but discarded, if any. The consensus has changed, maybe? In which case, should we remove that statement from there? -- Sivaraam QUOTE: “The three principal virtues of a programmer are Laziness, Impatience, and Hubris.” - Camel book Sivaraam? You possibly might have noticed that my signature recently changed from 'Kaartic' to 'Sivaraam' both of which are parts of my name. I find the new signature to be better for several reasons one of which is that the former signature has a lot of ambiguities in the place I live as it is a common name (NOTE: it's not a common spelling, just a common name). So, I switched signatures before it's too late. That said, I won't mind you calling me 'Kaartic' if you like it [of course ;-)]. You can always call me using either of the names. KIND NOTE TO THE NATIVE ENGLISH SPEAKER: As I'm not a native English speaker myself, there might be mistaeks in my usage of English. I apologise for any mistakes that I make. It would be "helpful" if you take the time to point out the mistakes. It would be "super helpful" if you could provide suggestions about how to correct those mistakes. Thanks in advance! [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] Makefile: make NO_ICONV really mean "no iconv" 2018-06-17 17:32 ` Kaartic Sivaraam @ 2018-06-17 18:00 ` Eric Sunshine 2018-06-17 18:25 ` Doc/SubmittingPatches: re-phrashing a sentence about alternate solutions (was Re: [PATCH] Makefile: make NO_ICONV really mean "no iconv") Kaartic Sivaraam 2018-06-18 16:09 ` [PATCH] Makefile: make NO_ICONV really mean "no iconv" Junio C Hamano 0 siblings, 2 replies; 15+ messages in thread From: Eric Sunshine @ 2018-06-17 18:00 UTC (permalink / raw) To: Kaartic Sivaraam; +Cc: simon, Git List, Mahmoud Al-Qudsi On Sun, Jun 17, 2018 at 1:32 PM Kaartic Sivaraam <kaartic.sivaraam@gmail.com> wrote: > On Friday 15 June 2018 01:13 PM, Eric Sunshine wrote: > > On Fri, Jun 15, 2018 at 2:58 AM Simon Ruderich <simon@ruderich.org> wrote: > >> Should we put the part about MacOS's make into the commit > >> message? Seems like relevant information for future readers. > > > > No. The bit of commentary mentioning MacOS's very old 'make' was just > > talking about a possible alternate way of implementing the change. > > That alternative was not chosen, so talking about old 'make' in the > > commit message would be confusing for readers. > > Interesting. Documentation/SubmittinPatches reads: > > The body should provide a meaningful commit message, which: > <snip> > . alternate solutions considered but discarded, if any. > > The consensus has changed, maybe? In which case, should we remove that > statement from there? 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 strive to convey the problem succinctly in the commit message, to not overload the reader with unnecessary (or confusing) information, while, at the same time, not be sparing with information which is genuinely needed to understand the problem and solution. Often, this can be done without talking about alternatives; often even without spelling out the solution in detail or at all since the solution may be "obvious", given a well-written problem description. Complex cases, or cases in which multiple solutions may be or seem valid, on the other hand, might warrant talking about those alternate solutions, so we probably don't want to drop that bullet point. Perhaps, instead, it can be re-worded a bit to make it sound something other than mandatory (but I can't think of a good way to phrase it; maybe you can?). ^ permalink raw reply [flat|nested] 15+ messages in thread
* Doc/SubmittingPatches: re-phrashing a sentence about alternate solutions (was Re: [PATCH] Makefile: make NO_ICONV really mean "no iconv") 2018-06-17 18:00 ` Eric Sunshine @ 2018-06-17 18:25 ` Kaartic Sivaraam 2018-06-18 4:20 ` Jeff King 2018-06-18 16:09 ` [PATCH] Makefile: make NO_ICONV really mean "no iconv" Junio C Hamano 1 sibling, 1 reply; 15+ messages in thread From: Kaartic Sivaraam @ 2018-06-17 18:25 UTC (permalink / raw) To: Eric Sunshine; +Cc: simon, Git List, Mahmoud Al-Qudsi 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 strive to convey the problem succinctly > in the commit message, to not overload the reader with unnecessary (or > confusing) information, while, at the same time, not be sparing with > information which is genuinely needed to understand the problem and > solution. > > Often, this can be done without talking about alternatives; often even > without spelling out the solution in detail or at all since the > solution may be "obvious", given a well-written problem description. > Complex cases, or cases in which multiple solutions may be or seem > valid, on the other hand, might warrant talking about those alternate > solutions, so we probably don't want to drop that bullet point. Well explained, thanks. (Thinking out loud, it might be even nice to including the above paragraphs into Documentation/SubmittingPatches as I find it to be more "humane" than the terse bullets. But I refrained from doing so as the document is already a bit too-long ;-) > Perhaps, instead, it can be re-worded a bit to make it sound something > other than mandatory (but I can't think of a good way to phrase it; > maybe you can?). How about the following patch? (warning: patch only for discussion purposes, might be white-space broken). It might be superfluous, though. diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches index a1d0feca3..565bc4397 100644 --- a/Documentation/SubmittingPatches +++ b/Documentation/SubmittingPatches @@ -128,7 +128,7 @@ The body should provide a meaningful commit message, which: . justifies the way the change solves the problem, i.e. why the result with the change is better. -. alternate solutions considered but discarded, if any. +. alternate solutions considered but discarded, where necessary. [[imperative-mood]] Describe your changes in imperative mood, e.g. "make xyzzy do frotz" Regards, Sivaraam ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: Doc/SubmittingPatches: re-phrashing a sentence about alternate solutions (was Re: [PATCH] Makefile: make NO_ICONV really mean "no iconv") 2018-06-17 18:25 ` Doc/SubmittingPatches: re-phrashing a sentence about alternate solutions (was Re: [PATCH] Makefile: make NO_ICONV really mean "no iconv") Kaartic Sivaraam @ 2018-06-18 4:20 ` Jeff King 0 siblings, 0 replies; 15+ messages in thread From: Jeff King @ 2018-06-18 4:20 UTC (permalink / raw) To: Kaartic Sivaraam; +Cc: Eric Sunshine, simon, Git List, Mahmoud Al-Qudsi 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 belongs in the "commentary" section of a > > patch. A patch author should strive to convey the problem succinctly > > in the commit message, to not overload the reader with unnecessary (or > > confusing) information, while, at the same time, not be sparing with > > information which is genuinely needed to understand the problem and > > solution. > > > > Often, this can be done without talking about alternatives; often even > > without spelling out the solution in detail or at all since the > > solution may be "obvious", given a well-written problem description. > > Complex cases, or cases in which multiple solutions may be or seem > > valid, on the other hand, might warrant talking about those alternate > > solutions, so we probably don't want to drop that bullet point. > > Well explained, thanks. (Thinking out loud, it might be even nice to > including the above paragraphs into Documentation/SubmittingPatches as > I find it to be more "humane" than the terse bullets. But I refrained > from doing so as the document is already a bit too-long ;-) Yes, the first paragraph especially. The _most_ important thing is writing well with consideration for your readers. All the other rules are really guidelines to help you remember how to do that. When in doubt follow the guidelines, but it's OK to break them if it serves the ultimate purpose. All IMHO, of course. :) -Peff ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] Makefile: make NO_ICONV really mean "no iconv" 2018-06-17 18:00 ` Eric Sunshine 2018-06-17 18:25 ` Doc/SubmittingPatches: re-phrashing a sentence about alternate solutions (was Re: [PATCH] Makefile: make NO_ICONV really mean "no iconv") Kaartic Sivaraam @ 2018-06-18 16:09 ` Junio C Hamano 1 sibling, 0 replies; 15+ messages in thread From: Junio C Hamano @ 2018-06-18 16:09 UTC (permalink / raw) To: Eric Sunshine; +Cc: Kaartic Sivaraam, simon, Git List, Mahmoud Al-Qudsi Eric Sunshine <sunshine@sunshineco.com> writes: > On Sun, Jun 17, 2018 at 1:32 PM Kaartic Sivaraam > <kaartic.sivaraam@gmail.com> wrote: >> On Friday 15 June 2018 01:13 PM, Eric Sunshine wrote: >> > On Fri, Jun 15, 2018 at 2:58 AM Simon Ruderich <simon@ruderich.org> wrote: >> >> Should we put the part about MacOS's make into the commit >> >> message? Seems like relevant information for future readers. >> > >> > No. The bit of commentary mentioning MacOS's very old 'make' was just >> > talking about a possible alternate way of implementing the change. >> > That alternative was not chosen, so talking about old 'make' in the >> > commit message would be confusing for readers. >> >> Interesting. Documentation/SubmittinPatches reads: >> >> The body should provide a meaningful commit message, which: >> <snip> >> . alternate solutions considered but discarded, if any. >> >> The consensus has changed, maybe? In which case, should we remove that >> statement from there? > > 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 strive to convey the problem succinctly > in the commit message, to not overload the reader with unnecessary (or > confusing) information, while, at the same time, not be sparing with > information which is genuinely needed to understand the problem and > solution. > > Often, this can be done without talking about alternatives; often even > without spelling out the solution in detail or at all since the > solution may be "obvious", given a well-written problem description. > Complex cases, or cases in which multiple solutions may be or seem > valid, on the other hand, might warrant talking about those alternate > solutions, so we probably don't want to drop that bullet point. > Perhaps, instead, it can be re-worded a bit to make it sound something > other than mandatory (but I can't think of a good way to phrase it; > maybe you can?). Yup, "if any" is a bad thing to say, as it does not set the bar for that "any" random garbage idea. A phrase like "when appropriate" is a relatively safe but mostly useless cop-out, as these guidelines are written primarily for those who don't yet have proper yardsticks to gauge what is appropriate and what isn't. I think it maybe better to either drop it or make it a sample way to do the second point, i.e. if there are seemingly valid alternative which may entice readers, explaining why the alternative does not work well and the solution you chose works better *is* a good way to justify the way you chose in your change. Off the top of my head, something like this? I am not very happy with the text, though. Documentation/SubmittingPatches | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches index 2488544407..4294d0f068 100644 --- a/Documentation/SubmittingPatches +++ b/Documentation/SubmittingPatches @@ -125,10 +125,12 @@ The body should provide a meaningful commit message, which: . explains the problem the change tries to solve, i.e. what is wrong with the current code without the change. -. justifies the way the change solves the problem, i.e. why the - result with the change is better. +. justifies the way the change solves the problem, i.e. why the result + with the change is better (e.g. explaining the reason why an + seemingly obvious alternative does not work but the solution in the + patch does may be a good way to illustrate the nature of the problem + and how your approach fits it better). -. alternate solutions considered but discarded, if any. [[imperative-mood]] Describe your changes in imperative mood, e.g. "make xyzzy do frotz" ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH] Makefile: make NO_ICONV really mean "no iconv" 2018-06-15 2:25 ` [PATCH] Makefile: make NO_ICONV really mean "no iconv" Eric Sunshine 2018-06-15 4:20 ` Jeff King 2018-06-15 6:58 ` Simon Ruderich @ 2018-06-15 8:15 ` Mahmoud Al-Qudsi 2 siblings, 0 replies; 15+ messages in thread From: Mahmoud Al-Qudsi @ 2018-06-15 8:15 UTC (permalink / raw) To: Eric Sunshine; +Cc: git On Thu, Jun 14, 2018 at 9:25 PM, Eric Sunshine <sunshine@sunshineco.com> 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 into the linker flags when NEEDS_LIBICONV is > defined, which breaks the build when iconv is not installed. > > On some platforms, iconv lives directly in libc, whereas, on others it > resides in libiconv. For the latter case, NEEDS_LIBICONV instructs the > Makefile to add "-liconv" to the linker flags. config.mak.uname > automatically defines NEEDS_LIBICONV for platforms which require it. > The adding of "-liconv" is done unconditionally, despite NO_ICONV. > > Work around this problem by making NO_ICONV take precedence over > NEEDS_LIBICONV. > > Reported by: Mahmoud Al-Qudsi <mqudsi@neosmart.net> > Signed-off-by: Eric Sunshine <sunshine@sunshineco.com> > --- > ... Thanks, Eric. I can confirm that on a clean FreeBSD 12 installation with libiconv and with this patch applied, git builds and installs from source (though other dependencies are obviously needed). Mahmoud Al-Qudsi NeoSmart Technologies ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: Is NO_ICONV misnamed or is it broken? 2018-06-14 22:47 Is NO_ICONV misnamed or is it broken? Mahmoud Al-Qudsi 2018-06-15 2:25 ` [PATCH] Makefile: make NO_ICONV really mean "no iconv" Eric Sunshine @ 2018-06-17 2:57 ` Christian Couder 2018-06-17 3:41 ` Eric Sunshine 1 sibling, 1 reply; 15+ messages in thread From: Christian Couder @ 2018-06-17 2:57 UTC (permalink / raw) To: Mahmoud Al-Qudsi; +Cc: git, Fernando J. Pereda, Joachim Schmitz, Junio C Hamano Hi, On Fri, Jun 15, 2018 at 12:47 AM, Mahmoud Al-Qudsi <mqudsi@neosmart.net> wrote: > Hello list, > > With regards to the Makefile define/variable `NO_ICONV` - the Makefile > comments imply that it should be used if "your libc doesn't properly support > iconv," which could mean anything from "a patch will be applied" to "iconv > won't be used." b6e56eca8a (Allow building Git in systems without iconv, 2006-02-16) which added NO_ICONV says: Systems using some uClibc versions do not properly support iconv stuff. This patch allows Git to be built on those systems by passing NO_ICONV=YesPlease to make. The only drawback is mailinfo won't do charset conversion in those systems. > Based off the name of the varibale, the assumption is that iconv is an > optional dependency that can be omitted if compiled with NO_ICONV. However, in > practice attempting to compile git with `make ... NO_ICONV=1` and libiconv not > installed results in linker errors as follows: > > ``` > ~> make clean > # omitted > ~> make NO_ICONV=1 > # ommitted > LINK git-credential-store > /usr/bin/ld: cannot find -liconv > cc: error: linker command failed with exit code 1 (use -v to see invocation) > gmake: *** [Makefile:2327: git-credential-store] Error 1 > ``` Yeah, this might be an issue with the Makefile options. > Am I misunderstanding the intended behavior when NO_ICONV is defined (i.e. it > does not remove the dependency on libiconv) or is this a bug and iconv should > not, in fact, be required? It's difficult to tell from reading the comments and commit messages. I think 597c9cc540 (Flatten tools/ directory to make build procedure simpler., 2005-09-07) which introduces NEEDS_LIBICONV is even older than the commit that introduced NO_ICONV (see above), so you might want to play with NEEDS_LIBICONV too and see if it works better for you. (I understand that "you might want to play with such and such other options" is perhaps not as helpful as what you expected, but I previously tried to tighten the way we handle dependencies in the Makefile and it was considered "too heavy handed". So yeah, we consider it ok if people have to tinker a bit when they want to build Git.) I CC'ed the people involved in related commits. Maybe they can give you a better answer. It might also help if you could tell us on which OS/Platform and perhaps for which purpose you want to compile Git. Best, Christian. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: Is NO_ICONV misnamed or is it broken? 2018-06-17 2:57 ` Is NO_ICONV misnamed or is it broken? Christian Couder @ 2018-06-17 3:41 ` Eric Sunshine 0 siblings, 0 replies; 15+ messages in thread From: Eric Sunshine @ 2018-06-17 3:41 UTC (permalink / raw) To: Christian Couder Cc: Mahmoud Al-Qudsi, Git List, ferdy, Joachim Schmitz, Junio C Hamano On Sat, Jun 16, 2018 at 10:57 PM Christian Couder <christian.couder@gmail.com> wrote: > On Fri, Jun 15, 2018 at 12:47 AM, Mahmoud Al-Qudsi <mqudsi@neosmart.net> wrote: > > ~> make NO_ICONV=1 > > # ommitted > > LINK git-credential-store > > /usr/bin/ld: cannot find -liconv > I think 597c9cc540 (Flatten tools/ directory to make build procedure > simpler., 2005-09-07) which introduces NEEDS_LIBICONV is even older > than the commit that introduced NO_ICONV (see above), so you might > want to play with NEEDS_LIBICONV too and see if it works better for > you. > > I CC'ed the people involved in related commits. Maybe they can give > you a better answer. It might also help if you could tell us on which > OS/Platform and perhaps for which purpose you want to compile Git. For completeness, for those reading this thread, a patch fixing this issue has already been sent[1] to the list. [1]: https://public-inbox.org/git/20180615022503.34111-1-sunshine@sunshineco.com/ ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2018-06-18 16:09 UTC | newest] Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-06-14 22:47 Is NO_ICONV misnamed or is it broken? Mahmoud Al-Qudsi 2018-06-15 2:25 ` [PATCH] Makefile: make NO_ICONV really mean "no iconv" Eric Sunshine 2018-06-15 4:20 ` Jeff King 2018-06-15 6:30 ` Eric Sunshine 2018-06-15 6:39 ` Jeff King 2018-06-15 6:58 ` Simon Ruderich 2018-06-15 7:43 ` Eric Sunshine 2018-06-17 17:32 ` Kaartic Sivaraam 2018-06-17 18:00 ` Eric Sunshine 2018-06-17 18:25 ` Doc/SubmittingPatches: re-phrashing a sentence about alternate solutions (was Re: [PATCH] Makefile: make NO_ICONV really mean "no iconv") Kaartic Sivaraam 2018-06-18 4:20 ` Jeff King 2018-06-18 16:09 ` [PATCH] Makefile: make NO_ICONV really mean "no iconv" Junio C Hamano 2018-06-15 8:15 ` Mahmoud Al-Qudsi 2018-06-17 2:57 ` Is NO_ICONV misnamed or is it broken? Christian Couder 2018-06-17 3:41 ` Eric Sunshine
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.