[PATCH v2] travis-ci: build documentation

classic Classic list List threaded Threaded
53 messages Options
123
Reply | Threaded
Open this post in threaded view
|

[PATCH v2] travis-ci: build documentation

larsxschneider
From: Lars Schneider <[hidden email]>

Run "make doc" as separate Travis CI build job to check if all
documentation can be built without errors.

Signed-off-by: Lars Schneider <[hidden email]>
---

diff to v1:
* add quick sanity check to documentation results (thanks Matthieu)
* fix typo in commits message (thanks Stefan)
* move 'make doc' into seperate Travis CI build job (thanks Peff)
* started to move CI helper scripts to /ci (thanks Matthieu & Junio)

I really like the idea of the "/ci" directory. Over time I plan to
migrate all non Travis-CI related code from .travis.yml to this directory.
This would ease the transition to or parallel execution with another CI
system (e.g. GitLab CI to test Git on Windows).

Thanks,
Lars


 .travis.yml              | 15 +++++++++++++++
 ci/test-documentation.sh | 23 +++++++++++++++++++++++
 2 files changed, 38 insertions(+)
 create mode 100755 ci/test-documentation.sh

diff --git a/.travis.yml b/.travis.yml
index 78e433b..9f71d23 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -32,6 +32,21 @@ env:
     # t9816 occasionally fails with "TAP out of sequence errors" on Travis CI OS X
     - GIT_SKIP_TESTS="t9810 t9816"

+matrix:
+  include:
+    - env: Documentation
+      os: linux
+      compiler: clang
+      addons:
+        apt:
+          packages:
+          - asciidoc
+          - xmlto
+      before_install:
+      before_script: make doc
+      script: ci/test-documentation.sh
+      after_failure:
+
 before_install:
   - >
     case "${TRAVIS_OS_NAME:-linux}" in
diff --git a/ci/test-documentation.sh b/ci/test-documentation.sh
new file mode 100755
index 0000000..329ff4b
--- /dev/null
+++ b/ci/test-documentation.sh
@@ -0,0 +1,23 @@
+#!/bin/sh
+#
+# Perform a quick sanity check on documentation generated with 'make doc'.
+#
+
+set -e
+
+test_file_count () {
+    SUFFIX=$1
+    EXPECTED_COUNT=$2
+    ACTUAL_COUNT=$(find Documentation -type f -name "*.$SUFFIX" | wc -l)
+    echo "$ACTUAL_COUNT *.$SUFFIX files found. $EXPECTED_COUNT expected."
+    test $ACTUAL_COUNT -eq $EXPECTED_COUNT
+}
+
+test -s Documentation/git.html
+test -s Documentation/git.xml
+test -s Documentation/git.1
+
+# The follow numbers need to be adjusted when new documentation is added.
+test_file_count html 233
+test_file_count xml 171
+test_file_count 1 152
--
2.5.1

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2] travis-ci: build documentation

Jeff King
On Fri, Apr 29, 2016 at 11:35:34AM +0200, [hidden email] wrote:

> +# The follow numbers need to be adjusted when new documentation is added.
> +test_file_count html 233
> +test_file_count xml 171
> +test_file_count 1 152

This seems like it will be really flaky and a pain in the future. I'm
not really sure what it's accomplishing, either. The earlier steps would
complain if something failed to render, wouldn't they? At some point we
have to have some faith in "make doc".

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2] travis-ci: build documentation

Matthieu Moy-2
Jeff King <[hidden email]> writes:

> On Fri, Apr 29, 2016 at 11:35:34AM +0200, [hidden email] wrote:
>
>> +# The follow numbers need to be adjusted when new documentation is added.
>> +test_file_count html 233
>> +test_file_count xml 171
>> +test_file_count 1 152
>
> This seems like it will be really flaky and a pain in the future. I'm
> not really sure what it's accomplishing, either. The earlier steps would
> complain if something failed to render, wouldn't they? At some point we
> have to have some faith in "make doc".

I agree. My proposal to check for a handful of generated files was just
because this extra paranoia was almost free (just 3 lines of code that
won't need particular maintenance).

In this case, I'm afraid the maintenance cost is much bigger than the
expected benefits.

--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2] travis-ci: build documentation

