[PATCH 0/2] Documentation of pack and repack

[PATCH 0/2] Documentation of pack and repack

[Damien Robert]

The first patch update the documentation, the second patch is a small clean
up I found when reading the source to try to understand the behaviour of
pack-objects when writing the documentation.

Damien Robert (2):
  doc: update the documentation of pack-objects and repack
  pack-objects: change the name of add_objects_in_unpacked_packs

 Documentation/git-pack-objects.txt | 27 +++++++++----
 Documentation/git-repack.txt       | 61 +++++++++++++++++++-----------
 builtin/pack-objects.c             |  4 +-
 3 files changed, 60 insertions(+), 32 deletions(-)

-- 
Patched on top of v2.25.1-377-g2d2118b814 (git version 2.25.1)

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

[Damien Robert]

For pack-objects:

  - add a documentation for --reflog, --indexed-objects and
  --write-bitmap-index.

  - clarify --keep-unreachable. Indeed the current description is out of
  date:

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

  For example --unpacked= option is now a boolean, and objects in .keep
  packs are packed except if --honor-pack-keep is given.
  What --keep-unreachable currently does is simply to add all packed
  objects to the object list. In particular this includes unreachable
  objects (both unreachable from the passed revs or even from all revs).

  - rework the grammar for --pack-loose-unreachable

  - --unpack-unreachable can also accept a time

For repack:

- Unify the notation to `git pack-objects` and not `git-pack-objects`.

- Specify all options that are passed to `git pack-objects`

Signed-off-by: Damien Robert <damien.olivier.robert+git@gmail.com>
---
 Documentation/git-pack-objects.txt | 27 +++++++++----
 Documentation/git-repack.txt       | 61 +++++++++++++++++++-----------
 2 files changed, 58 insertions(+), 30 deletions(-)

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
+
 --include-tag::
 	Include unasked-for annotated tags if the object they
 	reference was included in the resulting packfile.  This
@@ -123,6 +131,11 @@ depth is 4095.
 	The default is unlimited, unless the config variable
 	`pack.packSizeLimit` is set.
 
+--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.
+
 --honor-pack-keep::
 	This flag causes an object already in a local pack that
 	has a .keep file to be ignored, even if it would have
@@ -286,17 +299,17 @@ Unexpected missing object will raise an error.
 	promisor remote [with .promisor].)  This is used with partial clone.
 
 --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`.
 
 --pack-loose-unreachable::
-	Pack unreachable loose objects (and their loose counterparts
-	removed). This implies `--revs`.
+	Pack unreachable loose objects (and remove their loose counterparts).
+	This implies `--revs`.
 
---unpack-unreachable::
+--unpack-unreachable=<when>::
 	Keep unreachable objects in loose form. This implies `--revs`.
+	If `<when>` is specified, do not bother loosening any objects older
+	than `<when>`.
 
 --delta-islands::
 	Restrict delta matches based on "islands". See DELTA ISLANDS
diff --git a/Documentation/git-repack.txt b/Documentation/git-repack.txt
index 92f146d27d..267edce2d6 100644
--- a/Documentation/git-repack.txt
+++ b/Documentation/git-repack.txt
@@ -68,11 +68,11 @@ to the new separate pack will be written.
 	linkgit:git-pack-objects[1].
 
 -f::
-	Pass the `--no-reuse-delta` option to `git-pack-objects`, see
+	Pass the `--no-reuse-delta` option to `git pack-objects`, see
 	linkgit:git-pack-objects[1].
 
 -F::
-	Pass the `--no-reuse-object` option to `git-pack-objects`, see
+	Pass the `--no-reuse-object` option to `git pack-objects`, see
 	linkgit:git-pack-objects[1].
 
 -q::
@@ -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.
 +
 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".
 	`--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`.
 
 -b::
 --write-bitmap-index::
@@ -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`.
 
 --pack-kept-objects::
 	Include objects in `.keep` files when repacking.  Note that we
