Re: [GSoC Patch v7 1/3] path: extract append_formatted_path() and use in rev-parse

> > -     free(cwd);
> >  }
>
> ... now becomes this postimage.
>

> > +static void print_path(const char *path, const char *prefix,
> > +                    enum format_type format, enum default_type def)
> >  {
> > +     struct strbuf sb = STRBUF_INIT;
> > +     enum path_format fmt;
> > +
> > +     if (format == FORMAT_RELATIVE) {
> > +             fmt = PATH_FORMAT_RELATIVE;
> > +     } else if (format == FORMAT_CANONICAL) {
> > +             fmt = PATH_FORMAT_CANONICAL;
> > +     } else /* FORMAT_DEFAULT */ {
> > +             switch (def) {
> > +             case DEFAULT_RELATIVE:
> > +                     fmt = PATH_FORMAT_RELATIVE;
> > +                     break;
> > +             case DEFAULT_RELATIVE_IF_SHARED:
> > +                     fmt = PATH_FORMAT_RELATIVE_IF_SHARED;
> > +                     break;
> > +             case DEFAULT_CANONICAL:
> > +                     fmt = PATH_FORMAT_CANONICAL;
> > +                     break;
> > +             case DEFAULT_UNMODIFIED:
> > +             default:
> > +                     fmt = PATH_FORMAT_UNMODIFIED;
> > +                     break;
> >               }
> >       }
> > +
> > +     append_formatted_path(&sb, path, prefix, fmt);
> > +     puts(sb.buf);
> > +
> > +     strbuf_release(&sb);
> >  }
>
> Mostly, the code translates FORMAT_FOO constants into the new
> PATH_FORMAT_FOO constants, and lets append_formatted_path() do the
> heavy lifting.
>
> It is a minor point, but wouldn't it make it simpler to handle
> format_default first?  I.e.,
>
>         if (format == FORMAT_DEFAULT)
>                 switch (def) {
>                 case DEFAULT_RELATIVE:
>                         format = DEFAULT_RELATIVE;
>                         break;
>                 ...
>                 case DEFAULT_UNMODIFIED:
>                 default:
>                         format = DEFAULT_UNMODIFIED;
>                         break;
>         }
>         switch (format) {
>         case FORMAT_RELATIVE: fmt = PATH_FORMAT_RELATIVE; break;
>         case FORMAT_CANONICAL: fmt = PATH_FORMAT_CANONICAL; break;
>         ...
>         }
>
> Perhaps yes, perhaps not.  I dunno.
>

> > diff --git a/path.c b/path.c
> > index d7e17bf174..6d8e892ada 100644
> > --- a/path.c
> > +++ b/path.c
> > @@ -1579,6 +1579,75 @@ char *xdg_cache_home(const char *filename)
> >       return NULL;
> >  }
> >
> > +void append_formatted_path(struct strbuf *dest, const char *path,
> > +                        const char *prefix, enum path_format format)
> > +{
> > +     switch (format) {
> > +     case PATH_FORMAT_UNMODIFIED:
> > +             strbuf_addstr(dest, path);
> > +             break;
>
> In the orignal "print_path()", DEFAULT/UNMODIFIED did this "show
> unmodified".  OK.
>
> > +     case PATH_FORMAT_RELATIVE: {
> > +             struct strbuf relative_buf = STRBUF_INIT;
> > +             struct strbuf real_path = STRBUF_INIT;
> > +             struct strbuf real_prefix = STRBUF_INIT;
> > +             char *cwd = NULL;
> > +
> > +             /*
> > +              * We don't ever produce a relative path if prefix is NULL,
> > +              * so set the prefix to the current directory so that we can
> > +              * produce a relative path whenever possible.
> > +              */
> > +             if (!prefix)
> > +                     prefix = cwd = xgetcwd();
>
> This is what was done in the original "print_path()" upfront, with
> a similar comment to explay why this happens.  Looking good.  Also
> we no longer call xgetcwd() when we do not need to, which is goodd.
>
> > +             if (!is_absolute_path(path)) {
> > +                     strbuf_realpath_forgiving(&real_path, path, 1);
> > +                     path = real_path.buf;
> > +             }
> > +             if (!is_absolute_path(prefix)) {
> > +                     strbuf_realpath_forgiving(&real_prefix, prefix, 1);
> > +                     prefix = real_prefix.buf;
> > +             }
>
> There used to be a comment explaining why we make realpath calls,
> which is now lost.  Perhaps what the comment said was so obvious
> that we are better off without it?  I offhand do not know.
>

> What is done to make the paths real is the same as before, which is
> good.
>
> > +             strbuf_addstr(dest, relative_path(path, prefix, &relative_buf));
> > +
> > +             strbuf_release(&relative_buf);
> > +             strbuf_release(&real_path);
> > +             strbuf_release(&real_prefix);
> > +             free(cwd);
> > +             break;
> > +     }
>
> OK.
>
> > +     case PATH_FORMAT_RELATIVE_IF_SHARED: {
> > +             struct strbuf relative_buf = STRBUF_INIT;
> > +
> > +             /*
> > +              * If we're using RELATIVE_IF_SHARED mode, then we want an
> > +              * absolute path unless the two share a common prefix, so don't
> > +              * default the prefix to the current working directory. Doing so
> > +              * would cause a relative path to always be produced if possible.
> > +              */

> Identical to the original, which is good.
> > +
> > +     case PATH_FORMAT_CANONICAL: {
> > +             struct strbuf canonical_buf = STRBUF_INIT;
> > +
> > +             strbuf_realpath_forgiving(&canonical_buf, path, 1);
> > +             strbuf_addbuf(dest, &canonical_buf);
> > +
> > +             strbuf_release(&canonical_buf);
> > +             break;
> > +     }
> > +
> > +     default:
> > +             BUG("unknown path_format value %d", format);
> > +     }
> > +}
>
> OK.
>
> > +/**
> > + * Format a path according to the specified formatting strategy and append
> > + * the result to the given strbuf.
> > + *
> > + * `dest`   : The string buffer to append the formatted path to.
> > + * `path`   : The path string that needs to be formatted.
> > + * `prefix` : The directory prefix to calculate relative offsets against.
> > + * Pass NULL to default to the current working directory where applicable.
> > + * `format` : The formatting behavior rule to execute.
> > + */
> > +void append_formatted_path(struct strbuf *dest, const char *path,
> > +                        const char *prefix, enum path_format format);
> > +
>
> It is slightly unsatisfying that this function is defined to
> "append" to any existing value in the dest strbuf, rather than
> storing the result in the dest strbuf.  The original caller
> print_path() passes an empty strbuf to this helper, so it can let
> strbuf_realpath_*() functions to strbuf_reset() it (e.g.,
> abspath.c:get_root_part() called by strbuf_realpath_1(), wihch in
> turn is called by strbuf_realpath() and strbuf_realpath_forgiving())
> it freely, which means that use of temporary strbuf like
> canonical_buf only to copy it out to dest is wasteful and unneeded.
> But other callers we will have for this helper later may want to
> append to what they already have, so perhaps it is OK (on the other
> hand, we could say that preserving and appending is what these
> callers can do themselves).
>

> > ...
> > It is a minor point, but wouldn't it make it simpler to handle
> > format_default first?  I.e.,
> >
> >       if (format == FORMAT_DEFAULT)
> >               switch (def) {
> >               case DEFAULT_RELATIVE:
> >                       format = DEFAULT_RELATIVE;
> >                       break;
> >               ...
> >               case DEFAULT_UNMODIFIED:
> >               default:
> >                       format = DEFAULT_UNMODIFIED;
> >                       break;
> >       }
> >       switch (format) {
> >         case FORMAT_RELATIVE: fmt = PATH_FORMAT_RELATIVE; break;
> >       case FORMAT_CANONICAL: fmt = PATH_FORMAT_CANONICAL; break;
> >       ...
> >       }
> >
> > Perhaps yes, perhaps not.  I dunno.
>
> I do not consider the above an blocker, but it might make a
> difference if we are going to acquire more modes and formats, so
> once somebody tries to rewrite the logic and finds the resulting
> code harder to follow (or not easier to follow), I would be happy to
> see the above discarded ;-)
>

> >> +/**
> >> + * Format a path according to the specified formatting strategy and append
> >> + * the result to the given strbuf.
> >> + *
> >> + * `dest`   : The string buffer to append the formatted path to.
> >> + * `path`   : The path string that needs to be formatted.
> >> + * `prefix` : The directory prefix to calculate relative offsets against.
> >> + * Pass NULL to default to the current working directory where applicable.
> >> + * `format` : The formatting behavior rule to execute.
> >> + */
> >> +void append_formatted_path(struct strbuf *dest, const char *path,
> >> +                       const char *prefix, enum path_format format);
> >> +
> >
> > It is slightly unsatisfying that this function is defined to
> > "append" to any existing value in the dest strbuf, rather than
> > storing the result in the dest strbuf.  The original caller
> > print_path() passes an empty strbuf to this helper, so it can let
> > strbuf_realpath_*() functions to strbuf_reset() it (e.g.,
> > abspath.c:get_root_part() called by strbuf_realpath_1(), wihch in
> > turn is called by strbuf_realpath() and strbuf_realpath_forgiving())
> > it freely, which means that use of temporary strbuf like
> > canonical_buf only to copy it out to dest is wasteful and unneeded.
> > But other callers we will have for this helper later may want to
> > append to what they already have, so perhaps it is OK (on the other
> > hand, we could say that preserving and appending is what these
> > callers can do themselves).
>
> This one we may want to consider a bit more seriously, but it is
> entirely up to the future callers of the helper.  If it would make
> the callers much easier to write for this helper to have "append"
> semantics, I'd be happy to accept the semantics of the above as-is,
> but otherwise, I suspect it would be simpler to use if the helper is
> defined to replase dest with the result, instead of appending the
> result to dest.
>

> > Otherwise, looking good as a no-op bug-to-bug compatible rewrite,
> > with a slight optimization (to skip xgetcwd()).
>
> This part of the review does not change in any case.  The
> refactoring looks good.