Re: [PATCH v10 33/40] environment: add set_index_file()

[Junio C Hamano]

Christian Couder  writes:

> Yeah, it is feasible and perhaps even simpler using
> hold_lock_file_for_update() than with set_index_file(), so I
> dropped the set_index_file() patch and added a new one that uses
> hold_lock_file_for_update().

I wasn't paying too close an attention while reading the changes,
but anyway that is a great news.

Thanks.
--
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 v10 33/40] environment: add set_index_file()

[Christian Couder]

On Wed, Aug 10, 2016 at 7:34 PM, Junio C Hamano  wrote:
> Christian Couder  writes:
>
>>> Isn't the mention on NO_THE_INDEX_COMPATIBILITY_MACROS in the added
>>> comments (there are two) pure red-herring?
>>
>> Yeah, true.
>>
>> So do you want me to refactor the code to use
>> hold_lock_file_for_update() instead of hold_locked_index() and to
>> avoid the set_index_file() hack?
>
> I somehow thought that we already agreed to accept the technical
> debt, taking your "it is too much work" assessment at the face
> value.  If you now think it is feasible within the scope of the
> series, of course we'd prefer it done "right" ;-)

Yeah, it is feasible and perhaps even simpler using
hold_lock_file_for_update() than with set_index_file(), so I dropped
the set_index_file() patch and added a new one that uses
hold_lock_file_for_update().
Sorry to have understood only recently what you said in some previous
emails and thanks for the explanations.

> Is the problematic hold_locked_index() something you do yourself, or
> buried deep inside the callchain of a helper function you use?

It is something done by the apply code, so we can easily modify that.

>  I am
> sensing that it is the latter (otherwise you wouldn't be doing the
> hack or at least will trivially fixing it up in a later "now we did
> a faithful conversion from the previous code using GIT_INDEX_FILE
> enviornment variable in an earlier step. Let's clean it up by being
> in full control of what file we read from and write to" step in the
> series).

It was more a misunderstanding of how the index related functions are working.

> Do you also have an issue on the reading side (i.e. you write it out
> to temporary file and then later re-read the in-core cache from that
> temporary file, for example)?  Or is a single "write to a temporary
> index" the only thing you need to work around?

It looks like it is the latter.

Thanks,
Christian.
--
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 v10 33/40] environment: add set_index_file()

[Christian Couder]

On Tue, Aug 9, 2016 at 12:13 AM, Junio C Hamano  wrote:
> Christian Couder  writes:
>
>> Now if someone really needs to use this new function, 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);
>>
>> It is intended to be used by builtins commands, in fact only `git am`,
>> 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.
>
> That is OK, but I do not think NO_THE_INDEX_COMPATIBILITY_MACROS has
> much to do with this hack.  Even if you stop using the_index and
> have the caller pass its own temporary index_state, that structure
> does *not* know which file to read the (temporary) index from, or
> which file to write the (temporary) index to.  In fact, apply.c
> already does this in build_fake_ancestor():
>
> static int build_fake_ancestor(struct patch *list, const char *filename)
> {
> ...
> hold_lock_file_for_update(, filename, LOCK_DIE_ON_ERROR);
> res = write_locked_index(, , COMMIT_LOCK);
> ...
> }
>
> As you can see, this function works with a non-standard/default
> index file _without_ having to use non-default index_state.  What
> the set_index_file() hack allows you to do is to use interface that
> does *NOT* pass "filename" like the caller does to this function.
>
> Isn't the mention on NO_THE_INDEX_COMPATIBILITY_MACROS in the added
> comments (there are two) pure red-herring?

Yeah, true.

So do you want me to refactor the code to use
hold_lock_file_for_update() instead of hold_locked_index() and to
avoid the set_index_file() hack?

Or would the set_index_file() hack be ok with a commit message like
the following:

---
Introduce set_index_file() to be able to temporarily change the
index file.

Yeah, this is a short cut and this new function should not be used
by other code.

It adds a small technical debt, that could perhaps be avoided with a
refactoring and by using hold_lock_file_for_update() instead of
hold_locked_index() to pass the filename where the index should be
written.

Now if someone really needs to use this new function, 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);

It is intended to be used by builtins commands, in fact only `git am`,
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.
---

And with comments like this on top of set_index_file() definition and
declaration:

/*
 * Hack to temporarily change the index.
 * Yeah, the libification of 'apply' took a short-circuit by adding
 * this technical debt.
 * Please set the filename using for example hold_lock_file_for_update(),
 * instead of this function.
 * If you really need to use this function, please save the current
 * index file using get_index_file() before changing the index
 * file. And when finished, reset it to the saved value.
 */

?
--
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 v10 33/40] environment: add set_index_file()

[Junio C Hamano]

Christian Couder  writes:

>> Isn't the mention on NO_THE_INDEX_COMPATIBILITY_MACROS in the added
>> comments (there are two) pure red-herring?
>
> Yeah, true.
>
> So do you want me to refactor the code to use
> hold_lock_file_for_update() instead of hold_locked_index() and to
> avoid the set_index_file() hack?

I somehow thought that we already agreed to accept the technical
debt, taking your "it is too much work" assessment at the face
value.  If you now think it is feasible within the scope of the
series, of course we'd prefer it done "right" ;-)

> Or would the set_index_file() hack be ok with a commit message like
> the following:
>
> ---
> Introduce set_index_file() to be able to temporarily change the
> index file.
>
> Yeah, this is a short cut and this new function should not be used
> by other code.
>
> It adds a small technical debt, that could perhaps be avoided with a
> refactoring and by using hold_lock_file_for_update() instead of
> hold_locked_index() to pass the filename where the index should be
> written.

