git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: Felipe Contreras <felipe.contreras@gmail.com>
To: Jeff King <peff@peff.net>, Felipe Contreras <felipe.contreras@gmail.com>
Cc: git@vger.kernel.org, "Martin Ågren" <martin.agren@gmail.com>,
	"brian m . carlson" <sandals@crustytoothpaste.net>,
	"Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
Subject: Re: [PATCH 01/11] doc: allow the user to provide ASCIIDOC_EXTRA
Date: Mon, 17 May 2021 11:50:38 -0500	[thread overview]
Message-ID: <60a29edea3d13_12801e20873@natae.notmuch> (raw)
In-Reply-To: <YKJV8HBYCA7hEQiX@coredump.intra.peff.net>

Jeff King wrote:
> On Mon, May 17, 2021 at 05:53:25AM -0500, Felipe Contreras wrote:
> 
> > > It's meant for the caller of "make". Your proposed use is within
> > > doc-diff, but any user running "make ASCIIDOC_EXTRA=foo" would see the
> > > different behavior.
> > 
> > Yeah, they would, but I don't think it would be wrong behavior.
> 
> It depends what they're trying to do. If they write:
> 
>   make ASCIIDOC_EXTRA=--one-extra-option
> 
> then they probably intend to to add to the options we set. If they
> write:
> 
>   make ASCIIDOC_EXTRA='-acompat-mode -atabsize=4 ...etc...'
> 
> with the intent of replicating the flags but changing or removing some
> elements, then it would no longer do what they want.
> 
> I don't mean to imply one is more right than the other (I'd suspect even
> that the override behavior is more likely to be what somebody wants).

Yeah, but I am implying that one is more right than the other.

> I'm mostly pointing out that this is unlike the rest of our Makefiles,
> which do not ever use override (and that the effect is visible to the
> caller, depending on what they want to do).

It's used in the main Makefile, although in a different way.

I see how it is not consistent with the rest of the Makefiles, but I
wonder why it's not being used. It's rather useful.

> > > I'd probably call it ASCIIDOC_FLAGS (like we have CFLAGS and LDFLAGS
> > > that are meant for users to inform us of extra flags they'd like
> > > passed).
> > 
> > Right, but Makefiles do override those, like:
> > 
> >   override CFLAGS += -fPIC
> > 
> > Otherwise builds may fail.
> 
> Some Makefiles do, but in this project we have not historically used
> override. Instead, we provide defaults for things like CFLAGS, expect
> the use to replace them if they like, and then aggregate them (along
> with other internal variables) into things like ALL_CFLAGS.

I know, but status quo is not an argument.

If we always did things the way we've always done things there would
never be progress.

I'm aruging there's no value in giving the user the opportunity to break
the build by doing `make BASIC_CFLAGS=`. Yes, it's more historically
consistent, that doesn't mean it's good.

> > > Of course that may not solve your problem in a sense; if you want
> > > doc-diff to override it, then that might conflict with a theoretical
> > > ASCIIDOC_FLAGS somebody set in their config.mak (but we really are in
> > > the realm of hypothetical here).
> > 
> > Setting ASCIIDOC_FLAGS in config.mk would not override the
> > user-supplied flags any more than setting them in the Makefile (they are
> > virtually the same thing as one includes the other).
> > 
> > It's only if the user has `override ASCIIDOC_FLAGS` in config.mk that
> > such a problem would arise. And that's really hypothetical.
> 
> I mean that if your doc-diff runs:
> 
>   make USE_ASCIIDOCTOR=Yes ASCIIDOC_FLAGS=-adocdate=01/01/1970
> 
> then that will override anything the user put into config.mak. If they
> had some option like:
> 
>   ASCIIDOC_FLAGS = --load-path=/some/special/directory
> 
> they need for asciidoctor to run correctly on their system, then things
> would break for them. But we don't even have a user-facing
> ASCIIDOC_FLAGS now, and nobody is asking for it, so it's pretty
> hypothetical (I'd guess somebody in this situation would just set
> ASCIIDOC="asciidoctor --load-path=...", and that already doesn't work
> with doc-diff).