@@ -145,26 +148,38 @@ depth is 4095.
 	of having `.keep` file on the pack. `<pack-name>` is the
 	pack file name without leading directory (e.g. `pack-123.pack`).
 	The option could be specified multiple times to keep multiple
-	packs.
+	packs, and is passed to `git pack-objects`.
 
 --unpack-unreachable=<when>::
+	When used with `-ad`, this option is passed to `git pack-objects`.
 	When loosening unreachable objects, do not bother loosening any
 	objects older than `<when>`. This can be used to optimize out
 	the write of any objects that would be immediately pruned by
 	a follow-up `git prune`.
+	The `-A` option is synonymous with `-a --unpack-unreachable`.
 
 -k::
 --keep-unreachable::
-	When used with `-ad`, any unreachable objects from existing
-	packs will be appended to the end of the packfile instead of
-	being removed. In addition, any unreachable loose objects will
-	be packed (and their loose counterparts removed).
+	When used with `-ad`, '--keep-unreachable' and
+	'--pack-loose-unreachable' are passed to `git pack-objects`.
+	Any unreachable objects from existing packs will be appended to the
+	end of the packfile instead of being removed. In addition, any
+	unreachable loose objects will be packed (and their loose
+	counterparts removed).
 
 -i::
 --delta-islands::
-	Pass the `--delta-islands` option to `git-pack-objects`, see
+	Pass the `--delta-islands` option to `git pack-objects`, see
 	linkgit:git-pack-objects[1].
 
+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.
+
 Configuration
 -------------
 
-- 
Patched on top of v2.25.1-377-g2d2118b814 (git version 2.25.1)

[PATCH 2/2] pack-objects: change the name of add_objects_in_unpacked_packs

[Damien Robert]

`add_objects_in_unpacked_packs` was added in commit
08cdfb13374f31b0c1c47444f55042e7b72c3190 (Sep 2007) to handle the
`--keep-unreachable` option.

Back then this function would iterate through packs associated to a list
of revs, and add all objects that was not already in the object list,
hence the name.

Now the function simply iterate through all packs (more precisely all
local packs not marked as .keep), and add all objects not already in the
object list.

So rename the function to add_unreachable_packed_objects, to mimic the
naming of add_unreachable_loose_objects.

Signed-off-by: Damien Robert <damien.olivier.robert+git@gmail.com>
---
 builtin/pack-objects.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 940fbcb7b3..16c2efdbec 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -3031,7 +3031,7 @@ static int ofscmp(const void *a_, const void *b_)
 		return oidcmp(&a->object->oid, &b->object->oid);
 }
 
-static void add_objects_in_unpacked_packs(void)
+static void add_unreachable_packed_objects(void)
 {
 	struct packed_git *p;
 	struct in_pack in_pack;
@@ -3290,7 +3290,7 @@ static void get_object_list(int ac, const char **av)
 	}
 
 	if (keep_unreachable)
-		add_objects_in_unpacked_packs();
+		add_unreachable_packed_objects();
 	if (pack_loose_unreachable)
 		add_unreachable_loose_objects();
 	if (unpack_unreachable)
-- 
Patched on top of v2.25.1-377-g2d2118b814 (git version 2.25.1)

Re: [PATCH 2/2] pack-objects: change the name of add_objects_in_unpacked_packs

[Damien Robert]

From Damien Robert, Fri 28 Feb 2020 at 16:43:57 (+0100) :
> Now the function simply iterate through all packs (more precisely all
> local packs not marked as .keep), and add all objects not already in the
> object list.

By the way is there a corner case here?

When using `git repack --keep-unreachable --pack-kept-objects`, since
`add_objects_in_unpacked_packs` does not add objects in *.keep packs, then
unreachable objects there won't be added. Or am I missing something?

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

[Junio C Hamano]

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.

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

[Damien Robert]

From Junio C Hamano, Mon 02 Mar 2020 at 10:57:16 (-0800) :
> Missing full-stop (.) at the end.

