[PATCH 0/2] reftable: adjust permissions of compacted tables

[PATCH 0/2] reftable: adjust permissions of compacted tables

[Patrick Steinhardt]

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

Hi,

this small patch series fixes a bug in the reftable stack implementation
where the "default_permissions" write option is not honored for newly
written compacted tables. The effect is that the "core.sharedRepository"
config is ignored when performing compaction.

The series runs into two minor conflicts with jc/reftable-core-fsync
that can be fixed like follows. As this series has been merged to next
already I'd also be happy to rebase these patches on top of it. Please
let me know your preference.

Patrick

diff --cc reftable/stack.c
index 27cc586460,ab295341cc..0000000000
--- a/reftable/stack.c
+++ b/reftable/stack.c
@@@ -633,13 -643,13 +643,13 @@@ int reftable_addition_add(struct reftab
                err = REFTABLE_IO_ERROR;
                goto done;
        }
 -      if (add->stack->config.default_permissions) {
 -              if (chmod(temp_tab_file_name.buf, add->stack->config.default_permissions)) {
 -                      err = REFTABLE_IO_ERROR;
 -                      goto done;
 -              }
 +      if (add->stack->config.default_permissions &&
 +          fchmod(tab_fd, add->stack->config.default_permissions) < 0) {
 +              err = REFTABLE_IO_ERROR;
 +              goto done;
        }
 +
-       wr = reftable_new_writer(reftable_fd_write, &tab_fd,
+       wr = reftable_new_writer(reftable_fd_write, reftable_fd_flush, &tab_fd,
                                 &add->stack->config);
        err = write_table(wr, arg);
        if (err < 0)
@@@ -731,13 -741,7 +741,13 @@@ static int stack_compact_locked(struct
        strbuf_addstr(temp_tab, ".temp.XXXXXX");

        tab_fd = mkstemp(temp_tab->buf);
 +      if (st->config.default_permissions &&
 +          fchmod(tab_fd, st->config.default_permissions) < 0) {
 +              err = REFTABLE_IO_ERROR;
 +              goto done;
 +      }
 +
-       wr = reftable_new_writer(reftable_fd_write, &tab_fd, &st->config);
+       wr = reftable_new_writer(reftable_fd_write, reftable_fd_flush, &tab_fd, &st->config);

        err = stack_write_compact(st, wr, first, last, config);
        if (err < 0)

Patrick Steinhardt (2):
  reftable/stack: use fchmod(3P) to set permissions
  reftable/stack: adjust permissions of compacted tables

 reftable/stack.c      | 35 ++++++++++++++++++++---------------
 reftable/stack_test.c | 25 +++++++++++++++++++++++--
 2 files changed, 43 insertions(+), 17 deletions(-)

-- 
2.43.GIT


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 1/2] reftable/stack: use fchmod(3P) to set permissions

[Patrick Steinhardt]

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

We use chmod(3P) to modify permissions of "tables.list" locks as well as
temporary new tables we're writing. In all of these cases we do have a
file descriptor readily available though. So instead of using chmod(3P)
we can use fchmod(3P), which should both be more efficient while also
avoiding a potential race where we change permissions of the wrong file
in case it was swapped out after we have created it.

Refactor the code to do so.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 reftable/stack.c | 29 ++++++++++++++---------------
 1 file changed, 14 insertions(+), 15 deletions(-)

diff --git a/reftable/stack.c b/reftable/stack.c
index 7ffeb3ee10..c6e4dc4b2b 100644
--- a/reftable/stack.c
+++ b/reftable/stack.c
@@ -467,11 +467,11 @@ static int reftable_stack_init_addition(struct reftable_addition *add,
 		}
 		goto done;
 	}
-	if (st->config.default_permissions) {
-		if (chmod(add->lock_file->filename.buf, st->config.default_permissions) < 0) {
-			err = REFTABLE_IO_ERROR;
-			goto done;
-		}
+	if (st->config.default_permissions &&
+	    fchmod(get_tempfile_fd(add->lock_file),
+		   st->config.default_permissions) < 0) {
+		err = REFTABLE_IO_ERROR;
+		goto done;
 	}
 
 	err = stack_uptodate(st);
@@ -633,12 +633,12 @@ int reftable_addition_add(struct reftable_addition *add,
 		err = REFTABLE_IO_ERROR;
 		goto done;
 	}
-	if (add->stack->config.default_permissions) {
-		if (chmod(temp_tab_file_name.buf, add->stack->config.default_permissions)) {
-			err = REFTABLE_IO_ERROR;
-			goto done;
-		}
+	if (add->stack->config.default_permissions &&
+	    fchmod(tab_fd, add->stack->config.default_permissions) < 0) {
+		err = REFTABLE_IO_ERROR;
+		goto done;
 	}
+
 	wr = reftable_new_writer(reftable_fd_write, &tab_fd,
 				 &add->stack->config);
 	err = write_table(wr, arg);
@@ -967,11 +967,10 @@ static int stack_compact_range(struct reftable_stack *st, int first, int last,
 		goto done;
 	}
 	have_lock = 1;
-	if (st->config.default_permissions) {
-		if (chmod(lock_file_name.buf, st->config.default_permissions) < 0) {
-			err = REFTABLE_IO_ERROR;
-			goto done;
-		}
+	if (st->config.default_permissions &&
+	    fchmod(lock_file_fd, st->config.default_permissions) < 0) {
+		err = REFTABLE_IO_ERROR;
+		goto done;
 	}
 
 	format_name(&new_table_name, st->readers[first]->min_update_index,
-- 
2.43.GIT


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 2/2] reftable/stack: adjust permissions of compacted tables

[Patrick Steinhardt]

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

When creating a new compacted table from a range of preexisting ones we
don't set the default permissions on the resulting table when specified
by the user. This has the effect that the "core.sharedRepository" config
will not be honored correctly.

Fix this bug and add a test to catch this issue.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 reftable/stack.c      |  6 ++++++
 reftable/stack_test.c | 25 +++++++++++++++++++++++--
 2 files changed, 29 insertions(+), 2 deletions(-)

diff --git a/reftable/stack.c b/reftable/stack.c
index c6e4dc4b2b..27cc586460 100644
--- a/reftable/stack.c
+++ b/reftable/stack.c
@@ -731,6 +731,12 @@ static int stack_compact_locked(struct reftable_stack *st, int first, int last,
 	strbuf_addstr(temp_tab, ".temp.XXXXXX");
 
 	tab_fd = mkstemp(temp_tab->buf);
+	if (st->config.default_permissions &&
+	    fchmod(tab_fd, st->config.default_permissions) < 0) {
+		err = REFTABLE_IO_ERROR;
+		goto done;
+	}
+
 	wr = reftable_new_writer(reftable_fd_write, &tab_fd, &st->config);
 
 	err = stack_write_compact(st, wr, first, last, config);
diff --git a/reftable/stack_test.c b/reftable/stack_test.c
index 289e902146..2e7d1768b7 100644
--- a/reftable/stack_test.c
+++ b/reftable/stack_test.c
@@ -443,15 +443,16 @@ static void test_reftable_stack_add(void)
 	int err = 0;
 	struct reftable_write_options cfg = {
 		.exact_log_message = 1,
+		.default_permissions = 0660,
 	};
 	struct reftable_stack *st = NULL;
 	char *dir = get_tmp_dir(__LINE__);
-
 	struct reftable_ref_record refs[2] = { { NULL } };
 	struct reftable_log_record logs[2] = { { NULL } };
+	struct strbuf scratch = STRBUF_INIT;
+	struct stat stat_result;
 	int N = ARRAY_SIZE(refs);
 
-
 	err = reftable_new_stack(&st, dir, cfg);
 	EXPECT_ERR(err);
 	st->disable_auto_compact = 1;
@@ -509,12 +510,32 @@ static void test_reftable_stack_add(void)
 		reftable_log_record_release(&dest);
 	}
 
+#ifndef GIT_WINDOWS_NATIVE
+	strbuf_addstr(&scratch, dir);
+	strbuf_addstr(&scratch, "/tables.list");
+	err = stat(scratch.buf, &stat_result);
+	EXPECT(!err);
+	EXPECT((stat_result.st_mode & 0777) == cfg.default_permissions);
+
+	strbuf_reset(&scratch);
+	strbuf_addstr(&scratch, dir);
+	strbuf_addstr(&scratch, "/");
+	/* do not try at home; not an external API for reftable. */
+	strbuf_addstr(&scratch, st->readers[0]->name);
+	err = stat(scratch.buf, &stat_result);
+	EXPECT(!err);
+	EXPECT((stat_result.st_mode & 0777) == cfg.default_permissions);
+#else
+	(void) stat_result;
+#endif
+
 	/* cleanup */
 	reftable_stack_destroy(st);
 	for (i = 0; i < N; i++) {
 		reftable_ref_record_release(&refs[i]);
 		reftable_log_record_release(&logs[i]);
 	}
+	strbuf_release(&scratch);
 	clear_dir(dir);
 }
 
-- 
2.43.GIT


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v2] reftable/stack: adjust permissions of compacted tables

[Patrick Steinhardt]

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

When creating a new compacted table from a range of preexisting ones we
don't set the default permissions on the resulting table when specified
by the user. This has the effect that the "core.sharedRepository" config
will not be honored correctly.

Fix this bug and add a test to catch this issue.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---

Ugh, I totally forgot about the fact that fchmod(3P) isn't actually
available on Windows. I've thus evicted the first patch and changed the
second patch to use chmod(3P) instead. Too bad.

 reftable/stack.c      |  6 ++++++
 reftable/stack_test.c | 25 +++++++++++++++++++++++--
 2 files changed, 29 insertions(+), 2 deletions(-)

diff --git a/reftable/stack.c b/reftable/stack.c
index 7ffeb3ee10..38e784a8ab 100644
--- a/reftable/stack.c
+++ b/reftable/stack.c
@@ -731,6 +731,12 @@ static int stack_compact_locked(struct reftable_stack *st, int first, int last,
 	strbuf_addstr(temp_tab, ".temp.XXXXXX");
 
 	tab_fd = mkstemp(temp_tab->buf);
+	if (st->config.default_permissions &&
+	    chmod(temp_tab->buf, st->config.default_permissions) < 0) {
+		err = REFTABLE_IO_ERROR;
+		goto done;
+	}
+
 	wr = reftable_new_writer(reftable_fd_write, &tab_fd, &st->config);
 
 	err = stack_write_compact(st, wr, first, last, config);
diff --git a/reftable/stack_test.c b/reftable/stack_test.c
index 289e902146..2e7d1768b7 100644
--- a/reftable/stack_test.c
+++ b/reftable/stack_test.c
@@ -443,15 +443,16 @@ static void test_reftable_stack_add(void)
 	int err = 0;
 	struct reftable_write_options cfg = {
 		.exact_log_message = 1,
+		.default_permissions = 0660,
 	};
 	struct reftable_stack *st = NULL;
 	char *dir = get_tmp_dir(__LINE__);
-
 	struct reftable_ref_record refs[2] = { { NULL } };
 	struct reftable_log_record logs[2] = { { NULL } };
+	struct strbuf scratch = STRBUF_INIT;
+	struct stat stat_result;
 	int N = ARRAY_SIZE(refs);
 
-
 	err = reftable_new_stack(&st, dir, cfg);
 	EXPECT_ERR(err);
 	st->disable_auto_compact = 1;
@@ -509,12 +510,32 @@ static void test_reftable_stack_add(void)
 		reftable_log_record_release(&dest);
 	}
 
+#ifndef GIT_WINDOWS_NATIVE
+	strbuf_addstr(&scratch, dir);
+	strbuf_addstr(&scratch, "/tables.list");
+	err = stat(scratch.buf, &stat_result);
+	EXPECT(!err);
+	EXPECT((stat_result.st_mode & 0777) == cfg.default_permissions);
+
+	strbuf_reset(&scratch);
+	strbuf_addstr(&scratch, dir);
+	strbuf_addstr(&scratch, "/");
+	/* do not try at home; not an external API for reftable. */
+	strbuf_addstr(&scratch, st->readers[0]->name);
+	err = stat(scratch.buf, &stat_result);
+	EXPECT(!err);
+	EXPECT((stat_result.st_mode & 0777) == cfg.default_permissions);
+#else
+	(void) stat_result;
+#endif
+
 	/* cleanup */
 	reftable_stack_destroy(st);
 	for (i = 0; i < N; i++) {
 		reftable_ref_record_release(&refs[i]);
 		reftable_log_record_release(&logs[i]);
 	}
+	strbuf_release(&scratch);
 	clear_dir(dir);
 }
 

base-commit: e02ecfcc534e2021aae29077a958dd11c3897e4c
-- 
2.43.GIT


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2] reftable/stack: adjust permissions of compacted tables

