RFC v2revision: new rev^-n shorthand for rev^n..rev

> --- builtin/rev-parse.c
> +++ builtin/rev-parse.c
> @@ -292,6 +292,32 @@ static int try_difference(const char *arg)
>  	return 0;
>  }
>  
> +static int try_parent_exclusion(const char *arg)
> +{
> +	int ret = 0;
> +	char *to_rev = NULL;
> +	char *from_rev = NULL;
> +	unsigned char to_sha1[20];
> +	unsigned char from_sha1[20];

> I use rev^..rev daily, and I'm surely not the only one. To save typing
> (or copy-pasting, if the rev is long -- like a full SHA-1 or branch name)
> we can make rev^- a shorthand for that.
> 
> The existing syntax rev^! seems like it should do the same, but it
> doesn't really do the right thing for merge commits (it gives only the
> merge itself).
> 
> As a natural generalisation, we also accept rev^-n where n excludes the
> nth parent of rev, although this is expected to be generally less useful.
> 
> [v2: Use ^- instead of % as suggested by Junio Hamano and use some
>  common helper functions for parsing.]

>  Documentation/revisions.txt | 14 +++++++
>  builtin/rev-parse.c         | 28 ++++++++++++++
>  revision.c                  | 91 +++++++++++++++++++++++++++++++++++++++++++++
>  revision.h                  |  1 +
>  4 files changed, 134 insertions(+)
> 
> diff --git Documentation/revisions.txt Documentation/revisions.txt
> index 4bed5b1..6e33801 100644
> --- Documentation/revisions.txt
> +++ Documentation/revisions.txt
> @@ -281,6 +281,14 @@ is a shorthand for 'HEAD..origin' and asks "What did the origin do since
>  I forked from them?"  Note that '..' would mean 'HEAD..HEAD' which is an
>  empty range that is both reachable and unreachable from HEAD.
>  
> +Parent Exclusion Notation
> +~~~~~~~~~~~~~~~~~~~~~~~~~
> +The '<rev>{caret}-{<n>}', Parent Exclusion Notation::
> +Shorthand for '<rev>{caret}<n>..<rev>', with '<n>' = 1 if not
> +given. This is typically useful for merge commits where you
> +can just pass '<commit>{caret}-' to get all the commits in the branch
> +that was merged in merge commit '<commit>'.
> +
>  Other <rev>{caret} Parent Shorthand Notations
>  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>  Two other shorthands exist, particularly useful for merge commits,
> @@ -316,6 +324,10 @@ Revision Range Summary
>  	<rev2> but exclude those that are reachable from both.  When
>  	either <rev1> or <rev2> is omitted, it defaults to `HEAD`.
>  
> +'<rev>{caret}-{<n>}', e.g. 'HEAD{caret}, HEAD{caret}-2'::

> +	Equivalent to '<rev>{caret}<n>..<rev>', with '<n>' = 1 if not
> +	given.
> +
>  '<rev>{caret}@', e.g. 'HEAD{caret}@'::
>    A suffix '{caret}' followed by an at sign is the same as listing
>    all parents of '<rev>' (meaning, include anything reachable from
> @@ -339,6 +351,8 @@ spelt out:
>     C                            I J F C
>     B..C   = ^B C                C
>     B...C  = B ^F C              G H D E B C
> +   B^-    = B^..B
> +	  = B ^B^1              E I J F B
>     C^@    = C^1
>  	  = F                   I J F
>     B^@    = B^1 B^2 B^3
> diff --git builtin/rev-parse.c builtin/rev-parse.c
> index 76cf05e..ad5e6ac 100644
> --- builtin/rev-parse.c
> +++ builtin/rev-parse.c
> @@ -292,6 +292,32 @@ static int try_difference(const char *arg)
>  	return 0;
>  }
>  
> +static int try_parent_exclusion(const char *arg)
> +{
> +	int ret = 0;
> +	char *to_rev = NULL;
> +	char *from_rev = NULL;
> +	unsigned char to_sha1[20];
> +	unsigned char from_sha1[20];

> I use rev^..rev daily, and I'm surely not the only one. To save typing
> (or copy-pasting, if the rev is long -- like a full SHA-1 or branch name)
> we can make rev^- a shorthand for that.
> 
> The existing syntax rev^! seems like it should do the same, but it
> doesn't really do the right thing for merge commits (it gives only the
> merge itself).
> 
> As a natural generalisation, we also accept rev^-n where n excludes the
> nth parent of rev, although this is expected to be generally less useful.
> 
> [v2: Use ^- instead of % as suggested by Junio Hamano and use some
>  common helper functions for parsing.]

> 
> Signed-off-by: Vegard Nossum <vegard.nossum@oracle.com>
> ---
>  Documentation/revisions.txt | 14 +++++++
>  builtin/rev-parse.c         | 28 ++++++++++++++
>  revision.c                  | 91 +++++++++++++++++++++++++++++++++++++++++++++
>  revision.h                  |  1 +
>  4 files changed, 134 insertions(+)

> To save typing
> (or copy-pasting, if the rev is long -- like a full SHA-1 or branch name)
> we can make rev^- a shorthand for that.
>
> The existing syntax rev^! seems like it should do the same, but it
> doesn't really do the right thing for merge commits (it gives only the
> merge itself).

>
> Signed-off-by: Vegard Nossum <vegard.nossum@oracle.com>
> ---
> Documentation/revisions.txt | 14 +++++++
> builtin/rev-parse.c         | 28 ++++++++++++++
> revision.c                  | 91 
> +++++++++++++++++++++++++++++++++++++++++++++
> revision.h                  |  1 +
> 4 files changed, 134 insertions(+)
>
> diff --git Documentation/revisions.txt Documentation/revisions.txt
> index 4bed5b1..6e33801 100644
> --- Documentation/revisions.txt
> +++ Documentation/revisions.txt
> @@ -281,6 +281,14 @@ is a shorthand for 'HEAD..origin' and asks "What did 
> the origin do since
> I forked from them?"  Note that '..' would mean 'HEAD..HEAD' which is an
> empty range that is both reachable and unreachable from HEAD.
>
> +Parent Exclusion Notation
> +~~~~~~~~~~~~~~~~~~~~~~~~~
> +The '<rev>{caret}-{<n>}', Parent Exclusion Notation::
> +Shorthand for '<rev>{caret}<n>..<rev>', with '<n>' = 1 if not
> +given. This is typically useful for merge commits where you
> +can just pass '<commit>{caret}-' to get all the commits in the branch

