[PATCH 00/16] Trivial patches

Most of these have been sent before, but were not applied for one reason or
another.

Felipe Contreras (16):
  merge: simplify ff-only option
  t: replace pulls with merges
  pull: cleanup documentation
  fetch: add missing documentation
  revision: add missing include
  shortlog: add missing declaration
  branch: trivial style fix
  sha1-name: trivial style cleanup
  transport-helper: trivial style fix
  describe: trivial style fixes
  pretty: trivial style fix
  revision: trivial style fixes
  diff: trivial style fix
  run-command: trivial style fixes
  setup: trivial style fixes
  add: avoid yoda conditions

 Documentation/git-fetch.txt            |  3 +++
 Documentation/git-pull.txt             |  4 ++--
 builtin/add.c                          |  2 +-
 builtin/branch.c                       |  3 +--
 builtin/describe.c                     |  7 +++----
 builtin/diff.c                         |  2 +-
 builtin/merge.c                        | 11 ++---------
 pretty.c                               |  2 +-
 revision.c                             | 14 ++++++--------
 revision.h                             |  1 +
 run-command.c                          | 13 +++++--------
 setup.c                                |  4 ++--
 sha1_name.c                            |  1 -
 shortlog.h                             |  2 ++
 t/annotate-tests.sh                    |  2 +-
 t/t4200-rerere.sh                      |  2 +-
 t/t9114-git-svn-dcommit-merge.sh       |  2 +-
 t/t9500-gitweb-standalone-no-errors.sh |  2 +-
 transport-helper.c                     |  1 +
 19 files changed, 35 insertions(+), 43 deletions(-)

-- 
1.8.4.2+fc1

This is what the code intended.

No functional changes.

Signed-off-by: Felipe Contreras <redacted>
---
 t/annotate-tests.sh                    | 2 +-
 t/t4200-rerere.sh                      | 2 +-
 t/t9114-git-svn-dcommit-merge.sh       | 2 +-
 t/t9500-gitweb-standalone-no-errors.sh | 2 +-
 4 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/t/annotate-tests.sh b/t/annotate-tests.sh
index 99caa42..c9d105d 100644
--- a/t/annotate-tests.sh
+++ b/t/annotate-tests.sh

@@ -92,7 +92,7 @@ test_expect_success 'blame 2 authors + 1 branch2 author' '
 '
 
 test_expect_success 'merge branch1 & branch2' '
-	git pull . branch1
+	git merge branch1
 '
 
 test_expect_success 'blame 2 authors + 2 merged-in authors' '

diff --git a/t/t4200-rerere.sh b/t/t4200-rerere.sh
index 7f6666f..cf19eb7 100755
--- a/t/t4200-rerere.sh
+++ b/t/t4200-rerere.sh

@@ -172,7 +172,7 @@ test_expect_success 'first postimage wins' '
 	git show second^:a1 | sed "s/To die: t/To die! T/" >a1 &&
 	git commit -q -a -m third &&
 
-	test_must_fail git pull . first &&
+	test_must_fail git merge first &&
 	# rerere kicked in
 	! grep "^=======\$" a1 &&
 	test_cmp expect a1

diff --git a/t/t9114-git-svn-dcommit-merge.sh b/t/t9114-git-svn-dcommit-merge.sh
index f524d2f..d33d714 100755
--- a/t/t9114-git-svn-dcommit-merge.sh
+++ b/t/t9114-git-svn-dcommit-merge.sh

@@ -62,7 +62,7 @@ test_expect_success 'setup git mirror and merge' '
 	echo friend > README &&
 	cat tmp >> README &&
 	git commit -a -m "friend" &&
-	git pull . merge
+	git merge merge
 	'
 
 test_debug 'gitk --all & sleep 1'

diff --git a/t/t9500-gitweb-standalone-no-errors.sh b/t/t9500-gitweb-standalone-no-errors.sh
index 718014d..e74b9ab 100755
--- a/t/t9500-gitweb-standalone-no-errors.sh
+++ b/t/t9500-gitweb-standalone-no-errors.sh

@@ -328,7 +328,7 @@ test_expect_success \
 	 git add b &&
 	 git commit -a -m "On branch" &&
 	 git checkout master &&
