All of lore.kernel.org
 help / color / mirror / Atom feed
From: David Aguilar <davvid@gmail.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: "Perry Hutchison" <perryh@pluto.rain.com>,
	"Git Mailing List" <git@vger.kernel.org>,
	"Jonathan Nieder" <jrnieder@gmail.com>,
	"Jeff King" <peff@peff.net>, "René Scharfe" <l.s.r@web.de>
Subject: Re: [PATCH] help: ensure that common-cmds.h is only included by help.c
Date: Sun, 14 Sep 2014 00:35:13 -0700	[thread overview]
Message-ID: <20140914073511.GA39361@gmail.com> (raw)
In-Reply-To: <CAPc5daXDCAi3eP3YmXfcO+9ncN8=b6tCGUFUxwKE=MuRBuXvEg@mail.gmail.com>

On Sat, Sep 13, 2014 at 10:23:03PM -0700, Junio C Hamano wrote:
> On Sat, Sep 13, 2014 at 7:00 PM, Perry Hutchison <perryh@pluto.rain.com> wrote:
> > David Aguilar <davvid@gmail.com> wrote:
> >> Add a #ifndef guard to ensure that common-cmds.h can only
> >> be included by help.c.
> >
> > This strikes me as a very peculiar, and sub-optimal, way of
> > achieving the purpose.  If these definitions are intended to
> > be private to help.c, why not put them there and eliminate
> > common-cmds.h entirely?
> 
> Have you studied where common-cmds.h comes from?
> After you have done so, would you make the same suggestion?
> 
> Having said that, I also do not think this is such a good idea.
> Wouldn't the new "check" script added in this series a better
> place? For example, it may want to make sure that git-compat-util.h
> (or a couple of its equivalents) is the first file included in any mainline
> C source file, and such an inclusion is done unconditionally.
> 
> Which would mean that the checker would scan *.c files with grep
> or a Perl script. It would be trivial to enforce "nobody other than these
> small selected C files is allowed to include common-cmds.h" rule.

Good idea. I implemented this check and the tweaks to make it
pass are small and focused. I'll send these patches shortly.

> Regarding the other patch that butchers many *.h files, I am not
> still very enthused. Including cache.h at the beginning of branch.h,
> for example, would mean git-compat-util.h ends up included at the
> beginning of branch.h.

I can look into Jonathan's forward-decl approach later too.
That'll probably result in less of a butchering.
-- 
David

  reply	other threads:[~2014-09-14  7:35 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2014-09-14  1:11 [PATCH] help: ensure that common-cmds.h is only included by help.c David Aguilar
2014-09-14  2:00 ` Perry Hutchison
2014-09-14  5:23   ` Junio C Hamano
2014-09-14  7:35     ` David Aguilar [this message]
2014-09-14  7:55     ` Perry Hutchison
2014-09-14  8:44       ` David Aguilar

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

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

  git send-email \
    --in-reply-to=20140914073511.GA39361@gmail.com \
    --to=davvid@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=jrnieder@gmail.com \
    --cc=l.s.r@web.de \
    --cc=peff@peff.net \
    --cc=perryh@pluto.rain.com \
    /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
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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.