> +that was merged in merge commit '<commit>'.
> +
> Other <rev>{caret} Parent Shorthand Notations
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> Two other shorthands exist, particularly useful for merge commits,
> @@ -316,6 +324,10 @@ Revision Range Summary
>  <rev2> but exclude those that are reachable from both.  When
>  either <rev1> or <rev2> is omitted, it defaults to `HEAD`.
>
> +'<rev>{caret}-{<n>}', e.g. 'HEAD{caret}, HEAD{caret}-2'::
> + Equivalent to '<rev>{caret}<n>..<rev>', with '<n>' = 1 if not
> + given.
> +
> '<rev>{caret}@', e.g. 'HEAD{caret}@'::
>   A suffix '{caret}' followed by an at sign is the same as listing
>   all parents of '<rev>' (meaning, include anything reachable from
> @@ -339,6 +351,8 @@ spelt out:
>    C                            I J F C
>    B..C   = ^B C                C
>    B...C  = B ^F C              G H D E B C
> +   B^-    = B^..B
> +   = B ^B^1              E I J F B
>    C^@    = C^1
>    = F                   I J F
>    B^@    = B^1 B^2 B^3
> diff --git builtin/rev-parse.c builtin/rev-parse.c
> index 76cf05e..ad5e6ac 100644
> --- builtin/rev-parse.c
> +++ builtin/rev-parse.c
> @@ -292,6 +292,32 @@ static int try_difference(const char *arg)
>  return 0;
> }
>
> +static int try_parent_exclusion(const char *arg)
> +{
> + int ret = 0;
> + char *to_rev = NULL;
> + char *from_rev = NULL;
> + unsigned char to_sha1[20];
> + unsigned char from_sha1[20];
> +
> + if (parse_parent_exclusion(arg, &to_rev, &from_rev))
> + goto out;
> + if (get_sha1_committish(to_rev, to_sha1))
> + goto out;
> + if (get_sha1_committish(from_rev, from_sha1))
> + goto out;
> +
> + show_rev(NORMAL, to_sha1, to_rev);
> + show_rev(REVERSED, from_sha1, from_rev);
> +
> + ret = 1;
> +
> +out:
> + free(to_rev);
> + free(from_rev);
> + return ret;
> +}
> +
> static int try_parent_shorthands(const char *arg)
> {
>  char *dotdot;
> @@ -839,6 +865,8 @@ int cmd_rev_parse(int argc, const char **argv, const 
> char *prefix)
>  /* Not a flag argument */
>  if (try_difference(arg))
>  continue;
> + if (try_parent_exclusion(arg))
> + continue;
>  if (try_parent_shorthands(arg))
>  continue;
>  name = arg;
> diff --git revision.c revision.c
> index 969b3d1..0480f19 100644
> --- revision.c
> +++ revision.c
> @@ -1419,6 +1419,93 @@ static void prepare_show_merge(struct rev_info 
> *revs)
>  revs->limited = 1;
> }
>
> +/*
> + * If 'arg' is on the form '<rev>^-{<n>}', then return 0 and
> + * '*to_rev' and '*from_rev' will contain '<rev>' and '<rev>^<n>',
> + * respectively.
> + */
> +int parse_parent_exclusion(const char *arg, char **to_rev, char 
> **from_rev)
> +{
> + char *caret;
> + unsigned int n = 1;
> +
> + /*
> + * <rev>^-{<n>} is shorthand for <rev>^<n>..<rev>, with <n> = 1 if
> + * not given. This is typically used for merge commits where you
> + * can just pass '<merge>^-' and it will show you all the commits in
> + * the branch that was merged.
> + */
> +
> + if (!(caret = strstr(arg, "^-")))
> + return 1;
> + if (caret[2]) {
> + char *end;
> + n = strtoul(&caret[2], &end, 10);
> + if (*end != '\0')
> + return 1;
> + }
> + *to_rev = xstrndup(arg, caret - arg);
> + *from_rev = xstrfmt("%s^%u", *to_rev, n);
> + return 0;
> +}
> +
> +static int handle_parent_exclusion(const char *arg, struct rev_info 
> *revs, int flags)
> +{
> + int ret = 1;
> + char *to_rev = NULL;
> + char *from_rev = NULL;
> + unsigned char to_sha1[20];
> + unsigned char from_sha1[20];
> +
> + struct object *a_obj, *b_obj;
> + unsigned int flags_exclude = flags ^ (UNINTERESTING | BOTTOM);
> + unsigned int a_flags;
> +
> + /*
> + * <rev>^-{<n>} is shorthand for <rev>^<n>..<rev>, with <n> = 1 if
> + * not given. This is typically used for merge commits where you
> + * can just pass <merge>^- and it will show you all the commits in
> + * the branches that were merged.
> + */
> +
> + if (parse_parent_exclusion(arg, &to_rev, &from_rev))
> + goto out;
> +
> + if (get_sha1_committish(to_rev, to_sha1)) {
> + if (revs->ignore_missing)
> + goto out;
> + die("Unknown revision %s", to_rev);
> + }
> +
> + if (get_sha1_committish(from_rev, from_sha1)) {
> + if (revs->ignore_missing)
> + goto out;
> + die("Unknown revision %s", from_rev);
> + }
> +
> + a_obj = parse_object(from_sha1);
> + b_obj = parse_object(to_sha1);
> + if (!a_obj || !b_obj) {
> + if (revs->ignore_missing)
> + goto out;
> + die("Invalid revision range %s", arg);
> + }
> +
> + a_flags = flags_exclude;
> + a_obj->flags |= a_flags;
> + b_obj->flags |= flags;
> + add_rev_cmdline(revs, a_obj, from_rev, REV_CMD_LEFT, a_flags);
> + add_pending_object(revs, a_obj, from_rev);
> + add_rev_cmdline(revs, b_obj, to_rev, REV_CMD_RIGHT, flags);
> + add_pending_object(revs, b_obj, to_rev);
> +
> + ret = 0;
> +out:
> + free(to_rev);
> + free(from_rev);
> + return ret;
> +}
> +
> int handle_revision_arg(const char *arg_, struct rev_info *revs, int 
> flags, unsigned revarg_opt)
> {
>  struct object_context oc;
> @@ -1519,6 +1606,10 @@ int handle_revision_arg(const char *arg_, struct 
> rev_info *revs, int flags, unsi
>  }
>  *dotdot = '.';
>  }
> +
> + if (!handle_parent_exclusion(arg, revs, flags))
> + return 0;
> +
>  dotdot = strstr(arg, "^@");
>  if (dotdot && !dotdot[2]) {
>  *dotdot = 0;
> diff --git revision.h revision.h
> index 9fac1a6..ca5bebc 100644
> --- revision.h
> +++ revision.h
> @@ -243,6 +243,7 @@ extern int setup_revisions(int argc, const char 
> **argv, struct rev_info *revs,
> extern void parse_revision_opt(struct rev_info *revs, struct 
> parse_opt_ctx_t *ctx,
>         const struct option *options,
>         const char * const usagestr[]);
> +extern int parse_parent_exclusion(const char *arg, char **to_rev, char 
> **from_rev);
> #define REVARG_CANNOT_BE_FILENAME 01
> #define REVARG_COMMITTISH 02
> extern int handle_revision_arg(const char *arg, struct rev_info *revs,
> -- 
> 2.10.0.rc0.1.g07c9292
>
> 

> From: "Vegard Nossum" <vegard.nossum@oracle.com>
>>I use rev^..rev daily, and I'm surely not the only one.
>
> Not everyone knows the 'trick' and may not use it daily.
>
> Consider stating what it is useful for (e.g. "useful to get the
> commits and all  commits in the branches that were merged into commit"
> - paraphrased from the doc text)
>
>> To save typing
>> (or copy-pasting, if the rev is long -- like a full SHA-1 or branch name)
>> we can make rev^- a shorthand for that.
>>
>> The existing syntax rev^! seems like it should do the same, but it
>> doesn't really do the right thing for merge commits (it gives only the
>> merge itself).
>
> .. rather than the commit and those on side branches).
>> As a natural generalisation, we also accept rev^-n where n excludes the
>> nth parent of rev,
>
>> although this is expected to be generally less useful.
>
> Presumptious? for a two parent merge, surely(?) rev^-2 will give you
> what has been going on on the main line while the branch was being
> prepared... compare A^- and A^-2.

>> +Parent Exclusion Notation
>> +~~~~~~~~~~~~~~~~~~~~~~~~~
>> +The '<rev>{caret}-{<n>}', Parent Exclusion Notation::
>> +Shorthand for '<rev>{caret}<n>..<rev>', with '<n>' = 1 if not
>> +given. This is typically useful for merge commits where you
>> +can just pass '<commit>{caret}-' to get all the commits in the branch
>
> s/get all the/get the commit and all the/ ?
> It could be misread as a way of selecting just those commits that are
> within the side branch without including the given commit itself.
>
>> +that was merged in merge commit '<commit>'.
>> +
>> Other <rev>{caret} Parent Shorthand Notations
>> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> Two other shorthands exist, particularly useful for merge commits,

>> @@ -316,6 +324,10 @@ Revision Range Summary
>>  <rev2> but exclude those that are reachable from both.  When
>>  either <rev1> or <rev2> is omitted, it defaults to `HEAD`.
>>
>> +'<rev>{caret}-{<n>}', e.g. 'HEAD{caret}, HEAD{caret}-2'::

>> + Equivalent to '<rev>{caret}<n>..<rev>', with '<n>' = 1 if not
>> + given.
>> +
>> '<rev>{caret}@', e.g. 'HEAD{caret}@'::
>>   A suffix '{caret}' followed by an at sign is the same as listing
>>   all parents of '<rev>' (meaning, include anything reachable from
>> @@ -339,6 +351,8 @@ spelt out:
>>    C                            I J F C
>>    B..C   = ^B C                C
>>    B...C  = B ^F C              G H D E B C
>> +   B^-    = B^..B
>> +   = B ^B^1              E I J F B

>> diff --git builtin/rev-parse.c builtin/rev-parse.c
>> index 76cf05e..ad5e6ac 100644
>> --- builtin/rev-parse.c
>> +++ builtin/rev-parse.c
>> @@ -292,6 +292,32 @@ static int try_difference(const char *arg)
>>  return 0;
>> }
>>
>> +static int try_parent_exclusion(const char *arg)
>> +{
>> + int ret = 0;
>> + char *to_rev = NULL;
>> + char *from_rev = NULL;
>> + unsigned char to_sha1[20];
>> + unsigned char from_sha1[20];
>> +
>> + if (parse_parent_exclusion(arg, &to_rev, &from_rev))
>> + goto out;
>> + if (get_sha1_committish(to_rev, to_sha1))
>> + goto out;
>> + if (get_sha1_committish(from_rev, from_sha1))
>> + goto out;
>> +
>> + show_rev(NORMAL, to_sha1, to_rev);
>> + show_rev(REVERSED, from_sha1, from_rev);
>> +
>> + ret = 1;
>> +
>> +out:
>> + free(to_rev);
>> + free(from_rev);
>> + return ret;
>> +}
>> +
>> static int try_parent_shorthands(const char *arg)
>> {
>>  char *dotdot;

> "Philip Oakley" <philipoakley@iee.org> writes:
>
>> From: "Vegard Nossum" <vegard.nossum@oracle.com>
>>>I use rev^..rev daily, and I'm surely not the only one.
>>
>> Not everyone knows the 'trick' and may not use it daily.
>>
>> Consider stating what it is useful for (e.g. "useful to get the
>> commits and all  commits in the branches that were merged into commit"
>> - paraphrased from the doc text)
>>
>>> To save typing
>>> (or copy-pasting, if the rev is long -- like a full SHA-1 or branch 
>>> name)
>>> we can make rev^- a shorthand for that.
>>>
>>> The existing syntax rev^! seems like it should do the same, but it
>>> doesn't really do the right thing for merge commits (it gives only the
>>> merge itself).
>>
>> .. rather than the commit and those on side branches).
>>> As a natural generalisation, we also accept rev^-n where n excludes the
>>> nth parent of rev,
>>
>>> although this is expected to be generally less useful.
>>
>> Presumptious? for a two parent merge, surely(?) rev^-2 will give you
>> what has been going on on the main line while the branch was being
>> prepared... compare A^- and A^-2.
>
> All good comments.  It often is a good strategy to avoid subjective
> "this is useful" and "this is not useful" assessment, and instead
> let the feature itself find its supporters in the reading public.
>
>>> +Parent Exclusion Notation
>>> +~~~~~~~~~~~~~~~~~~~~~~~~~
>>> +The '<rev>{caret}-{<n>}', Parent Exclusion Notation::
>>> +Shorthand for '<rev>{caret}<n>..<rev>', with '<n>' = 1 if not
>>> +given. This is typically useful for merge commits where you
>>> +can just pass '<commit>{caret}-' to get all the commits in the branch
>>
>> s/get all the/get the commit and all the/ ?
>> It could be misread as a way of selecting just those commits that are
>> within the side branch without including the given commit itself.
>>
>>> +that was merged in merge commit '<commit>'.
>>> +
>>> Other <rev>{caret} Parent Shorthand Notations
>>> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>> Two other shorthands exist, particularly useful for merge commits,
>
> Is it just me that this new thing belongs to this "other shorthand
> notations", making the total to three from two?  It really is a
> closely related cousin of existing 'r1{caret}!'; instead of
> excluding all of its parents, it only excludes the specified one of
> its parents.  IOW, this new one is better described as the third
> other shorthand in this "Other Notations" section, without creating
> a new "Parent Exclusion Notation" section.
>

