Re: [PATCH] Rewrite diff-no-index.c:read_directory() to use is_dot_or_dotdot() and rename it to read_dir()

Akshay Aurora· Mar 15, 2014, 06:44 UTC · lore

Forgot to mention, this is one of the microprojects for GSoC this year. Would be great to have some feedback.

On Fri, Mar 14, 2014 at 6:09 PM, Akshay Aurora <akshayaurora@yahoo.com> wrote:

Show 51 quoted lines

> I have renamed diff-no-index.c:read_directory() to read_dir() to avoid name collision with dir.c:read_directory()
>
> Signed-off-by: Akshay Aurora <akshayaurora@yahoo.com>
> ---
>  diff-no-index.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/diff-no-index.c b/diff-no-index.c
> index 8e10bff..2a17c9f 100644
> --- a/diff-no-index.c
> +++ b/diff-no-index.c
> @@ -10,13 +10,14 @@
>  #include "blob.h"
>  #include "tag.h"
>  #include "diff.h"
> +#include "dir.h"
>  #include "diffcore.h"
>  #include "revision.h"
>  #include "log-tree.h"
>  #include "builtin.h"
>  #include "string-list.h"
>
> -static int read_directory(const char *path, struct string_list *list)
> +static int read_dir(const char *path, struct string_list *list)
>  {
>         DIR *dir;
>         struct dirent *e;
> @@ -25,7 +26,7 @@ static int read_directory(const char *path, struct string_list *list)
>                 return error("Could not open directory %s", path);
>
>         while ((e = readdir(dir)))
> -               if (strcmp(".", e->d_name) && strcmp("..", e->d_name))
> +               if (!is_dot_or_dotdot(e->d_name))
>                         string_list_insert(list, e->d_name);
>
>         closedir(dir);
> @@ -107,9 +108,9 @@ static int queue_diff(struct diff_options *o,
>                 int i1, i2, ret = 0;
>                 size_t len1 = 0, len2 = 0;
>
> -               if (name1 && read_directory(name1, &p1))
> +               if (name1 && read_dir(name1, &p1))
>                         return -1;
> -               if (name2 && read_directory(name2, &p2)) {
> +               if (name2 && read_dir(name2, &p2)) {
>                         string_list_clear(&p1, 0);
>                         return -1;
>                 }
> --
> 1.8.5.3
>

-- 
With Thanks & Warm Regards
Akshay Aurora
iakshay.net

Thomas Rast· Mar 16, 2014, 11:07 UTC · re: Akshay Aurora · lore

Akshay Aurora <akshayaurora@yahoo.com> writes:

Show 7 quoted lines

> Forgot to mention, this is one of the microprojects for GSoC this
> year. Would be great to have some feedback.
>
> On Fri, Mar 14, 2014 at 6:09 PM, Akshay Aurora <akshayaurora@yahoo.com> wrote:
>> I have renamed diff-no-index.c:read_directory() to read_dir() to avoid name collision with dir.c:read_directory()
>>
>> Signed-off-by: Akshay Aurora <akshayaurora@yahoo.com>

Hmm, the original mail never made it through to me, and gmane doesn't seem to have it either. What happened here? The headers suggest you used git-send-email, which should avoid these problems. Can you dig up the command and configuration you used to send it (but be careful to not post your password!)?

On the patch itself:

> Subject: Re: [PATCH] Rewrite diff-no-index.c:read_directory() to use is_dot_or_dotdot() and rename it to read_dir()

The subject line is very long. Aim for 50 characters, but certainly no more than 72.

You are also conflating two separate things into one patch. Try to avoid doing that.

Furthermore I am unconvinced that renaming a function from read_directory() to read_dir() is a win. What are you trying to improve by the rename? Renames are good if they improve clarity and/or consistency, but read_dir() just risks confusion with readdir() and I cannot see any gain in consistency that would compensate for it.

>> I have renamed diff-no-index.c:read_directory() to read_dir() to avoid name collision with dir.c:read_directory()

Please stick to the style outlined in SubmittingPatches:

  The body should provide a meaningful commit message, which:

    . explains the problem the change tries to solve, iow, what is wrong
      with the current code without the change.

    . justifies the way the change solves the problem, iow, why the
      result with the change is better.

    . alternate solutions considered but discarded, if any.

  Describe your changes in imperative mood, e.g. "make xyzzy do frotz"
  instead of "[This patch] makes xyzzy do frotz" or "[I] changed xyzzy
  to do frotz", as if you are giving orders to the codebase to change
  its behaviour.  Try to make sure your explanation can be understood
  without external resources. Instead of giving a URL to a mailing list
  archive, summarize the relevant points of the discussion.

Also, please wrap your commit messages at 72 characters.

>> ---
>>  diff-no-index.c | 9 +++++----

The microproject idea said

  Rewrite diff-no-index.c:read_directory() to use
  is_dot_or_dotdot(). Try to find other sites that can use that
  function.

Are there any others?

-- 
Thomas Rast
tr@thomasrast.ch

patchRe: [PATCH] Rewrite diff-no-index.c:read_directory() to use is_dot_or_dotdot() and rename it to read_dir()