[Karthik Nayak]

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

Patrick Steinhardt <ps@pks.im> writes:

> When creating a new compacted table from a range of preexisting ones we
> don't set the default permissions on the resulting table when specified
> by the user. This has the effect that the "core.sharedRepository" config
> will not be honored correctly.
>
> Fix this bug and add a test to catch this issue.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>
> Ugh, I totally forgot about the fact that fchmod(3P) isn't actually
> available on Windows. I've thus evicted the first patch and changed the
> second patch to use chmod(3P) instead. Too bad.
>

Good catch. This version looks good to me! Nothing to add :)

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]

Re: [PATCH v2] reftable/stack: adjust permissions of compacted tables

[Justin Tobler]

On Wed, Jan 24, 2024 at 7:21 AM Patrick Steinhardt <ps@pks.im> wrote:
>
> diff --git a/reftable/stack_test.c b/reftable/stack_test.c
> index 289e902146..2e7d1768b7 100644
> --- a/reftable/stack_test.c
> +++ b/reftable/stack_test.c
> @@ -443,15 +443,16 @@ static void test_reftable_stack_add(void)
>         int err = 0;
>         struct reftable_write_options cfg = {
>                 .exact_log_message = 1,
> +               .default_permissions = 0660,
>         };
>         struct reftable_stack *st = NULL;
>         char *dir = get_tmp_dir(__LINE__);
> -
>         struct reftable_ref_record refs[2] = { { NULL } };
>         struct reftable_log_record logs[2] = { { NULL } };
> +       struct strbuf scratch = STRBUF_INIT;

