git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH 10/13] revert: Persist data for continuation

From
Junio C Hamano <gitster@pobox.com>
Date
Jun 21, 2011, 19:49 UTC
Message-ID
<7v39j37ynu.fsf@alter.siamese.dyndns.org>
In-Reply-To
<1308661489-20080-11-git-send-email-artagnon@gmail.com>
Ramkumar Ramachandra <artagnon@gmail.com> writes:
> ...  Don't touch
> CHERRY_PICK_HEAD -- it will still be useful when a conflict is
> encountered.

What does "Don't touch" here mean? Keep doing the same thing as before so that people can rely on it? Or stop touching it? I presume the former, but the description is suboptimal.

Show 7 quoted lines
> @@ -25,6 +26,10 @@
>   * Copyright (c) 2005 Junio C Hamano
>   */
>  
> +#define SEQ_DIR		git_path("sequencer")
> +#define HEAD_FILE	git_path("sequencer/head")
> +#define TODO_FILE	git_path("sequencer/todo")

HEAD is to keep track of the original, or does it get updated during a sequencer run?

Show 21 quoted lines
> +static void walk_revs_populate_todo(struct commit_list **todo_list,
> +				struct replay_opts *opts)
>  {
>  	struct rev_info revs;
>  	struct commit *commit;
> +	struct commit_list *new_item;
> +	struct commit_list *cur = NULL;
> +
> +	/* Insert into todo_list in the same order */
> +	prepare_revs(&revs, opts);
> +	while ((commit = get_revision(&revs))) {
> +		new_item = xmalloc(sizeof(struct commit_list));
> +		new_item->item = commit;
> +		if (cur)
> +			cur->next = new_item;
> +		else
> +			*todo_list = new_item;
> +		cur = new_item;
> +	}
> +	cur->next = NULL;
> +}

Yuck; you do not want if/else to do this (I am _not_ saying "you can do that but here is a better way"). Have a variable that points at a pointer of type "struct commit_list *", initialize it to where todo_list points at, and then update to point at the next field of the new object.

Something like this:
	next = todo_list;
	while ((commit = ...) != NULL) {
		new = xcalloc(1, sizeof(struct commit_list));
                new->item = commit;
                *next = new;
                next = &new->next;
	}
	*next = NULL;

Doing it this way would avoid a segfault when get_revision() returned nothing as an added bonus ;-).

> +static void persist_head(const char *head)
s/persist/save/ perhaps?
Show 5 quoted lines
> +{
> +	static struct lock_file head_lock;
> +	struct strbuf buf = STRBUF_INIT;
> +	int fd;
> +
Show 11 quoted lines
> +	if (file_exists(SEQ_DIR)) {
> +		if (!is_directory(SEQ_DIR) && remove_path(SEQ_DIR) < 0) {
> +			strbuf_release(&buf);
> +			die(_("Could not remove %s"), SEQ_DIR);
> +		}
> +	} else {
> +		if (mkdir(SEQ_DIR, 0777) < 0) {
> +			strbuf_release(&buf);
> +			die_errno(_("Could not create sequencer directory '%s'."), SEQ_DIR);
> +		}
> +	}
Split this part into a separate helper.
Show 7 quoted lines
> +	fd = hold_lock_file_for_update(&head_lock, HEAD_FILE, LOCK_DIE_ON_ERROR);
> +	strbuf_addf(&buf, "%s\n", head);
> +	if (write_in_full(fd, buf.buf, buf.len) < 0)
> +		die_errno(_("Could not write to %s."), HEAD_FILE);
> +	if (commit_lock_file(&head_lock) < 0)
> +		die(_("Error wrapping up %s"), HEAD_FILE);
> +}

Is it sufficient to write the object name of the commit HEAD points at? Would it be useful to also record the branch if the HEAD points at one?

