[GSoC update] Iterating over a stable series

[GSoC update] Iterating over a stable series

[Ramkumar Ramachandra]

Hi,

The post midterm work is suffering because I'm constantly rewriting
the sequencer-stable series: I often resort to throwing away big
patches that move functions from builtin/revert.c to sequencer.c.
Hopefully, the latest iteration [1] will not require rewriting.

I'd like some early feedback for one of the "design patches" in my new
series: I've chosen to use a commit + action to represent a todo_list.
I'd initially tried a commit + opts keeping future expantion in mind
(allowing instruction-specific command-line options), but the result
is quite inelegant.  Although commit message/ tests are missing, I'd
like to describe the intent in detail:

This patch is a prerequisite for decoupling todo parsing from opts
parsing.  I want to decouple them so that I can achieve tighter
coupling between "git commit" and the sequencer [2].  After that, I
want to craft a nice API and move/ expose various functions in
builtin/revert.c starting with the parsing functions.

Thanks for reading.

[1]: http://thread.gmane.org/gmane.comp.version-control.git/178372
[2]: http://mid.gmane.org/CALkWK0=9kwgtZB-BA12tOQrQXS8XRbhTg6K=Ak00o2nurX16Fg@mail.gmail.com

Ramkumar Ramachandra (1):
  revert: Allow arbitrary sequencer instructions

 builtin/revert.c |  101 +++++++++++++++++++++++++++++-------------------------
 sequencer.h      |   10 +++++
 2 files changed, 64 insertions(+), 47 deletions(-)

-- 
1.7.6.351.gb35ac.dirty

[RFC/ PATCH] revert: Allow arbitrary sequencer instructions

[Ramkumar Ramachandra]

Allow arbitrary sequencer instructions in the instruction sheet.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 builtin/revert.c |  101 +++++++++++++++++++++++++++++-------------------------
 sequencer.h      |   10 +++++
 2 files changed, 64 insertions(+), 47 deletions(-)

diff --git a/builtin/revert.c b/builtin/revert.c
index bca1490..127b97e 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -646,49 +646,54 @@ static void read_and_refresh_cache(struct replay_opts *opts)
  *     assert(commit_list_count(list) == 2);
  *     return list;
  */
-struct commit_list **commit_list_append(struct commit *commit,
-					struct commit_list **next)
+struct replay_insn_list **replay_insn_list_append(struct replay_insn *insn,
+						struct replay_insn_list **next)
 {
-	struct commit_list *new = xmalloc(sizeof(struct commit_list));
-	new->item = commit;
+	struct replay_insn_list *new = xmalloc(sizeof(struct replay_insn_list));
+	new->item = insn;
 	*next = new;
 	new->next = NULL;
 	return &new->next;
 }
 
-static int format_todo(struct strbuf *buf, struct commit_list *todo_list,
-		struct replay_opts *opts)
+static int format_todo(struct strbuf *buf, struct replay_insn_list *todo_list)
 {
-	struct commit_list *cur = NULL;
+	struct replay_insn_list *cur = NULL;
+	struct commit *commit;
 	struct commit_message msg = { NULL, NULL, NULL, NULL, NULL };
 	const char *sha1_abbrev = NULL;
-	const char *action_str = opts->action == REVERT ? "revert" : "pick";
+	const char *action_str;
 
 	for (cur = todo_list; cur; cur = cur->next) {
-		sha1_abbrev = find_unique_abbrev(cur->item->object.sha1, DEFAULT_ABBREV);
-		if (get_message(cur->item, &msg))
+		commit = cur->item->commit;
+		action_str = cur->item->action == REVERT ? "revert" : "pick";
+
+		sha1_abbrev = find_unique_abbrev(commit->object.sha1, DEFAULT_ABBREV);
+		if (get_message(commit, &msg))
 			return error(_("Cannot get commit message for %s"), sha1_abbrev);
 		strbuf_addf(buf, "%s %s %s\n", action_str, sha1_abbrev, msg.subject);
 	}
 	return 0;
 }
 
-static struct commit *parse_insn_line(char *start, struct replay_opts *opts)
+static struct replay_insn *parse_insn_line(char *start)
 {
 	unsigned char commit_sha1[20];
 	char sha1_abbrev[40];
+	struct commit *commit;
+	struct replay_insn *insn;
 	enum replay_action action;
-	int insn_len = 0;
+	int keyword_len;
 	char *p, *q;
 
 	if (!prefixcmp(start, "pick ")) {
 		action = CHERRY_PICK;
-		insn_len = strlen("pick");
-		p = start + insn_len + 1;
+		keyword_len = strlen("pick");
+		p = start + keyword_len + 1;
 	} else if (!prefixcmp(start, "revert ")) {
 		action = REVERT;
-		insn_len = strlen("revert");
-		p = start + insn_len + 1;
+		keyword_len = strlen("revert");
+		p = start + keyword_len + 1;
 	} else
 		return NULL;
 
@@ -699,30 +704,25 @@ static struct commit *parse_insn_line(char *start, struct replay_opts *opts)
 
 	strlcpy(sha1_abbrev, p, q - p);
 
-	/*
-	 * Verify that the action matches up with the one in
-	 * opts; we don't support arbitrary instructions
-	 */
-	if (action != opts->action) {
-		const char *action_str;
-		action_str = action == REVERT ? "revert" : "cherry-pick";
-		error(_("Cannot %s during a %s"), action_str, action_name(opts));
-		return NULL;
-	}
-
 	if (get_sha1(sha1_abbrev, commit_sha1) < 0)
 		return NULL;
 
-	return lookup_commit_reference(commit_sha1);
+	commit = lookup_commit_reference(commit_sha1);
+	if (commit) {
+		insn = xmalloc(sizeof(struct replay_insn));
+		insn->commit = commit;
+		insn->action = action;
+		return insn;
+	}
+	return NULL;
 }
 
