patch, 4 partssha1_name: introduce getn_sha1() to take length

I have a lot of code left over from my work on the sequencer; I'm slowly rebasing the work and sending out patches. This series includes some minor improvements to sequencer.c. All tests pass.

Thanks for reading.

    Ram

Ramkumar Ramachandra (4):
  sha1_name: introduce getn_sha1() to take length
  revert: use getn_sha1() to simplify insn parsing
  revert: simplify insn parsing logic
  revert: report fine-grained errors from insn parser

 cache.h     |    1 +
 sequencer.c |   64 +++++++++++++++++++++++++++++++++++------------------------
 sha1_name.c |   23 +++++++++++++++++---
 3 files changed, 58 insertions(+), 30 deletions(-)

Introduce a variant of get_sha1() that additionally takes the length of the buffer, so it can parse object names from buffers that don't necessarily terminate with NUL.

Suggested-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 cache.h     |    1 +
 sha1_name.c |   23 +++++++++++++++++++----
 2 files changed, 20 insertions(+), 4 deletions(-)

(cc-ing Clément who is one of the last people to change this family of
 APIs, and Matthieu who knows these codepaths well and may have been a
 mentor for that project)
Hi,

Ramkumar Ramachandra wrote:

You're probably going to hate this, but oh well:

I love the new function. I really dislike its name. Do we have any other (function that takes a C-style string/function that takes a buffer with length) pairs in the spirit of strchr/memchr to draw inspiration from?

Holy cow this function is going crazy. So let's take a survey of the public functions in this family.

 get_sha1(name, sha1)::

	Looks up a sha1 expression like xyz^ and returns the
	corresponding object name in the 20-byte buffer sha1.
	Returns -1 on failure.

 get_sha1_with_mode(name, sha1, mode)::

	Like get_sha1, but when name refers to a path within
	a tree, also returns the mode in *mode.

 get_sha1_with_context(name, sha1, context)::

	Likewise, but also returns the parsed <tree, path>
	pair. 

 get_sha1_with_mode_1(name, sha1, mode, flag, prefix)::

	Like get_sha1_with_mode, but with some extra features:

	 - flag indicates whether we are in the "detailed diagnosis"
	   codepath and only calling this function for the
	   side-effect of die()-ing with a meaningful message.

	 - prefix indicates the cwd prefix for use in the "did you
	   mean?" diagnostic.

 get_sha1_with_context_1(name, sah1, context, flag, prefix)::

	Variant in the same vein for get_sha1_with_context.

Plus the corresponding variants with "name, namelen" pairs after your patch.

My first reaction is that the meaning of the _1 suffix is not going to be obvious to newcomers. Any ideas for addressing that?

"get_sha1_with_context_1" has no external callers so it could probably be made private. So there could be a sequence of functions with increasing detail:

	get_sha1(name, sha1)
	get_sha1_with_mode(name, sha1, mode)
	get_sha1_with_context(name, sha1, context)
	die_sha1_not_in_index(name, sha1, context, prefix)

The "len" argument could be introduced at some convenient place in this list to avoid having to change too many callers, for example:

	get_sha1(name, sha1)
	get_sha1_with_mode(name, sha1, mode)
	get_sha1_with_context(name, namelen, sha1, context)
	die_sha1_not_in_index(name, namelen, sha1, context, prefix)

Or maybe it makes sense to bite the bullet and add the length argument to all callers:

	get_sha1(name, namelen, sha1)
	get_sha1_with_mode(name, namelen, sha1, mode)
	get_sha1_with_context(name, namelen, sha1, context)
	die_sha1_not_in_index(name, namelen, sha1, context, prefix)

If I were doing it, I might just ask the maintainer to make the decision for me, by making a somewhat funny patch series:

 1. add "how to use the get_sha1 family" to Documentation/technical.
 2. make get_sha1_with_context_1 static.
 3. replace get_sha1_with_mode_1 with simpler die_sha1_not_in_index
    function.
 4. add namelen argument to die_sha1_not_... and
    get_sha1_with_context, adjust callers, and make use of
    get_sha1_with_context with this arg in sequencer code.
 5. add namelen arg to get_sha1_with_mode, adjust callers, and
    make sequencer code use this instead.
 6. add namelen arg to get_sha1, adjust callers, and make sequener
    code use this instead.