Oups.

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

Yeah I was not too happy with my formulation either...

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

Sorry. It looked to me like the documentation has an implicit wrap of
around 78 characters, so I rewrapped the paragraphs accordingly. But you
are right that since I was expecting comments on this patch anyway, I could
have rewrapped only in future versions, to simplify the diff for this
version.

> 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 also a question of consistency. Some options are already documented
as being passed to git-pack-objects. So when the user (ie me :)) sees an
option in git-repack that also exists in git pack-objects, it is natural to
assume it will be passed too, but since this is not explicitly stated for
*this option*, whereas it is stated for *other options*, the user may wonder
if this is truly the case. The user (still me :)) then looks at the source
code to check, but would have appreciated if this was already in the
documentation.

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

And it also helps to illustrate how a 'porcelain' command like 'git repack'
uses a plumbing command like 'git pack-objects'.

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

So currently in the documentation of git-repack, for options passed to
pack-objects, either
1) it is stated that this option is passed around and the option is not further documented (apart from refering to git-pack-objects(1)), eg -f, -F, -l
2) or the option is documented but git-pack-objects is not mentioned.

So for options of type 2) I added a link to git-pack-objects.
I agree with you that it would be good if options of type 1) were also
documented in `git-repack`; it is annoying for an user to have to open
another man page. I can do that in my next reroll.

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

The problem here is that
`--keep-true-parents`, `--no-empty`, `--all`, `--reflog`, `--indexed-objects`,
`--exclude-promisor-objects`
are always passed and not driven by any options of `git repack`, so I
did not know where else to put them.

-- 
Damien Robert
http://www.normalesup.org/~robert/pro

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

[Junio C Hamano]

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

> The problem here is that
> `--keep-true-parents`, `--no-empty`, `--all`, `--reflog`, `--indexed-objects`,
> `--exclude-promisor-objects`
> are always passed and not driven by any options of `git repack`, so I
> did not know where else to put them.

Ah, I think I may have misread the patch, then.  Why do readers who
wanted to learn 'git repack' even need to see what the command does
under the hood, driving what other low-level commands by passing
what options, in the first place?  Such implementation details can
change without affecting end-users, no?

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

[Damien Robert]

From Junio C Hamano, Tue 03 Mar 2020 at 10:49:43 (-0800) :
> Damien Robert <damien.olivier.robert@gmail.com> writes:
> 
> > The problem here is that
> > `--keep-true-parents`, `--no-empty`, `--all`, `--reflog`, `--indexed-objects`,
> > `--exclude-promisor-objects`
> > are always passed and not driven by any options of `git repack`, so I
> > did not know where else to put them.
> 
> Ah, I think I may have misread the patch, then.  Why do readers who
> wanted to learn 'git repack' even need to see what the command does
> under the hood, driving what other low-level commands by passing
> what options, in the first place?  Such implementation details can
> change without affecting end-users, no?

So do you suggest instead to remove all references to 'git-pack-objects'
in 'git-repack'? As I explained in my previous email, if some options
reference pack-objects, I think they should all do for consistency.

I also think that the situation of git-repack is a bit special:
first it is a very thin wrapper around git-pack-objects, so the
implementation details are not really abstracted from git-pack-objects.
Furthermore it is at an intermediate 'level' between a high level command
like 'git gc' and a plumbing command like 'git pack-objects'. So the user
interested in 'git repack' is probably interested in some low level
details.

Now to give an exemple, the doc of git-repack states:
    This command is used to combine all objects that do not currently
    reside in a "pack", into a pack.
This is a high level overview, but the user who knows a bit about Git
internals may wonder what exactly 'all' entails: non local objects, kept
objects, promisor objects, unreachable objects?
Knowing that the default options passed are:
`--keep-true-parents`, `--all`, `--reflog`, `--indexed-objects`, `--exclude-promisor-objects`
answers this questions: it is essentially all objects except unreachable ones.
Here I think that these technical details are more precise that whatever
sentence I could come up with, but I am happy to hear suggestions :)

