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

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi Stephan,

On Sat, 17 Dec 2016, Stephan Beyer wrote:

> On 12/14/2016 08:29 PM, Junio C Hamano wrote:
> > Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> >> -/* We will introduce the 'interactive rebase' mode later */
> >>  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();
> >>  }
> > 
> > This obviously makes the assumption made by 39784cd362 ("sequencer:
> > remove useless get_dir() function", 2016-12-07) invalid, where the
> > "remove useless" thought that the callers of this function wants a
> > single answer that does not depend on opts.
> > 
> > I'll revert that commit from the sb/sequencer-abort-safety topic (as
> > the topic is in 'next' already) to keep this one.  Please holler if
> > that is a mistaken decision.
> 
> It seems to be the right decision if one wants to keep the internal-path
> backwards-compatibility of "git rebase -i" (and it seems that Dscho
> wants to keep it).

You make it sound as if I had any choice there. But you probably know as
well as I do that scripted usage of rebase -i relies on the "internal-path
backwards-compatibility", and as one of my stated goals was not to break
anybody's existing rebase -i usage, well, you know.

> However, maintaining more than one directory (like "sequencer" for
> sequencer state and "rebase-merge" for rebase todo and log) can cause
> the necessity to be even more careful when hacking on sequencer... For
> example, the cleanup code must delete both of them, not only one of them.

That is incorrect. It depends on the options which directory is used. And
it is that directory (and not both) that should be cleaned up in the end.

Otherwise you run into a ton of pain e.g. when running a rebase -i with an
`exec git cherry-pick ...` line: all of a sudden, that little innocuous
line would simply destroy the state directory of the current rebase -i.

That's a rather big no-no.

> Hence, I think that it's a wiser choice to keep one directory for
> sequencer state *and* additional files.

See above for a good reason not to choose that.

> If we (a) save everything in "sequencer", we break the internal-path
> backwards-compatbility but we can get rid of get_dir()...

... and we break power users' scripts that use rebase -i. Making those
power users very angry.

Including me.

> If we (b) save everything in "rebase-merge" when using rebase -i and
> save everything in "sequencer" for pick/revert, we are 100%
> backwards-compatible, but we have to change every occurrence of
> git_path_seq_dir() to get_dir()

Yes, that is exactly what we have to do. And that is exactly what I did,
too, in the patch series I labeled "prepare-sequencer".

The only three uses of git_path_seq_dir() I left intact are:

- in builtin/commit.c, where it is used to determine whether a cherry-pick
  is really still in progress, after testing that there is a
  CHERRY_PICK_HEAD file

- in get_dir() (obviously)

- in create_seq_dir(), where we could technically avoid calling the
  function a whopping three times and use a parameter instead (not my
  construction site, though, so I leave this alone)

> and the GIT_PATH_FUNC definitions of git_path_{todo,opts,head}_file must
> be replaced by definitions using get_dir().

That is incorrect. The git_path_todo(), git_path_opts() and
git_path_head() functions need to be used in the cherry-pick/revert code
path. So they need to be left as-are.

Ciao,
Dscho