diff mbox series

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

Message ID 20191206160614.631724-7-phillip.wood123@gmail.com (mailing list archive)
State New, archived
Headers show
Series commit: fix advice for empty commits during rebases | expand

Commit Message

Phillip Wood Dec. 6, 2019, 4:06 p.m. UTC 
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(-)

Comments

Junio C Hamano Dec. 6, 2019, 6:24 p.m. UTC | #1 
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?
Phillip Wood Dec. 18, 2019, 2:26 p.m. UTC | #2 
On 06/12/2019 18:24, Junio C Hamano wrote:
> 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
diff mbox series

Patch

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;
+}
diff --git a/sequencer.h b/sequencer.h
index 56881a6baa..8d451dbfcb 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -3,12 +3,12 @@ 
 
 #include "cache.h"
 #include "strbuf.h"
+#include "wt-status.h"
 
 struct commit;
 struct repository;
 
 const char *git_path_commit_editmsg(void);
-const char *git_path_seq_dir(void);
 const char *rebase_path_todo(void);
 const char *rebase_path_todo_backup(void);
 
@@ -202,4 +202,5 @@  int write_basic_state(struct replay_opts *opts, const char *head_name,
 void sequencer_post_commit_cleanup(struct repository *r);
 int sequencer_get_last_command(struct repository* r,
 			       enum replay_action *action);
+int sequencer_determine_whence(struct repository *r, enum commit_whence *whence);
 #endif /* SEQUENCER_H */