[PATCH 0/4] Minor sequencer.c cleanups

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(-)

-- 
1.7.8.1.362.g5d6df.dirty

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 <redacted>
Signed-off-by: Ramkumar Ramachandra <redacted>
---
 cache.h     |    1 +
 sha1_name.c |   23 +++++++++++++++++++----
 2 files changed, 20 insertions(+), 4 deletions(-)

diff --git a/cache.h b/cache.h
index e5e1aa4..e52c354 100644
--- a/cache.h
+++ b/cache.h

@@ -813,6 +813,7 @@ struct object_context {
 };
 
 extern int get_sha1(const char *str, unsigned char *sha1);
+extern int getn_sha1(const char *name, int namelen, unsigned char *sha1);
 extern int get_sha1_with_mode_1(const char *str, unsigned char *sha1, unsigned *mode, int only_to_die, const char *prefix);
 static inline int get_sha1_with_mode(const char *str, unsigned char *sha1, unsigned *mode)
 {

diff --git a/sha1_name.c b/sha1_name.c
index 03ffc2c..31d412e 100644
--- a/sha1_name.c
+++ b/sha1_name.c

@@ -1019,12 +1019,11 @@ static char *resolve_relative_path(const char *rel)
 			   rel);
 }
 
-int get_sha1_with_context_1(const char *name, unsigned char *sha1,
-			    struct object_context *oc,
-			    int only_to_die, const char *prefix)
+static int getn_sha1_with_context_1(const char *name, int namelen,
+				unsigned char *sha1, struct object_context *oc,
+				int only_to_die, const char *prefix)
 {
 	int ret, bracket_depth;
-	int namelen = strlen(name);
 	const char *cp;
 
 	memset(oc, 0, sizeof(*oc));

@@ -1134,3 +1133,19 @@ int get_sha1_with_context_1(const char *name, unsigned char *sha1,
 	}
 	return ret;
 }
+
+int get_sha1_with_context_1(const char *name, unsigned char *sha1,
+			struct object_context *oc,
+			int only_to_die, const char *prefix)
+{
+	int namelen = strlen(name);
+	return getn_sha1_with_context_1(name, namelen, sha1,
+					oc, only_to_die, prefix);
+}
+
+/* A variant of get_sha1 that takes a length. */
+int getn_sha1(const char *name, int namelen, unsigned char *sha1)
+{
+	struct object_context unused;
+	return getn_sha1_with_context_1(name, namelen, sha1, &unused, 0, NULL);
+}

-- 
1.7.8.1.362.g5d6df.dirty

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 <redacted>
---
 sequencer.c |   12 +++---------
 1 files changed, 3 insertions(+), 9 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index a37846a..458cffb 100644
--- a/sequencer.c
+++ b/sequencer.c

