[GSoC update] Iterating over a stable series

All of lore.kernel.org
 help / color / mirror / Atom feed

* [GSoC update] Iterating over a stable series
@ 2011-08-02  4:54 Ramkumar Ramachandra
  2011-08-02  4:54 ` [RFC/ PATCH] revert: Allow arbitrary sequencer instructions Ramkumar Ramachandra
  2011-08-02  5:01 ` [GSoC update] Iterating over a stable series Ramkumar Ramachandra
  0 siblings, 2 replies; 6+ messages in thread
From: Ramkumar Ramachandra @ 2011-08-02  4:54 UTC (permalink / raw)
  To: Git List
  Cc: Junio C Hamano, Jonathan Nieder, Christian Couder,
	Daniel Barkalow, Jeff King

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [RFC/ PATCH] revert: Allow arbitrary sequencer instructions
  2011-08-02  4:54 [GSoC update] Iterating over a stable series Ramkumar Ramachandra
@ 2011-08-02  4:54 ` Ramkumar Ramachandra
  2011-08-02  7:52   ` Christian Couder
  2011-08-02 20:53   ` Jonathan Nieder
  2011-08-02  5:01 ` [GSoC update] Iterating over a stable series Ramkumar Ramachandra
  1 sibling, 2 replies; 6+ messages in thread
From: Ramkumar Ramachandra @ 2011-08-02  4:54 UTC (permalink / raw)
  To: Git List
  Cc: Junio C Hamano, Jonathan Nieder, Christian Couder,
	Daniel Barkalow, Jeff King

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

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [RFC/ PATCH] revert: Allow arbitrary sequencer instructions
  2011-08-02  4:54 ` [RFC/ PATCH] revert: Allow arbitrary sequencer instructions Ramkumar Ramachandra
@ 2011-08-02  7:52   ` Christian Couder
  2011-08-02 20:53   ` Jonathan Nieder
  1 sibling, 0 replies; 6+ messages in thread
From: Christian Couder @ 2011-08-02  7:52 UTC (permalink / raw)
  To: Ramkumar Ramachandra
  Cc: Git List, Junio C Hamano, Jonathan Nieder, Christian Couder,
	Daniel Barkalow, Jeff King

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.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [RFC/ PATCH] revert: Allow arbitrary sequencer instructions
  2011-08-02  4:54 ` [RFC/ PATCH] revert: Allow arbitrary sequencer instructions Ramkumar Ramachandra
  2011-08-02  7:52   ` Christian Couder
@ 2011-08-02 20:53   ` Jonathan Nieder
  2011-08-03  1:32     ` Ramkumar Ramachandra
  1 sibling, 1 reply; 6+ messages in thread
From: Jonathan Nieder @ 2011-08-02 20:53 UTC (permalink / raw)
  To: Ramkumar Ramachandra
  Cc: Git List, Junio C Hamano, Christian Couder, Daniel Barkalow,
	Jeff King

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.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [RFC/ PATCH] revert: Allow arbitrary sequencer instructions
  2011-08-02 20:53   ` Jonathan Nieder
@ 2011-08-03  1:32     ` Ramkumar Ramachandra
  0 siblings, 0 replies; 6+ messages in thread
From: Ramkumar Ramachandra @ 2011-08-03  1:32 UTC (permalink / raw)
  To: Jonathan Nieder
  Cc: Git List, Junio C Hamano, Christian Couder, Daniel Barkalow,
	Jeff King

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [GSoC update] Iterating over a stable series
  2011-08-02  4:54 [GSoC update] Iterating over a stable series Ramkumar Ramachandra
  2011-08-02  4:54 ` [RFC/ PATCH] revert: Allow arbitrary sequencer instructions Ramkumar Ramachandra
@ 2011-08-02  5:01 ` Ramkumar Ramachandra
  1 sibling, 0 replies; 6+ messages in thread
From: Ramkumar Ramachandra @ 2011-08-02  5:01 UTC (permalink / raw)
  To: Git List
  Cc: Junio C Hamano, Jonathan Nieder, Christian Couder,
	Daniel Barkalow, Jeff King

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2011-08-03  1:34 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-08-02  4:54 [GSoC update] Iterating over a stable series Ramkumar Ramachandra
2011-08-02  4:54 ` [RFC/ PATCH] revert: Allow arbitrary sequencer instructions Ramkumar Ramachandra
2011-08-02  7:52   ` Christian Couder
2011-08-02 20:53   ` Jonathan Nieder
2011-08-03  1:32     ` Ramkumar Ramachandra
2011-08-02  5:01 ` [GSoC update] Iterating over a stable series Ramkumar Ramachandra

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.