>>> @@ -316,6 +324,10 @@ Revision Range Summary
>>>  <rev2> but exclude those that are reachable from both.  When
>>>  either <rev1> or <rev2> is omitted, it defaults to `HEAD`.
>>>
>>> +'<rev>{caret}-{<n>}', e.g. 'HEAD{caret}, HEAD{caret}-2'::
>
> Huh?  Isn't the first example missing the necessary minus sign?
>
>>> + Equivalent to '<rev>{caret}<n>..<rev>', with '<n>' = 1 if not
>>> + given.
>>> +
>>> '<rev>{caret}@', e.g. 'HEAD{caret}@'::
>>>   A suffix '{caret}' followed by an at sign is the same as listing
>>>   all parents of '<rev>' (meaning, include anything reachable from
>>> @@ -339,6 +351,8 @@ spelt out:
>>>    C                            I J F C
>>>    B..C   = ^B C                C
>>>    B...C  = B ^F C              G H D E B C
>>> +   B^-    = B^..B
>>> +   = B ^B^1              E I J F B
>
> Even though these are order independent, the second line should say
>
>   = ^B^1 B              E I J F B
>
> to be consistent with the expansion of B..C, I would think.

>
>>> diff --git builtin/rev-parse.c builtin/rev-parse.c
>>> index 76cf05e..ad5e6ac 100644
>>> --- builtin/rev-parse.c
>>> +++ builtin/rev-parse.c
>>> @@ -292,6 +292,32 @@ static int try_difference(const char *arg)
>>>  return 0;
>>> }
>>>
>>> +static int try_parent_exclusion(const char *arg)
>>> +{
>>> + int ret = 0;
>>> + char *to_rev = NULL;
>>> + char *from_rev = NULL;
>>> + unsigned char to_sha1[20];
>>> + unsigned char from_sha1[20];
>>> +
>>> + if (parse_parent_exclusion(arg, &to_rev, &from_rev))
>>> + goto out;
>>> + if (get_sha1_committish(to_rev, to_sha1))
>>> + goto out;
>>> + if (get_sha1_committish(from_rev, from_sha1))
>>> + goto out;
>>> +
>>> + show_rev(NORMAL, to_sha1, to_rev);
>>> + show_rev(REVERSED, from_sha1, from_rev);
>>> +
>>> + ret = 1;
>>> +
>>> +out:
>>> + free(to_rev);
>>> + free(from_rev);
>>> + return ret;
>>> +}
>>> +
>>> static int try_parent_shorthands(const char *arg)
>>> {
>>>  char *dotdot;
>
> I did not expect that this needs an entirely new helper function,
> instead of being implemented as a new special case of existing
> try_parent_shorthands() function.  You'd need to strstr "^-" and
> parse a sequence of digits that follow it, which may want a helper
> to make sure you can error out if fed "some^-12thing" saying that
> "12thing" is not an integer, extend the existing "parents-only"
> thing so that it can represent three cases (i.e. @? !? or -?), and
> need a new variable to denote which parent is to be excluded when it
> is the '-' kind.  You'd need to temporarily *dotdot = '\0', parse
> what is before "^-" and revert *dotdot = '^' like existing helper
> function just the same.
>
> Exactly the same comment probably applies to the changes to the
> parser in revision.c, I would imagine, but I didn't read it ;-)