Re: [PATCH V4 2/2] object name: introduce '^{/!-<negative pattern>}' notation

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 13, 2016, 19:15 UTC
Message-ID: <xmqqh9ihnveu.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <1452660741-5616-1-git-send-email-ischis2@cox.net>

"Stephen P. Smith" <ischis2@cox.net> writes:

Show 35 quoted lines

> From: Will Palmer <wmpalmer@gmail.com>
>
> To name a commit, you can now use the :/!-<negative pattern> regex
> style, and consequentially, say
>
>     $ git rev-parse HEAD^{/!-foo}
>
> and it will return the hash of the first commit reachable from HEAD,
> whose commit message does not contain "foo". This is the opposite of the
> existing <rev>^{/<pattern>} syntax.
>
> The specific use-case this is intended for is to perform an operation,
> excluding the most-recent commits containing a particular marker. For
> example, if you tend to make "work in progress" commits, with messages
> beginning with "WIP", you work, then it could be useful to diff against
> "the most recent commit which was not a WIP commit". That sort of thing
> now possible, via commands such as:
>
>     $ git diff @^{/!-^WIP}
>
> The leader '/!-', rather than simply '/!', to denote a negative match,
> is chosen to leave room for additional modifiers in the future.
>
> Signed-off-by: Will Palmer <wmpalmer@gmail.com>
> Signed-off-by: Stephen P. Smith <ischis2@cox.net>
> ---
>
> Notes:
>     Changed |say|use the :/!-<negative pattern> regex style, and consequentially, say|.
>     
>     Chose not to chagne subject since it matches the end of
>     git rev-parse HEAD^{/!-foo}
>
>     Mailing list web interface is again not working; therefore, I don't 
>     have URLs for the earlier review comments.

Thanks, this looks good (and it looked good already at the previous round).

Show 19 quoted lines

> diff --git a/Documentation/revisions.txt b/Documentation/revisions.txt
> index d85e303..0c84d4f 100644
> --- a/Documentation/revisions.txt
> +++ b/Documentation/revisions.txt
> @@ -176,11 +176,12 @@ existing tag object.
>    A colon, followed by a slash, followed by a text, names
>    a commit whose commit message matches the specified regular expression.
>    This name returns the youngest matching commit which is
> -  reachable from any ref.  If the commit message starts with a
> -  '!' you have to repeat that;  the special sequence ':/!',
> -  followed by something else than '!', is reserved for now.
> -  The regular expression can match any part of the commit message. To
> -  match messages starting with a string, one can use e.g. ':/^foo'.
> +  reachable from any ref. The regular expression can match any part of the
> +  commit message. To match messages starting with a string, one can use
> +  e.g. ':/^foo'. The special sequence ':/!' is reserved for modifiers to what
> +  is matched. ':/!-foo' performs a negative match, while ':/!!foo' matches a
> +  literal '!' character, followed by 'foo'. Any other sequence beginning with
> +  ':/!' is reserved for now.

The original text reads as if ":/foo" looks for 'foo' anywhere in the log message while ":/!!foo" looks for '!foo' at the beginning, which was incorrect as far as I can tell, but the updated text corrects it. Good.

Show 7 quoted lines

> @@ -903,7 +913,7 @@ static int get_sha1_oneline(const char *prefix, unsigned char *sha1,
>  			continue;
>  		buf = get_commit_buffer(commit, NULL);
>  		p = strstr(buf, "\n\n");
> -		matches = p && !regexec(&regex, p + 2, 0, NULL, 0);
> +		matches = p && (negative ^ !regexec(&regex, p + 2, 0, NULL, 0));
>  		unuse_commit_buffer(commit, buf);

Hmph, without "negative pattern match", if you asked for ":/foo" and the commit did not have any body (which I do not think the current version of Git allows to create by default, but there may be such commits created by older versions of Git or reimplementation of Git made by others), p could be NULL. In such a case, any regex would not match, so I would expect that commit to be shown.

In other words, I wonder if the above should be

		matches = negative ^ (p && !regexec(&regex, p + 2, 0, NULL, 0));

This would not make practical difference, but I would expect any change to introduce "negative patch" to an original logic that is

	matches = ORIGINAL_LOGIC_TO_COMPUTE_MATCH

to become

	matches = negative ^ ORIGINAL_LOGIC_TO_COMPUTE_MATCH