Johannes Schindelin <johannes.schindelin@xxxxxx> writes:
> This option is intended to help with the transition away from the
> now-deprecated graft file.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@xxxxxx>
> ---
> Documentation/git-replace.txt | 11 +++++--
> builtin/replace.c | 59 ++++++++++++++++++++++++++++++++++-
> 2 files changed, 66 insertions(+), 4 deletions(-)
I expected you to remove convert-grafts-to-replace-refs.sh from the
contrib/ section in the same patch, actually. FWIW, I think it is a
much better approach to give the first-class UI for transition like
this patch does than "go fish for a good way to transition yourself,
we may have something useful in contrib/", which is what we had so
far.
> diff --git a/Documentation/git-replace.txt b/Documentation/git-replace.txt
> ...
> @@ -87,9 +88,13 @@ OPTIONS
> content as <commit> except that its parents will be
> [<parent>...] instead of <commit>'s parents. A replacement ref
> is then created to replace <commit> with the newly created
> - commit. See contrib/convert-grafts-to-replace-refs.sh for an
> - example script based on this option that can convert grafts to
> - replace refs.
> + commit. Use `--convert-graft-file` to convert a
> + `$GIT_DIR/info/grafts` file use replace refs instead.
> +
Nice.
> diff --git a/builtin/replace.c b/builtin/replace.c
> index 43264f0998e..4cdc00a96df 100644
> --- a/builtin/replace.c
> +++ b/builtin/replace.c
> @@ -20,6 +20,7 @@ static const char * const git_replace_usage[] = {
> N_("git replace [-f] <object> <replacement>"),
> N_("git replace [-f] --edit <object>"),
> N_("git replace [-f] --graft <commit> [<parent>...]"),
> + N_("git replace [-f] --convert-graft-file"),
> N_("git replace -d <object>..."),
> N_("git replace [--format=<format>] [-l [<pattern>]]"),
> NULL
> @@ -423,6 +424,53 @@ static int create_graft(int argc, const char **argv, int force)
> return replace_object_oid(old_ref, &old_oid, "replacement", &new_oid, force);
> }
>
> +static int convert_graft_file(int force)
> +{
> + const char *graft_file = get_graft_file();
> + FILE *fp = fopen_or_warn(graft_file, "r");
> + struct strbuf buf = STRBUF_INIT, err = STRBUF_INIT;
> + struct argv_array args = ARGV_ARRAY_INIT;
> +
> + if (!fp)
> + return -1;
Returning silently is fine as fopen_or_warn() would have said
something already. Good.
> + while (strbuf_getline(&buf, fp) != EOF) {
> + int i = 0, j;
> +
> + while (i != buf.len) {
> + char save;
> +
> + for (j = i; j < buf.len && !isspace(buf.buf[j]); j++)
> + ; /* look further */
> + save = buf.buf[j];
> + buf.buf[j] = '\0';
> + argv_array_push(&args, buf.buf + i);
> + buf.buf[j] = save;
It's a shame that we do not have a helper that splits the contents
of a strbuf at SP and shove the result into an argv_array(). [*1*]
*1* There is one that splits into an array of strbuf but the point
of splitting is often that these split pieces are the final thing we
want, and placing them in separate strbuf (whose strength is that
contents are easily manipulatable) is pointless.
> +
> + while (j < buf.len && isspace(buf.buf[j]))
> + j++;
> + i = j;
One difference I notice while comparing this with what is done by
contrib/convert-grafts-to-replace-refs.sh is that this does not
skip a line that begins with # or SP. I offhand do not know what
the point of skipping a line that begins with a SP, but I suspect
that skipping a line that begins with "#" is a desirable thing to
do, because commit.c::read_graft_line() does know that such a line
is a valid comment.
> + }
> +
> + if (create_graft(args.argc, args.argv, force))
> + strbuf_addf(&err, "\n\t%s", buf.buf);
> +
> + argv_array_clear(&args);
> + strbuf_reset(&buf);
Strictly speaking, this reset is redundant, as getline() will always
stuff the line into a fresh buffer (and after the loop there
correctly is a release).
> + }
> +
> + strbuf_release(&buf);
> + argv_array_clear(&args);
> +
> + if (!err.len)
> + return unlink_or_warn(graft_file);
> + warning(_("could not convert the following graft(s):\n%s"), err.buf);
> + strbuf_release(&err);
commit.c::read_graft_file() seems to ignore a broken graft line and
salvages other lines, and this one follows suit, which is good.
The remaining die() I pointed out in 1/2 can safely be turned into
return error() for this caller (I didn't check for existing callers,
though) and would automatically do the right thing. The real
consumer of the graft file, commit.c::read_graft_line(), shows an
error when oid cannot be parsed, and the above code, when
create_graft() is updated to return an error instead of dying, would
append the problematic record to buf.buf in the code above.
Looking basically-good modulo minor nits.
