Re: [PATCH v7 7/7] diff: teach diff to display submodule difference with an inline diff
On Wed, Aug 17, 2016 at 5:51 PM, Jacob Keller <jacob.e.keller@intel.com> wrote:
Show 10 quoted lines
> if (o->submodule_format == DIFF_SUBMODULE_LOG &&
> (!one->mode || S_ISGITLINK(one->mode)) &&
> (!two->mode || S_ISGITLINK(two->mode))) {
> @@ -2311,6 +2322,17 @@ static void builtin_diff(const char *name_a,
> two->dirty_submodule,
> meta, del, add, reset);
> return;
> + } else if (o->submodule_format == DIFF_SUBMODULE_INLINE_DIFF &&
> + (!one->mode || S_ISGITLINK(one->mode)) &&
> + (!two->mode || S_ISGITLINK(two->mode))) {The ! mode is for added and deleted submodules, I guess?
Show 28 quoted lines
> diff --git a/diff.h b/diff.h
> index ea5aba668eaa..192c0eedd0ff 100644
> --- a/diff.h
> +++ b/diff.h
> @@ -112,6 +112,7 @@ enum diff_words_type {
> enum diff_submodule_format {
> DIFF_SUBMODULE_SHORT = 0,
> DIFF_SUBMODULE_LOG,
> + DIFF_SUBMODULE_INLINE_DIFF,
> };
>
> struct diff_options {
> diff --git a/submodule.c b/submodule.c
> index e341ca7ffefd..e5f1138f4362 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -436,6 +436,67 @@ void show_submodule_summary(FILE *f, const char *path,
> clear_commit_marks(right, ~0);
> }
>
> +void show_submodule_inline_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 *del, const char *add, const char *reset,
> + const struct diff_options *o)
> +{
> + const char *old = EMPTY_TREE_SHA1_BIN, *new = EMPTY_TREE_SHA1_BIN;submodule.c: In function ‘show_submodule_inline_diff’:
cache.h:957:3: warning: pointer targets in initialization differ in
signedness [-Wpointer-sign]
((const unsigned char *) EMPTY_TREE_SHA1_BIN_LITERAL)
submodule.c:446:20: note: in expansion of macro ‘EMPTY_TREE_SHA1_BIN’
const char *old = EMPTY_TREE_SHA1_BIN, *new = EMPTY_TREE_SHA1_BIN;
Show 14 quoted lines
> + struct commit *left = NULL, *right = NULL;
> + struct strbuf submodule_dir = STRBUF_INIT;
> + struct child_process cp = CHILD_PROCESS_INIT;
> +
> + show_submodule_header(f, path, line_prefix, one, two, dirty_submodule,
> + meta, reset, &left, &right);
> +
> + /* We need a valid left and right commit to display a difference */
> + if (!(left || is_null_sha1(one)) ||
> + !(right || is_null_sha1(two)))
> + goto done;
> +
> + if (left)
> + old = one;
submodule.c:460:7: warning: pointer targets in assignment differ in
signedness [-Wpointer-sign]
old = one;
Show 24 quoted lines
> + if (right)
> + new = two;
> +
> + fflush(f);
> + cp.git_cmd = 1;
> + cp.dir = path;
> + cp.out = dup(fileno(f));
> + cp.no_stdin = 1;
> +
> + /* TODO: other options may need to be passed here. */
> + argv_array_pushl(&cp.args, "diff");
> + argv_array_pushf(&cp.args, "--line-prefix=%s", line_prefix);
> + if (DIFF_OPT_TST(o, REVERSE_DIFF)) {
> + argv_array_pushf(&cp.args, "--src-prefix=%s%s/",
> + o->b_prefix, path);
> + argv_array_pushf(&cp.args, "--dst-prefix=%s%s/",
> + o->a_prefix, path);
> + } else {
> + argv_array_pushf(&cp.args, "--src-prefix=%s%s/",
> + o->a_prefix, path);
> + argv_array_pushf(&cp.args, "--dst-prefix=%s%s/",
> + o->b_prefix, path);
> + }
> + argv_array_push(&cp.args, sha1_to_hex(old));submodule.c:484:2: warning: pointer targets in passing argument 1 of
‘sha1_to_hex’ differ in signedness [-Wpointer-sign]
argv_array_push(&cp.args, sha1_to_hex(old));
/*
* nit: the following comment doesn't adhere to Gits way of doing comments:
*/
Show 5 quoted lines
> + /* If the submodule has modified content, we will diff against the
> + * work tree, under the assumption that the user has asked for the
> + * diff format and wishes to actually see all differences even if they
> + * haven't yet been committed to the submodule yet.
> + */
Makes sort of sense when new is HEAD.
However if I have given an explicit sha1 in the superproject, e.g.:
# (in the e.g. gerrit with a dirty submodule, I'd still expect to
# get the dirtyness ignored, but the diff of those two states?)
git diff --submodule=diff cc82b24..5222e66 plugins/> + if (!(dirty_submodule & DIRTY_SUBMODULE_MODIFIED))
> + argv_array_push(&cp.args, sha1_to_hex(new));
> +
> +# Tested non-UTF-8 encoding
> +test_encoding="ISO8859-1"
> +# String "added" in German (translated with Google Translate), encoded in UTF-8,
> +# used in sample commit log messages in add_file() function below.
> +added=$(printf "hinzugef\303\274gt")
Show 33 quoted lines
> +add_file () {
> + (
> + cd "$1" &&
> + shift &&
> + for name
> + do
> + echo "$name" >"$name" &&
> + git add "$name" &&
> + test_tick &&
> + # "git commit -m" would break MinGW, as Windows refuse to pass
> + # $test_encoding encoded parameter to git.
> + echo "Add $name ($added $name)" | iconv -f utf-8 -t $test_encoding |
> + git -c "i18n.commitEncoding=$test_encoding" commit -F -
> + done >/dev/null &&
> + git rev-parse --short --verify HEAD
> + )
> +}
> +commit_file () {
> + test_tick &&
> + git commit "$@" -m "Commit $*" >/dev/null
> +}
> +
> +test_create_repo sm1 &&
> +add_file . foo >/dev/null
> +
> +head1=$(add_file sm1 foo1 foo2)
> +fullhead1=$(cd sm1; git rev-parse --verify HEAD)
> +
> +test_expect_success 'added submodule' '
> + git add sm1 &&
> + git diff-index -p --submodule=diff HEAD >actual &&
> + cat >expected <<-EOF &&
> + Submodule sm1 0000000...1beffeb (new submodule)Do we also want to make the 1beffeb a variable?
> + cat >expected <<-EOF &&
> + Submodule sm1 0000000...$head1 (new submodule)
In the prior test we have spelled out the sha1s, here we refer to a variable?
> + EOF
> + git config --unset diff.submodule &&
Use this one weird trick to make the tests more readable!
Use "test_config" from test-lib-functions.sh
(# Set git config, automatically unsetting it after the test is over.)
(I am involved in Git for 3 years now, but just recently was pointed at it)Stefan Beller <sbeller@google.com>