[PATCH 0/4] Recovery after interrupted HTTP(s) fetch

[PATCH 0/4] Recovery after interrupted HTTP(s) fetch

[Sergey Vlasov]

[-- Attachment #1: Type: text/plain, Size: 1431 bytes --]

Hello!

The current implementation of git-http-fetch, git-local-fetch,
git-ssh-fetch has a problem: if a fetch was interrupted in the middle,
some objects in the local repository will be missing, and there is no
easy way to recover.  If you just attempt to repeat the fetch, it will
notice that you already have the most recent commit object, and will
not fetch anything more - the repository will stay broken.  Seems that
the only way to go forward is to prune the repository (which will
remove the fetched objects, because they are not referenced) and
repeat the fetch again from the beginning, but this is obviously
wasteful.

The problem is caused by the behavior of fetch.c:process_commit() - it
skips parent commits which already exist locally.  This is a good
optimization if your repository is OK, but after an interrupted fetch
there will be some missing objects in the object tree; the only way to
find them is to scan the whole tree, so stopping at the existing
commits is wrong.

This set of patches adds the recovery mode to git-*-fetch and the
git-fetch script, which is enabled by the --recover option; in this
mode the program will walk through the whole object tree and try to
download any missing objects.  Obviously, this could take some time,
but it is better than throwing away all already fetched objects after
an error in the middle of a long fetch operation.

-- 
Sergey Vlasov

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

[PATCH 1/4] Do not try to process objects more than once during fetch

[Sergey Vlasov]

fetch.c:process() is often called more than once for the same object;
e.g., for unpacked git repository with 7313 objects there are 320997
calls to process() for objects which were already seen (and 7528 of
those calls are for tree objects, which is a real problem because of
recursive processing).

---

 fetch.c |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

f8285569e916a21c28379b55415165a89794272a
diff --git a/fetch.c b/fetch.c
--- a/fetch.c
+++ b/fetch.c
@@ -7,6 +7,8 @@
 #include "blob.h"
 #include "refs.h"
 
+#define SEEN		(1u << 0)
+
 const char *write_ref = NULL;
 
 const unsigned char *current_ref = NULL;
@@ -126,6 +128,12 @@ static int process_object(struct object 
 static int process(unsigned char *sha1, const char *type)
 {
 	struct object *obj = lookup_object_type(sha1, type);
+
+	/* Do not try to process objects more than once */
+	if (obj->flags & SEEN)
+		return 0;
+	obj->flags |= SEEN;
+
 	if (has_sha1_file(sha1)) {
 		parse_object(sha1);
 		/* We already have it, so we should scan it now. */

[PATCH 2/4] git-*-fetch: Gracefully recover from retrieval failure

[Sergey Vlasov]

If the fetch process is interrupted in the middle after retrieving
some commits, there was no easy way to recover by re-running fetch,
because fetch.c:process_commit() stops at commits which were already
retrieved.

This patch adds the --recover option to git-*-fetch family; with this
option git-*-fetch will walk the whole commit tree instead of stopping
at existing commits, so that missing commits in the middle (and trees
and blobs referenced by them) can be discovered and fetched.

---

 This patch really requires "[PATCH 1/4] Do not try to process objects
 more than once during fetch" - otherwise commit objects are processed
 zillion times, and the fetch just never completes.


 Documentation/git-http-fetch.txt  |    3 +++
 Documentation/git-local-fetch.txt |    3 +++
 Documentation/git-ssh-fetch.txt   |    3 +++
 fetch.c                           |    4 +++-
 fetch.h                           |    6 ++++++
 http-fetch.c                      |    2 ++
 local-fetch.c                     |    2 ++
 ssh-fetch.c                       |    2 ++
 8 files changed, 24 insertions(+), 1 deletions(-)

2b0b8ebb6bba7ac390cccc35a026acf0c4ee6c56
diff --git a/Documentation/git-http-fetch.txt b/Documentation/git-http-fetch.txt
--- a/Documentation/git-http-fetch.txt
+++ b/Documentation/git-http-fetch.txt
@@ -21,6 +21,9 @@ Downloads a remote GIT repository via HT
 	Get trees associated with the commit objects.
 -a::
 	Get all the objects.
+--recover::
+	Check all referenced commits instead of stopping at existing
+	ones to recover after earlier fetch that was interrupted.
 -v::
 	Report what is downloaded.
 
diff --git a/Documentation/git-local-fetch.txt b/Documentation/git-local-fetch.txt
--- a/Documentation/git-local-fetch.txt
+++ b/Documentation/git-local-fetch.txt
@@ -23,6 +23,9 @@ OPTIONS
 	Get trees associated with the commit objects.
 -a::
 	Get all the objects.
+--recover::
+	Check all referenced commits instead of stopping at existing
+	ones to recover after earlier fetch that was interrupted.
 -v::
 	Report what is downloaded.
 
diff --git a/Documentation/git-ssh-fetch.txt b/Documentation/git-ssh-fetch.txt
--- a/Documentation/git-ssh-fetch.txt
+++ b/Documentation/git-ssh-fetch.txt
@@ -31,6 +31,9 @@ commit-id::
 	Get trees associated with the commit objects.
 -a::
 	Get all the objects.
+--recover::
+	Check all referenced commits instead of stopping at existing
+	ones to recover after earlier fetch that was interrupted.
 -v::
 	Report what is downloaded.
 -w::
diff --git a/fetch.c b/fetch.c
--- a/fetch.c
+++ b/fetch.c
@@ -16,6 +16,7 @@ const unsigned char *current_ref = NULL;
 int get_tree = 0;
 int get_history = 0;
 int get_all = 0;
+int get_recover = 0;
 int get_verbosely = 0;
 static unsigned char current_commit_sha1[20];
 
@@ -80,7 +81,8 @@ static int process_commit(struct commit 
 	if (get_history) {
 		struct commit_list *parents = commit->parents;
 		for (; parents; parents = parents->next) {
-			if (has_sha1_file(parents->item->object.sha1))
+			if (!get_recover &&
+			    has_sha1_file(parents->item->object.sha1))
 				continue;
 			if (process(parents->item->object.sha1,
 				    commit_type))
diff --git a/fetch.h b/fetch.h
--- a/fetch.h
+++ b/fetch.h
@@ -37,6 +37,12 @@ extern int get_history;
 /* Set to fetch the trees in the commit history. */
 extern int get_all;
 
+/*
+ * Set to walk the whole commit tree without stopping at commits we already
+ * have (so that we could detect and fetch missing commits in the middle).
+ */
+extern int get_recover;
+
 /* Set to be verbose */
 extern int get_verbosely;
 
diff --git a/http-fetch.c b/http-fetch.c
--- a/http-fetch.c
+++ b/http-fetch.c
@@ -341,6 +341,8 @@ int main(int argc, char **argv)
 		} else if (argv[arg][1] == 'w') {
 			write_ref = argv[arg + 1];
 			arg++;
+		} else if (!strcmp(argv[arg], "--recover")) {
+			get_recover = 1;
 		}
 		arg++;
 	}
diff --git a/local-fetch.c b/local-fetch.c
--- a/local-fetch.c
+++ b/local-fetch.c
@@ -211,6 +211,8 @@ int main(int argc, char **argv)
 			get_verbosely = 1;
 		else if (argv[arg][1] == 'w')
 			write_ref = argv[++arg];
+		else if (!strcmp(argv[arg], "--recover"))
+			get_recover = 1;
 		else
 			usage(local_pull_usage);
 		arg++;
diff --git a/ssh-fetch.c b/ssh-fetch.c
--- a/ssh-fetch.c
+++ b/ssh-fetch.c
@@ -106,6 +106,8 @@ int main(int argc, char **argv)
 		} else if (argv[arg][1] == 'w') {
 			write_ref = argv[arg + 1];
 			arg++;
+		} else if (!strcmp(argv[arg], "--recover")) {
+			get_recover = 1;
 		}
 		arg++;
 	}

[PATCH 3/4] git-fetch: Add --recover option

[Sergey Vlasov]

Make git-fetch accept the --recover option and pass it to
git-http-fetch (for other protocols this option is ignored).

---

 I would add the documentation in the same patch, but noticed that
 other options of git-fetch were not documented, so I made a separate
 patch which documents all of them.


 git-fetch.sh |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

e19be8e89d20745918c76c4b587c286cef88d023
diff --git a/git-fetch.sh b/git-fetch.sh
--- a/git-fetch.sh
+++ b/git-fetch.sh
@@ -8,6 +8,7 @@ _x40="$_x40$_x40$_x40$_x40$_x40$_x40$_x4
 append=
 force=
 update_head_ok=
+recover_option=
 while case "$#" in 0) break ;; esac
 do
 	case "$1" in
@@ -22,6 +23,9 @@ do
 	--update-head-o|--update-head-ok)
 		update_head_ok=t
 		;;
+	-r|--r|--re|--rec|--reco|--recov|--recove|--recover)
+		recover_option="--recover"
+		;;
 	*)
 		break
 		;;
