Re: [RFC/PATCHv8 00/10] git notes

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Junio C Hamano <gitster@xxxxxxxxx> writes:

> Alex Riesen <raa.lkml@xxxxxxxxx> writes:
>
>> On Fri, Nov 20, 2009 at 11:46, Junio C Hamano <gitster@xxxxxxxxx> wrote:
>>> @@ -716,7 +719,7 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
>>>                if (!strcmp(arg, "-"))
>>>                        arg = "@{-1}";
>>>
>>> -               if (get_sha1(arg, rev)) {
>>> +               if (get_sha1_mb(arg, rev)) {
>>>                        if (has_dash_dash)          /* case (1) */
>>>                                die("invalid reference: %s", arg);
>>>                        if (!patch_mode &&
>>
>> This is a bit of a problem on Windows, as the arg (eventually containing
>> something like "master..."), will be passed to resolve_ref below.
>
> I don't see "resolve_ref below"; do you mean this part?
>
> 		/* we can't end up being in (2) anymore, eat the argument */
> 		argv++;
> 		argc--;
>
> 		new.name = arg;
> 		if ((new.commit = lookup_commit_reference_gently(rev, 1))) {
> 			setup_branch_path(&new);
> 			if (resolve_ref(new.path, rev, 1, NULL))
> 				new.commit = lookup_commit_reference(rev);
> 			else
> 				new.path = NULL;
> 			parse_commit(new.commit);
> 			source_tree = new.commit->tree;
> 		} else
> 			source_tree = parse_tree_indirect(rev);
>
> It is primarily to tell "git checkout master" and "git checkout master^0"
> apart (the latter must detach HEAD).  IOW, it is testing if you gave the
> "name" of the branch, or something that yields the commit object name that
> happens to be at the tip of the branch.  So the call to resolve_ref() is
> very relevant.
>
>> Now, Windows,
>> being the piece of shit it is, will lie and tell that a file
>> "refs/heads/master..."
>> exists and be same as "refs/heads/master".
>
> Meh; then windows users cannot use "git checkout A..." syntax (they can
> still use "git checkout A...HEAD", I presume).
>
> This is not a problem specific to three-dots, but if you give the function
> "refs/heads/master.."  it would also get "exists" back, no?  You could
> teach resolve_ref() about windows (namely, the filesystem may lie about
> anything that ends with a series of dots).
>
>> This breaks "checkout to merge base" on Windows and t2012 in particular.

I think the attached patch would help.  If this fix is Ok with Windows
people (J6t CC'ed), I'd like to apply this to 'master' so that we can ship
1.7.0-rc0 without breakage.

Thanks.

-- >8 --
Subject: Fix "checkout A..." synonym for "checkout A...HEAD" on Windows

When switching to a different commit, we first see the named rev exists
as a commit using lookup_commit_reference_gently(), and set new.path to
a string "refs/heads/" followed by the name the user gave us (but after
taking into special short-hands like @{-1} == "previous branch" and
"@{upstream}" == "the branch we merge with" into account).  If the
resulting string names an existsing ref, then we are switching to that
branch (and will be building new commits on top of it); otherwise we are
detaching HEAD at that commit.

When the "master..." syntax is used as a short-hand for "master...HEAD",
we do want to detach HEAD at the merge base.  However, on Windows, when
asked if ".git/refs/heads/master..." exists, the filesystem happily says
"it does" when ".git/refs/heads/master" exists.

Work this issue around by first calling check_ref_format(new.path) to see
if the string can possibly be a valid ref under "refs/heads/", before
asking resolve_ref().

We used to run another lookup_commit_reference(rev) even though we know it
succeeded and we have a good commit in new.commit already; this has been
with us from 782c2d6 (Build in checkout, 2008-02-07), the first version we
had "git checkout" implemented in C.  Drop it.

Noticed by Alex Riesen.

Signed-off-by: Junio C Hamano <gitster@xxxxxxxxx>
---
 builtin-checkout.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/builtin-checkout.c b/builtin-checkout.c
index fe7c858..ad3c01f 100644
--- a/builtin-checkout.c
+++ b/builtin-checkout.c
@@ -745,8 +745,10 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
 		new.name = arg;
 		if ((new.commit = lookup_commit_reference_gently(rev, 1))) {
 			setup_branch_path(&new);
-			if (resolve_ref(new.path, rev, 1, NULL))
-				new.commit = lookup_commit_reference(rev);
+
+			if ((check_ref_format(new.path) == CHECK_REF_FORMAT_OK) &&
+			    resolve_ref(new.path, rev, 1, NULL))
+				;
 			else
 				new.path = NULL;
 			parse_commit(new.commit);

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]