Re: [RFC PATCH 04/10] pack: move open_pack_index(), parse_pack_index()

[Jonathan Tan]

On Tue, 08 Aug 2017 13:19:23 -0700
Junio C Hamano  wrote:

> Jonathan Tan  writes:
> 
> > Signed-off-by: Jonathan Tan 
> > ---
> >  builtin/count-objects.c |   1 +
> >  cache.h |   8 ---
> >  pack.c  | 149 
> > 
> >  pack.h  |   8 +++
> >  sha1_file.c | 140 -
> >  sha1_name.c |   1 +
> >  6 files changed, 159 insertions(+), 148 deletions(-)
> 
> This patch is a bit strange...
> 
> > diff --git a/pack.c b/pack.c
> > index 60d9fc3b0..6edc43228 100644
> > --- a/pack.c
> > +++ b/pack.c
> > ...
> > +static struct packed_git *alloc_packed_git(int extra)
> > +{
> > +   struct packed_git *p = xmalloc(st_add(sizeof(*p), extra));
> > +   memset(p, 0, sizeof(*p));
> > +   p->pack_fd = -1;
> > +   return p;
> > +}
> > +
> > +struct packed_git *parse_pack_index(unsigned char *sha1, const char 
> > *idx_path)
> > +{
> > +   const char *path = sha1_pack_name(sha1);
> > +   size_t alloc = st_add(strlen(path), 1);
> > +   struct packed_git *p = alloc_packed_git(alloc);
> > +
> > +   memcpy(p->pack_name, path, alloc); /* includes NUL */
> > +   hashcpy(p->sha1, sha1);
> > +   if (check_packed_git_idx(idx_path, p)) {
> > +   free(p);
> > +   return NULL;
> > +   }
> > +
> > +   return p;
> > +}
> 
> We see these two functions appear in pack.c
> 
> > diff --git a/sha1_file.c b/sha1_file.c
> > index 0de39f480..2e414f5f5 100644
> > --- a/sha1_file.c
> > +++ b/sha1_file.c
> > ...
> > @@ -1300,22 +1176,6 @@ struct packed_git *add_packed_git(const char *path, 
> > size_t path_len, int local)
> > return p;
> >  }
> >  
> > -struct packed_git *parse_pack_index(unsigned char *sha1, const char 
> > *idx_path)
> > -{
> > -   const char *path = sha1_pack_name(sha1);
> > -   size_t alloc = st_add(strlen(path), 1);
> > -   struct packed_git *p = alloc_packed_git(alloc);
> > -
> > -   memcpy(p->pack_name, path, alloc); /* includes NUL */
> > -   hashcpy(p->sha1, sha1);
> > -   if (check_packed_git_idx(idx_path, p)) {
> > -   free(p);
> > -   return NULL;
> > -   }
> > -
> > -   return p;
> > -}
> > -
> 
> And we see parse_pack_index() came from sha1_file.c
> 
> But where did alloc_packed_git() come from?  Was the patch split
> incorrectly or something?
> 
> When I applied the whole series and did
> 
> git blame -s -w -M -C -C master.. pack.c
> 
> expecting that pretty much everything has come from sha1_file.c but
> noticed that some lines were actually blamed to a version of pack.c
> and these functions were among them.

alloc_packed_git() in pack.c is a duplicate of the function of the same
name in sha1_file.c in this patch, because at this patch, there are
still functions in both files using this function. A subsequent patch in
this patch set will remove it from pack.c.

I'll add a note explaining this to this patch in the next version.

Re: [RFC PATCH 04/10] pack: move open_pack_index(), parse_pack_index()

[Junio C Hamano]

Jonathan Tan  writes:

> Signed-off-by: Jonathan Tan 
> ---
>  builtin/count-objects.c |   1 +
>  cache.h |   8 ---
>  pack.c  | 149 
> 
>  pack.h  |   8 +++
>  sha1_file.c | 140 -
>  sha1_name.c |   1 +
>  6 files changed, 159 insertions(+), 148 deletions(-)

This patch is a bit strange...

> diff --git a/pack.c b/pack.c
> index 60d9fc3b0..6edc43228 100644
> --- a/pack.c
> +++ b/pack.c
> ...
> +static struct packed_git *alloc_packed_git(int extra)
> +{
> + struct packed_git *p = xmalloc(st_add(sizeof(*p), extra));
> + memset(p, 0, sizeof(*p));
> + p->pack_fd = -1;
> + return p;
> +}
> +
> +struct packed_git *parse_pack_index(unsigned char *sha1, const char 
> *idx_path)
> +{
> + const char *path = sha1_pack_name(sha1);
> + size_t alloc = st_add(strlen(path), 1);
> + struct packed_git *p = alloc_packed_git(alloc);
> +
> + memcpy(p->pack_name, path, alloc); /* includes NUL */
> + hashcpy(p->sha1, sha1);
> + if (check_packed_git_idx(idx_path, p)) {
> + free(p);
> + return NULL;
> + }
> +
> + return p;
> +}

We see these two functions appear in pack.c

> diff --git a/sha1_file.c b/sha1_file.c
> index 0de39f480..2e414f5f5 100644
> --- a/sha1_file.c
> +++ b/sha1_file.c
> ...
> @@ -1300,22 +1176,6 @@ struct packed_git *add_packed_git(const char *path, 
> size_t path_len, int local)
>   return p;
>  }
>  
> -struct packed_git *parse_pack_index(unsigned char *sha1, const char 
> *idx_path)
> -{
> - const char *path = sha1_pack_name(sha1);
> - size_t alloc = st_add(strlen(path), 1);
> - struct packed_git *p = alloc_packed_git(alloc);
> -
> - memcpy(p->pack_name, path, alloc); /* includes NUL */
> - hashcpy(p->sha1, sha1);
> - if (check_packed_git_idx(idx_path, p)) {
> - free(p);
> - return NULL;
> - }
> -
> - return p;
> -}
> -

And we see parse_pack_index() came from sha1_file.c

But where did alloc_packed_git() come from?  Was the patch split
incorrectly or something?

When I applied the whole series and did

git blame -s -w -M -C -C master.. pack.c

expecting that pretty much everything has come from sha1_file.c but
noticed that some lines were actually blamed to a version of pack.c
and these functions were among them.