Message-ID: <20170727085043.74b4jpgv2yhzzhee@gmail.com>
Date:   Thu, 27 Jul 2017 10:50:43 +0200
From:   Ingo Molnar <mingo@...nel.org>
To:     Dan Williams <dan.j.williams@...el.com>
Cc:     Arnaldo Carvalho de Melo <acme@...nel.org>,
        "linux-nvdimm@...ts.01.org" <linux-nvdimm@...ts.01.org>,
        Peter Zijlstra <peterz@...radead.org>,
        Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
        Ingo Molnar <mingo@...hat.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Peter Zijlstra <a.p.zijlstra@...llo.nl>,
        "H. Peter Anvin" <hpa@...or.com>,
        Andrew Morton <akpm@...ux-foundation.org>
Subject: Re: [PATCH 3/4] ndctl: switch to tools/include/linux/{kernel, list,
 bitmap}.h


* Dan Williams <dan.j.williams@...el.com> wrote:

> On Wed, Jul 26, 2017 at 10:19 AM, Ingo Molnar <mingo@...nel.org> wrote:
> >
> > * Dan Williams <dan.j.williams@...el.com> wrote:
> >
> >> On Wed, Jul 26, 2017 at 4:29 AM, Ingo Molnar <mingo@...nel.org> wrote:
> >> >
> >> > * Dan Williams <dan.j.williams@...el.com> wrote:
> >> >
> >> >> On Tue, Jul 25, 2017 at 4:55 PM, Arnaldo Carvalho de Melo
> >> >> <acme@...nel.org> wrote:
> >> >> > Em Tue, Jul 25, 2017 at 03:36:26PM -0700, Dan Williams escreveu:
> >> >> >> Replace the ccan implementation of list primitives, bitmap helpers and
> >> >> >> small utility macros with the common definitions available in
> >> >> >> tool/include/linux.
> >> >> >
> >> >> > You should first add what you need in separate patches, paving the way
> >> >> > to then use it, and some stuff are already there, see below:
> >> >>
> >> >> Ok, I'll break out those changes separately.
> >> >
> >> > BTW., another general observation I have is that ndctl uses autotools - while perf
> >> > uses its own build system, some of which is abstracted out into tools/build/ and
> >> > reused by other tooling projects as well.
> >> >
> >> > I despise autotools with a passion, it's slow, bloated, and encourages all sorts
> >> > of bad API/ABI practices that plagues many OSS projects. I know that Linus
> >> > explicitly did a Makefile based build system for Git for (I think) similar
> >> > reasons.
> >> >
> >> > It might be a good idea to not let autotools into the kernel tooling tree, not
> >> > because ndctl's use of autotools is bad in any fashion (it appears to be a fairly
> >> > straightforward use), but to generally encourage good API/ABI practices in our
> >> > tooling space, and to encourage enhancements to the tools/build/ infrastructure.
> >>
> >> That's a fair point. Regardless, autotools will be in the git history,
> >> but if you'd like to see the final merge product eliminate its use, I
> >> can't really argue otherwise. I was originally not concerned because
> >> tools/usb/usbip/ was an existing in tree autotools user. In any event
> >> if you want the autotools removal to be done out-of-tree I'll need to
> >> put this effort on the back burner until 4.15.
> >
> > So that was another thing I wanted to suggest: why not import the current ndctl
> > version as a single commit?
> >
> > I had a quick look, and there's quite a few of commits in the ndctl history that
> > don't conform to kernel standards, such as:
> >
> >   ce881c1e78f6: ndctl: seed tracking
> >
> > which doesn't have any Signed-off-by tags.
> >
> > There's also commits with ambiguous titles that would be confusing in the kernel
> > context - for example:
> >
> >   796b6f373dec: clarify copyright and license information
> >
> > ... which on the surface could be misunderstood as something talking about the
> > kernel copyright ...
> >
> > Or:
> >
> >   e38bd36e5d0a: completion: updates for file name completion
> >
> > which I could initially mistake for a commit about scheduler completions ;-)
> >
> > Or:
> >
> >   2ad6a39c9ae9: Fix attribute sizes to match NFIT 0.8s2
> >   cc7cb44385d3: Import initial infrastructure
> >
> > etc.
> >
> > I suppose all that could be corrected, SOBs added, titles clarified and prefixed
> > with tools/ndctl, but then it wouldn't really be unmodified history anymore,
> > right?
> >
> > At that point we might as well do a clean start - and not import ~500 extra
> > commits into the kernel tree?
> 
> I think keeping the history is worth it, similar reasoning to why we
> kept the btrfs history.

Well, there's two key differences:

 - btrfs _is_ kernel code and as such was developed as a kernel module,
   albeit out of tree. ndctl is a standalone tooling project.

 - all the old btrfs commit titles are either prefixed with 'btrfs:', or have it
   as a string in the title, which made the merge unproblematic and unambiguous.

> [...] Also, since the history is linear and already rewritten by 'git 
> filter-branch' to move everything to tools/ndctl/ it wouldn't be that much more 
> work to go clean up these few commits that are problematic.

So I think that's where to draw the line - grafting two kernel tree commit 
histories like for btrfs might be OK (just the repositories were incompatible), 
but if commit logs need to be rewritten manually then that's essentially 
whitewashing of history - which is a big no-no in Git flows.

Anyway, it's up to Linus I guess.

Thanks,

	Ingo

Powered by blists - more mailing lists