Re: [PATCH RFC v2] diff: add SUBMODULE_DIFF format to display submodule diff

[Stefan Beller <sbeller@google.com>]

On Tue, Aug 9, 2016 at 5:23 PM, Jacob Keller <jacob.e.keller@intel.com> wrote:
>
> +static int prepare_submodule_diff(struct strbuf *buf, const char *path,
> +               unsigned char one[20], unsigned char two[20])
> +{

This is not used any more, but the child is run directly below?

> +       strbuf_addf(&submodule_git_dir, "%s/.git", path);
> +       git_dir = read_gitfile(submodule_git_dir.buf);
> +       if (!git_dir)
> +               git_dir = submodule_git_dir.buf;
> +       if (!is_directory(git_dir)) {
> +               strbuf_reset(&submodule_git_dir);
> +               strbuf_addf(&submodule_git_dir, ".git/modules/%s", path);
> +               git_dir = submodule_git_dir.buf;
> +       }

This pattern seems familar, do we have a function for that?
(get a submodules git dir? As I was mainly working on the helper
I do not know off hand)

> +       if (start_command(&cp))
> +               die("Could not run 'git diff' command in submodule %s", path);
> +
> +       diff = fdopen(cp.out, "r");

diff is a FILE* pointer. cp.out is a standard int file descriptor

Maybe you'd want a similar thing as strbuf_getwholeline_fd()
just instead of adding it to the strbuf, adding it to `f` ?

(Which is what you have here? I just wonder about the buffer size,
as I think reading 1 by 1 is not beneficial)

> +
> +       c = fgetc(diff);
> +       while (c != EOF) {
> +               fputc(c, f);
> +               c = fgetc(diff);
> +       }
> +