Re: [PATCH 13/18] fill_sha1_file: write "boring" characters

[Junio C Hamano]

Jacob Keller  writes:

>> The cost of fill function having to do the same thing repeatedly is
>> negligible, so I am OK with the result, but for fairness, this was
>> not "make the callers do this extra thing", but was "the caller can
>> prepare these unchanging parts just once, and the fill function that
>> is repeatedly run does not have to."
>
> Sure, but it's a pretty minor optimization and I think the result is
> easier to understand.

Yes; in case it wasn't clear, my comment was merely for fairness to
the original code.  I do agree that the end result of this series
makes a very pleasant read.

Re: [PATCH 13/18] fill_sha1_file: write "boring" characters

[Jacob Keller]

On Tue, Oct 4, 2016 at 2:46 PM, Junio C Hamano  wrote:
> Jacob Keller  writes:
>
>> On Mon, Oct 3, 2016 at 1:35 PM, Jeff King  wrote:
>>> This function forms a sha1 as "xx/...", but skips over
>>> the slot for the slash rather than writing it, leaving it to
>>> the caller to do so. It also does not bother to put in a
>>> trailing NUL, even though every caller would want it (we're
>>> forming a path which by definition is not a directory, so
>>> the only thing to do with it is feed it to a system call).
>>>
>>> Let's make the lives of our callers easier by just writing
>>> out the internal "/" and the NUL.
>>> ...
>>
>> I think this makes a lot more sense than making the callers have to do this.
>
> The cost of fill function having to do the same thing repeatedly is
> negligible, so I am OK with the result, but for fairness, this was
> not "make the callers do this extra thing", but was "the caller can
> prepare these unchanging parts just once, and the fill function that
> is repeatedly run does not have to."
>

Sure, but it's a pretty minor optimization and I think the result is
easier to understand.

Thanks,
Jake

Re: [PATCH 13/18] fill_sha1_file: write "boring" characters

[Jeff King]

On Tue, Oct 04, 2016 at 02:46:44PM -0700, Junio C Hamano wrote:

> Jacob Keller  writes:
> 
> > On Mon, Oct 3, 2016 at 1:35 PM, Jeff King  wrote:
> >> This function forms a sha1 as "xx/...", but skips over
> >> the slot for the slash rather than writing it, leaving it to
> >> the caller to do so. It also does not bother to put in a
> >> trailing NUL, even though every caller would want it (we're
> >> forming a path which by definition is not a directory, so
> >> the only thing to do with it is feed it to a system call).
> >>
> >> Let's make the lives of our callers easier by just writing
> >> out the internal "/" and the NUL.
> >> ...
> >
> > I think this makes a lot more sense than making the callers have to do this.
> 
> The cost of fill function having to do the same thing repeatedly is
> negligible, so I am OK with the result, but for fairness, this was
> not "make the callers do this extra thing", but was "the caller can
> prepare these unchanging parts just once, and the fill function that
> is repeatedly run does not have to."

Yeah, perhaps "does not bother" in the commit message is not entirely
fair. But it really does feel like quite a premature optimization to
skip the writing of one "/" in the middle of the string, especially as
it impacts the interface.

-Peff

Re: [PATCH 13/18] fill_sha1_file: write "boring" characters

[Junio C Hamano]

Jacob Keller  writes:

> On Mon, Oct 3, 2016 at 1:35 PM, Jeff King  wrote:
>> This function forms a sha1 as "xx/...", but skips over
>> the slot for the slash rather than writing it, leaving it to
>> the caller to do so. It also does not bother to put in a
>> trailing NUL, even though every caller would want it (we're
>> forming a path which by definition is not a directory, so
>> the only thing to do with it is feed it to a system call).
>>
>> Let's make the lives of our callers easier by just writing
>> out the internal "/" and the NUL.
>> ...
>
> I think this makes a lot more sense than making the callers have to do this.

The cost of fill function having to do the same thing repeatedly is
negligible, so I am OK with the result, but for fairness, this was
not "make the callers do this extra thing", but was "the caller can
prepare these unchanging parts just once, and the fill function that
is repeatedly run does not have to."

Re: [PATCH 13/18] fill_sha1_file: write "boring" characters

[Jacob Keller]

