[PATCH] Doc: clarify that pack-objects makes packs, plural

[PATCH] Doc: clarify that pack-objects makes packs, plural

[Jonathan Tan]

The documentation for pack-objects describes that it creates "a packed
archive of objects", which is confusing because it may create multiple
packs if --max-pack-size is set. Update the documentation to clarify
this.

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
It took me quite some time before I realized that pack-objects actually
may write multiple packs, the opening lines of the doc confusing me.
Here's a doc update.
---
 Documentation/git-pack-objects.txt | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/Documentation/git-pack-objects.txt b/Documentation/git-pack-objects.txt
index 8973510a4..d8264ad57 100644
--- a/Documentation/git-pack-objects.txt
+++ b/Documentation/git-pack-objects.txt
@@ -3,7 +3,7 @@ git-pack-objects(1)
 
 NAME
 ----
-git-pack-objects - Create a packed archive of objects
+git-pack-objects - Create packed archives of objects
 
 
 SYNOPSIS
@@ -18,8 +18,9 @@ SYNOPSIS
 
 DESCRIPTION
 -----------
-Reads list of objects from the standard input, and writes a packed
-archive with specified base-name, or to the standard output.
+Reads list of objects from the standard input, and writes either one or
+more packed archives with the specified base-name to disk, or a packed
+archive to the standard output.
 
 A packed archive is an efficient way to transfer a set of objects
 between two repositories as well as an access efficient archival
@@ -47,9 +48,9 @@ transport by their peers.
 OPTIONS
 -------
 base-name::
-	Write into a pair of files (.pack and .idx), using
+	Write into pairs of files (.pack and .idx), using
 	<base-name> to determine the name of the created file.
-	When this option is used, the two files are written in
+	When this option is used, the two files in a pair are written in
 	<base-name>-<SHA-1>.{pack,idx} files.  <SHA-1> is a hash
 	based on the pack content and is written to the standard
 	output of the command.
-- 
2.14.1.342.g6490525c54-goog

Re: [PATCH] Doc: clarify that pack-objects makes packs, plural

[Junio C Hamano]

Jonathan Tan <jonathantanmy@google.com> writes:

> The documentation for pack-objects describes that it creates "a packed
> archive of objects", which is confusing because it may create multiple
> packs if --max-pack-size is set. Update the documentation to clarify
> this.
>
> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
> ---
> It took me quite some time before I realized that pack-objects actually
> may write multiple packs, the opening lines of the doc confusing me.
> Here's a doc update.
> ---

I have a mixed feeling about this one.  

Yes, the command _can_ be told to split a packfile into multiple
with an option, but the actual benefit of doing so is rather
dubious.  On boxes with smaller address space, I thought windowed
mmap access into large packfiles work just fine.  I also think the
motivation behind the "max-size" thing was to split into smaller
pieces so that sneaker-netting on multiple CD-ROMs becomes easier or
something (silly) like that---there should be a more suitable tool
that is not specific to Git for such usecase, I would imagine.

So I am OK with "and writes either one or more" in the description,
but I'd prefer to see that "--max-pack-size" thing gets described as
an aberration, not a norm.

IOW,

 - I think the "NAME" part that gives a single line summary of what
   the command is about can and should stay as before.  A single
   archive is the norm, and we do not particularly recommend people
   to think it is a good idea to produce multiple packfiles.

 - The change in this patch for description part, which should give
   a fairly complete view of what it can do, is good.

 - The change for 'base-name' documentation that stresses that
   .pack/.idx come in pairs and share the same <SHA-1> is good.

 - There should be an update to say max-pack-size is not something
   normal users would ever want.

For the last one, perhaps something like this:


 Documentation/git-pack-objects.txt | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-pack-objects.txt b/Documentation/git-pack-objects.txt
index 8973510a41..3aa6234501 100644
--- a/Documentation/git-pack-objects.txt
+++ b/Documentation/git-pack-objects.txt
@@ -108,9 +108,13 @@ base-name::
 	is taken from the `pack.windowMemory` configuration variable.
 
 --max-pack-size=<n>::
-	Maximum size of each output pack file. The size can be suffixed with
+	In unusual scenarios, you may not be able to create files
+	larger than certain size on your filesystem, and this option
+	can be used to tell the command to split the output packfile
+	into multiple independent packfiles and what the maximum
+	size of each packfile is. The size can be suffixed with
 	"k", "m", or "g". The minimum size allowed is limited to 1 MiB.
