From: Philip Oakley <philipoakley@iee.org>
Date: Mon, 26 Sep 2016 13:00:04 GMT
Subject: Re: [RFC PATCH v2] revision: new rev^-n shorthand for rev^n..rev
Message-ID: <1173701ACA474229ABC1A14468682474@PhilipOakley>
In-Reply-To: <xmqq7f9zwl2q.fsf@gitster.mtv.corp.google.com>

From: "Junio C Hamano" <gitster@pobox.com>
> "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.
>
True. It probably should be there.

>>> @@ -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.

Agreed.
>
>>> 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 ;-)

Sounds sensible. I hadn't double checked Vegard's implementation at this 
point.
--
Philip
>