{"thread":{"id":"11108","subject":"[PATCH/RFC] Use regex for :/ matching","startedAt":"2007-12-03T04:32:58Z","lastAt":"2007-12-06T12:13:37Z","messageCount":11,"participants":["Jeff King","Johannes Schindelin","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"61720","messageId":"20071203043258.GA16658@coredump.intra.peff.net","threadId":"11108","inReplyTo":null,"subject":"[PATCH/RFC] Use regex for :/ matching","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2007-12-03T04:32:58Z","receivedAt":"2007-12-03T04:32:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The sha1 syntax :/ used to be a strict prefix match.\nInstead, let's use a regular expression, which can save on\ntyping. E.g.,\n\n git show :/\"object name: introduce ':/'\"\n\nvs\n\n git show :/introduce..:/.oneline\n\nSigned-off-by: Jeff King \n---\nObviously this changes the semantics of existing queries.\nSpecifically:\n\n - regex metacharacters are now interpreted\n - the pattern is no longer anchored at the start\n\nI find it much more useful than the original, but perhaps it deserves\nits own syntax instead?\n\nI also considered that it might be much slower than the original, but it\nis not:\n\n # original\n $ time git log :/notfound >/dev/null\n real 0m1.055s\n user 0m1.020s\n sys 0m0.024s\n\n # regex\n $ time git log :/notfound >/dev/null\n real 0m1.065s\n user 0m1.028s\n sys 0m0.036s\n\nCuriously, both are much slower than --grep:\n\n $ time git log --grep=notfound >/dev/null\n real 0m0.677s\n user 0m0.640s\n sys 0m0.036s\n\nAnd finally, if accepted, a followup patch should change the \"prefix\"\nvariable to \"search\" or similar; putting it in here made the diff a bit\nnoisy.\n\n Documentation/git-rev-parse.txt | 4 ++--\n sha1_name.c | 25 ++++++++++++++++++++++---\n 2 files changed, 24 insertions(+), 5 deletions(-)\n\ndiff --git a/Documentation/git-rev-parse.txt b/Documentation/git-rev-parse.txt\nindex 329fce0..41dd684 100644\n--- a/Documentation/git-rev-parse.txt\n+++ b/Documentation/git-rev-parse.txt\n@@ -208,8 +208,8 @@ blobs contained in a commit.\n and dereference the tag recursively until a non-tag object is\n found.\n \n-* A colon, followed by a slash, followed by a text: this names\n- a commit whose commit message starts with the specified text.\n+* A colon, followed by a slash, followed by a regular expression: this names\n+ a commit whose commit message starts with a line matching the expression.\n This name returns the youngest matching commit which is\n reachable from any ref. If the commit message starts with a\n '!', you have to repeat that; the special sequence ':/!',\ndiff --git a/sha1_name.c b/sha1_name.c\nindex d364244..2ad91a3 100644\n--- a/sha1_name.c\n+++ b/sha1_name.c\n@@ -588,8 +588,8 @@ static int handle_one_ref(const char *path,\n \n /*\n * This interprets names like ':/Initial revision of \"git\"' by searching\n- * through history and returning the first commit whose message starts\n- * with the given string.\n+ * through history and returning the first commit whose first line\n+ * matches the given regex.\n *\n * For future extension, ':/!' is reserved. If you want to match a message\n * beginning with a '!', you have to repeat the exclamation mark.\n@@ -600,12 +600,22 @@ static int get_sha1_oneline(const char *prefix, unsigned char *sha1)\n {\n \tstruct commit_list *list = NULL, *backup = NULL, *l;\n \tint retval = -1;\n+\tregex_t re;\n+\tint r;\n \n \tif (prefix[0] == '!') {\n \t\tif (prefix[1] != '!')\n \t\t\tdie (\"Invalid search pattern: %s\", prefix);\n \t\tprefix++;\n \t}\n+\n+\tr = regcomp(&re, prefix, REG_NOSUB);\n+\tif (r != 0) {\n+\t\tchar errbuf[256];\n+\t\tregerror(r, &re, errbuf, sizeof(errbuf));\n+\t\treturn error(\"unable to compile regex: %s\", errbuf);\n+\t}\n+\n \tif (!save_commit_buffer)\n \t\treturn error(\"Could not expand oneline-name.\");\n \tfor_each_ref(handle_one_ref, &list);\n@@ -613,13 +623,21 @@ static int get_sha1_oneline(const char *prefix, unsigned char *sha1)\n \t\tcommit_list_insert(l->item, &backup);\n \twhile (list) {\n \t\tchar *p;\n+\t\tchar *end;\n \t\tstruct commit *commit;\n \n \t\tcommit = pop_most_recent_commit(&list, ONELINE_SEEN);\n \t\tparse_object(commit->object.sha1);\n \t\tif (!commit->buffer || !(p = strstr(commit->buffer, \"\\n\\n\")))\n \t\t\tcontinue;\n-\t\tif (!prefixcmp(p + 2, prefix)) {\n+\t\tp += 2;\n+\t\tend = strchr(p, '\\n');\n+\t\tif (end)\n+\t\t\t*end = '\\0';\n+\t\tr = regexec(&re, p, 0, NULL, 0);\n+\t\tif (end)\n+\t\t\t*end = '\\n';\n+\t\tif (r == 0) {\n \t\t\thashcpy(sha1, commit->object.sha1);\n \t\t\tretval = 0;\n \t\t\tbreak;\n@@ -628,6 +646,7 @@ static int get_sha1_oneline(const char *prefix, unsigned char *sha1)\n \tfree_commit_list(list);\n \tfor (l = backup; l; l = l->next)\n \t\tclear_commit_marks(l->item, ONELINE_SEEN);\n+\tregfree(&re);\n \treturn retval;\n }\n \n-- \n1.5.3.7.2068.g5949-dirty\n"},{"id":"61768","messageId":"Pine.LNX.4.64.0712031052410.27959@racer.site","threadId":"11108","inReplyTo":"20071203043258.GA16658@coredump.intra.peff.net","subject":"Re: [PATCH/RFC] Use regex for :/ matching","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2007-12-03T10:55:15Z","receivedAt":"2007-12-03T10:55:15Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Sun, 2 Dec 2007, Jeff King wrote:\n\n> The sha1 syntax :/ used to be a strict prefix match.\n> Instead, let's use a regular expression, which can save on\n> typing. E.g.,\n> \n> git show :/\"object name: introduce ':/'\"\n> \n> vs\n> \n> git show :/introduce..:/.oneline\n\nHeh: \nhttp://repo.or.cz/w/git/dscho.git?a=commitdiff;h=2546cd9732bb8d4bc1d2485ba7bbc1d5c8bac935\n\nExcept that I did not support \"..\" (does yours?), _and_ that my patch is \nnot as nice as yours.\n\nBut then, my patch also works when save_commit_buffer == 0. But I can \nrefactor this into its own patch, since it really is a separate issue.\n\nCiao,\nDscho\n"},{"id":"61792","messageId":"20071203173022.GA19219@coredump.intra.peff.net","threadId":"11108","inReplyTo":"Pine.LNX.4.64.0712031052410.27959@racer.site","subject":"Re: [PATCH/RFC] Use regex for :/ matching","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2007-12-03T17:30:22Z","receivedAt":"2007-12-03T17:30:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Dec 03, 2007 at 10:55:15AM +0000, Johannes Schindelin wrote:\n\n> > The sha1 syntax :/ used to be a strict prefix match.\n> > Instead, let's use a regular expression, which can save on\n> Heh: \n> http://repo.or.cz/w/git/dscho.git?a=commitdiff;h=2546cd9732bb8d4bc1d2485ba7bbc1d5c8bac935\n\nHmm, the major difference seems to be that you grep the entire body,\nwhereas I grep just the oneline. My goal was to avoid matching the\nsearch string in the message of a merge commit with merge summaries\nturned on.\n\n> Except that I did not support \"..\" (does yours?), _and_ that my patch is \n> not as nice as yours.\n\nNo, I didn't. I'm not sure it is sane, since :/ can contain free-form\ntext (and with a regex, .. is not that unlikely). And you can always do\ngit-log --not :/foo :/bar\n\n> But then, my patch also works when save_commit_buffer == 0. But I can \n> refactor this into its own patch, since it really is a separate issue.\n\nAgreed.\n\n-Peff\n"},{"id":"61801","messageId":"7vbq97iqxd.fsf@gitster.siamese.dyndns.org","threadId":"11108","inReplyTo":"20071203173022.GA19219@coredump.intra.peff.net","subject":"Re: [PATCH/RFC] Use regex for :/ matching","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-12-03T18:17:18Z","receivedAt":"2007-12-03T18:17:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Mon, Dec 03, 2007 at 10:55:15AM +0000, Johannes Schindelin wrote:\n>\n>> Except that I did not support \"..\" (does yours?), _and_ that my patch is \n>> not as nice as yours.\n>\n> No, I didn't. I'm not sure it is sane, since :/ can contain free-form\n> text (and with a regex, .. is not that unlikely). And you can always do\n> git-log --not :/foo :/bar\n>\n>> But then, my patch also works when save_commit_buffer == 0. But I can \n>> refactor this into its own patch, since it really is a separate issue.\n>\n> Agreed.\n\nWhat I found mildly irritating in the current syntax (and that is the\nprimary reason why I use it rarely) is that there is no way to tell it\nto dig from a particular ref (e.g. \"on master branch, find the latest\ncommit whose title matches this string\").\n\nFor \"git-log\", you can do \"git log master --grep=string -1\" to emulate\nthis, and it extends to \"git log maint..master --grep=string\" to limit\nthe revision ranges and \"find all not just latest\" very naturally.\n\nAlso once we start to talk about supporting \"ranges\", I suspect the\nsemantics becomes fuzzier, because \":/\" is defined as an extended SHA-1\nexpression that resolves to a single commit. \":/A..:/B\" is probably\nunneeded as you can always say \"^:/B :/A\", but making \":/A\" (we need an\nextended syntax to differentiate from the singleton case we currently\nhave) to mean \"all the commits that match this pattern\" is something\npeople might be interested to see. Unfortunately, that does not define\na revision range operator but a operator that gives back set of commits\nthat are not consecutive, primarily good for giving to nothing but \"git\nshow\", and again \"git log --grep\" would emulate it just as well.\n\nSo in short:\n\n * I do not think extending it to mean a set of commits (with some\n definition of how the set is computed) is a good idea. It can stay\n \"name one commit that matches this string\" without losing usefulness,\n and I think it should;\n\n * The definition of the \"match\" can be tweaked and introducing regexp\n might be a good way;\n\n * The definition of the \"match\" may become more useful if we can limit\n which refs to dig from.\n"},{"id":"61808","messageId":"Pine.LNX.4.64.0712031841520.27959@racer.site","threadId":"11108","inReplyTo":"20071203173022.GA19219@coredump.intra.peff.net","subject":"[PATCH] Allow ':/' syntax to work with save_commit_buffer == 0","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2007-12-03T18:42:39Z","receivedAt":"2007-12-03T18:42:39Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"\nSigned-off-by: Johannes Schindelin \n---\n\n\tOn Mon, 3 Dec 2007, Jeff King wrote:\n\n\t> On Mon, Dec 03, 2007 at 10:55:15AM +0000, Johannes Schindelin wrote:\n\t> \n\t> > But then, my patch also works when save_commit_buffer == 0. \n\t> > But I can refactor this into its own patch, since it really is \n\t> > a separate issue.\n\t> \n\t> Agreed.\n\n\tHere we go.\n\n sha1_name.c | 19 ++++++++++++++++---\n 1 files changed, 16 insertions(+), 3 deletions(-)\n\ndiff --git a/sha1_name.c b/sha1_name.c\nindex e78b2b9..da1aad1 100644\n--- a/sha1_name.c\n+++ b/sha1_name.c\n@@ -601,24 +601,35 @@ static int get_sha1_oneline(const char *prefix, unsigned char *sha1)\n {\n \tstruct commit_list *list = NULL, *backup = NULL, *l;\n \tint retval = -1;\n+\tchar *temp_commit_buffer = NULL;\n \n \tif (prefix[0] == '!') {\n \t\tif (prefix[1] != '!')\n \t\t\tdie (\"Invalid search pattern: %s\", prefix);\n \t\tprefix++;\n \t}\n-\tif (!save_commit_buffer)\n-\t\treturn error(\"Could not expand oneline-name.\");\n \tfor_each_ref(handle_one_ref, &list);\n \tfor (l = list; l; l = l->next)\n \t\tcommit_list_insert(l->item, &backup);\n \twhile (list) {\n \t\tchar *p;\n \t\tstruct commit *commit;\n+\t\tenum object_type type;\n+\t\tunsigned long size;\n \n \t\tcommit = pop_most_recent_commit(&list, ONELINE_SEEN);\n \t\tparse_object(commit->object.sha1);\n-\t\tif (!commit->buffer || !(p = strstr(commit->buffer, \"\\n\\n\")))\n+\t\tif (temp_commit_buffer)\n+\t\t\tfree(temp_commit_buffer);\n+\t\tif (commit->buffer)\n+\t\t\tp = commit->buffer;\n+\t\telse {\n+\t\t\tp = read_sha1_file(commit->object.sha1, &type, &size);\n+\t\t\tif (!p)\n+\t\t\t\tcontinue;\n+\t\t\ttemp_commit_buffer = p;\n+\t\t}\n+\t\tif (!(p = strstr(p, \"\\n\\n\")))\n \t\t\tcontinue;\n \t\tif (!prefixcmp(p + 2, prefix)) {\n \t\t\thashcpy(sha1, commit->object.sha1);\n@@ -626,6 +637,8 @@ static int get_sha1_oneline(const char *prefix, unsigned char *sha1)\n \t\t\tbreak;\n \t\t}\n \t}\n+\tif (temp_commit_buffer)\n+\t\tfree(temp_commit_buffer);\n \tfree_commit_list(list);\n \tfor (l = backup; l; l = l->next)\n \t\tclear_commit_marks(l->item, ONELINE_SEEN);\n-- \n1.5.3.7.2119.g774e1\n"},{"id":"61823","messageId":"7vhcizh38y.fsf@gitster.siamese.dyndns.org","threadId":"11108","inReplyTo":"Pine.LNX.4.64.0712031841520.27959@racer.site","subject":"Re: [PATCH] Allow ':/' syntax to work with save_commit_buffer == 0","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-12-03T21:34:05Z","receivedAt":"2007-12-03T21:34:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> Signed-off-by: Johannes Schindelin \n> ---\n>\n> \tOn Mon, 3 Dec 2007, Jeff King wrote:\n>\n> \t> On Mon, Dec 03, 2007 at 10:55:15AM +0000, Johannes Schindelin wrote:\n> \t> \n> \t> > But then, my patch also works when save_commit_buffer == 0. \n> \t> > But I can refactor this into its own patch, since it really is \n> \t> > a separate issue.\n> \t> \n> \t> Agreed.\n>\n> \tHere we go.\n\nThe log message is a bit lacking. I would have expected \"git-show\ncommand can take ':/prefix' as an argument to name a commit whose\nsubject begins with the prefix, but git-foobar command didn't and\ninstead errored out. This fixes it so that ':/prefix' syntax can be\nused for any command that wants to take a commit object name\".\n"},{"id":"61832","messageId":"Pine.LNX.4.64.0712032249260.27959@racer.site","threadId":"11108","inReplyTo":"7vhcizh38y.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] Allow ':/' syntax to work with save_commit_buffer == 0","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2007-12-03T22:52:21Z","receivedAt":"2007-12-03T22:52:21Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Mon, 3 Dec 2007, Junio C Hamano wrote:\n\n> Johannes Schindelin writes:\n> \n> > Signed-off-by: Johannes Schindelin \n> > ---\n> >\n> > \tOn Mon, 3 Dec 2007, Jeff King wrote:\n> >\n> > \t> On Mon, Dec 03, 2007 at 10:55:15AM +0000, Johannes Schindelin wrote:\n> > \t> \n> > \t> > But then, my patch also works when save_commit_buffer == 0. \n> > \t> > But I can refactor this into its own patch, since it really is \n> > \t> > a separate issue.\n> > \t> \n> > \t> Agreed.\n> >\n> > \tHere we go.\n> \n> The log message is a bit lacking. I would have expected \"git-show\n> command can take ':/prefix' as an argument to name a commit whose\n> subject begins with the prefix, but git-foobar command didn't and\n> instead errored out. This fixes it so that ':/prefix' syntax can be\n> used for any command that wants to take a commit object name\".\n\nSorry. Can I ask you to amend the commit message with this?\n\n-- snip --\nEarlier, ':/' would not work (i.e. die) with commands that \nset save_commit_buffer = 0, such as blame, describe, pack-objects, reflog \nand bundle.\n\nFix this.\n-- snap --\n\nThanks,\nDscho\n"},{"id":"62107","messageId":"20071206055200.GA23309@coredump.intra.peff.net","threadId":"11108","inReplyTo":"7vbq97iqxd.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH/RFC] Use regex for :/ matching","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2007-12-06T05:52:00Z","receivedAt":"2007-12-06T05:52:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Dec 03, 2007 at 10:17:18AM -0800, Junio C Hamano wrote:\n\n> So in short:\n> \n> * I do not think extending it to mean a set of commits (with some\n> definition of how the set is computed) is a good idea. It can stay\n> \"name one commit that matches this string\" without losing usefulness,\n> and I think it should;\n> \n> * The definition of the \"match\" can be tweaked and introducing regexp\n> might be a good way;\n> \n> * The definition of the \"match\" may become more useful if we can limit\n> which refs to dig from.\n\nObviously the overall design and usage of :/ is going to take some\nthinking and is not 1.5.4 material. However, we do have it in its\ncurrent form, and I think regex versus prefix string matching is\northogonal to the range issues. Should I post my rebased :/ regex patch,\nor do you want to just leave it for post-1.5.4?\n\n-Peff\n"},{"id":"62116","messageId":"7vaboogwmq.fsf@gitster.siamese.dyndns.org","threadId":"11108","inReplyTo":"20071206055200.GA23309@coredump.intra.peff.net","subject":"Re: [PATCH/RFC] Use regex for :/ matching","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-12-06T06:33:49Z","receivedAt":"2007-12-06T06:33:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Obviously the overall design and usage of :/ is going to take some\n> thinking and is not 1.5.4 material. However, we do have it in its\n> current form, and I think regex versus prefix string matching is\n> orthogonal to the range issues. Should I post my rebased :/ regex patch,\n> or do you want to just leave it for post-1.5.4?\n\nMaking what was string match to regex using the same syntax is a\nregression, isn't it? I do not use :/ very often myself, so I\npersonally would not mind but people who are used to using :/ may get\nupset about the change. I do not feel strongly enough for changing it\nto regex to declare such a change unilaterally.\n"},{"id":"62118","messageId":"20071206063945.GB18698@coredump.intra.peff.net","threadId":"11108","inReplyTo":"7vaboogwmq.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH/RFC] Use regex for :/ matching","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2007-12-06T06:39:45Z","receivedAt":"2007-12-06T06:39:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Dec 05, 2007 at 10:33:49PM -0800, Junio C Hamano wrote:\n\n> Making what was string match to regex using the same syntax is a\n> regression, isn't it? I do not use :/ very often myself, so I\n> personally would not mind but people who are used to using :/ may get\n> upset about the change. I do not feel strongly enough for changing it\n> to regex to declare such a change unilaterally.\n\nYes, it is a regression, which is why my initial patch was labeled RFC.\nNobody commented on that aspect, so either they don't care, or (as you\noften seem to find out) they will just complain later. :)\n\nI am fine with adding it with a new syntax, but that is going to take\nsome thought, so perhaps it is better left to post-1.5.4.\n\n-Peff\n"},{"id":"62138","messageId":"Pine.LNX.4.64.0712061212590.27959@racer.site","threadId":"11108","inReplyTo":"20071206063945.GB18698@coredump.intra.peff.net","subject":"Re: [PATCH/RFC] Use regex for :/ matching","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2007-12-06T12:13:37Z","receivedAt":"2007-12-06T12:13:37Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Thu, 6 Dec 2007, Jeff King wrote:\n\n> On Wed, Dec 05, 2007 at 10:33:49PM -0800, Junio C Hamano wrote:\n> \n> > Making what was string match to regex using the same syntax is a \n> > regression, isn't it? I do not use :/ very often myself, so I \n> > personally would not mind but people who are used to using :/ may get \n> > upset about the change. I do not feel strongly enough for changing it \n> > to regex to declare such a change unilaterally.\n> \n> Yes, it is a regression, which is why my initial patch was labeled RFC. \n> Nobody commented on that aspect, [...]\n\nI did. I sent a link to my own version of that patch, thereby displaying \nmy agreement with the intent.\n\nCiao,\nDscho\n"}]}