Re: [PATCH v9 0/8] submodule show inline diff

[Stefan Beller <sbeller@google.com>]

On Fri, Aug 19, 2016 at 4:34 PM, Jacob Keller <jacob.e.keller@intel.com> wrote:
> From: Jacob Keller <jacob.keller@gmail.com>
>
> More suggestions from Junio and a few changes to support submodule name
> lookup. Hopefully we're getting close to the goal!
>
> interdiff between v8 and current:
> diff --git c/builtin/rev-list.c w/builtin/rev-list.c
> index 21cde8dd6b31..8479f6ed28aa 100644
> --- c/builtin/rev-list.c
> +++ w/builtin/rev-list.c
> @@ -129,29 +129,29 @@ static void show_commit(struct commit *commit, void *data)
>                         graph_show_commit_msg(revs->graph, stdout, &buf);
>
>                         /*
> -                       * Add a newline after the commit message.
> -                       *
> -                       * Usually, this newline produces a blank
> -                       * padding line between entries, in which case
> -                       * we need to add graph padding on this line.
> -                       *
> -                       * However, the commit message may not end in a
> -                       * newline.  In this case the newline simply
> -                       * ends the last line of the commit message,
> -                       * and we don't need any graph output.  (This
> -                       * always happens with CMIT_FMT_ONELINE, and it
> -                       * happens with CMIT_FMT_USERFORMAT when the
> -                       * format doesn't explicitly end in a newline.)
> -                       */
> +                        * Add a newline after the commit message.
> +                        *
> +                        * Usually, this newline produces a blank
> +                        * padding line between entries, in which case
> +                        * we need to add graph padding on this line.
> +                        *
> +                        * However, the commit message may not end in a
> +                        * newline.  In this case the newline simply
> +                        * ends the last line of the commit message,
> +                        * and we don't need any graph output.  (This
> +                        * always happens with CMIT_FMT_ONELINE, and it
> +                        * happens with CMIT_FMT_USERFORMAT when the
> +                        * format doesn't explicitly end in a newline.)
> +                        */
>                         if (buf.len && buf.buf[buf.len - 1] == '\n')
>                                 graph_show_padding(revs->graph);
>                         putchar('\n');
>                 } else {
>                         /*
> -                               * If the message buffer is empty, just show
> -                               * the rest of the graph output for this
> -                               * commit.
> -                               */
> +                        * If the message buffer is empty, just show
> +                        * the rest of the graph output for this
> +                        * commit.
> +                        */
>                         if (graph_show_remainder(revs->graph))
>                                 putchar('\n');
>                         if (revs->commit_format == CMIT_FMT_ONELINE)
> diff --git c/cache.h w/cache.h
> index da9f0be67d7b..70428e92d7ed 100644
> --- c/cache.h
> +++ w/cache.h
> @@ -953,24 +953,39 @@ static inline void oidclr(struct object_id *oid)
>  #define EMPTY_TREE_SHA1_BIN_LITERAL \
>          "\x4b\x82\x5d\xc6\x42\xcb\x6e\xb9\xa0\x60" \
>          "\xe5\x4b\xf8\xd6\x92\x88\xfb\xee\x49\x04"
> -#define EMPTY_TREE_SHA1_BIN \
> -        ((const unsigned char *) EMPTY_TREE_SHA1_BIN_LITERAL)
> +extern const struct object_id empty_tree_oid;
> +#define EMPTY_TREE_SHA1_BIN (empty_tree_oid.hash)
>
>  #define EMPTY_BLOB_SHA1_HEX \
>         "e69de29bb2d1d6434b8b29ae775ad8c2e48c5391"
>  #define EMPTY_BLOB_SHA1_BIN_LITERAL \
>         "\xe6\x9d\xe2\x9b\xb2\xd1\xd6\x43\x4b\x8b" \
>         "\x29\xae\x77\x5a\xd8\xc2\xe4\x8c\x53\x91"
> -#define EMPTY_BLOB_SHA1_BIN \
> -       ((const unsigned char *) EMPTY_BLOB_SHA1_BIN_LITERAL)
> +extern const struct object_id empty_blob_oid;
> +#define EMPTY_BLOB_SHA1_BIN (empty_blob_oid.hash)
>
> -extern const struct object_id empty_tree_oid;
>
>  static inline int is_empty_blob_sha1(const unsigned char *sha1)
>  {
>         return !hashcmp(sha1, EMPTY_BLOB_SHA1_BIN);
>  }
>
> +static inline int is_empty_blob_oid(const struct object_id *oid)
> +{
> +       return !hashcmp(oid->hash, EMPTY_BLOB_SHA1_BIN);
> +}
> +
> +static inline int is_empty_tree_sha1(const unsigned char *sha1)
> +{
> +       return !hashcmp(sha1, EMPTY_TREE_SHA1_BIN);
> +}
> +
> +static inline int is_empty_tree_oid(const struct object_id *oid)
> +{
> +       return !hashcmp(oid->hash, EMPTY_TREE_SHA1_BIN);
> +}
> +
> +
>  int git_mkstemp(char *path, size_t n, const char *template);
>
>  /* set default permissions by passing mode arguments to open(2) */
> diff --git c/diff.h w/diff.h
> index 192c0eedd0ff..ec76a90522ea 100644
> --- c/diff.h
> +++ w/diff.h
> @@ -112,7 +112,7 @@ enum diff_words_type {
>  enum diff_submodule_format {
>         DIFF_SUBMODULE_SHORT = 0,
>         DIFF_SUBMODULE_LOG,
> -       DIFF_SUBMODULE_INLINE_DIFF,
> +       DIFF_SUBMODULE_INLINE_DIFF
>  };
>
>  struct diff_options {
> diff --git c/path.c w/path.c
> index 188abfebbafe..081a22c1163c 100644
> --- c/path.c
> +++ w/path.c
> @@ -6,6 +6,7 @@
>  #include "string-list.h"
>  #include "dir.h"
>  #include "worktree.h"
> +#include "submodule-config.h"
>
>  static int get_st_mode_bits(const char *path, int *mode)
>  {
> @@ -474,6 +475,7 @@ static void do_submodule_path(struct strbuf *buf, const char *path,
>         const char *git_dir;
>         struct strbuf git_submodule_common_dir = STRBUF_INIT;
>         struct strbuf git_submodule_dir = STRBUF_INIT;
> +       const struct submodule *submodule_config;
>
>         strbuf_addstr(buf, path);
>         strbuf_complete(buf, '/');
> @@ -486,7 +488,16 @@ static void do_submodule_path(struct strbuf *buf, const char *path,
>         }
>         if (!is_git_directory(buf->buf)) {
>                 strbuf_reset(buf);
> -               strbuf_git_path(buf, "%s/%s", "modules", path);
> +               /*
> +                * Lookup the submodule name from the config. If that fails
> +                * fall back to assuming the path is the name.
> +                */
> +               submodule_config = submodule_from_path(null_sha1, path);

In case you need to reroll: I'd got with just "sub" as the name for
the config object
(that seems to be used all the time and is shorter)


> +               if (submodule_config)
> +                       strbuf_git_path(buf, "%s/%s", "modules",
> +                                       submodule_config->name);
> +               else

I do not think we want to assume the path as the name for the fallback, though.

If `submodule_config == NULL` then
a) the path/name doesn't exist in the given version.
    (If null_sha1 is given, HEAD + working tree is assumed, whereas
    you could also check for a specific commit of the superproject
    with another sha1)

b) or the submodule config cache was not initialized
   (missing call to gitmodules_config())

c) There is no c) [at least I never came across another reason for a
NULL return here]

Using the path as the fallback is errorprone (e.g. to b. in the future
and then you get the wrong submodule repository which is shaded by
assuming the path and it is hard to debug later or write forward looking
tests for that now)

Thanks,
Stefan