Jacob Keller <jacob.keller@xxxxxxxxx> writes:
>>> + 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.
One caveat. If the superproject is doing "log -p --graph", the
output will be broken with this "splice in 'git -C submodule diff'
output in place" approach. I personally do not care if it is broken
as I do not use "-p" and "--graph" together (for that matter, I do
not think I'll use the "splice in 'git -C submodule diff'" feature
added by this patch, either, so I do not think I'd deeply care ;-),
but I am reasonably sure those who would use these would.
The way to solve it is to capture diff output and send out with the
current graph prefix (i.e. the set of vertial lines), and the
easiest and most expensive (probably too expensive to be practical)
way to do so would be to capture the whole thing into a strbuf, but
I think it is OK to teach a new option to "git diff" that tells to
prefix a fixed string given as its argument to each and every line
of its output, and that would be a more practical solution, which
can also be reused if you choose to later run "diff" internally
without spawning it as a separate process.
The graph machinery may have to be taught a way to tell what the
current graph prefix is if you go that route.
> 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...
You'd need to deal with options either way. You do not want to
assume all options should be passed down. You now know you need to
tweak the prefix, but you do not know if that is all that is
required.
--
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