git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: Johannes Schindelin <Johannes.Schindelin@gmx.de>
To: Jonathan Nieder <jrnieder@gmail.com>
Cc: "SZEDER Gábor" <szeder.dev@gmail.com>,
	"Josh Steadmon" <steadmon@google.com>,
	git@vger.kernel.org, gitster@pobox.com, git@jeffhostetler.com,
	avarab@gmail.com, peff@peff.net, jnareb@gmail.com
Subject: Re: [PATCH v3 0/3] Add a JSON Schema for trace2 events
Date: Sat, 3 Aug 2019 23:25:45 +0200 (CEST)	[thread overview]
Message-ID: <nycvar.QRO.7.76.6.1908032248010.46@tvgsbejvaqbjf.bet> (raw)
In-Reply-To: <20190802232559.GC109863@google.com>

[-- Attachment #1: Type: text/plain, Size: 5937 bytes --]

Hi Jonathan,

On Fri, 2 Aug 2019, Jonathan Nieder wrote:

> SZEDER Gábor wrote:
> > On Fri, Aug 02, 2019 at 09:59:13AM -0700, Jonathan Nieder wrote:
>
> >> In the short term, we can run tests internally to check that Git keeps
> >> following the schema.  Let's not block patches 1 and 2 by this ---
> >
> > To my understanding patch 2 is only a proof of concept: it starts
> > using a programming language that has not been used before in this
> > project, to implement functionality that is readily available in
> > several existing tools, without even arguing (let alone convincingly
> > arguing) in the commit message why this approach is a good idea.
>
> Well, Golang has been used in contrib/ before. ;-)

Yes, and the rules for contrib/ are a lot more lenient than for the core
of Git for a purpose: we tried to invite a lot of projects into that
folder that would otherwise have fallen undiscoverable and/or
unmaintained (this was before GitHub, and I would argue that the
contrib/ directory's original purpose was kinda outlived by the quite
popular Git hosters out there, with some things like the completions
probably wanting to be promoted to a better directory, i.e. a location
that does suggest that it is maintained within git.git proper, and not
an unbeloved step-child, and others simply be moved into their own
public repository).

But as both you and I know, the patch in question is not at all about
contrib/.

It is about a part that the Git project is asked to maintain, because it
is now part of the CI build, and even worse: it is now expected that the
contributors whose patches break the CI build are able to investigate,
even if it is a bug in that very new validator in that very language
that we did not require contributors to know beforehand.

It is easy for old Unix graybeards to forget that they, too, struggled
with new languages when they started out, and nowadays it is even
harder, as you are kinda stretched between languages and frameworks and
new techniques like multi-threading, closures, algebraic effects, etc
that Unix graybeards will never truly understand.

Just as a reminder: if you truly want to understand, or even only debug,
or even only scratch your own itch with, core Git, you have to know

- C
- Unix shell scripting (POSIX-compliant, including the vagaries of the
  options/particulars of sed, xargs, etc, in particular which ones are
  *not* limited to the GNU flavors)
- Perl
- GNU Makefile (you better understand macros, and the difference between
  `=` and `:=`, and `:` and `::`!)
- Javascript
- Markdown
- AsciiDoc (we seriously do expect contributors to know what AsciiDoc
  makes of a line consisting of a single `+` character!)
- Python
- RUby (for AsciiDoctor extensions)
- Tcl

And that's just core Git.

You won't hear me complaining too much about the above, as I am fluent
enough in all of the above, and since it is a relatively rare skillset,
it makes it easy for me to keep my job.

At the same time, you do realize that this makes it harder than
necessary to get contributions? That it fosters the kind of environment
that makes every potential contributor feel stupid, when it is totally
not their fault that _we_ created this environment?

Adding a new language to the fray, even something as universally
respected as Rust, would be a hard sell.

Golang is much, much harder a sell. I know that you, working at Google,
might think that Go is a mainstream language and not even controversial,
but that just looks a bit biased: Go is very much a language controlled
by Google, much like .NET Framework (although not as much .NET Core) is
controlled by Microsoft. Therefore, I would have considered it to go
without saying that both Go and C# are obviously bad choices for _new_
contributions to core Git, in particular if they are intended to become
part of the CI-tested portion of git.git.  It just looks a bit like
forcing your stuff onto others. I constantly push back against trying to
contribute C# code for that reason.

> If I understand [1] and [2] correctly, we haven't found an existing
> standalone tool that is able to efficiently validate a high volume of
> traces.

