patch, 22 partssequencer: use memoized sequencer directory path

> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  builtin/commit.c |  2 +-
>  sequencer.c      | 11 ++++++-----
>  sequencer.h      |  5 +----
>  3 files changed, 8 insertions(+), 10 deletions(-)

> diff --git a/builtin/commit.c b/builtin/commit.c
> index 77e3dc8..0221190 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -173,7 +173,7 @@ static void determine_whence(struct wt_status *s)
>  		whence = FROM_MERGE;
>  	else if (file_exists(git_path_cherry_pick_head())) {
>  		whence = FROM_CHERRY_PICK;
> -		if (file_exists(git_path(SEQ_DIR)))
> +		if (file_exists(git_path_seq_dir()))
>  			sequencer_in_use = 1;
>  	}
>  	else

> diff --git a/sequencer.c b/sequencer.c
> index b6481bb..4d2b4e3 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -21,10 +21,11 @@
>  const char sign_off_header[] = "Signed-off-by: ";
>  static const char cherry_picked_prefix[] = "(cherry picked from commit ";
>  
> -static GIT_PATH_FUNC(git_path_todo_file, SEQ_TODO_FILE)
> -static GIT_PATH_FUNC(git_path_opts_file, SEQ_OPTS_FILE)
> -static GIT_PATH_FUNC(git_path_seq_dir, SEQ_DIR)
> -static GIT_PATH_FUNC(git_path_head_file, SEQ_HEAD_FILE)
> +GIT_PATH_FUNC(git_path_seq_dir, "sequencer")
> +
> +static GIT_PATH_FUNC(git_path_todo_file, "sequencer/todo")
> +static GIT_PATH_FUNC(git_path_opts_file, "sequencer/opts")
> +static GIT_PATH_FUNC(git_path_head_file, "sequencer/head")

>  
>  static int is_rfc2822_line(const char *buf, int len)
>  {
> @@ -112,7 +113,7 @@ static void remove_sequencer_state(void)
>  {
>  	struct strbuf seq_dir = STRBUF_INIT;
>  
> -	strbuf_addstr(&seq_dir, git_path(SEQ_DIR));
> +	strbuf_addstr(&seq_dir, git_path_seq_dir());
>  	remove_dir_recursively(&seq_dir, 0);
>  	strbuf_release(&seq_dir);
>  }
> diff --git a/sequencer.h b/sequencer.h
> index 2ca096b..c955594 100644
> --- a/sequencer.h
> +++ b/sequencer.h
> @@ -1,10 +1,7 @@
>  #ifndef SEQUENCER_H
>  #define SEQUENCER_H
>  
> -#define SEQ_DIR		"sequencer"
> -#define SEQ_HEAD_FILE	"sequencer/head"
> -#define SEQ_TODO_FILE	"sequencer/todo"
> -#define SEQ_OPTS_FILE	"sequencer/opts"
> +const char *git_path_seq_dir(void);

> W dniu 29.08.2016 o 10:04, Johannes Schindelin pisze:
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> >  builtin/commit.c |  2 +-
> >  sequencer.c      | 11 ++++++-----
> >  sequencer.h      |  5 +----
> >  3 files changed, 8 insertions(+), 10 deletions(-)
> 
> Just a sidenote: it would be probably easier to read with *.h before
> *.c (at least this particular one).

> > diff --git a/builtin/commit.c b/builtin/commit.c
> > index 77e3dc8..0221190 100644
> > --- a/builtin/commit.c
> > +++ b/builtin/commit.c
> > @@ -173,7 +173,7 @@ static void determine_whence(struct wt_status *s)
> >  		whence = FROM_MERGE;
> >  	else if (file_exists(git_path_cherry_pick_head())) {
> >  		whence = FROM_CHERRY_PICK;
> > -		if (file_exists(git_path(SEQ_DIR)))
> > +		if (file_exists(git_path_seq_dir()))
> >  			sequencer_in_use = 1;
> >  	}
> >  	else
> 
> So it is more "Use memoized sequencer directory path" rather than
> "sequencer: use memoized sequencer directory path" - it replaces
> all occurrences of SEQ_DIR,... that's why it can be removed from
> 'sequencer.h'.
> 
> Though perhaps I misunderstood "sequencer: " prefix there.  Don't
> mind me then.

> On ma, 2016-08-29 at 10:03 +0200, Johannes Schindelin wrote:
> 
> > +#define REPLAY_OPTS_INIT { -1, -1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, NULL, NULL, NULL, 0, 0, NULL }
> 
> This looked off to me, as it replaces memset(..., 0, ...) so is not
> 100% equivalent. But the changed functions both set opts.action and
> call parse_args which sets opts.subcommand.

> On ma, 2016-08-29 at 10:03 +0200, Johannes Schindelin wrote:
> 
>> +#define REPLAY_OPTS_INIT { -1, -1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, NULL, NULL, NULL, 0, 0, NULL }
> 
> This looked off to me, as it replaces memset(..., 0, ...) so is not
> 100% equivalent. But the changed functions both set opts.action and
> call parse_args which sets opts.subcommand.

> W dniu 29.08.2016 o 11:19, Dennis Kaarsemaker pisze:
> > On ma, 2016-08-29 at 10:03 +0200, Johannes Schindelin wrote:
> > 
> >> +#define REPLAY_OPTS_INIT { -1, -1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, NULL, NULL, NULL, 0, 0, NULL }
> > 
> > This looked off to me, as it replaces memset(..., 0, ...) so is not
> > 100% equivalent. But the changed functions both set opts.action and
> > call parse_args which sets opts.subcommand.
> 
> This information would be nice to have in the commit message.

> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  sequencer.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/sequencer.c b/sequencer.c
> index 4d2b4e3..14ef79b 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -809,11 +809,11 @@ static int populate_opts_cb(const char *key, const char *value, void *data)
>  	return 0;
>  }
>  
> -static int read_populate_opts(struct replay_opts **opts)
> +static int read_populate_opts(struct replay_opts *opts)

