* [PATCH v2 4/4] gc: remove broken symrefs
2015-09-28 14:01 ` [PATCH v2 0/4] Fix gc failure when a remote HEAD goes stale Johannes Schindelin
@ 2015-09-28 14:02 ` Johannes Schindelin
2015-09-28 18:41 ` Junio C Hamano
2015-09-28 19:03 ` Jeff King
0 siblings, 2 replies; 8+ messages in thread
From: Johannes Schindelin @ 2015-09-28 14:02 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, git
When encountering broken symrefs, such as a stale remote HEAD (which can
happen if the active branch was renamed in the remote), it is more
helpful to remove those symrefs than to exit with an error.
This fixes https://github.com/git-for-windows/git/issues/423
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
builtin/prune.c | 12 +++++++++++-
t/t6500-gc.sh | 2 +-
2 files changed, 12 insertions(+), 2 deletions(-)
diff --git a/builtin/prune.c b/builtin/prune.c
index d6f664f..337b12a 100644
--- a/builtin/prune.c
+++ b/builtin/prune.c
@@ -6,6 +6,7 @@
#include "reachable.h"
#include "parse-options.h"
#include "progress.h"
+#include "refs.h"
static const char * const prune_usage[] = {
N_("git prune [-n] [-v] [--expire <time>] [--] [<head>...]"),
@@ -100,6 +101,7 @@ static void remove_temporary_files(const char *path)
int cmd_prune(int argc, const char **argv, const char *prefix)
{
struct rev_info revs;
+ struct string_list broken_symrefs = STRING_LIST_INIT_DUP;
struct progress *progress = NULL;
const struct option options[] = {
OPT__DRY_RUN(&show_only, N_("do not remove, show only")),
@@ -110,6 +112,7 @@ int cmd_prune(int argc, const char **argv, const char *prefix)
OPT_END()
};
char *s;
+ int i;
expire = ULONG_MAX;
save_commit_buffer = 0;
@@ -136,7 +139,14 @@ int cmd_prune(int argc, const char **argv, const char *prefix)
if (show_progress)
progress = start_progress_delay(_("Checking connectivity"), 0, 0, 2);
- mark_reachable_objects(&revs, 1, expire, progress, NULL);
+ revs.ignore_missing = 1;
+ mark_reachable_objects(&revs, 1, expire, progress, &broken_symrefs);
+ for (i = 0; i < broken_symrefs.nr; i++) {
+ char *path = broken_symrefs.items[i].string;
+ printf("Removing stale ref %s\n", path);
+ if (!show_only && delete_ref(path, NULL, REF_NODEREF))
+ die("Could not remove stale ref %s", path);
+ }
stop_progress(&progress);
for_each_loose_file_in_objdir(get_object_directory(), prune_object,
prune_cruft, prune_subdir, NULL);
diff --git a/t/t6500-gc.sh b/t/t6500-gc.sh
index b736774..0ae4271 100755
--- a/t/t6500-gc.sh
+++ b/t/t6500-gc.sh
@@ -30,7 +30,7 @@ test_expect_success 'gc -h with invalid configuration' '
test_i18ngrep "[Uu]sage" broken/usage
'
-test_expect_failure 'gc removes broken refs/remotes/<name>/HEAD' '
+test_expect_success 'gc removes broken refs/remotes/<name>/HEAD' '
git init remote &&
(
cd remote &&
--
2.5.3.windows.1.3.gc322723
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v2 4/4] gc: remove broken symrefs
2015-09-28 14:02 ` [PATCH v2 4/4] gc: remove broken symrefs Johannes Schindelin
@ 2015-09-28 18:41 ` Junio C Hamano
2015-09-28 18:49 ` Junio C Hamano
2015-09-28 19:03 ` Jeff King
1 sibling, 1 reply; 8+ messages in thread
From: Junio C Hamano @ 2015-09-28 18:41 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Jeff King, git
Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> When encountering broken symrefs, such as a stale remote HEAD (which can
> happen if the active branch was renamed in the remote), it is more
> helpful to remove those symrefs than to exit with an error.
I think this depends on the perspective. One side of me says that a
remote HEAD that points at refs/remotes/origin/topic that no longer
exists is still giving me a valuable information and it should take
a conscious action by the user to remove it, or evne better to
repoint it to a more useful place. And from that point of view,
removing is not all that helpful. Keeping them and not allowing
them to exit with an error would be a real improvement.
On the other hand, I can certainly understand a view that considers
that such a dangling symbolic ref is merely a cruft like any other
cruft, and "gc" is all about removing cruft.
It just feels to me that this is a bit more valuable than other
kinds of cruft, but maybe it is just me.
>
> This fixes https://github.com/git-for-windows/git/issues/423
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> builtin/prune.c | 12 +++++++++++-
> t/t6500-gc.sh | 2 +-
> 2 files changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/prune.c b/builtin/prune.c
> index d6f664f..337b12a 100644
> --- a/builtin/prune.c
> +++ b/builtin/prune.c
> @@ -6,6 +6,7 @@
> #include "reachable.h"
> #include "parse-options.h"
> #include "progress.h"
> +#include "refs.h"
>
> static const char * const prune_usage[] = {
> N_("git prune [-n] [-v] [--expire <time>] [--] [<head>...]"),
> @@ -100,6 +101,7 @@ static void remove_temporary_files(const char *path)
> int cmd_prune(int argc, const char **argv, const char *prefix)
> {
> struct rev_info revs;
> + struct string_list broken_symrefs = STRING_LIST_INIT_DUP;
> struct progress *progress = NULL;
> const struct option options[] = {
> OPT__DRY_RUN(&show_only, N_("do not remove, show only")),
> @@ -110,6 +112,7 @@ int cmd_prune(int argc, const char **argv, const char *prefix)
> OPT_END()
> };
> char *s;
> + int i;
>
> expire = ULONG_MAX;
> save_commit_buffer = 0;
> @@ -136,7 +139,14 @@ int cmd_prune(int argc, const char **argv, const char *prefix)
> if (show_progress)
> progress = start_progress_delay(_("Checking connectivity"), 0, 0, 2);
>
> - mark_reachable_objects(&revs, 1, expire, progress, NULL);
> + revs.ignore_missing = 1;
> + mark_reachable_objects(&revs, 1, expire, progress, &broken_symrefs);
> + for (i = 0; i < broken_symrefs.nr; i++) {
> + char *path = broken_symrefs.items[i].string;
> + printf("Removing stale ref %s\n", path);
> + if (!show_only && delete_ref(path, NULL, REF_NODEREF))
> + die("Could not remove stale ref %s", path);
> + }
> stop_progress(&progress);
> for_each_loose_file_in_objdir(get_object_directory(), prune_object,
> prune_cruft, prune_subdir, NULL);
> diff --git a/t/t6500-gc.sh b/t/t6500-gc.sh
> index b736774..0ae4271 100755
> --- a/t/t6500-gc.sh
> +++ b/t/t6500-gc.sh
> @@ -30,7 +30,7 @@ test_expect_success 'gc -h with invalid configuration' '
> test_i18ngrep "[Uu]sage" broken/usage
> '
>
> -test_expect_failure 'gc removes broken refs/remotes/<name>/HEAD' '
> +test_expect_success 'gc removes broken refs/remotes/<name>/HEAD' '
> git init remote &&
> (
> cd remote &&
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 4/4] gc: remove broken symrefs
2015-09-28 18:41 ` Junio C Hamano
@ 2015-09-28 18:49 ` Junio C Hamano
2015-09-28 19:58 ` Johannes Schindelin
0 siblings, 1 reply; 8+ messages in thread
From: Junio C Hamano @ 2015-09-28 18:49 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Jeff King, git
Junio C Hamano <gitster@pobox.com> writes:
> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
>
>> When encountering broken symrefs, such as a stale remote HEAD (which can
>> happen if the active branch was renamed in the remote), it is more
>> helpful to remove those symrefs than to exit with an error.
>
> I think this depends on the perspective. One side of me says that a
> remote HEAD that points at refs/remotes/origin/topic that no longer
> exists is still giving me a valuable information and it should take
> a conscious action by the user to remove it, or evne better to
> repoint it to a more useful place. And from that point of view,
> removing is not all that helpful. Keeping them and not allowing
> them to exit with an error would be a real improvement.
>
> On the other hand, I can certainly understand a view that considers
> that such a dangling symbolic ref is merely a cruft like any other
> cruft, and "gc" is all about removing cruft.
>
> It just feels to me that this is a bit more valuable than other
> kinds of cruft, but maybe it is just me.
Sorry, it is a bad habit of me to send out without concluding
remark, leaving the recipient hanging without knowing what the next
step should be.
I meant to say that I plan to, and I indeed did, queue these 4
without changes. I am not opposed to the removal so strongly to
reject [4/4].
The above comment was that I just do not know if this is the right
thing to do, or it will be hurting users.
Thanks.
>
>>
>> This fixes https://github.com/git-for-windows/git/issues/423
>>
>> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
>> ---
>> builtin/prune.c | 12 +++++++++++-
>> t/t6500-gc.sh | 2 +-
>> 2 files changed, 12 insertions(+), 2 deletions(-)
>>
>> diff --git a/builtin/prune.c b/builtin/prune.c
>> index d6f664f..337b12a 100644
>> --- a/builtin/prune.c
>> +++ b/builtin/prune.c
>> @@ -6,6 +6,7 @@
>> #include "reachable.h"
>> #include "parse-options.h"
>> #include "progress.h"
>> +#include "refs.h"
>>
>> static const char * const prune_usage[] = {
>> N_("git prune [-n] [-v] [--expire <time>] [--] [<head>...]"),
>> @@ -100,6 +101,7 @@ static void remove_temporary_files(const char *path)
>> int cmd_prune(int argc, const char **argv, const char *prefix)
>> {
>> struct rev_info revs;
>> + struct string_list broken_symrefs = STRING_LIST_INIT_DUP;
>> struct progress *progress = NULL;
>> const struct option options[] = {
>> OPT__DRY_RUN(&show_only, N_("do not remove, show only")),
>> @@ -110,6 +112,7 @@ int cmd_prune(int argc, const char **argv, const char *prefix)
>> OPT_END()
>> };
>> char *s;
>> + int i;
>>
>> expire = ULONG_MAX;
>> save_commit_buffer = 0;
>> @@ -136,7 +139,14 @@ int cmd_prune(int argc, const char **argv, const char *prefix)
>> if (show_progress)
>> progress = start_progress_delay(_("Checking connectivity"), 0, 0, 2);
>>
>> - mark_reachable_objects(&revs, 1, expire, progress, NULL);
>> + revs.ignore_missing = 1;
>> + mark_reachable_objects(&revs, 1, expire, progress, &broken_symrefs);
>> + for (i = 0; i < broken_symrefs.nr; i++) {
>> + char *path = broken_symrefs.items[i].string;
>> + printf("Removing stale ref %s\n", path);
>> + if (!show_only && delete_ref(path, NULL, REF_NODEREF))
>> + die("Could not remove stale ref %s", path);
>> + }
>> stop_progress(&progress);
>> for_each_loose_file_in_objdir(get_object_directory(), prune_object,
>> prune_cruft, prune_subdir, NULL);
>> diff --git a/t/t6500-gc.sh b/t/t6500-gc.sh
>> index b736774..0ae4271 100755
>> --- a/t/t6500-gc.sh
>> +++ b/t/t6500-gc.sh
>> @@ -30,7 +30,7 @@ test_expect_success 'gc -h with invalid configuration' '
>> test_i18ngrep "[Uu]sage" broken/usage
>> '
>>
>> -test_expect_failure 'gc removes broken refs/remotes/<name>/HEAD' '
>> +test_expect_success 'gc removes broken refs/remotes/<name>/HEAD' '
>> git init remote &&
>> (
>> cd remote &&
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 4/4] gc: remove broken symrefs
2015-09-28 14:02 ` [PATCH v2 4/4] gc: remove broken symrefs Johannes Schindelin
2015-09-28 18:41 ` Junio C Hamano
@ 2015-09-28 19:03 ` Jeff King
2015-09-28 20:05 ` Johannes Schindelin
1 sibling, 1 reply; 8+ messages in thread
From: Jeff King @ 2015-09-28 19:03 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Junio C Hamano, git
On Mon, Sep 28, 2015 at 04:02:08PM +0200, Johannes Schindelin wrote:
> @@ -136,7 +139,14 @@ int cmd_prune(int argc, const char **argv, const char *prefix)
> if (show_progress)
> progress = start_progress_delay(_("Checking connectivity"), 0, 0, 2);
>
> - mark_reachable_objects(&revs, 1, expire, progress, NULL);
> + revs.ignore_missing = 1;
> + mark_reachable_objects(&revs, 1, expire, progress, &broken_symrefs);
You should not need this ignore_missing anymore, right?
It is the dangerous thing I mentioned earlier, but I am puzzled why it
does not cause t5312 to fail.
-Peff
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 4/4] gc: remove broken symrefs
2015-09-28 18:49 ` Junio C Hamano
@ 2015-09-28 19:58 ` Johannes Schindelin
2015-10-05 22:06 ` Junio C Hamano
0 siblings, 1 reply; 8+ messages in thread
From: Johannes Schindelin @ 2015-09-28 19:58 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, git
Hi Junio,
On 2015-09-28 20:49, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
>>
>>> When encountering broken symrefs, such as a stale remote HEAD (which can
>>> happen if the active branch was renamed in the remote), it is more
>>> helpful to remove those symrefs than to exit with an error.
>>
>> I think this depends on the perspective. One side of me says that a
>> remote HEAD that points at refs/remotes/origin/topic that no longer
>> exists is still giving me a valuable information and it should take
>> a conscious action by the user to remove it, or evne better to
>> repoint it to a more useful place. And from that point of view,
>> removing is not all that helpful. Keeping them and not allowing
>> them to exit with an error would be a real improvement.
>>
>> On the other hand, I can certainly understand a view that considers
>> that such a dangling symbolic ref is merely a cruft like any other
>> cruft, and "gc" is all about removing cruft.
>>
>> It just feels to me that this is a bit more valuable than other
>> kinds of cruft, but maybe it is just me.
>
> Sorry, it is a bad habit of me to send out without concluding
> remark, leaving the recipient hanging without knowing what the next
> step should be.
>
> I meant to say that I plan to, and I indeed did, queue these 4
> without changes. I am not opposed to the removal so strongly to
> reject [4/4].
>
> The above comment was that I just do not know if this is the right
> thing to do, or it will be hurting users.
Oh, I appreciate your feedback. I am actually not all *that* certain that removing the broken symref is the correct thing. It is this sort of fruitful exchange that allows me to throw out an idea and be relatively certain that something better will come out of v3 or v8 of the patch series than what I had in mind.
To be honest, the most important outcome is probably 2/4 -- which should be enough to fix the issue reported by the Git for Windows user. I could adjust the test so that it no longer insists that `origin/HEAD` be deleted, but still requires that `git gc` succeeds.
I would have no problem to let this sit for a couple of days until the final verdict.
Ciao,
Dscho
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 4/4] gc: remove broken symrefs
2015-09-28 19:03 ` Jeff King
@ 2015-09-28 20:05 ` Johannes Schindelin
0 siblings, 0 replies; 8+ messages in thread
From: Johannes Schindelin @ 2015-09-28 20:05 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, git
Hi Peff,
On 2015-09-28 21:03, Jeff King wrote:
> On Mon, Sep 28, 2015 at 04:02:08PM +0200, Johannes Schindelin wrote:
>
>> @@ -136,7 +139,14 @@ int cmd_prune(int argc, const char **argv, const char *prefix)
>> if (show_progress)
>> progress = start_progress_delay(_("Checking connectivity"), 0, 0, 2);
>>
>> - mark_reachable_objects(&revs, 1, expire, progress, NULL);
>> + revs.ignore_missing = 1;
>> + mark_reachable_objects(&revs, 1, expire, progress, &broken_symrefs);
>
> You should not need this ignore_missing anymore, right?
>
> It is the dangerous thing I mentioned earlier, but I am puzzled why it
> does not cause t5312 to fail.
Gaaaaaah! I edited this out at least twice, yet it creeps back in. Bah!
Junio, would you mind fixing this up locally? If you want me to submit v3, just let me know, I will take care of it tomorrow.
Ciao,
Dscho
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 4/4] gc: remove broken symrefs
2015-09-28 19:58 ` Johannes Schindelin
@ 2015-10-05 22:06 ` Junio C Hamano
0 siblings, 0 replies; 8+ messages in thread
From: Junio C Hamano @ 2015-10-05 22:06 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Jeff King, git
Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> Oh, I appreciate your feedback. I am actually not all *that* certain
> that removing the broken symref is the correct thing. It is this sort
> of fruitful exchange that allows me to throw out an idea and be
> relatively certain that something better will come out of v3 or v8 of
> the patch series than what I had in mind.
>
> To be honest, the most important outcome is probably 2/4 -- which
> should be enough to fix the issue reported by the Git for Windows
> user. I could adjust the test so that it no longer insists that
> origin/HEAD` be deleted, but still requires that `git gc` succeeds.
>
> I would have no problem to let this sit for a couple of days until
> the final verdict.
... and a few days have passed. I am tempted to do the easy bits
first, discarding the parts that both of us feel iffy for now
without prejudice, keeping the first two commits with a bit of
tweak.
The primary tweak is to t6500 in the first patch, which I retitled
below (and the patch shows s/failure/success/ but in the first step
that only adds a failing test, it would of course expect a failure).
And with "pack-objects: do not get distracted", the test will start
passing.
diff --git a/t/t6500-gc.sh b/t/t6500-gc.sh
index b736774..5d7d414 100755
--- a/t/t6500-gc.sh
+++ b/t/t6500-gc.sh
@@ -30,7 +30,7 @@ test_expect_success 'gc -h with invalid configuration' '
test_i18ngrep "[Uu]sage" broken/usage
'
-test_expect_failure 'gc removes broken refs/remotes/<name>/HEAD' '
+test_expect_success 'gc is not aborted due to a stale symref' '
git init remote &&
(
cd remote &&
@@ -39,9 +39,7 @@ test_expect_failure 'gc removes broken refs/remotes/<name>/HEAD' '
git branch -m develop &&
cd ../client &&
git fetch --prune &&
- git gc &&
- git branch --list -r origin/HEAD >actual &&
- test_line_count = 0 actual
+ git gc
)
'
diff --git a/reachable.c b/reachable.c
index 6356ae8..4cfd0de 100644
--- a/reachable.c
+++ b/reachable.c
@@ -28,7 +28,7 @@ static int add_one_ref(const char *path, const unsigned char *sha1, int flag, vo
struct object *object;
if ((flag & REF_ISSYMREF) && (flag & REF_ISBROKEN)) {
- warning("ref is broken: %s", path);
+ warning("symbolic ref is dangling: %s", path);
return 0;
}
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v2 4/4] gc: remove broken symrefs
@ 2015-10-06 13:59 Johannes Schindelin
0 siblings, 0 replies; 8+ messages in thread
From: Johannes Schindelin @ 2015-10-06 13:59 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, git
Hi Junio,
On 2015-10-06 00:06, Junio C Hamano wrote:
> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
>
>> Oh, I appreciate your feedback. I am actually not all *that* certain
>> that removing the broken symref is the correct thing. It is this sort
>> of fruitful exchange that allows me to throw out an idea and be
>> relatively certain that something better will come out of v3 or v8 of
>> the patch series than what I had in mind.
>>
>> To be honest, the most important outcome is probably 2/4 -- which
>> should be enough to fix the issue reported by the Git for Windows
>> user. I could adjust the test so that it no longer insists that
>> origin/HEAD` be deleted, but still requires that `git gc` succeeds.
>>
>> I would have no problem to let this sit for a couple of days until
>> the final verdict.
>
> ... and a few days have passed. I am tempted to do the easy bits
> first, discarding the parts that both of us feel iffy for now
> without prejudice, keeping the first two commits with a bit of
> tweak.
I agree. And I just sent out v3 of the patch series.
Thanks,
Dscho
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2015-10-06 13:59 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-10-06 13:59 [PATCH v2 4/4] gc: remove broken symrefs Johannes Schindelin
-- strict thread matches above, loose matches on Subject: below --
2015-09-25 0:08 [PATCH 4/4] gc: remove broken refs Junio C Hamano
2015-09-28 14:01 ` [PATCH v2 0/4] Fix gc failure when a remote HEAD goes stale Johannes Schindelin
2015-09-28 14:02 ` [PATCH v2 4/4] gc: remove broken symrefs Johannes Schindelin
2015-09-28 18:41 ` Junio C Hamano
2015-09-28 18:49 ` Junio C Hamano
2015-09-28 19:58 ` Johannes Schindelin
2015-10-05 22:06 ` Junio C Hamano
2015-09-28 19:03 ` Jeff King
2015-09-28 20:05 ` Johannes Schindelin
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).