dir.c: Fix comments to agree with argument name

[nipunn1313]

Comments are out of date with the variable names.

Update since v1

• Change "SHA1" to "oid" for consistency as suggested by Peff

cc: Jeff King peff@peff.net
cc: Nipunn Koorapati nipunn1313@gmail.com

[gitgitgadget]

There is an issue in commit 5751971:
Prefixed commit message must be in lower case: dir.c: Fix comments to agree with argument name

[nipunn1313]

/submit

[gitgitgadget]

Submitted as pull.757.git.1602766160815.gitgitgadget@gmail.com

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git pr-757/nipunn1313/comments-v1

To fetch this version to local tag pr-757/nipunn1313/comments-v1:

git fetch --no-tags https://github.com/gitgitgadget/git tag pr-757/nipunn1313/comments-v1

[gitgitgadget]

On the Git mailing list, Jeff King wrote (reply to this):

On Thu, Oct 15, 2020 at 12:49:20PM +0000, Nipunn Koorapati via GitGitGadget wrote:

> diff --git a/dir.c b/dir.c
> index 78387110e6..4c79c4f0e1 100644
> --- a/dir.c
> +++ b/dir.c
> @@ -1040,9 +1040,9 @@ static int add_patterns_from_buffer(char *buf, size_t size,
>   * an index if 'istate' is non-null), parse it and store the
>   * exclude rules in "pl".
>   *
> - * If "ss" is not NULL, compute SHA-1 of the exclude file and fill
> + * If "oid_stat" is not NULL, compute SHA-1 of the exclude file and fill

Makes sense. This changed as part of 4b33e60201 (dir: convert struct
sha1_stat to use object_id, 2018-01-28). Perhaps it would likewise make
sense to stop saying "SHA-1" here, and just say "hash" (or even "object
id", though TBH I think the fact that the hash is the same as an
object-id is largely an implementation detail).

-Peff

[gitgitgadget]

User Jeff King <peff@peff.net> has been added to the cc: list.

[nipunn1313]

/submit

[gitgitgadget]

Submitted as pull.757.v2.git.1602779316760.gitgitgadget@gmail.com

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git pr-757/nipunn1313/comments-v2

To fetch this version to local tag pr-757/nipunn1313/comments-v2:

git fetch --no-tags https://github.com/gitgitgadget/git tag pr-757/nipunn1313/comments-v2

[gitgitgadget]

On the Git mailing list, Junio C Hamano wrote (reply to this):

Jeff King <peff@peff.net> writes:

>> - * If "ss" is not NULL, compute SHA-1 of the exclude file and fill
>> + * If "oid_stat" is not NULL, compute SHA-1 of the exclude file and fill
>
> Makes sense. This changed as part of 4b33e60201 (dir: convert struct
> sha1_stat to use object_id, 2018-01-28). Perhaps it would likewise make
> sense to stop saying "SHA-1" here, and just say "hash" (or even "object
> id", though TBH I think the fact that the hash is the same as an
> object-id is largely an implementation detail).

I do not quite get your "though TBH", though.  I do agree with you
that it is an implementation detail that an object is named after
the hash of its contents, so saying "compute object name" probably
makes sense in more context than "compute hash" outside the narrow
parts of the code that actually implements how object names are
computed.  So I would have expected "because TBH", not "though TBH".

Anyway.  Nipunn, can you fix both of them in the same commit, as
they are addressing a problem from the same cause (i.e. we are no
longer SHA-1 centric).

Thanks.

[gitgitgadget]

On the Git mailing list, Nipunn Koorapati wrote (reply to this):

Happy to update it to use the object based terminology, though I'm not
sure how the desired final result differs from above.
I believe I said "compute oid" in the comment - and it is all in one commit.

gitgitgadget appears to have shown a range-diff from the previous
iteration, but the latest iteration is still one commit.

--Nipunn

On Thu, Oct 15, 2020 at 7:41 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Jeff King <peff@peff.net> writes:
>
> >> - * If "ss" is not NULL, compute SHA-1 of the exclude file and fill
> >> + * If "oid_stat" is not NULL, compute SHA-1 of the exclude file and fill
> >
> > Makes sense. This changed as part of 4b33e60201 (dir: convert struct
> > sha1_stat to use object_id, 2018-01-28). Perhaps it would likewise make
> > sense to stop saying "SHA-1" here, and just say "hash" (or even "object
> > id", though TBH I think the fact that the hash is the same as an
> > object-id is largely an implementation detail).
>
> I do not quite get your "though TBH", though.  I do agree with you
> that it is an implementation detail that an object is named after
> the hash of its contents, so saying "compute object name" probably
> makes sense in more context than "compute hash" outside the narrow
> parts of the code that actually implements how object names are
> computed.  So I would have expected "because TBH", not "though TBH".
>
> Anyway.  Nipunn, can you fix both of them in the same commit, as
> they are addressing a problem from the same cause (i.e. we are no
> longer SHA-1 centric).
>
> Thanks.

