Re: [PATCH v8 32/41] environment: add set_index_file()

[Junio C Hamano]

Duy Nguyen  writes:

> On Fri, Jul 29, 2016 at 8:23 PM, Christian Couder
> 
>> Do you mean that it might be a source of micro-projects for the next
>> GSoC or Outreachy? ;-)
>
> No that's what I meant by boring. This is not something to interest
> GSoC candidates, and while it looks simple, sometimes it needs a good
> understanding of git overall, and it's definitely not small enough for
> a micro project.

I think "that's not what I meant" is what you meant ;-) but anyway,
I actually view that as part of the same "libify" project, and not
solving it and building an "am that makes an internal call to apply
that is not libified" is adding technical debt to the codebase.

It may be a good trade-off between having "am that internally calls
apply" earlier and the additional technical debt, but is not a good
thing to do to the overall health of the project in the longer term
to pretend as if this new set_index_file() is part of a good API.

Instead it probably should be marked with "the libification of
'apply' took a short-circuit by adding this technical debt; please
do not call this function in new codepaths", or something like that.



--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v8 32/41] environment: add set_index_file()

[Duy Nguyen]

On Fri, Jul 29, 2016 at 8:23 PM, Christian Couder
 wrote:
> On Fri, Jul 29, 2016 at 5:34 PM, Duy Nguyen  wrote:
>>
>> Yeah. If the libification movement is going strong, we can start
>> converting and at some point should be able to define
>> NO_THE_INDEX_COMPATIBILITY_MACROS globally (and avoid the_index along
>> the way)
>>
>> Without that, there is a risk. I looked at 'nm apply.o | grep ' [Uu]
>> '' and I don't see any external functions that would potentially
>> access the index, except ll_merge. Again there's a good chance I may
>> have missed something.
>>
>>> So it looks like it is a very big and different project to make the
>>> current libified code be explicit about which index it is using.
>>> And by the way perhaps this other project should just remove the
>>> "the_index" global altogether.
>>
>> This is probably the way to go. But it's the boring sort of work that
>> nobody wants to do :(
>
> Do you mean that it might be a source of micro-projects for the next
> GSoC or Outreachy? ;-)

No that's what I meant by boring. This is not something to interest
GSoC candidates, and while it looks simple, sometimes it needs a good
understanding of git overall, and it's definitely not small enough for
a micro project. It's very similar to i18n-izing the code base.
Luckily Vasco Almeida came out of nowhere and did (still do) that.
There may be another Vasco somewhere ;-)
-- 
Duy
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v8 32/41] environment: add set_index_file()

[Christian Couder]

On Fri, Jul 29, 2016 at 5:34 PM, Duy Nguyen  wrote:
>
> Yeah. If the libification movement is going strong, we can start
> converting and at some point should be able to define
> NO_THE_INDEX_COMPATIBILITY_MACROS globally (and avoid the_index along
> the way)
>
> Without that, there is a risk. I looked at 'nm apply.o | grep ' [Uu]
> '' and I don't see any external functions that would potentially
> access the index, except ll_merge. Again there's a good chance I may
> have missed something.
>
>> So it looks like it is a very big and different project to make the
>> current libified code be explicit about which index it is using.
>> And by the way perhaps this other project should just remove the
>> "the_index" global altogether.
>
> This is probably the way to go. But it's the boring sort of work that
> nobody wants to do :(

Do you mean that it might be a source of micro-projects for the next
GSoC or Outreachy? ;-)
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v8 32/41] environment: add set_index_file()

[Duy Nguyen]

On Fri, Jul 29, 2016 at 4:21 PM, Christian Couder
 wrote:
>> I agree we should avoid this. There's a bunch of cache_name_pos() (and
>> even read_cache()) in the libified apply.c, those will need to be
>> converted  to take struct index_state* directly (read_index_from or
>> index_name_pos).
>
> There is a lot of other "libified" code that is calling these functions:
>
> $ git grep -l cache_name_pos -- '*.c' | grep -v builtin
> apply.c
> diff.c
> dir.c
> merge-recursive.c
> pathspec.c
> rerere.c
> sha1_name.c
> submodule.c
> wt-status.c

Irrelevant?

> $ git grep -l read_cache -- '*.c' | grep -v builtin | egrep -v '^t/'
> apply.c
> check-racy.c
> diff.c
> dir.c
> merge-recursive.c
> merge.c
> read-cache.c
> rerere.c
> revision.c
> sequencer.c
> sha1_name.c
> submodule.c
>
> and some of that code is perhaps directly and indirectly called by the
> libified apply code.

Yeah. If the libification movement is going strong, we can start
converting and at some point should be able to define
NO_THE_INDEX_COMPATIBILITY_MACROS globally (and avoid the_index along
the way)

Without that, there is a risk. I looked at 'nm apply.o | grep ' [Uu]
'' and I don't see any external functions that would potentially
access the index, except ll_merge. Again there's a good chance I may
have missed something.

> So it looks like it is a very big and different project to make the
> current libified code be explicit about which index it is using.
> And by the way perhaps this other project should just remove the
> "the_index" global altogether.

This is probably the way to go. But it's the boring sort of work that
nobody wants to do :(
-- 
Duy
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v8 32/41] environment: add set_index_file()

[Christian Couder]

