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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Ramkumar Ramachandra <artagnon@xxxxxxxxx> writes:

> 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@xxxxxxxxx>

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 ...

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]