@@ -179,7 +183,7 @@ do
 	expr "$head" : "$_x40\$" >/dev/null ||
 		die "Failed to fetch $remote_name from $remote"
 	echo Fetching "$remote_name from $remote" using http
-	git-http-fetch -v -a "$head" "$remote/" || exit
+	git-http-fetch -v -a $recover_option "$head" "$remote/" || exit
 	;;
     rsync://*)
 	TMP_HEAD="$GIT_DIR/TMP_HEAD"

[PATCH 4/4] Document git-fetch options

[Sergey Vlasov]

Add documentation for git-fetch options (including the newly added
--recover option).

---

 Documentation/git-fetch.txt        |    6 ++++++
 Documentation/pull-fetch-param.txt |   19 +++++++++++++++++++
 2 files changed, 25 insertions(+), 0 deletions(-)

7685a3d50566e908fb8beb2ddb3088877aa9ae0f
diff --git a/Documentation/git-fetch.txt b/Documentation/git-fetch.txt
--- a/Documentation/git-fetch.txt
+++ b/Documentation/git-fetch.txt
@@ -26,6 +26,12 @@ OPTIONS
 -------
 include::pull-fetch-param.txt[]
 
+-u, \--update-head-ok::
+	By default 'git-fetch' refuses to update the head which
+	corresponds to the current branch.  This flag disables the
+	check.  Note that fetching into the current branch will not
+	update the index and working directory, so use it with care.
+
 
 Author
 ------
diff --git a/Documentation/pull-fetch-param.txt b/Documentation/pull-fetch-param.txt
--- a/Documentation/pull-fetch-param.txt
+++ b/Documentation/pull-fetch-param.txt
@@ -80,3 +80,22 @@
           <ref>: when pulling/fetching, and <ref>:<ref> when
           pushing.  That is, do not store it locally if
           fetching, and update the same name if pushing.
+
+-a, \--append::
+	Append ref names and object names of fetched refs to the
+	existing contents of $GIT_DIR/FETCH_HEAD.  Without this
+	option old data in $GIT_DIR/FETCH_HEAD will be overwritten.
+
+-f, \--force::
+	Usually, the command refuses to update a local ref that is
+	not an ancestor of the remote ref used to overwrite it.
+	This flag disables the check.  What this means is that the
+	local repository can lose commits; use it with care.
+
+-r, \--recover::
+	Recover after an interrupted fetch.  When fetching over
+	HTTP(s), this option is passed to 'git-http-fetch', which
+	will then walk through the whole commit tree and fetch all
+	missing objects.  For other protocols this option is
+	ignored, because they don't have problems with recovery
+	after an interrupted fetch.

Re: [PATCH 0/4] Recovery after interrupted HTTP(s) fetch

[Junio C Hamano]