larsxschneider

> On 29 Apr 2016, at 14:21, Matthieu Moy <[hidden email]> wrote:
>
> Jeff King <[hidden email]> writes:
>
>> On Fri, Apr 29, 2016 at 11:35:34AM +0200, [hidden email] wrote:
>>
>>> +# The follow numbers need to be adjusted when new documentation is added.
>>> +test_file_count html 233
>>> +test_file_count xml 171
>>> +test_file_count 1 152
>>
>> This seems like it will be really flaky and a pain in the future. I'm
>> not really sure what it's accomplishing, either. The earlier steps would
>> complain if something failed to render, wouldn't they? At some point we
>> have to have some faith in "make doc".
>
> I agree. My proposal to check for a handful of generated files was just
> because this extra paranoia was almost free (just 3 lines of code that
> won't need particular maintenance).
>
> In this case, I'm afraid the maintenance cost is much bigger than the
> expected benefits.

I agree, too. I wasn't sure about this check. That's why I added
the little comment above to point out the problem.

Should I reroll?

Thanks,
Lars--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2] travis-ci: build documentation

Jeff King
On Fri, Apr 29, 2016 at 04:22:05PM +0200, Lars Schneider wrote:

> >>> +# The follow numbers need to be adjusted when new documentation is added.
> >>> +test_file_count html 233
> >>> +test_file_count xml 171
> >>> +test_file_count 1 152
> [...]
> I agree, too. I wasn't sure about this check. That's why I added
> the little comment above to point out the problem.
>
> Should I reroll?

IMHO, yes.

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2] travis-ci: build documentation

Matthieu Moy-2
In reply to this post by larsxschneider
[hidden email] writes:

> +      before_install:
> +      before_script: make doc
> +      script: ci/test-documentation.sh

If you are to re-roll, I think before_script and script should be
merged, i.e. just write

script: ci/test-documentation.sh

and have ci/test-documentation.sh be:

#!/bin/sh

set -e

make doc
test -s Documentation/git.html
test -s Documentation/git.xml
test -s Documentation/git.1

