[PATCH 0/4] Minor sequencer.c cleanups

[PATCH 0/4] Minor sequencer.c cleanups

[Ramkumar Ramachandra]

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

[PATCH 1/4] sha1_name: introduce getn_sha1() to take length

[Ramkumar Ramachandra]

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

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

Re: [PATCH 1/4] sha1_name: introduce getn_sha1() to take length

[Jonathan Nieder]

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

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

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

Re: [PATCH 1/4] sha1_name: introduce getn_sha1() to take length

[Matthieu Moy]

Jonathan Nieder <jrnieder@gmail.com> writes:

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

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

> Holy cow this function is going crazy.  So let's take a survey of
> the public functions in this family.
>
>  [... nice explanations of different functions ...]

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

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

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.

> "get_sha1_with_context_1" has no external callers so it could probably
> be made private.

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.

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

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.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH 1/4] sha1_name: introduce getn_sha1() to take length

[Ramkumar Ramachandra]

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

Re: [PATCH 1/4] sha1_name: introduce getn_sha1() to take length

[Jonathan Nieder]

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

[PATCH 2/4] revert: use getn_sha1() to simplify insn parsing

[Ramkumar Ramachandra]

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

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

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

[Ramkumar Ramachandra]

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

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

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

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> 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@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 ...

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

[Ramkumar Ramachandra]

Hi Junio,

Junio C Hamano wrote:
> 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...

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

[PATCH 4/4] revert: report fine-grained errors from insn parser

[Ramkumar Ramachandra]

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

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