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

Jacob Keller [off-list ref] writes:

+void show_submodule_diff(FILE *f, const char *path,
+		const char *line_prefix,
+		unsigned char one[20], unsigned char two[20],
+		unsigned dirty_submodule, const char *meta,
+		const char *reset)
+{
+	struct child_process cp = CHILD_PROCESS_INIT;
+	struct strbuf sb = STRBUF_INIT;
+	struct strbuf submodule_git_dir = STRBUF_INIT;
+	const char *git_dir, *message = NULL;
+	int create_dirty_diff = 0;
+	FILE *diff;
+	char c;
+
+	if ((dirty_submodule & DIRTY_SUBMODULE_UNTRACKED) ||
+	    (dirty_submodule & DIRTY_SUBMODULE_MODIFIED))
+		create_dirty_diff = 1;
+
+	strbuf_addf(&sb, "%s%sSubmodule %s %s..", line_prefix, meta, path,
+			find_unique_abbrev(one, DEFAULT_ABBREV));
+	strbuf_addf(&sb, "%s:%s\n", !create_dirty_diff ?
+		    find_unique_abbrev(two, DEFAULT_ABBREV) : "", reset);
+	fwrite(sb.buf, sb.len, 1, f);
+
+	strbuf_addf(&submodule_git_dir, "%s/.git", path);
+	git_dir = read_gitfile(submodule_git_dir.buf);
+	if (!git_dir)
+		git_dir = submodule_git_dir.buf;
+	if (!is_directory(git_dir)) {
+		strbuf_reset(&submodule_git_dir);
+		strbuf_addf(&submodule_git_dir, ".git/modules/%s", path);
+		git_dir = submodule_git_dir.buf;
+	}
+
+	cp.git_cmd = 1;
+	if (!create_dirty_diff)
+		cp.dir = git_dir;
+	else
+		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.

On Tue, Aug 9, 2016 at 8:37 PM, Junio C Hamano [off-list ref] wrote:

Jacob Keller [off-list ref] writes:

quoted

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

quoted

+     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