git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: Derrick Stolee <derrickstolee@github.com>
Cc: git@vger.kernel.org, Junio C Hamano <gitster@pobox.com>,
	Johannes Schindelin <johannes.schindelin@gmx.de>,
	Victoria Dye <vdye@github.com>,
	Jiang Xin <worldhello.net@gmail.com>
Subject: Re: [PATCH v2 0/1] scalar: move to the top-level, test, CI and "install" support
Date: Fri, 24 Jun 2022 13:59:36 +0200	[thread overview]
Message-ID: <220624.86y1xmi5wo.gmgdl@evledraar.gmail.com> (raw)
In-Reply-To: <c93c8e75-6c88-ac99-d8c3-1e2e7dd06ea3@github.com>


On Thu, Jun 23 2022, Derrick Stolee wrote:

> On 6/23/2022 11:30 AM, Ævar Arnfjörð Bjarmason wrote:
>> 
>> On Thu, Jun 23 2022, Derrick Stolee wrote:
>> 
>>> On 6/23/2022 6:26 AM, Ævar Arnfjörð Bjarmason wrote:
>>>> This one-patch series integrates the "scalar" command to the
>>>> top-level, meaning we build the "scalar" binary by default, and run
>>>> its tests on "make test" and in CI. We'll also build and test its
>>>> documentation. We now also have "install" support, both for the
>>>> program and its docs, but you'll need to:
>>>>
>>>>     make <install-target> INSTALL_SCALAR=Y
>>>>
>>>> I'm sending this out now to avoid needless duplicate work.
>>>
>>> As mentioned on the list earlier, Victoria is taking over the
>>> remaining work to complete the Scalar project. Nothing has been
>>> sent to the list because we didn't want to cause a distraction
>>> from the release window.
>> 
>> I was on the fence about sending this out, but given that the "CI"
>> thread was going on until the start of that window, and wanting to save
>> her the work of re-discovering the subtle issues with the integration
>> I'd already fixed I thought it was better ot send it out.
>
> The CI thread was halted not just because of the release window,
> but because of a change in who was doing the work and taking time
> to revisit the plan of record. This was communicated on-list [1].

Part of this is me reading between the lines, but as mentioned in the CL
I thought it had more to do with the bug I pointed out on the CI
series's 2/2.

> [1] https://lore.kernel.org/git/nycvar.QRO.7.76.6.2206132246010.353@tvgsbejvaqbjf.bet/
>
> The point is that the end goal is still "Get Scalar out of contrib/"
> but the questions that are still being worked out are things like:
>
> 1. _When_ do we integrate Scalar into CI?
> 2. _When_ do we move Scalar out of contrib/?
> 3. _When_ do we implement the remaining Scalar features?
>
> All three of these questions depend on what the final result will
> look like, and Victoria is still working on developing her own
> opinion on that before presenting it to the list in a complete
> form.
>
> Your patch here is interrupting that process.

Coordination & collaboration can be difficult, but generally speaking we
try to use the ML as a means to decide how topics move forward, and for
coordination.

In this case I understand that there was off-list discussion about how a
part of the codebase should be moving forward among a group of
co-workers who contribute to git.

I don't think there's anything wrong with that, but this really seems
like an overly hostile response to someone who wasn't part of that
process.

Especially as I'm not insisting that I be the one to drive anything
forward on the scalar topic, I think it makes much more sense that it's
Victoria.

This patch is just offered as a way to help that effort along. Perhaps
she'd ack it as-is, find it useful as it reveals some edge cases she
didn't know about, or drop it and go for some other approach. Whatever
gets us to the end-goal sooner than later is fine by me.

Once you "git rm" the scalar Makefile there's not really a lot of ways
you can go other than something approximating the upthread patch. Given
that some of the edge cases are tricky I deemed it worthwhile to share
it.

