[PATCH 0/3] Add a function skip_prefix_if_present()

[PATCH 0/3] Add a function skip_prefix_if_present()

[Michael Haggerty]

These patches apply on top of gitster/nd/more-skip-prefix.

I noticed that Duy's new function skip_prefix_defval() was mostly
being called with its first and third arguments the same.  So
introduce a function skip_prefix_if_present() that implements this
pattern.

Michael Haggerty (3):
  Add and use a function skip_prefix_if_present()
  diff_scoreopt_parse(): use skip_prefix_if_present()
  rev_is_head(): use skip_prefix()

 builtin/checkout.c    |  4 ++--
 builtin/fast-export.c |  2 +-
 builtin/merge.c       |  2 +-
 builtin/show-branch.c | 15 ++++++++-------
 diff.c                |  6 +++---
 git-compat-util.h     |  5 +++++
 git.c                 |  2 +-
 notes.c               |  4 ++--
 refs.c                |  2 +-
 wt-status.c           |  4 ++--
 10 files changed, 26 insertions(+), 20 deletions(-)

-- 
1.8.5.3

[PATCH 1/3] Add and use a function skip_prefix_if_present()

[Michael Haggerty]

Most of the calls to skip_prefix_defval() had equal first and third
arguments, with the effect of skipping the prefix if present, but
otherwise returning the original string.  So define a new function
that does exactly that.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 builtin/checkout.c    | 4 ++--
 builtin/fast-export.c | 2 +-
 builtin/merge.c       | 2 +-
 builtin/show-branch.c | 6 +++---
 git-compat-util.h     | 5 +++++
 git.c                 | 2 +-
 notes.c               | 4 ++--
 refs.c                | 2 +-
 wt-status.c           | 4 ++--
 9 files changed, 18 insertions(+), 13 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index 6531ed4..84682f1 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -1151,8 +1151,8 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
 		const char *argv0 = argv[0];
 		if (!argc || !strcmp(argv0, "--"))
 			die (_("--track needs a branch name"));
-		argv0 = skip_prefix_defval(argv0, "refs/", argv0);
-		argv0 = skip_prefix_defval(argv0, "remotes/", argv0);
+		argv0 = skip_prefix_if_present(argv0, "refs/");
+		argv0 = skip_prefix_if_present(argv0, "remotes/");
 		argv0 = strchr(argv0, '/');
 		if (!argv0 || !argv0[1])
 			die (_("Missing branch name; try -b"));
diff --git a/builtin/fast-export.c b/builtin/fast-export.c
index cd0a302..c87c7ea 100644
--- a/builtin/fast-export.c
+++ b/builtin/fast-export.c
@@ -476,7 +476,7 @@ static void handle_tag(const char *name, struct tag *tag)
 		}
 	}
 
-	name = skip_prefix_defval(name, "refs/tags/", name);
+	name = skip_prefix_if_present(name, "refs/tags/");
 	printf("tag %s\nfrom :%d\n%.*s%sdata %d\n%.*s\n",
 	       name, tagged_mark,
 	       (int)(tagger_end - tagger), tagger,
diff --git a/builtin/merge.c b/builtin/merge.c
index 603f80a..7b01dcf 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -1109,7 +1109,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 	 */
 	branch = branch_to_free = resolve_refdup("HEAD", head_sha1, 0, &flag);
 	if (branch)
-		branch = skip_prefix_defval(branch, "refs/heads/", branch);
+		branch = skip_prefix_if_present(branch, "refs/heads/");
 	if (!branch || is_null_sha1(head_sha1))
 		head_commit = NULL;
 	else
diff --git a/builtin/show-branch.c b/builtin/show-branch.c
index 6078132..f2c3b19 100644
--- a/builtin/show-branch.c
+++ b/builtin/show-branch.c
@@ -284,7 +284,7 @@ static void show_one_commit(struct commit *commit, int no_name)
 		pp_commit_easy(CMIT_FMT_ONELINE, commit, &pretty);
 		pretty_str = pretty.buf;
 	}