In the perspective of using another CI tool, everything within ci/ is
potentially shared between systems so I'd tend to minimize the content
of .travis.yml in favor of ci/*

--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2] travis-ci: build documentation

Stefan Beller-4
In reply to this post by Matthieu Moy-2
On Fri, Apr 29, 2016 at 5:21 AM, Matthieu Moy
<[hidden email]> wrote:

> Jeff King <[hidden email]> writes:
>
>> On Fri, Apr 29, 2016 at 11:35:34AM +0200, [hidden email] wrote:
>>
>>> +# The follow numbers need to be adjusted when new documentation is added.
>>> +test_file_count html 233
>>> +test_file_count xml 171
>>> +test_file_count 1 152
>>
>> This seems like it will be really flaky and a pain in the future. I'm
>> not really sure what it's accomplishing, either. The earlier steps would
>> complain if something failed to render, wouldn't they? At some point we
>> have to have some faith in "make doc".
>
> I agree. My proposal to check for a handful of generated files was just
> because this extra paranoia was almost free (just 3 lines of code that
> won't need particular maintenance).
>
> In this case, I'm afraid the maintenance cost is much bigger than the
> expected benefits.

So you proposed to check a handful files for its exact content?

This could be less of maintenance if we'd check with a "larger as" operator
such as

    test_file_count_more_than html 200

using an arbitrary slightly smaller number.


>
> --
> Matthieu Moy
> http://www-verimag.imag.fr/~moy/
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to [hidden email]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2] travis-ci: build documentation

Matthieu Moy-2
Stefan Beller <[hidden email]> writes:

> On Fri, Apr 29, 2016 at 5:21 AM, Matthieu Moy
> <[hidden email]> wrote:
>> Jeff King <[hidden email]> writes:
>>
>>> On Fri, Apr 29, 2016 at 11:35:34AM +0200, [hidden email] wrote:
>>>
>>>> +# The follow numbers need to be adjusted when new documentation is added.
>>>> +test_file_count html 233
>>>> +test_file_count xml 171
>>>> +test_file_count 1 152
>>>
>>> This seems like it will be really flaky and a pain in the future. I'm
>>> not really sure what it's accomplishing, either. The earlier steps would
>>> complain if something failed to render, wouldn't they? At some point we
>>> have to have some faith in "make doc".
>>
>> I agree. My proposal to check for a handful of generated files was just
>> because this extra paranoia was almost free (just 3 lines of code that
>> won't need particular maintenance).
>>
>> In this case, I'm afraid the maintenance cost is much bigger than the
>> expected benefits.
>
> So you proposed to check a handful files for its exact content?

No, just to check that the files exist and are non-empty, i.e. the "test
-s" part of Lars' patch.

--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2] travis-ci: build documentation

larsxschneider
In reply to this post by Stefan Beller-4

On 29 Apr 2016, at 19:27, Stefan Beller <[hidden email]> wrote:

> On Fri, Apr 29, 2016 at 5:21 AM, Matthieu Moy
> <[hidden email]> wrote:
>> Jeff King <[hidden email]> writes:
>>
>>> On Fri, Apr 29, 2016 at 11:35:34AM +0200, [hidden email] wrote:
>>>
>>>> +# The follow numbers need to be adjusted when new documentation is added.
>>>> +test_file_count html 233
>>>> +test_file_count xml 171
>>>> +test_file_count 1 152
>>>
>>> This seems like it will be really flaky and a pain in the future. I'm
>>> not really sure what it's accomplishing, either. The earlier steps would
>>> complain if something failed to render, wouldn't they? At some point we
>>> have to have some faith in "make doc".
>>
>> I agree. My proposal to check for a handful of generated files was just
>> because this extra paranoia was almost free (just 3 lines of code that
>> won't need particular maintenance).
>>
>> In this case, I'm afraid the maintenance cost is much bigger than the
>> expected benefits.
>
> So you proposed to check a handful files for its exact content?
>
> This could be less of maintenance if we'd check with a "larger as" operator
> such as
>
>    test_file_count_more_than html 200
>
> using an arbitrary slightly smaller number.

Well, I was thinking about testing against something like
$(find . -type f -name "git*.txt" | wc -l) but it the end
all of this is not really meaningful I think...

- Lars


>
>
>>
>> --
>> Matthieu Moy
>> http://www-verimag.imag.fr/~moy/
>> --
>> To unsubscribe from this list: send the line "unsubscribe git" in
>> the body of a message to [hidden email]
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2] travis-ci: build documentation

Junio C Hamano
Lars Schneider <[hidden email]> writes:

>> This could be less of maintenance if we'd check with a "larger as" operator
>> such as
>>
>>    test_file_count_more_than html 200
>>
>> using an arbitrary slightly smaller number.
>
> Well, I was thinking about testing against something like
> $(find . -type f -name "git*.txt" | wc -l) but it the end
> all of this is not really meaningful I think...

Either is too much, I would say--I have too much faith in the exit
status from "make doc", I guess.

What would _REALLY_ be nice is a check that lets us catch an error
like this deliberate breakage:

    diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
    index 4b0318e..a684f2d 100644
    --- a/Documentation/diff-options.txt
    +++ b/Documentation/diff-options.txt
    @@ -560,4 +560,4 @@ endif::git-format-patch[]
            Do not show any source or destination prefix.

     For more detailed explanation on these common options, see also
    -linkgit:gitdiffcore[7].
    +linkgit:gitdiffnore[7].

We just get a dangling link in the result without an error exit from
"make doc"; neither "test -s" nor "size is about what we expect"
would catch such a breakage, though.

Other things that might be of interest are

    make check-builtins
    make check-docs

but I am not sure if the latter built target is up to date (it has a
whiltelist that needs to stay current).  We rarely add new commands
these days, so it is easy to forget what these build targets try to
check, which makes them good candidates to be thrown into the set of
automated tests.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

[PATCH v3 0/2] travis-ci: build documentation

larsxschneider
From: Lars Schneider <[hidden email]>

diff to v2:
* remove file count check for generated documentation as it is too flaky
* run `make doc` in `test-documentation.sh` to reduce the code in
  .travis.yml and ease the execution in other CI systems
* add linkgit check
* fix existing linkgit errors
* add `make check-builtins` and `make check-docs`

Thanks to Jeff, Matthieu, Stefan, and Junio for the review!

Cheers,
Lars

Lars Schneider (2):
  Documentation: fix linkgit references
  travis-ci: build documentation

 .travis.yml                         | 15 +++++++++++++++
 Documentation/config.txt            |  6 +++---
 Documentation/git-check-ignore.txt  |  2 +-
 Documentation/git-filter-branch.txt |  4 ++--
 Documentation/git-for-each-ref.txt  |  2 +-
 Documentation/git-help.txt          |  4 ++--
 Documentation/git-instaweb.txt      |  4 ++--
 Documentation/git-sh-i18n.txt       |  2 +-
 ci/test-documentation.sh            | 24 ++++++++++++++++++++++++
 9 files changed, 51 insertions(+), 12 deletions(-)
 create mode 100755 ci/test-documentation.sh

--
2.5.1

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

[PATCH v3 1/2] Documentation: fix linkgit references

larsxschneider
From: Lars Schneider <[hidden email]>

Signed-off-by: Lars Schneider <[hidden email]>
---
 Documentation/config.txt            | 6 +++---
 Documentation/git-check-ignore.txt  | 2 +-
 Documentation/git-filter-branch.txt | 4 ++--
 Documentation/git-for-each-ref.txt  | 2 +-
 Documentation/git-help.txt          | 4 ++--
 Documentation/git-instaweb.txt      | 4 ++--
 Documentation/git-sh-i18n.txt       | 2 +-
 7 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index c7bbe98..c5f1d6b 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -894,7 +894,7 @@ branch.<name>.description::
 browser.<tool>.cmd::
  Specify the command to invoke the specified browser. The
  specified command is evaluated in shell with the URLs passed
- as arguments. (See linkgit:git-web{litdd}browse[1].)
+ as arguments. (See linkgit:git-web--browse[1].)
 
 browser.<tool>.path::
  Override the path for the given tool that may be used to
@@ -1494,7 +1494,7 @@ gui.diffContext::
  made by the linkgit:git-gui[1]. The default is "5".
 
 gui.displayUntracked::
- Determines if linkgit::git-gui[1] shows untracked files
+ Determines if linkgit:git-gui[1] shows untracked files
  in the file list. The default is "true".
 
 gui.encoding::
@@ -1665,7 +1665,7 @@ http.cookieFile::
  File containing previously stored cookie lines which should be used
  in the Git http session, if they match the server. The file format
  of the file to read cookies from should be plain HTTP headers or
- the Netscape/Mozilla cookie file format (see linkgit:curl[1]).
+ the Netscape/Mozilla cookie file format (see `curl(1)`).
  NOTE that the file specified with http.cookieFile is only used as
  input unless http.saveCookies is set.
 
diff --git a/Documentation/git-check-ignore.txt b/Documentation/git-check-ignore.txt
index e94367a..9a85998 100644
--- a/Documentation/git-check-ignore.txt
+++ b/Documentation/git-check-ignore.txt
@@ -112,7 +112,7 @@ EXIT STATUS
 SEE ALSO
 --------
 linkgit:gitignore[5]
-linkgit:gitconfig[5]
+linkgit:git-config[5]
 linkgit:git-ls-files[1]
 
 GIT
diff --git a/Documentation/git-filter-branch.txt b/Documentation/git-filter-branch.txt
index 73fd9e8..6538cb1 100644
--- a/Documentation/git-filter-branch.txt
+++ b/Documentation/git-filter-branch.txt
@@ -205,8 +205,8 @@ to other tags will be rewritten to point to the underlying commit.
 Remap to ancestor
 ~~~~~~~~~~~~~~~~~
 
-By using linkgit:rev-list[1] arguments, e.g., path limiters, you can limit the
-set of revisions which get rewritten. However, positive refs on the command
+By using linkgit:git-rev-list[1] arguments, e.g., path limiters, you can limit
+the set of revisions which get rewritten. However, positive refs on the command
 line are distinguished: we don't let them be excluded by such limiters. For
 this purpose, they are instead rewritten to point at the nearest ancestor that
 was not excluded.
diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
index c52578b..d9d406d 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -179,7 +179,7 @@ returns an empty string instead.
 
 As a special case for the date-type fields, you may specify a format for
 the date by adding `:` followed by date format name (see the
-values the `--date` option to linkgit::git-rev-list[1] takes).
+values the `--date` option to linkgit:git-rev-list[1] takes).
 
 
 EXAMPLES
diff --git a/Documentation/git-help.txt b/Documentation/git-help.txt
index 3956525..7af18f9 100644
--- a/Documentation/git-help.txt
+++ b/Documentation/git-help.txt
@@ -72,7 +72,7 @@ The web browser can be specified using the configuration variable
 'help.browser', or 'web.browser' if the former is not set. If none of
 these config variables is set, the 'git web{litdd}browse' helper script
 (called by 'git help') will pick a suitable default. See
-linkgit:git-web{litdd}browse[1] for more information about this.
+linkgit:git-web--browse[1] for more information about this.
 
 CONFIGURATION VARIABLES
 -----------------------
@@ -95,7 +95,7 @@ help.browser, web.browser and browser.<tool>.path
 The 'help.browser', 'web.browser' and 'browser.<tool>.path' will also
 be checked if the 'web' format is chosen (either by command-line
 option or configuration variable). See '-w|--web' in the OPTIONS
-section above and linkgit:git-web{litdd}browse[1].
+section above and linkgit:git-web--browse[1].
 
 man.viewer
 ~~~~~~~~~~
diff --git a/Documentation/git-instaweb.txt b/Documentation/git-instaweb.txt
index cc75b25..b95cc15 100644
--- a/Documentation/git-instaweb.txt
+++ b/Documentation/git-instaweb.txt
@@ -46,7 +46,7 @@ OPTIONS
  The web browser that should be used to view the gitweb
  page. This will be passed to the 'git web{litdd}browse' helper
  script along with the URL of the gitweb instance. See
- linkgit:git-web{litdd}browse[1] for more information about this. If
+ linkgit:git-web--browse[1] for more information about this. If
  the script fails, the URL will be printed to stdout.
 
 start::
@@ -82,7 +82,7 @@ You may specify configuration in your .git/config
 
 If the configuration variable 'instaweb.browser' is not set,
 'web.browser' will be used instead if it is defined. See
-linkgit:git-web{litdd}browse[1] for more information about this.
+linkgit:git-web--browse[1] for more information about this.
 
 SEE ALSO
 --------
diff --git a/Documentation/git-sh-i18n.txt b/Documentation/git-sh-i18n.txt
index 60cf49c..eafa55a 100644
--- a/Documentation/git-sh-i18n.txt
+++ b/Documentation/git-sh-i18n.txt
@@ -35,7 +35,7 @@ gettext::
 eval_gettext::
  Currently a dummy fall-through function implemented as a wrapper
  around `printf(1)` with variables expanded by the
- linkgit:git-sh-i18n{litdd}envsubst[1] helper. Will be replaced by a
+ linkgit:git-sh-i18n--envsubst[1] helper. Will be replaced by a
  real gettext implementation in a later version.
 
 GIT
--
2.5.1

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

[PATCH v3 2/2] travis-ci: build documentation

larsxschneider
In reply to this post by larsxschneider
From: Lars Schneider <[hidden email]>

Build documentation as separate Travis CI job to check for
documentation errors.

Signed-off-by: Lars Schneider <[hidden email]>
---
 .travis.yml              | 15 +++++++++++++++
 ci/test-documentation.sh | 24 ++++++++++++++++++++++++
 2 files changed, 39 insertions(+)
 create mode 100755 ci/test-documentation.sh

diff --git a/.travis.yml b/.travis.yml
index 78e433b..55299bd 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -32,6 +32,21 @@ env:
     # t9816 occasionally fails with "TAP out of sequence errors" on Travis CI OS X
     - GIT_SKIP_TESTS="t9810 t9816"
 
+matrix:
+  include:
+    - env: Documentation
+      os: linux
+      compiler: clang
+      addons:
+        apt:
+          packages:
+          - asciidoc
+          - xmlto
+      before_install:
+      before_script:
+      script: ci/test-documentation.sh
+      after_failure:
+
 before_install:
   - >
     case "${TRAVIS_OS_NAME:-linux}" in
diff --git a/ci/test-documentation.sh b/ci/test-documentation.sh
new file mode 100755
index 0000000..889e6fd
--- /dev/null
+++ b/ci/test-documentation.sh
@@ -0,0 +1,24 @@
+#!/bin/sh
+#
+# Perform sanity checks on documentation and build it.
+#
+
+set -e
+
+LINKS=$(grep --recursive --only-matching --no-filename --perl-regexp \
+    '(?<=linkgit:).*?(?=\[\d+\])' Documentation/* \
+    | sort -u \
+)
+
+for LINK in $LINKS; do
+    echo "Checking linkgit:$LINK..."
+    test -s Documentation/$LINK.txt
+done
+
+make check-builtins
+make check-docs
+make doc
+
+test -s Documentation/git.html
+test -s Documentation/git.xml
+test -s Documentation/git.1
--
2.5.1

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v3 1/2] Documentation: fix linkgit references

Jeff King
In reply to this post by larsxschneider
On Mon, May 02, 2016 at 10:20:04PM +0200, [hidden email] wrote:

> From: Lars Schneider <[hidden email]>
>
> Signed-off-by: Lars Schneider <[hidden email]>
> ---

Fix how? Your commit message doesn't say why this is a good idea. Since
this is v3, I'm guessing that reasoning is on the list, but it needs to
be summarized here in the commit message.

> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index c7bbe98..c5f1d6b 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -894,7 +894,7 @@ branch.<name>.description::
>  browser.<tool>.cmd::
>   Specify the command to invoke the specified browser. The
>   specified command is evaluated in shell with the URLs passed
> - as arguments. (See linkgit:git-web{litdd}browse[1].)
> + as arguments. (See linkgit:git-web--browse[1].)

The existing code renders fine for me with "make git-config.1". But with
your patch, I get a unicode emdash, which is wrong:

--- old 2016-05-02 16:27:53.242050262 -0400
+++ new 2016-05-02 16:27:57.742050360 -0400
@@ -978,7 +978,7 @@
 
        browser.<tool>.cmd
            Specify the command to invoke the specified browser. The specified command is evaluated in shell with the
-           URLs passed as arguments. (See git-web--browse(1).)
+           URLs passed as arguments. (See git-web—browse(1).)
 
        browser.<tool>.path
            Override the path for the given tool that may be used to browse HTML help (see -w option in git-help(1))

In case it's hard to see with your font, the generated roff looks like
this:

-\fBgit-web--browse\fR(1)\&.)
+\fBgit-web\(embrowse\fR(1)\&.)

So I think that's a step backwards. I did check the asciidoctor
rendering on git-scm.com, though, and it gets the {litdd} case wrong. So
I think it does need fixing, but we need a solution that looks correct
in both cases. Maybe linkgit:`git-web--browse`[1] would work; it seems
OK with my version of asciidoc, but I have a feeling it will run into
the same problem with asciidoctor (if it's not respecting {litdd} in
that context, it's probably also not respecting backticks).

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v3 2/2] travis-ci: build documentation

Junio C Hamano
In reply to this post by larsxschneider
[hidden email] writes:

> +set -e
> +
> +LINKS=$(grep --recursive --only-matching --no-filename --perl-regexp \
> +    '(?<=linkgit:).*?(?=\[\d+\])' Documentation/* \
> +    | sort -u \
> +)
> +
> +for LINK in $LINKS; do
> +    echo "Checking linkgit:$LINK..."
> +    test -s Documentation/$LINK.txt
> +done

Please separate the above link check out of this step and do so
separately after the move of test body to a separate script
settles.

When you reintroduce the tests, please make sure the shell script
follow the coding style of other scripts.  E.g. I do not think the
last one in the $(...) needs a backslash continuation at all.  I am
assuming that you are doing this only on Linux, in which case use of
GNUism with grep may be fine.

> +make check-builtins
> +make check-docs
> +make doc
> +
> +test -s Documentation/git.html
> +test -s Documentation/git.xml
> +test -s Documentation/git.1
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v3 2/2] travis-ci: build documentation

larsxschneider

On 02 May 2016, at 22:45, Junio C Hamano <[hidden email]> wrote:

> [hidden email] writes:
>
>> +set -e
>> +
>> +LINKS=$(grep --recursive --only-matching --no-filename --perl-regexp \
>> +    '(?<=linkgit:).*?(?=\[\d+\])' Documentation/* \
>> +    | sort -u \
>> +)
>> +
>> +for LINK in $LINKS; do
>> +    echo "Checking linkgit:$LINK..."
>> +    test -s Documentation/$LINK.txt
>> +done
>
> Please separate the above link check out of this step and do so
> separately after the move of test body to a separate script
> settles.
OK. I also wonder if the link check should rather go to the
"check-docs" Makefile target?


> When you reintroduce the tests, please make sure the shell script
> follow the coding style of other scripts.  E.g. I do not think the
> last one in the $(...) needs a backslash continuation at all.  I am
> assuming that you are doing this only on Linux, in which case use of
> GNUism with grep may be fine.
OK.

Thanks for the review,
Lars


>
>> +make check-builtins
>> +make check-docs
>> +make doc
>> +
>> +test -s Documentation/git.html
>> +test -s Documentation/git.xml
>> +test -s Documentation/git.1

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v3 1/2] Documentation: fix linkgit references

larsxschneider
In reply to this post by Jeff King

On 02 May 2016, at 22:34, Jeff King <[hidden email]> wrote:

> On Mon, May 02, 2016 at 10:20:04PM +0200, [hidden email] wrote:
>
>> From: Lars Schneider <[hidden email]>
>>
>> Signed-off-by: Lars Schneider <[hidden email]>
>> ---
>
> Fix how? Your commit message doesn't say why this is a good idea. Since
> this is v3, I'm guessing that reasoning is on the list, but it needs to
> be summarized here in the commit message.
You are right, I should have explained my thinking a bit more detailed.
A few of the fixed linkgit references are just typos, e.g.:

-linkgit:gitconfig[5]
+linkgit:git-config[5]

-values the `--date` option to linkgit::git-rev-list[1] takes).
+values the `--date` option to linkgit:git-rev-list[1] takes).

- the Netscape/Mozilla cookie file format (see linkgit:curl[1]).
+ the Netscape/Mozilla cookie file format (see `curl(1)`).

I mistakenly assumed the "{litdd}" was a typo/bad search replace, too.
I checked this website and thought my change would fix it, too:

https://git-scm.com/docs/git-config

There it is rendered as "(See git-web{litdd}browse[1].)" and the link
is broken.


>> diff --git a/Documentation/config.txt b/Documentation/config.txt
>> index c7bbe98..c5f1d6b 100644
>> --- a/Documentation/config.txt
>> +++ b/Documentation/config.txt
>> @@ -894,7 +894,7 @@ branch.<name>.description::
>> browser.<tool>.cmd::
>> Specify the command to invoke the specified browser. The
>> specified command is evaluated in shell with the URLs passed
>> - as arguments. (See linkgit:git-web{litdd}browse[1].)
>> + as arguments. (See linkgit:git-web--browse[1].)
>
> The existing code renders fine for me with "make git-config.1". But with
> your patch, I get a unicode emdash, which is wrong:
>
> --- old 2016-05-02 16:27:53.242050262 -0400
> +++ new 2016-05-02 16:27:57.742050360 -0400
> @@ -978,7 +978,7 @@
>
>        browser.<tool>.cmd
>            Specify the command to invoke the specified browser. The specified command is evaluated in shell with the
> -           URLs passed as arguments. (See git-web--browse(1).)
> +           URLs passed as arguments. (See git-web—browse(1).)
>
>        browser.<tool>.path
>            Override the path for the given tool that may be used to browse HTML help (see -w option in git-help(1))
>
> In case it's hard to see with your font, the generated roff looks like
> this:
>
> -\fBgit-web--browse\fR(1)\&.)
> +\fBgit-web\(embrowse\fR(1)\&.)

I can confirm. Sorry, I indeed missed that.


> So I think that's a step backwards. I did check the asciidoctor
> rendering on git-scm.com, though, and it gets the {litdd} case wrong. So
> I think it does need fixing, but we need a solution that looks correct
> in both cases. Maybe linkgit:`git-web--browse`[1] would work; it seems
> OK with my version of asciidoc, but I have a feeling it will run into
> the same problem with asciidoctor (if it's not respecting {litdd} in
> that context, it's probably also not respecting backticks).
I will play with this to find a solution. Would it be an option to
replace "--" with "-"? Why do we need two dashes if they cause trouble?

Thanks for the review,
Lars--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v3 1/2] Documentation: fix linkgit references

Junio C Hamano
Lars Schneider <[hidden email]> writes:

> On 02 May 2016, at 22:34, Jeff King <[hidden email]> wrote:
>
>> On Mon, May 02, 2016 at 10:20:04PM +0200, [hidden email] wrote:
>>
>>> From: Lars Schneider <[hidden email]>
>>>
>>> Signed-off-by: Lars Schneider <[hidden email]>
>>> ---
>>
>> Fix how? Your commit message doesn't say why this is a good idea. Since
>> this is v3, I'm guessing that reasoning is on the list, but it needs to
>> be summarized here in the commit message.
> You are right, I should have explained my thinking a bit more detailed.
> A few of the fixed linkgit references are just typos, e.g.:
>
> -linkgit:gitconfig[5]
> +linkgit:git-config[5]

I didn't run "git blame" yet, so it might be that we used to have it
in section 5, but "git config --help" puts the latter in section 1.

Even though you obviously were fooled by AsciiDoctor regarding
literal double dash {litdd}, other parts of the changes did look
good typofixes.  Thanks for starting this.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v3 2/2] travis-ci: build documentation

Junio C Hamano
In reply to this post by larsxschneider
Lars Schneider <[hidden email]> writes:

> On 02 May 2016, at 22:45, Junio C Hamano <[hidden email]> wrote:
>
>> [hidden email] writes:
>>
>>> +set -e
>>> +
>>> +LINKS=$(grep --recursive --only-matching --no-filename --perl-regexp \
>>> +    '(?<=linkgit:).*?(?=\[\d+\])' Documentation/* \
>>> +    | sort -u \
>>> +)
>>> +
>>> +for LINK in $LINKS; do
>>> +    echo "Checking linkgit:$LINK..."
>>> +    test -s Documentation/$LINK.txt
>>> +done
>>
>> Please separate the above link check out of this step and do so
>> separately after the move of test body to a separate script
>> settles.
>
> OK. I also wonder if the link check should rather go to the
> "check-docs" Makefile target?

That sounds like a good direction.

Which in turn means that people on all platforms are welcome to run
it, which in turn means that the script must be even more portable,
with avoiding GNUism and bash-isms etc.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v3 1/2] Documentation: fix linkgit references

Jeff King
In reply to this post by larsxschneider
On Tue, May 03, 2016 at 10:30:09AM +0200, Lars Schneider wrote:

> > So I think that's a step backwards. I did check the asciidoctor
> > rendering on git-scm.com, though, and it gets the {litdd} case wrong. So
> > I think it does need fixing, but we need a solution that looks correct
> > in both cases. Maybe linkgit:`git-web--browse`[1] would work; it seems
> > OK with my version of asciidoc, but I have a feeling it will run into
> > the same problem with asciidoctor (if it's not respecting {litdd} in
> > that context, it's probably also not respecting backticks).
> I will play with this to find a solution. Would it be an option to
> replace "--" with "-"? Why do we need two dashes if they cause trouble?

We use two dashes to signify "internal" programs that users should not
rely on. So "git-web-browse" would be something we'd expect to support
forever, but "git-web--browse" is an implementation detail of one of our
scripts (that just happens to require an extra program).

So I don't think we want to switch away from that convention just to
make the documentation work.

AFAICT, the {litdd} is working fine with asciidoc; it's only asciidoctor
that is the problem. So the first step may be talking with asciidoctor
folks to see if it's a bug, or if they have a recommended workaround.

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
123