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

[Shawn Pearce <spearce@spearce.org>]

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]
> +		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.  :-)

> +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.  :-)