-	If specified, multiple packfiles may be created, which also
+	This option
 	prevents the creation of a bitmap index.
 	The default is unlimited, unless the config variable
 	`pack.packSizeLimit` is set.

[PATCH v2] Doc: clarify that pack-objects makes packs, plural

[Jonathan Tan]

The documentation for pack-objects describes that it creates "a packed
archive of objects", which is confusing because it may create multiple
packs if --max-pack-size is set. Update the documentation to clarify
this, and explaining in which cases such a feature would be useful.

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
Thanks. I've reverted the NAME change and used some of your suggestion
for the --max-pack-size documentation.
---
 Documentation/git-pack-objects.txt | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/Documentation/git-pack-objects.txt b/Documentation/git-pack-objects.txt
index 8973510a4..473a16135 100644
--- a/Documentation/git-pack-objects.txt
+++ b/Documentation/git-pack-objects.txt
@@ -18,8 +18,9 @@ SYNOPSIS
 
 DESCRIPTION
 -----------
-Reads list of objects from the standard input, and writes a packed
-archive with specified base-name, or to the standard output.
+Reads list of objects from the standard input, and writes either one or
+more packed archives with the specified base-name to disk, or a packed
+archive to the standard output.
 
 A packed archive is an efficient way to transfer a set of objects
 between two repositories as well as an access efficient archival
@@ -47,9 +48,9 @@ transport by their peers.
 OPTIONS
 -------
 base-name::
-	Write into a pair of files (.pack and .idx), using
+	Write into pairs of files (.pack and .idx), using
 	<base-name> to determine the name of the created file.
-	When this option is used, the two files are written in
+	When this option is used, the two files in a pair are written in
 	<base-name>-<SHA-1>.{pack,idx} files.  <SHA-1> is a hash
 	based on the pack content and is written to the standard
 	output of the command.
@@ -108,9 +109,13 @@ base-name::
 	is taken from the `pack.windowMemory` configuration variable.
 
 --max-pack-size=<n>::
-	Maximum size of each output pack file. The size can be suffixed with
+	In unusual scenarios, you may not be able to create files
+	larger than a certain size on your filesystem, and this option
+	can be used to tell the command to split the output packfile
+	into multiple independent packfiles, each not larger than the
+	given size. The size can be suffixed with
 	"k", "m", or "g". The minimum size allowed is limited to 1 MiB.
-	If specified, multiple packfiles may be created, which also
+	This option
 	prevents the creation of a bitmap index.
 	The default is unlimited, unless the config variable
 	`pack.packSizeLimit` is set.
-- 
2.14.1.342.g6490525c54-goog

Re: [PATCH] Doc: clarify that pack-objects makes packs, plural

[Jeff King]

On Tue, Aug 22, 2017 at 12:56:25PM -0700, Junio C Hamano wrote:

>  - There should be an update to say max-pack-size is not something
>    normal users would ever want.

Agreed.

> diff --git a/Documentation/git-pack-objects.txt b/Documentation/git-pack-objects.txt
> index 8973510a41..3aa6234501 100644
> --- a/Documentation/git-pack-objects.txt
> +++ b/Documentation/git-pack-objects.txt
> @@ -108,9 +108,13 @@ base-name::
>  	is taken from the `pack.windowMemory` configuration variable.
>  
>  --max-pack-size=<n>::
> -	Maximum size of each output pack file. The size can be suffixed with
> +	In unusual scenarios, you may not be able to create files
> +	larger than certain size on your filesystem, and this option
> +	can be used to tell the command to split the output packfile
> +	into multiple independent packfiles and what the maximum
> +	size of each packfile is. The size can be suffixed with
>  	"k", "m", or "g". The minimum size allowed is limited to 1 MiB.
> -	If specified, multiple packfiles may be created, which also
> +	This option
>  	prevents the creation of a bitmap index.
>  	The default is unlimited, unless the config variable
>  	`pack.packSizeLimit` is set.

I wonder if it is worth mentioning the other downside: that the sum of
the split packfiles may be substantially larger than a single packfile
would be (due to lost delta opportunities between the split packs).

For the sneaker-net case, you are much better off generating a single
pack and then using "split" and "cat" to reconstruct it on the other end
Not that I think we should go into such detail in the manpage, but I
have to wonder if --max-pack-size has outlived its usefulness. The only
use case I can think of is a filesystem that cannot hold files larger
than N bytes.

-Peff

Re: [PATCH] Doc: clarify that pack-objects makes packs, plural

