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

> Jacob Keller <jacob.e.keller@intel.com> writes:
>> +             cp.dir = path;
>> +     cp.out = -1;
>> +     cp.no_stdin = 1;
>> +     argv_array_push(&cp.args, "diff");
>> +     argv_array_pushf(&cp.args, "--src-prefix=a/%s/", path);
>> +     argv_array_pushf(&cp.args, "--dst-prefix=b/%s/", path);
>
> I think this is wrong.  Imagine when the caller gave you prefixes
> that are different from a/ and b/ (think: the extreme case is that
> you are a sub-sub-module, and the caller is recursing into you with
> its own prefix, perhaps set to a/sub and b/sub).  Use the prefixes
> you got for a/ and b/ instead of hardcoding them and you'd be fine,
> I'd guess.

>
>> +     argv_array_push(&cp.args, sha1_to_hex(one));
>> +
>> +     /*
>> +      * If the submodule has untracked or modified contents, diff between
>> +      * the old base and the current tip. This had the advantage of showing
>> +      * the full difference of the submodule contents.
>> +      */
>> +     if (!create_dirty_diff)
>> +             argv_array_push(&cp.args, sha1_to_hex(two));
>> +
>> +     if (start_command(&cp))
>> +             die("Could not run 'git diff' command in submodule %s", path);
>> +
>> +     diff = fdopen(cp.out, "r");
>> +
>> +     c = fgetc(diff);
>> +     while (c != EOF) {
>> +             fputc(c, f);
>> +             c = fgetc(diff);
>> +     }
>> +
>> +     fclose(diff);
>> +     finish_command(&cp);
>
> I do not think you need to do this.  If "f" is already a FILE * to
> which the output must go, then instead of reading from the pipe and
> writing it, you can just let the "diff" spit out its output to the
> same file descriptor, by doing something like:
>
>         fflush(f);
>         cp.out = dup(fileno(f));
>         ... other setup ...
>         run_command(&cp);
>
> no?  I do not offhand recall run_command() closes cp.out after done,
> and if it doesn't you may have to close it yourself after the above
> sequence.

>
> While I personally do not want to see code to access submodule's
> object store by temporarily adding it as alternates, the "show
> left-right log between the commits" code already does so, so I
> should point out that running "diff" internally without spawning it
> as a subprocess shouldn't be too hard.  The diff API is reasonably
> libified and there shouldn't be additional "libification" preparation
> work required to do this, if you really wanted to.