[PATCH/RFC 0/2] 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.

Signed-off-by: Jonathan Nieder <redacted>
---
 Documentation/git-fast-import.txt |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-fast-import.txt b/Documentation/git-fast-import.txt
index 4415e63..086b14f 100644
--- a/Documentation/git-fast-import.txt
+++ b/Documentation/git-fast-import.txt

@@ -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.

Signed-off-by: Jonathan Nieder <redacted>
---
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(-)

diff --git a/Documentation/git-fast-import.txt b/Documentation/git-fast-import.txt
index 086b14f..2393252 100644
--- a/Documentation/git-fast-import.txt
+++ b/Documentation/git-fast-import.txt

@@ -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`
 ~~~~~~~~

diff --git a/fast-import.c b/fast-import.c
index 60f26fe..bf2f9f8 100644
--- a/fast-import.c
+++ b/fast-import.c

@@ -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;
 	}

diff --git a/t/t9301-fast-import-notes.sh b/t/t9301-fast-import-notes.sh
index 7cf8cd8..463254c 100755
--- a/t/t9301-fast-import-notes.sh
+++ b/t/t9301-fast-import-notes.sh

@@ -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

Jonathan Nieder wrote:

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.

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,

On Wed, Feb 2, 2011 at 20:47, Thomas Rast [off-list ref] wrote:

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

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

Sverre Rabbelier wrote:

Perhaps we can submit a patch to maint to have it (the
oldest git that supports the 'feature' command) recognize 'feature
notes' though?

Smart idea.  My concerns would evaporate.

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

Sverre Rabbelier wrote:

Perhaps we can submit a patch to maint to have it (the
oldest git that supports the 'feature' command) recognize 'feature
notes' though?

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.

Signed-off-by: Jonathan Nieder <redacted>
Acked-by: Sverre Rabbelier <redacted>
Signed-off-by: Junio C Hamano <redacted>
Signed-off-by: Jonathan Nieder <redacted>
---
Just a cherry-pick.

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

diff --git a/Documentation/git-fast-import.txt b/Documentation/git-fast-import.txt
index 19082b0..3bf04e3 100644
--- a/Documentation/git-fast-import.txt
+++ b/Documentation/git-fast-import.txt

@@ -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.)

Improved-by: Thomas Rast [off-list ref]
Improved-by: Sverre Rabbelier [off-list ref]
Signed-off-by: Jonathan Nieder <redacted>
---
 Documentation/git-fast-import.txt |    6 ++++++
 fast-import.c                     |    2 ++
 t/t9301-fast-import-notes.sh      |    1 +
 3 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/Documentation/git-fast-import.txt b/Documentation/git-fast-import.txt
index 3bf04e3..becee8b 100644
--- a/Documentation/git-fast-import.txt
+++ b/Documentation/git-fast-import.txt

@@ -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

diff --git a/fast-import.c b/fast-import.c
index 74f08bd..ff56ea2 100644
--- a/fast-import.c
+++ b/fast-import.c

@@ -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;
 	}

diff --git a/t/t9301-fast-import-notes.sh b/t/t9301-fast-import-notes.sh
index a5c99d8..164edf0 100755
--- a/t/t9301-fast-import-notes.sh
+++ b/t/t9301-fast-import-notes.sh

@@ -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,

On Wed, Feb 9, 2011 at 23:43, Jonathan Nieder [off-list ref] wrote:

Improved-by: Thomas Rast [off-list ref]
Improved-by: Sverre Rabbelier [off-list ref]
Signed-off-by: Jonathan Nieder <redacted>

Acked as well. Thanks for backporting this!


-- 
Cheers,

Sverre Rabbelier