On Thu, Aug 18, 2016 at 12:47 PM, Stefan Beller <sbeller@xxxxxxxxxx> wrote:
> 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?
>
I think so? I don't really know, but it was there before for
show_submodule_summary so I left it this way.
>> 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;
>
This should actually be "EMPTY_TREE_SHA1_BIN_LITERAL, and these should
be unsigned, you're right. This will be fixed differently by using
struct object_id * instead, though.
Thanks,
Jake
>
>
>> + 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;
>
>
This will also be fixed by switching to object_id.
>
>> + 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:
> */
>
Yea, sorry. I have a habbit of the other format because the Linux
kernel networking tree requires this style, despite the rest of the
Linux kernel requiring the style used by git. So, I have to break out
of that habbit but sometimes I miss them.
>> + /* 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/
>
AFAIK it actually does, because dirty_submodule won't be set with
DIRTY_SUBMODULE_MODIFIED unless you're actually diffing against the
index or worktree directly. So if you diff two commits they never show
these flags regardless of what's in your work tree.
If you diff some commit against your work tree then you might have
this flag set.
>> +
>> +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?
>
Sure
>> + 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?
>
I copied these from another file and modified them to use diff, but I
can work this to use variables consistently.
>
>> + 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)
Yea, this was copied directly from another test file and modified to
use "inline diff" format instead of log format, which is why some of
these are done this way. I'll switch this to test_config.
Thanks,
Jake
--
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