Hi,
please don't take my comments as insults or even strict rules. It is
purely for your consideration. (I say this because I haven't seen you so
often on this list, so you might not know that discussions about patches
are sometimes, erm, lively...)
On Thu, 1 Mar 2007, Paolo Bonzini wrote:
> > (2) by reviewing the changes to the .config writer. That
> > traditionally has been one of the more fragile parts of the
> > system, and I am reluctant to look at it.
>
> Just FYI, this was broken into a separate patch.
>
> > I would just feel better to see a patch like this, which is a
> > significant improvement to the system, to be properly signed-off
> > by the submitter.
>
> I hope this is better, I tried to follow the steps in
> SubmittingPatches properly. The code is actually the same
> as take 3.
According to SubmittingPatches, this is a cover letter.
You often want to add additional explanation about the patch,
other than the commit message itself. Place such "cover letter"
material between the three dash lines and the diffstat.
So, please put it after the three dashes and the diffstat next time.
> * git-branch: register where to merge from, when branching off a remote branch.
This is the oneline description, which should have been the Subject of the
mail, preferably prefixed by "[PATCH]" to make it obvious that it is not
yet another reply in a medium-sized thread, but actually a code
contribution. SubmittingPatches is not clear about this: you can write
_anything_ in brackets, and it will be stripped from the commit message
automatically. In your case, I would have preferred "[PATCH, 3rd
version]".
> A rather standard (in 1.5) procedure for branching off a remote archive is:
Since this will go into the commit message, which is usually shown in the
output of "git log", indented, it would be nice to break lines early.
Again, I think that SubmittingPatches is not totally clear about this: I
try to maintain a maximum width of 76 characters (which seems to be the
default with pine -- my mail program -- anyway).
> The behavior is controlled by core.trackremotebranches,
I'd make it obvious here that it is on by default -- even if you state
that earlier, too.
[I leave comments on documentation to others, since I cannot write them
myself.]
> diff --git a/builtin-branch.c b/builtin-branch.c
> index d0179b0..20de049 100644
> --- a/builtin-branch.c
> +++ b/builtin-branch.c
> @@ -12,7 +12,7 @@
> #include "builtin.h"
>
> static const char builtin_branch_usage[] =
> - "git-branch [-r] (-d | -D) <branchname> | [-l] [-f] <branchname> [<start-point>] | (-m | -M) [<oldbranch>] <newbranch> | [--color | --no-color] [-r | -a] [-v [--abbrev=<length>]]";
> + "git-branch [-r] (-d | -D) <branchname> | [--track | --no-track] [-l] [-f] <branchname> [<start-point>] | (-m | -M) [<oldbranch>] <newbranch> | [--color | --no-color] [-r | -a] [-v [--abbrev=<length>]]";
>
> #define REF_UNKNOWN_TYPE 0x00
> #define REF_LOCAL_BRANCH 0x01
> @@ -308,15 +307,36 @@ static void print_ref_list(int kinds, int detached, int verbose, int abbrev)
> free_ref_list(&ref_list);
> }
>
> +static void register_branch_pull (const char *name, const char *remote_name)
It is not yet remote_name, right? it is branch_name. You extract the
remote_name by finding the first slash.
> +{
> + char *slash = strchr(remote_name, '/');
> +
> + char *config_key = xmalloc(strlen(name) + 15);
> + char *merge_value = xmalloc(strlen(remote_name) + 10);
> +
> + char *remote_value = xstrdup(remote_name);
I'd use "char key[1024], value[1024]" instead, erroring out if one of the
buffers are too small. It's not like you have to be memory efficient, and
it is easier to read.
> + remote_value[slash - remote_name] = 0;
You should check if slash == NULL and error out before using it.
> + sprintf(config_key, "branch.%s.remote", name);
This would be a snprintf(key, sizeof(key), "branch.%s.remote", name); and
snprintf(value, sizeof(value), "%.*s", slash - branch_name, branch_name);
> + git_config_set(config_key, remote_value);
> +
> + sprintf(merge_value, "refs/heads/%s", slash + 1);
> + sprintf(config_key, "branch.%s.merge", name);
> + git_config_set(config_key, merge_value);
> +
> + free (config_key);
> + free (remote_value);
> + free (merge_value);
> +}
> +
> static void create_branch(const char *name, const char *start_name,
> unsigned char *start_sha1,
> - int force, int reflog)
> + int force, int reflog, int track)
> {
> struct ref_lock *lock;
> struct commit *commit;
> unsigned char sha1[20];
> - char ref[PATH_MAX], msg[PATH_MAX + 20];
> - int forcing = 0;
> + char *real_ref = NULL, ref[PATH_MAX], msg[PATH_MAX + 20];
> + int forcing = 0, remote = 0;
>
> snprintf(ref, sizeof ref, "refs/heads/%s", name);
> if (check_ref_format(ref))
> @@ -333,7 +353,9 @@ static void create_branch(const char *name, const char *start_name,
> if (start_sha1)
> /* detached HEAD */
> hashcpy(sha1, start_sha1);
> - else if (get_sha1(start_name, sha1))
> + else if (dwim_ref(start_name, strlen (start_name), sha1, &real_ref))
> + remote = !prefixcmp(real_ref, "refs/remotes/");
> + else
> die("Not a valid object name: '%s'.", start_name);
Yes, that is how I imagined it. The rest of your patch looks perfect to
me.
Ciao,
Dscho
-
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