Hi Junio,
On Tue, 13 Dec 2016, Junio C Hamano wrote:
> Johannes Schindelin <johannes.schindelin@xxxxxx> 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