{"thread":{"id":"48713","subject":"Is NO_ICONV misnamed or is it broken?","startedAt":"2018-06-14T22:47:53Z","lastAt":"2018-06-18T16:09:40Z","messageCount":15,"participants":["Mahmoud Al-Qudsi","Eric Sunshine","Jeff King","Simon Ruderich","Christian Couder","Kaartic Sivaraam","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"350211","messageId":"CACcTrKePbgyCbXneN5NZ+cS-tiDyYe_dkdwttXpP0CUeEicvHw@mail.gmail.com","threadId":"48713","inReplyTo":null,"subject":"Is NO_ICONV misnamed or is it broken?","fromName":"Mahmoud Al-Qudsi","fromEmail":"mqudsi@neosmart.net","sentAt":"2018-06-14T22:47:28Z","receivedAt":"2018-06-14T22:47:53Z","isPatch":false,"sender":{"key":"mqudsi@neosmart.net","avatar":"https://gravatar.com/avatar/c2643dd7c6df61aed49d9f3d917ac6d61cafbbda5f9b1619f50d3b749dca415a?d=mp&s=160"},"body":"Hello list,\n\nWith regards to the Makefile define/variable `NO_ICONV` - the Makefile\ncomments imply that it should be used if \"your libc doesn't properly support\niconv,\" which could mean anything from \"a patch will be applied\" to \"iconv\nwon't be used.\"\n\nBased off the name of the varibale, the assumption is that iconv is an\noptional dependency that can be omitted if compiled with NO_ICONV. However, in\npractice attempting to compile git with `make ... NO_ICONV=1` and libiconv not\ninstalled results in linker errors as follows:\n\n```\n~> make clean\n# omitted\n~> make NO_ICONV=1\n# ommitted\n    LINK git-credential-store\n/usr/bin/ld: cannot find -liconv\ncc: error: linker command failed with exit code 1 (use -v to see invocation)\ngmake: *** [Makefile:2327: git-credential-store] Error 1\n```\n\nAm I misunderstanding the intended behavior when NO_ICONV is defined (i.e. it\ndoes not remove the dependency on libiconv) or is this a bug and iconv should\nnot, in fact, be required?\n\nMany thanks,\n\nMahmoud Al-Qudsi\nNeoSmart Technologies\n"},{"id":"350222","messageId":"20180615022503.34111-1-sunshine@sunshineco.com","threadId":"48713","inReplyTo":"CACcTrKePbgyCbXneN5NZ+cS-tiDyYe_dkdwttXpP0CUeEicvHw@mail.gmail.com","subject":"[PATCH] Makefile: make NO_ICONV really mean \"no iconv\"","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-06-15T02:25:03Z","receivedAt":"2018-06-15T02:25:55Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"The Makefile tweak NO_ICONV is meant to allow Git to be built without\niconv in case iconv is not installed or is otherwise dysfunctional.\nHowever, NO_ICONV's disabling of iconv is incomplete and can incorrectly\nallow \"-liconv\" to slip into the linker flags when NEEDS_LIBICONV is\ndefined, which breaks the build when iconv is not installed.\n\nOn some platforms, iconv lives directly in libc, whereas, on others it\nresides in libiconv. For the latter case, NEEDS_LIBICONV instructs the\nMakefile to add \"-liconv\" to the linker flags. config.mak.uname\nautomatically defines NEEDS_LIBICONV for platforms which require it.\nThe adding of \"-liconv\" is done unconditionally, despite NO_ICONV.\n\nWork around this problem by making NO_ICONV take precedence over\nNEEDS_LIBICONV.\n\nReported by: Mahmoud Al-Qudsi <mqudsi@neosmart.net>\nSigned-off-by: Eric Sunshine <sunshine@sunshineco.com>\n---\n\nThis patch is extra noisy due to the indentation change. Viewing it with\n\"git diff -w\" helps. An alternative to re-indenting would have been to\n\"undefine NEEDS_LIBICONV\", however, 'undefine' was added to GNU make in\n3.82 but MacOS is stuck on 3.81 (from 2006) so 'undefine' was avoided.\n\nReported here: https://public-inbox.org/git/CACcTrKePbgyCbXneN5NZ+cS-tiDyYe_dkdwttXpP0CUeEicvHw@mail.gmail.com/T/#u\n\n Makefile | 22 ++++++++++++----------\n 1 file changed, 12 insertions(+), 10 deletions(-)\n\ndiff --git a/Makefile b/Makefile\nindex 1d27f36365..e4b503d259 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -1351,17 +1351,19 @@ ifdef APPLE_COMMON_CRYPTO\n \tLIB_4_CRYPTO += -framework Security -framework CoreFoundation\n endif\n endif\n-ifdef NEEDS_LIBICONV\n-\tifdef ICONVDIR\n-\t\tBASIC_CFLAGS += -I$(ICONVDIR)/include\n-\t\tICONV_LINK = -L$(ICONVDIR)/$(lib) $(CC_LD_DYNPATH)$(ICONVDIR)/$(lib)\n-\telse\n-\t\tICONV_LINK =\n-\tendif\n-\tifdef NEEDS_LIBINTL_BEFORE_LIBICONV\n-\t\tICONV_LINK += -lintl\n+ifndef NO_ICONV\n+\tifdef NEEDS_LIBICONV\n+\t\tifdef ICONVDIR\n+\t\t\tBASIC_CFLAGS += -I$(ICONVDIR)/include\n+\t\t\tICONV_LINK = -L$(ICONVDIR)/$(lib) $(CC_LD_DYNPATH)$(ICONVDIR)/$(lib)\n+\t\telse\n+\t\t\tICONV_LINK =\n+\t\tendif\n+\t\tifdef NEEDS_LIBINTL_BEFORE_LIBICONV\n+\t\t\tICONV_LINK += -lintl\n+\t\tendif\n+\t\tEXTLIBS += $(ICONV_LINK) -liconv\n \tendif\n-\tEXTLIBS += $(ICONV_LINK) -liconv\n endif\n ifdef NEEDS_LIBGEN\n \tEXTLIBS += -lgen\n-- \n2.18.0.rc1.256.g331a1db143\n\n"},{"id":"350228","messageId":"20180615042023.GA31294@sigill.intra.peff.net","threadId":"48713","inReplyTo":"20180615022503.34111-1-sunshine@sunshineco.com","subject":"Re: [PATCH] Makefile: make NO_ICONV really mean \"no iconv\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-06-15T04:20:23Z","receivedAt":"2018-06-15T04:20:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jun 14, 2018 at 10:25:03PM -0400, Eric Sunshine wrote:\n\n> The Makefile tweak NO_ICONV is meant to allow Git to be built without\n> iconv in case iconv is not installed or is otherwise dysfunctional.\n> However, NO_ICONV's disabling of iconv is incomplete and can incorrectly\n> allow \"-liconv\" to slip into the linker flags when NEEDS_LIBICONV is\n> defined, which breaks the build when iconv is not installed.\n> \n> On some platforms, iconv lives directly in libc, whereas, on others it\n> resides in libiconv. For the latter case, NEEDS_LIBICONV instructs the\n> Makefile to add \"-liconv\" to the linker flags. config.mak.uname\n> automatically defines NEEDS_LIBICONV for platforms which require it.\n> The adding of \"-liconv\" is done unconditionally, despite NO_ICONV.\n> \n> Work around this problem by making NO_ICONV take precedence over\n> NEEDS_LIBICONV.\n\nNicely explained.\n\nWe have OLD_ICONV, too, which should probably do nothing if NO_ICONV is\nset. I think that works OK. We end up setting -DOLD_ICONV on the command\nline, but that's only consider inside \"#ifndef NO_ICONV\" within the\ncode.\n\n> This patch is extra noisy due to the indentation change. Viewing it with\n> \"git diff -w\" helps. An alternative to re-indenting would have been to\n> \"undefine NEEDS_LIBICONV\", however, 'undefine' was added to GNU make in\n> 3.82 but MacOS is stuck on 3.81 (from 2006) so 'undefine' was avoided.\n\nYeah, with \"-w\" it looks pretty obviously correct.\n\n-Peff\n"},{"id":"350236","messageId":"CAPig+cTSirqhayBDJFZhn2HL4EMr36GHuCOYAMaCeptqFtoR=w@mail.gmail.com","threadId":"48713","inReplyTo":"20180615042023.GA31294@sigill.intra.peff.net","subject":"Re: [PATCH] Makefile: make NO_ICONV really mean \"no iconv\"","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-06-15T06:30:43Z","receivedAt":"2018-06-15T06:30:58Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Jun 15, 2018 at 12:20 AM Jeff King <peff@peff.net> wrote:\n> We have OLD_ICONV, too, which should probably do nothing if NO_ICONV is\n> set. I think that works OK. We end up setting -DOLD_ICONV on the command\n> line, but that's only consider inside \"#ifndef NO_ICONV\" within the\n> code.\n\nRight, that was my conclusion, as well. Since it works as is, I'm not\nsure suppressing -DOLD_ICONV in Makefile is worth the extra patch\nnoise. I can re-roll with that change too, if someone thinks it's\nworthwhile, though.\n"},{"id":"350238","messageId":"20180615063937.GA10802@sigill.intra.peff.net","threadId":"48713","inReplyTo":"CAPig+cTSirqhayBDJFZhn2HL4EMr36GHuCOYAMaCeptqFtoR=w@mail.gmail.com","subject":"Re: [PATCH] Makefile: make NO_ICONV really mean \"no iconv\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-06-15T06:39:38Z","receivedAt":"2018-06-15T06:39:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jun 15, 2018 at 02:30:43AM -0400, Eric Sunshine wrote:\n\n> On Fri, Jun 15, 2018 at 12:20 AM Jeff King <peff@peff.net> wrote:\n> > We have OLD_ICONV, too, which should probably do nothing if NO_ICONV is\n> > set. I think that works OK. We end up setting -DOLD_ICONV on the command\n> > line, but that's only consider inside \"#ifndef NO_ICONV\" within the\n> > code.\n> \n> Right, that was my conclusion, as well. Since it works as is, I'm not\n> sure suppressing -DOLD_ICONV in Makefile is worth the extra patch\n> noise. I can re-roll with that change too, if someone thinks it's\n> worthwhile, though.\n\nNah, I was just thinking out loud. I don't think it's worth changing the\nMakefile (and I wouldn't be surprised if there are other \"dependent\"\ncases like this that work just fine because of the #ifdefs in the code).\n\n-Peff\n"},{"id":"350239","messageId":"20180615065805.GA15146@ruderich.org","threadId":"48713","inReplyTo":"20180615022503.34111-1-sunshine@sunshineco.com","subject":"Re: [PATCH] Makefile: make NO_ICONV really mean \"no iconv\"","fromName":"Simon Ruderich","fromEmail":"simon@ruderich.org","sentAt":"2018-06-15T06:58:05Z","receivedAt":"2018-06-15T06:58:10Z","isPatch":true,"sender":{"key":"simon@ruderich.org","avatar":"https://avatars.githubusercontent.com/u/390994?v=4"},"body":"On Thu, Jun 14, 2018 at 10:25:03PM -0400, Eric Sunshine wrote:\n> The Makefile tweak NO_ICONV is meant to allow Git to be built without\n> iconv in case iconv is not installed or is otherwise dysfunctional.\n> However, NO_ICONV's disabling of iconv is incomplete and can incorrectly\n> allow \"-liconv\" to slip into the linker flags when NEEDS_LIBICONV is\n> defined, which breaks the build when iconv is not installed.\n>\n> On some platforms, iconv lives directly in libc, whereas, on others it\n> resides in libiconv. For the latter case, NEEDS_LIBICONV instructs the\n> Makefile to add \"-liconv\" to the linker flags. config.mak.uname\n> automatically defines NEEDS_LIBICONV for platforms which require it.\n> The adding of \"-liconv\" is done unconditionally, despite NO_ICONV.\n>\n> Work around this problem by making NO_ICONV take precedence over\n> NEEDS_LIBICONV.\n>\n> Reported by: Mahmoud Al-Qudsi <mqudsi@neosmart.net>\n> Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>\n> ---\n>\n> This patch is extra noisy due to the indentation change. Viewing it with\n> \"git diff -w\" helps. An alternative to re-indenting would have been to\n> \"undefine NEEDS_LIBICONV\", however, 'undefine' was added to GNU make in\n> 3.82 but MacOS is stuck on 3.81 (from 2006) so 'undefine' was avoided.\n\nShould we put the part about MacOS's make into the commit\nmessage? Seems like relevant information for future readers.\n\nRegards\nSimon\n-- \n+ privacy is necessary\n+ using gnupg http://gnupg.org\n+ public key id: 0x92FEFDB7E44C32F9\n"},{"id":"350241","messageId":"CAPig+cQL8rTg+GASp2tSng7PPPYkfeeV2SNyi0D+6-Ep7JKaGg@mail.gmail.com","threadId":"48713","inReplyTo":"20180615065805.GA15146@ruderich.org","subject":"Re: [PATCH] Makefile: make NO_ICONV really mean \"no iconv\"","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-06-15T07:43:36Z","receivedAt":"2018-06-15T07:43:51Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Jun 15, 2018 at 2:58 AM Simon Ruderich <simon@ruderich.org> wrote:\n> On Thu, Jun 14, 2018 at 10:25:03PM -0400, Eric Sunshine wrote:\n> > This patch is extra noisy due to the indentation change. Viewing it with\n> > \"git diff -w\" helps. An alternative to re-indenting would have been to\n> > \"undefine NEEDS_LIBICONV\", however, 'undefine' was added to GNU make in\n> > 3.82 but MacOS is stuck on 3.81 (from 2006) so 'undefine' was avoided.\n>\n> Should we put the part about MacOS's make into the commit\n> message? Seems like relevant information for future readers.\n\nNo. The bit of commentary mentioning MacOS's very old 'make' was just\ntalking about a possible alternate way of implementing the change.\nThat alternative was not chosen, so talking about old 'make' in the\ncommit message would be confusing for readers. More importantly,\nalthough that alternative would have made a less noisy patch, the\nactual result would have made the Makefile itself noisier and uglier,\nparticularly for people just reading the Makefile in the future,\npeople who did not read the patch. Specifically, these alternatives\nwere considered:\n\n    ifdef NO_ICONV\n        undefine NEEDS_LIBICONV\n    endif\n    ifdef NEEDS_LIBICONV\n        ...as before...\n    endif\n\nand:\n\n    ifdef NO_ICONV\n        NEEDS_LIBICONV=\n    endif\n    ifeq ($(NEEDS_LIBICONV),)\n        ...as before...\n    endif\n\nBoth of which are uglier for a future reader of Makefile than the\nend-result actually implemented by this patch:\n\n    ifndef NO_ICONV\n        ifdef NEEDS_LIBICONV\n            ...as before...\n        endif\n    endif\n"},{"id":"350242","messageId":"CACcTrKcocbznJZc7PL_6Ld_Rbm9mu3iaUf1R_G6MWaQf2LbC=Q@mail.gmail.com","threadId":"48713","inReplyTo":"20180615022503.34111-1-sunshine@sunshineco.com","subject":"Re: [PATCH] Makefile: make NO_ICONV really mean \"no iconv\"","fromName":"Mahmoud Al-Qudsi","fromEmail":"mqudsi@neosmart.net","sentAt":"2018-06-15T08:15:36Z","receivedAt":"2018-06-15T08:16:03Z","isPatch":true,"sender":{"key":"mqudsi@neosmart.net","avatar":"https://gravatar.com/avatar/c2643dd7c6df61aed49d9f3d917ac6d61cafbbda5f9b1619f50d3b749dca415a?d=mp&s=160"},"body":"On Thu, Jun 14, 2018 at 9:25 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:\n> The Makefile tweak NO_ICONV is meant to allow Git to be built without\n> iconv in case iconv is not installed or is otherwise dysfunctional.\n> However, NO_ICONV's disabling of iconv is incomplete and can incorrectly\n> allow \"-liconv\" to slip into the linker flags when NEEDS_LIBICONV is\n> defined, which breaks the build when iconv is not installed.\n>\n> On some platforms, iconv lives directly in libc, whereas, on others it\n> resides in libiconv. For the latter case, NEEDS_LIBICONV instructs the\n> Makefile to add \"-liconv\" to the linker flags. config.mak.uname\n> automatically defines NEEDS_LIBICONV for platforms which require it.\n> The adding of \"-liconv\" is done unconditionally, despite NO_ICONV.\n>\n> Work around this problem by making NO_ICONV take precedence over\n> NEEDS_LIBICONV.\n>\n> Reported by: Mahmoud Al-Qudsi <mqudsi@neosmart.net>\n> Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>\n> ---\n> ...\n\nThanks, Eric. I can confirm that on a clean FreeBSD 12 installation with\nlibiconv and with this patch applied, git builds and installs from source\n(though other dependencies are obviously needed).\n\nMahmoud Al-Qudsi\nNeoSmart Technologies\n"},{"id":"350335","messageId":"CAP8UFD2SJF_gbP-mdXoAH0t_OmLjRbPuVK5vZZjgs2N9eJz5KQ@mail.gmail.com","threadId":"48713","inReplyTo":"CACcTrKePbgyCbXneN5NZ+cS-tiDyYe_dkdwttXpP0CUeEicvHw@mail.gmail.com","subject":"Re: Is NO_ICONV misnamed or is it broken?","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2018-06-17T02:57:38Z","receivedAt":"2018-06-17T02:57:42Z","isPatch":false,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Hi,\n\nOn Fri, Jun 15, 2018 at 12:47 AM, Mahmoud Al-Qudsi <mqudsi@neosmart.net> wrote:\n> Hello list,\n>\n> With regards to the Makefile define/variable `NO_ICONV` - the Makefile\n> comments imply that it should be used if \"your libc doesn't properly support\n> iconv,\" which could mean anything from \"a patch will be applied\" to \"iconv\n> won't be used.\"\n\nb6e56eca8a (Allow building Git in systems without iconv, 2006-02-16)\nwhich added NO_ICONV says:\n\n    Systems using some uClibc versions do not properly support\n    iconv stuff. This patch allows Git to be built on those\n    systems by passing NO_ICONV=YesPlease to make. The only\n    drawback is mailinfo won't do charset conversion in those\n    systems.\n\n> Based off the name of the varibale, the assumption is that iconv is an\n> optional dependency that can be omitted if compiled with NO_ICONV. However, in\n> practice attempting to compile git with `make ... NO_ICONV=1` and libiconv not\n> installed results in linker errors as follows:\n>\n> ```\n> ~> make clean\n> # omitted\n> ~> make NO_ICONV=1\n> # ommitted\n>     LINK git-credential-store\n> /usr/bin/ld: cannot find -liconv\n> cc: error: linker command failed with exit code 1 (use -v to see invocation)\n> gmake: *** [Makefile:2327: git-credential-store] Error 1\n> ```\n\nYeah, this might be an issue with the Makefile options.\n\n> Am I misunderstanding the intended behavior when NO_ICONV is defined (i.e. it\n> does not remove the dependency on libiconv) or is this a bug and iconv should\n> not, in fact, be required?\n\nIt's difficult to tell from reading the comments and commit messages.\n\nI think 597c9cc540 (Flatten tools/ directory to make build procedure\nsimpler., 2005-09-07) which introduces NEEDS_LIBICONV is even older\nthan the commit that introduced NO_ICONV (see above), so you might\nwant to play with NEEDS_LIBICONV too and see if it works better for\nyou.\n\n(I understand that \"you might want to play with such and such other\noptions\" is perhaps not as helpful as what you expected, but I\npreviously tried to tighten the way we handle dependencies in the\nMakefile and it was considered \"too heavy handed\". So yeah, we\nconsider it ok if people have to tinker a bit when they want to build\nGit.)\n\nI CC'ed the people involved in related commits. Maybe they can give\nyou a better answer. It might also help if you could tell us on which\nOS/Platform and perhaps for which purpose you want to compile Git.\n\nBest,\nChristian.\n"},{"id":"350338","messageId":"CAPig+cRR8=vbafsjL_pnoq6ssxBkkoVo0OMd_i8P_a8ze7eLHg@mail.gmail.com","threadId":"48713","inReplyTo":"CAP8UFD2SJF_gbP-mdXoAH0t_OmLjRbPuVK5vZZjgs2N9eJz5KQ@mail.gmail.com","subject":"Re: Is NO_ICONV misnamed or is it broken?","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-06-17T03:41:38Z","receivedAt":"2018-06-17T03:41:51Z","isPatch":false,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sat, Jun 16, 2018 at 10:57 PM Christian Couder\n<christian.couder@gmail.com> wrote:\n> On Fri, Jun 15, 2018 at 12:47 AM, Mahmoud Al-Qudsi <mqudsi@neosmart.net> wrote:\n> > ~> make NO_ICONV=1\n> > # ommitted\n> >     LINK git-credential-store\n> > /usr/bin/ld: cannot find -liconv\n> I think 597c9cc540 (Flatten tools/ directory to make build procedure\n> simpler., 2005-09-07) which introduces NEEDS_LIBICONV is even older\n> than the commit that introduced NO_ICONV (see above), so you might\n> want to play with NEEDS_LIBICONV too and see if it works better for\n> you.\n>\n> I CC'ed the people involved in related commits. Maybe they can give\n> you a better answer. It might also help if you could tell us on which\n> OS/Platform and perhaps for which purpose you want to compile Git.\n\nFor completeness, for those reading this thread, a patch fixing this\nissue has already been sent[1] to the list.\n\n[1]: https://public-inbox.org/git/20180615022503.34111-1-sunshine@sunshineco.com/\n"},{"id":"350363","messageId":"a079d636-e70d-f383-ae87-ab890a636441@gmail.com","threadId":"48713","inReplyTo":"CAPig+cQL8rTg+GASp2tSng7PPPYkfeeV2SNyi0D+6-Ep7JKaGg@mail.gmail.com","subject":"Re: [PATCH] Makefile: make NO_ICONV really mean \"no iconv\"","fromName":"Kaartic Sivaraam","fromEmail":"kaartic.sivaraam@gmail.com","sentAt":"2018-06-17T17:32:45Z","receivedAt":"2018-06-17T17:32:55Z","isPatch":true,"sender":{"key":"kaartic.sivaraam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12448084?v=4"},"body":"On Friday 15 June 2018 01:13 PM, Eric Sunshine wrote:\n> On Fri, Jun 15, 2018 at 2:58 AM Simon Ruderich <simon@ruderich.org> wrote:\n>> On Thu, Jun 14, 2018 at 10:25:03PM -0400, Eric Sunshine wrote:\n>>> This patch is extra noisy due to the indentation change. Viewing it with\n>>> \"git diff -w\" helps. An alternative to re-indenting would have been to\n>>> \"undefine NEEDS_LIBICONV\", however, 'undefine' was added to GNU make in\n>>> 3.82 but MacOS is stuck on 3.81 (from 2006) so 'undefine' was avoided.\n>>\n>> Should we put the part about MacOS's make into the commit\n>> message? Seems like relevant information for future readers.\n> \n> No. The bit of commentary mentioning MacOS's very old 'make' was just\n> talking about a possible alternate way of implementing the change.\n> That alternative was not chosen, so talking about old 'make' in the\n> commit message would be confusing for readers.\n\nInteresting. Documentation/SubmittinPatches reads:\n\n    The body should provide a meaningful commit message, which:\n    ...\n    <snip>\n    ...\n    . alternate solutions considered but discarded, if any.\n\nThe consensus has changed, maybe? In which case, should we remove that\nstatement from there?\n\n\n-- \nSivaraam\n\nQUOTE:\n\n“The three principal virtues of a programmer are Laziness, Impatience,\nand Hubris.”\n\n\t- Camel book\n\nSivaraam?\n\nYou possibly might have noticed that my signature recently changed from\n'Kaartic' to 'Sivaraam' both of which are parts of my name. I find the\nnew signature to be better for several reasons one of which is that the\nformer signature has a lot of ambiguities in the place I live as it is a\ncommon name (NOTE: it's not a common spelling, just a common name). So,\nI switched signatures before it's too late.\n\nThat said, I won't mind you calling me 'Kaartic' if you like it [of\ncourse ;-)]. You can always call me using either of the names.\n\n\nKIND NOTE TO THE NATIVE ENGLISH SPEAKER:\n\nAs I'm not a native English speaker myself, there might be mistaeks in\nmy usage of English. I apologise for any mistakes that I make.\n\nIt would be \"helpful\" if you take the time to point out the mistakes.\n\nIt would be \"super helpful\" if you could provide suggestions about how\nto correct those mistakes.\n\nThanks in advance!\n\n"},{"id":"350364","messageId":"CAPig+cTMEfu=x2dhUww3x2uk9-ANAK6eepC3hOsx4FE+1jTgBA@mail.gmail.com","threadId":"48713","inReplyTo":"a079d636-e70d-f383-ae87-ab890a636441@gmail.com","subject":"Re: [PATCH] Makefile: make NO_ICONV really mean \"no iconv\"","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-06-17T18:00:26Z","receivedAt":"2018-06-17T18:00:41Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Jun 17, 2018 at 1:32 PM Kaartic Sivaraam\n<kaartic.sivaraam@gmail.com> wrote:\n> On Friday 15 June 2018 01:13 PM, Eric Sunshine wrote:\n> > On Fri, Jun 15, 2018 at 2:58 AM Simon Ruderich <simon@ruderich.org> wrote:\n> >> Should we put the part about MacOS's make into the commit\n> >> message? Seems like relevant information for future readers.\n> >\n> > No. The bit of commentary mentioning MacOS's very old 'make' was just\n> > talking about a possible alternate way of implementing the change.\n> > That alternative was not chosen, so talking about old 'make' in the\n> > commit message would be confusing for readers.\n>\n> Interesting. Documentation/SubmittinPatches reads:\n>\n>     The body should provide a meaningful commit message, which:\n>     <snip>\n>     . alternate solutions considered but discarded, if any.\n>\n> The consensus has changed, maybe? In which case, should we remove that\n> statement from there?\n\nWhether or not to talk about alternate solutions in the commit message\nis a judgment call. Same for deciding what belongs in the commit\nmessage proper and what belongs in the \"commentary\" section of a\npatch. A patch author should strive to convey the problem succinctly\nin the commit message, to not overload the reader with unnecessary (or\nconfusing) information, while, at the same time, not be sparing with\ninformation which is genuinely needed to understand the problem and\nsolution.\n\nOften, this can be done without talking about alternatives; often even\nwithout spelling out the solution in detail or at all since the\nsolution may be \"obvious\", given a well-written problem description.\nComplex cases, or cases in which multiple solutions may be or seem\nvalid, on the other hand, might warrant talking about those alternate\nsolutions, so we probably don't want to drop that bullet point.\nPerhaps, instead, it can be re-worded a bit to make it sound something\nother than mandatory (but I can't think of a good way to phrase it;\nmaybe you can?).\n"},{"id":"350365","messageId":"1529259933.7225.2.camel@gmail.com","threadId":"48713","inReplyTo":"CAPig+cTMEfu=x2dhUww3x2uk9-ANAK6eepC3hOsx4FE+1jTgBA@mail.gmail.com","subject":"Doc/SubmittingPatches: re-phrashing a sentence about alternate solutions (was Re: [PATCH] Makefile: make NO_ICONV really mean \"no iconv\")","fromName":"Kaartic Sivaraam","fromEmail":"kaartic.sivaraam@gmail.com","sentAt":"2018-06-17T18:25:33Z","receivedAt":"2018-06-17T18:25:43Z","isPatch":true,"sender":{"key":"kaartic.sivaraam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12448084?v=4"},"body":"On Sun, 2018-06-17 at 14:00 -0400, Eric Sunshine wrote:\n> Whether or not to talk about alternate solutions in the commit message\n> is a judgment call. Same for deciding what belongs in the commit\n> message proper and what belongs in the \"commentary\" section of a\n> patch. A patch author should strive to convey the problem succinctly\n> in the commit message, to not overload the reader with unnecessary (or\n> confusing) information, while, at the same time, not be sparing with\n> information which is genuinely needed to understand the problem and\n> solution.\n> \n> Often, this can be done without talking about alternatives; often even\n> without spelling out the solution in detail or at all since the\n> solution may be \"obvious\", given a well-written problem description.\n> Complex cases, or cases in which multiple solutions may be or seem\n> valid, on the other hand, might warrant talking about those alternate\n> solutions, so we probably don't want to drop that bullet point.\n\nWell explained, thanks. (Thinking out loud, it might be even nice to\nincluding the above paragraphs into Documentation/SubmittingPatches as\nI find it to be more \"humane\" than the terse bullets. But I refrained\nfrom doing so as the document is already a bit too-long ;-)\n\n> Perhaps, instead, it can be re-worded a bit to make it sound something\n> other than mandatory (but I can't think of a good way to phrase it;\n> maybe you can?).\n\nHow about the following patch? (warning: patch only for discussion\npurposes, might be white-space broken). It might be superfluous,\nthough.\n\ndiff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches\nindex a1d0feca3..565bc4397 100644\n--- a/Documentation/SubmittingPatches\n+++ b/Documentation/SubmittingPatches\n@@ -128,7 +128,7 @@ The body should provide a meaningful commit message, which:\n . justifies the way the change solves the problem, i.e. why the\n   result with the change is better.\n \n-. alternate solutions considered but discarded, if any.\n+. alternate solutions considered but discarded, where necessary.\n \n [[imperative-mood]]\n Describe your changes in imperative mood, e.g. \"make xyzzy do frotz\"\n\n\nRegards,\nSivaraam\n"},{"id":"350371","messageId":"20180618042054.GB31125@sigill.intra.peff.net","threadId":"48713","inReplyTo":"1529259933.7225.2.camel@gmail.com","subject":"Re: Doc/SubmittingPatches: re-phrashing a sentence about alternate solutions (was Re: [PATCH] Makefile: make NO_ICONV really mean \"no iconv\")","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-06-18T04:20:54Z","receivedAt":"2018-06-18T04:20:59Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Jun 17, 2018 at 11:55:33PM +0530, Kaartic Sivaraam wrote:\n\n> On Sun, 2018-06-17 at 14:00 -0400, Eric Sunshine wrote:\n> > Whether or not to talk about alternate solutions in the commit message\n> > is a judgment call. Same for deciding what belongs in the commit\n> > message proper and what belongs in the \"commentary\" section of a\n> > patch. A patch author should strive to convey the problem succinctly\n> > in the commit message, to not overload the reader with unnecessary (or\n> > confusing) information, while, at the same time, not be sparing with\n> > information which is genuinely needed to understand the problem and\n> > solution.\n> > \n> > Often, this can be done without talking about alternatives; often even\n> > without spelling out the solution in detail or at all since the\n> > solution may be \"obvious\", given a well-written problem description.\n> > Complex cases, or cases in which multiple solutions may be or seem\n> > valid, on the other hand, might warrant talking about those alternate\n> > solutions, so we probably don't want to drop that bullet point.\n> \n> Well explained, thanks. (Thinking out loud, it might be even nice to\n> including the above paragraphs into Documentation/SubmittingPatches as\n> I find it to be more \"humane\" than the terse bullets. But I refrained\n> from doing so as the document is already a bit too-long ;-)\n\nYes, the first paragraph especially. The _most_ important thing is\nwriting well with consideration for your readers. All the other rules\nare really guidelines to help you remember how to do that. When in\ndoubt follow the guidelines, but it's OK to break them if it serves the\nultimate purpose.\n\nAll IMHO, of course. :)\n\n-Peff\n"},{"id":"350393","messageId":"xmqqk1qwxeip.fsf@gitster-ct.c.googlers.com","threadId":"48713","inReplyTo":"CAPig+cTMEfu=x2dhUww3x2uk9-ANAK6eepC3hOsx4FE+1jTgBA@mail.gmail.com","subject":"Re: [PATCH] Makefile: make NO_ICONV really mean \"no iconv\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-06-18T16:09:34Z","receivedAt":"2018-06-18T16:09:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine <sunshine@sunshineco.com> writes:\n\n> On Sun, Jun 17, 2018 at 1:32 PM Kaartic Sivaraam\n> <kaartic.sivaraam@gmail.com> wrote:\n>> On Friday 15 June 2018 01:13 PM, Eric Sunshine wrote:\n>> > On Fri, Jun 15, 2018 at 2:58 AM Simon Ruderich <simon@ruderich.org> wrote:\n>> >> Should we put the part about MacOS's make into the commit\n>> >> message? Seems like relevant information for future readers.\n>> >\n>> > No. The bit of commentary mentioning MacOS's very old 'make' was just\n>> > talking about a possible alternate way of implementing the change.\n>> > That alternative was not chosen, so talking about old 'make' in the\n>> > commit message would be confusing for readers.\n>>\n>> Interesting. Documentation/SubmittinPatches reads:\n>>\n>>     The body should provide a meaningful commit message, which:\n>>     <snip>\n>>     . alternate solutions considered but discarded, if any.\n>>\n>> The consensus has changed, maybe? In which case, should we remove that\n>> statement from there?\n>\n> Whether or not to talk about alternate solutions in the commit message\n> is a judgment call. Same for deciding what belongs in the commit\n> message proper and what belongs in the \"commentary\" section of a\n> patch. A patch author should strive to convey the problem succinctly\n> in the commit message, to not overload the reader with unnecessary (or\n> confusing) information, while, at the same time, not be sparing with\n> information which is genuinely needed to understand the problem and\n> solution.\n>\n> Often, this can be done without talking about alternatives; often even\n> without spelling out the solution in detail or at all since the\n> solution may be \"obvious\", given a well-written problem description.\n> Complex cases, or cases in which multiple solutions may be or seem\n> valid, on the other hand, might warrant talking about those alternate\n> solutions, so we probably don't want to drop that bullet point.\n> Perhaps, instead, it can be re-worded a bit to make it sound something\n> other than mandatory (but I can't think of a good way to phrase it;\n> maybe you can?).\n\nYup, \"if any\" is a bad thing to say, as it does not set the bar for\nthat \"any\" random garbage idea.  A phrase like \"when appropriate\" is\na relatively safe but mostly useless cop-out, as these guidelines\nare written primarily for those who don't yet have proper yardsticks\nto gauge what is appropriate and what isn't.\n\nI think it maybe better to either drop it or make it a sample way to\ndo the second point, i.e. if there are seemingly valid alternative\nwhich may entice readers, explaining why the alternative does not\nwork well and the solution you chose works better *is* a good way to\njustify the way you chose in your change.  Off the top of my head,\nsomething like this?  I am not very happy with the text, though.\n\n\n Documentation/SubmittingPatches | 8 +++++---\n 1 file changed, 5 insertions(+), 3 deletions(-)\n\ndiff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches\nindex 2488544407..4294d0f068 100644\n--- a/Documentation/SubmittingPatches\n+++ b/Documentation/SubmittingPatches\n@@ -125,10 +125,12 @@ The body should provide a meaningful commit message, which:\n . explains the problem the change tries to solve, i.e. what is wrong\n   with the current code without the change.\n \n-. justifies the way the change solves the problem, i.e. why the\n-  result with the change is better.\n+. justifies the way the change solves the problem, i.e. why the result\n+  with the change is better (e.g. explaining the reason why an\n+  seemingly obvious alternative does not work but the solution in the\n+  patch does may be a good way to illustrate the nature of the problem\n+  and how your approach fits it better).\n \n-. alternate solutions considered but discarded, if any.\n \n [[imperative-mood]]\n Describe your changes in imperative mood, e.g. \"make xyzzy do frotz\"\n"}]}