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

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 10, 2016, 17:10 UTC
Message-ID: <xmqqy4445z4l.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <CA+P7+xpGGqNC8B-fQiQXZcXWha2BBQa96sFvj+_MBVHWakMh_Q@mail.gmail.com>

Jacob Keller <jacob.keller@gmail.com> writes:

Show 31 quoted lines

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

Show 5 quoted lines

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