Lars Hjemli <hjemli@xxxxxxxxx> writes:
> +static void delete_branch(const char *branch, int force, struct commit *head_rev)
> {
>...
> +}
Refactoring the single ref deletion into this function feels
sane. I think you do not need a separate force parameter to
this function anymore; if the caller wants to force the deletion
it can send in a NULL for head_rev to signal that there is no
need for the "subset" check.
> +static void delete_branches(int argc, const char **argv, int force)
> +{
> + struct commit *head_rev;
> + int i;
> +
> + head_rev = lookup_commit_reference(head_sha1);
> + if (!head_rev)
> + die("Couldn't look up commit object for current HEAD.");
> + for (i = 0; i < argc; i++)
> + delete_branch(argv[i], force, head_rev);
> }
I do not think this die() is a good idea. I think it is
reasonable to allow the following sequence:
$ mkdir newdir && cd newdir
$ git init-db
$ git fetch $other_repo refs/heads/master:refs/heads/othre
$ git branch -D othre ;# oops, typo
$ git fetch $other_repo refs/heads/master:refs/heads/other
When forcing a deletion, we do not care about ancestry relation
between the HEAD and the branch being deleted, so we should not
even bother checking if HEAD is already valid. The original
code before your patch shares the same problem.
> -static void create_branch(const char *name, const char *start,
> +static char *create_branch(const char *name, const char *start,
> int force, int reflog)
This makes the returned names leak but probably we do not care
about it too much. However, the only caller that cares about
the new refname is rename_branch, and we are talking only about
branches, so I think handcrafting the refname just like you
handcraft the refname for oldname in rename_branch() would be
a better solution without introducing new leaks.
> +static void rename_branch(const char *newname, const char *oldname, int force, int reflog)
> +{
> + char ref[PATH_MAX];
> +
> + snprintf(ref, sizeof ref, "refs/heads/%s", oldname);
> + if (check_ref_format(ref))
> + die("'%s' is not a valid branch name.", oldname);
> +
> + newname = create_branch(newname, oldname, force, reflog);
This does not feel right. The 'start' parameter to
create_branch is arbitrary SHA-1 expression so it can take
'master', 'heads/master' and 'refs/heads/master' to mean the
same thing, as long as they are unambiguous, but here you would
want to accept only 'master' because the paramter is supposed to
be the name of the branch you are renaming. create_branch()
does not want to do that check for its start parameter, so you
should do the checking yourself here, and check_ref_format() is
not good enough for that. Probably calling resolve_ref() on ref
(= "refs/heads/oldname") for reading (because you also want to
make sure oldname talks about an existing branch) is needed.
> + if (!strcmp(oldname, head)) {
> + create_symref("HEAD", newname);
> + head = newname + 11;
> + }
> + delete_branch(oldname, force, NULL);
> }
What is the right thing that should happen when newname talks
about an existing branch (I am not asking "what does your code
do?")?
Without -f, it should barf. With -f, we would want the rename
to happen. In the latter case, I think it should work the same
way as deleting it and creating it anew, and that would make
sure that reflog for the old one will be lost and a new log is
started afresh; otherwise, the log would say old history for
that branch and it won't be a "rename" anymore.
Also what happens when oldname is "frotz" and newname is
"frotz/nitfol"? You would need to read the value of "frotz",
make sure you can delete it (perhaps the usual fast-forward
check as needed), and delete it to make room and then create
"frotz/nitfol". I suspect your patch does not handle that
case.
-
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html