threads / patch / 37460

patchreachable.c: add HEAD to reachability starting commits

Subject: [PATCH] reachable.c: add HEAD to reachability starting commits

## tl;dr

5 messages between Aug 30, 2014 and Sep 3, 2014. Diffs are folded; open one to read it.

replies: 4people: 3as markdown or json

Max Kirillov· Aug 30, 2014, 20:58 UTC · lore

HEAD is not explicitly used as a starting commit for calculating reachability, so if it's detached and reflogs are disabled it may be pruned.

Add test which demonstrates it.

Signed-off-by: Max Kirillov <max@max630.net>
---
Hi.

This is a followup of http://thread.gmane.org/gmane.comp.version-control.git/255996 I digged it a bit more and look what I found!

Actually, I think would be nice if someone familiar with the code greps for
for_each_ref() usage and for each case verify if head should be added.
 reachable.c               |  3 +++
 t/t5312-prune-detached.sh | 19 +++++++++++++++++++
 2 files changed, 22 insertions(+)
 create mode 100755 t/t5312-prune-detached.sh

Show changes to 2 files +22 −0

reachable.c, t/t5312-prune-detached.sh

diff --git a/reachable.c b/reachable.c
index 654a8c5..6f6835b 100644
--- a/reachable.c
+++ b/reachable.c
@@ -229,6 +229,9 @@ void mark_reachable_objects(struct rev_info *revs, int mark_reflog,
 	/* Add all external refs */
 	for_each_ref(add_one_ref, revs);
 
+	/* detached HEAD is not included in the list above */
+	head_ref(add_one_ref, revs);
+
 	/* Add all reflog info */
 	if (mark_reflog)
 		for_each_reflog(add_one_reflog, revs);
diff --git a/t/t5312-prune-detached.sh b/t/t5312-prune-detached.sh
new file mode 100755
index 0000000..fac93e1
--- /dev/null
+++ b/t/t5312-prune-detached.sh
@@ -0,0 +1,19 @@
+#!/bin/sh
+
+test_description='no prune detached head without reflog'
+. ./test-lib.sh
+
+test_expect_success 'make repo' '
+	git config core.logAllRefUpdates false
+	git commit --allow-empty -m commit1 &&
+	git commit --allow-empty -m commit2 &&
+	git checkout  --detach master &&
+	git commit --allow-empty -m commit3
+'
+
+test_expect_success 'prune does not delete anything' '
+	git prune -n >prune_actual &&
+	: >prune_expected &&
+	test_cmp prune_expected prune_actual'
+
+test_done

-- 
2.0.1.1697.g73c6810

Jeff King· Aug 31, 2014, 15:28 UTC · re: Max Kirillov · lore

Re: [PATCH] reachable.c: add HEAD to reachability starting commits

On Sat, Aug 30, 2014 at 11:58:35PM +0300, Max Kirillov wrote:

> HEAD is not explicitly used as a starting commit for
> calculating reachability, so if it's detached and reflogs
> are disabled it may be pruned.

Eek, you're right. I think nobody noticed because the HEAD reflog usually picks it up (and you do not usually detach HEAD on a bare repo). But I agree we should include it to cover this case.

Show 14 quoted lines

> diff --git a/reachable.c b/reachable.c
> index 654a8c5..6f6835b 100644
> --- a/reachable.c
> +++ b/reachable.c
> @@ -229,6 +229,9 @@ void mark_reachable_objects(struct rev_info *revs, int mark_reflog,
>  	/* Add all external refs */
>  	for_each_ref(add_one_ref, revs);
>  
> +	/* detached HEAD is not included in the list above */
> +	head_ref(add_one_ref, revs);
> +
>  	/* Add all reflog info */
>  	if (mark_reflog)
>  		for_each_reflog(add_one_reflog, revs);

Looks obviously correct.

Show 25 quoted lines

> diff --git a/t/t5312-prune-detached.sh b/t/t5312-prune-detached.sh
> new file mode 100755
> index 0000000..fac93e1
> --- /dev/null
> +++ b/t/t5312-prune-detached.sh
> @@ -0,0 +1,19 @@
> +#!/bin/sh
> +
> +test_description='no prune detached head without reflog'
> +. ./test-lib.sh
> +
> +test_expect_success 'make repo' '
> +	git config core.logAllRefUpdates false
> +	git commit --allow-empty -m commit1 &&
> +	git commit --allow-empty -m commit2 &&
> +	git checkout  --detach master &&
> +	git commit --allow-empty -m commit3
> +'
> +
> +test_expect_success 'prune does not delete anything' '
> +	git prune -n >prune_actual &&
> +	: >prune_expected &&
> +	test_cmp prune_expected prune_actual'
> +
> +test_done

Your test looks reasonable, but is there any reason it cannot go in t5304 with the other prune tests?

-Peff

Max Kirillov· Aug 31, 2014, 21:16 UTC · re: Jeff King · lore

[PATCH v2] reachable.c: add HEAD to reachability starting commits

HEAD is not explicitly used as a starting commit for calculating reachability, so if it's detached and reflogs are disabled it may be pruned.

Add tests which demonstrate it. Test 'prune: prune former HEAD after checking out branch' also reverts changes to repository.

Signed-off-by: Max Kirillov <max@max630.net>
---
Inserted test into existing script.
 reachable.c      |  3 +++
 t/t5304-prune.sh | 21 +++++++++++++++++++++
 2 files changed, 24 insertions(+)

Show changes to 2 files +24 −0

reachable.c, t/t5304-prune.sh

diff --git a/reachable.c b/reachable.c
index 654a8c5..6f6835b 100644
--- a/reachable.c
+++ b/reachable.c
@@ -229,6 +229,9 @@ void mark_reachable_objects(struct rev_info *revs, int mark_reflog,
 	/* Add all external refs */
 	for_each_ref(add_one_ref, revs);
 
+	/* detached HEAD is not included in the list above */
+	head_ref(add_one_ref, revs);
+
 	/* Add all reflog info */
 	if (mark_reflog)
 		for_each_reflog(add_one_reflog, revs);
diff --git a/t/t5304-prune.sh b/t/t5304-prune.sh
index 377d3d3..77cf064 100755
--- a/t/t5304-prune.sh
+++ b/t/t5304-prune.sh
@@ -104,6 +104,27 @@ test_expect_success 'prune: prune unreachable heads' '
 
 '
 
+test_expect_success 'prune: do not prune detached HEAD with no reflog' '
+
+	git config core.logAllRefUpdates false &&
+	test ! -e .git/logs &&
+	git checkout --detach --quiet &&
+	git commit --allow-empty -m "detached commit" &&
+	git prune -n >prune_actual &&
+	: >prune_expected &&
+	test_cmp prune_actual prune_expected
+
+'
+
+test_expect_success 'prune: prune former HEAD after checking out branch' '
+
+	head_sha1=`git rev-parse HEAD` &&
+	git checkout --quiet master &&
+	git prune -v >prune_actual &&
+	grep -q "$head_sha1" prune_actual
+
+'
+
 test_expect_success 'prune: do not prune heads listed as an argument' '
 
 	: > file2 &&

-- 
2.0.1.1697.g73c6810

Junio C Hamano· Sep 2, 2014, 17:47 UTC · re: Max Kirillov · lore

Re: [PATCH v2] reachable.c: add HEAD to reachability starting commits

Max Kirillov <max@max630.net> writes:

Show 25 quoted lines

> HEAD is not explicitly used as a starting commit for
> calculating reachability, so if it's detached and reflogs
> are disabled it may be pruned.
>
> Add tests which demonstrate it. Test 'prune: prune former HEAD after checking
> out branch' also reverts changes to repository.
>
> Signed-off-by: Max Kirillov <max@max630.net>
> ---
> Inserted test into existing script.
>  reachable.c      |  3 +++
>  t/t5304-prune.sh | 21 +++++++++++++++++++++
>  2 files changed, 24 insertions(+)
>
> diff --git a/reachable.c b/reachable.c
> index 654a8c5..6f6835b 100644
> --- a/reachable.c
> +++ b/reachable.c
> @@ -229,6 +229,9 @@ void mark_reachable_objects(struct rev_info *revs, int mark_reflog,
>  	/* Add all external refs */
>  	for_each_ref(add_one_ref, revs);
>  
> +	/* detached HEAD is not included in the list above */
> +	head_ref(add_one_ref, revs);
> +

Unlike the change to rev-parse, which I haven't decided if I like it, this is definitely the right thing to do.

Thanks for catching.

Show 15 quoted lines

>  	/* Add all reflog info */
>  	if (mark_reflog)
>  		for_each_reflog(add_one_reflog, revs);
> diff --git a/t/t5304-prune.sh b/t/t5304-prune.sh
> index 377d3d3..77cf064 100755
> --- a/t/t5304-prune.sh
> +++ b/t/t5304-prune.sh
> @@ -104,6 +104,27 @@ test_expect_success 'prune: prune unreachable heads' '
>  
>  '
>  
> +test_expect_success 'prune: do not prune detached HEAD with no reflog' '
> +
> +	git config core.logAllRefUpdates false &&
> +	test ! -e .git/logs &&

This test is somewhat iffy; if you had it just before "git prune", I would understand that it matches what the title of the test claims, though.

Show 11 quoted lines

> +	git checkout --detach --quiet &&
> +	git commit --allow-empty -m "detached commit" &&
> +	git prune -n >prune_actual &&
> +	: >prune_expected &&
> +	test_cmp prune_actual prune_expected
> +
> +'
> +
> +test_expect_success 'prune: prune former HEAD after checking out branch' '
> +
> +	head_sha1=`git rev-parse HEAD` &&

Favor $(...) over `...`.

> +	git checkout --quiet master &&
> +	git prune -v >prune_actual &&
> +	grep -q "$head_sha1" prune_actual

Please don't use "grep -q"; unless running the test with "-v", we wouldn't have to see the output anyway.

Show 6 quoted lines

> +
> +'
> +
>  test_expect_success 'prune: do not prune heads listed as an argument' '
>  
>  	: > file2 &&

Max Kirillov· Sep 3, 2014, 16:14 UTC · re: Junio C Hamano · lore

[PATCH v3] reachable.c: add HEAD to reachability starting commits

HEAD is not explicitly used as a starting commit for calculating reachability, so if it's detached and reflogs are disabled it may be pruned.

Add tests which demonstrate it. Test 'prune: prune former HEAD after checking out branch' also reverts changes to repository.

Signed-off-by: Max Kirillov <max@max630.net>
---
Fixed the notes.

(This filesystem stuff would not be needed if I had a separated repository
without reflogs from scratch, like in v1. Having to handle leftouts from
previous test cases does not add clarity here. Maybe it worth extracting all
cases with no reflogs into a separated script.)
 reachable.c      |  3 +++
 t/t5304-prune.sh | 22 ++++++++++++++++++++++
 2 files changed, 25 insertions(+)

Show changes to 2 files +25 −0

reachable.c, t/t5304-prune.sh

diff --git a/reachable.c b/reachable.c
index 654a8c5..6f6835b 100644
--- a/reachable.c
+++ b/reachable.c
@@ -229,6 +229,9 @@ void mark_reachable_objects(struct rev_info *revs, int mark_reflog,
 	/* Add all external refs */
 	for_each_ref(add_one_ref, revs);
 
+	/* detached HEAD is not included in the list above */
+	head_ref(add_one_ref, revs);
+
 	/* Add all reflog info */
 	if (mark_reflog)
 		for_each_reflog(add_one_reflog, revs);
diff --git a/t/t5304-prune.sh b/t/t5304-prune.sh
index 377d3d3..01c6a3f 100755
--- a/t/t5304-prune.sh
+++ b/t/t5304-prune.sh
@@ -104,6 +104,28 @@ test_expect_success 'prune: prune unreachable heads' '
 
 '
 
+test_expect_success 'prune: do not prune detached HEAD with no reflog' '
+
+	git checkout --detach --quiet &&
+	git commit --allow-empty -m "detached commit" &&
+	# verify that there is no reflogs
+	# (should be removed and disabled by previous test)
+	test ! -e .git/logs &&
+	git prune -n >prune_actual &&
+	: >prune_expected &&
+	test_cmp prune_actual prune_expected
+
+'
+
+test_expect_success 'prune: prune former HEAD after checking out branch' '
+
+	head_sha1=$(git rev-parse HEAD) &&
+	git checkout --quiet master &&
+	git prune -v >prune_actual &&
+	grep "$head_sha1" prune_actual
+
+'
+
 test_expect_success 'prune: do not prune heads listed as an argument' '
 
 	: > file2 &&

-- 
2.0.1.1697.g73c6810