Skip to content

G
git2
Commit 8a23597b by Patrick Steinhardt
Options

diff_generate: refactor `DIFF_FROM_ITERATORS` macro of doom 

While the `DIFF_FROM_ITERATORS` does make it shorter to implement the
various `git_diff_foo_to_bar` functions, it is a complex and unreadable
beast that implicitly assumes certain local variable names. This is not
something desirable to have at all and obstructs understanding and more
importantly debugging the code by quite a bit.

The `DIFF_FROM_ITERATORS` macro basically removed the burden of having
to derive the options for both iterators from a pair of iterator flags
and the diff options. This patch introduces a new function that does the
that exact and refactors all callers to manage the iterators by
themselves.

As we potentially need to allocate a shared prefix for the
iterator, we need to tell the caller to allocate that prefix as soon as
the options aren't required anymore. Thus, the function has a `char
**prefix` out pointer that will get set to the allocated string and
subsequently be free'd by the caller.

While this patch increases the line count, I personally deem this to an
acceptable tradeoff for increased readbiblity.
parent 0f40e68e
Showing with 113 additions and 64 deletions
src/diff_generate.c
...@@ -1262,29 +1262,30 @@ cleanup: ...@@ -1262,29 +1262,30 @@ cleanup:
return error; return error;
} }
#define DIFF_FROM_ITERATORS(MAKE_FIRST, FLAGS_FIRST, MAKE_SECOND, FLAGS_SECOND) do { \ static int diff_prepare_iterator_opts(char **prefix, git_iterator_options *a, int aflags,
git_iterator *a = NULL, *b = NULL; \ git_iterator_options *b, int bflags,
char *pfx = (opts && !(opts->flags & GIT_DIFF_DISABLE_PATHSPEC_MATCH)) ? \ const git_diff_options *opts)
git_pathspec_prefix(&opts->pathspec) : NULL; \ {
git_iterator_options a_opts = GIT_ITERATOR_OPTIONS_INIT, \ GIT_ERROR_CHECK_VERSION(opts, GIT_DIFF_OPTIONS_VERSION, "git_diff_options");
b_opts = GIT_ITERATOR_OPTIONS_INIT; \
a_opts.flags = FLAGS_FIRST; \ *prefix = NULL;
a_opts.start = pfx; \
a_opts.end = pfx; \ if (opts && (opts->flags & GIT_DIFF_DISABLE_PATHSPEC_MATCH)) {
b_opts.flags = FLAGS_SECOND; \ a->pathlist.strings = opts->pathspec.strings;
b_opts.start = pfx; \ a->pathlist.count = opts->pathspec.count;
b_opts.end = pfx; \ b->pathlist.strings = opts->pathspec.strings;
GIT_ERROR_CHECK_VERSION(opts, GIT_DIFF_OPTIONS_VERSION, "git_diff_options"); \ b->pathlist.count = opts->pathspec.count;
if (opts && (opts->flags & GIT_DIFF_DISABLE_PATHSPEC_MATCH)) { \ } else if (opts) {
a_opts.pathlist.strings = opts->pathspec.strings; \ *prefix = git_pathspec_prefix(&opts->pathspec);
a_opts.pathlist.count = opts->pathspec.count; \ }
b_opts.pathlist.strings = opts->pathspec.strings; \
b_opts.pathlist.count = opts->pathspec.count; \ a->flags = aflags;
} \ b->flags = bflags;
if (!error && !(error = MAKE_FIRST) && !(error = MAKE_SECOND)) \ a->start = b->start = *prefix;
error = git_diff__from_iterators(&diff, repo, a, b, opts); \ a->end = b->end = *prefix;
git__free(pfx); git_iterator_free(a); git_iterator_free(b); \
} while (0) return 0;
}
int git_diff_tree_to_tree( int git_diff_tree_to_tree(
git_diff **out, git_diff **out,
...@@ -1293,8 +1294,12 @@ int git_diff_tree_to_tree( ...@@ -1293,8 +1294,12 @@ int git_diff_tree_to_tree(
git_tree *new_tree, git_tree *new_tree,
const git_diff_options *opts) const git_diff_options *opts)
{ {
git_diff *diff = NULL;
git_iterator_flag_t iflag = GIT_ITERATOR_DONT_IGNORE_CASE; git_iterator_flag_t iflag = GIT_ITERATOR_DONT_IGNORE_CASE;
git_iterator_options a_opts = GIT_ITERATOR_OPTIONS_INIT,
b_opts = GIT_ITERATOR_OPTIONS_INIT;
git_iterator *a = NULL, *b = NULL;
git_diff *diff = NULL;
char *prefix = NULL;
int error = 0; int error = 0;
assert(out && repo); assert(out && repo);
...@@ -1308,13 +1313,19 @@ int git_diff_tree_to_tree( ...@@ -1308,13 +1313,19 @@ int git_diff_tree_to_tree(
if (opts && (opts->flags & GIT_DIFF_IGNORE_CASE) != 0) if (opts && (opts->flags & GIT_DIFF_IGNORE_CASE) != 0)
iflag = GIT_ITERATOR_IGNORE_CASE; iflag = GIT_ITERATOR_IGNORE_CASE;
DIFF_FROM_ITERATORS( if ((error = diff_prepare_iterator_opts(&prefix, &a_opts, iflag, &b_opts, iflag, opts)) < 0 ||
git_iterator_for_tree(&a, old_tree, &a_opts), iflag, (error = git_iterator_for_tree(&a, old_tree, &a_opts)) < 0 ||
git_iterator_for_tree(&b, new_tree, &b_opts), iflag (error = git_iterator_for_tree(&b, new_tree, &b_opts)) < 0 ||
); (error = git_diff__from_iterators(&diff, repo, a, b, opts)) < 0)
goto out;
if (!error)
*out = diff; *out = diff;
diff = NULL;
out:
git_iterator_free(a);
git_iterator_free(b);
git_diff_free(diff);
git__free(prefix);
return error; return error;
} }
...@@ -1337,9 +1348,13 @@ int git_diff_tree_to_index( ...@@ -1337,9 +1348,13 @@ int git_diff_tree_to_index(
git_index *index, git_index *index,
const git_diff_options *opts) const git_diff_options *opts)
{ {
git_diff *diff = NULL;
git_iterator_flag_t iflag = GIT_ITERATOR_DONT_IGNORE_CASE | git_iterator_flag_t iflag = GIT_ITERATOR_DONT_IGNORE_CASE |
GIT_ITERATOR_INCLUDE_CONFLICTS; GIT_ITERATOR_INCLUDE_CONFLICTS;
git_iterator_options a_opts = GIT_ITERATOR_OPTIONS_INIT,
b_opts = GIT_ITERATOR_OPTIONS_INIT;
git_iterator *a = NULL, *b = NULL;
git_diff *diff = NULL;
char *prefix = NULL;
bool index_ignore_case = false; bool index_ignore_case = false;
int error = 0; int error = 0;
...@@ -1352,17 +1367,23 @@ int git_diff_tree_to_index( ...@@ -1352,17 +1367,23 @@ int git_diff_tree_to_index(
index_ignore_case = index->ignore_case; index_ignore_case = index->ignore_case;
DIFF_FROM_ITERATORS( if ((error = diff_prepare_iterator_opts(&prefix, &a_opts, iflag, &b_opts, iflag, opts)) < 0 ||
git_iterator_for_tree(&a, old_tree, &a_opts), iflag, (error = git_iterator_for_tree(&a, old_tree, &a_opts)) < 0 ||
git_iterator_for_index(&b, repo, index, &b_opts), iflag (error = git_iterator_for_index(&b, repo, index, &b_opts)) < 0 ||
); (error = git_diff__from_iterators(&diff, repo, a, b, opts) < 0))
goto out;
/* if index is in case-insensitive order, re-sort deltas to match */ /* if index is in case-insensitive order, re-sort deltas to match */
if (!error && index_ignore_case) if (index_ignore_case)
git_diff__set_ignore_case(diff, true); git_diff__set_ignore_case(diff, true);
if (!error)
*out = diff; *out = diff;
diff = NULL;
out:
git_iterator_free(a);
git_iterator_free(b);
git_diff_free(diff);
git__free(prefix);
return error; return error;
} }
...@@ -1373,7 +1394,11 @@ int git_diff_index_to_workdir( ...@@ -1373,7 +1394,11 @@ int git_diff_index_to_workdir(
git_index *index, git_index *index,
const git_diff_options *opts) const git_diff_options *opts)
{ {
git_iterator_options a_opts = GIT_ITERATOR_OPTIONS_INIT,
b_opts = GIT_ITERATOR_OPTIONS_INIT;
git_iterator *a = NULL, *b = NULL;
git_diff *diff = NULL; git_diff *diff = NULL;
char *prefix = NULL;
int error = 0; int error = 0;
assert(out && repo); assert(out && repo);
...@@ -1383,20 +1408,24 @@ int git_diff_index_to_workdir( ...@@ -1383,20 +1408,24 @@ int git_diff_index_to_workdir(
if (!index && (error = diff_load_index(&index, repo)) < 0) if (!index && (error = diff_load_index(&index, repo)) < 0)
return error; return error;
DIFF_FROM_ITERATORS( if ((error = diff_prepare_iterator_opts(&prefix, &a_opts, GIT_ITERATOR_INCLUDE_CONFLICTS,
git_iterator_for_index(&a, repo, index, &a_opts), &b_opts, GIT_ITERATOR_DONT_AUTOEXPAND, opts)) < 0 ||
GIT_ITERATOR_INCLUDE_CONFLICTS, (error = git_iterator_for_index(&a, repo, index, &a_opts)) < 0 ||
(error = git_iterator_for_workdir(&b, repo, index, NULL, &b_opts)) < 0 ||
(error = git_diff__from_iterators(&diff, repo, a, b, opts)) < 0)
goto out;
git_iterator_for_workdir(&b, repo, index, NULL, &b_opts), if ((diff->opts.flags & GIT_DIFF_UPDATE_INDEX) && ((git_diff_generated *)diff)->index_updated)
GIT_ITERATOR_DONT_AUTOEXPAND if ((error = git_index_write(index)) < 0)
); goto out;
if (!error && (diff->opts.flags & GIT_DIFF_UPDATE_INDEX) != 0 &&
((git_diff_generated *)diff)->index_updated)
error = git_index_write(index);
if (!error)
*out = diff; *out = diff;
diff = NULL;
out:
git_iterator_free(a);
git_iterator_free(b);
git_diff_free(diff);
git__free(prefix);
return error; return error;
} }
...@@ -1407,24 +1436,33 @@ int git_diff_tree_to_workdir( ...@@ -1407,24 +1436,33 @@ int git_diff_tree_to_workdir(
git_tree *old_tree, git_tree *old_tree,
const git_diff_options *opts) const git_diff_options *opts)
{ {
git_iterator_options a_opts = GIT_ITERATOR_OPTIONS_INIT,
b_opts = GIT_ITERATOR_OPTIONS_INIT;
git_iterator *a = NULL, *b = NULL;
git_diff *diff = NULL; git_diff *diff = NULL;
char *prefix = NULL;
git_index *index; git_index *index;
int error = 0; int error;
assert(out && repo); assert(out && repo);
*out = NULL; *out = NULL;
if ((error = git_repository_index__weakptr(&index, repo))) if ((error = diff_prepare_iterator_opts(&prefix, &a_opts, 0,
return error; &b_opts, GIT_ITERATOR_DONT_AUTOEXPAND, opts) < 0) ||
(error = git_repository_index__weakptr(&index, repo)) < 0 ||
(error = git_iterator_for_tree(&a, old_tree, &a_opts)) < 0 ||
(error = git_iterator_for_workdir(&b, repo, index, old_tree, &b_opts)) < 0 ||
(error = git_diff__from_iterators(&diff, repo, a, b, opts)) < 0)
goto out;
DIFF_FROM_ITERATORS(
git_iterator_for_tree(&a, old_tree, &a_opts), 0,
git_iterator_for_workdir(&b, repo, index, old_tree, &b_opts), GIT_ITERATOR_DONT_AUTOEXPAND
);
if (!error)
*out = diff; *out = diff;
diff = NULL;
out:
git_iterator_free(a);
git_iterator_free(b);
git_diff_free(diff);
git__free(prefix);
return error; return error;
} }
...@@ -1468,24 +1506,35 @@ int git_diff_index_to_index( ...@@ -1468,24 +1506,35 @@ int git_diff_index_to_index(
git_index *new_index, git_index *new_index,
const git_diff_options *opts) const git_diff_options *opts)
{ {
git_diff *diff; git_iterator_options a_opts = GIT_ITERATOR_OPTIONS_INIT,
int error = 0; b_opts = GIT_ITERATOR_OPTIONS_INIT;
git_iterator *a = NULL, *b = NULL;
git_diff *diff = NULL;
char *prefix = NULL;
int error;
assert(out && old_index && new_index); assert(out && old_index && new_index);
*out = NULL; *out = NULL;
DIFF_FROM_ITERATORS( if ((error = diff_prepare_iterator_opts(&prefix, &a_opts, GIT_ITERATOR_DONT_IGNORE_CASE,
git_iterator_for_index(&a, repo, old_index, &a_opts), GIT_ITERATOR_DONT_IGNORE_CASE, &b_opts, GIT_ITERATOR_DONT_IGNORE_CASE, opts) < 0) ||
git_iterator_for_index(&b, repo, new_index, &b_opts), GIT_ITERATOR_DONT_IGNORE_CASE (error = git_iterator_for_index(&a, repo, old_index, &a_opts)) < 0 ||
); (error = git_iterator_for_index(&b, repo, new_index, &b_opts)) < 0 ||
(error = git_diff__from_iterators(&diff, repo, a, b, opts)) < 0)
goto out;
/* if index is in case-insensitive order, re-sort deltas to match */ /* if index is in case-insensitive order, re-sort deltas to match */
if (!error && (old_index->ignore_case || new_index->ignore_case)) if (old_index->ignore_case || new_index->ignore_case)
git_diff__set_ignore_case(diff, true); git_diff__set_ignore_case(diff, true);
if (!error)
*out = diff; *out = diff;
diff = NULL;
out:
git_iterator_free(a);
git_iterator_free(b);
git_diff_free(diff);
git__free(prefix);
return error; return error;
} }
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment