Re: [PATCH 1/2] difftool: add the builtin

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi David,

On Wed, 23 Nov 2016, David Aguilar wrote:

> On Tue, Nov 22, 2016 at 06:01:23PM +0100, Johannes Schindelin wrote:
>
> > +static const char * const builtin_difftool_usage[] = {
> > +	N_("git add [<options>] [--] <pathspec>..."),
> > +	NULL
> > +};
> 
> The usage should probably say "difftool" (or "builtin-difftool").

Ah, my dirty secret was spilled. I copy-edited this. *pours ashes over his
head*

> > [...]
> > +static void changed_files(struct hashmap *result, const char *index_path,
> > +			  const char *workdir)
> > +{
> > +[...]
> > +}
> > +
> > +#include "dir.h"
> 
> Can this mid-file #include go to the top of the file?

Yep, thanks.

In case you are interested: You probably guessed it, it was left for a
later clean-up. I worked a bit over the last weeks on getting Git to build
in Visual Studio, to be able to benefit from its quite nice features (I
was always a fan of Visual Studio, long before I started working at
Microsoft). I used the conversion of the difftool as an excuse to make use
of this myself: I did the entire conversion in Visual Studio, reverting to
the old, tedious command-line driven workflow to fix the bugs identified
by t7800-difftool.sh.

> > +static int run_dir_diff(const char *extcmd, int symlinks,
> > +			int argc, const char **argv)
> > +{
> > +	char tmpdir[PATH_MAX];
> > +	struct strbuf info = STRBUF_INIT, lpath = STRBUF_INIT;
> > +	struct strbuf rpath = STRBUF_INIT, buf = STRBUF_INIT;
> > +	struct strbuf ldir = STRBUF_INIT, rdir = STRBUF_INIT;
> > +	struct strbuf wtdir = STRBUF_INIT;
> > +	size_t ldir_len, rdir_len, wtdir_len;
> > +	struct cache_entry *ce = xcalloc(1, sizeof(ce) + PATH_MAX + 1);
> > +	const char *workdir, *tmp;
> > +	int ret = 0, i;
> > +	FILE *fp;
> > +	struct hashmap working_tree_dups, submodules, symlinks2;
> > +	struct hashmap_iter iter;
> > +	struct pair_entry *entry;
> > +	enum object_type type;
> > +	unsigned long size;
> > +	struct index_state wtindex;
> > +	struct checkout lstate, rstate;
> > +	int rc, flags = RUN_GIT_CMD, err = 0;
> > +	struct child_process child = CHILD_PROCESS_INIT;
> > +	const char *helper_argv[] = { "difftool--helper", NULL, NULL, NULL };
> > +	struct hashmap wt_modified, tmp_modified;
> > +	int indices_loaded = 0;
> > +
> > +	setup_work_tree();
> > +	workdir = get_git_work_tree();
> > +
> > +	/* Setup temp directories */
> > +	tmp = getenv("TMPDIR");
> > +	sprintf(tmpdir, "%s/git-difftool.XXXXXX", tmp ? tmp : "/tmp");
> 
> Maybe snprintf instead?
> 
> getenv() won't return anything longer than PATH_MAX for most
> users, but users are weird.

True.

> > +	if (!mkdtemp(tmpdir))
> > +		return error("could not create temporary directory");
> 
> Mention the tmpdir here?

Sure thing.

> > +	strbuf_addf(&ldir, "%s/left/", tmpdir);
> > +	strbuf_addf(&rdir, "%s/right/", tmpdir);
> > +	strbuf_addstr(&wtdir, workdir);
> > +	if (!wtdir.len || !is_dir_sep(wtdir.buf[wtdir.len - 1]))
> > +		strbuf_addch(&wtdir, '/');
> > +	mkdir(ldir.buf, 0777);
> > +	mkdir(rdir.buf, 0777);
> 
> Seeing the perl mkpath() default 0777 spelled out this way
> makes me wonder whether 0700 would be safer.
> 
> The mkdtemp() above is already using 0700 so it's ok, but it
> might be worth making it consistent (later, perhaps).

Ah, of course! I stupidly imitated other `mkdir()` calls elsewhere, but
they refer to directories within the Git worktree...

> > +	/*
> > +	 * In directory diff mode, 'git-difftool--helper' is called once
> > +	 * to compare the a / b directories.In file diff mode, 'git diff'
> > +	 * will invoke a separate instance of 'git-difftool--helper' for
> > +	 * each file that changed.
> > +	 */
> 
> Missing space after "." in the comment above.

Yep. It was two spaces and I deleted one too many (we are so way past
actual print, where the two spaces may have made sense...).

> > +	if (dir_diff)
> > +		return run_dir_diff(extcmd, symlinks, argc, argv);
> > +	return run_file_diff(prompt, argc, argv);
> > +}
> > diff --git a/git.c b/git.c
> > index efa1059..eaa0f67 100644
> > --- a/git.c
> > +++ b/git.c
> > @@ -424,6 +424,7 @@ static struct cmd_struct commands[] = {
> >  	{ "diff-files", cmd_diff_files, RUN_SETUP | NEED_WORK_TREE },
> >  	{ "diff-index", cmd_diff_index, RUN_SETUP },
> >  	{ "diff-tree", cmd_diff_tree, RUN_SETUP },
> > +	{ "builtin-difftool", cmd_builtin_difftool, RUN_SETUP | NEED_WORK_TREE },
> >  	{ "fast-export", cmd_fast_export, RUN_SETUP },
> >  	{ "fetch", cmd_fetch, RUN_SETUP },
> >  	{ "fetch-pack", cmd_fetch_pack, RUN_SETUP },
> 
> This isn't alphabetical anymore, but it actually is if you
> consider that the final plan is to change "builtin-difftool" to
> "difftool".

Exactly, that was my thinking.

> If we want to minimize that future diff we could name
> cmd_builtin_difftool() as cmd_difftool() for consistency now so
> that the future commit only needs to tweak the string here.

Yes!

For the record, this is a left-over from an impatient attempt at avoiding
problems with `make` overwriting the Perl version of `git difftool` by the
builtin version; I had originally assumed that a list of builtins was
generated from parsing git.c or builtin.h, but it turns out that the
BUILTIN_OBJS are actually responsible, i.e. the file name.

Fixed.

Thank you for your review!
Dscho