On Tue, Aug 9, 2016 at 8:37 PM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> Jacob Keller <jacob.e.keller@xxxxxxxxx> 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
correctly...
Thanks,
Jake
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html