[PATCH 0/4] start documenting core functions

[PATCH 0/4] start documenting core functions

[Stefan Beller]

The two single patches[1] are turned into a series here.

[1] https://public-inbox.org/git/20170117200147.25425-1-sbeller@google.com/

Thanks,
Stefan

Stefan Beller (4):
  document index_name_pos
  remove_index_entry_at: move documentation to cache.h
  document add_[file_]to_index
  documentation: retire unfinished documentation

 Documentation/technical/api-in-core-index.txt | 21 ----------------
 cache.h                                       | 35 +++++++++++++++++++++++----
 read-cache.c                                  |  1 -
 3 files changed, 30 insertions(+), 27 deletions(-)
 delete mode 100644 Documentation/technical/api-in-core-index.txt

-- 
2.11.0.299.g762782ba8a

[PATCH 1/4] document index_name_pos

[Stefan Beller]

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 cache.h | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/cache.h b/cache.h
index 1b67f078dd..270a0d0ea7 100644
--- a/cache.h
+++ b/cache.h
@@ -575,7 +575,22 @@ extern int verify_path(const char *path);
 extern int index_dir_exists(struct index_state *istate, const char *name, int namelen);
 extern void adjust_dirname_case(struct index_state *istate, char *name);
 extern struct cache_entry *index_file_exists(struct index_state *istate, const char *name, int namelen, int igncase);
+
+/*
+ * Searches for an entry defined by name and namelen in the given index.
+ * If the return value is positive (including 0) it is the position of an
+ * exact match. If the return value is negative, the negated value minus 1 is the
+ * position where the entry would be inserted.
+ * Example: In the current index we have the files b,d,e:
+ * index_name_pos(&index, "a", 1) -> -1
+ * index_name_pos(&index, "b", 1) ->  0
+ * index_name_pos(&index, "c", 1) -> -2
+ * index_name_pos(&index, "d", 1) ->  1
+ * index_name_pos(&index, "e", 1) ->  2
+ * index_name_pos(&index, "f", 1) -> -3
+ */
 extern int index_name_pos(const struct index_state *, const char *name, int namelen);
+
 #define ADD_CACHE_OK_TO_ADD 1		/* Ok to add */
 #define ADD_CACHE_OK_TO_REPLACE 2	/* Ok to replace file/directory */
 #define ADD_CACHE_SKIP_DFCHECK 4	/* Ok to skip DF conflict checks */
-- 
2.11.0.299.g762782ba8a

Re: [PATCH 1/4] document index_name_pos

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>  cache.h | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
>
> diff --git a/cache.h b/cache.h
> index 1b67f078dd..270a0d0ea7 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -575,7 +575,22 @@ extern int verify_path(const char *path);
>  extern int index_dir_exists(struct index_state *istate, const char *name, int namelen);
>  extern void adjust_dirname_case(struct index_state *istate, char *name);
>  extern struct cache_entry *index_file_exists(struct index_state *istate, const char *name, int namelen, int igncase);
> +
> +/*
> + * Searches for an entry defined by name and namelen in the given index.
> + * If the return value is positive (including 0) it is the position of an
> + * exact match. If the return value is negative, the negated value minus 1 is the
> + * position where the entry would be inserted.
> + * Example: In the current index we have the files b,d,e:
> + * index_name_pos(&index, "a", 1) -> -1
> + * index_name_pos(&index, "b", 1) ->  0
> + * index_name_pos(&index, "c", 1) -> -2
> + * index_name_pos(&index, "d", 1) ->  1
> + * index_name_pos(&index, "e", 1) ->  2

The above may not be wrong per-se, but it misses one important case.
A conflicted entry in the index with the same name is considered to
sort after the name this asks.  If there are stage #1 and stage #3
entries for path "g" in addition to the above, i.e.

	[0] [1] [2] [3] [4]
	b#0 d#0 e#0 g#1 g#3

then 

	index_name_pos(&index, "g", 1) -> -3 - 1 = -4
        index_name_pos(&index, "h", 1) -> -5 - 1 = -6

> + * index_name_pos(&index, "f", 1) -> -3
> + */

Shouldn't this be -4?  We originally have [0], [1], and [2] in the
index, and "f" needs to go to [3], so -3 - 1 = -4, no?

>  extern int index_name_pos(const struct index_state *, const char *name, int namelen);
> +
>  #define ADD_CACHE_OK_TO_ADD 1		/* Ok to add */
>  #define ADD_CACHE_OK_TO_REPLACE 2	/* Ok to replace file/directory */
>  #define ADD_CACHE_SKIP_DFCHECK 4	/* Ok to skip DF conflict checks */

Re: [PATCH 1/4] document index_name_pos

[Stefan Beller]

