Re: [RFC PATCH v4] revision: new rev^-n shorthand for rev^n..rev
On Mon, Sep 26, 2016 at 02:55:45PM -0700, Junio C Hamano wrote:
> Taking these two together, perhaps squashing this in may be
> sufficient.
> [...]
> diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
> index 2c3da19..9474c37 100644
> --- a/builtin/rev-parse.c
> +++ b/builtin/rev-parse.c
> @@ -333,8 +333,22 @@ static int try_parent_shorthands(const char *arg)
> if (include_rev)
> show_rev(NORMAL, sha1, arg);
> commit = lookup_commit_reference(sha1);
> +
> + if (exclude_parent) {
> + /* do we have enough parents? */
> + for (parent_number = 0, parents = commit->parents;
> + parents;
> + parents = parents->next)
> + parent_number++;
> + if (parent_number < exclude_parent) {
> + *dotdot = '^';
> + return 0;
> + }
> + }
I think you can use commit_list_count() to make this a bit shorter,
like:
if (exclude_parent &&
commit_list_count(commit->parents) < exclude_parent) {
*dotdot = '^';
return 0;
}
Technically you can drop the first half of the &&, but it is probably a
good idea to avoid the traversal when exclude_parent is not in use.
Also technically, you can stop counting when you hit exclude_parent
(which is only possible with a custom traversal), but it is unlikely
enough that it is probably not worth caring about.
-Peff
Re: [RFC PATCH v4] revision: new rev^-n shorthand for rev^n..rev
Vegard Nossum writes: > +test_expect_success 'rev-parse merge^-0' ' > + test_must_fail git rev-parse merge^-0 > +' > + > +test_expect_success 'rev-parse merge^-3' ' > + test_must_fail git rev-parse merge^-3 > +' > + > +test_expect_success 'rev-parse merge^-^' ' > + test_must_fail git rev-parse merge^-^ > +' > + > +test_expect_success 'rev-list merge^-0' ' > + test_must_fail git rev-list merge^-0 > +' > + > +test_expect_success 'rev-list merge^-3' ' > + test_must_fail git rev-list merge^-3 > +' > + > +test_expect_success 'rev-list merge^-^' ' > + test_must_fail git rev-list merge^-^ > +' This seems to be testing failure cases fairly thoroughly, which is a good sign. One thing not tested is "merge^-1x" to ensure that no change mistakenly will break the strtoul() check you have to parse the parent number in the future, but other than that (and possibly reusing the set-up of an already existing test), I am fairly happy with the tests in this patch. Thanks.
Re: [RFC PATCH v4] revision: new rev^-n shorthand for rev^n..rev
Junio C Hamano writes:
> Micronit. When splitting "for (init; fini; cont)" into multiple
> lines, it is often easier to read to make that into three lines:
>
> for (parent_number = 1, parents = commit->parents;
>parents;
>parents = parents->next, parent_number++) {
>
>> +if (exclude_parent && parent_number != exclude_parent)
>> +continue;
>> +
>> +show_rev(include_parents ? NORMAL : REVERSED,
>> + parents->item->object.oid.hash, arg);
>> +}
>
> It is very clear to see what is going on. Good job.
>
>> *dotdot = '^';
>> +if (exclude_parent >= parent_number)
>> +return 0;
>
> This is not quite nice. You've already called show_rev() number of
> times, and it is too late to signal an error here. I think you
> would need to count the number of parents much earlier when
> exclude_parent option is in effect and error out before making any
> call to show_rev().
> ...
> Likewise. It is way too late to say "Nah, this wasn't a valid rev^-
> notation after all" to the caller after calling add_rev_cmdline()
> and add_pending_object() in the above loop.
Taking these two together, perhaps squashing this in may be
sufficient.
Please do not use --no-prefix when sending a patch to this list, by
the way.
builtin/rev-parse.c | 18 +++---
revision.c | 17 -
2 files changed, 31 insertions(+), 4 deletions(-)
diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
index 2c3da19..9474c37 100644
--- a/builtin/rev-parse.c
+++ b/builtin/rev-parse.c
@@ -333,8 +333,22 @@ static int try_parent_shorthands(const char *arg)
if (include_rev)
show_rev(NORMAL, sha1, arg);
commit = lookup_commit_reference(sha1);
+
+ if (exclude_parent) {
+ /* do we have enough parents? */
+ for (parent_number = 0, parents = commit->parents;
+parents;
+parents = parents->next)
+ parent_number++;
+ if (parent_number < exclude_parent) {
+ *dotdot = '^';
+ return 0;
+ }
+ }
+
for (parent_number = 1, parents = commit->parents;
-parents; parents = parents->next, parent_number++) {
+parents;
+parents = parents->next, parent_number++) {
if (exclude_parent && parent_number != exclude_parent)
continue;
@@ -343,8 +357,6 @@ static int try_parent_shorthands(const char *arg)
}
*dotdot = '^';
- if (exclude_parent >= parent_number)
- return 0;
return 1;
}
diff --git a/revision.c b/revision.c
index 511e1ed..09da7f4 100644
--- a/revision.c
+++ b/revision.c
@@ -1318,8 +1318,23 @@ static int add_parents_only(struct rev_info *revs, const
char *arg_, int flags,
if (it->type != OBJ_COMMIT)
return 0;
commit = (struct commit *)it;
+
+ if (exclude_parent) {
+ struct commit_list *parents;
+ int parent_number;
+
+ /* do we have enough parents? */
+ for (parent_number = 0, parents = commit->parents;
+parents;
+parents = parents->next)
+ parent_number++;
+ if (parent_number < exclude_parent)
+ return 0;
+ }
+
for (parent_number = 1, parents = commit->parents;
-parents; parents = parents->next, parent_number++) {
+parents;
+parents = parents->next, parent_number++) {
if (exclude_parent && parent_number != exclude_parent)
continue;
Re: [RFC PATCH v4] revision: new rev^-n shorthand for rev^n..rev
Vegard Nossum writes:
> I often use rev^..rev to get all the commits in the branch that was merged
> in by the merge commit 'rev' (including the merge itself). 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 thing, but it
> doesn't really do the right thing for merge commits (it doesn't include
> the commits from side branches).
>
> As a natural generalisation, we also accept rev^-n where n excludes the
> nth parent of rev. For example, for a two-parent merge, you can use rev^-2
> to get the set of commits which were made to the main branch while the
> topic branch was prepared.
I am tempted to suggest that this four-line paragraph may be
sufficient:
"git log rev^..rev" is commonly used to show all work done on
and merged from a side branch. Introduce a short-hand "rev^-"
for this, and also allow it to take "rev^-$n" to mean "reachable
from rev, excluding what is reachable from n-th parent of rev".
This alone is not a strong enough reason to ask you to reroll the
patch.
> diff --git builtin/rev-parse.c builtin/rev-parse.c
> index 76cf05e..2c3da19 100644
> --- builtin/rev-parse.c
> +++ builtin/rev-parse.c
> @@ -298,14 +298,30 @@ static int try_parent_shorthands(const char *arg)
> unsigned char sha1[20];
> struct commit *commit;
> struct commit_list *parents;
> - int parents_only;
> -
> - if ((dotdot = strstr(arg, "^!")))
> - parents_only = 0;
> - else if ((dotdot = strstr(arg, "^@")))
> - parents_only = 1;
> -
> - if (!dotdot || dotdot[2])
> + int parent_number;
> + int include_rev = 0;
> + int include_parents = 0;
> + int exclude_parent = 0;
> +
> + if ((dotdot = strstr(arg, "^!"))) {
> + include_rev = 1;
> + if (dotdot[2])
> + return 0;
> + } else if ((dotdot = strstr(arg, "^@"))) {
> + include_parents = 1;
> + if (dotdot[2])
> + return 0;
> + } else if ((dotdot = strstr(arg, "^-"))) {
> + include_rev = 1;
> + exclude_parent = 1;
> +
> + if (dotdot[2]) {
> + char *end;
> + exclude_parent = strtoul(dotdot + 2, &end, 10);
> + if (*end != '\0' || !exclude_parent)
> + return 0;
> + }
> + } else
> return 0;
Nice; we can tell where this is going without looking at the rest,
which is a very good sign that the new variables are doing their
work of telling the readers what is going on clearly.
> @@ -314,14 +330,21 @@ static int try_parent_shorthands(const char *arg)
> return 0;
> }
>
> - if (!parents_only)
> + if (include_rev)
> show_rev(NORMAL, sha1, arg);
> commit = lookup_commit_reference(sha1);
> - for (parents = commit->parents; parents; parents = parents->next)
> - show_rev(parents_only ? NORMAL : REVERSED,
> - parents->item->object.oid.hash, arg);
> + for (parent_number = 1, parents = commit->parents;
> + parents; parents = parents->next, parent_number++) {
Micronit. When splitting "for (init; fini; cont)" into multiple
lines, it is often easier to read to make that into three lines:
for (parent_number = 1, parents = commit->parents;
parents;
parents = parents->next, parent_number++) {
> + if (exclude_parent && parent_number != exclude_parent)
> + continue;
> +
> + show_rev(include_parents ? NORMAL : REVERSED,
> + parents->item->object.oid.hash, arg);
> + }
It is very clear to see what is going on. Good job.
> *dotdot = '^';
> + if (exclude_parent >= parent_number)
> + return 0;
This is not quite nice. You've already called show_rev() number of
times, and it is too late to signal an error here. I think you
would need to count the number of parents much earlier when
exclude_parent option is in effect and error out before making any
call to show_rev().
> diff --git revision.c revision.c
> index 969b3d1..9ae95bf 100644
> --- revision.c
> +++ revision.c
> @@ -1289,12 +1289,14 @@ void add_index_objects_to_pending(struct rev_info
> *revs, unsigned flags)
> }
> }
>
> -static int add_parents_only(struct rev_info *revs, const char *arg_, int
> flags)
> +static int add_parents_only(struct rev_info *revs, const char *arg_, int
> flags,
> + int exclude_parent)
> {
> unsigned char sha1[20];
> struct object *it;
> struct commit *commit;
> struct commit_list *parents;
> + int parent_number;
> const char *arg = arg_;
>
> if (*arg == '^') {
> @@ -1316,12 +1318,18 @@ static int add_parents_only(struct rev_info *re
[RFC PATCH v4] revision: new rev^-n shorthand for rev^n..rev
I often use rev^..rev to get all the commits in the branch that was merged
in by the merge commit 'rev' (including the merge itself). 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 thing, but it
doesn't really do the right thing for merge commits (it doesn't include
the commits from side branches).
As a natural generalisation, we also accept rev^-n where n excludes the
nth parent of rev. For example, for a two-parent merge, you can use rev^-2
to get the set of commits which were made to the main branch while the
topic branch was prepared.
Signed-off-by: Vegard Nossum
---
[v2: Use ^- instead of % as suggested by Junio Hamano and use some
common helper functions for parsing.]
[v3: Use 'struct object_id' instead of 'char[20]' and add some tests as
suggested by Matthieu Moy; fix missing '-' in Documentation/revisions.txt
as suggested by Ramsay Jones; misc changelog + documentation fixes as
suggested by Philip Oakley.]
[v4: Documentation fixes and parsing rework suggested by Junio Hamano
and add some more tests.]
---
Documentation/revisions.txt | 17 +++-
builtin/rev-parse.c | 47 +++--
revision.c | 32 +--
t/t6070-rev-parent-exclusion.sh | 90 +
4 files changed, 168 insertions(+), 18 deletions(-)
create mode 100755 t/t6070-rev-parent-exclusion.sh
diff --git Documentation/revisions.txt Documentation/revisions.txt
index 4bed5b1..ba11b9c 100644
--- Documentation/revisions.txt
+++ Documentation/revisions.txt
@@ -283,7 +283,7 @@ empty range that is both reachable and unreachable from
HEAD.
Other {caret} Parent Shorthand Notations
~
-Two other shorthands exist, particularly useful for merge commits,
+Three other shorthands exist, particularly useful for merge commits,
for naming a set that is formed by a commit and its parent commits.
The 'r1{caret}@' notation means all parents of 'r1'.
@@ -291,8 +291,15 @@ The 'r1{caret}@' notation means all parents of 'r1'.
The 'r1{caret}!' notation includes commit 'r1' but excludes all of its parents.
By itself, this notation denotes the single commit 'r1'.
+The '{caret}-{}' notation includes '' but excludes the th
+parent (i.e. a shorthand for '{caret}..'), with '' = 1 if
+not given. This is typically useful for merge commits where you
+can just pass '{caret}-' to get all the commits in the branch
+that was merged in merge commit '' (including ''
+itself).
+
While '{caret}' was about specifying a single commit parent, these
-two notations consider all its parents. For example you can say
+three notations also consider its parents. For example you can say
'HEAD{caret}2{caret}@', however you cannot say 'HEAD{caret}@{caret}2'.
Revision Range Summary
@@ -326,6 +333,10 @@ Revision Range Summary
as giving commit '' and then all its parents prefixed with
'{caret}' to exclude them (and their ancestors).
+'{caret}-{}', e.g. 'HEAD{caret}-, HEAD{caret}-2'::
+ Equivalent to '{caret}..', with '' = 1 if not
+ given.
+
Here are a handful of examples using the Loeliger illustration above,
with each step in the notation's expansion and selection carefully
spelt out:
@@ -339,6 +350,8 @@ spelt out:
CI J F C
B..C = ^B CC
B...C = B ^F C G H D E B C
+ B^-= B^..B
+ = ^B^1 B 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..2c3da19 100644
--- builtin/rev-parse.c
+++ builtin/rev-parse.c
@@ -298,14 +298,30 @@ static int try_parent_shorthands(const char *arg)
unsigned char sha1[20];
struct commit *commit;
struct commit_list *parents;
- int parents_only;
-
- if ((dotdot = strstr(arg, "^!")))
- parents_only = 0;
- else if ((dotdot = strstr(arg, "^@")))
- parents_only = 1;
-
- if (!dotdot || dotdot[2])
+ int parent_number;
+ int include_rev = 0;
+ int include_parents = 0;
+ int exclude_parent = 0;
+
+ if ((dotdot = strstr(arg, "^!"))) {
+ include_rev = 1;
+ if (dotdot[2])
+ return 0;
+ } else if ((dotdot = strstr(arg, "^@"))) {
+ include_parents = 1;
+ if (dotdot[2])
+ return 0;
+ } else if ((dotdot = strstr(arg, "^-"))) {
+ include_rev = 1;
+ exclude_parent = 1;
+
+ if (dotdot[2]) {
+ char *end;
+ exclude_parent = strtoul(dotdot + 2, &end, 10);
+ if (*end != '\0' || !exclude_parent)
+ return 0;
+ }
+
