threads / patch / 34988

patchsymbolic-ref: trivial style fix

Subject: [PATCH] symbolic-ref: trivial style fix

## tl;dr

6 messages between Sep 21, 2013 and Oct 16, 2013. Diffs are folded; open one to read it.

replies: 5people: 3as markdown or json

Felipe Contreras· Sep 21, 2013, 16:29 UTC · lore
Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 builtin/symbolic-ref.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to builtin/symbolic-ref.c +1 −1
diff --git a/builtin/symbolic-ref.c b/builtin/symbolic-ref.c
index f481959..71286b4 100644
--- a/builtin/symbolic-ref.c
+++ b/builtin/symbolic-ref.c
@@ -47,7 +47,7 @@ int cmd_symbolic_ref(int argc, const char **argv, const char *prefix)
 	git_config(git_default_config, NULL);
 	argc = parse_options(argc, argv, prefix, options,
 			     git_symbolic_ref_usage, 0);
-	if (msg &&!*msg)
+	if (msg && !*msg)
 		die("Refusing to perform update with empty message");
 
 	if (delete) {
-- 
1.8.4-fc
Junio C Hamano· Oct 15, 2013, 22:16 UTC · re: Felipe Contreras · lore

Re: [PATCH] symbolic-ref: trivial style fix

Felipe Contreras <felipe.contreras@gmail.com> writes:
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
Let's do something like this instead.
 - We usually refrain from making such a tree-wide change in order
   to avoid unnecessary conflicts with other "real work" patches,
   but the end result does not have a potentially cumbersome
   tree-wide impact.
 - We also tend to frown upon an "I fixed it here only because I
   happened to notice this one, there may be others but it is up to
   the readers to see if there are other instances" half-assed code
   churn.

The point of the proposed log message is to tell readers that this is a tree-wide clean-up that is worth applying.

-- >8 --
From: Felipe Contreras <felipe.contreras@gmail.com>
Subject: C: have space around && and || operators
Correct all hits from
    git grep -e '\(&&\|||\)[^ ]' -e '[^	 ]\(&&\|||\)' -- '*.c'

i.e. && or || operators that are followed by anything but a SP, or that follow something other than a SP or a HT, so that these operators have a SP around it when necessary.

We usually refrain from making this kind of a tree-wide change in order to avoid unnecessary conflicts with other "real work" patches, but the end result does not have a potentially cumbersome tree-wide impact.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/read-tree.c    | 2 +-
 builtin/rev-list.c     | 2 +-
 builtin/symbolic-ref.c | 2 +-
 compat/regex/regcomp.c | 2 +-
 xdiff/xemit.c          | 2 +-
 5 files changed, 5 insertions(+), 5 deletions(-)
Show changes to 5 files +5 −5

builtin/read-tree.c, builtin/rev-list.c, builtin/symbolic-ref.c, compat/regex/regcomp.c, xdiff/xemit.c

diff --git a/builtin/read-tree.c b/builtin/read-tree.c
index 0f5d7fe..0d7ef84 100644
--- a/builtin/read-tree.c
+++ b/builtin/read-tree.c
@@ -178,7 +178,7 @@ int cmd_read_tree(int argc, const char **argv, const char *unused_prefix)
 
 	if (1 < opts.index_only + opts.update)
 		die("-u and -i at the same time makes no sense");
-	if ((opts.update||opts.index_only) && !opts.merge)
+	if ((opts.update || opts.index_only) && !opts.merge)
 		die("%s is meaningless without -m, --reset, or --prefix",
 		    opts.update ? "-u" : "-i");
 	if ((opts.dir && !opts.update))
diff --git a/builtin/rev-list.c b/builtin/rev-list.c
index 4fc1616..0745e2d 100644
--- a/builtin/rev-list.c
+++ b/builtin/rev-list.c
@@ -322,7 +322,7 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)
 		revs.commit_format = CMIT_FMT_RAW;
 
 	if ((!revs.commits &&
-	     (!(revs.tag_objects||revs.tree_objects||revs.blob_objects) &&
+	     (!(revs.tag_objects || revs.tree_objects || revs.blob_objects) &&
 	      !revs.pending.nr)) ||
 	    revs.diff)
 		usage(rev_list_usage);
diff --git a/builtin/symbolic-ref.c b/builtin/symbolic-ref.c
index f481959..71286b4 100644
--- a/builtin/symbolic-ref.c
+++ b/builtin/symbolic-ref.c
@@ -47,7 +47,7 @@ int cmd_symbolic_ref(int argc, const char **argv, const char *prefix)
 	git_config(git_default_config, NULL);
 	argc = parse_options(argc, argv, prefix, options,
 			     git_symbolic_ref_usage, 0);
-	if (msg &&!*msg)
+	if (msg && !*msg)
 		die("Refusing to perform update with empty message");
 
 	if (delete) {
diff --git a/compat/regex/regcomp.c b/compat/regex/regcomp.c
index b2c5d46..06f3088 100644
--- a/compat/regex/regcomp.c
+++ b/compat/regex/regcomp.c
@@ -339,7 +339,7 @@ re_compile_fastmap_iter (regex_t *bufp, const re_dfastate_t *init_state,
 	      p = buf;
 	      *p++ = dfa->nodes[node].opr.c;
 	      while (++node < dfa->nodes_len
-		     &&	dfa->nodes[node].type == CHARACTER
+		     && dfa->nodes[node].type == CHARACTER
 		     && dfa->nodes[node].mb_partial)
 		*p++ = dfa->nodes[node].opr.c;
 	      memset (&state, '\0', sizeof (state));
diff --git a/xdiff/xemit.c b/xdiff/xemit.c
index 4d86458..4266ada 100644
--- a/xdiff/xemit.c
+++ b/xdiff/xemit.c
@@ -108,7 +108,7 @@ static long def_ff(const char *rec, long len, char *buf, long sz, void *priv)
 {
 	if (len > 0 &&
 			(isalpha((unsigned char)*rec) || /* identifier? */
-			 *rec == '_' ||	/* also identifier? */
+			 *rec == '_' || /* also identifier? */
 			 *rec == '$')) { /* identifiers from VMS and other esoterico */
 		if (len > sz)
 			len = sz;
Jonathan Nieder· Oct 15, 2013, 23:16 UTC · re: Junio C Hamano · lore

Re: [PATCH] symbolic-ref: trivial style fix

Junio C Hamano wrote:
> From: Felipe Contreras <felipe.contreras@gmail.com>
> Subject: C: have space around && and || operators
[...]
Show 6 quoted lines
>  builtin/read-tree.c    | 2 +-
>  builtin/rev-list.c     | 2 +-
>  builtin/symbolic-ref.c | 2 +-
>  compat/regex/regcomp.c | 2 +-
>  xdiff/xemit.c          | 2 +-
>  5 files changed, 5 insertions(+), 5 deletions(-)
For what it's worth,
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
[...]
Show 8 quoted lines
> --- a/compat/regex/regcomp.c
> +++ b/compat/regex/regcomp.c
> @@ -339,7 +339,7 @@ re_compile_fastmap_iter (regex_t *bufp, const re_dfastate_t *init_state,
>  	      p = buf;
>  	      *p++ = dfa->nodes[node].opr.c;
>  	      while (++node < dfa->nodes_len
> -		     &&	dfa->nodes[node].type == CHARACTER
> +		     && dfa->nodes[node].type == CHARACTER

It took a little staring to see what changed here. The preimage has a tab, probably from an autoformatter gone wild. I don't think fixing it should interfere with importing new versions of compat/regex, so the change seems fine.

Thanks, Jonathan

Junio C Hamano· Oct 15, 2013, 23:34 UTC · re: Jonathan Nieder · lore

Re: [PATCH] symbolic-ref: trivial style fix

Jonathan Nieder <jrnieder@gmail.com> writes:
Show 7 quoted lines
>> -		     &&	dfa->nodes[node].type == CHARACTER
>> +		     && dfa->nodes[node].type == CHARACTER
>
> It took a little staring to see what changed here.  The preimage has
> a tab, probably from an autoformatter gone wild.  I don't think fixing
> it should interfere with importing new versions of compat/regex, so
> the change seems fine.

xdiff/xemit.c had the breakage of the same nature. Perhaps the commit log message should mention these two.

Felipe Contreras· Oct 16, 2013, 03:33 UTC · re: Junio C Hamano · lore

Re: [PATCH] symbolic-ref: trivial style fix

Junio C Hamano wrote:
Show 11 quoted lines
> Felipe Contreras <felipe.contreras@gmail.com> writes:
> 
> > Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> > ---
> 
> Let's do something like this instead.
> 
>  - We usually refrain from making such a tree-wide change in order
>    to avoid unnecessary conflicts with other "real work" patches,
>    but the end result does not have a potentially cumbersome
>    tree-wide impact.

You might want to change that phrasing to "we usually give low priority". Sure, this is low priority, but this is something has to be done sooner or later, if we don't the code-style will be forever inconsistent.

If there is a conflict it would be sensible to delay the change for another release perhaps, it's perfectly sensible to ask the submitter to set another version, and hopefully there would be no conflict.

Usually resolving the conflict is trivial, but there's a lot of options.

Refraining from making a code-style fix is not ideal. We want those. It's only a matter of how to implement them.

>  - We also tend to frown upon an "I fixed it here only because I
>    happened to notice this one, there may be others but it is up to
>    the readers to see if there are other instances" half-assed code
>    churn.

If you have more general patch, sure, but would you really reject a patch that fixes one thing, because it doesn't fix all similar things at the same time?

Show 6 quoted lines
> The point of the proposed log message is to tell readers that this
> is a tree-wide clean-up that is worth applying.
> 
> -- >8 --
> From: Felipe Contreras <felipe.contreras@gmail.com>
> Subject: C: have space around && and || operators

Saying this patch is from me is not really accurate, it's based on a patch by me, or it was reported by me, but it's not really from me. I don't have a problem taking the credit though, as probably half the change is finding out there's a problem, just saying.

-- 
Felipe Contreras
Junio C Hamano· Oct 16, 2013, 21:38 UTC · re: Felipe Contreras · lore

Re: [PATCH] symbolic-ref: trivial style fix

Felipe Contreras <felipe.contreras@gmail.com> writes:
> Saying this patch is from me is not really accurate, it's based on a patch by
> me, or it was reported by me, but it's not really from me.
OK, will reword.
Thanks.

← back to recent threads