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

[Stephan Beyer]

Hi Dscho,

>> 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.

Ahh, I see, there seems to be a misunderstanding on my side about how
you did it (I did not look into the other patches (obviously)).

Thanks for clarifying!

Best
  Stephan

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

[Johannes Schindelin]

Hi Stephan,

On Sat, 17 Dec 2016, Stephan Beyer wrote:

> On 12/14/2016 08:29 PM, Junio C Hamano wrote:
> > Johannes Schindelin  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

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

[Johannes Schindelin]

Hi Junio,

On Tue, 13 Dec 2016, Junio C Hamano wrote:

> Johannes Schindelin  writes:
> 
> > @@ -395,7 +414,10 @@ static int do_recursive_merge(struct commit *base, 
> > struct commit *next,
> >  
> > if (active_cache_changed &&
> > write_locked_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".
> > +*/

Aargh. I had fixed that in another patch series already, but failed to do
so in this here patch series. Sorry.

Fixed.

> > @@ -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?

No. Since its conception, edit-patch-series.sh (the artist now known as
rebase -i) processed each line of the "todo script" by writing a new
"todo" file, skipping the first line, and appending it to the "done" file.

The sequencer chooses to write the new "insn sheet" after a successful
operation.

This is not an option for rebase -i, as it has many error paths and it was
simply impossible to express "save the todo script in case of failure" in
shell script.

I added a comment to clarify why the current command is not written to
git-rebase-todo.

Ciao,
Dscho

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

[Stephan Beyer]

Hi,

On 12/14/2016 08:29 PM, Junio C Hamano wrote:
> Johannes Schindelin  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).

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.

Hence, I think that it's a wiser choice to keep one directory for
sequencer state *and* additional files.
If we (a) save everything in "sequencer", we break the internal-path
backwards-compatbility but we can get rid of get_dir()...
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() and the GIT_PATH_FUNC definitions of
git_path_{todo,opts,head}_file must be replaced by definitions using
get_dir().

Best
  Stephan

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

[Junio C Hamano]

Johannes Schindelin  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.

Thanks.

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

[Junio C Hamano]

Johannes Schindelin  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(_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(_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.