On Wed, Jul 27, 2016 at 5:14 PM, Duy Nguyen  wrote:
> On Tue, Jul 26, 2016 at 9:28 PM, Junio C Hamano  wrote:
>> Christian Couder  writes:
>>
>>> Introduce set_index_file() to be able to temporarily change the index file.
>>>
>>> It should be used like this:
>>>
>>> /* Save current index file */
>>> old_index_file = get_index_file();
>>> set_index_file((char *)tmp_index_file);
>>>
>>> /* Do stuff that will use tmp_index_file as the index file */
>>> ...
>>>
>>> /* When finished reset the index file */
>>> set_index_file(old_index_file);
>>
>> WHY is glaringly missing.
>
> It's used in a4aaa23 (builtin/am: use apply api in run_apply() -
> 2016-06-27) as a straight conversion of "GIT_INDEX_FILE=xxx git apply"
> .

Yeah, I would add something like the following to clarify the WHY:

"It is intended to be used by builtins commands to temporarily change the
index file used by libified code.

This is useful when libified code uses the global index, but a builtin
command wants another index file to be used instead."

>> Doesn't calling this have a global effect that is flowned upon when
>> used by a library-ish function?  Who is the expected caller in this
>> series that wants to "libify" a part of the system?

The expected caller is a builtin, like "builtin/am.c".

> I agree we should avoid this. There's a bunch of cache_name_pos() (and
> even read_cache()) in the libified apply.c, those will need to be
> converted  to take struct index_state* directly (read_index_from or
> index_name_pos).

There is a lot of other "libified" code that is calling these functions:

$ git grep -l cache_name_pos -- '*.c' | grep -v builtin
apply.c
diff.c
dir.c
merge-recursive.c
pathspec.c
rerere.c
sha1_name.c
submodule.c
wt-status.c

$ git grep -l read_cache -- '*.c' | grep -v builtin | egrep -v '^t/'
apply.c
check-racy.c
diff.c
dir.c
merge-recursive.c
merge.c
read-cache.c
rerere.c
revision.c
sequencer.c
sha1_name.c
submodule.c

and some of that code is perhaps directly and indirectly called by the
libified apply code.

Also I am not sure that read_cache() and cache_name_pos() are the only
functions that should be changed.

So it looks like it is a very big and different project to make the
current libified code be explicit about which index it is using.
And by the way perhaps this other project should just remove the
"the_index" global altogether.

> But at least read-cache.c has supported multiple
> indexes for a long time.

This is great, but it is unfortunately not the only lib file involved.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v8 32/41] environment: add set_index_file()

[Duy Nguyen]

On Tue, Jul 26, 2016 at 9:28 PM, Junio C Hamano  wrote:
> Christian Couder  writes:
>
>> Introduce set_index_file() to be able to temporarily change the index file.
>>
>> It should be used like this:
>>
>> /* Save current index file */
>> old_index_file = get_index_file();
>> set_index_file((char *)tmp_index_file);
>>
>> /* Do stuff that will use tmp_index_file as the index file */
>> ...
>>
>> /* When finished reset the index file */
>> set_index_file(old_index_file);
>
> WHY is glaringly missing.

It's used in a4aaa23 (builtin/am: use apply api in run_apply() -
2016-06-27) as a straight conversion of "GIT_INDEX_FILE=xxx git apply"
.

> Doesn't calling this have a global effect that is flowned upon when
> used by a library-ish function?  Who is the expected caller in this
> series that wants to "libify" a part of the system?

I agree we should avoid this. There's a bunch of cache_name_pos() (and
even read_cache()) in the libified apply.c, those will need to be
converted  to take struct index_state* directly (read_index_from or
index_name_pos). But at least read-cache.c has supported multiple
indexes for a long time.
-- 
Duy
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v8 32/41] environment: add set_index_file()

[Junio C Hamano]

Christian Couder  writes:

> Introduce set_index_file() to be able to temporarily change the index file.
>
> It should be used like this:
>
> /* Save current index file */
> old_index_file = get_index_file();
> set_index_file((char *)tmp_index_file);
>
> /* Do stuff that will use tmp_index_file as the index file */
> ...
>
> /* When finished reset the index file */
> set_index_file(old_index_file);

WHY is glaringly missing.

> Signed-off-by: Christian Couder 
> ---

Doesn't calling this have a global effect that is flowned upon when
used by a library-ish function?  Who is the expected caller in this
series that wants to "libify" a part of the system?

>  cache.h   |  1 +
>  environment.c | 10 ++
>  2 files changed, 11 insertions(+)
>
> diff --git a/cache.h b/cache.h
> index c73becb..8854365 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -461,6 +461,7 @@ extern int is_inside_work_tree(void);
>  extern const char *get_git_dir(void);
>  extern const char *get_git_common_dir(void);
>  extern char *get_object_directory(void);
> +extern void set_index_file(char *index_file);
>  extern char *get_index_file(void);
>  extern char *get_graft_file(void);
>  extern int set_git_dir(const char *path);
> diff --git a/environment.c b/environment.c
> index ca72464..7a53799 100644
> --- a/environment.c
> +++ b/environment.c
> @@ -292,6 +292,16 @@ int odb_pack_keep(char *name, size_t namesz, const 
> unsigned char *sha1)
>   return open(name, O_RDWR|O_CREAT|O_EXCL, 0600);
>  }
>  
> +/*
> + * Temporarily change the index file.
> + * Please save the current index file using get_index_file() before changing
> + * the index file. And when finished, reset it to the saved value.
> + */
> +void set_index_file(char *index_file)
> +{
> + git_index_file = index_file;
> +}
> +
>  char *get_index_file(void)
>  {
>   if (!git_index_file)
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html