Git Mailing List Archive on lore.kernel.org
 help / color / Atom feed
From: Damien Robert <damien.olivier.robert@gmail.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: git@vger.kernel.org
Subject: Re: [PATCH 1/2] doc: update the documentation of pack-objects and repack
Date: Tue, 3 Mar 2020 18:41:36 +0100
Message-ID: <20200303174136.ess5lfxrsrt6qvdu@feanor> (raw)
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'.

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

  reply index

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-02-28 15:43 [PATCH 0/2] Documentation of pack " Damien Robert
2020-02-28 15:43 ` [PATCH 1/2] doc: update the documentation of pack-objects " Damien Robert
2020-03-02 18:57   ` Junio C Hamano
2020-03-03 17:41     ` Damien Robert [this message]
2020-03-03 18:49       ` Junio C Hamano
2020-03-03 21:23         ` Damien Robert
2020-03-03 22:29           ` Junio C Hamano
2020-03-12 17:09   ` [PATCH v2 0/3] Documentation of pack " Damien Robert
2020-03-12 17:09     ` [PATCH v2 1/3] pack-objects: change the name of add_objects_in_unpacked_packs Damien Robert
2020-03-12 17:09     ` [PATCH v2 2/3] doc: update the documentation of pack-objects and repack Damien Robert
2020-03-12 17:09     ` [PATCH v2 3/3] doc: add a short explanation for git-repack options Damien Robert
2020-03-25 22:15     ` [PATCH v2 0/3] Documentation of pack and repack Damien Robert
2020-03-27 22:21       ` Junio C Hamano
2020-02-28 15:43 ` [PATCH 2/2] pack-objects: change the name of add_objects_in_unpacked_packs Damien Robert
2020-02-28 16:01   ` Damien Robert

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200303174136.ess5lfxrsrt6qvdu@feanor \
    --to=damien.olivier.robert@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Git Mailing List Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/git/0 git/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 git git/ https://lore.kernel.org/git \
		git@vger.kernel.org
	public-inbox-index git

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.git


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git