Exactly, so it's unclear how much value we get by talking about these.

Either way, I don't feel very strongly about `override ASCIIDOC_EXTRA`.
I think it's superior but ASCIIDOC_FLAGS requires less changes, so I'm
fine with that.

Cheers.

-- 
Felipe Contreras

  reply	other threads:[~2021-05-17 16:51 UTC|newest]

Thread overview: 25+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-05-14 12:14 [PATCH 00/11] doc: asciidoctor: direct man page creation and fixes Felipe Contreras
2021-05-14 12:14 ` [PATCH 01/11] doc: allow the user to provide ASCIIDOC_EXTRA Felipe Contreras
2021-05-15  9:32   ` Jeff King
2021-05-15  9:39     ` Jeff King
2021-05-15 12:13       ` Felipe Contreras
2021-05-17  8:57         ` Jeff King
2021-05-17 10:53           ` Felipe Contreras
2021-05-17 11:39             ` Jeff King
2021-05-17 16:50               ` Felipe Contreras [this message]
2021-05-14 12:14 ` [PATCH 02/11] doc: doc-diff: allow more than one flag Felipe Contreras
2021-05-15  9:37   ` Jeff King
2021-05-15 12:11     ` Felipe Contreras
2021-05-14 12:14 ` [PATCH 03/11] doc: doc-diff: set docdate manually Felipe Contreras
2021-05-14 15:43   ` Martin Ågren
2021-05-14 20:33     ` Felipe Contreras
2021-05-14 12:14 ` [PATCH 04/11] doc: use asciidoctor to build man pages directly Felipe Contreras
2021-05-14 15:38   ` Martin Ågren
2021-05-14 20:26     ` Felipe Contreras
2021-05-14 12:14 ` [PATCH 05/11] doc: asciidoctor: add linkgit macros in man pages Felipe Contreras
2021-05-14 12:14 ` [PATCH 06/11] doc: join mansource and manversion Felipe Contreras
2021-05-14 12:14 ` [PATCH 07/11] doc: add man pages workaround for asciidoctor Felipe Contreras
2021-05-14 12:14 ` [PATCH 08/11] doc: asciidoctor: add hack for xrefs Felipe Contreras
2021-05-14 12:14 ` [PATCH 09/11] doc: asciidoctor: add hack to improve links Felipe Contreras
2021-05-14 12:14 ` [PATCH 10/11] doc: asciidoctor: add support for baseurl Felipe Contreras
2021-05-14 12:14 ` [PATCH 11/11] doc: asciidoctor: cleanup man page hack Felipe Contreras

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=60a29edea3d13_12801e20873@natae.notmuch \
    --to=felipe.contreras@gmail.com \
    --cc=avarab@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=martin.agren@gmail.com \
    --cc=peff@peff.net \
    --cc=sandals@crustytoothpaste.net \
    --subject='Re: [PATCH 01/11] doc: allow the user to provide ASCIIDOC_EXTRA' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

git@vger.kernel.org list mirror (unofficial, one of many)

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V1 git git/ https://public-inbox.org/git \
		git@vger.kernel.org
	public-inbox-index git

Example config snippet for mirrors.
Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://7fh6tueqddpjyxjmgtdiueylzoqt6pt7hec3pukyptlmohoowvhde4yd.onion/inbox.comp.version-control.git
	nntp://ie5yzdi7fg72h7s4sdcztq5evakq23rdt33mfyfcddc5u3ndnw24ogqd.onion/inbox.comp.version-control.git
	nntp://4uok3hntl7oi7b4uf4rtfwefqeexfzil2w6kgk2jn5z2f764irre7byd.onion/inbox.comp.version-control.git
	nntp://news.gmane.io/gmane.comp.version-control.git
 note: .onion URLs require Tor: https://www.torproject.org/

code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

AGPL code for this site: git clone https://public-inbox.org/public-inbox.git