@@ -517,8 +517,7 @@ static struct commit *parse_insn_line(char *bol, char *eol, struct replay_opts *
 {
 	unsigned char commit_sha1[20];
 	enum replay_action action;
-	char *end_of_object_name;
-	int saved, status, padding;
+	int namelen, padding;
 
 	if (!prefixcmp(bol, "pick")) {
 		action = REPLAY_PICK;

@@ -535,12 +534,6 @@ static struct commit *parse_insn_line(char *bol, char *eol, struct replay_opts *
 		return NULL;
 	bol += padding;
 
-	end_of_object_name = bol + strcspn(bol, " \t\n");
-	saved = *end_of_object_name;
-	*end_of_object_name = '\0';
-	status = get_sha1(bol, commit_sha1);
-	*end_of_object_name = saved;
-
 	/*
 	 * Verify that the action matches up with the one in
 	 * opts; we don't support arbitrary instructions

@@ -552,8 +545,8 @@ static struct commit *parse_insn_line(char *bol, char *eol, struct replay_opts *
 		return NULL;
 	}
 
-	if (status < 0)
+	namelen = strcspn(bol, " \t\n");
+	if (getn_sha1(bol, namelen, commit_sha1))
 		return NULL;
 
 	return lookup_commit_reference(commit_sha1);

-- 
1.7.8.1.362.g5d6df.dirty

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 [off-list ref]
Helped-by: Jonathan Nieder [off-list ref]
Signed-off-by: Ramkumar Ramachandra <redacted>
---
 sequencer.c |   45 +++++++++++++++++++++++++++++++++------------
 1 files changed, 33 insertions(+), 12 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 02e58ad..cb93ee8 100644
--- a/sequencer.c
+++ b/sequencer.c

@@ -513,7 +513,22 @@ static int format_todo(struct strbuf *buf, struct commit_list *todo_list,
 	return 0;
 }
 
-static struct commit *parse_insn_line(char *bol, char *eol, struct replay_opts *opts)
+static int parse_error(const char *message, const char *file,
+		int lineno, char *error_line)
+{
+	const char *suffix = "";
+	int error_len = strcspn(error_line, " \t\n");
+
+	if (error_len > 20) {
+		error_len = 20;
+		suffix = "...";
+	}
+	return error(_("%s:%d: %s: %.*s%s"), file, lineno, message,
+		error_len, error_line, suffix);
+}
+
+int parse_insn_line(char *bol, char *eol, struct replay_opts *opts,
+		struct commit_list *list, int lineno)
 {
 	unsigned char commit_sha1[20];
 	enum replay_action action;

@@ -526,7 +541,8 @@ static struct commit *parse_insn_line(char *bol, char *eol, struct replay_opts *
 		action = REPLAY_REVERT;
 		bol += strlen("revert");
 	} else
-		return NULL;
+		return parse_error(_("unrecognized action"),
+				git_path(SEQ_TODO_FILE), lineno, bol);
 
 	/* Eat up extra spaces/ tabs before object name */
 	bol += strspn(bol, " \t");

@@ -538,32 +554,36 @@ static struct commit *parse_insn_line(char *bol, char *eol, struct replay_opts *
 	if (action != opts->action) {
 		const char *action_str;
 		action_str = action == REPLAY_REVERT ? "revert" : "cherry-pick";
-		error(_("Cannot %s during a %s"), action_str, action_name(opts));
-		return NULL;
+		return parse_error(_("invalid action"),
+				git_path(SEQ_TODO_FILE), lineno, bol);
 	}
 
 	namelen = strcspn(bol, " \t\n");
 	if (getn_sha1(bol, namelen, commit_sha1))
-		return NULL;
-
-	return lookup_commit_reference(commit_sha1);
+		return parse_error(_("malformed object name"),
+				git_path(SEQ_TODO_FILE), lineno, bol);
+
+	list->item = lookup_commit_reference(commit_sha1);
+	if (!list->item)
+		return parse_error(_("not a valid commit"),
+				git_path(SEQ_TODO_FILE), lineno, bol);
+	list->next = NULL;
+	return 0;
 }
 
 static int parse_insn_buffer(char *buf, struct commit_list **todo_list,
 			struct replay_opts *opts)
 {
 	struct commit_list **next = todo_list;
-	struct commit *commit;
+	struct commit_list list;
 	char *p = buf;
 	int i;
 
 	for (i = 1; *p; i++) {
 		char *eol = strchrnul(p, '\n');
-		commit = parse_insn_line(p, eol, opts);
-		if (!commit)
-			return error(_("Could not parse line %d."), i);
-		next = commit_list_append(commit, next);
+		if (parse_insn_line(p, eol, opts, &list, i))
+			return -1;
+		next = commit_list_append(list.item, next);
 		p = *eol ? eol + 1 : eol;
 	}
 	if (!*todo_list)

-- 
1.7.8.1.362.g5d6df.dirty

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

diff --git a/sequencer.c b/sequencer.c
index 458cffb..02e58ad 100644
--- a/sequencer.c
+++ b/sequencer.c

@@ -517,22 +517,19 @@ static struct commit *parse_insn_line(char *bol, char *eol, struct replay_opts *
 {
 	unsigned char commit_sha1[20];
 	enum replay_action action;
-	int namelen, padding;
+	int namelen;
 
-	if (!prefixcmp(bol, "pick")) {
+	if (!prefixcmp(bol, "pick ") || !prefixcmp(bol, "pick\t")) {
 		action = REPLAY_PICK;
 		bol += strlen("pick");
-	} else if (!prefixcmp(bol, "revert")) {
+	} else if (!prefixcmp(bol, "revert ") || !prefixcmp(bol, "revert\t")) {
 		action = REPLAY_REVERT;
 		bol += strlen("revert");
 	} else
 		return NULL;
 
 	/* Eat up extra spaces/ tabs before object name */
-	padding = strspn(bol, " \t");
-	if (!padding)
-		return NULL;
-	bol += padding;
+	bol += strspn(bol, " \t");
 
 	/*
 	 * Verify that the action matches up with the one in

-- 
1.7.8.1.362.g5d6df.dirty

(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:

[Subject: sha1_name: introduce getn_sha1() to take length]

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

Hi Jonathan,

Jonathan Nieder wrote:

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.

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

Thanks.

    Ram

Ramkumar Ramachandra wrote:

Makes a lot of sense.

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