patch, 3 partsbisect: rework some rev related functions to make them more reusable

This patches changes the "bisect_rev_setup" and "bisect_common" functions to make it easier to reuse them in a later patch.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 bisect.c |   25 ++++++++++++++-----------
 1 files changed, 14 insertions(+), 11 deletions(-)

This patch adds the "unparse_commit" function that returns a commit into an unparsed state by freeing its data and resetting its fields to 0.

Its parents are recursively unparsed too, because they might have been changed. But its tree is not unparsed as it should not have been modifed.

Note that as the "flags" and "used" fields may be used even if the object is not parsed, we have to reset them anyway.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 commit.c |   20 ++++++++++++++++++++
 commit.h |    2 ++
 2 files changed, 22 insertions(+), 0 deletions(-)

Christian Couder <chriscool@tuxfamily.org> writes:

It is a bug in any codepath if it used commit->tree without first checking if commit->parsed is true to begin with, so you could NULLify commit->tree but that shouldn't make any difference. I agree leaving the tree object as-is would make sense.

I am not convinced that unparsing all the _remaining_ parents recursively like your patch does is enough. Isn't there a codepath that

 - parses a commit A to find list of true parents X, Y, Z;

 - iterates over that list and descend into one of these parents X, doing
   nasty things such as pruning its parents list after parsing it; and

 - decides to prune that parent X from the parent list, making the parent
   list of A into Y and Z?

After such a sequence, calling your unparse_commit(A) will unparse A and remaining parents of Y and Z, but will still leave X in the dirty state.

Le lundi 18 mai 2009, Junio C Hamano a écrit :

I agree that unparsing all the remaining parents recursively may not be enough in some cases, but I don't know much about these cases. Is it only when revs->prune is set?

I think it should be ok when checking that all good revs are ancestor of the bad rev, but I may be missing something.

It should be ok too when using "git replace" and/or grafts as the parent list of a commit should be changed before its parents are changed.

I also found the clear_commit_marks function that could be used when checking ancestors so I will resend a patch series using it and without patch 2/3. But I can continue to work on unparsing commits to improve the "git replace" series. I need to find some test cases though, and I cannot think of any interesting one right now.

By the way, when you say that you suspect an attempt to replace an object that is directly listed on the command line would not work very well with the current replace series, is it related to the unparsing commit problem?

Thanks, Christian.

Christian Couder <chriscool@tuxfamily.org> writes:

What I fear is something like this.

	git push $there 04a8c^2:master

It would need to parse 04a8c to find its second parent and then start discussing what object to send with the other end. "04a8c^2" is a direct user input and should mean the same commit as git show "04a8c^2" would give the user, so it obviously needs to obey the replace rules (making 04a8c parsed), but the object transfer should not look at replace at all (that's the whole point of not using the "graft hack" that cannot be sanely transferred to the other end).

Junio C Hamano <gitster@pobox.com> writes:

First, I have always thought that you cannot push arbitrary SHA-1 (arbitrary commits) in git; you can only push via refs. Isn't it true?

Second, the "refs/replace" mechanism has the advantage over grafts that it is sanely transferrable. Whether "04a8c^2"^{replaced} exists on remote side depends on if other side has the same replacement, or if you push replacements in the same push.

Solution here could be to bail out and ask user to confirm whether other side has the same replacements... or something like that.

Jakub Narebski <jnareb@gmail.com> writes:

Hmm, you could try it...

[Pushing random commits has always worked for me though.]

-miles

Jakub Narebski <jnareb@gmail.com> writes:

No.

The reason why replace mechanism could be cleaner than grafts is because reachability traversal and transfer do not obey replacements, and local ancestry traversal will if there are refs/replace entries.

So "git log" and friends will obey refs/replace/*, but between a repository that replaces a commit and another that doesn't, they will transfer history without replace entries getting in the way. Whatever 04a8c^2 resolves to with replacements, the _real_ history between that commit and whatever the tips of refs on the other side has is transferred, and that commit will update the 'master' branch on the other side.

If the other side sees it as the second parent of 04a8c is a different matter. If refs/replace hierarchy is shared between the repositories, it will; otherwise it won't. And the beauty of the replace mechanism is, unlike grafts, because object transfer will always be done using _real_ history, you can sanely sync refs/replace hierarchy between the repositories via push or fetch. During the push of the 04a8c^2 object, the other side does not have to worry about the presense/absense of replace that changes the interpretation of that notation on either end. The exact same underlying history is transferred with or without the replacement objects.

On Tue, 19 May 2009, Junio C Hamano wrote:

Oh. I must have mistaken it with the protection in the opposite side: git-fetch doesn't allow fetching arbitrary SHA-1 (arbitrary commits), isn't it?

Side note: I wonder if any other DVCS has such shotgun of^W^W a feature ;-)

[cut]

Thanks for an explanation. So "refs/replace" is sanely transferrable because it can be transferred using local reachability only (without replacements turned on), isn't it?

As I understand the problem with replacement rules is that it cannot be treated simply as 'extended SHA1' syntax; the replacements must be done only for local operations, which probably means opt-in, and pushing it down to the commands itself... well, that or marking commands as local or remote...

Christian Couder schrieb:

I see a recursion here. Could this not overflow the stack if there is a long ancestry chain?

-- Hannes

Hi,

On Mon, 25 May 2009, Johannes Sixt wrote:

You mean tail recursion, i.e. something like

void unparse_commit(struct commit *item)
{
	item->object.flags = 0;
	item->object.used = 0;
	while (item->object.parsed) {
		struct commit *first;

		item->object.parsed = 0;
		if (!item->parents)
			break;
		if (item->parents->next)
			unparse_commit_list(item->parents->next);
		first = item->parents->item;
		free_commit_list(item->parents);
		item->parents = NULL;
		item = first;
	}
}

However, I am a bit concerned that this function is dangerous, as it just assumes that there is no reference to the commits left, which assumption is _very_ easy to break by mistake.

Ciao, Dscho

Hi,

Le Monday 25 May 2009, Johannes Schindelin a écrit :

Yes, it is better like this. Thanks and sorry about responding late.

Anyway I will send a 2 patch series that will use clear_commit_marks instead of this function.

Thanks, Christian.

We must save the pending commits that will be used during revision walking and unparse them after, because we want to leave a clean state for the next revision walking that will try to find the best bisection point.

As we don't fork a process anymore to call "git rev-list", we need to remove the use of GIT_TRACE to check how "git rev-list" is called from the t6030 test that uses it.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 bisect.c                    |   54 +++++++++++++++++-------------------------
 t/t6030-bisect-porcelain.sh |   13 +---------
 2 files changed, 23 insertions(+), 44 deletions(-)