On Wed, Jan 18, 2017 at 1:11 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> Signed-off-by: Stefan Beller <sbeller@google.com>
>> ---
>>  cache.h | 15 +++++++++++++++
>>  1 file changed, 15 insertions(+)
>>
>> diff --git a/cache.h b/cache.h
>> index 1b67f078dd..270a0d0ea7 100644
>> --- a/cache.h
>> +++ b/cache.h
>> @@ -575,7 +575,22 @@ extern int verify_path(const char *path);
>>  extern int index_dir_exists(struct index_state *istate, const char *name, int namelen);
>>  extern void adjust_dirname_case(struct index_state *istate, char *name);
>>  extern struct cache_entry *index_file_exists(struct index_state *istate, const char *name, int namelen, int igncase);
>> +
>> +/*
>> + * Searches for an entry defined by name and namelen in the given index.
>> + * If the return value is positive (including 0) it is the position of an
>> + * exact match. If the return value is negative, the negated value minus 1 is the
>> + * position where the entry would be inserted.
>> + * Example: In the current index we have the files b,d,e:
>> + * index_name_pos(&index, "a", 1) -> -1
>> + * index_name_pos(&index, "b", 1) ->  0
>> + * index_name_pos(&index, "c", 1) -> -2
>> + * index_name_pos(&index, "d", 1) ->  1
>> + * index_name_pos(&index, "e", 1) ->  2
>
> The above may not be wrong per-se, but it misses one important case.
> A conflicted entry in the index with the same name is considered to
> sort after the name this asks.  If there are stage #1 and stage #3
> entries for path "g" in addition to the above, i.e.
>
>         [0] [1] [2] [3] [4]
>         b#0 d#0 e#0 g#1 g#3
>
> then
>
>         index_name_pos(&index, "g", 1) -> -3 - 1 = -4
>         index_name_pos(&index, "h", 1) -> -5 - 1 = -6
>
>> + * index_name_pos(&index, "f", 1) -> -3
>> + */

Oh, I see. With this property in mind, we know that
when using index_name_pos for sorting, the stages for a
given path are ordered correctly (in ascending order,
0 comes before 1, which comes before 3).

>
> Shouldn't this be -4?  We originally have [0], [1], and [2] in the
> index, and "f" needs to go to [3], so -3 - 1 = -4, no?

yes, it should be -4.

[PATCH 2/4] remove_index_entry_at: move documentation to cache.h

