Re: [RFC PATCH v4] revision: new rev^-n shorthand for rev^n..rev

[Junio C Hamano]

Junio C Hamano <gits...@pobox.com> 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;