Re: [PATCH] branch as a builtin (again)

From: Shawn Pearce <spearce@spearce.org>
Date: Aug 21, 2006, 20:41 UTC
Message-ID: <20060821204110.GA24465@spearce.org>
In-Reply-To: <59ad55d30608211312u51a4657eyd52311314a6ee03c@mail.gmail.com>

Kristian H?gsberg <krh@bitplanet.net> wrote:

> +static void delete_branches(int argc, const char **argv, int force)

[snip]

> +		name = git_path("refs/heads/%s", argv[i]);
> +		if (!resolve_ref(name, sha1, 1))
> +			die("Branch '%s' not found.", argv[i]);

[snip]

Show 5 quoted lines

> +		unlink(name);
> +
> +		/* Unlink reflog if it exists. */
> +		reflog = git_path("logs/refs/heads/%s", argv[i]);
> +		unlink(reflog);

Hmm. So git-branch.sh doesn't deal with symrefs, eh? I guess this is OK but I'm wondering why not put this code into refs.c to lock the ref (refs.c:lock_ref_sha1) then instead of unlocking it delete it and its log (add new function to do this).

The downside of this is that we'll chase a symref, which means that if refs/heads/FOO is a symref to refs/heads/master and the user calls `git-branch -D FOO` we'll kill refs/heads/master. Maybe that's not what the the user would want to have happen. :-)

Show 15 quoted lines

> +static void create_reflog(struct ref_lock *lock)
> +{
> +	struct stat stbuf;
> +	int fd;
> +
> +	if (!stat(lock->log_file, &stbuf) && S_ISREG(stbuf.st_mode))
> +		return;
> +	if (safe_create_leading_directories(lock->log_file) < 0)
> +		die("Unable to create directory for %s.", lock->log_file);
> +	fd = open(lock->log_file, O_CREAT | O_TRUNC | O_WRONLY, 0666);
> +	if (fd < 0)
> +		die("Unable to create ref log %s: %s.",
> +		    lock->log_file, strerror(errno));
> +	close(fd);
> +}

This probably should move into refs.c. Look at log_ref_write, specifically around the if (log_all_ref_updates). If this took an additional parameter to force creation of the log even if the log isn't present and OR'd against log_all_ref_updates then it would be possible to have the refs.c code create the log for you in the "library" part of GIT.

Or maybe it is better to add this as a flag to the struct ref_lock, defaulting to false and letting the caller set it to true before invoking write_ref_sha1. I only suggest this because of the number of parameters already in play here.

> +static void create_branch(const char *name, const char *start,
> +			  int force, int reflog)

This all looked correct to me, at least as far as dealing with the reflog. :-)