Re: [PATCH v3 1/9] rebase -i: generate the script via rebase--helper

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

Hi Junio,

On Wed, 26 Apr 2017, Junio C Hamano wrote:

> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> 
> > diff --git a/sequencer.c b/sequencer.c
> > index 77afecaebf0..e858a976279 100644
> > --- a/sequencer.c
> > +++ b/sequencer.c
> > @@ -2388,3 +2388,48 @@ void append_signoff(struct strbuf *msgbuf, int ignore_footer, unsigned flag)
> >  
> >  	strbuf_release(&sob);
> >  }
> > +
> > +int sequencer_make_script(int keep_empty, FILE *out,
> > +		int argc, const char **argv)
> > +{
> > +	char *format = xstrdup("%s");
> > +	struct pretty_print_context pp = {0};
> > +	struct strbuf buf = STRBUF_INIT;
> > +	struct rev_info revs;
> > +	struct commit *commit;
> > +
> > +	init_revisions(&revs, NULL);
> > +	revs.verbose_header = 1;
> > +	revs.max_parents = 1;
> > +	revs.cherry_pick = 1;
> > +	revs.limited = 1;
> > +	revs.reverse = 1;
> > +	revs.right_only = 1;
> > +	revs.sort_order = REV_SORT_IN_GRAPH_ORDER;
> > +	revs.topo_order = 1;
> > +
> > +	revs.pretty_given = 1;
> > +	git_config_get_string("rebase.instructionFormat", &format);
> > +	get_commit_format(format, &revs);
> > +	free(format);
> > +	pp.fmt = revs.commit_format;
> > +	pp.output_encoding = get_log_output_encoding();
> 
> All of the above feels like inviting unnecessary future breakages by
> knowing too much about the implementation the current version of
> revision.c happens to use.

You mean that the `--reverse` option gets translated into the `reverse`
bit, and the other settings?

:-)

> A more careful implementation would be to allocate our own av[] and
> prepare "--reverse", "--left-right", "--cherry-pick", etc. to be parsed
> by setup_revisions() call we see below.

Oh, so you were not joking.

Part of why I think we should stay away from shell scripts has nothing to
do with performance (which would already be worth it) nor portability
issues (which also would already be worth it) nor requiring contributors to
know more than C (which also would already be worth it), but static
typing.

What you are asking is to do away with the strong, static typing (which
would show a breakage pretty quickly if that part of revision.c's API was
changed, therefore I think your concern is a little curious) in favor of
loose typing which would demonstrate breakages only upon use.

That is the exact opposite direction of where I want to go.

> The parsing is not an expensive part of the operation anyway,

... but why, oh why make things more complicated than they need to be? The
revision API is an API, yes, an internal one, but an API, for crying out
loud.

> and that way we do not have to worry about one less thing.

Not that I don't mind no double or triple negations, but no, not this one.

> > +	if (setup_revisions(argc, argv, &revs, NULL) > 1)
> > +		return error(_("make_script: unhandled options"));
> > +
> > +	if (prepare_revision_walk(&revs) < 0)
> > +		return error(_("make_script: error preparing revisions"));
> > +
> > +	while ((commit = get_revision(&revs))) {
> > +		strbuf_reset(&buf);
> > +		if (!keep_empty && is_original_commit_empty(commit))
> > +			strbuf_addf(&buf, "%c ", comment_line_char);
> 
> Presumably callers of this function (which does not exist yet at
> this step) are expected to have done the configuration dance to
> prepare comment_line_char to whatever the end-user specified?

Yes. Just like they have to take care of discovering the .git/ directory.

I guess I kind of fail to see your point. Of course the configuration has
to be read at this point... This is an internal API function that has the
same contract as all the other internal API functions: you have to set up
and configure everything needed to run the API function beforehand.

But maybe what you really wanted to ask is: How do we know that
comment_line_char is initialized correctly at this point?

If that is the question, I understand your puzzlement, and it is easy to
dispell: comment_line_char is configured as part of
git_default_core_config(), and initialized to '#' before Git even starts
to run.

So we're safe here, as long as the default config handling runs. The
intended user is obviously the rebase--helper, which runs git_config()
even before parsing the options.

Meaning: the code is safe.

Ciao,
Dscho