Re: [PATCH] convert any hard coded .gitmodules file string to the MACRO

[Brandon Williams]

On 08/01, Junio C Hamano wrote:
> Stefan Beller  writes:
> 
> >>>   @@ -233,18 +233,18 @@ void gitmodules_config(void)
> >>> strbuf_addstr(_path, "/.gitmodules");
> >>
> >>
> >> Did you mean to also change "/.gitmodules" ??
> >
> > Goog point. We should pick that up as well. However as we do not have
> > a macro for that, we'd have to have 2 calls to strbuf API
> >
> > strbuf_addch(, '/');
> > strbuf_addstr(, GITMODULES);
> 
> Ehh, doesn't string literal concatenation work here?  I.e. something
> like:
> 
> strbuf_addstr(_path, "/" GITMODULES_FILE);

Also this doesn't really matter much since this line is removed latter
on in the series, but I'll go with the string literal concatenation for
the intermediate state.

> 
> 
> >>> if (pos < 0) { /* .gitmodules not found or isn't merged */
> >>> pos = -1 - pos;
> >>> if (active_nr > pos) {  /* there is a .gitmodules
> >>> */
> >>
> >>
> >> It might also be nice to change the literals in the comments to
> >> use the macro.
> 
> The reason you want this patch is not like we want to make it easy
> to rename the file to ".gitprojects" later, right?  The patch is
> about avoiding misspelled string constant, like "/.gitmdoules",
> without getting caught by the compiler, no?
> 
> Assuming that I am correctly guessing the intention, I think it is a
> bad idea to rename these in the comments.
> 
> 

-- 
Brandon Williams

Re: [PATCH] convert any hard coded .gitmodules file string to the MACRO

[Brandon Williams]

On 08/01, Junio C Hamano wrote:
> Stefan Beller  writes:
> 
> >>>   @@ -233,18 +233,18 @@ void gitmodules_config(void)
> >>> strbuf_addstr(_path, "/.gitmodules");
> >>
> >>
> >> Did you mean to also change "/.gitmodules" ??
> >
> > Goog point. We should pick that up as well. However as we do not have
> > a macro for that, we'd have to have 2 calls to strbuf API
> >
> > strbuf_addch(, '/');
> > strbuf_addstr(, GITMODULES);
> 
> Ehh, doesn't string literal concatenation work here?  I.e. something
> like:
> 
> strbuf_addstr(_path, "/" GITMODULES_FILE);
> 
> 
> >>> if (pos < 0) { /* .gitmodules not found or isn't merged */
> >>> pos = -1 - pos;
> >>> if (active_nr > pos) {  /* there is a .gitmodules
> >>> */
> >>
> >>
> >> It might also be nice to change the literals in the comments to
> >> use the macro.
> 
> The reason you want this patch is not like we want to make it easy
> to rename the file to ".gitprojects" later, right?  The patch is
> about avoiding misspelled string constant, like "/.gitmdoules",
> without getting caught by the compiler, no?

Yes, it was mostly about preventing mistakes and having the compiler
help you out a bit, so changing the comments isn't really needed.

> 
> Assuming that I am correctly guessing the intention, I think it is a
> bad idea to rename these in the comments.
> 
> 

-- 
Brandon Williams

Re: [PATCH] convert any hard coded .gitmodules file string to the MACRO

[Junio C Hamano]

Stefan Beller  writes:

>>>   @@ -233,18 +233,18 @@ void gitmodules_config(void)
>>> strbuf_addstr(_path, "/.gitmodules");
>>
>>
>> Did you mean to also change "/.gitmodules" ??
>
> Goog point. We should pick that up as well. However as we do not have
> a macro for that, we'd have to have 2 calls to strbuf API
>
> strbuf_addch(, '/');
> strbuf_addstr(, GITMODULES);

Ehh, doesn't string literal concatenation work here?  I.e. something
like:

strbuf_addstr(_path, "/" GITMODULES_FILE);


>>> if (pos < 0) { /* .gitmodules not found or isn't merged */
>>> pos = -1 - pos;
>>> if (active_nr > pos) {  /* there is a .gitmodules
>>> */
>>
>>
>> It might also be nice to change the literals in the comments to
>> use the macro.

The reason you want this patch is not like we want to make it easy
to rename the file to ".gitprojects" later, right?  The patch is
about avoiding misspelled string constant, like "/.gitmdoules",
without getting caught by the compiler, no?

Assuming that I am correctly guessing the intention, I think it is a
bad idea to rename these in the comments.

Re: [PATCH] convert any hard coded .gitmodules file string to the MACRO

[Stefan Beller]

On Tue, Aug 1, 2017 at 6:14 AM, Jeff Hostetler  wrote:
>
>
> On 7/31/2017 7:11 PM, Stefan Beller wrote:
>>
>> I used these commands:
>>$ cat sem.cocci
>>@@
>>@@
>>- ".gitmodules"
>>+ GITMODULES_FILE
>>
>>$ spatch --in-place --sp-file sem.cocci builtin/*.c *.c *.h
>>
>> Feel free to regenerate or squash it in or have it as a separate commit.
>>
>> Signed-off-by: Stefan Beller 
>> ---
>>   submodule.c| 18 +-
>>   unpack-trees.c |  2 +-
>>   2 files changed, 10 insertions(+), 10 deletions(-)
>>
>> diff --git a/submodule.c b/submodule.c
>> index 37f4a92872..b75d02ba7b 100644
>> --- a/submodule.c
>> +++ b/submodule.c
>>   @@ -233,18 +233,18 @@ void gitmodules_config(void)
>> strbuf_addstr(_path, "/.gitmodules");
>
>
> Did you mean to also change "/.gitmodules" ??

Goog point. We should pick that up as well. However as we do not have
a macro for that, we'd have to have 2 calls to strbuf API

strbuf_addch(, '/');
strbuf_addstr(, GITMODULES);

>
>> if (read_cache() < 0)
>> die("index file corrupt");
>> -   pos = cache_name_pos(".gitmodules", 11);
>> +   pos = cache_name_pos(GITMODULES_FILE, 11);
>> if (pos < 0) { /* .gitmodules not found or isn't merged */
>> pos = -1 - pos;
>> if (active_nr > pos) {  /* there is a .gitmodules
>> */
>
>
> It might also be nice to change the literals in the comments to
> use the macro.

Yes, I wondered if sed would have been better for this job.

>
> Jeff
>

Re: [PATCH] convert any hard coded .gitmodules file string to the MACRO

[Jeff Hostetler]

On 7/31/2017 7:11 PM, Stefan Beller wrote:

I used these commands:
   $ cat sem.cocci
   @@
   @@
   - ".gitmodules"
   + GITMODULES_FILE

   $ spatch --in-place --sp-file sem.cocci builtin/*.c *.c *.h

Feel free to regenerate or squash it in or have it as a separate commit.

Signed-off-by: Stefan Beller 
---
  submodule.c| 18 +-
  unpack-trees.c |  2 +-
  2 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/submodule.c b/submodule.c
index 37f4a92872..b75d02ba7b 100644
--- a/submodule.c
+++ b/submodule.c
  
@@ -233,18 +233,18 @@ void gitmodules_config(void)

strbuf_addstr(_path, "/.gitmodules");


Did you mean to also change "/.gitmodules" ??


if (read_cache() < 0)
die("index file corrupt");
-   pos = cache_name_pos(".gitmodules", 11);
+   pos = cache_name_pos(GITMODULES_FILE, 11);
if (pos < 0) { /* .gitmodules not found or isn't merged */
pos = -1 - pos;
if (active_nr > pos) {  /* there is a .gitmodules */


It might also be nice to change the literals in the comments to
use the macro.

Jeff

[PATCH] convert any hard coded .gitmodules file string to the MACRO

[Stefan Beller]

I used these commands:
  $ cat sem.cocci
  @@
  @@
  - ".gitmodules"
  + GITMODULES_FILE

  $ spatch --in-place --sp-file sem.cocci builtin/*.c *.c *.h

Feel free to regenerate or squash it in or have it as a separate commit.

Signed-off-by: Stefan Beller 
---
 submodule.c| 18 +-
 unpack-trees.c |  2 +-
 2 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/submodule.c b/submodule.c
index 37f4a92872..b75d02ba7b 100644
--- a/submodule.c
+++ b/submodule.c
@@ -63,7 +63,7 @@ int update_path_in_gitmodules(const char *oldpath, const char 
*newpath)
struct strbuf entry = STRBUF_INIT;
const struct submodule *submodule;
 
-   if (!file_exists(".gitmodules")) /* Do nothing without .gitmodules */
+   if (!file_exists(GITMODULES_FILE)) /* Do nothing without .gitmodules */
return -1;
 
if (gitmodules_is_unmerged)
@@ -77,7 +77,7 @@ int update_path_in_gitmodules(const char *oldpath, const char 
*newpath)
strbuf_addstr(, "submodule.");
strbuf_addstr(, submodule->name);
strbuf_addstr(, ".path");
-   if (git_config_set_in_file_gently(".gitmodules", entry.buf, newpath) < 
0) {
+   if (git_config_set_in_file_gently(GITMODULES_FILE, entry.buf, newpath) 
< 0) {
/* Maybe the user already did that, don't error out here */
warning(_("Could not update .gitmodules entry %s"), entry.buf);
strbuf_release();
@@ -97,7 +97,7 @@ int remove_path_from_gitmodules(const char *path)
struct strbuf sect = STRBUF_INIT;
const struct submodule *submodule;
 
-   if (!file_exists(".gitmodules")) /* Do nothing without .gitmodules */
+   if (!file_exists(GITMODULES_FILE)) /* Do nothing without .gitmodules */
return -1;
 
if (gitmodules_is_unmerged)
@@ -110,7 +110,7 @@ int remove_path_from_gitmodules(const char *path)
}
strbuf_addstr(, "submodule.");
strbuf_addstr(, submodule->name);
-   if (git_config_rename_section_in_file(".gitmodules", sect.buf, NULL) < 
0) {
+   if (git_config_rename_section_in_file(GITMODULES_FILE, sect.buf, NULL) 
< 0) {
/* Maybe the user already did that, don't error out here */
warning(_("Could not remove .gitmodules entry for %s"), path);
strbuf_release();
@@ -122,7 +122,7 @@ int remove_path_from_gitmodules(const char *path)
 
 void stage_updated_gitmodules(void)
 {
-   if (add_file_to_cache(".gitmodules", 0))
+   if (add_file_to_cache(GITMODULES_FILE, 0))
die(_("staging updated .gitmodules failed"));
 }
 
@@ -233,18 +233,18 @@ void gitmodules_config(void)
strbuf_addstr(_path, "/.gitmodules");
if (read_cache() < 0)
die("index file corrupt");
-   pos = cache_name_pos(".gitmodules", 11);
+   pos = cache_name_pos(GITMODULES_FILE, 11);
if (pos < 0) { /* .gitmodules not found or isn't merged */
pos = -1 - pos;
if (active_nr > pos) {  /* there is a .gitmodules */
const struct cache_entry *ce = 
active_cache[pos];
if (ce_namelen(ce) == 11 &&
-   !memcmp(ce->name, ".gitmodules", 11))
+   !memcmp(ce->name, GITMODULES_FILE, 11))
gitmodules_is_unmerged = 1;
}
} else if (pos < active_nr) {
struct stat st;
-   if (lstat(".gitmodules", ) == 0 &&
+   if (lstat(GITMODULES_FILE, ) == 0 &&
ce_match_stat(active_cache[pos], , 0) & 
DATA_CHANGED)
gitmodules_is_modified = 1;
}
@@ -264,7 +264,7 @@ static int gitmodules_cb(const char *var, const char 
*value, void *data)
 
 void repo_read_gitmodules(struct repository *repo)
 {
-   char *gitmodules_path = repo_worktree_path(repo, ".gitmodules");
+   char *gitmodules_path = repo_worktree_path(repo, GITMODULES_FILE);
 
git_config_from_file(gitmodules_cb, gitmodules_path, repo);
free(gitmodules_path);
diff --git a/unpack-trees.c b/unpack-trees.c
index dd535bc849..05335fe5bf 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -286,7 +286,7 @@ static void reload_gitmodules_file(struct index_state 
*index,
for (i = 0; i < index->cache_nr; i++) {
struct cache_entry *ce = index->cache[i];
if (ce->ce_flags & CE_UPDATE) {
-   int r = strcmp(ce->name, ".gitmodules");
+   int r = strcmp(ce->name, GITMODULES_FILE);
if (r < 0)
continue;
else if (r == 0) {
-- 
2.14.0.rc0.3.g6c2e499285