From 74e89110a38fb52be29615a1468e86fb75077433 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Fri, 19 May 2017 08:54:56 -0400 Subject: t4063: add tests of direct blob diffs The git-diff command can directly compare two blobs (or a blob and a file), but we don't test this at all. Let's add some basic tests that reveal a few problems. There are basically four interesting inputs: 1. sha1 against sha1 (where diff has no information beyond the contents) 2. tree:path against tree:path (where it can get information via get_sha1_with_context) 3. Same as (2), but using the ".." range syntax 4. tree:path against a filename And beyond generating a sane diff, we care about a few little bits: which paths they show in the diff header, and whether they correctly pick up a mode change. They should all be able to show a mode except for (1), though note that case (3) is currently broken. For the headers, we would ideally show the path within the tree if we have it, making: git diff a:path b:path look the same as: git diff a b -- path We can't always do that (e.g., in the direct sha1/sha1 diff, we have no path to show), in which case we should fall back to the name that resolved to the blob (which is nonsense from the repository's perspective, but is the best we can do). Aside from the fallback case in (1), none of the cases get this right. Cases (2) and (3) always show the full tree:path, even though we should be able to know just the "path" portion. Case (4) picks up the filename path, but assigns it to _both_ sides of the diff. So this works for: git diff tree:path path but not for: git diff tree:other_path path The appropriate tests are marked to expect failure. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- t/t4063-diff-blobs.sh | 91 +++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 91 insertions(+) create mode 100755 t/t4063-diff-blobs.sh (limited to 't/t4063-diff-blobs.sh') diff --git a/t/t4063-diff-blobs.sh b/t/t4063-diff-blobs.sh new file mode 100755 index 0000000000..90c6f6b85b --- /dev/null +++ b/t/t4063-diff-blobs.sh @@ -0,0 +1,91 @@ +#!/bin/sh + +test_description='test direct comparison of blobs via git-diff' +. ./test-lib.sh + +run_diff () { + # use full-index to make it easy to match the index line + git diff --full-index "$@" >diff +} + +check_index () { + grep "^index $1\\.\\.$2" diff +} + +check_mode () { + grep "^old mode $1" diff && + grep "^new mode $2" diff +} + +check_paths () { + grep "^diff --git a/$1 b/$2" diff +} + +test_expect_success 'create some blobs' ' + echo one >one && + echo two >two && + chmod +x two && + git add . && + + # cover systems where modes are ignored + git update-index --chmod=+x two && + + git commit -m base && + + sha1_one=$(git rev-parse HEAD:one) && + sha1_two=$(git rev-parse HEAD:two) +' + +test_expect_success 'diff by sha1' ' + run_diff $sha1_one $sha1_two +' +test_expect_success 'index of sha1 diff' ' + check_index $sha1_one $sha1_two +' +test_expect_success 'sha1 diff uses arguments as paths' ' + check_paths $sha1_one $sha1_two +' +test_expect_success 'sha1 diff has no mode change' ' + ! grep mode diff +' + +test_expect_success 'diff by tree:path (run)' ' + run_diff HEAD:one HEAD:two +' +test_expect_success 'index of tree:path diff' ' + check_index $sha1_one $sha1_two +' +test_expect_failure 'tree:path diff uses filenames as paths' ' + check_paths one two +' +test_expect_success 'tree:path diff shows mode change' ' + check_mode 100644 100755 +' + +test_expect_success 'diff by ranged tree:path' ' + run_diff HEAD:one..HEAD:two +' +test_expect_success 'index of ranged tree:path diff' ' + check_index $sha1_one $sha1_two +' +test_expect_failure 'ranged tree:path diff uses filenames as paths' ' + check_paths one two +' +test_expect_failure 'ranged tree:path diff shows mode change' ' + check_mode 100644 100755 +' + +test_expect_success 'diff blob against file' ' + run_diff HEAD:one two +' +test_expect_success 'index of blob-file diff' ' + check_index $sha1_one $sha1_two +' +test_expect_failure 'blob-file diff uses filename as paths' ' + check_paths one two +' +test_expect_success FILEMODE 'blob-file diff shows mode change' ' + check_mode 100644 100755 +' + +test_done -- cgit 1.2.3-korg From 101dd4de16eea300a59e432452fd4fc06e1ac7f2 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Fri, 19 May 2017 08:55:11 -0400 Subject: handle_revision_arg: record modes for "a..b" endpoints The "a..b" revision syntax was designed to handle commits, so it doesn't bother to record any mode we find while traversing a "tree:path" endpoint. These days "git diff" can diff blobs using either "a:path..b:path" (with dots) or "a:path b:path" (without), but the two behave inconsistently, as the with-dots version fails to notice the mode. Let's teach the dot-dot range parser to record modes; it doesn't cost us anything, and it makes this case work. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- revision.c | 10 ++++++---- t/t4063-diff-blobs.sh | 2 +- 2 files changed, 7 insertions(+), 5 deletions(-) (limited to 't/t4063-diff-blobs.sh') diff --git a/revision.c b/revision.c index 49707695ab..5b56f0e921 100644 --- a/revision.c +++ b/revision.c @@ -1448,9 +1448,11 @@ static int handle_dotdot_1(const char *arg, char *dotdot, const char *a_name, *b_name; struct object_id a_oid, b_oid; struct object *a_obj, *b_obj; + struct object_context a_oc, b_oc; unsigned int a_flags, b_flags; int symmetric = 0; unsigned int flags_exclude = flags ^ (UNINTERESTING | BOTTOM); + unsigned int oc_flags = GET_SHA1_COMMITTISH; a_name = arg; if (!*a_name) @@ -1464,8 +1466,8 @@ static int handle_dotdot_1(const char *arg, char *dotdot, if (!*b_name) b_name = "HEAD"; - if (get_sha1_committish(a_name, a_oid.hash) || - get_sha1_committish(b_name, b_oid.hash)) + if (get_sha1_with_context(a_name, oc_flags, a_oid.hash, &a_oc) || + get_sha1_with_context(b_name, oc_flags, b_oid.hash, &b_oc)) return -1; if (!cant_be_filename) { @@ -1507,8 +1509,8 @@ static int handle_dotdot_1(const char *arg, char *dotdot, b_obj->flags |= b_flags; add_rev_cmdline(revs, a_obj, a_name, REV_CMD_LEFT, a_flags); add_rev_cmdline(revs, b_obj, b_name, REV_CMD_RIGHT, b_flags); - add_pending_object(revs, a_obj, a_name); - add_pending_object(revs, b_obj, b_name); + add_pending_object_with_mode(revs, a_obj, a_name, a_oc.mode); + add_pending_object_with_mode(revs, b_obj, b_name, b_oc.mode); return 0; } diff --git a/t/t4063-diff-blobs.sh b/t/t4063-diff-blobs.sh index 90c6f6b85b..df9c35b2dd 100755 --- a/t/t4063-diff-blobs.sh +++ b/t/t4063-diff-blobs.sh @@ -71,7 +71,7 @@ test_expect_success 'index of ranged tree:path diff' ' test_expect_failure 'ranged tree:path diff uses filenames as paths' ' check_paths one two ' -test_expect_failure 'ranged tree:path diff shows mode change' ' +test_expect_success 'ranged tree:path diff shows mode change' ' check_mode 100644 100755 ' -- cgit 1.2.3-korg From 158b06caee5f9ea2987f444b8e49bd2d678b187d Mon Sep 17 00:00:00 2001 From: Jeff King Date: Fri, 19 May 2017 08:59:15 -0400 Subject: diff: use pending "path" if it is available There's a subtle distinction between "name" and "path" for a blob that we resolve: the name is what the user told us on the command line, and the path is what we traversed when finding the blob within a tree (if we did so). When we diff blobs directly, we use "name", but "path" is more likely to be useful to the user (it will find the correct .gitattributes, and give them a saner diff header). We still have to fall back to using the name for some cases (i.e., any blob reference that isn't of the form tree:path). That's the best we can do in such a case. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- builtin/diff.c | 7 ++++++- t/t4063-diff-blobs.sh | 4 ++-- 2 files changed, 8 insertions(+), 3 deletions(-) (limited to 't/t4063-diff-blobs.sh') diff --git a/builtin/diff.c b/builtin/diff.c index 4c0811d6fc..1a1149eed4 100644 --- a/builtin/diff.c +++ b/builtin/diff.c @@ -23,6 +23,11 @@ static const char builtin_diff_usage[] = "git diff [] [ []] [--] [...]"; +static const char *blob_path(struct object_array_entry *entry) +{ + return entry->path ? entry->path : entry->name; +} + static void stuff_change(struct diff_options *opt, unsigned old_mode, unsigned new_mode, const struct object_id *old_oid, @@ -110,7 +115,7 @@ static int builtin_diff_blobs(struct rev_info *revs, blob[0]->mode, blob[1]->mode, &blob[0]->item->oid, &blob[1]->item->oid, 1, 1, - blob[0]->name, blob[1]->name); + blob_path(blob[0]), blob_path(blob[1])); diffcore_std(&revs->diffopt); diff_flush(&revs->diffopt); return 0; diff --git a/t/t4063-diff-blobs.sh b/t/t4063-diff-blobs.sh index df9c35b2dd..80ce033ab6 100755 --- a/t/t4063-diff-blobs.sh +++ b/t/t4063-diff-blobs.sh @@ -55,7 +55,7 @@ test_expect_success 'diff by tree:path (run)' ' test_expect_success 'index of tree:path diff' ' check_index $sha1_one $sha1_two ' -test_expect_failure 'tree:path diff uses filenames as paths' ' +test_expect_success 'tree:path diff uses filenames as paths' ' check_paths one two ' test_expect_success 'tree:path diff shows mode change' ' @@ -68,7 +68,7 @@ test_expect_success 'diff by ranged tree:path' ' test_expect_success 'index of ranged tree:path diff' ' check_index $sha1_one $sha1_two ' -test_expect_failure 'ranged tree:path diff uses filenames as paths' ' +test_expect_success 'ranged tree:path diff uses filenames as paths' ' check_paths one two ' test_expect_success 'ranged tree:path diff shows mode change' ' -- cgit 1.2.3-korg From 30d005c02014680403b5d35ef274047ab91fa5bd Mon Sep 17 00:00:00 2001 From: Jeff King Date: Fri, 19 May 2017 08:59:34 -0400 Subject: diff: use blob path for blob/file diffs When we diff a blob against a working tree file like: git diff HEAD:Makefile Makefile we always use the working tree filename for both sides of the diff. In most cases that's fine, as the two would be the same anyway, as above. And until recently, we used the "name" for the blob, not the path, which would have the messy "HEAD:" on the beginning. But when they don't match, like: git diff HEAD:old_path new_path it makes sense to show both names. This patch uses the blob's path field if it's available, and otherwise falls back to using the filename (in preference to the blob's name, which is likely to be garbage like a raw sha1). Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- builtin/diff.c | 3 ++- t/t4063-diff-blobs.sh | 7 ++++++- 2 files changed, 8 insertions(+), 2 deletions(-) (limited to 't/t4063-diff-blobs.sh') diff --git a/builtin/diff.c b/builtin/diff.c index 1a1149eed4..5e7c6428c9 100644 --- a/builtin/diff.c +++ b/builtin/diff.c @@ -90,7 +90,8 @@ static int builtin_diff_b_f(struct rev_info *revs, blob[0]->mode, canon_mode(st.st_mode), &blob[0]->item->oid, &null_oid, 1, 0, - path, path); + blob[0]->path ? blob[0]->path : path, + path); diffcore_std(&revs->diffopt); diff_flush(&revs->diffopt); return 0; diff --git a/t/t4063-diff-blobs.sh b/t/t4063-diff-blobs.sh index 80ce033ab6..bc69e26c52 100755 --- a/t/t4063-diff-blobs.sh +++ b/t/t4063-diff-blobs.sh @@ -81,11 +81,16 @@ test_expect_success 'diff blob against file' ' test_expect_success 'index of blob-file diff' ' check_index $sha1_one $sha1_two ' -test_expect_failure 'blob-file diff uses filename as paths' ' +test_expect_success 'blob-file diff uses filename as paths' ' check_paths one two ' test_expect_success FILEMODE 'blob-file diff shows mode change' ' check_mode 100644 100755 ' +test_expect_success 'blob-file diff prefers filename to sha1' ' + run_diff $sha1_one two && + check_paths two two +' + test_done -- cgit 1.2.3-korg