-	 git pull . b &&
+	 git merge b &&
 	 git tag merge_commit'
 
 test_expect_success \

-- 
1.8.4.2+fc1

No functional changes.

Signed-off-by: Felipe Contreras <redacted>
---
 builtin/merge.c | 11 ++---------
 1 file changed, 2 insertions(+), 9 deletions(-)

diff --git a/builtin/merge.c b/builtin/merge.c
index 02a69c1..41fb66d 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c

@@ -186,13 +186,6 @@ static int option_parse_n(const struct option *opt,
 	return 0;
 }
 
-static int option_parse_ff_only(const struct option *opt,
-			  const char *arg, int unset)
-{
-	fast_forward = FF_ONLY;
-	return 0;
-}
-
 static struct option builtin_merge_options[] = {
 	{ OPTION_CALLBACK, 'n', NULL, NULL, NULL,
 		N_("do not show a diffstat at the end of the merge"),

@@ -210,9 +203,9 @@ static struct option builtin_merge_options[] = {
 	OPT_BOOL('e', "edit", &option_edit,
 		N_("edit message before committing")),
 	OPT_SET_INT(0, "ff", &fast_forward, N_("allow fast-forward (default)"), FF_ALLOW),
-	{ OPTION_CALLBACK, 0, "ff-only", NULL, NULL,
+	{ OPTION_SET_INT, 0, "ff-only", &fast_forward, NULL,
 		N_("abort if fast-forward is not possible"),
-		PARSE_OPT_NOARG | PARSE_OPT_NONEG, option_parse_ff_only },
+		PARSE_OPT_NOARG | PARSE_OPT_NONEG, NULL, FF_ONLY },
 	OPT_RERERE_AUTOUPDATE(&allow_rerere_auto),
 	OPT_BOOL(0, "verify-signatures", &verify_signatures,
 		N_("Verify that the named commit has a valid GPG signature")),

-- 
1.8.4.2+fc1

'origin/master' is very clear, no need to specify the 'remotes/' prefix,
or babysit the user.

Signed-off-by: Felipe Contreras <redacted>
---
 Documentation/git-pull.txt | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-pull.txt b/Documentation/git-pull.txt
index beea10b..03a39bc 100644
--- a/Documentation/git-pull.txt
+++ b/Documentation/git-pull.txt

@@ -39,7 +39,7 @@ Assume the following history exists and the current branch is
 "`master`":
 
 ------------
-	  A---B---C master on origin
+	  A---B---C origin/master
 	 /
     D---E---F---G master
 ------------

@@ -51,7 +51,7 @@ result in a new commit along with the names of the two parent commits
 and a log message from the user describing the changes.
 
 ------------
-	  A---B---C remotes/origin/master
+	  A---B---C origin/master
 	 /         \
     D---E---F---G---H master
 ------------

-- 
1.8.4.2+fc1

Otherwise we would have to include commit.h.

Signed-off-by: Felipe Contreras <redacted>
---
 shortlog.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/shortlog.h b/shortlog.h
index de4f86f..54bc07c 100644
--- a/shortlog.h
+++ b/shortlog.h

@@ -19,6 +19,8 @@ struct shortlog {
 	struct string_list mailmap;
 };
 
+struct commit;
+
 void shortlog_init(struct shortlog *log);
 
 void shortlog_add_commit(struct shortlog *log, struct commit *commit);

-- 
1.8.4.2+fc1

Otherwise we might not have 'struct diff_options'.

Signed-off-by: Felipe Contreras <redacted>
---
 revision.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/revision.h b/revision.h
index e7f1d21..89132df 100644
--- a/revision.h
+++ b/revision.h

@@ -5,6 +5,7 @@
 #include "grep.h"
 #include "notes.h"
 #include "commit.h"
+#include "diff.h"
 
 #define SEEN		(1u<<0)
 #define UNINTERESTING   (1u<<1)

-- 
1.8.4.2+fc1

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

Signed-off-by: Felipe Contreras <redacted>
---
 Documentation/git-fetch.txt | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Documentation/git-fetch.txt b/Documentation/git-fetch.txt
index e08a028..7e75dc4 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 the 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.4.2+fc1

Signed-off-by: Felipe Contreras <redacted>
---
 builtin/branch.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index ad0f86d..5696cf0 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c

@@ -975,9 +975,8 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 			die(_("no such branch '%s'"), argv[0]);
 		}
 
-		if (!branch_has_merge_config(branch)) {
+		if (!branch_has_merge_config(branch))
 			die(_("Branch '%s' has no upstream information"), branch->name);
-		}
 
 		strbuf_addf(&buf, "branch.%s.remote", branch->name);
 		git_config_set_multivar(buf.buf, NULL, NULL, 1);

-- 
1.8.4.2+fc1

Signed-off-by: Felipe Contreras <redacted>
---
 sha1_name.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/sha1_name.c b/sha1_name.c
index 0e5fe7f..e9c2999 100644
--- a/sha1_name.c
+++ b/sha1_name.c

@@ -343,7 +343,6 @@ static int get_short_sha1(const char *name, int len, unsigned char *sha1,
 	return status;
 }
 
-
 int for_each_abbrev(const char *prefix, each_abbrev_fn fn, void *cb_data)
 {
 	char hex_pfx[40];

-- 
1.8.4.2+fc1

Signed-off-by: Felipe Contreras <redacted>
---
 transport-helper.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/transport-helper.c b/transport-helper.c
index b32e2d6..673b7c2 100644
--- a/transport-helper.c
+++ b/transport-helper.c

@@ -269,6 +269,7 @@ static const char *unsupported_options[] = {
 	TRANS_OPT_THIN,
 	TRANS_OPT_KEEP
 	};
+
 static const char *boolean_options[] = {
 	TRANS_OPT_THIN,
 	TRANS_OPT_KEEP,

-- 
1.8.4.2+fc1

Signed-off-by: Felipe Contreras <redacted>
---
 revision.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/revision.c b/revision.c
index 3fdea51..956040c 100644
--- a/revision.c
+++ b/revision.c

@@ -1519,7 +1519,7 @@ struct cmdline_pathspec {
 static void append_prune_data(struct cmdline_pathspec *prune, const char **av)
 {
 	while (*av) {
-		ALLOC_GROW(prune->path, prune->nr+1, prune->alloc);
+		ALLOC_GROW(prune->path, prune->nr + 1, prune->alloc);
 		prune->path[prune->nr++] = *(av++);
 	}
 }

@@ -1531,7 +1531,7 @@ static void read_pathspec_from_stdin(struct rev_info *revs, struct strbuf *sb,
 		int len = sb->len;
 		if (len && sb->buf[len - 1] == '\n')
 			sb->buf[--len] = '\0';
-		ALLOC_GROW(prune->path, prune->nr+1, prune->alloc);
+		ALLOC_GROW(prune->path, prune->nr + 1, prune->alloc);
 		prune->path[prune->nr++] = xstrdup(sb->buf);
 	}
 }

@@ -2134,7 +2134,7 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
 		 *	call init_pathspec() to set revs->prune_data here.
 		 * }
 		 */
-		ALLOC_GROW(prune_data.path, prune_data.nr+1, prune_data.alloc);
+		ALLOC_GROW(prune_data.path, prune_data.nr + 1, prune_data.alloc);
 		prune_data.path[prune_data.nr++] = NULL;
 		parse_pathspec(&revs->prune_data, 0, 0,
 			       revs->prefix, prune_data.path);

@@ -2987,7 +2987,7 @@ static struct commit *get_revision_internal(struct rev_info *revs)
 	if (revs->max_count) {
 		c = get_revision_1(revs);
 		if (c) {
-			while (0 < revs->skip_count) {
+			while (revs->skip_count > 0) {
 				revs->skip_count--;
 				c = get_revision_1(revs);
 				if (!c)

@@ -3002,9 +3002,8 @@ static struct commit *get_revision_internal(struct rev_info *revs)
 	if (c)
 		c->object.flags |= SHOWN;
 
-	if (!revs->boundary) {
+	if (!revs->boundary)
 		return c;
-	}
 
 	if (!c) {
 		/*

@@ -3050,9 +3049,8 @@ struct commit *get_revision(struct rev_info *revs)
 
 	if (revs->reverse) {
 		reversed = NULL;
-		while ((c = get_revision_internal(revs))) {
+		while ((c = get_revision_internal(revs)))
 			commit_list_insert(c, &reversed);
-		}
 		revs->commits = reversed;
 		revs->reverse = 0;
 		revs->reverse_output_stage = 1;

-- 
1.8.4.2+fc1

Signed-off-by: Felipe Contreras <redacted>
---
 pretty.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/pretty.c b/pretty.c
index b4e32b7..962e82b 100644
--- a/pretty.c
+++ b/pretty.c

@@ -497,7 +497,7 @@ void pp_user_info(struct pretty_print_context *pp,
 static int is_empty_line(const char *line, int *len_p)
 {
 	int len = *len_p;
-	while (len && isspace(line[len-1]))
+	while (len && isspace(line[len - 1]))
 		len--;
 	*len_p = len;
 	return !len;

-- 
1.8.4.2+fc1

Signed-off-by: Felipe Contreras <redacted>
---
 builtin/describe.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/builtin/describe.c b/builtin/describe.c
index b9d3603..6f62109 100644
--- a/builtin/describe.c
+++ b/builtin/describe.c

@@ -9,7 +9,7 @@
 #include "hash.h"
 #include "argv-array.h"
 
-#define SEEN		(1u<<0)
+#define SEEN		(1u << 0)
 #define MAX_TAGS	(FLAG_BITS - 1)
 
 static const char * const describe_usage[] = {

@@ -36,7 +36,6 @@ static const char *diff_index_args[] = {
 	"diff-index", "--quiet", "HEAD", "--", NULL
 };
 
-
 struct commit_name {
 	struct commit_name *next;
 	unsigned char peeled[20];

@@ -46,6 +45,7 @@ struct commit_name {
 	unsigned char sha1[20];
 	char *path;
 };
+
 static const char *prio_names[] = {
 	"head", "lightweight", "annotated",
 };

@@ -488,9 +488,8 @@ int cmd_describe(int argc, const char **argv, const char *prefix)
 	} else if (dirty) {
 		die(_("--dirty is incompatible with commit-ishes"));
 	} else {
-		while (argc-- > 0) {
+		while (argc-- > 0)
 			describe(*argv++, argc == 0);
-		}
 	}
 	return 0;
 }

-- 
1.8.4.2+fc1

Signed-off-by: Felipe Contreras <redacted>
---
 builtin/diff.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/diff.c b/builtin/diff.c
index 2fb8c5d..adb93a9 100644
--- a/builtin/diff.c
+++ b/builtin/diff.c

@@ -169,7 +169,7 @@ static int builtin_diff_tree(struct rev_info *revs,
 	if (ent1->item->flags & UNINTERESTING)
 		swap = 1;
 	sha1[swap] = ent0->item->sha1;
-	sha1[1-swap] = ent1->item->sha1;
+	sha1[1 - swap] = ent1->item->sha1;
 	diff_tree_sha1(sha1[0], sha1[1], "", &revs->diffopt);
 	log_tree_diff_flush(revs);
 	return 0;

-- 
1.8.4.2+fc1

Signed-off-by: Felipe Contreras <redacted>
---
 builtin/add.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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.4.2+fc1

Signed-off-by: Felipe Contreras <redacted>
---
 setup.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/setup.c b/setup.c
index dbf4138..5432a31 100644
--- a/setup.c
+++ b/setup.c

@@ -563,7 +563,7 @@ static const char *setup_git_directory_gently_1(int *nongit_ok)
 {
 	const char *env_ceiling_dirs = getenv(CEILING_DIRECTORIES_ENVIRONMENT);
 	struct string_list ceiling_dirs = STRING_LIST_INIT_DUP;
-	static char cwd[PATH_MAX+1];
+	static char cwd[PATH_MAX + 1];
 	const char *gitdirenv, *ret;
 	char *gitfile;
 	int len, offset, offset_parent, ceil_offset = -1;

@@ -578,7 +578,7 @@ static const char *setup_git_directory_gently_1(int *nongit_ok)
 	if (nongit_ok)
 		*nongit_ok = 0;
 
-	if (!getcwd(cwd, sizeof(cwd)-1))
+	if (!getcwd(cwd, sizeof(cwd) - 1))
 		die_errno("Unable to read current working directory");
 	offset = len = strlen(cwd);
 

-- 
1.8.4.2+fc1

Signed-off-by: Felipe Contreras <redacted>
---
 run-command.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/run-command.c b/run-command.c
index 1b7f88e..3914d9c 100644
--- a/run-command.c
+++ b/run-command.c

@@ -406,13 +406,12 @@ fail_pipe:
 					unsetenv(*cmd->env);
 			}
 		}
-		if (cmd->git_cmd) {
+		if (cmd->git_cmd)
 			execv_git_cmd(cmd->argv);
-		} else if (cmd->use_shell) {
+		else if (cmd->use_shell)
 			execv_shell_cmd(cmd->argv);
-		} else {
+		else
 			sane_execvp(cmd->argv[0], (char *const*) cmd->argv);
-		}
 		if (errno == ENOENT) {
 			if (!cmd->silent_exec_failure)
 				error("cannot run %s: %s", cmd->argv[0],

@@ -446,7 +445,6 @@ fail_pipe:
 		cmd->pid = -1;
 	}
 	close(notify_pipe[0]);
-
 }
 #else
 {

@@ -480,11 +478,10 @@ fail_pipe:
 	if (cmd->env)
 		env = make_augmented_environ(cmd->env);
 
-	if (cmd->git_cmd) {
+	if (cmd->git_cmd)
 		cmd->argv = prepare_git_cmd(cmd->argv);
-	} else if (cmd->use_shell) {
+	else if (cmd->use_shell)
 		cmd->argv = prepare_shell_cmd(cmd->argv);
-	}
 
 	cmd->pid = mingw_spawnvpe(cmd->argv[0], cmd->argv, env, cmd->dir,
 				  fhin, fhout, fherr);

-- 
1.8.4.2+fc1

On 31.10.2013, at 10:25, Felipe Contreras [off-list ref] wrote:

Most of these have been sent before, but were not applied for one reason or
another.

All of these look fine and sensible to me. Some of the latter patches in the series might be a bit subjective (e.g. I personally don't mind "yoda" conditions at all), but none do harm, and most are a clear improvement. So I am all for applying this.

Cheers,
Max

Felipe Contreras (16):
 merge: simplify ff-only option
 t: replace pulls with merges
 pull: cleanup documentation
 fetch: add missing documentation
 revision: add missing include
 shortlog: add missing declaration
 branch: trivial style fix
 sha1-name: trivial style cleanup
 transport-helper: trivial style fix
 describe: trivial style fixes
 pretty: trivial style fix
 revision: trivial style fixes
 diff: trivial style fix
 run-command: trivial style fixes
 setup: trivial style fixes
 add: avoid yoda conditions

Documentation/git-fetch.txt            |  3 +++
Documentation/git-pull.txt             |  4 ++--
builtin/add.c                          |  2 +-
builtin/branch.c                       |  3 +--
builtin/describe.c                     |  7 +++----
builtin/diff.c                         |  2 +-
builtin/merge.c                        | 11 ++---------
pretty.c                               |  2 +-
revision.c                             | 14 ++++++--------
revision.h                             |  1 +
run-command.c                          | 13 +++++--------
setup.c                                |  4 ++--
sha1_name.c                            |  1 -
shortlog.h                             |  2 ++
t/annotate-tests.sh                    |  2 +-
t/t4200-rerere.sh                      |  2 +-
t/t9114-git-svn-dcommit-merge.sh       |  2 +-
t/t9500-gitweb-standalone-no-errors.sh |  2 +-
transport-helper.c                     |  1 +
19 files changed, 35 insertions(+), 43 deletions(-)

-- 
1.8.4.2+fc1

--
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 [off-list ref] writes:

I recall there were typofix comments on this line.

+unless there's an upstream branch configured for the current branch.

Also there was a phrasing comment on this one, I think.

Resending without rerolling is not very much appreciated.

+
 OPTIONS
 -------
 include::fetch-options.txt[]

Felipe Contreras [off-list ref] writes:

This is a step in the right direction to change the contract between
this header file and its consumers, but I think it falls short of
doing a good job at it.

The rule used to be that "if you use a declaration in revision.h,
you must include diff.h before including it, even if you do not use
any declaration made in diff.h yourself". 

The new rule this patch introduces is "if you use a declaration in
foo.h, include foo.h, period---foo.h should handle its requirement
on its own internally and consumers should not have to care", which
is much saner.

But the patch needs to also remove '#include "diff.h"' from existing
consumers that themselves do not use any declaration from "diff.h"
(e.g. bundle.c; there are others), while keeping the inclusion in
those that do (e.g. builtin/commit.c). That can be a separate patch
that immediately follow this one, or a part of the same patch.

Thanks.

Felipe Contreras [off-list ref] writes:

This change is wrong; the illustration depicts the distributed world
(i.e. a fetch has not happened yet).  The next sentence after this
picture reads:

    Then "`git pull`" will fetch and replay the changes from the remote
    `master` branch since it diverged from the local `master` 

In other words, your (remotes/)origin/master has _not_ caught up to
the reality.

This is a good change, especially in today's world.

Thanks.

On Thu, Oct 31, 2013 at 12:11 PM, Junio C Hamano [off-list ref] wrote:

Felipe Contreras [off-list ref] writes:

quoted

--- a/Documentation/git-pull.txt
+++ b/Documentation/git-pull.txt

@@ -39,7 +39,7 @@ Assume the following history exists and the current branch is
 "`master`":

 ------------
-       A---B---C master on origin
+       A---B---C origin/master
       /
     D---E---F---G master
 ------------

This change is wrong; the illustration depicts the distributed world
(i.e. a fetch has not happened yet).

That is an irrelevant implementation detail, specially at this high
level. In the user's mind origin/master means master on origin.

If you want to be pedantic, this is the "reality":

  ------------
      D---E---F---G master
  ------------

-- 
Felipe Contreras

On Thu, Oct 31, 2013 at 12:10 PM, Junio C Hamano [off-list ref] wrote:

Felipe Contreras [off-list ref] writes:

quoted

diff --git a/Documentation/git-fetch.txt b/Documentation/git-fetch.txt
index e08a028..7e75dc4 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 the default the `origin` remote will be used,

I recall there were typofix comments on this line.

--- a/Documentation/git-fetch.txt
+++ b/Documentation/git-fetch.txt

@@ -37,7 +37,7 @@ 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 the default the `origin` remote will be used,
+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

quoted

+unless there's an upstream branch configured for the current branch.

Also there was a phrasing comment on this one, I think.

There was no constructive comment, no alternative was proposed.

The conclusion of the discussion (at least mine) is that the phrasing is fine.

Resending without rerolling is not very much appreciated.

I missed a valid comment in one of my 160 pending patches. Sue me.

-- 
Felipe Contreras

I was recently confused by the yoda condition in this block of code from [1]

+ for (i = 0; i < revs.nr; i++)
+ if (&bases->item->object == &revs.commit[i]->object)
+ break; /* found */
+ if (revs.nr <= i)

I think I was particularly surprised because it came so soon after the
"i < revs.nr". I didn't bother commenting because it seemed too
subjective and the code base has tons of these. Something as simple as

  git grep '[0-9] [<>]' *.c

finds a bunch (probably with lots of false positives and negatives).

I guess what I'm trying to say is that either we accept them and get
used to reading them without being surprised, or we can change a bit
more than one at a time perhaps? I understand that this was an
occurrence you just happened to run into, and I'm not saying that a
patch has to deal with _all_ occurrences. I'm more just wondering if
we want mention our position, whatever it is, in CodingGuidelines.

Martin

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

On Thu, Oct 31, 2013 at 2:25 AM, Felipe Contreras
[off-list ref] wrote:

On Thu, Oct 31, 2013 at 1:48 PM, Martin von Zweigbergk
[off-list ref] wrote:

I guess what I'm trying to say is that either we accept them and get
used to reading them without being surprised, or we can change a bit
more than one at a time perhaps? I understand that this was an
occurrence you just happened to run into, and I'm not saying that a
patch has to deal with _all_ occurrences. I'm more just wondering if
we want mention our position, whatever it is, in CodingGuidelines.

Yes, I'm all in favor of updating CodingGuidelines with that.

-- 
Felipe Contreras