[PATCH] doc: camelCase the config variables to improve readability

[PATCH] doc: camelCase the config variables to improve readability

[Kaartic Sivaraam]

The config variable used weren't readable as they were in the
crude form of how git stores/uses it's config variables.

Improve it's readability by replacing them with camelCased versions
of config variables as it doesn't have any impact on it's usage.

Signed-off-by: Kaartic Sivaraam <kaarticsivaraam91196@gmail.com>
---
 Documentation/git-branch.txt | 4 ++--
 Documentation/git-tag.txt    | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt
index e292737b9c5ab..58f1e5c9c74e1 100644
--- a/Documentation/git-branch.txt
+++ b/Documentation/git-branch.txt
@@ -92,10 +92,10 @@ OPTIONS
 	all changes made to the branch ref, enabling use of date
 	based sha1 expressions such as "<branchname>@\{yesterday}".
 	Note that in non-bare repositories, reflogs are usually
-	enabled by default by the `core.logallrefupdates` config option.
+	enabled by default by the `core.logAllRefUpdates` config option.
 	The negated form `--no-create-reflog` only overrides an earlier
 	`--create-reflog`, but currently does not negate the setting of
-	`core.logallrefupdates`.
+	`core.logAllRefUpdates`.
 
 -f::
 --force::
diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
index 543fb425ee7c1..95e9f391d88fc 100644
--- a/Documentation/git-tag.txt
+++ b/Documentation/git-tag.txt
@@ -174,7 +174,7 @@ This option is only applicable when listing tags without annotation lines.
 	`core.logAllRefUpdates` in linkgit:git-config[1].
 	The negated form `--no-create-reflog` only overrides an earlier
 	`--create-reflog`, but currently does not negate the setting of
-	`core.logallrefupdates`.
+	`core.logAllRefUpdates`.
 
 <tagname>::
 	The name of the tag to create, delete, or describe.

--
https://github.com/git/git/pull/407

Re: [PATCH] doc: camelCase the config variables to improve readability

[Jonathan Nieder]

Hi,

Kaartic Sivaraam wrote:

> The config variable used weren't readable as they were in the
> crude form of how git stores/uses it's config variables.

nit: Git's commit messages describe the current behavior of Git in the
present tense.  Something like:

	These manpages' references to config variables are hard to read because
	they use all-lowercase names, without indicating where each word ends
	and begins in the configuration variable names.

	Improve readability by using camelCase instead.  Git treats these names
	case-insensitively so this does not affect functionality.

> Improve it's readability by replacing them with camelCased versions
> of config variables as it doesn't have any impact on it's usage.

nit: s/it's/its/ (in both places)

> Signed-off-by: Kaartic Sivaraam <kaarticsivaraam91196@gmail.com>
> ---
>  Documentation/git-branch.txt | 4 ++--
>  Documentation/git-tag.txt    | 2 +-
>  2 files changed, 3 insertions(+), 3 deletions(-)

This also is just more consistent with the rest of the docs.

FWIW, with or without the commit message tweaks mentioned above,
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Thanks for your attention to detail.

Re: [PATCH] doc: camelCase the config variables to improve readability

[Kaartic Sivaraam]

On Wednesday 20 September 2017 01:52 AM, Jonathan Nieder wrote:
> nit: Git's commit messages describe the current behavior of Git in the
> present tense.  Something like:

Thanks for the clarification. I was once searching 
Documentation/SubmittingPatches
to see if there is any guideline about the tense to use. Unfortunately, 
this seems to
be an undocumented guideline. Fixed it.

My little nit: I don't think this is really the a "current behaviour of 
*Git*". It's just
a document, you see ;-)

> 	These manpages' references to config variables are hard to read because
> 	they use all-lowercase names, without indicating where each word ends
> 	and begins in the configuration variable names.
>
> 	Improve readability by using camelCase instead.  Git treats these names
> 	case-insensitively so this does not affect functionality.

Thanks for providing the solution, also. Used it with a little tweak.

> This also is just more consistent with the rest of the docs.

Yes.

> FWIW, with or without the commit message tweaks mentioned above,
> Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Thanks.


> Thanks for your attention to detail.

You're welcome. Just for the note, I sent this because I had a hard time 
finding
the words that made up those config variable names when reading that part of
the 'git branch' documentation. I thought I could help a little in 
improving it.

---
Kaartic

[PATCH v2] doc: camelCase the config variables to improve readability

[Kaartic Sivaraam]

A few configuration variable names of Git are composite words. References
to such variables in manpages are hard to read because they use all-lowercase
names, without indicating where each word ends and begins.

