fast-import: introduce "feature notes" command

Hi,

As promised, here is a "feature" command for streams to use to require support for the notemodify (N) command.

Patch 1 explains how to delete notes. On first reading, I had thought that feature was missing.

Patch 2 is the title feature. The relevant message explains why this is an rfc.

Thoughts welcome, as always.

Jonathan Nieder (2):
Documentation/fast-import: explain how to delete a note
fast-import: introduce "feature notes" command

 Documentation/git-fast-import.txt |   18 ++++++++++--------
 fast-import.c                     |    6 ++++--
 t/t9301-fast-import-notes.sh      |    1 +
 3 files changed, 15 insertions(+), 10 deletions(-)

A notemodify (N) command with blob id consisting of 40 zeroes (so is_null_sha1 is true) means to not add a note to replace the existing, removed one.

---
 Documentation/git-fast-import.txt |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

index 4415e63..086b14f 100644
@@ -651,9 +651,9 @@ External data format::
 	'N' SP <dataref> SP <committish> LF
 ....
 +
-Here `<dataref>` can be either a mark reference (`:<idnum>`)
-set by a prior `blob` command, or a full 40-byte SHA-1 of an
-existing Git blob object.
+Here `<dataref>` can be a mark reference (`:<idnum>`)
+set by a prior `blob` command, a full 40-byte SHA-1 of an
+existing Git blob object, or 40 zeroes, to remove a note.
 
 Inline data format::
 	The data content for the note has not been supplied yet.

1.7.2.3

Here is a "feature" command for streams to use to require support for the notemodify (N) command.

Support for importing notes was added to git fast-import quite a while ago (v1.6.6-rc0~21^2~8, 2009-10-09), before the 'feature' facility was introduced (v1.7.0-rc0~95^2~4, fast-import: add feature command, 2009-12-04) so for compatibility with older git versions, authors of existing frontends should not start using the "feature notes" command. Most git versions in wide use support notemodify already.

The purpose of the "feature notes" declaration is instead to distinguish between git and fast-import backends that do not support notemodify. In git "feature notes" will be a no-op while in other current fast-import backends it will error out with a clear error message.

---
Still to do: the documentation does not hint at the point mentioned in paragraphs 2 and 3 above. Ideas for wording?

Aside from that, I think this is ready. Thanks for reading.

 Documentation/git-fast-import.txt |   12 +++++++-----
 fast-import.c                     |    6 ++++--
 t/t9301-fast-import-notes.sh      |    1 +
 3 files changed, 12 insertions(+), 7 deletions(-)

index 086b14f..2393252 100644
@@ -942,11 +942,13 @@ import-marks::
 	any "feature import-marks" command in the stream.
 
 cat-blob::
-	Ignored.  Versions of fast-import not supporting the
-	"cat-blob" command will exit with a message indicating so.
-	This lets the import error out early with a clear message,
-	rather than wasting time on the early part of an import
-	before the unsupported command is detected.
+	Require that the backend support the 'cat-blob' command.
+	Versions of fast-import not supporting the 'cat-blob'
+	command will exit with a message indicating so.
+
+notes::
+	Require that the backend support the 'notemodify' (N)
+	subcommand to the 'commit' command.
 
 `option`
 ~~~~~~~~

index 60f26fe..bf2f9f8 100644
@@ -2983,14 +2983,16 @@ static int parse_one_feature(const char *feature, int from_stream)
 		option_import_marks(feature + 13, from_stream);
 	} else if (!prefixcmp(feature, "export-marks=")) {
 		option_export_marks(feature + 13);
-	} else if (!strcmp(feature, "cat-blob")) {
-		; /* Don't die - this feature is supported */
 	} else if (!prefixcmp(feature, "relative-marks")) {
 		relative_marks_paths = 1;
 	} else if (!prefixcmp(feature, "no-relative-marks")) {
 		relative_marks_paths = 0;
 	} else if (!prefixcmp(feature, "force")) {
 		force_update = 1;
+	}
+	/* These features are present; don't error out for them. */
+	else if (!strcmp(feature, "cat-blob")) {
+	} else if (!strcmp(feature, "notes")) {
 	} else {
 		return 0;
 	}

index 7cf8cd8..463254c 100755
@@ -120,6 +120,7 @@ test_expect_success 'add notes with simple M command' '
 
 test_tick
 cat >input <<INPUT_END
+feature notes
 commit refs/notes/test
 committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE
 data <<COMMIT

1.7.2.3

So in summary, don't use "feature notes" because it would fail with old gits, but do use "feature notes" because it will fail for non-git?

Isn't that a bit backwards? I mean, a tool author would either just use it and say "if it doesn't read this, upgrade your git" or run

echo feature notes | scm fast-import

to test, and use notes depending on success. In both cases old gits will be regarded as incompatible. Or am I missing something?

Thomas Rast
trast@{inf,student}.ethz.ch

Heya,

I agree, old gits breaking on "feature notes", is/should be intended behavior. Perhaps we can submit a patch to maint to have it (the oldest git that supports the 'feature' command) recognize 'feature notes' though?

