Re: [PATCH 3/4] revert: simplify insn parsing logic

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 2, 2012, 20:33 UTC
Message-ID: <7vr4w5euvg.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1333029495-10034-4-git-send-email-artagnon@gmail.com>

Ramkumar Ramachandra <artagnon@gmail.com> writes:

Show 8 quoted lines

> Our instruction sheet parser first looks for a valid action by
> checking that the buffer starts with either "pick" or "revert".  Then,
> it looks for either spaces or tabs before looking for the object name,
> erroring out if it doesn't find any.  Simplify this logic without
> making any functional changes by looking for ("pick " or "pick\t") or
> ("revert " or "revert\t") in the first place.
>
> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>

Hrm.

In the longer term, I think the right approach is to cut the first token on the line, and then switch the action based on what the token is.

The reason why the original complains against "pickle <commit> <msg>" is not because the headword "pickle" is an unknown verb, but because the recognised token "pick" is followed by 'l' which is not a whitespace, and the updated code may slightly be closer to the ideal, but not that much.

Perhaps something like this?

I suspect that this may be a bit over-engineered, but on the other hand, if we do not foresee that we would be adding many other verbs, I do not think there is much point in your patch to clean up the verb parsing part, either, so...

-- >8 --

static struct {
	const char *name;
	int len;
	enum replay_action action;
} replay_action[] = {
	{ "pick", 4, REPLAY_PICK },
	{ "revert", 6, REPLAY_REVERT },
};

static enum replay_action parse_action(char **cp_, int *namelen_)
{
	char *cp = *cp_;
        int namelen = strcspn(cp, "\t ");
	int i, padlen;

	for (i = 0; i < ARRAY_SIZE(replay_action); i++)
        	if (namelen == replay_action[i].len &&
                    !memcmp(cp, replay_action[i].name, namelen)) {
			if (namelen_)
				*namelen_ = namelen;
			padlen = strspn(cp + namelen, "\t ");
                        *cp_ = cp + namelen + padlen;
			return replay_action[i].action;
		}
	return -1;
}

static struct commit *parse_insn_line(char *bol, ...)
{
	...

	action = parse_action(&bol, NULL);
	/*
         * Verify that the action matches ...