I agree however that `--non-empty` is an implementation detail.

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

[Junio C Hamano]

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

> So do you suggest instead to remove all references to 'git-pack-objects'
> in 'git-repack'?

Not really.  If you are trying to remedy ...

> This is a high level overview, but the user who knows a bit about Git
> internals may wonder what exactly 'all' entails: non local objects, kept
> objects, promisor objects, unreachable objects?

... this kind of thing, I do not think that a solution that is the
best for readers is not this:

> Knowing that the default options passed are:
> `--keep-true-parents`, `--all`, `--reflog`, `--indexed-objects`, `--exclude-promisor-objects`

that requires them to go down and read what --indexed-objects (for
example) means in the documentation for "git pack-objects" command,
and piece together what they mean when they are used together.

> answers this questions: it is essentially all objects except unreachable ones.

Yes, "all objects except unreachable ones", which you came up with,
is a good description, I would think.

[PATCH v2 0/3] Documentation of pack and repack

[Damien Robert]

The first patch is a small clean up I found when reading the source to try
to understand the behaviour of pack-objects when writing the documentation.

The second patch update the documentation.

These two are not changed from my previous round, except
- I changed the order
- I took into account the typos mentioned by Junio
- I removed the default options section which referred to the default
  options passed to `git pack-objects` by `git repack`

In particular in the second patch the only update to the options description
in git repack is still 'are passed to `git pack-objects`'.

I added a third patch that add some quick description of the options
-l, -f, -F and -q.

Damien Robert (3):
  pack-objects: change the name of add_objects_in_unpacked_packs
  doc: update the documentation of pack-objects and repack
  doc: add a short explanation for git-repack options

 Documentation/git-pack-objects.txt | 27 +++++++++----
 Documentation/git-repack.txt       | 62 +++++++++++++++++++-----------
 builtin/pack-objects.c             |  4 +-
 3 files changed, 61 insertions(+), 32 deletions(-)

-- 
Patched on top of v2.26.0-rc1-6-ga56d361f66 (git version 2.25.1)

[PATCH v2 1/3] pack-objects: change the name of add_objects_in_unpacked_packs

[Damien Robert]

`add_objects_in_unpacked_packs` was added in commit
08cdfb13374f31b0c1c47444f55042e7b72c3190 (Sep 2007) to handle the
`--keep-unreachable` option.

Back then this function would iterate through packs associated to a list
of revs, and add all objects that was not already in the object list,
hence the name.

Now the function simply iterate through all packs (more precisely all
local packs not marked as .keep), and add all objects not already in the
object list.

So rename the function to add_unreachable_packed_objects, to mimic the
naming of add_unreachable_loose_objects.

Signed-off-by: Damien Robert <damien.olivier.robert+git@gmail.com>
---
 builtin/pack-objects.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 02aa6ee480..7c563d636c 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -3033,7 +3033,7 @@ static int ofscmp(const void *a_, const void *b_)
 		return oidcmp(&a->object->oid, &b->object->oid);
 }
 
-static void add_objects_in_unpacked_packs(void)
+static void add_unreachable_packed_objects(void)
 {
 	struct packed_git *p;
 	struct in_pack in_pack;
@@ -3293,7 +3293,7 @@ static void get_object_list(int ac, const char **av)
 	}
 
 	if (keep_unreachable)
-		add_objects_in_unpacked_packs();
+		add_unreachable_packed_objects();
 	if (pack_loose_unreachable)
 		add_unreachable_loose_objects();
 	if (unpack_unreachable)
-- 
Patched on top of v2.26.0-rc1-6-ga56d361f66 (git version 2.25.1)

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

[Damien Robert]

