threads / patch / 46649

patchDoc: clarify that pack-objects makes packs, plural

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

## tl;dr

7 messages between Aug 22, 2017 and Aug 24, 2017. Diffs are folded; open one to read it.

replies: 6people: 4as markdown or json

Jonathan Tan· Aug 22, 2017, 18:22 UTC · lore

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(-)
Show changes to Documentation/git-pack-objects.txt +6 −5
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
Junio C Hamano· Aug 22, 2017, 19:56 UTC · re: Jonathan Tan · lore

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

Jonathan Tan <jonathantanmy@google.com> writes:
Show 11 quoted lines
> 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(-)
Show changes to Documentation/git-pack-objects.txt +6 −2
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.
Jonathan Tan· Aug 23, 2017, 00:40 UTC · re: Junio C Hamano · lore

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

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(-)
Show changes to Documentation/git-pack-objects.txt +11 −6
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
Jeff King· Aug 23, 2017, 21:22 UTC · re: Junio C Hamano · lore

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

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.
Show 20 quoted lines
> 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
Jacob Keller· Aug 24, 2017, 07:27 UTC · re: Jeff King · lore

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

On Wed, Aug 23, 2017 at 2:22 PM, Jeff King <peff@peff.net> wrote:
Show 40 quoted lines
> 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

Jeff King· Aug 24, 2017, 13:38 UTC · re: Jacob Keller · lore

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

On Thu, Aug 24, 2017 at 12:27:52AM -0700, Jacob Keller wrote:
Show 10 quoted lines
> > 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
Junio C Hamano· Aug 24, 2017, 19:09 UTC · re: Jeff King · lore

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

Jeff King <peff@peff.net> writes:
Show 19 quoted lines
> 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.

← back to recent threads