git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
* [PATCH 0/1] maintenance: Fix a SEGFAULT when no repository when running git maintenance run/start
@ 2020-11-24 16:44 Rafael Silva
  2020-11-24 16:44 ` [PATCH 1/1] maintenance: fix a SEGFAULT when no repository Rafael Silva
  2020-11-26 20:41 ` [PATCH v2 0/1] maintenance: Fix SEGFAULT when running outside of a repository Rafael Silva
  0 siblings, 2 replies; 22+ messages in thread
From: Rafael Silva @ 2020-11-24 16:44 UTC (permalink / raw)
  To: git
  Cc: Derrick Stolee, Nguyễn Thái Ngọc Duy, Jeff King,
	Rafael Silva

In d7514f6ed5 (maintenance: take a lock on the objects directory, 2020-09-17) [1],
and 2fec604f8d (maintenance: add start/stop subcommands, 2020-09-11) [2] The
"git maintenance run" and "git maintenance start" was taught to hold a file-based
lock at the .git/maintenance.lock and .git/schedule.lock respectively because these
operations involves writing data into the .git/repository. 

The lock file path string is built using the the_repository->objects->odb->path,
in case the_repository->objects->odb is NULL when there is not repository available,
resulting in a SEGFAULT.

[1] https://lore.kernel.org/git/1a0a3eebb825ac3eabfdd86f82ed7ef6abb454c5.1600366313.git.gitgitgadget@gmail.com/
[2] https://lore.kernel.org/git/5194f6b1facbd14cc17eea0337c0cc397a2a51fc.1602782524.git.gitgitgadget@gmail.com/

In order to reproduce the error, one can execute maintenance "run" and/or
"start" subcommand with a non valid repository: 

    $ git -C /tmp maintenance start
    Segmentation fault

    $ git -C /tmp maintenance run
    Segmentation fault

The above test was executed from a git built from commit: faefdd61ec (Sixth batch, 2020-11-18):

For reference here's the output from GDB when debugging the "start" command

	Program received signal SIGSEGV, Segmentation fault.
	0x00005555555b9b4c in maintenance_run_tasks (opts=0x7fffffffded4) at builtin/gc.c:1268
	1268		char *lock_path = xstrfmt("%s/maintenance", r->objects->odb->path);

This patch serie adds a check on the maintenance_run_tasks() and update_background_schedule()
to check if the_repository->git_dir is set to validate if a git repository is available and
return error otherwise. The logic is borrowed from maintenance_unregister() that performs
the same validation with different error message.

A new test case is added into t/t7900-maintenance.sh to cover these cases. The test is based on
the assumption that the `/tmp` directory is available, readable and is not a git repository
which I hope is fine for the running the test is all platforms. 

This patch is based on faefdd61ec (Sixth batch, 2020-11-18) (master branch) as the both commits
that introduced the file-based lock are graduated to master already. Hope this also plays nice
with the integration branches maintenance-part-v[1,4]. 

Rafael Silva (1):
  maintenance: fix a SEGFAULT when no repository

 builtin/gc.c           | 14 ++++++++++++--
 t/t7900-maintenance.sh |  5 +++++
 2 files changed, 17 insertions(+), 2 deletions(-)

-- 
2.29.2.505.g04529851e5


^ permalink raw reply	[flat|nested] 22+ messages in thread

* [PATCH 1/1] maintenance: fix a SEGFAULT when no repository
  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 ` Rafael Silva
  2020-11-24 17:22   ` Derrick Stolee
                     ` (2 more replies)
  2020-11-26 20:41 ` [PATCH v2 0/1] maintenance: Fix SEGFAULT when running outside of a repository Rafael Silva
  1 sibling, 3 replies; 22+ messages in thread
From: Rafael Silva @ 2020-11-24 16:44 UTC (permalink / raw)
  To: git
  Cc: Derrick Stolee, Nguyễn Thái Ngọc Duy, Jeff King,
	Rafael Silva

The "git maintenance run" and "git maintenance start" commands holds a
file-based lock at the .git/maintenance.lock and .git/schedule.lock
respectively. These locks are used to ensure only one maintenance process
is executed at the time as both operations involves writing data into
the git repository.

The path to the lock file is built using the "the_repository->objects->odb->path"
that results in SEGFAULT when we have no repository available as
"the_repository->objects->odb" is set to NULL.

Let's teach the maintenance_run_tasks() and update_background_schedule() to return
an error and fails the command when we have no repository available.

Signed-off-by: Rafael Silva <rafaeloliveira.cs@gmail.com>

---
 builtin/gc.c           | 14 ++++++++++++--
 t/t7900-maintenance.sh |  5 +++++
 2 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/builtin/gc.c b/builtin/gc.c
