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

From: Stefan Beller <sbeller@google.com>
Date: Aug 10, 2016, 00:53 UTC
Message-ID: <CAGZ79kYLLJCq0OZANSyrYmuq1Mm6w3Ks8CHC-_0uicDps0srpg@mail.gmail.com>
In-Reply-To: <20160810002315.25236-1-jacob.e.keller@intel.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?

Show 9 quoted lines

> +       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)

Show 7 quoted lines

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