From: Junio C Hamano <gitster@pobox•com>
To: Christian Couder <christian.couder@gmail•com>
Cc: git <git@vger•kernel.org>, "Jeff King" <peff@peff•net>,
"Ævar Arnfjörð" <avarab@gmail•com>,
"Karsten Blees" <karsten.blees@gmail•com>,
"Nguyen Thai Ngoc Duy" <pclouds@gmail•com>,
"Stefan Beller" <sbeller@google•com>,
"Eric Sunshine" <sunshine@sunshineco•com>,
"Ramsay Jones" <ramsay@ramsayjones•plus.com>,
"Johannes Sixt" <j6t@kdbg•org>, "René Scharfe" <l.s.r@web•de>,
"Christian Couder" <chriscool@tuxfamily•org>
Subject: Re: [PATCH v10 33/40] environment: add set_index_file()
Date: Wed, 10 Aug 2016 10:34:02 -0700 [thread overview]
Message-ID: <xmqqlh045y0l.fsf@gitster.mtv.corp.google.com> (raw)
In-Reply-To: <CAP8UFD2ZAdUjQnO-4qnum2_AK84SfBN2_yO=py+Jj+pkV8pk-w@mail.gmail.com> (Christian Couder's message of "Wed, 10 Aug 2016 18:52:38 +0200")
Christian Couder <christian.couder@gmail•com> 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.
next prev parent reply other threads:[~2016-08-10 18:10 UTC|newest]
Thread overview: 51+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-08-08 21:02 [PATCH v10 00/40] libify apply and use lib in am, part 2 Christian Couder
2016-08-08 21:02 ` [PATCH v10 01/40] apply: make some names more specific Christian Couder
2016-08-09 14:51 ` stefan.naewe
2016-08-11 8:40 ` Christian Couder
2016-08-11 8:55 ` stefan.naewe
2016-08-08 21:02 ` [PATCH v10 02/40] apply: move 'struct apply_state' to apply.h Christian Couder
2016-08-08 21:03 ` [PATCH v10 03/40] builtin/apply: make apply_patch() return -1 or -128 instead of die()ing Christian Couder
2016-08-08 21:03 ` [PATCH v10 04/40] builtin/apply: read_patch_file() return -1 " Christian Couder
2016-08-08 21:03 ` [PATCH v10 05/40] builtin/apply: make find_header() return -128 " Christian Couder
2016-08-08 21:03 ` [PATCH v10 06/40] builtin/apply: make parse_chunk() return a negative integer on error Christian Couder
2016-08-08 21:03 ` [PATCH v10 07/40] builtin/apply: make parse_single_patch() return -1 " Christian Couder
2016-08-08 21:03 ` [PATCH v10 08/40] builtin/apply: make parse_whitespace_option() return -1 instead of die()ing Christian Couder
2016-08-08 21:03 ` [PATCH v10 09/40] builtin/apply: make parse_ignorewhitespace_option() " Christian Couder
2016-08-08 21:03 ` [PATCH v10 10/40] builtin/apply: move init_apply_state() to apply.c Christian Couder
2016-08-08 21:03 ` [PATCH v10 11/40] apply: make init_apply_state() return -1 instead of exit()ing Christian Couder
2016-08-08 21:03 ` [PATCH v10 12/40] builtin/apply: make check_apply_state() return -1 instead of die()ing Christian Couder
2016-08-08 21:03 ` [PATCH v10 13/40] builtin/apply: move check_apply_state() to apply.c Christian Couder
2016-08-08 21:03 ` [PATCH v10 14/40] builtin/apply: make apply_all_patches() return 128 or 1 on error Christian Couder
2016-08-08 21:03 ` [PATCH v10 15/40] builtin/apply: make parse_traditional_patch() return -1 " Christian Couder
2016-08-08 21:03 ` [PATCH v10 16/40] builtin/apply: make gitdiff_*() return 1 at end of header Christian Couder
2016-08-08 21:03 ` [PATCH v10 17/40] builtin/apply: make gitdiff_*() return -1 on error Christian Couder
2016-08-08 21:03 ` [PATCH v10 18/40] builtin/apply: change die_on_unsafe_path() to check_unsafe_path() Christian Couder
2016-08-08 21:03 ` [PATCH v10 19/40] builtin/apply: make build_fake_ancestor() return -1 on error Christian Couder
2016-08-08 21:03 ` [PATCH v10 20/40] builtin/apply: make remove_file() " Christian Couder
2016-08-08 21:03 ` [PATCH v10 21/40] builtin/apply: make add_conflicted_stages_file() " Christian Couder
2016-08-08 21:03 ` [PATCH v10 22/40] builtin/apply: make add_index_file() " Christian Couder
2016-08-08 21:03 ` [PATCH v10 23/40] builtin/apply: make create_file() " Christian Couder
2016-08-08 21:03 ` [PATCH v10 24/40] builtin/apply: make write_out_one_result() " Christian Couder
2016-08-08 21:03 ` [PATCH v10 25/40] builtin/apply: make write_out_results() " Christian Couder
2016-08-08 21:03 ` [PATCH v10 26/40] builtin/apply: make try_create_file() " Christian Couder
2016-08-08 21:03 ` [PATCH v10 27/40] builtin/apply: make create_one_file() " Christian Couder
2016-08-08 21:03 ` [PATCH v10 28/40] builtin/apply: rename option parsing functions Christian Couder
2016-08-09 14:55 ` stefan.naewe
2016-08-08 21:03 ` [PATCH v10 29/40] apply: rename and move opt constants to apply.h Christian Couder
2016-08-08 21:03 ` [PATCH v10 31/40] apply: make some parsing functions static again Christian Couder
2016-08-08 21:03 ` [PATCH v10 32/40] apply: use error_errno() where possible Christian Couder
2016-08-08 21:03 ` [PATCH v10 33/40] environment: add set_index_file() Christian Couder
2016-08-08 22:13 ` Junio C Hamano
2016-08-10 16:52 ` Christian Couder
2016-08-10 17:34 ` Junio C Hamano [this message]
2016-08-11 19:08 ` Christian Couder
2016-08-11 19:30 ` Junio C Hamano
2016-08-08 21:03 ` [PATCH v10 34/40] apply: make it possible to silently apply Christian Couder
2016-08-08 21:03 ` [PATCH v10 35/40] apply: don't print on stdout in verbosity_silent mode Christian Couder
2016-08-08 21:03 ` [PATCH v10 36/40] usage: add set_warn_routine() Christian Couder
2016-08-08 21:03 ` [PATCH v10 37/40] usage: add get_error_routine() and get_warn_routine() Christian Couder
2016-08-08 21:03 ` [PATCH v10 38/40] apply: change error_routine when silent Christian Couder
2016-08-08 21:03 ` [PATCH v10 39/40] apply: refactor `git apply` option parsing Christian Couder
2016-08-08 21:03 ` [PATCH v10 40/40] builtin/am: use apply api in run_apply() Christian Couder
2016-08-08 21:23 ` [PATCH v10 00/40] libify apply and use lib in am, part 2 Christian Couder
2016-08-08 22:16 ` Junio C Hamano
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=xmqqlh045y0l.fsf@gitster.mtv.corp.google.com \
--to=gitster@pobox$(echo .)com \
--cc=avarab@gmail$(echo .)com \
--cc=chriscool@tuxfamily$(echo .)org \
--cc=christian.couder@gmail$(echo .)com \
--cc=git@vger$(echo .)kernel.org \
--cc=j6t@kdbg$(echo .)org \
--cc=karsten.blees@gmail$(echo .)com \
--cc=l.s.r@web$(echo .)de \
--cc=pclouds@gmail$(echo .)com \
--cc=peff@peff$(echo .)net \
--cc=ramsay@ramsayjones$(echo .)plus.com \
--cc=sbeller@google$(echo .)com \
--cc=sunshine@sunshineco$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox