Git development
 help / color / mirror / Atom feed
* Re: [PATCH] git-p4: import utf16 file properly
From: Pete Wyckoff @ 2011-09-18  1:19 UTC (permalink / raw)
  To: Chris Li; +Cc: Luke Diamand, git
In-Reply-To: <CANeU7QnPqJ+igcmS1JX_vasCXr+Wpcx2b4Z-sy_=0qKEkG+v_w@mail.gmail.com>

git@chrisli.org wrote on Wed, 14 Sep 2011 11:39 -0700:
> On Wed, Sep 14, 2011 at 11:29 AM, Chris Li <git@chrisli.org> wrote:
> >> Does this change do the right thing with RCS keywords in UTF16 files?
> >
> > I don't know what is the rules about the RCS keyword in UTF16 files.
> 
> I did a little bit research and found this:
> 
> http://www.perforce.com/perforce/doc.current/manuals/p4guide/ab_filetypes.html
> 
> RCS keyword expand should only happen for "+k" or "+ko" modifiers.
> There for, "utf16" files without modifier should not be converted.
> In that regard, the patch is correct. But both the original and patched version
> did not handle "utf16+k" type of files. I still consider it as a separate issue.

Your patch looks good and this all makes sense.  I redid
it, adding a test case, and some more patches to clean up
some of the filetype detection code.  I'll send it out for
review soon here.

Luke:  thanks for the comments; they prompted me to think
about keywords and beyond.

		-- Pete

^ permalink raw reply

* Re: [PATCH v3] git svn dcommit: new option --interactive.
From: Eric Wong @ 2011-09-18  1:13 UTC (permalink / raw)
  To: Carlos Martín Nieto; +Cc: Frédéric Heitzmann, git
In-Reply-To: <1316261904.6897.9.camel@bee.lab.cmartin.tk>

Carlos Martín Nieto <carlos@cmartin.tk> wrote:
> On Fri, 2011-09-16 at 23:02 +0200, Frédéric Heitzmann wrote:
> > Allow the user to check the patch set before it is commited to SNV. It is
> 
> Typo: SNV -> SVN

Thanks, fixed locally

> My perl-foo isn't strong enough to properly review the rest.

I also squashed the following cleanup:

--- a/git-svn.perl
+++ b/git-svn.perl
@@ -802,7 +802,6 @@ sub cmd_dcommit {
 	if (defined $_interactive){
 		my $ask_default = "y";
 		foreach my $d (@$linear_refs){
-			print "debug : d = $d\n";
 			my ($fh, $ctx) = command_output_pipe(qw(show --summary), "$d");
 			while (<$fh>){
 				print $_;


Otherwise things good to me.  Acked and pushed out to master of
git://bogomips.org/git-svn.git

^ permalink raw reply

* Re: git web--browse error handling URL with & in it (Was Re: [RFC/PATCH] Configurable hyperlinking in gitk)
From: Chris Packham @ 2011-09-18  0:32 UTC (permalink / raw)
  To: Jeff Epler; +Cc: git
In-Reply-To: <4E753BB9.7030804@gmail.com>

On 18/09/11 12:30, Chris Packham wrote:
> On 18/09/11 11:33, Chris Packham wrote:
>> On 18/09/11 01:45, Jeff Epler wrote:
>>>>> There are probably better names for the configuration options, too.
>>>>
>>>> It'd be nice if the config variables weren't gitk specific. .re and .sub
>>>> could be applied to gitweb and maybe other git viewers outside of
>>>> gig.git might decide to use them. My bikeshedding suggestion would be to
>>>> just drop the gitk prefix and have linkify.re and linkify.sub.
>>>
>>> This seems like a reasonable idea, though since the implementation
>>> languages of gitk and gitweb are different it means some REs might get
>>> different interpretations in the different programs.
>>>
>>>> Sometimes when a commit fixes multiple bugs we put all the bug numbers
>>>> in separated by commas. I don't know Tcl well enough to tell if your
>>>> code supports that or not.
>>>
>>> Multiple matches per line are OK, but they must be non-overlapping.
>>>
>>> Looking at the actual practice in Debian changelogs, I see that they do
>>> this:
>>>     evince/changelog.Debian.gz:        (Closes: #388368, #396467, #405130)
>>> so my original example would only linkify "Closes: #388638".  But a
>>> revised pattern of #(\d+) would linkify "#388368", "#396467" and "#405130".
>>> (but risk a few more "false positive" links).  I should revise my
>>> example accordingly.
>>>
>>> As for the problems with your substitutions, "&" is special in a tcl
>>> regsub (it stands for the whole matched string, like \0), so you'd want
>>> to use a substitution like
>>>     git config gitk.linkify.debian-bts.sub \
>>>         'http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=\1\&foo=bar'
>>
>> Hmm no joy with \&. Seems to upset the invocation of git web-browse
>>
>>   git config gitk.linkify.bugtracker.sub \
>>        'https://internalhost/code\&stuff/bugs.php?id=\1'
>>
>>   gitk
>>   /home/chrisp/libexec/git-core/git-web--browse: line 167:
>> stuff/bugs.php?id=bug123: No such file or directory
>>   fatal: 'web--browse' appears to be a git command, but we were not
>>   able to execute it. Maybe git-web--browse is broken?
> 
> This is probably a issue with git web--browse and nothing to do with
> your changes.
> 
> Sure enough this works fine
> 
>   git web--browse --browser=firefox \
>       https://internalhost/code\&stuff/bugs.php?id=foo
> 
> While this doesn't
> 
>   git web--browse https://internalhost/code\&stuff/bugs.php?id=foo
> 
> /home/chrisp/libexec/git-core/git-web--browse: line 167:
> stuff/bugs.php?id=foo: No such file or directory
> fatal: 'web--browse' appears to be a git command, but we were not
> able to execute it. Maybe git-web--browse is broken?
> 
> Neither does this
> 
>   git web--browse --browser=konqueror \
>      https://internalhost/code\&stuff/bugs.php?id=foo
> 
> A little bit more info that might help diagnose the issue - I'm running
> openSUSE 11.4 (kde 4.6) which ships with firefox set as the default web
> browser so 'kfmclient newTab http://www.example.com' actually opens firefox.
> 
> However trying kfmclient with my funny URL still works
> 
>   kfmclient newTab https://internalhost/code\&stuff/bugs.php?id=foo
> 
> I'm a little stumped as to what is going wrong in git web--browse.
> 

Update: it's the call to eval that causes the problem

  eval kfmclient newTab https://internalhost/code\&stuff/bugs.php?id=foo
  [1] 14728
  bash: stuff/bugs.php?id=foo: No such file or directory

^ permalink raw reply

* git web--browse error handling URL with & in it (Was Re: [RFC/PATCH] Configurable hyperlinking in gitk)
From: Chris Packham @ 2011-09-18  0:30 UTC (permalink / raw)
  To: Jeff Epler; +Cc: git
In-Reply-To: <4E752E32.2010208@gmail.com>

On 18/09/11 11:33, Chris Packham wrote:
> On 18/09/11 01:45, Jeff Epler wrote:
>>>> There are probably better names for the configuration options, too.
>>>
>>> It'd be nice if the config variables weren't gitk specific. .re and .sub
>>> could be applied to gitweb and maybe other git viewers outside of
>>> gig.git might decide to use them. My bikeshedding suggestion would be to
>>> just drop the gitk prefix and have linkify.re and linkify.sub.
>>
>> This seems like a reasonable idea, though since the implementation
>> languages of gitk and gitweb are different it means some REs might get
>> different interpretations in the different programs.
>>
>>> Sometimes when a commit fixes multiple bugs we put all the bug numbers
>>> in separated by commas. I don't know Tcl well enough to tell if your
>>> code supports that or not.
>>
>> Multiple matches per line are OK, but they must be non-overlapping.
>>
>> Looking at the actual practice in Debian changelogs, I see that they do
>> this:
>>     evince/changelog.Debian.gz:        (Closes: #388368, #396467, #405130)
>> so my original example would only linkify "Closes: #388638".  But a
>> revised pattern of #(\d+) would linkify "#388368", "#396467" and "#405130".
>> (but risk a few more "false positive" links).  I should revise my
>> example accordingly.
>>
>> As for the problems with your substitutions, "&" is special in a tcl
>> regsub (it stands for the whole matched string, like \0), so you'd want
>> to use a substitution like
>>     git config gitk.linkify.debian-bts.sub \
>>         'http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=\1\&foo=bar'
> 
> Hmm no joy with \&. Seems to upset the invocation of git web-browse
> 
>   git config gitk.linkify.bugtracker.sub \
>        'https://internalhost/code\&stuff/bugs.php?id=\1'
> 
>   gitk
>   /home/chrisp/libexec/git-core/git-web--browse: line 167:
> stuff/bugs.php?id=bug123: No such file or directory
>   fatal: 'web--browse' appears to be a git command, but we were not
>   able to execute it. Maybe git-web--browse is broken?

This is probably a issue with git web--browse and nothing to do with
your changes.

Sure enough this works fine

  git web--browse --browser=firefox \
      https://internalhost/code\&stuff/bugs.php?id=foo

While this doesn't

  git web--browse https://internalhost/code\&stuff/bugs.php?id=foo

/home/chrisp/libexec/git-core/git-web--browse: line 167:
stuff/bugs.php?id=foo: No such file or directory
fatal: 'web--browse' appears to be a git command, but we were not
able to execute it. Maybe git-web--browse is broken?

Neither does this

  git web--browse --browser=konqueror \
     https://internalhost/code\&stuff/bugs.php?id=foo

A little bit more info that might help diagnose the issue - I'm running
openSUSE 11.4 (kde 4.6) which ships with firefox set as the default web
browser so 'kfmclient newTab http://www.example.com' actually opens firefox.

However trying kfmclient with my funny URL still works

  kfmclient newTab https://internalhost/code\&stuff/bugs.php?id=foo

I'm a little stumped as to what is going wrong in git web--browse.

^ permalink raw reply

* Most elegant way to reference to SVN from GIT?
From: Manuel Reimer @ 2011-09-17 23:51 UTC (permalink / raw)
  To: git

Hello,

I'm using GIT for a project.

Now someone offered to contribute a translation. This translation is hosted on a 
SVN server.

How can I get his work into my GIT tree and how can I keep things updated?

Thanks in advance

Yours

Manuel

^ permalink raw reply

* Re: [RFC/PATCH] Configurable hyperlinking in gitk
From: Chris Packham @ 2011-09-17 23:33 UTC (permalink / raw)
  To: Jeff Epler; +Cc: git
In-Reply-To: <20110917134527.GA28463@unpythonic.net>

On 18/09/11 01:45, Jeff Epler wrote:
>>> There are probably better names for the configuration options, too.
>>
>> It'd be nice if the config variables weren't gitk specific. .re and .sub
>> could be applied to gitweb and maybe other git viewers outside of
>> gig.git might decide to use them. My bikeshedding suggestion would be to
>> just drop the gitk prefix and have linkify.re and linkify.sub.
> 
> This seems like a reasonable idea, though since the implementation
> languages of gitk and gitweb are different it means some REs might get
> different interpretations in the different programs.
> 
>> Sometimes when a commit fixes multiple bugs we put all the bug numbers
>> in separated by commas. I don't know Tcl well enough to tell if your
>> code supports that or not.
> 
> Multiple matches per line are OK, but they must be non-overlapping.
> 
> Looking at the actual practice in Debian changelogs, I see that they do
> this:
>     evince/changelog.Debian.gz:        (Closes: #388368, #396467, #405130)
> so my original example would only linkify "Closes: #388638".  But a
> revised pattern of #(\d+) would linkify "#388368", "#396467" and "#405130".
> (but risk a few more "false positive" links).  I should revise my
> example accordingly.
> 
> As for the problems with your substitutions, "&" is special in a tcl
> regsub (it stands for the whole matched string, like \0), so you'd want
> to use a substitution like
>     git config gitk.linkify.debian-bts.sub \
>         'http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=\1\&foo=bar'

Hmm no joy with \&. Seems to upset the invocation of git web-browse

  git config gitk.linkify.bugtracker.sub \
       'https://internalhost/code\&stuff/bugs.php?id=\1'

  gitk
  /home/chrisp/libexec/git-core/git-web--browse: line 167:
stuff/bugs.php?id=bug123: No such file or directory
  fatal: 'web--browse' appears to be a git command, but we were not
  able to execute it. Maybe git-web--browse is broken?

Using the following works as expected with no error with your updated patch.

  git config gitk.linkify.bugtracker.sub \
       'https://internalhost/code%26stuff/bugs.php?id=\1'

> The problem with "%" has to do with Tk's event substitution and it's a
> bug that this happens; I should manually double the % at the proper
> point.
> 

^ permalink raw reply

* Re: zealous git convert determined to set up git server
From: fREW Schmidt @ 2011-09-17 21:04 UTC (permalink / raw)
  To: Joshua Stoutenburg; +Cc: Jakub Narebski, Git List
In-Reply-To: <CAOZxsTqtW=DD7zFwQLjknJR8g0nnh0WPUPna6_np4bVoGnSntQ@mail.gmail.com>

> Question 2: It seems gitolite is the popular choice for git user
> management.  Any reason why?

Another thing that I personally LOVE as the "Version Control Czar" at
my company is the fact that users can easily get a list of repo's that
they have access to using gitolite.  Instead of people asking me all
the time "what's the name of the Foo repo?" they can just do `ssh
cesium info` and get a list of repos.

It's a small feature but it really is super handy.

--
fREW Schmidt
http://blog.afoolishmanifesto.com

^ permalink raw reply

* Really removing a file named \\ from repository (gitolite)
From: robert mena @ 2011-09-17 15:35 UTC (permalink / raw)
  To: git

Hi,

I have a problem.  I've created a repository and when I try to clone
it from netbeans under windows it gives me an IOException complaint
that he can't delete directory X.   I can do it fine from command line
(linux) and the same development build from netbeans mac.

While investigating I found that there was a file named \\ under the X
directory when I created the repository in the first place.  I've
successfully removed (rm and git commit / git push) but still the
netbeans under windows does not work with the same exception.

Any ideas?

Do I have to somehow nuke this file from the git repository so the IDE
does not try to fetch it?

^ permalink raw reply

* Re: [RFC/PATCH] Configurable hyperlinking in gitk
From: Jeff Epler @ 2011-09-17 13:45 UTC (permalink / raw)
  To: Chris Packham; +Cc: git
In-Reply-To: <4E7467B7.1090201@gmail.com>

> > There are probably better names for the configuration options, too.
> 
> It'd be nice if the config variables weren't gitk specific. .re and .sub
> could be applied to gitweb and maybe other git viewers outside of
> gig.git might decide to use them. My bikeshedding suggestion would be to
> just drop the gitk prefix and have linkify.re and linkify.sub.

This seems like a reasonable idea, though since the implementation
languages of gitk and gitweb are different it means some REs might get
different interpretations in the different programs.

> Sometimes when a commit fixes multiple bugs we put all the bug numbers
> in separated by commas. I don't know Tcl well enough to tell if your
> code supports that or not.

Multiple matches per line are OK, but they must be non-overlapping.

Looking at the actual practice in Debian changelogs, I see that they do
this:
    evince/changelog.Debian.gz:        (Closes: #388368, #396467, #405130)
so my original example would only linkify "Closes: #388638".  But a
revised pattern of #(\d+) would linkify "#388368", "#396467" and "#405130".
(but risk a few more "false positive" links).  I should revise my
example accordingly.

As for the problems with your substitutions, "&" is special in a tcl
regsub (it stands for the whole matched string, like \0), so you'd want
to use a substitution like
    git config gitk.linkify.debian-bts.sub \
        'http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=\1\&foo=bar'
The problem with "%" has to do with Tk's event substitution and it's a
bug that this happens; I should manually double the % at the proper
point.

This revised patch fixes the problem with % in substitutions and changes
the suggested RE for matching debian bts items, but it does not rename
the configuration options.

-- >8 --
Many projects use project-specific notations in changelogs to refer
to bug trackers and the like.  One example is the "Closes: #12345"
notation used in Debian.

Make gitk configurable so that arbitrary strings can be turned into
clickable links that are opened in a web browser.
---
 Documentation/config.txt |   31 ++++++++++++++++++-
 gitk-git/gitk            |   75 +++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 103 insertions(+), 3 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index ae9913b..13e8aa6 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1064,6 +1064,33 @@ All gitcvs variables except for 'gitcvs.usecrlfattr' and
 is one of "ext" and "pserver") to make them apply only for the given
 access method.
 
+gitk.linkify.<name>.re::
+	Specify a Tcl regular expression (which may not span lines)
+	defining a class of strings to automatically convert to hyperlinks.
+	You must also specify 'gitk.linkify.<name>.sub'.
+
+gitk.linkify.<name>.sub::
+	Specify a substitution that results in the target URL for the
+	related regular expression.  Back-references like '\1' refer
+	to capturing groups in the associated regular expression.
+	You must also specify 'gitk.linkify.<name>.re'.
+
+gitk.browser::
+	Specify the browser that will be used to display the linked
+	web page.
+
+For example, to automatically link from Debian-style "Closes: #nnnn"
+message to the Debian BTS,
+
+--------
+    git config gitk.linkify.debian-bts.re '#(\d+)\M'
+    git config gitk.linkify.debian-bts.sub 'http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=\1'
+--------
+
+Regular expressions are as described in re_syntax(n).  Replacements
+are as described in regsub(n).  If multiple regular expressions match at
+the same location, it is undefined which match is used.
+
 grep.lineNumber::
 	If set to true, enable '-n' option by default.
 
@@ -1870,5 +1897,5 @@ user.signingkey::
 
 web.browser::
 	Specify a web browser that may be used by some commands.
-	Currently only linkgit:git-instaweb[1] and linkgit:git-help[1]
-	may use it.
+	Currently only linkgit:git-instaweb[1], linkgit:gitk[1],
+	and linkgit:git-help[1] may use it.
diff --git a/gitk-git/gitk b/gitk-git/gitk
index 4cde0c4..5532869 100755
--- a/gitk-git/gitk
+++ b/gitk-git/gitk
@@ -6684,7 +6684,7 @@ proc commit_descriptor {p} {
 # append some text to the ctext widget, and make any SHA1 ID
 # that we know about be a clickable link.
 proc appendwithlinks {text tags} {
-    global ctext linknum curview
+    global ctext linknum curview linkmakers
 
     set start [$ctext index "end - 1c"]
     $ctext insert end $text $tags
@@ -6699,6 +6699,30 @@ proc appendwithlinks {text tags} {
 	setlink $linkid link$linknum
 	incr linknum
     }
+
+    if {$linkmakers == {}} return
+
+    set link_re {}
+    foreach {re rep} $linkmakers { lappend link_re $re }
+    set link_re "([join $link_re {)|(}])"
+
+    set ee 0
+    while {[regexp -indices -start $ee -- $link_re $text l]} {
+	set s [lindex $l 0]
+	set e [lindex $l 1]
+	set linktext [string range $text $s $e]
+	incr e
+	set ee $e
+
+	foreach {re rep} $linkmakers {
+	    if {![regsub $re $linktext $rep linkurl]} continue
+	    $ctext tag delete link$linknum
+	    $ctext tag add link$linknum "$start + $s c" "$start + $e c"
+	    seturllink $linkurl link$linknum
+	    incr linknum
+	    break
+	}
+    }
 }
 
 proc setlink {id lk} {
@@ -6726,6 +6750,53 @@ proc setlink {id lk} {
     }
 }
 
+proc get_link_config {} {
+    if {[catch {exec git config -z --get-regexp {^gitk\.linkify\.}} linkers]} {
+	return {}
+    }
+
+    set linktypes [list]
+    foreach item [split $linkers "\0"] {
+	if {$item == ""} continue
+	if {![regexp {gitk\.linkify\.(\S+)\.(re|sub)\s(.*)} $item _ k t v]} {
+	    continue
+	}
+	set linkconfig($t,$k) $v
+	if {$t == "re"} { lappend linktypes $k }
+    }
+
+    set linkmakers [list]
+    foreach k $linktypes {
+	if {![info exists linkconfig(sub,$k)]} {
+	    puts stderr "Warning: link `$k' is missing a substitution string"
+	} elseif {[catch {regexp -inline -- $linkconfig(re,$k) ""} err]} {
+	    puts stderr "Warning: link `$k': $err"
+	} else {
+	    lappend linkmakers $linkconfig(re,$k) $linkconfig(sub,$k)
+	}
+	unset linkconfig(re,$k)
+	unset -nocomplain linkconfig(sub,$k)
+    }
+    foreach k [array names linkconfig] {
+	regexp "sub,(.*)" $k _ k
+	puts stderr "Warning: link `$k' is missing a regular expression"
+    }
+    set linkmakers
+}
+
+proc openlink {url} {
+    exec git web--browse --config=gitk.browser $url &
+}
+
+proc seturllink {url lk} {
+    set qurl [string map {% %%} $url]
+    global ctext
+    $ctext tag conf $lk -foreground blue -underline 1
+    $ctext tag bind $lk <1> [list openlink $qurl]
+    $ctext tag bind $lk <Enter> {linkcursor %W 1}
+    $ctext tag bind $lk <Leave> {linkcursor %W -1}
+}
+
 proc appendshortlink {id {pre {}} {post {}}} {
     global ctext linknum
 
@@ -11693,6 +11764,8 @@ if {[tk windowingsystem] eq "win32"} {
     focus -force .
 }
 
+set linkmakers [get_link_config]
+
 getcommits {}
 
 # Local variables:
-- 
1.7.2.5

^ permalink raw reply related

* Re: [PATCH v3] git svn dcommit: new option --interactive.
From: Frédéric Heitzmann @ 2011-09-17 13:11 UTC (permalink / raw)
  To: Carlos Martín Nieto; +Cc: git, normalperson
In-Reply-To: <1316261904.6897.9.camel@bee.lab.cmartin.tk>


Le 17/09/2011 14:18, Carlos Martín Nieto a écrit :
> [removed Junio from CC as he doesn't want to know about this patch at
> this stage]
>
> On Fri, 2011-09-16 at 23:02 +0200, Frédéric Heitzmann wrote:
>> Allow the user to check the patch set before it is commited to SNV. It is
> Typo: SNV ->  SVN
>
> My perl-foo isn't strong enough to properly review the rest.
>
>     cmn
Thanks.

--
Fred

^ permalink raw reply

* Re: [PATCH v3] git svn dcommit: new option --interactive.
From: Carlos Martín Nieto @ 2011-09-17 12:18 UTC (permalink / raw)
  To: Frédéric Heitzmann; +Cc: git, normalperson
In-Reply-To: <1316206921-29311-1-git-send-email-frederic.heitzmann@gmail.com>

[-- Attachment #1: Type: text/plain, Size: 321 bytes --]

[removed Junio from CC as he doesn't want to know about this patch at
this stage]

On Fri, 2011-09-16 at 23:02 +0200, Frédéric Heitzmann wrote:
> Allow the user to check the patch set before it is commited to SNV. It is

Typo: SNV -> SVN

My perl-foo isn't strong enough to properly review the rest.

   cmn

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

^ permalink raw reply

* [PATCH v5 4/4] Accept tags in HEAD or MERGE_HEAD
From: Nguyễn Thái Ngọc Duy @ 2011-09-17 11:57 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Nguyễn Thái Ngọc Duy
In-Reply-To: <1316260665-1728-1-git-send-email-pclouds@gmail.com>

HEAD and MERGE_HEAD (among other branch tips) should never hold a
tag. That can only be caused by broken tools and is cumbersome to fix
by an end user with:

  $ git update-ref HEAD $(git rev-parse HEAD^{commit})

which may look like a magic to a new person.

Be easy, warn users (so broken tools can be fixed if they bother to
report) and move on.

Be robust, if the given SHA-1 cannot be resolved to a commit object,
die (therefore return value is always valid).

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/commit.c        |    6 ++++--
 builtin/fmt-merge-msg.c |    2 +-
 builtin/merge.c         |    7 ++-----
 commit.c                |   12 ++++++++++++
 commit.h                |    7 +++++++
 http-push.c             |    8 ++++----
 revision.c              |    6 ++++--
 7 files changed, 34 insertions(+), 14 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 1a65319..402eb5a 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1396,7 +1396,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 	if (get_sha1("HEAD", sha1))
 		current_head = NULL;
 	else {
-		current_head = lookup_commit(sha1);
+		current_head = lookup_commit_or_die(sha1, "HEAD");
 		if (!current_head || parse_commit(current_head))
 			die(_("could not parse HEAD commit"));
 	}
@@ -1431,6 +1431,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 			pptr = &commit_list_insert(c->item, pptr)->next;
 	} else if (whence == FROM_MERGE) {
 		struct strbuf m = STRBUF_INIT;
+		struct commit *commit;
 		FILE *fp;
 
 		if (!reflog_msg)
@@ -1444,7 +1445,8 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 			unsigned char sha1[20];
 			if (get_sha1_hex(m.buf, sha1) < 0)
 				die(_("Corrupt MERGE_HEAD file (%s)"), m.buf);
-			pptr = &commit_list_insert(lookup_commit(sha1), pptr)->next;
+			commit = lookup_commit_or_die(sha1, "MERGE_HEAD");
+			pptr = &commit_list_insert(commit, pptr)->next;
 		}
 		fclose(fp);
 		strbuf_release(&m);
diff --git a/builtin/fmt-merge-msg.c b/builtin/fmt-merge-msg.c
index 7581632..7e2f225 100644
--- a/builtin/fmt-merge-msg.c
+++ b/builtin/fmt-merge-msg.c
@@ -293,7 +293,7 @@ static int do_fmt_merge_msg(int merge_title, struct strbuf *in,
 		struct commit *head;
 		struct rev_info rev;
 
-		head = lookup_commit(head_sha1);
+		head = lookup_commit_or_die(head_sha1, "HEAD");
 		init_revisions(&rev, NULL);
 		rev.commit_format = CMIT_FMT_ONELINE;
 		rev.ignore_merges = 1;
diff --git a/builtin/merge.c b/builtin/merge.c
index f5eb3f5..9567d60 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -1036,11 +1036,8 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 		branch += 11;
 	if (!branch || is_null_sha1(head_sha1))
 		head_commit = NULL;
-	else {
-		head_commit = lookup_commit(head_sha1);
-		if (!head_commit)
-			die(_("could not parse HEAD"));
-	}
+	else
+		head_commit = lookup_commit_or_die(head_sha1, "HEAD");
 
 	git_config(git_merge_config, NULL);
 
diff --git a/commit.c b/commit.c
index ac337c7..50fcf96 100644
--- a/commit.c
+++ b/commit.c
@@ -39,6 +39,18 @@ struct commit *lookup_commit_reference(const unsigned char *sha1)
 	return lookup_commit_reference_gently(sha1, 0);
 }
 
+struct commit *lookup_commit_or_die(const unsigned char *sha1, const char *ref_name)
+{
+	struct commit *c = lookup_commit_reference(sha1);
+	if (!c)
+		die(_("could not parse %s"), ref_name);
+	if (hashcmp(sha1, c->object.sha1)) {
+		warning(_("%s %s is not a commit!"),
+			ref_name, sha1_to_hex(sha1));
+	}
+	return c;
+}
+
 struct commit *lookup_commit(const unsigned char *sha1)
 {
 	struct object *obj = lookup_object(sha1);
diff --git a/commit.h b/commit.h
index a2d571b..044134a 100644
--- a/commit.h
+++ b/commit.h
@@ -37,6 +37,13 @@ struct commit *lookup_commit_reference(const unsigned char *sha1);
 struct commit *lookup_commit_reference_gently(const unsigned char *sha1,
 					      int quiet);
 struct commit *lookup_commit_reference_by_name(const char *name);
+/*
+ * Look sha1 up for a commit, defer if needed. If dereference occurs,
+ * update "sha1" for consistency with retval->object.sha1. Also warn
+ * users this case because it is expected that sha1 points directly to
+ * a commit.
+ */
+struct commit *lookup_commit_or_die(const unsigned char *sha1, const char *ref_name);
 
 int parse_commit_buffer(struct commit *item, const void *buffer, unsigned long size);
 int parse_commit(struct commit *item);
diff --git a/http-push.c b/http-push.c
index 28bfe76..d432b30 100644
--- a/http-push.c
+++ b/http-push.c
@@ -1606,10 +1606,10 @@ static void fetch_symref(const char *path, char **symref, unsigned char *sha1)
 	strbuf_release(&buffer);
 }
 
-static int verify_merge_base(unsigned char *head_sha1, unsigned char *branch_sha1)
+static int verify_merge_base(unsigned char *head_sha1, struct ref *remote)
 {
-	struct commit *head = lookup_commit(head_sha1);
-	struct commit *branch = lookup_commit(branch_sha1);
+	struct commit *head = lookup_commit_or_die(head_sha1, "HEAD");
+	struct commit *branch = lookup_commit_or_die(remote->old_sha1, remote->name);
 	struct commit_list *merge_bases = get_merge_bases(head, branch, 1);
 
 	return (merge_bases && !merge_bases->next && merge_bases->item == branch);
@@ -1680,7 +1680,7 @@ static int delete_remote_branch(const char *pattern, int force)
 			return error("Remote branch %s resolves to object %s\nwhich does not exist locally, perhaps you need to fetch?", remote_ref->name, sha1_to_hex(remote_ref->old_sha1));
 
 		/* Remote branch must be an ancestor of remote HEAD */
-		if (!verify_merge_base(head_sha1, remote_ref->old_sha1)) {
+		if (!verify_merge_base(head_sha1, remote_ref)) {
 			return error("The branch '%s' is not an ancestor "
 				     "of your current HEAD.\n"
 				     "If you are sure you want to delete it,"
diff --git a/revision.c b/revision.c
index c46cfaa..5e057a0 100644
--- a/revision.c
+++ b/revision.c
@@ -986,10 +986,12 @@ static void prepare_show_merge(struct rev_info *revs)
 	const char **prune = NULL;
 	int i, prune_num = 1; /* counting terminating NULL */
 
-	if (get_sha1("HEAD", sha1) || !(head = lookup_commit(sha1)))
+	if (get_sha1("HEAD", sha1))
 		die("--merge without HEAD?");
-	if (get_sha1("MERGE_HEAD", sha1) || !(other = lookup_commit(sha1)))
+	head = lookup_commit_or_die(sha1, "HEAD");
+	if (get_sha1("MERGE_HEAD", sha1))
 		die("--merge without MERGE_HEAD?");
+	other = lookup_commit_or_die(sha1, "MERGE_HEAD");
 	add_pending_object(revs, &head->object, "HEAD");
 	add_pending_object(revs, &other->object, "MERGE_HEAD");
 	bases = get_merge_bases(head, other, 1);
-- 
1.7.3.1.256.g2539c.dirty

^ permalink raw reply related

* [PATCH v5 3/4] merge: remove global variable head[]
From: Nguyễn Thái Ngọc Duy @ 2011-09-17 11:57 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Nguyễn Thái Ngọc Duy
In-Reply-To: <1316260665-1728-1-git-send-email-pclouds@gmail.com>

Also kill head_invalid in favor of "head_commit == NULL".

Local variable "head" in cmd_merge() is renamed to "head_sha1" to make
sure I don't miss any access because this variable should not be used
after head_commit is set (use head_commit->object.sha1 instead).

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/merge.c |   97 ++++++++++++++++++++++++++++++-------------------------
 1 files changed, 53 insertions(+), 44 deletions(-)

diff --git a/builtin/merge.c b/builtin/merge.c
index c371484..f5eb3f5 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -50,7 +50,6 @@ static int fast_forward_only;
 static int allow_trivial = 1, have_message;
 static struct strbuf merge_msg;
 static struct commit_list *remoteheads;
-static unsigned char head[20];
 static struct strategy **use_strategies;
 static size_t use_strategies_nr, use_strategies_alloc;
 static const char **xopts;
@@ -279,7 +278,8 @@ static void reset_hard(unsigned const char *sha1, int verbose)
 		die(_("read-tree failed"));
 }
 
-static void restore_state(const unsigned char *stash)
+static void restore_state(const unsigned char *head,
+			  const unsigned char *stash)
 {
 	struct strbuf sb = STRBUF_INIT;
 	const char *args[] = { "stash", "apply", NULL, NULL };
@@ -309,10 +309,9 @@ static void finish_up_to_date(const char *msg)
 	drop_save();
 }
 
-static void squash_message(void)
+static void squash_message(struct commit *commit)
 {
 	struct rev_info rev;
-	struct commit *commit;
 	struct strbuf out = STRBUF_INIT;
 	struct commit_list *j;
 	int fd;
@@ -327,7 +326,6 @@ static void squash_message(void)
 	rev.ignore_merges = 1;
 	rev.commit_format = CMIT_FMT_MEDIUM;
 
-	commit = lookup_commit(head);
 	commit->object.flags |= UNINTERESTING;
 	add_pending_object(&rev, &commit->object, NULL);
 
@@ -356,9 +354,11 @@ static void squash_message(void)
 	strbuf_release(&out);
 }
 
-static void finish(const unsigned char *new_head, const char *msg)
+static void finish(struct commit *head_commit,
+		   const unsigned char *new_head, const char *msg)
 {
 	struct strbuf reflog_message = STRBUF_INIT;
+	const unsigned char *head = head_commit->object.sha1;
 
 	if (!msg)
 		strbuf_addstr(&reflog_message, getenv("GIT_REFLOG_ACTION"));
@@ -369,7 +369,7 @@ static void finish(const unsigned char *new_head, const char *msg)
 			getenv("GIT_REFLOG_ACTION"), msg);
 	}
 	if (squash) {
-		squash_message();
+		squash_message(head_commit);
 	} else {
 		if (verbosity >= 0 && !merge_msg.len)
 			printf(_("No merge message -- not updating HEAD\n"));
@@ -664,7 +664,7 @@ int try_merge_command(const char *strategy, size_t xopts_nr,
 }
 
 static int try_merge_strategy(const char *strategy, struct commit_list *common,
-			      const char *head_arg)
+			      struct commit *head, const char *head_arg)
 {
 	int index_fd;
 	struct lock_file *lock = xcalloc(1, sizeof(struct lock_file));
@@ -710,7 +710,7 @@ static int try_merge_strategy(const char *strategy, struct commit_list *common,
 			commit_list_insert(j->item, &reversed);
 
 		index_fd = hold_locked_index(lock, 1);
-		clean = merge_recursive(&o, lookup_commit(head),
+		clean = merge_recursive(&o, head,
 				remoteheads->item, reversed, &result);
 		if (active_cache_changed &&
 				(write_cache(index_fd, active_cache, active_nr) ||
@@ -861,25 +861,26 @@ static void run_prepare_commit_msg(void)
 	read_merge_msg();
 }
 
-static int merge_trivial(void)
+static int merge_trivial(struct commit *head)
 {
 	unsigned char result_tree[20], result_commit[20];
 	struct commit_list *parent = xmalloc(sizeof(*parent));
 
 	write_tree_trivial(result_tree);
 	printf(_("Wonderful.\n"));
-	parent->item = lookup_commit(head);
+	parent->item = head;
 	parent->next = xmalloc(sizeof(*parent->next));
 	parent->next->item = remoteheads->item;
 	parent->next->next = NULL;
 	run_prepare_commit_msg();
 	commit_tree(merge_msg.buf, result_tree, parent, result_commit, NULL);
-	finish(result_commit, "In-index merge");
+	finish(head, result_commit, "In-index merge");
 	drop_save();
 	return 0;
 }
 
-static int finish_automerge(struct commit_list *common,
+static int finish_automerge(struct commit *head,
+			    struct commit_list *common,
 			    unsigned char *result_tree,
 			    const char *wt_strategy)
 {
@@ -890,12 +891,12 @@ static int finish_automerge(struct commit_list *common,
 	free_commit_list(common);
 	if (allow_fast_forward) {
 		parents = remoteheads;
-		commit_list_insert(lookup_commit(head), &parents);
+		commit_list_insert(head, &parents);
 		parents = reduce_heads(parents);
 	} else {
 		struct commit_list **pptr = &parents;
 
-		pptr = &commit_list_insert(lookup_commit(head),
+		pptr = &commit_list_insert(head,
 				pptr)->next;
 		for (j = remoteheads; j; j = j->next)
 			pptr = &commit_list_insert(j->item, pptr)->next;
@@ -905,7 +906,7 @@ static int finish_automerge(struct commit_list *common,
 	run_prepare_commit_msg();
 	commit_tree(merge_msg.buf, result_tree, parents, result_commit, NULL);
 	strbuf_addf(&buf, "Merge made by %s.", wt_strategy);
-	finish(result_commit, buf.buf);
+	finish(head, result_commit, buf.buf);
 	strbuf_release(&buf);
 	drop_save();
 	return 0;
@@ -939,7 +940,8 @@ static int suggest_conflicts(int renormalizing)
 	return 1;
 }
 
-static struct commit *is_old_style_invocation(int argc, const char **argv)
+static struct commit *is_old_style_invocation(int argc, const char **argv,
+					      const unsigned char *head)
 {
 	struct commit *second_token = NULL;
 	if (argc > 2) {
@@ -1012,9 +1014,11 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 {
 	unsigned char result_tree[20];
 	unsigned char stash[20];
+	unsigned char head_sha1[20];
+	struct commit *head_commit;
 	struct strbuf buf = STRBUF_INIT;
 	const char *head_arg;
-	int flag, head_invalid = 0, i;
+	int flag, i;
 	int best_cnt = -1, merge_was_ok = 0, automerge_was_ok = 0;
 	struct commit_list *common = NULL;
 	const char *best_strategy = NULL, *wt_strategy = NULL;
@@ -1027,11 +1031,16 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 	 * Check if we are _not_ on a detached HEAD, i.e. if there is a
 	 * current branch.
 	 */
-	branch = resolve_ref("HEAD", head, 0, &flag);
+	branch = resolve_ref("HEAD", head_sha1, 0, &flag);
 	if (branch && !prefixcmp(branch, "refs/heads/"))
 		branch += 11;
-	if (!branch || is_null_sha1(head))
-		head_invalid = 1;
+	if (!branch || is_null_sha1(head_sha1))
+		head_commit = NULL;
+	else {
+		head_commit = lookup_commit(head_sha1);
+		if (!head_commit)
+			die(_("could not parse HEAD"));
+	}
 
 	git_config(git_merge_config, NULL);
 
@@ -1112,12 +1121,13 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 	 * additional safety measure to check for it.
 	 */
 
-	if (!have_message && is_old_style_invocation(argc, argv)) {
+	if (!have_message && head_commit &&
+	    is_old_style_invocation(argc, argv, head_commit->object.sha1)) {
 		strbuf_addstr(&merge_msg, argv[0]);
 		head_arg = argv[1];
 		argv += 2;
 		argc -= 2;
-	} else if (head_invalid) {
+	} else if (!head_commit) {
 		struct object *remote_head;
 		/*
 		 * If the merged head is a valid one there is no reason
@@ -1164,7 +1174,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 		}
 	}
 
-	if (head_invalid || !argc)
+	if (!head_commit || !argc)
 		usage_with_options(builtin_merge_usage,
 			builtin_merge_options);
 
@@ -1205,17 +1215,16 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 	}
 
 	if (!remoteheads->next)
-		common = get_merge_bases(lookup_commit(head),
-				remoteheads->item, 1);
+		common = get_merge_bases(head_commit, remoteheads->item, 1);
 	else {
 		struct commit_list *list = remoteheads;
-		commit_list_insert(lookup_commit(head), &list);
+		commit_list_insert(head_commit, &list);
 		common = get_octopus_merge_bases(list);
 		free(list);
 	}
 
-	update_ref("updating ORIG_HEAD", "ORIG_HEAD", head, NULL, 0,
-		DIE_ON_ERR);
+	update_ref("updating ORIG_HEAD", "ORIG_HEAD", head_commit->object.sha1,
+		   NULL, 0, DIE_ON_ERR);
 
 	if (!common)
 		; /* No common ancestors found. We need a real merge. */
@@ -1229,13 +1238,13 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 		return 0;
 	} else if (allow_fast_forward && !remoteheads->next &&
 			!common->next &&
-			!hashcmp(common->item->object.sha1, head)) {
+			!hashcmp(common->item->object.sha1, head_commit->object.sha1)) {
 		/* Again the most common case of merging one remote. */
 		struct strbuf msg = STRBUF_INIT;
 		struct object *o;
 		char hex[41];
 
-		strcpy(hex, find_unique_abbrev(head, DEFAULT_ABBREV));
+		strcpy(hex, find_unique_abbrev(head_commit->object.sha1, DEFAULT_ABBREV));
 
 		if (verbosity >= 0)
 			printf(_("Updating %s..%s\n"),
@@ -1251,10 +1260,10 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 		if (!o)
 			return 1;
 
-		if (checkout_fast_forward(head, remoteheads->item->object.sha1))
+		if (checkout_fast_forward(head_commit->object.sha1, remoteheads->item->object.sha1))
 			return 1;
 
-		finish(o->sha1, msg.buf);
+		finish(head_commit, o->sha1, msg.buf);
 		drop_save();
 		return 0;
 	} else if (!remoteheads->next && common->next)
@@ -1274,8 +1283,8 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 			git_committer_info(IDENT_ERROR_ON_NO_NAME);
 			printf(_("Trying really trivial in-index merge...\n"));
 			if (!read_tree_trivial(common->item->object.sha1,
-					head, remoteheads->item->object.sha1))
-				return merge_trivial();
+					head_commit->object.sha1, remoteheads->item->object.sha1))
+				return merge_trivial(head_commit);
 			printf(_("Nope.\n"));
 		}
 	} else {
@@ -1294,8 +1303,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 			 * merge_bases again, otherwise "git merge HEAD^
 			 * HEAD^^" would be missed.
 			 */
-			common_one = get_merge_bases(lookup_commit(head),
-				j->item, 1);
+			common_one = get_merge_bases(head_commit, j->item, 1);
 			if (hashcmp(common_one->item->object.sha1,
 				j->item->object.sha1)) {
 				up_to_date = 0;
@@ -1333,7 +1341,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 		int ret;
 		if (i) {
 			printf(_("Rewinding the tree to pristine...\n"));
-			restore_state(stash);
+			restore_state(head_commit->object.sha1, stash);
 		}
 		if (use_strategies_nr != 1)
 			printf(_("Trying merge strategy %s...\n"),
@@ -1345,7 +1353,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 		wt_strategy = use_strategies[i]->name;
 
 		ret = try_merge_strategy(use_strategies[i]->name,
-			common, head_arg);
+					 common, head_commit, head_arg);
 		if (!option_commit && !ret) {
 			merge_was_ok = 1;
 			/*
@@ -1387,14 +1395,15 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 	 * auto resolved the merge cleanly.
 	 */
 	if (automerge_was_ok)
-		return finish_automerge(common, result_tree, wt_strategy);
+		return finish_automerge(head_commit, common, result_tree,
+					wt_strategy);
 
 	/*
 	 * Pick the result from the best strategy and have the user fix
 	 * it up.
 	 */
 	if (!best_strategy) {
-		restore_state(stash);
+		restore_state(head_commit->object.sha1, stash);
 		if (use_strategies_nr > 1)
 			fprintf(stderr,
 				_("No merge strategy handled the merge.\n"));
@@ -1406,14 +1415,14 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 		; /* We already have its result in the working tree. */
 	else {
 		printf(_("Rewinding the tree to pristine...\n"));
-		restore_state(stash);
+		restore_state(head_commit->object.sha1, stash);
 		printf(_("Using the %s to prepare resolving by hand.\n"),
 			best_strategy);
-		try_merge_strategy(best_strategy, common, head_arg);
+		try_merge_strategy(best_strategy, common, head_commit, head_arg);
 	}
 
 	if (squash)
-		finish(NULL, NULL);
+		finish(head_commit, NULL, NULL);
 	else {
 		int fd;
 		struct commit_list *j;
-- 
1.7.3.1.256.g2539c.dirty

^ permalink raw reply related

* [PATCH v5 2/4] merge: use return value of resolve_ref() to determine if HEAD is invalid
From: Nguyễn Thái Ngọc Duy @ 2011-09-17 11:57 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Nguyễn Thái Ngọc Duy
In-Reply-To: <1316260665-1728-1-git-send-email-pclouds@gmail.com>

resolve_ref() only updates "head" when it returns non NULL value (it
may update "head" even when returning NULL, but not in all cases).

Because "head" is not initialized before the call, is_null_sha1() is
not enough. Check also resolve_ref() return value.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 > Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
 >> -     if (is_null_sha1(head))
 >> -             head_invalid = 1;
 >> +     if (!is_null_sha1(head)) {
 >> +             head_commit = lookup_commit(head);
 >> +             if (!head_commit)
 >> +                     die(_("could not parse HEAD"));
 >> +     }
 >
 > Is this is_null_sha1() valid without first clearing head[]?

 No it's not.

 builtin/merge.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/builtin/merge.c b/builtin/merge.c
index a068660..c371484 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -1030,7 +1030,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 	branch = resolve_ref("HEAD", head, 0, &flag);
 	if (branch && !prefixcmp(branch, "refs/heads/"))
 		branch += 11;
-	if (is_null_sha1(head))
+	if (!branch || is_null_sha1(head))
 		head_invalid = 1;
 
 	git_config(git_merge_config, NULL);
-- 
1.7.3.1.256.g2539c.dirty

^ permalink raw reply related

* [PATCH v5 1/4] merge: keep stash[] a local variable
From: Nguyễn Thái Ngọc Duy @ 2011-09-17 11:57 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Nguyễn Thái Ngọc Duy

A stash is created by save_state() and used by restore_state(). Pass
SHA-1 explicitly for clarity and keep stash[] to cmd_merge().

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 v4 was http://thread.gmane.org/gmane.comp.version-control.git/179375/focus=179706

 builtin/merge.c |   33 ++++++++++++++++-----------------
 1 files changed, 16 insertions(+), 17 deletions(-)

diff --git a/builtin/merge.c b/builtin/merge.c
index 325891e..a068660 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -50,7 +50,7 @@ static int fast_forward_only;
 static int allow_trivial = 1, have_message;
 static struct strbuf merge_msg;
 static struct commit_list *remoteheads;
-static unsigned char head[20], stash[20];
+static unsigned char head[20];
 static struct strategy **use_strategies;
 static size_t use_strategies_nr, use_strategies_alloc;
 static const char **xopts;
@@ -217,7 +217,7 @@ static void drop_save(void)
 	unlink(git_path("MERGE_MODE"));
 }
 
-static void save_state(void)
+static int save_state(unsigned char *stash)
 {
 	int len;
 	struct child_process cp;
@@ -236,11 +236,12 @@ static void save_state(void)
 
 	if (finish_command(&cp) || len < 0)
 		die(_("stash failed"));
-	else if (!len)
-		return;
+	else if (!len)		/* no changes */
+		return -1;
 	strbuf_setlen(&buffer, buffer.len-1);
 	if (get_sha1(buffer.buf, stash))
 		die(_("not a valid object: %s"), buffer.buf);
+	return 0;
 }
 
 static void read_empty(unsigned const char *sha1, int verbose)
@@ -278,7 +279,7 @@ static void reset_hard(unsigned const char *sha1, int verbose)
 		die(_("read-tree failed"));
 }
 
-static void restore_state(void)
+static void restore_state(const unsigned char *stash)
 {
 	struct strbuf sb = STRBUF_INIT;
 	const char *args[] = { "stash", "apply", NULL, NULL };
@@ -1010,6 +1011,7 @@ static int setup_with_upstream(const char ***argv)
 int cmd_merge(int argc, const char **argv, const char *prefix)
 {
 	unsigned char result_tree[20];
+	unsigned char stash[20];
 	struct strbuf buf = STRBUF_INIT;
 	const char *head_arg;
 	int flag, head_invalid = 0, i;
@@ -1320,21 +1322,18 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 	 * sync with the head commit.  The strategies are responsible
 	 * to ensure this.
 	 */
-	if (use_strategies_nr != 1) {
-		/*
-		 * Stash away the local changes so that we can try more
-		 * than one.
-		 */
-		save_state();
-	} else {
-		memcpy(stash, null_sha1, 20);
-	}
+	if (use_strategies_nr == 1 ||
+	    /*
+	     * Stash away the local changes so that we can try more than one.
+	     */
+	    save_state(stash))
+		hashcpy(stash, null_sha1);
 
 	for (i = 0; i < use_strategies_nr; i++) {
 		int ret;
 		if (i) {
 			printf(_("Rewinding the tree to pristine...\n"));
-			restore_state();
+			restore_state(stash);
 		}
 		if (use_strategies_nr != 1)
 			printf(_("Trying merge strategy %s...\n"),
@@ -1395,7 +1394,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 	 * it up.
 	 */
 	if (!best_strategy) {
-		restore_state();
+		restore_state(stash);
 		if (use_strategies_nr > 1)
 			fprintf(stderr,
 				_("No merge strategy handled the merge.\n"));
@@ -1407,7 +1406,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 		; /* We already have its result in the working tree. */
 	else {
 		printf(_("Rewinding the tree to pristine...\n"));
-		restore_state();
+		restore_state(stash);
 		printf(_("Using the %s to prepare resolving by hand.\n"),
 			best_strategy);
 		try_merge_strategy(best_strategy, common, head_arg);
-- 
1.7.3.1.256.g2539c.dirty

^ permalink raw reply related

* [PATCH] Prevent users from adding the file that has all-zero SHA-1
From: Nguyễn Thái Ngọc Duy @ 2011-09-17 11:39 UTC (permalink / raw)
  To: git; +Cc: Nguyễn Thái Ngọc Duy

This particular SHA-1 has special meaning to git, very much like NULL
in C. If a user adds a file that has this SHA-1, unexpected things can
happen.

Granted, the chance is probably near zero because the content must
also start with valid blob header. But extra safety does not harm.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 Another way than die() is to detect this situation and update header a
 little to give different SHA-1 (for example a leading 0 in object
 size in header). Older git versions may not be happy with such an
 approach.

 The same check can be added to commit, tree, tag creation and fsck.
 Maybe I'm too paranoid.

 By the way, are any other SHA-1s sensitive to git like this one?

 sha1_file.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/sha1_file.c b/sha1_file.c
index 064a330..76be0dd 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -2748,6 +2748,11 @@ int index_fd(unsigned char *sha1, int fd, struct stat *st,
 	else
 		ret = index_stream(sha1, fd, size, type, path, flags);
 	close(fd);
+	if (!ret && is_null_sha1(sha1))
+		die(_("You are very unluckly.\n"
+		      "You cannot add '%s' because this particular SHA-1 is used internally by git.\n"
+		      "Any chance you can modify this file just a little to give different SHA-1?"),
+		    path);
 	return ret;
 }
 
-- 
1.7.3.1.256.g2539c.dirty

^ permalink raw reply related

* Re: [RFC/PATCH] Configurable hyperlinking in gitk
From: Chris Packham @ 2011-09-17 10:01 UTC (permalink / raw)
  To: Jeff Epler; +Cc: git
In-Reply-To: <4E7467B7.1090201@gmail.com>

On 17/09/11 21:26, Chris Packham wrote:
> Hi,
> 
> On 17/09/11 14:29, Jeff Epler wrote:
>> Some time ago I hardcoded this into gitk for $DAY_JOB and find it very
>> useful.  I made it configurable in the hopes that it might be adopted
>> upstream. (unfortunately, the configurable version is radically
>> different from the original hard-coded version, so I can't say this
>> has had much testing yet)
> 
> This is definitely something folks at my $dayjob would be interested in.
> We've already done some customisation of gitweb to do something similar.
> I'm not actually sure what the changes where or how configurable they
> are. I'll see if I can dig them out on Monday someone else might want to
> polish them into something suitable (I might do it myself if I get some
> tuits).
> 
>> The definition of the allowed regular expression in the docs
>> probably needs some refinement.  Basically, they have to also be REs
>> that can be concatenated with the "|" character, which is not true
>> of REs that begin with the *** flavor selector (which I had not
>> heard of before rereading `man re_syntax` just now) or (?xyz)
>> embedded options.  Or maybe there's an efficient alternate approach
>> to scanning for the next non-overlapping match among several
>> patterns that doesn't involve concatenating the patterns.
>>
>> I'm not sure about the "one line" restriction; at first I thought
>> that everything was fed to 'appendwithlinks' in arbitrary chunks,
>> but not I see that they are mostly logical chunks (and probably only
>> the comment, not the headers or commit descriptors, will have
>> anything to linkify).  The problem again seems to be how to succinctly
>> describe what is permitted.
> 
> For my use case the one line restriction is fine. We tend to put the bug
> number in the headline anyway.
> 
> Sometimes when a commit fixes multiple bugs we put all the bug numbers
> in separated by commas. I don't know Tcl well enough to tell if your
> code supports that or not.
> 
>> There are probably better names for the configuration options, too.
> 
> It'd be nice if the config variables weren't gitk specific. .re and .sub
> could be applied to gitweb and maybe other git viewers outside of
> gig.git might decide to use them. My bikeshedding suggestion would be to
> just drop the gitk prefix and have linkify.re and linkify.sub.

That should be linkify.<name>.re and linkify.<name>.sub

>> Suggestions?  Problems?  Successes?
> 
> Re-compiling now. I won't be able to actually test it properly until I'm
> back in the office but I can at least check that the links are generated.

Slight complication. The URL of our bug tracker has an ampersand '&' in
it. Tcl's substitution does what one might expect and puts the matched
text where the '&' is. I've tried using url friendly %26 but something
eats the %. I've also tried backslashes to no avail.

To answer my own question since I started writing this email I've found
that using %% works (only the first one gets eaten). Not sure if that's
expected behaviour or not (printf escaping maybe?).

Also since I've been playing around I've tried a commit with multiple
bug numbers on one line and that works as expected.

Thanks
Chris

^ permalink raw reply

* Re: [RFC/PATCH] Configurable hyperlinking in gitk
From: Chris Packham @ 2011-09-17  9:26 UTC (permalink / raw)
  To: Jeff Epler; +Cc: git
In-Reply-To: <20110917022903.GA2445@unpythonic.net>

Hi,

On 17/09/11 14:29, Jeff Epler wrote:
> Some time ago I hardcoded this into gitk for $DAY_JOB and find it very
> useful.  I made it configurable in the hopes that it might be adopted
> upstream. (unfortunately, the configurable version is radically
> different from the original hard-coded version, so I can't say this
> has had much testing yet)

This is definitely something folks at my $dayjob would be interested in.
We've already done some customisation of gitweb to do something similar.
I'm not actually sure what the changes where or how configurable they
are. I'll see if I can dig them out on Monday someone else might want to
polish them into something suitable (I might do it myself if I get some
tuits).

> The definition of the allowed regular expression in the docs
> probably needs some refinement.  Basically, they have to also be REs
> that can be concatenated with the "|" character, which is not true
> of REs that begin with the *** flavor selector (which I had not
> heard of before rereading `man re_syntax` just now) or (?xyz)
> embedded options.  Or maybe there's an efficient alternate approach
> to scanning for the next non-overlapping match among several
> patterns that doesn't involve concatenating the patterns.
> 
> I'm not sure about the "one line" restriction; at first I thought
> that everything was fed to 'appendwithlinks' in arbitrary chunks,
> but not I see that they are mostly logical chunks (and probably only
> the comment, not the headers or commit descriptors, will have
> anything to linkify).  The problem again seems to be how to succinctly
> describe what is permitted.

For my use case the one line restriction is fine. We tend to put the bug
number in the headline anyway.

Sometimes when a commit fixes multiple bugs we put all the bug numbers
in separated by commas. I don't know Tcl well enough to tell if your
code supports that or not.

> There are probably better names for the configuration options, too.

It'd be nice if the config variables weren't gitk specific. .re and .sub
could be applied to gitweb and maybe other git viewers outside of
gig.git might decide to use them. My bikeshedding suggestion would be to
just drop the gitk prefix and have linkify.re and linkify.sub.

> Suggestions?  Problems?  Successes?

Re-compiling now. I won't be able to actually test it properly until I'm
back in the office but I can at least check that the links are generated.

^ permalink raw reply

* Merging back from master but keeping a subtree
From: Steinar Bang @ 2011-09-17  7:49 UTC (permalink / raw)
  To: git

I have a long lived branch that changes a directory and its
subdirectory, ie. 
 top/middle/mydirectory

Now I want to merge in an updated remoterepo/master and keep everything
from that master, except for mydirectory and its subdirectory, where I
would like to keep everything from my branch.

I tried a regular merge, and used
 git checkout --ours
 git add
and 
 git checkout --theirs
 git add
as appropriate on all conflicts.

But the result didn't build, and the build errors don't make much sense,
so I think they are caused by "successful" merges giving bad results.

Is there a better way to do this?

Would it be possible to unstage the already staged files and apply the
"checkout --ours" and "checkout --theirs", and then git add on the
checked out files?

Even that would be clumsy... I would have preferred something like
 git checkout --theirs top
 git checkout --ours top/middle/mydirectory
 git add-only-those-modified-wrt-my-branch


Thanks!

- Steinar

^ permalink raw reply

* Re: zealous git convert determined to set up git server
From: Sitaram Chamarty @ 2011-09-17  2:45 UTC (permalink / raw)
  To: Jakub Narebski; +Cc: Joshua Stoutenburg, Git List
In-Reply-To: <201109170039.22954.jnareb@gmail.com>

On Sat, Sep 17, 2011 at 4:09 AM, Jakub Narebski <jnareb@gmail.com> wrote:
> Joshua Stoutenburg wrote:
>> 2011/9/15 Jakub Narebski <jnareb@gmail.com>:

>> Question 2: It seems gitolite is the popular choice for git user
>> management.  Any reason why?
>
> From Gitosis and Gitolite, both git repository management tools, Gitosis
> requires setuptools beside Python, and looks like it is not developed
> anymore, while Gitolite (which started as rewrite of Gitosis in Perl)
> requires only Perl and is actively developed.
>
> Nb. even Gitosis author recommends Gitolite:
> http://scie.nti.st/2007/11/14/hosting-git-repositories-the-easy-and-secure-way
>
>  Update (12-12-2010): For additional features not present in gitosis,
>  check out gitolite.

Minor correction.  That blog is someone else's (a friend of the
gitosis author).  I asked him to add that text because we constantly
had (still have) people land up on #git asking for help but neither he
nor the author are active in git-land anymore and won't answer
questions and no one else was willing to pick up the slack.

Fortunately he agreed, and was gracious enough to put that in,
although I would have preferred wording that includes the word
"unsupported" (IMO lack of support is a bigger reason than lack of
features).

^ permalink raw reply

* Re: zealous git convert determined to set up git server
From: Sitaram Chamarty @ 2011-09-17  2:40 UTC (permalink / raw)
  To: Andreas Krey; +Cc: Git List
In-Reply-To: <20110916204032.GA13922@inner.h.iocl.org>

On Sat, Sep 17, 2011 at 2:10 AM, Andreas Krey <a.krey@gmx.de> wrote:
> On Fri, 16 Sep 2011 22:30:35 +0000, Sitaram Chamarty wrote:
> ...
>> Well it *is* pretty darn powerful (I'm the author; allow me some
>> preening!) but I believe the real reason is that it is the most
>> *transparent* solution.
>
> It well looks so, but I have a question: It seems that it assumes a
> flat set of *.git repos. Unfortunately my current setup has the repos

No.  Perhaps I should state that explicitly somewhere though...

^ permalink raw reply

* [RFC/PATCH] Configurable hyperlinking in gitk
From: Jeff Epler @ 2011-09-17  2:29 UTC (permalink / raw)
  To: git

Many projects use project-specific notations in comments to refer to
bug trackers and the like.  One example is the "Closes: #nnnnn"
notation used in Debian.

Make gitk configurable so that arbitrary strings can be turned into
clickable links that are opened in a web browser.
---
Some time ago I hardcoded this into gitk for $DAY_JOB and find it very
useful.  I made it configurable in the hopes that it might be adopted
upstream. (unfortunately, the configurable version is radically
different from the original hard-coded version, so I can't say this
has had much testing yet)

The definition of the allowed regular expression in the docs
probably needs some refinement.  Basically, they have to also be REs
that can be concatenated with the "|" character, which is not true
of REs that begin with the *** flavor selector (which I had not
heard of before rereading `man re_syntax` just now) or (?xyz)
embedded options.  Or maybe there's an efficient alternate approach
to scanning for the next non-overlapping match among several
patterns that doesn't involve concatenating the patterns.

I'm not sure about the "one line" restriction; at first I thought
that everything was fed to 'appendwithlinks' in arbitrary chunks,
but not I see that they are mostly logical chunks (and probably only
the comment, not the headers or commit descriptors, will have
anything to linkify).  The problem again seems to be how to succinctly
describe what is permitted.

There are probably better names for the configuration options, too.

Suggestions?  Problems?  Successes?

Jeff


 Documentation/config.txt |   31 ++++++++++++++++++-
 gitk-git/gitk            |   74 +++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 102 insertions(+), 3 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 750c86d..67ed436 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1102,6 +1102,33 @@ All gitcvs variables except for 'gitcvs.usecrlfattr' and
 is one of "ext" and "pserver") to make them apply only for the given
 access method.
 
+gitk.linkify.<name>.re::
+	Specify a Tcl regular expression (which may not span lines)
+	defining a class of strings to automatically convert to hyperlinks.
+	You must also specify 'gitk.linkify.<name>.sub'.
+
+gitk.linkify.<name>.sub::
+	Specify a substitution that results in the target URL for the
+	related regular expression.  Back-references like '\1' refer
+	to capturing groups in the associated regular expression.
+	You must also specify 'gitk.linkify.<name>.re'.
+
+gitk.browser::
+	Specify the browser that will be used to display the linked
+	web page.
+
+For example, to automatically link from Debian-style "Closes: #nnnn"
+message to the Debian BTS,
+
+--------
+    git config gitk.linkify.debian-bts.re 'Closes: #(\d+)'
+    git config gitk.linkify.debian-bts.sub 'http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=\1'
+--------
+
+Regular expressions are as described in re_syntax(n).  Replacements
+are as described in regsub(n).  If multiple regular expressions match at
+the same location, it is undefined which match is used.
+
 grep.lineNumber::
 	If set to true, enable '-n' option by default.
 
@@ -1901,5 +1928,5 @@ user.signingkey::
 
 web.browser::
 	Specify a web browser that may be used by some commands.
-	Currently only linkgit:git-instaweb[1] and linkgit:git-help[1]
-	may use it.
+	Currently only linkgit:git-instaweb[1], linkgit:gitk[1],
+	and linkgit:git-help[1] may use it.
diff --git a/gitk-git/gitk b/gitk-git/gitk
index 4cde0c4..a21eea1 100755
--- a/gitk-git/gitk
+++ b/gitk-git/gitk
@@ -6684,7 +6684,7 @@ proc commit_descriptor {p} {
 # append some text to the ctext widget, and make any SHA1 ID
 # that we know about be a clickable link.
 proc appendwithlinks {text tags} {
-    global ctext linknum curview
+    global ctext linknum curview linkmakers
 
     set start [$ctext index "end - 1c"]
     $ctext insert end $text $tags
@@ -6699,6 +6699,30 @@ proc appendwithlinks {text tags} {
 	setlink $linkid link$linknum
 	incr linknum
     }
+
+    if {$linkmakers == {}} return
+
+    set link_re {}
+    foreach {re rep} $linkmakers { lappend link_re $re }
+    set link_re "([join $link_re {)|(}])"
+
+    set ee 0
+    while {[regexp -indices -start $ee -- $link_re $text l]} {
+	set s [lindex $l 0]
+	set e [lindex $l 1]
+	set linktext [string range $text $s $e]
+	incr e
+	set ee $e
+
+	foreach {re rep} $linkmakers {
+	    if {![regsub $re $linktext $rep linkurl]} continue
+	    $ctext tag delete link$linknum
+	    $ctext tag add link$linknum "$start + $s c" "$start + $e c"
+	    seturllink $linkurl link$linknum
+	    incr linknum
+	    break
+	}
+    }
 }
 
 proc setlink {id lk} {
@@ -6726,6 +6750,52 @@ proc setlink {id lk} {
     }
 }
 
+proc get_link_config {} {
+    if {[catch {exec git config -z --get-regexp {^gitk\.linkify\.}} linkers]} {
+	return {}
+    }
+
+    set linktypes [list]
+    foreach item [split $linkers "\0"] {
+	if {$item == ""} continue
+	if {![regexp {gitk\.linkify\.(\S+)\.(re|sub)\s(.*)} $item _ k t v]} {
+	    continue
+	}
+	set linkconfig($t,$k) $v
+	if {$t == "re"} { lappend linktypes $k }
+    }
+
+    set linkmakers [list]
+    foreach k $linktypes {
+	if {![info exists linkconfig(sub,$k)]} {
+	    puts stderr "Warning: link `$k' is missing a substitution string"
+	} elseif {[catch {regexp -inline -- $linkconfig(re,$k) ""} err]} {
+	    puts stderr "Warning: link `$k': $err"
+	} else {
+	    lappend linkmakers $linkconfig(re,$k) $linkconfig(sub,$k)
+	}
+	unset linkconfig(re,$k)
+	unset -nocomplain linkconfig(sub,$k)
+    }
+    foreach k [array names linkconfig] {
+	regexp "sub,(.*)" $k _ k
+	puts stderr "Warning: link `$k' is missing a regular expression"
+    }
+    set linkmakers
+}
+
+proc openlink {url} {
+    exec git web--browse --config=gitk.browser $url &
+}
+
+proc seturllink {url lk} {
+    global ctext
+    $ctext tag conf $lk -foreground blue -underline 1
+    $ctext tag bind $lk <1> [list openlink $url]
+    $ctext tag bind $lk <Enter> {linkcursor %W 1}
+    $ctext tag bind $lk <Leave> {linkcursor %W -1}
+}
+
 proc appendshortlink {id {pre {}} {post {}}} {
     global ctext linknum
 
@@ -11693,6 +11763,8 @@ if {[tk windowingsystem] eq "win32"} {
     focus -force .
 }
 
+set linkmakers [get_link_config]
+
 getcommits {}
 
 # Local variables:
-- 
1.7.0.4

^ permalink raw reply related

* Re: git branch --set-upstream regression in master
From: Jay Soffian @ 2011-09-17  0:19 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, conrad.irwin
In-Reply-To: <7vy5xocafg.fsf@alter.siamese.dyndns.org>

On Fri, Sep 16, 2011 at 7:52 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Jay Soffian <jaysoffian@gmail.com> writes:
>
>> This used to be possible on the checked out branch:
>>
>>   $ git branch --set-upstream origin/master master
>>
>> Now it gives "fatal: Cannot force update the current branch."
>> broken.
>
> Let's do this. I would be happy if I can include it in -rc2 with Tested-by:
> or something ;-)

Tested-by: Jay Soffian

Thanks for the quick turnaround. I'll try to find some time to make
--set-upstream a proper option for post 1.7.7.

j.

^ permalink raw reply

* Re: git branch --set-upstream regression in master
From: Junio C Hamano @ 2011-09-16 23:52 UTC (permalink / raw)
  To: Jay Soffian; +Cc: git, conrad.irwin
In-Reply-To: <CAG+J_DyxNpPevwfrJVkv3GBmv0tEXgW2LZtdHgarFoXb9Qqghw@mail.gmail.com>

Jay Soffian <jaysoffian@gmail.com> writes:

> This used to be possible on the checked out branch:
>
>   $ git branch --set-upstream origin/master master
>
> Now it gives "fatal: Cannot force update the current branch."
> broken.

Let's do this. I would be happy if I can include it in -rc2 with Tested-by:
or something ;-)

-- >8 --
Subject: [PATCH] branch --set-upstream: regression fix

The "git branch" command, while not in listing mode, calls create_branch()
even when the target branch already exists, and it does so even when it is
not interested in updating the value of the branch (i.e. the name of the
commit object that sits at the tip of the existing branch). This happens
when the command is run with "--set-upstream" option.

The earlier safety-measure to prevent "git branch -f $branch $commit" from
updating the currently checked out branch did not take it into account,
and we no longer can update the tracking information of the current branch.

Minimally fix this regression by telling the validation code if it is
called to really update the value of a potentially existing branch, or if
the caller merely is interested in updating auxiliary aspects of a branch.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 branch.c           |   21 ++++++++++++---------
 branch.h           |   12 +++++++++++-
 builtin/branch.c   |    2 +-
 builtin/checkout.c |    3 ++-
 t/t3200-branch.sh  |   13 +++++++++++++
 5 files changed, 39 insertions(+), 12 deletions(-)

diff --git a/branch.c b/branch.c
index 1fe3078..4338a90 100644
--- a/branch.c
+++ b/branch.c
@@ -135,23 +135,25 @@ static int setup_tracking(const char *new_ref, const char *orig_ref,
 	return 0;
 }
 
-int validate_new_branchname(const char *name, struct strbuf *ref, int force)
+int validate_new_branchname(const char *name, struct strbuf *ref,
+			    int force, int attr_only)
 {
-	const char *head;
-	unsigned char sha1[20];
-
 	if (strbuf_check_branch_ref(ref, name))
 		die("'%s' is not a valid branch name.", name);
 
 	if (!ref_exists(ref->buf))
 		return 0;
-	else if (!force)
+	else if (!force && !attr_only)
 		die("A branch named '%s' already exists.", ref->buf + strlen("refs/heads/"));
 
-	head = resolve_ref("HEAD", sha1, 0, NULL);
-	if (!is_bare_repository() && head && !strcmp(head, ref->buf))
-		die("Cannot force update the current branch.");
+	if (!attr_only) {
+		const char *head;
+		unsigned char sha1[20];
 
+		head = resolve_ref("HEAD", sha1, 0, NULL);
+		if (!is_bare_repository() && head && !strcmp(head, ref->buf))
+			die("Cannot force update the current branch.");
+	}
 	return 1;
 }
 
@@ -171,7 +173,8 @@ void create_branch(const char *head,
 	if (track == BRANCH_TRACK_EXPLICIT || track == BRANCH_TRACK_OVERRIDE)
 		explicit_tracking = 1;
 
-	if (validate_new_branchname(name, &ref, force || track == BRANCH_TRACK_OVERRIDE)) {
+	if (validate_new_branchname(name, &ref, force,
+				    track == BRANCH_TRACK_OVERRIDE)) {
 		if (!force)
 			dont_change_ref = 1;
 		else
diff --git a/branch.h b/branch.h
index 01544e2..1285158 100644
--- a/branch.h
+++ b/branch.h
@@ -20,8 +20,18 @@ void create_branch(const char *head, const char *name, const char *start_name,
  * interpreted ref in ref, force indicates whether (non-head) branches
  * may be overwritten. A non-zero return value indicates that the force
  * parameter was non-zero and the branch already exists.
+ *
+ * Contrary to all of the above, when attr_only is 1, the caller is
+ * not interested in verifying if it is Ok to update the named
+ * branch to point at a potentially different commit. It is merely
+ * asking if it is OK to change some attribute for the named branch
+ * (e.g. tracking upstream).
+ *
+ * NEEDSWORK: This needs to be split into two separate functions in the
+ * longer run for sanity.
+ *
  */
-int validate_new_branchname(const char *name, struct strbuf *ref, int force);
+int validate_new_branchname(const char *name, struct strbuf *ref, int force, int attr_only);
 
 /*
  * Remove information about the state of working on the current
diff --git a/builtin/branch.c b/builtin/branch.c
index 40f885c..5fb3d85 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -566,7 +566,7 @@ static void rename_branch(const char *oldname, const char *newname, int force)
 			die(_("Invalid branch name: '%s'"), oldname);
 	}
 
-	validate_new_branchname(newname, &newref, force);
+	validate_new_branchname(newname, &newref, force, 0);
 
 	strbuf_addf(&logmsg, "Branch: renamed %s to %s",
 		 oldref.buf, newref.buf);
diff --git a/builtin/checkout.c b/builtin/checkout.c
index ddefec0..909a334 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -1072,7 +1072,8 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
 	if (opts.new_branch) {
 		struct strbuf buf = STRBUF_INIT;
 
-		opts.branch_exists = validate_new_branchname(opts.new_branch, &buf, !!opts.new_branch_force);
+		opts.branch_exists = validate_new_branchname(opts.new_branch, &buf,
+							     !!opts.new_branch_force, 0);
 
 		strbuf_release(&buf);
 	}
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index cb6458d..7633930 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -554,4 +554,17 @@ test_expect_success 'attempt to delete a branch merged to its base' '
 	test_must_fail git branch -d my10
 '
 
+test_expect_success 'use set-upstream on the current branch' '
+	git checkout master &&
+	git --bare init myupstream.git &&
+	git push myupstream.git master:refs/heads/frotz &&
+	git remote add origin myupstream.git &&
+	git fetch &&
+	git branch --set-upstream master origin/frotz &&
+
+	test "z$(git config branch.master.remote)" = "zorigin" &&
+	test "z$(git config branch.master.merge)" = "zrefs/heads/frotz"
+
+'
+
 test_done
-- 
1.7.7.rc1.4.g26e426

^ permalink raw reply related

* Re: git branch --set-upstream regression in master
From: Junio C Hamano @ 2011-09-16 23:14 UTC (permalink / raw)
  To: Jay Soffian; +Cc: git, conrad.irwin
In-Reply-To: <7v7h58dri4.fsf@alter.siamese.dyndns.org>

Junio C Hamano <gitster@pobox.com> writes:

> I took a brief look at --set-upstream codepath, and I have to say that the
> implementation is totally broken with respect to an existing branch.
>
> Given
>
> 	$ git branch master --set-upstream origin/master
>
> it passes the exact same codepath as
>
> 	$ git branch master origin/master
>
> uses, only with a different "track" flag, no?  That is, it calls a
> function that is meant to _create_ branch "master" from given branch point
> "origin/master", namely create_branch().  And then create_branch(),
> contrary to its name, is littered with "dont_change_ref" special case to
> work it around, depending on the value of "track".

So here is a quick-and-dirty patch, which may or may not compile or pass
tests.

 branch.c           |   21 ++++++++++++---------
 branch.h           |   12 +++++++++++-
 builtin/branch.c   |    2 +-
 builtin/checkout.c |    3 ++-
 4 files changed, 26 insertions(+), 12 deletions(-)

diff --git a/branch.c b/branch.c
index 478d825..fecedd3 100644
--- a/branch.c
+++ b/branch.c
@@ -135,23 +135,25 @@ static int setup_tracking(const char *new_ref, const char *orig_ref,
 	return 0;
 }
 
-int validate_new_branchname(const char *name, struct strbuf *ref, int force)
+int validate_new_branchname(const char *name, struct strbuf *ref,
+			    int force, int attr_only)
 {
-	const char *head;
-	unsigned char sha1[20];
-
 	if (strbuf_check_branch_ref(ref, name))
 		die("'%s' is not a valid branch name.", name);
 
 	if (!ref_exists(ref->buf))
 		return 0;
-	else if (!force)
+	else if (!force && !attr_only)
 		die("A branch named '%s' already exists.", ref->buf + strlen("refs/heads/"));
 
-	head = resolve_ref("HEAD", sha1, 0, NULL);
-	if (!is_bare_repository() && head && !strcmp(head, ref->buf))
-		die("Cannot force update the current branch.");
+	if (!attr_only) {
+		const char *head;
+		unsigned char sha1[20];
 
+		head = resolve_ref("HEAD", sha1, 0, NULL);
+		if (!is_bare_repository() && head && !strcmp(head, ref->buf))
+			die("Cannot force update the current branch.");
+	}
 	return 1;
 }
 
@@ -171,7 +173,8 @@ void create_branch(const char *head,
 	if (track == BRANCH_TRACK_EXPLICIT || track == BRANCH_TRACK_OVERRIDE)
 		explicit_tracking = 1;
 
-	if (validate_new_branchname(name, &ref, force || track == BRANCH_TRACK_OVERRIDE)) {
+	if (validate_new_branchname(name, &ref, force,
+				    track == BRANCH_TRACK_OVERRIDE)) {
 		if (!force)
 			dont_change_ref = 1;
 		else
diff --git a/branch.h b/branch.h
index 01544e2..1285158 100644
--- a/branch.h
+++ b/branch.h
@@ -20,8 +20,18 @@ void create_branch(const char *head, const char *name, const char *start_name,
  * interpreted ref in ref, force indicates whether (non-head) branches
  * may be overwritten. A non-zero return value indicates that the force
  * parameter was non-zero and the branch already exists.
+ *
+ * Contrary to all of the above, when attr_only is 1, the caller is
+ * not interested in verifying if it is Ok to update the named
+ * branch to point at a potentially different commit. It is merely
+ * asking if it is OK to change some attribute for the named branch
+ * (e.g. tracking upstream).
+ *
+ * NEEDSWORK: This needs to be split into two separate functions in the
+ * longer run for sanity.
+ *
  */
-int validate_new_branchname(const char *name, struct strbuf *ref, int force);
+int validate_new_branchname(const char *name, struct strbuf *ref, int force, int attr_only);
 
 /*
  * Remove information about the state of working on the current
diff --git a/builtin/branch.c b/builtin/branch.c
index aa705a0..f49596f 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -566,7 +566,7 @@ static void rename_branch(const char *oldname, const char *newname, int force)
 			die(_("Invalid branch name: '%s'"), oldname);
 	}
 
-	validate_new_branchname(newname, &newref, force);
+	validate_new_branchname(newname, &newref, force, 0);
 
 	strbuf_addf(&logmsg, "Branch: renamed %s to %s",
 		 oldref.buf, newref.buf);
diff --git a/builtin/checkout.c b/builtin/checkout.c
index 3bb6525..5e356a6 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -1073,7 +1073,8 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
 	if (opts.new_branch) {
 		struct strbuf buf = STRBUF_INIT;
 
-		opts.branch_exists = validate_new_branchname(opts.new_branch, &buf, !!opts.new_branch_force);
+		opts.branch_exists = validate_new_branchname(opts.new_branch, &buf,
+							     !!opts.new_branch_force, 0);
 
 		strbuf_release(&buf);
 	}

^ permalink raw reply related


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox