Vegard Nossum <vegard.nossum@xxxxxxxxxx> 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.