* [PATCH 0/2] Make git-gc more robust with regard to grafts @ 2009-07-23 15:33 Johannes Schindelin 2009-07-23 15:33 ` [PATCH 1/2] Add a test showing that 'git repack' throws away grafted-away parents Johannes Schindelin 2009-07-23 15:33 ` [PATCH 2/2] git repack: keep commits hidden by a graft Johannes Schindelin 0 siblings, 2 replies; 10+ messages in thread From: Johannes Schindelin @ 2009-07-23 15:33 UTC (permalink / raw) To: git, gitster; +Cc: Björn Steinbrink [-- Attachment #1: Type: TEXT/PLAIN, Size: 819 bytes --] More and more Git newbies are taught the wonders of Git grafts. But that is dangerous if the exercise is not to use filter-branch right away. So let's make git-repack (and thereby, git-gc) aware of grafts and avoid culling the parent commits hidden by grafts. Björn Steinbrink (1): Add a test showing that 'git repack' throws away grafted-away parents Johannes Schindelin (1): git repack: keep commits hidden by a graft Documentation/git-pack-objects.txt | 7 ++++++- builtin-pack-objects.c | 4 ++++ cache.h | 2 ++ commit.c | 2 +- environment.c | 1 + git-repack.sh | 2 +- t/t7700-repack.sh | 12 ++++++++++++ 7 files changed, 27 insertions(+), 3 deletions(-) ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 1/2] Add a test showing that 'git repack' throws away grafted-away parents 2009-07-23 15:33 [PATCH 0/2] Make git-gc more robust with regard to grafts Johannes Schindelin @ 2009-07-23 15:33 ` Johannes Schindelin 2009-07-23 16:09 ` Björn Steinbrink 2009-07-23 15:33 ` [PATCH 2/2] git repack: keep commits hidden by a graft Johannes Schindelin 1 sibling, 1 reply; 10+ messages in thread From: Johannes Schindelin @ 2009-07-23 15:33 UTC (permalink / raw) To: git, gitster; +Cc: Johannes Schindelin, Björn Steinbrink Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> --- Bjoern said that the reflogs are not thrown away if there is no "sleep 1" (which I turned into "test-chmtime -1 .git/logs/HEAD .git/logs/refs/heads/master" only to delete it as things worked here without it). t/t7700-repack.sh | 12 ++++++++++++ 1 files changed, 12 insertions(+), 0 deletions(-) diff --git a/t/t7700-repack.sh b/t/t7700-repack.sh index 87c9b0e..a4dddb7 100755 --- a/t/t7700-repack.sh +++ b/t/t7700-repack.sh @@ -149,5 +149,17 @@ test_expect_success 'local packed unreachable obs that exist in alternate ODB ar test_must_fail git show $csha1 ' +test_expect_failure 'objects made unreachable by grafts only are kept' ' + test_tick && + git commit --allow-empty -m "commit 4" && + H0=$(git rev-parse HEAD) && + H1=$(git rev-parse HEAD^) && + H2=$(git rev-parse HEAD^^) && + echo "$H0 $H2" > .git/info/grafts && + git reflog expire --expire=now --expire-unreachable=now --all && + git repack -a -d && + git cat-file -t $H1 + ' + test_done -- 1.6.4.rc1.289.gf87df ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] Add a test showing that 'git repack' throws away grafted-away parents 2009-07-23 15:33 ` [PATCH 1/2] Add a test showing that 'git repack' throws away grafted-away parents Johannes Schindelin @ 2009-07-23 16:09 ` Björn Steinbrink 0 siblings, 0 replies; 10+ messages in thread From: Björn Steinbrink @ 2009-07-23 16:09 UTC (permalink / raw) To: Johannes Schindelin; +Cc: git, gitster On 2009.07.23 17:33:45 +0200, Johannes Schindelin wrote: > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> Signed-off-by: Björn Steinbrink <B.Steinbrink@gmx.de> > --- > > Bjoern said that the reflogs are not thrown away if there is no > "sleep 1" (which I turned into "test-chmtime -1 .git/logs/HEAD > .git/logs/refs/heads/master" only to delete it as things worked > here without it). My original version of the test lacked the test_tick, with it, the test works without "sleep 1" for me, too. Björn ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 2/2] git repack: keep commits hidden by a graft 2009-07-23 15:33 [PATCH 0/2] Make git-gc more robust with regard to grafts Johannes Schindelin 2009-07-23 15:33 ` [PATCH 1/2] Add a test showing that 'git repack' throws away grafted-away parents Johannes Schindelin @ 2009-07-23 15:33 ` Johannes Schindelin 2009-07-24 5:04 ` Junio C Hamano 2009-07-24 5:13 ` Junio C Hamano 1 sibling, 2 replies; 10+ messages in thread From: Johannes Schindelin @ 2009-07-23 15:33 UTC (permalink / raw) To: git, gitster; +Cc: Björn Steinbrink When you have grafts that pretend that a given commit has different parents than the ones recorded in the commit object, it is dangerous to let 'git repack' remove those hidden parents, as you can easily remove the graft and end up with a broken repository. So let's play it safe and keep those parent objects and everything that is reachable by them. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> --- Maybe we should not even bother documenting this option? Documentation/git-pack-objects.txt | 7 ++++++- builtin-pack-objects.c | 4 ++++ cache.h | 2 ++ commit.c | 2 +- environment.c | 1 + git-repack.sh | 2 +- t/t7700-repack.sh | 2 +- 7 files changed, 16 insertions(+), 4 deletions(-) diff --git a/Documentation/git-pack-objects.txt b/Documentation/git-pack-objects.txt index 7d4c1a7..06390e3 100644 --- a/Documentation/git-pack-objects.txt +++ b/Documentation/git-pack-objects.txt @@ -11,7 +11,8 @@ SYNOPSIS [verse] 'git pack-objects' [-q] [--no-reuse-delta] [--delta-base-offset] [--non-empty] [--local] [--incremental] [--window=N] [--depth=N] [--all-progress] - [--revs [--unpacked | --all]*] [--stdout | base-name] < object-list + [--revs [--unpacked | --all]*] [--stdout | base-name] + [--grafts-replace-parents=no] < object-list DESCRIPTION @@ -197,6 +198,10 @@ base-name:: to force the version for the generated pack index, and to force 64-bit index entries on objects located above the given offset. +--grafts-replace-parents=no:: + With this option, parents that are hidden by grafts are packed + nevertheless. + Author ------ diff --git a/builtin-pack-objects.c b/builtin-pack-objects.c index 1689bd1..77bff85 100644 --- a/builtin-pack-objects.c +++ b/builtin-pack-objects.c @@ -2257,6 +2257,10 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix) die("bad %s", arg); continue; } + if (!strcmp(arg, "--grafts-replace-parents=no")) { + grafts_replace_parents = 0; + continue; + } usage(pack_usage); } diff --git a/cache.h b/cache.h index dbe460c..1a2a3c9 100644 --- a/cache.h +++ b/cache.h @@ -566,6 +566,8 @@ enum object_creation_mode { extern enum object_creation_mode object_creation_mode; +extern int grafts_replace_parents; + #define GIT_REPO_VERSION 0 extern int repository_format_version; extern int check_repository_format(void); diff --git a/commit.c b/commit.c index a47fb4d..ef8e911 100644 --- a/commit.c +++ b/commit.c @@ -262,7 +262,7 @@ int parse_commit_buffer(struct commit *item, void *buffer, unsigned long size) bufptr[47] != '\n') return error("bad parents in commit %s", sha1_to_hex(item->object.sha1)); bufptr += 48; - if (graft) + if (graft && (graft->nr_parent < 0 || grafts_replace_parents)) continue; new_parent = lookup_commit(parent); if (new_parent) diff --git a/environment.c b/environment.c index 95aa8a6..d478125 100644 --- a/environment.c +++ b/environment.c @@ -51,6 +51,7 @@ enum push_default_type push_default = PUSH_DEFAULT_MATCHING; #define OBJECT_CREATION_MODE OBJECT_CREATION_USES_HARDLINKS #endif enum object_creation_mode object_creation_mode = OBJECT_CREATION_MODE; +int grafts_replace_parents = 1; /* Parallel index stat data preload? */ int core_preload_index = 0; diff --git a/git-repack.sh b/git-repack.sh index 1bf2394..1bafba8 100755 --- a/git-repack.sh +++ b/git-repack.sh @@ -81,7 +81,7 @@ case ",$all_into_one," in esac args="$args $local ${GIT_QUIET:+-q} $no_reuse$extra" -names=$(git pack-objects --honor-pack-keep --non-empty --all --reflog $args </dev/null "$PACKTMP") || +names=$(git pack-objects --grafts-replace-parents=no --honor-pack-keep --non-empty --all --reflog $args </dev/null "$PACKTMP") || exit 1 if [ -z "$names" ]; then say Nothing new to pack. diff --git a/t/t7700-repack.sh b/t/t7700-repack.sh index a4dddb7..f4aa054 100755 --- a/t/t7700-repack.sh +++ b/t/t7700-repack.sh @@ -149,7 +149,7 @@ test_expect_success 'local packed unreachable obs that exist in alternate ODB ar test_must_fail git show $csha1 ' -test_expect_failure 'objects made unreachable by grafts only are kept' ' +test_expect_success 'objects made unreachable by grafts only are kept' ' test_tick && git commit --allow-empty -m "commit 4" && H0=$(git rev-parse HEAD) && -- 1.6.4.rc1.289.gf87df ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] git repack: keep commits hidden by a graft 2009-07-23 15:33 ` [PATCH 2/2] git repack: keep commits hidden by a graft Johannes Schindelin @ 2009-07-24 5:04 ` Junio C Hamano 2009-07-24 9:36 ` Johannes Schindelin 2009-07-24 5:13 ` Junio C Hamano 1 sibling, 1 reply; 10+ messages in thread From: Junio C Hamano @ 2009-07-24 5:04 UTC (permalink / raw) To: Johannes Schindelin; +Cc: git, Björn Steinbrink Johannes Schindelin <johannes.schindelin@gmx.de> writes: > When you have grafts that pretend that a given commit has different > parents than the ones recorded in the commit object, it is dangerous > to let 'git repack' remove those hidden parents, as you can easily > remove the graft and end up with a broken repository. > > So let's play it safe and keep those parent objects and everything > that is reachable by them. > > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> > --- > > Maybe we should not even bother documenting this option? Why isn't it "--[no-]grafts-replace-parents"? "--grafts-replace-parents=no" tempts users to say =never or =false, and also suggests we might add settings with other semantics later, but I do not think of any offhand. As far as I can tell, what the updated code does when this option is in effect is not "do not replace parents", but is "use both real parents and grafted parents", which is the right thing to do (if we said "use only the true parents", then parents reachable only through grafts will be lost after repacking, which is not what we want). It would probably make more sense to call it "--keep-real-parents" or something. Do we want to cull duplicated parents? I do not think it matters in this particular context of checking the reachability, but I am just double checking if you thought about the issue. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] git repack: keep commits hidden by a graft 2009-07-24 5:04 ` Junio C Hamano @ 2009-07-24 9:36 ` Johannes Schindelin 0 siblings, 0 replies; 10+ messages in thread From: Johannes Schindelin @ 2009-07-24 9:36 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Björn Steinbrink Hi, On Thu, 23 Jul 2009, Junio C Hamano wrote: > It would probably make more sense to call it "--keep-real-parents" or > something. Much better. > Do we want to cull duplicated parents? I do not think it matters in > this particular context of checking the reachability, but I am just > double checking if you thought about the issue. I thought about it, and decided it is not worth the hassle: originally I had the option in setup_revisions() until I realized that this is never called by pack-objects. So how about this addition to the commit message: As this behavior can only be triggered by git pack-objects, and as that command handles duplicate parents gracefully, we do not bother to cull such parents. Ciao, Dscho ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] git repack: keep commits hidden by a graft 2009-07-23 15:33 ` [PATCH 2/2] git repack: keep commits hidden by a graft Johannes Schindelin 2009-07-24 5:04 ` Junio C Hamano @ 2009-07-24 5:13 ` Junio C Hamano 2009-07-24 5:35 ` Björn Steinbrink 1 sibling, 1 reply; 10+ messages in thread From: Junio C Hamano @ 2009-07-24 5:13 UTC (permalink / raw) To: Johannes Schindelin; +Cc: git, Björn Steinbrink Johannes Schindelin <johannes.schindelin@gmx.de> writes: > diff --git a/commit.c b/commit.c > index a47fb4d..ef8e911 100644 > --- a/commit.c > +++ b/commit.c > @@ -262,7 +262,7 @@ int parse_commit_buffer(struct commit *item, void *buffer, unsigned long size) > bufptr[47] != '\n') > return error("bad parents in commit %s", sha1_to_hex(item->object.sha1)); > bufptr += 48; > - if (graft) > + if (graft && (graft->nr_parent < 0 || grafts_replace_parents)) > continue; Hmm, what is this "if it is negative" check for? I did not see it mentioned in the proposed commit log message. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] git repack: keep commits hidden by a graft 2009-07-24 5:13 ` Junio C Hamano @ 2009-07-24 5:35 ` Björn Steinbrink 2009-07-24 9:37 ` Johannes Schindelin 0 siblings, 1 reply; 10+ messages in thread From: Björn Steinbrink @ 2009-07-24 5:35 UTC (permalink / raw) To: Junio C Hamano; +Cc: Johannes Schindelin, git On 2009.07.23 22:13:43 -0700, Junio C Hamano wrote: > Johannes Schindelin <johannes.schindelin@gmx.de> writes: > > > diff --git a/commit.c b/commit.c > > index a47fb4d..ef8e911 100644 > > --- a/commit.c > > +++ b/commit.c > > @@ -262,7 +262,7 @@ int parse_commit_buffer(struct commit *item, void *buffer, unsigned long size) > > bufptr[47] != '\n') > > return error("bad parents in commit %s", sha1_to_hex(item->object.sha1)); > > bufptr += 48; > > - if (graft) > > + if (graft && (graft->nr_parent < 0 || grafts_replace_parents)) > > continue; > > Hmm, what is this "if it is negative" check for? I did not see it > mentioned in the proposed commit log message. That's for the special grafts for shallow clones. They override the other grafts, and are identified by the negative nr_parent value. Those have to stay in effect as the original parents aren't present in the repo. Björn ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] git repack: keep commits hidden by a graft 2009-07-24 5:35 ` Björn Steinbrink @ 2009-07-24 9:37 ` Johannes Schindelin 2009-07-24 15:24 ` Junio C Hamano 0 siblings, 1 reply; 10+ messages in thread From: Johannes Schindelin @ 2009-07-24 9:37 UTC (permalink / raw) To: Björn Steinbrink; +Cc: Junio C Hamano, git [-- Attachment #1: Type: TEXT/PLAIN, Size: 1178 bytes --] Hi, On Fri, 24 Jul 2009, Björn Steinbrink wrote: > On 2009.07.23 22:13:43 -0700, Junio C Hamano wrote: > > Johannes Schindelin <johannes.schindelin@gmx.de> writes: > > > > > diff --git a/commit.c b/commit.c > > > index a47fb4d..ef8e911 100644 > > > --- a/commit.c > > > +++ b/commit.c > > > @@ -262,7 +262,7 @@ int parse_commit_buffer(struct commit *item, void *buffer, unsigned long size) > > > bufptr[47] != '\n') > > > return error("bad parents in commit %s", sha1_to_hex(item->object.sha1)); > > > bufptr += 48; > > > - if (graft) > > > + if (graft && (graft->nr_parent < 0 || grafts_replace_parents)) > > > continue; > > > > Hmm, what is this "if it is negative" check for? I did not see it > > mentioned in the proposed commit log message. > > That's for the special grafts for shallow clones. They override the > other grafts, and are identified by the negative nr_parent value. Those > have to stay in effect as the original parents aren't present in the > repo. Right. How about a comment above the if(): /* * If nr_parent is negative, the commit is shallow, and * we must not traverse its real parents. */ Hmm? Ciao, Dscho ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] git repack: keep commits hidden by a graft 2009-07-24 9:37 ` Johannes Schindelin @ 2009-07-24 15:24 ` Junio C Hamano 0 siblings, 0 replies; 10+ messages in thread From: Junio C Hamano @ 2009-07-24 15:24 UTC (permalink / raw) To: Johannes Schindelin; +Cc: Björn Steinbrink, git Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: > Right. How about a comment above the if(): > > /* > * If nr_parent is negative, the commit is shallow, and > * we must not traverse its real parents. > */ That's much better. ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2009-07-24 15:24 UTC | newest] Thread overview: 10+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2009-07-23 15:33 [PATCH 0/2] Make git-gc more robust with regard to grafts Johannes Schindelin 2009-07-23 15:33 ` [PATCH 1/2] Add a test showing that 'git repack' throws away grafted-away parents Johannes Schindelin 2009-07-23 16:09 ` Björn Steinbrink 2009-07-23 15:33 ` [PATCH 2/2] git repack: keep commits hidden by a graft Johannes Schindelin 2009-07-24 5:04 ` Junio C Hamano 2009-07-24 9:36 ` Johannes Schindelin 2009-07-24 5:13 ` Junio C Hamano 2009-07-24 5:35 ` Björn Steinbrink 2009-07-24 9:37 ` Johannes Schindelin 2009-07-24 15:24 ` Junio C Hamano
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).