Re: [PATCH v2 01/34] sequencer: support a new action: 'interactive rebase'
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Dec 13, 2016, 20:32 UTC
- Message-ID
- <xmqqk2b31sfs.fsf@gitster.mtv.corp.google.com>
- In-Reply-To
- <297140020a7312af03136848dcdd0353ee3abdfe.1481642927.git.johannes.schindelin@gmx.de>
Johannes Schindelin <johannes.schindelin@gmx.de> writes:
Show 35 quoted lines
> 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.
Show 9 quoted lines
> @@ -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". > + */
Show 7 quoted lines
> @@ -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.