[Jacob Keller]

On Wed, Aug 23, 2017 at 2:22 PM, Jeff King <peff@peff.net> wrote:
> On Tue, Aug 22, 2017 at 12:56:25PM -0700, Junio C Hamano wrote:
>
>>  - There should be an update to say max-pack-size is not something
>>    normal users would ever want.
>
> Agreed.
>
>> diff --git a/Documentation/git-pack-objects.txt b/Documentation/git-pack-objects.txt
>> index 8973510a41..3aa6234501 100644
>> --- a/Documentation/git-pack-objects.txt
>> +++ b/Documentation/git-pack-objects.txt
>> @@ -108,9 +108,13 @@ base-name::
>>       is taken from the `pack.windowMemory` configuration variable.
>>
>>  --max-pack-size=<n>::
>> -     Maximum size of each output pack file. The size can be suffixed with
>> +     In unusual scenarios, you may not be able to create files
>> +     larger than certain size on your filesystem, and this option
>> +     can be used to tell the command to split the output packfile
>> +     into multiple independent packfiles and what the maximum
>> +     size of each packfile is. The size can be suffixed with
>>       "k", "m", or "g". The minimum size allowed is limited to 1 MiB.
>> -     If specified, multiple packfiles may be created, which also
>> +     This option
>>       prevents the creation of a bitmap index.
>>       The default is unlimited, unless the config variable
>>       `pack.packSizeLimit` is set.
>
> I wonder if it is worth mentioning the other downside: that the sum of
> the split packfiles may be substantially larger than a single packfile
> would be (due to lost delta opportunities between the split packs).
>
> For the sneaker-net case, you are much better off generating a single
> pack and then using "split" and "cat" to reconstruct it on the other end
> Not that I think we should go into such detail in the manpage, but I
> have to wonder if --max-pack-size has outlived its usefulness. The only
> use case I can think of is a filesystem that cannot hold files larger
> than N bytes.
>
> -Peff

Is it possible to detect on the file system that we can't store files
that large, and remove the option, while enabling it only when we
detect the filesystem is unable to store large files?

Thanks,
Jake

Re: [PATCH] Doc: clarify that pack-objects makes packs, plural

[Jeff King]

On Thu, Aug 24, 2017 at 12:27:52AM -0700, Jacob Keller wrote:

> > For the sneaker-net case, you are much better off generating a single
> > pack and then using "split" and "cat" to reconstruct it on the other end
> > Not that I think we should go into such detail in the manpage, but I
> > have to wonder if --max-pack-size has outlived its usefulness. The only
> > use case I can think of is a filesystem that cannot hold files larger
> > than N bytes.
> 
> Is it possible to detect on the file system that we can't store files
> that large, and remove the option, while enabling it only when we
> detect the filesystem is unable to store large files?

I'm not sure how easy it would be to do such a check. But even if it
was, I'm not sure that buys us much. We'd still carry the code. We could
in theory remove the option, simplifying the interface. But that removes
the possibility of somebody wanting to stage the smaller packfiles on a
more capable filesystem in preparation for moving them to the
more-limited one.

-Peff

Re: [PATCH] Doc: clarify that pack-objects makes packs, plural

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Thu, Aug 24, 2017 at 12:27:52AM -0700, Jacob Keller wrote:
>
>> > For the sneaker-net case, you are much better off generating a single
>> > pack and then using "split" and "cat" to reconstruct it on the other end
>> > Not that I think we should go into such detail in the manpage, but I
>> > have to wonder if --max-pack-size has outlived its usefulness. The only
>> > use case I can think of is a filesystem that cannot hold files larger
>> > than N bytes.
>> 
>> Is it possible to detect on the file system that we can't store files
>> that large, and remove the option, while enabling it only when we
>> detect the filesystem is unable to store large files?
>
> I'm not sure how easy it would be to do such a check. But even if it
> was, I'm not sure that buys us much. We'd still carry the code. We could
> in theory remove the option, simplifying the interface. But that removes
> the possibility of somebody wanting to stage the smaller packfiles on a
> more capable filesystem in preparation for moving them to the
> more-limited one.

I agree that it would not help anybody to _disable_ --max-pack-size
feature for those on certain filesystems, but it _might_ make sense
to automatically _enable_ it (and set it to the maximum number) when
your destination filesystem is limited.

Even in that case, failing with an error code from the filesystem
and then asking the user to redo with --max-pack-size specified
wouldn't be the end of the world, though.