Re: [PATCH] defaults for where to merge from (take 3, inline)

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Mar 1, 2007, 22:01 UTC
Message-ID: <Pine.LNX.4.63.0703012240210.22628@wbgn013.biozentrum.uni-wuerzburg.de>
In-Reply-To: <45E70041.1030705@lu.unisi.ch>

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:

Show 13 quoted lines

> >  (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.]

Show 18 quoted lines

> 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.

Show 7 quoted lines

> +{
> +	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);

Show 35 quoted lines

> +	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