Re: [PATCH v7 7/7] diff: teach diff to display submodule difference with an inline diff

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, Aug 17, 2016 at 5:51 PM, Jacob Keller <jacob.e.keller@xxxxxxxxx> wrote:



>         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?

> 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;



> +       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;



> +       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:
 */

> +       /* 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")


> +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)
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]