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

[Junio C Hamano <gitster@pobox.com>]

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.

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?