The variable name `scratch` seems rather vague to me as opposed to something
like `path`. After a quick search though, `scratch` appears to be a fairly
common name used in similar scenarios. So probably not a big deal, but
something
I thought I'd mention.

> +       struct stat stat_result;
>         int N = ARRAY_SIZE(refs);
>
> -
>         err = reftable_new_stack(&st, dir, cfg);
>         EXPECT_ERR(err);
>         st->disable_auto_compact = 1;
> @@ -509,12 +510,32 @@ static void test_reftable_stack_add(void)
>                 reftable_log_record_release(&dest);
>         }
>
> +#ifndef GIT_WINDOWS_NATIVE
> +       strbuf_addstr(&scratch, dir);
> +       strbuf_addstr(&scratch, "/tables.list");
> +       err = stat(scratch.buf, &stat_result);
> +       EXPECT(!err);
> +       EXPECT((stat_result.st_mode & 0777) == cfg.default_permissions);
> +
> +       strbuf_reset(&scratch);
> +       strbuf_addstr(&scratch, dir);
> +       strbuf_addstr(&scratch, "/");
> +       /* do not try at home; not an external API for reftable. */
> +       strbuf_addstr(&scratch, st->readers[0]->name);
> +       err = stat(scratch.buf, &stat_result);
> +       EXPECT(!err);
> +       EXPECT((stat_result.st_mode & 0777) == cfg.default_permissions);
> +#else
> +       (void) stat_result;
> +#endif

