threads / patch / 25993

patch, 2 parts[RFD] Using gitrevisions :/search style with other operators

Subject: [PATCH 0/2] [RFD] Using gitrevisions :/search style with other operators

## tl;dr

32 messages between Dec 8, 2010 and Dec 10, 2010. Diffs are folded; open one to read it.

replies: 31people: 7as markdown or json

Nguyễn Thái Ngọc Duy· Dec 8, 2010, 14:58 UTC · lore

Let's start off from where the previous discussion [1] stopped. People seem to agree ref^{/regex} is a good choice. But we have not come to conclusion how to specify the count yet. Possible suggestions are

 - ref^{/foo}2
 - ref^{2/foo}
 - ref^{:2/foo}
 - ref^{2nd/foo}

For whatever syntax chosen, :/ should benefit too. I notice that :/! is reserved for future use. Perhaps :/!2/regex is not too cryptic?

I'd also like to do case-insensitive regex, by the way. :/!2i/regex looks a bit ugly.

[1] http://mid.gmane.org/9D675671-693D-4B59-AF2A-0EFE4C537362@sb.org
Nguyễn Thái Ngọc Duy (2):
  get_sha1_oneline: allow to input commit_list
  get_sha1: support ref^{/regex} syntax
 Documentation/revisions.txt |    7 ++++++
 sha1_name.c                 |   45 ++++++++++++++++++++++++++++++++----------
 2 files changed, 41 insertions(+), 11 deletions(-)
-- 
1.7.3.2.316.gda8b3
Nguyễn Thái Ngọc Duy· Dec 8, 2010, 14:58 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH 1/2] get_sha1_oneline: allow to input commit_list

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 sha1_name.c |   19 ++++++++++++-------
 1 files changed, 12 insertions(+), 7 deletions(-)
Show changes to sha1_name.c +12 −7
diff --git a/sha1_name.c b/sha1_name.c
index 2c3a5fb..f4ccdc5 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -686,13 +686,14 @@ static int handle_one_ref(const char *path,
 	if (object->type != OBJ_COMMIT)
 		return 0;
 	insert_by_date((struct commit *)object, list);
-	object->flags |= ONELINE_SEEN;
 	return 0;
 }
 