Sergey Vlasov <vsu@altlinux.ru> writes:

> The problem is caused by the behavior of fetch.c:process_commit() - it
> skips parent commits which already exist locally.  This is a good
> optimization if your repository is OK, but after an interrupted fetch
> there will be some missing objects in the object tree; the only way to
> find them is to scan the whole tree, so stopping at the existing
> commits is wrong.

Hmph.  I could almost swear that I've seen the 'recovery' fix
from Daniel and merged it some time ago -- but I do not see that
in the git-whatchanged output.  The only '--recover' I see is
the leftover command line option description to recover from
delta-fetch failure, which would not happen because we do not
have deltified objects anymore.

Thanks for the patch.

Re: [PATCH 0/4] Recovery after interrupted HTTP(s) fetch

[Linus Torvalds]

On Wed, 14 Sep 2005, Junio C Hamano wrote:
> 
> Thanks for the patch.

This is wrong.

Any "fetch" logic that writes the refs before all the objects are gathered 
is _buggy_. It's not about "recovery", it should never do that in the 
first place.

Is it just the http one that is this broken?

		Linus

Re: [PATCH 0/4] Recovery after interrupted HTTP(s) fetch

[Junio C Hamano]

Linus Torvalds <torvalds@osdl.org> writes:

> Any "fetch" logic that writes the refs before all the objects are gathered 
> is _buggy_. It's not about "recovery", it should never do that in the 
> first place.

I agree about refs, but in this case what is stored, and is used
as a signal not to refetch to the next run, is the commit
object.  Ideally we should defer creating trees until we store
all blobs and subtrees, and commits until we do its tree, but
the fetch.c (formerly called pull.c) is not written that way
from day one.

> Is it just the http one that is this broken?

I belive all of the commit walkers share this property.

Re: [PATCH 0/4] Recovery after interrupted HTTP(s) fetch

[Daniel Barkalow]

On Wed, 14 Sep 2005, Linus Torvalds wrote:

> On Wed, 14 Sep 2005, Junio C Hamano wrote:
> > 
> > Thanks for the patch.
> 
> This is wrong.
> 
> Any "fetch" logic that writes the refs before all the objects are gathered 
> is _buggy_. It's not about "recovery", it should never do that in the 
> first place.

It's not about the refs at all. The issue is that, when it reaches a 
commit that we already have, it assumes that we have everything that 
commit references and stops there. It really ought to not do that, but I 
think it might be too slow if it goes through everything all the time. We 
probably want to keep a cache listing some commits we have, and use that.

	-Daniel
*This .sig left intentionally blank*

Re: [PATCH 0/4] Recovery after interrupted HTTP(s) fetch

[Linus Torvalds]

On Wed, 14 Sep 2005, Daniel Barkalow wrote:
> 
> It's not about the refs at all. The issue is that, when it reaches a 
> commit that we already have, it assumes that we have everything that 
> commit references and stops there. It really ought to not do that, but I 
> think it might be too slow if it goes through everything all the time.

No, you don't need to go through everything all the time.

Do what the pack-sending stuff does: it assumes that it has everything
that is reachable from the _old_ refs. It doesn't walk all the way to the 
root, it "just" walks far enough that it can ignore anything that was 
reachable from the old refs.

Yes, it's slightly more complex than assuming "oh, I have this object, so
I must have all of its ancestors", but it's a lot safer. Then the rule
just becomes: only update refs once you've fetched all the objects.

		Linus

Re: [PATCH 0/4] Recovery after interrupted HTTP(s) fetch

[Daniel Barkalow]

On Wed, 14 Sep 2005, Linus Torvalds wrote:

> On Wed, 14 Sep 2005, Daniel Barkalow wrote:
> > 
> > It's not about the refs at all. The issue is that, when it reaches a 
> > commit that we already have, it assumes that we have everything that 
> > commit references and stops there. It really ought to not do that, but I 
> > think it might be too slow if it goes through everything all the time.
> 
> No, you don't need to go through everything all the time.
> 
> Do what the pack-sending stuff does: it assumes that it has everything
> that is reachable from the _old_ refs. It doesn't walk all the way to the 
> root, it "just" walks far enough that it can ignore anything that was 
> reachable from the old refs.

