[PATCH v2] branch: segfault fixes and validation

[Nguyễn Thái Ngọc Duy]

branch_get() can return NULL (so far on detached HEAD only) but some
code paths in builtin/branch.c cannot deal with that and cause
segfaults. While at there, make sure we bail out when the user gives 2
or more arguments, but we only process the first one and silently
ignore the rest.

Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com
---
 On Fri, Feb 22, 2013 at 12:47 AM, Junio C Hamano gits...@pobox.com wrote:
  Nguyễn Thái Ngọc Duy  pclo...@gmail.com writes:
 
  branch_get() can return NULL (so far on detached HEAD only)...
 
  Do you anticipate any other cases where the API call should validly
  return NULL?
 
 No. But I do not see any guarantee that it may never do that in
 future either. Which is why I was deliberately vague with could not
 figure out But you also correctly observed my laziness there. So
 how about this? It makes a special case for HEAD but not insist on
 detached HEAD as the only cause.

 builtin/branch.c  | 24 
 t/t3200-branch.sh | 21 +
 2 files changed, 45 insertions(+)

diff --git a/builtin/branch.c b/builtin/branch.c
index 6371bf9..82ed337 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -889,6 +889,15 @@ int cmd_branch(int argc, const char **argv, const char 
*prefix)
} else if (new_upstream) {
struct branch *branch = branch_get(argv[0]);
 
+   if (argc  1)
+   die(_(too many branches to set new upstream));
+
+   if (!branch) {
+   if (!argc || !strcmp(argv[0], HEAD))
+   die(_(HEAD does not point to any branch. Is it 
detached?));
+   die(_(no such branch '%s'), argv[0]);
+   }
+
if (!ref_exists(branch-refname))
die(_(branch '%s' does not exist), branch-name);
 
@@ -901,6 +910,15 @@ int cmd_branch(int argc, const char **argv, const char 
*prefix)
struct branch *branch = branch_get(argv[0]);
struct strbuf buf = STRBUF_INIT;
 
+   if (argc  1)
+   die(_(too many branches to unset upstream));
+
+   if (!branch) {
+   if (!argc || !strcmp(argv[0], HEAD))
+   die(_(HEAD does not point to any branch. Is it 
detached?));
+   die(_(no such branch '%s'), argv[0]);
+   }
+
if (!branch_has_merge_config(branch)) {
die(_(Branch '%s' has no upstream information), 
branch-name);
}
@@ -916,6 +934,12 @@ int cmd_branch(int argc, const char **argv, const char 
*prefix)
int branch_existed = 0, remote_tracking = 0;
struct strbuf buf = STRBUF_INIT;
 
+   if (!strcmp(argv[0], HEAD))
+   die(_(it does not make sense to create 'HEAD' 
manually));
+
+   if (!branch)
+   die(_(no such branch '%s'), argv[0]);
+
if (kinds != REF_LOCAL_BRANCH)
die(_(-a and -r options to 'git branch' do not make 
sense with a branch name));
 
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index f3e0e4a..12f1e4a 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -42,6 +42,10 @@ test_expect_success \
 'git branch a/b/c should create a branch' \
 'git branch a/b/c  test_path_is_file .git/refs/heads/a/b/c'
 
+test_expect_success \
+'git branch HEAD should fail' \
+'test_must_fail git branch HEAD'
+
 cat expect EOF
 $_z40 $HEAD $GIT_COMMITTER_NAME $GIT_COMMITTER_EMAIL 1117150200 +
branch: Created from master
 EOF
@@ -388,6 +392,14 @@ test_expect_success \
 'git tag foobar 
  test_must_fail git branch --track my11 foobar'
 
+test_expect_success '--set-upstream-to fails on multiple branches' \
+'test_must_fail git branch --set-upstream-to master a b c'
+
+test_expect_success '--set-upstream-to fails on detached HEAD' \
+'git checkout HEAD^{} 
+ test_must_fail git branch --set-upstream-to master 
+ git checkout -'
+
 test_expect_success 'use --set-upstream-to modify HEAD' \
 'test_config branch.master.remote foo 
  test_config branch.master.merge foo 
@@ -417,6 +429,15 @@ test_expect_success 'test --unset-upstream on HEAD' \
  test_must_fail git branch --unset-upstream
 '
 
+test_expect_success '--unset-upstream should fail on multiple branches' \
+'test_must_fail git branch --unset-upstream a b c'
+
+test_expect_success '--unset-upstream should fail on detached HEAD' \
+'git checkout HEAD^{} 
+ test_must_fail git branch --unset-upstream 
+ git checkout -
+'
+
 test_expect_success 'test --unset-upstream on a particular branch' \
 'git branch my15
  git branch --set-upstream-to master my14 
-- 
1.8.1.2.536.gf441e6d

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

Re: [PATCH v2] branch: segfault fixes and validation

[Junio C Hamano]

Nguyễn Thái Ngọc Duy  pclo...@gmail.com writes:

 branch_get() can return NULL (so far on detached HEAD only) but some
 code paths in builtin/branch.c cannot deal with that and cause
 segfaults. While at there, make sure we bail out when the user gives 2
 or more arguments, but we only process the first one and silently
 ignore the rest.

Explain 2 or more arguments in what context, perhaps?  Otherwise
it makes it sound as if git branch foo bar baz is covered with
this patch, no?

 Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com
 ---
  On Fri, Feb 22, 2013 at 12:47 AM, Junio C Hamano gits...@pobox.com wrote:
   Nguyễn Thái Ngọc Duy  pclo...@gmail.com writes:
  
   branch_get() can return NULL (so far on detached HEAD only)...
  
   Do you anticipate any other cases where the API call should validly
   return NULL?
  
  No. But I do not see any guarantee that it may never do that in
  future either. Which is why I was deliberately vague with could not
  figure out But you also correctly observed my laziness there. So
  how about this? It makes a special case for HEAD but not insist on
  detached HEAD as the only cause.

Sure.  It looks better.

What you can do is to have a single helper function that can explain
why branch_get() returned NULL (or extend branch_get() to serve that
purpose as well); then you do not have to duplicate the logic twice
on the caller's side (and there may be other callers that want to do
the same).

 diff --git a/builtin/branch.c b/builtin/branch.c
 index 6371bf9..82ed337 100644
 --- a/builtin/branch.c
 +++ b/builtin/branch.c
 @@ -889,6 +889,15 @@ int cmd_branch(int argc, const char **argv, const char 
 *prefix)
   } else if (new_upstream) {
   struct branch *branch = branch_get(argv[0]);
  
 + if (argc  1)
 + die(_(too many branches to set new upstream));
 +
 + if (!branch) {
 + if (!argc || !strcmp(argv[0], HEAD))
 + die(_(HEAD does not point to any branch. Is it 
 detached?));
 + die(_(no such branch '%s'), argv[0]);
 + }
 +
   if (!ref_exists(branch-refname))
   die(_(branch '%s' does not exist), branch-name);

The latter part of the new code triggers when branch_get() returns
NULL while doing git branch --set-upstream-to=X [Y].  When Y is
string HEAD or missing, the first die() is triggered and says a
funny thing. If HEAD does not point to any branch, by definition it
is detached.  The user may say Yes, I know it is detached. but the
message does not help the user to figure out what to do next.

Instead of asking Is it detached?, perhaps we can say something
like You told me to set the upstream of HEAD to branch X, but  in
front?  At least, that will be a better explanation for the reason
why the operation is failing.

The existing test might be wrong, by the way.  Your HEAD may point
at a branch Y but you may not have any commit on it yet, and you may
want to allow setting the upstream of that to-be-born branch to
another branch X with branch --set-upstream-to=X [Y|HEAD].

While I think it is insane to do anything before creating the first
commit on your current branch (or using checkout --orphan in
general) and it may not be worth our time to babysit users who do
so, but the following sequence may feel natural to them:

git checkout --orphan X
git branch --set-upstream-to=master

... perhaps create an initial commit, perhaps not ...

git merge @{upstream}

For that to work sanely, perhaps the pattern

branch = branch_get();
if (!branch)
die due to no branch;
if (!ref_exists(branch-refname))
die due to typo in branch name

may need to be fixed globally, replacing ref_exists(branch-refname)
with branch_exists(branch) that returns true if branch-refname is
an existing ref, or the branch in question was obtained by checking
with current_branch (in remote.c), or something like that.
--
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