Re: [PATCH 13/22] config: change write_error() to take a (struct lock_file *) argument

2014-04-06 Thread Michael Haggerty 
On 04/02/2014 08:58 AM, Torsten Bögershausen wrote:
 On 04/01/2014 05:58 PM, Michael Haggerty wrote:
 Reduce the amount of code that has to know about the lock_file's
 filename field.

 Signed-off-by: Michael Haggerty mhag...@alum.mit.edu
 ---
   config.c | 10 +-
   1 file changed, 5 insertions(+), 5 deletions(-)

 diff --git a/config.c b/config.c
 index 6821cef..1ea3f39 100644
 --- a/config.c
 +++ b/config.c
 @@ -1303,9 +1303,9 @@ static int store_aux(const char *key, const char
 *value, void *cb)
   return 0;
   }
   -static int write_error(const char *filename)
 +static int write_error(struct lock_file *lk)
 Does the write_error() really need to know about  struct lock_file ?
 (The name of the function does not indicate that it is doing something
 with lk)
 And if, would it make sense to rename it into
 write_error_and_do_something() ?

I'm going to leave this part out of the next re-roll, because you are
right: this change is mostly a distraction and probably not an improvement.

Michael

-- 
Michael Haggerty
mhag...@alum.mit.edu
http://softwareswirl.blogspot.com/
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 13/22] config: change write_error() to take a (struct lock_file *) argument

2014-04-02 Thread Torsten Bögershausen 

On 04/01/2014 05:58 PM, Michael Haggerty wrote:

Reduce the amount of code that has to know about the lock_file's
filename field.

Signed-off-by: Michael Haggerty mhag...@alum.mit.edu
---
  config.c | 10 +-
  1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/config.c b/config.c
index 6821cef..1ea3f39 100644
--- a/config.c
+++ b/config.c
@@ -1303,9 +1303,9 @@ static int store_aux(const char *key, const char *value, 
void *cb)
return 0;
  }
  
-static int write_error(const char *filename)

+static int write_error(struct lock_file *lk)

Does the write_error() really need to know about  struct lock_file ?
(The name of the function does not indicate that it is doing something 
with lk)
And if, would it make sense to rename it into 
write_error_and_do_something() ?

  {
-   error(failed to write new configuration file %s, filename);
+   error(failed to write new configuration file %s, lk-filename);
  
  	/* Same error code as failed to rename. */

return 4;
@@ -1706,7 +1706,7 @@ out_free:
return ret;
  
  write_err_out:

-   ret = write_error(lock-filename);
+   ret = write_error(lock);
goto out_free;
  
  }

@@ -1821,7 +1821,7 @@ int git_config_rename_section_in_file(const char 
*config_filename,
}
store.baselen = strlen(new_name);
if (!store_write_section(out_fd, new_name)) {
-   ret = write_error(lock-filename);
+   ret = write_error(lock);
goto out;
}
/*
@@ -1847,7 +1847,7 @@ int git_config_rename_section_in_file(const char 
*config_filename,
continue;
length = strlen(output);
if (write_in_full(out_fd, output, length) != length) {
-   ret = write_error(lock-filename);
+   ret = write_error(lock);
goto out;
}
}


--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 13/22] config: change write_error() to take a (struct lock_file *) argument

2014-04-02 Thread Junio C Hamano 
Michael Haggerty mhag...@alum.mit.edu writes:

 Reduce the amount of code that has to know about the lock_file's
 filename field.

 Signed-off-by: Michael Haggerty mhag...@alum.mit.edu
 ---
  config.c | 10 +-
  1 file changed, 5 insertions(+), 5 deletions(-)

 diff --git a/config.c b/config.c
 index 6821cef..1ea3f39 100644
 --- a/config.c
 +++ b/config.c
 @@ -1303,9 +1303,9 @@ static int store_aux(const char *key, const char 
 *value, void *cb)
   return 0;
  }
  
 -static int write_error(const char *filename)
 +static int write_error(struct lock_file *lk)
  {

The earlier one would have been usable for reporting an error while
writing any file, but the caller must hold a lock file on it with a
new one.  Would this change warrant a renaming of the function, I
wonder.

It is a file-scope static, so all callers know about how they are
supposed to call it, hence the keeping the original name would be
OK, I would think.

This hunk triggered my smello-meter, primarily because write-error
would not be the name I would pick for this function if I were
writing everything in this file from scratch (before or after this
particular patch).

 - error(failed to write new configuration file %s, filename);
 + error(failed to write new configuration file %s, lk-filename);
  
   /* Same error code as failed to rename. */
   return 4;
 @@ -1706,7 +1706,7 @@ out_free:
   return ret;
  
  write_err_out:
 - ret = write_error(lock-filename);
 + ret = write_error(lock);
   goto out_free;
  
  }
 @@ -1821,7 +1821,7 @@ int git_config_rename_section_in_file(const char 
 *config_filename,
   }
   store.baselen = strlen(new_name);
   if (!store_write_section(out_fd, new_name)) {
 - ret = write_error(lock-filename);
 + ret = write_error(lock);
   goto out;
   }
   /*
 @@ -1847,7 +1847,7 @@ int git_config_rename_section_in_file(const char 
 *config_filename,
   continue;
   length = strlen(output);
   if (write_in_full(out_fd, output, length) != length) {
 - ret = write_error(lock-filename);
 + ret = write_error(lock);
   goto out;
   }
   }
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html