Re: [PATCH v2 03/34] sequencer (rebase -i): implement the 'edit' command

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

Hi Junio,

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

> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> 
> > @@ -43,6 +44,20 @@ static GIT_PATH_FUNC(rebase_path_todo, "rebase-merge/git-rebase-todo")
> >   */
> >  static GIT_PATH_FUNC(rebase_path_author_script, "rebase-merge/author-script")
> >  /*
> 
> It is minor, but please have a blank line to separate the logical
> blocks.

Ah, but where to draw the line? These comments in front of some of the
rebase_path_* functions clarify the particular role of the corresponding
paths, but all of those functions belong to the same block of
rebase_path_* functions. That latter circumstance is what made me decide
to *not* insert blank lines here, but only a blank line before that block
(to separate from the sequencer path functions) and after that block (to
separate from the rest of the code).

> If you have "comment for thing A" before "thing A", then having a blank
> after that before "comment for thing B" and "thing B" that follow would
> help.  Otherwise "thing A" immediately followed by "comment for thing B"
> are (mis)read together, leading to nonsense.

In this case, I think it is quite obvious that the comments belong to the
immediately following line, and that all of the path functions are part of
a bigger block.

> > + * When an "edit" rebase command is being processed, the SHA1 of the
> > + * commit to be edited is recorded in this file.  When "git rebase
> > + * --continue" is executed, if there are any staged changes then they
> > + * will be amended to the HEAD commit, but only provided the HEAD
> > + * commit is still the commit to be edited.  When any other rebase
> > + * command is processed, this file is deleted.
> > + */
> > +static GIT_PATH_FUNC(rebase_path_amend, "rebase-merge/amend")
> > +/*
> > + * When we stop at a given patch via the "edit" command, this file contains
> > + * the long commit name of the corresponding patch.
> 
> If you abbreviate an object name to 38-hex that is still long but
> that is not full; if you meant full 40-hex, better spell it out as
> "full"---that conveys useful information to programmers (e.g. they
> can just use get_sha1_hex()).
> 
> But I think you are writing short_commit_name() to it?  So perhaps
> "an abbreviated commit object name"?

Fixed.

> > @@ -1301,9 +1318,87 @@ static int save_opts(struct replay_opts *opts)
> >  	return res;
> >  }
> >  
> > +static int make_patch(struct commit *commit, struct replay_opts *opts)
> > +{
> > +	struct strbuf buf = STRBUF_INIT;
> > +	struct rev_info log_tree_opt;
> > +	const char *commit_buffer = get_commit_buffer(commit, NULL), *subject, *p;
> > +	int res = 0;
> > +
> > +	p = short_commit_name(commit);
> > +	if (write_message(p, strlen(p), rebase_path_stopped_sha(), 1) < 0)
> > +		return -1;
> > +
> > +	strbuf_addf(&buf, "%s/patch", get_dir(opts));
> > +	memset(&log_tree_opt, 0, sizeof(log_tree_opt));
> > +	init_revisions(&log_tree_opt, NULL);
> > +	log_tree_opt.abbrev = 0;
> > +	log_tree_opt.diff = 1;
> > +	log_tree_opt.diffopt.output_format = DIFF_FORMAT_PATCH;
> > +	log_tree_opt.disable_stdin = 1;
> > +	log_tree_opt.no_commit_id = 1;
> > +	log_tree_opt.diffopt.file = fopen(buf.buf, "w");
> > +	log_tree_opt.diffopt.use_color = GIT_COLOR_NEVER;
> > +	if (!log_tree_opt.diffopt.file)
> > +		res |= error_errno(_("could not open '%s'"), buf.buf);
> > +	else {
> > +		res |= log_tree_commit(&log_tree_opt, commit);
> > +		fclose(log_tree_opt.diffopt.file);
> > +	}
> > +	strbuf_reset(&buf);
> > +	strbuf_addf(&buf, "%s/message", get_dir(opts));
> > +	if (!file_exists(buf.buf)) {
> > +		find_commit_subject(commit_buffer, &subject);
> > +		res |= write_message(subject, strlen(subject), buf.buf, 1);
> > +		unuse_commit_buffer(commit, commit_buffer);
> > +	}
> > +	strbuf_release(&buf);
> > +
> > +	return res;
> > +}
> 
> OK.  This seems to match what scripted make_patch does in a handful
> of lines.  We probably should have given you a helper to reduce
> boilerplate that sets up log_tree_opt so that this function does not
> have to be this long, but that is a separate topic.
> 
> Does it matter output_format is set to FORMAT_PATCH here, though?
> With --no-commit-id set, I suspect there is no log message or
> authorship information given to the output.
> 
> As you are only interested in seeing the patch/diff in this file and
> the log is stored in a separate "message" file, as long as "patch"
> file gets the patch correctly, it is not a problem, but it just
> looked strange to see FORMAT_PATCH there.

I am indifferent as to FORMAT_PATCH. It is there simply as a direct
translation of the `diff-tree -p` command in
https://github.com/git/git/blob/v2.11.0/git-rebase--interactive.sh#L188

Ciao,
Dscho