Re: [PATCH] show-branch: fix crash with long ref name

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

 



On Tue, Feb 14, 2017 at 10:29:46PM +0100, Christian Couder wrote:

> > I notice Christian's patch added a few tests. I don't know if we'd want
> > to squash them in (I didn't mean to override his patch at all; I was
> > about to send mine out when I noticed his, and I wondered if we wanted
> > to combine the two efforts).
> 
> I think it would be nice to have at least one test. Feel free to
> squash mine if you want.

I started to add some tests, but I had second thoughts. It _is_ nice
to show off the fix, but as far as regressions go, this specific case is
unlikely to come up again. What would be more valuable, I think, is a
test script which set up a very long refname (not just 150 bytes or
whatever) and ran it through a series of git commands.

But then you run into all sorts of portability annoyances with pathname
restrictions (you can hack around creation by writing the refname
directly into packed-refs, but most manipulations will want to take the
.lock in the filesystem). So I dunno. It seems like being thorough is a
lot of hassle for not much gain. Being not-thorough is easy, but is
mostly a token that is unlikely to find any real bugs.

So I punted, at least for now.

I see the patches are marked for 'next' in the latest What's Cooking.
If it is not too late in today's integration cycle, here is a re-roll of
patch 3 that squashes in Pranit's suggestion (if it is too late, then
Pranit, you may want to re-send it as a squash on top).

-- >8 --
Subject: [PATCH] show-branch: use skip_prefix to drop magic numbers

We make several starts_with() calls, only to advance
pointers. This is exactly what skip_prefix() is for, which
lets us avoid manually-counted magic numbers.

Helped-by: Pranit Bauva <pranit.bauva@xxxxxxxxx>
Signed-off-by: Jeff King <peff@xxxxxxxx>
---
 builtin/show-branch.c | 19 ++++++++-----------
 1 file changed, 8 insertions(+), 11 deletions(-)

diff --git a/builtin/show-branch.c b/builtin/show-branch.c
index 404c4d09a..19756595d 100644
--- a/builtin/show-branch.c
+++ b/builtin/show-branch.c
@@ -275,8 +275,7 @@ static void show_one_commit(struct commit *commit, int no_name)
 		pp_commit_easy(CMIT_FMT_ONELINE, commit, &pretty);
 		pretty_str = pretty.buf;
 	}
-	if (starts_with(pretty_str, "[PATCH] "))
-		pretty_str += 8;
+	skip_prefix(pretty_str, "[PATCH] ", &pretty_str);
 
 	if (!no_name) {
 		if (name && name->head_name) {
@@ -470,17 +469,14 @@ static void snarf_refs(int head, int remotes)
 	}
 }
 
-static int rev_is_head(char *head, char *name,
+static int rev_is_head(const char *head, const char *name,
 		       unsigned char *head_sha1, unsigned char *sha1)
 {
 	if (!head || (head_sha1 && sha1 && hashcmp(head_sha1, sha1)))
 		return 0;
-	if (starts_with(head, "refs/heads/"))
-		head += 11;
-	if (starts_with(name, "refs/heads/"))
-		name += 11;
-	else if (starts_with(name, "heads/"))
-		name += 6;
+	skip_prefix(head, "refs/heads/", &head);
+	if (!skip_prefix(name, "refs/heads/", &name))
+		skip_prefix(name, "heads/", &name);
 	return !strcmp(head, name);
 }
 
@@ -799,8 +795,9 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)
 				has_head++;
 		}
 		if (!has_head) {
-			int offset = starts_with(head, "refs/heads/") ? 11 : 0;
-			append_one_rev(head + offset);
+			const char *name = head;
+			skip_prefix(name, "refs/heads/", &name);
+			append_one_rev(name);
 		}
 	}
 
-- 
2.12.0.rc1.541.g3e32dea89




[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]