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.
next prev parent reply other threads:[~2022-06-24 12:48 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
* 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.
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 NNTP newsgroup(s).