Cheers,

Sverre Rabbelier

Smart idea. My concerns would evaporate.

[v1.7.0-rc0~95^2~4 (2009-12-04) is the relevant git.]

Thanks, both. Here's a series based against v1.7.0.9 to do that (since 1.7.0.y is the oldest series with support for 'feature'.)

For ease of patching, patch 1 backports a related documentation tweak from v1.7.4-rc0. Patch 2 introduces the "feature notes" command itself.

Jonathan Nieder (2):
fast-import: clarify documentation of "feature" command
fast-import: introduce "feature notes" command

 Documentation/git-fast-import.txt |   37 ++++++++++++++++++++-----------------
 fast-import.c                     |    2 ++
 t/t9301-fast-import-notes.sh      |    1 +
 3 files changed, 23 insertions(+), 17 deletions(-)

Date: Sun, 28 Nov 2010 13:43:57 -0600

The "feature" command allows streams to specify options for the import that must not be ignored. Logically, they are part of the stream, even though technically most supported features are synonyms to command-line options.

Make this more obvious by being more explicit about how the analogy between most "feature" commands and command-line options works. Treat the feature (import-marks) that does not fit this analogy separately.

---
Just a cherry-pick.

 Documentation/git-fast-import.txt |   33 +++++++++++++++------------------
 1 files changed, 15 insertions(+), 18 deletions(-)

index 19082b0..3bf04e3 100644
@@ -878,28 +878,25 @@ Require that fast-import supports the specified feature, or abort if
 it does not.
 
 ....
-	'feature' SP <feature> LF
+	'feature' SP <feature> ('=' <argument>)? LF
 ....
 
-The <feature> part of the command may be any string matching
-^[a-zA-Z][a-zA-Z-]*$ and should be understood by fast-import.
+The <feature> part of the command may be any one of the following:
 
-Feature work identical as their option counterparts with the
-exception of the import-marks feature, see below.
+date-format::
+export-marks::
+relative-marks::
+no-relative-marks::
+force::
+	Act as though the corresponding command-line option with
+	a leading '--' was passed on the command line
+	(see OPTIONS, above).
 
-The following features are currently supported:
-
-* date-format
-* import-marks
-* export-marks
-* relative-marks
-* no-relative-marks
-* force
-
-The import-marks behaves differently from when it is specified as
-commandline option in that only one "feature import-marks" is allowed
-per stream. Also, any --import-marks= specified on the commandline
-will override those from the stream (if any).
+import-marks::
+	Like --import-marks except in two respects: first, only one
+	"feature import-marks" command is allowed per stream;
+	second, an --import-marks= command-line option overrides
+	any "feature import-marks" command in the stream.
 
 `option`
 ~~~~~~~~

1.7.4

Here is a 'feature' command for streams to use to require support for the notemodify (N) command.

When the 'feature' facility was introduced (v1.7.0-rc0~95^2~4, 2009-12-04), the notes import feature was old news (v1.6.6-rc0~21^2~8, 2009-10-09) and it was not obvious it deserved to be a named feature. But now that is clear, since all major non-git fast-import backends lack support for it.

Details: on git version with this patch applied, any "feature notes" command in the features/options section at the beginning of a stream will be treated as a no-op. On fast-import implementations without the feature (and older git versions), the command instead errors out with a message like

This version of fast-import does not support feature notes.

So by declaring use of notes at the beginning of a stream, frontends can avoid wasting time and other resources when the backend does not support notes. (This would be especially important for backends that do not support rewinding history after a botched import.)

---
 Documentation/git-fast-import.txt |    6 ++++++
 fast-import.c                     |    2 ++
 t/t9301-fast-import-notes.sh      |    1 +
 3 files changed, 9 insertions(+), 0 deletions(-)

index 3bf04e3..becee8b 100644
@@ -898,6 +898,12 @@ import-marks::
 	second, an --import-marks= command-line option overrides
 	any "feature import-marks" command in the stream.
 
+notes::
+	Require that the backend support the 'notemodify' (N)
+	subcommand to the 'commit' command.
+	Versions of fast-import not supporting notes will exit
+	with a message indicating so.
+
 `option`
 ~~~~~~~~
 Processes the specified option so that git fast-import behaves in a

index 74f08bd..ff56ea2 100644
@@ -2821,6 +2821,8 @@ static int parse_one_feature(const char *feature, int from_stream)
 		relative_marks_paths = 0;
 	} else if (!prefixcmp(feature, "force")) {
 		force_update = 1;
+	} else if (!strcmp(feature, "notes")) {
+		; /* do nothing; we have the feature */
 	} else {
 		return 0;
 	}

index a5c99d8..164edf0 100755
@@ -120,6 +120,7 @@ test_expect_success 'add notes with simple M command' '
 
 test_tick
 cat >input <<INPUT_END
+feature notes
 commit refs/notes/test
 committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE
 data <<COMMIT

1.7.4

Heya,

Acked as well. Thanks for backporting this!

Cheers,

Sverre Rabbelier