On Tue, Aug 9, 2016 at 8:37 PM, Junio C Hamano <gits...@pobox.com> wrote:
> Jacob Keller <jacob.e.kel...@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.

I'll have to get these passed, but yes this makes more sense, will look into it.

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

That makes a lot more sense, yes. I hadn't thought of dup, (long time
since I've had to use file descriptors).

the child_process api does close the descriptor itself. That's a much
better way to get the result we want, and it is less code, so I'll try
this out.

> 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.

I looked at trying to call diff for this, but I think it's not worth
it. Using the child process API makes more sense and is a cleaner
method. I'll go this route as it appears to work pretty well. The only
major concern I have is that options may not get forwarded

To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to