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:55 UTC

Message-ID

<xmqqh992pbq6.fsf@gitster.mtv.corp.google.com>

In-Reply-To

<xmqqr386pd7d.fsf@gitster.mtv.corp.google.com>

Junio C Hamano <gitster@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.

 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;