On 24/03/06 12:59PM, Patrick Steinhardt wrote:
> On Tue, Mar 05, 2024 at 04:30:18PM -0600, Justin Tobler wrote:
> > On 24/03/04 12:10PM, Patrick Steinhardt wrote:
> > > We do not register new tables which we're about to add to the stack with
> > > the tempfile API. Those tables will thus not be deleted in case Git gets
> > > killed.
> > >
> > > Refactor the code to register tables as tempfiles.
> > >
> > > Signed-off-by: Patrick Steinhardt <ps@xxxxxx>
> > > ---
> > > reftable/stack.c | 29 ++++++++++++-----------------
> > > 1 file changed, 12 insertions(+), 17 deletions(-)
> > >
> > > diff --git a/reftable/stack.c b/reftable/stack.c
> > > index b64e55648a..81544fbfa0 100644
> > > --- a/reftable/stack.c
> > > +++ b/reftable/stack.c
> > > @@ -737,8 +737,9 @@ int reftable_addition_add(struct reftable_addition *add,
> > > struct strbuf tab_file_name = STRBUF_INIT;
> > > struct strbuf next_name = STRBUF_INIT;
> > > struct reftable_writer *wr = NULL;
> > > + struct tempfile *tab_file = NULL;
> > > int err = 0;
> > > - int tab_fd = 0;
> > > + int tab_fd;
> > >
> > > strbuf_reset(&next_name);
> > > format_name(&next_name, add->next_update_index, add->next_update_index);
> > > @@ -746,17 +747,20 @@ int reftable_addition_add(struct reftable_addition *add,
> > > stack_filename(&temp_tab_file_name, add->stack, next_name.buf);
> > > strbuf_addstr(&temp_tab_file_name, ".temp.XXXXXX");
> > >
> > > - tab_fd = mkstemp(temp_tab_file_name.buf);
> > > - if (tab_fd < 0) {
> > > + tab_file = mks_tempfile(temp_tab_file_name.buf);
> > > + if (!tab_file) {
> > > err = REFTABLE_IO_ERROR;
> > > goto done;
> > > }
> > > if (add->stack->config.default_permissions) {
> > > - if (chmod(temp_tab_file_name.buf, add->stack->config.default_permissions)) {
> > > + if (chmod(get_tempfile_path(tab_file),
> > > + add->stack->config.default_permissions)) {
> > > err = REFTABLE_IO_ERROR;
> > > goto done;
> > > }
> > > }
> >
> > Since the tempfile is now being created through the tempfile API, I
> > think the file mode can be set directly through `mks_tempfile_m()`
> > instead of creating the tempfile and then using chmod. Just something I
> > thought to mention.
>
> Unfortunately not. The problem is that `mks_tempfile_m()` will munge
> passed-in permissions via "core.sharedRepository", but we already pre
> calculated the target mode in `config.default_permissions`. Thus, the
> result would have wrong permissions if we used `mks_tempfile_m()`.
Ah that makes sense. Thanks for the explanation!
-Justin
