Re: [PATCH v3 1/9] show, log: provide a --remerge-diff capability

[Elijah Newren <newren@gmail.com>]

On Wed, Jan 19, 2022 at 8:06 AM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
>
> On Thu, Dec 30 2021, Elijah Newren via GitGitGadget wrote:
>
> > From: Elijah Newren <newren@gmail.com>
> > +     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@evledraar.gmail.com/
> 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.