[Outreachy] Move doc to header files #434
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
/submit |
|
Submitted as pull.434.git.1572343246.gitgitgadget@gmail.com |
![gitgitgadget[bot]](https://avatars.githubusercontent.com/in/12836?s=60&v=4){kind=small}
| @@ -1,72 +0,0 @@ | |||
| revision walking API | |||
There was a problem hiding this comment.
On the Git mailing list, Emily Shaffer wrote (reply to this):
On Tue, Oct 29, 2019 at 10:00:45AM +0000, Heba Waly via GitGitGadget wrote:
> From: Heba Waly <heba.waly@gmail.com>
>
> Move the documentation from Documentation/technical/api-revision-walking.txt
> to revision.h as it's easier for the developers to find the usage
> information beside the code instead of looking for it in another doc file.
>
> Also documentation/technical/api-revision-walking.txt is removed because the
> information it has is now redundant and it'll be hard to keep it up to
> date and synchronized with the documentation in the header file.
This commit looks nice to me.
It also looks like new work for me to update
Documentation/MyFirstObjectWalk.txt to reflect this when it's merged
later :)
Reviewed-by: Emily Shaffer <emilyshaffer@google.com>
>
> Signed-off-by: Heba Waly <heba.waly@gmail.com>
> ---
> .../technical/api-revision-walking.txt | 72 -------------------
> revision.h | 59 +++++++++++++++
> 2 files changed, 59 insertions(+), 72 deletions(-)
> delete mode 100644 Documentation/technical/api-revision-walking.txt
>
> diff --git a/Documentation/technical/api-revision-walking.txt b/Documentation/technical/api-revision-walking.txt
> deleted file mode 100644
> index 03f9ea6ac4..0000000000
> --- a/Documentation/technical/api-revision-walking.txt
> +++ /dev/null
> @@ -1,72 +0,0 @@
> -revision walking API
> -====================
> -
> -The revision walking API offers functions to build a list of revisions
> -and then iterate over that list.
> -
> -Calling sequence
> -----------------
> -
> -The walking API has a given calling sequence: first you need to
> -initialize a rev_info structure, then add revisions to control what kind
> -of revision list do you want to get, finally you can iterate over the
> -revision list.
> -
> -Functions
> ----------
> -
> -`repo_init_revisions`::
> -
> - Initialize a rev_info structure with default values. The third
> - parameter may be NULL or can be prefix path, and then the `.prefix`
> - variable will be set to it. This is typically the first function you
> - want to call when you want to deal with a revision list. After calling
> - this function, you are free to customize options, like set
> - `.ignore_merges` to 0 if you don't want to ignore merges, and so on. See
> - `revision.h` for a complete list of available options.
> -
> -`add_pending_object`::
> -
> - This function can be used if you want to add commit objects as revision
> - information. You can use the `UNINTERESTING` object flag to indicate if
> - you want to include or exclude the given commit (and commits reachable
> - from the given commit) from the revision list.
> -+
> -NOTE: If you have the commits as a string list then you probably want to
> -use setup_revisions(), instead of parsing each string and using this
> -function.
> -
> -`setup_revisions`::
> -
> - Parse revision information, filling in the `rev_info` structure, and
> - removing the used arguments from the argument list. Returns the number
> - of arguments left that weren't recognized, which are also moved to the
> - head of the argument list. The last parameter is used in case no
> - parameter given by the first two arguments.
> -
> -`prepare_revision_walk`::
> -
> - Prepares the rev_info structure for a walk. You should check if it
> - returns any error (non-zero return code) and if it does not, you can
> - start using get_revision() to do the iteration.
> -
> -`get_revision`::
> -
> - Takes a pointer to a `rev_info` structure and iterates over it,
> - returning a `struct commit *` each time you call it. The end of the
> - revision list is indicated by returning a NULL pointer.
> -
> -`reset_revision_walk`::
> -
> - Reset the flags used by the revision walking api. You can use
> - this to do multiple sequential revision walks.
> -
> -Data structures
> ----------------
> -
> -Talk about <revision.h>, things like:
> -
> -* two diff_options, one for path limiting, another for output;
> -* remaining functions;
> -
> -(Linus, JC, Dscho)
> diff --git a/revision.h b/revision.h
> index 4134dc6029..983ffc0f12 100644
> --- a/revision.h
> +++ b/revision.h
> @@ -9,6 +9,19 @@
> #include "diff.h"
> #include "commit-slab-decl.h"
>
> +/**
> + * The revision walking API offers functions to build a list of revisions
> + * and then iterate over that list.
> + *
> + * Calling sequence
> + * ----------------
> + *
> + * The walking API has a given calling sequence: first you need to initialize
> + * a rev_info structure, then add revisions to control what kind of revision
> + * list do you want to get, finally you can iterate over the revision list.
> + *
> + */
> +
> /* Remember to update object flag allocation in object.h */
> #define SEEN (1u<<0)
> #define UNINTERESTING (1u<<1)
> @@ -306,11 +319,29 @@ struct setup_revision_opt {
> #ifndef NO_THE_REPOSITORY_COMPATIBILITY_MACROS
> #define init_revisions(revs, prefix) repo_init_revisions(the_repository, revs, prefix)
> #endif
> +
> +/**
> + * Initialize a rev_info structure with default values. The third parameter may
> + * be NULL or can be prefix path, and then the `.prefix` variable will be set
> + * to it. This is typically the first function you want to call when you want
> + * to deal with a revision list. After calling this function, you are free to
> + * customize options, like set `.ignore_merges` to 0 if you don't want to
> + * ignore merges, and so on.
> + */
> void repo_init_revisions(struct repository *r,
> struct rev_info *revs,
> const char *prefix);
> +
> +/**
> + * Parse revision information, filling in the `rev_info` structure, and
> + * removing the used arguments from the argument list. Returns the number
> + * of arguments left that weren't recognized, which are also moved to the
> + * head of the argument list. The last parameter is used in case no
> + * parameter given by the first two arguments.
> + */
> int setup_revisions(int argc, const char **argv, struct rev_info *revs,
> struct setup_revision_opt *);
> +
> void parse_revision_opt(struct rev_info *revs, struct parse_opt_ctx_t *ctx,
> const struct option *options,
> const char * const usagestr[]);
> @@ -319,9 +350,26 @@ void parse_revision_opt(struct rev_info *revs, struct parse_opt_ctx_t *ctx,
> int handle_revision_arg(const char *arg, struct rev_info *revs,
> int flags, unsigned revarg_opt);
>
> +/**
> + * Reset the flags used by the revision walking api. You can use this to do
> + * multiple sequential revision walks.
> + */
> void reset_revision_walk(void);
> +
> +/**
> + * Prepares the rev_info structure for a walk. You should check if it returns
> + * any error (non-zero return code) and if it does not, you can start using
> + * get_revision() to do the iteration.
> + */
> int prepare_revision_walk(struct rev_info *revs);
> +
> +/**
> + * Takes a pointer to a `rev_info` structure and iterates over it, returning a
> + * `struct commit *` each time you call it. The end of the revision list is
> + * indicated by returning a NULL pointer.
> + */
> struct commit *get_revision(struct rev_info *revs);
> +
> char *get_revision_mark(const struct rev_info *revs,
> const struct commit *commit);
> void put_revision_mark(const struct rev_info *revs,
> @@ -333,8 +381,19 @@ void mark_trees_uninteresting_sparse(struct repository *r, struct oidset *trees)
>
> void show_object_with_name(FILE *, struct object *, const char *);
>
> +/**
> + * This function can be used if you want to add commit objects as revision
> + * information. You can use the `UNINTERESTING` object flag to indicate if
> + * you want to include or exclude the given commit (and commits reachable
> + * from the given commit) from the revision list.
> + *
> + * NOTE: If you have the commits as a string list then you probably want to
> + * use setup_revisions(), instead of parsing each string and using this
> + * function.
> + */
> void add_pending_object(struct rev_info *revs,
> struct object *obj, const char *name);
> +
> void add_pending_oid(struct rev_info *revs,
> const char *name, const struct object_id *oid,
> unsigned int flags);
> --
> gitgitgadget
>
| @@ -1,104 +0,0 @@ | |||
| merge API | |||
There was a problem hiding this comment.
On the Git mailing list, Elijah Newren wrote (reply to this):
Hi Heba,
Thanks for the contribution. I know you weren't the original author
of most this stuff, but I was curious if it really all belonged in
ll-merge.c and then noticed other issues...
On Tue, Oct 29, 2019 at 11:49 AM Heba Waly via GitGitGadget
<gitgitgadget@gmail.com> wrote:
[...]
> diff --git a/ll-merge.h b/ll-merge.h
> index e78973dd55..ec3617c627 100644
> --- a/ll-merge.h
> +++ b/ll-merge.h
> @@ -7,16 +7,94 @@
>
> #include "xdiff/xdiff.h"
>
> +/**
> + * The merge API helps a program to reconcile two competing sets of
Is this talking about xdiff/xmerge.c, ll_merge.c, merge-recursive.c,
or builtin/merge.c? Those are all different level of "merge API" and
it's not clear. Perhaps "The Low Level Merge API" or something like
that since you are moving it into ll-merge.h?
> + * improvements to some files (e.g., unregistered changes from the work
> + * tree versus changes involved in switching to a new branch), reporting
> + * conflicts if found.
Seems weird to bring up checkout -m without mentioning in by name
given that it isn't the default checkout behavior. Would seem more
natural to mention a merge or rebase case.
> + * The library called through this API is
> + * responsible for a few things.
> + *
> + * - determining which trees to merge (recursive ancestor consolidation);
Um, that's done at the merge-recursive.c level, not at the ll-merge.c
level. I'm confused why it'd be mentioned here.
> + * - lining up corresponding files in the trees to be merged (rename
> + * detection, subtree shifting), reporting edge cases like add/add
> + * and rename/rename conflicts to the user;
All of that is also clearly stuff for merge-recursive.c; I'm not sure
why it'd be mentioned in the Low-Level merge file.
> + * - performing a three-way merge of corresponding files, taking
> + * path-specific merge drivers (specified in `.gitattributes`)
> + * into account.
This, however, is ll-merge.c stuff.
> + *
> + * Calling sequence:
> + * ----------------
> + *
> + * - Prepare a `struct ll_merge_options` to record options.
> + * If you have no special requests, skip this and pass `NULL`
> + * as the `opts` parameter to use the default options.
> + *
> + * - Allocate an mmbuffer_t variable for the result.
> + *
> + * - Allocate and fill variables with the file's original content
> + * and two modified versions (using `read_mmfile`, for example).
> + *
> + * - Call `ll_merge()`.
> + *
> + * - Read the merged content from `result_buf.ptr` and `result_buf.size`.
> + *
> + * - Release buffers when finished. A simple
> + * `free(ancestor.ptr); free(ours.ptr); free(theirs.ptr);
> + * free(result_buf.ptr);` will do.
> + *
> + * If the modifications do not merge cleanly, `ll_merge` will return a
> + * nonzero value and `result_buf` will generally include a description of
> + * the conflict bracketed by markers such as the traditional `<<<<<<<`
> + * and `>>>>>>>`.
> + *
> + * The `ancestor_label`, `our_label`, and `their_label` parameters are
> + * used to label the different sides of a conflict if the merge driver
> + * supports this.
> + */
This part looks good.
> +/**
> + * This describes the set of options the calling program wants to affect
> + * the operation of a low-level (single file) merge.
> + */
> struct ll_merge_options {
> +
> + /**
> + * Behave as though this were part of a merge between common ancestors in
> + * a recursive merge. If a helper program is specified by the
> + * `[merge "<driver>"] recursive` configuration, it will be used.
> + */
This kind of leaves out the why. Maybe add "(merges of binary files
may need to be handled differently in such cases, for example)" to the
end of the first sentence?
> unsigned virtual_ancestor : 1;
> - unsigned variant : 2; /* favor ours, favor theirs, or union merge */
> +
> + /**
> + * Resolve local conflicts automatically in favor of one side or the other
> + * (as in 'git merge-file' `--ours`/`--theirs`/`--union`). Can be `0`,
> + * `XDL_MERGE_FAVOR_OURS`, `XDL_MERGE_FAVOR_THEIRS`,
> + * or `XDL_MERGE_FAVOR_UNION`.
> + */
> + unsigned variant : 2;
> +
> + /**
> + * Resmudge and clean the "base", "theirs" and "ours" files before merging.
> + * Use this when the merge is likely to have overlapped with a change in
> + * smudge/clean or end-of-line normalization rules.
> + */
> unsigned renormalize : 1;
All looks good.
> +
> unsigned extra_marker_size;
No documentation for this one? Perhaps:
/*
* Increase the length of conflict markers so that nested conflicts
* can be differentiated.
*/
> long xdl_opts;
Perhaps document this one with:
/* Extra xpparam_t flags as defined in xdiff/xdiff.h. */
> };
>
> +/**
> + * Perform a three-way single-file merge in core. This is a thin wrapper
> + * around `xdl_merge` that takes the path and any merge backend specified in
> + * `.gitattributes` or `.git/info/attributes` into account.
> + * Returns 0 for a clean merge.
> + */
> int ll_merge(mmbuffer_t *result_buf,
> const char *path,
> mmfile_t *ancestor, const char *ancestor_label,
There was a problem hiding this comment.
On the Git mailing list, Heba Waly wrote (reply to this):
On Thu, Oct 31, 2019 at 11:09 AM Elijah Newren <newren@gmail.com> wrote:
>
> Hi Heba,
> Thanks for the contribution. I know you weren't the original author
> of most this stuff, but I was curious if it really all belonged in
> ll-merge.c and then noticed other issues...
Hi Elijah, thanks a lot for the feedback.
This is my first interaction with the merge API, so I wasn't
completely sure where the intro of the doc needed to go, looks like
ll-merge.h is not the perfect place, is there a top level merge file
where this generic intro would be more suitable and helpful?
> On Tue, Oct 29, 2019 at 11:49 AM Heba Waly via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
> [...]
> > diff --git a/ll-merge.h b/ll-merge.h
> > index e78973dd55..ec3617c627 100644
> > --- a/ll-merge.h
> > +++ b/ll-merge.h
> > @@ -7,16 +7,94 @@
> >
> > #include "xdiff/xdiff.h"
> >
> > +/**
> > + * The merge API helps a program to reconcile two competing sets of
>
> Is this talking about xdiff/xmerge.c, ll_merge.c, merge-recursive.c,
> or builtin/merge.c? Those are all different level of "merge API" and
> it's not clear. Perhaps "The Low Level Merge API" or something like
> that since you are moving it into ll-merge.h?
Yea, that's why I'm thinking maybe move this paragraph to another
top-level file?
> > + * improvements to some files (e.g., unregistered changes from the work
> > + * tree versus changes involved in switching to a new branch), reporting
> > + * conflicts if found.
>
> Seems weird to bring up checkout -m without mentioning in by name
> given that it isn't the default checkout behavior. Would seem more
> natural to mention a merge or rebase case.
I agree with you, can change the example if we agreed on keeping this paragraph.
> > + * The library called through this API is
> > + * responsible for a few things.
> > + *
> > + * - determining which trees to merge (recursive ancestor consolidation);
>
> Um, that's done at the merge-recursive.c level, not at the ll-merge.c
> level. I'm confused why it'd be mentioned here.
you're right.
> > + * - lining up corresponding files in the trees to be merged (rename
> > + * detection, subtree shifting), reporting edge cases like add/add
> > + * and rename/rename conflicts to the user;
>
> All of that is also clearly stuff for merge-recursive.c; I'm not sure
> why it'd be mentioned in the Low-Level merge file.
got it.
> > + * - performing a three-way merge of corresponding files, taking
> > + * path-specific merge drivers (specified in `.gitattributes`)
> > + * into account.
>
> This, however, is ll-merge.c stuff.
So, move the whole paragraph to another file?
because, I think the paragraph is helpful as a whole, and I don't see
the value in dividing it between merge-recursive and ll-merge.
What do you think?
> > + *
> > + * Calling sequence:
> > + * ----------------
> > + *
> > + * - Prepare a `struct ll_merge_options` to record options.
> > + * If you have no special requests, skip this and pass `NULL`
> > + * as the `opts` parameter to use the default options.
> > + *
> > + * - Allocate an mmbuffer_t variable for the result.
> > + *
> > + * - Allocate and fill variables with the file's original content
> > + * and two modified versions (using `read_mmfile`, for example).
> > + *
> > + * - Call `ll_merge()`.
> > + *
> > + * - Read the merged content from `result_buf.ptr` and `result_buf.size`.
> > + *
> > + * - Release buffers when finished. A simple
> > + * `free(ancestor.ptr); free(ours.ptr); free(theirs.ptr);
> > + * free(result_buf.ptr);` will do.
> > + *
> > + * If the modifications do not merge cleanly, `ll_merge` will return a
> > + * nonzero value and `result_buf` will generally include a description of
> > + * the conflict bracketed by markers such as the traditional `<<<<<<<`
> > + * and `>>>>>>>`.
> > + *
> > + * The `ancestor_label`, `our_label`, and `their_label` parameters are
> > + * used to label the different sides of a conflict if the merge driver
> > + * supports this.
> > + */
>
> This part looks good.
>
> > +/**
> > + * This describes the set of options the calling program wants to affect
> > + * the operation of a low-level (single file) merge.
> > + */
> > struct ll_merge_options {
> > +
> > + /**
> > + * Behave as though this were part of a merge between common ancestors in
> > + * a recursive merge. If a helper program is specified by the
> > + * `[merge "<driver>"] recursive` configuration, it will be used.
> > + */
>
> This kind of leaves out the why. Maybe add "(merges of binary files
> may need to be handled differently in such cases, for example)" to the
> end of the first sentence?
Yes, ok.
> > unsigned virtual_ancestor : 1;
> > - unsigned variant : 2; /* favor ours, favor theirs, or union merge */
> > +
> > + /**
> > + * Resolve local conflicts automatically in favor of one side or the other
> > + * (as in 'git merge-file' `--ours`/`--theirs`/`--union`). Can be `0`,
> > + * `XDL_MERGE_FAVOR_OURS`, `XDL_MERGE_FAVOR_THEIRS`,
> > + * or `XDL_MERGE_FAVOR_UNION`.
> > + */
> > + unsigned variant : 2;
> > +
> > + /**
> > + * Resmudge and clean the "base", "theirs" and "ours" files before merging.
> > + * Use this when the merge is likely to have overlapped with a change in
> > + * smudge/clean or end-of-line normalization rules.
> > + */
> > unsigned renormalize : 1;
>
> All looks good.
>
> > +
> > unsigned extra_marker_size;
>
> No documentation for this one? Perhaps:
>
> /*
> * Increase the length of conflict markers so that nested conflicts
> * can be differentiated.
> */
sure.
> > long xdl_opts;
>
> Perhaps document this one with:
>
> /* Extra xpparam_t flags as defined in xdiff/xdiff.h. */
great. thanks!
>
>
> > };
> >
> > +/**
> > + * Perform a three-way single-file merge in core. This is a thin wrapper
> > + * around `xdl_merge` that takes the path and any merge backend specified in
> > + * `.gitattributes` or `.git/info/attributes` into account.
> > + * Returns 0 for a clean merge.
> > + */
> > int ll_merge(mmbuffer_t *result_buf,
> > const char *path,
> > mmfile_t *ancestor, const char *ancestor_label,
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
Elijah Newren <newren@gmail.com> writes:
> Thanks for the contribution. I know you weren't the original author
> of most this stuff, but I was curious if it really all belonged in
> ll-merge.c and then noticed other issues...
>
Thanks, both. Especially thanks Elijah for all good comments.
HebaWaly
force-pushed
the
move-doc-to-header
branch
from
5757216
to
4c3215e
Compare
|
/submit |
|
Submitted as pull.434.v2.git.1573034387.gitgitgadget@gmail.com |
| @@ -1,41 +0,0 @@ | |||
| sigchain API | |||
There was a problem hiding this comment.
On the Git mailing list, Emily Shaffer wrote (reply to this):
On Wed, Nov 06, 2019 at 09:59:38AM +0000, Heba Waly via GitGitGadget wrote:
> From: Heba Waly <heba.waly@gmail.com>
>
> Move the documentation from Documentation/technical/api-sigchain.txt
> to sigchain.h as it's easier for the developers to find the usage
> information beside the code instead of looking for it in another doc file.
>
> Also documentation/technical/api-sigchain.txt is removed because the
> information it has is now redundant and it'll be hard to keep it up to
> date and synchronized with the documentation in the header file.
>
> Signed-off-by: Heba Waly <heba.waly@gmail.com>
> ---
> Documentation/technical/api-sigchain.txt | 41 -----------------------
> sigchain.h | 42 ++++++++++++++++++++++++
> 2 files changed, 42 insertions(+), 41 deletions(-)
> delete mode 100644 Documentation/technical/api-sigchain.txt
>
> diff --git a/Documentation/technical/api-sigchain.txt b/Documentation/technical/api-sigchain.txt
> deleted file mode 100644
> index 9e1189ef01..0000000000
> --- a/Documentation/technical/api-sigchain.txt
> +++ /dev/null
> @@ -1,41 +0,0 @@
> -sigchain API
> -============
> -
> -Code often wants to set a signal handler to clean up temporary files or
> -other work-in-progress when we die unexpectedly. For multiple pieces of
> -code to do this without conflicting, each piece of code must remember
> -the old value of the handler and restore it either when:
> -
> - 1. The work-in-progress is finished, and the handler is no longer
> - necessary. The handler should revert to the original behavior
> - (either another handler, SIG_DFL, or SIG_IGN).
> -
> - 2. The signal is received. We should then do our cleanup, then chain
> - to the next handler (or die if it is SIG_DFL).
> -
> -Sigchain is a tiny library for keeping a stack of handlers. Your handler
> -and installation code should look something like:
> -
> -------------------------------------------
> - void clean_foo_on_signal(int sig)
> - {
> - clean_foo();
> - sigchain_pop(sig);
> - raise(sig);
> - }
> -
> - void other_func()
> - {
> - sigchain_push_common(clean_foo_on_signal);
> - mess_up_foo();
> - clean_foo();
> - }
> -------------------------------------------
> -
> -Handlers are given the typedef of sigchain_fun. This is the same type
> -that is given to signal() or sigaction(). It is perfectly reasonable to
> -push SIG_DFL or SIG_IGN onto the stack.
> -
> -You can sigchain_push and sigchain_pop individual signals. For
> -convenience, sigchain_push_common will push the handler onto the stack
> -for many common signals.
> diff --git a/sigchain.h b/sigchain.h
> index 138b20f54b..a990f18cf6 100644
> --- a/sigchain.h
> +++ b/sigchain.h
> @@ -1,12 +1,54 @@
> #ifndef SIGCHAIN_H
> #define SIGCHAIN_H
>
> +/**
> + * Code often wants to set a signal handler to clean up temporary files or
> + * other work-in-progress when we die unexpectedly. For multiple pieces of
> + * code to do this without conflicting, each piece of code must remember
> + * the old value of the handler and restore it either when:
> + *
> + * 1. The work-in-progress is finished, and the handler is no longer
> + * necessary. The handler should revert to the original behavior
> + * (either another handler, SIG_DFL, or SIG_IGN).
> + *
> + * 2. The signal is received. We should then do our cleanup, then chain
> + * to the next handler (or die if it is SIG_DFL).
> + *
> + * Sigchain is a tiny library for keeping a stack of handlers. Your handler
> + * and installation code should look something like:
> + *
> + * ------------------------------------------
> + * void clean_foo_on_signal(int sig)
> + * {
> + * clean_foo();
> + * sigchain_pop(sig);
> + * raise(sig);
> + * }
> + *
> + * void other_func()
> + * {
> + * sigchain_push_common(clean_foo_on_signal);
> + * mess_up_foo();
> + * clean_foo();
> + * }
> + * ------------------------------------------
> + *
> + */
> +
> +/**
> + * Handlers are given the typedef of sigchain_fun. This is the same type
> + * that is given to signal() or sigaction(). It is perfectly reasonable to
> + * push SIG_DFL or SIG_IGN onto the stack.
> + */
> typedef void (*sigchain_fun)(int);
>
> +/* You can sigchain_push and sigchain_pop individual signals. */
> int sigchain_push(int sig, sigchain_fun f);
> int sigchain_pop(int sig);
>
> +/* push the handler onto the stack for many common signals. */
It was lacking in the original doc too, but I want to know which common
signals it pushes for. Is it too much work for you to peek in the
implementation and let us know?
> void sigchain_push_common(sigchain_fun f);
> +
> void sigchain_pop_common(void);
>
> #endif /* SIGCHAIN_H */
> --
> gitgitgadget
>
Otherwise this one looks fine for me.
- Emily
There was a problem hiding this comment.
On the Git mailing list, Heba Waly wrote (reply to this):
On Thu, Nov 7, 2019 at 11:03 AM Emily Shaffer <emilyshaffer@google.com> wrote:
>
> On Wed, Nov 06, 2019 at 09:59:38AM +0000, Heba Waly via GitGitGadget wrote:
> > From: Heba Waly <heba.waly@gmail.com>
> >
> > Move the documentation from Documentation/technical/api-sigchain.txt
> > to sigchain.h as it's easier for the developers to find the usage
> > information beside the code instead of looking for it in another doc file.
> >
> > Also documentation/technical/api-sigchain.txt is removed because the
> > information it has is now redundant and it'll be hard to keep it up to
> > date and synchronized with the documentation in the header file.
> >
> > Signed-off-by: Heba Waly <heba.waly@gmail.com>
> > ---
> > Documentation/technical/api-sigchain.txt | 41 -----------------------
> > sigchain.h | 42 ++++++++++++++++++++++++
> > 2 files changed, 42 insertions(+), 41 deletions(-)
> > delete mode 100644 Documentation/technical/api-sigchain.txt
> >
> > diff --git a/Documentation/technical/api-sigchain.txt b/Documentation/technical/api-sigchain.txt
> > deleted file mode 100644
> > index 9e1189ef01..0000000000
> > --- a/Documentation/technical/api-sigchain.txt
> > +++ /dev/null
> > @@ -1,41 +0,0 @@
> > -sigchain API
> > -============
> > -
> > -Code often wants to set a signal handler to clean up temporary files or
> > -other work-in-progress when we die unexpectedly. For multiple pieces of
> > -code to do this without conflicting, each piece of code must remember
> > -the old value of the handler and restore it either when:
> > -
> > - 1. The work-in-progress is finished, and the handler is no longer
> > - necessary. The handler should revert to the original behavior
> > - (either another handler, SIG_DFL, or SIG_IGN).
> > -
> > - 2. The signal is received. We should then do our cleanup, then chain
> > - to the next handler (or die if it is SIG_DFL).
> > -
> > -Sigchain is a tiny library for keeping a stack of handlers. Your handler
> > -and installation code should look something like:
> > -
> > -------------------------------------------
> > - void clean_foo_on_signal(int sig)
> > - {
> > - clean_foo();
> > - sigchain_pop(sig);
> > - raise(sig);
> > - }
> > -
> > - void other_func()
> > - {
> > - sigchain_push_common(clean_foo_on_signal);
> > - mess_up_foo();
> > - clean_foo();
> > - }
> > -------------------------------------------
> > -
> > -Handlers are given the typedef of sigchain_fun. This is the same type
> > -that is given to signal() or sigaction(). It is perfectly reasonable to
> > -push SIG_DFL or SIG_IGN onto the stack.
> > -
> > -You can sigchain_push and sigchain_pop individual signals. For
> > -convenience, sigchain_push_common will push the handler onto the stack
> > -for many common signals.
> > diff --git a/sigchain.h b/sigchain.h
> > index 138b20f54b..a990f18cf6 100644
> > --- a/sigchain.h
> > +++ b/sigchain.h
> > @@ -1,12 +1,54 @@
> > #ifndef SIGCHAIN_H
> > #define SIGCHAIN_H
> >
> > +/**
> > + * Code often wants to set a signal handler to clean up temporary files or
> > + * other work-in-progress when we die unexpectedly. For multiple pieces of
> > + * code to do this without conflicting, each piece of code must remember
> > + * the old value of the handler and restore it either when:
> > + *
> > + * 1. The work-in-progress is finished, and the handler is no longer
> > + * necessary. The handler should revert to the original behavior
> > + * (either another handler, SIG_DFL, or SIG_IGN).
> > + *
> > + * 2. The signal is received. We should then do our cleanup, then chain
> > + * to the next handler (or die if it is SIG_DFL).
> > + *
> > + * Sigchain is a tiny library for keeping a stack of handlers. Your handler
> > + * and installation code should look something like:
> > + *
> > + * ------------------------------------------
> > + * void clean_foo_on_signal(int sig)
> > + * {
> > + * clean_foo();
> > + * sigchain_pop(sig);
> > + * raise(sig);
> > + * }
> > + *
> > + * void other_func()
> > + * {
> > + * sigchain_push_common(clean_foo_on_signal);
> > + * mess_up_foo();
> > + * clean_foo();
> > + * }
> > + * ------------------------------------------
> > + *
> > + */
> > +
> > +/**
> > + * Handlers are given the typedef of sigchain_fun. This is the same type
> > + * that is given to signal() or sigaction(). It is perfectly reasonable to
> > + * push SIG_DFL or SIG_IGN onto the stack.
> > + */
> > typedef void (*sigchain_fun)(int);
> >
> > +/* You can sigchain_push and sigchain_pop individual signals. */
> > int sigchain_push(int sig, sigchain_fun f);
> > int sigchain_pop(int sig);
> >
> > +/* push the handler onto the stack for many common signals. */
> It was lacking in the original doc too, but I want to know which common
> signals it pushes for. Is it too much work for you to peek in the
> implementation and let us know?
Sure, no problem, I'll add the signals to the comment.
> > void sigchain_push_common(sigchain_fun f);
> > +
> > void sigchain_pop_common(void);
> >
> > #endif /* SIGCHAIN_H */
> > --
> > gitgitgadget
> >
>
> Otherwise this one looks fine for me.
>
> - Emily
| @@ -1,39 +0,0 @@ | |||
| allocation growing API | |||
There was a problem hiding this comment.
On the Git mailing list, Emily Shaffer wrote (reply to this):
On Wed, Nov 06, 2019 at 09:59:39AM +0000, Heba Waly via GitGitGadget wrote:
> From: Heba Waly <heba.waly@gmail.com>
>
> Move the documentation from Documentation/technical/api-allocation-growing.txt
> to cache.h as it's easier for the developers to find the usage
> information beside the code instead of looking for it in another doc file.
>
> Also documentation/technical/api-allocation-growing.txt is removed because the
> information it has is now redundant and it'll be hard to keep it up to
> date and synchronized with the documentation in the header file.
>
> Signed-off-by: Heba Waly <heba.waly@gmail.com>
Reviewed-by: Emily Shaffer <emilyshaffer@google.com>
| @@ -1,130 +0,0 @@ | |||
| directory listing API | |||
There was a problem hiding this comment.
On the Git mailing list, Emily Shaffer wrote (reply to this):
On Wed, Nov 06, 2019 at 09:59:29AM +0000, Heba Waly via GitGitGadget wrote:
> From: Heba Waly <heba.waly@gmail.com>
>
> Move the documentation from Documentation/technical/api-directory-listing.txt
> to dir.h as it's easier for the developers to find the usage information
> beside the code instead of looking for it in another doc file.
>
> Also documentation/technical/api-directory-listing.txt is removed because
> the information it has is now redundant and it'll be hard to keep it up to
> date and synchronized with the documentation in the header files.
>
> Signed-off-by: Heba Waly <heba.waly@gmail.com>
> ---
> .../technical/api-directory-listing.txt | 130 ------------------
> dir.h | 118 +++++++++++++++-
> 2 files changed, 113 insertions(+), 135 deletions(-)
> delete mode 100644 Documentation/technical/api-directory-listing.txt
>
> diff --git a/Documentation/technical/api-directory-listing.txt b/Documentation/technical/api-directory-listing.txt
> deleted file mode 100644
> index 76b6e4f71b..0000000000
> --- a/Documentation/technical/api-directory-listing.txt
> +++ /dev/null
> @@ -1,130 +0,0 @@
> -directory listing API
> -=====================
> -
> -The directory listing API is used to enumerate paths in the work tree,
> -optionally taking `.git/info/exclude` and `.gitignore` files per
> -directory into account.
> -
> -Data structure
> ---------------
> -
> -`struct dir_struct` structure is used to pass directory traversal
> -options to the library and to record the paths discovered. A single
> -`struct dir_struct` is used regardless of whether or not the traversal
> -recursively descends into subdirectories.
> -
> -The notable options are:
> -
> -`exclude_per_dir`::
> -
> - The name of the file to be read in each directory for excluded
> - files (typically `.gitignore`).
> -
> -`flags`::
> -
> - A bit-field of options:
> -
> -`DIR_SHOW_IGNORED`:::
> -
> - Return just ignored files in `entries[]`, not untracked
> - files. This flag is mutually exclusive with
> - `DIR_SHOW_IGNORED_TOO`.
> -
> -`DIR_SHOW_IGNORED_TOO`:::
> -
> - Similar to `DIR_SHOW_IGNORED`, but return ignored files in
> - `ignored[]` in addition to untracked files in
> - `entries[]`. This flag is mutually exclusive with
> - `DIR_SHOW_IGNORED`.
> -
> -`DIR_KEEP_UNTRACKED_CONTENTS`:::
> -
> - Only has meaning if `DIR_SHOW_IGNORED_TOO` is also set; if this is set, the
> - untracked contents of untracked directories are also returned in
> - `entries[]`.
> -
> -`DIR_SHOW_IGNORED_TOO_MODE_MATCHING`:::
> -
> - Only has meaning if `DIR_SHOW_IGNORED_TOO` is also set; if
> - this is set, returns ignored files and directories that match
> - an exclude pattern. If a directory matches an exclude pattern,
> - then the directory is returned and the contained paths are
> - not. A directory that does not match an exclude pattern will
> - not be returned even if all of its contents are ignored. In
> - this case, the contents are returned as individual entries.
> -+
> -If this is set, files and directories that explicitly match an ignore
> -pattern are reported. Implicitly ignored directories (directories that
> -do not match an ignore pattern, but whose contents are all ignored)
> -are not reported, instead all of the contents are reported.
> -
> -`DIR_COLLECT_IGNORED`:::
> -
> - Special mode for git-add. Return ignored files in `ignored[]` and
> - untracked files in `entries[]`. Only returns ignored files that match
> - pathspec exactly (no wildcards). Does not recurse into ignored
> - directories.
> -
> -`DIR_SHOW_OTHER_DIRECTORIES`:::
> -
> - Include a directory that is not tracked.
> -
> -`DIR_HIDE_EMPTY_DIRECTORIES`:::
> -
> - Do not include a directory that is not tracked and is empty.
> -
> -`DIR_NO_GITLINKS`:::
> -
> - If set, recurse into a directory that looks like a Git
> - directory. Otherwise it is shown as a directory.
> -
> -The result of the enumeration is left in these fields:
> -
> -`entries[]`::
> -
> - An array of `struct dir_entry`, each element of which describes
> - a path.
> -
> -`nr`::
> -
> - The number of members in `entries[]` array.
> -
> -`alloc`::
> -
> - Internal use; keeps track of allocation of `entries[]` array.
> -
> -`ignored[]`::
> -
> - An array of `struct dir_entry`, used for ignored paths with the
> - `DIR_SHOW_IGNORED_TOO` and `DIR_COLLECT_IGNORED` flags.
> -
> -`ignored_nr`::
> -
> - The number of members in `ignored[]` array.
> -
> -Calling sequence
> -----------------
> -
> -Note: index may be looked at for .gitignore files that are CE_SKIP_WORKTREE
> -marked. If you to exclude files, make sure you have loaded index first.
> -
> -* Prepare `struct dir_struct dir` and clear it with `memset(&dir, 0,
> - sizeof(dir))`.
> -
> -* To add single exclude pattern, call `add_pattern_list()` and then
> - `add_pattern()`.
> -
> -* To add patterns from a file (e.g. `.git/info/exclude`), call
> - `add_patterns_from_file()` , and/or set `dir.exclude_per_dir`. A
> - short-hand function `setup_standard_excludes()` can be used to set
> - up the standard set of exclude settings.
> -
> -* Set options described in the Data Structure section above.
> -
> -* Call `read_directory()`.
> -
> -* Use `dir.entries[]`.
> -
> -* Call `clear_directory()` when none of the contained elements are no longer in use.
> -
> -(JC)
> diff --git a/dir.h b/dir.h
> index 2fbdef014f..1b41d29c07 100644
> --- a/dir.h
> +++ b/dir.h
> @@ -1,11 +1,43 @@
> #ifndef DIR_H
> #define DIR_H
>
> -/* See Documentation/technical/api-directory-listing.txt */
> -
> #include "cache.h"
> #include "strbuf.h"
>
> +/**
> + * The directory listing API is used to enumerate paths in the work tree,
> + * optionally taking `.git/info/exclude` and `.gitignore` files per directory
> + * into account.
> + */
> +
> +/**
> + * Calling sequence
> + * ----------------
> + *
> + * Note: index may be looked at for .gitignore files that are CE_SKIP_WORKTREE
> + * marked. If you to exclude files, make sure you have loaded index first.
I know this is verbatim from the old doc, but the grammar is a little
off. Might be a good chance to fix it up (or add another patch on top
doing so?)
> + *
> + * - Prepare `struct dir_struct dir` and clear it with `memset(&dir, 0,
> + * sizeof(dir))`.
> + *
> + * - To add single exclude pattern, call `add_pattern_list()` and then
> + * `add_pattern()`.
> + *
> + * - To add patterns from a file (e.g. `.git/info/exclude`), call
> + * `add_patterns_from_file()` , and/or set `dir.exclude_per_dir`. A
> + * short-hand function `setup_standard_excludes()` can be used to set
> + * up the standard set of exclude settings.
> + *
> + * - Set options described in the Data Structure section above.
> + *
> + * - Call `read_directory()`.
> + *
> + * - Use `dir.entries[]`.
> + *
> + * - Call `clear_directory()` when none of the contained elements are no longer in use.
> + *
> + */
> +
> struct dir_entry {
> unsigned int len;
> char name[FLEX_ARRAY]; /* more */
> @@ -144,25 +176,101 @@ struct untracked_cache {
> unsigned int use_fsmonitor : 1;
> };
>
> +/**
> + * pass directory traversal options to the library and to record the paths
> + * discovered. A single `struct dir_struct` is used regardless of whether or
> + * not the traversal recursively descends into subdirectories.
I wouldn't mind seeing some minor rewording to make the language here
agree with itself, since it's no longer being led with a subject noun.
Or, you could still tack "This struct is used to" or even "Used to" onto
the front so that the language makes sense again.
The way the language is now with the subject cropped out, it sounds like
you're describing a function which does the pass + record for you (to
me).
> + */
> struct dir_struct {
> - int nr, alloc;
> - int ignored_nr, ignored_alloc;
> +
> + /* The number of members in `entries[]` array. */
> + int nr;
> +
> + /* Internal use; keeps track of allocation of `entries[]` array.*/
> + int alloc;
> +
> + /* The number of members in `ignored[]` array. */
> + int ignored_nr;
> +
> + int ignored_alloc;
> +
> + /* bit-field of options */
> enum {
> +
> + /**
> + * Return just ignored files in `entries[]`, not untracked files.
> + * This flag is mutually exclusive with `DIR_SHOW_IGNORED_TOO`.
> + */
I think something went wrong with the whitespace on this section (most of
the rest looks OK).
> DIR_SHOW_IGNORED = 1<<0,
> +
> + /* Include a directory that is not tracked. */
> DIR_SHOW_OTHER_DIRECTORIES = 1<<1,
> +
> + /* Do not include a directory that is not tracked and is empty. */
> DIR_HIDE_EMPTY_DIRECTORIES = 1<<2,
> +
> + /**
> + * If set, recurse into a directory that looks like a Git directory.
> + * Otherwise it is shown as a directory.
> + */
> DIR_NO_GITLINKS = 1<<3,
> +
> + /**
> + * Special mode for git-add. Return ignored files in `ignored[]` and
> + * untracked files in `entries[]`. Only returns ignored files that match
> + * pathspec exactly (no wildcards). Does not recurse into ignored
> + * directories.
> + */
> DIR_COLLECT_IGNORED = 1<<4,
> +
> + /**
> + * Similar to `DIR_SHOW_IGNORED`, but return ignored files in
> + * `ignored[]` in addition to untracked files in `entries[]`.
> + * This flag is mutually exclusive with `DIR_SHOW_IGNORED`.
> + */
> DIR_SHOW_IGNORED_TOO = 1<<5,
> +
> DIR_COLLECT_KILLED_ONLY = 1<<6,
> +
> + /**
> + * Only has meaning if `DIR_SHOW_IGNORED_TOO` is also set; if this is
> + * set, the untracked contents of untracked directories are also
> + * returned in `entries[]`.
> + */
> DIR_KEEP_UNTRACKED_CONTENTS = 1<<7,
> +
> + /**
> + * Only has meaning if `DIR_SHOW_IGNORED_TOO` is also set; if this is
> + * set, returns ignored files and directories that match an exclude
> + * pattern. If a directory matches an exclude pattern, then the
> + * directory is returned and the contained paths are not. A directory
> + * that does not match an exclude pattern will not be returned even if
> + * all of its contents are ignored. In this case, the contents are
> + * returned as individual entries.
> + *
> + * If this is set, files and directories that explicitly match an ignore
> + * pattern are reported. Implicitly ignored directories (directories that
> + * do not match an ignore pattern, but whose contents are all ignored)
> + * are not reported, instead all of the contents are reported.
> + */
> DIR_SHOW_IGNORED_TOO_MODE_MATCHING = 1<<8,
> +
> DIR_SKIP_NESTED_GIT = 1<<9
> } flags;
> +
> + /* An array of `struct dir_entry`, each element of which describes a path. */
> struct dir_entry **entries;
> +
> + /**
> + * used for ignored paths with the `DIR_SHOW_IGNORED_TOO` and
> + * `DIR_COLLECT_IGNORED` flags.
> + */
> struct dir_entry **ignored;
>
> - /* Exclude info */
> + /**
> + * The name of the file to be read in each directory for excluded files
> + * (typically `.gitignore`).
> + */
> const char *exclude_per_dir;
>
> /*
> --
> gitgitgadget
>
There was a problem hiding this comment.
On the Git mailing list, Heba Waly wrote (reply to this):
On Thu, Nov 7, 2019 at 2:16 PM Emily Shaffer <emilyshaffer@google.com> wrote:
>
> On Wed, Nov 06, 2019 at 09:59:29AM +0000, Heba Waly via GitGitGadget wrote:
> > From: Heba Waly <heba.waly@gmail.com>
> >
> > Move the documentation from Documentation/technical/api-directory-listing.txt
> > to dir.h as it's easier for the developers to find the usage information
> > beside the code instead of looking for it in another doc file.
> >
> > Also documentation/technical/api-directory-listing.txt is removed because
> > the information it has is now redundant and it'll be hard to keep it up to
> > date and synchronized with the documentation in the header files.
> >
> > Signed-off-by: Heba Waly <heba.waly@gmail.com>
> > ---
> > .../technical/api-directory-listing.txt | 130 ------------------
> > dir.h | 118 +++++++++++++++-
> > 2 files changed, 113 insertions(+), 135 deletions(-)
> > delete mode 100644 Documentation/technical/api-directory-listing.txt
> >
> > diff --git a/Documentation/technical/api-directory-listing.txt b/Documentation/technical/api-directory-listing.txt
> > deleted file mode 100644
> > index 76b6e4f71b..0000000000
> > --- a/Documentation/technical/api-directory-listing.txt
> > +++ /dev/null
> > @@ -1,130 +0,0 @@
> > -directory listing API
> > -=====================
> > -
> > -The directory listing API is used to enumerate paths in the work tree,
> > -optionally taking `.git/info/exclude` and `.gitignore` files per
> > -directory into account.
> > -
> > -Data structure
> > ---------------
> > -
> > -`struct dir_struct` structure is used to pass directory traversal
> > -options to the library and to record the paths discovered. A single
> > -`struct dir_struct` is used regardless of whether or not the traversal
> > -recursively descends into subdirectories.
> > -
> > -The notable options are:
> > -
> > -`exclude_per_dir`::
> > -
> > - The name of the file to be read in each directory for excluded
> > - files (typically `.gitignore`).
> > -
> > -`flags`::
> > -
> > - A bit-field of options:
> > -
> > -`DIR_SHOW_IGNORED`:::
> > -
> > - Return just ignored files in `entries[]`, not untracked
> > - files. This flag is mutually exclusive with
> > - `DIR_SHOW_IGNORED_TOO`.
> > -
> > -`DIR_SHOW_IGNORED_TOO`:::
> > -
> > - Similar to `DIR_SHOW_IGNORED`, but return ignored files in
> > - `ignored[]` in addition to untracked files in
> > - `entries[]`. This flag is mutually exclusive with
> > - `DIR_SHOW_IGNORED`.
> > -
> > -`DIR_KEEP_UNTRACKED_CONTENTS`:::
> > -
> > - Only has meaning if `DIR_SHOW_IGNORED_TOO` is also set; if this is set, the
> > - untracked contents of untracked directories are also returned in
> > - `entries[]`.
> > -
> > -`DIR_SHOW_IGNORED_TOO_MODE_MATCHING`:::
> > -
> > - Only has meaning if `DIR_SHOW_IGNORED_TOO` is also set; if
> > - this is set, returns ignored files and directories that match
> > - an exclude pattern. If a directory matches an exclude pattern,
> > - then the directory is returned and the contained paths are
> > - not. A directory that does not match an exclude pattern will
> > - not be returned even if all of its contents are ignored. In
> > - this case, the contents are returned as individual entries.
> > -+
> > -If this is set, files and directories that explicitly match an ignore
> > -pattern are reported. Implicitly ignored directories (directories that
> > -do not match an ignore pattern, but whose contents are all ignored)
> > -are not reported, instead all of the contents are reported.
> > -
> > -`DIR_COLLECT_IGNORED`:::
> > -
> > - Special mode for git-add. Return ignored files in `ignored[]` and
> > - untracked files in `entries[]`. Only returns ignored files that match
> > - pathspec exactly (no wildcards). Does not recurse into ignored
> > - directories.
> > -
> > -`DIR_SHOW_OTHER_DIRECTORIES`:::
> > -
> > - Include a directory that is not tracked.
> > -
> > -`DIR_HIDE_EMPTY_DIRECTORIES`:::
> > -
> > - Do not include a directory that is not tracked and is empty.
> > -
> > -`DIR_NO_GITLINKS`:::
> > -
> > - If set, recurse into a directory that looks like a Git
> > - directory. Otherwise it is shown as a directory.
> > -
> > -The result of the enumeration is left in these fields:
> > -
> > -`entries[]`::
> > -
> > - An array of `struct dir_entry`, each element of which describes
> > - a path.
> > -
> > -`nr`::
> > -
> > - The number of members in `entries[]` array.
> > -
> > -`alloc`::
> > -
> > - Internal use; keeps track of allocation of `entries[]` array.
> > -
> > -`ignored[]`::
> > -
> > - An array of `struct dir_entry`, used for ignored paths with the
> > - `DIR_SHOW_IGNORED_TOO` and `DIR_COLLECT_IGNORED` flags.
> > -
> > -`ignored_nr`::
> > -
> > - The number of members in `ignored[]` array.
> > -
> > -Calling sequence
> > -----------------
> > -
> > -Note: index may be looked at for .gitignore files that are CE_SKIP_WORKTREE
> > -marked. If you to exclude files, make sure you have loaded index first.
> > -
> > -* Prepare `struct dir_struct dir` and clear it with `memset(&dir, 0,
> > - sizeof(dir))`.
> > -
> > -* To add single exclude pattern, call `add_pattern_list()` and then
> > - `add_pattern()`.
> > -
> > -* To add patterns from a file (e.g. `.git/info/exclude`), call
> > - `add_patterns_from_file()` , and/or set `dir.exclude_per_dir`. A
> > - short-hand function `setup_standard_excludes()` can be used to set
> > - up the standard set of exclude settings.
> > -
> > -* Set options described in the Data Structure section above.
> > -
> > -* Call `read_directory()`.
> > -
> > -* Use `dir.entries[]`.
> > -
> > -* Call `clear_directory()` when none of the contained elements are no longer in use.
> > -
> > -(JC)
> > diff --git a/dir.h b/dir.h
> > index 2fbdef014f..1b41d29c07 100644
> > --- a/dir.h
> > +++ b/dir.h
> > @@ -1,11 +1,43 @@
> > #ifndef DIR_H
> > #define DIR_H
> >
> > -/* See Documentation/technical/api-directory-listing.txt */
> > -
> > #include "cache.h"
> > #include "strbuf.h"
> >
> > +/**
> > + * The directory listing API is used to enumerate paths in the work tree,
> > + * optionally taking `.git/info/exclude` and `.gitignore` files per directory
> > + * into account.
> > + */
> > +
> > +/**
> > + * Calling sequence
> > + * ----------------
> > + *
> > + * Note: index may be looked at for .gitignore files that are CE_SKIP_WORKTREE
> > + * marked. If you to exclude files, make sure you have loaded index first.
>
> I know this is verbatim from the old doc, but the grammar is a little
> off. Might be a good chance to fix it up (or add another patch on top
> doing so?)
>
Yes, will fix it up.
> > + *
> > + * - Prepare `struct dir_struct dir` and clear it with `memset(&dir, 0,
> > + * sizeof(dir))`.
> > + *
> > + * - To add single exclude pattern, call `add_pattern_list()` and then
> > + * `add_pattern()`.
> > + *
> > + * - To add patterns from a file (e.g. `.git/info/exclude`), call
> > + * `add_patterns_from_file()` , and/or set `dir.exclude_per_dir`. A
> > + * short-hand function `setup_standard_excludes()` can be used to set
> > + * up the standard set of exclude settings.
> > + *
> > + * - Set options described in the Data Structure section above.
> > + *
> > + * - Call `read_directory()`.
> > + *
> > + * - Use `dir.entries[]`.
> > + *
> > + * - Call `clear_directory()` when none of the contained elements are no longer in use.
> > + *
> > + */
> > +
> > struct dir_entry {
> > unsigned int len;
> > char name[FLEX_ARRAY]; /* more */
> > @@ -144,25 +176,101 @@ struct untracked_cache {
> > unsigned int use_fsmonitor : 1;
> > };
> >
> > +/**
> > + * pass directory traversal options to the library and to record the paths
> > + * discovered. A single `struct dir_struct` is used regardless of whether or
> > + * not the traversal recursively descends into subdirectories.
>
> I wouldn't mind seeing some minor rewording to make the language here
> agree with itself, since it's no longer being led with a subject noun.
> Or, you could still tack "This struct is used to" or even "Used to" onto
> the front so that the language makes sense again.
>
> The way the language is now with the subject cropped out, it sounds like
> you're describing a function which does the pass + record for you (to
> me).
I agree.
> > + */
> > struct dir_struct {
> > - int nr, alloc;
> > - int ignored_nr, ignored_alloc;
> > +
> > + /* The number of members in `entries[]` array. */
> > + int nr;
> > +
> > + /* Internal use; keeps track of allocation of `entries[]` array.*/
> > + int alloc;
> > +
> > + /* The number of members in `ignored[]` array. */
> > + int ignored_nr;
> > +
> > + int ignored_alloc;
> > +
> > + /* bit-field of options */
> > enum {
> > +
> > + /**
> > + * Return just ignored files in `entries[]`, not untracked files.
> > + * This flag is mutually exclusive with `DIR_SHOW_IGNORED_TOO`.
> > + */
>
> I think something went wrong with the whitespace on this section (most of
> the rest looks OK).
Oh, thank you, my new editor needed some settings adjustments, will go
through all changes to make sure nothing was missed.
> > DIR_SHOW_IGNORED = 1<<0,
> > +
> > + /* Include a directory that is not tracked. */
> > DIR_SHOW_OTHER_DIRECTORIES = 1<<1,
> > +
> > + /* Do not include a directory that is not tracked and is empty. */
> > DIR_HIDE_EMPTY_DIRECTORIES = 1<<2,
> > +
> > + /**
> > + * If set, recurse into a directory that looks like a Git directory.
> > + * Otherwise it is shown as a directory.
> > + */
> > DIR_NO_GITLINKS = 1<<3,
> > +
> > + /**
> > + * Special mode for git-add. Return ignored files in `ignored[]` and
> > + * untracked files in `entries[]`. Only returns ignored files that match
> > + * pathspec exactly (no wildcards). Does not recurse into ignored
> > + * directories.
> > + */
> > DIR_COLLECT_IGNORED = 1<<4,
> > +
> > + /**
> > + * Similar to `DIR_SHOW_IGNORED`, but return ignored files in
> > + * `ignored[]` in addition to untracked files in `entries[]`.
> > + * This flag is mutually exclusive with `DIR_SHOW_IGNORED`.
> > + */
> > DIR_SHOW_IGNORED_TOO = 1<<5,
> > +
> > DIR_COLLECT_KILLED_ONLY = 1<<6,
> > +
> > + /**
> > + * Only has meaning if `DIR_SHOW_IGNORED_TOO` is also set; if this is
> > + * set, the untracked contents of untracked directories are also
> > + * returned in `entries[]`.
> > + */
> > DIR_KEEP_UNTRACKED_CONTENTS = 1<<7,
> > +
> > + /**
> > + * Only has meaning if `DIR_SHOW_IGNORED_TOO` is also set; if this is
> > + * set, returns ignored files and directories that match an exclude
> > + * pattern. If a directory matches an exclude pattern, then the
> > + * directory is returned and the contained paths are not. A directory
> > + * that does not match an exclude pattern will not be returned even if
> > + * all of its contents are ignored. In this case, the contents are
> > + * returned as individual entries.
> > + *
> > + * If this is set, files and directories that explicitly match an ignore
> > + * pattern are reported. Implicitly ignored directories (directories that
> > + * do not match an ignore pattern, but whose contents are all ignored)
> > + * are not reported, instead all of the contents are reported.
> > + */
> > DIR_SHOW_IGNORED_TOO_MODE_MATCHING = 1<<8,
> > +
> > DIR_SKIP_NESTED_GIT = 1<<9
> > } flags;
> > +
> > + /* An array of `struct dir_entry`, each element of which describes a path. */
> > struct dir_entry **entries;
> > +
> > + /**
> > + * used for ignored paths with the `DIR_SHOW_IGNORED_TOO` and
> > + * `DIR_COLLECT_IGNORED` flags.
> > + */
> > struct dir_entry **ignored;
> >
> > - /* Exclude info */
> > + /**
> > + * The name of the file to be read in each directory for excluded files
> > + * (typically `.gitignore`).
> > + */
> > const char *exclude_per_dir;
> >
> > /*
> > --
> > gitgitgadget
> >
Thanks,
Heba
| @@ -1,47 +0,0 @@ | |||
| setup API | |||
There was a problem hiding this comment.
On the Git mailing list, Emily Shaffer wrote (reply to this):
On Wed, Nov 06, 2019 at 09:59:37AM +0000, Heba Waly via GitGitGadget wrote:
> From: Heba Waly <heba.waly@gmail.com>
>
> Move the documentation from Documentation/technical/api-setup.txt
> to pathspec.h as it's easier for the developers to find the usage
> information beside the code instead of looking for it in another doc file.
>
> Also documentation/technical/api-setup.txt is removed because the
> information it has is now redundant and it'll be hard to keep it up to
> date and synchronized with the documentation in the header file.
>
> Signed-off-by: Heba Waly <heba.waly@gmail.com>
> ---
> Documentation/technical/api-setup.txt | 47 ---------------------------
> pathspec.h | 34 ++++++++++++++++++-
> 2 files changed, 33 insertions(+), 48 deletions(-)
> delete mode 100644 Documentation/technical/api-setup.txt
>
> diff --git a/Documentation/technical/api-setup.txt b/Documentation/technical/api-setup.txt
> deleted file mode 100644
> index eb1fa9853e..0000000000
> --- a/Documentation/technical/api-setup.txt
> +++ /dev/null
> @@ -1,47 +0,0 @@
> -setup API
> -=========
> -
> -Talk about
> -
> -* setup_git_directory()
> -* setup_git_directory_gently()
> -* is_inside_git_dir()
> -* is_inside_work_tree()
> -* setup_work_tree()
> -
> -(Dscho)
> -
> -Pathspec
> ---------
> -
> -See glossary-context.txt for the syntax of pathspec. In memory, a
I kind of miss this pointer after the change. But, maybe other folks in
the project prefer to let someone grep for this themselves.
Otherwise it looks fine to me. Thanks.
- Emily
There was a problem hiding this comment.
On the Git mailing list, Heba Waly wrote (reply to this):
On Thu, Nov 7, 2019 at 2:26 PM Emily Shaffer <emilyshaffer@google.com> wrote:
>
> On Wed, Nov 06, 2019 at 09:59:37AM +0000, Heba Waly via GitGitGadget wrote:
> > From: Heba Waly <heba.waly@gmail.com>
> >
> > Move the documentation from Documentation/technical/api-setup.txt
> > to pathspec.h as it's easier for the developers to find the usage
> > information beside the code instead of looking for it in another doc file.
> >
> > Also documentation/technical/api-setup.txt is removed because the
> > information it has is now redundant and it'll be hard to keep it up to
> > date and synchronized with the documentation in the header file.
> >
> > Signed-off-by: Heba Waly <heba.waly@gmail.com>
> > ---
> > Documentation/technical/api-setup.txt | 47 ---------------------------
> > pathspec.h | 34 ++++++++++++++++++-
> > 2 files changed, 33 insertions(+), 48 deletions(-)
> > delete mode 100644 Documentation/technical/api-setup.txt
> >
> > diff --git a/Documentation/technical/api-setup.txt b/Documentation/technical/api-setup.txt
> > deleted file mode 100644
> > index eb1fa9853e..0000000000
> > --- a/Documentation/technical/api-setup.txt
> > +++ /dev/null
> > @@ -1,47 +0,0 @@
> > -setup API
> > -=========
> > -
> > -Talk about
> > -
> > -* setup_git_directory()
> > -* setup_git_directory_gently()
> > -* is_inside_git_dir()
> > -* is_inside_work_tree()
> > -* setup_work_tree()
> > -
> > -(Dscho)
> > -
> > -Pathspec
> > ---------
> > -
> > -See glossary-context.txt for the syntax of pathspec. In memory, a
> I kind of miss this pointer after the change. But, maybe other folks in
> the project prefer to let someone grep for this themselves.
You're Emily, I'll add it.
>
> Otherwise it looks fine to me. Thanks.
>
> - Emily
Thanks a lot
| @@ -1,140 +0,0 @@ | |||
| trace API | |||
There was a problem hiding this comment.
On the Git mailing list, Emily Shaffer wrote (reply to this):
On Wed, Nov 06, 2019 at 09:59:44AM +0000, Heba Waly via GitGitGadget wrote:
> From: Heba Waly <heba.waly@gmail.com>
>
> Move the documentation from Documentation/technical/api-trace.txt
> to trace.h as it's easier for the developers to find the usage
> information beside the code instead of looking for it in another doc file.
>
> Documentation/technical/api-trace.txt is removed because the
> information it has is now redundant and it'll be hard to keep it up to
> date and synchronized with the documentation in the header file.
>
> Signed-off-by: Heba Waly <heba.waly@gmail.com>
Reviewed-by: Emily Shaffer <emilyshaffer@google.com>
| @@ -487,7 +487,7 @@ Try and run `./bin-wrappers/git psuh -h`. Your command should crash at the end. | |||
| That's because `-h` is a special case which your command should handle by | |||
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
"Heba Waly via GitGitGadget" <gitgitgadget@gmail.com> writes:
> + * Steps to parse options
> + * ----------------------
> + *
> + * - `#include "parse-options.h"`
> + *
> + * - define a NULL-terminated
> + * `static const char * const builtin_foo_usage[]` array
> + * containing alternative usage strings
> + *
> + * - define `builtin_foo_options` array as described below
> + * in section 'Data Structure'.
> + *
> + * - in `cmd_foo(int argc, const char **argv, const char *prefix)`
> + * call
> + *
> + * argc = parse_options(argc, argv, prefix, builtin_foo_options, builtin_foo_usage, flags);
> + *
> + * `parse_options()` will filter out the processed options of `argv[]` and leave the
> + * non-option arguments in `argv[]`.
> + * `argc` is updated appropriately because of the assignment.
> + *
> + * You can also pass NULL instead of a usage array as the fifth parameter of
> + * parse_options(), to avoid displaying a help screen with usage info and
> + * option list. This should only be done if necessary, e.g. to implement
> + * a limited parser for only a subset of the options that needs to be run
> + * before the full parser, which in turn shows the full help message.
After just looking at Documentation/technical/api-parse-options.txt
for real reasons (i.e. not for the purpose of reviewing this patch,
but to review a patch that uses parse-options API), I must say that
applying this change would have made the documentation quite less
useful, at least for my purpose.
My reading began by "git grep PARSE_OPT_STOP_AT_NON_OPTION" to find
the page that this patch proposes to remove, and in that document,
the list of flags the PARSE_OPT_STOP_AT_NON_OPTION belongs to comes
immediately after the above message, so it was much easier to see
where in the overall API usage the option comes into picture.
With the description moved close to enum{}, I know there would be
less risk of adding a new member without documenting it, but that is
for convenience by the developers of the parse-options API, and not
necessarily makes it convenient to learn and use the API. I would
have been forced to jump around to hunt for the necessary pieces of
info sprinkled in the header file to form the overall picture, that
a simple flat-file text document would have much easily given me.
So, I dunno.
There was a problem hiding this comment.
On the Git mailing list, Heba Waly wrote (reply to this):
On Mon, Nov 11, 2019 at 3:45 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> "Heba Waly via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > + * Steps to parse options
> > + * ----------------------
> > + *
> > + * - `#include "parse-options.h"`
> > + *
> > + * - define a NULL-terminated
> > + * `static const char * const builtin_foo_usage[]` array
> > + * containing alternative usage strings
> > + *
> > + * - define `builtin_foo_options` array as described below
> > + * in section 'Data Structure'.
> > + *
> > + * - in `cmd_foo(int argc, const char **argv, const char *prefix)`
> > + * call
> > + *
> > + * argc = parse_options(argc, argv, prefix, builtin_foo_options, builtin_foo_usage, flags);
> > + *
> > + * `parse_options()` will filter out the processed options of `argv[]` and leave the
> > + * non-option arguments in `argv[]`.
> > + * `argc` is updated appropriately because of the assignment.
> > + *
> > + * You can also pass NULL instead of a usage array as the fifth parameter of
> > + * parse_options(), to avoid displaying a help screen with usage info and
> > + * option list. This should only be done if necessary, e.g. to implement
> > + * a limited parser for only a subset of the options that needs to be run
> > + * before the full parser, which in turn shows the full help message.
>
> After just looking at Documentation/technical/api-parse-options.txt
> for real reasons (i.e. not for the purpose of reviewing this patch,
> but to review a patch that uses parse-options API), I must say that
> applying this change would have made the documentation quite less
> useful, at least for my purpose.
>
> My reading began by "git grep PARSE_OPT_STOP_AT_NON_OPTION" to find
> the page that this patch proposes to remove, and in that document,
> the list of flags the PARSE_OPT_STOP_AT_NON_OPTION belongs to comes
> immediately after the above message, so it was much easier to see
> where in the overall API usage the option comes into picture.
>
> With the description moved close to enum{}, I know there would be
> less risk of adding a new member without documenting it, but that is
> for convenience by the developers of the parse-options API, and not
> necessarily makes it convenient to learn and use the API. I would
> have been forced to jump around to hunt for the necessary pieces of
> info sprinkled in the header file to form the overall picture, that
> a simple flat-file text document would have much easily given me.
>
> So, I dunno.
Hi Junio,
I can see your point, let me share my perspective as I've thought
about that several times while moving the docs. As a new contributor
to the code base, when I come across a function or a data structure
that I want to understand what it does, I'd first try to find it in
the .h or .c files, looking for its documentation, unless I find a
link there to a file named
Documentation/technical/api-parse-options.txt I won't know about its
existence and I'll feel frustrated to find no documentation in the
code.
On the other hand some doc files had too many details for a header
file (or that's what I thought) like for example
Documentation/technical/api-trace2.txt. So the approach I took there
which is pending on review/discussions was to remove the functions
documentation from the text file to the .h file and leave the rest of
the details as is in the text file. (you can refer to this patch here
(https://public-inbox.org/git/a337f88a55ddaafd5754492ae0399137966738a8.1573507684.git.gitgitgadget@gmail.com/)).
So my proposal for this matter is to investigate the possibility of
using a doc generators that'd extract the documentations from the code
to a single doc file per library.
Thanks,
Heba
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
Heba Waly <heba.waly@gmail.com> writes:
> So my proposal for this matter is to investigate the possibility of
> using a doc generators that'd extract the documentations from the code
> to a single doc file per library.
Something like that may become necessary to bring docs in some of
the *.h files up to par with D/t/api-*.txt.
Note that the quality of the latter is quite uneven. The one I
noticed perhaps is exceptionally well-structured (even if some of
the details of its contents may have gotten stale) and to match its
structure, the order of presentation in the generated doc may have
to be different from the order of definitions in the header.
But for the ones with poor structure with stale contents, getting
rid of the stale D/t/api-*.txt and describing the API in *.h files
is a vast improvement.
There was a problem hiding this comment.
On the Git mailing list, Heba Waly wrote (reply to this):
On Tue, Nov 12, 2019 at 6:57 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Heba Waly <heba.waly@gmail.com> writes:
>
> > So my proposal for this matter is to investigate the possibility of
> > using a doc generators that'd extract the documentations from the code
> > to a single doc file per library.
>
> Something like that may become necessary to bring docs in some of
> the *.h files up to par with D/t/api-*.txt.
>
> Note that the quality of the latter is quite uneven. The one I
> noticed perhaps is exceptionally well-structured (even if some of
> the details of its contents may have gotten stale) and to match its
> structure, the order of presentation in the generated doc may have
> to be different from the order of definitions in the header.
>
> But for the ones with poor structure with stale contents, getting
> rid of the stale D/t/api-*.txt and describing the API in *.h files
> is a vast improvement.
I agree.
Back to the main issue you raised in the beginning: lets look at two
different scenarios before and after moving the docs, the first
scenario is the one you've been through trying to look for
PARSE_OPT_STOP_AT_NON_OPTION, luckily you found it in
Documentation/technical/api-parse-options.txt and the doc was pretty
useful for you. But if you were looking for PARSE_OPT_ONE_SHOT
instead, which is not documented in the doc file, you'd have ended up
in parse-options.h with no documentation, even searching for the enum
parse_opt_flags itself won't lead to anything useful I assume.
On the other hand, after moving the docs to the header file in the
last version of this patch, if we're trying to look for either
PARSE_OPT_STOP_AT_NON_OPTION or PARSE_OPT_ONE_SHOT, we'll find that
it's a member of parse_opt_flags, searching for the enum will lead us
to the explanation paragraph in the top of the header file (which was
moved from the D/t/api-parse-options.txt) which I think sounds
convenient enough as it's in the same file, and didn't go through much
trouble to find the needed info.
Happy to discuss more suggestions though.
Thanks,
Heba
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
Heba Waly <heba.waly@gmail.com> writes:
>> Note that the quality of the latter is quite uneven. The one I
>> noticed perhaps is exceptionally well-structured (even if some of
>> the details of its contents may have gotten stale) ...
> ...
> Back to the main issue you raised in the beginning: lets look at two
> different scenarios before and after moving the docs, the first
> scenario is the one you've been through trying to look for
> PARSE_OPT_STOP_AT_NON_OPTION, luckily you found it in
> Documentation/technical/api-parse-options.txt and the doc was pretty
> useful for you. But if you were looking for PARSE_OPT_ONE_SHOT
> instead, which is not documented in the doc file, you'd have ended up
> in parse-options.h with no documentation, even searching for the enum
> parse_opt_flags itself won't lead to anything useful I assume.
That is a longwinded way to say that the Doc version is outdated. I
am *not* advocating to keep it up-to-date just like the header (I've
given up on that), but if it were kept fresh, then those who looked
for ONE_SHOT would have the same ease to learn where in the larger
picture it fits, like I did with STOP_AT_NON_OPTION.
It is not like "In the header file, it is likely that we can keep
these up to date more easily. A dedicated file in Documentation/
hiearchy may be able to offer you a much better structure, but may
not describe the option at all. Which one do you want?" At least,
it should not have to be. That is what I meant when I responded to
your earlier
> So my proposal for this matter is to investigate the possibility of
> using a doc generators that'd extract the documentations from the code
> to a single doc file per library.
Extracting is just the necessary first step. It would probably need
to let us leave notes (in the header file used as the source of the
documentation) to reorder things for ease of reading through.
> last version of this patch, if we're trying to look for either
> PARSE_OPT_STOP_AT_NON_OPTION or PARSE_OPT_ONE_SHOT, we'll find that
> it's a member of parse_opt_flags, searching for the enum will lead us
> to ....
... which is what I found to be a frustrating experience of being
forced to jump around to hunt for the necessary pieces of info
sprinkled in the header file to form the overall picture, that a
simple flat-file text document would have much easily given me
(i.e. go back to my original post).
There was a problem hiding this comment.
On the Git mailing list, Emily Shaffer wrote (reply to this):
On Fri, Nov 15, 2019 at 08:37:35PM +0900, Junio C Hamano wrote:
> Heba Waly <heba.waly@gmail.com> writes:
>
> >> Note that the quality of the latter is quite uneven. The one I
> >> noticed perhaps is exceptionally well-structured (even if some of
> >> the details of its contents may have gotten stale) ...
> > ...
> > Back to the main issue you raised in the beginning: lets look at two
> > different scenarios before and after moving the docs, the first
> > scenario is the one you've been through trying to look for
> > PARSE_OPT_STOP_AT_NON_OPTION, luckily you found it in
> > Documentation/technical/api-parse-options.txt and the doc was pretty
> > useful for you. But if you were looking for PARSE_OPT_ONE_SHOT
> > instead, which is not documented in the doc file, you'd have ended up
> > in parse-options.h with no documentation, even searching for the enum
> > parse_opt_flags itself won't lead to anything useful I assume.
>
> That is a longwinded way to say that the Doc version is outdated. I
> am *not* advocating to keep it up-to-date just like the header (I've
> given up on that), but if it were kept fresh, then those who looked
> for ONE_SHOT would have the same ease to learn where in the larger
> picture it fits, like I did with STOP_AT_NON_OPTION.
>
> It is not like "In the header file, it is likely that we can keep
> these up to date more easily. A dedicated file in Documentation/
> hiearchy may be able to offer you a much better structure, but may
> not describe the option at all. Which one do you want?" At least,
> it should not have to be. That is what I meant when I responded to
> your earlier
>
> > So my proposal for this matter is to investigate the possibility of
> > using a doc generators that'd extract the documentations from the code
> > to a single doc file per library.
>
> Extracting is just the necessary first step. It would probably need
> to let us leave notes (in the header file used as the source of the
> documentation) to reorder things for ease of reading through.
I think doc generation from headers is a reasonable goal, although a
much larger one than "get rid of the worst offenders in
Documentation/technical".
>
> > last version of this patch, if we're trying to look for either
> > PARSE_OPT_STOP_AT_NON_OPTION or PARSE_OPT_ONE_SHOT, we'll find that
> > it's a member of parse_opt_flags, searching for the enum will lead us
> > to ....
>
> ... which is what I found to be a frustrating experience of being
> forced to jump around to hunt for the necessary pieces of info
> sprinkled in the header file to form the overall picture, that a
> simple flat-file text document would have much easily given me
> (i.e. go back to my original post).
For what it's worth, I tend to agree that api-parse-options.txt is a
particularly well-structured documentation page. I like that one and
refer to it often.
Having read this back and forth, my own take is something like this:
- Like Junio said, the quality of the the contents of
Documentation/technical/ ranges from useless to excellent.
- It's true that moving documentation into a header denies us some of
the nice organization of a freeform doc.
- The nice thing about headers is that we can reorganize declarations
however we want, and if doing so makes the code more readable, then
I think we should.
- But, if we won't get up to the same level of readability as the old
doc, then I don't see a reason to "throw the baby out with the
bathwater," so to speak. It seems like api-parse-options.txt is good
enough to stay.
By the way, this does make me start to wonder about the feasibility of
linking in documentation generated from code comments, or other sorts of
trickery to get the best of both worlds. But again, that's a very
different task from "get rid of misleading or useless docs". :)
For now, maybe it's enough to add a link to api-parse-options.txt at the
top of parse-options.h. Part of me wants to say maybe we should
duplicate the per-function briefs into the header too, but then we have
two sources, and a single one is easier to keep fresh.
- Emily
There was a problem hiding this comment.
On the Git mailing list, Heba Waly wrote (reply to this):
On Sat, Nov 16, 2019 at 12:28 PM Emily Shaffer <emilyshaffer@google.com> wrote:
>
> For now, maybe it's enough to add a link to api-parse-options.txt at the
> top of parse-options.h. Part of me wants to say maybe we should
> duplicate the per-function briefs into the header too, but then we have
> two sources, and a single one is easier to keep fresh.
>
Cool, I'll update this patch to keep the doc file and link to it in the header.
> - Emily
Thanks,
Heba
4c3215e
to
65120e0
Compare
HebaWaly
force-pushed
the
move-doc-to-header
branch
from
65120e0
to
e187c61
Compare
|
/submit |
|
Submitted as pull.434.v3.git.1573507684.gitgitgadget@gmail.com |
| @@ -182,267 +182,7 @@ All Trace2 API functions send a messsage to all of the active | |||
| Trace2 Targets. This section describes the set of available | |||
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
"Heba Waly via GitGitGadget" <gitgitgadget@gmail.com> writes:
> @@ -182,267 +182,7 @@ All Trace2 API functions send a messsage to all of the active
> Trace2 Targets. This section describes the set of available
> messages.
>
We said "this section describes..." and then ...
> +Refer to trace2.h for details about trace2 functions.
... say this?
There was a problem hiding this comment.
On the Git mailing list, Heba Waly wrote (reply to this):
On Tue, Nov 12, 2019 at 8:02 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> "Heba Waly via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > @@ -182,267 +182,7 @@ All Trace2 API functions send a messsage to all of the active
> > Trace2 Targets. This section describes the set of available
> > messages.
> >
>
> We said "this section describes..." and then ...
>
> > +Refer to trace2.h for details about trace2 functions.
>
> ... say this?
right, the paragraph doesn't make sense this way, I'll change it and
move the messages headers back to the text file.
Thanks,
Heba
| @@ -17,7 +17,7 @@ revision walk is used for operations like `git log`. | |||
|
|
|||
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
> diff --git a/Documentation/MyFirstObjectWalk.txt b/Documentation/MyFirstObjectWalk.txt
> index 321c0ba6a4..6629122703 100644
> --- a/Documentation/MyFirstObjectWalk.txt
> +++ b/Documentation/MyFirstObjectWalk.txt
> @@ -119,9 +119,8 @@ parameters provided by the user over the CLI.
>
> `nr` represents the number of `rev_cmdline_entry` present in the array.
>
> -`alloc` is used by the `ALLOC_GROW` macro. Check
> -`Documentation/technical/api-allocation-growing.txt` - this variable is used to
> -track the allocated size of the list.
> +`alloc` is used by the `ALLOC_GROW` macro. Check `cache.h` - this variable is
> +used to track the allocated size of the list.
Trailing whitespace.
There was a problem hiding this comment.
On the Git mailing list, Heba Waly wrote (reply to this):
On Tue, Nov 12, 2019 at 8:05 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> > diff --git a/Documentation/MyFirstObjectWalk.txt b/Documentation/MyFirstObjectWalk.txt
> > index 321c0ba6a4..6629122703 100644
> > --- a/Documentation/MyFirstObjectWalk.txt
> > +++ b/Documentation/MyFirstObjectWalk.txt
> > @@ -119,9 +119,8 @@ parameters provided by the user over the CLI.
> >
> > `nr` represents the number of `rev_cmdline_entry` present in the array.
> >
> > -`alloc` is used by the `ALLOC_GROW` macro. Check
> > -`Documentation/technical/api-allocation-growing.txt` - this variable is used to
> > -track the allocated size of the list.
> > +`alloc` is used by the `ALLOC_GROW` macro. Check `cache.h` - this variable is
> > +used to track the allocated size of the list.
>
> Trailing whitespace.
Right, thanks!
| @@ -1,174 +0,0 @@ | |||
| diff API | |||
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
"Heba Waly via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Heba Waly <heba.waly@gmail.com>
>
> Move the documentation from Documentation/technical/api-diff.txt to both
> diff.h and diffcore.h as it's easier for the developers to find the usage
> information beside the code instead of looking for it in another doc file.
>
> Also documentation/technical/api-diff.txt is removed because the information
> it has is now redundant and it'll be hard to keep it up to date and
> synchronized with the documentation in the header files.
> @@ -245,6 +370,7 @@ void diff_emit_submodule_error(struct diff_options *o, const char *err);
> void diff_emit_submodule_pipethrough(struct diff_options *o,
> const char *line, int len);
>
> +/* Output should be colored. */
I am not sure the comment belongs here. Especially if this was
lifted from the description for COLOR_DIFF.
Those preprocessor constants have long been migrated to 1-bit
bitfields in the diff_flags structure and the documentation was left
stale---description on COLOR_DIFF and friends this patch removes from
the doc should be reused to explain these fields, I would think.
Thanks.
There was a problem hiding this comment.
On the Git mailing list, Heba Waly wrote (reply to this):
On Tue, Nov 12, 2019 at 8:20 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> "Heba Waly via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > From: Heba Waly <heba.waly@gmail.com>
> >
> > Move the documentation from Documentation/technical/api-diff.txt to both
> > diff.h and diffcore.h as it's easier for the developers to find the usage
> > information beside the code instead of looking for it in another doc file.
> >
> > Also documentation/technical/api-diff.txt is removed because the information
> > it has is now redundant and it'll be hard to keep it up to date and
> > synchronized with the documentation in the header files.
>
> > @@ -245,6 +370,7 @@ void diff_emit_submodule_error(struct diff_options *o, const char *err);
> > void diff_emit_submodule_pipethrough(struct diff_options *o,
> > const char *line, int len);
> >
> > +/* Output should be colored. */
>
> I am not sure the comment belongs here. Especially if this was
> lifted from the description for COLOR_DIFF.
>
> Those preprocessor constants have long been migrated to 1-bit
> bitfields in the diff_flags structure and the documentation was left
> stale---description on COLOR_DIFF and friends this patch removes from
> the doc should be reused to explain these fields, I would think.
You're right, that comment was misplaced by mistake, it was supposed
to be a member of the diff_options structure, but that member doesn't
exist anymore, so I'll just remove this comment.
> Thanks.
Thanks,
Heba
|
This branch is now known as |
|
This patch series was integrated into pu via git@5798950. |
|
This patch series was integrated into pu via git@95f2799. |
|
This patch series was integrated into pu via git@17d5a34. |
|
This patch series was integrated into pu via git@605d053. |
Move the documentation from Documentation/technical/api-diff.txt to both diff.h and diffcore.h as it's easier for the developers to find the usage information beside the code instead of looking for it in another doc file. Also documentation/technical/api-diff.txt is removed because the information it has is now redundant and it'll be hard to keep it up to date and synchronized with the documentation in the header files. There are three members documented in the doc file that weren't found in the header files, assuming the doc wasn't up to date and the members no longer exist: touched_flags, COLOR_DIFF_WORDS and QUIET. Signed-off-by: Heba Waly <heba.waly@gmail.com>
Move the documentation from Documentation/technical/api-directory-listing.txt to dir.h as it's easier for the developers to find the usage information beside the code instead of looking for it in another doc file. Also documentation/technical/api-directory-listing.txt is removed because the information it has is now redundant and it'll be hard to keep it up to date and synchronized with the documentation in the header files. Signed-off-by: Heba Waly <heba.waly@gmail.com>
Move the documentation from Documentation/technical/api-history-graph.txt to graph.h and graph.c as it's easier for the developers to find the usage information beside the code instead of looking for it in another doc file. The graph library was already well documented, so few comments were added to both graph.h and graph.c Also documentation/technical/api-history-graph.txt is removed because the information it has is now redundant and it'll be hard to keep it up to date and synchronized with the documentation in the header file. Signed-off-by: Heba Waly <heba.waly@gmail.com>
Move the related documentation from Documentation/technical/api-merge.txt to ll-merge.h as it's easier for the developers to find the usage information beside the code instead of looking for it in another doc file. Only the ll-merge related doc is removed from documentation/technical/api-merge.txt because this information will be redundant and it'll be hard to keep it up to date and synchronized with the documentation in ll-merge.h. Signed-off-by: Heba Waly <heba.waly@gmail.com>
Move the documentation from Documentation/technical/api-oid-array.txt to sha1-array.h as it's easier for the developers to find the usage information beside the code instead of looking for it in another doc file. Also documentation/technical/api-oid-array.txt is removed because the information it has is now redundant and it'll be hard to keep it up to date and synchronized with the documentation in the header file. Signed-off-by: Heba Waly <heba.waly@gmail.com>
Move the documentation from Documentation/technical/api-remote.txt to remote.h and refspec.h as it's easier for the developers to find the usage information beside the code instead of looking for it in another doc file. N.B. The doc for both push and fetch members of the remote struct aren't moved because they are out of date, as the members were changed from arrays of rspecs to struct refspec 2 years ago. Also documentation/technical/api-remote.txt is removed because the information it has is now redundant and it'll be hard to keep it up to date and synchronized with the documentation in the header file. Signed-off-by: Heba Waly <heba.waly@gmail.com>
Move the documentation from Documentation/technical/api-ref-iteration.txt to refs.h as it's easier for the developers to find the usage information beside the code instead of looking for it in another doc file. Also documentation/technical/api-ref-iteration.txt is removed because the information it has is now redundant and it'll be hard to keep it up to date and synchronized with the documentation in the header file. Signed-off-by: Heba Waly <heba.waly@gmail.com>
Move the documentation from Documentation/technical/api-gitattributes.txt to attr.h as it's easier for the developers to find the usage information beside the code instead of looking for it in another doc file. Also documentation/technical/api-gitattributes.txt is removed because the information it has is now redundant and it'll be hard to keep it up to date and synchronized with the documentation in the header file. Signed-off-by: Heba Waly <heba.waly@gmail.com>
Move the documentation from Documentation/technical/api-revision-walking.txt to revision.h as it's easier for the developers to find the usage information beside the code instead of looking for it in another doc file. Also documentation/technical/api-revision-walking.txt is removed because the information it has is now redundant and it'll be hard to keep it up to date and synchronized with the documentation in the header file. Signed-off-by: Heba Waly <heba.waly@gmail.com>
Move the documentation from Documentation/technical/api-setup.txt to pathspec.h as it's easier for the developers to find the usage information beside the code instead of looking for it in another doc file. Also documentation/technical/api-setup.txt is removed because the information it has is now redundant and it'll be hard to keep it up to date and synchronized with the documentation in the header file. Signed-off-by: Heba Waly <heba.waly@gmail.com>
Move the documentation from Documentation/technical/api-sigchain.txt to sigchain.h as it's easier for the developers to find the usage information beside the code instead of looking for it in another doc file. Also documentation/technical/api-sigchain.txt is removed because the information it has is now redundant and it'll be hard to keep it up to date and synchronized with the documentation in the header file. Signed-off-by: Heba Waly <heba.waly@gmail.com>
Move the documentation from Documentation/technical/api-allocation-growing.txt to cache.h as it's easier for the developers to find the usage information beside the code instead of looking for it in another doc file. Also documentation/technical/api-allocation-growing.txt is removed because the information it has is now redundant and it'll be hard to keep it up to date and synchronized with the documentation in the header file. Signed-off-by: Heba Waly <heba.waly@gmail.com>
Move the documentation from Documentation/technical/api-argv-array.txt to argv-array.h as it's easier for the developers to find the usage information beside the code instead of looking for it in another doc file. Also documentation/technical/api-argv-array.txt is removed because the information it has is now redundant and it'll be hard to keep it up to date and synchronized with the documentation in the header file. Signed-off-by: Heba Waly <heba.waly@gmail.com>
Move the documentation from Documentation/technical/api-credentials.txt to credential.h as it's easier for the developers to find the usage information beside the code instead of looking for it in another doc file. Documentation/technical/api-credentials.txt is removed because the information it has is now redundant and it'll be hard to keep it up to date and synchronized with the documentation in the header file. Documentation/git-credential.txt and Documentation/gitcredentials.txt now link to credential.h instead of Documentation/technical/api-credentials.txt for details about the credetials API. Signed-off-by: Heba Waly <heba.waly@gmail.com>
HebaWaly
force-pushed
the
move-doc-to-header
branch
from
e187c61
to
5f21417
Compare
|
This patch series was integrated into pu via git@1d7946e. |
|
/submit |
|
Submitted as pull.434.v4.git.1573811626.gitgitgadget@gmail.com |
HebaWaly
force-pushed
the
move-doc-to-header
branch
from
5f21417
to
f5ecefa
Compare
Add a link to Documentation/technical/api-parse-options.txt in parse-options.h So the developers would know where to find more info about the API. Signed-off-by: Heba Waly <heba.waly@gmail.com>
Move the documentation from Documentation/technical/api-run-command.txt to run-command.h as it's easier for the developers to find the usage information beside the code instead of looking for it in another doc file. Documentation/technical/api-run-command.txt is removed because the information it has is now redundant and it'll be hard to keep it up to date and synchronized with the documentation in the header file. Signed-off-by: Heba Waly <heba.waly@gmail.com>
Move the documentation from Documentation/technical/api-trace.txt to trace.h as it's easier for the developers to find the usage information beside the code instead of looking for it in another doc file. Documentation/technical/api-trace.txt is removed because the information it has is now redundant and it'll be hard to keep it up to date and synchronized with the documentation in the header file. Signed-off-by: Heba Waly <heba.waly@gmail.com>
Move the documentation from Documentation/technical/api-tree-walking.txt to tree-walk.h as it's easier for the developers to find the usage information beside the code instead of looking for it in another doc file. Documentation/technical/api-tree-walking.txt is removed because the information it has is now redundant and it'll be hard to keep it up to date and synchronized with the documentation in the header file. Signed-off-by: Heba Waly <heba.waly@gmail.com>
Move the documentation from Documentation/technical/api-submodule-config.txt to submodule-config.h as it's easier for the developers to find the usage information beside the code instead of looking for it in another doc file. Documentation/technical/api-submodule-config.txt is removed because the information it has is now redundant and it'll be hard to keep it up to date and synchronized with the documentation in the header file. The documentation of parse_submodule_config_option() is discarded as the function was removed 2 years ago. Signed-off-by: Heba Waly <heba.waly@gmail.com>
Move the functions documentation from Documentation/technical/api-trace2.txt to trace2.h as it's easier for the developers to find the usage information beside the code instead of looking for it in another doc file. Only the functions documentation section is removed from Documentation/technical/api-trace2.txt as the file is full of details that seemed more appropriate to be in a separate doc file as it is, with a link to the doc file added in the trace2.h. Also the functions doc is removed to avoid having redundandt info which will be hard to keep syncronized with the documentation in the header file. Signed-off-by: Heba Waly <heba.waly@gmail.com>
Remove both api-index.txt and api-index-skel.txt as the API documentation is being moved to the header files, so the index is not needed anymore because the doc files (Documentation/technical/api-*.txt) will be gone. Make changes to Documentation/Makefile accordingly. Signed-off-by: Heba Waly <heba.waly@gmail.com>
HebaWaly
force-pushed
the
move-doc-to-header
branch
from
f5ecefa
to
3771f7e
Compare
|
/submit |
|
Submitted as pull.434.v5.git.1574024700.gitgitgadget@gmail.com |
|
This patch series was integrated into pu via git@267f915. |
|
@HebaWaly what is the status of this PR? Can it be closed? |
Yes, it should be in master. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Move the documentation from Documentation/technical/api-*.txt to the
corresponding header file, as it's easier for the developers to find the
usage information beside the code instead of looking for it in another
doc file.
Also documentation/technical/api-*.txt is removed because the
information it has is will be redundant and it'll be hard to keep it up to
date and synchronized with the documentation in the header files.
api-trace2.txt is not removed because it has lots of valuable information that seems more appropriate to be in a separate doc file not in the trace2.h although the functions documentation is moved to the trace2.h
api-error-handling.txt is not removed as well because no other file seemed to be more suitable for the doc it contains. I'm open to suggestions though.
The ll-merge related doc was removed from api-merge.txt to ll-merge.h, while the rest of the file is left as is.
Signed-off-by: Heba Waly heba.waly@gmail.com