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

From: Damien Robert <damien.olivier.robert@gmail.com>
Date: Mar 3, 2020, 17:41 UTC
Message-ID: <20200303174136.ess5lfxrsrt6qvdu@feanor>
In-Reply-To: <xmqqk142bn5f.fsf@gitster-ct.c.googlers.com>

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

Show 16 quoted lines

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

Show 15 quoted lines

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