Re: [PATCH v2 10/11] sha1_name: reorganize get_sha1_basic()

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

 



Junio C Hamano <gitster@xxxxxxxxx> writes:

> Also, a few points this patch highlights in the code before the
> change:
>
>  - If we were on a branch with 40-hex name at nth prior checkout,
>    would we mistake it as being detached at the commit?
>
>  - If we were on a branch 'foo' at nth prior checkout, would our
>    previous get_sha1_1() have made us mistake it as referring to a
>    tag 'foo' with the same name if it exists?
>
> The former needs a fix to either writing of reflog or reading by
> interpret_nth_prior_checkout() so that we can tell these cases apart
> more reliably.  Then the latter can be solved by splicing
> refs/heads/ in front of the branch name before recursively calling
> get_sha1_1() in the original code (and similar fix can be
> forward-ported to this patch).
>
> Incidentally, I think if we prefix refs/heads/ in front and feed
> that to dwim_ref/log, we would also avoid running @{-N} twice (which
> I suspect might be more expensive than a simple recursion, as it
> needs to go through the reflog file), which may be a nice side
> effect of such a fix we would get for free.

Here is the first step (i.e. more reliable interpret_nth_prior).

I looked at all the existing callers of interpret_branch_name() and
it appears to me that most of them currently assume they are getting
a branch name, because they want to work on a ref, and some of them
do not care, because they are working on a revision.  For the
former, they can (and should) error out instead of relying on not
having refs/heads/$detached_SHA1 that will prevent them from working
on a ref which is what they currently do, if they had the "detached"
information.  For the latter, if we give "detached" information,
they can either prefix "refs/heads/" (if the result is "not
detached") to call resolve_ref(), or call get_sha1_hex (if the
result is "detached"), which would be the solution for the second
issue I noticed in the message I am replying to.

The next step on top of this patch may be to expose the "detached"
bit up in the API chain to let callers of interpret_branch_name() to
know about the distinction.

 sha1_name.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/sha1_name.c b/sha1_name.c
index 3820f28..3dd6667 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -862,6 +862,7 @@ static int get_sha1_oneline(const char *prefix, unsigned char *sha1,
 struct grab_nth_branch_switch_cbdata {
 	int remaining;
 	struct strbuf buf;
+	int detached;
 };
 
 static int grab_nth_branch_switch(unsigned char *osha1, unsigned char *nsha1,
@@ -880,9 +881,14 @@ static int grab_nth_branch_switch(unsigned char *osha1, unsigned char *nsha1,
 	if (!match || !target)
 		return 0;
 	if (--(cb->remaining) == 0) {
+		unsigned char sha1[20];
+
 		len = target - match;
 		strbuf_reset(&cb->buf);
 		strbuf_add(&cb->buf, match, len);
+		cb->detached = (len == 40 &&
+				!get_sha1_hex(match, sha1) &&
+				!hashcmp(osha1, sha1));
 		return 1; /* we are done */
 	}
 	return 0;
@@ -892,7 +898,7 @@ static int grab_nth_branch_switch(unsigned char *osha1, unsigned char *nsha1,
  * Parse @{-N} syntax, return the number of characters parsed
  * if successful; otherwise signal an error with negative value.
  */
-static int interpret_nth_prior_checkout(const char *name, struct strbuf *buf)
+static int interpret_nth_prior_checkout(const char *name, struct strbuf *buf, int *detached)
 {
 	long nth;
 	int retval;
@@ -917,6 +923,8 @@ static int interpret_nth_prior_checkout(const char *name, struct strbuf *buf)
 	if (0 < for_each_reflog_ent_reverse("HEAD", grab_nth_branch_switch, &cb)) {
 		strbuf_reset(buf);
 		strbuf_add(buf, cb.buf.buf, cb.buf.len);
+		if (detached)
+			*detached = cb.detached;
 		retval = brace - name + 1;
 	}
 
@@ -992,7 +1000,7 @@ int interpret_branch_name(const char *name, struct strbuf *buf)
 	char *cp;
 	struct branch *upstream;
 	int namelen = strlen(name);
-	int len = interpret_nth_prior_checkout(name, buf);
+	int len = interpret_nth_prior_checkout(name, buf, NULL);
 	int tmp_len;
 
 	if (!len)
--
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]