Re: [RFC PATCH 2/8] commit: move code to update HEAD to libgit

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 07/10/17 10:54, Junio C Hamano wrote:
> Phillip Wood <phillip.wood@xxxxxxxxxxxx> writes:
> 
>> From: Phillip Wood <phillip.wood@xxxxxxxxxxxxx>
>>
>> Signed-off-by: Phillip Wood <phillip.wood@xxxxxxxxxxxxx>
>> ---
> 
> This seems to do a lot more than just moving code, most notably, it
> uses setenv() to affect what happens in any subprocesses we may
> spawn, and it is unclear if it was verified that this patch is free
> of unwanted consequences due to that change (and any others I may
> have missed while reading this patch, if any).
>
> I suspect that it would be sufficient to make update_head() helper
> function take the reflog action message as another parameter
> instead to fix the above, but there may be other reasons why you
> chose to do it this way---I cannot read it in your empty log
> message, though.

Good point, sorry I should have added some explanation about that. I
went with using setenv() rather than passing a reflog message to
update_head() as it meant there were no changes needed on the sequencer
side as it already sets GIT_REFLOG_ACTION. As the sequencer already sets
GIT_REFLOG_ACTION, and git-commit does not fork any subprocesses I don't
think this change has any unwanted consequences (I pushed a branch to
github before submitting the patches and the test suite passes on
travis). It would however be clearer to add a parameter to update_head()
for the reflog message as you suggested.

> 
> I will not give line-by-line style nitpick but in general we do not
> leave a SP between function name and the open parenthesis that
> starts its argument list.  New code in this patch seems to use
> mixture of styles.

Sorry I should have spotted those before I posted this series, I go
though all the patches and fix them (this would be a good opportunity
for me to try using git-clang-format from next)

Thanks for looking at this, did you have time to look at the other
changes in this series or did this patch put you off looking further?
I'll update and repost probably towards the end of next week. If I
continue to base these patches on master then I think the patch that
moves the code to print the commit summary will have (trivial) conflicts
with the changes in ao/check-resolve-ref-unsafe-result in pu do you want
the new patches based pu or are you happy with them based on master?

Best Wishes

Phillip

