[U-Boot] [PATCH v3 2/6] env: make himport_r() selective on variables

Simon Glass
2012-04-04T07:41:16Z

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;
>
>