Improve its readability by using camelCase instead.  Git treats these
names case-insensitively so this does not affect functionality. This
also ensures consistency with other parts of the docs that use camelCase
fo refer to configuration variable names.

Signed-off-by: Kaartic Sivaraam <kaarticsivaraam91196@gmail.com>
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
---
 Triple checked the 'In-reply-to' Mail-Id this time ;-)

 Documentation/git-branch.txt | 4 ++--
 Documentation/git-tag.txt    | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt
index e292737b9..58f1e5c9c 100644
--- a/Documentation/git-branch.txt
+++ b/Documentation/git-branch.txt
@@ -92,10 +92,10 @@ OPTIONS
 	all changes made to the branch ref, enabling use of date
 	based sha1 expressions such as "<branchname>@\{yesterday}".
 	Note that in non-bare repositories, reflogs are usually
-	enabled by default by the `core.logallrefupdates` config option.
+	enabled by default by the `core.logAllRefUpdates` config option.
 	The negated form `--no-create-reflog` only overrides an earlier
 	`--create-reflog`, but currently does not negate the setting of
-	`core.logallrefupdates`.
+	`core.logAllRefUpdates`.
 
 -f::
 --force::
diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
index 543fb425e..95e9f391d 100644
--- a/Documentation/git-tag.txt
+++ b/Documentation/git-tag.txt
@@ -174,7 +174,7 @@ This option is only applicable when listing tags without annotation lines.
 	`core.logAllRefUpdates` in linkgit:git-config[1].
 	The negated form `--no-create-reflog` only overrides an earlier
 	`--create-reflog`, but currently does not negate the setting of
-	`core.logallrefupdates`.
+	`core.logAllRefUpdates`.
 
 <tagname>::
 	The name of the tag to create, delete, or describe.
-- 
2.14.1.868.g66c78774b

Re: [PATCH v2] doc: camelCase the config variables to improve readability

[Junio C Hamano]

Kaartic Sivaraam <kaarticsivaraam91196@gmail.com> writes:

> A few configuration variable names of Git are composite words. References
> to such variables in manpages are hard to read because they use all-lowercase
> names, without indicating where each word ends and begins.
>
> Improve its readability by using camelCase instead.  Git treats these
> names case-insensitively so this does not affect functionality. This
> also ensures consistency with other parts of the docs that use camelCase
> fo refer to configuration variable names.

s/fo/to/ (or s/fo/in order to/)?  

I think the one I have on 'pu' uses Jonathan's suggested rewrite of
the log message verbatim, and I suspect that it is already clear
enough, even though I admit that I stuttered a bit while reading its
first sentence.

I'd avoid "A few" above, as it is my impression that we use
multi-word names quite a bit, not just a few.

Perhaps

	References to multi-word configuration variable names in our
	documentation must consistently use camelCase to highlight
	where the word boundaries are, even though these are treated
	case insensitively.

	Fix a few places that spell them in all lowercase, which
	makes them harder to read.

may be a more succinct way to say the same thing.  We state the rule
upfront, explain what the rule is for, and tell the codebase to
apply the rule.  That should cover everything your version and
Jonathan's version wanted to convey, I'd think.

Re: [PATCH v2] doc: camelCase the config variables to improve readability

[Kaartic Sivaraam]

On Sun, 2017-09-24 at 09:28 +0900, Junio C Hamano wrote:
> Kaartic Sivaraam <kaarticsivaraam91196@gmail.com> writes:
> 
> > A few configuration variable names of Git are composite words. References
> > to such variables in manpages are hard to read because they use all-lowercase
> > names, without indicating where each word ends and begins.
> > 
> > Improve its readability by using camelCase instead.  Git treats these
> > names case-insensitively so this does not affect functionality. This
> > also ensures consistency with other parts of the docs that use camelCase
> > fo refer to configuration variable names.
> 
> s/fo/to/ (or s/fo/in order to/)?  
> 

Yeah, a typo that I missed.

> Perhaps
> 
> 	References to multi-word configuration variable names in our
> 	documentation must consistently use camelCase to highlight
> 	where the word boundaries are, even though these are treated
> 	case insensitively.
> 
> 	Fix a few places that spell them in all lowercase, which
> 	makes them harder to read.
> 
> may be a more succinct way to say the same thing.  We state the rule
> upfront, explain what the rule is for, and tell the codebase to
> apply the rule.  That should cover everything your version and
> Jonathan's version wanted to convey, I'd think.
> 

Much better, thanks. Will resend with this updated message.

---
Kaartic

[PATCH v3] doc: camelCase the config variables to improve readability

[Kaartic Sivaraam]