Why do we ignore Windows here? And would it warrant explaining in the commit
message?

-Justin

Re: [PATCH v2] reftable/stack: adjust permissions of compacted tables

[Patrick Steinhardt]

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

On Thu, Jan 25, 2024 at 10:02:15AM -0600, Justin Tobler wrote:
> On Wed, Jan 24, 2024 at 7:21 AM Patrick Steinhardt <ps@pks.im> wrote:
> >
> > diff --git a/reftable/stack_test.c b/reftable/stack_test.c
> > index 289e902146..2e7d1768b7 100644
> > --- a/reftable/stack_test.c
> > +++ b/reftable/stack_test.c
> > @@ -443,15 +443,16 @@ static void test_reftable_stack_add(void)
> >         int err = 0;
> >         struct reftable_write_options cfg = {
> >                 .exact_log_message = 1,
> > +               .default_permissions = 0660,
> >         };
> >         struct reftable_stack *st = NULL;
> >         char *dir = get_tmp_dir(__LINE__);
> > -
> >         struct reftable_ref_record refs[2] = { { NULL } };
> >         struct reftable_log_record logs[2] = { { NULL } };
> > +       struct strbuf scratch = STRBUF_INIT;
> 
> The variable name `scratch` seems rather vague to me as opposed to something
> like `path`. After a quick search though, `scratch` appears to be a fairly
> common name used in similar scenarios. So probably not a big deal, but
> something
> I thought I'd mention.