> +static void persist_todo(struct commit_list *todo_list, struct replay_opts *opts)
s/persist/save/ perhaps?
Show 5 quoted lines
> +{
> +	static struct lock_file todo_lock;
> +	struct strbuf buf = STRBUF_INIT;
> +	int fd;
> +	fd = hold_lock_file_for_update(&todo_lock, TODO_FILE, LOCK_DIE_ON_ERROR);

The current set of callers might always save head and then todo, but it probably is a good idea to lift that restriction (which is why I suggested to separate the initialization of seq-dir into a separate helper).

Show 10 quoted lines
> +static int pick_commits(struct replay_opts *opts)
> +{
> +	struct commit_list *todo_list = NULL;
> +	unsigned char sha1[20];
> +	struct commit_list *cur;
> +	int res;
>  
> -	setenv(GIT_REFLOG_ACTION, me, 0);
>  	read_and_refresh_cache(me, opts);
> +	setenv(GIT_REFLOG_ACTION, me, 0);
Why?
Show 18 quoted lines
> -	prepare_revs(&revs, opts);
> +	walk_revs_populate_todo(&todo_list, opts);
> +	if (!get_sha1("HEAD", sha1))
> +		persist_head(sha1_to_hex(sha1));
> +	persist_todo(todo_list, opts);
>  
> -	while ((commit = get_revision(&revs))) {
> -		int res = do_pick_commit(commit, opts);
> +	for (cur = todo_list; cur; cur = cur->next) {
> +		persist_todo(cur, opts);
> +		res = do_pick_commit(cur->item, opts);
>  		if (res)
>  			return res;
>  	}
>  
> -	return 0;
> +	/* Sequence of picks finished successfully; cleanup by
> +	   removing the .git/sequencer directory */
Just a style nit.
        /*
         * We write our multi-line comments
         * this way.
         */
> +	return cleanup_sequencer_data();
>  }
Previous: Jonathan NiederNext: Ramkumar Ramachandra
Message 37 of 47 in “Sequencer with continuation features”
  1. 00/13 Sequencer with continuation featuresRamkumar Ramachandra, Jun 21, 2011
  2. 01/13 advice: Introduce error_resolve_conflictRamkumar Ramachandra, Jun 21, 2011
  3. Jonathan NiederJun 21, 2011
  4. Junio C HamanoJun 21, 2011
  5. Ramkumar RamachandraJul 2, 2011
  6. Jonathan NiederJul 2, 2011
  7. 02/13 revert: Factor out add_message_to_msg functionRamkumar Ramachandra, Jun 21, 2011
  8. Jonathan NiederJun 21, 2011
  9. Junio C HamanoJun 21, 2011
  10. 03/13 revert: Don't check lone argument in get_encodingRamkumar Ramachandra, Jun 21, 2011
  11. Jonathan NiederJun 21, 2011
  12. 04/13 revert: Propogate errors upwards from do_pick_commitRamkumar Ramachandra, Jun 21, 2011
  13. Jonathan NiederJun 21, 2011
  14. Junio C HamanoJun 21, 2011
  15. Jonathan NiederJun 21, 2011
  16. Junio C HamanoJun 21, 2011
  17. Ramkumar RamachandraJul 2, 2011
  18. 05/13 revert: Eliminate global "commit" variableRamkumar Ramachandra, Jun 21, 2011
  19. Jonathan NiederJun 21, 2011
  20. Junio C HamanoJun 21, 2011
  21. 06/13 revert: Rename no_replay to record_originRamkumar Ramachandra, Jun 21, 2011
  22. 07/13 revert: Introduce struct to keep command-line optionsRamkumar Ramachandra, Jun 21, 2011
  23. Jonathan NiederJun 21, 2011
  24. Junio C HamanoJun 21, 2011
  25. 08/13 revert: Separate cmdline parsing from functional codeRamkumar Ramachandra, Jun 21, 2011
  26. Jonathan NiederJun 21, 2011
  27. 09/13 revert: Catch incompatible command-line options earlyRamkumar Ramachandra, Jun 21, 2011
  28. Jonathan NiederJun 21, 2011
  29. Ramkumar RamachandraJul 2, 2011
  30. Jonathan NiederJul 2, 2011
  31. Jonathan NiederJul 2, 2011
  32. Ramkumar RamachandraJul 2, 2011
  33. Jonathan NiederJul 2, 2011
  34. Jonathan NiederJul 2, 2011
  35. 10/13 revert: Persist data for continuationRamkumar Ramachandra, Jun 21, 2011
  36. Jonathan NiederJun 21, 2011
  37. Junio C HamanoJun 21, 2011
  38. 11/13 revert: Introduce a layer of indirection over pick_commitsRamkumar Ramachandra, Jun 21, 2011
  39. Junio C HamanoJun 21, 2011
  40. 12/13 revert: Introduce skip-all to cleanup sequencer dataRamkumar Ramachandra, Jun 21, 2011
  41. Junio C HamanoJun 21, 2011
  42. Ramkumar RamachandraJul 2, 2011
  43. Miles BaderJul 4, 2011
  44. Ramkumar RamachandraJul 5, 2011
  45. 13/13 revert: Introduce --continue to continue the operationRamkumar Ramachandra, Jun 21, 2011
  46. Jonathan NiederJun 21, 2011
  47. Jonathan NiederJun 21, 2011

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.