References to multi-word configuration variable names in our
documentation must consistently use camelCase to highlight
where the word boundaries are, even though these are treated
case insensitively.

Fix a few places that spell them in all lowercase, which
makes them harder to read.

Signed-off-by: Kaartic Sivaraam <kaarticsivaraam91196@gmail.com>
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
---

 Changes in v3:

    - Used the commit message as suggested by Junio

 Documentation/git-branch.txt | 4 ++--
 Documentation/git-tag.txt    | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt
index e292737b9..58f1e5c9c 100644
--- a/Documentation/git-branch.txt
+++ b/Documentation/git-branch.txt
@@ -92,10 +92,10 @@ OPTIONS
 	all changes made to the branch ref, enabling use of date
 	based sha1 expressions such as "<branchname>@\{yesterday}".
 	Note that in non-bare repositories, reflogs are usually
-	enabled by default by the `core.logallrefupdates` config option.
+	enabled by default by the `core.logAllRefUpdates` config option.
 	The negated form `--no-create-reflog` only overrides an earlier
 	`--create-reflog`, but currently does not negate the setting of
-	`core.logallrefupdates`.
+	`core.logAllRefUpdates`.
 
 -f::
 --force::
diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
index 543fb425e..95e9f391d 100644
--- a/Documentation/git-tag.txt
+++ b/Documentation/git-tag.txt
@@ -174,7 +174,7 @@ This option is only applicable when listing tags without annotation lines.
 	`core.logAllRefUpdates` in linkgit:git-config[1].
 	The negated form `--no-create-reflog` only overrides an earlier
 	`--create-reflog`, but currently does not negate the setting of
-	`core.logallrefupdates`.
+	`core.logAllRefUpdates`.
 
 <tagname>::
 	The name of the tag to create, delete, or describe.
-- 
2.14.1.539.g78797280a.dirty

[PATCH v2] for_each_string_list_item: avoid undefined behavior for empty list

[Jonathan Nieder]

From: Michael Haggerty <mhagger@alum.mit.edu>

If you pass a newly initialized or newly cleared `string_list` to
`for_each_string_list_item()`, then the latter does

    for (
            item = (list)->items; /* NULL */
            item < (list)->items + (list)->nr; /* NULL + 0 */
            ++item)

Even though this probably works almost everywhere, it is undefined
behavior, and it could plausibly cause highly-optimizing compilers to
misbehave.  C99 section 6.5.6 paragraph 8 explains:

    If both the pointer operand and the result point to elements
    of the same array object, or one past the last element of the
    array object, the evaluation shall not produce an overflow;
    otherwise, the behavior is undefined.

and (6.3.2.3.3) a null pointer does not point to anything.

Guard the loop with a NULL check to make the intent crystal clear to
even the most pedantic compiler.  A suitably clever compiler could let
the NULL check only run in the first iteration, but regardless, this
overhead is likely to be dwarfed by the work to be done on each item.

This problem was noticed by Coverity.

[jn: using a NULL check instead of a placeholder empty list;
 fleshed out the commit message based on mailing list discussion]

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 string-list.h | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Junio C Hamano wrote:
> Jonathan Nieder <jrnieder@gmail.com> writes:

>> ...  But a quick test with gcc 4.8.4
>> -O2 finds that at least this compiler does not contain such an
>> optimization.  The overhead Michael Haggerty mentioned is real.
>
> Still, I have a feeling that users of string_list wouldn't care
> the overhead of single pointer NULL-ness check.
>
>  - apply.c collects conflicted paths and reports them with fprintf().
>
>  - builtin/clean.c uses the function to walk the list of paths to be
>    removed, and either does a human interaction (for "-i" codepath)
>    or goes to the filesystem to remove things.
>
>  - builtin/config.c uses it in get_urlmatch() in preparation for
>    doing network-y things.
>
>  - builtin/describe.c walks the list of exclude and include patterns
>    to run wildmatch on the candidate reference name to filter it out.
>
>  ...
>
> In all of these examples, what happens for each item in the loop
> seems to me far heavier than the overhead this macro adds.

Yes, agreed.  As a small tweak,

   #define for_each_string_list_item(item, list) \
	for (item = ...; item && ...; ...)

produces nicer assembly than

   #define for_each_string_list_item(item, list) \
	for (item = ...; list->items && ...; ...)