Yeah. I basically copied the below checks from another test where we
already had the permission checks, and also adopted the name of the
`scratch` variable. I agree though that `path` would be a better name,
so let me change it.

> > +       struct stat stat_result;
> >         int N = ARRAY_SIZE(refs);
> >
> > -
> >         err = reftable_new_stack(&st, dir, cfg);
> >         EXPECT_ERR(err);
> >         st->disable_auto_compact = 1;
> > @@ -509,12 +510,32 @@ static void test_reftable_stack_add(void)
> >                 reftable_log_record_release(&dest);
> >         }
> >
> > +#ifndef GIT_WINDOWS_NATIVE
> > +       strbuf_addstr(&scratch, dir);
> > +       strbuf_addstr(&scratch, "/tables.list");
> > +       err = stat(scratch.buf, &stat_result);
> > +       EXPECT(!err);
> > +       EXPECT((stat_result.st_mode & 0777) == cfg.default_permissions);
> > +
> > +       strbuf_reset(&scratch);
> > +       strbuf_addstr(&scratch, dir);
> > +       strbuf_addstr(&scratch, "/");
> > +       /* do not try at home; not an external API for reftable. */
> > +       strbuf_addstr(&scratch, st->readers[0]->name);
> > +       err = stat(scratch.buf, &stat_result);
> > +       EXPECT(!err);
> > +       EXPECT((stat_result.st_mode & 0777) == cfg.default_permissions);
> > +#else
> > +       (void) stat_result;
> > +#endif
> 
> Why do we ignore Windows here? And would it warrant explaining in the commit
> message?

Because Windows has a different acccess control model for files and
doesn't natively use POSIX permissions. I'm not a 100% sure whether we
do emulate the permission bits or not, but I cannot test on Windows and
the other test where this was ripped out of also makes the code
conditional.

Will explain in the commit message.

Thanks!

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v3] reftable/stack: adjust permissions of compacted tables

[Patrick Steinhardt]

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

When creating a new compacted table from a range of preexisting ones we
don't set the default permissions on the resulting table when specified
by the user. This has the effect that the "core.sharedRepository" config
will not be honored correctly.

Fix this bug and add a test to catch this issue. Note that we only test
on non-Windows platforms because Windows does not use POSIX permissions
natively.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---

Changes compared to v2:

  - Extended the commit message to say why we don't test on Windows
    systems.

  - Renamed the `scratch` variable to `path`.

Thanks!

