Re: [PATCH v2 6/9] commit: encapsulate determine_whence() for sequencer

From: Phillip Wood <phillip.wood123@gmail.com>
Date: Dec 18, 2019, 14:26 UTC
Message-ID: <141f95b0-cae0-06f6-2c29-618dc22ae000@gmail.com>
In-Reply-To: <xmqqwob9wbwe.fsf@gitster-ct.c.googlers.com>

On 06/12/2019 18:24, Junio C Hamano wrote:

Show 71 quoted lines

> Phillip Wood <phillip.wood123@gmail.com> writes:
> 
>> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>>
>> Working out which command wants to create a commit requires detailed
>> knowledge of the sequencer internals and that knowledge is going to
>> increase in subsequent commits. With that in mind lets encapsulate that
>> knowledge in sequencer.c rather than spreading it into builtin/commit.c.
>>
>> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
>> ---
>>   builtin/commit.c |  5 +----
>>   sequencer.c      | 13 ++++++++++++-
>>   sequencer.h      |  3 ++-
>>   3 files changed, 15 insertions(+), 6 deletions(-)
>>
>> diff --git a/builtin/commit.c b/builtin/commit.c
>> index 3b463522be..d8d4c8e419 100644
>> --- a/builtin/commit.c
>> +++ b/builtin/commit.c
>> @@ -178,10 +178,7 @@ static void determine_whence(struct wt_status *s)
>>   {
>>   	if (file_exists(git_path_merge_head(the_repository)))
>>   		whence = FROM_MERGE;
>> -	else if (file_exists(git_path_cherry_pick_head(the_repository)))
>> -		whence = file_exists(git_path_seq_dir()) ?
>> -			FROM_CHERRY_PICK_MULTI : FROM_CHERRY_PICK_SINGLE;
>> -	else
>> +	else if (!sequencer_determine_whence(the_repository, &whence))
>>   		whence = FROM_COMMIT;
>>   	if (s)
>>   		s->whence = whence;
>> diff --git a/sequencer.c b/sequencer.c
>> index 4e0370277b..98e007556c 100644
>> --- a/sequencer.c
>> +++ b/sequencer.c
>> @@ -40,7 +40,7 @@ static const char cherry_picked_prefix[] = "(cherry picked from commit ";
>>   
>>   GIT_PATH_FUNC(git_path_commit_editmsg, "COMMIT_EDITMSG")
>>   
>> -GIT_PATH_FUNC(git_path_seq_dir, "sequencer")
>> +static 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")
>> @@ -5256,3 +5256,14 @@ int todo_list_rearrange_squash(struct todo_list *todo_list)
>>   
>>   	return 0;
>>   }
>> +
>> +int sequencer_determine_whence(struct repository *r, enum commit_whence *whence)
>> +{
>> +	if (file_exists(git_path_cherry_pick_head(r))) {
>> +		*whence = file_exists(git_path_seq_dir()) ?
>> +			FROM_CHERRY_PICK_MULTI : FROM_CHERRY_PICK_SINGLE;
>> +		return 1;
>> +	}
>> +
>> +	return 0;
>> +}
> 
> I am not sure if this is a good move---determine_whence() that can
> tell not just we are in the middle of cherry-pick (either a single
> or multi) but also during a merge may be at the right abstraction
> level.  Why would we want to invent a separate function that says "I
> dunno" during a merge, instead of moving the logic for merge to the
> new helper as well?  The original determine_whence that takes
> wt_status and populates it still has to call the new helper either
> way.  Also for the matter FROM_COMMIT may also want to be part of
> the helper.  This all depends on the new callers you plan to invent,
> of course.

The idea was for determine_whence() to be able to delegate to the sequencer to ask if it is doing something without having to expose all the implementation details of that check in builtin/commit.c. It is simple enough at this stage but the next patches add more complexity which would mean exposing various sequencer state files to builtin/commit.c. This function is only meant to be called from determine_whence() - callers that want to know if any operations (merge, cherry-pick etc.) are in progress should be calling that not the function added here.

> Not part of this topic, but the call to file_exists() may want to
> become a call to dir_exists() as git-path-seq-dir is clearly a
> directory and cannot be a file, right?

Yes

Best Wishes

Phillip