Re: [RFC PATCH v4] revision: new rev^-n shorthand for rev^n..rev
From: Junio C Hamano <hidden>
Date: 2016-09-26 21:24:00
Vegard Nossum [off-list ref] 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.
quoted hunk
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.
quoted hunk
@@ -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().
quoted hunk
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?
quoted hunk
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.