That way, the maintainer could take patches 1 - 3 to get the basic API cleanup, patch 4 to get the sequencer enhancement and make a judgement call about whether to take patches 5 and 6 depending on how much of a pain the churn would be given other patches in flight.

What do you think? Jonathan

Jonathan Nieder <jrnieder@gmail.com> writes:

I wouldn't say I know them well, but I did touch them in the past.

Good job. I think your explanations could actually be added as comments to cache.h to document the corresponding functions.

It seems I'm the one who introduced get_sha1_with_context_1. I meant "internal variant of the one without _1", which is a convention used in other places of Git's code, but usually as static functions, not in .h files.

I kept it public as a very small implementation detail: I tried not to introduce performance penalty, hence made get_sha1_with_mode inline (since it is really a trivial wrapper). But we probably wouldn't notice the difference in performance making the _1 version private and losing the "static inline"-ness of the public version.

Another way to say this is: get_sha1_with_context_1() does the real job (perhaps it should be called get_sha1_real()?), and others are convenience wrappers. Since convenience wrappers are convenient, nobody use the actual function directly.

I don't think so. Convenience wrappers are meant to be simple to call, and I don't think we want to force everybody to call strlen() before calling them.

Hi Jonathan,

Jonathan Nieder wrote:

Makes a lot of sense.  I'll re-roll shortly.

Thanks.

    Ram

Ramkumar Ramachandra wrote:

Ok, but please keep in mind Matthieu's advice about keeping convenience wrappers convenient as well. I trust him more in these questions.

Thanks. Jonathan

To read the object name in the instruction sheet, we currently manipulate the buffer to artificially introduce a NUL after the supposed object name, and then use get_sha1() to read the object name before restoring the buffer. Get rid of this ugliness by using getn_sha1(), a function introduced in the previous patch.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 sequencer.c |   12 +++---------
 1 files changed, 3 insertions(+), 9 deletions(-)

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>
---
 sequencer.c |   11 ++++-------
 1 files changed, 4 insertions(+), 7 deletions(-)

Ramkumar Ramachandra <artagnon@gmail.com> writes:

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

Hi Junio,

Junio C Hamano wrote:

interesting; yes, it does look a little over-engineered for the
moment.  I should have been clearer in the commit message: this patch
exists mainly so that [4/4] can report a sensible error message
instead of a literal "Missing space after valid verb".  So, we have
four options from here:
1. Squash this part into [4/4].  Will this give rise to additional confusion?
2. Use your version of the patch.
3. Append the following to the commit message: "This patch exists so
that the next patch can report a sensible error message when an
invalid verb is encountered".
4. If you don't think [4/4] is valuable yet, just apply the first two
parts of this series.

I'm personally in favor of (1) now.

Thanks.

    Ram

The infrastructure that parses '.git/sequencer/todo' is meant to handle arbitrary user input some day, so it can be used as the implementation of 'git rebase --interactive' and 'git sequencer --edit'. It is currently sub-optimal for that purpose because the parse error messages just say:

  error: Could not parse line 5.

This patch shifts responsibility to parse_insn_line(), which can come up with a more detailed message like:

  error: .git/sequencer/todo:5: unrecognized action: frobnicate

Once the operator is allowed to edit the sequence, the message might be adjusted to something like:

  error: <sequence you just gave me>:5: unrecognized action: frobnicate

instead of exposing an implementation detail. Some day 'git sequencer --edit' could even re-launch the editor with an error message in a comment before the problematic line and the cursor pointing there. For now, pointing to the explicit filename is useful since this should only happen if there was filesystem corruption, tampering, or a git bug.

Helped-by: Junio C Hamano <gitster@pobox.com>
Helped-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 sequencer.c |   45 +++++++++++++++++++++++++++++++++------------
 1 files changed, 33 insertions(+), 12 deletions(-)