(By the way, the potential optimization I described isn't valid: we
know that when item == NULL and list->items == NULL, list->nr is
always zero, but the compiler has no way to know that.  So it can't
eliminate the NULL test.  For comparison, a suitably smart compiler
should be able to eliminate a 'list->nr != 0 &&' guard if 'list'
doesn't escape in the loop body.)

Recapping the other proposed fixes:

A. Make it an invariant of string_list that items is never NULL and
   update string_list_init et al to use an empty array.  This is
   pretty painless until you notice some other structs that embed
   string_list without using STRING_LIST_INIT.  Updating all those
   would be too painful.

B. #define for_each_string_list_item(item, list) \
	if (list->items) \
		for (item = ...; ...; ... )

   This breaks a caller like
	if (foo)
		for_each_string_list_item(item, list)
			...
	else
		...

   making it a non-starter.

C. As Gábor suggested,
   #define for_each_string_list_item(item, list) \
   	if (!list->items) \
		; /* nothing to do */ \
	else \
		for (item = ...; ...; ...)

   This handles the caller from (B) correctly.  But it produces
   compiler warnings for a caller like

	if (foo)
		for_each_string_list_item(item, list)
			...

   There is only one instance of that construct in git today.  It
   looks nicer anyway with braces, so this approach would also be
   promising.

D. Eliminate for_each_string_list_item and let callers just do

	unsigned int i;
	for (i = 0; i < list->nr; i++) {
		struct string_list_item *item = list->items[i];
		...
	}

   Having to declare item is unnecessarily verbose, decreasing the
   appeal of this option.  I think I like it anyway, but I wasn't able
   to convince coccinelle to do it.

E. Use subtraction instead of addition:
   #define for_each_string_list_item(item, list) \
   	for (item = ...; \
	     (item == list->items ? 0 : item - list->items) < nr; \
	     item++)

   I expected the compiler to figure out that this is a long way of writing
   (item - list->items), but at least with gcc 4.8.4 -O2, no such
   luck.  This generates uglier assembly than the NULL check.

diff --git a/string-list.h b/string-list.h
index 29bfb7ae45..79ae567cbc 100644
--- a/string-list.h
+++ b/string-list.h
@@ -32,8 +32,10 @@ void string_list_clear_func(struct string_list *list, string_list_clear_func_t c
 typedef int (*string_list_each_func_t)(struct string_list_item *, void *);
 int for_each_string_list(struct string_list *list,
 			 string_list_each_func_t, void *cb_data);
-#define for_each_string_list_item(item,list) \
-	for (item = (list)->items; item < (list)->items + (list)->nr; ++item)
+#define for_each_string_list_item(item,list)            \
+	for (item = (list)->items;                      \
+	     item && item < (list)->items + (list)->nr; \
+	     ++item)
 
 /*
  * Apply want to each item in list, retaining only the ones for which
-- 
2.14.1.821.g8fa685d3b7

[PATCH v2] doc: camelCase the config variables to improve readability

[Kaartic Sivaraam]

A few configuration variable names of Git are composite words. References
to such variables in manpages are hard to read because they use all-lowercase
names, without indicating where each word ends and begins.

Improve its readability by using camelCase instead.  Git treats these
names case-insensitively so this does not affect functionality. This
also ensures consistency with other parts of the docs that use camelCase
fo refer to configuration variable names.

Signed-off-by: Kaartic Sivaraam <kaarticsivaraam91196@gmail.com>
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
---
 Documentation/git-branch.txt | 4 ++--
 Documentation/git-tag.txt    | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt
index e292737b9c5ab..58f1e5c9c74e1 100644
--- a/Documentation/git-branch.txt
+++ b/Documentation/git-branch.txt
@@ -92,10 +92,10 @@ OPTIONS
 	all changes made to the branch ref, enabling use of date
 	based sha1 expressions such as "<branchname>@\{yesterday}".
 	Note that in non-bare repositories, reflogs are usually
-	enabled by default by the `core.logallrefupdates` config option.
+	enabled by default by the `core.logAllRefUpdates` config option.
 	The negated form `--no-create-reflog` only overrides an earlier
 	`--create-reflog`, but currently does not negate the setting of
-	`core.logallrefupdates`.
+	`core.logAllRefUpdates`.
 
 -f::
 --force::
diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
index 543fb425ee7c1..95e9f391d88fc 100644
--- a/Documentation/git-tag.txt
+++ b/Documentation/git-tag.txt
@@ -174,7 +174,7 @@ This option is only applicable when listing tags without annotation lines.
 	`core.logAllRefUpdates` in linkgit:git-config[1].
 	The negated form `--no-create-reflog` only overrides an earlier
 	`--create-reflog`, but currently does not negate the setting of
-	`core.logallrefupdates`.
+	`core.logAllRefUpdates`.
 
 <tagname>::
 	The name of the tag to create, delete, or describe.

--
https://github.com/git/git/pull/407