Re: [PATCH 1/2] doc: update the documentation of pack-objects and repack

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

Damien Robert <damien.olivier.robert@gmail.com> writes:

> diff --git a/Documentation/git-pack-objects.txt b/Documentation/git-pack-objects.txt
> index fecdf2600c..7f4923ddea 100644
> --- a/Documentation/git-pack-objects.txt
> +++ b/Documentation/git-pack-objects.txt
> @@ -80,6 +80,14 @@ base-name::
>  	as if all refs under `refs/` are specified to be
>  	included.
>  
> +--reflog::
> +	This implies `--revs`.
> +	Include objects referred by reflog entries.
> +
> +--indexed-objects::
> +	This implies `--revs`.
> +	Include objects referred to by the index
> +

Missing full-stop (.) at the end.

> +--write-bitmap-index::
> +	Write a reachability bitmap index as part of the pack. This
> +	only makes sense when used with `--all` and the pack is not
> +	outputted to stdout.

Using "output" as a verb and conjugating it like this makes my head
hurt.  Let's instead borrow the phrase used in the description for
the "--stdout" option, i.e.

	... and the pack is not written to the standard output.

>  --keep-unreachable::
> -	Objects unreachable from the refs in packs named with
> -	--unpacked= option are added to the resulting pack, in
> -	addition to the reachable objects that are not in packs marked
> -	with *.keep files. This implies `--revs`.
> +	Unreachable packed objects are added to the resulting pack.
> +	This implies `--revs`.

Well spotted, and this update is very much appreciated.
`--unpacked` does not take a name of a packfile at all, at least
since 03a9683d ("Simplify is_kept_pack()", 2009-02-28).

> @@ -88,13 +88,14 @@ to the new separate pack will be written.
>  
>  --window=<n>::
>  --depth=<n>::
> -	These two options affect how the objects contained in the pack are
> -	stored using delta compression. The objects are first internally
> -	sorted by type, size and optionally names and compared against the
> -	other objects within `--window` to see if using delta compression saves
> -	space. `--depth` limits the maximum delta depth; making it too deep
> -	affects the performance on the unpacker side, because delta data needs
> -	to be applied that many times to get to the necessary object.
> +	These two options are passed to `git pack-objects` and affect how
> +	the objects contained in the pack are stored using delta
> +	compression. The objects are first internally sorted by type, size
> +	and optionally names and compared against the other objects within
> +	`--window` to see if using delta compression saves space. `--depth`
> +	limits the maximum delta depth; making it too deep affects the
> +	performance on the unpacker side, because delta data needs to be
> +	applied that many times to get to the necessary object.

It took me a while to realize that this only inserts "are passed to
`git pack-objects` and" and does nothing else.  It would have saved
reviewers' time if the whole paragraph did not get rewrapped.

I wonder if it helps the readers to tell the implementation detail
(i.e. are passed to X) upfront like the updated text.  It is true
that it would help the interested readers who want to know _more_
to tell them that these corresponds to the options the underlying
command has so they can go to the documentation of that other
command and read more about them, though.  

>  The default value for --window is 10 and --depth is 50. The maximum
>  depth is 4095.
> @@ -103,13 +104,13 @@ depth is 4095.
>  	This option is passed through to `git pack-objects`.
>  
>  --window-memory=<n>::
> -	This option provides an additional limit on top of `--window`;
> -	the window size will dynamically scale down so as to not take
> -	up more than '<n>' bytes in memory.  This is useful in
> -	repositories with a mix of large and small objects to not run
> -	out of memory with a large window, but still be able to take
> -	advantage of the large window for the smaller objects.  The
> -	size can be suffixed with "k", "m", or "g".
> +	This option is passed to `git pack-objects` and provides an
> +	additional limit on top of `--window`; the window size will
> +	dynamically scale down so as to not take up more than '<n>' bytes
> +	in memory.  This is useful in repositories with a mix of large and
> +	small objects to not run out of memory with a large window, but
> +	still be able to take advantage of the large window for the smaller
> +	objects.  The size can be suffixed with "k", "m", or "g".

Likewise.

>  	`--window-memory=0` makes memory usage unlimited.  The default
>  	is taken from the `pack.windowMemory` configuration variable.
>  	Note that the actual memory usage will be the limit multiplied
> @@ -122,6 +123,7 @@ depth is 4095.
>  	prevents the creation of a bitmap index.
>  	The default is unlimited, unless the config variable
>  	`pack.packSizeLimit` is set.
> +	This option is passed to `git pack-objects`.

Here, you use a different way to add the information to help readers
who would want to learn _more_.  And I think this approach makes
more sense than the previous two.  All readers would appreciate if
they can learn what they need to know to drive _this_ subcommand on
the documentation page for _this_ subcommand without having to
consulte another page, but those interested _can_ use reference like
this.

> @@ -129,7 +131,8 @@ depth is 4095.
>  	only makes sense when used with `-a` or `-A`, as the bitmaps
>  	must be able to refer to all reachable objects. This option
>  	overrides the setting of `repack.writeBitmaps`.  This option
> -	has no effect if multiple packfiles are created.
> +	has no effect if multiple packfiles are created, and is passed to
> +	`git pack-objects`.

Ditto.

> +Default options
> +---------------
> +
> +The command passes the following options to `git pack-objects`:
> +`--keep-true-parents`, `--no-empty`, `--all`, `--reflog`, `--indexed-objects`.
> +It also add `--exclude-promisor-objects` if there exists a promisor remote,
> +and `--honor-pack-keep` except if `--pack-kept-objects` is passed.

This is somewhat unconventional.  I think we usually say, when
describing each option --<option>, if it is enabled by default.

I kind of like this sort of summary where options that are on by
default can be seen in a single place, but (1) if we can reach a
concensus that this is a good practice, we should do it in more
places, and (2) if the sections for these individual options do not
say that they are on by default, we should make them say so.

Thanks.