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

On Wed, Aug 10, 2016 at 7:34 PM, Junio C Hamano [off-list ref] wrote:

Christian Couder [off-list ref] writes:

quoted

quoted

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.