index 3d258b60c2..d133d93a86 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -1265,9 +1265,14 @@ static int maintenance_run_tasks(struct maintenance_run_opts *opts)
 {
 	int i, found_selected = 0;
 	int result = 0;
+	char *lock_path;
 	struct lock_file lk;
 	struct repository *r = the_repository;
-	char *lock_path = xstrfmt("%s/maintenance", r->objects->odb->path);
+
+	if (!r || !r->gitdir)
+		return error(_("not a git repository"));
+
+	lock_path = xstrfmt("%s/maintenance", the_repository->objects->odb->path);
 
 	if (hold_lock_file_for_update(&lk, lock_path, LOCK_NO_DEREF) < 0) {
 		/*
@@ -1513,8 +1518,13 @@ static int update_background_schedule(int run_maintenance)
 	FILE *cron_list, *cron_in;
 	const char *crontab_name;
 	struct strbuf line = STRBUF_INIT;
+	char *lock_path;
 	struct lock_file lk;
-	char *lock_path = xstrfmt("%s/schedule", the_repository->objects->odb->path);
+
+	if (!the_repository || !the_repository->gitdir)
+		return error(_("not a git repository"));
+
+	lock_path = xstrfmt("%s/schedule", the_repository->objects->odb->path);
 
 	if (hold_lock_file_for_update(&lk, lock_path, LOCK_NO_DEREF) < 0)
 		return error(_("another process is scheduling background maintenance"));
diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
index d9e68bb2bf..bb3556888d 100755
--- a/t/t7900-maintenance.sh
+++ b/t/t7900-maintenance.sh
@@ -441,4 +441,9 @@ test_expect_success 'register preserves existing strategy' '
 	test_config maintenance.strategy incremental
 '
 
+test_expect_success 'run and start command fails when no git repository' '
+	test_must_fail git -C /tmp/ maintenance run &&
+	test_must_fail git -C /tmp/ maintenance start
+'
+
 test_done
-- 
2.29.2.505.g04529851e5


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 1/1] maintenance: fix a SEGFAULT when no repository
  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-26  8:22     ` Rafael Silva
  2020-11-24 17:24   ` Eric Sunshine
  2020-11-24 19:03   ` Martin Ågren
  2 siblings, 2 replies; 22+ messages in thread
From: Derrick Stolee @ 2020-11-24 17:22 UTC (permalink / raw)
  To: Rafael Silva, git
  Cc: Derrick Stolee, Nguyễn Thái Ngọc Duy, Jeff King

On 11/24/2020 11:44 AM, Rafael Silva wrote:
> The "git maintenance run" and "git maintenance start" commands holds a
> file-based lock at the .git/maintenance.lock and .git/schedule.lock
> respectively. These locks are used to ensure only one maintenance process
> is executed at the time as both operations involves writing data into
> the git repository.
> 
> The path to the lock file is built using the "the_repository->objects->odb->path"
> that results in SEGFAULT when we have no repository available as
> "the_repository->objects->odb" is set to NULL.
> 
> Let's teach the maintenance_run_tasks() and update_background_schedule() to return
> an error and fails the command when we have no repository available.

Thank you for noticing this problem, and for a quick fix.

While I don't necessarily have a problem with this approach, perhaps
it would be more robust to change the options in git.c to require a
GIT_DIR, as in this diff?

-- >8 --

diff --git a/git.c b/git.c
index 1cab64b5d1..c3dabd2553 100644
--- a/git.c
+++ b/git.c
@@ -530,7 +530,7 @@ static struct cmd_struct commands[] = {
        { "ls-tree", cmd_ls_tree, RUN_SETUP },
        { "mailinfo", cmd_mailinfo, RUN_SETUP_GENTLY | NO_PARSEOPT },
        { "mailsplit", cmd_mailsplit, NO_PARSEOPT },
-       { "maintenance", cmd_maintenance, RUN_SETUP_GENTLY | NO_PARSEOPT },
+       { "maintenance", cmd_maintenance, RUN_SETUP | NO_PARSEOPT },
        { "merge", cmd_merge, RUN_SETUP | NEED_WORK_TREE },
        { "merge-base", cmd_merge_base, RUN_SETUP },
        { "merge-file", cmd_merge_file, RUN_SETUP_GENTLY },

-- >8 --

If the above code change fixes your test (below), then that would
probably be a safer change.

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.

> diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
> index d9e68bb2bf..bb3556888d 100755
> --- a/t/t7900-maintenance.sh
> +++ b/t/t7900-maintenance.sh
> @@ -441,4 +441,9 @@ test_expect_success 'register preserves existing strategy' '
>  	test_config maintenance.strategy incremental
>  '
>  
> +test_expect_success 'run and start command fails when no git repository' '
> +	test_must_fail git -C /tmp/ maintenance run &&
> +	test_must_fail git -C /tmp/ maintenance start
> +'
> +
>  test_done

Thanks,
-Stolee


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 1/1] maintenance: fix a SEGFAULT when no repository
  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 17:24   ` Eric Sunshine
  2020-11-24 19:14     ` SZEDER Gábor
  2020-11-24 19:03   ` Martin Ågren
  2 siblings, 1 reply; 22+ messages in thread
From: Eric Sunshine @ 2020-11-24 17:24 UTC (permalink / raw)
  To: Rafael Silva
  Cc: Git List, Derrick Stolee, Nguyễn Thái Ngọc Duy,
	Jeff King

On Tue, Nov 24, 2020 at 11:45 AM Rafael Silva
<rafaeloliveira.cs@gmail.com> wrote:
> The "git maintenance run" and "git maintenance start" commands holds a
> file-based lock at the .git/maintenance.lock and .git/schedule.lock
> respectively. These locks are used to ensure only one maintenance process
> is executed at the time as both operations involves writing data into
> the git repository.
>
> The path to the lock file is built using the "the_repository->objects->odb->path"
> that results in SEGFAULT when we have no repository available as
> "the_repository->objects->odb" is set to NULL.

This issue came up in review recently[1] in an unrelated way.

[1]: https://lore.kernel.org/git/CAPig+cRFQfg-NLx5dO+BjQpYduhOYs-_+ZRd=DhO8ebWjGB0iA@mail.gmail.com/

> Let's teach the maintenance_run_tasks() and update_background_schedule() to return
> an error and fails the command when we have no repository available.
>
> Signed-off-by: Rafael Silva <rafaeloliveira.cs@gmail.com>
> ---
> diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
> @@ -441,4 +441,9 @@ test_expect_success 'register preserves existing strategy' '
> +test_expect_success 'run and start command fails when no git repository' '
> +       test_must_fail git -C /tmp/ maintenance run &&
> +       test_must_fail git -C /tmp/ maintenance start
> +'

I wouldn't feel comfortable relying upon existence of /tmp/. It might
be sufficient to do this instead:

    mv .git save.git &&
    test_when_finished "mv save.git .git" &&
    test_must_fail git maintenance run &&
    test_must_fail git maintenance start

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 1/1] maintenance: fix a SEGFAULT when no repository
  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 17:24   ` Eric Sunshine
@ 2020-11-24 19:03   ` Martin Ågren
  2020-11-26  7:07     ` Rafael Silva
  2 siblings, 1 reply; 22+ messages in thread
From: Martin Ågren @ 2020-11-24 19:03 UTC (permalink / raw)
  To: Rafael Silva
  Cc: Git Mailing List, Derrick Stolee,
	Nguyễn Thái Ngọc Duy, Jeff King

On Tue, 24 Nov 2020 at 17:47, Rafael Silva <rafaeloliveira.cs@gmail.com> wrote:
> @@ -1265,9 +1265,14 @@ static int maintenance_run_tasks(struct maintenance_run_opts *opts)
>  {
>         int i, found_selected = 0;
>         int result = 0;
> +       char *lock_path;
>         struct lock_file lk;
>         struct repository *r = the_repository;
> -       char *lock_path = xstrfmt("%s/maintenance", r->objects->odb->path);
> +
> +       if (!r || !r->gitdir)
> +               return error(_("not a git repository"));
> +
> +       lock_path = xstrfmt("%s/maintenance", the_repository->objects->odb->path);

s/the_repository/r/

(The preimage uses "r" and you check using "r".)

> @@ -1513,8 +1518,13 @@ static int update_background_schedule(int run_maintenance)
>         FILE *cron_list, *cron_in;
>         const char *crontab_name;
>         struct strbuf line = STRBUF_INIT;
> +       char *lock_path;
>         struct lock_file lk;
> -       char *lock_path = xstrfmt("%s/schedule", the_repository->objects->odb->path);
> +
> +       if (!the_repository || !the_repository->gitdir)
> +               return error(_("not a git repository"));
> +
> +       lock_path = xstrfmt("%s/schedule", the_repository->objects->odb->path);

Here it's "the_repository" both before and after. Ok.

Martin

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 1/1] maintenance: fix a SEGFAULT when no repository
  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
  0 siblings, 2 replies; 22+ messages in thread
From: SZEDER Gábor @ 2020-11-24 19:14 UTC (permalink / raw)
  To: Eric Sunshine
  Cc: Rafael Silva, Git List, Derrick Stolee,
	Nguyễn Thái Ngọc Duy, Jeff King

On Tue, Nov 24, 2020 at 12:24:57PM -0500, Eric Sunshine wrote:
> On Tue, Nov 24, 2020 at 11:45 AM Rafael Silva
> <rafaeloliveira.cs@gmail.com> wrote:
> > diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
> > @@ -441,4 +441,9 @@ test_expect_success 'register preserves existing strategy' '
> > +test_expect_success 'run and start command fails when no git repository' '
> > +       test_must_fail git -C /tmp/ maintenance run &&
> > +       test_must_fail git -C /tmp/ maintenance start
> > +'
> 
> I wouldn't feel comfortable relying upon existence of /tmp/.

Indeed.

> It might
> be sufficient to do this instead:
> 
>     mv .git save.git &&
>     test_when_finished "mv save.git .git" &&
>     test_must_fail git maintenance run &&
>     test_must_fail git maintenance start

Our test library contains the 'nongit' helper function exactly for
this purpose:

    nongit test_must_fail git maintenance run &&
    nongit test_must_fail git maintenance start


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 1/1] maintenance: fix a SEGFAULT when no repository
  2020-11-24 19:14     ` SZEDER Gábor
@ 2020-11-24 19:34       ` Eric Sunshine
  2020-11-26  7:13       ` Rafael Silva
  1 sibling, 0 replies; 22+ messages in thread
From: Eric Sunshine @ 2020-11-24 19:34 UTC (permalink / raw)
  To: SZEDER Gábor
  Cc: Rafael Silva, Git List, Derrick Stolee,
	Nguyễn Thái Ngọc Duy, Jeff King

On Tue, Nov 24, 2020 at 2:14 PM SZEDER Gábor <szeder.dev@gmail.com> wrote:
> On Tue, Nov 24, 2020 at 12:24:57PM -0500, Eric Sunshine wrote:
> >     mv .git save.git &&
> >     test_when_finished "mv save.git .git" &&
> >     test_must_fail git maintenance run &&
> >     test_must_fail git maintenance start
>
> Our test library contains the 'nongit' helper function exactly for
> this purpose:
>
>     nongit test_must_fail git maintenance run &&
>     nongit test_must_fail git maintenance start

Perfect. I forgot about nongit(). Thanks.

I had intended on suggesting GIT_CEILING_DIRECTORIES in my response --
which is what nongit() employs -- but couldn't get it to work for some
reason, so I instead suggested moving .git/ aside temporarily.

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 1/1] maintenance: fix a SEGFAULT when no repository
  2020-11-24 17:22   ` Derrick Stolee
@ 2020-11-24 21:48     ` Junio C Hamano
  2020-11-24 21:59       ` Derrick Stolee
  2020-11-26  8:22     ` Rafael Silva
  1 sibling, 1 reply; 22+ messages in thread
From: Junio C Hamano @ 2020-11-24 21:48 UTC (permalink / raw)
  To: Derrick Stolee
  Cc: Rafael Silva, git, Derrick Stolee,
	Nguyễn Thái Ngọc Duy, Jeff King

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.

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?

But then, being prepared for that future already is also OK, so I
can go either way.  Please figure it out between you ;-)

Thanks, all.

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 1/1] maintenance: fix a SEGFAULT when no repository
  2020-11-24 21:48     ` Junio C Hamano
@ 2020-11-24 21:59       ` Derrick Stolee
  0 siblings, 0 replies; 22+ messages in thread
From: Derrick Stolee @ 2020-11-24 21:59 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: Rafael Silva, git, Derrick Stolee,
	Nguyễn Thái Ngọc Duy, Jeff King

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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 1/1] maintenance: fix a SEGFAULT when no repository
  2020-11-24 19:03   ` Martin Ågren
@ 2020-11-26  7:07     ` Rafael Silva
  0 siblings, 0 replies; 22+ messages in thread
From: Rafael Silva @ 2020-11-26  7:07 UTC (permalink / raw)
  To: Martin Ågren
  Cc: Git Mailing List, Derrick Stolee,
	Nguyễn Thái Ngọc Duy, Jeff King

On Tue, Nov 24, 2020 at 08:03:33PM +0100, Martin Ågren wrote:
> On Tue, 24 Nov 2020 at 17:47, Rafael Silva <rafaeloliveira.cs@gmail.com> wrote:
> > @@ -1265,9 +1265,14 @@ static int maintenance_run_tasks(struct maintenance_run_opts *opts)
> >  {
> >         int i, found_selected = 0;
> >         int result = 0;
> > +       char *lock_path;
> >         struct lock_file lk;
> >         struct repository *r = the_repository;
> > -       char *lock_path = xstrfmt("%s/maintenance", r->objects->odb->path);
> > +
> > +       if (!r || !r->gitdir)
> > +               return error(_("not a git repository"));
> > +
> > +       lock_path = xstrfmt("%s/maintenance", the_repository->objects->odb->path);
> 
> s/the_repository/r/
> 
> (The preimage uses "r" and you check using "r".)

Thanks. will revise this in the next patch version.

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 1/1] maintenance: fix a SEGFAULT when no repository
  2020-11-24 19:14     ` SZEDER Gábor
  2020-11-24 19:34       ` Eric Sunshine
@ 2020-11-26  7:13       ` Rafael Silva
  1 sibling, 0 replies; 22+ messages in thread
From: Rafael Silva @ 2020-11-26  7:13 UTC (permalink / raw)
  To: SZEDER Gábor
  Cc: Eric Sunshine, Git List, Derrick Stolee,
	Nguyễn Thái Ngọc Duy, Jeff King

On Tue, Nov 24, 2020 at 08:14:07PM +0100, SZEDER Gábor wrote:
> On Tue, Nov 24, 2020 at 12:24:57PM -0500, Eric Sunshine wrote:
> > On Tue, Nov 24, 2020 at 11:45 AM Rafael Silva
> > <rafaeloliveira.cs@gmail.com> wrote:
> > > diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
> > > @@ -441,4 +441,9 @@ test_expect_success 'register preserves existing strategy' '
> > > +test_expect_success 'run and start command fails when no git repository' '
> > > +       test_must_fail git -C /tmp/ maintenance run &&
> > > +       test_must_fail git -C /tmp/ maintenance start
> > > +'
> > 
> > I wouldn't feel comfortable relying upon existence of /tmp/.
> 
> Indeed.
> 
> > It might
> > be sufficient to do this instead:
> > 
> >     mv .git save.git &&
> >     test_when_finished "mv save.git .git" &&
> >     test_must_fail git maintenance run &&
> >     test_must_fail git maintenance start
> 
> Our test library contains the 'nongit' helper function exactly for
> this purpose:
> 
>     nongit test_must_fail git maintenance run &&
>     nongit test_must_fail git maintenance start
> 

I did not know that we have such a test helper and will definitely
change on the next revision.

Thank you. 

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 1/1] maintenance: fix a SEGFAULT when no repository
  2020-11-24 17:22   ` Derrick Stolee
  2020-11-24 21:48     ` Junio C Hamano
@ 2020-11-26  8:22     ` Rafael Silva
  2020-11-26 11:21       ` Derrick Stolee
  1 sibling, 1 reply; 22+ messages in thread
From: Rafael Silva @ 2020-11-26  8:22 UTC (permalink / raw)
  To: Derrick Stolee
  Cc: git, Derrick Stolee, Nguyễn Thái Ngọc Duy, Jeff King

On Tue, Nov 24, 2020 at 12:22:40PM -0500, Derrick Stolee wrote:
> On 11/24/2020 11:44 AM, Rafael Silva wrote:
> > The "git maintenance run" and "git maintenance start" commands holds a
> > file-based lock at the .git/maintenance.lock and .git/schedule.lock
> > respectively. These locks are used to ensure only one maintenance process
> > is executed at the time as both operations involves writing data into
> > the git repository.
> > 
> > The path to the lock file is built using the "the_repository->objects->odb->path"
> > that results in SEGFAULT when we have no repository available as
> > "the_repository->objects->odb" is set to NULL.
> > 
> > Let's teach the maintenance_run_tasks() and update_background_schedule() to return
> > an error and fails the command when we have no repository available.
> 
> Thank you for noticing this problem, and for a quick fix.
> 
> While I don't necessarily have a problem with this approach, perhaps
> it would be more robust to change the options in git.c to require a
> GIT_DIR, as in this diff?
> 
> -- >8 --
> 
> diff --git a/git.c b/git.c
> index 1cab64b5d1..c3dabd2553 100644
> --- a/git.c
> +++ b/git.c
> @@ -530,7 +530,7 @@ static struct cmd_struct commands[] = {
>         { "ls-tree", cmd_ls_tree, RUN_SETUP },
>         { "mailinfo", cmd_mailinfo, RUN_SETUP_GENTLY | NO_PARSEOPT },
>         { "mailsplit", cmd_mailsplit, NO_PARSEOPT },
> -       { "maintenance", cmd_maintenance, RUN_SETUP_GENTLY | NO_PARSEOPT },
> +       { "maintenance", cmd_maintenance, RUN_SETUP | NO_PARSEOPT },
>         { "merge", cmd_merge, RUN_SETUP | NEED_WORK_TREE },
>         { "merge-base", cmd_merge_base, RUN_SETUP },
>         { "merge-file", cmd_merge_file, RUN_SETUP_GENTLY },
> 
> -- >8 --
> 

Thank you for the review and the attached patch!

> If the above code change fixes your test (below), then that would
> probably be a safer change.

I agree, switching the maintenance command option to use RUN_SETUP
seems like a nicer approach here. Given the all current operations
requires the command to be executed inside the a git repository this
will make the command consistent across the subcommand. 

Also, it seems this provides an opportunity to cleanup the
register and unregister subcommands that currently implement the
check to ensure the commands are running from a git repository.

> 
> 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.
> 

Indeed. Aside from this reason, another concern that I have is that
switching the validation to all subcommands (on this case by switching
the maintenance command option) will change a bit the behaviour of register
subcommand. Currently, the behaviour of "register" subcommand is to return
with 0 without any messages when running outside of repository and switching
will make the command fail instead.

Nevertheless, I am inclined to go with your suggestion given that it seems
better approach to support the automatically and make the behaviour
consistent for all subcommands given that changing the behaviour of
"git maintenance register" command will (hopefully) be okay.

Thanks,
Rafael

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 1/1] maintenance: fix a SEGFAULT when no repository
  2020-11-26  8:22     ` Rafael Silva
@ 2020-11-26 11:21       ` Derrick Stolee
  0 siblings, 0 replies; 22+ messages in thread
From: Derrick Stolee @ 2020-11-26 11:21 UTC (permalink / raw)
  To: Rafael Silva
  Cc: git, Derrick Stolee, Nguyễn Thái Ngọc Duy, Jeff King

On 11/26/2020 3:22 AM, Rafael Silva wrote:
> On Tue, Nov 24, 2020 at 12:22:40PM -0500, Derrick Stolee wrote:
>> If the above code change fixes your test (below), then that would
>> probably be a safer change.
> 
> I agree, switching the maintenance command option to use RUN_SETUP
> seems like a nicer approach here. Given the all current operations
> requires the command to be executed inside the a git repository this
> will make the command consistent across the subcommand. 
> 
> Also, it seems this provides an opportunity to cleanup the
> register and unregister subcommands that currently implement the
> check to ensure the commands are running from a git repository.
> 
>>
>> 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.
>>
> 
> Indeed. Aside from this reason, another concern that I have is that
> switching the validation to all subcommands (on this case by switching
> the maintenance command option) will change a bit the behaviour of register
> subcommand. Currently, the behaviour of "register" subcommand is to return
> with 0 without any messages when running outside of repository and switching
> will make the command fail instead.

Excellent point. It would be good to cover this case with a test, to
demonstrate that as _intended_ behavior. It makes sense to fail instead
of "succeed" when doing nothing.

> Nevertheless, I am inclined to go with your suggestion given that it seems
> better approach to support the automatically and make the behaviour
> consistent for all subcommands given that changing the behaviour of
> "git maintenance register" command will (hopefully) be okay.

Yes. I would say that changing that behavior aligns it with what it
should be doing. The best news is that the 'register' subcommand does
not exist in a released version of Git, so no one depends on the
current behavior.

Thanks,
-Stolee


^ permalink raw reply	[flat|nested] 22+ messages in thread

* [PATCH v2 0/1] maintenance: Fix SEGFAULT when running outside of a repository
  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-26 20:41 ` Rafael Silva
  2020-11-26 20:41   ` [PATCH v2 1/1] maintenance: fix SEGFAULT when no repository Rafael Silva
  1 sibling, 1 reply; 22+ messages in thread
From: Rafael Silva @ 2020-11-26 20:41 UTC (permalink / raw)
  To: git
  Cc: Derrick Stolee, Nguyễn Thái Ngọc Duy,
	Junio C Hamano, Eric Sunshine, Martin Ågren,
	SZEDER Gábor, Rafael Silva

In d7514f6ed5 (maintenance: take a lock on the objects directory, 2020-09-17) [1],
and 2fec604f8d (maintenance: add start/stop subcommands, 2020-09-11) [2] The
"git maintenance run" and "git maintenance start" was taught to hold a file-based
lock at the .git/maintenance.lock and .git/schedule.lock respectively because these
operations involves writing data into the .git/repository. 

The lock file path string is built using the the_repository->objects->odb->path,
in case the_repository->objects->odb is NULL when there is not repository available,
resulting in a SEGFAULT.

In order to reproduce the error, one can execute maintenance "run" and/or
"start" subcommand with a non valid repository: 

    $ git -C /tmp maintenance start
    Segmentation fault

    $ git -C /tmp maintenance run
    Segmentation fault

The above test was executed from a git built from commit: faefdd61ec (Sixth batch, 2020-11-18):

For reference here's the output from GDB when debugging the "start" command

	Program received signal SIGSEGV, Segmentation fault.
	0x00005555555b9b4c in maintenance_run_tasks (opts=0x7fffffffded4) at builtin/gc.c:1268
	1268		char *lock_path = xstrfmt("%s/maintenance", r->objects->odb->path);


Updates in v2
=============

  * Instead of implementing the check on the subcommands, a more robust approach
    is taken by replacing the "maintenance" command option to use RUN_SETUP
    instead of RUN_SETUP_GENTLY as suggested by Derrick Stolee in [3] as part of
    review cycle from v1.

    This provides protection for all maintenance subcommands given that
    currently all the commands are required to be executed inside a repository.

  * As the RUN_SETUP will enable protection to all commands, the checks in
    maintenance_register() and maintenance_unregister() are removed as they
    are not required anymore.

  * Use the "nongit" helper function for testing it instead of relying on the
    `/tmp` directory for a non valid git repository as suggested by
    SZEDER Gábor and Eric Sunshine in [4].

  * All "git maintenance" subcommands are included on the test to ensure the
   behaviour is consistent for all subcommands. In particular the "register"
   command that current exists without a message and code 0 which will be
   changed by this patch to fail when running outside of a repository. 

   It also worth noting that "register" command does not exists in a released
   version of Git as mentioned in [5] which make it easier for changing the
   current behaviour of the command

[1] https://lore.kernel.org/git/1a0a3eebb825ac3eabfdd86f82ed7ef6abb454c5.1600366313.git.gitgitgadget@gmail.com/
[2] https://lore.kernel.org/git/5194f6b1facbd14cc17eea0337c0cc397a2a51fc.1602782524.git.gitgitgadget@gmail.com/
[3] https://lore.kernel.org/git/1bfd84da-5b74-be10-fc2c-dee80111ee2d@gmail.com/
[4] https://lore.kernel.org/git/20201124191407.GC8396@szeder.dev/
[5] https://lore.kernel.org/git/54fa678c-7150-8c48-50e5-b33923a69249@gmail.com/

Thanks everyone for the insightful and helpful feedback.

Rafael Silva (1):
  maintenance: fix SEGFAULT when no repository

 builtin/gc.c           | 7 -------
 git.c                  | 2 +-
 t/t7900-maintenance.sh | 8 ++++++++
 3 files changed, 9 insertions(+), 8 deletions(-)

-- 
2.29.2.367.g37477fb670


^ permalink raw reply	[flat|nested] 22+ messages in thread

* [PATCH v2 1/1] maintenance: fix SEGFAULT when no repository
  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   ` Rafael Silva
  2020-11-27 20:43     ` Derrick Stolee
  2020-12-08 20:12     ` Josh Steadmon
  0 siblings, 2 replies; 22+ messages in thread
From: Rafael Silva @ 2020-11-26 20:41 UTC (permalink / raw)
  To: git
  Cc: Derrick Stolee, Nguyễn Thái Ngọc Duy,
	Junio C Hamano, Eric Sunshine, Martin Ågren,
	SZEDER Gábor, Rafael Silva

The "git maintenance run" and "git maintenance start/stop" commands
holds a file-based lock at the .git/maintenance.lock and
.git/schedule.lock respectively. These locks are used to ensure only
one maintenance process is executed at the time as both operations
involves writing data into the git repository.

The path to the lock file is built using
"the_repository->objects->odb->path" that results in SEGFAULT when we
have no repository available as "the_repository->objects->odb" is
set to NULL.

Let's teach maintenance command to use RUN_SETUP option that will
provide the validation and fail when running outside of a repository.
Hence fixing the SEGFAULT for all three operations and making the
behaviour consistent across all subcommands.

Setting the RUN_SETUP also provides the same protection for all
subcommands given that the "register" and "unregister" also requires to
be executed inside a repository.

Furthermore let's remove the local validation implemented by the
"register" and "unregister" as this will not be required anymore with
the new option.

Signed-off-by: Rafael Silva <rafaeloliveira.cs@gmail.com>
---
 builtin/gc.c           | 7 -------
 git.c                  | 2 +-
 t/t7900-maintenance.sh | 8 ++++++++
 3 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/builtin/gc.c b/builtin/gc.c
index bc25ad52c7..ebb0158308 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -1446,10 +1446,6 @@ static int maintenance_register(void)
 	struct child_process config_set = CHILD_PROCESS_INIT;
 	struct child_process config_get = CHILD_PROCESS_INIT;
 
-	/* There is no current repository, so skip registering it */
-	if (!the_repository || !the_repository->gitdir)
-		return 0;
-
 	/* Disable foreground maintenance */
 	git_config_set("maintenance.auto", "false");
 
@@ -1486,9 +1482,6 @@ static int maintenance_unregister(void)
 {
 	struct child_process config_unset = CHILD_PROCESS_INIT;
 
-	if (!the_repository || !the_repository->gitdir)
-		return error(_("no current repository to unregister"));
-
 	config_unset.git_cmd = 1;
 	strvec_pushl(&config_unset.args, "config", "--global", "--unset",
 		     "maintenance.repo",
diff --git a/git.c b/git.c
index 4b7bd77b80..a00a0a4d94 100644
--- a/git.c
+++ b/git.c
@@ -535,7 +535,7 @@ static struct cmd_struct commands[] = {
 	{ "ls-tree", cmd_ls_tree, RUN_SETUP },
 	{ "mailinfo", cmd_mailinfo, RUN_SETUP_GENTLY | NO_PARSEOPT },
 	{ "mailsplit", cmd_mailsplit, NO_PARSEOPT },
-	{ "maintenance", cmd_maintenance, RUN_SETUP_GENTLY | NO_PARSEOPT },
+	{ "maintenance", cmd_maintenance, RUN_SETUP | NO_PARSEOPT },
 	{ "merge", cmd_merge, RUN_SETUP | NEED_WORK_TREE },
 	{ "merge-base", cmd_merge_base, RUN_SETUP },
 	{ "merge-file", cmd_merge_file, RUN_SETUP_GENTLY },
diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
index d9e68bb2bf..ae5c29b0ff 100755
--- a/t/t7900-maintenance.sh
+++ b/t/t7900-maintenance.sh
@@ -441,4 +441,12 @@ test_expect_success 'register preserves existing strategy' '
 	test_config maintenance.strategy incremental
 '
 
+test_execpt_success 'fails when running outside of a repository' '
+	nongit test_must_fail git maintenance run &&
+	nongit test_must_fail git maintenance stop &&
+	nongit test_must_fail git maintenance start &&
+	nongit test_must_fail git maintenance register &&
+	nongit test_must_fail git maintenance unregister
+'
+
 test_done
-- 
2.29.2.367.g37477fb670


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v2 1/1] maintenance: fix SEGFAULT when no repository
  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
  1 sibling, 0 replies; 22+ messages in thread
From: Derrick Stolee @ 2020-11-27 20:43 UTC (permalink / raw)
  To: Rafael Silva, git
  Cc: Derrick Stolee, Nguyễn Thái Ngọc Duy,
	Junio C Hamano, Eric Sunshine, Martin Ågren,
	SZEDER Gábor

On 11/26/2020 3:41 PM, Rafael Silva wrote:
> The "git maintenance run" and "git maintenance start/stop" commands
> holds a file-based lock at the .git/maintenance.lock and
> .git/schedule.lock respectively. These locks are used to ensure only
> one maintenance process is executed at the time as both operations
> involves writing data into the git repository.
> 
> The path to the lock file is built using
> "the_repository->objects->odb->path" that results in SEGFAULT when we
> have no repository available as "the_repository->objects->odb" is
> set to NULL.
> 
> Let's teach maintenance command to use RUN_SETUP option that will
> provide the validation and fail when running outside of a repository.
> Hence fixing the SEGFAULT for all three operations and making the
> behaviour consistent across all subcommands.
> 
> Setting the RUN_SETUP also provides the same protection for all
> subcommands given that the "register" and "unregister" also requires to
> be executed inside a repository.
> 
> Furthermore let's remove the local validation implemented by the
> "register" and "unregister" as this will not be required anymore with
> the new option.

Thank you for this very clean patch!

Reviewed-by: Derrick Stolee <dstolee@microsoft.com>


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v2 1/1] maintenance: fix SEGFAULT when no repository
  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-09  9:29       ` Rafael Silva
  1 sibling, 2 replies; 22+ messages in thread
From: Josh Steadmon @ 2020-12-08 20:12 UTC (permalink / raw)
  To: Rafael Silva
  Cc: git, Derrick Stolee, Nguyễn Thái Ngọc Duy,
	Junio C Hamano, Eric Sunshine, Martin Ågren,
	SZEDER Gábor

On 2020.11.26 20:41, Rafael Silva wrote:
> diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
> index d9e68bb2bf..ae5c29b0ff 100755
> --- a/t/t7900-maintenance.sh
> +++ b/t/t7900-maintenance.sh
> @@ -441,4 +441,12 @@ test_expect_success 'register preserves existing strategy' '
>  	test_config maintenance.strategy incremental
>  '
>  
> +test_execpt_success 'fails when running outside of a repository' '
> +	nongit test_must_fail git maintenance run &&
> +	nongit test_must_fail git maintenance stop &&
> +	nongit test_must_fail git maintenance start &&
> +	nongit test_must_fail git maintenance register &&
> +	nongit test_must_fail git maintenance unregister
> +'
> +
>  test_done

Caught a typo here, sending this as a squash patch since it's already in
next:

-- >8 --
Subject: [PATCH] t7900: fix typo: "test_execpt_success"

Signed-off-by: Josh Steadmon <steadmon@google.com>
---
 t/t7900-maintenance.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
index 4ca3617173..8c061197a6 100755
--- a/t/t7900-maintenance.sh
+++ b/t/t7900-maintenance.sh
@@ -441,7 +441,7 @@ test_expect_success 'register preserves existing strategy' '
 	test_config maintenance.strategy incremental
 '
 
-test_execpt_success 'fails when running outside of a repository' '
+test_expect_success 'fails when running outside of a repository' '
 	nongit test_must_fail git maintenance run &&
 	nongit test_must_fail git maintenance stop &&
 	nongit test_must_fail git maintenance start &&
-- 
2.29.2.576.ga3fc446d84-goog


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v2 1/1] maintenance: fix SEGFAULT when no repository
  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-09  9:29       ` Rafael Silva
  1 sibling, 1 reply; 22+ messages in thread
From: Junio C Hamano @ 2020-12-08 21:58 UTC (permalink / raw)
  To: Josh Steadmon
  Cc: Rafael Silva, git, Derrick Stolee,
	Nguyễn Thái Ngọc Duy, Eric Sunshine,
	Martin Ågren, SZEDER Gábor

Josh Steadmon <steadmon@google.com> writes:

> Caught a typo here, sending this as a squash patch since it's already in
> next:

The breakage and the fix looks obvious to me, but...

How did CI allow 'next' to pass with such a typo, I wonder?
Moreover, my pre-push tests of all the integration branches
I didn't notice this to fail, but I cannot see how it could
have been succeeded.  Puzzled...

Will queue, thanks.

>
> -- >8 --
> Subject: [PATCH] t7900: fix typo: "test_execpt_success"
>
> Signed-off-by: Josh Steadmon <steadmon@google.com>
> ---
>  t/t7900-maintenance.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
> index 4ca3617173..8c061197a6 100755
> --- a/t/t7900-maintenance.sh
> +++ b/t/t7900-maintenance.sh
> @@ -441,7 +441,7 @@ test_expect_success 'register preserves existing strategy' '
>  	test_config maintenance.strategy incremental
>  '
>  
> -test_execpt_success 'fails when running outside of a repository' '
> +test_expect_success 'fails when running outside of a repository' '
>  	nongit test_must_fail git maintenance run &&
>  	nongit test_must_fail git maintenance stop &&
>  	nongit test_must_fail git maintenance start &&

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v2 1/1] maintenance: fix SEGFAULT when no repository
  2020-12-08 21:58       ` Junio C Hamano
@ 2020-12-08 22:17         ` Junio C Hamano
  2020-12-24  8:12           ` Christian Couder
  0 siblings, 1 reply; 22+ messages in thread
From: Junio C Hamano @ 2020-12-08 22:17 UTC (permalink / raw)
  To: git
  Cc: Rafael Silva, Derrick Stolee,
	Nguyễn Thái Ngọc Duy, Eric Sunshine,
	Martin Ågren, SZEDER Gábor, Josh Steadmon

Junio C Hamano <gitster@pobox.com> writes:

> Josh Steadmon <steadmon@google.com> writes:
>
>> Caught a typo here, sending this as a squash patch since it's already in
>> next:
>
> The breakage and the fix looks obvious to me, but...
>
> How did CI allow 'next' to pass with such a typo, I wonder?
> Moreover, my pre-push tests of all the integration branches
> I didn't notice this to fail, but I cannot see how it could
> have been succeeded.  Puzzled...

That is because of this:

    $ (sh t7900-maintenance.sh 2>&1; echo $?) | tail -5
    ok 25 - register preserves existing strategy
    t7900-maintenance.sh: line 444: test_execpt_success: command not found
    # passed all 25 test(s)
    1..25
    0

The story is the same with prove.

    $ prove t7900-maintenance-sh
    t7900-maintenance.sh .. 24/? t7900-maintenance.sh: line 444: test_execpt_success: command not found
    t7900-maintenance.sh .. ok
    All tests successful.
    Files=1, Tests=25,  2 wallclock secs ( 0.02 usr  0.01 sys +  0.97 cusr  0.97 csys =  1.97 CPU)
    Result: PASS

Since this typo appeared immediately before test_done, we _could_
improve test_done to pay attention to $? when it starts (and in a
similar fashion, we _could_ also check $? at the beginning of the
test_expect_* for the previous step), but I do not think that is a
good approach that would scale well.  There are legitimate reasons
we have to write things other than test_expect_* at the top level
of the script (e.g. test helper function may have to be defined to
be shared amongst the test pieces in the same script).

I wonder if it is a good direction to go to run the tests with the
"set -e" option on, and accept its peculiarities.

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v2 1/1] maintenance: fix SEGFAULT when no repository
  2020-12-08 20:12     ` Josh Steadmon
  2020-12-08 21:58       ` Junio C Hamano
@ 2020-12-09  9:29       ` Rafael Silva
  1 sibling, 0 replies; 22+ messages in thread
From: Rafael Silva @ 2020-12-09  9:29 UTC (permalink / raw)
  To: Josh Steadmon
  Cc: git, Derrick Stolee, Nguyễn Thái Ngọc Duy,
	Junio C Hamano, Eric Sunshine, Martin Ågren,
	SZEDER Gábor


Josh Steadmon writes:

> On 2020.11.26 20:41, Rafael Silva wrote:
>> diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
>> index d9e68bb2bf..ae5c29b0ff 100755
>> --- a/t/t7900-maintenance.sh
>> +++ b/t/t7900-maintenance.sh
>> @@ -441,4 +441,12 @@ test_expect_success 'register preserves existing strategy' '
>>  	test_config maintenance.strategy incremental
>>  '
>>  
>> +test_execpt_success 'fails when running outside of a repository' '
>> +	nongit test_must_fail git maintenance run &&
>> +	nongit test_must_fail git maintenance stop &&
>> +	nongit test_must_fail git maintenance start &&
>> +	nongit test_must_fail git maintenance register &&
>> +	nongit test_must_fail git maintenance unregister
>> +'
>> +
>>  test_done
>
> Caught a typo here, sending this as a squash patch since it's already in
> next:
>

Ufff. For some reason I completely missed the test error message when
working on the v2.

Thank you Josh, for the catch and quick patch.

Apologize guys for such mistake.

-- 
Thanks, Rafael

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v2 1/1] maintenance: fix SEGFAULT when no repository
  2020-12-08 22:17         ` Junio C Hamano
@ 2020-12-24  8:12           ` Christian Couder
  2020-12-24 14:14             ` Junio C Hamano
  0 siblings, 1 reply; 22+ messages in thread
From: Christian Couder @ 2020-12-24  8:12 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: git, Rafael Silva, Derrick Stolee,
	Nguyễn Thái Ngọc Duy, Eric Sunshine,
	Martin Ågren, SZEDER Gábor, Josh Steadmon

On Wed, Dec 9, 2020 at 2:16 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Junio C Hamano <gitster@pobox.com> writes:
>
> > Josh Steadmon <steadmon@google.com> writes:
> >
> >> Caught a typo here, sending this as a squash patch since it's already in
> >> next:
> >
> > The breakage and the fix looks obvious to me, but...
> >
> > How did CI allow 'next' to pass with such a typo, I wonder?
> > Moreover, my pre-push tests of all the integration branches
> > I didn't notice this to fail, but I cannot see how it could
> > have been succeeded.  Puzzled...
>
> That is because of this:
>
>     $ (sh t7900-maintenance.sh 2>&1; echo $?) | tail -5
>     ok 25 - register preserves existing strategy
>     t7900-maintenance.sh: line 444: test_execpt_success: command not found
>     # passed all 25 test(s)
>     1..25
>     0
>
> The story is the same with prove.
>
>     $ prove t7900-maintenance-sh
>     t7900-maintenance.sh .. 24/? t7900-maintenance.sh: line 444: test_execpt_success: command not found
>     t7900-maintenance.sh .. ok
>     All tests successful.
>     Files=1, Tests=25,  2 wallclock secs ( 0.02 usr  0.01 sys +  0.97 cusr  0.97 csys =  1.97 CPU)
>     Result: PASS
>
> Since this typo appeared immediately before test_done, we _could_
> improve test_done to pay attention to $? when it starts (and in a
> similar fashion, we _could_ also check $? at the beginning of the
> test_expect_* for the previous step), but I do not think that is a
> good approach that would scale well.  There are legitimate reasons
> we have to write things other than test_expect_* at the top level
> of the script (e.g. test helper function may have to be defined to
> be shared amongst the test pieces in the same script).
>
> I wonder if it is a good direction to go to run the tests with the
> "set -e" option on, and accept its peculiarities.

Another solution could be to define a command_not_found_handle
function as bourne shells should call that.

By the way it's not the first time we get such an issue, see:

https://lore.kernel.org/git/CAP8UFD15+p+xKwJ=B9WVsrc+2TvLHKmu78SBCLUFZVSYoTtbbg@mail.gmail.com/

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v2 1/1] maintenance: fix SEGFAULT when no repository
  2020-12-24  8:12           ` Christian Couder
@ 2020-12-24 14:14             ` Junio C Hamano
  0 siblings, 0 replies; 22+ messages in thread
From: Junio C Hamano @ 2020-12-24 14:14 UTC (permalink / raw)
  To: Christian Couder
  Cc: git, Rafael Silva, Derrick Stolee,
	Nguyễn Thái Ngọc Duy, Eric Sunshine,
	Martin Ågren, SZEDER Gábor, Josh Steadmon

Christian Couder <christian.couder@gmail.com> writes:

>> I wonder if it is a good direction to go to run the tests with the
>> "set -e" option on, and accept its peculiarities.
>
> Another solution could be to define a command_not_found_handle
> function as bourne shells should call that.

I am reasonably sure it is bash-ism and a rather stale stackexchange
question seems to say that command_not_found_handler function (note
the 'r' at the end) is its equivalent in zsh.

Having said that, we already have other bash-specific debugging
support in our test harness, and it may not be a bad idea to use the
facility to catch these bugs, even if the support is available only
when running the tests under bash and no other shell.

> By the way it's not the first time we get such an issue, see:
>
> https://lore.kernel.org/git/CAP8UFD15+p+xKwJ=B9WVsrc+2TvLHKmu78SBCLUFZVSYoTtbbg@mail.gmail.com/

Excellent memory.  Thanks.

^ permalink raw reply	[flat|nested] 22+ messages in thread

end of thread, other threads:[~2020-12-24 14:17 UTC | newest]

Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
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
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

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).