>  {
>  	if (!file_exists(git_path_opts_file()))
>  		return 0;
> -	if (git_config_from_file(populate_opts_cb, git_path_opts_file(), *opts) < 0)
> +	if (git_config_from_file(populate_opts_cb, git_path_opts_file(), opts) < 0)
>  		return error(_("Malformed options sheet: %s"),
>  			git_path_opts_file());
>  	return 0;
> @@ -1038,7 +1038,7 @@ static int sequencer_continue(struct replay_opts *opts)
>  
>  	if (!file_exists(git_path_todo_file()))
>  		return continue_single_pick();
> -	if (read_populate_opts(&opts) ||
> +	if (read_populate_opts(opts) ||
>  			read_populate_todo(&todo_list, opts))
>  		return -1;
>  
> 

> W dniu 29.08.2016 o 10:04, Johannes Schindelin pisze:
> 
> > We really do not need the *pointer to a* pointer to the options in
> > the read_populate_opts() function.
> 
> Right.
>  
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> >  sequencer.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/sequencer.c b/sequencer.c
> > index 4d2b4e3..14ef79b 100644
> > --- a/sequencer.c
> > +++ b/sequencer.c
> > @@ -809,11 +809,11 @@ static int populate_opts_cb(const char *key, const char *value, void *data)
> >  	return 0;
> >  }
> >  
> > -static int read_populate_opts(struct replay_opts **opts)
> > +static int read_populate_opts(struct replay_opts *opts)
> 
> Especially that other *_populate_*() use 'struct replay_opts *opts':
> 
>    read_populate_todo(struct commit_list **todo_list, struct replay_opts *opts)
>    walk_revs_populate_todo(struct commit_list **todo_list, struct replay_opts *opts)
> 
> Though they use **todo_list, because they modify this list;
> maybe that was why read_populate_opts was using **opts instead
> of *opts?

> On ma, 2016-08-29 at 10:04 +0200, Johannes Schindelin wrote:
> 
> > +       if (read_and_refresh_cache(opts))
> > +               return -1;
> > +
> 
> This doesn't seem to be related to the get_dir changes?

> On Mon, 29 Aug 2016, Dennis Kaarsemaker wrote:
> 
> > On ma, 2016-08-29 at 10:04 +0200, Johannes Schindelin wrote:
> > 
> > > +       if (read_and_refresh_cache(opts))
> > > +               return -1;
> > > +
> > 
> > This doesn't seem to be related to the get_dir changes?
> 
> Good eyes.
> 
> Let me investigate why I have it here...

> The sequencer is our attempt to lib-ify cherry-pick. Yet it behaves
> like a one-shot command when it reads its configuration: memory is
> allocated and released only when the command exits.
> 
> This is kind of okay for git-cherry-pick, which *is* a one-shot
> command. All the work to make the sequencer its work horse was
> done to allow using the functionality as a library function, though,
> including proper clean-up after use.
> 
> This patch introduces an API to pass the responsibility of releasing
> certain memory to the sequencer.

> 
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  sequencer.c | 13 +++++++++++++
>  sequencer.h |  8 +++++++-
>  2 files changed, 20 insertions(+), 1 deletion(-)
> 
> diff --git a/sequencer.c b/sequencer.c
> index c4b223b..b5be0f9 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -114,9 +114,22 @@ static int has_conforming_footer(struct strbuf *sb, struct strbuf *sob,
>  	return 1;
>  }
>  
> +void *sequencer_entrust(struct replay_opts *opts, void *set_me_free_after_use)
> +{
> +	ALLOC_GROW(opts->owned, opts->owned_nr + 1, opts->owned_alloc);
> +	opts->owned[opts->owned_nr++] = set_me_free_after_use;
> +
> +	return set_me_free_after_use;

> +}
> +
>  static void remove_sequencer_state(const struct replay_opts *opts)
>  {
>  	struct strbuf dir = STRBUF_INIT;
> +	int i;
> +
> +	for (i = 0; i < opts->owned_nr; i++)
> +		free(opts->owned[i]);

> +	free(opts->owned);
>  
>  	strbuf_addf(&dir, "%s", get_dir(opts));
>  	remove_dir_recursively(&dir, 0);
> diff --git a/sequencer.h b/sequencer.h
> index c955594..20b708a 100644
> --- a/sequencer.h
> +++ b/sequencer.h
> @@ -43,8 +43,14 @@ struct replay_opts {
>  
>  	/* Only used by REPLAY_NONE */
>  	struct rev_info *revs;
> +
> +	/* malloc()ed data entrusted to the sequencer */
> +	void **owned;
> +	int owned_nr, owned_alloc;

> +
> +void *sequencer_entrust(struct replay_opts *opts, void *set_me_free_after_use);
>  
>  int sequencer_pick_revisions(struct replay_opts *opts);
>  
> 

> W dniu 29.08.2016 o 10:04, Johannes Schindelin pisze:
>> -#define REPLAY_OPTS_INIT { -1, -1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, NULL, NULL, NULL, 0, 0, NULL }
>> +#define REPLAY_OPTS_INIT { -1, -1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, NULL, NULL, NULL, 0, 0, NULL, NULL, 0, 0 }
>
> Nb. it is a pity that we cannot use named initializers for structs,
> so called designated inits.  It would make this macro more readable.

> Am 29.08.2016 um 23:59 schrieb Jakub Narębski:
> > W dniu 29.08.2016 o 10:04, Johannes Schindelin pisze:
> > > -#define REPLAY_OPTS_INIT { -1, -1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, NULL,
> > > NULL, NULL, 0, 0, NULL }
> > > +#define REPLAY_OPTS_INIT { -1, -1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, NULL,
> > > NULL, NULL, 0, 0, NULL, NULL, 0, 0 }
> >
> > Nb. it is a pity that we cannot use named initializers for structs,
> > so called designated inits.  It would make this macro more readable.
> 
> It is actually pointless to add the 0's and NULL's here. This should  be
> sufficient:
> 
> #define REPLAY_OPTS_INIT { -1, -1 }
> 
> because initialization with 0 (or NULL) is the default for any omitted
> members.

> W dniu 29.08.2016 o 10:04, Johannes Schindelin pisze:
> 
> > The sequencer is our attempt to lib-ify cherry-pick. Yet it behaves
> > like a one-shot command when it reads its configuration: memory is
> > allocated and released only when the command exits.
> > 
> > This is kind of okay for git-cherry-pick, which *is* a one-shot
> > command. All the work to make the sequencer its work horse was done to
> > allow using the functionality as a library function, though, including
> > proper clean-up after use.
> > 
> > This patch introduces an API to pass the responsibility of releasing
> > certain memory to the sequencer.
> 
> So how this API would be / is meant to be used?

> > +void *sequencer_entrust(struct replay_opts *opts, void *set_me_free_after_use)
> > +{
> > +	ALLOC_GROW(opts->owned, opts->owned_nr + 1, opts->owned_alloc);
> > +	opts->owned[opts->owned_nr++] = set_me_free_after_use;
> > +
> > +	return set_me_free_after_use;
> 
> I was wondering what this 'set_me_free_after_use' parameter is about;
> wouldn't it be more readable if this parameter was called 'owned_data'
> or 'owned_ptr'?

> >  static void remove_sequencer_state(const struct replay_opts *opts)
> >  {
> >  	struct strbuf dir = STRBUF_INIT;
> > +	int i;
> > +
> > +	for (i = 0; i < opts->owned_nr; i++)
> > +		free(opts->owned[i]);
> 
> I guess you can remove owned data in any order, regardless if you
> store struct or its members first...

> > diff --git a/sequencer.h b/sequencer.h
> > index c955594..20b708a 100644
> > --- a/sequencer.h
> > +++ b/sequencer.h
> > @@ -43,8 +43,14 @@ struct replay_opts {
> >  
> >  	/* Only used by REPLAY_NONE */
> >  	struct rev_info *revs;
> > +
> > +	/* malloc()ed data entrusted to the sequencer */
> > +	void **owned;
> > +	int owned_nr, owned_alloc;
> 
> I'm not sure about naming conventions for those types of data, but
> wouldn't 'owned_data' be a better name?  I could be wrong here...

>>> +void *sequencer_entrust(struct replay_opts *opts, void *set_me_free_after_use)
>>> +{
>>> +	ALLOC_GROW(opts->owned, opts->owned_nr + 1, opts->owned_alloc);
>>> +	opts->owned[opts->owned_nr++] = set_me_free_after_use;
>>> +
>>> +	return set_me_free_after_use;
>>
>> I was wondering what this 'set_me_free_after_use' parameter is about;
>> wouldn't it be more readable if this parameter was called 'owned_data'
>> or 'owned_ptr'?
> 
> If I read "owned_ptr" as a function's parameter, I would assume that the
> associated memory is owned by the caller. So I would be puzzled reading
> that name.

> Jakub Narębski <jnareb@gmail.com> writes:
> 
> > In my personal opinion 'set_me_free_after_use' is not the best name,
> > but I unfortunately do not have a better proposal.  Maybe 'entrust_ptr',
> > or 'entrusted_data' / 'entrusted_ptr' / 'entrusted'?
> 
> Is this to accumulate to-be-freed pointers?

> It is a bit surprising that the careless memory management in this
> codepath leaks only the dumb pieces of memory (as opposed to
> pointers to structures like string list that needs _clear()
> functions, in which case we cannot get away with list of
> to-be-freed).  I guess we were somewhat lucky ;-)

> The sequencer reads options from disk and stores them in its struct
> for use during sequencer's operations.
> 
> With this patch, the memory is released afterwards, plugging a
> memory leak.
> 
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  sequencer.c | 13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/sequencer.c b/sequencer.c
> index b5be0f9..8d79091 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -131,6 +131,8 @@ static void remove_sequencer_state(const struct replay_opts *opts)
>  		free(opts->owned[i]);
>  	free(opts->owned);
>  
> +	free(opts->xopts);
> +

>  		opts->allow_ff = git_config_bool_or_int(key, value, &error_flag);
>  	else if (!strcmp(key, "options.mainline"))
>  		opts->mainline = git_config_int(key, value);
> -	else if (!strcmp(key, "options.strategy"))
> +	else if (!strcmp(key, "options.strategy")) {
>  		git_config_string(&opts->strategy, key, value);
> +		sequencer_entrust(opts, (char *) opts->strategy);

> +	}
> -	else if (!strcmp(key, "options.gpg-sign"))
> +	else if (!strcmp(key, "options.gpg-sign")) {
>  		git_config_string(&opts->gpg_sign, key, value);
> +		sequencer_entrust(opts, (char *) opts->gpg_sign);
> +	}
>  	else if (!strcmp(key, "options.strategy-option")) {
>  		ALLOC_GROW(opts->xopts, opts->xopts_nr + 1, opts->xopts_alloc);
> -		opts->xopts[opts->xopts_nr++] = xstrdup(value);
> +		opts->xopts[opts->xopts_nr++] =
> +			sequencer_entrust(opts, xstrdup(value));

> W dniu 29.08.2016 o 10:04, Johannes Schindelin pisze:
> 
> > The sequencer reads options from disk and stores them in its struct
> > for use during sequencer's operations.
> > 
> > With this patch, the memory is released afterwards, plugging a
> > memory leak.
> > 
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> >  sequencer.c | 13 ++++++++++---
> >  1 file changed, 10 insertions(+), 3 deletions(-)
> > 
> > diff --git a/sequencer.c b/sequencer.c
> > index b5be0f9..8d79091 100644
> > --- a/sequencer.c
> > +++ b/sequencer.c
> > @@ -131,6 +131,8 @@ static void remove_sequencer_state(const struct replay_opts *opts)
> >  		free(opts->owned[i]);
> >  	free(opts->owned);
> >  
> > +	free(opts->xopts);
> > +
> 
> This looks like independent change, not related to using the
> sequencer_entrust() to store options read from disk in replay_opts
> struct to be able to free memory afterwards.
> 
> I guess you wanted to avoid one line changes...

> > @@ -811,13 +813,18 @@ static int populate_opts_cb(const char *key, const char *value, void *data)
> 
> Sidenote: this patch would be easier to read if lines were reordered
> as below, but I don't think any slider heuristics could help achieve
> that automatically.  Also, the patch might be invalid...
> 
> >  		opts->allow_ff = git_config_bool_or_int(key, value, &error_flag);
> >  	else if (!strcmp(key, "options.mainline"))
> >  		opts->mainline = git_config_int(key, value);
> > -	else if (!strcmp(key, "options.strategy"))
> > +	else if (!strcmp(key, "options.strategy")) {
> >  		git_config_string(&opts->strategy, key, value);
> > +		sequencer_entrust(opts, (char *) opts->strategy);
> 
> I wonder if the ability to free strings dup-ed by git_config_string()
> be something that is part of replay_opts, or rather remove_sequencer_state(),
> that is a list of
> 
> 	free(opts->strategy);
> 	free(opts->gpg_sign);

> Am 30.08.2016 um 19:52 schrieb Johannes Schindelin:
>> Right, but that is exactly what I wanted to avoid, because it is rather
>> inelegant to strdup() strings just so that we do not have to record what
>> to free() and what not to free().
>
> Please, excuse, but when I have to choose what is more "elegant":
>
>  1. strdup() sometimes so that I can later free() always
>  2. use sequencer_entrust()
>
> I would choose 1. at all times.

> Hi Kuba,
> 
> On Tue, 30 Aug 2016, Jakub Narębski wrote:
> 
>> W dniu 29.08.2016 o 10:04, Johannes Schindelin pisze:
>>
>>> The sequencer reads options from disk and stores them in its struct
>>> for use during sequencer's operations.
>>>
>>> With this patch, the memory is released afterwards, plugging a
>>> memory leak.
>>>
>>> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
>>> ---
>>>  sequencer.c | 13 ++++++++++---
>>>  1 file changed, 10 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/sequencer.c b/sequencer.c
>>> index b5be0f9..8d79091 100644
>>> --- a/sequencer.c
>>> +++ b/sequencer.c
>>> @@ -131,6 +131,8 @@ static void remove_sequencer_state(const struct replay_opts *opts)
>>>  		free(opts->owned[i]);
>>>  	free(opts->owned);
>>>  
>>> +	free(opts->xopts);
>>> +
>>
>> This looks like independent change, not related to using the
>> sequencer_entrust() to store options read from disk in replay_opts
>> struct to be able to free memory afterwards.
>>
>> I guess you wanted to avoid one line changes...
> 
> Actually, it is not an independent change, but it free()s memory that has
> been allocated while reading the options, as the commit message says ;-)
> 
>>> @@ -811,13 +813,18 @@ static int populate_opts_cb(const char *key, const char *value, void *data)
>>
>> Sidenote: this patch would be easier to read if lines were reordered
>> as below, but I don't think any slider heuristics could help achieve
>> that automatically.  Also, the patch might be invalid...
>>
>>>  		opts->allow_ff = git_config_bool_or_int(key, value, &error_flag);
>>>  	else if (!strcmp(key, "options.mainline"))
>>>  		opts->mainline = git_config_int(key, value);
>>> -	else if (!strcmp(key, "options.strategy"))
>>> +	else if (!strcmp(key, "options.strategy")) {
>>>  		git_config_string(&opts->strategy, key, value);
>>> +		sequencer_entrust(opts, (char *) opts->strategy);
>>
>> I wonder if the ability to free strings dup-ed by git_config_string()
>> be something that is part of replay_opts, or rather remove_sequencer_state(),
>> that is a list of
>>
>> 	free(opts->strategy);
>> 	free(opts->gpg_sign);
> 
> That is not necessarily possible because the way sequencer works, the
> options may have not actually be read from the file, but may be populated
> by the caller (in which case we do not necessarily want to require
> strdup()ing the strings just so that the sequencer can clean stuff up
> afterwards).

>> The *_entrust() mechanism is more generic, but do we use this general-ness?
>> Well, it could be xstrdup or git_config_string doing entrust'ing...
> 
> Right, but that is exactly what I wanted to avoid, because it is rather
> inelegant to strdup() strings just so that we do not have to record what
> to free() and what not to free().

> @@ -811,13 +813,18 @@ static int populate_opts_cb(const char *key, const char *value, void *data)
>  		opts->allow_ff = git_config_bool_or_int(key, value, &error_flag);
>  	else if (!strcmp(key, "options.mainline"))
>  		opts->mainline = git_config_int(key, value);
> -	else if (!strcmp(key, "options.strategy"))
> +	else if (!strcmp(key, "options.strategy")) {
>  		git_config_string(&opts->strategy, key, value);
> -	else if (!strcmp(key, "options.gpg-sign"))
> +		sequencer_entrust(opts, (char *) opts->strategy);
> +	}
> +	else if (!strcmp(key, "options.gpg-sign")) {
>  		git_config_string(&opts->gpg_sign, key, value);
> +		sequencer_entrust(opts, (char *) opts->gpg_sign);
> +	}
>  	else if (!strcmp(key, "options.strategy-option")) {
>  		ALLOC_GROW(opts->xopts, opts->xopts_nr + 1, opts->xopts_alloc);
> -		opts->xopts[opts->xopts_nr++] = xstrdup(value);
> +		opts->xopts[opts->xopts_nr++] =
> +			sequencer_entrust(opts, xstrdup(value));
>  	} else
>  		return error(_("Invalid key: %s"), key);

> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> 
> > @@ -811,13 +813,18 @@ static int populate_opts_cb(const char *key, const char *value, void *data)
> >  		opts->allow_ff = git_config_bool_or_int(key, value, &error_flag);
> >  	else if (!strcmp(key, "options.mainline"))
> >  		opts->mainline = git_config_int(key, value);
> > -	else if (!strcmp(key, "options.strategy"))
> > +	else if (!strcmp(key, "options.strategy")) {
> >  		git_config_string(&opts->strategy, key, value);
> > -	else if (!strcmp(key, "options.gpg-sign"))
> > +		sequencer_entrust(opts, (char *) opts->strategy);
> > +	}
> > +	else if (!strcmp(key, "options.gpg-sign")) {
> >  		git_config_string(&opts->gpg_sign, key, value);
> > +		sequencer_entrust(opts, (char *) opts->gpg_sign);
> > +	}
> >  	else if (!strcmp(key, "options.strategy-option")) {
> >  		ALLOC_GROW(opts->xopts, opts->xopts_nr + 1, opts->xopts_alloc);
> > -		opts->xopts[opts->xopts_nr++] = xstrdup(value);
> > +		opts->xopts[opts->xopts_nr++] =
> > +			sequencer_entrust(opts, xstrdup(value));
> >  	} else
> >  		return error(_("Invalid key: %s"), key);
> 
> Hmm.
> 
> I would have expected a call to sequencer_opts_clear(&opts) once the
> machinery is done with the options structure,

> Over the next commits, we will work on improving the sequencer to the
> point where it can process the edit script of an interactive rebase. To
> that end, we will need to teach the sequencer to read interactive
> rebase's todo file. In preparation, we consolidate all places where
> that todo file is needed to call a function that we will later extend.
> 
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  sequencer.c | 18 +++++++++++-------
>  1 file changed, 11 insertions(+), 7 deletions(-)
> 
> diff --git a/sequencer.c b/sequencer.c
> index 8d79091..982b6e9 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -32,6 +32,11 @@ static const char *get_dir(const struct replay_opts *opts)
>  	return git_path_seq_dir();
>  }
>  
> +static const char *get_todo_path(const struct replay_opts *opts)
> +{
> +	return git_path_todo_file();
> +}

> +
>  static int is_rfc2822_line(const char *buf, int len)
>  {
>  	int i;
> @@ -772,25 +777,24 @@ static int parse_insn_buffer(char *buf, struct commit_list **todo_list,
>  static int read_populate_todo(struct commit_list **todo_list,
>  			struct replay_opts *opts)
>  {
> +	const char *todo_file = get_todo_path(opts);

>  	struct strbuf buf = STRBUF_INIT;
>  	int fd, res;
>  
> -	fd = open(git_path_todo_file(), O_RDONLY);
> +	fd = open(todo_file, O_RDONLY);
>  	if (fd < 0)
> -		return error_errno(_("Could not open %s"),
> -				   git_path_todo_file());
> +		return error_errno(_("Could not open %s"), todo_file);

>  	if (strbuf_read(&buf, fd, 0) < 0) {
>  		close(fd);
>  		strbuf_release(&buf);
> -		return error(_("Could not read %s."), git_path_todo_file());
> +		return error(_("Could not read %s."), todo_file);
>  	}
>  	close(fd);
>  
>  	res = parse_insn_buffer(buf.buf, todo_list, opts);
>  	strbuf_release(&buf);
>  	if (res)
> -		return error(_("Unusable instruction sheet: %s"),
> -			git_path_todo_file());
> +		return error(_("Unusable instruction sheet: %s"), todo_file);
>  	return 0;
>  }
>  
> @@ -1064,7 +1068,7 @@ static int sequencer_continue(struct replay_opts *opts)
>  {
>  	struct commit_list *todo_list = NULL;
>  
> -	if (!file_exists(git_path_todo_file()))
> +	if (!file_exists(get_todo_path(opts)))

> W dniu 29.08.2016 o 10:04, Johannes Schindelin pisze:
> 
> > Over the next commits, we will work on improving the sequencer to the
> > point where it can process the edit script of an interactive rebase. To
> > that end, we will need to teach the sequencer to read interactive
> > rebase's todo file. In preparation, we consolidate all places where
> > that todo file is needed to call a function that we will later extend.
> > 
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> >  sequencer.c | 18 +++++++++++-------
> >  1 file changed, 11 insertions(+), 7 deletions(-)
> > 
> > diff --git a/sequencer.c b/sequencer.c
> > index 8d79091..982b6e9 100644
> > --- a/sequencer.c
> > +++ b/sequencer.c
> > @@ -32,6 +32,11 @@ static const char *get_dir(const struct replay_opts *opts)
> >  	return git_path_seq_dir();
> >  }
> >  
> > +static const char *get_todo_path(const struct replay_opts *opts)
> > +{
> > +	return git_path_todo_file();
> > +}
> 
> I guess that in the future commit the return value of get_todo_path()
> would change depending on what sequencer is used for, cherry-pick or
> interactive rebase, that is, contents of replay_opts...

> >  static int is_rfc2822_line(const char *buf, int len)
> >  {
> >  	int i;
> > @@ -772,25 +777,24 @@ static int parse_insn_buffer(char *buf, struct commit_list **todo_list,
> >  static int read_populate_todo(struct commit_list **todo_list,
> >  			struct replay_opts *opts)
> >  {
> > +	const char *todo_file = get_todo_path(opts);
> 
> ...and that's why you have added this temporary variable here, to
> not repeat get_todo_path(opts) calculations...

> > -	fd = open(git_path_todo_file(), O_RDONLY);
> > +	fd = open(todo_file, O_RDONLY);
> >  	if (fd < 0)
> > -		return error_errno(_("Could not open %s"),
> > -				   git_path_todo_file());
> > +		return error_errno(_("Could not open %s"), todo_file);
> 
> ... So that's why it is s/git_path_todo_file()/todo_file/ replacement,
> and not simply...
> 
> > @@ -1064,7 +1068,7 @@ static int sequencer_continue(struct replay_opts *opts)
> >  {
> >  	struct commit_list *todo_list = NULL;
> >  
> > -	if (!file_exists(git_path_todo_file()))
> > +	if (!file_exists(get_todo_path(opts)))
> 
> ...the s/git_path_todo_file()/git_todo_path(opts)/, isn't it?

> The sequencer was introduced to make the cherry-pick and revert
> functionality available as library function, with the original idea
> being to extend the sequencer to also implement the rebase -i
> functionality.
> 
> The test to ensure that all of the commands in the script are identical
> to the overall operation does not mesh well with that.

> 
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  t/t3510-cherry-pick-sequence.sh | 11 -----------
>  1 file changed, 11 deletions(-)
> 
> diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh
> index 7b7a89d..6465edf 100755
> --- a/t/t3510-cherry-pick-sequence.sh
> +++ b/t/t3510-cherry-pick-sequence.sh
> @@ -459,17 +459,6 @@ test_expect_success 'malformed instruction sheet 1' '
>  	test_expect_code 128 git cherry-pick --continue
>  '
>  
> -test_expect_success 'malformed instruction sheet 2' '

> -	pristine_detach initial &&
> -	test_expect_code 1 git cherry-pick base..anotherpick &&
> -	echo "resolved" >foo &&
> -	git add foo &&
> -	git commit &&
> -	sed "s/pick/revert/" .git/sequencer/todo >new_sheet &&
> -	cp new_sheet .git/sequencer/todo &&
> -	test_expect_code 128 git cherry-pick --continue
> -'
> -
>  test_expect_success 'empty commit set' '
>  	pristine_detach initial &&
>  	test_expect_code 128 git cherry-pick base..base
> 

> W dniu 29.08.2016 o 10:04, Johannes Schindelin pisze:
> > The sequencer was introduced to make the cherry-pick and revert
> > functionality available as library function, with the original idea
> > being to extend the sequencer to also implement the rebase -i
> > functionality.
> > 
> > The test to ensure that all of the commands in the script are identical
> > to the overall operation does not mesh well with that.
> 
> Actually the question is what does the test that got removed in this
> commit actually check.  Is it high-level sanity check that todo list
> for git-cherry-pick contains only 'pick', and for git-revert contains
> only 'revert'?

> > Therefore let's just get rid of the test that wants to verify that this
> > limitation is still in place, in preparation for the upcoming work to
> > teach the sequencer to do rebase -i's work.
> 
> Is it "upcoming work" as in one of the patches in this series?
> If so, which patch?

> > diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh
> > index 7b7a89d..6465edf 100755
> > --- a/t/t3510-cherry-pick-sequence.sh
> > +++ b/t/t3510-cherry-pick-sequence.sh
> > @@ -459,17 +459,6 @@ test_expect_success 'malformed instruction sheet 1' '
> >  	test_expect_code 128 git cherry-pick --continue
> >  '
> >  
> > -test_expect_success 'malformed instruction sheet 2' '
> 
> Hmmm... the description is somewhat lacking (especially compared to
> the rest of test), anyway.
> 
> BTW. we should probably rename 'malformed instruction sheet 2'
> to 'malformed instruction sheet' if there are no further such
> tests after this removal, isn't it?

>>> Therefore let's just get rid of the test that wants to verify that this
>>> limitation is still in place, in preparation for the upcoming work to
>>> teach the sequencer to do rebase -i's work.
>>
>> Is it "upcoming work" as in one of the patches in this series?
>> If so, which patch?
> 
> I left this a little vague, didn't I? ;-)
> 
> The problem is that the `git-rebase-todo` most definitely does *not* want
> to be restricted to a single command.
> 
> So if you must have a patch that disagrees with this overzealous check,
> the "revamp todo parsing" one is probably the first. But it is better to
> think of this at a higher level than just patches: it is wrong to limit
> the todo script to contain only identical commands.

>>> diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh
>>> index 7b7a89d..6465edf 100755
>>> --- a/t/t3510-cherry-pick-sequence.sh
>>> +++ b/t/t3510-cherry-pick-sequence.sh
>>> @@ -459,17 +459,6 @@ test_expect_success 'malformed instruction sheet 1' '
>>>  	test_expect_code 128 git cherry-pick --continue
>>>  '
>>>  
>>> -test_expect_success 'malformed instruction sheet 2' '
>>
>> Hmmm... the description is somewhat lacking (especially compared to
>> the rest of test), anyway.
>>
>> BTW. we should probably rename 'malformed instruction sheet 2'
>> to 'malformed instruction sheet' if there are no further such
>> tests after this removal, isn't it?
> 
> No, we cannot rename it after this patch because the patch removes it ;-)
> (It is not a file name but really a label for a test case.)

> > On Wed, 31 Aug 2016, Jakub Narębski wrote: 
> >> W dniu 29.08.2016 o 10:04, Johannes Schindelin pisze:
>  
> >>> diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh
> >>> index 7b7a89d..6465edf 100755
> >>> --- a/t/t3510-cherry-pick-sequence.sh
> >>> +++ b/t/t3510-cherry-pick-sequence.sh
> >>> @@ -459,17 +459,6 @@ test_expect_success 'malformed instruction sheet 1' '
> >>>  	test_expect_code 128 git cherry-pick --continue
> >>>  '
> >>>  
> >>> -test_expect_success 'malformed instruction sheet 2' '
> >>
> >> Hmmm... the description is somewhat lacking (especially compared to
> >> the rest of test), anyway.
> >>
> >> BTW. we should probably rename 'malformed instruction sheet 2'
> >> to 'malformed instruction sheet' if there are no further such
> >> tests after this removal, isn't it?
> > 
> > No, we cannot rename it after this patch because the patch removes it ;-)
> > (It is not a file name but really a label for a test case.)
> 
> Ooops.  What I wanted to say that after removing the test case named
> 'malformed instruction sheet 2' we should also rename *earlier* test
> case from 'malformed instruction sheet 1' to 'malformed instruction sheet',
> as it is now the only 'malformed instruction sheet *' test case.

> On Wed, 31 Aug 2016, Jakub Narębski wrote:
>> W dniu 31.08.2016 o 20:36, Johannes Schindelin pisze:
>>
>> I wonder: would 'git cherry-pick --continue' be able to finish
>> 'git revert', and vice versa, then?  Or 'git sequencer --continue'?
> 
> I just tested this, via
> 
> 	diff --git a/t/t3510-cherry-pick-sequence.sh
> 	b/t/t3510-cherry-pick-sequence.sh
> 	index 96c7640..085d8bc 100755
> 	--- a/t/t3510-cherry-pick-sequence.sh
> 	+++ b/t/t3510-cherry-pick-sequence.sh
> 	@@ -55,7 +55,7 @@ test_expect_success 'cherry-pick
> 	mid-cherry-pick-sequence' '
> 		git checkout HEAD foo &&
> 		git cherry-pick base &&
> 		git cherry-pick picked &&
> 	-       git cherry-pick --continue &&
> 	+       git revert --continue &&
> 		git diff --exit-code anotherpick
> 
> (Danger! Whitespace corrupted!!!)
> 
> It appears that this passes now.

> 
>>> On Wed, 31 Aug 2016, Jakub Narębski wrote: 
>>>> W dniu 29.08.2016 o 10:04, Johannes Schindelin pisze:
>>  
>>>>> diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh
>>>>> index 7b7a89d..6465edf 100755
>>>>> --- a/t/t3510-cherry-pick-sequence.sh
>>>>> +++ b/t/t3510-cherry-pick-sequence.sh
>>>>> @@ -459,17 +459,6 @@ test_expect_success 'malformed instruction sheet 1' '
>>>>>  	test_expect_code 128 git cherry-pick --continue
>>>>>  '
>>>>>  
>>>>> -test_expect_success 'malformed instruction sheet 2' '
>>>>
>>>> Hmmm... the description is somewhat lacking (especially compared to
>>>> the rest of test), anyway.
>>>>
>>>> BTW. we should probably rename 'malformed instruction sheet 2'
>>>> to 'malformed instruction sheet' if there are no further such
>>>> tests after this removal, isn't it?
>>>
>>> No, we cannot rename it after this patch because the patch removes it ;-)
>>> (It is not a file name but really a label for a test case.)
>>
>> Ooops.  What I wanted to say that after removing the test case named
>> 'malformed instruction sheet 2' we should also rename *earlier* test
>> case from 'malformed instruction sheet 1' to 'malformed instruction sheet',
>> as it is now the only 'malformed instruction sheet *' test case.
> 
> Actually, you know, I completely missed the fact that there was a
> "malformed instruction sheet 3". I renumbered it.

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > So if you must have a patch that disagrees with this overzealous
> > check, the "revamp todo parsing" one is probably the first. But it is
> > better to think of this at a higher level than just patches: it is
> > wrong to limit the todo script to contain only identical commands.
> 
> So if you think of this at even higher level, the check done in
> parse_insn_line() that _assumes_ that opts->action must match the
> actions on each line is _WRONG_, but what this test expects to see is
> perfectly reasonable, I would think.
> 
> It is a different matter if it makes sense to _verify_ that the user
> didn't make nonsensical change to the generated insn and error out when
> s/he did.  I tend to think it is pointless, and I wouldn't object if
> this test is removed due to that reason.

> When we came up with the "sequencer" idea, we really wanted to have
> kind of a plumbing equivalent of the interactive rebase. Hence the
> choice of words: the "todo" script, a "pick", etc.
> 
> However, when it came time to implement the entire shebang, somehow this
> idea got lost and the sequencer was used as working horse for
> cherry-pick and revert instead. So as not to interfere with the
> interactive rebase, it even uses a separate directory to store its
> state.
> 
> Furthermore, it also is stupidly strict about the "todo" script it
> accepts: while it parses commands in a way that was *designed* to be
> similar to the interactive rebase, it then goes on to *error out* if the
> commands disagree with the overall action (cherry-pick or revert).

>
> Finally, the sequencer code chose to deviate from the interactive rebase
> code insofar that it *reformats* the "todo" script instead of just
> writing the part of the parsed script that were not yet processed. This
> is not only unnecessary churn, but might well lose information that is
> valuable to the user (i.e. comments after the commands).

>
> Let's just bite the bullet and rewrite the entire parser; the code now
> becomes not only more elegant: it allows us to go on and teach the
> sequencer how to parse *true* "todo" scripts as used by the interactive
> rebase itself. In a way, the sequencer is about to grow up to do its
> older brother's job. Better.

> 
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  sequencer.c | 241 +++++++++++++++++++++++++++++++++---------------------------
>  1 file changed, 134 insertions(+), 107 deletions(-)

> 
> diff --git a/sequencer.c b/sequencer.c
> index 982b6e9..cbdce6d 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -473,7 +473,26 @@ static int allow_empty(struct replay_opts *opts, struct commit *commit)
>  		return 1;
>  }
>  
> -static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
> +enum todo_command {
> +	TODO_PICK,
> +	TODO_REVERT
> +};

> +
> +static const char *todo_command_strings[] = {
> +	"pick",
> +	"revert"
> +};

> +
> +static const char *command_to_string(const enum todo_command command)
> +{
> +	if (command < ARRAY_SIZE(todo_command_strings))
> +		return todo_command_strings[command];
> +	die("Unknown command: %d", command);
> +}
> +
> +
> +static int do_pick_commit(enum todo_command command, struct commit *commit,
> +		struct replay_opts *opts)
>  {
>  	unsigned char head[20];
>  	struct commit *base, *next, *parent;
> @@ -535,7 +554,8 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
>  		/* TRANSLATORS: The first %s will be "revert" or
>  		   "cherry-pick", the second %s a SHA1 */
>  		return error(_("%s: cannot parse parent commit %s"),

> -			action_name(opts), oid_to_hex(&parent->object.oid));
> +			command_to_string(command),
> +			oid_to_hex(&parent->object.oid));
>  
>  	if (get_message(commit, &msg) != 0)
>  		return error(_("Cannot get commit message for %s"),
> @@ -548,7 +568,7 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)

>  	 * reverse of it if we are revert.
>  	 */
>  
> -	if (opts->action == REPLAY_REVERT) {
> +	if (command == TODO_REVERT) {
>  		base = commit;
>  		base_label = msg.label;
>  		next = parent;
> @@ -589,7 +609,7 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
>  		}
>  	}
>  
> -	if (!opts->strategy || !strcmp(opts->strategy, "recursive") || opts->action == REPLAY_REVERT) {
> +	if (!opts->strategy || !strcmp(opts->strategy, "recursive") || command == TODO_REVERT) {
>  		res = do_recursive_merge(base, next, base_label, next_label,
>  					 head, &msgbuf, opts);
>  		if (res < 0)
> @@ -615,17 +635,17 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
>  	 * However, if the merge did not even start, then we don't want to
>  	 * write it at all.
>  	 */
> -	if (opts->action == REPLAY_PICK && !opts->no_commit && (res == 0 || res == 1) &&
> +	if (command == TODO_PICK && !opts->no_commit && (res == 0 || res == 1) &&
>  	    update_ref(NULL, "CHERRY_PICK_HEAD", commit->object.oid.hash, NULL,
>  		       REF_NODEREF, UPDATE_REFS_MSG_ON_ERR))
>  		res = -1;
> -	if (opts->action == REPLAY_REVERT && ((opts->no_commit && res == 0) || res == 1) &&
> +	if (command == TODO_REVERT && ((opts->no_commit && res == 0) || res == 1) &&
>  	    update_ref(NULL, "REVERT_HEAD", commit->object.oid.hash, NULL,
>  		       REF_NODEREF, UPDATE_REFS_MSG_ON_ERR))
>  		res = -1;
>  
>  	if (res) {
> -		error(opts->action == REPLAY_REVERT
> +		error(command == TODO_REVERT
>  		      ? _("could not revert %s... %s")
>  		      : _("could not apply %s... %s"),
>  		      find_unique_abbrev(commit->object.oid.hash, DEFAULT_ABBREV),

> @@ -683,116 +703,107 @@ static int read_and_refresh_cache(struct replay_opts *opts)
>  	return 0;
>  }
>  
> -static int format_todo(struct strbuf *buf, struct commit_list *todo_list,
> -		struct replay_opts *opts)
> +struct todo_item {
> +	enum todo_command command;
> +	struct commit *commit;
> +	size_t offset_in_buf;
> +};
> +
> +struct todo_list {
> +	struct strbuf buf;
> +	struct todo_item *items;
> +	int nr, alloc, current;
> +};

> -	struct commit_list *cur = NULL;
> -	const char *sha1_abbrev = NULL;
> -	const char *action_str = opts->action == REPLAY_REVERT ? "revert" : "pick";
> -	const char *subject;
> -	int subject_len;
> +	strbuf_release(&todo_list->buf);
> +	free(todo_list->items);
> +	todo_list->items = NULL;
> +	todo_list->nr = todo_list->alloc = 0;
> +}
>  
> -	for (cur = todo_list; cur; cur = cur->next) {
> -		const char *commit_buffer = get_commit_buffer(cur->item, NULL);
> -		sha1_abbrev = find_unique_abbrev(cur->item->object.oid.hash, DEFAULT_ABBREV);
> -		subject_len = find_commit_subject(commit_buffer, &subject);
> -		strbuf_addf(buf, "%s %s %.*s\n", action_str, sha1_abbrev,
> -			subject_len, subject);
> -		unuse_commit_buffer(cur->item, commit_buffer);
> -	}
> -	return 0;
> +struct todo_item *append_todo(struct todo_list *todo_list)

> +{
> +	ALLOC_GROW(todo_list->items, todo_list->nr + 1, todo_list->alloc);
> +	return todo_list->items + todo_list->nr++;
>  }
>  
> -static struct commit *parse_insn_line(char *bol, char *eol, struct replay_opts *opts)
> +static int parse_insn_line(struct todo_item *item, const char *bol, char *eol)

>  {
>  	unsigned char commit_sha1[20];
> -	enum replay_action action;
>  	char *end_of_object_name;
> -	int saved, status, padding;
> -
> -	if (starts_with(bol, "pick")) {
> -		action = REPLAY_PICK;
> -		bol += strlen("pick");
> -	} else if (starts_with(bol, "revert")) {
> -		action = REPLAY_REVERT;
> -		bol += strlen("revert");
> -	} else
> -		return NULL;
> +	int i, saved, status, padding;

> +	if (i >= ARRAY_SIZE(todo_command_strings))
> +		return -1;
>  
>  	/* Eat up extra spaces/ tabs before object name */
>  	padding = strspn(bol, " \t");
>  	if (!padding)
> -		return NULL;
> +		return -1;
>  	bol += padding;
>  
> -	end_of_object_name = bol + strcspn(bol, " \t\n");
> +	end_of_object_name = (char *) bol + strcspn(bol, " \t\n");

>  	saved = *end_of_object_name;
>  	*end_of_object_name = '\0';
>  	status = get_sha1(bol, commit_sha1);
>  	*end_of_object_name = saved;
>  
> -	/*
> -	 * Verify that the action matches up with the one in
> -	 * opts; we don't support arbitrary instructions
> -	 */
> -	if (action != opts->action) {
> -		if (action == REPLAY_REVERT)
> -		      error((opts->action == REPLAY_REVERT)
> -			    ? _("Cannot revert during another revert.")

> -			    : _("Cannot revert during a cherry-pick."));
> -		else
> -		      error((opts->action == REPLAY_REVERT)
> -			    ? _("Cannot cherry-pick during a revert.")
> -			    : _("Cannot cherry-pick during another cherry-pick."));
> -		return NULL;
> -	}

> -
>  	if (status < 0)
> -		return NULL;
> +		return -1;
>  
> -	return lookup_commit_reference(commit_sha1);
> +	item->commit = lookup_commit_reference(commit_sha1);
> +	return !item->commit;
>  }
>  
> -static int parse_insn_buffer(char *buf, struct commit_list **todo_list,
> -			struct replay_opts *opts)
> +static int parse_insn_buffer(char *buf, struct todo_list *todo_list)
>  {
> -	struct commit_list **next = todo_list;
> -	struct commit *commit;
> +	struct todo_item *item;
>  	char *p = buf;
> -	int i;
> +	int i, res = 0;
>  
>  	for (i = 1; *p; i++) {
>  		char *eol = strchrnul(p, '\n');
> -		commit = parse_insn_line(p, eol, opts);
> -		if (!commit)
> -			return error(_("Could not parse line %d."), i);
> -		next = commit_list_append(commit, next);
> +
> +		item = append_todo(todo_list);

> +			item->command = -1;
> +		}
>  		p = *eol ? eol + 1 : eol;
>  	}
> -	if (!*todo_list)
> +	if (!todo_list->nr)
>  		return error(_("No commits parsed."));
> -	return 0;
> +	return res;

>  }
>  
> -static int read_populate_todo(struct commit_list **todo_list,
> +static int read_populate_todo(struct todo_list *todo_list,
>  			struct replay_opts *opts)
>  {
>  	const char *todo_file = get_todo_path(opts);

> -	struct strbuf buf = STRBUF_INIT;
>  	int fd, res;
>  
> +	strbuf_reset(&todo_list->buf);
>  	fd = open(todo_file, O_RDONLY);
>  	if (fd < 0)
>  		return error_errno(_("Could not open %s"), todo_file);
> -	if (strbuf_read(&buf, fd, 0) < 0) {
> +	if (strbuf_read(&todo_list->buf, fd, 0) < 0) {
>  		close(fd);
> -		strbuf_release(&buf);

>  		return error(_("Could not read %s."), todo_file);
>  	}
>  	close(fd);
>  
> -	res = parse_insn_buffer(buf.buf, todo_list, opts);
> +	res = parse_insn_buffer(todo_list->buf.buf, todo_list);
> -	strbuf_release(&buf);
>  	if (res)
>  		return error(_("Unusable instruction sheet: %s"), todo_file);
>  	return 0;

> @@ -848,18 +859,33 @@ static int read_populate_opts(struct replay_opts *opts)
>  	return 0;
>  }
>  
> -static int walk_revs_populate_todo(struct commit_list **todo_list,
> +static int walk_revs_populate_todo(struct todo_list *todo_list,
>  				struct replay_opts *opts)
>  {
> +	enum todo_command command = opts->action == REPLAY_PICK ?
> +		TODO_PICK : TODO_REVERT;
>  	struct commit *commit;
> -	struct commit_list **next;
>  
>  	if (prepare_revs(opts))
>  		return -1;
>  
> -	next = todo_list;
> -	while ((commit = get_revision(opts->revs)))
> -		next = commit_list_append(commit, next);
> +	while ((commit = get_revision(opts->revs))) {
> +		struct todo_item *item = append_todo(todo_list);
> +		const char *commit_buffer = get_commit_buffer(commit, NULL);

> +		const char *subject;
> +		int subject_len;
> +
> +		item->command = command;
> +		item->commit = commit;
> +		item->offset_in_buf = todo_list->buf.len;
> +		subject_len = find_commit_subject(commit_buffer, &subject);
> +		strbuf_addf(&todo_list->buf, "%s %s %.*s\n",
> +			opts->action == REPLAY_PICK ?  "pick" : "revert",

> +		unuse_commit_buffer(commit, commit_buffer);
> +	}
>  	return 0;
>  }
>  
> @@ -964,30 +990,24 @@ static int sequencer_rollback(struct replay_opts *opts)
>  	return -1;
>  }
>  
> -static int save_todo(struct commit_list *todo_list, struct replay_opts *opts)
> +static int save_todo(struct todo_list *todo_list, struct replay_opts *opts)
>  {
>  	static struct lock_file todo_lock;
> -	struct strbuf buf = STRBUF_INIT;
> -	int fd;
> +	const char *todo_path = get_todo_path(opts);
> +	int next = todo_list->current, offset, fd;

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

> -	}
> -	if (write_in_full(fd, buf.buf, buf.len) < 0) {
> -		strbuf_release(&buf);
> -		return error_errno(_("Could not write to %s"),
> -				   git_path_todo_file());
> -	}
> +	offset = next < todo_list->nr ?
> +		todo_list->items[next].offset_in_buf : todo_list->buf.len;
> +	if (write_in_full(fd, todo_list->buf.buf + offset,
> +			todo_list->buf.len - offset) < 0)
> +		return error(_("Could not write to %s (%s)"),
> +			todo_path, strerror(errno));

> -	if (commit_lock_file(&todo_lock) < 0) {
> -		strbuf_release(&buf);
> -		return error(_("Error wrapping up %s."), git_path_todo_file());
> -	}
> -	strbuf_release(&buf);
> +	if (commit_lock_file(&todo_lock) < 0)
> +		return error(_("Error wrapping up %s."), todo_path);

>  	return 0;
>  }
>  
> @@ -1026,9 +1046,8 @@ static int save_opts(struct replay_opts *opts)
>  	return res;
>  }
>  
> -static int pick_commits(struct commit_list *todo_list, struct replay_opts *opts)
> +static int pick_commits(struct todo_list *todo_list, struct replay_opts *opts)
>  {
> -	struct commit_list *cur;
>  	int res;
>  
>  	setenv(GIT_REFLOG_ACTION, action_name(opts), 0);
> @@ -1038,10 +1057,12 @@ static int pick_commits(struct commit_list *todo_list, struct replay_opts *opts)
>  	if (read_and_refresh_cache(opts))
>  		return -1;
>  
> -	for (cur = todo_list; cur; cur = cur->next) {
> +	while (todo_list->current < todo_list->nr) {

> +		struct todo_item *item = todo_list->items + todo_list->current;
> -		if (save_todo(cur, opts))
> +		if (save_todo(todo_list, opts))
>  			return -1;
> -		res = do_pick_commit(cur->item, opts);
> +		res = do_pick_commit(item->command, item->commit, opts);

> +		todo_list->current++;
>  		if (res)
>  			return res;
>  	}
> @@ -1066,7 +1087,8 @@ static int continue_single_pick(void)
>  
>  static int sequencer_continue(struct replay_opts *opts)
>  {
> -	struct commit_list *todo_list = NULL;
> +	struct todo_list todo_list = TODO_LIST_INIT;
> +	int res;
>  
>  	if (!file_exists(get_todo_path(opts)))
>  		return continue_single_pick();
> @@ -1083,21 +1105,24 @@ static int sequencer_continue(struct replay_opts *opts)
>  	}
>  	if (index_differs_from("HEAD", 0))
>  		return error_dirty_index(opts);
> -	todo_list = todo_list->next;
> -	return pick_commits(todo_list, opts);
> +	todo_list.current++;
> +	res = pick_commits(&todo_list, opts);
> +	todo_list_release(&todo_list);
> +	return res;

>  }
>  
>  static int single_pick(struct commit *cmit, struct replay_opts *opts)
>  {
>  	setenv(GIT_REFLOG_ACTION, action_name(opts), 0);
> -	return do_pick_commit(cmit, opts);
> +	return do_pick_commit(opts->action == REPLAY_PICK ?
> +		TODO_PICK : TODO_REVERT, cmit, opts);

>  }
>  
>  int sequencer_pick_revisions(struct replay_opts *opts)
>  {
> -	struct commit_list *todo_list = NULL;
> +	struct todo_list todo_list = TODO_LIST_INIT;
>  	unsigned char sha1[20];
> -	int i;
> +	int i, res;
>  
>  	if (opts->subcommand == REPLAY_NONE)
>  		assert(opts->revs);
> @@ -1171,7 +1196,9 @@ int sequencer_pick_revisions(struct replay_opts *opts)
>  	if (save_head(sha1_to_hex(sha1)) ||
>  			save_opts(opts))
>  		return -1;
> -	return pick_commits(todo_list, opts);
> +	res = pick_commits(&todo_list, opts);
> +	todo_list_release(&todo_list);
> +	return res;

> On Wed, Aug 31, 2016 at 10:29 AM, Jakub Narębski <jnareb@gmail.com> wrote:
> 
> >
> > BTW. perhaps we would be able to continue with 'git continue', regardless
> > of what we have started with, I wonder...
> >
> 
> git continue as a shorthand for `git <relevant-cmd> --continue` sounds great.

>> git continue as a shorthand for `git <relevant-cmd> --continue` sounds great.
>
> Before we get ahead of ourselves:
>
> 1) this has nothing to do with the patch series at hand, and
>
> 2) if we were to introduce `git continue`, we would need to think long and
>    hard about the following issues:
>
> 	I) are there potentially ambiguous <relevant-cmd>s that the user
> 	   may want to continue?
>
> 	II) what about options? You can say `git rebase --continue
> 	    --no-ff`, for example, but not `git cherry-pick --continue
> 	    --no-ff`...
>
> 	III) Would it not be confusing to have a subcommand `continue`
> 	     that does *not* serve a *single* purpose? It's kinda flying
> 	     into the face of the Unix philosophy.

> W dniu 29.08.2016 o 10:05, Johannes Schindelin pisze:
> 
> > When we came up with the "sequencer" idea, we really wanted to have
> > kind of a plumbing equivalent of the interactive rebase. Hence the
> > choice of words: the "todo" script, a "pick", etc.
> > 
> > However, when it came time to implement the entire shebang, somehow this
> > idea got lost and the sequencer was used as working horse for
> > cherry-pick and revert instead. So as not to interfere with the
> > interactive rebase, it even uses a separate directory to store its
> > state.
> > 
> > Furthermore, it also is stupidly strict about the "todo" script it
> > accepts: while it parses commands in a way that was *designed* to be
> > similar to the interactive rebase, it then goes on to *error out* if the
> > commands disagree with the overall action (cherry-pick or revert).
> 
> Does this mean that after the change you would be able to continue
> "git revert" with "git cherry-pick --continue", and vice versa?  Or that
> it would be possible for git-cherry-pick to do reverts (e.g. with ^<rev>)?

> > Let's just bite the bullet and rewrite the entire parser; the code now
> > becomes not only more elegant: it allows us to go on and teach the
> > sequencer how to parse *true* "todo" scripts as used by the interactive
> > rebase itself. In a way, the sequencer is about to grow up to do its
> > older brother's job. Better.
> 
> Sidenote: this is not your fault, but Git doesn't do a good job on
> changes which are mostly rewrites, trying to match stray '}' and the
> like in generated diff.  I wonder if existing diff heuristic options
> could help here.

> > While at it, do not stop at the first problem, but list *all* of the
> > problems. This helps the user by allowing to address all issues in
> > one go rather than going back and forth until the todo list is valid.
> 
> That is also a good change, though I wonder how often users need
> to worry about this outside interactive rebase case.  If it is
> preparation for rebase -i, where instruction list is written by
> prone to errors human, it would be nice to have this information
> in the commit message.

> > diff --git a/sequencer.c b/sequencer.c
> > index 982b6e9..cbdce6d 100644
> > --- a/sequencer.c
> > +++ b/sequencer.c
> > @@ -473,7 +473,26 @@ static int allow_empty(struct replay_opts *opts, struct commit *commit)
> >  		return 1;
> >  }
> >  
> > -static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
> > +enum todo_command {
> > +	TODO_PICK,
> > +	TODO_REVERT
> > +};
> 
> Do we have a naming convention for enums elements?  Or are we explicitly
> making enums and #defines interchangeable?  I wonder...
> 
> ...uh, I see we don't have naming convention, but all caps snake-case
> names dominate:
> 
>   $ git grep -A2 'enum .* {'
>   [...]
>   diff.h:enum color_diff {
>   diff.h- DIFF_RESET = 0,
>   diff.h- DIFF_CONTEXT = 1,
>   --
>   dir.c:enum path_treatment {
>   dir.c-  path_none = 0,
>   dir.c-  path_recurse,
>   --
> 
> Shouldn't we say 'TODO_PICK = 0' explicitly, though?

> > +static const char *todo_command_strings[] = {
> > +	"pick",
> > +	"revert"
> > +};
> 
> It's a bit pity that we cannot use designated inits, and hanging comma,
> (from ISO C99 standard).  That is:
> 
>   +static const char *todo_command_strings[] = {
>   +	[TODO_PICK]   = "pick",
>   +	[TODO_REVERT] = "revert",
>   +};

> > @@ -548,7 +568,7 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
> 
> From here on changes are about
> 
>   s/opts->action == REPLAY_\(PICK\|REVERT\)/command == TODO_\1/
> 
> Do we still need opts->action, or it is just needed less,
> and it is 'todo' instruction that decides about command
> (as it should)?

> > [...]
> >  	if (res) {
> > -		error(opts->action == REPLAY_REVERT
> > +		error(command == TODO_REVERT
> >  		      ? _("could not revert %s... %s")
> >  		      : _("could not apply %s... %s"),
> >  		      find_unique_abbrev(commit->object.oid.hash, DEFAULT_ABBREV),
> 
> And here those changes end.
> 
>   s/opts->action == REPLAY_\(PICK\|REVERT\)/command == TODO_\1/
> 
> I wonder if Coccinelle / Undebt would help here; or would simple
> sed or query-and-replace-regexp be enough...

> > @@ -683,116 +703,107 @@ static int read_and_refresh_cache(struct replay_opts *opts)
> >  	return 0;
> >  }
> >  
> > -static int format_todo(struct strbuf *buf, struct commit_list *todo_list,
> > -		struct replay_opts *opts)
> > +struct todo_item {
> > +	enum todo_command command;
> > +	struct commit *commit;
> > +	size_t offset_in_buf;
> > +};
> > +
> > +struct todo_list {
> > +	struct strbuf buf;
> > +	struct todo_item *items;
> > +	int nr, alloc, current;
> > +};
> 
> So there should be s/commit_list [*]{1,2}todo_list/todo_list *todo_list/
> from here on?

> > +#define TODO_LIST_INIT { STRBUF_INIT, NULL, 0, 0, 0 }
> 
> Same as with other patches in this series, it would be enough to
> 
>   +#define TODO_LIST_INIT { STRBUF_INIT }

> > -	struct commit_list *cur = NULL;
> > -	const char *sha1_abbrev = NULL;
> > -	const char *action_str = opts->action == REPLAY_REVERT ? "revert" : "pick";
> > -	const char *subject;
> > -	int subject_len;
> > +	strbuf_release(&todo_list->buf);
> > +	free(todo_list->items);
> > +	todo_list->items = NULL;
> > +	todo_list->nr = todo_list->alloc = 0;
> > +}
> >  
> > -	for (cur = todo_list; cur; cur = cur->next) {
> > -		const char *commit_buffer = get_commit_buffer(cur->item, NULL);
> > -		sha1_abbrev = find_unique_abbrev(cur->item->object.oid.hash, DEFAULT_ABBREV);
> > -		subject_len = find_commit_subject(commit_buffer, &subject);
> > -		strbuf_addf(buf, "%s %s %.*s\n", action_str, sha1_abbrev,
> > -			subject_len, subject);
> > -		unuse_commit_buffer(cur->item, commit_buffer);
> > -	}
> > -	return 0;
> > +struct todo_item *append_todo(struct todo_list *todo_list)
> 
> Errr... I don't quite understand the name of this function.
> What are you appending here to the todo_list?

> > +	ALLOC_GROW(todo_list->items, todo_list->nr + 1, todo_list->alloc);
> > +	return todo_list->items + todo_list->nr++;
> >  }
> >  
> > -static struct commit *parse_insn_line(char *bol, char *eol, struct replay_opts *opts)
> > +static int parse_insn_line(struct todo_item *item, const char *bol, char *eol)
> 
> Why the change of return type?  

> > +	for (i = 0; i < ARRAY_SIZE(todo_command_strings); i++)
> > +		if (skip_prefix(bol, todo_command_strings[i], &bol)) {
> 
> skip_prefix() is such a nice abstraction...
> 
> > +			item->command = i;
> > +			break;
> > +		}
> 
> Nice.  Replacing if-elsif chain with loop.  
> 
> I guess any hashmap would be serious overkill, as there are and would be
> only a few actions possible.

> > +	if (i >= ARRAY_SIZE(todo_command_strings))
> > +		return -1;
> >  
> >  	/* Eat up extra spaces/ tabs before object name */
> >  	padding = strspn(bol, " \t");
> >  	if (!padding)
> > -		return NULL;
> > +		return -1;
> >  	bol += padding;
> >  
> > -	end_of_object_name = bol + strcspn(bol, " \t\n");
> > +	end_of_object_name = (char *) bol + strcspn(bol, " \t\n");
> 
> Why is this cast needed?

> > -	/*
> > -	 * Verify that the action matches up with the one in
> > -	 * opts; we don't support arbitrary instructions
> > -	 */
> > -	if (action != opts->action) {
> > -		if (action == REPLAY_REVERT)
> > -		      error((opts->action == REPLAY_REVERT)
> > -			    ? _("Cannot revert during another revert.")
> 
> Errr... could the above ever happen?  Namely
> 
>   action != opts->action && action == REPLAY_REVERT && opts->action == REPLAY_REVERT
> 
> Surely not.

> > -			    : _("Cannot revert during a cherry-pick."));
> > -		else
> > -		      error((opts->action == REPLAY_REVERT)
> > -			    ? _("Cannot cherry-pick during a revert.")
> > -			    : _("Cannot cherry-pick during another cherry-pick."));
> > -		return NULL;
> > -	}
> 
> Anyway, while it is / would be a good idea to prevent starting any
> sequencer-based command (cherry-pick, revert, soon rebase -i) when
> other command is in progress (cherry-pick, revert, soon rebase -i).
> That is, if cherry-pick / revert waits for user action, you cannot
> run another cherry-pick or revert.
> 
> Which I guess the above code was not about...

> > +		item = append_todo(todo_list);
> 
> A better name, in my personal option, would be
> 
>   +		item = todo_list_next(todo_list);
> 
> Or todo_next(todo_list).

> > +		item->offset_in_buf = p - todo_list->buf.buf;
> > +		if (parse_insn_line(item, p, eol)) {
> > +			error("Invalid line: %.*s", (int)(eol - p), p);
> 
> This error message should, I think, be also translatable:
> 
>   +			error(_("Invalid line: %.*s"), (int)(eol - p), p);
> 
> > +			res |= error(_("Could not parse line %d."), i);

> > -static int read_populate_todo(struct commit_list **todo_list,
> > +static int read_populate_todo(struct todo_list *todo_list,
> >  			struct replay_opts *opts)
> >  {
> >  	const char *todo_file = get_todo_path(opts);
> 
> If I understand it correctly, replay_opts is used only to find out
> correct todo_file, isn't it?

> > -	if (strbuf_read(&buf, fd, 0) < 0) {
> > +	if (strbuf_read(&todo_list->buf, fd, 0) < 0) {
> >  		close(fd);
> > -		strbuf_release(&buf);
> 
> A question: when is todo_list->buf released?

> > -static int walk_revs_populate_todo(struct commit_list **todo_list,
> > +static int walk_revs_populate_todo(struct todo_list *todo_list,
> >  				struct replay_opts *opts)
> >  {
> > +	enum todo_command command = opts->action == REPLAY_PICK ?
> > +		TODO_PICK : TODO_REVERT;
> >  	struct commit *commit;
> > -	struct commit_list **next;
> >  
> >  	if (prepare_revs(opts))
> >  		return -1;
> >  
> > -	next = todo_list;
> > -	while ((commit = get_revision(opts->revs)))
> > -		next = commit_list_append(commit, next);
> > +	while ((commit = get_revision(opts->revs))) {
> > +		struct todo_item *item = append_todo(todo_list);
> > +		const char *commit_buffer = get_commit_buffer(commit, NULL);
> 
> I see that you are creating todo file contents while walking revision list,
> something that was left for later in current / previous implementation
> of the sequencer...

> > +		const char *subject;
> > +		int subject_len;
> > +
> > +		item->command = command;
> > +		item->commit = commit;
> > +		item->offset_in_buf = todo_list->buf.len;
> > +		subject_len = find_commit_subject(commit_buffer, &subject);
> > +		strbuf_addf(&todo_list->buf, "%s %s %.*s\n",
> > +			opts->action == REPLAY_PICK ?  "pick" : "revert",
> 
> Wouldn't it be simpler to use
> 
>   +			todo_command_strings[command],
> 
> Also, this string does not change during the loop, though I guess
> compiler should be able to optimize it.

> > +			find_unique_abbrev(commit->object.oid.hash,
> > +				DEFAULT_ABBREV),
> > +			subject_len, subject);
> 
> ...Did format of the 'todo' file changed?  And if yes, was it in backward
> compatible way, so that "git revert" or "git cherry-pick" started with
> old version of Git can be continued with new version, and what is also
> important (for somebody who sometimes uses system-installed Git, and
> sometimes user-compiled one) the reverse: started with new, continued
> with old?

> > @@ -964,30 +990,24 @@ static int sequencer_rollback(struct replay_opts *opts)
> >  	return -1;
> >  }
> >  
> > -static int save_todo(struct commit_list *todo_list, struct replay_opts *opts)
> > +static int save_todo(struct todo_list *todo_list, struct replay_opts *opts)
> >  {
> >  	static struct lock_file todo_lock;
> > -	struct strbuf buf = STRBUF_INIT;
> > -	int fd;
> > +	const char *todo_path = get_todo_path(opts);
> > +	int next = todo_list->current, offset, fd;
> 
> The "next = todo_list->current" looks a bit strange.

> Also, from 'next', 'offset' and 'fd', all those are different uses of
> int: the index (int, rarely size_t), the offset in string (formally
> ptrdiff_t, or size_t, but usually int), and the file descriptor.  I
> think from those the file descriptor could be kept in separate line; it
> would help diff to be more readable.  But this is fairly marginal
> nitpicking, and a matter of personal opinion.

> > -	fd = hold_lock_file_for_update(&todo_lock, git_path_todo_file(), 0);
> > +	fd = hold_lock_file_for_update(&todo_lock, todo_path, 0);
> >  	if (fd < 0)
> >  		return error_errno(_("Could not lock '%s'"),
> >  				   git_path_todo_file());
> 
> We should use 'todo_path' here...

> > -	if (format_todo(&buf, todo_list, opts) < 0) {
> > -		strbuf_release(&buf);
> > -		return error(_("Could not format %s."), git_path_todo_file());
> 
> Can we still get this error?  Could we get this error anyway,
> and under what conditions?

> > -	}
> > -	if (write_in_full(fd, buf.buf, buf.len) < 0) {
> > -		strbuf_release(&buf);
> > -		return error_errno(_("Could not write to %s"),
> > -				   git_path_todo_file());
> > -	}
> > +	offset = next < todo_list->nr ?
> > +		todo_list->items[next].offset_in_buf : todo_list->buf.len;
> > +	if (write_in_full(fd, todo_list->buf.buf + offset,
> > +			todo_list->buf.len - offset) < 0)
> > +		return error(_("Could not write to %s (%s)"),
> > +			todo_path, strerror(errno));
> 
> Ah, so it saves the remaining todo_items on todo_list, not the
> whole todo_list... the name does not fully show it.

> 
> > -	if (commit_lock_file(&todo_lock) < 0) {
> > -		strbuf_release(&buf);
> > -		return error(_("Error wrapping up %s."), git_path_todo_file());
> > -	}
> > -	strbuf_release(&buf);
> > +	if (commit_lock_file(&todo_lock) < 0)
> > +		return error(_("Error wrapping up %s."), todo_path);
> 
> Note: this is unrelated change, but we usually put paths in quotes, like this
> 
>   +		return error(_("Error wrapping up '%s'."), todo_path);
> 
> (in this and earlier error message), so that paths containing spaces show
> correctly and readably to the user.  Though this possibly is not a problem
> for this path.

> >  static int single_pick(struct commit *cmit, struct replay_opts *opts)
> >  {
> >  	setenv(GIT_REFLOG_ACTION, action_name(opts), 0);
> > -	return do_pick_commit(cmit, opts);
> > +	return do_pick_commit(opts->action == REPLAY_PICK ?
> > +		TODO_PICK : TODO_REVERT, cmit, opts);
> 
> The ternary conditional operator here translates one enum to other enum,
> isn't it?

>> Hmmm... commit_list is, as defined in commit.h, a linked list.
> 
> That is the most prominent reason why the rest is not a mindless
> conversion from commit_list to todo_list.
> 
> And we need todo_list as an array, because we need to be able to peek, or
> even move, backwards from the current command.
> 
>> Here todo_list uses growable array implementation of list.  Which
>> is I guess better on current CPU architecture, with slow memory,
>> limited-size caches, and adjacency prefetching.
> 
> That is not the reason that an array is used here. The array allows us
> much more flexibility.

>>> +struct todo_item *append_todo(struct todo_list *todo_list)
>>
>> Errr... I don't quite understand the name of this function.
>> What are you appending here to the todo_list?
> 
> A new item.
> 
>> Compare string_list_append() and string_list_append_nodup(),
>> where the second parameter is item to append.
> 
> Yes, that is correct. In the case of a todo_item, things are a lot more
> complicated, though. Some of the values have to be determined tediously
> (such as the offset and length of the oneline after the "pick <oid>"
> command). I just put those values directly into the newly allocated item,
> is all.

>>> +	ALLOC_GROW(todo_list->items, todo_list->nr + 1, todo_list->alloc);
>>> +	return todo_list->items + todo_list->nr++;
>>>  }
>>>  
>>> -static struct commit *parse_insn_line(char *bol, char *eol, struct replay_opts *opts)
>>> +static int parse_insn_line(struct todo_item *item, const char *bol, char *eol)
>>
>> Why the change of return type?  
> 
> Because it makes no sense to return a commit here because not all commands
> are about commits (think rebase -i's `exec`). It makes tons of sense to
> return an error condition, though.

>> Why now struct todo_item is first when struct replay_opts was last?
> 
> Those play very, very different roles.
> 
> The opts parameter used to provide parse_insn_line() with enough
> information to complain loudly when the overall command was not identical
> to the parsed command.
> 
> The item parameter is a receptacle for the parsed data. It will contain
> the pointer to the commit that was previously returned, if any. But it
> will also contain much more information, such as the command, the oneline,
> the offset in the buffer, etc etc
> 
> So "opts" was an "in" parameter while "item" is an "out" one. Apples and
> oranges.

>>> -	end_of_object_name = bol + strcspn(bol, " \t\n");
>>> +	end_of_object_name = (char *) bol + strcspn(bol, " \t\n");
>>
>> Why is this cast needed?
> 
> Because bol is a "const char *" and we need to put "NUL" temporarily to
> *end_of_object_name:

> 
>>>  	saved = *end_of_object_name;
>>>  	*end_of_object_name = '\0';
>>>  	status = get_sha1(bol, commit_sha1);
>>>  	*end_of_object_name = saved;
> 
> Technically, this would have made a fine excuse to teach get_sha1() a mode
> where it expects a length parameter instead of relying on a NUL-terminated
> string.
> 
> Practically, such fine excuses cost me months in this rebase--helper
> project already, and I need to protect my time better.

> 
>>> -	/*
>>> -	 * Verify that the action matches up with the one in
>>> -	 * opts; we don't support arbitrary instructions
>>> -	 */
>>> -	if (action != opts->action) {
>>> -		if (action == REPLAY_REVERT)
>>> -		      error((opts->action == REPLAY_REVERT)
>>> -			    ? _("Cannot revert during another revert.")
>>
>> Errr... could the above ever happen?  Namely
>>
>>   action != opts->action && action == REPLAY_REVERT && opts->action == REPLAY_REVERT
>>
>> Surely not.
> 
> Your reply pointed to the very circumstance when this may happen: `git
> cherry-pick --continue` after an interrupted `git revert`.

>>> -			    : _("Cannot revert during a cherry-pick."));
>>> -		else
>>> -		      error((opts->action == REPLAY_REVERT)
>>> -			    ? _("Cannot cherry-pick during a revert.")
>>> -			    : _("Cannot cherry-pick during another cherry-pick."));
>>> -		return NULL;
>>> -	}
>>
>> Anyway, while it is / would be a good idea to prevent starting any
>> sequencer-based command (cherry-pick, revert, soon rebase -i) when
>> other command is in progress (cherry-pick, revert, soon rebase -i).
>> That is, if cherry-pick / revert waits for user action, you cannot
>> run another cherry-pick or revert.
>>
>> Which I guess the above code was not about...
> 
> It was about that, though.

> 
> It went about it in a pretty round-about way: opts->action comes from the
> name of the command ("was I called as `git revert` or `git cherry-pick`?")
> and action comes from the todo script, which was assumed to be written by
> a previous run of the sequencer, using the then-current value of
> opts->action.

> 
> So it wrote that command into *every single line* of the todo script, *for
> the sole purpose* of verifying that it was the same action when running
> via --continue.
> 
> As I said earlier, I would not complain at all if an interrupted `git
> revert` could be continued via `git cherry-pick --continue`.
> 
> If that is not desirable, I can reintroduce that overzealous check, but
> that will have to wait until after v2.10.0. And it would require an
> argument that convinces me.

> 
>>> +		item = append_todo(todo_list);
>>
>> A better name, in my personal option, would be
>>
>>   +		item = todo_list_next(todo_list);
>>
>> Or todo_next(todo_list).
> 
> That sounds more like a function that performs the next command in the
> todo_list.
> 
> While I agree that naming is hard, I still think that `append_todo()` with
> the todo_list as single parameter and returning a todo_item is pretty much
> self-explanatory: it appends a new item to the todo_list and returns a
> pointer to it.

>>> -	if (strbuf_read(&buf, fd, 0) < 0) {
>>> +	if (strbuf_read(&todo_list->buf, fd, 0) < 0) {
>>>  		close(fd);
>>> -		strbuf_release(&buf);
>>
>> A question: when is todo_list->buf released?
> 
> Why, I am glad you asked! It is released in todo_list_release(), called at
> the end e.g. of sequencer_continue().

>>> -static int walk_revs_populate_todo(struct commit_list **todo_list,
>>> +static int walk_revs_populate_todo(struct todo_list *todo_list,
>>>  				struct replay_opts *opts)
>>>  {
>>> +	enum todo_command command = opts->action == REPLAY_PICK ?
>>> +		TODO_PICK : TODO_REVERT;
>>>  	struct commit *commit;
>>> -	struct commit_list **next;
>>>  
>>>  	if (prepare_revs(opts))
>>>  		return -1;
>>>  
>>> -	next = todo_list;
>>> -	while ((commit = get_revision(opts->revs)))
>>> -		next = commit_list_append(commit, next);
>>> +	while ((commit = get_revision(opts->revs))) {
>>> +		struct todo_item *item = append_todo(todo_list);
>>> +		const char *commit_buffer = get_commit_buffer(commit, NULL);
>>
>> I see that you are creating todo file contents while walking revision list,
>> something that was left for later in current / previous implementation
>> of the sequencer...
> 
> Not really. This function was always about generating a todo_list. It just
> did not format it yet.
> 
> With the change of keeping the original formatting of the todo script
> instead of re-formatting it in save_todo(), this function now has to
> format the todo_list itself.

>>> -static int save_todo(struct commit_list *todo_list, struct replay_opts *opts)
>>> +static int save_todo(struct todo_list *todo_list, struct replay_opts *opts)
>>>  {
>>>  	static struct lock_file todo_lock;
>>> -	struct strbuf buf = STRBUF_INIT;
>>> -	int fd;
>>> +	const char *todo_path = get_todo_path(opts);
>>> +	int next = todo_list->current, offset, fd;
>>
>> The "next = todo_list->current" looks a bit strange.
> 
> Depending whether we need rebase -i processing or revert/cherry-pick's
> slightly different one, the "current" position points to the next one
> already...
> 
>> Also, we do not change todo_list->current, we use it in one place, so it
>> can be used directly without help of temporary / helper variable.  But
>> that is just my personal opinion.
> 
> No, it has nothing to do with opinion. It prepares the code to keep it
> readable even when REPLAY_INTERACTIVE_REBASE is introduced.

>> and this should be done in one of earlier patches, isn't it?
> 
> No. I deliberately skipped save_todo() from "future-proofing" as I planned
> to rewrite it anyway. There is no point in future-proofing something you
> are going to toss in a minute.
> 

>>> -	if (commit_lock_file(&todo_lock) < 0) {
>>> -		strbuf_release(&buf);
>>> -		return error(_("Error wrapping up %s."), git_path_todo_file());
>>> -	}
>>> -	strbuf_release(&buf);
>>> +	if (commit_lock_file(&todo_lock) < 0)
>>> +		return error(_("Error wrapping up %s."), todo_path);
>>
>> Note: this is unrelated change, but we usually put paths in quotes, like this
>>
>>   +		return error(_("Error wrapping up '%s'."), todo_path);
>>
>> (in this and earlier error message), so that paths containing spaces show
>> correctly and readably to the user.  Though this possibly is not a problem
>> for this path.
> 
> Right.
> 
>> Also, how user is to understand "wrapping up"?
> 
> The same as before: the removed lines already had the error message,
> missing the quotes, too.
> 
> Don't get me wrong: I am a big fan of consistency, and I wish that Git's
> source code had more of it. So I would love to see a patch series that
> makes all error messages consistently reporting paths enclosed in single
> quotes.
> 
> I am also a big fan of the separation of concerns, though. And this patch
> series' concern is consistency *with the existing code*.
> 
> So I won't change the error message that I inherited at this point.

>>>  static int single_pick(struct commit *cmit, struct replay_opts *opts)
>>>  {
>>>  	setenv(GIT_REFLOG_ACTION, action_name(opts), 0);
>>> -	return do_pick_commit(cmit, opts);
>>> +	return do_pick_commit(opts->action == REPLAY_PICK ?
>>> +		TODO_PICK : TODO_REVERT, cmit, opts);
>>
>> The ternary conditional operator here translates one enum to other enum,
>> isn't it?
> 
> Well, almost. Please note that the enum will receive a new value in the
> sequencer-i patch series. And there is no equivalent todo_command for
> REPLAY_INTERACTIVE_REBASE.

> W dniu 01.09.2016 o 09:49, Johannes Schindelin pisze:
> > On Wed, 31 Aug 2016, Jakub Narębski wrote: 
> 
> >> Here todo_list uses growable array implementation of list.  Which is
> >> I guess better on current CPU architecture, with slow memory,
> >> limited-size caches, and adjacency prefetching.
> > 
> > That is not the reason that an array is used here. The array allows us
> > much more flexibility.
> 
> It would be nice if this reasoning (behind the change from linked list
> to growable array) was mentioned in appropriate commit message, and
> perhaps also in the cover letter for the series.  It is IMVHO quite
> important information (that you thought obvious).

> >>> +struct todo_item *append_todo(struct todo_list *todo_list)
> >>
> >> Errr... I don't quite understand the name of this function.
> >> What are you appending here to the todo_list?
> > 
> > A new item.
> > 
> >> Compare string_list_append() and string_list_append_nodup(),
> >> where the second parameter is item to append.
> > 
> > Yes, that is correct. In the case of a todo_item, things are a lot more
> > complicated, though. Some of the values have to be determined tediously
> > (such as the offset and length of the oneline after the "pick <oid>"
> > command). I just put those values directly into the newly allocated item,
> > is all.
> 
> I would expect sth_append command to take a list (or other collection),
> an element, and return [modified] collection with the new element added.
> Such API would require temporary variable in caller and memcopy in the
> sth_append() function.
> 
> This is not it.  It creates a new element, expanding a list (a collection),
> and then expose this element.  Which spares us memcopy... on non-critical
> path.
> 
> I don't know how to name operation "grow list and return new element".
> But "append" it is not.

> >>> -	end_of_object_name = bol + strcspn(bol, " \t\n");
> >>> +	end_of_object_name = (char *) bol + strcspn(bol, " \t\n");
> >>
> >> Why is this cast needed?
> > 
> > Because bol is a "const char *" and we need to put "NUL" temporarily to
> > *end_of_object_name:
> 
> Would compiler complain without this const'ness-stripping cast?

> >>>  	saved = *end_of_object_name;
> >>>  	*end_of_object_name = '\0';
> >>>  	status = get_sha1(bol, commit_sha1);
> >>>  	*end_of_object_name = saved;
> > 
> > Technically, this would have made a fine excuse to teach get_sha1() a
> > mode where it expects a length parameter instead of relying on a
> > NUL-terminated string.
> > 
> > Practically, such fine excuses cost me months in this rebase--helper
> > project already, and I need to protect my time better.
> 
> Put it in TODO list (and perhaps add a TODO comment) ;-).

> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  sequencer.c | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/sequencer.c b/sequencer.c
> index cbdce6d..1b65202 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -232,11 +232,8 @@ static int error_dirty_index(struct replay_opts *opts)
>  	if (read_cache_unmerged())
>  		return error_resolve_conflict(action_name(opts));
>  
> -	/* Different translation strings for cherry-pick and revert */
> -	if (opts->action == REPLAY_PICK)
> -		error(_("Your local changes would be overwritten by cherry-pick."));
> -	else
> -		error(_("Your local changes would be overwritten by revert."));
> +	error(_("Your local changes would be overwritten by %s."),
> +		action_name(opts));

> CC-ed to Jiang Xin, L10N coordinator.
> 
> W dniu 29.08.2016 o 10:05, Johannes Schindelin pisze:
> 
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> >  sequencer.c | 7 ++-----
> >  1 file changed, 2 insertions(+), 5 deletions(-)
> > 
> > diff --git a/sequencer.c b/sequencer.c
> > index cbdce6d..1b65202 100644
> > --- a/sequencer.c
> > +++ b/sequencer.c
> > @@ -232,11 +232,8 @@ static int error_dirty_index(struct replay_opts *opts)
> >  	if (read_cache_unmerged())
> >  		return error_resolve_conflict(action_name(opts));
> >  
> > -	/* Different translation strings for cherry-pick and revert */
> > -	if (opts->action == REPLAY_PICK)
> > -		error(_("Your local changes would be overwritten by cherry-pick."));
> > -	else
> > -		error(_("Your local changes would be overwritten by revert."));
> > +	error(_("Your local changes would be overwritten by %s."),
> > +		action_name(opts));
> 
> If I understand it correctly, it would make "revert" or "cherry-pick"
> untranslated part of error message.  You would need to use translation
> on the result with "_(action_name(opts))", you would have to mark
> todo_command_strings elements for gettext lexicon with N_(...).
> 
> I am rather against this change (see also below).

>>> -	/* Different translation strings for cherry-pick and revert */
>>> -	if (opts->action == REPLAY_PICK)
>>> -		error(_("Your local changes would be overwritten by cherry-pick."));
>>> -	else
>>> -		error(_("Your local changes would be overwritten by revert."));
>>> +	error(_("Your local changes would be overwritten by %s."),
>>> +		action_name(opts));
>>
>> If I understand it correctly, it would make "revert" or "cherry-pick"
>> untranslated part of error message.  You would need to use translation
>> on the result with "_(action_name(opts))", you would have to mark
>> todo_command_strings elements for gettext lexicon with N_(...).
>>
>> I am rather against this change (see also below).
> 
> Okay.
> 
> Unfortunately, I have to focus on the correctness of the code at the
> moment (and Git for Windows does ship *without* translations for the time
> being anyway, mostly to save on space, but also because users complained).

> 
> So I will take care of this after v2.10.0.
> 
> For the record, how is this supposed to be handled, in particular when I
> introduce a new action whose action_name(opts) will be "rebase -i"? Do I
> really need to repeat myself three times?

> W dniu 01.09.2016 o 09:52, Johannes Schindelin pisze:
> > On Wed, 31 Aug 2016, Jakub Narębski wrote:
> >> CC-ed to Jiang Xin, L10N coordinator.
> >> W dniu 29.08.2016 o 10:05, Johannes Schindelin pisze:
> 
> [...]
> >>> -	/* Different translation strings for cherry-pick and revert */
> >>> -	if (opts->action == REPLAY_PICK)
> >>> -		error(_("Your local changes would be overwritten by cherry-pick."));
> >>> -	else
> >>> -		error(_("Your local changes would be overwritten by revert."));
> >>> +	error(_("Your local changes would be overwritten by %s."),
> >>> +		action_name(opts));
> >>
> >> If I understand it correctly, it would make "revert" or "cherry-pick"
> >> untranslated part of error message.  You would need to use translation
> >> on the result with "_(action_name(opts))", you would have to mark
> >> todo_command_strings elements for gettext lexicon with N_(...).

> The `git-rebase-todo` file contains a list of commands. Most of those
> commands have the form
> 
> 	<verb> <sha1> <oneline>
> 
> The <oneline> is displayed primarily for the user's convenience, as
> rebase -i really interprets only the <verb> <sha1> part. However, there
> are *some* places in interactive rebase where the <oneline> is used to
> display messages, e.g. for reporting at which commit we stopped.
> 
> So let's just remember it when parsing the todo file; we keep a copy of
> the entire todo file anyway (to write out the new `done` and
> `git-rebase-todo` file just before processing each command), so all we
> need to do is remember the begin and end offsets.

> ---
>  sequencer.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/sequencer.c b/sequencer.c
> index 06759d4..3398774 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -709,6 +709,8 @@ static int read_and_refresh_cache(struct replay_opts *opts)
>  struct todo_item {
>  	enum todo_command command;
>  	struct commit *commit;
> +	const char *arg;
> +	int arg_len;

>  	size_t offset_in_buf;
>  };
>  
> @@ -760,6 +762,9 @@ static int parse_insn_line(struct todo_item *item, const char *bol, char *eol)
>  	status = get_sha1(bol, commit_sha1);
>  	*end_of_object_name = saved;
>  
> +	item->arg = end_of_object_name + strspn(end_of_object_name, " \t");
> +	item->arg_len = (int)(eol - item->arg);
> +

>  	if (status < 0)
>  		return -1;
>  
> @@ -880,6 +885,8 @@ static int walk_revs_populate_todo(struct todo_list *todo_list,
>  
>  		item->command = command;
>  		item->commit = commit;
> +		item->arg = NULL;
> +		item->arg_len = 0;
>  		item->offset_in_buf = todo_list->buf.len;
>  		subject_len = find_commit_subject(commit_buffer, &subject);
>  		strbuf_addf(&todo_list->buf, "%s %s %.*s\n",
> 

>> diff --git a/sequencer.c b/sequencer.c
>> index 06759d4..3398774 100644
>> --- a/sequencer.c
>> +++ b/sequencer.c
>> @@ -709,6 +709,8 @@ static int read_and_refresh_cache(struct replay_opts *opts)
>>  struct todo_item {
>>  	enum todo_command command;
>>  	struct commit *commit;
>> +	const char *arg;
>> +	int arg_len;
>
> Why 'arg', and not 'oneline', or 'subject'?
> I'm not saying it is bad name.

> Jakub Narębski <jnareb@gmail.com> writes:
> 
>>> diff --git a/sequencer.c b/sequencer.c
>>> index 06759d4..3398774 100644
>>> --- a/sequencer.c
>>> +++ b/sequencer.c
>>> @@ -709,6 +709,8 @@ static int read_and_refresh_cache(struct replay_opts *opts)
>>>  struct todo_item {
>>>  	enum todo_command command;
>>>  	struct commit *commit;
>>> +	const char *arg;
>>> +	int arg_len;

>
> So I am not sure if we want a parsed commit there (I would not
> object if we kept the texual object name read from the file,
> though).  The "one sequencer to rule them all" may even have to say
> "now give name ':1' to the result of the previous operation" in one
> step and in another later step have an instruction "merge ':1'".
> When that happens, you cannot even pre-populate the commit object
> when the sequencer reads the file, as the commit has not yet been
> created at that point.

>>>> @@ -709,6 +709,8 @@ static int read_and_refresh_cache(struct replay_opts *opts)
>>>>  struct todo_item {
>>>>  	enum todo_command command;
>>>>  	struct commit *commit;
>>>> +	const char *arg;
>>>> +	int arg_len;
>  
>> I am not sure what the "commit" field of type "struct commit *" is
>> for.  It is not needed until it is the commit's turn to be picked or
>> reverted; if we end up stopping in the middle, parsing the commit
>> object for later steps will end up being wasted effort.
>
> From what I understand this was what sequencer did before this
> series, so it is not a regression (I think; the commit parsing
> was in different function, but I think at the same place in
> the callchain).

> Jakub Narębski <jnareb@gmail.com> writes:
> 
> >>>> @@ -709,6 +709,8 @@ static int read_and_refresh_cache(struct replay_opts *opts)
> >>>>  struct todo_item {
> >>>>  	enum todo_command command;
> >>>>  	struct commit *commit;
> >>>> +	const char *arg;
> >>>> +	int arg_len;
> >  
> >> I am not sure what the "commit" field of type "struct commit *" is
> >> for.  It is not needed until it is the commit's turn to be picked or
> >> reverted; if we end up stopping in the middle, parsing the commit
> >> object for later steps will end up being wasted effort.
> >
> > From what I understand this was what sequencer did before this
> > series, so it is not a regression (I think; the commit parsing
> > was in different function, but I think at the same place in
> > the callchain).
> 
> Yes, I agree with you and I didn't mean "this is a new bug" at all.
> It just is an indication that further refactoring after this step is
> needed, and it is likely to involve removal or modification of this
> field.

> W dniu 31.08.2016 o 21:10, Junio C Hamano pisze:
> > Jakub Narębski <jnareb@gmail.com> writes:
> > 
> >>> diff --git a/sequencer.c b/sequencer.c
> >>> index 06759d4..3398774 100644
> >>> --- a/sequencer.c
> >>> +++ b/sequencer.c
> >>> @@ -709,6 +709,8 @@ static int read_and_refresh_cache(struct replay_opts *opts)
> >>>  struct todo_item {
> >>>  	enum todo_command command;
> >>>  	struct commit *commit;
> >>> +	const char *arg;
> >>> +	int arg_len;
>  
> > I am not sure what the "commit" field of type "struct commit *" is
> > for.  It is not needed until it is the commit's turn to be picked or
> > reverted; if we end up stopping in the middle, parsing the commit
> > object for later steps will end up being wasted effort.
> 
> From what I understand this was what sequencer did before this
> series, so it is not a regression (I think; the commit parsing
> was in different function, but I think at the same place in
> the callchain).

> > Also, when the sequencer becomes one sequencer to rule them all, the
> > command set may contain something that does not even mention any
> > commit at all (think: exec).
> 
> The "exec" line is a bit of exception, all other rebase -i commands
> take commit as parameter.  It could always use NULL.

> > So I am not sure if we want a parsed commit there (I would not
> > object if we kept the texual object name read from the file,
> > though).  The "one sequencer to rule them all" may even have to say
> > "now give name ':1' to the result of the previous operation" in one
> > step and in another later step have an instruction "merge ':1'".
> > When that happens, you cannot even pre-populate the commit object
> > when the sequencer reads the file, as the commit has not yet been
> > created at that point.
> 
> True, --preserve-merges rebase is well, different.

>>> So I am not sure if we want a parsed commit there (I would not
>>> object if we kept the texual object name read from the file,
>>> though).  The "one sequencer to rule them all" may even have to say
>>> "now give name ':1' to the result of the previous operation" in one
>>> step and in another later step have an instruction "merge ':1'".
>>> When that happens, you cannot even pre-populate the commit object
>>> when the sequencer reads the file, as the commit has not yet been
>>> created at that point.
>>
>> True, --preserve-merges rebase is well, different.
> 
> It is mis-designed. And I can be that harsh because it was my design.
> 
> In the meantime I came up with a much better design, and implemented it as
> a shell script on top of rebase -i. Since shell scripts run like slow
> molasses, even more so on Windows, I have a loose plan to implement its
> functionality as a new --recreate-merges option, and to deprecate
> --preserve-merges when that new option works.
> 
> It needs to be a new option (not a --preserve-merges=v2) because it is a
> totally different beast. For starters, it does not need its own code path
> that overrides pick_one, as --preserve-merges does.

> Jakub Narębski <jnareb@gmail.com> writes:
> 
> >> diff --git a/sequencer.c b/sequencer.c
> >> index 06759d4..3398774 100644
> >> --- a/sequencer.c
> >> +++ b/sequencer.c
> >> @@ -709,6 +709,8 @@ static int read_and_refresh_cache(struct replay_opts *opts)
> >>  struct todo_item {
> >>  	enum todo_command command;
> >>  	struct commit *commit;
> >> +	const char *arg;
> >> +	int arg_len;
> >
> > Why 'arg', and not 'oneline', or 'subject'?
> > I'm not saying it is bad name.
> 
> I am not sure what the "commit" field of type "struct commit *" is
> for.  It is not needed until it is the commit's turn to be picked or
> reverted; if we end up stopping in the middle, parsing the commit
> object for later steps will end up being wasted effort.

> So I am not sure if we want a parsed commit there (I would not
> object if we kept the texual object name read from the file,
> though).  The "one sequencer to rule them all" may even have to say
> "now give name ':1' to the result of the previous operation" in one
> step and in another later step have an instruction "merge ':1'".
> When that happens, you cannot even pre-populate the commit object
> when the sequencer reads the file, as the commit has not yet been
> created at that point.

>> though).  The "one sequencer to rule them all" may even have to say
>> "now give name ':1' to the result of the previous operation" in one
>> step and in another later step have an instruction "merge ':1'".
>> When that happens, you cannot even pre-populate the commit object
>> when the sequencer reads the file, as the commit has not yet been
>> created at that point.
>
> These considerations are pretty hypothetical. I would even place a bet
> that we will *never* have ":1" as names, not if I have anything to say...
> ;-)

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> >> though).  The "one sequencer to rule them all" may even have to say
> >> "now give name ':1' to the result of the previous operation" in one
> >> step and in another later step have an instruction "merge ':1'".
> >> When that happens, you cannot even pre-populate the commit object
> >> when the sequencer reads the file, as the commit has not yet been
> >> created at that point.
> >
> > These considerations are pretty hypothetical. I would even place a bet
> > that we will *never* have ":1" as names, not if I have anything to say...
> > ;-)
> 
> If you can always work with pre-existing commit, then you can
> validate all object references that appear in the instructions
> upfront.

> I was sort of expecting that, when you do the preserve-merges mode
> of "rebase -i", you would need to jump around, doing "we have
> reconstructed the side branch on a new 'onto', let's give the result
> this temporary name ':1', and then switch to the trunk (which would
> call for 'reset <commit>' instruction) and merge that thing (which
> would be 'merge :1' or perhaps called 'pick :1')", and at that point
> you no longer validate the object references upfront.

>> I was sort of expecting that, when you do the preserve-merges mode
>> of "rebase -i", you would need to jump around, doing "we have
>> reconstructed the side branch on a new 'onto', let's give the result
>> this temporary name ':1', and then switch to the trunk (which would
>> call for 'reset <commit>' instruction) and merge that thing (which
>> would be 'merge :1' or perhaps called 'pick :1')", and at that point
>> you no longer validate the object references upfront.
> 
> Except that is not how --preserve-merges works: it *still* uses the SHA-1s
> as identifiers, even when the SHA-1 may have changed in the meantime.
> 
> That is part of why it was a bad design.

> Hello Johannes,
> 
> W dniu 09.09.2016 o 17:12, Johannes Schindelin napisał:
> > On Thu, 1 Sep 2016, Junio C Hamano wrote: 
> >> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> >> I was sort of expecting that, when you do the preserve-merges mode
> >> of "rebase -i", you would need to jump around, doing "we have
> >> reconstructed the side branch on a new 'onto', let's give the result
> >> this temporary name ':1', and then switch to the trunk (which would
> >> call for 'reset <commit>' instruction) and merge that thing (which
> >> would be 'merge :1' or perhaps called 'pick :1')", and at that point
> >> you no longer validate the object references upfront.
> > 
> > Except that is not how --preserve-merges works: it *still* uses the SHA-1s
> > as identifiers, even when the SHA-1 may have changed in the meantime.
> > 
> > That is part of why it was a bad design.
> 
> When preserving merges, there are (as far as I understand it), two
> problems:
>  - what it means to preserve changes (which change to pick,
>    that is what is the mainline changes rebase is re-applying)
>  - what are parents of the merge commit (at least one parent
>    would be usually rewritten)
> 
> Maybe the internal (and perhaps also user-visible) representation
> of merge in instruction sheet could use the notation of filter-branch,
> that is 'map(<sha-1>)'... it could also imply the mainline.
> 
> That is the instruction in the internal instruction sheet could
> look like this:
> 
>   merge -m 1 map(2fd4e1c67a2d28fced849ee1bb76e7391b93eb12) da39a3ee5e6b4b0d3255bfef95601890afd80709 \t Merge 'foo' into master  
> 
> 
> Note that it has nothing to do with this series!

>> When preserving merges, there are (as far as I understand it), two
>> problems:
>>  - what it means to preserve changes (which change to pick,
>>    that is what is the mainline changes rebase is re-applying)
>>  - what are parents of the merge commit (at least one parent
>>    would be usually rewritten)
>>
>> Maybe the internal (and perhaps also user-visible) representation
>> of merge in instruction sheet could use the notation of filter-branch,
>> that is 'map(<sha-1>)'... it could also imply the mainline.
>>
>> That is the instruction in the internal instruction sheet could
>> look like this:
>>
>>   merge -m 1 map(2fd4e1c6...) da39a3ee... \t Merge 'foo' into master  
>>
>>
>> Note that it has nothing to do with this series!
> 
> Right. But I did solve that already. In the Git garden shears [*1*]
> (essentially my New And Improved attempt at recreating branch structures
> while rebasing), I generate and process scripts like this:
> 
> 	mark onto
> 
> 	# Branch: super-cool-feature
> 	rewind onto
> 	pick 00001 feature
> 	pick 00002 documentation
> 	mark super-cool-feature
> 
> 	# Branch: typo-fix
> 	rewind onto
> 	pick 0000a fix a tyop

> 
> 	rewind onto
> 	merge -C cafebabe super-cool-feature
> 	merge -C babecafe typo-fix
> 
> 	cleanup super-cool-feature typo-fix
> 
> Of course this will change a little, still, once I get around to implement
> this on top of the rebase--helper.

> 
> For example, I am not so hot about the "merge -C ..." syntax. I'll
> probably split that into a "remerge <SHA-1> <mark>" and a new "merge
> <mark>" command (the latter asking interactively for the merge commit
> message).

> 
> And also: the cleanup stage should not be necessary, as the "mark"
> commands can accumulate the known marks into a file in the state
> directory.
> 
> But you get the idea.

> 
> Ciao,
> Dscho
> 
> Footnote *1*:
> https://github.com/git-for-windows/build-extra/blob/master/shears.sh

> W dniu 11.09.2016 o 10:33, Johannes Schindelin napisał: 
> > On Fri, 9 Sep 2016, Jakub Narębski wrote:
> [...]
> 
> >> When preserving merges, there are (as far as I understand it), two
> >> problems:
> >>  - what it means to preserve changes (which change to pick,
> >>    that is what is the mainline changes rebase is re-applying)
> >>  - what are parents of the merge commit (at least one parent
> >>    would be usually rewritten)
> >>
> >> Maybe the internal (and perhaps also user-visible) representation
> >> of merge in instruction sheet could use the notation of filter-branch,
> >> that is 'map(<sha-1>)'... it could also imply the mainline.
> >>
> >> That is the instruction in the internal instruction sheet could
> >> look like this:
> >>
> >>   merge -m 1 map(2fd4e1c6...) da39a3ee... \t Merge 'foo' into master  
> >>
> >>
> >> Note that it has nothing to do with this series!
> > 
> > Right. But I did solve that already. In the Git garden shears [*1*]
> > (essentially my New And Improved attempt at recreating branch structures
> > while rebasing), I generate and process scripts like this:
> > 
> > 	mark onto
> > 
> > 	# Branch: super-cool-feature
> > 	rewind onto
> > 	pick 00001 feature
> > 	pick 00002 documentation
> > 	mark super-cool-feature
> > 
> > 	# Branch: typo-fix
> > 	rewind onto
> > 	pick 0000a fix a tyop
> 
> There probably should be there
> 
>  	mark typo-fix

> > 	rewind onto
> > 	merge -C cafebabe super-cool-feature
> > 	merge -C babecafe typo-fix
> > 
> > 	cleanup super-cool-feature typo-fix
> > 
> > Of course this will change a little, still, once I get around to implement
> > this on top of the rebase--helper.
> 
> Do I understand it correctly that it is user-visible instruction sheet, and
> not the internal instruction sheet for sequencer?  This looks very nice
> and is well readable.

> As I see, there are three basic topologies of non-linear branch to be
> rebased; all else is combination of thereof, or derivative:
> 
> 1. Merge commit without branching point, that is we need to go
>    from the following situation
> 
>    *---*---*---#---o---o---o        <-- old base
>                \\
>                 \\=a===b===M===c    <-- branch being rebased
>                           /
>          ...---x---x---x-/          <-- side branch
> 
>   to the following:
> 
>    *---*---*---#---o---o---o        
>                             \
>                              \-a'--b'--M'--c' 
>                                       /
>          ...---x---x---x-------------/          

> 2. Branching point without accompanying merge commit, or in other words
>    rebasing many branches tied together; a shrub if you will.  That is,
>    we want to go from the following situation:
> 
>    *---*---*---#---o---o---o           <-- old base
>                \
>                 \--a---b---c           <-- branch being rebased
>                         \
>                          \-1           <-- dependent branch
> 
>    to the following one:
> 
>    *---*---*---#---o---o---o
>                             \
>                              \--a'--b'--c'
>                                      \
>                                       \-1'

> 3. Branching point with merge point, that is subbranch created and
>    merged - an "eye" (it is not a loop in DAG):
> 
>    *---*---*---#---o---o---o             <-- old base
>                \
>                 \--a---b---c---M---d     <-- branch being rebased
>                     \         /
>                      \-1---2-/         [ <-- possibly a branch ]
> 
>    All edges are directed edges, with arrows pointing from right to
>    left; that is  *---*  is really  *<---*
> 
>    The expected result is:
> 
>    *---*---*---#---o---o---o
>                             \
>                              \--a'--b'--c'--M'--d'
>                                  \         /
>                                   \-1'--2'/
> 
> I guess that is the main purpose of your git-garden-shears script,
> isn't it?

> > For example, I am not so hot about the "merge -C ..." syntax. I'll
> > probably split that into a "remerge <SHA-1> <mark>" and a new "merge
> > <mark>" command (the latter asking interactively for the merge commit
> > message).
> 
> There is also an additional complication in that merge commit message
> may be *partially* automatically generated. First there is the subject
> generated by 'git merge' ("Merge branch 'foo'") or 'git pull <URL>'.
> It might have been translated, or extended.  Second there is a place
> for branch cover letter. Third, subject to merge.log / merge.summary
> there is a shortlog.

> > No :1 or some such. That's machine readable. But it's utter nonsense for
> > user-facing UIs.
> 
> Of course. It's all right for machine-facing instructions, like the
> 'todo' file for the sequencer, or for git-fast-import stream...

> On Wed, 31 Aug 2016, Junio C Hamano wrote:
>> Jakub Narębski <jnareb@gmail.com> writes:
>>
>>>> diff --git a/sequencer.c b/sequencer.c
>>>> index 06759d4..3398774 100644
>>>> --- a/sequencer.c
>>>> +++ b/sequencer.c
>>>> @@ -709,6 +709,8 @@ static int read_and_refresh_cache(struct replay_opts *opts)
>>>>  struct todo_item {
>>>>  	enum todo_command command;
>>>>  	struct commit *commit;
>>>> +	const char *arg;
>>>> +	int arg_len;
>>>
>>> Why 'arg', and not 'oneline', or 'subject'?
>>> I'm not saying it is bad name.
>>
>> I am not sure what the "commit" field of type "struct commit *" is
>> for.  It is not needed until it is the commit's turn to be picked or
>> reverted; if we end up stopping in the middle, parsing the commit
>> object for later steps will end up being wasted effort.
> 
> No, it won't be wasted effort, as we *validate* the todo script this way.
> And since we may very well need the info later (most rebases do not fail
> in the middle), we store it, too.

>                                [...] after parsing the todo_list, we will
> have to act on the information contained therein. For example we will have
> to cherry-pick some of the indicated commits (requiring a struct commit *
> for use in do_pick_commit()). Another example: we may need to determine
> the oneline for use in fixup!/squash! reordering.
> 
> So: keeping *that* aspect of the previous todo_list parsing, i.e. store a
> pointer to the already-parsed commit, is the right thing to do.

> Jakub Narębski <jnareb@gmail.com> writes:
> 
> > I wonder how probable is situation where we save instruction sheet
> > for interactive rebase, with shortened SHA-1, and during rebase
> > shortened SHA-1 stops being unambiguous...
> 
> It is my understanding that the shortened ones are only for end-user
> consumption.  The insn sheet internally uses fully expanded form for
> this exact reason, and then abbreviated back at each step before the
> updated one is presented to the end-user.  Uniqueness guarantee is
> enforced with new objects created during each step taken into
> account by doing it this way.

> W dniu 29.08.2016 o 10:05, Johannes Schindelin pisze:
> 
> > The `git-rebase-todo` file contains a list of commands. Most of those
> > commands have the form
> > 
> > 	<verb> <sha1> <oneline>
> > 
> > The <oneline> is displayed primarily for the user's convenience, as
> > rebase -i really interprets only the <verb> <sha1> part. However, there
> > are *some* places in interactive rebase where the <oneline> is used to
> > display messages, e.g. for reporting at which commit we stopped.
> > 
> > So let's just remember it when parsing the todo file; we keep a copy of
> > the entire todo file anyway (to write out the new `done` and
> > `git-rebase-todo` file just before processing each command), so all we
> > need to do is remember the begin and end offsets.
> 
> Actually what we remember is pointer and length, or begin offset and length,
> not offset and offset.

> > diff --git a/sequencer.c b/sequencer.c
> > index 06759d4..3398774 100644
> > --- a/sequencer.c
> > +++ b/sequencer.c
> > @@ -709,6 +709,8 @@ static int read_and_refresh_cache(struct replay_opts *opts)
> >  struct todo_item {
> >  	enum todo_command command;
> >  	struct commit *commit;
> > +	const char *arg;
> > +	int arg_len;
> 
> Why 'arg', and not 'oneline', or 'subject'?
> I'm not saying it is bad name.

> > @@ -760,6 +762,9 @@ static int parse_insn_line(struct todo_item *item, const char *bol, char *eol)
> >  	status = get_sha1(bol, commit_sha1);
> >  	*end_of_object_name = saved;
> >  
> > +	item->arg = end_of_object_name + strspn(end_of_object_name, " \t");
> > +	item->arg_len = (int)(eol - item->arg);
> > +
> 
> Does it work correctly for line without <oneline>, that is
> 
>   	<verb> <sha1>
> 
> I think it does, but I not entirely sure.

> @@ -377,20 +387,72 @@ static int is_index_unchanged(void)
>  	return !hashcmp(active_cache_tree->sha1, head_commit->tree->object.oid.hash);
>  }
>  
> +static char **read_author_script(void)
> +{
> +	struct strbuf script = STRBUF_INIT;
> +	int i, count = 0;
> +	char *p, *p2, **env;
> +	size_t env_size;
> +
> +	if (strbuf_read_file(&script, rebase_path_author_script(), 256) <= 0)
> +		return NULL;
> +
> +	for (p = script.buf; *p; p++)
> +		if (skip_prefix(p, "'\\\\''", (const char **)&p2))
> +			strbuf_splice(&script, p - script.buf, p2 - p, "'", 1);
> +		else if (*p == '\'')
> +			strbuf_splice(&script, p-- - script.buf, 1, "", 0);
> +		else if (*p == '\n') {
> +			*p = '\0';
> +			count++;
> +		}

> +	env_size = (count + 1) * sizeof(*env);
> +	strbuf_grow(&script, env_size);
> +	memmove(script.buf + env_size, script.buf, script.len);
> +	p = script.buf + env_size;
> +	env = (char **)strbuf_detach(&script, NULL);
> +
> +	for (i = 0; i < count; i++) {
> +		env[i] = p;
> +		p += strlen(p) + 1;
> +	}
> +	env[count] = NULL;
> +
> +	return env;
> +}
> +
>  /*
>   * If we are cherry-pick, and if the merge did not result in
>   * hand-editing, we will hit this commit and inherit the original
>   * author date and name.
>   * If we are revert, or if our cherry-pick results in a hand merge,
> - * we had better say that the current user is responsible for that.
> + * we had better say that the current user is responsible for that
> + * (except, of course, while running an interactive rebase).
>   */

> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> 
> > +/* We will introduce the 'interactive rebase' mode later */
> > +#define IS_REBASE_I() 0
> 
> I do not see a point in naming this all caps.

> > @@ -377,20 +387,72 @@ static int is_index_unchanged(void)
> >  	return !hashcmp(active_cache_tree->sha1, head_commit->tree->object.oid.hash);
> >  }
> >  
> > +static char **read_author_script(void)
> > +{
> > +	struct strbuf script = STRBUF_INIT;
> > +	int i, count = 0;
> > +	char *p, *p2, **env;
> > +	size_t env_size;
> > +
> > +	if (strbuf_read_file(&script, rebase_path_author_script(), 256) <= 0)
> > +		return NULL;
> > +
> > +	for (p = script.buf; *p; p++)
> > +		if (skip_prefix(p, "'\\\\''", (const char **)&p2))
> > +			strbuf_splice(&script, p - script.buf, p2 - p, "'", 1);
> > +		else if (*p == '\'')
> > +			strbuf_splice(&script, p-- - script.buf, 1, "", 0);
> > +		else if (*p == '\n') {
> > +			*p = '\0';
> > +			count++;
> > +		}
> 
> Hmph.  What is this loop doing?  Is it decoding a sq-quoted buffer
> or something?  Don't we have a helper function to do that?

> > +	env_size = (count + 1) * sizeof(*env);
> > +	strbuf_grow(&script, env_size);
> > +	memmove(script.buf + env_size, script.buf, script.len);
> > +	p = script.buf + env_size;
> > +	env = (char **)strbuf_detach(&script, NULL);
> > +
> > +	for (i = 0; i < count; i++) {
> > +		env[i] = p;
> > +		p += strlen(p) + 1;
> > +	}
> > +	env[count] = NULL;
> > +
> > +	return env;
> > +}
> > +
> >  /*
> >   * If we are cherry-pick, and if the merge did not result in
> >   * hand-editing, we will hit this commit and inherit the original
> >   * author date and name.
> >   * If we are revert, or if our cherry-pick results in a hand merge,
> > - * we had better say that the current user is responsible for that.
> > + * we had better say that the current user is responsible for that
> > + * (except, of course, while running an interactive rebase).
> >   */
> 
> The added "(except, ...)" reads as if "even if we are reverting, if
> that is done as part of an interactive rebase, the authorship rule
> for a revert does not apply".
> 
> If that is not what you meant, i.e. if you did not mean to imply
> that "rebase -i" doing a revert is a normal thing, this needs to be
> rephrased to avoid the misinterpretation.

>> > +static char **read_author_script(void)
>> > +{
>> > +	struct strbuf script = STRBUF_INIT;
>> > +	int i, count = 0;
>> > +	char *p, *p2, **env;
>> > +	size_t env_size;
>> > +
>> > +	if (strbuf_read_file(&script, rebase_path_author_script(), 256) <= 0)
>> > +		return NULL;
>> > +
>> > +	for (p = script.buf; *p; p++)
>> > +		if (skip_prefix(p, "'\\\\''", (const char **)&p2))
>> > +			strbuf_splice(&script, p - script.buf, p2 - p, "'", 1);
>> > +		else if (*p == '\'')
>> > +			strbuf_splice(&script, p-- - script.buf, 1, "", 0);
>> > +		else if (*p == '\n') {
>> > +			*p = '\0';
>> > +			count++;
>> > +		}
>> 
>> Hmph.  What is this loop doing?  Is it decoding a sq-quoted buffer
>> or something?  Don't we have a helper function to do that?
>
> Well, it is not just decoding an sq-quoted buffer, but several lines with
> definitions we sq-quoted ourselves, individually.
>
> The quote.[ch] code currently has no code to dequote lines individually.

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> >> > +static char **read_author_script(void)
> >> > +{
> >> > +	struct strbuf script = STRBUF_INIT;
> >> > +	int i, count = 0;
> >> > +	char *p, *p2, **env;
> >> > +	size_t env_size;
> >> > +
> >> > +	if (strbuf_read_file(&script, rebase_path_author_script(), 256) <= 0)
> >> > +		return NULL;
> >> > +
> >> > +	for (p = script.buf; *p; p++)
> >> > +		if (skip_prefix(p, "'\\\\''", (const char **)&p2))
> >> > +			strbuf_splice(&script, p - script.buf, p2 - p, "'", 1);
> >> > +		else if (*p == '\'')
> >> > +			strbuf_splice(&script, p-- - script.buf, 1, "", 0);
> >> > +		else if (*p == '\n') {
> >> > +			*p = '\0';
> >> > +			count++;
> >> > +		}
> >> 
> >> Hmph.  What is this loop doing?  Is it decoding a sq-quoted buffer
> >> or something?  Don't we have a helper function to do that?
> >
> > Well, it is not just decoding an sq-quoted buffer, but several lines with
> > definitions we sq-quoted ourselves, individually.
> >
> > The quote.[ch] code currently has no code to dequote lines individually.
> 
> There is a function with exactly the same name in builtin/am.c and I
> assume that it is reading from a file with the same format, which
> uses a helper to read one variable line at a time.  Hopefully that
> can be refactored so that more parsing is shared between the two
> users.
> 
> Two functions with the same name reading from the same format, even
> when they expect to produce the same result in different internal
> format, without even being aware of each other is a bad enough
> "regression" in maintainability of the code.  One of them not even
> using sq_dequote() helper and reinventing is even worse.

> Two functions with the same name reading from the same format, even
> when they expect to produce the same result in different internal
> format, without even being aware of each other is a bad enough
> "regression" in maintainability of the code.  One of them not even
> using sq_dequote() helper and reinventing is even worse.

> From: Junio C Hamano <gitster@pobox.com>
> Date: Tue, 30 Aug 2016 12:36:42 -0700
> Subject: [PATCH] am: refactor read_author_script()
> 
> [...]

> 
> This patch teaches the sequencer that trick, as part of the bigger
> effort to make the sequencer the work horse of the interactive rebase.
> 
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  sequencer.c | 48 +++++++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 43 insertions(+), 5 deletions(-)
> 
> diff --git a/sequencer.c b/sequencer.c
> index 4204cc8..e094ac2 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -15,6 +15,7 @@
>  #include "merge-recursive.h"
>  #include "refs.h"
>  #include "argv-array.h"
> +#include "quote.h"
>  
>  #define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION"
>  
> @@ -33,6 +34,11 @@ static GIT_PATH_FUNC(git_path_head_file, "sequencer/head")
>   * being rebased.
>   */
>  static GIT_PATH_FUNC(rebase_path_author_script, "rebase-merge/author-script")
> +/*
> + * The following files are written by git-rebase just after parsing the
> + * command-line (and are only consumed, not modified, by the sequencer).
> + */

>  
>  /* We will introduce the 'interactive rebase' mode later */
>  #define IS_REBASE_I() 0
> @@ -129,6 +135,16 @@ static int has_conforming_footer(struct strbuf *sb, struct strbuf *sob,
>  	return 1;
>  }
>  
> +static const char *gpg_sign_opt_quoted(struct replay_opts *opts)
> +{
> +	static struct strbuf buf = STRBUF_INIT;
> +
> +	strbuf_reset(&buf);
> +	if (opts->gpg_sign)
> +		sq_quotef(&buf, "-S%s", opts->gpg_sign);
> +	return buf.buf;
> +}

> +
>  void *sequencer_entrust(struct replay_opts *opts, void *set_me_free_after_use)
>  {
>  	ALLOC_GROW(opts->owned, opts->owned_nr + 1, opts->owned_alloc);
> @@ -471,17 +487,20 @@ int sequencer_commit(const char *defmsg, struct replay_opts *opts,
>  
>  	if (IS_REBASE_I()) {
>  		env = read_author_script();
> -		if (!env)
> +		if (!env) {
> +			const char *gpg_opt = gpg_sign_opt_quoted(opts);
> +
>  			return error("You have staged changes in your working "
>  				"tree. If these changes are meant to be\n"
>  				"squashed into the previous commit, run:\n\n"
> -				"  git commit --amend $gpg_sign_opt_quoted\n\n"

> +				"  git commit --amend %s\n\n"
>  				"If they are meant to go into a new commit, "
>  				"run:\n\n"
> -				"  git commit $gpg_sign_opt_quoted\n\n"
> +				"  git commit %s\n\n"
>  				"In both case, once you're done, continue "
>  				"with:\n\n"
> -				"  git rebase --continue\n");
> +				"  git rebase --continue\n", gpg_opt, gpg_opt);

>  	}
>  
>  	argv_array_init(&array);
> @@ -955,8 +974,27 @@ static int populate_opts_cb(const char *key, const char *value, void *data)
>  
>  static int read_populate_opts(struct replay_opts *opts)
>  {
> -	if (IS_REBASE_I())
> +	if (IS_REBASE_I()) {
> +		struct strbuf buf = STRBUF_INIT;
> +
> +		if (read_oneliner(&buf, rebase_path_gpg_sign_opt(), 1)) {
> +			if (buf.len && buf.buf[buf.len - 1] == '\n') {
> +				if (--buf.len &&
> +				    buf.buf[buf.len - 1] == '\r')
> +					buf.len--;
> +				buf.buf[buf.len] = '\0';
> +			}

> +			}
> +		}
> +
>  		return 0;
> +	}
>  
>  	if (!file_exists(git_path_opts_file()))
>  		return 0;
> 

>> +			else {
>> +				opts->gpg_sign = buf.buf + 2;
>> +				strbuf_detach(&buf, NULL);
>
> Wouldn't we leak 2 characters that got skipped?  Maybe xstrdup would
> be better (if it is leaked, and not reattached)?

>>> +			else {
>>> +				opts->gpg_sign = buf.buf + 2;
>>> +				strbuf_detach(&buf, NULL);
>>
>> Wouldn't we leak 2 characters that got skipped?  Maybe xstrdup would
>> be better (if it is leaked, and not reattached)?
> 
> An attempt to avoid leaking by calling free(opts->gpg_sign) would
> make it crash, which would be even worse ;-).

> Note that if the last line was
> 
>     +				sequencer_entrust(opts, strbuf_detach(&buf, NULL));
> 
> we can make it not leak.  A bit tricksy, though.

> Jakub Narębski <jnareb@gmail.com> writes:
> 
> >> +			else {
> >> +				opts->gpg_sign = buf.buf + 2;
> >> +				strbuf_detach(&buf, NULL);
> >
> > Wouldn't we leak 2 characters that got skipped?  Maybe xstrdup would
> > be better (if it is leaked, and not reattached)?
> 
> An attempt to avoid leaking by calling free(opts->gpg_sign) would
> make it crash, which would be even worse ;-).

> W dniu 29.08.2016 o 10:06, Johannes Schindelin pisze:
> 
> > The rebase command sports a `--gpg-sign` option that is heeded by the
> > interactive rebase.
> 
> Should it be "sports" or "supports"?

> > +static GIT_PATH_FUNC(rebase_path_gpg_sign_opt, "rebase-merge/gpg_sign_opt")
> 
> I know it is not your fault, but I wonder why this file uses
> snake_case_name, while all other use kebab-case-names.  That is,
> why it is gpg_sign_opt and not gpg-sign-opt.

> > @@ -471,17 +487,20 @@ int sequencer_commit(const char *defmsg, struct replay_opts *opts,
> >  
> >  	if (IS_REBASE_I()) {
> >  		env = read_author_script();
> > -		if (!env)
> > +		if (!env) {
> > +			const char *gpg_opt = gpg_sign_opt_quoted(opts);
> > +
> >  			return error("You have staged changes in your working "
> >  				"tree. If these changes are meant to be\n"
> >  				"squashed into the previous commit, run:\n\n"
> > -				"  git commit --amend $gpg_sign_opt_quoted\n\n"
> 
> How did this get expanded by error(), and why we want to replace
> it if it works?

> 
> > +				"  git commit --amend %s\n\n"
> >  				"If they are meant to go into a new commit, "
> >  				"run:\n\n"
> > -				"  git commit $gpg_sign_opt_quoted\n\n"
> > +				"  git commit %s\n\n"
> >  				"In both case, once you're done, continue "
> >  				"with:\n\n"
> > -				"  git rebase --continue\n");
> > +				"  git rebase --continue\n", gpg_opt, gpg_opt);
> 
> Instead of passing option twice, why not make use of %1$s (arg reordering),
> that is
> 
>   +				"  git commit --amend %1$s\n\n"
> [...]
>   +				"  git commit %1$s\n\n"

> > @@ -955,8 +974,27 @@ static int populate_opts_cb(const char *key, const char *value, void *data)
> >  
> >  static int read_populate_opts(struct replay_opts *opts)
> >  {
> > -	if (IS_REBASE_I())
> > +	if (IS_REBASE_I()) {
> > +		struct strbuf buf = STRBUF_INIT;
> > +
> > +		if (read_oneliner(&buf, rebase_path_gpg_sign_opt(), 1)) {
> > +			if (buf.len && buf.buf[buf.len - 1] == '\n') {
> > +				if (--buf.len &&
> > +				    buf.buf[buf.len - 1] == '\r')
> > +					buf.len--;
> > +				buf.buf[buf.len] = '\0';
> > +			}
> 
> Isn't there some strbuf_chomp() / strbuf_strip_eof() function?
> Though as strbuf_getline() uses something similar...

> > +			else {
> > +				opts->gpg_sign = buf.buf + 2;
> > +				strbuf_detach(&buf, NULL);
> 
> Wouldn't we leak 2 characters that got skipped?  Maybe xstrdup would
> be better (if it is leaked, and not reattached)?

>>> @@ -471,17 +487,20 @@ int sequencer_commit(const char *defmsg, struct replay_opts *opts,
>>>  
>>>  	if (IS_REBASE_I()) {
>>>  		env = read_author_script();
>>> -		if (!env)
>>> +		if (!env) {
>>> +			const char *gpg_opt = gpg_sign_opt_quoted(opts);
>>> +
>>>  			return error("You have staged changes in your working "
>>>  				"tree. If these changes are meant to be\n"
>>>  				"squashed into the previous commit, run:\n\n"
>>> -				"  git commit --amend $gpg_sign_opt_quoted\n\n"
>>
>> How did this get expanded by error(), and why we want to replace
>> it if it works?

>>
>>> +				"  git commit --amend %s\n\n"
>>>  				"If they are meant to go into a new commit, "
>>>  				"run:\n\n"
>>> -				"  git commit $gpg_sign_opt_quoted\n\n"
>>> +				"  git commit %s\n\n"
>>>  				"In both case, once you're done, continue "
>>>  				"with:\n\n"
>>> -				"  git rebase --continue\n");
>>> +				"  git rebase --continue\n", gpg_opt, gpg_opt);
>>
>> Instead of passing option twice, why not make use of %1$s (arg reordering),
>> that is
>>
>>   +				"  git commit --amend %1$s\n\n"
>> [...]
>>   +				"  git commit %1$s\n\n"
> 
> Cute. But would this not drive the l10ners insane?
> 

> +       if (strbuf_read_file(buf, path, 0) < 0) {
> +               warning_errno("could not read '%s'", path);
> +               return 0;
> +       }
> +
> +       if (buf->len > orig_len && buf->buf[buf->len - 1] == '\n') {
> +               if (--buf->len > orig_len && buf->buf[buf->len - 1]
> == '\r')
> +                       --buf->len;
> +               buf->buf[buf->len] = '\0';
> +       }

> On ma, 2016-08-29 at 10:06 +0200, Johannes Schindelin wrote:
> > +       if (strbuf_read_file(buf, path, 0) < 0) {
> > +               warning_errno("could not read '%s'", path);
> > +               return 0;
> > +       }
> > +
> > +       if (buf->len > orig_len && buf->buf[buf->len - 1] == '\n') {
> > +               if (--buf->len > orig_len && buf->buf[buf->len - 1]
> > == '\r')
> > +                       --buf->len;
> > +               buf->buf[buf->len] = '\0';
> > +       }
> 
> Why not use open + strbuf_getline instead of hand-rolling a newline
> eradicator?

> In the upcoming commits, we will implement more and more of rebase
> -i's functionality. One particular feature of the commands to come is
> that some of them allow editing the commit message while others don't,
> i.e. we cannot define in the replay_opts whether the commit message
> should be edited or not.

> 
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  sequencer.c | 6 +++---
>  sequencer.h | 2 +-
>  2 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/sequencer.c b/sequencer.c
> index e094ac2..7e17d14 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -478,7 +478,7 @@ static char **read_author_script(void)
>   * (except, of course, while running an interactive rebase).
>   */
>  int sequencer_commit(const char *defmsg, struct replay_opts *opts,
> -			  int allow_empty)
> +			  int allow_empty, int edit)
>  {
>  	char **env = NULL;
>  	struct argv_array array;
> @@ -513,7 +513,7 @@ int sequencer_commit(const char *defmsg, struct replay_opts *opts,
>  		argv_array_push(&array, "-s");
>  	if (defmsg)
>  		argv_array_pushl(&array, "-F", defmsg, NULL);
> -	if (opts->edit)
> +	if (edit)
>  		argv_array_push(&array, "-e");
>  	else if (!opts->signoff && !opts->record_origin &&
>  		 git_config_get_value("commit.cleanup", &value))
> @@ -779,7 +779,7 @@ static int do_pick_commit(enum todo_command command, struct commit *commit,
>  	}
>  	if (!opts->no_commit)
>  		res = sequencer_commit(opts->edit ? NULL : git_path_merge_msg(),
> -			opts, allow);
> +			opts, allow, opts->edit);
>  
>  leave:
>  	free_message(commit, &msg);
> diff --git a/sequencer.h b/sequencer.h
> index 9f63c31..fd02baf 100644
> --- a/sequencer.h
> +++ b/sequencer.h
> @@ -50,7 +50,7 @@ int sequencer_rollback(struct replay_opts *opts);
>  int sequencer_remove_state(struct replay_opts *opts);
>  
>  int sequencer_commit(const char *defmsg, struct replay_opts *opts,
> -			  int allow_empty);
> +			  int allow_empty, int edit);
>  
>  extern const char sign_off_header[];
>  
> 

> W dniu 29.08.2016 o 10:06, Johannes Schindelin pisze:
> 
> > In the upcoming commits, we will implement more and more of rebase
> > -i's functionality. One particular feature of the commands to come is
> > that some of them allow editing the commit message while others don't,
> > i.e. we cannot define in the replay_opts whether the commit message
> > should be edited or not.
> 
> It's a nice, pretty and self contained refactoring step.  Small
> enough that it is easy to review.
> 
> I would like to have in the commit message that it is sequencer_commit()
> function that needs to rely on new parameter, instead of on a property
> of command (of its replay_opts).  And that currently it simply passes
> the buck to caller, which uses opts->edit, but in the future the
> caller that is rebase -i would use todo_item and replay_opts based
> expression.

> This teaches the sequencer_commit() function to take an argument that
> will allow us to implement "todo" commands that need to amend the commit
> messages ("fixup", "squash" and "reword").
> 
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  sequencer.c | 6 ++++--
>  sequencer.h | 2 +-
>  2 files changed, 5 insertions(+), 3 deletions(-)

> 
> diff --git a/sequencer.c b/sequencer.c
> index 7e17d14..20f7590 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -478,7 +478,7 @@ static char **read_author_script(void)
>   * (except, of course, while running an interactive rebase).
>   */
>  int sequencer_commit(const char *defmsg, struct replay_opts *opts,
> -			  int allow_empty, int edit)
> +			  int allow_empty, int edit, int amend)

>  {
>  	char **env = NULL;
>  	struct argv_array array;
> @@ -507,6 +507,8 @@ int sequencer_commit(const char *defmsg, struct replay_opts *opts,
>  	argv_array_push(&array, "commit");
>  	argv_array_push(&array, "-n");
>  
> +	if (amend)
> +		argv_array_push(&array, "--amend");
>  	if (opts->gpg_sign)
>  		argv_array_pushf(&array, "-S%s", opts->gpg_sign);
>  	if (opts->signoff)
> @@ -779,7 +781,7 @@ static int do_pick_commit(enum todo_command command, struct commit *commit,
>  	}
>  	if (!opts->no_commit)
>  		res = sequencer_commit(opts->edit ? NULL : git_path_merge_msg(),
> -			opts, allow, opts->edit);
> +			opts, allow, opts->edit, 0);

>  
>  leave:
>  	free_message(commit, &msg);
> diff --git a/sequencer.h b/sequencer.h
> index fd02baf..2106c0d 100644
> --- a/sequencer.h
> +++ b/sequencer.h
> @@ -50,7 +50,7 @@ int sequencer_rollback(struct replay_opts *opts);
>  int sequencer_remove_state(struct replay_opts *opts);
>  
>  int sequencer_commit(const char *defmsg, struct replay_opts *opts,
> -			  int allow_empty, int edit);
> +			  int allow_empty, int edit, int amend);
>  
>  extern const char sign_off_header[];
>  
> 

> W dniu 29.08.2016 o 10:06, Johannes Schindelin pisze:
> 
> > diff --git a/sequencer.c b/sequencer.c
> > index 7e17d14..20f7590 100644
> > --- a/sequencer.c
> > +++ b/sequencer.c
> > @@ -478,7 +478,7 @@ static char **read_author_script(void)
> >   * (except, of course, while running an interactive rebase).
> >   */
> >  int sequencer_commit(const char *defmsg, struct replay_opts *opts,
> > -			  int allow_empty, int edit)
> > +			  int allow_empty, int edit, int amend)
> 
> I guess we won't get much more parameters; it would get unwieldy
> (and hard to remember).  Five is all right.

> >  {
> >  	char **env = NULL;
> >  	struct argv_array array;
> > @@ -507,6 +507,8 @@ int sequencer_commit(const char *defmsg, struct replay_opts *opts,
> >  	argv_array_push(&array, "commit");
> >  	argv_array_push(&array, "-n");
> >  
> > +	if (amend)
> > +		argv_array_push(&array, "--amend");
> >  	if (opts->gpg_sign)
> >  		argv_array_pushf(&array, "-S%s", opts->gpg_sign);
> >  	if (opts->signoff)
> > @@ -779,7 +781,7 @@ static int do_pick_commit(enum todo_command command, struct commit *commit,
> >  	}
> >  	if (!opts->no_commit)
> >  		res = sequencer_commit(opts->edit ? NULL : git_path_merge_msg(),
> > -			opts, allow, opts->edit);
> > +			opts, allow, opts->edit, 0);
> 
> ... even of this makes one need to check the calling convention,
> what does this 0 mean.

> The sequencer_commit() function already knows how to amend commits, and
> with this new option, it can also clean up commit messages (i.e. strip
> out commented lines). This is needed to implement rebase -i's 'fixup'
> and 'squash' commands as sequencer commands.
> 
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  sequencer.c | 10 +++++++---
>  sequencer.h |  3 ++-
>  2 files changed, 9 insertions(+), 4 deletions(-)

> 
> diff --git a/sequencer.c b/sequencer.c
> index 20f7590..5ec956f 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -478,7 +478,8 @@ static char **read_author_script(void)
>   * (except, of course, while running an interactive rebase).
>   */
>  int sequencer_commit(const char *defmsg, struct replay_opts *opts,
> -			  int allow_empty, int edit, int amend)
> +			  int allow_empty, int edit, int amend,
> +			  int cleanup_commit_message)

>  {
>  	char **env = NULL;
>  	struct argv_array array;
> @@ -515,9 +516,12 @@ int sequencer_commit(const char *defmsg, struct replay_opts *opts,
>  		argv_array_push(&array, "-s");
>  	if (defmsg)
>  		argv_array_pushl(&array, "-F", defmsg, NULL);
> +	if (cleanup_commit_message)
> +		argv_array_push(&array, "--cleanup=strip");

>  
> @@ -781,7 +785,7 @@ static int do_pick_commit(enum todo_command command, struct commit *commit,
>  	}
>  	if (!opts->no_commit)
>  		res = sequencer_commit(opts->edit ? NULL : git_path_merge_msg(),
> -			opts, allow, opts->edit, 0);
> +			opts, allow, opts->edit, 0, 0);

>  
>  leave:
>  	free_message(commit, &msg);
> diff --git a/sequencer.h b/sequencer.h
> index 2106c0d..e272549 100644
> --- a/sequencer.h
> +++ b/sequencer.h
> @@ -50,7 +50,8 @@ int sequencer_rollback(struct replay_opts *opts);
>  int sequencer_remove_state(struct replay_opts *opts);
>  
>  int sequencer_commit(const char *defmsg, struct replay_opts *opts,
> -			  int allow_empty, int edit, int amend);
> +			  int allow_empty, int edit, int amend,
> +			  int cleanup_commit_message);
>  
>  extern const char sign_off_header[];
>  
> 

> W dniu 29.08.2016 o 10:06, Johannes Schindelin pisze:
> 
> > @@ -781,7 +785,7 @@ static int do_pick_commit(enum todo_command command, struct commit *commit,
> >  	}
> >  	if (!opts->no_commit)
> >  		res = sequencer_commit(opts->edit ? NULL : git_path_merge_msg(),
> > -			opts, allow, opts->edit, 0);
> > +			opts, allow, opts->edit, 0, 0);
> 
> The calling convention begins to look unwieldy, but we have only
> a single such callsite, and there are quite a bit callsites in
> Git code that have similar API ("git grep ', 0, 0' -- '*.c'").
> So we don't need to think about alternatives.  Yet.

> It's a pity that emulation of named parameters in C requires
> relying on designated inits from C99
> 
>   typedef struct {
>     double pressure, moles, temp;
>   } ideal_struct;
> 
>   #define ideal_pressure(...) ideal_pressure_base((ideal_struct){.pressure=1,   \
>                                         .moles=1, .temp=273.15, __VA_ARGS__})
> 
>   double ideal_pressure_base(ideal_struct in)
>   {
>     return 8.314 * in.moles*in.temp/in.pressure;
>   }
> 
>   ... ideal_pressure(.moles=2, .temp=373.15) ...

>> It's a pity that emulation of named parameters in C requires
>> relying on designated inits from C99
>>
>>   typedef struct {
>>     double pressure, moles, temp;
>>   } ideal_struct;
>>
>>   #define ideal_pressure(...) ideal_pressure_base((ideal_struct){.pressure=1,   \
>>                                         .moles=1, .temp=273.15, __VA_ARGS__})
>>
>>   double ideal_pressure_base(ideal_struct in)
>>   {
>>     return 8.314 * in.moles*in.temp/in.pressure;
>>   }
>>
>>   ... ideal_pressure(.moles=2, .temp=373.15) ...

> On ma, 2016-08-29 at 10:06 +0200, Johannes Schindelin wrote:
> 
> > The return value of do_recursive_merge() may be positive (indicating merge
> > conflicts), se let's OR later error conditions so as not to overwrite them
> > with 0.
> 
> s/se/so/?

> diff --git a/sequencer.c b/sequencer.c
> index 5ec956f..0614b90 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -623,7 +623,7 @@ static int do_pick_commit(enum todo_command command, struct commit *commit,
>  	const char *base_label, *next_label;
>  	struct commit_message msg = { NULL, NULL, NULL, NULL };
>  	struct strbuf msgbuf = STRBUF_INIT;
> -	int res, unborn = 0, allow;
> +	int res = 0, unborn = 0, allow;

> W dniu 29.08.2016 o 10:06, Johannes Schindelin pisze:
>
>> diff --git a/sequencer.c b/sequencer.c
>> index 5ec956f..0614b90 100644
>> --- a/sequencer.c
>> +++ b/sequencer.c
>> @@ -623,7 +623,7 @@ static int do_pick_commit(enum todo_command command, struct commit *commit,
>>  	const char *base_label, *next_label;
>>  	struct commit_message msg = { NULL, NULL, NULL, NULL };
>>  	struct strbuf msgbuf = STRBUF_INIT;
>> -	int res, unborn = 0, allow;
>> +	int res = 0, unborn = 0, allow;
>
> Not that I am against this part of change, making initialization
> explicit, but why we are initializing automatic variables with 0,
> which would be the default value anyway?

> On ma, 2016-08-29 at 10:05 +0200, Johannes Schindelin wrote:
> 
> <snip actual commit>
> 
> I fail to see the point of this patch, would you mind enlightening me?

>
> Let's allow for such use cases by refactoring write_message() to allow
> for a convenience function write_file_gently(). As some of the upcoming
> callers of that new function will want to append a newline character,
> let's just add a flag for that, too.

> 
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  sequencer.c | 21 ++++++++++++++++++---
>  1 file changed, 18 insertions(+), 3 deletions(-)
> 
> diff --git a/sequencer.c b/sequencer.c
> index 5efed2e..f5b5e5e 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -239,22 +239,37 @@ static void print_advice(int show_hint, struct replay_opts *opts)
>  	}
>  }
>  
> -static int write_message(struct strbuf *msgbuf, const char *filename)
> +static int write_with_lock_file(const char *filename,
> +				const void *buf, size_t len, int append_eol)
>  {
>  	static struct lock_file msg_file;
>  
>  	int msg_fd = hold_lock_file_for_update(&msg_file, filename, 0);
>  	if (msg_fd < 0)
>  		return error_errno(_("Could not lock '%s'"), filename);
> -	if (write_in_full(msg_fd, msgbuf->buf, msgbuf->len) < 0)
> +	if (write_in_full(msg_fd, buf, len) < 0)
>  		return error_errno(_("Could not write to %s"), filename);

>  
>  	return 0;
>  }
>  
> +static int write_message(struct strbuf *msgbuf, const char *filename)
> +{
> +	int res = write_with_lock_file(filename, msgbuf->buf, msgbuf->len, 0);
> +	strbuf_release(msgbuf);
> +	return res;
> +}

> +
> +static int write_file_gently(const char *filename,
> +			     const char *text, int append_eol)
> +{
> +	return write_with_lock_file(filename, text, strlen(text), append_eol);
> +}

> +
>  /*
>   * Reads a file that was presumably written by a shell script, i.e.
>   * with an end-of-line marker that needs to be stripped.
> 

> W dniu 29.08.2016 o 10:06, Johannes Schindelin pisze:
> 
> > The write_message() function safely writes an strbuf to a file.
> > Sometimes this is inconvenient, though: the text to be written may not
> > be stored in a strbuf, or the strbuf should not be released after
> > writing.
> 
> By "this" you mean "using strbuf", isn't it?  It is not very obvious,
> and I think it would be better to say it explicitly.

> > Let's allow for such use cases by refactoring write_message() to allow
> > for a convenience function write_file_gently(). As some of the
> > upcoming callers of that new function will want to append a newline
> > character, let's just add a flag for that, too.
> 
> This paragraph feels a bit convoluted.
> 
> As I understand it, you refactor "safely writing string to a file"
> into write_with_lock_file(), and make write_message() use it.  The
> new function makes it easy to create new convenience function 
> write_file_gently(); as some of the upcoming callers of this new
> function would want to append a newline character, add a flag for
> it in write_file_gently(), and thus in write_with_lock_file().
> 
> Isn't it better / easier to understand?

> > diff --git a/sequencer.c b/sequencer.c
> > index 5efed2e..f5b5e5e 100644
> > --- a/sequencer.c
> > +++ b/sequencer.c
> > @@ -239,22 +239,37 @@ static void print_advice(int show_hint, struct replay_opts *opts)
> >  	}
> >  }
> >  
> > -static int write_message(struct strbuf *msgbuf, const char *filename)
> > +static int write_with_lock_file(const char *filename,
> > +				const void *buf, size_t len, int append_eol)
> >  {
> >  	static struct lock_file msg_file;
> >  
> >  	int msg_fd = hold_lock_file_for_update(&msg_file, filename, 0);
> >  	if (msg_fd < 0)
> >  		return error_errno(_("Could not lock '%s'"), filename);
> > -	if (write_in_full(msg_fd, msgbuf->buf, msgbuf->len) < 0)
> > +	if (write_in_full(msg_fd, buf, len) < 0)
> >  		return error_errno(_("Could not write to %s"), filename);
> 
> You could have, for consistency, add quotes around filename (see previous
> error_errno callsite), *while at it*:
> 
>   		return error_errno(_("Could not write to '%s'"), filename);

> > -	strbuf_release(msgbuf);
> > +	if (append_eol && write(msg_fd, "\n", 1) < 0)
> > +		return error_errno(_("Could not write eol to %s"), filename);
> 
> Same here, and it wouldn't even be 'while at it'

>   +		return error_errno(_("Could not write eol to '%s'"), filename);
> 
> 
> >  	if (commit_lock_file(&msg_file) < 0)
> >  		return error(_("Error wrapping up %s."), filename);
> 
> Another "while at it"... though the one that can be safely postponed
> (well, the make message easier to understand part, not the quote
> filename part):
> 
>   		return error(_("Error wrapping up writing to '%s'."), filename);

>>>  	if (commit_lock_file(&msg_file) < 0)
>>>  		return error(_("Error wrapping up %s."), filename);
>>
>> Another "while at it"... though the one that can be safely postponed
>> (well, the make message easier to understand part, not the quote
>> filename part):
>>
>>   		return error(_("Error wrapping up writing to '%s'."), filename);
> 
> As I inherited this message, I'll keep it.

> W dniu 01.09.2016 o 16:20, Johannes Schindelin pisze:
> > On Thu, 1 Sep 2016, Jakub Narębski wrote: 
> >> W dniu 29.08.2016 o 10:06, Johannes Schindelin pisze:
> 
> >>>  	if (commit_lock_file(&msg_file) < 0)
> >>>  		return error(_("Error wrapping up %s."), filename);
> >>
> >> Another "while at it"... though the one that can be safely postponed
> >> (well, the make message easier to understand part, not the quote
> >> filename part):
> >>
> >>   		return error(_("Error wrapping up writing to '%s'."), filename);
> > 
> > As I inherited this message, I'll keep it.
> 
> Well, please then add quotes while at it, at least, for consistency
> 
>   		return error(_("Error wrapping up '%s'."), filename);

> On Fri, 2 Sep 2016, Jakub Narębski wrote:
>> W dniu 01.09.2016 o 16:20, Johannes Schindelin pisze:
>>> On Thu, 1 Sep 2016, Jakub Narębski wrote: 
>>>> W dniu 29.08.2016 o 10:06, Johannes Schindelin pisze:
>>
>>>>>  	if (commit_lock_file(&msg_file) < 0)
>>>>>  		return error(_("Error wrapping up %s."), filename);
>>>>
>>>> Another "while at it"... though the one that can be safely postponed
>>>> (well, the make message easier to understand part, not the quote
>>>> filename part):
>>>>
>>>>   		return error(_("Error wrapping up writing to '%s'."), filename);
>>>
>>> As I inherited this message, I'll keep it.
>>
>> Well, please then add quotes while at it, at least, for consistency
>>
>>   		return error(_("Error wrapping up '%s'."), filename);
> 
> I may do that as a final patch, once all the other concerns are addressed.
> I really do not want to change the error message during the conversion.

> W dniu 09.09.2016 o 16:40, Johannes Schindelin napisał:
> > On Fri, 2 Sep 2016, Jakub Narębski wrote:
> >> W dniu 01.09.2016 o 16:20, Johannes Schindelin pisze:
> >>> On Thu, 1 Sep 2016, Jakub Narębski wrote: 
> >>>> W dniu 29.08.2016 o 10:06, Johannes Schindelin pisze:
> >>
> >>>>>  	if (commit_lock_file(&msg_file) < 0)
> >>>>>  		return error(_("Error wrapping up %s."), filename);
> >>>>
> >>>> Another "while at it"... though the one that can be safely postponed
> >>>> (well, the make message easier to understand part, not the quote
> >>>> filename part):
> >>>>
> >>>>   		return error(_("Error wrapping up writing to '%s'."), filename);
> >>>
> >>> As I inherited this message, I'll keep it.
> >>
> >> Well, please then add quotes while at it, at least, for consistency
> >>
> >>   		return error(_("Error wrapping up '%s'."), filename);
> > 
> > I may do that as a final patch, once all the other concerns are addressed.
> > I really do not want to change the error message during the conversion.
> 
> Is not wanting to change error messages during conversion because of
> your use of Scientist tool to catch errors in conversion process?

> 
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  sequencer.c | 3 +++
>  1 file changed, 3 insertions(+)

> 
> diff --git a/sequencer.c b/sequencer.c
> index 0614b90..5efed2e 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -864,6 +864,9 @@ static int parse_insn_line(struct todo_item *item, const char *bol, char *eol)
>  	char *end_of_object_name;
>  	int i, saved, status, padding;
>  
> +	/* left-trim */
> +	bol += strspn(bol, " \t");
> +

> W dniu 29.08.2016 o 10:06, Johannes Schindelin pisze:
> 
> Subject: [PATCH 21/22] sequencer: left-trim the lines read from the script
> 
> In the subject, it should probably be without "the", as "lines"
> are plural.
> 
> s/left-trim the lines/left-trim lines/

> > Interactive rebase's scripts may be indented; We need to handle this
> > case, too, now that we prepare the sequencer to process interactive
> > rebases.
> 
> s/; We need/; we need/

>> > Interactive rebase's scripts may be indented; We need to handle this
>> > case, too, now that we prepare the sequencer to process interactive
>> > rebases.
>> 
>> s/; We need/; we need/
>
> Hrmpf. From http://grammar.ccc.commnet.edu/grammar/marks/colon.htm:
>
> 	There is some disagreement among writing reference manuals about
> 	when you should capitalize an independent clause following a
> 	colon. Most of the manuals advise that when you have more than one
> 	sentence in your explanation or when your sentence(s) is a formal
> 	quotation, a capital is a good idea. The NYPL Writer's Guide urges
> 	consistency within a document; the Chicago Manual of Style says
> 	you may begin an independent clause with a lowercase letter unless it's
> 	one of those two things (a quotation or more than one sentence).
> 	The APA Publication Manual is the most extreme: it advises us to
> 	always capitalize an independent clause following a colon. The advice
> 	given above is consistent with the Gregg Reference Manual.
>
> Based on that, I think that a capital is the correct case here.

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> >> > Interactive rebase's scripts may be indented; We need to handle this
> >> > case, too, now that we prepare the sequencer to process interactive
> >> > rebases.
> >> 
> >> s/; We need/; we need/
> >
> > Hrmpf. From http://grammar.ccc.commnet.edu/grammar/marks/colon.htm:
> >
> > 	There is some disagreement among writing reference manuals about
> > 	when you should capitalize an independent clause following a
> > 	colon. Most of the manuals advise that when you have more than one
> > 	sentence in your explanation or when your sentence(s) is a formal
> > 	quotation, a capital is a good idea. The NYPL Writer's Guide urges
> > 	consistency within a document; the Chicago Manual of Style says
> > 	you may begin an independent clause with a lowercase letter unless it's
> > 	one of those two things (a quotation or more than one sentence).
> > 	The APA Publication Manual is the most extreme: it advises us to
> > 	always capitalize an independent clause following a colon. The advice
> > 	given above is consistent with the Gregg Reference Manual.
> >
> > Based on that, I think that a capital is the correct case here.
> 
> Does that manual have anything to say about semicolons, which is a
> different thing?

> Hello Johannes,
> 
> W dniu 01.09.2016 o 16:13, Johannes Schindelin pisze: 
> > On Thu, 1 Sep 2016, Jakub Narębski wrote:
>  
> >> 'bol' is beginning-of-line, isn't it (a complement to eol)?
> > 
> > Yep. How did you guess? :-)
> 
> Wouldn't 'beg' and 'end' instead of 'bol' and 'eol' be easier
> to understand, thus more readable?

> 
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  builtin/revert.c | 36 ++++++++++++++++--------------------
>  sequencer.c      | 35 +++++++++++------------------------
>  sequencer.h      | 13 ++++---------
>  3 files changed, 31 insertions(+), 53 deletions(-)

> W dniu 29.08.2016 o 10:05, Johannes Schindelin pisze:
> 
> > While at it, ensure that the subcommands return an error code so that
> > they do not have to die() all over the place (bad practice for library
> > functions...).
> 
> This perhaps should be moved to a separate patch, but I guess
> there is a reason behind "while at it".

> On ma, 2016-08-29 at 10:03 +0200, Johannes Schindelin wrote:
> 
> > Therefore I would be most grateful for every in-depth review.
> 
> Tried to do that, but could come up only with a few nits. I think the
> approach is sensible.

> 
> The patches in this series merely prepare the sequencer code for the
> next patch series that actually teaches the sequencer to run an
> interactive rebase.
> 
> The reason to split these two patch series is simple: to keep them at a
> sensible size.

> 
> The two patch series after that are much smaller: a two-patch "series"
> that switches rebase -i to use the sequencer (except with --root or
> --preserve-merges), and a couple of patches to move several pretty
> expensive script processing steps to C (think: autosquash).

>   sequencer: support amending commits
>   sequencer: support cleaning up commit messages
>   sequencer: remember do_recursive_merge()'s return value
>   sequencer: left-trim the lines read from the script
>   sequencer: refactor write_message()

> 
>  builtin/commit.c                |   2 +-
>  builtin/revert.c                |  42 ++-
>  sequencer.c                     | 573 +++++++++++++++++++++++++++-------------
>  sequencer.h                     |  27 +-
>  t/t3510-cherry-pick-sequence.sh |  11 -
>  5 files changed, 428 insertions(+), 227 deletions(-)
> 
> Based-On: libify-sequencer at https://github.com/dscho/git
> Fetch-Base-Via: git fetch https://github.com/dscho/git libify-sequencer
> Published-As: https://github.com/dscho/git/releases/tag/prepare-sequencer-v1
> Fetch-It-Via: git fetch https://github.com/dscho/git prepare-sequencer-v1

> W dniu 29.08.2016 o 10:03, Johannes Schindelin pisze:
> 
> > This patch series marks the  '4' in the countdown to speed up rebase -i
> > by implementing large parts in C. It is based on the `libify-sequencer`
> > patch series that I submitted last week.
> 
> Which of those got reviewed (and perhaps accepted), and which of those
> needs review still?  What is subject of their cover letter?

> > The two patch series after that are much smaller: a two-patch "series"
> > that switches rebase -i to use the sequencer (except with --root or
> > --preserve-merges), and a couple of patches to move several pretty
> > expensive script processing steps to C (think: autosquash).
> 
> I can understand --preserve-merges, but what is the problem with --root?

> > The end game of this patch series is a git-rebase--helper that makes
> > rebase -i 5x faster on Windows (according to t/perf/p3404). Travis
> > says that even MacOSX and Linux benefit (4x and 3x, respectively).
> 
> So do I understand correctly that end goal for *this* series is to move
> most of processing to git-rebase--helper, but full builtin-ification
> (and retiring git-rebase.sh to contrib/examples/) would have to wait for
> later?

> [...]
> 
> I'd like here to summarize the discussion (my review, Dennis review,
> Johannes Sixt and Junio comments).
> 
> If there are no comments, it means no problems or minor changes.

> > Johannes Schindelin (22):
> >   sequencer: use static initializers for replay_opts
> There is no need for putting zeros in static initializer.  Commit
> message expanded.
> 
> >   sequencer: use memoized sequencer directory path
> >   sequencer: avoid unnecessary indirection
> >   sequencer: future-proof remove_sequencer_state()
> Leftover unrelated chunk removed.
> 
> >   sequencer: allow the sequencer to take custody of malloc()ed data
> Is introducing new *_entrust() mechanism (which needs docs, at least
> as comments) worth it, instead of just strdup everything and free?
> If it is: naming of function parameter + example in commit message.
> 
> >   sequencer: release memory that was allocated when reading options
> See above.
> 
> >   sequencer: future-proof read_populate_todo()
> Possibly mention which functions were not future-proofed because
> of planned for the subsequent patch full rewrite.

> >   sequencer: avoid completely different messages for different actions
> Fix l10n or drop (and not introduce lego translation).
> 
> >   sequencer: get rid of the subcommand field
> >   sequencer: refactor the code to obtain a short commit name
> Explain reason behind this change in the commit mesage.
> 
> >   sequencer: remember the onelines when parsing the todo file
> Lazy or eager again; "exec", "noop" and --preserve-merges.
> 
> >   sequencer: prepare for rebase -i's commit functionality
> Add helper function, possibly extract helper function.  Rephrase block
> comment.
> 
> "[PATCH] am: refactor read_author_script()" from Junio.
> 
> >   sequencer: introduce a helper to read files written by scripts
> Perhaps add why not use open + strbuf_getline to commit message...
> 
> >   sequencer: prepare for rebase -i's GPG settings
> Possibly fixes bug.  Use *_entrust() or strdup to not leak memory
> (and to not crash when freeing memory).
> 
> >   sequencer: allow editing the commit message on a case-by-case basis
> Enhance the commit message.
> 
> >   sequencer: support amending commits
> >   sequencer: support cleaning up commit messages
> >   sequencer: remember do_recursive_merge()'s return value
> >   sequencer: left-trim the lines read from the script
> >   sequencer: refactor write_message()
> Enhance the commit message.  Quote path in messages while at it.

> > Based-On: libify-sequencer at https://github.com/dscho/git
> > Fetch-Base-Via: git fetch https://github.com/dscho/git libify-sequencer
> > Published-As: https://github.com/dscho/git/releases/tag/prepare-sequencer-v1
> > Fetch-It-Via: git fetch https://github.com/dscho/git prepare-sequencer-v1
> 
> An unrelated question: Dscho, how are you generating above lines?

> This change is not completely faithful: instead of initializing all fields
> to 0, we choose to initialize command and subcommand to -1 (instead of
> defaulting to REPLAY_REVERT and REPLAY_NONE, respectively). Practically,
> it makes no difference at all, but future-proofs the code to require
> explicit assignments for both fields.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---

> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  builtin/commit.c |  2 +-
>  sequencer.c      | 11 ++++++-----
>  sequencer.h      |  5 +----
>  3 files changed, 8 insertions(+), 10 deletions(-)

> -#define SEQ_DIR		"sequencer"
> -#define SEQ_HEAD_FILE	"sequencer/head"
> -#define SEQ_TODO_FILE	"sequencer/todo"
> -#define SEQ_OPTS_FILE	"sequencer/opts"
> +const char *git_path_seq_dir(void);

> -static int read_populate_opts(struct replay_opts **opts)
> +static int read_populate_opts(struct replay_opts *opts)
>  {
>  	if (!file_exists(git_path_opts_file()))
>  		return 0;
> @@ -823,7 +823,7 @@ static int read_populate_opts(struct replay_opts **opts)
>  	 * about this case, though, because we wrote that file ourselves, so we
>  	 * are pretty certain that it is syntactically correct.
>  	 */
> -	if (git_config_from_file(populate_opts_cb, git_path_opts_file(), *opts) < 0)
> +	if (git_config_from_file(populate_opts_cb, git_path_opts_file(), opts) < 0)
>  		return error(_("Malformed options sheet: %s"),
>  			git_path_opts_file());
>  	return 0;
> @@ -1054,7 +1054,7 @@ static int sequencer_continue(struct replay_opts *opts)
>  
>  	if (!file_exists(git_path_todo_file()))
>  		return continue_single_pick();
> -	if (read_populate_opts(&opts) ||
> +	if (read_populate_opts(opts) ||
>  			read_populate_todo(&todo_list, opts))
>  		return -1;

> The sequencer is our attempt to lib-ify cherry-pick. Yet it behaves
> like a one-shot command when it reads its configuration: memory is
> allocated and released only when the command exits.
>
> This is kind of okay for git-cherry-pick, which *is* a one-shot
> command. All the work to make the sequencer its work horse was
> done to allow using the functionality as a library function, though,
> including proper clean-up after use.
>
> This patch introduces an API to pass the responsibility of releasing
> certain memory to the sequencer. Example:
>
> 	const char *label =
> 		sequencer_entrust(opts, xstrfmt("From: %s", email));

> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> 
> > The sequencer is our attempt to lib-ify cherry-pick. Yet it behaves
> > like a one-shot command when it reads its configuration: memory is
> > allocated and released only when the command exits.
> >
> > This is kind of okay for git-cherry-pick, which *is* a one-shot
> > command. All the work to make the sequencer its work horse was
> > done to allow using the functionality as a library function, though,
> > including proper clean-up after use.
> >
> > This patch introduces an API to pass the responsibility of releasing
> > certain memory to the sequencer. Example:
> >
> > 	const char *label =
> > 		sequencer_entrust(opts, xstrfmt("From: %s", email));
> 
> I thought we (not just me) were already pretty clear during the last
> round of review that we will not want this entrust() thing.

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
>> If you prefer to accept such sloppy work, I will change it of course,
>> feeling dirty that it has my name on it.
> 
> I do want neither leaks nor sloppyness, and I thought that was
> understood by everybody, hence I thought the last round made it
> clear that _entrust() must not exist.

> 
> Let's try again.
> 
> We manage lifetime of a field in a structure in one of three ways in
> our codebase [*1*].
> 
>  * A field can always point at a borrowed piece of memory that the
>    destruction of the containing structure or re-assignment to the
>    field do not have to free the current value of the field.  This
>    is a minority, simply because it is rare for a field to be always
>    able to borrow from others.  The destruction of the containing
>    structure or re-assignment to the field needs to do nothing
>    special.
> 
>  * A field can own the piece of memory that it points at.  The
>    destruction of the containing structure or re-assignment to the
>    field needs to free the current value of the field [*2*].  A
>    field that can be assigned a value from the configuration (which
>    is typically xstrdup()'ed) is an example of such a field, and if
>    a command line option also can assign to the field, we'd need to
>    xstrdup() it, even though we know we could borrow from argv[],
>    because cleaning-up needs to be able to free(3) it.
> 
>  * A field can sometimes own and sometimes borrow the memory, and it
>    is accompanied by another field to tell which case it is, so that
>    cleaning-up can tell when it needs to be free(3)d.  This is a
>    minority case, and we generally avoid it especially in modern
>    code for small allocation, as it makes the lifetime rule more
>    complex than it is worth.

> The _entrust() thing may have been an excellent fourth option if it
> were cost-free.  xstrdup() that the second approach necessitates for
> literal strings (like part of argv[]) would become unnecessary and
> we can treat as if all the fields in a structure were all borrowing
> the memory from elsewhere (in fact, _entrust() creates the missing
> owners of pieces of memory that need to be freed later); essentially
> it turns a "mixed ownership" case into the first approach.
> 
> But it is not cost-free.  The mechanism needs to allocate memory to
> become the missing owner and keep track of which pieces of memory
> need to be freed later, and the resulting code does not become any
> easier to follow.  The programmer still needs to know which ones to
> _entrust() just like the programmer in the model #2 needs to make
> sure xstrdup() is done for appropriate pieces of memory--the only
> difference is that an _entrust() programmer needs to mark the pieces
> of memory to be freed, while a #2 programmer needs to xstrdup() the
> pieces of memory that are being "borrowed".
> 
> So let's not add the fourth way to our codebase.  "mixed ownership"
> case should be turned into "field owns the memory", i.e. approach #2.

> 
> [Footnotes]
> 
> *1* It is normal for different fields in the same structure follow
>     different lifetime rules.
> 
> *2* Unless leaks are allowed, that is.  I think we have instances
>     where "git cmd --opt=A --opt=B" allocates and stores a new piece
>     of memory that is computed based on A and store it to a field,
>     and then overwrites the field with another allocation of a value
>     based on B without freeing old value, saying "the caller can
>     avoid passing the same thing twice, and such a leak is miniscule
>     anyway".  That is not nice, and we've been reducing them over
>     time.
> 

>> We manage lifetime of a field in a structure in one of three ways in
>> our codebase [*1*].
>> 
>>  ...
>>  * A field can sometimes own and sometimes borrow the memory, and it
>>    is accompanied by another field to tell which case it is, so that
>>    cleaning-up can tell when it needs to be free(3)d.  This is a
>>    minority case, and we generally avoid it especially in modern
>>    code for small allocation, as it makes the lifetime rule more
>>    complex than it is worth.
> ...
> On the other hand the _entrust() mechanism might be a good solution
> if the amount of memory was large, for example order of magnitude more
> than what would be needed to keep ownership info *and* borrowing would
> not be possible for some reason.

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > If you prefer to accept such sloppy work, I will change it of course,
> > feeling dirty that it has my name on it.
> 
> I do want neither leaks nor sloppyness, and I thought that was
> understood by everybody, hence I thought the last round made it
> clear that _entrust() must not exist.

> -static void remove_sequencer_state(void)
> +static void remove_sequencer_state(const struct replay_opts *opts)
>  {
> -	struct strbuf seq_dir = STRBUF_INIT;
> +	struct strbuf dir = STRBUF_INIT;
>  
> -	strbuf_addstr(&seq_dir, git_path_seq_dir());
> -	remove_dir_recursively(&seq_dir, 0);
> -	strbuf_release(&seq_dir);
> +	strbuf_addf(&dir, "%s", get_dir(opts));
> +	remove_dir_recursively(&dir, 0);
> +	strbuf_release(&dir);
>  }

> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> 
> > +static const char *get_dir(const struct replay_opts *opts)
> > +{
> > +	return git_path_seq_dir();
> > +}
> 
> Presumably this is what "In a couple of commits" meant, i.e. opts
> will be used soonish.