git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Johannes Schindelin <johannes.schindelin@gmx.de>
Cc: git@vger.kernel.org, Jakub Narebski <jnareb@gmail.com>,
	Stefan Beller <sbeller@google.com>,
	Eric Sunshine <sunshine@sunshineco.com>
Subject: Re: [PATCH v2 2/7] replace: introduce --convert-graft-file
Date: Fri, 20 Apr 2018 10:05:44 +0900	[thread overview]
Message-ID: <xmqqd0yud6vr.fsf@gitster-ct.c.googlers.com> (raw)
In-Reply-To: <cecb82973f80fdd2db940410db2a7c3b23107c4b.1524125760.git.johannes.schindelin@gmx.de> (Johannes Schindelin's message of "Thu, 19 Apr 2018 10:17:50 +0200 (DST)")

Johannes Schindelin <johannes.schindelin@gmx.de> writes:

> This option is intended to help with the transition away from the
> now-deprecated graft file.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  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.

  parent reply	other threads:[~2018-04-20  1:05 UTC|newest]

Thread overview: 115+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-04-13 11:11 [PATCH] Deprecate support for .git/info/grafts Johannes Schindelin
2018-04-13 18:22 ` Stefan Beller
2018-04-13 22:35   ` Johannes Schindelin
2018-04-13 22:45     ` Stefan Beller
2018-04-19  8:15       ` Johannes Schindelin
2018-04-13 18:57 ` Eric Sunshine
2018-04-19  8:14   ` Johannes Schindelin
2018-04-19  8:17 ` [PATCH v2 0/7] Deprecate .git/info/grafts Johannes Schindelin
2018-04-19  8:17   ` [PATCH v2 1/7] replace: "libify" create_graft() Johannes Schindelin
2018-04-20  0:25     ` Junio C Hamano
2018-04-20 11:51       ` Jakub Narebski
2018-04-20 15:32       ` Johannes Schindelin
2018-04-19  8:17   ` [PATCH v2 2/7] replace: introduce --convert-graft-file Johannes Schindelin
2018-04-19 10:09     ` Christian Couder
2018-04-20 13:57       ` Johannes Schindelin
2018-04-20  1:05     ` Junio C Hamano [this message]
2018-04-20 15:26       ` Johannes Schindelin
2018-04-19  8:18   ` [PATCH v2 3/7] Add a test for `git replace --convert-graft-file` Johannes Schindelin
2018-04-20  1:21     ` Junio C Hamano
2018-04-20 15:31       ` Johannes Schindelin
2018-04-19  8:18   ` [PATCH v2 4/7] Deprecate support for .git/info/grafts Johannes Schindelin
2018-04-19  8:18   ` [PATCH v2 5/7] filter-branch: stop suggesting to use grafts Johannes Schindelin
2018-04-19  8:19   ` [PATCH v2 6/7] technical/shallow: describe the relationship with replace refs Johannes Schindelin
2018-04-19  8:21   ` [PATCH v2 7/7] technical/shallow: describe why shallow cannot use " Johannes Schindelin
2018-04-20 22:20   ` [PATCH v3 00/11] Deprecate .git/info/grafts Johannes Schindelin
2018-04-20 22:20     ` [PATCH v3 01/11] argv_array: offer to split a string by whitespace Johannes Schindelin
2018-04-20 23:29       ` Stefan Beller
2018-04-21  9:39         ` Johannes Schindelin
2018-04-20 22:21     ` [PATCH v3 02/11] commit: Let the callback of for_each_mergetag return on error Johannes Schindelin
2018-04-20 22:21     ` [PATCH v3 03/11] replace: avoid using die() to indicate a bug Johannes Schindelin
2018-04-20 22:21     ` [PATCH v3 04/11] replace: "libify" create_graft() and callees Johannes Schindelin
2018-04-20 22:22     ` [PATCH v3 05/11] replace: introduce --convert-graft-file Johannes Schindelin
2018-04-20 22:23     ` [PATCH v3 06/11] Add a test for `git replace --convert-graft-file` Johannes Schindelin
2018-04-21  6:20       ` SZEDER Gábor
2018-04-21  9:42         ` Johannes Schindelin
2018-04-20 22:24     ` [PATCH v3 07/11] Deprecate support for .git/info/grafts Johannes Schindelin
2018-04-20 22:25     ` [PATCH v3 08/11] filter-branch: stop suggesting to use grafts Johannes Schindelin
2018-04-20 22:26     ` [PATCH v3 09/11] technical/shallow: describe the relationship with replace refs Johannes Schindelin
2018-04-22 15:16       ` Philip Oakley
2018-04-24 19:10         ` Johannes Schindelin
2018-04-24 21:34           ` Philip Oakley
2018-04-25  0:40             ` Junio C Hamano
2018-04-25  7:17               ` Johannes Schindelin
2018-04-20 22:26     ` [PATCH v3 10/11] technical/shallow: describe why shallow cannot use " Johannes Schindelin
2018-04-20 22:27     ` [PATCH v3 11/11] Remove obsolete script to convert grafts to " Johannes Schindelin
2018-04-21  9:43     ` [PATCH v4 00/11] Deprecate .git/info/grafts Johannes Schindelin
2018-04-21  9:46       ` [PATCH v4 01/11] argv_array: offer to split a string by whitespace Johannes Schindelin
2018-04-24  1:15         ` Junio C Hamano
2018-04-24  2:38           ` Junio C Hamano
2018-04-21  9:46       ` [PATCH v4 02/11] commit: Let the callback of for_each_mergetag return on error Johannes Schindelin
2018-04-21  9:47       ` [PATCH v4 03/11] replace: avoid using die() to indicate a bug Johannes Schindelin
2018-04-21  9:47       ` [PATCH v4 04/11] replace: "libify" create_graft() and callees Johannes Schindelin
2018-04-23 19:08         ` Stefan Beller
2018-04-24 18:51           ` Johannes Schindelin
2018-04-24 19:03             ` Stefan Beller
2018-04-25  7:10               ` Johannes Schindelin
2018-04-21  9:48       ` [PATCH v4 05/11] replace: introduce --convert-graft-file Johannes Schindelin
2018-04-22  3:30         ` Eric Sunshine
2018-04-24 19:04           ` Johannes Schindelin
2018-04-21  9:48       ` [PATCH v4 06/11] Add a test for `git replace --convert-graft-file` Johannes Schindelin
2018-04-21  9:49       ` [PATCH v4 07/11] Deprecate support for .git/info/grafts Johannes Schindelin
2018-04-21  9:49       ` [PATCH v4 08/11] filter-branch: stop suggesting to use grafts Johannes Schindelin
2018-04-21  9:51       ` [PATCH v4 09/11] technical/shallow: describe the relationship with replace refs Johannes Schindelin
2018-04-21  9:56       ` [PATCH v4 10/11] technical/shallow: describe why shallow cannot use " Johannes Schindelin
2018-04-21  9:56       ` [PATCH v4 11/11] Remove obsolete script to convert grafts to " Johannes Schindelin
2018-04-23 19:24       ` [PATCH v4 00/11] Deprecate .git/info/grafts Stefan Beller
2018-04-25  9:53       ` [PATCH v5 " Johannes Schindelin
2018-04-25  9:53         ` [PATCH v5 01/11] argv_array: offer to split a string by whitespace Johannes Schindelin
2018-04-25  9:54         ` [PATCH v5 02/11] commit: Let the callback of for_each_mergetag return on error Johannes Schindelin
2018-04-25  9:54         ` [PATCH v5 03/11] replace: avoid using die() to indicate a bug Johannes Schindelin
2018-04-25  9:54         ` [PATCH v5 04/11] replace: "libify" create_graft() and callees Johannes Schindelin
2018-04-25  9:54         ` [PATCH v5 05/11] replace: introduce --convert-graft-file Johannes Schindelin
2018-04-25  9:54         ` [PATCH v5 06/11] Add a test for `git replace --convert-graft-file` Johannes Schindelin
2018-04-25  9:54         ` [PATCH v5 07/11] Deprecate support for .git/info/grafts Johannes Schindelin
2018-04-25  9:54         ` [PATCH v5 08/11] filter-branch: stop suggesting to use grafts Johannes Schindelin
2018-04-25  9:54         ` [PATCH v5 09/11] technical/shallow: stop referring to grafts Johannes Schindelin
2018-04-25 12:25           ` Jakub Narębski
2018-04-26  9:19             ` Johannes Schindelin
2018-04-25  9:54         ` [PATCH v5 10/11] technical/shallow: describe why shallow cannot use replace refs Johannes Schindelin
2018-04-25  9:54         ` [PATCH v5 11/11] Remove obsolete script to convert grafts to " Johannes Schindelin
2018-04-26  4:10         ` [PATCH v5 00/11] Deprecate .git/info/grafts Junio C Hamano
2018-04-27 21:03           ` Johannes Schindelin
2018-04-27 21:39         ` [PATCH v6 " Johannes Schindelin
2018-04-27 21:39           ` [PATCH v6 01/11] argv_array: offer to split a string by whitespace Johannes Schindelin
2018-04-27 21:39           ` [PATCH v6 02/11] commit: Let the callback of for_each_mergetag return on error Johannes Schindelin
2018-04-27 21:39           ` [PATCH v6 03/11] replace: avoid using die() to indicate a bug Johannes Schindelin
2018-04-27 21:39           ` [PATCH v6 04/11] replace: "libify" create_graft() and callees Johannes Schindelin
2018-04-27 21:39           ` [PATCH v6 05/11] replace: introduce --convert-graft-file Johannes Schindelin
2018-04-27 21:39           ` [PATCH v6 06/11] Add a test for `git replace --convert-graft-file` Johannes Schindelin
2018-04-28  1:25             ` SZEDER Gábor
2018-04-28 13:07               ` Johannes Schindelin
2018-04-27 21:39           ` [PATCH v6 07/11] Deprecate support for .git/info/grafts Johannes Schindelin
2018-04-27 21:39           ` [PATCH v6 08/11] filter-branch: stop suggesting to use grafts Johannes Schindelin
2018-04-27 21:40           ` [PATCH v6 09/11] technical/shallow: stop referring to grafts Johannes Schindelin
2018-04-27 21:40           ` [PATCH v6 10/11] technical/shallow: describe why shallow cannot use replace refs Johannes Schindelin
2018-04-27 21:40           ` [PATCH v6 11/11] Remove obsolete script to convert grafts to " Johannes Schindelin
2018-04-28  9:04             ` Philip Oakley
2018-04-28 13:13               ` Johannes Schindelin
2018-04-28 15:00               ` Stefan Beller
2018-04-28 22:47           ` [Re-send PATCH v7 00/12] Deprecate .git/info/grafts Johannes Schindelin
2018-04-28 22:44             ` [PATCH v7 01/12] argv_array: offer to split a string by whitespace Johannes Schindelin
2018-04-28 22:44             ` [PATCH v7 02/12] commit: Let the callback of for_each_mergetag return on error Johannes Schindelin
2018-04-28 22:44             ` [PATCH v7 03/12] replace: avoid using die() to indicate a bug Johannes Schindelin
2018-04-28 22:44             ` [PATCH v7 04/12] replace: "libify" create_graft() and callees Johannes Schindelin
2018-04-28 22:44             ` [PATCH v7 05/12] replace: prepare create_graft() for converting graft files wholesale Johannes Schindelin
2018-04-28 22:44             ` [PATCH v7 06/12] replace: introduce --convert-graft-file Johannes Schindelin
2018-04-28 22:44             ` [PATCH v7 07/12] Add a test for `git replace --convert-graft-file` Johannes Schindelin
2018-04-28 22:44             ` [PATCH v7 08/12] Deprecate support for .git/info/grafts Johannes Schindelin
2018-11-27 20:12               ` [PATCH] advice: don't pointlessly suggest --convert-graft-file Ævar Arnfjörð Bjarmason
2018-11-28  6:34                 ` Junio C Hamano
2018-11-28  9:03                 ` Johannes Schindelin
2018-04-28 22:44             ` [PATCH v7 09/12] filter-branch: stop suggesting to use grafts Johannes Schindelin
2018-04-28 22:45             ` [PATCH v7 10/12] technical/shallow: stop referring to grafts Johannes Schindelin
2018-04-28 22:45             ` [PATCH v7 11/12] technical/shallow: describe why shallow cannot use replace refs Johannes Schindelin
2018-04-28 22:45             ` [PATCH v7 12/12] Remove obsolete script to convert grafts to " Johannes Schindelin

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqqd0yud6vr.fsf@gitster-ct.c.googlers.com \
    --to=gitster@pobox.com \
    --cc=git@vger.kernel.org \
    --cc=jnareb@gmail.com \
    --cc=johannes.schindelin@gmx.de \
    --cc=sbeller@google.com \
    --cc=sunshine@sunshineco.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).