Unresolved issues

I tagged 1.5.5-rc0 Sunday night (my time, obviously).

Our contributors have been busy inventing new features and reinventing old
ones in C during the 1.5.5 cycle, and we have a fair number of known
breakages.  Here is a short list of issues I know (or I think I've heard)
about, that we would like to address (either "resolve", or "declare to
postpone") before the final release, but I am sure I missed some things.

Let's hope contributors are as responsive in fixing their own mess as they
are responsive in scratching their own itch, and we can resolve most of
them shortly.

* synopsys: use {} instead of () for grouping alternatives (Jari Aalto)
  $gmane/72243

  This was discussed during 1.5.4 pre-freeze timeframe but never
  materialized.

* "git remote" showing remotes/origin/HEAD as a candidate for pruning,
  and pruning it results in removal of what is pointed at by it.

  Pointers?  This may not be a regression but bug-to-bug compatibility
  with the older implementation, but this should better be fixed.

* fetch with "refs/*:refs/*" errors out erroneously
  $gmane/77335

  Breakage exposed by recent git allowing "mirror" layout with "git remote
  add --mirror".

* fetch with tag following uses smudged object database
  $gmane/74141

  Regression introduced by recent C-rewrite of git-fetch.

* "git fetch" does not exit with non-zero status when it failed to update
  some refs due to non-ffness
  $gmane/77178

  Regression introduced by recent C-rewrite of git-fetch.

* "git fetch" shows error when dangling symref exists at the remote
  but does not really error out
  $gmane/76658

  I am not sure what the right course of action is.  Maybe we should
  ignore dangling symrefs in upload-pack?

* D/F conflict to merge a tree with D into a tree with F
  $gmane/77352

  Needs more info.

* revision.c::limit_list() breakage
  $gmane/72324
  t/t6009

  When you run "git rev-list A..B C", and there is a commit in the chain
  between A and B whose timestamp is much older than its parent, sometimes
  we fail to mark C as reachable from A (hence not interesting) even when
  it actualy is.  This is very expensive to solve in general, and we are
  not going to introduce "generation number" field to the commit objects,
  so we may have to settle with a heuristic.

* "[alias] st = status" and "cd .git && git st" (Jeff King)
  $gmane/72327

  This shows everything as deleted, I believe it hasn't resolved.  I am
  not sure if this is worth resolving, though.

On Mon, Mar 17, 2008 at 06:12:02PM -0700, Junio C Hamano wrote:

* "[alias] st = status" and "cd .git && git st" (Jeff King)
  $gmane/72327

  This shows everything as deleted, I believe it hasn't resolved.  I am
  not sure if this is worth resolving, though.

I started on a patch for this, but got mired in trying to clean up the
worktree handling in this area, which is quite confusing and fragile. I
have a suspicion that Duy's recent patch series will impact this, but I
haven't had time yet to review that carefully.

-Peff

On Mon, 17 Mar 2008, Junio C Hamano wrote:

* revision.c::limit_list() breakage
  $gmane/72324
  t/t6009

  When you run "git rev-list A..B C", and there is a commit in the chain
  between A and B whose timestamp is much older than its parent, sometimes
  we fail to mark C as reachable from A (hence not interesting) even when
  it actualy is.  This is very expensive to solve in general, and we are
  not going to introduce "generation number" field to the commit objects,
  so we may have to settle with a heuristic.

Here is the already posted heuristic that fixes both t/t6009 and the 
real-world case that triggered the whole discussion.

It's certainly not perfect, but I think it's likely an improvement on what 
we have now, and it should be robust in the face of the _occasional_ wrong 
date.

Now, if there are consistently totally bogus dates, the SLOP thing won't 
help, but ...

		Linus
---
From: Linus Torvalds <torvalds@woody.linux-foundation.org>

Make revision limiting more robust against occasional bad commit dates

The revision limiter uses the commit date to decide when it has seen
enough commits to finalize the revision list, but that can get confused
if there are incorrect dates far in the past on some commits.

This makes the logic a bit more robust by

 - we always walk an extra SLOP commits from the source list even if we
   decide that the source list is probably all done (unless the source is
   entirely empty, of course, because then we really can't do anything at
   all)

 - we keep track of the date of the last commit we added to the
   destination list (this will *generally* be the oldest entry we've seen
   so far)

 - we compare that with the youngest entry (the first one) of the source
   list, and if the destination is older than the source, we know we want
   to look at the source.

which causes occasional date mishaps to be handled cleanly.

Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
---
 revision.c |   46 +++++++++++++++++++++++++++++++++++++---------
 1 files changed, 37 insertions(+), 9 deletions(-)

diff --git a/revision.c b/revision.c
index 63bf2c5..196fedc 100644
--- a/revision.c
+++ b/revision.c

@@ -564,14 +564,39 @@ static void cherry_pick_list(struct commit_list *list, struct rev_info *revs)
 	free_patch_ids(&ids);
 }
 
-static void add_to_list(struct commit_list **p, struct commit *commit, struct commit_list *n)
+/* How many extra uninteresting commits we want to see.. */
+#define SLOP 5
+
+static int still_interesting(struct commit_list *src, unsigned long date, int slop)
 {
-	p = &commit_list_insert(commit, p)->next;
-	*p = n;
+	/*
+	 * No source list at all? We're definitely done..
+	 */
+	if (!src)
+		return 0;
+
+	/*
+	 * Does the destination list contain entries with a date
+	 * before the source list? Definitely _not_ done.
+	 */
+	if (date < src->item->date)
+		return SLOP;
+
+	/*
+	 * Does the source list still have interesting commits in
+	 * it? Definitely not done..
+	 */
+	if (!everybody_uninteresting(src))
+		return SLOP;
+
+	/* Ok, we're closing in.. */
+	return slop-1;
 }
 
 static int limit_list(struct rev_info *revs)
 {
+	int slop = SLOP;
+	unsigned long date = ~0ul;
 	struct commit_list *list = revs->commits;
 	struct commit_list *newlist = NULL;
 	struct commit_list **p = &newlist;

@@ -591,16 +616,19 @@ static int limit_list(struct rev_info *revs)
 			return -1;
 		if (obj->flags & UNINTERESTING) {
 			mark_parents_uninteresting(commit);
-			if (everybody_uninteresting(list)) {
-				if (revs->show_all)
-					add_to_list(p, commit, list);
-				break;
-			}
-			if (!revs->show_all)
-				continue;
+			if (revs->show_all)
+				p = &commit_list_insert(commit, p)->next;
+			slop = still_interesting(list, date, slop);
+			if (slop)
+				continue;
+			/* If showing all, add the whole pending list to the end */
+			if (revs->show_all)
+				*p = list;
+			break;
 		}
 		if (revs->min_age != -1 && (commit->date > revs->min_age))
 			continue;
+		date = commit->date;
 		p = &commit_list_insert(commit, p)->next;
 
 		show = show_early_output;

For symbolic refs, a sane notion of being "stale" is that the ref
they point to no longer exists.  Since this is checked already,
"remote show" does not need to show them at all.

Incidentally, this fixes the issue that "HEAD" was shown as a
stale ref by "remote show" in a freshly cloned repository.

Signed-off-by: Johannes Schindelin <redacted>
---

	On Mon, 17 Mar 2008, Junio C Hamano wrote:

	> "git remote" showing remotes/origin/HEAD as a candidate for 
	> pruning, and pruning it results in removal of what is pointed at 
	> by it.
	>
	> Pointers?  This may not be a regression but bug-to-bug 
	> compatibility with the older implementation, but this should 
	> better be fixed.

	How about this?

 builtin-remote.c  |    5 ++++-
 t/t5505-remote.sh |   10 ++++++++++
 2 files changed, 14 insertions(+), 1 deletions(-)

diff --git a/builtin-remote.c b/builtin-remote.c
index 8e7fa6a..67139ec 100644
--- a/builtin-remote.c
+++ b/builtin-remote.c

@@ -210,7 +210,10 @@ static int handle_one_branch(const char *refname,
 	if (!remote_find_tracking(states->remote, &refspec)) {
 		struct path_list_item *item;
 		const char *name = skip_prefix(refspec.src, "refs/heads/");
-		if (unsorted_path_list_has_path(&states->tracked, name) ||
+		/* symbolic refs pointing nowhere were handled already */
+		if ((flags & REF_ISSYMREF) ||
+				unsorted_path_list_has_path(&states->tracked,
+					name) ||
 				unsorted_path_list_has_path(&states->new,
 					name))
 			return 0;

diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh
index ecfc999..004a8dc 100755
--- a/t/t5505-remote.sh
+++ b/t/t5505-remote.sh

@@ -237,4 +237,14 @@ test_expect_success 'update default (overridden, with funny whitespace)' '
 
 '
 
+test_expect_success '"remote show" does not show symbolic refs' '
+
+	git clone one three &&
+	(cd three &&
+	 git remote show origin > output &&
+	 ! grep HEAD < output &&
+	 ! grep -i stale < output)
+
+'
+
 test_done

-- 
1.5.5.rc0.173.g90d70

Linus Torvalds wrote:

quoted

  When you run "git rev-list A..B C", and there is a commit in the chain
  between A and B whose timestamp is much older than its parent, sometimes
  we fail to mark C as reachable from A (hence not interesting) even when
  it actualy is.  This is very expensive to solve in general, and we are
  not going to introduce "generation number" field to the commit objects,
  so we may have to settle with a heuristic.

Here is the already posted heuristic that fixes both t/t6009 and the 
real-world case that triggered the whole discussion.
It's certainly not perfect, but I think it's likely an improvement on what 
we have now, and it should be robust in the face of the _occasional_ wrong 
date.
Now, if there are consistently totally bogus dates, the SLOP thing won't 
help, but ...

Ouch - I had always supposed that topology was king, and that the commit
dates were purely informational.  In particular the Perl history that I
produced in general takes a position of blatant and wanton disregard to
such consistency.

I can't find the other thread you refer to - is there a good summary of
the issues somewhere?  The test script is not very descriptive.

If timewise out-of-order commits are bad, perhaps git-filter-branch
should warn when it is creating histories whose topology disagrees with
their chronology... and also the user manual should probably describe this.

Sam