For pack-objects:

  - add a documentation for --reflog, --indexed-objects and
  --write-bitmap-index.

  - clarify --keep-unreachable. Indeed the current description is out of
  date:

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

  For example --unpacked= option is now a boolean, and objects in .keep
  packs are packed except if --honor-pack-keep is given.
  What --keep-unreachable currently does is simply to add all packed
  objects to the object list. In particular this includes unreachable
  objects (both unreachable from the passed revs or even from all revs).

  - rework the grammar for --pack-loose-unreachable

  - --unpack-unreachable can also accept a time

For repack:

- Unify the notation to `git pack-objects` and not `git-pack-objects`.

- Specify all options that are passed to `git pack-objects`

Signed-off-by: Damien Robert <damien.olivier.robert+git@gmail.com>
---
 Documentation/git-pack-objects.txt | 27 +++++++++++----
 Documentation/git-repack.txt       | 53 +++++++++++++++++-------------
 2 files changed, 50 insertions(+), 30 deletions(-)

diff --git a/Documentation/git-pack-objects.txt b/Documentation/git-pack-objects.txt
index fecdf2600c..cb4db37a03 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.
+
 --include-tag::
 	Include unasked-for annotated tags if the object they
 	reference was included in the resulting packfile.  This
@@ -123,6 +131,11 @@ depth is 4095.
 	The default is unlimited, unless the config variable
 	`pack.packSizeLimit` is set.
 
+--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
+	written to the standard output.
+
 --honor-pack-keep::
 	This flag causes an object already in a local pack that
 	has a .keep file to be ignored, even if it would have
@@ -286,17 +299,17 @@ Unexpected missing object will raise an error.
 	promisor remote [with .promisor].)  This is used with partial clone.
 
 --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`.
 
 --pack-loose-unreachable::
-	Pack unreachable loose objects (and their loose counterparts
-	removed). This implies `--revs`.
+	Pack unreachable loose objects (and remove their loose counterparts).
+	This implies `--revs`.
 
---unpack-unreachable::
+--unpack-unreachable=<when>::
 	Keep unreachable objects in loose form. This implies `--revs`.
+	If `<when>` is specified, do not bother loosening any objects older
+	than `<when>`.
 
 --delta-islands::
 	Restrict delta matches based on "islands". See DELTA ISLANDS
diff --git a/Documentation/git-repack.txt b/Documentation/git-repack.txt
index 92f146d27d..0962562c17 100644
--- a/Documentation/git-repack.txt
+++ b/Documentation/git-repack.txt
@@ -68,11 +68,11 @@ to the new separate pack will be written.
 	linkgit:git-pack-objects[1].
 
 -f::
-	Pass the `--no-reuse-delta` option to `git-pack-objects`, see
+	Pass the `--no-reuse-delta` option to `git pack-objects`, see
 	linkgit:git-pack-objects[1].
 
 -F::
-	Pass the `--no-reuse-object` option to `git-pack-objects`, see
+	Pass the `--no-reuse-object` option to `git pack-objects`, see
 	linkgit:git-pack-objects[1].
 
 -q::
@@ -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.
 +
 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".
 	`--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`.
 
 -b::
 --write-bitmap-index::
@@ -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`.
 
 --pack-kept-objects::
 	Include objects in `.keep` files when repacking.  Note that we
@@ -145,24 +148,28 @@ depth is 4095.
 	of having `.keep` file on the pack. `<pack-name>` is the
 	pack file name without leading directory (e.g. `pack-123.pack`).
 	The option could be specified multiple times to keep multiple
-	packs.
+	packs, and is passed to `git pack-objects`.
 
 --unpack-unreachable=<when>::
+	When used with `-ad`, this option is passed to `git pack-objects`.
 	When loosening unreachable objects, do not bother loosening any
 	objects older than `<when>`. This can be used to optimize out
 	the write of any objects that would be immediately pruned by
 	a follow-up `git prune`.
+	The `-A` option is synonymous with `-a --unpack-unreachable`.
 
 -k::
 --keep-unreachable::