Is the problematic hold_locked_index() something you do yourself, or
buried deep inside the callchain of a helper function you use?  I am
sensing that it is the latter (otherwise you wouldn't be doing the
hack or at least will trivially fixing it up in a later "now we did
a faithful conversion from the previous code using GIT_INDEX_FILE
enviornment variable in an earlier step. Let's clean it up by being
in full control of what file we read from and write to" step in the
series).

Do you also have an issue on the reading side (i.e. you write it out
to temporary file and then later re-read the in-core cache from that
temporary file, for example)?  Or is a single "write to a temporary
index" the only thing you need to work around?  

The "Yeah, this is a short cut ..." admission of guilt is much much
less interesting than showing later people a way forward when they
help us by cleaning up the mess we decided to leave behind for
expediency.  I am not seeing that "here are the callchains that need
to be proper refactored, if we want to avoid this hack" yet; but
that is what would help them.

Thanks.

--
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 v10 33/40] environment: add set_index_file()

[Junio C Hamano]

Christian Couder  writes:

> Now if someone really needs to use this new function, 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);
>
> It is intended to be used by builtins commands, in fact only `git am`,
> 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.

That is OK, but I do not think NO_THE_INDEX_COMPATIBILITY_MACROS has
much to do with this hack.  Even if you stop using the_index and
have the caller pass its own temporary index_state, that structure
does *not* know which file to read the (temporary) index from, or
which file to write the (temporary) index to.  In fact, apply.c
already does this in build_fake_ancestor():

static int build_fake_ancestor(struct patch *list, const char *filename)
{
...
hold_lock_file_for_update(, filename, LOCK_DIE_ON_ERROR);
res = write_locked_index(, , COMMIT_LOCK);
...
}

As you can see, this function works with a non-standard/default
index file _without_ having to use non-default index_state.  What
the set_index_file() hack allows you to do is to use interface that
does *NOT* pass "filename" like the caller does to this function.

Isn't the mention on NO_THE_INDEX_COMPATIBILITY_MACROS in the added
comments (there are two) pure red-herring?
--
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

[PATCH v10 33/40] environment: add set_index_file()

[Christian Couder]

Introduce set_index_file() to be able to temporarily change the
index file.

Yeah, this is a short cut and this new function should not be used
by other code.

It adds a small technical debt, because unfortunately a very big
technical debt already exists as the apply code and a lot of other
"libified" code already call functions like read_cache(),
discard_cache() and cache_name_pos() instead of functions like
read_index_from(), discard_index() and index_name_pos() that are
available when the NO_THE_INDEX_COMPATIBILITY_MACROS env variable
is defined.

Avoiding this small new technical debt is unfortunately not as simple
as just changing these functions in "apply.c", as these functions can
be called by other "libified" code that can indirectly be called by
apply code.

For example cache_name_pos() is used in "dir.c" and "diff.c", and then
"dir.h" and "diff.h" are included in many other "libified" *.c files
(including "apply.c"). So it is very difficult to make sure that apply
code doesn't indirectly call any of the problematic functions.

And even if it was possible to make sure that now "apply.c" doesn't
indirectly call any of these functions, how could we make sure that
later a refactoring in other "libified" code will not change a
function that is indirectly called by "apply.c" to make it call another
function that indirectly calls a problematic function?

So it's a different project altogether to remove calls to problematic
functions (like read_cache(), discard_cache(), cache_name_pos() and so
on) in all the libified code, starting maybe with "dir.c" and "diff.c".

Now if someone really needs to use this new function, 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);

It is intended to be used by builtins commands, in fact only `git am`,
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.

Helped-by: Stefan Beller 
Signed-off-by: Christian Couder 
---
 cache.h   | 13 +
 environment.c | 16 
 2 files changed, 29 insertions(+)

diff --git a/cache.h b/cache.h
index b5f76a4..c9ad7f9 100644
--- a/cache.h
+++ b/cache.h
@@ -471,6 +471,19 @@ extern const char *strip_namespace(const char 
*namespaced_ref);
 extern const char *get_git_work_tree(void);
 
 /*
+ * Hack to temporarily change the index.
+ * Yeah, the libification of 'apply' took a short-circuit by adding
+ * this technical debt.
+ * Please use functions available when
+ * NO_THE_INDEX_COMPATIBILITY_MACROS is defined, instead of this
+ * function.
+ * If you really need to use this function, please save the current
+ * index file using get_index_file() before changing the index
+ * file. And when finished, reset it to the saved value.
+ */
+extern void set_index_file(char *index_file);
+
+/*
  * Return true if the given path is a git directory; note that this _just_
  * looks at the directory itself. If you want to know whether "foo/.git"
  * is a repository, you must feed that path, not just "foo".
diff --git a/environment.c b/environment.c
index ca72464..55b2b6b 100644
--- a/environment.c
+++ b/environment.c
@@ -292,6 +292,22 @@ int odb_pack_keep(char *name, size_t namesz, const 
unsigned char *sha1)
return open(name, O_RDWR|O_CREAT|O_EXCL, 0600);
 }
 
+/*
+ * Hack to temporarily change the index.
+ * Yeah, the libification of 'apply' took a short-circuit by adding
+ * this technical debt.
+ * Please use functions available when
+ * NO_THE_INDEX_COMPATIBILITY_MACROS is defined, instead of this
+ * function.
+ * If you really need to use this function, 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)
-- 
2.9.2.614.g4980f51

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