{"thread":{"id":"45968","subject":"[PATCH 0/2] Demonstrate and partially work around a gitattributes problem","startedAt":"2017-05-15T15:24:10Z","lastAt":"2017-05-24T09:57:29Z","messageCount":33,"participants":["Johannes Schindelin","Jeff King","Junio C Hamano","Philip Oakley"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"319791","messageId":"cover.1494861793.git.johannes.schindelin@gmx.de","threadId":"45968","inReplyTo":null,"subject":"[PATCH 0/2] Demonstrate and partially work around a gitattributes problem","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-05-15T15:23:44Z","receivedAt":"2017-05-15T15:24:10Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Granted, it is a bit of a less common use case to call\n\n\tgit diff : :\n\nThere are valid scenarios for such calls, of course.\n\nAnd sometimes, one may want to compare even files that are stored in\nsubdirectories instead of the top-level directory. Example:\n\n\tgit diff cafebabe:t/README bee7e55:t/README\n\nNow, the surprising part is that Git tries to read a .gitattributes\nfiles interpreting the *entire* prefix `cafebabe:t/` as a *directory\npath*. I.e. it will try to read the *file* (not the blob)\n`cafebabe:t/.gitattributes`.\n\nA remarkable side effect prevents this from happening for files in the\ntop-level directory: there is no slash in the argument, therefore the\ntop-level .gitattributes (which have been read from the index/worktree\nalready) is used.\n\nUnless, of course, one specifies the commit via a ref whose name\ncontains slashes.\n\nAs mentioned in the commit message of the patch demonstrating the\nproblem, I fear that this issue is *really* hard to fix. Certainly too\ncomplicated for me alone.\n\nSide note: I was really, really surprised, in a very positive way, that\nGit handled the scenario gracefully where I *created* files with the\nactual file paths : and :, i.e. where\n`git diff : :` is ambiguous because it\ncould refer to two objects or to two files. In this case, Git warns\nabout the ambiguity (it is *slightly* misleading that it says to\nseparate *revisions* using `--`, as we do not want to compare\nrevisions... but it is definitely better than picking one side of the\nambiguity and running with it).\n\nGit for Windows carries the second patch for ages already, and I would\nhave contributed it much earlier if I had not been busy with other\npatches. The reason I submit it now is that it conflicts with Duy's\nfopen_or_warn() patch series.\n\n\nJohannes Schindelin (2):\n gitattributes: demonstrate that Git tries to read a bogus file\n mingw: Suppress warning that :.gitattributes does not exist\n\n attr.c | 2 +-\n t/t0003-attributes.sh | 21 +++++++++++++++++++++\n 2 files changed, 22 insertions(+), 1 deletion(-)\n\n\nbase-commit: b06d3643105c8758ed019125a4399cb7efdcce2c\nPublished-As: https://github.com/dscho/git/releases/tag/no-commit-colon-gitattributes-v1\nFetch-It-Via: git fetch https://github.com/dscho/git no-commit-colon-gitattributes-v1\n-- \n2.13.0.windows.1\n\n"},{"id":"319792","messageId":"8d5e663dc74241176219a51e75624549668373f1.1494861793.git.johannes.schindelin@gmx.de","threadId":"45968","inReplyTo":"cover.1494861793.git.johannes.schindelin@gmx.de","subject":"[PATCH 1/2] gitattributes: demonstrate that Git tries to read a bogus file","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-05-15T15:23:57Z","receivedAt":"2017-05-15T15:24:12Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"This problem has been reported originally in August 2015, as\n\n\thttps://github.com/git-for-windows/git/issues/255\n\nThe symptom: when passing :/ style arguments to\n`git diff`, Git tries to read the attributes from a file called\n:/.gitattributes.\n\nThis symptom is more prominent on Windows because the colon in the file\nname is illegal, and therefore reported to the user. On Linux, the colon\nis legal, and it just so happens that that file typically does not\nexist, and therefore there are no adverse consequences.\n\nHowever, it is still a bug: Git should not even attempt to open that\nfile. Let's add a test case to demonstrate that problem, even on Linux\nand MacOSX.\n\nThe underlying problem will be really tricky to fix: the run_diff*()\nfamily of functions expects the path passed via the diff_filespec\nstructs to be the path as if it were in the worktree. However, when\nprocessing the `git diff ` invocation, Git uses\nsetup_revisions()'s parsing of the pending objects to fill in this\ninformation, and setup_revisions() simply copies the command-line\nargument, rather than reconstructing the actual *file* path.\n\nSigned-off-by: Johannes Schindelin \n---\n t/t0003-attributes.sh | 21 +++++++++++++++++++++\n 1 file changed, 21 insertions(+)\n\ndiff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh\nindex f19ae4f8ccd..a7820022d8d 100755\n--- a/t/t0003-attributes.sh\n+++ b/t/t0003-attributes.sh\n@@ -323,4 +323,25 @@ test_expect_success 'bare repository: test info/attributes' '\n \t)\n '\n \n+test_expect_failure 'a file :.gitattributes is ignored' '\n+\tgit init bogus-file &&\n+\t(\n+\t\tcd bogus-file &&\n+\t\tmkdir sub &&\n+\t\ttest_commit sub/file &&\n+\t\ttest_commit sub/file2 &&\n+\t\tcommit=$(git rev-parse HEAD) &&\n+\t\tif test_have_prereq !MINGW\n+\t\tthen\n+\t\t\t# Windows does not support colons in filenames\n+\t\t\tmkdir $commit:sub &&\n+\t\t\techo \"* -inva/id\" >$commit:sub/.gitattributes\n+\t\tfi &&\n+\t\tgit diff $commit:sub/file.t..$commit:sub/file2.t >out 2>err &&\n+\t\t! grep \"is not a valid attribute name\" err &&\n+\t\t# On Windows, there will be a warning because of the colon\n+\t\t! grep \"warning: unable to access .$commit:sub\" err\n+\t)\n+'\n+\n test_done\n-- \n2.13.0.windows.1\n\n\n"},{"id":"319793","messageId":"7db27408616e21be1d917165d04a1e69adc0db04.1494861793.git.johannes.schindelin@gmx.de","threadId":"45968","inReplyTo":"cover.1494861793.git.johannes.schindelin@gmx.de","subject":"[PATCH 2/2] mingw: Suppress warning that :.gitattributes does not exist","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-05-15T15:24:14Z","receivedAt":"2017-05-15T15:24:33Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"On Windows, a file name containing a colon is illegal. We should\ntherefore expect the corresponding errno when `fopen()` is called for a\npath of the form :.gitattributes.\n\nThis fixes the symptom reported in\n\n\thttps://github.com/git-for-windows/git/issues/255\n\nSigned-off-by: Johannes Schindelin \n---\n attr.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/attr.c b/attr.c\nindex 7e2134471cb..a0297088633 100644\n--- a/attr.c\n+++ b/attr.c\n@@ -726,7 +726,7 @@ static struct attr_stack *read_attr_from_file(const char *path, int macro_ok)\n \tint lineno = 0;\n \n \tif (!fp) {\n-\t\tif (errno != ENOENT && errno != ENOTDIR)\n+\t\tif (errno != ENOENT && errno != ENOTDIR && errno != EINVAL)\n \t\t\twarn_on_inaccessible(path);\n \t\treturn NULL;\n \t}\n-- \n2.13.0.windows.1\n"},{"id":"319906","messageId":"20170516075418.m3knwvdsr5ab6vzy@sigill.intra.peff.net","threadId":"45968","inReplyTo":"cover.1494861793.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH 0/2] Demonstrate and partially work around a gitattributes problem","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-05-16T07:54:18Z","receivedAt":"2017-05-16T07:54:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, May 15, 2017 at 05:23:44PM +0200, Johannes Schindelin wrote:\n\n> Granted, it is a bit of a less common use case to call\n> \n> \tgit diff : :\n> \n> There are valid scenarios for such calls, of course.\n> \n> And sometimes, one may want to compare even files that are stored in\n> subdirectories instead of the top-level directory. Example:\n> \n> \tgit diff cafebabe:t/README bee7e55:t/README\n> \n> Now, the surprising part is that Git tries to read a .gitattributes\n> files interpreting the *entire* prefix `cafebabe:t/` as a *directory\n> path*. I.e. it will try to read the *file* (not the blob)\n> `cafebabe:t/.gitattributes`.\n\nYou also get a very silly-looking diff header:\n\n $ git diff HEAD:Makefile HEAD~100:Makefile\n diff --git a/HEAD:Makefile b/HEAD~100:Makefile\n index e35542e63..9b36068ac 100644\n --- a/HEAD:Makefile\n +++ b/HEAD~100:Makefile\n [...]\n\nI think the problem is that diff's blob-compare should be feeding the\nactual path to the diff machinery, rather than the original name. True,\nthat carries less information, but it matches the other cases. For\ninstance:\n\n git diff HEAD~100:Makefile Makefile\n\nwill diff a blob and a file, and uses the file's path (so \"HEAD~100\"\ndoes not appear at all in the output).\n\nLikewise, doing:\n\n git diff HEAD~100 HEAD -- Makefile\n\nwill show you the diff over \"Makefile\", without bothering to mention\nthe commit endpoints.\n\nSo I think we should just be feeding \"Makefile\" to the diff code even\nfor the 2-blob case.\n\nSomething like the patch below almost gets us there, but it suffers from\nthe fact that the revision machinery doesn't record the path for the\nblobs it parses. So:\n\n 1. It's a little inefficient, because it has to re-resolve the names\n again.\n\n 2. It works for \"git diff $commit:file1 $commit:file2\", because each\n of the blobs has a name we can re-resolve. But connecting them with\n \"$commit:file1..$commit:file2\" doesn't work, because that name is\n given to _both_ blobs, and can't be resolved as a single sha1. I'd\n argue that this is actually a bug in the revision code.\n\nBoth would be solved if handle_revision_arg used get_sha1_with_context\nto record the path while resolving (struct object_entry already has a\nslot for it, but the revision code never sets it).\n\n-Peff\n\n---\ndiff --git a/builtin/diff.c b/builtin/diff.c\nindex d184aafab..e8c541f00 100644\n--- a/builtin/diff.c\n+++ b/builtin/diff.c\n@@ -406,10 +406,30 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n \t\t\tobj->flags |= flags;\n \t\t\tadd_object_array(obj, name, &ent);\n \t\t} else if (obj->type == OBJ_BLOB) {\n+\t\t\tstruct object_context oc;\n+\n \t\t\tif (2 <= blobs)\n \t\t\t\tdie(_(\"more than two blobs given: '%s'\"), name);\n-\t\t\thashcpy(blob[blobs].oid.hash, obj->oid.hash);\n-\t\t\tblob[blobs].name = name;\n+\n+\t\t\t/*\n+\t\t\t * We already resolved this once as a blob via the\n+\t\t\t * rev_info machinery, but it did not bother to collect\n+\t\t\t * any path information. Let's re-resolve to get it.\n+\t\t\t *\n+\t\t\t * There are a few corner cases where there is no path\n+\t\t\t * information (e.g., a tag pointing straight at a\n+\t\t\t * blob). In that case we'll fall back to just using\n+\t\t\t * the name the user gave us.\n+\t\t\t */\n+\t\t\tif (!get_sha1_with_context(entry->name, 0, blob[blobs].oid.hash, &oc) &&\n+\t\t\t !hashcmp(blob[blobs].oid.hash, obj->oid.hash) &&\n+\t\t\t oc.path[0]) {\n+\t\t\t\tblob[blobs].name = xstrdup(oc.path);\n+\t\t\t} else {\n+\t\t\t\tblob[blobs].name = name;\n+\t\t\t\thashcpy(blob[blobs].oid.hash, obj->oid.hash);\n+\t\t\t}\n+\n \t\t\tblob[blobs].mode = entry->mode;\n \t\t\tblobs++;\n \n"},{"id":"319908","messageId":"20170516081023.lh3zflnf473jiviq@sigill.intra.peff.net","threadId":"45968","inReplyTo":"20170516075418.m3knwvdsr5ab6vzy@sigill.intra.peff.net","subject":"Re: [PATCH 0/2] Demonstrate and partially work around a gitattributes problem","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-05-16T08:10:23Z","receivedAt":"2017-05-16T08:10:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, May 16, 2017 at 03:54:18AM -0400, Jeff King wrote:\n\n> Something like the patch below almost gets us there, but it suffers from\n> the fact that the revision machinery doesn't record the path for the\n> blobs it parses. So:\n> \n> 1. It's a little inefficient, because it has to re-resolve the names\n> again.\n> \n> 2. It works for \"git diff $commit:file1 $commit:file2\", because each\n> of the blobs has a name we can re-resolve. But connecting them with\n> \"$commit:file1..$commit:file2\" doesn't work, because that name is\n> given to _both_ blobs, and can't be resolved as a single sha1. I'd\n> argue that this is actually a bug in the revision code.\n> \n> Both would be solved if handle_revision_arg used get_sha1_with_context\n> to record the path while resolving (struct object_entry already has a\n> slot for it, but the revision code never sets it).\n\nThis turned out much easier than I feared. With the patch below:\n\n - your t0003 test passes\n\n - the diff headers are much more sensible (IMHO)\n\n - it fixes a semi-related bug in which \"git diff $blob1 $blob2\"\n compared the modes correctly, but \"git diff $blob1..$blob2\" did not\n (because the \"..\" code path did not correctly record the modes).\n\nI'll stop here for now to get any comments/feedback. There are a few\nchanges I'd make to polish this up:\n\n - the mode fix should be its own preparatory patch\n\n - teaching revision.c to pass out path info should be its own patch\n\n - the \"struct blobinfo\" in builtin/diff.c should probably call it\n \"path\" instead of \"name\" for clarity. In fact, I kind of wonder if\n this should just be an object_array element, as that has all of\n the information.\n\n - the way that \"struct object_context\" handles the paths with a\n fixed-size buffer is badly designed (by me, long ago). I may see\n what it would take to refactor that to match the technique in\n sha1_object_info_extended(), which I think has worked well in\n practice.\n\n That's orthogonal to this series, but it's been bugging me for a\n long time, and obviously the caller here would have to adapt.\n\n---\ndiff --git a/builtin/diff.c b/builtin/diff.c\nindex d184aafab..8ed1e99e3 100644\n--- a/builtin/diff.c\n+++ b/builtin/diff.c\n@@ -409,7 +409,7 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n \t\t\tif (2 <= blobs)\n \t\t\t\tdie(_(\"more than two blobs given: '%s'\"), name);\n \t\t\thashcpy(blob[blobs].oid.hash, obj->oid.hash);\n-\t\t\tblob[blobs].name = name;\n+\t\t\tblob[blobs].name = entry->path ? entry->path : name;\n \t\t\tblob[blobs].mode = entry->mode;\n \t\t\tblobs++;\n \ndiff --git a/revision.c b/revision.c\nindex 8a8c1789c..72b9af7de 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -1431,7 +1431,7 @@ static void prepare_show_merge(struct rev_info *revs)\n \n int handle_revision_arg(const char *arg_, struct rev_info *revs, int flags, unsigned revarg_opt)\n {\n-\tstruct object_context oc;\n+\tstruct object_context oc, oc2;\n \tchar *dotdot;\n \tstruct object *object;\n \tunsigned char sha1[20];\n@@ -1470,8 +1470,8 @@ int handle_revision_arg(const char *arg_, struct rev_info *revs, int flags, unsi\n \t\t\t\treturn -1;\n \t\t\t}\n \t\t}\n-\t\tif (!get_sha1_committish(this, from_sha1) &&\n-\t\t !get_sha1_committish(next, sha1)) {\n+\t\tif (!get_sha1_with_context(this, GET_SHA1_COMMITTISH, from_sha1, &oc) &&\n+\t\t !get_sha1_with_context(next, GET_SHA1_COMMITTISH, sha1, &oc2)) {\n \t\t\tstruct object *a_obj, *b_obj;\n \n \t\t\tif (!cant_be_filename) {\n@@ -1523,8 +1523,12 @@ int handle_revision_arg(const char *arg_, struct rev_info *revs, int flags, unsi\n \t\t\t\t\tREV_CMD_LEFT, a_flags);\n \t\t\tadd_rev_cmdline(revs, b_obj, next,\n \t\t\t\t\tREV_CMD_RIGHT, flags);\n-\t\t\tadd_pending_object(revs, a_obj, this);\n-\t\t\tadd_pending_object(revs, b_obj, next);\n+\t\t\tadd_pending_object_with_path(revs, a_obj, this,\n+\t\t\t\t\t\t oc.mode,\n+\t\t\t\t\t\t oc.path[0] ? oc.path : NULL);\n+\t\t\tadd_pending_object_with_path(revs, b_obj, next,\n+\t\t\t\t\t\t oc2.mode,\n+\t\t\t\t\t\t oc2.path[0] ? oc2.path : NULL);\n \t\t\treturn 0;\n \t\t}\n \t\t*dotdot = '.';\n@@ -1574,7 +1578,7 @@ int handle_revision_arg(const char *arg_, struct rev_info *revs, int flags, unsi\n \t\tverify_non_filename(revs->prefix, arg);\n \tobject = get_reference(revs, arg, sha1, flags ^ local_flags);\n \tadd_rev_cmdline(revs, object, arg_, REV_CMD_REV, flags ^ local_flags);\n-\tadd_pending_object_with_mode(revs, object, arg, oc.mode);\n+\tadd_pending_object_with_path(revs, object, arg, oc.mode, oc.path);\n \treturn 0;\n }\n \n"},{"id":"319982","messageId":"xmqqshk4z2hv.fsf@gitster.mtv.corp.google.com","threadId":"45968","inReplyTo":"20170516081023.lh3zflnf473jiviq@sigill.intra.peff.net","subject":"Re: [PATCH 0/2] Demonstrate and partially work around a gitattributes problem","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-05-17T01:38:36Z","receivedAt":"2017-05-17T01:38:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> diff --git a/builtin/diff.c b/builtin/diff.c\n> index d184aafab..8ed1e99e3 100644\n> --- a/builtin/diff.c\n> +++ b/builtin/diff.c\n> @@ -409,7 +409,7 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n> \t\t\tif (2 <= blobs)\n> \t\t\t\tdie(_(\"more than two blobs given: '%s'\"), name);\n> \t\t\thashcpy(blob[blobs].oid.hash, obj->oid.hash);\n> -\t\t\tblob[blobs].name = name;\n> +\t\t\tblob[blobs].name = entry->path ? entry->path : name;\n> \t\t\tblob[blobs].mode = entry->mode;\n> \t\t\tblobs++;\n> ...\n> -\t\t\tadd_pending_object(revs, a_obj, this);\n> -\t\t\tadd_pending_object(revs, b_obj, next);\n> +\t\t\tadd_pending_object_with_path(revs, a_obj, this,\n> +\t\t\t\t\t\t oc.mode,\n> +\t\t\t\t\t\t oc.path[0] ? oc.path : NULL);\n> +\t\t\tadd_pending_object_with_path(revs, b_obj, next,\n> +\t\t\t\t\t\t oc2.mode,\n> +\t\t\t\t\t\t oc2.path[0] ? oc2.path : NULL);\n\nThe fix is surprisingly simple, and I think it definitely goes in\nthe right direction.\n\nSomehow, it bothers me to be forced to view (a_obj, this, from_sha1,\noc) vs (b_obj, next, sha1, oc2) as a sensibly corresponding pair of\ntuples, but that is not something your introduction of \"oc2\"\nstarted, so I won't complain ;-).\n\n> \t\t\treturn 0;\n> \t\t}\n> \t\t*dotdot = '.';\n> @@ -1574,7 +1578,7 @@ int handle_revision_arg(const char *arg_, struct rev_info *revs, int flags, unsi\n> \t\tverify_non_filename(revs->prefix, arg);\n> \tobject = get_reference(revs, arg, sha1, flags ^ local_flags);\n> \tadd_rev_cmdline(revs, object, arg_, REV_CMD_REV, flags ^ local_flags);\n> -\tadd_pending_object_with_mode(revs, object, arg, oc.mode);\n> +\tadd_pending_object_with_path(revs, object, arg, oc.mode, oc.path);\n> \treturn 0;\n> }\n> \n"},{"id":"319985","messageId":"20170517020535.qqmw2yncfomd3hfb@sigill.intra.peff.net","threadId":"45968","inReplyTo":"xmqqshk4z2hv.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 0/2] Demonstrate and partially work around a gitattributes problem","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-05-17T02:05:35Z","receivedAt":"2017-05-17T02:05:41Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, May 17, 2017 at 10:38:36AM +0900, Junio C Hamano wrote:\n\n> > -\t\t\tadd_pending_object(revs, a_obj, this);\n> > -\t\t\tadd_pending_object(revs, b_obj, next);\n> > +\t\t\tadd_pending_object_with_path(revs, a_obj, this,\n> > +\t\t\t\t\t\t oc.mode,\n> > +\t\t\t\t\t\t oc.path[0] ? oc.path : NULL);\n> > +\t\t\tadd_pending_object_with_path(revs, b_obj, next,\n> > +\t\t\t\t\t\t oc2.mode,\n> > +\t\t\t\t\t\t oc2.path[0] ? oc2.path : NULL);\n> \n> The fix is surprisingly simple, and I think it definitely goes in\n> the right direction.\n> \n> Somehow, it bothers me to be forced to view (a_obj, this, from_sha1,\n> oc) vs (b_obj, next, sha1, oc2) as a sensibly corresponding pair of\n> tuples, but that is not something your introduction of \"oc2\"\n> started, so I won't complain ;-).\n\nYes, in my polishing I switched this out at least \"oc_a\" and \"oc_b\" so\nwe could be sure they match correctly. I think this whole \"dotdot\"\nconditional could be pulled out to its own function, and probably\nconsistently use \"a\" and \"b\" for the endpoints. I'll see what\nrefactoring I can do to make it more readable.\n\n-Peff\n"},{"id":"320180","messageId":"alpine.DEB.2.21.1.1705182122430.3610@virtualbox","threadId":"45968","inReplyTo":"20170517020535.qqmw2yncfomd3hfb@sigill.intra.peff.net","subject":"Re: [PATCH 0/2] Demonstrate and partially work around a gitattributes problem","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-05-18T19:23:17Z","receivedAt":"2017-05-18T19:23:39Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Tue, 16 May 2017, Jeff King wrote:\n\n> On Wed, May 17, 2017 at 10:38:36AM +0900, Junio C Hamano wrote:\n> \n> > > -\t\t\tadd_pending_object(revs, a_obj, this);\n> > > -\t\t\tadd_pending_object(revs, b_obj, next);\n> > > +\t\t\tadd_pending_object_with_path(revs, a_obj, this,\n> > > +\t\t\t\t\t\t oc.mode,\n> > > +\t\t\t\t\t\t oc.path[0] ? oc.path : NULL);\n> > > +\t\t\tadd_pending_object_with_path(revs, b_obj, next,\n> > > +\t\t\t\t\t\t oc2.mode,\n> > > +\t\t\t\t\t\t oc2.path[0] ? oc2.path : NULL);\n> > \n> > The fix is surprisingly simple, and I think it definitely goes in\n> > the right direction.\n> > \n> > Somehow, it bothers me to be forced to view (a_obj, this, from_sha1,\n> > oc) vs (b_obj, next, sha1, oc2) as a sensibly corresponding pair of\n> > tuples, but that is not something your introduction of \"oc2\"\n> > started, so I won't complain ;-).\n> \n> Yes, in my polishing I switched this out at least \"oc_a\" and \"oc_b\" so\n> we could be sure they match correctly. I think this whole \"dotdot\"\n> conditional could be pulled out to its own function, and probably\n> consistently use \"a\" and \"b\" for the endpoints. I'll see what\n> refactoring I can do to make it more readable.\n\nSo you drive this forward? That would really help me a lot. Please make my\n1/2 part of your patches to that end.\n\nThanks,\nDscho\n"},{"id":"320243","messageId":"20170519000016.bvqaml6d3o6q6qus@sigill.intra.peff.net","threadId":"45968","inReplyTo":"alpine.DEB.2.21.1.1705182122430.3610@virtualbox","subject":"Re: [PATCH 0/2] Demonstrate and partially work around a gitattributes problem","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-05-19T00:00:16Z","receivedAt":"2017-05-19T00:00:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, May 18, 2017 at 09:23:17PM +0200, Johannes Schindelin wrote:\n\n> So you drive this forward? That would really help me a lot. Please make my\n> 1/2 part of your patches to that end.\n\nYes, sorry, I haven't gotten a chance to polish the patches yet.\n\n-Peff\n"},{"id":"320255","messageId":"20170519124651.4q7waz75rmzfopgn@sigill.intra.peff.net","threadId":"45968","inReplyTo":"20170517020535.qqmw2yncfomd3hfb@sigill.intra.peff.net","subject":"[PATCH 0/15] retain blob info for git diff HEAD:foo HEAD:bar","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-05-19T12:46:51Z","receivedAt":"2017-05-19T12:47:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, May 16, 2017 at 10:05:35PM -0400, Jeff King wrote:\n\n> On Wed, May 17, 2017 at 10:38:36AM +0900, Junio C Hamano wrote:\n> \n> > > -\t\t\tadd_pending_object(revs, a_obj, this);\n> > > -\t\t\tadd_pending_object(revs, b_obj, next);\n> > > +\t\t\tadd_pending_object_with_path(revs, a_obj, this,\n> > > +\t\t\t\t\t\t oc.mode,\n> > > +\t\t\t\t\t\t oc.path[0] ? oc.path : NULL);\n> > > +\t\t\tadd_pending_object_with_path(revs, b_obj, next,\n> > > +\t\t\t\t\t\t oc2.mode,\n> > > +\t\t\t\t\t\t oc2.path[0] ? oc2.path : NULL);\n> > \n> > The fix is surprisingly simple, and I think it definitely goes in\n> > the right direction.\n> > \n> > Somehow, it bothers me to be forced to view (a_obj, this, from_sha1,\n> > oc) vs (b_obj, next, sha1, oc2) as a sensibly corresponding pair of\n> > tuples, but that is not something your introduction of \"oc2\"\n> > started, so I won't complain ;-).\n> \n> Yes, in my polishing I switched this out at least \"oc_a\" and \"oc_b\" so\n> we could be sure they match correctly. I think this whole \"dotdot\"\n> conditional could be pulled out to its own function, and probably\n> consistently use \"a\" and \"b\" for the endpoints. I'll see what\n> refactoring I can do to make it more readable.\n\nThe cleanup ended up being a bit bigger than I expected, but I did find\nan obscure bug while I was there. And I think the end result looks\npretty decent.\n\nThe patches are listed below. There are a few things I didn't do (and\ndon't plan to in the near future):\n\n - I didn't pick up Dscho's gitattributes test. The tests in patch 9\n cover that in a much more direct fashion by looking at the diff\n headers. We _could_ still check that they respect gitattributes, but\n there's no reason that they wouldn't; the bug was before we even hit\n the diff machinery at all.\n\n - There is an interesting related case with attributes that wasn't\n tested by that case. If you do:\n\n git diff $(git rev-parse HEAD:one) $(git rev-parse HEAD:two)\n\n then we'll feed those sha1s to the diff machinery as the path (we\n have enough information at that point to know they aren't actually\n paths, but we have to give _something_ to the diff code to display).\n\n That means we'll also look for .gitattributes files that match those\n names. It would be hard to fix (because the diff machinery doesn't\n have a notion of \"this path is for display, but not for attributes).\n And I doubt anybody cares (AIUI, the motivation for the original was\n not that somebody was concerned with reading what is likely to be a\n non-existent attributes file, but rather that looking at names with\n colons produces a bogus warning on Windows).\n\n - I noticed that the dotdot parser jumps into the middle of the string\n with a strstr(), and never tries to find other dotdots. That means\n things like:\n\n HEAD@{2..days.ago}...HEAD@{3..days.ago}\n\n does not parse (we find the first \"..\", realize that it's not a\n range operator, and then give up looking for more range operators).\n That could be solved by either parsing left-to-right, or by trying\n each \"..\" in the string in turn. I doubt anybody cares overly much.\n\n I think we do get the related:\n\n git show HEAD:foo..bar\n\n correct, because we see that \"HEAD:foo\" is not a commit and bail to\n trying the whole thing as a single unit.\n\nI think those are all minor problems that have likely been around for\n10+ years, and would take a lot of digging and work to fix. Unless\nsomebody actually hits one in practice, I'm happy to punt for another\ndecade.\n\n [01/15]: handle_revision_arg: reset \"dotdot\" consistently\n [02/15]: handle_revision_arg: simplify commit reference lookups\n [03/15]: handle_revision_arg: stop using \"dotdot\" as a generic pointer\n [04/15]: handle_revision_arg: hoist \"..\" check out of range parsing\n [05/15]: handle_revision_arg: add handle_dotdot() helper\n [06/15]: sha1_name: consistently refer to object_context as \"oc\"\n [07/15]: get_sha1_with_context: always initialize oc->symlink_path\n [08/15]: get_sha1_with_context: dynamically allocate oc->path\n [09/15]: t4063: add tests of direct blob diffs\n [10/15]: handle_revision_arg: record modes for \"a..b\" endpoints\n [11/15]: handle_revision_arg: record paths for pending objects\n [12/15]: diff: pass whole pending entry in blobinfo\n [13/15]: diff: use the word \"path\" instead of \"name\" for blobs\n [14/15]: diff: use pending \"path\" if it is available\n [15/15]: diff: use blob path for blob/file diffs\n\n builtin/cat-file.c | 4 +-\n builtin/diff.c | 60 ++++++-------\n builtin/grep.c | 4 +-\n builtin/log.c | 10 ++-\n cache.h | 10 ++-\n revision.c | 243 +++++++++++++++++++++++++++++---------------------\n sha1_name.c | 11 ++-\n t/t4063-diff-blobs.sh | 96 ++++++++++++++++++++\n t/t4202-log.sh | 9 ++\n tree-walk.c | 1 -\n 10 files changed, 301 insertions(+), 147 deletions(-)\n create mode 100755 t/t4063-diff-blobs.sh\n\n-Peff\n"},{"id":"320256","messageId":"20170519124834.ipk42isngqck2egu@sigill.intra.peff.net","threadId":"45968","inReplyTo":"20170519124651.4q7waz75rmzfopgn@sigill.intra.peff.net","subject":"[PATCH 01/15] handle_revision_arg: reset \"dotdot\" consistently","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-05-19T12:48:34Z","receivedAt":"2017-05-19T12:48:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we are parsing a range like \"a..b\", we write a\ntemporary NUL over the first \".\", so that we can access the\nnames \"a\" and \"b\" as C strings. But our restoration of the\noriginal \".\" is done at inconsistent times, which can lead\nto confusing results.\n\nFor most calls, we restore the \".\" after we resolve the\nnames, but before we call verify_non_filename(). This means\nthat when we later call add_pending_object(), the name for\nthe left-hand \"a\" has been re-expanded to \"a..b\". You can\nsee this with:\n\n git log --source a...b\n\nwhere \"b\" will be correctly marked with \"b\", but \"a\" will be\nmarked with \"a...b\". Likewise with \"a..b\" (though you need\nto use --boundary to even see \"a\" at all in that case).\n\nTo top off the confusion, when the REVARG_CANNOT_BE_FILENAME\nflag is set, we skip the non-filename check, and leave the\nNUL in place.\n\nThat means we do report the correct name for \"a\" in the\npending array. But some code paths try to show the whole\n\"a..b\" name in error messages, and these erroneously show\nonly \"a\" instead of \"a..b\". E.g.:\n\n $ git cherry-pick HEAD:foo..HEAD:foo\n error: object d95f3ad14dee633a758d2e331151e950dd13e4ed is a blob, not a commit\n error: object d95f3ad14dee633a758d2e331151e950dd13e4ed is a blob, not a commit\n fatal: Invalid symmetric difference expression HEAD:foo\n\n(That last message should be \"HEAD:foo...HEAD:foo\"; I used\ncherry-pick because it passes the CANNOT_BE_FILENAME flag).\n\nAs an interesting side note, cherry-pick actually looks at\nand re-resolves the arguments from the pending->name fields.\nSo it would have been visibly broken by the first bug, but\nthe effect was canceled out by the second one.\n\nThis patch makes the whole function consistent by re-writing\nthe NUL immediately after calling verify_non_filename(), and\nthen restoring the \".\" as appropriate in some error-printing\nand early-return code paths.\n\nSigned-off-by: Jeff King \n---\nI also considered just making a copy of the string rather than this\nin-place munging (technically we get it as a pointer-to-const; it's only\nthe use of strstr() that lets us quietly drop the const). But it doesn't\nreally make the code any cleaner; now instead of restoring the dot you\nhave to remember to free() the string in each code path.\n\n revision.c | 3 +++\n t/t4202-log.sh | 9 +++++++++\n 2 files changed, 12 insertions(+)\n\ndiff --git a/revision.c b/revision.c\nindex 8a8c1789c..014bf52e3 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -1477,12 +1477,14 @@ int handle_revision_arg(const char *arg_, struct rev_info *revs, int flags, unsi\n \t\t\tif (!cant_be_filename) {\n \t\t\t\t*dotdot = '.';\n \t\t\t\tverify_non_filename(revs->prefix, arg);\n+\t\t\t\t*dotdot = '\\0';\n \t\t\t}\n \n \t\t\ta_obj = parse_object(from_sha1);\n \t\t\tb_obj = parse_object(sha1);\n \t\t\tif (!a_obj || !b_obj) {\n \t\t\tmissing:\n+\t\t\t\t*dotdot = '.';\n \t\t\t\tif (revs->ignore_missing)\n \t\t\t\t\treturn 0;\n \t\t\t\tdie(symmetric\n@@ -1525,6 +1527,7 @@ int handle_revision_arg(const char *arg_, struct rev_info *revs, int flags, unsi\n \t\t\t\t\tREV_CMD_RIGHT, flags);\n \t\t\tadd_pending_object(revs, a_obj, this);\n \t\t\tadd_pending_object(revs, b_obj, next);\n+\t\t\t*dotdot = '.';\n \t\t\treturn 0;\n \t\t}\n \t\t*dotdot = '.';\ndiff --git a/t/t4202-log.sh b/t/t4202-log.sh\nindex f57799071..6da1bbe91 100755\n--- a/t/t4202-log.sh\n+++ b/t/t4202-log.sh\n@@ -1380,4 +1380,13 @@ test_expect_success 'log --source paints tag names' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'log --source paints symmetric ranges' '\n+\tcat >expect <<-\\EOF &&\n+\t09e12a9\tsource-b three\n+\t8e393e1\tsource-a two\n+\tEOF\n+\tgit log --oneline --source source-a...source-b >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.13.0.219.g63f6bc368\n\n"},{"id":"320257","messageId":"20170519124846.svasfmiukrm3r7dj@sigill.intra.peff.net","threadId":"45968","inReplyTo":"20170519124651.4q7waz75rmzfopgn@sigill.intra.peff.net","subject":"[PATCH 02/15] handle_revision_arg: simplify commit reference lookups","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-05-19T12:48:46Z","receivedAt":"2017-05-19T12:49:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The \"dotdot\" range parser avoids calling\nlookup_commit_reference() if we are directly fed two\ncommits. But its casts are unnecessarily complex; that\nfunction will just return a commit we pass into it.\n\nJust calling the function all the time is much simpler, and\ndoesn't do any significant extra work (the object is already\nparsed, and deref_tag() on a non-tag is a noop; we do incur\none extra lookup_object() call, but that's fairly trivial).\n\nSigned-off-by: Jeff King \n---\n revision.c | 8 ++------\n 1 file changed, 2 insertions(+), 6 deletions(-)\n\ndiff --git a/revision.c b/revision.c\nindex 014bf52e3..b6031fb35 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -1500,12 +1500,8 @@ int handle_revision_arg(const char *arg_, struct rev_info *revs, int flags, unsi\n \t\t\t\tstruct commit *a, *b;\n \t\t\t\tstruct commit_list *exclude;\n \n-\t\t\t\ta = (a_obj->type == OBJ_COMMIT\n-\t\t\t\t ? (struct commit *)a_obj\n-\t\t\t\t : lookup_commit_reference(a_obj->oid.hash));\n-\t\t\t\tb = (b_obj->type == OBJ_COMMIT\n-\t\t\t\t ? (struct commit *)b_obj\n-\t\t\t\t : lookup_commit_reference(b_obj->oid.hash));\n+\t\t\t\ta = lookup_commit_reference(a_obj->oid.hash);\n+\t\t\t\tb = lookup_commit_reference(b_obj->oid.hash);\n \t\t\t\tif (!a || !b)\n \t\t\t\t\tgoto missing;\n \t\t\t\texclude = get_merge_bases(a, b);\n-- \n2.13.0.219.g63f6bc368\n\n"},{"id":"320258","messageId":"20170519125006.gbvzr2dqrcycmpx7@sigill.intra.peff.net","threadId":"45968","inReplyTo":"20170519124651.4q7waz75rmzfopgn@sigill.intra.peff.net","subject":"[PATCH 03/15] handle_revision_arg: stop using \"dotdot\" as a generic pointer","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-05-19T12:50:07Z","receivedAt":"2017-05-19T12:50:13Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The handle_revision_arg() function has a \"dotdot\" variable\nthat it uses to find a \"..\" or \"...\" in the argument. If we\ndon't find one, we look for other marks, like \"^!\". But we\njust keep re-using the \"dotdot\" variable, which is\nconfusing.\n\nLet's introduce a separate \"mark\" variable that can be used\nfor these other marks. They still reuse the same variable,\nbut at least the name is no longer actively misleading.\n\nSigned-off-by: Jeff King \n---\nIt may make sense to pull each of these into its own helper. I didn't\nreally look because they're so small, and because the return semantics\nseemed confusing to me. Some of them return, and some of them keep\nparsing. Some of them restore the NUL they overwrite, and some do not.\n\nI didn't dig in to see if there are weird corner cases where they\nmisbehave.\n\n revision.c | 29 +++++++++++++++--------------\n 1 file changed, 15 insertions(+), 14 deletions(-)\n\ndiff --git a/revision.c b/revision.c\nindex b6031fb35..9c683874d 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -1433,6 +1433,7 @@ int handle_revision_arg(const char *arg_, struct rev_info *revs, int flags, unsi\n {\n \tstruct object_context oc;\n \tchar *dotdot;\n+\tchar *mark;\n \tstruct object *object;\n \tunsigned char sha1[20];\n \tint local_flags;\n@@ -1529,33 +1530,33 @@ int handle_revision_arg(const char *arg_, struct rev_info *revs, int flags, unsi\n \t\t*dotdot = '.';\n \t}\n \n-\tdotdot = strstr(arg, \"^@\");\n-\tif (dotdot && !dotdot[2]) {\n-\t\t*dotdot = 0;\n+\tmark = strstr(arg, \"^@\");\n+\tif (mark && !mark[2]) {\n+\t\t*mark = 0;\n \t\tif (add_parents_only(revs, arg, flags, 0))\n \t\t\treturn 0;\n-\t\t*dotdot = '^';\n+\t\t*mark = '^';\n \t}\n-\tdotdot = strstr(arg, \"^!\");\n-\tif (dotdot && !dotdot[2]) {\n-\t\t*dotdot = 0;\n+\tmark = strstr(arg, \"^!\");\n+\tif (mark && !mark[2]) {\n+\t\t*mark = 0;\n \t\tif (!add_parents_only(revs, arg, flags ^ (UNINTERESTING | BOTTOM), 0))\n-\t\t\t*dotdot = '^';\n+\t\t\t*mark = '^';\n \t}\n-\tdotdot = strstr(arg, \"^-\");\n-\tif (dotdot) {\n+\tmark = strstr(arg, \"^-\");\n+\tif (mark) {\n \t\tint exclude_parent = 1;\n \n-\t\tif (dotdot[2]) {\n+\t\tif (mark[2]) {\n \t\t\tchar *end;\n-\t\t\texclude_parent = strtoul(dotdot + 2, &end, 10);\n+\t\t\texclude_parent = strtoul(mark + 2, &end, 10);\n \t\t\tif (*end != '\\0' || !exclude_parent)\n \t\t\t\treturn -1;\n \t\t}\n \n-\t\t*dotdot = 0;\n+\t\t*mark = 0;\n \t\tif (!add_parents_only(revs, arg, flags ^ (UNINTERESTING | BOTTOM), exclude_parent))\n-\t\t\t*dotdot = '^';\n+\t\t\t*mark = '^';\n \t}\n \n \tlocal_flags = 0;\n-- \n2.13.0.219.g63f6bc368\n\n"},{"id":"320259","messageId":"20170519125139.rrrlwpptqsvigus3@sigill.intra.peff.net","threadId":"45968","inReplyTo":"20170519124651.4q7waz75rmzfopgn@sigill.intra.peff.net","subject":"[PATCH 04/15] handle_revision_arg: hoist \"..\" check out of range parsing","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-05-19T12:51:40Z","receivedAt":"2017-05-19T12:51:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Since 003c84f6d (specifying ranges: we did not mean to make\n\"..\" an empty set, 2011-05-02), we treat the argument \"..\"\nspecially. We detect it by noticing that both sides of the\nrange are empty, and that this is a non-symmetric two-dot\nrange.\n\nWhile correct, this makes the code overly complicated. We\ncan just detect \"..\" up front before we try to do further\nparsing. This avoids having to de-munge the NUL from dotdot,\nand lets us eliminate an extra const array (which we needed\nonly to do direct pointer comparisons).\n\nIt also removes the one code path from the range-parsing\nconditional that requires us to return -1. That will make it\nsimpler to pull the dotdot parsing out into its own\nfunction.\n\nSigned-off-by: Jeff King \n---\nThere's a similar call in rev-parse which could maybe take the same\ncleanup. I didn't bother with it because my main goal was removing an\nobstacle to factoring out the dotdot parsing (though in an ideal world,\nrev-parse would actually use the same dotdot parsing code; I've no idea\nhow complicated that would be).\n\n revision.c | 24 ++++++++++--------------\n 1 file changed, 10 insertions(+), 14 deletions(-)\n\ndiff --git a/revision.c b/revision.c\nindex 9c683874d..dec06ff8b 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -1443,6 +1443,14 @@ int handle_revision_arg(const char *arg_, struct rev_info *revs, int flags, unsi\n \n \tflags = flags & UNINTERESTING ? flags | BOTTOM : flags & ~BOTTOM;\n \n+\tif (!cant_be_filename && !strcmp(arg, \"..\")) {\n+\t\t/*\n+\t\t * Just \"..\"? That is not a range but the\n+\t\t * pathspec for the parent directory.\n+\t\t */\n+\t\treturn -1;\n+\t}\n+\n \tdotdot = strstr(arg, \"..\");\n \tif (dotdot) {\n \t\tunsigned char from_sha1[20];\n@@ -1450,27 +1458,15 @@ int handle_revision_arg(const char *arg_, struct rev_info *revs, int flags, unsi\n \t\tconst char *this = arg;\n \t\tint symmetric = *next == '.';\n \t\tunsigned int flags_exclude = flags ^ (UNINTERESTING | BOTTOM);\n-\t\tstatic const char head_by_default[] = \"HEAD\";\n \t\tunsigned int a_flags;\n \n \t\t*dotdot = 0;\n \t\tnext += symmetric;\n \n \t\tif (!*next)\n-\t\t\tnext = head_by_default;\n+\t\t\tnext = \"HEAD\";\n \t\tif (dotdot == arg)\n-\t\t\tthis = head_by_default;\n-\t\tif (this == head_by_default && next == head_by_default &&\n-\t\t !symmetric) {\n-\t\t\t/*\n-\t\t\t * Just \"..\"? That is not a range but the\n-\t\t\t * pathspec for the parent directory.\n-\t\t\t */\n-\t\t\tif (!cant_be_filename) {\n-\t\t\t\t*dotdot = '.';\n-\t\t\t\treturn -1;\n-\t\t\t}\n-\t\t}\n+\t\t\tthis = \"HEAD\";\n \t\tif (!get_sha1_committish(this, from_sha1) &&\n \t\t !get_sha1_committish(next, sha1)) {\n \t\t\tstruct object *a_obj, *b_obj;\n-- \n2.13.0.219.g63f6bc368\n\n"},{"id":"320260","messageId":"20170519125200.mtpchnyg26re5u4h@sigill.intra.peff.net","threadId":"45968","inReplyTo":"20170519124651.4q7waz75rmzfopgn@sigill.intra.peff.net","subject":"[PATCH 05/15] handle_revision_arg: add handle_dotdot() helper","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-05-19T12:52:00Z","receivedAt":"2017-05-19T12:52:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The handle_revision_arg function is rather long, and a big\nchunk of it is handling the range operators. Let's pull that\nout to a separate helper. While we're doing so, we can clean\nup a few of the rough edges that made the flow hard to\nfollow:\n\n - instead of manually restoring *dotdot (that we overwrote\n with a NUL), do the real work in a sub-helper, which\n makes it clear that the munge/restore lines are a\n matched pair\n\n - eliminate a goto which wasn't actually used for control\n flow, but only to avoid duplicating a few lines\n (instead, those lines are pushed into another helper\n function)\n\n - use early returns instead of deep nesting\n\n - consistently name all variables for the left-hand side\n of the range as \"a\" (rather than \"this\" or \"from\") and\n the right-hand side as \"b\" (rather than \"next\", or using\n the unadorned \"sha1\" or \"flags\" from the main function).\n\nSigned-off-by: Jeff King \n---\n revision.c | 177 +++++++++++++++++++++++++++++++++++--------------------------\n 1 file changed, 102 insertions(+), 75 deletions(-)\n\ndiff --git a/revision.c b/revision.c\nindex dec06ff8b..eb45501fd 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -1429,10 +1429,109 @@ static void prepare_show_merge(struct rev_info *revs)\n \trevs->limited = 1;\n }\n \n+static int dotdot_missing(const char *arg, char *dotdot,\n+\t\t\t struct rev_info *revs, int symmetric)\n+{\n+\tif (revs->ignore_missing)\n+\t\treturn 0;\n+\t/* de-munge so we report the full argument */\n+\t*dotdot = '.';\n+\tdie(symmetric\n+\t ? \"Invalid symmetric difference expression %s\"\n+\t : \"Invalid revision range %s\", arg);\n+}\n+\n+static int handle_dotdot_1(const char *arg, char *dotdot,\n+\t\t\t struct rev_info *revs, int flags,\n+\t\t\t int cant_be_filename)\n+{\n+\tconst char *a_name, *b_name;\n+\tstruct object_id a_oid, b_oid;\n+\tstruct object *a_obj, *b_obj;\n+\tunsigned int a_flags, b_flags;\n+\tint symmetric = 0;\n+\tunsigned int flags_exclude = flags ^ (UNINTERESTING | BOTTOM);\n+\n+\ta_name = arg;\n+\tif (!*a_name)\n+\t\ta_name = \"HEAD\";\n+\n+\tb_name = dotdot + 2;\n+\tif (*b_name == '.') {\n+\t\tsymmetric = 1;\n+\t\tb_name++;\n+\t}\n+\tif (!*b_name)\n+\t\tb_name = \"HEAD\";\n+\n+\tif (get_sha1_committish(a_name, a_oid.hash) ||\n+\t get_sha1_committish(b_name, b_oid.hash))\n+\t\treturn -1;\n+\n+\tif (!cant_be_filename) {\n+\t\t*dotdot = '.';\n+\t\tverify_non_filename(revs->prefix, arg);\n+\t\t*dotdot = '\\0';\n+\t}\n+\n+\ta_obj = parse_object(a_oid.hash);\n+\tb_obj = parse_object(b_oid.hash);\n+\tif (!a_obj || !b_obj)\n+\t\treturn dotdot_missing(arg, dotdot, revs, symmetric);\n+\n+\tif (!symmetric) {\n+\t\t/* just A..B */\n+\t\tb_flags = flags;\n+\t\ta_flags = flags_exclude;\n+\t} else {\n+\t\t/* A...B -- find merge bases between the two */\n+\t\tstruct commit *a, *b;\n+\t\tstruct commit_list *exclude;\n+\n+\t\ta = lookup_commit_reference(a_obj->oid.hash);\n+\t\tb = lookup_commit_reference(b_obj->oid.hash);\n+\t\tif (!a || !b)\n+\t\t\treturn dotdot_missing(arg, dotdot, revs, symmetric);\n+\n+\t\texclude = get_merge_bases(a, b);\n+\t\tadd_rev_cmdline_list(revs, exclude, REV_CMD_MERGE_BASE,\n+\t\t\t\t flags_exclude);\n+\t\tadd_pending_commit_list(revs, exclude, flags_exclude);\n+\t\tfree_commit_list(exclude);\n+\n+\t\tb_flags = flags;\n+\t\ta_flags = flags | SYMMETRIC_LEFT;\n+\t}\n+\n+\ta_obj->flags |= a_flags;\n+\tb_obj->flags |= b_flags;\n+\tadd_rev_cmdline(revs, a_obj, a_name, REV_CMD_LEFT, a_flags);\n+\tadd_rev_cmdline(revs, b_obj, b_name, REV_CMD_RIGHT, b_flags);\n+\tadd_pending_object(revs, a_obj, a_name);\n+\tadd_pending_object(revs, b_obj, b_name);\n+\treturn 0;\n+}\n+\n+static int handle_dotdot(const char *arg,\n+\t\t\t struct rev_info *revs, int flags,\n+\t\t\t int cant_be_filename)\n+{\n+\tchar *dotdot = strstr(arg, \"..\");\n+\tint ret;\n+\n+\tif (!dotdot)\n+\t\treturn -1;\n+\n+\t*dotdot = '\\0';\n+\tret = handle_dotdot_1(arg, dotdot, revs, flags, cant_be_filename);\n+\t*dotdot = '.';\n+\n+\treturn ret;\n+}\n+\n int handle_revision_arg(const char *arg_, struct rev_info *revs, int flags, unsigned revarg_opt)\n {\n \tstruct object_context oc;\n-\tchar *dotdot;\n \tchar *mark;\n \tstruct object *object;\n \tunsigned char sha1[20];\n@@ -1451,80 +1550,8 @@ int handle_revision_arg(const char *arg_, struct rev_info *revs, int flags, unsi\n \t\treturn -1;\n \t}\n \n-\tdotdot = strstr(arg, \"..\");\n-\tif (dotdot) {\n-\t\tunsigned char from_sha1[20];\n-\t\tconst char *next = dotdot + 2;\n-\t\tconst char *this = arg;\n-\t\tint symmetric = *next == '.';\n-\t\tunsigned int flags_exclude = flags ^ (UNINTERESTING | BOTTOM);\n-\t\tunsigned int a_flags;\n-\n-\t\t*dotdot = 0;\n-\t\tnext += symmetric;\n-\n-\t\tif (!*next)\n-\t\t\tnext = \"HEAD\";\n-\t\tif (dotdot == arg)\n-\t\t\tthis = \"HEAD\";\n-\t\tif (!get_sha1_committish(this, from_sha1) &&\n-\t\t !get_sha1_committish(next, sha1)) {\n-\t\t\tstruct object *a_obj, *b_obj;\n-\n-\t\t\tif (!cant_be_filename) {\n-\t\t\t\t*dotdot = '.';\n-\t\t\t\tverify_non_filename(revs->prefix, arg);\n-\t\t\t\t*dotdot = '\\0';\n-\t\t\t}\n-\n-\t\t\ta_obj = parse_object(from_sha1);\n-\t\t\tb_obj = parse_object(sha1);\n-\t\t\tif (!a_obj || !b_obj) {\n-\t\t\tmissing:\n-\t\t\t\t*dotdot = '.';\n-\t\t\t\tif (revs->ignore_missing)\n-\t\t\t\t\treturn 0;\n-\t\t\t\tdie(symmetric\n-\t\t\t\t ? \"Invalid symmetric difference expression %s\"\n-\t\t\t\t : \"Invalid revision range %s\", arg);\n-\t\t\t}\n-\n-\t\t\tif (!symmetric) {\n-\t\t\t\t/* just A..B */\n-\t\t\t\ta_flags = flags_exclude;\n-\t\t\t} else {\n-\t\t\t\t/* A...B -- find merge bases between the two */\n-\t\t\t\tstruct commit *a, *b;\n-\t\t\t\tstruct commit_list *exclude;\n-\n-\t\t\t\ta = lookup_commit_reference(a_obj->oid.hash);\n-\t\t\t\tb = lookup_commit_reference(b_obj->oid.hash);\n-\t\t\t\tif (!a || !b)\n-\t\t\t\t\tgoto missing;\n-\t\t\t\texclude = get_merge_bases(a, b);\n-\t\t\t\tadd_rev_cmdline_list(revs, exclude,\n-\t\t\t\t\t\t REV_CMD_MERGE_BASE,\n-\t\t\t\t\t\t flags_exclude);\n-\t\t\t\tadd_pending_commit_list(revs, exclude,\n-\t\t\t\t\t\t\tflags_exclude);\n-\t\t\t\tfree_commit_list(exclude);\n-\n-\t\t\t\ta_flags = flags | SYMMETRIC_LEFT;\n-\t\t\t}\n-\n-\t\t\ta_obj->flags |= a_flags;\n-\t\t\tb_obj->flags |= flags;\n-\t\t\tadd_rev_cmdline(revs, a_obj, this,\n-\t\t\t\t\tREV_CMD_LEFT, a_flags);\n-\t\t\tadd_rev_cmdline(revs, b_obj, next,\n-\t\t\t\t\tREV_CMD_RIGHT, flags);\n-\t\t\tadd_pending_object(revs, a_obj, this);\n-\t\t\tadd_pending_object(revs, b_obj, next);\n-\t\t\t*dotdot = '.';\n-\t\t\treturn 0;\n-\t\t}\n-\t\t*dotdot = '.';\n-\t}\n+\tif (!handle_dotdot(arg, revs, flags, revarg_opt))\n+\t\treturn 0;\n \n \tmark = strstr(arg, \"^@\");\n \tif (mark && !mark[2]) {\n-- \n2.13.0.219.g63f6bc368\n\n"},{"id":"320261","messageId":"20170519125206.z4uhvstoij6l2zi4@sigill.intra.peff.net","threadId":"45968","inReplyTo":"20170519124651.4q7waz75rmzfopgn@sigill.intra.peff.net","subject":"[PATCH 06/15] sha1_name: consistently refer to object_context as \"oc\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-05-19T12:52:06Z","receivedAt":"2017-05-19T12:52:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"An early version of the patch to add object_context used the\nname object_resolve_context. This was later shortened to\njust object_context, but the \"orc\" variable name stuck in a\nfew places. Let's use \"oc\", which is used elsewhere in the\ncode.\n\nSigned-off-by: Jeff King \n---\n cache.h | 2 +-\n sha1_name.c | 4 ++--\n 2 files changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex 188811920..656341b8e 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1363,7 +1363,7 @@ extern int get_sha1_tree(const char *str, unsigned char *sha1);\n extern int get_sha1_treeish(const char *str, unsigned char *sha1);\n extern int get_sha1_blob(const char *str, unsigned char *sha1);\n extern void maybe_die_on_misspelt_object_name(const char *name, const char *prefix);\n-extern int get_sha1_with_context(const char *str, unsigned flags, unsigned char *sha1, struct object_context *orc);\n+extern int get_sha1_with_context(const char *str, unsigned flags, unsigned char *sha1, struct object_context *oc);\n \n extern int get_oid(const char *str, struct object_id *oid);\n \ndiff --git a/sha1_name.c b/sha1_name.c\nindex 35c1e2a9e..a11d08dd8 100644\n--- a/sha1_name.c\n+++ b/sha1_name.c\n@@ -1638,9 +1638,9 @@ void maybe_die_on_misspelt_object_name(const char *name, const char *prefix)\n \tget_sha1_with_context_1(name, GET_SHA1_ONLY_TO_DIE, prefix, sha1, &oc);\n }\n \n-int get_sha1_with_context(const char *str, unsigned flags, unsigned char *sha1, struct object_context *orc)\n+int get_sha1_with_context(const char *str, unsigned flags, unsigned char *sha1, struct object_context *oc)\n {\n \tif (flags & GET_SHA1_FOLLOW_SYMLINKS && flags & GET_SHA1_ONLY_TO_DIE)\n \t\tdie(\"BUG: incompatible flags for get_sha1_with_context\");\n-\treturn get_sha1_with_context_1(str, flags, NULL, sha1, orc);\n+\treturn get_sha1_with_context_1(str, flags, NULL, sha1, oc);\n }\n-- \n2.13.0.219.g63f6bc368\n\n"},{"id":"320262","messageId":"20170519125224.gnfifzcyly2n5te6@sigill.intra.peff.net","threadId":"45968","inReplyTo":"20170519124651.4q7waz75rmzfopgn@sigill.intra.peff.net","subject":"[PATCH 07/15] get_sha1_with_context: always initialize oc->symlink_path","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-05-19T12:52:25Z","receivedAt":"2017-05-19T12:52:31Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The get_sha1_with_context() function zeroes out the\noc->symlink_path strbuf, but doesn't use strbuf_init() to\nset up the usual invariants (like pointing to the slopbuf).\nWe don't actually write to the oc->symlink_path strbuf\nunless we call get_tree_entry_follow_symlinks(), and that\nfunction does initialize it. However, readers may still look\nat the zero'd strbuf.\n\nIn practice this isn't a triggerable bug. The only caller\nthat looks at it only does so when the mode we found is 0.\nThis doesn't happen for non-tree-entries (where we return\nS_IFINVALID). A broken tree entry could have a mode of 0,\nbut canon_mode() quietly rewrites that into S_IFGITLINK.\nSo the \"0\" mode should only come up when we did indeed find\na symlink.\n\nThis is mostly just an accident of how the code happens to\nwork, though. Let's future-proof ourselves to make sure the\nstrbuf is properly initialized for all calls (it's only a\nfew struct member assignments, not a heap allocation).\n\nSigned-off-by: Jeff King \n---\n sha1_name.c | 1 +\n tree-walk.c | 1 -\n 2 files changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/sha1_name.c b/sha1_name.c\nindex a11d08dd8..35b16efc6 100644\n--- a/sha1_name.c\n+++ b/sha1_name.c\n@@ -1511,6 +1511,7 @@ static int get_sha1_with_context_1(const char *name,\n \n \tmemset(oc, 0, sizeof(*oc));\n \toc->mode = S_IFINVALID;\n+\tstrbuf_init(&oc->symlink_path, 0);\n \tret = get_sha1_1(name, namelen, sha1, flags);\n \tif (!ret)\n \t\treturn ret;\ndiff --git a/tree-walk.c b/tree-walk.c\nindex ff7760568..c7ecfc856 100644\n--- a/tree-walk.c\n+++ b/tree-walk.c\n@@ -589,7 +589,6 @@ enum follow_symlinks_result get_tree_entry_follow_symlinks(unsigned char *tree_s\n \tint i;\n \n \tinit_tree_desc(&t, NULL, 0UL);\n-\tstrbuf_init(result_path, 0);\n \tstrbuf_addstr(&namebuf, name);\n \thashcpy(current_tree_sha1, tree_sha1);\n \n-- \n2.13.0.219.g63f6bc368\n\n"},{"id":"320263","messageId":"20170519125443.7fknotkezbpfwdht@sigill.intra.peff.net","threadId":"45968","inReplyTo":"20170519124651.4q7waz75rmzfopgn@sigill.intra.peff.net","subject":"[PATCH 08/15] get_sha1_with_context: dynamically allocate oc->path","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-05-19T12:54:43Z","receivedAt":"2017-05-19T12:54:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When a sha1 lookup returns the tree path via \"struct\nobject_context\", it just copies it into a fixed-size buffer.\nThis means the result can be truncated, and it means our\n\"struct object_context\" consumes a lot of stack space.\n\nInstead, let's allocate a string on the heap. Because most\ncallers don't care about this information, we'll avoid doing\nit by default (so they don't all have to start calling\nfree() on the result). There are basically two options for\nthe caller to signal to us that it's interested:\n\n 1. By setting a pointer to storage in the object_context.\n\n 2. By passing a flag in another parameter.\n\nDoing (1) would match the way that sha1_object_info_extended()\nworks. But it would mean that every caller would have to\ninitialize the object_context, which they don't currently\nhave to do.\n\nThis patch does (2), and adds a new bit to the function's\nflags field. All of the callers that look at the \"path\"\nfield are updated to pass the new flag.\n\nSigned-off-by: Jeff King \n---\nIf there's a topic in flight that adds a new call without the flag, note\nthat it will stop filling the \"path\" field (which is what most calls\nwould want). And you won't get a compile error, because the pointer can\nbe used in largely the same way as the array.\n\nI find it unlikely that there's a case that would care, but if we wanted\nto be really paranoid, we could change the name of oc->path (at the cost\nof making this diff much noisier).\n\n builtin/cat-file.c | 4 +++-\n builtin/grep.c | 4 +++-\n builtin/log.c | 10 +++++++---\n cache.h | 8 +++++++-\n sha1_name.c | 6 ++++--\n 5 files changed, 24 insertions(+), 8 deletions(-)\n\ndiff --git a/builtin/cat-file.c b/builtin/cat-file.c\nindex 1890d7a63..421709517 100644\n--- a/builtin/cat-file.c\n+++ b/builtin/cat-file.c\n@@ -61,7 +61,8 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name,\n \tif (unknown_type)\n \t\tflags |= LOOKUP_UNKNOWN_OBJECT;\n \n-\tif (get_sha1_with_context(obj_name, 0, oid.hash, &obj_context))\n+\tif (get_sha1_with_context(obj_name, GET_SHA1_RECORD_PATH,\n+\t\t\t\t oid.hash, &obj_context))\n \t\tdie(\"Not a valid object name %s\", obj_name);\n \n \tif (!path)\n@@ -165,6 +166,7 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name,\n \t\tdie(\"git cat-file %s: bad file\", obj_name);\n \n \twrite_or_die(1, buf, size);\n+\tfree(obj_context.path);\n \treturn 0;\n }\n \ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex 3ffb5b4e8..254c1c784 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -1190,7 +1190,8 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n \t\t\tbreak;\n \t\t}\n \n-\t\tif (get_sha1_with_context(arg, 0, oid.hash, &oc)) {\n+\t\tif (get_sha1_with_context(arg, GET_SHA1_RECORD_PATH,\n+\t\t\t\t\t oid.hash, &oc)) {\n \t\t\tif (seen_dashdash)\n \t\t\t\tdie(_(\"unable to resolve revision: %s\"), arg);\n \t\t\tbreak;\n@@ -1200,6 +1201,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n \t\tif (!seen_dashdash)\n \t\t\tverify_non_filename(prefix, arg);\n \t\tadd_object_array_with_path(object, arg, &list, oc.mode, oc.path);\n+\t\tfree(oc.path);\n \t}\n \n \t/*\ndiff --git a/builtin/log.c b/builtin/log.c\nindex fd3d10ec2..a0d5233dc 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -483,16 +483,20 @@ static int show_blob_object(const struct object_id *oid, struct rev_info *rev, c\n \t !DIFF_OPT_TST(&rev->diffopt, ALLOW_TEXTCONV))\n \t\treturn stream_blob_to_fd(1, oid, NULL, 0);\n \n-\tif (get_sha1_with_context(obj_name, 0, oidc.hash, &obj_context))\n+\tif (get_sha1_with_context(obj_name, GET_SHA1_RECORD_PATH,\n+\t\t\t\t oidc.hash, &obj_context))\n \t\tdie(_(\"Not a valid object name %s\"), obj_name);\n-\tif (!obj_context.path[0] ||\n-\t !textconv_object(obj_context.path, obj_context.mode, &oidc, 1, &buf, &size))\n+\tif (!obj_context.path ||\n+\t !textconv_object(obj_context.path, obj_context.mode, &oidc, 1, &buf, &size)) {\n+\t\tfree(obj_context.path);\n \t\treturn stream_blob_to_fd(1, oid, NULL, 0);\n+\t}\n \n \tif (!buf)\n \t\tdie(_(\"git show %s: bad file\"), obj_name);\n \n \twrite_or_die(1, buf, size);\n+\tfree(obj_context.path);\n \treturn 0;\n }\n \ndiff --git a/cache.h b/cache.h\nindex 656341b8e..e219c45ed 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1333,13 +1333,18 @@ static inline int hex2chr(const char *s)\n \n struct object_context {\n \tunsigned char tree[20];\n-\tchar path[PATH_MAX];\n \tunsigned mode;\n \t/*\n \t * symlink_path is only used by get_tree_entry_follow_symlinks,\n \t * and only for symlinks that point outside the repository.\n \t */\n \tstruct strbuf symlink_path;\n+\t/*\n+\t * If GET_SHA1_RECORD_PATH is set, this will record path (if any)\n+\t * found when resolving the name. The caller is responsible for\n+\t * releasing the memory.\n+\t */\n+\tchar *path;\n };\n \n #define GET_SHA1_QUIETLY 01\n@@ -1349,6 +1354,7 @@ struct object_context {\n #define GET_SHA1_TREEISH 020\n #define GET_SHA1_BLOB 040\n #define GET_SHA1_FOLLOW_SYMLINKS 0100\n+#define GET_SHA1_RECORD_PATH 0200\n #define GET_SHA1_ONLY_TO_DIE 04000\n \n #define GET_SHA1_DISAMBIGUATORS \\\ndiff --git a/sha1_name.c b/sha1_name.c\nindex 35b16efc6..70f096749 100644\n--- a/sha1_name.c\n+++ b/sha1_name.c\n@@ -1550,7 +1550,8 @@ static int get_sha1_with_context_1(const char *name,\n \t\t\tnamelen = strlen(cp);\n \t\t}\n \n-\t\tstrlcpy(oc->path, cp, sizeof(oc->path));\n+\t\tif (flags & GET_SHA1_RECORD_PATH)\n+\t\t\toc->path = xstrdup(cp);\n \n \t\tif (!active_cache)\n \t\t\tread_cache();\n@@ -1613,7 +1614,8 @@ static int get_sha1_with_context_1(const char *name,\n \t\t\t\t}\n \t\t\t}\n \t\t\thashcpy(oc->tree, tree_sha1);\n-\t\t\tstrlcpy(oc->path, filename, sizeof(oc->path));\n+\t\t\tif (flags & GET_SHA1_RECORD_PATH)\n+\t\t\t\toc->path = xstrdup(filename);\n \n \t\t\tfree(new_filename);\n \t\t\treturn ret;\n-- \n2.13.0.219.g63f6bc368\n\n"},{"id":"320264","messageId":"20170519125456.j3xnwazaahtogwni@sigill.intra.peff.net","threadId":"45968","inReplyTo":"20170519124651.4q7waz75rmzfopgn@sigill.intra.peff.net","subject":"[PATCH 09/15] t4063: add tests of direct blob diffs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-05-19T12:54:56Z","receivedAt":"2017-05-19T12:55:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The git-diff command can directly compare two blobs (or a\nblob and a file), but we don't test this at all. Let's add\nsome basic tests that reveal a few problems.\n\nThere are basically four interesting inputs:\n\n 1. sha1 against sha1 (where diff has no information beyond\n the contents)\n\n 2. tree:path against tree:path (where it can get\n information via get_sha1_with_context)\n\n 3. Same as (2), but using the \"..\" range syntax\n\n 4. tree:path against a filename\n\nAnd beyond generating a sane diff, we care about a few\nlittle bits: which paths they show in the diff header, and\nwhether they correctly pick up a mode change.\n\nThey should all be able to show a mode except for (1),\nthough note that case (3) is currently broken.\n\nFor the headers, we would ideally show the path within the\ntree if we have it, making:\n\n git diff a:path b:path\n\nlook the same as:\n\n git diff a b -- path\n\nWe can't always do that (e.g., in the direct sha1/sha1 diff,\nwe have no path to show), in which case we should fall back\nto the name that resolved to the blob (which is nonsense\nfrom the repository's perspective, but is the best we can\ndo).\n\nAside from the fallback case in (1), none of the cases get\nthis right. Cases (2) and (3) always show the full\ntree:path, even though we should be able to know just the\n\"path\" portion.\n\nCase (4) picks up the filename path, but assigns it to\n_both_ sides of the diff. So this works for:\n\n git diff tree:path path\n\nbut not for:\n\n git diff tree:other_path path\n\nThe appropriate tests are marked to expect failure.\n\nSigned-off-by: Jeff King \n---\n t/t4063-diff-blobs.sh | 91 +++++++++++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 91 insertions(+)\n create mode 100755 t/t4063-diff-blobs.sh\n\ndiff --git a/t/t4063-diff-blobs.sh b/t/t4063-diff-blobs.sh\nnew file mode 100755\nindex 000000000..90c6f6b85\n--- /dev/null\n+++ b/t/t4063-diff-blobs.sh\n@@ -0,0 +1,91 @@\n+#!/bin/sh\n+\n+test_description='test direct comparison of blobs via git-diff'\n+. ./test-lib.sh\n+\n+run_diff () {\n+\t# use full-index to make it easy to match the index line\n+\tgit diff --full-index \"$@\" >diff\n+}\n+\n+check_index () {\n+\tgrep \"^index $1\\\\.\\\\.$2\" diff\n+}\n+\n+check_mode () {\n+\tgrep \"^old mode $1\" diff &&\n+\tgrep \"^new mode $2\" diff\n+}\n+\n+check_paths () {\n+\tgrep \"^diff --git a/$1 b/$2\" diff\n+}\n+\n+test_expect_success 'create some blobs' '\n+\techo one >one &&\n+\techo two >two &&\n+\tchmod +x two &&\n+\tgit add . &&\n+\n+\t# cover systems where modes are ignored\n+\tgit update-index --chmod=+x two &&\n+\n+\tgit commit -m base &&\n+\n+\tsha1_one=$(git rev-parse HEAD:one) &&\n+\tsha1_two=$(git rev-parse HEAD:two)\n+'\n+\n+test_expect_success 'diff by sha1' '\n+\trun_diff $sha1_one $sha1_two\n+'\n+test_expect_success 'index of sha1 diff' '\n+\tcheck_index $sha1_one $sha1_two\n+'\n+test_expect_success 'sha1 diff uses arguments as paths' '\n+\tcheck_paths $sha1_one $sha1_two\n+'\n+test_expect_success 'sha1 diff has no mode change' '\n+\t! grep mode diff\n+'\n+\n+test_expect_success 'diff by tree:path (run)' '\n+\trun_diff HEAD:one HEAD:two\n+'\n+test_expect_success 'index of tree:path diff' '\n+\tcheck_index $sha1_one $sha1_two\n+'\n+test_expect_failure 'tree:path diff uses filenames as paths' '\n+\tcheck_paths one two\n+'\n+test_expect_success 'tree:path diff shows mode change' '\n+\tcheck_mode 100644 100755\n+'\n+\n+test_expect_success 'diff by ranged tree:path' '\n+\trun_diff HEAD:one..HEAD:two\n+'\n+test_expect_success 'index of ranged tree:path diff' '\n+\tcheck_index $sha1_one $sha1_two\n+'\n+test_expect_failure 'ranged tree:path diff uses filenames as paths' '\n+\tcheck_paths one two\n+'\n+test_expect_failure 'ranged tree:path diff shows mode change' '\n+\tcheck_mode 100644 100755\n+'\n+\n+test_expect_success 'diff blob against file' '\n+\trun_diff HEAD:one two\n+'\n+test_expect_success 'index of blob-file diff' '\n+\tcheck_index $sha1_one $sha1_two\n+'\n+test_expect_failure 'blob-file diff uses filename as paths' '\n+\tcheck_paths one two\n+'\n+test_expect_success FILEMODE 'blob-file diff shows mode change' '\n+\tcheck_mode 100644 100755\n+'\n+\n+test_done\n-- \n2.13.0.219.g63f6bc368\n\n"},{"id":"320265","messageId":"20170519125511.uz7hsdalem57n77d@sigill.intra.peff.net","threadId":"45968","inReplyTo":"20170519124651.4q7waz75rmzfopgn@sigill.intra.peff.net","subject":"[PATCH 10/15] handle_revision_arg: record modes for \"a..b\" endpoints","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-05-19T12:55:11Z","receivedAt":"2017-05-19T12:55:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The \"a..b\" revision syntax was designed to handle commits,\nso it doesn't bother to record any mode we find while\ntraversing a \"tree:path\" endpoint. These days \"git diff\" can\ndiff blobs using either \"a:path..b:path\" (with dots) or\n\"a:path b:path\" (without), but the two behave\ninconsistently, as the with-dots version fails to notice the\nmode.\n\nLet's teach the dot-dot range parser to record modes; it\ndoesn't cost us anything, and it makes this case work.\n\nSigned-off-by: Jeff King \n---\n revision.c | 10 ++++++----\n t/t4063-diff-blobs.sh | 2 +-\n 2 files changed, 7 insertions(+), 5 deletions(-)\n\ndiff --git a/revision.c b/revision.c\nindex eb45501fd..96427e3c2 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -1448,9 +1448,11 @@ static int handle_dotdot_1(const char *arg, char *dotdot,\n \tconst char *a_name, *b_name;\n \tstruct object_id a_oid, b_oid;\n \tstruct object *a_obj, *b_obj;\n+\tstruct object_context a_oc, b_oc;\n \tunsigned int a_flags, b_flags;\n \tint symmetric = 0;\n \tunsigned int flags_exclude = flags ^ (UNINTERESTING | BOTTOM);\n+\tunsigned int oc_flags = GET_SHA1_COMMITTISH;\n \n \ta_name = arg;\n \tif (!*a_name)\n@@ -1464,8 +1466,8 @@ static int handle_dotdot_1(const char *arg, char *dotdot,\n \tif (!*b_name)\n \t\tb_name = \"HEAD\";\n \n-\tif (get_sha1_committish(a_name, a_oid.hash) ||\n-\t get_sha1_committish(b_name, b_oid.hash))\n+\tif (get_sha1_with_context(a_name, oc_flags, a_oid.hash, &a_oc) ||\n+\t get_sha1_with_context(b_name, oc_flags, b_oid.hash, &b_oc))\n \t\treturn -1;\n \n \tif (!cant_be_filename) {\n@@ -1507,8 +1509,8 @@ static int handle_dotdot_1(const char *arg, char *dotdot,\n \tb_obj->flags |= b_flags;\n \tadd_rev_cmdline(revs, a_obj, a_name, REV_CMD_LEFT, a_flags);\n \tadd_rev_cmdline(revs, b_obj, b_name, REV_CMD_RIGHT, b_flags);\n-\tadd_pending_object(revs, a_obj, a_name);\n-\tadd_pending_object(revs, b_obj, b_name);\n+\tadd_pending_object_with_mode(revs, a_obj, a_name, a_oc.mode);\n+\tadd_pending_object_with_mode(revs, b_obj, b_name, b_oc.mode);\n \treturn 0;\n }\n \ndiff --git a/t/t4063-diff-blobs.sh b/t/t4063-diff-blobs.sh\nindex 90c6f6b85..df9c35b2d 100755\n--- a/t/t4063-diff-blobs.sh\n+++ b/t/t4063-diff-blobs.sh\n@@ -71,7 +71,7 @@ test_expect_success 'index of ranged tree:path diff' '\n test_expect_failure 'ranged tree:path diff uses filenames as paths' '\n \tcheck_paths one two\n '\n-test_expect_failure 'ranged tree:path diff shows mode change' '\n+test_expect_success 'ranged tree:path diff shows mode change' '\n \tcheck_mode 100644 100755\n '\n \n-- \n2.13.0.219.g63f6bc368\n\n"},{"id":"320266","messageId":"20170519125526.dxezhtsuib267byg@sigill.intra.peff.net","threadId":"45968","inReplyTo":"20170519124651.4q7waz75rmzfopgn@sigill.intra.peff.net","subject":"[PATCH 11/15] handle_revision_arg: record paths for pending objects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-05-19T12:55:26Z","receivedAt":"2017-05-19T12:55:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"If the revision parser sees an argument like tree:path, we\nparse it down to the correct blob (or tree), but throw away\nthe \"path\" portion. Let's ask get_sha1_with_context() to\nrecord it, and pass it along in the pending array.\n\nThis will let programs like git-diff which rely on the\nrevision-parser show more accurate paths.\n\nNote that the implementation is a little tricky; we have to\nmake sure we free oc.path in all code paths. For handle_dotdot(),\nwe can piggy-back on the existing cleanup-wrapper pattern.\nThe real work happens in handle_dotdot_1(), but the\nhandle_dotdot() wrapper makes sure that the path is freed no\nmatter how we exit the function (and for that reason we make\nsure that the object_context struct is zero'd, so if we fail\nto even get to the get_sha1_with_context() call, we just end\nup calling free(NULL)).\n\nSigned-off-by: Jeff King \n---\n revision.c | 32 +++++++++++++++++++++-----------\n 1 file changed, 21 insertions(+), 11 deletions(-)\n\ndiff --git a/revision.c b/revision.c\nindex 96427e3c2..a7b93dcc8 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -1443,16 +1443,17 @@ static int dotdot_missing(const char *arg, char *dotdot,\n \n static int handle_dotdot_1(const char *arg, char *dotdot,\n \t\t\t struct rev_info *revs, int flags,\n-\t\t\t int cant_be_filename)\n+\t\t\t int cant_be_filename,\n+\t\t\t struct object_context *a_oc,\n+\t\t\t struct object_context *b_oc)\n {\n \tconst char *a_name, *b_name;\n \tstruct object_id a_oid, b_oid;\n \tstruct object *a_obj, *b_obj;\n-\tstruct object_context a_oc, b_oc;\n \tunsigned int a_flags, b_flags;\n \tint symmetric = 0;\n \tunsigned int flags_exclude = flags ^ (UNINTERESTING | BOTTOM);\n-\tunsigned int oc_flags = GET_SHA1_COMMITTISH;\n+\tunsigned int oc_flags = GET_SHA1_COMMITTISH | GET_SHA1_RECORD_PATH;\n \n \ta_name = arg;\n \tif (!*a_name)\n@@ -1466,8 +1467,8 @@ static int handle_dotdot_1(const char *arg, char *dotdot,\n \tif (!*b_name)\n \t\tb_name = \"HEAD\";\n \n-\tif (get_sha1_with_context(a_name, oc_flags, a_oid.hash, &a_oc) ||\n-\t get_sha1_with_context(b_name, oc_flags, b_oid.hash, &b_oc))\n+\tif (get_sha1_with_context(a_name, oc_flags, a_oid.hash, a_oc) ||\n+\t get_sha1_with_context(b_name, oc_flags, b_oid.hash, b_oc))\n \t\treturn -1;\n \n \tif (!cant_be_filename) {\n@@ -1509,8 +1510,8 @@ static int handle_dotdot_1(const char *arg, char *dotdot,\n \tb_obj->flags |= b_flags;\n \tadd_rev_cmdline(revs, a_obj, a_name, REV_CMD_LEFT, a_flags);\n \tadd_rev_cmdline(revs, b_obj, b_name, REV_CMD_RIGHT, b_flags);\n-\tadd_pending_object_with_mode(revs, a_obj, a_name, a_oc.mode);\n-\tadd_pending_object_with_mode(revs, b_obj, b_name, b_oc.mode);\n+\tadd_pending_object_with_path(revs, a_obj, a_name, a_oc->mode, a_oc->path);\n+\tadd_pending_object_with_path(revs, b_obj, b_name, b_oc->mode, b_oc->path);\n \treturn 0;\n }\n \n@@ -1518,16 +1519,24 @@ static int handle_dotdot(const char *arg,\n \t\t\t struct rev_info *revs, int flags,\n \t\t\t int cant_be_filename)\n {\n+\tstruct object_context a_oc, b_oc;\n \tchar *dotdot = strstr(arg, \"..\");\n \tint ret;\n \n \tif (!dotdot)\n \t\treturn -1;\n \n+\tmemset(&a_oc, 0, sizeof(a_oc));\n+\tmemset(&b_oc, 0, sizeof(b_oc));\n+\n \t*dotdot = '\\0';\n-\tret = handle_dotdot_1(arg, dotdot, revs, flags, cant_be_filename);\n+\tret = handle_dotdot_1(arg, dotdot, revs, flags, cant_be_filename,\n+\t\t\t &a_oc, &b_oc);\n \t*dotdot = '.';\n \n+\tfree(a_oc.path);\n+\tfree(b_oc.path);\n+\n \treturn ret;\n }\n \n@@ -1540,7 +1549,7 @@ int handle_revision_arg(const char *arg_, struct rev_info *revs, int flags, unsi\n \tint local_flags;\n \tconst char *arg = arg_;\n \tint cant_be_filename = revarg_opt & REVARG_CANNOT_BE_FILENAME;\n-\tunsigned get_sha1_flags = 0;\n+\tunsigned get_sha1_flags = GET_SHA1_RECORD_PATH;\n \n \tflags = flags & UNINTERESTING ? flags | BOTTOM : flags & ~BOTTOM;\n \n@@ -1591,7 +1600,7 @@ int handle_revision_arg(const char *arg_, struct rev_info *revs, int flags, unsi\n \t}\n \n \tif (revarg_opt & REVARG_COMMITTISH)\n-\t\tget_sha1_flags = GET_SHA1_COMMITTISH;\n+\t\tget_sha1_flags |= GET_SHA1_COMMITTISH;\n \n \tif (get_sha1_with_context(arg, get_sha1_flags, sha1, &oc))\n \t\treturn revs->ignore_missing ? 0 : -1;\n@@ -1599,7 +1608,8 @@ int handle_revision_arg(const char *arg_, struct rev_info *revs, int flags, unsi\n \t\tverify_non_filename(revs->prefix, arg);\n \tobject = get_reference(revs, arg, sha1, flags ^ local_flags);\n \tadd_rev_cmdline(revs, object, arg_, REV_CMD_REV, flags ^ local_flags);\n-\tadd_pending_object_with_mode(revs, object, arg, oc.mode);\n+\tadd_pending_object_with_path(revs, object, arg, oc.mode, oc.path);\n+\tfree(oc.path);\n \treturn 0;\n }\n \n-- \n2.13.0.219.g63f6bc368\n\n"},{"id":"320267","messageId":"20170519125730.yakprrok7nom3kjh@sigill.intra.peff.net","threadId":"45968","inReplyTo":"20170519124651.4q7waz75rmzfopgn@sigill.intra.peff.net","subject":"[PATCH 12/15] diff: pass whole pending entry in blobinfo","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-05-19T12:57:30Z","receivedAt":"2017-05-19T12:57:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When diffing blobs directly, git-diff picks the blobs out of\nthe rev_info's pending array and copies the relevant bits to\na custom \"struct blobinfo\". But the pending array entry\nalready has all of this information (and more, which we'll\nuse in future patches). Let's just pass the original entry\ninstead.\n\nIn practice, these two blobs are probably adjacent in the\nrevs->pending array, and we could just pass the whole array.\nBut the current code is careful to pick each blob out\nseparately and put it into another array, so we'll continue\nto do so and make our own array-of-pointers.\n\nSigned-off-by: Jeff King \n---\nI looked into just passing \"blob_a\" and \"blob_b\", instead of passing an\narray and letting the called functions decide how many elements\nshould be in it. But I don't know if we would ever want to handle\narbitrary numbers of blobs (e.g., n-way combined diffs). It would be a\nstep backwards in that case, so I decided not to.\n\n builtin/diff.c | 38 +++++++++++++++-----------------------\n 1 file changed, 15 insertions(+), 23 deletions(-)\n\ndiff --git a/builtin/diff.c b/builtin/diff.c\nindex d184aafab..8b276ae57 100644\n--- a/builtin/diff.c\n+++ b/builtin/diff.c\n@@ -20,12 +20,6 @@\n #define DIFF_NO_INDEX_EXPLICIT 1\n #define DIFF_NO_INDEX_IMPLICIT 2\n \n-struct blobinfo {\n-\tstruct object_id oid;\n-\tconst char *name;\n-\tunsigned mode;\n-};\n-\n static const char builtin_diff_usage[] =\n \"git diff [] [ []] [--] [...]\";\n \n@@ -65,7 +59,7 @@ static void stuff_change(struct diff_options *opt,\n \n static int builtin_diff_b_f(struct rev_info *revs,\n \t\t\t int argc, const char **argv,\n-\t\t\t struct blobinfo *blob)\n+\t\t\t struct object_array_entry **blob)\n {\n \t/* Blob vs file in the working tree*/\n \tstruct stat st;\n@@ -84,12 +78,12 @@ static int builtin_diff_b_f(struct rev_info *revs,\n \n \tdiff_set_mnemonic_prefix(&revs->diffopt, \"o/\", \"w/\");\n \n-\tif (blob[0].mode == S_IFINVALID)\n-\t\tblob[0].mode = canon_mode(st.st_mode);\n+\tif (blob[0]->mode == S_IFINVALID)\n+\t\tblob[0]->mode = canon_mode(st.st_mode);\n \n \tstuff_change(&revs->diffopt,\n-\t\t blob[0].mode, canon_mode(st.st_mode),\n-\t\t &blob[0].oid, &null_oid,\n+\t\t blob[0]->mode, canon_mode(st.st_mode),\n+\t\t &blob[0]->item->oid, &null_oid,\n \t\t 1, 0,\n \t\t path, path);\n \tdiffcore_std(&revs->diffopt);\n@@ -99,24 +93,24 @@ static int builtin_diff_b_f(struct rev_info *revs,\n \n static int builtin_diff_blobs(struct rev_info *revs,\n \t\t\t int argc, const char **argv,\n-\t\t\t struct blobinfo *blob)\n+\t\t\t struct object_array_entry **blob)\n {\n \tunsigned mode = canon_mode(S_IFREG | 0644);\n \n \tif (argc > 1)\n \t\tusage(builtin_diff_usage);\n \n-\tif (blob[0].mode == S_IFINVALID)\n-\t\tblob[0].mode = mode;\n+\tif (blob[0]->mode == S_IFINVALID)\n+\t\tblob[0]->mode = mode;\n \n-\tif (blob[1].mode == S_IFINVALID)\n-\t\tblob[1].mode = mode;\n+\tif (blob[1]->mode == S_IFINVALID)\n+\t\tblob[1]->mode = mode;\n \n \tstuff_change(&revs->diffopt,\n-\t\t blob[0].mode, blob[1].mode,\n-\t\t &blob[0].oid, &blob[1].oid,\n+\t\t blob[0]->mode, blob[1]->mode,\n+\t\t &blob[0]->item->oid, &blob[1]->item->oid,\n \t\t 1, 1,\n-\t\t blob[0].name, blob[1].name);\n+\t\t blob[0]->name, blob[1]->name);\n \tdiffcore_std(&revs->diffopt);\n \tdiff_flush(&revs->diffopt);\n \treturn 0;\n@@ -259,7 +253,7 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n \tstruct rev_info rev;\n \tstruct object_array ent = OBJECT_ARRAY_INIT;\n \tint blobs = 0, paths = 0;\n-\tstruct blobinfo blob[2];\n+\tstruct object_array_entry *blob[2];\n \tint nongit = 0, no_index = 0;\n \tint result = 0;\n \n@@ -408,9 +402,7 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n \t\t} else if (obj->type == OBJ_BLOB) {\n \t\t\tif (2 <= blobs)\n \t\t\t\tdie(_(\"more than two blobs given: '%s'\"), name);\n-\t\t\thashcpy(blob[blobs].oid.hash, obj->oid.hash);\n-\t\t\tblob[blobs].name = name;\n-\t\t\tblob[blobs].mode = entry->mode;\n+\t\t\tblob[blobs] = entry;\n \t\t\tblobs++;\n \n \t\t} else {\n-- \n2.13.0.219.g63f6bc368\n\n"},{"id":"320268","messageId":"20170519125804.c23a7ex6i4dfnwe6@sigill.intra.peff.net","threadId":"45968","inReplyTo":"20170519124651.4q7waz75rmzfopgn@sigill.intra.peff.net","subject":"[PATCH 13/15] diff: use the word \"path\" instead of \"name\" for blobs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-05-19T12:58:05Z","receivedAt":"2017-05-19T12:58:11Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The stuff_change() function makes diff_filespecs out of\nblobs. The term we generally use for filespecs is \"path\",\nnot \"name\", so let's be consistent here. That will make\nthings less confusing when the next patch starts caring\nabout the path/name distinction inside the pending object\narray.\n\nSigned-off-by: Jeff King \n---\n builtin/diff.c | 14 +++++++-------\n 1 file changed, 7 insertions(+), 7 deletions(-)\n\ndiff --git a/builtin/diff.c b/builtin/diff.c\nindex 8b276ae57..4c0811d6f 100644\n--- a/builtin/diff.c\n+++ b/builtin/diff.c\n@@ -29,8 +29,8 @@ static void stuff_change(struct diff_options *opt,\n \t\t\t const struct object_id *new_oid,\n \t\t\t int old_oid_valid,\n \t\t\t int new_oid_valid,\n-\t\t\t const char *old_name,\n-\t\t\t const char *new_name)\n+\t\t\t const char *old_path,\n+\t\t\t const char *new_path)\n {\n \tstruct diff_filespec *one, *two;\n \n@@ -41,16 +41,16 @@ static void stuff_change(struct diff_options *opt,\n \tif (DIFF_OPT_TST(opt, REVERSE_DIFF)) {\n \t\tSWAP(old_mode, new_mode);\n \t\tSWAP(old_oid, new_oid);\n-\t\tSWAP(old_name, new_name);\n+\t\tSWAP(old_path, new_path);\n \t}\n \n \tif (opt->prefix &&\n-\t (strncmp(old_name, opt->prefix, opt->prefix_length) ||\n-\t strncmp(new_name, opt->prefix, opt->prefix_length)))\n+\t (strncmp(old_path, opt->prefix, opt->prefix_length) ||\n+\t strncmp(new_path, opt->prefix, opt->prefix_length)))\n \t\treturn;\n \n-\tone = alloc_filespec(old_name);\n-\ttwo = alloc_filespec(new_name);\n+\tone = alloc_filespec(old_path);\n+\ttwo = alloc_filespec(new_path);\n \tfill_filespec(one, old_oid->hash, old_oid_valid, old_mode);\n \tfill_filespec(two, new_oid->hash, new_oid_valid, new_mode);\n \n-- \n2.13.0.219.g63f6bc368\n\n"},{"id":"320269","messageId":"20170519125915.eaw7zb3sjjs4xz3b@sigill.intra.peff.net","threadId":"45968","inReplyTo":"20170519124651.4q7waz75rmzfopgn@sigill.intra.peff.net","subject":"[PATCH 14/15] diff: use pending \"path\" if it is available","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-05-19T12:59:15Z","receivedAt":"2017-05-19T12:59:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"There's a subtle distinction between \"name\" and \"path\" for a\nblob that we resolve: the name is what the user told us on\nthe command line, and the path is what we traversed when\nfinding the blob within a tree (if we did so).\n\nWhen we diff blobs directly, we use \"name\", but \"path\" is\nmore likely to be useful to the user (it will find the\ncorrect .gitattributes, and give them a saner diff header).\n\nWe still have to fall back to using the name for some cases\n(i.e., any blob reference that isn't of the form tree:path).\nThat's the best we can do in such a case.\n\nSigned-off-by: Jeff King \n---\n builtin/diff.c | 7 ++++++-\n t/t4063-diff-blobs.sh | 4 ++--\n 2 files changed, 8 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/diff.c b/builtin/diff.c\nindex 4c0811d6f..1a1149eed 100644\n--- a/builtin/diff.c\n+++ b/builtin/diff.c\n@@ -23,6 +23,11 @@\n static const char builtin_diff_usage[] =\n \"git diff [] [ []] [--] [...]\";\n \n+static const char *blob_path(struct object_array_entry *entry)\n+{\n+\treturn entry->path ? entry->path : entry->name;\n+}\n+\n static void stuff_change(struct diff_options *opt,\n \t\t\t unsigned old_mode, unsigned new_mode,\n \t\t\t const struct object_id *old_oid,\n@@ -110,7 +115,7 @@ static int builtin_diff_blobs(struct rev_info *revs,\n \t\t blob[0]->mode, blob[1]->mode,\n \t\t &blob[0]->item->oid, &blob[1]->item->oid,\n \t\t 1, 1,\n-\t\t blob[0]->name, blob[1]->name);\n+\t\t blob_path(blob[0]), blob_path(blob[1]));\n \tdiffcore_std(&revs->diffopt);\n \tdiff_flush(&revs->diffopt);\n \treturn 0;\ndiff --git a/t/t4063-diff-blobs.sh b/t/t4063-diff-blobs.sh\nindex df9c35b2d..80ce033ab 100755\n--- a/t/t4063-diff-blobs.sh\n+++ b/t/t4063-diff-blobs.sh\n@@ -55,7 +55,7 @@ test_expect_success 'diff by tree:path (run)' '\n test_expect_success 'index of tree:path diff' '\n \tcheck_index $sha1_one $sha1_two\n '\n-test_expect_failure 'tree:path diff uses filenames as paths' '\n+test_expect_success 'tree:path diff uses filenames as paths' '\n \tcheck_paths one two\n '\n test_expect_success 'tree:path diff shows mode change' '\n@@ -68,7 +68,7 @@ test_expect_success 'diff by ranged tree:path' '\n test_expect_success 'index of ranged tree:path diff' '\n \tcheck_index $sha1_one $sha1_two\n '\n-test_expect_failure 'ranged tree:path diff uses filenames as paths' '\n+test_expect_success 'ranged tree:path diff uses filenames as paths' '\n \tcheck_paths one two\n '\n test_expect_success 'ranged tree:path diff shows mode change' '\n-- \n2.13.0.219.g63f6bc368\n\n"},{"id":"320270","messageId":"20170519125934.6kz7qukpmf43lcld@sigill.intra.peff.net","threadId":"45968","inReplyTo":"20170519124651.4q7waz75rmzfopgn@sigill.intra.peff.net","subject":"[PATCH 15/15] diff: use blob path for blob/file diffs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-05-19T12:59:34Z","receivedAt":"2017-05-19T12:59:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we diff a blob against a working tree file like:\n\n git diff HEAD:Makefile Makefile\n\nwe always use the working tree filename for both sides of\nthe diff. In most cases that's fine, as the two would be the\nsame anyway, as above. And until recently, we used the\n\"name\" for the blob, not the path, which would have the\nmessy \"HEAD:\" on the beginning.\n\nBut when they don't match, like:\n\n git diff HEAD:old_path new_path\n\nit makes sense to show both names.\n\nThis patch uses the blob's path field if it's available, and\notherwise falls back to using the filename (in preference to\nthe blob's name, which is likely to be garbage like a raw\nsha1).\n\nSigned-off-by: Jeff King \n---\n builtin/diff.c | 3 ++-\n t/t4063-diff-blobs.sh | 7 ++++++-\n 2 files changed, 8 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/diff.c b/builtin/diff.c\nindex 1a1149eed..5e7c6428c 100644\n--- a/builtin/diff.c\n+++ b/builtin/diff.c\n@@ -90,7 +90,8 @@ static int builtin_diff_b_f(struct rev_info *revs,\n \t\t blob[0]->mode, canon_mode(st.st_mode),\n \t\t &blob[0]->item->oid, &null_oid,\n \t\t 1, 0,\n-\t\t path, path);\n+\t\t blob[0]->path ? blob[0]->path : path,\n+\t\t path);\n \tdiffcore_std(&revs->diffopt);\n \tdiff_flush(&revs->diffopt);\n \treturn 0;\ndiff --git a/t/t4063-diff-blobs.sh b/t/t4063-diff-blobs.sh\nindex 80ce033ab..bc69e26c5 100755\n--- a/t/t4063-diff-blobs.sh\n+++ b/t/t4063-diff-blobs.sh\n@@ -81,11 +81,16 @@ test_expect_success 'diff blob against file' '\n test_expect_success 'index of blob-file diff' '\n \tcheck_index $sha1_one $sha1_two\n '\n-test_expect_failure 'blob-file diff uses filename as paths' '\n+test_expect_success 'blob-file diff uses filename as paths' '\n \tcheck_paths one two\n '\n test_expect_success FILEMODE 'blob-file diff shows mode change' '\n \tcheck_mode 100644 100755\n '\n \n+test_expect_success 'blob-file diff prefers filename to sha1' '\n+\trun_diff $sha1_one two &&\n+\tcheck_paths two two\n+'\n+\n test_done\n-- \n2.13.0.219.g63f6bc368\n"},{"id":"320315","messageId":"F66E608B6CE34520BD764C404A2AD36C@PhilipOakley","threadId":"45968","inReplyTo":"20170519124834.ipk42isngqck2egu@sigill.intra.peff.net","subject":"Re: [PATCH 01/15] handle_revision_arg: reset \"dotdot\" consistently","fromName":"Philip Oakley","fromEmail":"philipoakley@iee.org","sentAt":"2017-05-20T14:56:32Z","receivedAt":"2017-05-20T14:56:38Z","isPatch":true,"sender":{"key":"philipoakley@iee.email","avatar":"https://avatars.githubusercontent.com/u/914343?v=4"},"body":"From: \"Jeff King\" \n> When we are parsing a range like \"a..b\", we write a\n> temporary NUL over the first \".\", so that we can access the\n> names \"a\" and \"b\" as C strings. But our restoration of the\n> original \".\" is done at inconsistent times, which can lead\n> to confusing results.\n>\n> For most calls, we restore the \".\" after we resolve the\n> names, but before we call verify_non_filename(). This means\n> that when we later call add_pending_object(), the name for\n> the left-hand \"a\" has been re-expanded to \"a..b\". You can\n> see this with:\n>\n> git log --source a...b\n>\n> where \"b\" will be correctly marked with \"b\", but \"a\" will be\n> marked with \"a...b\". Likewise with \"a..b\" (though you need\n> to use --boundary to even see \"a\" at all in that case).\n>\n> To top off the confusion, when the REVARG_CANNOT_BE_FILENAME\n> flag is set, we skip the non-filename check, and leave the\n> NUL in place.\n>\n> That means we do report the correct name for \"a\" in the\n> pending array. But some code paths try to show the whole\n> \"a..b\" name in error messages, and these erroneously show\n> only \"a\" instead of \"a..b\". E.g.:\n>\n> $ git cherry-pick HEAD:foo..HEAD:foo\n\nshouldn't this be three dots? Also the para above uses two dot examples in \nits description but the paras before that start by describing the three dot \ncase.\n--\nPhilip\n\n> error: object d95f3ad14dee633a758d2e331151e950dd13e4ed is a blob, not a \n> commit\n> error: object d95f3ad14dee633a758d2e331151e950dd13e4ed is a blob, not a \n> commit\n> fatal: Invalid symmetric difference expression HEAD:foo\n>\n> (That last message should be \"HEAD:foo...HEAD:foo\"; I used\n> cherry-pick because it passes the CANNOT_BE_FILENAME flag).\n>\n> As an interesting side note, cherry-pick actually looks at\n> and re-resolves the arguments from the pending->name fields.\n> So it would have been visibly broken by the first bug, but\n> the effect was canceled out by the second one.\n>\n> This patch makes the whole function consistent by re-writing\n> the NUL immediately after calling verify_non_filename(), and\n> then restoring the \".\" as appropriate in some error-printing\n> and early-return code paths.\n>\n> Signed-off-by: Jeff King \n> ---\n> I also considered just making a copy of the string rather than this\n> in-place munging (technically we get it as a pointer-to-const; it's only\n> the use of strstr() that lets us quietly drop the const). But it doesn't\n> really make the code any cleaner; now instead of restoring the dot you\n> have to remember to free() the string in each code path.\n>\n> revision.c | 3 +++\n> t/t4202-log.sh | 9 +++++++++\n> 2 files changed, 12 insertions(+)\n>\n> diff --git a/revision.c b/revision.c\n> index 8a8c1789c..014bf52e3 100644\n> --- a/revision.c\n> +++ b/revision.c\n> @@ -1477,12 +1477,14 @@ int handle_revision_arg(const char *arg_, struct \n> rev_info *revs, int flags, unsi\n> if (!cant_be_filename) {\n> *dotdot = '.';\n> verify_non_filename(revs->prefix, arg);\n> + *dotdot = '\\0';\n> }\n>\n> a_obj = parse_object(from_sha1);\n> b_obj = parse_object(sha1);\n> if (!a_obj || !b_obj) {\n> missing:\n> + *dotdot = '.';\n> if (revs->ignore_missing)\n> return 0;\n> die(symmetric\n> @@ -1525,6 +1527,7 @@ int handle_revision_arg(const char *arg_, struct \n> rev_info *revs, int flags, unsi\n> REV_CMD_RIGHT, flags);\n> add_pending_object(revs, a_obj, this);\n> add_pending_object(revs, b_obj, next);\n> + *dotdot = '.';\n> return 0;\n> }\n> *dotdot = '.';\n> diff --git a/t/t4202-log.sh b/t/t4202-log.sh\n> index f57799071..6da1bbe91 100755\n> --- a/t/t4202-log.sh\n> +++ b/t/t4202-log.sh\n> @@ -1380,4 +1380,13 @@ test_expect_success 'log --source paints tag names' \n> '\n> test_cmp expect actual\n> '\n>\n> +test_expect_success 'log --source paints symmetric ranges' '\n> + cat >expect <<-\\EOF &&\n> + 09e12a9 source-b three\n> + 8e393e1 source-a two\n> + EOF\n> + git log --oneline --source source-a...source-b >actual &&\n> + test_cmp expect actual\n> +'\n> +\n> test_done\n> -- \n> 2.13.0.219.g63f6bc368\n> \n\n"},{"id":"320594","messageId":"20170523195132.s57ikef4romy3n3r@sigill.intra.peff.net","threadId":"45968","inReplyTo":"F66E608B6CE34520BD764C404A2AD36C@PhilipOakley","subject":"Re: [PATCH 01/15] handle_revision_arg: reset \"dotdot\" consistently","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-05-23T19:51:32Z","receivedAt":"2017-05-23T19:51:39Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, May 20, 2017 at 03:56:32PM +0100, Philip Oakley wrote:\n\n> > That means we do report the correct name for \"a\" in the\n> > pending array. But some code paths try to show the whole\n> > \"a..b\" name in error messages, and these erroneously show\n> > only \"a\" instead of \"a..b\". E.g.:\n> > \n> > $ git cherry-pick HEAD:foo..HEAD:foo\n> \n> shouldn't this be three dots? Also the para above uses two dot examples in\n> its description but the paras before that start by describing the three dot\n> case.\n\nYeah, it should be. The problem happens with the two-dot case, too (it's\nthe same code) but to provoke cherry-pick to actually show the error in\nquestion, you need to use three-dots. There's probably a way to provoke\na broken error message with the two-dot case, but I didn't dig further\nafter finding this one.\n\nThanks for being a careful reader. Here's the patch with a modified\ncommit message. I don't think this series otherwise needs a re-roll so\nfar.\n\n-- >8 --\nSubject: [PATCH] handle_revision_arg: reset \"dotdot\" consistently\n\nWhen we are parsing a range like \"a..b\", we write a\ntemporary NUL over the first \".\", so that we can access the\nnames \"a\" and \"b\" as C strings. But our restoration of the\noriginal \".\" is done at inconsistent times, which can lead\nto confusing results.\n\nFor most calls, we restore the \".\" after we resolve the\nnames, but before we call verify_non_filename(). This means\nthat when we later call add_pending_object(), the name for\nthe left-hand \"a\" has been re-expanded to \"a..b\". You can\nsee this with:\n\n git log --source a...b\n\nwhere \"b\" will be correctly marked with \"b\", but \"a\" will be\nmarked with \"a...b\". Likewise with \"a..b\" (though you need\nto use --boundary to even see \"a\" at all in that case).\n\nTo top off the confusion, when the REVARG_CANNOT_BE_FILENAME\nflag is set, we skip the non-filename check, and leave the\nNUL in place.\n\nThat means we do report the correct name for \"a\" in the\npending array. But some code paths try to show the whole\n\"a...b\" name in error messages, and these erroneously show\nonly \"a\" instead of \"a...b\". E.g.:\n\n $ git cherry-pick HEAD:foo...HEAD:foo\n error: object d95f3ad14dee633a758d2e331151e950dd13e4ed is a blob, not a commit\n error: object d95f3ad14dee633a758d2e331151e950dd13e4ed is a blob, not a commit\n fatal: Invalid symmetric difference expression HEAD:foo\n\n(That last message should be \"HEAD:foo...HEAD:foo\"; I used\ncherry-pick because it passes the CANNOT_BE_FILENAME flag).\n\nAs an interesting side note, cherry-pick actually looks at\nand re-resolves the arguments from the pending->name fields.\nSo it would have been visibly broken by the first bug, but\nthe effect was canceled out by the second one.\n\nThis patch makes the whole function consistent by re-writing\nthe NUL immediately after calling verify_non_filename(), and\nthen restoring the \".\" as appropriate in some error-printing\nand early-return code paths.\n\nSigned-off-by: Jeff King \n---\n revision.c | 3 +++\n t/t4202-log.sh | 9 +++++++++\n 2 files changed, 12 insertions(+)\n\ndiff --git a/revision.c b/revision.c\nindex 8a8c1789c..014bf52e3 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -1477,12 +1477,14 @@ int handle_revision_arg(const char *arg_, struct rev_info *revs, int flags, unsi\n \t\t\tif (!cant_be_filename) {\n \t\t\t\t*dotdot = '.';\n \t\t\t\tverify_non_filename(revs->prefix, arg);\n+\t\t\t\t*dotdot = '\\0';\n \t\t\t}\n \n \t\t\ta_obj = parse_object(from_sha1);\n \t\t\tb_obj = parse_object(sha1);\n \t\t\tif (!a_obj || !b_obj) {\n \t\t\tmissing:\n+\t\t\t\t*dotdot = '.';\n \t\t\t\tif (revs->ignore_missing)\n \t\t\t\t\treturn 0;\n \t\t\t\tdie(symmetric\n@@ -1525,6 +1527,7 @@ int handle_revision_arg(const char *arg_, struct rev_info *revs, int flags, unsi\n \t\t\t\t\tREV_CMD_RIGHT, flags);\n \t\t\tadd_pending_object(revs, a_obj, this);\n \t\t\tadd_pending_object(revs, b_obj, next);\n+\t\t\t*dotdot = '.';\n \t\t\treturn 0;\n \t\t}\n \t\t*dotdot = '.';\ndiff --git a/t/t4202-log.sh b/t/t4202-log.sh\nindex 1c7d6729c..76c511973 100755\n--- a/t/t4202-log.sh\n+++ b/t/t4202-log.sh\n@@ -1392,4 +1392,13 @@ test_expect_success 'log --source paints tag names' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'log --source paints symmetric ranges' '\n+\tcat >expect <<-\\EOF &&\n+\t09e12a9\tsource-b three\n+\t8e393e1\tsource-a two\n+\tEOF\n+\tgit log --oneline --source source-a...source-b >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.13.0.387.gec0afcebb.dirty\n\n"},{"id":"320605","messageId":"25505945D62246FDB2EE3E4C7CD76E6E@PhilipOakley","threadId":"45968","inReplyTo":"20170523195132.s57ikef4romy3n3r@sigill.intra.peff.net","subject":"Re: [PATCH 01/15] handle_revision_arg: reset \"dotdot\" consistently","fromName":"Philip Oakley","fromEmail":"philipoakley@iee.org","sentAt":"2017-05-23T23:47:00Z","receivedAt":"2017-05-23T23:47:09Z","isPatch":true,"sender":{"key":"philipoakley@iee.email","avatar":"https://avatars.githubusercontent.com/u/914343?v=4"},"body":"From: \"Jeff King\" \n> On Sat, May 20, 2017 at 03:56:32PM +0100, Philip Oakley wrote:\n>\n>> > That means we do report the correct name for \"a\" in the\n>> > pending array. But some code paths try to show the whole\n>> > \"a..b\" name in error messages, and these erroneously show\n>> > only \"a\" instead of \"a..b\". E.g.:\n>> >\n>> > $ git cherry-pick HEAD:foo..HEAD:foo\n>>\n>> shouldn't this be three dots? Also the para above uses two dot examples \n>> in\n>> its description but the paras before that start by describing the three \n>> dot\n>> case.\n>\n> Yeah, it should be. The problem happens with the two-dot case, too (it's\n> the same code) but to provoke cherry-pick to actually show the error in\n> question, you need to use three-dots. There's probably a way to provoke\n> a broken error message with the two-dot case, but I didn't dig further\n> after finding this one.\n>\n> Thanks for being a careful reader. Here's the patch with a modified\n> commit message. I don't think this series otherwise needs a re-roll so\n> far.\n\nAgreed, it's only a small point. It is difficult to cover both the two and \nthree dot cases mid flow. Perhaps show the two options very early....\n>\n> -- >8 --\n> Subject: [PATCH] handle_revision_arg: reset \"dotdot\" consistently\n>\n> When we are parsing a range like \"a..b\", we write a\n\nPerhaps this early? s/\"a..b\"/\"a..b\" or \"a...b\"/\n\n> temporary NUL over the first \".\", so that we can access the\n> names \"a\" and \"b\" as C strings. But our restoration of the\n> original \".\" is done at inconsistent times, which can lead\n> to confusing results.\n>\n> For most calls, we restore the \".\" after we resolve the\n> names, but before we call verify_non_filename(). This means\n> that when we later call add_pending_object(), the name for\n> the left-hand \"a\" has been re-expanded to \"a..b\". You can\n> see this with:\n>\nThis now looks 'correct' relative to the initial multi-dot options.\n\n> git log --source a...b\n>\n> where \"b\" will be correctly marked with \"b\", but \"a\" will be\n> marked with \"a...b\". Likewise with \"a..b\" (though you need\n> to use --boundary to even see \"a\" at all in that case).\n>\n> To top off the confusion, when the REVARG_CANNOT_BE_FILENAME\n> flag is set, we skip the non-filename check, and leave the\n> NUL in place.\n>\n> That means we do report the correct name for \"a\" in the\n> pending array. But some code paths try to show the whole\n> \"a...b\" name in error messages, and these erroneously show\n> only \"a\" instead of \"a...b\". E.g.:\n>\n> $ git cherry-pick HEAD:foo...HEAD:foo\n> error: object d95f3ad14dee633a758d2e331151e950dd13e4ed is a blob, not a \n> commit\n> error: object d95f3ad14dee633a758d2e331151e950dd13e4ed is a blob, not a \n> commit\n> fatal: Invalid symmetric difference expression HEAD:foo\n>\n> (That last message should be \"HEAD:foo...HEAD:foo\"; I used\n> cherry-pick because it passes the CANNOT_BE_FILENAME flag).\n>\n> As an interesting side note, cherry-pick actually looks at\n> and re-resolves the arguments from the pending->name fields.\n> So it would have been visibly broken by the first bug, but\n> the effect was canceled out by the second one.\n>\n> This patch makes the whole function consistent by re-writing\n> the NUL immediately after calling verify_non_filename(), and\n> then restoring the \".\" as appropriate in some error-printing\n> and early-return code paths.\n>\n> Signed-off-by: Jeff King \n--\nPhilip\n\n\n> ---\n> revision.c | 3 +++\n> t/t4202-log.sh | 9 +++++++++\n> 2 files changed, 12 insertions(+)\n>\n> diff --git a/revision.c b/revision.c\n> index 8a8c1789c..014bf52e3 100644\n> --- a/revision.c\n> +++ b/revision.c\n> @@ -1477,12 +1477,14 @@ int handle_revision_arg(const char *arg_, struct \n> rev_info *revs, int flags, unsi\n> if (!cant_be_filename) {\n> *dotdot = '.';\n> verify_non_filename(revs->prefix, arg);\n> + *dotdot = '\\0';\n> }\n>\n> a_obj = parse_object(from_sha1);\n> b_obj = parse_object(sha1);\n> if (!a_obj || !b_obj) {\n> missing:\n> + *dotdot = '.';\n> if (revs->ignore_missing)\n> return 0;\n> die(symmetric\n> @@ -1525,6 +1527,7 @@ int handle_revision_arg(const char *arg_, struct \n> rev_info *revs, int flags, unsi\n> REV_CMD_RIGHT, flags);\n> add_pending_object(revs, a_obj, this);\n> add_pending_object(revs, b_obj, next);\n> + *dotdot = '.';\n> return 0;\n> }\n> *dotdot = '.';\n> diff --git a/t/t4202-log.sh b/t/t4202-log.sh\n> index 1c7d6729c..76c511973 100755\n> --- a/t/t4202-log.sh\n> +++ b/t/t4202-log.sh\n> @@ -1392,4 +1392,13 @@ test_expect_success 'log --source paints tag names' \n> '\n> test_cmp expect actual\n> '\n>\n> +test_expect_success 'log --source paints symmetric ranges' '\n> + cat >expect <<-\\EOF &&\n> + 09e12a9 source-b three\n> + 8e393e1 source-a two\n> + EOF\n> + git log --oneline --source source-a...source-b >actual &&\n> + test_cmp expect actual\n> +'\n> +\n> test_done\n> -- \n> 2.13.0.387.gec0afcebb.dirty\n> \n\n"},{"id":"320610","messageId":"xmqqinkrhtqo.fsf@gitster.mtv.corp.google.com","threadId":"45968","inReplyTo":"20170519125200.mtpchnyg26re5u4h@sigill.intra.peff.net","subject":"Re: [PATCH 05/15] handle_revision_arg: add handle_dotdot() helper","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-05-24T02:30:07Z","receivedAt":"2017-05-24T02:30:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> The handle_revision_arg function is rather long, and a big\n> chunk of it is handling the range operators. Let's pull that\n> out to a separate helper. While we're doing so, we can clean\n> up a few of the rough edges that made the flow hard to\n> follow:\n>\n> - instead of manually restoring *dotdot (that we overwrote\n> with a NUL), do the real work in a sub-helper, which\n> makes it clear that the munge/restore lines are a\n> matched pair\n>\n> - eliminate a goto which wasn't actually used for control\n> flow, but only to avoid duplicating a few lines\n> (instead, those lines are pushed into another helper\n> function)\n>\n> - use early returns instead of deep nesting\n>\n> - consistently name all variables for the left-hand side\n> of the range as \"a\" (rather than \"this\" or \"from\") and\n> the right-hand side as \"b\" (rather than \"next\", or using\n> the unadorned \"sha1\" or \"flags\" from the main function).\n>\n> Signed-off-by: Jeff King \n> ---\n> revision.c | 177 +++++++++++++++++++++++++++++++++++--------------------------\n> 1 file changed, 102 insertions(+), 75 deletions(-)\n\nNicely done. Together with [PATCH 04/15] the resulting code wrt \"..\"\nis much easier to follow.\n\n> diff --git a/revision.c b/revision.c\n> index dec06ff8b..eb45501fd 100644\n> --- a/revision.c\n> +++ b/revision.c\n> @@ -1429,10 +1429,109 @@ static void prepare_show_merge(struct rev_info *revs)\n> \trevs->limited = 1;\n> }\n> \n> +static int dotdot_missing(const char *arg, char *dotdot,\n> +\t\t\t struct rev_info *revs, int symmetric)\n> +{\n> +\tif (revs->ignore_missing)\n> +\t\treturn 0;\n> +\t/* de-munge so we report the full argument */\n> +\t*dotdot = '.';\n> +\tdie(symmetric\n> +\t ? \"Invalid symmetric difference expression %s\"\n> +\t : \"Invalid revision range %s\", arg);\n> +}\n> +\n> +static int handle_dotdot_1(const char *arg, char *dotdot,\n> +\t\t\t struct rev_info *revs, int flags,\n> +\t\t\t int cant_be_filename)\n> +{\n> +\tconst char *a_name, *b_name;\n> +\tstruct object_id a_oid, b_oid;\n> +\tstruct object *a_obj, *b_obj;\n> +\tunsigned int a_flags, b_flags;\n> +\tint symmetric = 0;\n> +\tunsigned int flags_exclude = flags ^ (UNINTERESTING | BOTTOM);\n> +\n> +\ta_name = arg;\n> +\tif (!*a_name)\n> +\t\ta_name = \"HEAD\";\n> +\n> +\tb_name = dotdot + 2;\n> +\tif (*b_name == '.') {\n> +\t\tsymmetric = 1;\n> +\t\tb_name++;\n> +\t}\n> +\tif (!*b_name)\n> +\t\tb_name = \"HEAD\";\n> +\n> +\tif (get_sha1_committish(a_name, a_oid.hash) ||\n> +\t get_sha1_committish(b_name, b_oid.hash))\n> +\t\treturn -1;\n> +\n> +\tif (!cant_be_filename) {\n> +\t\t*dotdot = '.';\n> +\t\tverify_non_filename(revs->prefix, arg);\n> +\t\t*dotdot = '\\0';\n> +\t}\n> +\n> +\ta_obj = parse_object(a_oid.hash);\n> +\tb_obj = parse_object(b_oid.hash);\n> +\tif (!a_obj || !b_obj)\n> +\t\treturn dotdot_missing(arg, dotdot, revs, symmetric);\n> +\n> +\tif (!symmetric) {\n> +\t\t/* just A..B */\n> +\t\tb_flags = flags;\n> +\t\ta_flags = flags_exclude;\n> +\t} else {\n> +\t\t/* A...B -- find merge bases between the two */\n> +\t\tstruct commit *a, *b;\n> +\t\tstruct commit_list *exclude;\n> +\n> +\t\ta = lookup_commit_reference(a_obj->oid.hash);\n> +\t\tb = lookup_commit_reference(b_obj->oid.hash);\n> +\t\tif (!a || !b)\n> +\t\t\treturn dotdot_missing(arg, dotdot, revs, symmetric);\n> +\n> +\t\texclude = get_merge_bases(a, b);\n> +\t\tadd_rev_cmdline_list(revs, exclude, REV_CMD_MERGE_BASE,\n> +\t\t\t\t flags_exclude);\n> +\t\tadd_pending_commit_list(revs, exclude, flags_exclude);\n> +\t\tfree_commit_list(exclude);\n> +\n> +\t\tb_flags = flags;\n> +\t\ta_flags = flags | SYMMETRIC_LEFT;\n> +\t}\n> +\n> +\ta_obj->flags |= a_flags;\n> +\tb_obj->flags |= b_flags;\n> +\tadd_rev_cmdline(revs, a_obj, a_name, REV_CMD_LEFT, a_flags);\n> +\tadd_rev_cmdline(revs, b_obj, b_name, REV_CMD_RIGHT, b_flags);\n> +\tadd_pending_object(revs, a_obj, a_name);\n> +\tadd_pending_object(revs, b_obj, b_name);\n> +\treturn 0;\n> +}\n> +\n> +static int handle_dotdot(const char *arg,\n> +\t\t\t struct rev_info *revs, int flags,\n> +\t\t\t int cant_be_filename)\n> +{\n> +\tchar *dotdot = strstr(arg, \"..\");\n> +\tint ret;\n> +\n> +\tif (!dotdot)\n> +\t\treturn -1;\n> +\n> +\t*dotdot = '\\0';\n> +\tret = handle_dotdot_1(arg, dotdot, revs, flags, cant_be_filename);\n> +\t*dotdot = '.';\n> +\n> +\treturn ret;\n> +}\n> +\n> int handle_revision_arg(const char *arg_, struct rev_info *revs, int flags, unsigned revarg_opt)\n> {\n> \tstruct object_context oc;\n> -\tchar *dotdot;\n> \tchar *mark;\n> \tstruct object *object;\n> \tunsigned char sha1[20];\n> @@ -1451,80 +1550,8 @@ int handle_revision_arg(const char *arg_, struct rev_info *revs, int flags, unsi\n> \t\treturn -1;\n> \t}\n> \n> -\tdotdot = strstr(arg, \"..\");\n> -\tif (dotdot) {\n> -\t\tunsigned char from_sha1[20];\n> -\t\tconst char *next = dotdot + 2;\n> -\t\tconst char *this = arg;\n> -\t\tint symmetric = *next == '.';\n> -\t\tunsigned int flags_exclude = flags ^ (UNINTERESTING | BOTTOM);\n> -\t\tunsigned int a_flags;\n> -\n> -\t\t*dotdot = 0;\n> -\t\tnext += symmetric;\n> -\n> -\t\tif (!*next)\n> -\t\t\tnext = \"HEAD\";\n> -\t\tif (dotdot == arg)\n> -\t\t\tthis = \"HEAD\";\n> -\t\tif (!get_sha1_committish(this, from_sha1) &&\n> -\t\t !get_sha1_committish(next, sha1)) {\n> -\t\t\tstruct object *a_obj, *b_obj;\n> -\n> -\t\t\tif (!cant_be_filename) {\n> -\t\t\t\t*dotdot = '.';\n> -\t\t\t\tverify_non_filename(revs->prefix, arg);\n> -\t\t\t\t*dotdot = '\\0';\n> -\t\t\t}\n> -\n> -\t\t\ta_obj = parse_object(from_sha1);\n> -\t\t\tb_obj = parse_object(sha1);\n> -\t\t\tif (!a_obj || !b_obj) {\n> -\t\t\tmissing:\n> -\t\t\t\t*dotdot = '.';\n> -\t\t\t\tif (revs->ignore_missing)\n> -\t\t\t\t\treturn 0;\n> -\t\t\t\tdie(symmetric\n> -\t\t\t\t ? \"Invalid symmetric difference expression %s\"\n> -\t\t\t\t : \"Invalid revision range %s\", arg);\n> -\t\t\t}\n> -\n> -\t\t\tif (!symmetric) {\n> -\t\t\t\t/* just A..B */\n> -\t\t\t\ta_flags = flags_exclude;\n> -\t\t\t} else {\n> -\t\t\t\t/* A...B -- find merge bases between the two */\n> -\t\t\t\tstruct commit *a, *b;\n> -\t\t\t\tstruct commit_list *exclude;\n> -\n> -\t\t\t\ta = lookup_commit_reference(a_obj->oid.hash);\n> -\t\t\t\tb = lookup_commit_reference(b_obj->oid.hash);\n> -\t\t\t\tif (!a || !b)\n> -\t\t\t\t\tgoto missing;\n> -\t\t\t\texclude = get_merge_bases(a, b);\n> -\t\t\t\tadd_rev_cmdline_list(revs, exclude,\n> -\t\t\t\t\t\t REV_CMD_MERGE_BASE,\n> -\t\t\t\t\t\t flags_exclude);\n> -\t\t\t\tadd_pending_commit_list(revs, exclude,\n> -\t\t\t\t\t\t\tflags_exclude);\n> -\t\t\t\tfree_commit_list(exclude);\n> -\n> -\t\t\t\ta_flags = flags | SYMMETRIC_LEFT;\n> -\t\t\t}\n> -\n> -\t\t\ta_obj->flags |= a_flags;\n> -\t\t\tb_obj->flags |= flags;\n> -\t\t\tadd_rev_cmdline(revs, a_obj, this,\n> -\t\t\t\t\tREV_CMD_LEFT, a_flags);\n> -\t\t\tadd_rev_cmdline(revs, b_obj, next,\n> -\t\t\t\t\tREV_CMD_RIGHT, flags);\n> -\t\t\tadd_pending_object(revs, a_obj, this);\n> -\t\t\tadd_pending_object(revs, b_obj, next);\n> -\t\t\t*dotdot = '.';\n> -\t\t\treturn 0;\n> -\t\t}\n> -\t\t*dotdot = '.';\n> -\t}\n> +\tif (!handle_dotdot(arg, revs, flags, revarg_opt))\n> +\t\treturn 0;\n> \n> \tmark = strstr(arg, \"^@\");\n> \tif (mark && !mark[2]) {\n"},{"id":"320611","messageId":"xmqqefvfht29.fsf@gitster.mtv.corp.google.com","threadId":"45968","inReplyTo":"20170519124651.4q7waz75rmzfopgn@sigill.intra.peff.net","subject":"Re: [PATCH 0/15] retain blob info for git diff HEAD:foo HEAD:bar","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-05-24T02:44:46Z","receivedAt":"2017-05-24T02:44:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patches around [PATCH 06-08/15] made some unexpected (at least to\nme) turns but the series told a coherent story, building on top of\nwhat has been achieved in the previous steps.\n\nThanks for a pleasant read. \n"},{"id":"320613","messageId":"xmqqzie3gegc.fsf@gitster.mtv.corp.google.com","threadId":"45968","inReplyTo":"20170519125006.gbvzr2dqrcycmpx7@sigill.intra.peff.net","subject":"Re: [PATCH 03/15] handle_revision_arg: stop using \"dotdot\" as a generic pointer","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-05-24T02:45:39Z","receivedAt":"2017-05-24T02:45:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> The handle_revision_arg() function has a \"dotdot\" variable\n> that it uses to find a \"..\" or \"...\" in the argument. If we\n> don't find one, we look for other marks, like \"^!\". But we\n> just keep re-using the \"dotdot\" variable, which is\n> confusing.\n>\n> Let's introduce a separate \"mark\" variable that can be used\n> for these other marks. They still reuse the same variable,\n> but at least the name is no longer actively misleading.\n>\n> Signed-off-by: Jeff King \n> ---\n> It may make sense to pull each of these into its own helper. I didn't\n> really look because they're so small, and because the return semantics\n> seemed confusing to me. Some of them return, and some of them keep\n> parsing. Some of them restore the NUL they overwrite, and some do not.\n>\n> I didn't dig in to see if there are weird corner cases where they\n> misbehave.\n\nI do not quite know what corner cases you meant, but I agree that\nmany places in the codepath we forget to restore \"^\" we temporarily\noverwrite. I suspect that none of them is deliberately leaving \"^\"\nunrestored and they are just being careless (or they truly do not\ncare because they assume nobody will look at arg later).\n\nAnd I think not restoring cannot be a correct thing to do. After\nall of these parsing, add_rev_cmdline() wants to see arg_ intact.\n\nBut let's keep reading; perhaps they are addressed in a later patch,\nor they are left as-is, and either is OK.\n"},{"id":"320656","messageId":"20170524095459.ojcarze45ery76oi@sigill.intra.peff.net","threadId":"45968","inReplyTo":"xmqqzie3gegc.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 03/15] handle_revision_arg: stop using \"dotdot\" as a generic pointer","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-05-24T09:55:00Z","receivedAt":"2017-05-24T09:55:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, May 24, 2017 at 11:45:39AM +0900, Junio C Hamano wrote:\n\n> > It may make sense to pull each of these into its own helper. I didn't\n> > really look because they're so small, and because the return semantics\n> > seemed confusing to me. Some of them return, and some of them keep\n> > parsing. Some of them restore the NUL they overwrite, and some do not.\n> >\n> > I didn't dig in to see if there are weird corner cases where they\n> > misbehave.\n> \n> I do not quite know what corner cases you meant, but I agree that\n> many places in the codepath we forget to restore \"^\" we temporarily\n> overwrite. I suspect that none of them is deliberately leaving \"^\"\n> unrestored and they are just being careless (or they truly do not\n> care because they assume nobody will look at arg later).\n> \n> And I think not restoring cannot be a correct thing to do. After\n> all of these parsing, add_rev_cmdline() wants to see arg_ intact.\n> \n> But let's keep reading; perhaps they are addressed in a later patch,\n> or they are left as-is, and either is OK.\n\nI don't really know what corner cases I meant, either. :) I just saw\nthat the code looked funny, but nobody noticed for the common cases, so\nI presumed any misbehavior would be from uncommon ones.\n\nAs far as \"maybe not restoring is intentional\", I wondered if there are\ncases where we might allow multiple marks. E.g., if we wanted to allow\n\"foo^@^!\", then we might need to progressively pull items off the end,\nshortening the string. But I don't think that can be correct:\n\n - these marks generally don't make sense to combine in the first place\n\n - even if we allowed combinations, since we make only a single pass\n through the function, we'd require the combinations to come in a\n particular order\n\n - even if it were intentional to do so, we'd still be adding weird\n stuff to add_rev_cmdline(), as you noted\n\nBut I had some other questions, too, about what's supposed to be in the\n\"name\" field of \"pending\" in some of these cases. For instance, try:\n\n git tag foo 6a0bc7cf0efbefa5a949d958947e68e29534f04d\n git log --oneline --source foo^-\n\nAll of the commits are marked as coming from \"foo\". That's because of\ntwo things:\n\n 1. When we parse \"^-\", we turn the first character to NUL. So our call\n to add_parents_only() sees just \"foo\", with no tail.\n\n 2. We never restore the \"^\". So later when we add \"foo\" itself, the\n arg name is still \"foo\".\n\nSo arguably that's correct (these all came from \"foo\", which is a\nresolvable name, and I think that's what the name field of\nadd_pending_object is going for). But that means add_rev_cmdline() sees\nthe same munged string, which is probably wrong. We can't possibly get\nthat case right with munging since the add_rev_cmdline() and\nadd_pending_object() calls come in pairs. We'd have to actually copy the\npending name into a separate string instead.\n\nSo like I said, I was sufficiently confused about what was supposed to\nhappen that I didn't try fixing it.\n\n-Peff\n"},{"id":"320657","messageId":"20170524095715.wgkft6fofjop5jco@sigill.intra.peff.net","threadId":"45968","inReplyTo":"xmqqefvfht29.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 0/15] retain blob info for git diff HEAD:foo HEAD:bar","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-05-24T09:57:15Z","receivedAt":"2017-05-24T09:57:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, May 24, 2017 at 11:44:46AM +0900, Junio C Hamano wrote:\n\n> Patches around [PATCH 06-08/15] made some unexpected (at least to\n> me) turns but the series told a coherent story, building on top of\n> what has been achieved in the previous steps.\n\nYeah, those patches are not technically required for the rest of it. The\n\"strbuf\" conversion for oc->path is just something I've wanted to fix\nfor a long time. It made sense to me to do it before teaching\nhandle_dotdot() to look at oc->path (because the interface to do so\nchanged a bit).\n\n-Peff\n"}]}