| Message ID | 1333391204-16318-3-git-send-email-gerlando.falauto@keymile.com |
|---|---|
| State | Accepted |
| Commit | 348b1f1c6064990210a6797c86514fd358b73062 |
| Headers | show |
Dear Gerlando Falauto, > Add 2 new arguments to himport_r(): > > o "nvars", "vars": number and list of variables to take into account > (0 means ALL) > Looks nice and clean. But man, these patches are tough to review (not because you made bad job, just because they're hard code), lot of headache from them ;-) > NOTE: This patch does not change the current behaviour. > > Signed-off-by: Gerlando Falauto <gerlando.falauto@keymile.com> > --- > common/cmd_nvedit.c | 3 ++- > common/env_common.c | 6 ++++-- > include/search.h | 6 +++++- > lib/hashtable.c | 27 ++++++++++++++++++++++++++- > 4 files changed, 37 insertions(+), 5 deletions(-) > > diff --git a/common/cmd_nvedit.c b/common/cmd_nvedit.c > index e762e76..59668a6 100644 > --- a/common/cmd_nvedit.c > +++ b/common/cmd_nvedit.c > @@ -930,7 +930,8 @@ static int do_env_import(cmd_tbl_t *cmdtp, int flag, > addr = (char *)ep->data; > } > > - if (himport_r(&env_htab, addr, size, sep, del ? 0 : H_NOCLEAR) == 0) { > + if (himport_r(&env_htab, addr, size, sep, del ? 0 : H_NOCLEAR, > + 0, NULL) == 0) { > error("Environment import failed: errno = %d\n", errno); > return 1; > } > diff --git a/common/env_common.c b/common/env_common.c > index c33d22d..a93c062 100644 > --- a/common/env_common.c > +++ b/common/env_common.c > @@ -182,7 +182,8 @@ void set_default_env(const char *s) > } > > if (himport_r(&env_htab, (char *)default_environment, > - sizeof(default_environment), '\0', 0) == 0) > + sizeof(default_environment), '\0', 0, > + 0, NULL) == 0) > error("Environment import failed: errno = %d\n", errno); > > gd->flags |= GD_FLG_ENV_READY; > @@ -207,7 +208,8 @@ int env_import(const char *buf, int check) > } > } > > - if (himport_r(&env_htab, (char *)ep->data, ENV_SIZE, '\0', 0)) { > + if (himport_r(&env_htab, (char *)ep->data, ENV_SIZE, '\0', 0, > + 0, NULL)) { > gd->flags |= GD_FLG_ENV_READY; > return 1; > } > diff --git a/include/search.h b/include/search.h > index a4a5ef4..94d75fc 100644 > --- a/include/search.h > +++ b/include/search.h > @@ -94,9 +94,13 @@ extern ssize_t hexport_r(struct hsearch_data *__htab, > const char __sep, char **__resp, size_t __size, > int argc, char * const argv[]); > > +/* > + * nvars: length of vars array > + * vars: array of strings (variable names) to import (nvars == 0 means > all) + */ > extern int himport_r(struct hsearch_data *__htab, > const char *__env, size_t __size, const char __sep, > - int __flag); > + int __flag, int nvars, char * const vars[]); > > /* Flags for himport_r() */ > #define H_NOCLEAR (1 << 0) /* do not clear hash table before importing */ > diff --git a/lib/hashtable.c b/lib/hashtable.c > index abd61c8..0610e86 100644 > --- a/lib/hashtable.c > +++ b/lib/hashtable.c > @@ -603,6 +603,24 @@ ssize_t hexport_r(struct hsearch_data *htab, const > char sep, * himport() > */ > > +/* Check whether variable name is amongst vars[] */ > +static int is_var_in_set(const char *name, int nvars, char * const vars[]) > +{ > + int i = 0; > + > + /* No variables specified means process all of them */ > + if (nvars == 0) > + return 1; > + > + for (i = 0; i < nvars; i++) { > + if (!strcmp(name, vars[i])) > + return 1; > + } > + debug("Skipping non-listed variable %s\n", name); > + > + return 0; > +} > + > /* > * Import linearized data into hash table. > * > @@ -639,7 +657,8 @@ ssize_t hexport_r(struct hsearch_data *htab, const char > sep, */ > > int himport_r(struct hsearch_data *htab, > - const char *env, size_t size, const char sep, int flag) > + const char *env, size_t size, const char sep, int flag, > + int nvars, char * const vars[]) > { > char *data, *sp, *dp, *name, *value; > > @@ -726,6 +745,8 @@ int himport_r(struct hsearch_data *htab, > *dp++ = '\0'; /* terminate name */ > > debug("DELETE CANDIDATE: \"%s\"\n", name); > + if (!is_var_in_set(name, nvars, vars)) > + continue; > > if (hdelete_r(name, htab) == 0) > debug("DELETE ERROR ##############################\n"); > @@ -743,6 +764,10 @@ int himport_r(struct hsearch_data *htab, > *sp++ = '\0'; /* terminate value */ > ++dp; > > + /* Skip variables which are not supposed to be processed */ > + if (!is_var_in_set(name, nvars, vars)) > + continue; > + > /* enter into hash table */ > e.key = name; > e.data = value;
On 04/02/2012 08:57 PM, Marek Vasut wrote: > Dear Gerlando Falauto, > >> Add 2 new arguments to himport_r(): >> >> o "nvars", "vars": number and list of variables to take into account >> (0 means ALL) >> > > Looks nice and clean. But man, these patches are tough to review (not because > you made bad job, just because they're hard code), lot of headache from them ;-) Tell me about it. I've been reworking them several times, usually after several weeks from posting them. Long enough to have forgotten everything, and to give you a bad headache. So I really appreciate your prompt feedback, thanks! Gerlando P.S. For the whole process I am using interactive rebase, which is painful enough. If anyone knows of any other way to "patch the patches", some advice would be very much appreciated... :-) >> NOTE: This patch does not change the current behaviour. >> >> Signed-off-by: Gerlando Falauto<gerlando.falauto@keymile.com> >> --- >> common/cmd_nvedit.c | 3 ++- >> common/env_common.c | 6 ++++-- >> include/search.h | 6 +++++- >> lib/hashtable.c | 27 ++++++++++++++++++++++++++- >> 4 files changed, 37 insertions(+), 5 deletions(-) >> >> diff --git a/common/cmd_nvedit.c b/common/cmd_nvedit.c >> index e762e76..59668a6 100644 >> --- a/common/cmd_nvedit.c >> +++ b/common/cmd_nvedit.c >> @@ -930,7 +930,8 @@ static int do_env_import(cmd_tbl_t *cmdtp, int flag, >> addr = (char *)ep->data; >> } >> >> - if (himport_r(&env_htab, addr, size, sep, del ? 0 : H_NOCLEAR) == 0) { >> + if (himport_r(&env_htab, addr, size, sep, del ? 0 : H_NOCLEAR, >> + 0, NULL) == 0) { >> error("Environment import failed: errno = %d\n", errno); >> return 1; >> } >> diff --git a/common/env_common.c b/common/env_common.c >> index c33d22d..a93c062 100644 >> --- a/common/env_common.c >> +++ b/common/env_common.c >> @@ -182,7 +182,8 @@ void set_default_env(const char *s) >> } >> >> if (himport_r(&env_htab, (char *)default_environment, >> - sizeof(default_environment), '\0', 0) == 0) >> + sizeof(default_environment), '\0', 0, >> + 0, NULL) == 0) >> error("Environment import failed: errno = %d\n", errno); >> >> gd->flags |= GD_FLG_ENV_READY; >> @@ -207,7 +208,8 @@ int env_import(const char *buf, int check) >> } >> } >> >> - if (himport_r(&env_htab, (char *)ep->data, ENV_SIZE, '\0', 0)) { >> + if (himport_r(&env_htab, (char *)ep->data, ENV_SIZE, '\0', 0, >> + 0, NULL)) { >> gd->flags |= GD_FLG_ENV_READY; >> return 1; >> } >> diff --git a/include/search.h b/include/search.h >> index a4a5ef4..94d75fc 100644 >> --- a/include/search.h >> +++ b/include/search.h >> @@ -94,9 +94,13 @@ extern ssize_t hexport_r(struct hsearch_data *__htab, >> const char __sep, char **__resp, size_t __size, >> int argc, char * const argv[]); >> >> +/* >> + * nvars: length of vars array >> + * vars: array of strings (variable names) to import (nvars == 0 means >> all) + */ >> extern int himport_r(struct hsearch_data *__htab, >> const char *__env, size_t __size, const char __sep, >> - int __flag); >> + int __flag, int nvars, char * const vars[]); >> >> /* Flags for himport_r() */ >> #define H_NOCLEAR (1<< 0) /* do not clear hash table before > importing */ >> diff --git a/lib/hashtable.c b/lib/hashtable.c >> index abd61c8..0610e86 100644 >> --- a/lib/hashtable.c >> +++ b/lib/hashtable.c >> @@ -603,6 +603,24 @@ ssize_t hexport_r(struct hsearch_data *htab, const >> char sep, * himport() >> */ >> >> +/* Check whether variable name is amongst vars[] */ >> +static int is_var_in_set(const char *name, int nvars, char * const vars[]) >> +{ >> + int i = 0; >> + >> + /* No variables specified means process all of them */ >> + if (nvars == 0) >> + return 1; >> + >> + for (i = 0; i< nvars; i++) { >> + if (!strcmp(name, vars[i])) >> + return 1; >> + } >> + debug("Skipping non-listed variable %s\n", name); >> + >> + return 0; >> +} >> + >> /* >> * Import linearized data into hash table. >> * >> @@ -639,7 +657,8 @@ ssize_t hexport_r(struct hsearch_data *htab, const char >> sep, */ >> >> int himport_r(struct hsearch_data *htab, >> - const char *env, size_t size, const char sep, int flag) >> + const char *env, size_t size, const char sep, int flag, >> + int nvars, char * const vars[]) >> { >> char *data, *sp, *dp, *name, *value; >> >> @@ -726,6 +745,8 @@ int himport_r(struct hsearch_data *htab, >> *dp++ = '\0'; /* terminate name */ >> >> debug("DELETE CANDIDATE: \"%s\"\n", name); >> + if (!is_var_in_set(name, nvars, vars)) >> + continue; >> >> if (hdelete_r(name, htab) == 0) >> debug("DELETE ERROR > ##############################\n"); >> @@ -743,6 +764,10 @@ int himport_r(struct hsearch_data *htab, >> *sp++ = '\0'; /* terminate value */ >> ++dp; >> >> + /* Skip variables which are not supposed to be processed */ >> + if (!is_var_in_set(name, nvars, vars)) >> + continue; >> + >> /* enter into hash table */ >> e.key = name; >> e.data = value;
Hi Gerlando, On Mon, Apr 2, 2012 at 1:43 PM, Gerlando Falauto <gerlando.falauto@keymile.com> wrote: > On 04/02/2012 08:57 PM, Marek Vasut wrote: >> >> Dear Gerlando Falauto, >> >>> Add 2 new arguments to himport_r(): >>> >>> o "nvars", "vars": number and list of variables to take into account >>> (0 means ALL) >>> >> >> Looks nice and clean. But man, these patches are tough to review (not >> because >> you made bad job, just because they're hard code), lot of headache from >> them ;-) I certainly agree with that - I still remember this from last time :-) > > > Tell me about it. I've been reworking them several times, usually after > several weeks from posting them. Long enough to have forgotten everything, > and to give you a bad headache. > So I really appreciate your prompt feedback, thanks! > > Gerlando > > P.S. For the whole process I am using interactive rebase, which is painful > enough. If anyone knows of any other way to "patch the patches", some advice > would be very much appreciated... :-) I don't know if this helps, but I'll say it anyway. I do a lot of this :-) Normally if there is a lot of stuff I mess around with the patches on the top of the branch, with a 'wip' commit. That way I can test easily. I often have setup commits before the series and a wip one on top (patman makes it easy to skip these when sending to the mailing list). Then when I am ready to put the new changes back into the correct commits, I do a 'git rebase -i' and edit each commit. For each commit I diff if against the top of the branch but only on files changed by the *current* commit. So in the case where 25 files were changed, but only 2 were mentioned in this commit, it is likely that I only want to bring over changes in those two files into the current commit. I have an alias that does this: function next_commit_changes() { out=$(git log --numstat --pretty=format: -n1 | tail -n+2) echo "$out" | awk '{printf("%s ", $3)}' } function diff_branch() { echo "Changes in this commit:" echo git log --stat --oneline -n1 echo echo "Now performing diff against branch on those files only" # print a stat list first echo $(next_commit_changes) git diff $1 --stat -- $(next_commit_changes) # now run meld on each diff git diff $1 -- $(next_commit_changes) } # db <branch> # this is for splitting a large commit into multiple ones # Usage: # git checkout mmc2 # git rebase -i HEAD~4 # on a commit use: # db mmc2 # this will print a list of changes to the files used by THIS commit # which have been made at mmc2 alias db='diff_branch' Basically I then just use the arrows in meld to move over the changes I made into the right commit. Then 'git add -u' and 'git rebase --continue' and a line or two to the commit change log. Although I actually have two character aliases for nearly everything now... Regards, Simon > > >>> NOTE: This patch does not change the current behaviour. >>> >>> Signed-off-by: Gerlando Falauto<gerlando.falauto@keymile.com> >>> --- >>> common/cmd_nvedit.c | 3 ++- >>> common/env_common.c | 6 ++++-- >>> include/search.h | 6 +++++- >>> lib/hashtable.c | 27 ++++++++++++++++++++++++++- >>> 4 files changed, 37 insertions(+), 5 deletions(-) >>> >>> diff --git a/common/cmd_nvedit.c b/common/cmd_nvedit.c >>> index e762e76..59668a6 100644 >>> --- a/common/cmd_nvedit.c >>> +++ b/common/cmd_nvedit.c >>> @@ -930,7 +930,8 @@ static int do_env_import(cmd_tbl_t *cmdtp, int flag, >>> addr = (char *)ep->data; >>> } >>> >>> - if (himport_r(&env_htab, addr, size, sep, del ? 0 : H_NOCLEAR) == >>> 0) { >>> + if (himport_r(&env_htab, addr, size, sep, del ? 0 : H_NOCLEAR, >>> + 0, NULL) == 0) { >>> error("Environment import failed: errno = %d\n", errno); >>> return 1; >>> } >>> diff --git a/common/env_common.c b/common/env_common.c >>> index c33d22d..a93c062 100644 >>> --- a/common/env_common.c >>> +++ b/common/env_common.c >>> @@ -182,7 +182,8 @@ void set_default_env(const char *s) >>> } >>> >>> if (himport_r(&env_htab, (char *)default_environment, >>> - sizeof(default_environment), '\0', 0) == 0) >>> + sizeof(default_environment), '\0', 0, >>> + 0, NULL) == 0) >>> error("Environment import failed: errno = %d\n", errno); >>> >>> gd->flags |= GD_FLG_ENV_READY; >>> @@ -207,7 +208,8 @@ int env_import(const char *buf, int check) >>> } >>> } >>> >>> - if (himport_r(&env_htab, (char *)ep->data, ENV_SIZE, '\0', 0)) { >>> + if (himport_r(&env_htab, (char *)ep->data, ENV_SIZE, '\0', 0, >>> + 0, NULL)) { >>> gd->flags |= GD_FLG_ENV_READY; >>> return 1; >>> } >>> diff --git a/include/search.h b/include/search.h >>> index a4a5ef4..94d75fc 100644 >>> --- a/include/search.h >>> +++ b/include/search.h >>> @@ -94,9 +94,13 @@ extern ssize_t hexport_r(struct hsearch_data *__htab, >>> const char __sep, char **__resp, size_t __size, >>> int argc, char * const argv[]); >>> >>> +/* >>> + * nvars: length of vars array >>> + * vars: array of strings (variable names) to import (nvars == 0 means >>> all) + */ >>> extern int himport_r(struct hsearch_data *__htab, >>> const char *__env, size_t __size, const char __sep, >>> - int __flag); >>> + int __flag, int nvars, char * const vars[]); >>> >>> /* Flags for himport_r() */ >>> #define H_NOCLEAR (1<< 0) /* do not clear hash table before >> >> importing */ >>> >>> diff --git a/lib/hashtable.c b/lib/hashtable.c >>> index abd61c8..0610e86 100644 >>> --- a/lib/hashtable.c >>> +++ b/lib/hashtable.c >>> @@ -603,6 +603,24 @@ ssize_t hexport_r(struct hsearch_data *htab, const >>> char sep, * himport() >>> */ >>> >>> +/* Check whether variable name is amongst vars[] */ >>> +static int is_var_in_set(const char *name, int nvars, char * const >>> vars[]) >>> +{ >>> + int i = 0; >>> + >>> + /* No variables specified means process all of them */ >>> + if (nvars == 0) >>> + return 1; >>> + >>> + for (i = 0; i< nvars; i++) { >>> + if (!strcmp(name, vars[i])) >>> + return 1; >>> + } >>> + debug("Skipping non-listed variable %s\n", name); >>> + >>> + return 0; >>> +} >>> + >>> /* >>> * Import linearized data into hash table. >>> * >>> @@ -639,7 +657,8 @@ ssize_t hexport_r(struct hsearch_data *htab, const >>> char >>> sep, */ >>> >>> int himport_r(struct hsearch_data *htab, >>> - const char *env, size_t size, const char sep, int flag) >>> + const char *env, size_t size, const char sep, int flag, >>> + int nvars, char * const vars[]) >>> { >>> char *data, *sp, *dp, *name, *value; >>> >>> @@ -726,6 +745,8 @@ int himport_r(struct hsearch_data *htab, >>> *dp++ = '\0'; /* terminate name */ >>> >>> debug("DELETE CANDIDATE: \"%s\"\n", name); >>> + if (!is_var_in_set(name, nvars, vars)) >>> + continue; >>> >>> if (hdelete_r(name, htab) == 0) >>> debug("DELETE ERROR >> >> ##############################\n"); >>> >>> @@ -743,6 +764,10 @@ int himport_r(struct hsearch_data *htab, >>> *sp++ = '\0'; /* terminate value */ >>> ++dp; >>> >>> + /* Skip variables which are not supposed to be processed >>> */ >>> + if (!is_var_in_set(name, nvars, vars)) >>> + continue; >>> + >>> /* enter into hash table */ >>> e.key = name; >>> e.data = value; > >
diff --git a/common/cmd_nvedit.c b/common/cmd_nvedit.c index e762e76..59668a6 100644 --- a/common/cmd_nvedit.c +++ b/common/cmd_nvedit.c @@ -930,7 +930,8 @@ static int do_env_import(cmd_tbl_t *cmdtp, int flag, addr = (char *)ep->data; } - if (himport_r(&env_htab, addr, size, sep, del ? 0 : H_NOCLEAR) == 0) { + if (himport_r(&env_htab, addr, size, sep, del ? 0 : H_NOCLEAR, + 0, NULL) == 0) { error("Environment import failed: errno = %d\n", errno); return 1; } diff --git a/common/env_common.c b/common/env_common.c index c33d22d..a93c062 100644 --- a/common/env_common.c +++ b/common/env_common.c @@ -182,7 +182,8 @@ void set_default_env(const char *s) } if (himport_r(&env_htab, (char *)default_environment, - sizeof(default_environment), '\0', 0) == 0) + sizeof(default_environment), '\0', 0, + 0, NULL) == 0) error("Environment import failed: errno = %d\n", errno); gd->flags |= GD_FLG_ENV_READY; @@ -207,7 +208,8 @@ int env_import(const char *buf, int check) } } - if (himport_r(&env_htab, (char *)ep->data, ENV_SIZE, '\0', 0)) { + if (himport_r(&env_htab, (char *)ep->data, ENV_SIZE, '\0', 0, + 0, NULL)) { gd->flags |= GD_FLG_ENV_READY; return 1; } diff --git a/include/search.h b/include/search.h index a4a5ef4..94d75fc 100644 --- a/include/search.h +++ b/include/search.h @@ -94,9 +94,13 @@ extern ssize_t hexport_r(struct hsearch_data *__htab, const char __sep, char **__resp, size_t __size, int argc, char * const argv[]); +/* + * nvars: length of vars array + * vars: array of strings (variable names) to import (nvars == 0 means all) + */ extern int himport_r(struct hsearch_data *__htab, const char *__env, size_t __size, const char __sep, - int __flag); + int __flag, int nvars, char * const vars[]); /* Flags for himport_r() */ #define H_NOCLEAR (1 << 0) /* do not clear hash table before importing */ diff --git a/lib/hashtable.c b/lib/hashtable.c index abd61c8..0610e86 100644 --- a/lib/hashtable.c +++ b/lib/hashtable.c @@ -603,6 +603,24 @@ ssize_t hexport_r(struct hsearch_data *htab, const char sep, * himport() */ +/* Check whether variable name is amongst vars[] */ +static int is_var_in_set(const char *name, int nvars, char * const vars[]) +{ + int i = 0; + + /* No variables specified means process all of them */ + if (nvars == 0) + return 1; + + for (i = 0; i < nvars; i++) { + if (!strcmp(name, vars[i])) + return 1; + } + debug("Skipping non-listed variable %s\n", name); + + return 0; +} + /* * Import linearized data into hash table. * @@ -639,7 +657,8 @@ ssize_t hexport_r(struct hsearch_data *htab, const char sep, */ int himport_r(struct hsearch_data *htab, - const char *env, size_t size, const char sep, int flag) + const char *env, size_t size, const char sep, int flag, + int nvars, char * const vars[]) { char *data, *sp, *dp, *name, *value; @@ -726,6 +745,8 @@ int himport_r(struct hsearch_data *htab, *dp++ = '\0'; /* terminate name */ debug("DELETE CANDIDATE: \"%s\"\n", name); + if (!is_var_in_set(name, nvars, vars)) + continue; if (hdelete_r(name, htab) == 0) debug("DELETE ERROR ##############################\n"); @@ -743,6 +764,10 @@ int himport_r(struct hsearch_data *htab, *sp++ = '\0'; /* terminate value */ ++dp; + /* Skip variables which are not supposed to be processed */ + if (!is_var_in_set(name, nvars, vars)) + continue; + /* enter into hash table */ e.key = name; e.data = value;
Add 2 new arguments to himport_r(): o "nvars", "vars": number and list of variables to take into account (0 means ALL) NOTE: This patch does not change the current behaviour. Signed-off-by: Gerlando Falauto <gerlando.falauto@keymile.com> --- common/cmd_nvedit.c | 3 ++- common/env_common.c | 6 ++++-- include/search.h | 6 +++++- lib/hashtable.c | 27 ++++++++++++++++++++++++++- 4 files changed, 37 insertions(+), 5 deletions(-)