-	pretty_str = skip_prefix_defval(pretty_str, "[PATCH] ", pretty_str);
+	pretty_str = skip_prefix_if_present(pretty_str, "[PATCH] ");
 
 	if (!no_name) {
 		if (name && name->head_name) {
@@ -478,7 +478,7 @@ static int rev_is_head(const char *head, int headlen, const char *name,
 	if ((!head[0]) ||
 	    (head_sha1 && sha1 && hashcmp(head_sha1, sha1)))
 		return 0;
-	head = skip_prefix_defval(head, "refs/heads/", head);
+	head = skip_prefix_if_present(head, "refs/heads/");
 	if (starts_with(name, "refs/heads/"))
 		name += 11;
 	else if (starts_with(name, "heads/"))
@@ -810,7 +810,7 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)
 				has_head++;
 		}
 		if (!has_head)
-			append_one_rev(skip_prefix_defval(head, "refs/heads/", head));
+			append_one_rev(skip_prefix_if_present(head, "refs/heads/"));
 	}
 
 	if (!ref_name_cnt) {
diff --git a/git-compat-util.h b/git-compat-util.h
index 59265af..cff946c 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -368,6 +368,11 @@ static inline const char *skip_prefix(const char *str, const char *prefix)
 	return skip_prefix_defval(str, prefix, NULL);
 }
 