[Stefan Beller]

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 cache.h      | 3 +++
 read-cache.c | 1 -
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/cache.h b/cache.h
index 270a0d0ea7..26632065a5 100644
--- a/cache.h
+++ b/cache.h
@@ -599,7 +599,10 @@ extern int index_name_pos(const struct index_state *, const char *name, int name
 #define ADD_CACHE_KEEP_CACHE_TREE 32	/* Do not invalidate cache-tree */
 extern int add_index_entry(struct index_state *, struct cache_entry *ce, int option);
 extern void rename_index_entry_at(struct index_state *, int pos, const char *new_name);
+
+/* Remove entry, return 1 if there are more entries after pos. */
 extern int remove_index_entry_at(struct index_state *, int pos);
+
 extern void remove_marked_cache_entries(struct index_state *istate);
 extern int remove_file_from_index(struct index_state *, const char *path);
 #define ADD_CACHE_VERBOSE 1
diff --git a/read-cache.c b/read-cache.c
index 2eca639cce..63a414cdb5 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -503,7 +503,6 @@ int index_name_pos(const struct index_state *istate, const char *name, int namel
 	return index_name_stage_pos(istate, name, namelen, 0);
 }
 
-/* Remove entry, return true if there are more entries to go.. */
 int remove_index_entry_at(struct index_state *istate, int pos)
 {
 	struct cache_entry *ce = istate->cache[pos];
-- 
2.11.0.299.g762782ba8a

Re: [PATCH 2/4] remove_index_entry_at: move documentation to cache.h

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>  cache.h      | 3 +++
>  read-cache.c | 1 -
>  2 files changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/cache.h b/cache.h
> index 270a0d0ea7..26632065a5 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -599,7 +599,10 @@ extern int index_name_pos(const struct index_state *, const char *name, int name
>  #define ADD_CACHE_KEEP_CACHE_TREE 32	/* Do not invalidate cache-tree */
>  extern int add_index_entry(struct index_state *, struct cache_entry *ce, int option);
>  extern void rename_index_entry_at(struct index_state *, int pos, const char *new_name);
> +
> +/* Remove entry, return 1 if there are more entries after pos. */
>  extern int remove_index_entry_at(struct index_state *, int pos);

What is the reason why this now promise to return 1, as opposed to
the original that were allowed to return anything that is "true"?
Is it because you are adding other return values that mean different
things?  

If that is the case it may be fine (it depends on what these other
values mean and what use case it supports), but please do that in a
separate patch.

> +
>  extern void remove_marked_cache_entries(struct index_state *istate);
>  extern int remove_file_from_index(struct index_state *, const char *path);
>  #define ADD_CACHE_VERBOSE 1
> diff --git a/read-cache.c b/read-cache.c
> index 2eca639cce..63a414cdb5 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -503,7 +503,6 @@ int index_name_pos(const struct index_state *istate, const char *name, int namel
>  	return index_name_stage_pos(istate, name, namelen, 0);
>  }
>  
> -/* Remove entry, return true if there are more entries to go.. */
>  int remove_index_entry_at(struct index_state *istate, int pos)
>  {
>  	struct cache_entry *ce = istate->cache[pos];

Re: [PATCH 2/4] remove_index_entry_at: move documentation to cache.h

[Stefan Beller]

On Wed, Jan 18, 2017 at 1:16 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> Signed-off-by: Stefan Beller <sbeller@google.com>
>> ---
>>  cache.h      | 3 +++
>>  read-cache.c | 1 -
>>  2 files changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/cache.h b/cache.h
>> index 270a0d0ea7..26632065a5 100644
>> --- a/cache.h
>> +++ b/cache.h
>> @@ -599,7 +599,10 @@ extern int index_name_pos(const struct index_state *, const char *name, int name
>>  #define ADD_CACHE_KEEP_CACHE_TREE 32 /* Do not invalidate cache-tree */
>>  extern int add_index_entry(struct index_state *, struct cache_entry *ce, int option);
>>  extern void rename_index_entry_at(struct index_state *, int pos, const char *new_name);
>> +
>> +/* Remove entry, return 1 if there are more entries after pos. */
>>  extern int remove_index_entry_at(struct index_state *, int pos);
>
> What is the reason why this now promise to return 1, as opposed to
> the original that were allowed to return anything that is "true"?
> Is it because you are adding other return values that mean different
> things?
>
> If that is the case it may be fine (it depends on what these other
> values mean and what use case it supports), but please do that in a
> separate patch.
>

Actually my line of thinking was to improve the correctness by being more
specific.

In a reroll I move the comment verbatim.

[PATCH 3/4] document add_[file_]to_index

[Stefan Beller]

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 cache.h | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/cache.h b/cache.h
index 26632065a5..acc639d6e0 100644
--- a/cache.h
+++ b/cache.h
@@ -605,13 +605,20 @@ extern int remove_index_entry_at(struct index_state *, int pos);
 
 extern void remove_marked_cache_entries(struct index_state *istate);
 extern int remove_file_from_index(struct index_state *, const char *path);
-#define ADD_CACHE_VERBOSE 1
-#define ADD_CACHE_PRETEND 2
-#define ADD_CACHE_IGNORE_ERRORS	4
-#define ADD_CACHE_IGNORE_REMOVAL 8
-#define ADD_CACHE_INTENT 16
+
+#define ADD_CACHE_VERBOSE 1		/* verbose */
+#define ADD_CACHE_PRETEND 2 		/* dry run */
+#define ADD_CACHE_IGNORE_ERRORS 4	/* ignore errors */
+#define ADD_CACHE_IGNORE_REMOVAL 8	/* do not remove files from index */
+#define ADD_CACHE_INTENT 16		/* intend to add later; stage empty file */
+/*
+ * Adds the given path the index, respecting the repsitory configuration, e.g.
+ * in case insensitive file systems, the path is normalized.
+ */
 extern int add_to_index(struct index_state *, const char *path, struct stat *, int flags);
+/* stat the file then call add_to_index */
 extern int add_file_to_index(struct index_state *, const char *path, int flags);
+
 extern struct cache_entry *make_cache_entry(unsigned int mode, const unsigned char *sha1, const char *path, int stage, unsigned int refresh_options);
 extern int chmod_index_entry(struct index_state *, struct cache_entry *ce, char flip);
 extern int ce_same_name(const struct cache_entry *a, const struct cache_entry *b);
-- 
2.11.0.299.g762782ba8a

Re: [PATCH 3/4] document add_[file_]to_index

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>  cache.h | 17 ++++++++++++-----
>  1 file changed, 12 insertions(+), 5 deletions(-)
>
> diff --git a/cache.h b/cache.h
> index 26632065a5..acc639d6e0 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -605,13 +605,20 @@ extern int remove_index_entry_at(struct index_state *, int pos);
>  
>  extern void remove_marked_cache_entries(struct index_state *istate);
>  extern int remove_file_from_index(struct index_state *, const char *path);
> -#define ADD_CACHE_VERBOSE 1
> -#define ADD_CACHE_PRETEND 2
> -#define ADD_CACHE_IGNORE_ERRORS	4
> -#define ADD_CACHE_IGNORE_REMOVAL 8
> -#define ADD_CACHE_INTENT 16
> +
> +#define ADD_CACHE_VERBOSE 1		/* verbose */
> +#define ADD_CACHE_PRETEND 2 		/* dry run */
> +#define ADD_CACHE_IGNORE_ERRORS 4	/* ignore errors */
> +#define ADD_CACHE_IGNORE_REMOVAL 8	/* do not remove files from index */
> +#define ADD_CACHE_INTENT 16		/* intend to add later; stage empty file */

These repeat pretty much the same thing, which is an indication that
the macro names are chosen well not to require extraneous comments
like these, no?

> +/*
> + * Adds the given path the index, respecting the repsitory configuration, e.g.
> + * in case insensitive file systems, the path is normalized.
> + */
>  extern int add_to_index(struct index_state *, const char *path, struct stat *, int flags);

s/repsitory/repository/;

> +/* stat the file then call add_to_index */
>  extern int add_file_to_index(struct index_state *, const char *path, int flags);
> +

As you do not say "use the provided stat info to mark the cache
entry up-to-date" in the add_to_index(), I am not sure if mentioning
"stat the file then" has much value.  Besides, you are supposed to
lstat(2) the file, not "stat", no?

I'd cover these two under the same heading and comment if I were
doing this.

	These two are used to add the contents of the file at path
	to the index, marking the working tree up-to-date by storing
	the cached stat info in the resulting cache entry.  A caller
	that has already run lstat(2) on the path can call
	add_to_index(), and all others can call add_file_to_index();
	the latter will do necessary lstat(2) internally before
	calling the former.

or something along that line.

>  extern struct cache_entry *make_cache_entry(unsigned int mode, const unsigned char *sha1, const char *path, int stage, unsigned int refresh_options);
>  extern int chmod_index_entry(struct index_state *, struct cache_entry *ce, char flip);
>  extern int ce_same_name(const struct cache_entry *a, const struct cache_entry *b);

Re: [PATCH 3/4] document add_[file_]to_index

[Stefan Beller]

On Wed, Jan 18, 2017 at 1:22 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> Signed-off-by: Stefan Beller <sbeller@google.com>
>> ---
>>  cache.h | 17 ++++++++++++-----
>>  1 file changed, 12 insertions(+), 5 deletions(-)
>>
>> diff --git a/cache.h b/cache.h
>> index 26632065a5..acc639d6e0 100644
>> --- a/cache.h
>> +++ b/cache.h
>> @@ -605,13 +605,20 @@ extern int remove_index_entry_at(struct index_state *, int pos);
>>
>>  extern void remove_marked_cache_entries(struct index_state *istate);
>>  extern int remove_file_from_index(struct index_state *, const char *path);
>> -#define ADD_CACHE_VERBOSE 1
>> -#define ADD_CACHE_PRETEND 2
>> -#define ADD_CACHE_IGNORE_ERRORS      4
>> -#define ADD_CACHE_IGNORE_REMOVAL 8
>> -#define ADD_CACHE_INTENT 16
>> +
>> +#define ADD_CACHE_VERBOSE 1          /* verbose */
>> +#define ADD_CACHE_PRETEND 2          /* dry run */
>> +#define ADD_CACHE_IGNORE_ERRORS 4    /* ignore errors */
>> +#define ADD_CACHE_IGNORE_REMOVAL 8   /* do not remove files from index */
>> +#define ADD_CACHE_INTENT 16          /* intend to add later; stage empty file */
>
> These repeat pretty much the same thing, which is an indication that
> the macro names are chosen well not to require extraneous comments
> like these, no?

Well I got confused for pretend and intent, so I researched them;
I did not want to comment only half the constants.


>
>> +/*
>> + * Adds the given path the index, respecting the repsitory configuration, e.g.
>> + * in case insensitive file systems, the path is normalized.
>> + */
>>  extern int add_to_index(struct index_state *, const char *path, struct stat *, int flags);
>
> s/repsitory/repository/;
>
>> +/* stat the file then call add_to_index */
>>  extern int add_file_to_index(struct index_state *, const char *path, int flags);
>> +
>
> As you do not say "use the provided stat info to mark the cache
> entry up-to-date" in the add_to_index(), I am not sure if mentioning
> "stat the file then" has much value.  Besides, you are supposed to
> lstat(2) the file, not "stat", no?
>
> I'd cover these two under the same heading and comment if I were
> doing this.
>
>         These two are used to add the contents of the file at path
>         to the index, marking the working tree up-to-date by storing
>         the cached stat info in the resulting cache entry.  A caller
>         that has already run lstat(2) on the path can call
>         add_to_index(), and all others can call add_file_to_index();
>         the latter will do necessary lstat(2) internally before
>         calling the former.
>
> or something along that line.

That sounds better than what I had.

Thanks,
Stefan

>
>>  extern struct cache_entry *make_cache_entry(unsigned int mode, const unsigned char *sha1, const char *path, int stage, unsigned int refresh_options);
>>  extern int chmod_index_entry(struct index_state *, struct cache_entry *ce, char flip);
>>  extern int ce_same_name(const struct cache_entry *a, const struct cache_entry *b);

[PATCH 4/4] documentation: retire unfinished documentation

[Stefan Beller]

When looking for documentation for a specific function, you may be tempted
to run

  git -C Documentation grep index_name_pos

only to find the file technical/api-in-core-index.txt, which doesn't
help for understanding the given function. It would be better to not find
these functions in the documentation, such that people directly dive into
the code instead.

In the previous patches we have documented
* index_name_pos()
* remove_index_entry_at()
* add_[file_]to_index()
in cache.h

We already have documentation for:
* add_index_entry()
* read_index()

Which leaves us with a TODO for:
* cache -> the_index macros
* refresh_index()
* discard_index()
* ie_match_stat() and ie_modified(); how they are different and when to
  use which.
* write_index() that was renamed to write_locked_index
* cache_tree_invalidate_path()
* cache_tree_update()

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 Documentation/technical/api-in-core-index.txt | 21 ---------------------
 1 file changed, 21 deletions(-)
 delete mode 100644 Documentation/technical/api-in-core-index.txt

diff --git a/Documentation/technical/api-in-core-index.txt b/Documentation/technical/api-in-core-index.txt
deleted file mode 100644
index adbdbf5d75..0000000000
--- a/Documentation/technical/api-in-core-index.txt
+++ /dev/null
@@ -1,21 +0,0 @@
-in-core index API
-=================
-
-Talk about <read-cache.c> and <cache-tree.c>, things like:
-
-* cache -> the_index macros
-* read_index()
-* write_index()
-* ie_match_stat() and ie_modified(); how they are different and when to
-  use which.
-* index_name_pos()
-* remove_index_entry_at()
-* remove_file_from_index()
-* add_file_to_index()
-* add_index_entry()
-* refresh_index()
-* discard_index()
-* cache_tree_invalidate_path()
-* cache_tree_update()
-
-(JC, Linus)
-- 
2.11.0.299.g762782ba8a

Re: [PATCH 4/4] documentation: retire unfinished documentation

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> When looking for documentation for a specific function, you may be tempted
> to run
>
>   git -C Documentation grep index_name_pos
>
> only to find the file technical/api-in-core-index.txt, which doesn't
> help for understanding the given function. It would be better to not find
> these functions in the documentation, such that people directly dive into
> the code instead.
>
> In the previous patches we have documented
> * index_name_pos()
> * remove_index_entry_at()
> * add_[file_]to_index()
> in cache.h
>
> We already have documentation for:
> * add_index_entry()
> * read_index()
>
> Which leaves us with a TODO for:
> * cache -> the_index macros
> * refresh_index()
> * discard_index()
> * ie_match_stat() and ie_modified(); how they are different and when to
>   use which.
> * write_index() that was renamed to write_locked_index
> * cache_tree_invalidate_path()
> * cache_tree_update()

Thanks.

[PATCHv2 0/4] start documenting core functions

[Stefan Beller]

v2:
included all suggestions from Junio

v1:
The two single patches[1] are turned into a series here.

[1] https://public-inbox.org/git/20170117200147.25425-1-sbeller@google.com/

Thanks,
Stefan

Stefan Beller (4):
  cache.h: document index_name_pos
  cache.h: document remove_index_entry_at
  cache.h: document add_[file_]to_index
  documentation: retire unfinished documentation

 Documentation/technical/api-in-core-index.txt | 21 -------------
 cache.h                                       | 43 +++++++++++++++++++++++----
 read-cache.c                                  |  1 -
 3 files changed, 38 insertions(+), 27 deletions(-)
 delete mode 100644 Documentation/technical/api-in-core-index.txt

-- 
2.11.0.299.g762782ba8a

[PATCHv2 1/4] cache.h: document index_name_pos

[Stefan Beller]

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 cache.h | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/cache.h b/cache.h
index 1b67f078dd..3dbba69aec 100644
--- a/cache.h
+++ b/cache.h
@@ -575,7 +575,26 @@ extern int verify_path(const char *path);
 extern int index_dir_exists(struct index_state *istate, const char *name, int namelen);
 extern void adjust_dirname_case(struct index_state *istate, char *name);
 extern struct cache_entry *index_file_exists(struct index_state *istate, const char *name, int namelen, int igncase);
+
+/*
+ * Searches for an entry defined by name and namelen in the given index.
+ * If the return value is positive (including 0) it is the position of an
+ * exact match. If the return value is negative, the negated value minus 1
+ * is the position where the entry would be inserted.
+ * Example: The current index consists of these files and its stages:
+ *
+ *   b#0, d#0, f#1, f#3
+ *
+ * index_name_pos(&index, "a", 1) -> -1
+ * index_name_pos(&index, "b", 1) ->  0
+ * index_name_pos(&index, "c", 1) -> -2
+ * index_name_pos(&index, "d", 1) ->  1
+ * index_name_pos(&index, "e", 1) -> -3
+ * index_name_pos(&index, "f", 1) ->  2
+ * index_name_pos(&index, "g", 1) -> -5
+ */
 extern int index_name_pos(const struct index_state *, const char *name, int namelen);
+
 #define ADD_CACHE_OK_TO_ADD 1		/* Ok to add */
 #define ADD_CACHE_OK_TO_REPLACE 2	/* Ok to replace file/directory */
 #define ADD_CACHE_SKIP_DFCHECK 4	/* Ok to skip DF conflict checks */
-- 
2.11.0.299.g762782ba8a

[PATCHv3 0/4] start documenting core functions

[Stefan Beller]

v3:
* dropped commets on constants as they were not helpful
* fixed one result in the example for  index_name_pos(&index, "f", 1) (which
  is -3 now)

v2:
included all suggestions from Junio

v1:
The two single patches[1] are turned into a series here.

[1] https://public-inbox.org/git/20170117200147.25425-1-sbeller@google.com/

Thanks,
Stefan

Stefan Beller (4):
  cache.h: document index_name_pos
  cache.h: document remove_index_entry_at
  cache.h: document add_[file_]to_index
  documentation: retire unfinished documentation

 Documentation/technical/api-in-core-index.txt | 21 -------------
 cache.h                                       | 43 +++++++++++++++++++++++----
 read-cache.c                                  |  1 -
 3 files changed, 38 insertions(+), 27 deletions(-)
 delete mode 100644 Documentation/technical/api-in-core-index.txt

-- 
2.11.0.299.g762782ba8a

[PATCHv3 1/4] cache.h: document index_name_pos

[Stefan Beller]

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 cache.h | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/cache.h b/cache.h
index 1b67f078dd..1469ddeafe 100644
--- a/cache.h
+++ b/cache.h
@@ -575,7 +575,26 @@ extern int verify_path(const char *path);
 extern int index_dir_exists(struct index_state *istate, const char *name, int namelen);
 extern void adjust_dirname_case(struct index_state *istate, char *name);
 extern struct cache_entry *index_file_exists(struct index_state *istate, const char *name, int namelen, int igncase);
+
+/*
+ * Searches for an entry defined by name and namelen in the given index.
+ * If the return value is positive (including 0) it is the position of an
+ * exact match. If the return value is negative, the negated value minus 1
+ * is the position where the entry would be inserted.
+ * Example: The current index consists of these files and its stages:
+ *
+ *   b#0, d#0, f#1, f#3
+ *
+ * index_name_pos(&index, "a", 1) -> -1
+ * index_name_pos(&index, "b", 1) ->  0
+ * index_name_pos(&index, "c", 1) -> -2
+ * index_name_pos(&index, "d", 1) ->  1
+ * index_name_pos(&index, "e", 1) -> -3
+ * index_name_pos(&index, "f", 1) -> -3
+ * index_name_pos(&index, "g", 1) -> -5
+ */
 extern int index_name_pos(const struct index_state *, const char *name, int namelen);
+
 #define ADD_CACHE_OK_TO_ADD 1		/* Ok to add */
 #define ADD_CACHE_OK_TO_REPLACE 2	/* Ok to replace file/directory */
 #define ADD_CACHE_SKIP_DFCHECK 4	/* Ok to skip DF conflict checks */
-- 
2.11.0.299.g762782ba8a

Re: [PATCHv3 1/4] cache.h: document index_name_pos

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>  cache.h | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
>
> diff --git a/cache.h b/cache.h
> index 1b67f078dd..1469ddeafe 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -575,7 +575,26 @@ extern int verify_path(const char *path);
>  extern int index_dir_exists(struct index_state *istate, const char *name, int namelen);
>  extern void adjust_dirname_case(struct index_state *istate, char *name);
>  extern struct cache_entry *index_file_exists(struct index_state *istate, const char *name, int namelen, int igncase);
> +
> +/*
> + * Searches for an entry defined by name and namelen in the given index.
> + * If the return value is positive (including 0) it is the position of an
> + * exact match. If the return value is negative, the negated value minus 1
> + * is the position where the entry would be inserted.

So if the return value is -3, you negate it to get 3 and then
subtract 1 to get 2.  The function is telling you that "e" will
sit at active_cache[2] in the following example.

Which is correct.

> + * Example: The current index consists of these files and its stages:
> + *
> + *   b#0, d#0, f#1, f#3
> + *
> + * index_name_pos(&index, "a", 1) -> -1
> + * index_name_pos(&index, "b", 1) ->  0
> + * index_name_pos(&index, "c", 1) -> -2
> + * index_name_pos(&index, "d", 1) ->  1
> + * index_name_pos(&index, "e", 1) -> -3
> + * index_name_pos(&index, "f", 1) -> -3
> + * index_name_pos(&index, "g", 1) -> -5
> + */

This time the counting seems correct.

Thanks.

[PATCHv3 2/4] cache.h: document remove_index_entry_at

[Stefan Beller]

Do this by moving the existing documentation from
read-cache.c to cache.h.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 cache.h      | 3 +++
 read-cache.c | 1 -
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/cache.h b/cache.h
index 1469ddeafe..929474d7a9 100644
--- a/cache.h
+++ b/cache.h
@@ -603,7 +603,10 @@ extern int index_name_pos(const struct index_state *, const char *name, int name
 #define ADD_CACHE_KEEP_CACHE_TREE 32	/* Do not invalidate cache-tree */
 extern int add_index_entry(struct index_state *, struct cache_entry *ce, int option);
 extern void rename_index_entry_at(struct index_state *, int pos, const char *new_name);
+
+/* Remove entry, return true if there are more entries to go. */
 extern int remove_index_entry_at(struct index_state *, int pos);
+
 extern void remove_marked_cache_entries(struct index_state *istate);
 extern int remove_file_from_index(struct index_state *, const char *path);
 #define ADD_CACHE_VERBOSE 1
diff --git a/read-cache.c b/read-cache.c
index 2eca639cce..63a414cdb5 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -503,7 +503,6 @@ int index_name_pos(const struct index_state *istate, const char *name, int namel
 	return index_name_stage_pos(istate, name, namelen, 0);
 }
 
-/* Remove entry, return true if there are more entries to go.. */
 int remove_index_entry_at(struct index_state *istate, int pos)
 {
 	struct cache_entry *ce = istate->cache[pos];
-- 
2.11.0.299.g762782ba8a

[PATCHv3 3/4] cache.h: document add_[file_]to_index

[Stefan Beller]

Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
---
 cache.h | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/cache.h b/cache.h
index 929474d7a9..12394eb541 100644
--- a/cache.h
+++ b/cache.h
@@ -614,8 +614,18 @@ extern int remove_file_from_index(struct index_state *, const char *path);
 #define ADD_CACHE_IGNORE_ERRORS	4
 #define ADD_CACHE_IGNORE_REMOVAL 8
 #define ADD_CACHE_INTENT 16
+/*
+ * These two are used to add the contents of the file at path
+ * to the index, marking the working tree up-to-date by storing
+ * the cached stat info in the resulting cache entry.  A caller
+ * that has already run lstat(2) on the path can call
+ * add_to_index(), and all others can call add_file_to_index();
+ * the latter will do necessary lstat(2) internally before
+ * calling the former.
+ */
 extern int add_to_index(struct index_state *, const char *path, struct stat *, int flags);
 extern int add_file_to_index(struct index_state *, const char *path, int flags);
+
 extern struct cache_entry *make_cache_entry(unsigned int mode, const unsigned char *sha1, const char *path, int stage, unsigned int refresh_options);
 extern int chmod_index_entry(struct index_state *, struct cache_entry *ce, char flip);
 extern int ce_same_name(const struct cache_entry *a, const struct cache_entry *b);
-- 
2.11.0.299.g762782ba8a

[PATCHv3 4/4] documentation: retire unfinished documentation

[Stefan Beller]

When looking for documentation for a specific function, you may be tempted
to run

  git -C Documentation grep index_name_pos

only to find the file technical/api-in-core-index.txt, which doesn't
help for understanding the given function. It would be better to not find
these functions in the documentation, such that people directly dive into
the code instead.

In the previous patches we have documented
* index_name_pos()
* remove_index_entry_at()
* add_[file_]to_index()
in cache.h

We already have documentation for:
* add_index_entry()
* read_index()

Which leaves us with a TODO for:
* cache -> the_index macros
* refresh_index()
* discard_index()
* ie_match_stat() and ie_modified(); how they are different and when to
  use which.
* write_index() that was renamed to write_locked_index
* cache_tree_invalidate_path()
* cache_tree_update()

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 Documentation/technical/api-in-core-index.txt | 21 ---------------------
 1 file changed, 21 deletions(-)
 delete mode 100644 Documentation/technical/api-in-core-index.txt

diff --git a/Documentation/technical/api-in-core-index.txt b/Documentation/technical/api-in-core-index.txt
deleted file mode 100644
index adbdbf5d75..0000000000
--- a/Documentation/technical/api-in-core-index.txt
+++ /dev/null
@@ -1,21 +0,0 @@
-in-core index API
-=================
-
-Talk about <read-cache.c> and <cache-tree.c>, things like:
-
-* cache -> the_index macros
-* read_index()
-* write_index()
-* ie_match_stat() and ie_modified(); how they are different and when to
-  use which.
-* index_name_pos()
-* remove_index_entry_at()
-* remove_file_from_index()
-* add_file_to_index()
-* add_index_entry()
-* refresh_index()
-* discard_index()
-* cache_tree_invalidate_path()
-* cache_tree_update()
-
-(JC, Linus)
-- 
2.11.0.299.g762782ba8a

[PATCHv2 2/4] cache.h: document remove_index_entry_at

[Stefan Beller]

Do this by moving the existing documentation from
read-cache.c to cache.h.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 cache.h      | 3 +++
 read-cache.c | 1 -
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/cache.h b/cache.h
index 3dbba69aec..87eccdb211 100644
--- a/cache.h
+++ b/cache.h
@@ -603,7 +603,10 @@ extern int index_name_pos(const struct index_state *, const char *name, int name
 #define ADD_CACHE_KEEP_CACHE_TREE 32	/* Do not invalidate cache-tree */
 extern int add_index_entry(struct index_state *, struct cache_entry *ce, int option);
 extern void rename_index_entry_at(struct index_state *, int pos, const char *new_name);
+
+/* Remove entry, return true if there are more entries to go. */
 extern int remove_index_entry_at(struct index_state *, int pos);
+
 extern void remove_marked_cache_entries(struct index_state *istate);
 extern int remove_file_from_index(struct index_state *, const char *path);
 #define ADD_CACHE_VERBOSE 1
diff --git a/read-cache.c b/read-cache.c
index 2eca639cce..63a414cdb5 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -503,7 +503,6 @@ int index_name_pos(const struct index_state *istate, const char *name, int namel
 	return index_name_stage_pos(istate, name, namelen, 0);
 }
 
-/* Remove entry, return true if there are more entries to go.. */
 int remove_index_entry_at(struct index_state *istate, int pos)
 {
 	struct cache_entry *ce = istate->cache[pos];
-- 
2.11.0.299.g762782ba8a

[PATCHv2 3/4] cache.h: document add_[file_]to_index

[Stefan Beller]

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 cache.h | 21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)

diff --git a/cache.h b/cache.h
index 87eccdb211..03c46b9b99 100644
--- a/cache.h
+++ b/cache.h
@@ -609,13 +609,24 @@ extern int remove_index_entry_at(struct index_state *, int pos);
 
 extern void remove_marked_cache_entries(struct index_state *istate);
 extern int remove_file_from_index(struct index_state *, const char *path);
-#define ADD_CACHE_VERBOSE 1
-#define ADD_CACHE_PRETEND 2
-#define ADD_CACHE_IGNORE_ERRORS	4
-#define ADD_CACHE_IGNORE_REMOVAL 8
-#define ADD_CACHE_INTENT 16
+
+#define ADD_CACHE_VERBOSE 1		/* verbose */
+#define ADD_CACHE_PRETEND 2 		/* dry run */
+#define ADD_CACHE_IGNORE_ERRORS 4	/* ignore errors */
+#define ADD_CACHE_IGNORE_REMOVAL 8	/* do not remove files from index */
+#define ADD_CACHE_INTENT 16		/* intend to add later; stage empty file */
+/*
+ * These two are used to add the contents of the file at path
+ * to the index, marking the working tree up-to-date by storing
+ * the cached stat info in the resulting cache entry.  A caller
+ * that has already run lstat(2) on the path can call
+ * add_to_index(), and all others can call add_file_to_index();
+ * the latter will do necessary lstat(2) internally before
+ * calling the former.
+ */
 extern int add_to_index(struct index_state *, const char *path, struct stat *, int flags);
 extern int add_file_to_index(struct index_state *, const char *path, int flags);
+
 extern struct cache_entry *make_cache_entry(unsigned int mode, const unsigned char *sha1, const char *path, int stage, unsigned int refresh_options);
 extern int chmod_index_entry(struct index_state *, struct cache_entry *ce, char flip);
 extern int ce_same_name(const struct cache_entry *a, const struct cache_entry *b);
-- 
2.11.0.299.g762782ba8a

Re: [PATCHv2 3/4] cache.h: document add_[file_]to_index

[Brandon Williams]

On 01/18, Stefan Beller wrote:
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>  cache.h | 21 ++++++++++++++++-----
>  1 file changed, 16 insertions(+), 5 deletions(-)
> 
> diff --git a/cache.h b/cache.h
> index 87eccdb211..03c46b9b99 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -609,13 +609,24 @@ extern int remove_index_entry_at(struct index_state *, int pos);
>  
>  extern void remove_marked_cache_entries(struct index_state *istate);
>  extern int remove_file_from_index(struct index_state *, const char *path);
> -#define ADD_CACHE_VERBOSE 1
> -#define ADD_CACHE_PRETEND 2
> -#define ADD_CACHE_IGNORE_ERRORS	4
> -#define ADD_CACHE_IGNORE_REMOVAL 8
> -#define ADD_CACHE_INTENT 16
> +
> +#define ADD_CACHE_VERBOSE 1		/* verbose */
> +#define ADD_CACHE_PRETEND 2 		/* dry run */
> +#define ADD_CACHE_IGNORE_ERRORS 4	/* ignore errors */
> +#define ADD_CACHE_IGNORE_REMOVAL 8	/* do not remove files from index */
> +#define ADD_CACHE_INTENT 16		/* intend to add later; stage empty file */

I usually prefer having defines like these use shift operators to set
the desired bit '(1<<2)' instead of '4', etc.  Is there a preference for
git as a whole?  I know this is just a documentation change so maybe
this isn't even the place to discuss this.

-- 
Brandon Williams

Re: [PATCHv2 3/4] cache.h: document add_[file_]to_index

[Stefan Beller]

On Wed, Jan 18, 2017 at 4:00 PM, Brandon Williams <bmwill@google.com> wrote:
> On 01/18, Stefan Beller wrote:
>> Signed-off-by: Stefan Beller <sbeller@google.com>
>> ---
>>  cache.h | 21 ++++++++++++++++-----
>>  1 file changed, 16 insertions(+), 5 deletions(-)
>>
>> diff --git a/cache.h b/cache.h
>> index 87eccdb211..03c46b9b99 100644
>> --- a/cache.h
>> +++ b/cache.h
>> @@ -609,13 +609,24 @@ extern int remove_index_entry_at(struct index_state *, int pos);
>>
>>  extern void remove_marked_cache_entries(struct index_state *istate);
>>  extern int remove_file_from_index(struct index_state *, const char *path);
>> -#define ADD_CACHE_VERBOSE 1
>> -#define ADD_CACHE_PRETEND 2
>> -#define ADD_CACHE_IGNORE_ERRORS      4
>> -#define ADD_CACHE_IGNORE_REMOVAL 8
>> -#define ADD_CACHE_INTENT 16
>> +
>> +#define ADD_CACHE_VERBOSE 1          /* verbose */
>> +#define ADD_CACHE_PRETEND 2          /* dry run */
>> +#define ADD_CACHE_IGNORE_ERRORS 4    /* ignore errors */
>> +#define ADD_CACHE_IGNORE_REMOVAL 8   /* do not remove files from index */
>> +#define ADD_CACHE_INTENT 16          /* intend to add later; stage empty file */
>
> I usually prefer having defines like these use shift operators to set
> the desired bit '(1<<2)' instead of '4', etc.  Is there a preference for
> git as a whole?  I know this is just a documentation change so maybe
> this isn't even the place to discuss this.

eh, and I forgot to remove the comments that Junio thought of as redundant.
I agree that (1<<N)) is usually better than the actual number. But I think
we do not want to change that for the same reason as we don't want to add
these comments there: Digging into history just got more complicated here.
("Who introduced ADD_CACHE_INTENT and why?" you need to skip the
reformatting/adding document patch to actually find the answer.)

Thanks for spotting,
Stefan

>
> --
> Brandon Williams

[PATCHv2 4/4] documentation: retire unfinished documentation

[Stefan Beller]

When looking for documentation for a specific function, you may be tempted
to run

  git -C Documentation grep index_name_pos

only to find the file technical/api-in-core-index.txt, which doesn't
help for understanding the given function. It would be better to not find
these functions in the documentation, such that people directly dive into
the code instead.

In the previous patches we have documented
* index_name_pos()
* remove_index_entry_at()
* add_[file_]to_index()
in cache.h

We already have documentation for:
* add_index_entry()
* read_index()

Which leaves us with a TODO for:
* cache -> the_index macros
* refresh_index()
* discard_index()
* ie_match_stat() and ie_modified(); how they are different and when to
  use which.
* write_index() that was renamed to write_locked_index
* cache_tree_invalidate_path()
* cache_tree_update()

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 Documentation/technical/api-in-core-index.txt | 21 ---------------------
 1 file changed, 21 deletions(-)
 delete mode 100644 Documentation/technical/api-in-core-index.txt

diff --git a/Documentation/technical/api-in-core-index.txt b/Documentation/technical/api-in-core-index.txt
deleted file mode 100644
index adbdbf5d75..0000000000
--- a/Documentation/technical/api-in-core-index.txt
+++ /dev/null
@@ -1,21 +0,0 @@
-in-core index API
-=================
-
-Talk about <read-cache.c> and <cache-tree.c>, things like:
-
-* cache -> the_index macros
-* read_index()
-* write_index()
-* ie_match_stat() and ie_modified(); how they are different and when to
-  use which.
-* index_name_pos()
-* remove_index_entry_at()
-* remove_file_from_index()
-* add_file_to_index()
-* add_index_entry()
-* refresh_index()
-* discard_index()
-* cache_tree_invalidate_path()
-* cache_tree_update()
-
-(JC, Linus)
-- 
2.11.0.299.g762782ba8a