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