git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH 07/11] commit.c: use ref transactions for updates

From
Michael Haggerty <mhagger@alum.mit.edu>
Date
Apr 19, 2014, 19:23 UTC
Message-ID
<5352CD14.4060808@alum.mit.edu>
In-Reply-To
<1397763987-4453-8-git-send-email-sahlberg@google.com>
On 04/17/2014 09:46 PM, Ronnie Sahlberg wrote:
Show 33 quoted lines
> Change commit.c to use ref transactions for all ref updates.
> 
> Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
> ---
>  builtin/commit.c | 22 ++++++++++++----------
>  1 file changed, 12 insertions(+), 10 deletions(-)
> 
> diff --git a/builtin/commit.c b/builtin/commit.c
> index d9550c5..b8e4389 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -1541,11 +1541,11 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
>  	const char *index_file, *reflog_msg;
>  	char *nl;
>  	unsigned char sha1[20];
> -	struct ref_lock *ref_lock;
>  	struct commit_list *parents = NULL, **pptr = &parents;
>  	struct stat statbuf;
>  	struct commit *current_head = NULL;
>  	struct commit_extra_header *extra = NULL;
> +	struct ref_transaction *transaction;
>  
>  	if (argc == 2 && !strcmp(argv[1], "-h"))
>  		usage_with_options(builtin_commit_usage, builtin_commit_options);
> @@ -1667,12 +1667,6 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
>  	strbuf_release(&author_ident);
>  	free_commit_extra_headers(extra);
>  
> -	ref_lock = lock_any_ref_for_update("HEAD",
> -					   !current_head
> -					   ? NULL
> -					   : current_head->object.sha1,
> -					   0, NULL);
The old version, above, contemplates that current_head might be NULL...
Show 19 quoted lines
> -
>  	nl = strchr(sb.buf, '\n');
>  	if (nl)
>  		strbuf_setlen(&sb, nl + 1 - sb.buf);
> @@ -1681,14 +1675,22 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
>  	strbuf_insert(&sb, 0, reflog_msg, strlen(reflog_msg));
>  	strbuf_insert(&sb, strlen(reflog_msg), ": ", 2);
>  
> -	if (!ref_lock) {
> +	transaction = ref_transaction_begin();
> +	if (!transaction) {
>  		rollback_index_files();
> -		die(_("cannot lock HEAD ref"));
> +		die(_("HEAD: cannot start transaction"));
>  	}
> -	if (write_ref_sha1(ref_lock, sha1, sb.buf) < 0) {
> +	if (ref_transaction_update(transaction, "HEAD", sha1,
> +				   current_head->object.sha1,
> +				   0, !!current_head)) {
...but here you dereference current_head without checking it first.

It upsets me that the test suite didn't catch this NULL pointer dereference. Either

1. current_head cannot in fact be NULL, in which case the commit message
should explain that fact and the code should be simplified
or
2. the test suite is incomplete.  If so, it would be great if you would
add a test that exercises this branch of the code (and catches your
error), and then fix the error.
Show 12 quoted lines
>  		rollback_index_files();
>  		die(_("cannot update HEAD ref"));
>  	}
> +	if (ref_transaction_commit(transaction, sb.buf,
> +				   UPDATE_REFS_QUIET_ON_ERR)) {
> +		rollback_index_files();
> +		die(_("cannot commit HEAD ref"));
> +	}
>  
>  	unlink(git_path("CHERRY_PICK_HEAD"));
>  	unlink(git_path("REVERT_HEAD"));
> 
Michael
-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/
Previous: Ronnie SahlbergNext: Ronnie Sahlberg
Message 16 of 24 in “Use ref transactions from most callers”
  1. 00/11 Use ref transactions from most callersRonnie Sahlberg, Apr 17, 2014
  2. 01/11 refs.c: constify the sha arguments for ref_transaction_create|delete|updateRonnie Sahlberg, Apr 17, 2014
  3. Michael HaggertyApr 19, 2014
  4. 02/11 refs.c: change ref_transaction_update() to do error checking and return statusRonnie Sahlberg, Apr 17, 2014
  5. Michael HaggertyApr 19, 2014
  6. Jonathan NiederApr 25, 2014
  7. 03/11 refs.c: change ref_transaction_create to do error checking and return statusRonnie Sahlberg, Apr 17, 2014
  8. Michael HaggertyApr 19, 2014
  9. 04/11 refs.c: ref_transaction_delete to check for error and return statusRonnie Sahlberg, Apr 17, 2014
  10. Michael HaggertyApr 19, 2014
  11. 05/11 tag.c: use ref transactions when doing updatesRonnie Sahlberg, Apr 17, 2014
  12. Michael HaggertyApr 19, 2014
  13. 06/11 replace.c: use the ref transaction functions for updatesRonnie Sahlberg, Apr 17, 2014
  14. Michael HaggertyApr 19, 2014
  15. 07/11 commit.c: use ref transactions for updatesRonnie Sahlberg, Apr 17, 2014
  16. Michael HaggertyApr 19, 2014
  17. Ronnie SahlbergApr 21, 2014
  18. 08/11 sequencer.c: use ref transactions for all ref updatesRonnie Sahlberg, Apr 17, 2014
  19. 09/11 fast-import.c: change update_branch to use ref transactionsRonnie Sahlberg, Apr 17, 2014
  20. 10/11 branch.c: use ref transaction for all ref updatesRonnie Sahlberg, Apr 17, 2014
  21. 11/11 walker.c: use ref transaction for ref updatesRonnie Sahlberg, Apr 17, 2014
  22. Michael HaggertyApr 19, 2014
  23. Junio C HamanoApr 21, 2014
  24. Ronnie SahlbergApr 21, 2014

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.