-	When used with `-ad`, any unreachable objects from existing
-	packs will be appended to the end of the packfile instead of
-	being removed. In addition, any unreachable loose objects will
-	be packed (and their loose counterparts removed).
+	When used with `-ad`, '--keep-unreachable' and
+	'--pack-loose-unreachable' are passed to `git pack-objects`.
+	Any unreachable objects from existing packs will be appended to the
+	end of the packfile instead of being removed. In addition, any
+	unreachable loose objects will be packed (and their loose
+	counterparts removed).
 
 -i::
 --delta-islands::
-	Pass the `--delta-islands` option to `git-pack-objects`, see
+	Pass the `--delta-islands` option to `git pack-objects`, see
 	linkgit:git-pack-objects[1].
 
 Configuration
-- 
Patched on top of v2.26.0-rc1-6-ga56d361f66 (git version 2.25.1)

[PATCH v2 3/3] doc: add a short explanation for git-repack options

[Damien Robert]

For some git-repack options passed on to git-pack-objects, there was
only a link to git-pack-objects(1). Add a short documentation explaining
their meaning so that the reader does not have to consult another man
page.

We also explain that unreachable objects are not packed.

Signed-off-by: Damien Robert <damien.olivier.robert+git@gmail.com>
---
 Documentation/git-repack.txt | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-repack.txt b/Documentation/git-repack.txt
index 0962562c17..8f7f1140c3 100644
--- a/Documentation/git-repack.txt
+++ b/Documentation/git-repack.txt
@@ -14,9 +14,9 @@ SYNOPSIS
 DESCRIPTION
 -----------
 
-This command is used to combine all objects that do not currently
-reside in a "pack", into a pack.  It can also be used to re-organize
-existing packs into a single, more efficient pack.
+This command is used to combine all objects (except unreachable ones) that
+do not currently reside in a "pack", into a pack.  It can also be used to
+re-organize existing packs into a single, more efficient pack.
 
 A pack is a collection of objects, individually compressed, with
 delta compression applied, stored in a single file, with an
@@ -66,18 +66,27 @@ to the new separate pack will be written.
 -l::
 	Pass the `--local` option to 'git pack-objects'. See
 	linkgit:git-pack-objects[1].
+	This causes an object that is borrowed from an alternate
+	object store to be ignored even if it would have otherwise been
+	packed.
 
 -f::
 	Pass the `--no-reuse-delta` option to `git pack-objects`, see
 	linkgit:git-pack-objects[1].
+	The repack will not reuse existing deltas but compute them from
+	scratch.
 
 -F::
 	Pass the `--no-reuse-object` option to `git pack-objects`, see
 	linkgit:git-pack-objects[1].
+	The repack will not reuse existing object data at all, including
+	non deltified object, forcing recompression of everything.
 
 -q::
 	Pass the `-q` option to 'git pack-objects'. See
 	linkgit:git-pack-objects[1].
+	This flag makes the command not to report its progress
+	on the standard error stream.
 
 -n::
 	Do not update the server information with
-- 
Patched on top of v2.26.0-rc1-6-ga56d361f66 (git version 2.25.1)

Re: [PATCH v2 0/3] Documentation of pack and repack

[Damien Robert]

Just a friendly reminder about this series.

In particular, it would be nice to get a review on
>   doc: add a short explanation for git-repack options
since I am not a native english speaker.

Thanks!

-- 
Damien

Re: [PATCH v2 0/3] Documentation of pack and repack

[Junio C Hamano]

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

> Just a friendly reminder about this series.
>
> In particular, it would be nice to get a review on
>>   doc: add a short explanation for git-repack options
> since I am not a native english speaker.

Anybody wants to help by taking this one to see the accuracy of
description in the updated doc (being a native English speaker is
not a hard requirement, and while it may help, being correct would
be more important than being grammatically perfect)?

The series starts here:

https://lore.kernel.org/git/20200312170931.2392490-1-damien.olivier.robert+git@gmail.com/

Thanks.