On Tue, 2012-07-10 at 19:20 +0200, Matthieu Moy wrote:
> Carlos Martín Nieto <c...@elego.de> writes:
> 
> > --- a/builtin/branch.c
> > +++ b/builtin/branch.c
> > @@ -864,10 +864,32 @@ int cmd_branch(int argc, const char **argv, const 
> > char *prefix)
> >                info and making sure new_upstream is correct */
> >             create_branch(head, branch->name, new_upstream, 0, 0, 0, quiet, 
> > BRANCH_TRACK_OVERRIDE);
> >     } else if (argc > 0 && argc <= 2) {
> > +           struct branch *branch = branch_get(argv[0]);
> > +           const char *old_upstream = NULL;
> > +           int branch_existed = 0;
> > +
> >             if (kinds != REF_LOCAL_BRANCH)
> >                     die(_("-a and -r options to 'git branch' do not make 
> > sense with a branch name"));
> > +
> > +           /* Save what argv[0] was pointing to so we can give
> > +              the --set-upstream-to hint */
> 
> Multi-line comments are usually written in Git as
> 
> /*
>  * multi-line
>  * comment
>  */

I've seen this style often, but sure.

> 
> > +           if (branch_has_merge_config(branch))
> > +             old_upstream = shorten_unambiguous_ref(branch->merge[0]->dst, 
> > 0);
> 
> Broken indentation.

Yeah, sorry. New laptop, hadn't got the default style fixed in the
config.

> 
> > +           if (argc == 1) {
> > +                   printf("If you wanted to make '%s' track '%s', do 
> > this:\n", head, argv[0]);
> 
> Could be marked for translation with _("...").

Done.

> 
> > +                   if (branch_existed)
> > +                           printf(" $ git branch --set-upstream '%s' 
> > '%s'\n", argv[0], old_upstream);
> 
> old_upstream may be NULL at this point. I guess you want to skip this
> line if old_upsteam is NULL.

We've just set up tracking for it, so we'd want to undo that. Which
means --unset-upstream would have to move earlier in the series so we
can suggest that.

> 
> The fact that I could find this bug suggests that this lacks a few new
> tests too ;-).

Indeed :) the next round will have them.

> 
> > +                   else
> > +                           printf(" $ git branch -d '%s'\n", argv[0]);
> > +
> > +                   printf(" $ git branch --set-upstream-to '%s'\n", 
> > argv[0]);
> 
> For the 3 printf()s: we usually display commands without the "$", and
> separate them from text with a blank line. See for example what "git
> commit" says when you didn't provide authorship:

Yeah, I was going by what Junio wrote in his mail. We should probably
have a double-LF as well, like in the message below.

> 
> You can suppress this message by setting them explicitly:
> 
>     git config --global user.name "Your Name"
>     git config --global user.email y...@example.com
> 
> After doing this, you may fix the identity used for this commit with:
> 
>     git commit --amend --reset-author
> 
> (the absence of $ sign avoids the temptation to cut-and-paste it)
> 



--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to