>>> Victoria is taking time to incorporate your previous thoughts on
>>> how Scalar is built and its location in the codebase and create
>>> a complete narrative of how to get from our current state to that
>>> point.
>> 
>> I wasn't sure she was even aware of it, and given that the WIP patch I
>> saw in my "git fetch" was pretty much a subset of the upthread v1 it
>> seemed that there was needless duplicate work going on.
>> 
>> It seemed clear that that WIP patch was attempting to head in the same
>> direction, but hadn't yet discovered some of the hurdles with
>> e.g. documentation building & installation that I'd fixed
>> already. There's also the CMake integration, which I finished up for
>> this v2.
>
> Poking around in people's forks to discover what they have as WIP
> is not a good measurement of what work is being done or not. A lot
> of mental energy is being spent in this area.
>
> Saying "What I see in this person's fork isn't good enough" is not
> a reason to move forward on your own. WIP work is WORK IN PROGRESS
> and is not assumed to be complete or up to expectation for review
> on the list.

I'm not saying that, but "you seem to be trying to do X, and may or may
not be aware of a past patch that does X, here is is in case it help!.

>> FWIW I have this local change queued on top of this v2, it's all
>> cosmetic, but probably a good idea.
>
> Please stop motivating current patches by work that you have been
> playing with in your local tree. You do this frequently but it is
> not helpful.

After I submit patches to the ML I tend to "bump" the branch I sent it
from, that range-diff was against the submitted v2 to local changes I
applied afterwards, having missed some spots in the initial submission.

I'm not clear on what part of this you're taking issue with...

>> The $(SCALAR_SOURCES) bit is something I missed, but which Victoria
>> didn't in her WIP patch (I stole it from there).
>
> You also mention this in your cover letter (but is notably absent from
> the patch itself).

...yes, hence the "local change queued on top" above, it's a minor
detail I noticed after submitting the patch.

> I can only attribute your insistence here as a lack of respect for
> your fellow contributors. [...]

I really think that's unhelpful. Let's try to assume some mutual good
intent. I think I've explained my motivations above.

> [...]There is no rush for this to happen immediately, so the only
> reason is that you don't trust that it would be done correctly without
> you submitting the work.

Some of the edge cases in the Makefile integration are subtle. I trust
that someone looking at it would probably discover those eventually. I
think it took me about a day of poking around, much of which was a slow
ping-pong with the CI. So someone with a local Windows box could do it
faster.

But if I'd have gotten a patch from my future self with all the learned
edge cases beforehand I'd have appreciated, so I figured I'd send it in.

  reply	other threads:[~2022-06-24 12:49 UTC|newest]

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-10-27 21:51 [Discussion] The architecture of Scalar (and others) within Git Derrick Stolee
2021-10-28  6:56 ` Johannes Schindelin
2021-10-28 15:29 ` Philip Oakley
2021-10-28 18:56 ` [PATCH] contrib: build & test scalar by default, optionally install Ævar Arnfjörð Bjarmason
2022-06-23 10:26   ` [PATCH v2 0/1] scalar: move to the top-level, test, CI and "install" support Ævar Arnfjörð Bjarmason
2022-06-23 10:26     ` [PATCH v2 1/1] scalar: reorganize from contrib/, still keep it "a contrib command" Ævar Arnfjörð Bjarmason
2022-06-23 15:02     ` [PATCH v2 0/1] scalar: move to the top-level, test, CI and "install" support Derrick Stolee
2022-06-23 15:30       ` Ævar Arnfjörð Bjarmason
2022-06-23 16:12         ` Derrick Stolee
2022-06-24 11:59           ` Ævar Arnfjörð Bjarmason [this message]
2022-06-24 21:53             ` Junio C Hamano
2021-10-28 18:58 ` [Discussion] The architecture of Scalar (and others) within Git Ævar Arnfjörð Bjarmason
2021-11-04 17:20   ` Derrick Stolee
2021-11-01 23:20 ` Junio C Hamano
2021-11-04 10:25   ` Johannes Schindelin
2021-11-05  4:20     ` Junio C Hamano

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=220624.86y1xmi5wo.gmgdl@evledraar.gmail.com \
    --to=avarab@gmail.com \
    --cc=derrickstolee@github.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=johannes.schindelin@gmx.de \
    --cc=vdye@github.com \
    --cc=worldhello.net@gmail.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.
Code repositories for project(s) associated with this public 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).