Oh, right. Yeah, that's the obvious solution. I'll do that tonight.

	-Daniel
*This .sig left intentionally blank*

Re: [PATCH 0/4] Recovery after interrupted HTTP(s) fetch

[Junio C Hamano]

Daniel Barkalow <barkalow@iabervon.org> writes:

> On Wed, 14 Sep 2005, Linus Torvalds wrote:
>
>> Do what the pack-sending stuff does: it assumes that it has everything
>> that is reachable from the _old_ refs. It doesn't walk all the way to the 
>> root, it "just" walks far enough that it can ignore anything that was 
>> reachable from the old refs.
>
> Oh, right. Yeah, that's the obvious solution. I'll do that tonight.

Thanks.  In the meantime I have Sergey's patch in the "pu"
branch but we can replace it with your fix.

Re: [PATCH 0/4] Recovery after interrupted HTTP(s) fetch

[Matthias Urlichs]

Hi, Junio C Hamano wrote:

> Daniel Barkalow <barkalow@iabervon.org> writes:
>> Oh, right. Yeah, that's the obvious solution. I'll do that tonight.
> 
> Thanks.  In the meantime I have Sergey's patch in the "pu"
> branch but we can replace it with your fix.

Personally I'd rather combine the two. The point being, broken
repositories do happen -- for instance, when the file system is
inconsistent after a crash, or when the referent of an info/alternates
directory vanishes. :-/

I'd rather have a simple repair flag than do a "download everything again
and then throw most of it away" job when something like that happens.

-- 
Matthias Urlichs   |   {M:U} IT Design @ m-u-it.de   |  smurf@smurf.noris.de
Disclaimer: The quote was selected randomly. Really. | http://smurf.noris.de
 - -
Experience keeps a dear school, but it's a hell of a campaign tactic.
					-- Poor Jimmy's Almanac

Re: [PATCH 0/4] Recovery after interrupted HTTP(s) fetch

[Daniel Barkalow]

On Thu, 15 Sep 2005, Matthias Urlichs wrote:

> Hi, Junio C Hamano wrote:
> 
> > Daniel Barkalow <barkalow@iabervon.org> writes:
> >> Oh, right. Yeah, that's the obvious solution. I'll do that tonight.
> > 
> > Thanks.  In the meantime I have Sergey's patch in the "pu"
> > branch but we can replace it with your fix.
> 
> Personally I'd rather combine the two. The point being, broken
> repositories do happen -- for instance, when the file system is
> inconsistent after a crash, or when the referent of an info/alternates
> directory vanishes. :-/
> 
> I'd rather have a simple repair flag than do a "download everything again
> and then throw most of it away" job when something like that happens.

Agreed; but we want a new patch for --recover, because with the new code, 
the easy way is to skip the for_each_ref call, at which point it will just 
not assume you have anything.

	-Daniel
*This .sig left intentionally blank*

Re: [PATCH 0/4] Recovery after interrupted HTTP(s) fetch

[Junio C Hamano]

Matthias Urlichs <smurf@smurf.noris.de> writes:

> Personally I'd rather combine the two. The point being, broken
> repositories do happen -- for instance, when the file system is
> inconsistent after a crash, or when the referent of an info/alternates
> directory vanishes. :-/
>
> I'd rather have a simple repair flag than do a "download everything again
> and then throw most of it away" job when something like that happens.

If you are talking about disaster recovery, wouldn't you rather
clone the remote into a clean repository, and fetch what are
salvageable from the potentially corrupt old repository into
this clean repository you know is OK?  I think that would
suggest that you do not need another "download everything again"
added to git-http-fetch.  You can already run git-clone from the
remote to "download everything again", and then git-fetch from
the repository being salvaged.