Deprecate support for .git/info/grafts

The grafts feature was a convenient way to "stich together" ancient history to the fresh start of linux.git.

Its implementation is, however, not up to Git's standards, as there are too many ways where it can lead to surprising and unwelcome behavior.

For example, when pushing from a repository with active grafts, it is possible to miss commits that have been "grafted out", resulting in a broken state on the other side.

Also, the grafts feature is limited to "rewriting" commits' list of parents, it cannot replace anything else.

The much younger feature implemented as git replace set out to remedy those limitations and dangerous bugs.

Seeing as git replace is pretty mature by now, it is time to deprecate support for the graft file, and to retire it eventually.

---
 advice.c                  | 2 ++
 advice.h                  | 1 +
 commit.c                  | 9 +++++++++
 t/t6001-rev-list-graft.sh | 9 +++++++++
 4 files changed, 21 insertions(+)

index 406efc183ba..4411704fd45 100644
@@ -19,6 +19,7 @@ int advice_rm_hints = 1;
 int advice_add_embedded_repo = 1;
 int advice_ignored_hook = 1;
 int advice_waiting_for_editor = 1;
+int advice_graft_file_deprecated = 1;
 
 static struct {
 	const char *name;

@@ -42,6 +43,7 @@ static struct {
 	{ "addembeddedrepo", &advice_add_embedded_repo },
 	{ "ignoredhook", &advice_ignored_hook },
 	{ "waitingforeditor", &advice_waiting_for_editor },
+	{ "graftfiledeprecated", &advice_graft_file_deprecated },
 
 	/* make this an alias for backward compatibility */
 	{ "pushnonfastforward", &advice_push_update_rejected }

index 70568fa7922..9f5064e82a8 100644
@@ -21,6 +21,7 @@ extern int advice_rm_hints;
 extern int advice_add_embedded_repo;
 extern int advice_ignored_hook;
 extern int advice_waiting_for_editor;
+extern int advice_graft_file_deprecated;
 
 int git_default_advice_config(const char *var, const char *value);
 __attribute__((format (printf, 1, 2)))

index ca474a7c112..a96b0a27154 100644
@@ -12,6 +12,7 @@
 #include "prio-queue.h"
 #include "sha1-lookup.h"
 #include "wt-status.h"
+#include "advice.h"
 
 static struct commit_extra_header *read_commit_extra_header_lines(const char *buf, size_t len, const char **);
 

@@ -176,6 +177,14 @@ static int read_graft_file(const char *graft_file)
 	struct strbuf buf = STRBUF_INIT;
 	if (!fp)
 		return -1;
+	if (advice_graft_file_deprecated)
+		advise(_("Support for <GIT_DIR>/info/grafts is deprecated\n"
+			 "and will be removed in a future Git version.\n"
+			 "\n"
+			 "Please use \"git replace --graft [...]\" instead.\n"
+			 "\n"
+			 "Turn this message off by running\n"
+			 "\"git config advice.graftFileDeprecated false\""));
 	while (!strbuf_getwholeline(&buf, fp, '\n')) {
 		/* The format is just "Commit Parent1 Parent2 ...\n" */
 		struct commit_graft *graft = read_graft_line(&buf);

index 05ddc69cf2a..7504ba47511 100755
@@ -110,4 +110,13 @@ do
 	"
 
 done
+
+test_expect_success 'show advice that grafts are deprecated' '
+	git show HEAD 2>err &&
+	test_i18ngrep "git replace" err &&
+	test_config advice.graftFileDeprecated false &&
+	git show HEAD 2>err &&
+	test_i18ngrep ! "git replace" err
+'
+
 test_done

base-commit: fe0a9eaf31dd0c349ae4308498c33a5c3794b293

2.17.0.windows.1.4.g7e4058d72e3

Published-As: https://github.com/dscho/git/releases/tag/deprecate-grafts-v1
Fetch-It-Via: git fetch https://github.com/dscho/git deprecate-grafts-v1

Hi Johannes,

Did you mean: stitch?

It seems that the maturity needed for this commit was reached in 4228e8bc98 (replace: add --graft option, 2014-07-19)

Hi Stefan,

Yes ;-)

Right. I'll add that to the commit message.

There is contrib/convert-grafts-to-replace-refs.sh.

I wonder whether we have to care enough to implement a git replace --convert-graft-file...

Ciao,
Dscho

Hi Johannes,

Oh cool! I wonder if we want to expose it more such that people discover it.

I don't think so.

Hi Stefan,

Nah.

After reflecting about this in the back of my mind, I actually do. So I implemented it.

Ciao,
Dscho

Perhaps, as part of this deprecation, the example in Documentation/git-filter-branch.txt should be updated to suggest git-replace instead of .git/info/grafts.

Maybe, also, Documentation/shallow.txt should talk about replace-refs rather than .git/info/grafts.

Hi Eric,

Good point!

Sure. Internally, of course, "shallow" is still handled very much like the graft file. In that sense, it is probably a good thing to keep referring to the graft file in technical/shallow *in addition* to replace refs.

The reason why the graft file should be deprecated does not apply to the shallow file at all, either: the entire set of problems with grafts comes from the fact that you can *replace* the parents *with other parents*. That allows for pushing corrupt history to public repositories IIRC. The same problems do not arise with the shallow feature, where you can *cut* history, but not replace it.

There is a notable difference between shallow commits and replace refs which I did not think about earlier (it actually only came up in testing):
we currently disallow completely to replace merge commits when they contain mergetags, i.e. entries in the commit header that record the hash of the *tag* that was merged (if any). That includes the case where you want to replace the commit with a root commit, as would be needed for shallow commits.

However, there are more reasons not to conflate the shallow commits with replaced commits: by nature, the "shallow" attribute is a lot more volatile than the "replaced" one, as we want to keep it easy to deepen the history (or to "unshallow" it).

Ciao,
Dscho

It is fragile, as there is no way for the revision machinery to say "but now I want to traverse the graph ignoring the graft file" e.g. when pushing commits to a remote repository (which, as a consequence, can miss commits).

And we already have a better solution with git replace --graft <comit> [<parent>...].

Changes since v1:

- Fixed typo (stich -> stitch).

- Added reference in the commit message to the patch where git replace reached feature parity with the graft file.

- Added the --convert-graft-file option to git replace (with tests).

- Adjusted the advice to suggest --convert-graft-file instead.

- Adjusted the documentation of filter-branch and technical/shallow to reflect the fact that grafts are deprecated.

Johannes Schindelin (7):
replace: "libify" create_graft()
replace: introduce --convert-graft-file
Add a test for git replace --convert-graft-file Deprecate support for .git/info/grafts
filter-branch: stop suggesting to use grafts technical/shallow: describe the relationship with replace refs technical/shallow: describe why shallow cannot use replace refs

 Documentation/git-filter-branch.txt |  2 +-
 Documentation/git-replace.txt       | 11 +++--
 Documentation/technical/shallow.txt | 24 +++++++----
 advice.c                            |  2 +
 advice.h                            |  1 +
 builtin/replace.c                   | 63 ++++++++++++++++++++++++++++-
 commit.c                            | 10 +++++
 t/t6001-rev-list-graft.sh           |  9 +++++
 t/t6050-replace.sh                  | 20 +++++++++
 9 files changed, 129 insertions(+), 13 deletions(-)

base-commit: fe0a9eaf31dd0c349ae4308498c33a5c3794b293
Published-As: https://github.com/dscho/git/releases/tag/deprecate-grafts-v2
Fetch-It-Via: git fetch https://github.com/dscho/git deprecate-grafts-v2

Interdiff vs v1:
diff --git a/Documentation/git-filter-branch.txt b/Documentation/git-filter-branch.txt index b634043183b..1d4d2f86045 100644
--- a/Documentation/git-filter-branch.txt
+++ b/Documentation/git-filter-branch.txt
@@ -288,7 +288,7 @@ git filter-branch --parent-filter \
or even simpler:

-----------------------------------------------
-echo "$commit-id $graft-id" >> .git/info/grafts
+git replace --graft $commit-id $graft-id
git filter-branch $graft-id..HEAD
-----------------------------------------------

diff --git a/Documentation/git-replace.txt b/Documentation/git-replace.txt index e5c57ae6ef4..4dc0686f7d6 100644
--- a/Documentation/git-replace.txt
+++ b/Documentation/git-replace.txt
@@ -11,6 +11,7 @@ SYNOPSIS
'git replace' [-f] <object> <replacement>
'git replace' [-f] --edit <object>
'git replace' [-f] --graft <commit> [<parent>...]
+'git replace' [-f] --convert-graft-file
'git replace' -d <object>...
'git replace' [--format=<format>] [-l [<pattern>]]

@@ -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.
+
+--convert-graft-file::
+ Creates graft commits for all entries in $GIT_DIR/info/grafts
+ and deletes that file upon success. The purpose is to help users
+ with transitioning off of the now-deprecated graft file.

-l <pattern>::
--list <pattern>::
diff --git a/Documentation/technical/shallow.txt b/Documentation/technical/shallow.txt index 5183b154229..cb79181c2bb 100644
--- a/Documentation/technical/shallow.txt
+++ b/Documentation/technical/shallow.txt
@@ -9,19 +9,29 @@ these commits have no parents.
*********************************************************

The basic idea is to write the SHA-1s of shallow commits into
-$GIT_DIR/shallow, and handle its contents like the contents -of $GIT_DIR/info/grafts (with the difference that shallow -cannot contain parent information). +$GIT_DIR/shallow, and handle its contents similar to replace +refs (with the difference that shallow does not actually +create those replace refs) and very much like the deprecated +graft file (with the difference that shallow commits will +always have their parents grafted away, not replaced by +different parents).

-This information is stored in a new file instead of grafts, or -even the config, since the user should not touch that file -at all (even throughout development of the shallow clone, it -was never manually edited!).
+This information is stored in a special-purpose file because the +user should not touch that file at all (even throughout +development of the shallow clone, it was never manually +edited!).

Each line contains exactly one SHA-1. When read, a commit_graft will be constructed, which has nr_parent < 0 to make it easier to discern from user provided grafts.

+Note that the shallow feature could not be changed easily to +use replace refs: a commit containing a mergetag is not allowed +to be replaced, not even by a root commit. Such a commit can be +made shallow, though. Also, having a shallow file explicitly +listing all the commits made shallow makes it a *lot* easier to +do shallow-specific things such as to deepen the history. +
Since fsck-objects relies on the library to read the objects, it honours shallow commits automatically.

diff --git a/builtin/replace.c b/builtin/replace.c
index 935647be6bd..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
@@ -395,7 +396,9 @@ static int create_graft(int argc, const char **argv, int force)

if (get_oid(old_ref, &old_oid) < 0)
die(_("Not a valid object name: '%s'"), old_ref);
- commit = lookup_commit_or_die(&old_oid, old_ref);
+ commit = lookup_commit_reference(&old_oid);
+ if (!commit)
+ return error(_("could not parse %s"), old_ref);

buffer = get_commit_buffer(commit, &size);
strbuf_add(&buf, buffer, size);
@@ -421,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;
+
+ 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;
+
+ while (j < buf.len && isspace(buf.buf[j]))
+ j++;
+ i = j;
+ }
+
+ if (create_graft(args.argc, args.argv, force))
+ strbuf_addf(&err, "\n\t%s", buf.buf);
+
+ argv_array_clear(&args);
+ strbuf_reset(&buf);
+ }
+
+ 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);
+
+ return -1;
+}
+
int cmd_replace(int argc, const char **argv, const char *prefix)
{
int force = 0;
@@ -432,6 +482,7 @@ int cmd_replace(int argc, const char **argv, const char *prefix)
MODE_DELETE,
MODE_EDIT,
MODE_GRAFT,
+ MODE_CONVERT_GRAFT_FILE,
MODE_REPLACE
} cmdmode = MODE_UNSPECIFIED;
struct option options[] = {
@@ -439,6 +490,7 @@ int cmd_replace(int argc, const char **argv, const char *prefix)
OPT_CMDMODE('d', "delete", &cmdmode, N_("delete replace refs"), MODE_DELETE), OPT_CMDMODE('e', "edit", &cmdmode, N_("edit existing object"), MODE_EDIT), OPT_CMDMODE('g', "graft", &cmdmode, N_("change a commit's parents"), MODE_GRAFT),
+ OPT_CMDMODE(0, "convert-graft-file", &cmdmode, N_("convert existing graft file"), MODE_CONVERT_GRAFT_FILE),
OPT_BOOL_F('f', "force", &force, N_("replace the ref if it exists"),
PARSE_OPT_NOCOMPLETE),
OPT_BOOL(0, "raw", &raw, N_("do not pretty-print contents for --edit")),
@@ -461,7 +513,8 @@ int cmd_replace(int argc, const char **argv, const char *prefix)
if (force &&
cmdmode != MODE_REPLACE &&
cmdmode != MODE_EDIT &&

 -	    cmdmode != MODE_GRAFT)
 +	    cmdmode != MODE_GRAFT &&
 +	    cmdmode != MODE_CONVERT_GRAFT_FILE)

usage_msg_opt("-f only makes sense when writing a replacement",
git_replace_usage, options);

@@ -494,6 +547,12 @@ int cmd_replace(int argc, const char **argv, const char *prefix)
git_replace_usage, options);
return create_graft(argc, argv, force);

