Re: [PATCH 3/5] setup_revisions: remember whether a ref was positive or not

Sverre Rabbelier <srabbelier@gmail.com> writes:

I would think that the information you are trying to convey is more in line with the spirit of "name" field, not "mode". Originally, the pending object array was to only collect the given arguments at the object level with "is this interesting or uninteresting?" flag and nothing more, but we later wanted to have richer information to deduce what the user wanted from how the user gave the object information to us (i.e. "not just the tree object named by the object name, but I meant the object at this path"), primarily to give a better error message.

Let's clarify what I hinted as "some parsed representation of how the revs were specified" in an earlier message with a concrete example. There is a code block at the end of builtin/diff.c that says something like:

	If we have more than 2 tree-ishes, and the first one is marked as
	UNINTERESTING, it must have come from "diff A...B". In this case,
	we know ent[0] is one of the merge-bases, ent[ents-2] is A, and
	ent[ents-1] is B, so turn it into "diff ent[0] B".

These entries in ent[] come from the pending objects array, and because the revision machinery loses information regarding how the command line arguments were spelled, we have to play such games to _guess_ what the user meant to say. This will lead to "diff ^C A B" running "diff C B", instead of barfing with "What are you talking about???", which should happen instead.

Wouldn't it be wonderful if the revision machinery left richer clue in each element of the pending object array while parsing, so that the caller does not have to guess? For example, suppose that it parsed "A...B" into this N element array of pending objects:

	^MB0 (the first merge base of A and B)
	^MB1 (the second merge base of A and B)
        ...
	A
	B

In addition to a single "mode" integer, which says if it is supposed to be a tree or a blob, we could allocate a single structure that records something like this:

	struct parsed_rev {
                enum {
			SHA1, REF, RANGE, SYMMETRIC_RANGE, REFLOG_ENT, ...
                        // there are others like OBJ^!, OBJ@!, ...
                } kind;
                const char *string;
                union {
			struct {
				const char *real_ref;
			} ref;
			struct {
				struct parsed_rev *bottom;
				struct parsed_rev *top;
                        } range;
			...
                } u;
	};

And then all the elements in the pending object array resulting from parsing "maint...master" would all point at a single instance of this "struct parsed_rev" that may read like this:

	{
        	.kind = SYMMETRIC_RANGE;
		.string = "maint...master";
                .u.range = {
                	.bottom = {
                        	.kind = REF;
                                .string = "maint";
                		.u.ref.real_ref = "refs/heads/maint";
			};
                	.top = {
                        	.kind = REF;
                                .string = "master";
                		.u.ref.real_ref = "refs/heads/master";
			};
		};
	};

In a similar fashion, if you wanted to make sure that you do not discard "master" as totally uninteresting, even though the end user gave you a list of arguments that ends up marking the commit object "master" as uninteresting, e.g. "master^0..master", you could do so if we updated the revision parsing machinery so that the resulting two elements in the pending array would both point at (in addition to the "uninteresting commit object 'master^0'") a structure that would look like this:

	{
        	.kind = RANGE;
                .string = "master^0..master";
                .u.range = {
			...
                        .top = {
				.kind = REF;
                                .string = "master";
                		.u.ref.real_ref = "refs/heads/master";
			};
		};
	};

And by looking at .u.range.top, you know that the user meant to do something to the "master" branch.