Gábor put it very aptly: the problem is the high volume. You are free to
test this as part of your development, but to push this high volume
testing (for almost nobody's benefit) down the throat of git.git's
regular CI is a bit... much.

> But for the purpose of sanity-checking that running a few typical
> commands generates a trace that fits the schema, it's not important
> that the validator be super fast.  So we can use a tool like jq,
> picking one using the criteria that it
>
> - allows performing the validation without too much fuss
> - is likely to already be present on some developers' machines

I don't really care what you use, as long as it is something we don't
need to maintain ourselves, and as long as it is well-maintained in its
own right.

`jq` seems to fit that bill.

I do care as soon as you increase the average runtime of the CI build by
a _double-digit_ percentage, especially if it is for the purpose of a
feature that your team and my team and maybe two or five other teams
might care about, i.e. maybe even as much as 50 people in total, but not
the millions and millions of Git users out there.

We _cannot_ penalize the CI build for a feature a couple of dozen users
want and nobody else, no matter how useful the feature _to them_.

It would have been a totally different matter if the contribution would
have come as a patch to contrib/, with a few test cases marked up with a
prereq that is off unless a user sets a specific environment variable,
so that you could run your integration tests, and nobody else would have
to pay the price.

But that's not what was proposed, and that's why I object.

Ciao,
Dscho

  reply	other threads:[~2019-08-03 21:26 UTC|newest]

Thread overview: 47+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-06-11 23:31 [RFC PATCH " Josh Steadmon
2019-06-11 23:31 ` [RFC PATCH 1/3] trace2: correct trace2 field name documentation Josh Steadmon
2019-06-12 18:00   ` Junio C Hamano
2019-06-12 18:14     ` Josh Steadmon
2019-06-14 15:53   ` Jeff Hostetler
2019-06-11 23:31 ` [RFC PATCH 2/3] trace2: Add a JSON schema for trace2 events Josh Steadmon
2019-06-14 15:59   ` Jeff Hostetler
2019-06-20 17:26     ` Josh Steadmon
2019-06-11 23:31 ` [RFC PATCH 3/3] trace2: add a schema validator " Josh Steadmon
2019-06-12 13:28   ` Ævar Arnfjörð Bjarmason
2019-06-12 16:23     ` Josh Steadmon
2019-06-12 19:18       ` Jeff King
2019-06-20 18:15         ` Josh Steadmon
2019-06-21 11:53       ` Jakub Narebski
2019-06-27 13:57         ` Jeff Hostetler
2019-07-09 23:05 ` [RFC PATCH v2 0/3] Add a JSON Schema " Josh Steadmon
2019-07-09 23:05   ` [RFC PATCH v2 1/3] trace2: Add a JSON schema " Josh Steadmon
2019-07-10 18:32     ` Jakub Narebski
2019-07-24 22:37       ` Josh Steadmon
2019-07-09 23:05   ` [RFC PATCH v2 2/3] trace2: add a schema validator " Josh Steadmon
2019-07-11 13:35     ` Jakub Narebski
2019-07-24 22:47       ` Josh Steadmon
2019-07-09 23:05   ` [RFC PATCH v2 3/3] ci: run trace2 schema validation in the CI suite Josh Steadmon
2019-07-24 23:06 ` [PATCH v3 0/3] Add a JSON Schema for trace2 events Josh Steadmon
2019-07-24 23:06   ` [PATCH v3 1/3] trace2: Add a JSON schema " Josh Steadmon
2019-07-25 16:55     ` Junio C Hamano
2019-07-24 23:06   ` [PATCH v3 2/3] trace2: add a schema validator " Josh Steadmon
2019-07-24 23:06   ` [PATCH v3 3/3] ci: run trace2 schema validation in the CI suite Josh Steadmon
2019-07-25 11:18   ` [PATCH v3 0/3] Add a JSON Schema for trace2 events SZEDER Gábor
2019-07-25 16:14     ` Junio C Hamano
2019-07-26 21:16       ` Josh Steadmon
2019-07-25 23:42   ` SZEDER Gábor
2019-07-26 12:12     ` Johannes Schindelin
2019-07-26 13:53       ` SZEDER Gábor
2019-07-31 11:00         ` Johannes Schindelin
2019-07-26 22:03       ` Josh Steadmon
2019-08-01 18:08         ` Josh Steadmon
2019-08-02  1:52           ` Jonathan Nieder
2019-08-02 11:56             ` Johannes Schindelin
2019-08-02 16:59               ` Jonathan Nieder
2019-08-02 19:38                 ` SZEDER Gábor
2019-08-02 23:25                   ` Jonathan Nieder
2019-08-03 21:25                     ` Johannes Schindelin [this message]
2019-08-02 19:16             ` SZEDER Gábor
2019-08-02 23:06               ` Jonathan Nieder
2019-08-03  7:35                 ` SZEDER Gábor
2019-08-03  7:40                   ` SZEDER Gábor

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=nycvar.QRO.7.76.6.1908032248010.46@tvgsbejvaqbjf.bet \
    --to=johannes.schindelin@gmx.de \
    --cc=avarab@gmail.com \
    --cc=git@jeffhostetler.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=jnareb@gmail.com \
    --cc=jrnieder@gmail.com \
    --cc=peff@peff.net \
    --cc=steadmon@google.com \
    --cc=szeder.dev@gmail.com \
    --subject='Re: [PATCH v3 0/3] Add a JSON Schema for trace2 events' \
    /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

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

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

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).