Re: [PATCH] checkout: add a test for creating a new branch with regexp as a starting point

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 2, 2010, 21:04 UTC
Message-ID: <7v1vaglo9w.fsf@alter.siamese.dyndns.org>
In-Reply-To: <201007300136.13501.trast@student.ethz.ch>

Thomas Rast <trast@student.ethz.ch> writes:

Show 13 quoted lines

>> > +test_expect_success checkout '
>> > +       git checkout -b new_branch :/first
>> > +'
>> > +
>> > +test_done
>> 
>> ...it looks like this can just be added to the end of
>> t2018-checkout-branch.sh instead of creating a new test. Creating a
>> new file just for a single test for such a simple feature is a bit of
>> an overkill.
>
> It should also use test_expect_failure unless you expect to have a fix
> soon, otherwise it would stop the test suite from running through.

I think this is an ancient bug from the very beginning of the ":/" notation. Because it temporarily uses one bit in the object flags, but the get_sha1_oneline() parser can be called multiple times, it tries to clear the flag bits it smudged at the end, but fails.

The basic pattern for traversing the ancestry while avoiding duplication is:

    prepare commits you traverse from in "list";
    while (list) {
        commit = pop_most_recent_commit(&list, MARK);
        try to use commit;
    }

pop_most_recent_commit() removes a commit from the list, pushes parents of the commit that are _not_ marked with any of the bits in the MARK to the given list, and returns that commit.

While introducing the feature ':/', however, 28a4d94 (object name: introduce ':/<oneline prefix>' notation, 2007-02-24) wanted to avoid leaving the mark used in the loop before leaving the function (which is a right thing to do), by doing this:

    prepare commits you traverse from in "list";
+   keep a copy of "list" in "backup";
    while (list) {
        commit = pop_most_recent_commit(&list, MARK);
        try to use commit;
    }
+   for (commit in backup)
+       clear_commit_marks(commit, MARK);

which is wrong, as clear_commit_marks() will ignore if the starting commit does not have the MARK.

I think the fix should be just as simple as this. Essentially, X_SEEN mark means "we have marked and placed this item on the list (so do not put it again back on the list when it appears as a parent of some other item)" so marking commits we initially put on the list with the bit should be the right thing to do regardless of this clean-up business.

 sha1_name.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/sha1_name.c b/sha1_name.c
index 4f2af8d..b935688 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -704,8 +704,10 @@ static int get_sha1_oneline(const char *prefix, unsigned char *sha1)
 		die("Invalid search pattern: %s", prefix);
 
 	for_each_ref(handle_one_ref, &list);
-	for (l = list; l; l = l->next)
+	for (l = list; l; l = l->next) {
 		commit_list_insert(l->item, &backup);
+		l->item->object.flags |= ONELINE_SEEN;
+	}
 	while (list) {
 		char *p;
 		struct commit *commit;