Re: [RFC PATCH v4] revision: new rev^-n shorthand for rev^n..rev
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Sep 26, 2016, 21:23 UTC
- Message-ID
- <xmqqr386pd7d.fsf@gitster.mtv.corp.google.com>
- In-Reply-To
- <20160926204959.26007-1-vegard.nossum@oracle.com>
Vegard Nossum <vegard.nossum@oracle.com> writes:
Show 13 quoted lines
> 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.
Show 41 quoted lines
> 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.
Show 13 quoted lines
> @@ -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++) {Show 6 quoted lines
> + 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().
Show 37 quoted lines
> 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.