On Mon, Oct 3, 2016 at 1:35 PM, Jeff King  wrote:
> This function forms a sha1 as "xx/...", but skips over
> the slot for the slash rather than writing it, leaving it to
> the caller to do so. It also does not bother to put in a
> trailing NUL, even though every caller would want it (we're
> forming a path which by definition is not a directory, so
> the only thing to do with it is feed it to a system call).
>
> Let's make the lives of our callers easier by just writing
> out the internal "/" and the NUL.
>

Ya this makes sense.

> Signed-off-by: Jeff King 
> ---
>  sha1_file.c | 12 +---
>  1 file changed, 5 insertions(+), 7 deletions(-)
>
> diff --git a/sha1_file.c b/sha1_file.c
> index 70c3e2f..c6308c1 100644
> --- a/sha1_file.c
> +++ b/sha1_file.c
> @@ -178,10 +178,12 @@ static void fill_sha1_path(char *pathbuf, const 
> unsigned char *sha1)
> for (i = 0; i < 20; i++) {
> static char hex[] = "0123456789abcdef";
> unsigned int val = sha1[i];
> -   char *pos = pathbuf + i*2 + (i > 0);
> -   *pos++ = hex[val >> 4];
> -   *pos = hex[val & 0xf];
> +   *pathbuf++ = hex[val >> 4];
> +   *pathbuf++ = hex[val & 0xf];
> +   if (!i)
> +   *pathbuf++ = '/';
> }
> +   *pathbuf = '\0';

I think this makes a lot more sense than making the callers have to do this.

Thanks,
Jake

>  }
>
>  const char *sha1_file_name(const unsigned char *sha1)
> @@ -198,8 +200,6 @@ const char *sha1_file_name(const unsigned char *sha1)
> die("insanely long object directory %s", objdir);
> memcpy(buf, objdir, len);
> buf[len] = '/';
> -   buf[len+3] = '/';
> -   buf[len+42] = '\0';
> fill_sha1_path(buf + len + 1, sha1);
> return buf;
>  }
> @@ -406,8 +406,6 @@ struct alternate_object_database *alloc_alt_odb(const 
> char *dir)
>
> ent->name = ent->scratch + dirlen + 1;
> ent->scratch[dirlen] = '/';
> -   ent->scratch[dirlen + 3] = '/';
> -   ent->scratch[entlen-1] = 0;
>
> return ent;
>  }
> --
> 2.10.0.618.g82cc264
>

[PATCH 13/18] fill_sha1_file: write "boring" characters

[Jeff King]

This function forms a sha1 as "xx/...", but skips over
the slot for the slash rather than writing it, leaving it to
the caller to do so. It also does not bother to put in a
trailing NUL, even though every caller would want it (we're
forming a path which by definition is not a directory, so
the only thing to do with it is feed it to a system call).

Let's make the lives of our callers easier by just writing
out the internal "/" and the NUL.

Signed-off-by: Jeff King 
---
 sha1_file.c | 12 +---
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/sha1_file.c b/sha1_file.c
index 70c3e2f..c6308c1 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -178,10 +178,12 @@ static void fill_sha1_path(char *pathbuf, const unsigned 
char *sha1)
for (i = 0; i < 20; i++) {
static char hex[] = "0123456789abcdef";
unsigned int val = sha1[i];
-   char *pos = pathbuf + i*2 + (i > 0);
-   *pos++ = hex[val >> 4];
-   *pos = hex[val & 0xf];
+   *pathbuf++ = hex[val >> 4];
+   *pathbuf++ = hex[val & 0xf];
+   if (!i)
+   *pathbuf++ = '/';
}
+   *pathbuf = '\0';
 }
 
 const char *sha1_file_name(const unsigned char *sha1)
@@ -198,8 +200,6 @@ const char *sha1_file_name(const unsigned char *sha1)
die("insanely long object directory %s", objdir);
memcpy(buf, objdir, len);
buf[len] = '/';
-   buf[len+3] = '/';
-   buf[len+42] = '\0';
fill_sha1_path(buf + len + 1, sha1);
return buf;
 }
@@ -406,8 +406,6 @@ struct alternate_object_database *alloc_alt_odb(const char 
*dir)
 
ent->name = ent->scratch + dirlen + 1;
ent->scratch[dirlen] = '/';
-   ent->scratch[dirlen + 3] = '/';
-   ent->scratch[entlen-1] = 0;
 
return ent;
 }
-- 
2.10.0.618.g82cc264