+static inline const char *skip_prefix_if_present(const char *str, const char *prefix)
+{
+	return skip_prefix_defval(str, prefix, str);
+}
+
 static inline int starts_with(const char *str, const char *prefix)
 {
 	return skip_prefix(str, prefix) != NULL;
diff --git a/git.c b/git.c
index 321ae81..f3357d8 100644
--- a/git.c
+++ b/git.c
@@ -579,7 +579,7 @@ int main(int argc, char **av)
 	argc--;
 	handle_options(&argv, &argc, NULL);
 	if (argc > 0) {
-		argv[0] = skip_prefix_defval(argv[0], "--", argv[0]);
+		argv[0] = skip_prefix_if_present(argv[0], "--");
 	} else {
 		/* The user didn't specify a command; give them help */
 		commit_pager_choice();
diff --git a/notes.c b/notes.c
index 31f513b..15c49d8 100644
--- a/notes.c
+++ b/notes.c
@@ -1243,8 +1243,8 @@ static void format_note(struct notes_tree *t, const unsigned char *object_sha1,
 		if (!ref || !strcmp(ref, GIT_NOTES_DEFAULT_REF)) {
 			strbuf_addstr(sb, "\nNotes:\n");
 		} else {
-			ref = skip_prefix_defval(ref, "refs/", ref);
-			ref = skip_prefix_defval(ref, "notes/", ref);
+			ref = skip_prefix_if_present(ref, "refs/");
+			ref = skip_prefix_if_present(ref, "notes/");
 			strbuf_addf(sb, "\nNotes (%s):\n", ref);
 		}
 	}
diff --git a/refs.c b/refs.c
index 217093f..808985d 100644
--- a/refs.c
+++ b/refs.c
@@ -2318,7 +2318,7 @@ static void try_remove_empty_parents(char *name)
 /* make sure nobody touched the ref, and unlink */
 static void prune_ref(struct ref_to_prune *r)
 {
-	const char *name = skip_prefix_defval(r->name, "refs/", r->name);
+	const char *name = skip_prefix_if_present(r->name, "refs/");
 	struct ref_lock *lock = lock_ref_sha1(name, r->sha1);
 
 	if (lock) {
diff --git a/wt-status.c b/wt-status.c
index 185fa81..e7fab5c 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -1179,8 +1179,8 @@ static void wt_status_get_detached_from(struct wt_status_state *state)
 	     ((commit = lookup_commit_reference_gently(sha1, 1)) != NULL &&
 	      !hashcmp(cb.nsha1, commit->object.sha1)))) {
 		const char *p;
-		if ((p = skip_prefix_defval(ref, "refs/tags/", ref)) == ref)
-			p = skip_prefix_defval(ref, "refs/remotes/", ref);
+		if (!(p = skip_prefix(ref, "refs/tags/")))
+			p = skip_prefix_if_present(ref, "refs/remotes/");
 		state->detached_from = xstrdup(p);
 	} else
 		state->detached_from =
-- 
1.8.5.3

[PATCH 2/3] diff_scoreopt_parse(): use skip_prefix_if_present()

[Michael Haggerty]

Whenever skip_prefix_defval() was called, opt was still equal to
*opt_p.  So we can use skip_prefix_if_present() instead.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 diff.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/diff.c b/diff.c
index d629cc5..a07fe9d 100644
--- a/diff.c
+++ b/diff.c
@@ -3890,13 +3890,13 @@ static int diff_scoreopt_parse(const char **opt_p)
 	*opt_p = opt;
 	if (cmd == '-') {
 		/* convert the long-form arguments into short-form versions */
-		if ((opt = skip_prefix_defval(opt, "break-rewrites", *opt_p)) != *opt_p) {
+		if ((opt = skip_prefix_if_present(opt, "break-rewrites")) != *opt_p) {
 			if (*opt == 0 || *opt++ == '=')
 				cmd = 'B';
-		} else if ((opt = skip_prefix_defval(opt, "find-copies", *opt_p)) != *opt_p) {
+		} else if ((opt = skip_prefix_if_present(opt, "find-copies")) != *opt_p) {
 			if (*opt == 0 || *opt++ == '=')
 				cmd = 'C';
-		} else if ((opt = skip_prefix_defval(opt, "find-renames", *opt_p)) != *opt_p) {
+		} else if ((opt = skip_prefix_if_present(opt, "find-renames")) != *opt_p) {
 			if (*opt == 0 || *opt++ == '=')
 				cmd = 'M';
 		}
-- 
1.8.5.3

[PATCH 3/3] rev_is_head(): use skip_prefix()

[Michael Haggerty]

Change the logic a little bit so that we can use skip_prefix() instead
of doing pointer arithmetic with hardcoded numbers.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 builtin/show-branch.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/builtin/show-branch.c b/builtin/show-branch.c
index f2c3b19..aee7fe5 100644
--- a/builtin/show-branch.c
+++ b/builtin/show-branch.c
@@ -475,14 +475,15 @@ static void snarf_refs(int head, int remotes)
 static int rev_is_head(const char *head, int headlen, const char *name,
 		       unsigned char *head_sha1, unsigned char *sha1)
 {
+	const char *short_name;
+
 	if ((!head[0]) ||
 	    (head_sha1 && sha1 && hashcmp(head_sha1, sha1)))
 		return 0;
 	head = skip_prefix_if_present(head, "refs/heads/");
-	if (starts_with(name, "refs/heads/"))
-		name += 11;
-	else if (starts_with(name, "heads/"))
-		name += 6;
+	if ((short_name = skip_prefix(name, "refs/heads/")) ||
+	    (short_name = skip_prefix(name, "heads/")))
+		return !strcmp(head, short_name);
 	return !strcmp(head, name);
 }
 
-- 
1.8.5.3

Re: [PATCH 0/3] Add a function skip_prefix_if_present()

[Michael Haggerty]

On 02/05/2014 07:25 AM, Michael Haggerty wrote:
> These patches apply on top of gitster/nd/more-skip-prefix.
> 
> I noticed that Duy's new function skip_prefix_defval() was mostly
> being called with its first and third arguments the same.  So
> introduce a function skip_prefix_if_present() that implements this
> pattern.

I see I should have read the whole previous thread [1] before firing off
this patch series.  What I learned when I read it just now:

* Johannes Sixt didn't think changes like the following improve
  readability:

> -		if (starts_with(arg, "--upload-pack=")) {
> -			args.uploadpack = arg + 14;
> +		if ((optarg = skip_prefix(arg, "--upload-pack=")) != NULL) {
> +			args.uploadpack = optarg;

* René Scharfe submitted a patch to use a function parse_prefix()
  (originally suggested by Peff) instead of Duy's suggested approach:

      http://article.gmane.org/gmane.comp.version-control.git/239569

  His patch appears to have been overlooked.

* Duy seemed to offer to rewrite his patch series, but I don't think
  that it has happened yet.

And then the conversation was drowned by Christmas eggnog.

I don't have a strong feeling about (Duy's proposal plus my patches) vs.
(René's parse_prefix() approach).  But I definitely *do* like the idea
of getting rid of all those awkward magic numbers everywhere.

Michael

[1]
http://thread.gmane.org/gmane.comp.version-control.git/239438/focus=239569

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/

Re: [PATCH 0/3] Add a function skip_prefix_if_present()

[Jeff King]

On Wed, Feb 05, 2014 at 07:55:09AM +0100, Michael Haggerty wrote:

> * René Scharfe submitted a patch to use a function parse_prefix()
>   (originally suggested by Peff) instead of Duy's suggested approach:
> 
>       http://article.gmane.org/gmane.comp.version-control.git/239569
> 
>   His patch appears to have been overlooked.
> 
> * Duy seemed to offer to rewrite his patch series, but I don't think
>   that it has happened yet.
> 
> And then the conversation was drowned by Christmas eggnog.
> 
> I don't have a strong feeling about (Duy's proposal plus my patches) vs.
> (René's parse_prefix() approach).  But I definitely *do* like the idea
> of getting rid of all those awkward magic numbers everywhere.

FWIW, after coming down off my eggnog bender, I think I still prefer
René's (my?) approach.

-Peff

Re: [PATCH 0/3] Add a function skip_prefix_if_present()

[Duy Nguyen]

On Wed, Feb 5, 2014 at 1:55 PM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
> * Duy seemed to offer to rewrite his patch series, but I don't think
>   that it has happened yet.

It's really low in my todo list. So if you want to pick it up, please do.
-- 
Duy