-static int get_sha1_oneline(const char *prefix, unsigned char *sha1)
+static int get_sha1_oneline(const char *prefix,
+			    unsigned char *sha1,
+			    struct commit_list *list_)
 {
-	struct commit_list *list = NULL, *backup = NULL, *l;
+	struct commit_list *list = list_, *backup = NULL, *l;
 	int retval = -1;
 	char *temp_commit_buffer = NULL;
 	regex_t regex;
@@ -706,9 +707,12 @@ static int get_sha1_oneline(const char *prefix, unsigned char *sha1)
 	if (regcomp(&regex, prefix, REG_EXTENDED))
 		die("Invalid search pattern: %s", prefix);
 
-	for_each_ref(handle_one_ref, &list);
-	for (l = list; l; l = l->next)
+	if (!list)
+		for_each_ref(handle_one_ref, &list);
+	for (l = list; l; l = l->next) {
 		commit_list_insert(l->item, &backup);
+		l->item->object.flags |= ONELINE_SEEN;
+	}
 	while (list) {
 		char *p;
 		struct commit *commit;
@@ -737,7 +741,8 @@ static int get_sha1_oneline(const char *prefix, unsigned char *sha1)
 	}
 	regfree(&regex);
 	free(temp_commit_buffer);
-	free_commit_list(list);
+	if (!list_)
+		free_commit_list(list);
 	for (l = backup; l; l = l->next)
 		clear_commit_marks(l->item, ONELINE_SEEN);
 	return retval;
@@ -1090,7 +1095,7 @@ int get_sha1_with_context_1(const char *name, unsigned char *sha1,
 		int pos;
 		if (namelen > 2 && name[1] == '/')
 			/* don't need mode for commit */
-			return get_sha1_oneline(name + 2, sha1);
+			return get_sha1_oneline(name + 2, sha1, NULL);
 		if (namelen < 3 ||
 		    name[2] != ':' ||
 		    name[1] < '0' || '3' < name[1])
-- 
1.7.3.2.316.gda8b3
Thiago Farina· Dec 8, 2010, 15:11 UTC · re: Nguyễn Thái Ngọc Duy · lore

Re: [PATCH 1/2] get_sha1_oneline: allow to input commit_list

2010/12/8 Nguyễn Thái Ngọc Duy <pclouds@gmail.com>:
Show 7 quoted lines
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> -static int get_sha1_oneline(const char *prefix, unsigned char *sha1)
> +static int get_sha1_oneline(const char *prefix,
> +                           unsigned char *sha1,
> +                           struct commit_list *list_)
>  {

micronit: can we have a better name for |list_|, the suffix _ is very ugly and uncommon :(

Nguyễn Thái Ngọc Duy· Dec 8, 2010, 14:58 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH 2/2] get_sha1: support ref^{/regex} syntax

This works like :/ syntax, but only limited to one ref.
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 Documentation/revisions.txt |    7 +++++++
 sha1_name.c                 |   26 ++++++++++++++++++++++----
 2 files changed, 29 insertions(+), 4 deletions(-)
Show changes to 2 files +29 −4

Documentation/revisions.txt, sha1_name.c

diff --git a/Documentation/revisions.txt b/Documentation/revisions.txt
index 3d4b79c..fbe6245 100644
--- a/Documentation/revisions.txt
+++ b/Documentation/revisions.txt
@@ -106,6 +106,13 @@ the `$GIT_DIR/refs` directory or from the `$GIT_DIR/packed-refs` file.
   and dereference the tag recursively until a non-tag object is
   found.
 
+* A suffix '{caret}' to a revision parameter followed by a brace
+  pair that contains a text led by a slash (e.g. `HEAD^{/fix nasty bug}`):
+  this names a commit whose commit message matches the specified
+  regular expression. This name returns the youngest matching commit
+  which is reachable from the dereferenced commit. The leading '!'
+  in the text is treated especially like in `:/` syntax below.
+
 * A colon, followed by a slash, followed by a text (e.g. `:/fix nasty bug`): this names
   a commit whose commit message matches the specified regular expression.
   This name returns the youngest matching commit which is
diff --git a/sha1_name.c b/sha1_name.c
index f4ccdc5..00e52b0 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -527,6 +527,7 @@ struct object *peel_to_type(const char *name, int namelen,
 	}
 }
 
+static int get_sha1_oneline(const char *, unsigned char *, struct commit_list *);
 static int peel_onion(const char *name, int len, unsigned char *sha1)
 {
 	unsigned char outer[20];
@@ -562,6 +563,11 @@ static int peel_onion(const char *name, int len, unsigned char *sha1)
 		expected_type = OBJ_BLOB;
 	else if (sp[0] == '}')
 		expected_type = OBJ_NONE;
+	else if (sp[0] == '/') {
+		if (sp[1] == '}')
+			return -1;
+		expected_type = OBJ_COMMIT;
+	}
 	else
 		return -1;
 
@@ -584,11 +590,23 @@ static int peel_onion(const char *name, int len, unsigned char *sha1)
 		 * barf.
 		 */
 		o = peel_to_type(name, len, o, expected_type);
-		if (o) {
-			hashcpy(sha1, o->sha1);
-			return 0;
+		if (!o)
+			return -1;
+
+		hashcpy(sha1, o->sha1);
+		if (sp[0] == '/') { /* ^{/foo} */
+			struct commit_list *list = NULL;
+			char *prefix;
+			int ret;
+
+			commit_list_insert((struct commit *)o, &list);
+			prefix = xstrndup(sp + 1, name + len - 1 - (sp + 1));
+			ret = get_sha1_oneline(prefix, sha1, list);
+			free(prefix);
+			free_commit_list(list);
+			return ret;
 		}
-		return -1;
+		return 0;
 	}
 	return 0;
 }
-- 
1.7.3.2.316.gda8b3
Junio C Hamano· Dec 8, 2010, 22:50 UTC · re: Nguyễn Thái Ngọc Duy · lore

Re: [PATCH 2/2] get_sha1: support ref^{/regex} syntax

Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
Show 11 quoted lines
> diff --git a/sha1_name.c b/sha1_name.c
> index f4ccdc5..00e52b0 100644
> --- a/sha1_name.c
> +++ b/sha1_name.c
> @@ -562,6 +563,11 @@ static int peel_onion(const char *name, int len, unsigned char *sha1)
>  		expected_type = OBJ_BLOB;
>  	else if (sp[0] == '}')
>  		expected_type = OBJ_NONE;
> +	else if (sp[0] == '/') {
> +		if (sp[1] == '}')
> +			return -1;

Why? $commit^{/} may be a no-op but I do not see a strong reason to waste extra two lines to forbid it.

Show 9 quoted lines
> @@ -584,11 +590,23 @@ static int peel_onion(const char *name, int len, unsigned char *sha1)
>  		 * barf.
>  		 */
>  		o = peel_to_type(name, len, o, expected_type);
> -		if (o) {
> -			hashcpy(sha1, o->sha1);
> -			return 0;
> +		if (!o)
> +			return -1;
I can see you are trying to reduce nesting of
        if (o) {
		do true thing
                return 0
	}
        return -1;

but then we should apply the same to outer "if (!expected_type) ... else", too, to unnest the "else" clause by returning from the true branch of that "if".

Jonathan Nieder· Dec 8, 2010, 18:06 UTC · re: Nguyễn Thái Ngọc Duy · lore

Re: [PATCH 0/2] [RFD] Using gitrevisions :/search style with other operators

Nguyễn Thái Ngọc Duy wrote:
Show 8 quoted lines
> Let's start off from where the previous discussion [1] stopped. People
> seem to agree ref^{/regex} is a good choice. But we have not come to
> conclusion how to specify the count yet. Possible suggestions are
>
>  - ref^{/foo}2
>  - ref^{2/foo}
>  - ref^{:2/foo}
>  - ref^{2nd/foo}
How about
	ref^{/foo}^^{/foo}
?
Jakub Narebski· Dec 8, 2010, 19:51 UTC · re: Jonathan Nieder · lore

Re: [PATCH 0/2] [RFD] Using gitrevisions :/search style with other operators

Dnia środa 8. grudnia 2010 19:06, Jonathan Nieder napisał:
Show 16 quoted lines
> Nguyễn Thái Ngọc Duy wrote:
> 
> > Let's start off from where the previous discussion [1] stopped. People
> > seem to agree ref^{/regex} is a good choice. But we have not come to
> > conclusion how to specify the count yet. Possible suggestions are
> >
> >  - ref^{/foo}2
> >  - ref^{2/foo}
> >  - ref^{:2/foo}
> >  - ref^{2nd/foo}
> 
> How about
> 
> 	ref^{/foo}^^{/foo}
> 
> ?
I'll assume that there is invisible ";)" emoticon here.

First, it would be ref^{/foo}^@^{/foo}, otherwise you would follow only first parent.

Second, consider ref^{:nth(10)/foo} in your workaround...
;-)
-- 
Jakub Narebski
Poland
Nguyen Thai Ngoc Duy· Dec 9, 2010, 01:28 UTC · re: Jakub Narebski · lore

Re: [PATCH 0/2] [RFD] Using gitrevisions :/search style with other operators

On Thu, Dec 9, 2010 at 2:51 AM, Jakub Narebski <jnareb@gmail.com> wrote:
Show 25 quoted lines
> Dnia środa 8. grudnia 2010 19:06, Jonathan Nieder napisał:
>> Nguyễn Thái Ngọc Duy wrote:
>>
>> > Let's start off from where the previous discussion [1] stopped. People
>> > seem to agree ref^{/regex} is a good choice. But we have not come to
>> > conclusion how to specify the count yet. Possible suggestions are
>> >
>> >  - ref^{/foo}2
>> >  - ref^{2/foo}
>> >  - ref^{:2/foo}
>> >  - ref^{2nd/foo}
>>
>> How about
>>
>>       ref^{/foo}^^{/foo}
>>
>> ?
>
> I'll assume that there is invisible ";)" emoticon here.
>
>
> First, it would be ref^{/foo}^@^{/foo}, otherwise you would follow only
> first parent.
>
> Second, consider ref^{:nth(10)/foo} in your workaround...

Maybe we should generalize this to apply to all operators. Currently foo~3 is expanded to foo^^^. How about ~~X (or xN) denote repeat the last operator N times? For example, HEAD^2x3 => HEAD^2^2^2, HEAD^{/foo}x3 => HEAD^{/foo}^{/foo}^{/foo}.

-- 
Duy
Jakub Narebski· Dec 9, 2010, 01:54 UTC · re: Nguyen Thai Ngoc Duy · lore

Re: [PATCH 0/2] [RFD] Using gitrevisions :/search style with other operators

Nguyen Thai Ngoc Duy wrote:
Show 31 quoted lines
> On Thu, Dec 9, 2010 at 2:51 AM, Jakub Narebski <jnareb@gmail.com> wrote:
>> Dnia środa 8. grudnia 2010 19:06, Jonathan Nieder napisał:
>>> Nguyễn Thái Ngọc Duy wrote:
>>>
>>>> Let's start off from where the previous discussion [1] stopped. People
>>>> seem to agree ref^{/regex} is a good choice. But we have not come to
>>>> conclusion how to specify the count yet. Possible suggestions are
>>>>
>>>>  - ref^{/foo}2
>>>>  - ref^{2/foo}
>>>>  - ref^{:2/foo}
>>>>  - ref^{2nd/foo}
>>>
>>> How about
>>>
>>>       ref^{/foo}^^{/foo}
>>>
>>> ?
>>
>> I'll assume that there is invisible ";)" emoticon here.
>>
>>
>> First, it would be ref^{/foo}^@^{/foo}, otherwise you would follow only
>> first parent.
>>
>> Second, consider ref^{:nth(10)/foo} in your workaround...
> 
> Maybe we should generalize this to apply to all operators. Currently
> foo~3 is expanded to foo^^^. How about ~~X (or xN) denote repeat the
> last operator N times? For example, HEAD^2x3 => HEAD^2^2^2,
> HEAD^{/foo}x3 => HEAD^{/foo}^{/foo}^{/foo}.

Unless you allow grouping, it wouldn't help in the case of ^{/foo}, because ^{/foo} is idempotent. HEAD^{/foo} finds first commit that contains "foo", and HEAD^{/foo}^{/foo} finds first commit containing "foo" starting from *and including* first commit from HEAD containing "foo" - which is HEAD^{/foo}

  HEAD^{/foo}^{/foo} === HEAD^{/foo}

You would need HEAD{^{/foo}^@}x3, or use special rule that HEAD^{/foo}x2 means really HEAD^{/foo}^@^{/foo}, with ^@ used to join them.

-- 
Jakub Narebski
Poland
Jonathan Nieder· Dec 9, 2010, 01:59 UTC · re: Jakub Narebski · lore

Re: [PATCH 0/2] [RFD] Using gitrevisions :/search style with other operators

Jakub Narebski wrote:
> You would need HEAD{^{/foo}^@}x3, or use special rule that HEAD^{/foo}x2
> means really HEAD^{/foo}^@^{/foo}, with ^@ used to join them.

That said, does ^2x500 really do something meaningful that a person would ever need? I like the

	^{:nth(3)/foo}

syntax because perl6 supports m:nth(3)/foo/, suggesting a menu of already-defined modifiers to implement when they prove useful, known already to a certain subset of the audience and proven useful already in a different context.

Kevin Ballard· Dec 9, 2010, 02:02 UTC · re: Jonathan Nieder · lore

Re: [PATCH 0/2] [RFD] Using gitrevisions :/search style with other operators

On Dec 8, 2010, at 5:59 PM, Jonathan Nieder wrote:
Show 14 quoted lines
> Jakub Narebski wrote:
> 
>> You would need HEAD{^{/foo}^@}x3, or use special rule that HEAD^{/foo}x2
>> means really HEAD^{/foo}^@^{/foo}, with ^@ used to join them.
> 
> That said, does ^2x500 really do something meaningful that a person
> would ever need?  I like the
> 
> 	^{:nth(3)/foo}
> 
> syntax because perl6 supports m:nth(3)/foo/, suggesting a menu of
> already-defined modifiers to implement when they prove useful, known
> already to a certain subset of the audience and proven useful already
> in a different context.

I like the ^{:nth(3)/foo} syntax as well. Though I'm not familiar with Perl 6, this does have the benefit of being fairly obvious to the reader as to what it means.

-Kevin Ballard
Nguyen Thai Ngoc Duy· Dec 9, 2010, 02:06 UTC · re: Kevin Ballard · lore

Re: [PATCH 0/2] [RFD] Using gitrevisions :/search style with other operators

On Thu, Dec 9, 2010 at 9:02 AM, Kevin Ballard <kevin@sb.org> wrote:
Show 20 quoted lines
> On Dec 8, 2010, at 5:59 PM, Jonathan Nieder wrote:
>
>> Jakub Narebski wrote:
>>
>>> You would need HEAD{^{/foo}^@}x3, or use special rule that HEAD^{/foo}x2
>>> means really HEAD^{/foo}^@^{/foo}, with ^@ used to join them.
>>
>> That said, does ^2x500 really do something meaningful that a person
>> would ever need?  I like the
>>
>>       ^{:nth(3)/foo}
>>
>> syntax because perl6 supports m:nth(3)/foo/, suggesting a menu of
>> already-defined modifiers to implement when they prove useful, known
>> already to a certain subset of the audience and proven useful already
>> in a different context.
>
> I like the ^{:nth(3)/foo} syntax as well. Though I'm not familiar with Perl 6,
> this does have the benefit of being fairly obvious to the reader as to what it
> means.
OK so :nth(3)/foo for all branches?
-- 
Duy
Jonathan Nieder· Dec 9, 2010, 02:11 UTC · re: Nguyen Thai Ngoc Duy · lore

Re: [PATCH 0/2] [RFD] Using gitrevisions :/search style with other operators

Nguyen Thai Ngoc Duy wrote:
> OK so :nth(3)/foo for all branches?

That steals namespace from "the path 'nth(3)/foo' in the index". But is "the third instance of foo in all branches" something that needs to be possible to say? Branches do not have a well defined order, anyway. A command to list all commits with "foo" in the subject like

	git log --oneline --grep-subject=foo

sounds more useful (assuming --grep=foo yields too many false positives).

Junio C Hamano· Dec 9, 2010, 06:22 UTC · re: Jonathan Nieder · lore

Re: [PATCH 0/2] [RFD] Using gitrevisions :/search style with other operators

Jonathan Nieder <jrnieder@gmail.com> writes:
Show 12 quoted lines
> Jakub Narebski wrote:
>
>> You would need HEAD{^{/foo}^@}x3, or use special rule that HEAD^{/foo}x2
>> means really HEAD^{/foo}^@^{/foo}, with ^@ used to join them.
>
> That said, does ^2x500 really do something meaningful that a person
> would ever need?  I like the
>
> 	^{:nth(3)/foo}
>
> syntax because perl6 supports m:nth(3)/foo/, suggesting a menu of
> already-defined modifiers to implement when they prove useful,...
Can you explain what the colon in "$commit^{:nth(3)/foo}" is doing?  

Are we declaring anything that begins with ':' is a magic inside ^{...} construct?

I do not think nth($n) without specifying where to start (iow, what the current ":/foo" implementation does but with "three levels deep") makes any sense, but because the main point of your argument is that we can have modifies other than nth($n) that may make sense in such a context, I would want to make sure anything we come up with is extensible to that syntax.

On the "starting from any ref" front, I think "!" (as in ":/!some magic") was the introducer we reserved for such a magic some time ago, so perhaps on the "starting from this commit" side, "^{!magic/foo}" may be more appropriate?

Jakub Narebski· Dec 9, 2010, 11:38 UTC · re: Junio C Hamano · lore

Re: [PATCH 0/2] [RFD] Using gitrevisions :/search style with other operators

On Thu, 9 Dec 2010, Junio C Hamano wrote:
Show 18 quoted lines
> Jonathan Nieder <jrnieder@gmail.com> writes:
>> Jakub Narebski wrote:
>>
>>> You would need HEAD{^{/foo}^@}x3, or use special rule that HEAD^{/foo}x2
>>> means really HEAD^{/foo}^@^{/foo}, with ^@ used to join them.
>>
>> That said, does ^2x500 really do something meaningful that a person
>> would ever need?  I like the
>>
>> 	^{:nth(3)/foo}
>>
>> syntax because perl6 supports m:nth(3)/foo/, suggesting a menu of
>> already-defined modifiers to implement when they prove useful,...
> 
> Can you explain what the colon in "$commit^{:nth(3)/foo}" is doing?  
> 
> Are we declaring anything that begins with ':' is a magic inside ^{...}
> construct?
The idea is to follow Perl 6 rule of combining modifiers "Every modifier
must start with its own colon."
  http://perlcabal.org/syn/S05.html#Modifiers
This allow for easy combining modifiers without introducing special rules,
for example ^{:g:i/foo} would return all matches (:g == :global) instead
of only the first, ignoring case (:i == :ignorecase).
 
Show 5 quoted lines
> I do not think nth($n) without specifying where to start (iow, what the
> current ":/foo" implementation does but with "three levels deep") makes
> any sense, but because the main point of your argument is that we can have
> modifies other than nth($n) that may make sense in such a context, I would
> want to make sure anything we come up with is extensible to that syntax.

Hmmm... I haven't thought about it. I guess that ^{:nth(N)/foo} should return Nth element in the list that ^{:g/foo} / --grep=foo would return.

This way one could use
  $ git log pu@{1}..pu --merges --grep="Merge branch 'nd/"
to find all new Nguyen contributions in 'pu', and
  $ git log pu..pu^{:2nd/Merge branch 'nd/}^2
to view 2nd (2nd == nth(2)) such branch on the list.
Show 5 quoted lines
> 
> On the "starting from any ref" front, I think "!" (as in ":/!some magic")
> was the introducer we reserved for such a magic some time ago, so perhaps
> on the "starting from this commit" side, "^{!magic/foo}" may be more
> appropriate?

I'd like to deprecate ":/foo" in favor of "--all ^{/foo}", or even special case it to "^{/foo}" (if not attached to ref, ^{(:<modifier>)*/<regexp>} acts on positive revs replacing them).

But ^{/foo} might be overengineered...
-- 
Jakub Narebski
Poland
Nguyen Thai Ngoc Duy· Dec 10, 2010, 13:25 UTC · re: Junio C Hamano · lore

Re: [PATCH 0/2] [RFD] Using gitrevisions :/search style with other operators

On Thu, Dec 9, 2010 at 1:22 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 25 quoted lines
> Jonathan Nieder <jrnieder@gmail.com> writes:
>
>> Jakub Narebski wrote:
>>
>>> You would need HEAD{^{/foo}^@}x3, or use special rule that HEAD^{/foo}x2
>>> means really HEAD^{/foo}^@^{/foo}, with ^@ used to join them.
>>
>> That said, does ^2x500 really do something meaningful that a person
>> would ever need?  I like the
>>
>>       ^{:nth(3)/foo}
>>
>> syntax because perl6 supports m:nth(3)/foo/, suggesting a menu of
>> already-defined modifiers to implement when they prove useful,...
>
> Can you explain what the colon in "$commit^{:nth(3)/foo}" is doing?
>
> Are we declaring anything that begins with ':' is a magic inside ^{...}
> construct?
>
> I do not think nth($n) without specifying where to start (iow, what the
> current ":/foo" implementation does but with "three levels deep") makes
> any sense, but because the main point of your argument is that we can have
> modifies other than nth($n) that may make sense in such a context, I would
> want to make sure anything we come up with is extensible to that syntax.

There's also another similar operation: note search. I don't think we need to invent another syntax for note search, a modifier for ^{/foo} may be a good choice.

> On the "starting from any ref" front, I think "!" (as in ":/!some magic")
> was the introducer we reserved for such a magic some time ago, so perhaps
> on the "starting from this commit" side, "^{!magic/foo}" may be more
> appropriate?

Can we use ! modifier for other ^{} too? What I have in mind is how to say ^{commit} that has two parents. Or even better, "search from the given tip for a commit that has two parents and the commit message matches 'foo'". Hmm.. too complex. Perhaps "^{grep: <grep arguments>}" that pulls the whole git-grep functionality in.

-- 
Duy
Jonathan Nieder· Dec 10, 2010, 19:03 UTC · re: Nguyen Thai Ngoc Duy · lore

Re: [PATCH 0/2] [RFD] Using gitrevisions :/search style with other operators

Nguyen Thai Ngoc Duy wrote:
Show 5 quoted lines
> Can we use ! modifier for other ^{} too? What I have in mind is how to
> say ^{commit} that has two parents. Or even better, "search from the
> given tip for a commit that has two parents and the commit message
> matches 'foo'". Hmm.. too complex. Perhaps "^{grep: <grep arguments>}"
> that pulls the whole git-grep functionality in.
My thoughts, in no particular order:
 - '!' can be a pain in the neck to supply on the bash command line.
   Single quotes and backslash quoting work while double quotes do
   not, unless 'set +H' has been run.  But that's not a huge deal and
   arguably it's a bash misfeature.
 - What is the intended use for this family of modifiers?  I sort
   of understand ^{:i/... } for people that forget what case they
   have used, but why the :nth and others?
 - Why do we have to carve out the namespace right away, anyway?  If
   we just ^{/... } for the ordinary "start here" search, that leaves
   room for anything after the { other than "/<pattern>" and a few
   fixed strings like "upstream", "tree", etc, right?  It might be
   easier to anticipate what syntax will be useful when there is
   functionality to go with it.
Jakub Narebski· Dec 10, 2010, 19:26 UTC · re: Jonathan Nieder · lore

Re: [PATCH 0/2] [RFD] Using gitrevisions :/search style with other operators

On Fri, 10 Dec 2010 20:03, Jonathan Nieder wrote:
Show 18 quoted lines
> Nguyen Thai Ngoc Duy wrote:
> 
> > Can we use ! modifier for other ^{} too? What I have in mind is how to
> > say ^{commit} that has two parents. Or even better, "search from the
> > given tip for a commit that has two parents and the commit message
> > matches 'foo'". Hmm.. too complex. Perhaps "^{grep: <grep arguments>}"
> > that pulls the whole git-grep functionality in.
> 
> My thoughts, in no particular order:
> 
>  - '!' can be a pain in the neck to supply on the bash command line.
>    Single quotes and backslash quoting work while double quotes do
>    not, unless 'set +H' has been run.  But that's not a huge deal and
>    arguably it's a bash misfeature.
> 
>  - What is the intended use for this family of modifiers?  I sort
>    of understand ^{:i/... } for people that forget what case they
>    have used, but why the :nth and others?
* :nth(N) and :g, to show nth match and all matches, respectively; might
  be not necessary as it impinges a bit on --grep=<regexp> domain
* :N / :notesRef to search also through attached notes
If we was possible, perhaps also
* :b / :basechar to ignore accents and other marks (according to commit
  encoding).
-- 
Jakub Narebski
Poland
Kevin Ballard· Dec 10, 2010, 21:21 UTC · re: Jonathan Nieder · lore

Re: [PATCH 0/2] [RFD] Using gitrevisions :/search style with other operators

On Dec 10, 2010, at 11:03 AM, Jonathan Nieder wrote:
> - What is the intended use for this family of modifiers?  I sort
>   of understand ^{:i/... } for people that forget what case they
>   have used, but why the :nth and others?

In my particular case, I was glancing through the logs, and I wanted to grab the second branch that someone else had made that was merged into pu. I would have loved to be able to run something like

  git merge origin/pu^{:nth(2)/nd/}

While we're speaking of modifiers, could we use one that says "only search the first parent hierarchy", e.g. something equivalent to git log's --first-parent flag?

-Kevin Ballard
Jeff King· Dec 10, 2010, 21:30 UTC · re: Kevin Ballard · lore

Re: [PATCH 0/2] [RFD] Using gitrevisions :/search style with other operators

On Fri, Dec 10, 2010 at 01:21:15PM -0800, Kevin Ballard wrote:
Show 15 quoted lines
> On Dec 10, 2010, at 11:03 AM, Jonathan Nieder wrote:
> 
> > - What is the intended use for this family of modifiers?  I sort
> >   of understand ^{:i/... } for people that forget what case they
> >   have used, but why the :nth and others?
> 
> In my particular case, I was glancing through the logs, and I wanted to grab
> the second branch that someone else had made that was merged into pu. I would
> have loved to be able to run something like
> 
>   git merge origin/pu^{:nth(2)/nd/}
> 
> While we're speaking of modifiers, could we use one that says "only search
> the first parent hierarchy", e.g. something equivalent to git log's --first-parent
> flag?

As neat as this modifier syntax is getting, are we perhaps just recreating the wheel?

How about:
  git merge `git rev-list -2 --grep=nd/ origin/pu | tail -1`
for the nth one, and:
  git merge `git rev-list --first-parent -1 --grep=nd/ origin/pu`

for a first parent search (I will leave combining them as an exercise to the reader).

It's not that I'm opposed to a handy ref-specifying syntax. I just wonder if it is really worth building in all of these obscure scenarios.

-Peff
Junio C Hamano· Dec 10, 2010, 23:08 UTC · re: Kevin Ballard · lore

Re: [PATCH 0/2] [RFD] Using gitrevisions :/search style with other operators

Kevin Ballard <kevin@sb.org> writes:
Show 15 quoted lines
> On Dec 10, 2010, at 11:03 AM, Jonathan Nieder wrote:
>
>> - What is the intended use for this family of modifiers?  I sort
>>   of understand ^{:i/... } for people that forget what case they
>>   have used, but why the :nth and others?
>
> In my particular case, I was glancing through the logs, and I wanted to grab
> the second branch that someone else had made that was merged into pu. I would
> have loved to be able to run something like
>
>   git merge origin/pu^{:nth(2)/nd/}
>
> While we're speaking of modifiers, could we use one that says "only search
> the first parent hierarchy", e.g. something equivalent to git log's --first-parent
> flag?
Both feels like a very made-up example to me.

The reason you can so sure that you can to give nth(2) not nth(3) nor nth(1) and run "merge" in the example is probably because you looked at the output from "git log --first-parent --oneline origin..origin/pu", no?

  d414638 Merge branch 'rj/msvc-fix' into pu
  e5f5e49 Merge branch 'ak/describe-exact' into pu
  439932d Merge branch 'jn/svn-fe' into pu
  d60b33b Merge branch 'pd/bash-4-completion' into pu
  09cbbde Merge branch 'tf/commit-list-prefix' into pu
  1b2ea00 Merge branch 'mg/cvsimport' into pu
  c6d41f4 Merge branch 'nd/maint-relative' into pu
  81f395e Merge branch 'ab/i18n' into pu
  9f5471f Merge branch 'nd/setup' into pu
  06f74a4 Merge branch 'yd/dir-rename' into pu
  d8a2ec8 Merge branch 'en/object-list-with-pathspec' into pu

After looking at this output, do you really want to say ":nth(2)/nd/" instead of 9f5471f?

To come up with the "(2)" part you need to carefully scan the other lines and make sure that there is only one "nd/" after what you want, and the string does not appear in an unexpected places.

Kevin Ballard· Dec 10, 2010, 23:11 UTC · re: Junio C Hamano · lore

Re: [PATCH 0/2] [RFD] Using gitrevisions :/search style with other operators

On Dec 10, 2010, at 3:08 PM, Junio C Hamano wrote:
Show 19 quoted lines
> Kevin Ballard <kevin@sb.org> writes:
> 
>> On Dec 10, 2010, at 11:03 AM, Jonathan Nieder wrote:
>> 
>>> - What is the intended use for this family of modifiers?  I sort
>>>  of understand ^{:i/... } for people that forget what case they
>>>  have used, but why the :nth and others?
>> 
>> In my particular case, I was glancing through the logs, and I wanted to grab
>> the second branch that someone else had made that was merged into pu. I would
>> have loved to be able to run something like
>> 
>>  git merge origin/pu^{:nth(2)/nd/}
>> 
>> While we're speaking of modifiers, could we use one that says "only search
>> the first parent hierarchy", e.g. something equivalent to git log's --first-parent
>> flag?
> 
> Both feels like a very made-up example to me.

And yet the example I gave is pretty much precisely what prompted me to start this discussion in the first place.

Show 18 quoted lines
> The reason you can so sure that you can to give nth(2) not nth(3) nor
> nth(1) and run "merge" in the example is probably because you looked at
> the output from "git log --first-parent --oneline origin..origin/pu", no?
> 
>  d414638 Merge branch 'rj/msvc-fix' into pu
>  e5f5e49 Merge branch 'ak/describe-exact' into pu
>  439932d Merge branch 'jn/svn-fe' into pu
>  d60b33b Merge branch 'pd/bash-4-completion' into pu
>  09cbbde Merge branch 'tf/commit-list-prefix' into pu
>  1b2ea00 Merge branch 'mg/cvsimport' into pu
>  c6d41f4 Merge branch 'nd/maint-relative' into pu
>  81f395e Merge branch 'ab/i18n' into pu
>  9f5471f Merge branch 'nd/setup' into pu
>  06f74a4 Merge branch 'yd/dir-rename' into pu
>  d8a2ec8 Merge branch 'en/object-list-with-pathspec' into pu
> 
> After looking at this output, do you really want to say ":nth(2)/nd/"
> instead of 9f5471f?

Yep. Doing the latter either requires me to swap over to my mouse, copy the sha1, and paste in, or requires me to peer at the sha1 and re-type enough characters. It's a lot easier to just glance at that list, realize the 2nd one is the one I want, and type `git merge :^{nth(2)/nd/}`. It may not necessarily be faster than retyping the sha1, but it's a lot less prone to transcription errors.

-Kevin Ballard
Junio C Hamano· Dec 10, 2010, 23:36 UTC · re: Kevin Ballard · lore

Re: [PATCH 0/2] [RFD] Using gitrevisions :/search style with other operators

Kevin Ballard <kevin@sb.org> writes:
Show 19 quoted lines
> On Dec 10, 2010, at 3:08 PM, Junio C Hamano wrote:
> ...
>>  d414638 Merge branch 'rj/msvc-fix' into pu
>>  ...
>>  1b2ea00 Merge branch 'mg/cvsimport' into pu
>>  c6d41f4 Merge branch 'nd/maint-relative' into pu
>>  81f395e Merge branch 'ab/i18n' into pu
>>  9f5471f Merge branch 'nd/setup' into pu
>>  06f74a4 Merge branch 'yd/dir-rename' into pu
>>  d8a2ec8 Merge branch 'en/object-list-with-pathspec' into pu
>> 
>> After looking at this output, do you really want to say ":nth(2)/nd/"
>> instead of 9f5471f?
>
> Yep. Doing the latter either requires me to swap over to my mouse, copy the sha1,
> and paste in, or requires me to peer at the sha1 and re-type enough characters.
> It's a lot easier to just glance at that list, realize the 2nd one is the one I
> want, and type `git merge :^{nth(2)/nd/}`. It may not necessarily be faster than
> retyping the sha1, but it's a lot less prone to transcription errors.

What you said heavily depends on the way in which I give names to the branches, and also on the fact that "nd/" happens to be not very common prefix at this moment. If the branches were named without nd/ part and still be unique, you would not be arguing for nth(2) at all to begin with.

So it is dubious that your argument is convincing.
Junio C Hamano· Dec 9, 2010, 05:15 UTC · re: Jakub Narebski · lore

Re: [PATCH 0/2] [RFD] Using gitrevisions :/search style with other operators

Jakub Narebski <jnareb@gmail.com> writes:
> Second, consider ref^{:nth(10)/foo} in your workaround...
Feels way over-engineered to me.
Jakub Narebski· Dec 8, 2010, 19:47 UTC · re: Nguyễn Thái Ngọc Duy · lore

Re: [PATCH 0/2] [RFD] Using gitrevisions :/search style with other operators

On Wed, 8 Dec 2010, Nguyễn Thái Ngọc Duy wrote:
Show 11 quoted lines
> Let's start off from where the previous discussion [1] stopped. People
> seem to agree ref^{/regex} is a good choice. But we have not come to
> conclusion how to specify the count yet. Possible suggestions are
> 
>  - ref^{/foo}2
>  - ref^{2/foo}
>  - ref^{:2/foo}
>  - ref^{2nd/foo}
> 
> For whatever syntax chosen, :/ should benefit too. I notice that :/!
> is reserved for future use. Perhaps :/!2/regex is not too cryptic?

I wonder if it would be possible to make :/<regex> (which looks a bit like searching the index) to be an alias to --all^{/<regex>}...

Or if we can make ^{/<regex>} to act on revision range specified by earlier commits, so for example foo..bar^{/<regex>} would work.

> I'd also like to do case-insensitive regex, by the way. :/!2i/regex
> looks a bit ugly.

The '2nd' idea came from Perl 6 regexp / grammars, see for example https://github.com/perlpilot/perl6-docs/blob/master/intro/p6-regex-intro.pod

 There are two other modifiers for matching a pattern some number of times
 or only matching, say, the third time we see a pattern in a string. These
 modifiers are a little strange in that their short-hand forms consist of
 a number followed by some text:
    modifier        short-hand              meaning
    :x()            :1x,:4x,:12x            match some number of times
    :nth()          :1st,:2nd,:3rd,:4th     match only the Nth occurance
 Here are some examples to illustrate these modifiers:
    $_ = "foo bar baz blat";
    m :3x/ a /              # matches the "a" characters in each word
    m :nth(3)/ \w+ /        # matches "baz"

So it could be e.g. 'foo^{:2nd/<regexp>}' (note that there is no trailing / closing regexp, i.e. it is not 'foo^{:2nd/<regexp>/}').

So if we chose this, why don't we follow Perl 6 rule of combining modifiers http://perlcabal.org/syn/S05.html#Modifiers, so it would be

   foo^{:2nd:i/<regexp>}
or
   foo^{:i:nth(2)/<regexp>}

As to :/!<regexp> form: isn't it reserved for non-match? If not, then perhaps

  :/!2nd:i/<regexp>
Show 9 quoted lines
> [1] http://mid.gmane.org/9D675671-693D-4B59-AF2A-0EFE4C537362@sb.org
> 
> Nguyễn Thái Ngọc Duy (2):
>   get_sha1_oneline: allow to input commit_list
>   get_sha1: support ref^{/regex} syntax
> 
>  Documentation/revisions.txt |    7 ++++++
>  sha1_name.c                 |   45 ++++++++++++++++++++++++++++++++----------
>  2 files changed, 41 insertions(+), 11 deletions(-)
Thank you for working on this.
-- 
Jakub Narebski
Poland
Jonathan Nieder· Dec 8, 2010, 20:40 UTC · re: Jakub Narebski · lore

Re: [PATCH 0/2] [RFD] Using gitrevisions :/search style with other operators

Jakub Narebski wrote:
Show 8 quoted lines
> So if we chose this, why don't we follow Perl 6 rule of combining modifiers
> http://perlcabal.org/syn/S05.html#Modifiers, so it would be
>
>    foo^{:2nd:i/<regexp>}
>
> or
>
>    foo^{:i:nth(2)/<regexp>}
Very nice.
Nguyen Thai Ngoc Duy· Dec 9, 2010, 00:30 UTC · re: Jakub Narebski · lore

Re: [PATCH 0/2] [RFD] Using gitrevisions :/search style with other operators

2010/12/9 Jakub Narebski <jnareb@gmail.com>:
> I wonder if it would be possible to make :/<regex> (which looks a bit
> like searching the index) to be an alias to --all^{/<regex>}...

It looks a bit strange to my eyes to merge normal option name with revision syntax. But I think it's possible. Do we allow branch/tag name with leading '-'?

> Or if we can make ^{/<regex>} to act on revision range specified by
> earlier commits, so for example foo..bar^{/<regex>} would work.

There is another case: branch/tag selection. Instead of looking in all refs, people may want to look only in nd/* branches. My branches are almost flat, so I don't find any use. But someone might. And we can solve the "all branches" case above with simply "*". The exact syntax, I don't know.

> As to :/!<regexp> form: isn't it reserved for non-match?
It is reserved and not attached with any meaning.
> Thank you for working on this.

You're welcome. I needed to look for my branches in pu and was tired of copy/paste.

-- 
Duy
Jakub Narebski· Dec 9, 2010, 00:44 UTC · re: Nguyen Thai Ngoc Duy · lore

Re: [PATCH 0/2] [RFD] Using gitrevisions :/search style with other operators

On Thu, 9 Dec 2010, Nguyen Thai Ngoc Duy wrote:
> 2010/12/9 Jakub Narebski <jnareb@gmail.com>:
Show 6 quoted lines
> > I wonder if it would be possible to make :/<regex> (which looks a bit
> > like searching the index) to be an alias to --all^{/<regex>}...
> 
> It looks a bit strange to my eyes to merge normal option name with
> revision syntax. But I think it's possible. Do we allow branch/tag
> name with leading '-'?
Well, with below proposal it would simply be
  --all ^{/<regexp>}
 
Show 8 quoted lines
> > Or if we can make ^{/<regex>} to act on revision range specified by
> > earlier commits, so for example foo..bar^{/<regex>} would work.
> 
> There is another case: branch/tag selection. Instead of looking in all
> refs, people may want to look only in nd/* branches. My branches are
> almost flat, so I don't find any use. But someone might. And we can
> solve the "all branches" case above with simply "*". The exact syntax,
> I don't know.
  --glob=heads/nd/ ^{/<regexp>}
Similarly to the --all case.
-- 
Jakub Narebski
Poland
Nguyen Thai Ngoc Duy· Dec 9, 2010, 01:42 UTC · re: Jakub Narebski · lore

Re: [PATCH 0/2] [RFD] Using gitrevisions :/search style with other operators

On Thu, Dec 9, 2010 at 7:44 AM, Jakub Narebski <jnareb@gmail.com> wrote:
Show 13 quoted lines
> On Thu, 9 Dec 2010, Nguyen Thai Ngoc Duy wrote:
>> 2010/12/9 Jakub Narebski <jnareb@gmail.com>:
>
>> > I wonder if it would be possible to make :/<regex> (which looks a bit
>> > like searching the index) to be an alias to --all^{/<regex>}...
>>
>> It looks a bit strange to my eyes to merge normal option name with
>> revision syntax. But I think it's possible. Do we allow branch/tag
>> name with leading '-'?
>
> Well, with below proposal it would simply be
>
>  --all ^{/<regexp>}

This hardly works with range and may conflict with "--all" being already used by some commands.

I think we can move '/' out of {}, the space between '/' and '{' can be used for optional parameters: ^/{foo}.

-- 
Duy
Kevin Ballard· Dec 9, 2010, 01:46 UTC · re: Nguyen Thai Ngoc Duy · lore

Re: [PATCH 0/2] [RFD] Using gitrevisions :/search style with other operators

On Dec 8, 2010, at 5:42 PM, Nguyen Thai Ngoc Duy wrote:
Show 20 quoted lines
> On Thu, Dec 9, 2010 at 7:44 AM, Jakub Narebski <jnareb@gmail.com> wrote:
>> On Thu, 9 Dec 2010, Nguyen Thai Ngoc Duy wrote:
>>> 2010/12/9 Jakub Narebski <jnareb@gmail.com>:
>> 
>>>> I wonder if it would be possible to make :/<regex> (which looks a bit
>>>> like searching the index) to be an alias to --all^{/<regex>}...
>>> 
>>> It looks a bit strange to my eyes to merge normal option name with
>>> revision syntax. But I think it's possible. Do we allow branch/tag
>>> name with leading '-'?
>> 
>> Well, with below proposal it would simply be
>> 
>>  --all ^{/<regexp>}
> 
> This hardly works with range and may conflict with "--all" being
> already used by some commands.
> 
> I think we can move '/' out of {}, the space between '/' and '{' can
> be used for optional parameters: ^/{foo}

I thought ^{} was going to be an arbitrary grouping operator, capable of embedding any other modifier, but primarily only useful for regex. This change explicitly makes it an alternative regex syntax.

-Kevin Ballard
Jakub Narebski· Dec 9, 2010, 11:43 UTC · re: Nguyen Thai Ngoc Duy · lore

Re: [PATCH 0/2] [RFD] Using gitrevisions :/search style with other operators

On Thu, 9 Dec 2010, Nguyen Thai Ngoc Duy wrote:
Show 17 quoted lines
> On Thu, Dec 9, 2010 at 7:44 AM, Jakub Narebski <jnareb@gmail.com> wrote:
>> On Thu, 9 Dec 2010, Nguyen Thai Ngoc Duy wrote:
>>> 2010/12/9 Jakub Narebski <jnareb@gmail.com>:
>>
>>>> I wonder if it would be possible to make :/<regex> (which looks a bit
>>>> like searching the index) to be an alias to --all^{/<regex>}...
>>>
>>> It looks a bit strange to my eyes to merge normal option name with
>>> revision syntax. But I think it's possible. Do we allow branch/tag
>>> name with leading '-'?
>>
>> Well, with below proposal it would simply be
>>
>>  --all ^{/<regexp>}
> 
> This hardly works with range and may conflict with "--all" being
> already used by some commands.
It is '--all' like in "git log --all".
 
The proposed semantics for ^{/foo} (i.e. not attached to revision)
would be that it acts on all positive revs on the left of it, replacing
them.  But that might be not easy to do, and it feels a bit 
overengineered.
> I think we can move '/' out of {}, the space between '/' and '{' can
> be used for optional parameters: ^/{foo}.

Do you mean using e.g. ^/:i{foo} for :ignorecase, instead of ^{:i/foo} or ^{i/foo}?

-- 
Jakub Narebski
Poland
Nguyen Thai Ngoc Duy· Dec 9, 2010, 11:53 UTC · re: Jakub Narebski · lore

Re: [PATCH 0/2] [RFD] Using gitrevisions :/search style with other operators

On Thu, Dec 9, 2010 at 6:43 PM, Jakub Narebski <jnareb@gmail.com> wrote:
Show 25 quoted lines
> On Thu, 9 Dec 2010, Nguyen Thai Ngoc Duy wrote:
>> On Thu, Dec 9, 2010 at 7:44 AM, Jakub Narebski <jnareb@gmail.com> wrote:
>>> On Thu, 9 Dec 2010, Nguyen Thai Ngoc Duy wrote:
>>>> 2010/12/9 Jakub Narebski <jnareb@gmail.com>:
>>>
>>>>> I wonder if it would be possible to make :/<regex> (which looks a bit
>>>>> like searching the index) to be an alias to --all^{/<regex>}...
>>>>
>>>> It looks a bit strange to my eyes to merge normal option name with
>>>> revision syntax. But I think it's possible. Do we allow branch/tag
>>>> name with leading '-'?
>>>
>>> Well, with below proposal it would simply be
>>>
>>>  --all ^{/<regexp>}
>>
>> This hardly works with range and may conflict with "--all" being
>> already used by some commands.
>
> It is '--all' like in "git log --all".
>
> The proposed semantics for ^{/foo} (i.e. not attached to revision)
> would be that it acts on all positive revs on the left of it, replacing
> them.  But that might be not easy to do, and it feels a bit
> overengineered.
Yes, maybe.
Show 5 quoted lines
>> I think we can move '/' out of {}, the space between '/' and '{' can
>> be used for optional parameters: ^/{foo}.
>
> Do you mean using e.g. ^/:i{foo} for :ignorecase, instead of ^{:i/foo}
> or ^{i/foo}?

Yes. I find "^/" easier to read than "^{/". But "^{/" is more consistent to the rest.

-- 
Duy

← back to recent threads