Re: [PATCH 4/5] Make sequencer abort safer

From: Stephan Beyer <s-beyer@gmx.net>
Date: Dec 8, 2016, 19:17 UTC
Message-ID: <c02708de-8b47-e490-4a1e-77f5727b1156@gmx.net>
In-Reply-To: <xmqqr35itjor.fsf@gitster.mtv.corp.google.com>

Hi,

I'm a little afraid of feeding Parkinson's law of triviality here, but... ;)

On 12/08/2016 06:27 PM, Junio C Hamano wrote:

Show 20 quoted lines

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
>> On Wed, 7 Dec 2016, Stephan Beyer wrote:
>>
>>> diff --git a/sequencer.c b/sequencer.c
>>> index 30b10ba14..c9b560ac1 100644
>>> --- a/sequencer.c
>>> +++ b/sequencer.c
>>> @@ -27,6 +27,7 @@ GIT_PATH_FUNC(git_path_seq_dir, "sequencer")
>>>  static GIT_PATH_FUNC(git_path_todo_file, "sequencer/todo")
>>>  static GIT_PATH_FUNC(git_path_opts_file, "sequencer/opts")
>>>  static GIT_PATH_FUNC(git_path_head_file, "sequencer/head")
>>> +static GIT_PATH_FUNC(git_path_curr_file, "sequencer/current")
>>
>> Is it required by law to have a four-letter infix, or can we have a nicer
>> variable name (e.g. git_path_current_file)?
> 
> I agree with you that, as other git_path_*_file variables match the
> actual name on the filesystem, this one should too, together with
> the update_curr_file() function.

I totally agree with that (and I don't know why I used "curr", probably just because it looked consistent and good...).

However:

> -static void update_curr_file()
> +static void update_current_file(void)

This function name could lead to the impression that there is some current file (defined by a global state or whatever) that is updated.

So I'd rather rename the *file* to one of

 * sequencer/abort-safety (consistent to am, describes its purpose)
 * sequencer/safety (shorter, still describes the purpose)
 * sequencer/current-head (describes what it contains)
 * sequencer/last (a four-letter word, not totally unambiguous though)

> By the way, this step seems to be a fix to an existing problem, and
> the new test added in 3/5 seems to be a demonstration of the issue.
> If that is the case, shouldn't the new test initially expect failure
> and updated by this step to expect success?

That's usually a matter of taste that I sometimes also discuss with colleagues in other projects... However, for the git test suite with its "known breakage" behavior, your recommendation is surely the best way to do it (aside from introducing the test and the fix in one commit... but that does not show in the history that there actually was that breakage)

~Stephan