From: Derrick Stolee <stolee@gmail.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: "Rafael Silva" <rafaeloliveira.cs@gmail.com>,
git@vger.kernel.org, "Derrick Stolee" <dstolee@microsoft.com>,
"Nguyễn Thái Ngọc Duy" <pclouds@gmail.com>,
"Jeff King" <peff@peff.net>
Subject: Re: [PATCH 1/1] maintenance: fix a SEGFAULT when no repository
Date: Tue, 24 Nov 2020 16:59:08 -0500 [thread overview]
Message-ID: <7fbe28ff-6d0f-f377-d86f-0b32f12ddee6@gmail.com> (raw)
In-Reply-To: <xmqqim9uifz8.fsf@gitster.c.googlers.com>
On 11/24/2020 4:48 PM, Junio C Hamano wrote:
> Derrick Stolee <stolee@gmail.com> writes:
>
>> The reason to use RUN_SETUP_GENTLY was probably due to some thought
>> of modifying the background maintenance schedule without being in a
>> Git repository. However, we currently run the [un]register logic
>> inside of the stop|start subcommands, so a GIT_DIR is required there,
>> too.
>
> Meaning all the operations we currently support requires to be done
> in a repository? If so, that may be acceptable.
That is the current case.
> When a new operation that does not require to be in a repository is
> added, or when an existing operation is updated not to require to be
> in a repository, reverting the change and then checking in the
> implementation of each operation if we are in a repository instead
> should be easy enough---it pretty much should amount to Rafael's
> patch, right?
Yes, I agree.
> But then, being prepared for that future already is also OK, so I
> can go either way. Please figure it out between you ;-)
The only thing I can think is that using RUN_SETUP protects all
subcommands immediately. It is equally possible that we add a
new subcommand that _also_ requires the GIT_DIR and we need to
be sure to implement this check at the beginning of that method,
too!
But again, I'm just happy that this was found and is being fixed.
Rafael: please feel free to take or ignore my suggestion at your
discretion. The existing review has also been illuminating.
Thanks,
-Stolee
next prev parent reply other threads:[~2020-11-24 21:59 UTC|newest]
Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-11-24 16:44 [PATCH 0/1] maintenance: Fix a SEGFAULT when no repository when running git maintenance run/start Rafael Silva
2020-11-24 16:44 ` [PATCH 1/1] maintenance: fix a SEGFAULT when no repository Rafael Silva
2020-11-24 17:22 ` Derrick Stolee
2020-11-24 21:48 ` Junio C Hamano
2020-11-24 21:59 ` Derrick Stolee [this message]
2020-11-26 8:22 ` Rafael Silva
2020-11-26 11:21 ` Derrick Stolee
2020-11-24 17:24 ` Eric Sunshine
2020-11-24 19:14 ` SZEDER Gábor
2020-11-24 19:34 ` Eric Sunshine
2020-11-26 7:13 ` Rafael Silva
2020-11-24 19:03 ` Martin Ågren
2020-11-26 7:07 ` Rafael Silva
2020-11-26 20:41 ` [PATCH v2 0/1] maintenance: Fix SEGFAULT when running outside of a repository Rafael Silva
2020-11-26 20:41 ` [PATCH v2 1/1] maintenance: fix SEGFAULT when no repository Rafael Silva
2020-11-27 20:43 ` Derrick Stolee
2020-12-08 20:12 ` Josh Steadmon
2020-12-08 21:58 ` Junio C Hamano
2020-12-08 22:17 ` Junio C Hamano
2020-12-24 8:12 ` Christian Couder
2020-12-24 14:14 ` Junio C Hamano
2020-12-09 9:29 ` Rafael Silva
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=7fbe28ff-6d0f-f377-d86f-0b32f12ddee6@gmail.com \
--to=stolee@gmail.com \
--cc=dstolee@microsoft.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=pclouds@gmail.com \
--cc=peff@peff.net \
--cc=rafaeloliveira.cs@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).