Patrick

 reftable/stack.c      |  6 ++++++
 reftable/stack_test.c | 25 +++++++++++++++++++++++--
 2 files changed, 29 insertions(+), 2 deletions(-)

diff --git a/reftable/stack.c b/reftable/stack.c
index 7ffeb3ee10..38e784a8ab 100644
--- a/reftable/stack.c
+++ b/reftable/stack.c
@@ -731,6 +731,12 @@ static int stack_compact_locked(struct reftable_stack *st, int first, int last,
 	strbuf_addstr(temp_tab, ".temp.XXXXXX");
 
 	tab_fd = mkstemp(temp_tab->buf);
+	if (st->config.default_permissions &&
+	    chmod(temp_tab->buf, st->config.default_permissions) < 0) {
+		err = REFTABLE_IO_ERROR;
+		goto done;
+	}
+
 	wr = reftable_new_writer(reftable_fd_write, &tab_fd, &st->config);
 
 	err = stack_write_compact(st, wr, first, last, config);
diff --git a/reftable/stack_test.c b/reftable/stack_test.c
index 289e902146..5089392f7b 100644
--- a/reftable/stack_test.c
+++ b/reftable/stack_test.c
@@ -443,15 +443,16 @@ static void test_reftable_stack_add(void)
 	int err = 0;
 	struct reftable_write_options cfg = {
 		.exact_log_message = 1,
+		.default_permissions = 0660,
 	};
 	struct reftable_stack *st = NULL;
 	char *dir = get_tmp_dir(__LINE__);
-
 	struct reftable_ref_record refs[2] = { { NULL } };
 	struct reftable_log_record logs[2] = { { NULL } };
+	struct strbuf path = STRBUF_INIT;
+	struct stat stat_result;
 	int N = ARRAY_SIZE(refs);
 
-
 	err = reftable_new_stack(&st, dir, cfg);
 	EXPECT_ERR(err);
 	st->disable_auto_compact = 1;
@@ -509,12 +510,32 @@ static void test_reftable_stack_add(void)
 		reftable_log_record_release(&dest);
 	}
 
+#ifndef GIT_WINDOWS_NATIVE
+	strbuf_addstr(&path, dir);
+	strbuf_addstr(&path, "/tables.list");
+	err = stat(path.buf, &stat_result);
+	EXPECT(!err);
+	EXPECT((stat_result.st_mode & 0777) == cfg.default_permissions);
+
+	strbuf_reset(&path);
+	strbuf_addstr(&path, dir);
+	strbuf_addstr(&path, "/");
+	/* do not try at home; not an external API for reftable. */
+	strbuf_addstr(&path, st->readers[0]->name);
+	err = stat(path.buf, &stat_result);
+	EXPECT(!err);
+	EXPECT((stat_result.st_mode & 0777) == cfg.default_permissions);
+#else
+	(void) stat_result;
+#endif
+
 	/* cleanup */
 	reftable_stack_destroy(st);
 	for (i = 0; i < N; i++) {
 		reftable_ref_record_release(&refs[i]);
 		reftable_log_record_release(&logs[i]);
 	}
+	strbuf_release(&path);
 	clear_dir(dir);
 }
 

base-commit: e02ecfcc534e2021aae29077a958dd11c3897e4c
-- 
2.43.GIT


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v3] reftable/stack: adjust permissions of compacted tables

[Justin Tobler]

On Fri, Jan 26, 2024 at 4:09 AM Patrick Steinhardt <ps@pks.im> wrote:
>
> When creating a new compacted table from a range of preexisting ones we
> don't set the default permissions on the resulting table when specified
> by the user. This has the effect that the "core.sharedRepository" config
> will not be honored correctly.
>
> Fix this bug and add a test to catch this issue. Note that we only test
> on non-Windows platforms because Windows does not use POSIX permissions
> natively.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>
> Changes compared to v2:
>
>   - Extended the commit message to say why we don't test on Windows
>     systems.
>
>   - Renamed the `scratch` variable to `path`.

Thanks Patrick! This version looks good to me. I have nothing else to add

-Justin