Re: [PATCH] [GSOC] get_non_kept_pack_filenames(): reimplement using iterators

From: Stefan Beller <sbeller@google.com>
Date: Mar 28, 2017, 18:23 UTC
Message-ID: <CAGZ79kbtXEBKBaw75-tLay4LO1F3i3nDz6CU8FgTkJ7iwEY5QQ@mail.gmail.com>
In-Reply-To: <1490665185-11809-1-git-send-email-robert.stanca7@gmail.com>

On Mon, Mar 27, 2017 at 6:39 PM, Robert Stanca <robert.stanca7@gmail.com> wrote:

Show 62 quoted lines

> Replaces recursive traversing of opendir with dir_iterator.
>
> Signed-off-by: Robert Stanca <robert.stanca7@gmail.com>
> ---
>  builtin/repack.c | 15 ++++++---------
>  1 file changed, 6 insertions(+), 9 deletions(-)
>
> diff --git a/builtin/repack.c b/builtin/repack.c
> index 677bc7c..27a5597 100644
> --- a/builtin/repack.c
> +++ b/builtin/repack.c
> @@ -7,6 +7,8 @@
>  #include "strbuf.h"
>  #include "string-list.h"
>  #include "argv-array.h"
> +#include "iterator.h"
> +#include "dir-iterator.h"
>
>  static int delta_base_offset = 1;
>  static int pack_kept_objects = -1;
> @@ -86,26 +88,21 @@ static void remove_pack_on_signal(int signo)
>   */
>  static void get_non_kept_pack_filenames(struct string_list *fname_list)
>  {
> -       DIR *dir;
> -       struct dirent *e;
> +       struct dir_iterator *diter = dir_iterator_begin(packdir);
>         char *fname;
>
> -       if (!(dir = opendir(packdir)))
> -               return;
> -
> -       while ((e = readdir(dir)) != NULL) {
> +       while (dir_iterator_advance(diter) == ITER_OK) {
>                 size_t len;
> -               if (!strip_suffix(e->d_name, ".pack", &len))
> +               if (!strip_suffix(diter->relative_path, ".pack", &len))
>                         continue;
>
> -               fname = xmemdupz(e->d_name, len);
> +               fname = xmemdupz(diter->relative_path, len);
>
>                 if (!file_exists(mkpath("%s/%s.keep", packdir, fname)))
>                         string_list_append_nodup(fname_list, fname);
>                 else
>                         free(fname);
>         }
> -       closedir(dir);
>  }
>
>  static void remove_redundant_pack(const char *dir_name, const char *base_name)
> --
> 2.7.4
>
>
>
>
> Hi , this is my first patch submission for Git Gsoc. I ran full tests and local tests with
> prove --timer --jobs 15 ./t*pack*.sh .
>
> Have a great day,
>              Robert.

Hi and welcome to the Git community!

The patch looks like a faithful conversion with no side effects to me.
Reviewed-by: Stefan Beller <sbeller@google.com>

Note: mail readers usually collapse long signatures (e.g. everything
after "--\n2.7.4"  in this email. So I did not know or even read your
comments on how you tested this patch before hitting reply.
You can also put these lines after the "---" after the sign off,
right before the diff stat.  A good example is
https://public-inbox.org/git/20170324231013.23346-1-avarab@gmail.com/
as the patch is also long enough, such that people may not scroll to the
bottom.  Once you're used to it, it is very easy to spot the chatter
that will not be part of the commit message, though.

Thanks, Stefan