threads / patch / 35499

patch, 7 partsTrivial patches

Subject: [PATCH 0/7] Trivial patches

## tl;dr

11 messages between Dec 8, 2013 and Dec 9, 2013. Diffs are folded; open one to read it.

replies: 10people: 3as markdown or json

Felipe Contreras· Dec 8, 2013, 05:56 UTC · lore
Felipe Contreras (7):
  config: avoid yoda conditions
  add: avoid yoda conditions
  abspath: trivial style fix
  t: trivial whitespace cleanups
  fetch: add missing documentation
  sha1_name: cleanup interpret_branch_name()
  sha1_name: simplify track finding
 Documentation/git-fetch.txt |  3 +++
 abspath.c                   |  2 +-
 builtin/add.c               |  2 +-
 config.c                    |  4 ++--
 sha1_name.c                 | 33 +++++++++++++++++----------------
 t/t0002-gitfile.sh          |  3 +--
 t/t0003-attributes.sh       |  1 -
 7 files changed, 25 insertions(+), 23 deletions(-)
-- 
1.8.5.1+fc1.2.gebd1fb1
Felipe Contreras· Dec 8, 2013, 05:56 UTC · re: Felipe Contreras · lore

[PATCH 1/7] config: avoid yoda conditions

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 config.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
Show changes to config.c +2 −2
diff --git a/config.c b/config.c
index e1d66a1..7a44414 100644
--- a/config.c
+++ b/config.c
@@ -610,7 +610,7 @@ static int git_config_maybe_bool_text(const char *name, const char *value)
 int git_config_maybe_bool(const char *name, const char *value)
 {
 	int v = git_config_maybe_bool_text(name, value);
-	if (0 <= v)
+	if (v >= 0)
 		return v;
 	if (git_parse_int(value, &v))
 		return !!v;
@@ -620,7 +620,7 @@ int git_config_maybe_bool(const char *name, const char *value)
 int git_config_bool_or_int(const char *name, const char *value, int *is_bool)
 {
 	int v = git_config_maybe_bool_text(name, value);
-	if (0 <= v) {
+	if (v >= 0) {
 		*is_bool = 1;
 		return v;
 	}
-- 
1.8.5.1+fc1.2.gebd1fb1
Felipe Contreras· Dec 8, 2013, 05:56 UTC · re: Felipe Contreras · lore

[PATCH 2/7] add: avoid yoda conditions

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 builtin/add.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to builtin/add.c +1 −1
diff --git a/builtin/add.c b/builtin/add.c
index 226f758..9b30356 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -429,7 +429,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 	argc--;
 	argv++;
 
-	if (0 <= addremove_explicit)
+	if (addremove_explicit >= 0)
 		addremove = addremove_explicit;
 	else if (take_worktree_changes && ADDREMOVE_DEFAULT)
 		addremove = 0; /* "-u" was given but not "-A" */
-- 
1.8.5.1+fc1.2.gebd1fb1
Felipe Contreras· Dec 8, 2013, 05:56 UTC · re: Felipe Contreras · lore

[PATCH 3/7] abspath: trivial style fix

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 abspath.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to abspath.c +1 −1
diff --git a/abspath.c b/abspath.c
index e390994..8b3385a 100644
--- a/abspath.c
+++ b/abspath.c
@@ -143,7 +143,7 @@ static const char *real_path_internal(const char *path, int die_on_error)
 error_out:
 	free(last_elem);
 	if (*cwd && chdir(cwd))
-		die_errno ("Could not change back to '%s'", cwd);
+		die_errno("Could not change back to '%s'", cwd);
 
 	return retval;
 }
-- 
1.8.5.1+fc1.2.gebd1fb1
Junio C Hamano· Dec 9, 2013, 21:24 UTC · re: Felipe Contreras · lore

Re: [PATCH 3/7] abspath: trivial style fix

Felipe Contreras <felipe.contreras@gmail.com> writes:
Show 15 quoted lines
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>  abspath.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/abspath.c b/abspath.c
> index e390994..8b3385a 100644
> --- a/abspath.c
> +++ b/abspath.c
> @@ -143,7 +143,7 @@ static const char *real_path_internal(const char *path, int die_on_error)
>  error_out:
>  	free(last_elem);
>  	if (*cwd && chdir(cwd))
> -		die_errno ("Could not change back to '%s'", cwd);
> +		die_errno("Could not change back to '%s'", cwd);
Will queue; thanks.
By the way, there are quite a few hits from
	git grep -e '\(die\|error\|warning\) (' -- \*.c

even if we ignore borrowed code that we do not want to touch up with this kind of change. Some may have overlapping changes in flight that makes it better to leave them as they are for now, but some are in reasonably quiescent areas.

Felipe Contreras· Dec 8, 2013, 05:56 UTC · re: Felipe Contreras · lore

[PATCH 4/7] t: trivial whitespace cleanups

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 t/t0002-gitfile.sh    | 3 +--
 t/t0003-attributes.sh | 1 -
 2 files changed, 1 insertion(+), 3 deletions(-)
Show changes to 2 files +1 −3

t/t0002-gitfile.sh, t/t0003-attributes.sh

diff --git a/t/t0002-gitfile.sh b/t/t0002-gitfile.sh
index cb14425..37e9396 100755
--- a/t/t0002-gitfile.sh
+++ b/t/t0002-gitfile.sh
@@ -7,7 +7,7 @@ Verify that plumbing commands work when .git is a file
 . ./test-lib.sh
 
 objpath() {
-    echo "$1" | sed -e 's|\(..\)|\1/|'
+	echo "$1" | sed -e 's|\(..\)|\1/|'
 }
 
 objck() {
@@ -19,7 +19,6 @@ objck() {
 	fi
 }
 
-
 test_expect_success 'initial setup' '
 	REAL="$(pwd)/.real" &&
 	mv .git "$REAL"
diff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh
index 0b98b6f..b9d7947 100755
--- a/t/t0003-attributes.sh
+++ b/t/t0003-attributes.sh
@@ -13,7 +13,6 @@ attr_check () {
 	test_line_count = 0 err
 }
 
-
 test_expect_success 'setup' '
 	mkdir -p a/b/d a/c b &&
 	(
-- 
1.8.5.1+fc1.2.gebd1fb1
Felipe Contreras· Dec 8, 2013, 05:56 UTC · re: Felipe Contreras · lore

[PATCH 5/7] fetch: add missing documentation

There's no mention of the 'origin' default, or the fact that the upstream tracking branch remote is used.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 Documentation/git-fetch.txt | 3 +++
 1 file changed, 3 insertions(+)
Show changes to Documentation/git-fetch.txt +3 −0
diff --git a/Documentation/git-fetch.txt b/Documentation/git-fetch.txt
index e08a028..a7b245d 100644
--- a/Documentation/git-fetch.txt
+++ b/Documentation/git-fetch.txt
@@ -37,6 +37,9 @@ or from several repositories at once if <group> is given and
 there is a remotes.<group> entry in the configuration file.
 (See linkgit:git-config[1]).
 
+When no remote is specified, by default the `origin` remote will be used,
+unless there's an upstream branch configured for the current branch.
+
 OPTIONS
 -------
 include::fetch-options.txt[]
-- 
1.8.5.1+fc1.2.gebd1fb1
Junio C Hamano· Dec 9, 2013, 21:26 UTC · re: Felipe Contreras · lore

Re: [PATCH 5/7] fetch: add missing documentation

Felipe Contreras <felipe.contreras@gmail.com> writes:
Show 19 quoted lines
> There's no mention of the 'origin' default, or the fact that the
> upstream tracking branch remote is used.
>
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>  Documentation/git-fetch.txt | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/Documentation/git-fetch.txt b/Documentation/git-fetch.txt
> index e08a028..a7b245d 100644
> --- a/Documentation/git-fetch.txt
> +++ b/Documentation/git-fetch.txt
> @@ -37,6 +37,9 @@ or from several repositories at once if <group> is given and
>  there is a remotes.<group> entry in the configuration file.
>  (See linkgit:git-config[1]).
>  
> +When no remote is specified, by default the `origin` remote will be used,
> +unless there's an upstream branch configured for the current branch.
> +

It is better than saying nothing, but the reader will be left wondering what happens when "upstream branch" is configured with the above text.

    When no remote is specified, by default the `origin` remote will
    be used, unless there's an upstream branch configured for the
    current branch, in which case the `fetch` goes to the remote the
    specified upstream remote-tracking branch is taken from.
or something, perhaps?
>  OPTIONS
>  -------
>  include::fetch-options.txt[]
Felipe Contreras· Dec 8, 2013, 05:56 UTC · re: Felipe Contreras · lore

[PATCH 6/7] sha1_name: cleanup interpret_branch_name()

The 'upstream' variable doesn't hold an "upstream", but a branch, so make it clearer.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 sha1_name.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)
Show changes to sha1_name.c +9 −9
diff --git a/sha1_name.c b/sha1_name.c
index e9c2999..e27db88 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -1072,7 +1072,7 @@ static int reinterpret(const char *name, int namelen, int len, struct strbuf *bu
 int interpret_branch_name(const char *name, int namelen, struct strbuf *buf)
 {
 	char *cp;
-	struct branch *upstream;
+	struct branch *branch;
 	int len = interpret_nth_prior_checkout(name, buf);
 	int tmp_len;
 
@@ -1102,26 +1102,26 @@ int interpret_branch_name(const char *name, int namelen, struct strbuf *buf)
 
 	len = cp + tmp_len - name;
 	cp = xstrndup(name, cp - name);
-	upstream = branch_get(*cp ? cp : NULL);
+	branch = branch_get(*cp ? cp : NULL);
 	/*
 	 * Upstream can be NULL only if cp refers to HEAD and HEAD
 	 * points to something different than a branch.
 	 */
-	if (!upstream)
+	if (!branch)
 		die(_("HEAD does not point to a branch"));
-	if (!upstream->merge || !upstream->merge[0]->dst) {
-		if (!ref_exists(upstream->refname))
+	if (!branch->merge || !branch->merge[0]->dst) {
+		if (!ref_exists(branch->refname))
 			die(_("No such branch: '%s'"), cp);
-		if (!upstream->merge) {
+		if (!branch->merge) {
 			die(_("No upstream configured for branch '%s'"),
-				upstream->name);
+				branch->name);
 		}
 		die(
 			_("Upstream branch '%s' not stored as a remote-tracking branch"),
-			upstream->merge[0]->src);
+			branch->merge[0]->src);
 	}
 	free(cp);
-	cp = shorten_unambiguous_ref(upstream->merge[0]->dst, 0);
+	cp = shorten_unambiguous_ref(branch->merge[0]->dst, 0);
 	strbuf_reset(buf);
 	strbuf_addstr(buf, cp);
 	free(cp);
-- 
1.8.5.1+fc1.2.gebd1fb1
Philip Oakley· Dec 8, 2013, 05:56 UTC · re: Felipe Contreras · lore

Re: [PATCH 6/7] sha1_name: cleanup interpret_branch_name()

From: "Felipe Contreras" <felipe.contreras@gmail.com>
Show 33 quoted lines
> The 'upstream' variable doesn't hold an "upstream", but a branch, so
> make it clearer.
>
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
> sha1_name.c | 18 +++++++++---------
> 1 file changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/sha1_name.c b/sha1_name.c
> index e9c2999..e27db88 100644
> --- a/sha1_name.c
> +++ b/sha1_name.c
> @@ -1072,7 +1072,7 @@ static int reinterpret(const char *name, int 
> namelen, int len, struct strbuf *bu
> int interpret_branch_name(const char *name, int namelen, struct strbuf 
> *buf)
> {
>  char *cp;
> - struct branch *upstream;
> + struct branch *branch;
>  int len = interpret_nth_prior_checkout(name, buf);
>  int tmp_len;
>
> @@ -1102,26 +1102,26 @@ int interpret_branch_name(const char *name, 
> int namelen, struct strbuf *buf)
>
>  len = cp + tmp_len - name;
>  cp = xstrndup(name, cp - name);
> - upstream = branch_get(*cp ? cp : NULL);
> + branch = branch_get(*cp ? cp : NULL);
>  /*
>  * Upstream can be NULL only if cp refers to HEAD and HEAD
>  * points to something different than a branch.

Doesn't this comment also need updating, or at least clarifying any contradictions between variable names and the concepts described. That is does "Upstream" refer to the original variable 'upstream', and now the final "branch" can be confused with the new variable name.

I'm not arguing against the change, rather just noting an additional consequence.

Show 34 quoted lines
>  */
> - if (!upstream)
> + if (!branch)
>  die(_("HEAD does not point to a branch"));
> - if (!upstream->merge || !upstream->merge[0]->dst) {
> - if (!ref_exists(upstream->refname))
> + if (!branch->merge || !branch->merge[0]->dst) {
> + if (!ref_exists(branch->refname))
>  die(_("No such branch: '%s'"), cp);
> - if (!upstream->merge) {
> + if (!branch->merge) {
>  die(_("No upstream configured for branch '%s'"),
> - upstream->name);
> + branch->name);
>  }
>  die(
>  _("Upstream branch '%s' not stored as a remote-tracking branch"),
> - upstream->merge[0]->src);
> + branch->merge[0]->src);
>  }
>  free(cp);
> - cp = shorten_unambiguous_ref(upstream->merge[0]->dst, 0);
> + cp = shorten_unambiguous_ref(branch->merge[0]->dst, 0);
>  strbuf_reset(buf);
>  strbuf_addstr(buf, cp);
>  free(cp);
> -- 
> 1.8.5.1+fc1.2.gebd1fb1
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
Felipe Contreras· Dec 8, 2013, 05:57 UTC · re: Felipe Contreras · lore

[PATCH 7/7] sha1_name: simplify track finding

It's more efficient to check for the braces first, and we already have the length of the whole thing.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 sha1_name.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)
Show changes to sha1_name.c +8 −7
diff --git a/sha1_name.c b/sha1_name.c
index e27db88..1a68103 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -417,7 +417,7 @@ static int ambiguous_path(const char *path, int len)
 
 static inline int upstream_mark(const char *string, int len)
 {
-	const char *suffix[] = { "@{upstream}", "@{u}" };
+	const char *suffix[] = { "upstream", "u" };
 	int i;
 
 	for (i = 0; i < ARRAY_SIZE(suffix); i++) {
@@ -475,7 +475,7 @@ static int get_sha1_basic(const char *str, int len, unsigned char *sha1)
 					nth_prior = 1;
 					continue;
 				}
-				if (!upstream_mark(str + at, len - at)) {
+				if (!upstream_mark(str + at + 2, len - at - 3)) {
 					reflog_len = (len-1) - (at+2);
 					len = at;
 				}
@@ -1074,7 +1074,6 @@ int interpret_branch_name(const char *name, int namelen, struct strbuf *buf)
 	char *cp;
 	struct branch *branch;
 	int len = interpret_nth_prior_checkout(name, buf);
-	int tmp_len;
 
 	if (!namelen)
 		namelen = strlen(name);
@@ -1096,11 +1095,13 @@ int interpret_branch_name(const char *name, int namelen, struct strbuf *buf)
 	if (len > 0)
 		return reinterpret(name, namelen, len, buf);
 
-	tmp_len = upstream_mark(cp, namelen - (cp - name));
-	if (!tmp_len)
+	len = namelen - (cp - name);
+	if (cp[1] != '{' && cp[len - 1] != '}')
+		return -1;
+
+	if (!upstream_mark(cp + 2, len - 3))
 		return -1;
 
-	len = cp + tmp_len - name;
 	cp = xstrndup(name, cp - name);
 	branch = branch_get(*cp ? cp : NULL);
 	/*
@@ -1125,7 +1126,7 @@ int interpret_branch_name(const char *name, int namelen, struct strbuf *buf)
 	strbuf_reset(buf);
 	strbuf_addstr(buf, cp);
 	free(cp);
-	return len;
+	return namelen;
 }
 
 int strbuf_branchname(struct strbuf *sb, const char *name)
-- 
1.8.5.1+fc1.2.gebd1fb1

← back to recent threads