Re: [PATCH v2 01/34] sequencer: support a new action: 'interactive rebase'

[Junio C Hamano <gitster@pobox.com>]

Johannes Schindelin <johannes.schindelin@gmx.de> writes:

>  static inline int is_rebase_i(const struct replay_opts *opts)
>  {
> -	return 0;
> +	return opts->action == REPLAY_INTERACTIVE_REBASE;
>  }
>  
>  static const char *get_dir(const struct replay_opts *opts)
>  {
> +	if (is_rebase_i(opts))
> +		return rebase_path();
>  	return git_path_seq_dir();
>  }
>  
>  static const char *get_todo_path(const struct replay_opts *opts)
>  {
> +	if (is_rebase_i(opts))
> +		return rebase_path_todo();
>  	return git_path_todo_file();
>  }
>  
> @@ -168,7 +179,15 @@ int sequencer_remove_state(struct replay_opts *opts)
>  
>  static const char *action_name(const struct replay_opts *opts)
>  {
> -	return opts->action == REPLAY_REVERT ? N_("revert") : N_("cherry-pick");
> +	switch (opts->action) {
> +	case REPLAY_REVERT:
> +		return N_("revert");
> +	case REPLAY_PICK:
> +		return N_("cherry-pick");
> +	case REPLAY_INTERACTIVE_REBASE:
> +		return N_("rebase -i");
> +	}
> +	die(_("Unknown action: %d"), opts->action);
>  }

This case statement which looks perfectly sensible---it says that
there are three equal modes the subsystem operates in.  

This is just a mental note and not a suggestion to change anything
immediately, but it makes me wonder if git_dir/get_todo_path would
also want to do so, moving towards retiring is_rebase_i() which is
"everything else vs one oddball which is rebase-i" mindset.

> @@ -395,7 +414,10 @@ static int do_recursive_merge(struct commit *base, struct commit *next,
>  
>  	if (active_cache_changed &&
>  	    write_locked_index(&the_index, &index_lock, COMMIT_LOCK))
> -		/* TRANSLATORS: %s will be "revert" or "cherry-pick" */
> +		/*
> +		 * TRANSLATORS: %s will be "revert", "cherry-pick" or
> +		 * "rebase -i".
> +		 */

IIRC, the "TRANSLATORS:" comment has to deviate from our coding
style due to tool limitation and has to be done like this:

> +		/* TRANSLATORS: %s will be "revert", "cherry-pick" or
> +		 * "rebase -i".
> +		 */

> @@ -1204,6 +1226,9 @@ static int save_todo(struct todo_list *todo_list, struct replay_opts *opts)
>  	const char *todo_path = get_todo_path(opts);
>  	int next = todo_list->current, offset, fd;
>  
> +	if (is_rebase_i(opts))
> +		next++;
> +

This is because...?  Everybody else counts 0-based while rebase-i
counts from 1 or something?

>  	fd = hold_lock_file_for_update(&todo_lock, todo_path, 0);
>  	if (fd < 0)
>  		return error_errno(_("could not lock '%s'"), todo_path);

Everything else in the patch is understandable.  This bit isn't
without explanation, at least to me.