-static void read_populate_todo(struct commit_list **todo_list,
-			struct replay_opts *opts)
+static void read_populate_todo(struct replay_insn_list **todo_list)
 {
 	const char *todo_file = git_path(SEQ_TODO_FILE);
 	struct strbuf buf = STRBUF_INIT;
-	struct commit_list **next;
-	struct commit *commit;
+	struct replay_insn *insn;
+	struct replay_insn_list **next;
 	char *p;
 	int fd;
 
@@ -738,10 +738,10 @@ static void read_populate_todo(struct commit_list **todo_list,
 
 	next = todo_list;
 	for (p = buf.buf; *p; p = strchr(p, '\n') + 1) {
-		commit = parse_insn_line(p, opts);
-		if (!commit)
+		insn = parse_insn_line(p);
+		if (!insn)
 			goto error;
-		next = commit_list_append(commit, next);
+		next = replay_insn_list_append(insn, next);
 	}
 	if (!*todo_list)
 		goto error;
@@ -795,18 +795,23 @@ static void read_populate_opts(struct replay_opts **opts_ptr)
 		die(_("Malformed options sheet: %s"), opts_file);
 }
 
-static void walk_revs_populate_todo(struct commit_list **todo_list,
+static void walk_revs_populate_todo(struct replay_insn_list **todo_list,
 				struct replay_opts *opts)
 {
 	struct rev_info revs;
 	struct commit *commit;
-	struct commit_list **next;
+	struct replay_insn *insn;
+	struct replay_insn_list **next;
 
 	prepare_revs(&revs, opts);
 
 	next = todo_list;
-	while ((commit = get_revision(&revs)))
-		next = commit_list_append(commit, next);
+	while ((commit = get_revision(&revs))) {
+		insn = xmalloc(sizeof(struct replay_insn));
+		insn->commit = commit;
+		insn->action = opts->action;
+		next = replay_insn_list_append(insn, next);
+	}
 }
 
 static int create_seq_dir(void)
@@ -835,7 +840,7 @@ static void save_head(const char *head)
 		die(_("Error wrapping up %s."), head_file);
 }
 
-static void save_todo(struct commit_list *todo_list, struct replay_opts *opts)
+static void save_todo(struct replay_insn_list *todo_list)
 {
 	const char *todo_file = git_path(SEQ_TODO_FILE);
 	static struct lock_file todo_lock;
@@ -843,7 +848,7 @@ static void save_todo(struct commit_list *todo_list, struct replay_opts *opts)
 	int fd;
 
 	fd = hold_lock_file_for_update(&todo_lock, todo_file, LOCK_DIE_ON_ERROR);
-	if (format_todo(&buf, todo_list, opts) < 0)
+	if (format_todo(&buf, todo_list) < 0)
 		die(_("Could not format %s."), todo_file);
 	if (write_in_full(fd, buf.buf, buf.len) < 0) {
 		strbuf_release(&buf);
@@ -887,9 +892,10 @@ static void save_opts(struct replay_opts *opts)
 	}
 }
 
-static int pick_commits(struct commit_list *todo_list, struct replay_opts *opts)
+static int pick_commits(struct replay_insn_list *todo_list,
+			struct replay_opts *opts)
 {
-	struct commit_list *cur;
+	struct replay_insn_list *cur;
 	int res;
 
 	setenv(GIT_REFLOG_ACTION, action_name(opts), 0);
@@ -899,8 +905,9 @@ static int pick_commits(struct commit_list *todo_list, struct replay_opts *opts)
 	read_and_refresh_cache(opts);
 
 	for (cur = todo_list; cur; cur = cur->next) {
-		save_todo(cur, opts);
-		res = do_pick_commit(cur->item, opts);
+		save_todo(cur);
+		opts->action = cur->item->action;
+		res = do_pick_commit(cur->item->commit, opts);
 		if (res) {
 			if (!cur->next)
 				/*
@@ -925,7 +932,7 @@ static int pick_commits(struct commit_list *todo_list, struct replay_opts *opts)
 
 static int pick_revisions(struct replay_opts *opts)
 {
-	struct commit_list *todo_list = NULL;
+	struct replay_insn_list *todo_list = NULL;
 	unsigned char sha1[20];
 
 	read_and_refresh_cache(opts);
@@ -942,7 +949,7 @@ static int pick_revisions(struct replay_opts *opts)
 		if (!file_exists(git_path(SEQ_TODO_FILE)))
 			goto error;
 		read_populate_opts(&opts);
-		read_populate_todo(&todo_list, opts);
+		read_populate_todo(&todo_list);
 
 		/* Verify that the conflict has been resolved */
 		if (!index_differs_from("HEAD", 0))
diff --git a/sequencer.h b/sequencer.h
index 931733c..eebee65 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -32,6 +32,16 @@ struct replay_opts {
 	size_t xopts_nr, xopts_alloc;
 };
 
+struct replay_insn {
+	struct commit *commit;
+	enum replay_action action;
+};
+
+struct replay_insn_list {
+	struct replay_insn *item;
+	struct replay_insn_list *next;
+};
+
 /*
  * Removes SEQ_OLD_DIR and renames SEQ_DIR to SEQ_OLD_DIR, ignoring
  * any errors.  Intended to be used by 'git reset'.
-- 
1.7.6.351.gb35ac.dirty

Re: [GSoC update] Iterating over a stable series

[Ramkumar Ramachandra]

Hi,

Ramkumar Ramachandra writes:
> Subject: [GSoC update] Iterating over a stable series

Er, I just noticed that another GSoC update from me has a very similar
subject and it refers to something entirely different.  My apologies.
A more appropriate subject: Building on rr/revert-cherry-pick-continue
(or what I call sequencer-stable).

-- Ram

Re: [RFC/ PATCH] revert: Allow arbitrary sequencer instructions

[Christian Couder]

Hi Ram,

On Tue, Aug 2, 2011 at 6:54 AM, Ramkumar Ramachandra <artagnon@gmail.com> wrote:
>
>        if (get_sha1(sha1_abbrev, commit_sha1) < 0)
>                return NULL;
>
> -       return lookup_commit_reference(commit_sha1);
> +       commit = lookup_commit_reference(commit_sha1);
> +       if (commit) {
> +               insn = xmalloc(sizeof(struct replay_insn));
> +               insn->commit = commit;
> +               insn->action = action;
> +               return insn;
> +       }
> +       return NULL;
>  }

I'd suggest:

       commit = lookup_commit_reference(commit_sha1);
       if (!commit)
               return NULL;

       insn = xmalloc(sizeof(struct replay_insn));
       insn->commit = commit;
       insn->action = action;
       return insn;

Thanks,
Christian.

Re: [RFC/ PATCH] revert: Allow arbitrary sequencer instructions

[Jonathan Nieder]

Ramkumar Ramachandra wrote:

> Allow arbitrary sequencer instructions in the instruction sheet.

"So now I can ..." wait, what does this allow me to do?  Your audience
hasn't read the patch yet.

> --- a/sequencer.h
> +++ b/sequencer.h
> @@ -32,6 +32,16 @@ struct replay_opts {
>  	size_t xopts_nr, xopts_alloc;
>  };
>  
> +struct replay_insn {
> +	struct commit *commit;
> +	enum replay_action action;
> +};
> +
> +struct replay_insn_list {
> +	struct replay_insn *item;
> +	struct replay_insn_list *next;
> +};

Ah, so this allows sequences like

	revert A
	pick B
	pick C
	revert D

Nit: why isn't the list-item struct something like

	struct replay_insn item;
	struct replay_insn_list *next;

which would save a little memory management and memory access
overhead (or even

	enum replay_action action;
	struct commit *operand;
	struct replay_insn_list *next;

since every "struct replay_insn" exists in the context of an
insn list afaict)?

Anyway, the general idea seems good.

Re: [RFC/ PATCH] revert: Allow arbitrary sequencer instructions

[Ramkumar Ramachandra]

Hi Jonathan,

Jonathan Nieder writes:
> Ramkumar Ramachandra wrote:
>> --- a/sequencer.h
>> +++ b/sequencer.h
>> @@ -32,6 +32,16 @@ struct replay_opts {
>>       size_t xopts_nr, xopts_alloc;
>>  };
>>
>> +struct replay_insn {
>> +     struct commit *commit;
>> +     enum replay_action action;
>> +};
>> +
>> +struct replay_insn_list {
>> +     struct replay_insn *item;
>> +     struct replay_insn_list *next;
>> +};
>
> Ah, so this allows sequences like
>
>        revert A
>        pick B
>        pick C
>        revert D

Exactly.

> Nit: why isn't the list-item struct something like
>
>        struct replay_insn item;
>        struct replay_insn_list *next;
>
> which would save a little memory management and memory access
> overhead (or even
>
>        enum replay_action action;
>        struct commit *operand;
>        struct replay_insn_list *next;
>
> since every "struct replay_insn" exists in the context of an
> insn list afaict)?

Right.  Good suggestion.

> Anyway, the general idea seems good.

Nice.  Thanks for the quick early feedback.  Much appreciated.

-- Ram