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

[Junio C Hamano <gitster@pobox.com>]

Jacob Keller <jacob.e.keller@intel.com> 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.