Re: [PATCH 2/3] doc/fast-import: document feature import-marks-if-exists

[Junio C Hamano <gitster@pobox.com>]

Dmitry Ivankov <divanorama@gmail.com> writes:

> fast-import parameter --import-marks-if-exists was introduced in
> commit dded4.. --import-marks can be set via a "feature" command in
> a fast-import stream and --import-marks-if-exists has support for
> such specification too, for free.
>
> Document "feature import-marks-if-exists=<file>" command and add a
> minimalistic test for it.
>
> Signed-off-by: Dmitry Ivankov <divanorama@gmail.com>

Thanks.

>  Documentation/git-fast-import.txt |    5 +++++
>  t/t9300-fast-import.sh            |   15 +++++++++++++++
>  2 files changed, 20 insertions(+), 0 deletions(-)
>
> diff --git a/Documentation/git-fast-import.txt b/Documentation/git-fast-import.txt
> index 3f5b912..bdcc81c 100644
> --- a/Documentation/git-fast-import.txt
> +++ b/Documentation/git-fast-import.txt
> @@ -1005,6 +1005,11 @@ import-marks::
>  	second, an --import-marks= command-line option overrides
>  	any "feature import-marks" command in the stream.
>  
> +import-marks-if-exists::
> +	Like import-marks but instead of erroring out, silently
> +	skips the file if it does not exist. Differences from --
> +	version are the same as with import-marks feature above.

Wouldn't it be more concise and complete to do it this way instead?  It
avoids duplication and also clarifies you cannot mix these two.

diff --git a/Documentation/git-fast-import.txt b/Documentation/git-fast-import.txt
index 249249a..e939e74 100644
--- a/Documentation/git-fast-import.txt
+++ b/Documentation/git-fast-import.txt
@@ -995,10 +995,14 @@ force::
 	(see OPTIONS, above).
 
 import-marks::
+import-marks-if-exists::
 	Like --import-marks except in two respects: first, only one
-	"feature import-marks" command is allowed per stream;
+	"feature import-marks" command or "feature import-marks-if-exists"
+	is allowed per stream;
 	second, an --import-marks= command-line option overrides
-	any "feature import-marks" command in the stream.
+	any "feature import-marks" command in the stream. Similarly,
+	import-marks-if-exists corresponds to --import-marks-if-exists
+	from the command line.
 
 cat-blob::
 ls::

>  
> +test_expect_success 'R: feature import-marks-if-exists' '
> +	rm -f io.marks &&
> +	blob=$(echo hi | git hash-object --stdin) &&
> +	echo ":1 $blob" >expect &&
> +	git fast-import --import-marks-if-exists=io.marks --export-marks=io.marks <<-\EOF &&
> +	feature import-marks-if-exists=io.marks
> +	blob
> +	mark :1
> +	data 3
> +	hi
> +
> +	EOF
> +	test_cmp expect io.marks
> +'

Do you really want both the command line option and feature in this test?

There are at least four cases [*1*] you would want to test (a part of
blame lies in the original test you copied this from).

 - "if exists" should not fail if the marks file does not exist;
 - "if exists" reads marks if exists (not tested in this patch);
 - having "feature import-marks-if-exists" will trigger this feature
   without command line option (not tested in this patch); and
 - having "feature import-marks-if-exists" will be overridden by
   the command line option (not tested in this patch).




[Footnote]

*1* Ideally, the test should also cover these 16 combinations:

  io.marks file (2 cases: exists or missing)

times

  --import-mark-if-exists (2 cases: exists or missing)

times

 feature import-marks-if-exists (4 cases: missing,
 				same as command line,
                                different from command line,
				given more than one)