Vegard Nossum <vegard.nos...@oracle.com> 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 *revs, 
> const char *arg_, int flags)
>       if (it->type != OBJ_COMMIT)
>               return 0;
>       commit = (struct commit *)it;
> -     for (parents = commit->parents; parents; parents = parents->next) {
> +     for (parent_number = 1, parents = commit->parents;
> +          parents; parents = parents->next, parent_number++) {
> +             if (exclude_parent && parent_number != exclude_parent)
> +                     continue;
> +
>               it = &parents->item->object;
>               it->flags |= flags;
>               add_rev_cmdline(revs, it, arg_, REV_CMD_PARENTS_ONLY, flags);
>               add_pending_object(revs, it, arg);
>       }
> +     if (exclude_parent >= parent_number)
> +             return 0;

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.  Just like "blob^-"
silently returns 0 in the pre-context in this hunk, count the number
of parents before entering this loop when exclude_parent is in
effect, and if the number after '-' exceeds the actual number of
parents, silently return 0, perhaps?

> diff --git t/t6070-rev-parent-exclusion.sh t/t6070-rev-parent-exclusion.sh

We already seem to have t6101 as the best place to add test for this
new feature.  Near the end of that script, ^@ and ^! are tested.

Thanks.

Reply via email to