+ case MODE_CONVERT_GRAFT_FILE:
+ if (argc != 0)
+ usage_msg_opt("--convert-graft-file takes no argument",
+ git_replace_usage, options);
+ return !!convert_graft_file(force);
+
case MODE_LIST:
if (argc > 1)
usage_msg_opt("only one pattern can be given with -l",
diff --git a/commit.c b/commit.c
index a96b0a27154..1a5e8777617 100644
--- a/commit.c
+++ b/commit.c
@@ -181,7 +181,8 @@ static int read_graft_file(const char *graft_file)
advise(_("Support for <GIT_DIR>/info/grafts is deprecated\n"
"and will be removed in a future Git version.\n" "\n"
- "Please use \"git replace --graft [...]\" instead.\n"
+ "Please use \"git replace --convert-graft-file\"\n"
+ "to convert the grafts into replace refs.\n"
"\n"
"Turn this message off by running\n"
"\"git config advice.graftFileDeprecated false\""));
diff --git a/t/t6050-replace.sh b/t/t6050-replace.sh
index c630aba657e..77ded6df653 100755
--- a/t/t6050-replace.sh
+++ b/t/t6050-replace.sh
@@ -444,4 +444,24 @@ test_expect_success GPG '--graft on a commit with a mergetag' '
git replace -d $HASH10
'

+test_expect_success '--convert-graft-file' '
+ : add and convert graft file &&
+ printf "%s\n%s %s\n%s\n" \
+ $(git rev-parse HEAD^^ HEAD^ HEAD^^ HEAD^2) \
+ >.git/info/grafts &&
+ git replace --convert-graft-file &&
+ test_path_is_missing .git/info/grafts &&
+
+ : verify that the history is now "grafted" &&
+ git rev-list HEAD >out &&
+ test_line_count = 4 out &&
+
+ : create invalid graft file and verify that it is not deleted &&
+ test_when_finished "rm -f .git/info/grafts" &&
+ echo $EMPTY_BLOB $EMPTY_TREE >.git/info/grafts &&
+ test_must_fail git replace --convert-graft-file 2>err &&
+ grep "$EMPTY_BLOB $EMPTY_TREE" err &&
+ grep "$EMPTY_BLOB $EMPTY_TREE" .git/info/grafts
+'
+
test_done

2.17.0.windows.1.4.g7e4058d72e3

It is quite convenient to simply die() in builtins, in the absence of proper exception handling, because it allows us to just go belly up without having to implement error handling chains.

Of course, for reusable library functions, this is a big no-no, so we (try to) restrict the usage of die() to one-shot commands, i.e. places where we know that the caller does not want to, say, give the user a useful high-level error message, i.e. a message that the function calling die() could not possibly know.

The problem with this reasoning is that sooner or later, pretty much all useful functions will *need* to be libified: the more useful a function, the more likely it is to be called from a call chain where the outer function implements a high-level operation that needs to provide additional advice to the user in case of failure.

This is the case here: the create_graft() function is useful enough to be called in a loop, say, in the upcoming patch to convert a graft file in one fell swoop. Therefore, this function must not be allowed to die(), nor any of its callees.

---
 builtin/replace.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

index 935647be6bd..43264f0998e 100644
@@ -395,7 +395,9 @@ static int create_graft(int argc, const char **argv, int force)
 
 	if (get_oid(old_ref, &old_oid) < 0)
 		die(_("Not a valid object name: '%s'"), old_ref);
-	commit = lookup_commit_or_die(&old_oid, old_ref);
+	commit = lookup_commit_reference(&old_oid);
+	if (!commit)
+		return error(_("could not parse %s"), old_ref);
 
 	buffer = get_commit_buffer(commit, &size);
 	strbuf_add(&buf, buffer, size);

2.17.0.windows.1.4.g7e4058d72e3

All of the first three paragraphs are already widely known to the project, and I do not think you need to verbosely repeat it, if brevity demands it.

One thing that the proposed log message for this step would be far more useful to say is that the current callers of create_graft() is fine with the behaviour change (i.e. prepared to act on an error return).

Also it may want to say why the other die() we see in this function in the pre-context is OK. I actually think it is not OK and should return error(not a valid object name) the same way (I suspect that this is mere omission/mistake, and you did not intend to leave it dying). As long as the caller is prepared to deal with an error return, that is, which was the previous point.

FWIW, I fully agree with the goal of making this (or other pieces that would be useful if they were reusable) reusable.

[taking the role of Advocatus Diaboli]

Well, this information (first three paragraphs) is maybe widely known to the project, but do we have it explicitely documented somwehere, like CodingGuidelines, or FAQ on some web site?

Right.

[taking the role of Advocatus Diaboli]

Maybe they are errors that caller cannot handle sanely in any way? If I understand things correctly the problem is with parameter to the --graft option...

Hi Junio,

Great! That's something I'll gladly take into my break.

Ciao,
Dscho

This option is intended to help with the transition away from the now-deprecated graft file.

---
 Documentation/git-replace.txt | 11 +++++--
 builtin/replace.c             | 59 ++++++++++++++++++++++++++++++++++-
 2 files changed, 66 insertions(+), 4 deletions(-)

index e5c57ae6ef4..4dc0686f7d6 100644
@@ -11,6 +11,7 @@ SYNOPSIS
 'git replace' [-f] <object> <replacement>
 'git replace' [-f] --edit <object>
 'git replace' [-f] --graft <commit> [<parent>...]
+'git replace' [-f] --convert-graft-file
 'git replace' -d <object>...
 'git replace' [--format=<format>] [-l [<pattern>]]
 

@@ -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.
+
+--convert-graft-file::
+	Creates graft commits for all entries in `$GIT_DIR/info/grafts`
+	and deletes that file upon success. The purpose is to help users
+	with transitioning off of the now-deprecated graft file.
 
 -l <pattern>::
 --list <pattern>::

2.17.0.windows.1.4.g7e4058d72e3

Hi Christian,

Ouch... thanks for catching this!

I do not think so. Those old_grafts will only lie around uselessly... and are no longer necessary.

Ciao,
Dscho

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.

Hi Junio,

Sorry, I was still under the impression that contrib/ was somewhat off limits when replacing scripts by C code (it used to be the rule, but I did see that the contrib/examples/*.sh files went poof).

Will change.

Obviously I agree. It just makes for such a vastly superior user experience to get an explanation that yes, something has been deprecated, but Do Not Panic, this is what you do to get out of this fix.

In the same vein, I considered some real hackery that would make the deprecation notice in commit.c less annoying: I wanted to use some tentative date in the future as cut-off, and then use some arithmetic based on the time left until that date to show the deprecation notice increasingly more often.

However, this would have made it really frustrating for users seeing the notice fly by, say, in a lengthy script's output, and then trying to reproduce the message. So as much fun as it would have been to come up with that formula, I refrained from it.

FWIW I considered introducing such a helper. But I really want to have the full line to show to the user, so I went with the current version.

Based on your comment, I realized that since argv_array_push() duplicates the string *anyway*, I could have implemented argv_array_split().

Briefly deterred by the insight that some readers will invariably think that this function performs de-quoting, Unix shell style, I almost decided against that.

But only almost.

If anybody needs a de-quoting version of argv_array_split(), or a version that uses a different delimiter than white-space, my version should provide a neat starting point (new parameters should be added for those purposes, of course, since we really need a non-de-quoting version in --convert-graft-file).

So the next iteration of this patch series will sport a shiny new argv_array_split(). Enjoy.

Riiiight! I meant to look at the parser to verify that I do the same, but forgot (I had the incorrect recollection that the graft file cannot contain comments or empty lines).

So I remedied that now. Including correct handling of empty lines.

Good point. I did assume that strbuf_getline() would append, and I was wrong. I dropped the needless strbuf_reset() call.

Thanks.

It is a *lot* worse than just one remaining die(), unfortunately:
create_graft() calls replace_parents() and check_mergetags(), both without checking their return value. Because neither return anything. Because they all die (cue Dash from The Incredibles: We're gonna DIIIIIE!). Also, replace_object_oid(), while it returns an int indicating an error, chooses to die() in plenty of places. After following down to for_each_mergetag(), export_object() and check_ref_valid(), I decided to just go ahead and libify the entire builtin/replace.c, which seemed to be the vastly more efficient route, and I really wish we would start deprecating die(). Thanks for sending me down that rabbit hole ;-)

Note: there is a call to get_commit_buffer() which still die()s if it cannot read the object or if it is not a commit. If we care enough about the concocted scenario where somebody would have installed a graft file with a line that references, say, a tree object instead of a commit object in its first oid, we need to do something about that: the graft line would simply have been ignored, as grafts were only used when a corresponding commit was parsed (which would never happen for the concocted example I gave). I am rather certain, though, that I do not want to care about such a scenario: a user would have to go out of their way to get into that situation.

Ciao,
Dscho

The proof, as the saying goes, lies in the pudding. So here is a regression test that not only demonstrates what the option is supposed to accomplish, but also demonstrates that it does accomplish it.

---
 t/t6050-replace.sh | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

index c630aba657e..77ded6df653 100755
@@ -444,4 +444,24 @@ test_expect_success GPG '--graft on a commit with a mergetag' '
 	git replace -d $HASH10
 '
 
+test_expect_success '--convert-graft-file' '
+	: add and convert graft file &&
+	printf "%s\n%s %s\n%s\n" \
+		$(git rev-parse HEAD^^ HEAD^ HEAD^^ HEAD^2) \
+		>.git/info/grafts &&
+	git replace --convert-graft-file &&
+	test_path_is_missing .git/info/grafts &&
+
+	: verify that the history is now "grafted" &&
+	git rev-list HEAD >out &&
+	test_line_count = 4 out &&
+
+	: create invalid graft file and verify that it is not deleted &&
+	test_when_finished "rm -f .git/info/grafts" &&
+	echo $EMPTY_BLOB $EMPTY_TREE >.git/info/grafts &&
+	test_must_fail git replace --convert-graft-file 2>err &&
+	grep "$EMPTY_BLOB $EMPTY_TREE" err &&
+	grep "$EMPTY_BLOB $EMPTY_TREE" .git/info/grafts
+'
+
 test_done

2.17.0.windows.1.4.g7e4058d72e3

The above spreads the misconception that the value of the test is "what I wrote works, look here". It is not. "Here is how this thing is supposed to work. You are free to improve it, but do not break the basic promises these tests outline" to protect the resulting system is.

Hi Junio,

I am but a lousy foreigner, but to me, basically we said the same.

Well, don't you know, that is how my first version looked. It failed, though, as git rev-parse HEAD^ HEAD^^ outputs two *lines*.

And the version with a here-doc and inlined $(git rev-parse ...) really looks even uglier.

I disagree that it is so horrible to read. If a regression occurs, you will have the .git/info/grafts file as a reference anyway, so there is little you need to decipher.

Ciao,
Dscho

The grafts feature was a convenient way to "stitch together" ancient history to the fresh start of linux.git.

Its implementation is, however, not up to Git's standards, as there are too many ways where it can lead to surprising and unwelcome behavior.

For example, when pushing from a repository with active grafts, it is possible to miss commits that have been "grafted out", resulting in a broken state on the other side.

Also, the grafts feature is limited to "rewriting" commits' list of parents, it cannot replace anything else.

The much younger feature implemented as git replace set out to remedy those limitations and dangerous bugs.

Seeing as git replace is pretty mature by now (since 4228e8bc98 (replace: add --graft option, 2014-07-19) it can perform the graft file's duties), it is time to deprecate support for the graft file, and to retire it eventually.

---
 advice.c                  |  2 ++
 advice.h                  |  1 +
 commit.c                  | 10 ++++++++++
 t/t6001-rev-list-graft.sh |  9 +++++++++
 4 files changed, 22 insertions(+)

index 406efc183ba..4411704fd45 100644
@@ -19,6 +19,7 @@ int advice_rm_hints = 1;
 int advice_add_embedded_repo = 1;
 int advice_ignored_hook = 1;
 int advice_waiting_for_editor = 1;
+int advice_graft_file_deprecated = 1;
 
 static struct {
 	const char *name;

@@ -42,6 +43,7 @@ static struct {
 	{ "addembeddedrepo", &advice_add_embedded_repo },
 	{ "ignoredhook", &advice_ignored_hook },
 	{ "waitingforeditor", &advice_waiting_for_editor },
+	{ "graftfiledeprecated", &advice_graft_file_deprecated },
 
 	/* make this an alias for backward compatibility */
 	{ "pushnonfastforward", &advice_push_update_rejected }

index 70568fa7922..9f5064e82a8 100644
@@ -21,6 +21,7 @@ extern int advice_rm_hints;
 extern int advice_add_embedded_repo;
 extern int advice_ignored_hook;
 extern int advice_waiting_for_editor;
+extern int advice_graft_file_deprecated;
 
 int git_default_advice_config(const char *var, const char *value);
 __attribute__((format (printf, 1, 2)))

index ca474a7c112..1a5e8777617 100644
@@ -12,6 +12,7 @@
 #include "prio-queue.h"
 #include "sha1-lookup.h"
 #include "wt-status.h"
+#include "advice.h"
 
 static struct commit_extra_header *read_commit_extra_header_lines(const char *buf, size_t len, const char **);
 

@@ -176,6 +177,15 @@ static int read_graft_file(const char *graft_file)
 	struct strbuf buf = STRBUF_INIT;
 	if (!fp)
 		return -1;
+	if (advice_graft_file_deprecated)
+		advise(_("Support for <GIT_DIR>/info/grafts is deprecated\n"
+			 "and will be removed in a future Git version.\n"
+			 "\n"
+			 "Please use \"git replace --convert-graft-file\"\n"
+			 "to convert the grafts into replace refs.\n"
+			 "\n"
+			 "Turn this message off by running\n"
+			 "\"git config advice.graftFileDeprecated false\""));
 	while (!strbuf_getwholeline(&buf, fp, '\n')) {
 		/* The format is just "Commit Parent1 Parent2 ...\n" */
 		struct commit_graft *graft = read_graft_line(&buf);

index 05ddc69cf2a..7504ba47511 100755
@@ -110,4 +110,13 @@ do
 	"
 
 done
+
+test_expect_success 'show advice that grafts are deprecated' '
+	git show HEAD 2>err &&
+	test_i18ngrep "git replace" err &&
+	test_config advice.graftFileDeprecated false &&
+	git show HEAD 2>err &&
+	test_i18ngrep ! "git replace" err
+'
+
 test_done

2.17.0.windows.1.4.g7e4058d72e3

The graft file is deprecated now, so let's use replace refs in the example in filter-branch's man page instead.

---
 Documentation/git-filter-branch.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index b634043183b..1d4d2f86045 100644
@@ -288,7 +288,7 @@ git filter-branch --parent-filter \
 or even simpler:
 
 -----------------------------------------------
-echo "$commit-id $graft-id" >> .git/info/grafts
+git replace --graft $commit-id $graft-id
 git filter-branch $graft-id..HEAD
 -----------------------------------------------
 

2.17.0.windows.1.4.g7e4058d72e3

Now that grafts are deprecated, we should start to assume that readers have no idea what grafts are. So it makes more sense to describe the "shallow" feature in terms of replace refs.

---
 Documentation/technical/shallow.txt | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

index 5183b154229..b3ff23c25f6 100644
@@ -9,14 +9,17 @@ these commits have no parents.
 *********************************************************
 
 The basic idea is to write the SHA-1s of shallow commits into
-$GIT_DIR/shallow, and handle its contents like the contents
-of $GIT_DIR/info/grafts (with the difference that shallow
-cannot contain parent information).
-
-This information is stored in a new file instead of grafts, or
-even the config, since the user should not touch that file
-at all (even throughout development of the shallow clone, it
-was never manually edited!).
+$GIT_DIR/shallow, and handle its contents similar to replace
+refs (with the difference that shallow does not actually
+create those replace refs) and very much like the deprecated
+graft file (with the difference that shallow commits will
+always have their parents grafted away, not replaced by
+different parents).
+
+This information is stored in a special-purpose file because the
+user should not touch that file at all (even throughout
+development of the shallow clone, it was never manually
+edited!).
 
 Each line contains exactly one SHA-1. When read, a commit_graft
 will be constructed, which has nr_parent < 0 to make it easier

2.17.0.windows.1.4.g7e4058d72e3

It is tempting to do away with commit_graft altogether (in the long haul), now that grafts are deprecated.

However, the shallow feature needs a couple of things that the replace refs cannot fulfill. Let's point that out in the documentation.

---

By the way, I did not squash this into the previous patch on
purpose: in my mind, describing the relationship with replace refs is different enough from pointing out from the goal of this here patch to merit a separate commit. And that goal is: to describe why "shallow" cannot be implemented on top of "replace refs".

 Documentation/technical/shallow.txt | 7 +++++++
 1 file changed, 7 insertions(+)

index b3ff23c25f6..cb79181c2bb 100644
@@ -25,6 +25,13 @@ Each line contains exactly one SHA-1. When read, a commit_graft
 will be constructed, which has nr_parent < 0 to make it easier
 to discern from user provided grafts.
 
+Note that the shallow feature could not be changed easily to
+use replace refs: a commit containing a `mergetag` is not allowed
+to be replaced, not even by a root commit. Such a commit can be
+made shallow, though. Also, having a `shallow` file explicitly
+listing all the commits made shallow makes it a *lot* easier to
+do shallow-specific things such as to deepen the history.
+
 Since fsck-objects relies on the library to read the objects,
 it honours shallow commits automatically.
 

2.17.0.windows.1.4.g7e4058d72e3

It is fragile, as there is no way for the revision machinery to say "but now I want to traverse the graph ignoring the graft file" e.g. when pushing commits to a remote repository (which, as a consequence, can miss commits).

And we already have a better solution with git replace --graft <comit> [<parent>...].

Changes since v2:

- Fixed tyop in the description of --graft where it suggests --convert-graft-file instead.

- Dropped the rant from the "libify create_graft()" commit.

- Changed a die("BUG: ...") call to BUG() (instead of libifying it).

- Truly libified create_graft() by replacing every single instance where die() (or lookup_commit_or_die()) was called with a proper error(), adding resource handling to avoid leaks, so that that rant that was dropped from the commit message would now truly be merited.

- Graft files with commented lines are now handled correctly.

- Graft files with empty lines are now handled correctly, too.

- The graft file parser is now based on a shiny new argv_array_split().

- The script in contrib/ whose functionality is superseded by git replace --convert-graft-file was removed.

Johannes Schindelin (11):
argv_array: offer to split a string by whitespace
commit: Let the callback of for_each_mergetag return on error
replace: avoid using die() to indicate a bug
replace: "libify" create_graft() and callees
replace: introduce --convert-graft-file
Add a test for git replace --convert-graft-file Deprecate support for .git/info/grafts
filter-branch: stop suggesting to use grafts technical/shallow: describe the relationship with replace refs technical/shallow: describe why shallow cannot use replace refs Remove obsolete script to convert grafts to replace refs

 Documentation/git-filter-branch.txt       |   2 +-
 Documentation/git-replace.txt             |  11 +-
 Documentation/technical/shallow.txt       |  24 ++-
 advice.c                                  |   2 +
 advice.h                                  |   1 +
 argv-array.c                              |  20 +++
 argv-array.h                              |   1 +
 builtin/replace.c                         | 189 +++++++++++++++-------
 commit.c                                  |  18 ++-
 commit.h                                  |   4 +-
 contrib/convert-grafts-to-replace-refs.sh |  28 ----
 log-tree.c                                |  13 +-
 t/t6001-rev-list-graft.sh                 |   9 ++
 t/t6050-replace.sh                        |  20 +++
 14 files changed, 235 insertions(+), 107 deletions(-)
 delete mode 100755 contrib/convert-grafts-to-replace-refs.sh

base-commit: fe0a9eaf31dd0c349ae4308498c33a5c3794b293
Published-As: https://github.com/dscho/git/releases/tag/deprecate-grafts-v3
Fetch-It-Via: git fetch https://github.com/dscho/git deprecate-grafts-v3

Interdiff vs v2:
diff --git a/Documentation/git-replace.txt b/Documentation/git-replace.txt index 4dc0686f7d6..246dc9943c2 100644
--- a/Documentation/git-replace.txt
+++ b/Documentation/git-replace.txt
@@ -89,7 +89,7 @@ OPTIONS
[<parent>...] instead of <commit>'s parents. A replacement ref is then created to replace <commit> with the newly created commit. Use --convert-graft-file to convert a
- $GIT_DIR/info/grafts file use replace refs instead.
+ $GIT_DIR/info/grafts file and use replace refs instead.

--convert-graft-file::
Creates graft commits for all entries in $GIT_DIR/info/grafts
diff --git a/argv-array.c b/argv-array.c
index 5d370fa3366..cb5bcd2c064 100644
--- a/argv-array.c
+++ b/argv-array.c
@@ -64,6 +64,26 @@ void argv_array_pop(struct argv_array *array)
array->argc--;
}

+void argv_array_split(struct argv_array *array, const char *to_split) +{
+ while (isspace(*to_split))
+ to_split++;
+ for (;;) {
+ const char *p = to_split;
+
+ if (!*p)
+ break;
+
+ while (*p && !isspace(*p))
+ p++;
+ argv_array_push_nodup(array, xstrndup(to_split, p - to_split));
+
+ while (isspace(*p))
+ p++;
+ to_split = p;
+ }
+}
+
void argv_array_clear(struct argv_array *array)
{
if (array->argv != empty_argv) {
diff --git a/argv-array.h b/argv-array.h
index 29056e49a12..c7c397695df 100644
--- a/argv-array.h
+++ b/argv-array.h
@@ -19,6 +19,7 @@ LAST_ARG_MUST_BE_NULL
void argv_array_pushl(struct argv_array *, ...);
void argv_array_pushv(struct argv_array *, const char **); void argv_array_pop(struct argv_array *);
+void argv_array_split(struct argv_array *, const char *);
void argv_array_clear(struct argv_array *);
const char **argv_array_detach(struct argv_array *);

diff --git a/builtin/replace.c b/builtin/replace.c
index 4cdc00a96df..6b3e0301e90 100644
--- a/builtin/replace.c
+++ b/builtin/replace.c
@@ -80,9 +80,9 @@ static int list_replace_refs(const char *pattern, const char *format)
else if (!strcmp(format, "long"))
data.format = REPLACE_FORMAT_LONG;
else
- die("invalid replace format '%s'\n"
- "valid formats are 'short', 'medium' and 'long'\n",
- format);
+ return error("invalid replace format '%s'\n"
+ "valid formats are 'short', 'medium' and 'long'\n",
+ format);

  	for_each_replace_ref(show_reference, (void *)&data);

@@ -135,7 +135,7 @@ static int delete_replace_ref(const char *name, const char *ref,
return 0;
}

-static void check_ref_valid(struct object_id *object,
+static int check_ref_valid(struct object_id *object,
struct object_id *prev,
struct strbuf *ref,
int force)
@@ -143,12 +143,13 @@ static void check_ref_valid(struct object_id *object,
strbuf_reset(ref);
strbuf_addf(ref, "%s%s", git_replace_ref_base, oid_to_hex(object)); if (check_refname_format(ref->buf, 0))
- die("'%s' is not a valid ref name.", ref->buf);
+ return error("'%s' is not a valid ref name.", ref->buf);

if (read_ref(ref->buf, prev))
oidclr(prev);
else if (!force)
- die("replace ref '%s' already exists", ref->buf);
+ return error("replace ref '%s' already exists", ref->buf);
+ return 0;
}

static int replace_object_oid(const char *object_ref,
@@ -162,28 +163,31 @@ static int replace_object_oid(const char *object_ref,
struct strbuf ref = STRBUF_INIT;
struct ref_transaction *transaction;
struct strbuf err = STRBUF_INIT;
+ int res = 0;

obj_type = oid_object_info(object, NULL);
repl_type = oid_object_info(repl, NULL);
if (!force && obj_type != repl_type)
- die("Objects must be of the same type.\n"

 -		    "'%s' points to a replaced object of type '%s'\n"
 -		    "while '%s' points to a replacement object of type '%s'.",
 -		    object_ref, type_name(obj_type),
 -		    replace_ref, type_name(repl_type));
 -
 -	check_ref_valid(object, &prev, &ref, force);
 +		return error("Objects must be of the same type.\n"
 +			     "'%s' points to a replaced object of type '%s'\n"
 +			     "while '%s' points to a replacement object of "
 +			     "type '%s'.",
 +			     object_ref, type_name(obj_type),
 +			     replace_ref, type_name(repl_type));

+
+ if (check_ref_valid(object, &prev, &ref, force))
+ return -1;

transaction = ref_transaction_begin(&err);
if (!transaction ||
ref_transaction_update(transaction, ref.buf, repl, &prev,
0, NULL, &err) ||
ref_transaction_commit(transaction, &err))
- die("%s", err.buf);
+ res = error("%s", err.buf);

ref_transaction_free(transaction);
strbuf_release(&ref);
- return 0;
+ return res;
}

static int replace_object(const char *object_ref, const char *replace_ref, int force)
@@ -191,9 +195,11 @@ static int replace_object(const char *object_ref, const char *replace_ref, int f
struct object_id object, repl;

if (get_oid(object_ref, &object))
- die("Failed to resolve '%s' as a valid ref.", object_ref);
+ return error("Failed to resolve '%s' as a valid ref.",
+ object_ref);
if (get_oid(replace_ref, &repl))
- die("Failed to resolve '%s' as a valid ref.", replace_ref);
+ return error("Failed to resolve '%s' as a valid ref.",
+ replace_ref);

return replace_object_oid(object_ref, &object, replace_ref, &repl, force);
}
@@ -203,7 +209,7 @@ static int replace_object(const char *object_ref, const char *replace_ref, int f
* If "raw" is true, then the object's raw contents are printed according to
* "type". Otherwise, we pretty-print the contents for human editing.
*/
-static void export_object(const struct object_id *oid, enum object_type type, +static int export_object(const struct object_id *oid, enum object_type type,
int raw, const char *filename)
{
struct child_process cmd = CHILD_PROCESS_INIT;
@@ -211,7 +217,7 @@ static void export_object(const struct object_id *oid, enum object_type type,

fd = open(filename, O_WRONLY | O_CREAT | O_TRUNC, 0666); if (fd < 0)
- die_errno("unable to open %s for writing", filename);
+ return error_errno("unable to open %s for writing", filename);

argv_array_push(&cmd.args, "--no-replace-objects"); argv_array_push(&cmd.args, "cat-file");
@@ -224,7 +230,8 @@ static void export_object(const struct object_id *oid, enum object_type type,
cmd.out = fd;

if (run_command(&cmd))
- die("cat-file reported failure");
+ return error("cat-file reported failure");
+ return 0;
}

/*
@@ -232,14 +239,14 @@ static void export_object(const struct object_id *oid, enum object_type type,
* interpreting it as "type", and writing the result to the object database.
* The sha1 of the written object is returned via sha1.
*/
-static void import_object(struct object_id *oid, enum object_type type,
+static int import_object(struct object_id *oid, enum object_type type,
int raw, const char *filename)
{
int fd;

fd = open(filename, O_RDONLY);
if (fd < 0)
- die_errno("unable to open %s for reading", filename);
+ return error_errno("unable to open %s for reading", filename);

if (!raw && type == OBJ_TREE) {
const char *argv[] = { "mktree", NULL };
@@ -251,27 +258,38 @@ static void import_object(struct object_id *oid, enum object_type type,
cmd.in = fd;
cmd.out = -1;

- if (start_command(&cmd))
- die("unable to spawn mktree");
+ if (start_command(&cmd)) {
+ close(fd);
+ return error("unable to spawn mktree");
+ }

- if (strbuf_read(&result, cmd.out, 41) < 0)
- die_errno("unable to read from mktree");
+ if (strbuf_read(&result, cmd.out, 41) < 0) {
+ close(fd);
+ close(cmd.out);
+ return error_errno("unable to read from mktree");
+ }
close(cmd.out);

- if (finish_command(&cmd))
- die("mktree reported failure");
- if (get_oid_hex(result.buf, oid) < 0)
- die("mktree did not return an object name");
+ if (finish_command(&cmd)) {
+ strbuf_release(&result);
+ return error("mktree reported failure");
+ }
+ if (get_oid_hex(result.buf, oid) < 0) {
+ strbuf_release(&result);
+ return error("mktree did not return an object name");
+ }

  		strbuf_release(&result);
  	} else {
  		struct stat st;
  		int flags = HASH_FORMAT_CHECK | HASH_WRITE_OBJECT;

- if (fstat(fd, &st) < 0)
- die_errno("unable to fstat %s", filename);
+ if (fstat(fd, &st) < 0) {
+ close(fd);
+ return error_errno("unable to fstat %s", filename);
+ }
if (index_fd(oid, fd, &st, type, NULL, flags) < 0)
- die("unable to write object to database");
+ return error("unable to write object to database");
/* index_fd close()s fd for us */
}

@@ -279,6 +297,7 @@ static void import_object(struct object_id *oid, enum object_type type,
* No need to close(fd) here; both run-command and index-fd
* will have done it for us.
*/
+ return 0;
}

static int edit_and_replace(const char *object_ref, int force, int raw)
@@ -289,19 +308,23 @@ static int edit_and_replace(const char *object_ref, int force, int raw)
struct strbuf ref = STRBUF_INIT;

if (get_oid(object_ref, &old_oid) < 0)
- die("Not a valid object name: '%s'", object_ref);
+ return error("Not a valid object name: '%s'", object_ref);

type = oid_object_info(&old_oid, NULL);
if (type < 0)
- die("unable to get object type for %s", oid_to_hex(&old_oid));
+ return error("unable to get object type for %s",
+ oid_to_hex(&old_oid));

- check_ref_valid(&old_oid, &prev, &ref, force);
+ if (check_ref_valid(&old_oid, &prev, &ref, force))
+ return -1;
strbuf_release(&ref);

- export_object(&old_oid, type, raw, tmpfile);
+ if (export_object(&old_oid, type, raw, tmpfile))
+ return -1;
if (launch_editor(tmpfile, NULL, NULL) < 0)
- die("editing object file failed");
- import_object(&new_oid, type, raw, tmpfile);
+ return error("editing object file failed");
+ if (import_object(&new_oid, type, raw, tmpfile))
+ return -1;

  	free(tmpfile);

@@ -311,7 +334,7 @@ static int edit_and_replace(const char *object_ref, int force, int raw)
return replace_object_oid(object_ref, &old_oid, "replacement", &new_oid, force);
}

-static void replace_parents(struct strbuf *buf, int argc, const char **argv) +static int replace_parents(struct strbuf *buf, int argc, const char **argv)
{
struct strbuf new_parents = STRBUF_INIT;
const char *parent_start, *parent_end;
@@ -329,8 +352,10 @@ static void replace_parents(struct strbuf *buf, int argc, const char **argv)
for (i = 0; i < argc; i++) {
struct object_id oid;
if (get_oid(argv[i], &oid) < 0)
- die(_("Not a valid object name: '%s'"), argv[i]);
- lookup_commit_or_die(&oid, argv[i]);
+ return error(_("Not a valid object name: '%s'"),
+ argv[i]);
+ if (!lookup_commit_reference(&oid))
+ return error(_("could not parse %s"), argv[i]);
strbuf_addf(&new_parents, "parent %s\n", oid_to_hex(&oid));
}

@@ -339,6 +364,7 @@ static void replace_parents(struct strbuf *buf, int argc, const char **argv)
new_parents.buf, new_parents.len);

strbuf_release(&new_parents);
+ return 0;
}

struct check_mergetag_data {
@@ -346,7 +372,7 @@ struct check_mergetag_data {
const char **argv;
};

-static void check_one_mergetag(struct commit *commit,
+static int check_one_mergetag(struct commit *commit,
struct commit_extra_header *extra, void *data)
{
@@ -359,30 +385,32 @@ static void check_one_mergetag(struct commit *commit,
hash_object_file(extra->value, extra->len, type_name(OBJ_TAG), &tag_oid); tag = lookup_tag(&tag_oid);
if (!tag)
- die(_("bad mergetag in commit '%s'"), ref);
+ return error(_("bad mergetag in commit '%s'"), ref);
if (parse_tag_buffer(tag, extra->value, extra->len))
- die(_("malformed mergetag in commit '%s'"), ref);
+ return error(_("malformed mergetag in commit '%s'"), ref);

/* iterate over new parents */
for (i = 1; i < mergetag_data->argc; i++) {
struct object_id oid;
if (get_oid(mergetag_data->argv[i], &oid) < 0)
- die(_("Not a valid object name: '%s'"), mergetag_data->argv[i]);
+ return error(_("Not a valid object name: '%s'"),
+ mergetag_data->argv[i]);
if (!oidcmp(&tag->tagged->oid, &oid))
- return; /* found */
+ return 0; /* found */
}

- die(_("original commit '%s' contains mergetag '%s' that is discarded; "
- "use --edit instead of --graft"), ref, oid_to_hex(&tag_oid));
+ return error(_("original commit '%s' contains mergetag '%s' that is "
+ "discarded; use --edit instead of --graft"), ref,
+ oid_to_hex(&tag_oid));
}

-static void check_mergetags(struct commit *commit, int argc, const char **argv) +static int check_mergetags(struct commit *commit, int argc, const char **argv)
{
struct check_mergetag_data mergetag_data;

mergetag_data.argc = argc;
mergetag_data.argv = argv;
- for_each_mergetag(check_one_mergetag, commit, &mergetag_data);
+ return for_each_mergetag(check_one_mergetag, commit, &mergetag_data);
}

static int create_graft(int argc, const char **argv, int force)
@@ -395,7 +423,7 @@ static int create_graft(int argc, const char **argv, int force)
unsigned long size;

if (get_oid(old_ref, &old_oid) < 0)
- die(_("Not a valid object name: '%s'"), old_ref);
+ return error(_("Not a valid object name: '%s'"), old_ref);
commit = lookup_commit_reference(&old_oid);
if (!commit)
return error(_("could not parse %s"), old_ref);
@@ -404,17 +432,20 @@ static int create_graft(int argc, const char **argv, int force)
strbuf_add(&buf, buffer, size);
unuse_commit_buffer(commit, buffer);

- replace_parents(&buf, argc - 1, &argv[1]);
+ if (replace_parents(&buf, argc - 1, &argv[1]) < 0)
+ return -1;

if (remove_signature(&buf)) {
warning(_("the original commit '%s' has a gpg signature."), old_ref); warning(_("the signature will be removed in the replacement commit!"));
}

- check_mergetags(commit, argc, argv);
+ if (check_mergetags(commit, argc, argv))
+ return -1;

if (write_object_file(buf.buf, buf.len, commit_type, &new_oid))
- die(_("could not write replacement commit for: '%s'"), old_ref);
+ return error(_("could not write replacement commit for: '%s'"),
+ old_ref);

  	strbuf_release(&buf);

@@ -435,28 +466,13 @@ static int convert_graft_file(int force)
return -1;

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;
-
- while (j < buf.len && isspace(buf.buf[j]))
- j++;
- i = j;
- }
+ if (*buf.buf == '#')
+ continue;

- if (create_graft(args.argc, args.argv, force))
+ argv_array_split(&args, buf.buf);
+ if (args.argc && create_graft(args.argc, args.argv, force))
strbuf_addf(&err, "\n\t%s", buf.buf);
-
argv_array_clear(&args);
- strbuf_reset(&buf);
}

strbuf_release(&buf);
@@ -560,6 +576,6 @@ int cmd_replace(int argc, const char **argv, const char *prefix)
return list_replace_refs(argv[0], format);

default:
- die("BUG: invalid cmdmode %d", (int)cmdmode);
+ BUG("invalid cmdmode %d", (int)cmdmode);
}
}
diff --git a/commit.c b/commit.c
index 1a5e8777617..451d3ce8dfe 100644
--- a/commit.c
+++ b/commit.c
@@ -1298,17 +1298,19 @@ struct commit_extra_header *read_commit_extra_headers(struct commit *commit,
return extra;
}

-void for_each_mergetag(each_mergetag_fn fn, struct commit *commit, void *data) +int for_each_mergetag(each_mergetag_fn fn, struct commit *commit, void *data)
{
struct commit_extra_header *extra, *to_free;
+ int res = 0;

to_free = read_commit_extra_headers(commit, NULL);
- for (extra = to_free; extra; extra = extra->next) {
+ for (extra = to_free; !res && extra; extra = extra->next) {
if (strcmp(extra->key, "mergetag"))
continue; /* not a merge tag */
- fn(commit, extra, data);
+ res = fn(commit, extra, data);
}
free_commit_extra_headers(to_free);
+ return res;
}

static inline int standard_header_field(const char *field, size_t len)
diff --git a/commit.h b/commit.h
index 0fb8271665c..9000895ad91 100644
--- a/commit.h
+++ b/commit.h
@@ -291,10 +291,10 @@ extern const char *find_commit_header(const char *msg, const char *key,
/* Find the end of the log message, the right place for a new trailer. */ extern int ignore_non_trailer(const char *buf, size_t len);

-typedef void (*each_mergetag_fn)(struct commit *commit, struct commit_extra_header *extra, +typedef int (*each_mergetag_fn)(struct commit *commit, struct commit_extra_header *extra,
void *cb_data);

-extern void for_each_mergetag(each_mergetag_fn fn, struct commit *commit, void *data); +extern int for_each_mergetag(each_mergetag_fn fn, struct commit *commit, void *data);

struct merge_remote_desc {
struct object *obj; /* the named object, could be a tag */
diff --git a/contrib/convert-grafts-to-replace-refs.sh b/contrib/convert-grafts-to-replace-refs.sh deleted file mode 100755
index 0cbc917b8cf..00000000000
--- a/contrib/convert-grafts-to-replace-refs.sh
+++ /dev/null
@@ -1,28 +0,0 @@
-#!/bin/sh
-
-# You should execute this script in the repository where you
-# want to convert grafts to replace refs.
-
-GRAFTS_FILE="${GIT_DIR:-.git}/info/grafts"
-
-. $(git --exec-path)/git-sh-setup
-
-test -f "$GRAFTS_FILE" || die "Could not find graft file: '$GRAFTS_FILE'"
-
-grep '^[^# ]' "$GRAFTS_FILE" |
-while read definition
-do
- if test -n "$definition"
- then
- echo "Converting: $definition"
- git replace --graft $definition ||
- die "Conversion failed for: $definition"
- fi
-done
-
-mv "$GRAFTS_FILE" "$GRAFTS_FILE.bak" ||
- die "Could not rename '$GRAFTS_FILE' to '$GRAFTS_FILE.bak'"
-
-echo "Success!"
-echo "All the grafts in '$GRAFTS_FILE' have been converted to replace refs!" -echo "The grafts file '$GRAFTS_FILE' has been renamed: '$GRAFTS_FILE.bak'"
diff --git a/log-tree.c b/log-tree.c
index d1c0bedf244..f3a51a6e726 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -488,9 +488,9 @@ static int is_common_merge(const struct commit *commit)
&& !commit->parents->next->next);
}

-static void show_one_mergetag(struct commit *commit,
- struct commit_extra_header *extra,
- void *data)
+static int show_one_mergetag(struct commit *commit,
+ struct commit_extra_header *extra,
+ void *data)
{
struct rev_info *opt = (struct rev_info *)data;
struct object_id oid;
@@ -502,7 +502,7 @@ static void show_one_mergetag(struct commit *commit,
hash_object_file(extra->value, extra->len, type_name(OBJ_TAG), &oid); tag = lookup_tag(&oid);
if (!tag)
- return; /* error message already given */
+ return -1; /* error message already given */

strbuf_init(&verify_message, 256);
if (parse_tag_buffer(tag, extra->value, extra->len))
@@ -536,11 +536,12 @@ static void show_one_mergetag(struct commit *commit,

show_sig_lines(opt, status, verify_message.buf); strbuf_release(&verify_message);
+ return 0;
}

-static void show_mergetag(struct rev_info *opt, struct commit *commit) +static int show_mergetag(struct rev_info *opt, struct commit *commit)
{
- for_each_mergetag(show_one_mergetag, commit, opt);
+ return for_each_mergetag(show_one_mergetag, commit, opt);
}

void show_log(struct rev_info *opt)
diff --git a/t/t6050-replace.sh b/t/t6050-replace.sh
index 77ded6df653..8a3ee7c3db9 100755
--- a/t/t6050-replace.sh
+++ b/t/t6050-replace.sh
@@ -446,7 +446,7 @@ test_expect_success GPG '--graft on a commit with a mergetag' '

test_expect_success '--convert-graft-file' '
: add and convert graft file &&
- printf "%s\n%s %s\n%s\n" \
+ printf "%s\n%s %s\n\n# comment\n%s\n" \
$(git rev-parse HEAD^^ HEAD^ HEAD^^ HEAD^2) \
>.git/info/grafts &&
git replace --convert-graft-file &&

2.17.0.windows.1.15.gaa56ade3205

This is a simple function that will interpret a string as a whitespace delimited list of values, and add those values into the array.

Note: this function does not (yet) offer to split by arbitrary delimiters, or keep empty values in case of runs of whitespace, or de-quote Unix shell style. All fo this functionality can be added later, when and if needed.

---
 argv-array.c | 20 ++++++++++++++++++++
 argv-array.h |  1 +
 2 files changed, 21 insertions(+)

index 5d370fa3366..cb5bcd2c064 100644
@@ -64,6 +64,26 @@ void argv_array_pop(struct argv_array *array)
 	array->argc--;
 }
 
+void argv_array_split(struct argv_array *array, const char *to_split)
+{
+	while (isspace(*to_split))
+		to_split++;
+	for (;;) {
+		const char *p = to_split;
+
+		if (!*p)
+			break;
+
+		while (*p && !isspace(*p))
+			p++;
+		argv_array_push_nodup(array, xstrndup(to_split, p - to_split));
+
+		while (isspace(*p))
+			p++;
+		to_split = p;
+	}
+}
+
 void argv_array_clear(struct argv_array *array)
 {
 	if (array->argv != empty_argv) {

index 29056e49a12..c7c397695df 100644
@@ -19,6 +19,7 @@ LAST_ARG_MUST_BE_NULL
 void argv_array_pushl(struct argv_array *, ...);
 void argv_array_pushv(struct argv_array *, const char **);
 void argv_array_pop(struct argv_array *);
+void argv_array_split(struct argv_array *, const char *);
 void argv_array_clear(struct argv_array *);
 const char **argv_array_detach(struct argv_array *);
 

2.17.0.windows.1.15.gaa56ade3205

Hi Stefan,

Maybe we could reconcile all of this by introducing yet another layer of indirection? I am wary of this, as I tend to think that layers of indirection make things tedious to debug.

But I could imagine that we could introduce something like

    typedef int (*split_string_fn)(const char *item, size_t len, void *data);
    int split_string(const char *string, int delimiter, int maxsplit,
		     split_string_fn fn, void *fn_data);

And then argv_array_split() would be implemented like this:

    struct argv_array_split_data {
	struct argv_array *array;
    };

    static int argv_array_split_fn(const char *item, size_t len, void *data)
    {
	struct argv_array *array =
	    ((struct argv_array_split_data *)data)->array;
	argv_array_push_nodup(array, xstrndup(item, len));
	return 0;
    }

    void argv_array_split(struct argv_array *array, const char *string,
			  int delimiter, int maxsplit)
    {
	struct argv_array_split_data data;
	data.array = array;
	split_string(string, delimiter, maxsplit, argv_array_split_fn, &data);
    }

Possible? Yes. Desirable? Dunno. Looks like a lot of effort for little gain so far.

That was my thinking.

And I thought further: once argv_array_split() becomes more complex, we would need to consider again whether we want that level of indirection (and consolidation), and then also whether we need a comment.

On the other hand, argv_array's name suggests that it handles command-lines, so I think you are correct that at least a little comment is needed to state that this does not handle quoted arguments.

Ciao,
Dscho

This is yet another patch to be filed under the keyword "libification".

There is one subtle change in behavior here, where a git log that has been asked to show the mergetags would now stop reporting the mergetags upon the first failure, whereas previously, it would have continued to the next mergetag, if any.

In practice, that change should not matter, as it is 1) uncommon to perform octopus merges using multiple tags as merge heads, and 2) when the user asks to be shown those tags, they really should be there.

---
 builtin/replace.c |  8 ++++----
 commit.c          |  8 +++++---
 commit.h          |  4 ++--
 log-tree.c        | 13 +++++++------
 4 files changed, 18 insertions(+), 15 deletions(-)

index 935647be6bd..245d3f4164e 100644
@@ -345,7 +345,7 @@ struct check_mergetag_data {
 	const char **argv;
 };
 
-static void check_one_mergetag(struct commit *commit,
+static int check_one_mergetag(struct commit *commit,
 			       struct commit_extra_header *extra,
 			       void *data)
 {

@@ -368,20 +368,20 @@ static void check_one_mergetag(struct commit *commit,
 		if (get_oid(mergetag_data->argv[i], &oid) < 0)
 			die(_("Not a valid object name: '%s'"), mergetag_data->argv[i]);
 		if (!oidcmp(&tag->tagged->oid, &oid))
-			return; /* found */
+			return 0; /* found */
 	}
 
 	die(_("original commit '%s' contains mergetag '%s' that is discarded; "
 	      "use --edit instead of --graft"), ref, oid_to_hex(&tag_oid));
 }
 
-static void check_mergetags(struct commit *commit, int argc, const char **argv)
+static int check_mergetags(struct commit *commit, int argc, const char **argv)
 {
 	struct check_mergetag_data mergetag_data;
 
 	mergetag_data.argc = argc;
 	mergetag_data.argv = argv;
-	for_each_mergetag(check_one_mergetag, commit, &mergetag_data);
+	return for_each_mergetag(check_one_mergetag, commit, &mergetag_data);
 }
 
 static int create_graft(int argc, const char **argv, int force)

index ca474a7c112..2952ec987c5 100644
@@ -1288,17 +1288,19 @@ struct commit_extra_header *read_commit_extra_headers(struct commit *commit,
 	return extra;
 }
 
-void for_each_mergetag(each_mergetag_fn fn, struct commit *commit, void *data)
+int for_each_mergetag(each_mergetag_fn fn, struct commit *commit, void *data)
 {
 	struct commit_extra_header *extra, *to_free;
+	int res = 0;
 
 	to_free = read_commit_extra_headers(commit, NULL);
-	for (extra = to_free; extra; extra = extra->next) {
+	for (extra = to_free; !res && extra; extra = extra->next) {
 		if (strcmp(extra->key, "mergetag"))
 			continue; /* not a merge tag */
-		fn(commit, extra, data);
+		res = fn(commit, extra, data);
 	}
 	free_commit_extra_headers(to_free);
+	return res;
 }
 
 static inline int standard_header_field(const char *field, size_t len)

index 0fb8271665c..9000895ad91 100644
@@ -291,10 +291,10 @@ extern const char *find_commit_header(const char *msg, const char *key,
 /* Find the end of the log message, the right place for a new trailer. */
 extern int ignore_non_trailer(const char *buf, size_t len);
 
-typedef void (*each_mergetag_fn)(struct commit *commit, struct commit_extra_header *extra,
+typedef int (*each_mergetag_fn)(struct commit *commit, struct commit_extra_header *extra,
 				 void *cb_data);
 
-extern void for_each_mergetag(each_mergetag_fn fn, struct commit *commit, void *data);
+extern int for_each_mergetag(each_mergetag_fn fn, struct commit *commit, void *data);
 
 struct merge_remote_desc {
 	struct object *obj; /* the named object, could be a tag */

index d1c0bedf244..f3a51a6e726 100644
@@ -488,9 +488,9 @@ static int is_common_merge(const struct commit *commit)
 		&& !commit->parents->next->next);
 }
 
-static void show_one_mergetag(struct commit *commit,
-			      struct commit_extra_header *extra,
-			      void *data)
+static int show_one_mergetag(struct commit *commit,
+			     struct commit_extra_header *extra,
+			     void *data)
 {
 	struct rev_info *opt = (struct rev_info *)data;
 	struct object_id oid;

@@ -502,7 +502,7 @@ static void show_one_mergetag(struct commit *commit,
 	hash_object_file(extra->value, extra->len, type_name(OBJ_TAG), &oid);
 	tag = lookup_tag(&oid);
 	if (!tag)
-		return; /* error message already given */
+		return -1; /* error message already given */
 
 	strbuf_init(&verify_message, 256);
 	if (parse_tag_buffer(tag, extra->value, extra->len))

@@ -536,11 +536,12 @@ static void show_one_mergetag(struct commit *commit,
 
 	show_sig_lines(opt, status, verify_message.buf);
 	strbuf_release(&verify_message);
+	return 0;
 }
 
-static void show_mergetag(struct rev_info *opt, struct commit *commit)
+static int show_mergetag(struct rev_info *opt, struct commit *commit)
 {
-	for_each_mergetag(show_one_mergetag, commit, opt);
+	return for_each_mergetag(show_one_mergetag, commit, opt);
 }
 
 void show_log(struct rev_info *opt)

2.17.0.windows.1.15.gaa56ade3205

We have the BUG() macro for that purpose.

---
 builtin/replace.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 245d3f4164e..e345a5a0f1c 100644
@@ -501,6 +501,6 @@ int cmd_replace(int argc, const char **argv, const char *prefix)
 		return list_replace_refs(argv[0], format);
 
 	default:
-		die("BUG: invalid cmdmode %d", (int)cmdmode);
+		BUG("invalid cmdmode %d", (int)cmdmode);
 	}
 }

2.17.0.windows.1.15.gaa56ade3205

File this away as yet another patch in the "libification" category.

As with all useful functions, in the next commit we want to use create_graft() from a higher-level function where it would be inconvenient if the called function simply die()s: if there is a problem, we want to let the user know how to proceed, and the callee simply has no way of knowing what to say.

---
 builtin/replace.c | 135 ++++++++++++++++++++++++++++------------------
 1 file changed, 84 insertions(+), 51 deletions(-)

index e345a5a0f1c..f63df405fd0 100644
@@ -79,9 +79,9 @@ static int list_replace_refs(const char *pattern, const char *format)
 	else if (!strcmp(format, "long"))
 		data.format = REPLACE_FORMAT_LONG;
 	else
-		die("invalid replace format '%s'\n"
-		    "valid formats are 'short', 'medium' and 'long'\n",
-		    format);
+		return error("invalid replace format '%s'\n"
+			     "valid formats are 'short', 'medium' and 'long'\n",
+			     format);
 
 	for_each_replace_ref(show_reference, (void *)&data);
 

@@ -134,7 +134,7 @@ static int delete_replace_ref(const char *name, const char *ref,
 	return 0;
 }
 
-static void check_ref_valid(struct object_id *object,
+static int check_ref_valid(struct object_id *object,
 			    struct object_id *prev,
 			    struct strbuf *ref,
 			    int force)

@@ -142,12 +142,13 @@ static void check_ref_valid(struct object_id *object,
 	strbuf_reset(ref);
 	strbuf_addf(ref, "%s%s", git_replace_ref_base, oid_to_hex(object));
 	if (check_refname_format(ref->buf, 0))
-		die("'%s' is not a valid ref name.", ref->buf);
+		return error("'%s' is not a valid ref name.", ref->buf);
 
 	if (read_ref(ref->buf, prev))
 		oidclr(prev);
 	else if (!force)
-		die("replace ref '%s' already exists", ref->buf);
+		return error("replace ref '%s' already exists", ref->buf);
+	return 0;
 }
 
 static int replace_object_oid(const char *object_ref,

@@ -161,28 +162,31 @@ static int replace_object_oid(const char *object_ref,
 	struct strbuf ref = STRBUF_INIT;
 	struct ref_transaction *transaction;
 	struct strbuf err = STRBUF_INIT;
+	int res = 0;
 
 	obj_type = oid_object_info(object, NULL);
 	repl_type = oid_object_info(repl, NULL);
 	if (!force && obj_type != repl_type)
-		die("Objects must be of the same type.\n"
-		    "'%s' points to a replaced object of type '%s'\n"
-		    "while '%s' points to a replacement object of type '%s'.",
-		    object_ref, type_name(obj_type),
-		    replace_ref, type_name(repl_type));
+		return error("Objects must be of the same type.\n"
+			     "'%s' points to a replaced object of type '%s'\n"
+			     "while '%s' points to a replacement object of "
+			     "type '%s'.",
+			     object_ref, type_name(obj_type),
+			     replace_ref, type_name(repl_type));
 
-	check_ref_valid(object, &prev, &ref, force);
+	if (check_ref_valid(object, &prev, &ref, force))
+		return -1;
 
 	transaction = ref_transaction_begin(&err);
 	if (!transaction ||
 	    ref_transaction_update(transaction, ref.buf, repl, &prev,
 				   0, NULL, &err) ||
 	    ref_transaction_commit(transaction, &err))
-		die("%s", err.buf);
+		res = error("%s", err.buf);
 
 	ref_transaction_free(transaction);
 	strbuf_release(&ref);
-	return 0;
+	return res;
 }
 
 static int replace_object(const char *object_ref, const char *replace_ref, int force)

@@ -190,9 +194,11 @@ static int replace_object(const char *object_ref, const char *replace_ref, int f
 	struct object_id object, repl;
 
 	if (get_oid(object_ref, &object))
-		die("Failed to resolve '%s' as a valid ref.", object_ref);
+		return error("Failed to resolve '%s' as a valid ref.",
+			     object_ref);
 	if (get_oid(replace_ref, &repl))
-		die("Failed to resolve '%s' as a valid ref.", replace_ref);
+		return error("Failed to resolve '%s' as a valid ref.",
+			     replace_ref);
 
 	return replace_object_oid(object_ref, &object, replace_ref, &repl, force);
 }

@@ -202,7 +208,7 @@ static int replace_object(const char *object_ref, const char *replace_ref, int f
  * If "raw" is true, then the object's raw contents are printed according to
  * "type". Otherwise, we pretty-print the contents for human editing.
  */
-static void export_object(const struct object_id *oid, enum object_type type,
+static int export_object(const struct object_id *oid, enum object_type type,
 			  int raw, const char *filename)
 {
 	struct child_process cmd = CHILD_PROCESS_INIT;

@@ -210,7 +216,7 @@ static void export_object(const struct object_id *oid, enum object_type type,
 
 	fd = open(filename, O_WRONLY | O_CREAT | O_TRUNC, 0666);
 	if (fd < 0)
-		die_errno("unable to open %s for writing", filename);
+		return error_errno("unable to open %s for writing", filename);
 
 	argv_array_push(&cmd.args, "--no-replace-objects");
 	argv_array_push(&cmd.args, "cat-file");

@@ -223,7 +229,8 @@ static void export_object(const struct object_id *oid, enum object_type type,
 	cmd.out = fd;
 
 	if (run_command(&cmd))
-		die("cat-file reported failure");
+		return error("cat-file reported failure");
+	return 0;
 }
 
 /*

@@ -231,14 +238,14 @@ static void export_object(const struct object_id *oid, enum object_type type,
  * interpreting it as "type", and writing the result to the object database.
  * The sha1 of the written object is returned via sha1.
  */
-static void import_object(struct object_id *oid, enum object_type type,
+static int import_object(struct object_id *oid, enum object_type type,
 			  int raw, const char *filename)
 {
 	int fd;
 
 	fd = open(filename, O_RDONLY);
 	if (fd < 0)
-		die_errno("unable to open %s for reading", filename);
+		return error_errno("unable to open %s for reading", filename);
 
 	if (!raw && type == OBJ_TREE) {
 		const char *argv[] = { "mktree", NULL };

@@ -250,27 +257,38 @@ static void import_object(struct object_id *oid, enum object_type type,
 		cmd.in = fd;
 		cmd.out = -1;
 
-		if (start_command(&cmd))
-			die("unable to spawn mktree");
+		if (start_command(&cmd)) {
+			close(fd);
+			return error("unable to spawn mktree");
+		}
 
-		if (strbuf_read(&result, cmd.out, 41) < 0)
-			die_errno("unable to read from mktree");
+		if (strbuf_read(&result, cmd.out, 41) < 0) {
+			close(fd);
+			close(cmd.out);
+			return error_errno("unable to read from mktree");
+		}
 		close(cmd.out);
 
-		if (finish_command(&cmd))
-			die("mktree reported failure");
-		if (get_oid_hex(result.buf, oid) < 0)
-			die("mktree did not return an object name");
+		if (finish_command(&cmd)) {
+			strbuf_release(&result);
+			return error("mktree reported failure");
+		}
+		if (get_oid_hex(result.buf, oid) < 0) {
+			strbuf_release(&result);
+			return error("mktree did not return an object name");
+		}
 
 		strbuf_release(&result);
 	} else {
 		struct stat st;
 		int flags = HASH_FORMAT_CHECK | HASH_WRITE_OBJECT;
 
-		if (fstat(fd, &st) < 0)
-			die_errno("unable to fstat %s", filename);
+		if (fstat(fd, &st) < 0) {
+			close(fd);
+			return error_errno("unable to fstat %s", filename);
+		}
 		if (index_fd(oid, fd, &st, type, NULL, flags) < 0)
-			die("unable to write object to database");
+			return error("unable to write object to database");
 		/* index_fd close()s fd for us */
 	}
 

@@ -278,6 +296,7 @@ static void import_object(struct object_id *oid, enum object_type type,
 	 * No need to close(fd) here; both run-command and index-fd
 	 * will have done it for us.
 	 */
+	return 0;
 }
 
 static int edit_and_replace(const char *object_ref, int force, int raw)

@@ -288,19 +307,23 @@ static int edit_and_replace(const char *object_ref, int force, int raw)
 	struct strbuf ref = STRBUF_INIT;
 
 	if (get_oid(object_ref, &old_oid) < 0)
-		die("Not a valid object name: '%s'", object_ref);
+		return error("Not a valid object name: '%s'", object_ref);
 
 	type = oid_object_info(&old_oid, NULL);
 	if (type < 0)
-		die("unable to get object type for %s", oid_to_hex(&old_oid));
+		return error("unable to get object type for %s",
+			     oid_to_hex(&old_oid));
 
-	check_ref_valid(&old_oid, &prev, &ref, force);
+	if (check_ref_valid(&old_oid, &prev, &ref, force))
+		return -1;
 	strbuf_release(&ref);
 
-	export_object(&old_oid, type, raw, tmpfile);
+	if (export_object(&old_oid, type, raw, tmpfile))
+		return -1;
 	if (launch_editor(tmpfile, NULL, NULL) < 0)
-		die("editing object file failed");
-	import_object(&new_oid, type, raw, tmpfile);
+		return error("editing object file failed");
+	if (import_object(&new_oid, type, raw, tmpfile))
+		return -1;
 
 	free(tmpfile);
 

@@ -310,7 +333,7 @@ static int edit_and_replace(const char *object_ref, int force, int raw)
 	return replace_object_oid(object_ref, &old_oid, "replacement", &new_oid, force);
 }
 
-static void replace_parents(struct strbuf *buf, int argc, const char **argv)
+static int replace_parents(struct strbuf *buf, int argc, const char **argv)
 {
 	struct strbuf new_parents = STRBUF_INIT;
 	const char *parent_start, *parent_end;

@@ -328,8 +351,10 @@ static void replace_parents(struct strbuf *buf, int argc, const char **argv)
 	for (i = 0; i < argc; i++) {
 		struct object_id oid;
 		if (get_oid(argv[i], &oid) < 0)
-			die(_("Not a valid object name: '%s'"), argv[i]);
-		lookup_commit_or_die(&oid, argv[i]);
+			return error(_("Not a valid object name: '%s'"),
+				     argv[i]);
+		if (!lookup_commit_reference(&oid))
+			return error(_("could not parse %s"), argv[i]);
 		strbuf_addf(&new_parents, "parent %s\n", oid_to_hex(&oid));
 	}
 

@@ -338,6 +363,7 @@ static void replace_parents(struct strbuf *buf, int argc, const char **argv)
 		      new_parents.buf, new_parents.len);
 
 	strbuf_release(&new_parents);
+	return 0;
 }
 
 struct check_mergetag_data {

@@ -358,21 +384,23 @@ static int check_one_mergetag(struct commit *commit,
 	hash_object_file(extra->value, extra->len, type_name(OBJ_TAG), &tag_oid);
 	tag = lookup_tag(&tag_oid);
 	if (!tag)
-		die(_("bad mergetag in commit '%s'"), ref);
+		return error(_("bad mergetag in commit '%s'"), ref);
 	if (parse_tag_buffer(tag, extra->value, extra->len))
-		die(_("malformed mergetag in commit '%s'"), ref);
+		return error(_("malformed mergetag in commit '%s'"), ref);
 
 	/* iterate over new parents */
 	for (i = 1; i < mergetag_data->argc; i++) {
 		struct object_id oid;
 		if (get_oid(mergetag_data->argv[i], &oid) < 0)
-			die(_("Not a valid object name: '%s'"), mergetag_data->argv[i]);
+			return error(_("Not a valid object name: '%s'"),
+				     mergetag_data->argv[i]);
 		if (!oidcmp(&tag->tagged->oid, &oid))
 			return 0; /* found */
 	}
 
-	die(_("original commit '%s' contains mergetag '%s' that is discarded; "
-	      "use --edit instead of --graft"), ref, oid_to_hex(&tag_oid));
+	return error(_("original commit '%s' contains mergetag '%s' that is "
+		       "discarded; use --edit instead of --graft"), ref,
+		     oid_to_hex(&tag_oid));
 }
 
 static int check_mergetags(struct commit *commit, int argc, const char **argv)

@@ -394,24 +422,29 @@ static int create_graft(int argc, const char **argv, int force)
 	unsigned long size;
 
 	if (get_oid(old_ref, &old_oid) < 0)
-		die(_("Not a valid object name: '%s'"), old_ref);
-	commit = lookup_commit_or_die(&old_oid, old_ref);
+		return error(_("Not a valid object name: '%s'"), old_ref);
+	commit = lookup_commit_reference(&old_oid);
+	if (!commit)
+		return error(_("could not parse %s"), old_ref);
 
 	buffer = get_commit_buffer(commit, &size);
 	strbuf_add(&buf, buffer, size);
 	unuse_commit_buffer(commit, buffer);
 
-	replace_parents(&buf, argc - 1, &argv[1]);
+	if (replace_parents(&buf, argc - 1, &argv[1]) < 0)
+		return -1;
 
 	if (remove_signature(&buf)) {
 		warning(_("the original commit '%s' has a gpg signature."), old_ref);
 		warning(_("the signature will be removed in the replacement commit!"));
 	}
 
-	check_mergetags(commit, argc, argv);
+	if (check_mergetags(commit, argc, argv))
+		return -1;
 
 	if (write_object_file(buf.buf, buf.len, commit_type, &new_oid))
-		die(_("could not write replacement commit for: '%s'"), old_ref);
+		return error(_("could not write replacement commit for: '%s'"),
+			     old_ref);
 
 	strbuf_release(&buf);
 

2.17.0.windows.1.15.gaa56ade3205

This option is intended to help with the transition away from the now-deprecated graft file.

---
 Documentation/git-replace.txt | 11 ++++++---
 builtin/replace.c             | 44 ++++++++++++++++++++++++++++++++++-
 2 files changed, 51 insertions(+), 4 deletions(-)

index e5c57ae6ef4..246dc9943c2 100644
@@ -11,6 +11,7 @@ SYNOPSIS
 'git replace' [-f] <object> <replacement>
 'git replace' [-f] --edit <object>
 'git replace' [-f] --graft <commit> [<parent>...]
+'git replace' [-f] --convert-graft-file
 'git replace' -d <object>...
 'git replace' [--format=<format>] [-l [<pattern>]]
 

@@ -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 and use replace refs instead.
+
+--convert-graft-file::
+	Creates graft commits for all entries in `$GIT_DIR/info/grafts`
+	and deletes that file upon success. The purpose is to help users
+	with transitioning off of the now-deprecated graft file.
 
 -l <pattern>::
 --list <pattern>::

2.17.0.windows.1.15.gaa56ade3205

The proof, as the saying goes, lies in the pudding. So here is a regression test that not only demonstrates what the option is supposed to accomplish, but also demonstrates that it does accomplish it.

---
 t/t6050-replace.sh | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

index c630aba657e..8a3ee7c3db9 100755
@@ -444,4 +444,24 @@ test_expect_success GPG '--graft on a commit with a mergetag' '
 	git replace -d $HASH10
 '
 
+test_expect_success '--convert-graft-file' '
+	: add and convert graft file &&
+	printf "%s\n%s %s\n\n# comment\n%s\n" \
+		$(git rev-parse HEAD^^ HEAD^ HEAD^^ HEAD^2) \
+		>.git/info/grafts &&
+	git replace --convert-graft-file &&
+	test_path_is_missing .git/info/grafts &&
+
+	: verify that the history is now "grafted" &&
+	git rev-list HEAD >out &&
+	test_line_count = 4 out &&
+
+	: create invalid graft file and verify that it is not deleted &&
+	test_when_finished "rm -f .git/info/grafts" &&
+	echo $EMPTY_BLOB $EMPTY_TREE >.git/info/grafts &&
+	test_must_fail git replace --convert-graft-file 2>err &&
+	grep "$EMPTY_BLOB $EMPTY_TREE" err &&
+	grep "$EMPTY_BLOB $EMPTY_TREE" .git/info/grafts
+'
+
 test_done

2.17.0.windows.1.15.gaa56ade3205

Hi Gábor,

Sure enough, you're right! I thought it would not be translated, what with being an parameter to a formatted string...

Will fix,
Dscho

The grafts feature was a convenient way to "stitch together" ancient history to the fresh start of linux.git.

Its implementation is, however, not up to Git's standards, as there are too many ways where it can lead to surprising and unwelcome behavior.

For example, when pushing from a repository with active grafts, it is possible to miss commits that have been "grafted out", resulting in a broken state on the other side.

Also, the grafts feature is limited to "rewriting" commits' list of parents, it cannot replace anything else.

The much younger feature implemented as git replace set out to remedy those limitations and dangerous bugs.

Seeing as git replace is pretty mature by now (since 4228e8bc98 (replace: add --graft option, 2014-07-19) it can perform the graft file's duties), it is time to deprecate support for the graft file, and to retire it eventually.

---
 advice.c                  |  2 ++
 advice.h                  |  1 +
 commit.c                  | 10 ++++++++++
 t/t6001-rev-list-graft.sh |  9 +++++++++
 4 files changed, 22 insertions(+)

index 406efc183ba..4411704fd45 100644
@@ -19,6 +19,7 @@ int advice_rm_hints = 1;
 int advice_add_embedded_repo = 1;
 int advice_ignored_hook = 1;
 int advice_waiting_for_editor = 1;
+int advice_graft_file_deprecated = 1;
 
 static struct {
 	const char *name;

@@ -42,6 +43,7 @@ static struct {
 	{ "addembeddedrepo", &advice_add_embedded_repo },
 	{ "ignoredhook", &advice_ignored_hook },
 	{ "waitingforeditor", &advice_waiting_for_editor },
+	{ "graftfiledeprecated", &advice_graft_file_deprecated },
 
 	/* make this an alias for backward compatibility */
 	{ "pushnonfastforward", &advice_push_update_rejected }

index 70568fa7922..9f5064e82a8 100644
@@ -21,6 +21,7 @@ extern int advice_rm_hints;
 extern int advice_add_embedded_repo;
 extern int advice_ignored_hook;
 extern int advice_waiting_for_editor;
+extern int advice_graft_file_deprecated;
 
 int git_default_advice_config(const char *var, const char *value);
 __attribute__((format (printf, 1, 2)))

index 2952ec987c5..451d3ce8dfe 100644
@@ -12,6 +12,7 @@
 #include "prio-queue.h"
 #include "sha1-lookup.h"
 #include "wt-status.h"
+#include "advice.h"
 
 static struct commit_extra_header *read_commit_extra_header_lines(const char *buf, size_t len, const char **);
 

@@ -176,6 +177,15 @@ static int read_graft_file(const char *graft_file)
 	struct strbuf buf = STRBUF_INIT;
 	if (!fp)
 		return -1;
+	if (advice_graft_file_deprecated)
+		advise(_("Support for <GIT_DIR>/info/grafts is deprecated\n"
+			 "and will be removed in a future Git version.\n"
+			 "\n"
+			 "Please use \"git replace --convert-graft-file\"\n"
+			 "to convert the grafts into replace refs.\n"
+			 "\n"
+			 "Turn this message off by running\n"
+			 "\"git config advice.graftFileDeprecated false\""));
 	while (!strbuf_getwholeline(&buf, fp, '\n')) {
 		/* The format is just "Commit Parent1 Parent2 ...\n" */
 		struct commit_graft *graft = read_graft_line(&buf);

index 05ddc69cf2a..7504ba47511 100755
@@ -110,4 +110,13 @@ do
 	"
 
 done
+
+test_expect_success 'show advice that grafts are deprecated' '
+	git show HEAD 2>err &&
+	test_i18ngrep "git replace" err &&
+	test_config advice.graftFileDeprecated false &&
+	git show HEAD 2>err &&
+	test_i18ngrep ! "git replace" err
+'
+
 test_done

2.17.0.windows.1.15.gaa56ade3205

The graft file is deprecated now, so let's use replace refs in the example in filter-branch's man page instead.

---
 Documentation/git-filter-branch.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index b634043183b..1d4d2f86045 100644
@@ -288,7 +288,7 @@ git filter-branch --parent-filter \
 or even simpler:
 
 -----------------------------------------------
-echo "$commit-id $graft-id" >> .git/info/grafts
+git replace --graft $commit-id $graft-id
 git filter-branch $graft-id..HEAD
 -----------------------------------------------
 

2.17.0.windows.1.15.gaa56ade3205

Now that grafts are deprecated, we should start to assume that readers have no idea what grafts are. So it makes more sense to describe the "shallow" feature in terms of replace refs.

---
 Documentation/technical/shallow.txt | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

index 5183b154229..b3ff23c25f6 100644
@@ -9,14 +9,17 @@ these commits have no parents.
 *********************************************************
 
 The basic idea is to write the SHA-1s of shallow commits into
-$GIT_DIR/shallow, and handle its contents like the contents
-of $GIT_DIR/info/grafts (with the difference that shallow
-cannot contain parent information).
-
-This information is stored in a new file instead of grafts, or
-even the config, since the user should not touch that file
-at all (even throughout development of the shallow clone, it
-was never manually edited!).
+$GIT_DIR/shallow, and handle its contents similar to replace
+refs (with the difference that shallow does not actually
+create those replace refs) and very much like the deprecated
+graft file (with the difference that shallow commits will
+always have their parents grafted away, not replaced by
+different parents).
+
+This information is stored in a special-purpose file because the
+user should not touch that file at all (even throughout
+development of the shallow clone, it was never manually
+edited!).
 
 Each line contains exactly one SHA-1. When read, a commit_graft
 will be constructed, which has nr_parent < 0 to make it easier

2.17.0.windows.1.15.gaa56ade3205

From: "Johannes Schindelin" <[email protected]>

Hi Philip,

Internally, shallow commits are implemented using the graft code path, and they always will be: we will always need a list of the shallow commits, and we will always need to be able to lift the "shallow" attribute quickly, when deepening a shallow clone.

So it makes sense to mention that here, because we are deep in technical details in Documentation/technical/.

But the commits will typically have parents. So they really will have their parents grafted away as long as they are marked "shallow"...

Thank you for reviewing!
Dscho

Hi dscho

From: "Johannes Schindelin" <[email protected]> : Tuesday, April 24, 2018 8:10 PM

Here we say we should drop the term "grafts"

however the change here is just to the documentation, independent of th code path's name.

I was looking to snip this 'graft' reference, as per the commit message..

Then I botched this substitution

OK, maybe I mis-used the figurative 'no parents', when it means the literal 'parents not present'.

Perhaps something like:
+$GIT_DIR/shallow, and handle its contents similar to replace +refs (with the difference that shallow does not actually +create those replace refs) with the difference that shallow commits will +always have their parents not present.

--
Philip

Hi Philip & Junio,

Good point. Thank you for that suggestion, I'll take it as-is.

Ciao,
Dscho

It is tempting to do away with commit_graft altogether (in the long haul), now that grafts are deprecated.

However, the shallow feature needs a couple of things that the replace refs cannot fulfill. Let's point that out in the documentation.

---
 Documentation/technical/shallow.txt | 7 +++++++
 1 file changed, 7 insertions(+)

index b3ff23c25f6..cb79181c2bb 100644
@@ -25,6 +25,13 @@ Each line contains exactly one SHA-1. When read, a commit_graft
 will be constructed, which has nr_parent < 0 to make it easier
 to discern from user provided grafts.
 
+Note that the shallow feature could not be changed easily to
+use replace refs: a commit containing a `mergetag` is not allowed
+to be replaced, not even by a root commit. Such a commit can be
+made shallow, though. Also, having a `shallow` file explicitly
+listing all the commits made shallow makes it a *lot* easier to
+do shallow-specific things such as to deepen the history.
+
 Since fsck-objects relies on the library to read the objects,
 it honours shallow commits automatically.
 

2.17.0.windows.1.15.gaa56ade3205

The functionality is now implemented as git replace --convert-graft-file.

---
 contrib/convert-grafts-to-replace-refs.sh | 28 -----------------------
 1 file changed, 28 deletions(-)
 delete mode 100755 contrib/convert-grafts-to-replace-refs.sh

deleted file mode 100755
index 0cbc917b8cf..00000000000
@@ -1,28 +0,0 @@
-#!/bin/sh
-
-# You should execute this script in the repository where you
-# want to convert grafts to replace refs.
-
-GRAFTS_FILE="${GIT_DIR:-.git}/info/grafts"
-
-. $(git --exec-path)/git-sh-setup
-
-test -f "$GRAFTS_FILE" || die "Could not find graft file: '$GRAFTS_FILE'"
-
-grep '^[^# ]' "$GRAFTS_FILE" |
-while read definition
-do
-	if test -n "$definition"
-	then
-		echo "Converting: $definition"
-		git replace --graft $definition ||
-			die "Conversion failed for: $definition"
-	fi
-done
-
-mv "$GRAFTS_FILE" "$GRAFTS_FILE.bak" ||
-	die "Could not rename '$GRAFTS_FILE' to '$GRAFTS_FILE.bak'"
-
-echo "Success!"
-echo "All the grafts in '$GRAFTS_FILE' have been converted to replace refs!"
-echo "The grafts file '$GRAFTS_FILE' has been renamed: '$GRAFTS_FILE.bak'"

2.17.0.windows.1.15.gaa56ade3205

It is fragile, as there is no way for the revision machinery to say "but now I want to traverse the graph ignoring the graft file" e.g. when pushing commits to a remote repository (which, as a consequence, can miss commits).

And we already have a better solution with git replace --graft <comit> [<parent>...].

Changes since v3:

- The argv_array_split() declaration now has a clear comment indicating that it does not perform any "un-quoting" but goes purely by whitespace.

- Fixed t6050 under GETTEXT_POISON.

Johannes Schindelin (11):
argv_array: offer to split a string by whitespace
commit: Let the callback of for_each_mergetag return on error
replace: avoid using die() to indicate a bug
replace: "libify" create_graft() and callees
replace: introduce --convert-graft-file
Add a test for git replace --convert-graft-file Deprecate support for .git/info/grafts
filter-branch: stop suggesting to use grafts technical/shallow: describe the relationship with replace refs technical/shallow: describe why shallow cannot use replace refs Remove obsolete script to convert grafts to replace refs

 Documentation/git-filter-branch.txt       |   2 +-
 Documentation/git-replace.txt             |  11 +-
 Documentation/technical/shallow.txt       |  24 ++-
 advice.c                                  |   2 +
 advice.h                                  |   1 +
 argv-array.c                              |  20 +++
 argv-array.h                              |   2 +
 builtin/replace.c                         | 189 +++++++++++++++-------
 commit.c                                  |  18 ++-
 commit.h                                  |   4 +-
 contrib/convert-grafts-to-replace-refs.sh |  28 ----
 log-tree.c                                |  13 +-
 t/t6001-rev-list-graft.sh                 |   9 ++
 t/t6050-replace.sh                        |  20 +++
 14 files changed, 236 insertions(+), 107 deletions(-)
 delete mode 100755 contrib/convert-grafts-to-replace-refs.sh

base-commit: fe0a9eaf31dd0c349ae4308498c33a5c3794b293
Published-As: https://github.com/dscho/git/releases/tag/deprecate-grafts-v4
Fetch-It-Via: git fetch https://github.com/dscho/git deprecate-grafts-v4

Interdiff vs v3:
diff --git a/argv-array.h b/argv-array.h
index c7c397695df..750c30d2f2c 100644
--- a/argv-array.h
+++ b/argv-array.h
@@ -19,6 +19,7 @@ LAST_ARG_MUST_BE_NULL
void argv_array_pushl(struct argv_array *, ...);
void argv_array_pushv(struct argv_array *, const char **); void argv_array_pop(struct argv_array *);
+/* Splits by whitespace; does not handle quoted arguments! */
void argv_array_split(struct argv_array *, const char *); void argv_array_clear(struct argv_array *);
const char **argv_array_detach(struct argv_array *);
diff --git a/t/t6050-replace.sh b/t/t6050-replace.sh index 8a3ee7c3db9..bed86a0af3d 100755
--- a/t/t6050-replace.sh
+++ b/t/t6050-replace.sh
@@ -460,8 +460,8 @@ test_expect_success '--convert-graft-file' '
test_when_finished "rm -f .git/info/grafts" && echo $EMPTY_BLOB $EMPTY_TREE >.git/info/grafts && test_must_fail git replace --convert-graft-file 2>err &&
- grep "$EMPTY_BLOB $EMPTY_TREE" err &&
- grep "$EMPTY_BLOB $EMPTY_TREE" .git/info/grafts
+ test_i18ngrep "$EMPTY_BLOB $EMPTY_TREE" err &&
+ test_i18ngrep "$EMPTY_BLOB $EMPTY_TREE" .git/info/grafts
'

test_done

2.17.0.windows.1.15.gaa56ade3205

This is a simple function that will interpret a string as a whitespace delimited list of values, and add those values into the array.

Note: this function does not (yet) offer to split by arbitrary delimiters, or keep empty values in case of runs of whitespace, or de-quote Unix shell style. All fo this functionality can be added later, when and if needed.

---
 argv-array.c | 20 ++++++++++++++++++++
 argv-array.h |  2 ++
 2 files changed, 22 insertions(+)

index 5d370fa3366..cb5bcd2c064 100644
@@ -64,6 +64,26 @@ void argv_array_pop(struct argv_array *array)
 	array->argc--;
 }
 
+void argv_array_split(struct argv_array *array, const char *to_split)
+{
+	while (isspace(*to_split))
+		to_split++;
+	for (;;) {
+		const char *p = to_split;
+
+		if (!*p)
+			break;
+
+		while (*p && !isspace(*p))
+			p++;
+		argv_array_push_nodup(array, xstrndup(to_split, p - to_split));
+
+		while (isspace(*p))
+			p++;
+		to_split = p;
+	}
+}
+
 void argv_array_clear(struct argv_array *array)
 {
 	if (array->argv != empty_argv) {

index 29056e49a12..750c30d2f2c 100644
@@ -19,6 +19,8 @@ LAST_ARG_MUST_BE_NULL
 void argv_array_pushl(struct argv_array *, ...);
 void argv_array_pushv(struct argv_array *, const char **);
 void argv_array_pop(struct argv_array *);
+/* Splits by whitespace; does not handle quoted arguments! */
+void argv_array_split(struct argv_array *, const char *);
 void argv_array_clear(struct argv_array *);
 const char **argv_array_detach(struct argv_array *);
 

2.17.0.windows.1.15.gaa56ade3205

My thinko. We do want to stop on NUL even though our isspace() says NUL is not a space. And using while (isspace(*p)) below without checking for NUL also makes sense.

Sorry for the noise.

This is yet another patch to be filed under the keyword "libification".

There is one subtle change in behavior here, where a git log that has been asked to show the mergetags would now stop reporting the mergetags upon the first failure, whereas previously, it would have continued to the next mergetag, if any.

In practice, that change should not matter, as it is 1) uncommon to perform octopus merges using multiple tags as merge heads, and 2) when the user asks to be shown those tags, they really should be there.

---
 builtin/replace.c |  8 ++++----
 commit.c          |  8 +++++---
 commit.h          |  4 ++--
 log-tree.c        | 13 +++++++------
 4 files changed, 18 insertions(+), 15 deletions(-)

index 935647be6bd..245d3f4164e 100644
@@ -345,7 +345,7 @@ struct check_mergetag_data {
 	const char **argv;
 };
 
-static void check_one_mergetag(struct commit *commit,
+static int check_one_mergetag(struct commit *commit,
 			       struct commit_extra_header *extra,
 			       void *data)
 {

@@ -368,20 +368,20 @@ static void check_one_mergetag(struct commit *commit,
 		if (get_oid(mergetag_data->argv[i], &oid) < 0)
 			die(_("Not a valid object name: '%s'"), mergetag_data->argv[i]);
 		if (!oidcmp(&tag->tagged->oid, &oid))
-			return; /* found */
+			return 0; /* found */
 	}
 
 	die(_("original commit '%s' contains mergetag '%s' that is discarded; "
 	      "use --edit instead of --graft"), ref, oid_to_hex(&tag_oid));
 }
 
-static void check_mergetags(struct commit *commit, int argc, const char **argv)
+static int check_mergetags(struct commit *commit, int argc, const char **argv)
 {
 	struct check_mergetag_data mergetag_data;
 
 	mergetag_data.argc = argc;
 	mergetag_data.argv = argv;
-	for_each_mergetag(check_one_mergetag, commit, &mergetag_data);
+	return for_each_mergetag(check_one_mergetag, commit, &mergetag_data);
 }
 
 static int create_graft(int argc, const char **argv, int force)

index ca474a7c112..2952ec987c5 100644
@@ -1288,17 +1288,19 @@ struct commit_extra_header *read_commit_extra_headers(struct commit *commit,
 	return extra;
 }
 
-void for_each_mergetag(each_mergetag_fn fn, struct commit *commit, void *data)
+int for_each_mergetag(each_mergetag_fn fn, struct commit *commit, void *data)
 {
 	struct commit_extra_header *extra, *to_free;
+	int res = 0;
 
 	to_free = read_commit_extra_headers(commit, NULL);
-	for (extra = to_free; extra; extra = extra->next) {
+	for (extra = to_free; !res && extra; extra = extra->next) {
 		if (strcmp(extra->key, "mergetag"))
 			continue; /* not a merge tag */
-		fn(commit, extra, data);
+		res = fn(commit, extra, data);
 	}
 	free_commit_extra_headers(to_free);
+	return res;
 }
 
 static inline int standard_header_field(const char *field, size_t len)

index 0fb8271665c..9000895ad91 100644
@@ -291,10 +291,10 @@ extern const char *find_commit_header(const char *msg, const char *key,
 /* Find the end of the log message, the right place for a new trailer. */
 extern int ignore_non_trailer(const char *buf, size_t len);
 
-typedef void (*each_mergetag_fn)(struct commit *commit, struct commit_extra_header *extra,
+typedef int (*each_mergetag_fn)(struct commit *commit, struct commit_extra_header *extra,
 				 void *cb_data);
 
-extern void for_each_mergetag(each_mergetag_fn fn, struct commit *commit, void *data);
+extern int for_each_mergetag(each_mergetag_fn fn, struct commit *commit, void *data);
 
 struct merge_remote_desc {
 	struct object *obj; /* the named object, could be a tag */

index d1c0bedf244..f3a51a6e726 100644
@@ -488,9 +488,9 @@ static int is_common_merge(const struct commit *commit)
 		&& !commit->parents->next->next);
 }
 
-static void show_one_mergetag(struct commit *commit,
-			      struct commit_extra_header *extra,
-			      void *data)
+static int show_one_mergetag(struct commit *commit,
+			     struct commit_extra_header *extra,
+			     void *data)
 {
 	struct rev_info *opt = (struct rev_info *)data;
 	struct object_id oid;

@@ -502,7 +502,7 @@ static void show_one_mergetag(struct commit *commit,
 	hash_object_file(extra->value, extra->len, type_name(OBJ_TAG), &oid);
 	tag = lookup_tag(&oid);
 	if (!tag)
-		return; /* error message already given */
+		return -1; /* error message already given */
 
 	strbuf_init(&verify_message, 256);
 	if (parse_tag_buffer(tag, extra->value, extra->len))

@@ -536,11 +536,12 @@ static void show_one_mergetag(struct commit *commit,
 
 	show_sig_lines(opt, status, verify_message.buf);
 	strbuf_release(&verify_message);
+	return 0;
 }
 
-static void show_mergetag(struct rev_info *opt, struct commit *commit)
+static int show_mergetag(struct rev_info *opt, struct commit *commit)
 {
-	for_each_mergetag(show_one_mergetag, commit, opt);
+	return for_each_mergetag(show_one_mergetag, commit, opt);
 }
 
 void show_log(struct rev_info *opt)

2.17.0.windows.1.15.gaa56ade3205

We have the BUG() macro for that purpose.

---
 builtin/replace.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 245d3f4164e..e345a5a0f1c 100644
@@ -501,6 +501,6 @@ int cmd_replace(int argc, const char **argv, const char *prefix)
 		return list_replace_refs(argv[0], format);
 
 	default:
-		die("BUG: invalid cmdmode %d", (int)cmdmode);
+		BUG("invalid cmdmode %d", (int)cmdmode);
 	}
 }

2.17.0.windows.1.15.gaa56ade3205

File this away as yet another patch in the "libification" category.

As with all useful functions, in the next commit we want to use create_graft() from a higher-level function where it would be inconvenient if the called function simply die()s: if there is a problem, we want to let the user know how to proceed, and the callee simply has no way of knowing what to say.

---
 builtin/replace.c | 135 ++++++++++++++++++++++++++++------------------
 1 file changed, 84 insertions(+), 51 deletions(-)

index e345a5a0f1c..f63df405fd0 100644
@@ -79,9 +79,9 @@ static int list_replace_refs(const char *pattern, const char *format)
 	else if (!strcmp(format, "long"))
 		data.format = REPLACE_FORMAT_LONG;
 	else
-		die("invalid replace format '%s'\n"
-		    "valid formats are 'short', 'medium' and 'long'\n",
-		    format);
+		return error("invalid replace format '%s'\n"
+			     "valid formats are 'short', 'medium' and 'long'\n",
+			     format);
 
 	for_each_replace_ref(show_reference, (void *)&data);
 

@@ -134,7 +134,7 @@ static int delete_replace_ref(const char *name, const char *ref,
 	return 0;
 }
 
-static void check_ref_valid(struct object_id *object,
+static int check_ref_valid(struct object_id *object,
 			    struct object_id *prev,
 			    struct strbuf *ref,
 			    int force)

@@ -142,12 +142,13 @@ static void check_ref_valid(struct object_id *object,
 	strbuf_reset(ref);
 	strbuf_addf(ref, "%s%s", git_replace_ref_base, oid_to_hex(object));
 	if (check_refname_format(ref->buf, 0))
-		die("'%s' is not a valid ref name.", ref->buf);
+		return error("'%s' is not a valid ref name.", ref->buf);
 
 	if (read_ref(ref->buf, prev))
 		oidclr(prev);
 	else if (!force)
-		die("replace ref '%s' already exists", ref->buf);
+		return error("replace ref '%s' already exists", ref->buf);
+	return 0;
 }
 
 static int replace_object_oid(const char *object_ref,

@@ -161,28 +162,31 @@ static int replace_object_oid(const char *object_ref,
 	struct strbuf ref = STRBUF_INIT;
 	struct ref_transaction *transaction;
 	struct strbuf err = STRBUF_INIT;
+	int res = 0;
 
 	obj_type = oid_object_info(object, NULL);
 	repl_type = oid_object_info(repl, NULL);
 	if (!force && obj_type != repl_type)
-		die("Objects must be of the same type.\n"
-		    "'%s' points to a replaced object of type '%s'\n"
-		    "while '%s' points to a replacement object of type '%s'.",
-		    object_ref, type_name(obj_type),
-		    replace_ref, type_name(repl_type));
+		return error("Objects must be of the same type.\n"
+			     "'%s' points to a replaced object of type '%s'\n"
+			     "while '%s' points to a replacement object of "
+			     "type '%s'.",
+			     object_ref, type_name(obj_type),
+			     replace_ref, type_name(repl_type));
 
-	check_ref_valid(object, &prev, &ref, force);
+	if (check_ref_valid(object, &prev, &ref, force))
+		return -1;
 
 	transaction = ref_transaction_begin(&err);
 	if (!transaction ||
 	    ref_transaction_update(transaction, ref.buf, repl, &prev,
 				   0, NULL, &err) ||
 	    ref_transaction_commit(transaction, &err))
-		die("%s", err.buf);
+		res = error("%s", err.buf);
 
 	ref_transaction_free(transaction);
 	strbuf_release(&ref);
-	return 0;
+	return res;
 }
 
 static int replace_object(const char *object_ref, const char *replace_ref, int force)

@@ -190,9 +194,11 @@ static int replace_object(const char *object_ref, const char *replace_ref, int f
 	struct object_id object, repl;
 
 	if (get_oid(object_ref, &object))
-		die("Failed to resolve '%s' as a valid ref.", object_ref);
+		return error("Failed to resolve '%s' as a valid ref.",
+			     object_ref);
 	if (get_oid(replace_ref, &repl))
-		die("Failed to resolve '%s' as a valid ref.", replace_ref);
+		return error("Failed to resolve '%s' as a valid ref.",
+			     replace_ref);
 
 	return replace_object_oid(object_ref, &object, replace_ref, &repl, force);
 }

@@ -202,7 +208,7 @@ static int replace_object(const char *object_ref, const char *replace_ref, int f
  * If "raw" is true, then the object's raw contents are printed according to
  * "type". Otherwise, we pretty-print the contents for human editing.
  */
-static void export_object(const struct object_id *oid, enum object_type type,
+static int export_object(const struct object_id *oid, enum object_type type,
 			  int raw, const char *filename)
 {
 	struct child_process cmd = CHILD_PROCESS_INIT;

@@ -210,7 +216,7 @@ static void export_object(const struct object_id *oid, enum object_type type,
 
 	fd = open(filename, O_WRONLY | O_CREAT | O_TRUNC, 0666);
 	if (fd < 0)
-		die_errno("unable to open %s for writing", filename);
+		return error_errno("unable to open %s for writing", filename);
 
 	argv_array_push(&cmd.args, "--no-replace-objects");
 	argv_array_push(&cmd.args, "cat-file");

@@ -223,7 +229,8 @@ static void export_object(const struct object_id *oid, enum object_type type,
 	cmd.out = fd;
 
 	if (run_command(&cmd))
-		die("cat-file reported failure");
+		return error("cat-file reported failure");
+	return 0;
 }
 
 /*

@@ -231,14 +238,14 @@ static void export_object(const struct object_id *oid, enum object_type type,
  * interpreting it as "type", and writing the result to the object database.
  * The sha1 of the written object is returned via sha1.
  */
-static void import_object(struct object_id *oid, enum object_type type,
+static int import_object(struct object_id *oid, enum object_type type,
 			  int raw, const char *filename)
 {
 	int fd;
 
 	fd = open(filename, O_RDONLY);
 	if (fd < 0)
-		die_errno("unable to open %s for reading", filename);
+		return error_errno("unable to open %s for reading", filename);
 
 	if (!raw && type == OBJ_TREE) {
 		const char *argv[] = { "mktree", NULL };

@@ -250,27 +257,38 @@ static void import_object(struct object_id *oid, enum object_type type,
 		cmd.in = fd;
 		cmd.out = -1;
 
-		if (start_command(&cmd))
-			die("unable to spawn mktree");
+		if (start_command(&cmd)) {
+			close(fd);
+			return error("unable to spawn mktree");
+		}
 
-		if (strbuf_read(&result, cmd.out, 41) < 0)
-			die_errno("unable to read from mktree");
+		if (strbuf_read(&result, cmd.out, 41) < 0) {
+			close(fd);
+			close(cmd.out);
+			return error_errno("unable to read from mktree");
+		}
 		close(cmd.out);
 
-		if (finish_command(&cmd))
-			die("mktree reported failure");
-		if (get_oid_hex(result.buf, oid) < 0)
-			die("mktree did not return an object name");
+		if (finish_command(&cmd)) {
+			strbuf_release(&result);
+			return error("mktree reported failure");
+		}
+		if (get_oid_hex(result.buf, oid) < 0) {
+			strbuf_release(&result);
+			return error("mktree did not return an object name");
+		}
 
 		strbuf_release(&result);
 	} else {
 		struct stat st;
 		int flags = HASH_FORMAT_CHECK | HASH_WRITE_OBJECT;
 
-		if (fstat(fd, &st) < 0)
-			die_errno("unable to fstat %s", filename);
+		if (fstat(fd, &st) < 0) {
+			close(fd);
+			return error_errno("unable to fstat %s", filename);
+		}
 		if (index_fd(oid, fd, &st, type, NULL, flags) < 0)
-			die("unable to write object to database");
+			return error("unable to write object to database");
 		/* index_fd close()s fd for us */
 	}
 

@@ -278,6 +296,7 @@ static void import_object(struct object_id *oid, enum object_type type,
 	 * No need to close(fd) here; both run-command and index-fd
 	 * will have done it for us.
 	 */
+	return 0;
 }
 
 static int edit_and_replace(const char *object_ref, int force, int raw)

@@ -288,19 +307,23 @@ static int edit_and_replace(const char *object_ref, int force, int raw)
 	struct strbuf ref = STRBUF_INIT;
 
 	if (get_oid(object_ref, &old_oid) < 0)
-		die("Not a valid object name: '%s'", object_ref);
+		return error("Not a valid object name: '%s'", object_ref);
 
 	type = oid_object_info(&old_oid, NULL);
 	if (type < 0)
-		die("unable to get object type for %s", oid_to_hex(&old_oid));
+		return error("unable to get object type for %s",
+			     oid_to_hex(&old_oid));
 
-	check_ref_valid(&old_oid, &prev, &ref, force);
+	if (check_ref_valid(&old_oid, &prev, &ref, force))
+		return -1;
 	strbuf_release(&ref);
 
-	export_object(&old_oid, type, raw, tmpfile);
+	if (export_object(&old_oid, type, raw, tmpfile))
+		return -1;
 	if (launch_editor(tmpfile, NULL, NULL) < 0)
-		die("editing object file failed");
-	import_object(&new_oid, type, raw, tmpfile);
+		return error("editing object file failed");
+	if (import_object(&new_oid, type, raw, tmpfile))
+		return -1;
 
 	free(tmpfile);
 

@@ -310,7 +333,7 @@ static int edit_and_replace(const char *object_ref, int force, int raw)
 	return replace_object_oid(object_ref, &old_oid, "replacement", &new_oid, force);
 }
 
-static void replace_parents(struct strbuf *buf, int argc, const char **argv)
+static int replace_parents(struct strbuf *buf, int argc, const char **argv)
 {
 	struct strbuf new_parents = STRBUF_INIT;
 	const char *parent_start, *parent_end;

@@ -328,8 +351,10 @@ static void replace_parents(struct strbuf *buf, int argc, const char **argv)
 	for (i = 0; i < argc; i++) {
 		struct object_id oid;
 		if (get_oid(argv[i], &oid) < 0)
-			die(_("Not a valid object name: '%s'"), argv[i]);
-		lookup_commit_or_die(&oid, argv[i]);
+			return error(_("Not a valid object name: '%s'"),
+				     argv[i]);
+		if (!lookup_commit_reference(&oid))
+			return error(_("could not parse %s"), argv[i]);
 		strbuf_addf(&new_parents, "parent %s\n", oid_to_hex(&oid));
 	}
 

@@ -338,6 +363,7 @@ static void replace_parents(struct strbuf *buf, int argc, const char **argv)
 		      new_parents.buf, new_parents.len);
 
 	strbuf_release(&new_parents);
+	return 0;
 }
 
 struct check_mergetag_data {

@@ -358,21 +384,23 @@ static int check_one_mergetag(struct commit *commit,
 	hash_object_file(extra->value, extra->len, type_name(OBJ_TAG), &tag_oid);
 	tag = lookup_tag(&tag_oid);
 	if (!tag)
-		die(_("bad mergetag in commit '%s'"), ref);
+		return error(_("bad mergetag in commit '%s'"), ref);
 	if (parse_tag_buffer(tag, extra->value, extra->len))
-		die(_("malformed mergetag in commit '%s'"), ref);
+		return error(_("malformed mergetag in commit '%s'"), ref);
 
 	/* iterate over new parents */
 	for (i = 1; i < mergetag_data->argc; i++) {
 		struct object_id oid;
 		if (get_oid(mergetag_data->argv[i], &oid) < 0)
-			die(_("Not a valid object name: '%s'"), mergetag_data->argv[i]);
+			return error(_("Not a valid object name: '%s'"),
+				     mergetag_data->argv[i]);
 		if (!oidcmp(&tag->tagged->oid, &oid))
 			return 0; /* found */
 	}
 
-	die(_("original commit '%s' contains mergetag '%s' that is discarded; "
-	      "use --edit instead of --graft"), ref, oid_to_hex(&tag_oid));
+	return error(_("original commit '%s' contains mergetag '%s' that is "
+		       "discarded; use --edit instead of --graft"), ref,
+		     oid_to_hex(&tag_oid));
 }
 
 static int check_mergetags(struct commit *commit, int argc, const char **argv)