Fix remarks post-devkit removal by arvidj · Pull Request #15 · ahrefs/passage

arvidj · 2026-02-10T08:28:16Z

What

Respond to @rr0gi's remarks on #11

arvidj · 2026-02-10T08:29:25Z

bin/main.ml

        let target_file = Path.project target_file in
-        let flags = [ Open_wronly; Open_creat; Open_trunc; Open_binary ] in
-        Out_channel.(with_open_gen flags 0o600 target_file (fun oc -> output_string oc contents))
+        Util.save_as ~path:target_file ~mode:0o600 (fun oc -> output_string oc contents)


Restored the original usage of Devkit.Files.save_as here. But the question remains as to whether all other writes of passage should be made atomic in the same manner (they weren't before I removed devkit).

IIUC, this is what Storage.Secrets.encrypt_using_tmpfile is doing, so we're good there.

There are two cases of writing recipients to disk in commands.ml where we are using Out_channel.with_open_text.

I would say that we could use the same function everywhere, even if only for a matter of consistency. AFAIU, there are no downsides to having atomic writes here, right?

arvidj · 2026-02-10T08:49:13Z

bin/prompt.ml

-    Fun.protect ~finally:(fun () -> try Sys.remove tmpfile with _ -> ()) (fun () -> f (tmpfile, tmpfile_oc))
+    Fun.protect
+      ~finally:(fun () ->
+        Stdlib.close_out_noerr tmpfile_oc;


IIUC, this should ensure that this function will not leak file descriptors in case f fails.

lgtmttbomk
(looks good to me, to the best of my knowledge)

thatportugueseguy

lgtm, we could probably have the atomic writes everywhere for consistency

thatportugueseguy · 2026-02-10T12:43:24Z

bin/main.ml

        let target_file = Path.project target_file in
-        let flags = [ Open_wronly; Open_creat; Open_trunc; Open_binary ] in
-        Out_channel.(with_open_gen flags 0o600 target_file (fun oc -> output_string oc contents))
+        Util.save_as ~path:target_file ~mode:0o600 (fun oc -> output_string oc contents)


IIUC, this is what Storage.Secrets.encrypt_using_tmpfile is doing, so we're good there.

There are two cases of writing recipients to disk in commands.ml where we are using Out_channel.with_open_text.

I would say that we could use the same function everywhere, even if only for a matter of consistency. AFAIU, there are no downsides to having atomic writes here, right?

thatportugueseguy · 2026-02-10T12:44:51Z

bin/prompt.ml

-    Fun.protect ~finally:(fun () -> try Sys.remove tmpfile with _ -> ()) (fun () -> f (tmpfile, tmpfile_oc))
+    Fun.protect
+      ~finally:(fun () ->
+        Stdlib.close_out_noerr tmpfile_oc;


lgtmttbomk
(looks good to me, to the best of my knowledge)

thatportugueseguy · 2026-02-10T12:45:21Z

lib/types.ml

i don't like the module name, but i don't have a better one.

we could have just type.ml if you prefer? it's modelled on Devkit.Prelude.Fresh, where you'd have Fresh (String), now you have Abstract_type.Make (String). I don't think Fresh is particularly clear, but if you know you know. We could have a name that is reminiscent to Fresh, might make it more clear?

now you have Abstract_type.Make (String)

Actually you have Abstract_type.Fresh (String), that's why it probably makes me a bit icky. Also i feel like Abstract_type should be under some Utils module, or maybe under a Types one?

What do you feel about Types.Fresh (String)? I know that the Fresh is unclear at first, but after you have a name for the thing, can you use another one? 😅 In any case, this still doesn't make me happy, but i guess it's that old thing about naming in programming..

Types.Fresh is OK for me, renamed in latest version.

I would say that we could use the same function everywhere, even if only for a matter of consistency. AFAIU, there are no downsides to having atomic writes here, right?

Latest version I use save_as for the other case of writing recipient files. Except for that, couldn't find any file writes in the project.

Btw, we also have encrypt_using_tmpfile. It seems like it could be use Util.save_as.

In the latest version I use save_as in encrypt_using_tmpfile.

I also move Util.save_as to Storage.save_as. I do this so that I can use it in Storage.Secrets.encrypt_using_tmpfile, and since Util depends on Storage this would induce a dependency cycle. I think I would've preferred to keep it in some separate module though, with some base functionality. Util is kind of that, but not really since it depends on Storage.

thatportugueseguy · 2026-02-10T12:47:05Z

lib/util.mli

+    will be renamed to [path] after [f] has succesfully terminated. Therefore this guarantee that either [filename] will
+    not be modified or will contain whatever [f] was writing to it as a side-effect.


Suggested change

will be renamed to [path] after [f] has succesfully terminated. Therefore this guarantee that either [filename] will

not be modified or will contain whatever [f] was writing to it as a side-effect.

will be renamed to [path] after [f] has successfully terminated. Therefore, this guarantees that either [filename] will

not be modified or will contain whatever [f] was writing to it as a side-effect.

is it a side-effect? It's the intended result

see latest version. I think the original authors meant side-effect as in, "this function is not pure, it has a side-effect". but I think that is redundant, and confusing as you point out, so I removed it.

arvidj added 2 commits February 10, 2026 09:27

add [Util.save_as] modeled on Devkit's dito and use to update recips

bbabf01

ensure out_ch:s opened with [with_secure_tmpfile] are closed

b46b19f

arvidj force-pushed the aj/fix-ex-devkit-remarks branch from a71da34 to 9908d2c Compare February 10, 2026 09:30

arvidj commented Feb 10, 2026

View reviewed changes

arvidj changed the title ~~add [Util.save_as] modeled on Devkit's dito and use to update recips~~ Fix remarks post-devkit removal Feb 10, 2026

arvidj force-pushed the aj/fix-ex-devkit-remarks branch 2 times, most recently from acf204c to 84ce2f1 Compare February 10, 2026 09:49

arvidj marked this pull request as ready for review February 10, 2026 09:50

arvidj mentioned this pull request Feb 10, 2026

Remove devkit dependency #11

Merged

arvidj added 2 commits February 10, 2026 11:28

introduce [Abstract_string], replace previous usages of Devkit's Fresh

21fc1ec

use [Util.save_as] in [Recipients.add_recipients_if_none_exists]

7994a7d

arvidj force-pushed the aj/fix-ex-devkit-remarks branch from 84ce2f1 to 7994a7d Compare February 10, 2026 10:28

thatportugueseguy reviewed Feb 10, 2026

View reviewed changes

arvidj added 5 commits February 10, 2026 16:33

fix [save_as] doc string

957d6b6

use [Print_exc.register_printer]

a93fb10

rename [Abstract_types => Types]

8cff155

also use [save_as] in [rewrite_recipients_file]

2ecb922

move [save_as] to [Storage] and use for [encrypt_using_tmpfile]

4dbcfdc

arvidj merged commit 604d631 into ahrefs:master Feb 18, 2026
2 checks passed

		will be renamed to [path] after [f] has succesfully terminated. Therefore this guarantee that either [filename] will
		not be modified or will contain whatever [f] was writing to it as a side-effect.

Conversation

arvidj commented Feb 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thatportugueseguy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

arvidj commented Feb 10, 2026 •

edited

Loading