On Wed, Jan 19, 2022 at 8:06 AM Ævar Arnfjörð Bjarmason
<avarab@xxxxxxxxx> wrote:
>
> On Thu, Dec 30 2021, Elijah Newren via GitGitGadget wrote:
>
> > From: Elijah Newren <newren@xxxxxxxxx>
> > + struct tmp_objdir *remerge_objdir = NULL;
> > +
> > + if (rev->remerge_diff) {
> > + remerge_objdir = tmp_objdir_create("remerge-diff");
> > + if (!remerge_objdir)
> > + die_errno(_("unable to create temporary object directory"));
> > + tmp_objdir_replace_primary_odb(remerge_objdir, 1);
> > + }
>
> Re the errno feedback on v1
> https://lore.kernel.org/git/211221.864k71r6kz.gmgdl@xxxxxxxxxxxxxxxxxxx/
> the API might lose the "errno" due to e.g. the remove_dir_recurse()
> codepath. This seems like it would take care of that:
>
> diff --git a/builtin/log.c b/builtin/log.c
> index 944d9c0d9b5..d4b8b1aa4b6 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -424,9 +424,9 @@ static int cmd_log_walk(struct rev_info *rev)
> int saved_dcctc = 0;
>
> if (rev->remerge_diff) {
> - rev->remerge_objdir = tmp_objdir_create("remerge-diff");
> + rev->remerge_objdir = tmp_objdir_create_gently("remerge-diff", 0);
> if (!rev->remerge_objdir)
> - die_errno(_("unable to create temporary object directory"));
> + exit(128);
> tmp_objdir_replace_primary_odb(rev->remerge_objdir, 1);
> }
>
> diff --git a/tmp-objdir.c b/tmp-objdir.c
> index adf6033549e..3c656120003 100644
> --- a/tmp-objdir.c
> +++ b/tmp-objdir.c
> @@ -121,19 +121,21 @@ static void env_replace(struct strvec *env, const char *key, const char *val)
> strvec_pushf(env, "%s=%s", key, val);
> }
>
> -static int setup_tmp_objdir(const char *root)
> +static int setup_tmp_objdir(const char *root, int quiet)
> {
> char *path;
> int ret = 0;
>
> path = xstrfmt("%s/pack", root);
> ret = mkdir(path, 0777);
> + if (!quiet && ret < 0)
> + die_errno(_("unable to create temporary object directory '%s'"), path);
> free(path);
>
> return ret;
> }
>
> -struct tmp_objdir *tmp_objdir_create(const char *prefix)
> +struct tmp_objdir *tmp_objdir_create_gently(const char *prefix, int quiet)
> {
> static int installed_handlers;
> struct tmp_objdir *t;
> @@ -161,6 +163,8 @@ struct tmp_objdir *tmp_objdir_create(const char *prefix)
> strbuf_grow(&t->path, 1024);
>
> if (!mkdtemp(t->path.buf)) {
> + if (!quiet)
> + error_errno(_("unable to create temporary directory '%s'"), t->path.buf);
> /* free, not destroy, as we never touched the filesystem */
> tmp_objdir_free(t);
> return NULL;
> @@ -173,7 +177,7 @@ struct tmp_objdir *tmp_objdir_create(const char *prefix)
> installed_handlers++;
> }
>
> - if (setup_tmp_objdir(t->path.buf)) {
> + if (setup_tmp_objdir(t->path.buf, quiet)) {
> tmp_objdir_destroy(t);
> return NULL;
> }
> diff --git a/tmp-objdir.h b/tmp-objdir.h
> index 76efc7edee5..5072fb860d9 100644
> --- a/tmp-objdir.h
> +++ b/tmp-objdir.h
> @@ -24,8 +24,15 @@ struct tmp_objdir;
> /*
> * Create a new temporary object directory with the specified prefix;
> * returns NULL on failure.
> + *
> + * The tmp_objdir_create() is an a wrapper for
> + * tmp_objdir_create_gently(..., 1).
> */
> -struct tmp_objdir *tmp_objdir_create(const char *prefix);
> +struct tmp_objdir *tmp_objdir_create_gently(const char *prefix, int quiet);
> +static inline struct tmp_objdir *tmp_objdir_create(const char *prefix)
> +{
> + return tmp_objdir_create_gently(prefix, 1);
> +}
>
> /*
> * Return a list of environment strings, suitable for use with
Yeah, I think this suggests that switching from die() to die_errno()
was a mistake. Your patch looks right (though most of it belongs as
part of ns/tmp-objdir rather than this series), but I think it makes
the code uglier and I don't see why this theoretical error path is
worth all this trouble. A die() is totally sufficient here.