[gitgitgadget]

User Nipunn Koorapati <nipunn1313@gmail.com> has been added to the cc: list.

[gitgitgadget]

On the Git mailing list, Jeff King wrote (reply to this):

On Thu, Oct 15, 2020 at 11:41:36AM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> >> - * If "ss" is not NULL, compute SHA-1 of the exclude file and fill
> >> + * If "oid_stat" is not NULL, compute SHA-1 of the exclude file and fill
> >
> > Makes sense. This changed as part of 4b33e60201 (dir: convert struct
> > sha1_stat to use object_id, 2018-01-28). Perhaps it would likewise make
> > sense to stop saying "SHA-1" here, and just say "hash" (or even "object
> > id", though TBH I think the fact that the hash is the same as an
> > object-id is largely an implementation detail).
> 
> I do not quite get your "though TBH", though.  I do agree with you
> that it is an implementation detail that an object is named after
> the hash of its contents, so saying "compute object name" probably
> makes sense in more context than "compute hash" outside the narrow
> parts of the code that actually implements how object names are
> computed.  So I would have expected "because TBH", not "though TBH".

Sorry, I was just confused.

The implementation detail I meant is that we are using a "struct
object_id" in the oid_stat type (and also that "oid_stat" is likewise
exposing too much). I thought this was another version of our
stat_validity, where we are checking quickly to see if a random file
that is not necessarily part of the working tree has been updated.

And indeed, we do use it that way for files like .git/info/exclude,
where having an "object_id" is really irrelevant. But we also use it for
checking the validity of tracked files, where we populate it from an
actual blob (see dir.c:do_read_blob).

So it is actually reasonable to expose that in the name, and to think
about it as an object_id.

> Anyway.  Nipunn, can you fix both of them in the same commit, as
> they are addressing a problem from the same cause (i.e. we are no
> longer SHA-1 centric).

The v2 that Nipunn sent with "oid" in the comment looks good to me.

-Peff

[gitgitgadget]

On the Git mailing list, Junio C Hamano wrote (reply to this):

Jeff King <peff@peff.net> writes:

>> Anyway.  Nipunn, can you fix both of them in the same commit, as
>> they are addressing a problem from the same cause (i.e. we are no
>> longer SHA-1 centric).
>
> The v2 that Nipunn sent with "oid" in the comment looks good to me.

OK, then all is good.  Thanks.

[gitgitgadget]

On the Git mailing list, Nipunn Koorapati wrote (reply to this):

Sounds good to me. Please let me know if there's anything further I need to do.
I'm relatively new to git contributions - and my understanding is that
at this point a maintainer would merge the commit at their leisure.
Happy to do any further cleanup required to make that possible

Thanks
--Nipunn

On Thu, Oct 15, 2020 at 9:23 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Jeff King <peff@peff.net> writes:
>
> >> Anyway.  Nipunn, can you fix both of them in the same commit, as
> >> they are addressing a problem from the same cause (i.e. we are no
> >> longer SHA-1 centric).
> >
> > The v2 that Nipunn sent with "oid" in the comment looks good to me.
>
> OK, then all is good.  Thanks.

[gitgitgadget]

This branch is now known as nk/dir-c-comment-update.

[gitgitgadget]

This patch series was integrated into seen via git@543a35d.

[gitgitgadget]

This patch series was integrated into seen via git@71751eb.

[gitgitgadget]

This patch series was integrated into seen via git@0546f9e.

[gitgitgadget]

This patch series was integrated into seen via git@2ce5306.

[gitgitgadget]

This patch series was integrated into seen via git@dd7930f.

[gitgitgadget]

This patch series was integrated into seen via git@aeb9b97.

[gitgitgadget]

This patch series was integrated into seen via git@a5cc593.

[gitgitgadget]

This patch series was integrated into seen via git@d0f2553.

[gitgitgadget]

This patch series was integrated into seen via git@21ebc17.

[gitgitgadget]

This patch series was integrated into seen via git@0f648c0.

[gitgitgadget]

This patch series was integrated into seen via git@bef23da.

[gitgitgadget]

This patch series was integrated into next via git@3a4cb82.

[gitgitgadget]

This patch series was integrated into seen via git@f8c203a.

[gitgitgadget]

This patch series was integrated into seen via git@c7a8adf.

[gitgitgadget]

This patch series was integrated into seen via git@543b267.

[gitgitgadget]

This patch series was integrated into seen via git@3578d15.

[gitgitgadget]

This patch series was integrated into seen via git@1643f0d.

[gitgitgadget]

This patch series was integrated into seen via git@03cd25e.

[gitgitgadget]

This patch series was integrated into next via git@03cd25e.

[gitgitgadget]

This patch series was integrated into master via git@03cd25e.

[gitgitgadget]

Closed via 03cd25e.