>> diff --git a/builtin/commit.c b/builtin/commit.c
>> index 0b8c1ef6f57cfed328d12255e6834adb4bda4137..497778ba2c02afdd4a337969a27ca781e8389040 100644
>> --- a/builtin/commit.c
>> +++ b/builtin/commit.c
>> @@ -1578,13 +1578,11 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
>>  	struct strbuf sb = STRBUF_INIT;
>>  	struct strbuf author_ident = STRBUF_INIT;
>>  	const char *index_file, *reflog_msg;
>> -	char *nl;
>>  	struct object_id oid;
>>  	struct commit_list *parents = NULL;
>>  	struct stat statbuf;
>>  	struct commit *current_head = NULL;
>>  	struct commit_extra_header *extra = NULL;
>> -	struct ref_transaction *transaction;
>>  	struct strbuf err = STRBUF_INIT;
>>  
>>  	if (argc == 2 && !strcmp(argv[1], "-h"))
>> @@ -1625,10 +1623,10 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
>>  	reflog_msg = getenv("GIT_REFLOG_ACTION");
>>  	if (!current_head) {
>>  		if (!reflog_msg)
>> -			reflog_msg = "commit (initial)";
>> +			setenv ("GIT_REFLOG_ACTION", "commit (initial)", 1);
>>  	} else if (amend) {
>>  		if (!reflog_msg)
>> -			reflog_msg = "commit (amend)";
>> +			setenv("GIT_REFLOG_ACTION", "commit (amend)", 1);
>>  		parents = copy_commit_list(current_head->parents);
>>  	} else if (whence == FROM_MERGE) {
>>  		struct strbuf m = STRBUF_INIT;
>> @@ -1637,7 +1635,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
>>  		struct commit_list **pptr = &parents;
>>  
>>  		if (!reflog_msg)
>> -			reflog_msg = "commit (merge)";
>> +			setenv("GIT_REFLOG_ACTION", "commit (merge)", 1);
>>  		pptr = commit_list_append(current_head, pptr);
>>  		fp = xfopen(git_path_merge_head(), "r");
>>  		while (strbuf_getline_lf(&m, fp) != EOF) {
>> @@ -1660,9 +1658,9 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
>>  			parents = reduce_heads(parents);
>>  	} else {
>>  		if (!reflog_msg)
>> -			reflog_msg = (whence == FROM_CHERRY_PICK)
>> -					? "commit (cherry-pick)"
>> -					: "commit";
>> +			setenv("GIT_REFLOG_ACTION", (whence == FROM_CHERRY_PICK)
>> +						? "commit (cherry-pick)"
>> +						: "commit", 1);
>>  		commit_list_insert(current_head, &parents);
>>  	}
>>  
>> @@ -1707,25 +1705,10 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
>>  	strbuf_release(&author_ident);
>>  	free_commit_extra_headers(extra);
>>  
>> -	nl = strchr(sb.buf, '\n');
>> -	if (nl)
>> -		strbuf_setlen(&sb, nl + 1 - sb.buf);
>> -	else
>> -		strbuf_addch(&sb, '\n');
>> -	strbuf_insert(&sb, 0, reflog_msg, strlen(reflog_msg));
>> -	strbuf_insert(&sb, strlen(reflog_msg), ": ", 2);
>> -
>> -	transaction = ref_transaction_begin(&err);
>> -	if (!transaction ||
>> -	    ref_transaction_update(transaction, "HEAD", oid.hash,
>> -				   current_head
>> -				   ? current_head->object.oid.hash : null_sha1,
>> -				   0, sb.buf, &err) ||
>> -	    ref_transaction_commit(transaction, &err)) {
>> +	if (update_head (current_head, &oid, &sb, &err)) {
>>  		rollback_index_files();
>>  		die("%s", err.buf);
>>  	}
>> -	ref_transaction_free(transaction);
>>  
>>  	unlink(git_path_cherry_pick_head());
>>  	unlink(git_path_revert_head());
>> diff --git a/sequencer.c b/sequencer.c
>> index 319208afb3de36c97b6c62d4ecf6e641245e7a54..917ad4a16216b30adb2c2c9650217926d8db8ba7 100644
>> --- a/sequencer.c
>> +++ b/sequencer.c
>> @@ -1,10 +1,10 @@
>>  #include "cache.h"
>>  #include "config.h"
>>  #include "lockfile.h"
>> -#include "sequencer.h"
>>  #include "dir.h"
>>  #include "object.h"
>>  #include "commit.h"
>> +#include "sequencer.h"
>>  #include "tag.h"
>>  #include "run-command.h"
>>  #include "exec_cmd.h"
>> @@ -750,6 +750,43 @@ int template_untouched(const struct strbuf *sb, const char *template_file,
>>  	return rest_is_empty(sb, start - sb->buf);
>>  }
>>  
>> +int update_head(const struct commit *old_head, const struct object_id *new_head,
>> +		const struct strbuf *msg, struct strbuf *err)
>> +{
>> +	struct ref_transaction *transaction;
>> +	struct strbuf sb = STRBUF_INIT;
>> +	const char *nl, *reflog_msg;
>> +	int ret = 0;
>> +
>> +	reflog_msg = getenv("GIT_REFLOG_ACTION");
>> +	if (!reflog_msg)
>> +		reflog_msg="";
>> +
>> +	nl = strchr(msg->buf, '\n');
>> +	if (nl) {
>> +		strbuf_add(&sb, msg->buf, nl + 1 - msg->buf);
>> +	} else {
>> +		strbuf_addbuf(&sb, msg);
>> +		strbuf_addch(&sb, '\n');
>> +	}
>> +	strbuf_insert(&sb, 0, reflog_msg, strlen(reflog_msg));
>> +	strbuf_insert(&sb, strlen(reflog_msg), ": ", 2);
>> +
>> +	transaction = ref_transaction_begin(err);
>> +	if (!transaction ||
>> +	    ref_transaction_update(transaction, "HEAD", new_head->hash,
>> +				   old_head
>> +				   ? old_head->object.oid.hash : null_sha1,
>> +				   0, sb.buf, err) ||
>> +	    ref_transaction_commit(transaction, err)) {
>> +		ret = -1;
>> +	}
>> +	ref_transaction_free(transaction);
>> +	strbuf_release(&sb);
>> +
>> +	return ret;
>> +}
>> +
>>  static int is_original_commit_empty(struct commit *commit)
>>  {
>>  	const struct object_id *ptree_oid;
>> diff --git a/sequencer.h b/sequencer.h
>> index dd071cfcd82d165bd23726814b74cbf3384e1a17..87edf40e5274d59f48d5af57678100ea220d2c8a 100644
>> --- a/sequencer.h
>> +++ b/sequencer.h
>> @@ -60,4 +60,6 @@ enum cleanup_mode {
>>  int message_is_empty(const struct strbuf *sb, enum cleanup_mode cleanup_mode);
>>  int template_untouched(const struct strbuf *sb, const char *template_file,
>>  		       enum cleanup_mode cleanup_mode);
>> +int update_head(const struct commit *old_head, const struct object_id *new_head,
>> +		const struct strbuf *msg, struct strbuf *err);
>>  #endif




[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]

  Powered by Linux