[PATCH] index-pack: do not segfault when keep_name is NULL

[PATCH] index-pack: do not segfault when keep_name is NULL

[Nguyễn Thái Ngọc Duy]

keep_name is used to print error messages a couple lines down. Reset
it to the real path returned by odb_pack_keep() if it's set to NULL by
caller.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 One of these moments I will make git log and friends optionally recognize
 "Diff-Options:" line in commit message. Adding -U14 in this case
 should help the reviewer to see how those error messages are printed.

 builtin/index-pack.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index a6b1c17..d95c3dc 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -1283,9 +1283,10 @@ static void final(const char *final_pack_name, const char *curr_pack_name,
 	if (keep_msg) {
 		int keep_fd, keep_msg_len = strlen(keep_msg);
 
-		if (!keep_name)
+		if (!keep_name) {
 			keep_fd = odb_pack_keep(name, sizeof(name), sha1);
-		else
+			keep_name = name;
+		} else
 			keep_fd = open(keep_name, O_RDWR|O_CREAT|O_EXCL, 0600);
 
 		if (keep_fd < 0) {
-- 
1.9.0.40.gaa8c3ea

Re: [PATCH] index-pack: do not segfault when keep_name is NULL

[Junio C Hamano]

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

> keep_name is used to print error messages a couple lines down. Reset
> it to the real path returned by odb_pack_keep() if it's set to NULL by
> caller.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  One of these moments I will make git log and friends optionally recognize
>  "Diff-Options:" line in commit message. Adding -U14 in this case
>  should help the reviewer to see how those error messages are printed.
>
>  builtin/index-pack.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/index-pack.c b/builtin/index-pack.c
> index a6b1c17..d95c3dc 100644
> --- a/builtin/index-pack.c
> +++ b/builtin/index-pack.c
> @@ -1283,9 +1283,10 @@ static void final(const char *final_pack_name, const char *curr_pack_name,
>  	if (keep_msg) {
>  		int keep_fd, keep_msg_len = strlen(keep_msg);
>  
> -		if (!keep_name)
> +		if (!keep_name) {
>  			keep_fd = odb_pack_keep(name, sizeof(name), sha1);
> -		else
> +			keep_name = name;
> +		} else
>  			keep_fd = open(keep_name, O_RDWR|O_CREAT|O_EXCL, 0600);

I think this fixes the right problem in a wrong way that hurts
longer-term maintainability.  Why not do

	keep_name ? keep_name : name

at the place where the name is used?  Otherwise you will have to
worry about affecting later codepaths that may want to try to use
!keep_name to switch between two codepaths, no?

>  
>  		if (keep_fd < 0) {

[PATCH v2] index-pack: do not segfault when keep_name is NULL

[Nguyễn Thái Ngọc Duy]

Use the name that is returned by odb_pack_keep() when the caller
passes NULL in keep_name.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/index-pack.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index a6b1c17..b9f6e12 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -1291,7 +1291,7 @@ static void final(const char *final_pack_name, const char *curr_pack_name,
 		if (keep_fd < 0) {
 			if (errno != EEXIST)
 				die_errno(_("cannot write keep file '%s'"),
-					  keep_name);
+					  keep_name ? keep_name : name);
 		} else {
 			if (keep_msg_len > 0) {
 				write_or_die(keep_fd, keep_msg, keep_msg_len);
@@ -1299,7 +1299,7 @@ static void final(const char *final_pack_name, const char *curr_pack_name,
 			}
 			if (close(keep_fd) != 0)
 				die_errno(_("cannot close written keep file '%s'"),
-				    keep_name);
+					  keep_name ? keep_name : name);
 			report = "keep";
 		}
 	}
-- 
1.9.0.40.gaa8c3ea

[PATCH 0/4] Better "gc --aggressive"

[Nguyễn Thái Ngọc Duy]

See [1] for the discussion that led to this series. It attempts to
pack the repo with two different depths: old history tightly packed
(smaller but also takes longer time to access) and recent history on
the opposite.

First draft, probably still some bugs lurking in pack_old_history().
It would be great if people could try it out on large repos and report
back the results (pack size between the old and new aggressive, gc
time, git log and blame speed...)

[1] http://thread.gmane.org/gmane.comp.version-control.git/242277

Nguyễn Thái Ngọc Duy (4):
  environment.c: fix constness for odb_pack_keep()
  pack-objects: support --keep
  gc --aggressive: make --depth configurable
  gc --aggressive: three phase repacking

 Documentation/config.txt           |  24 ++++++++
 Documentation/git-gc.txt           |   3 +
 Documentation/git-pack-objects.txt |   4 ++
 builtin/gc.c                       | 113 ++++++++++++++++++++++++++++++++++++-
 builtin/pack-objects.c             |  26 +++++++++
 environment.c                      |   2 +-
 git-compat-util.h                  |   2 +-
 7 files changed, 169 insertions(+), 5 deletions(-)

-- 
1.9.0.40.gaa8c3ea

[PATCH] index-pack: do not segfault when keep_name is NULL

[Nguyễn Thái Ngọc Duy]

keep_name is used to print error messages a couple lines down. Reset
it to the real path returned by odb_pack_keep() if it's set to NULL by
caller.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 One of these moments I will make git log and friends optionally recognize
 "Diff-Options:" line in commit message. Adding -U14 in this case
 should help the reviewer to see how those error messages are printed.

 builtin/index-pack.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index a6b1c17..d95c3dc 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -1283,9 +1283,10 @@ static void final(const char *final_pack_name, const char *curr_pack_name,
 	if (keep_msg) {
 		int keep_fd, keep_msg_len = strlen(keep_msg);
 
-		if (!keep_name)
+		if (!keep_name) {
 			keep_fd = odb_pack_keep(name, sizeof(name), sha1);
-		else
+			keep_name = name;
+		} else
 			keep_fd = open(keep_name, O_RDWR|O_CREAT|O_EXCL, 0600);
 
 		if (keep_fd < 0) {
-- 
1.9.0.40.gaa8c3ea