Fwd: Re: [PATCH v3] git-p4: add "--path-encoding" option
Sorry if this is possible re-sending
Forwarded Message
Subject:Re: [PATCH v3] git-p4: add "--path-encoding" option
Date: Tue, 1 Sep 2015 06:37:59 +0200
From: Torsten Bögershausen
To: larsxschnei...@gmail.com, git@vger.kernel.org
CC: l...@diamand.org, gits...@pobox.com
On 01/09/15 00:10, larsxschnei...@gmail.com wrote:
From: Lars Schneider
Signed-off-by: Lars Schneider
---
Documentation/git-p4.txt| 5 +
git-p4.py | 6 ++
t/t9821-git-p4-path-encoding.sh | 38 ++
3 files changed, 49 insertions(+)
create mode 100755 t/t9821-git-p4-path-encoding.sh
diff --git a/Documentation/git-p4.txt b/Documentation/git-p4.txt
index 82aa5d6..14bb79c 100644
--- a/Documentation/git-p4.txt
+++ b/Documentation/git-p4.txt
@@ -252,6 +252,11 @@ Git repository:
Use a client spec to find the list of interesting files in p4.
See the "CLIENT SPEC" section below.
+--path-encoding ::
+ The encoding to use when reading p4 client paths. With this option
+ non ASCII paths are properly stored in Git. For example, the encoding
+ 'cp1252' is often used on Windows systems.
+
-/ ::
Exclude selected depot paths when cloning or syncing.
diff --git a/git-p4.py b/git-p4.py
index 073f87b..2b3bfc4 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -1981,6 +1981,8 @@ class P4Sync(Command, P4UserMap):
optparse.make_option("--silent", dest="silent",
action="store_true"),
optparse.make_option("--detect-labels", dest="detectLabels",
action="store_true"),
optparse.make_option("--import-labels", dest="importLabels",
action="store_true"),
+optparse.make_option("--path-encoding", dest="pathEncoding",
type="string",
+ help="Encoding to use for paths"),
optparse.make_option("--import-local", dest="importIntoRemotes",
action="store_false",
help="Import into refs/heads/ , not
refs/remotes"),
optparse.make_option("--max-changes", dest="maxChanges",
@@ -2025,6 +2027,7 @@ class P4Sync(Command, P4UserMap):
self.clientSpecDirs = None
self.tempBranches = []
self.tempBranchLocation = "git-p4-tmp"
+self.pathEncoding = None
if gitConfig("git-p4.syncFromOrigin") == "false":
self.syncWithOrigin = False
@@ -2213,6 +2216,9 @@ class P4Sync(Command, P4UserMap):
text = regexp.sub(r'$\1$', text)
contents = [ text ]
+if self.pathEncoding:
+relPath = relPath.decode(self.pathEncoding).encode('utf8',
'replace')
+
self.gitStream.write("M %s inline %s\n" % (git_mode, relPath))
# total length...
diff --git a/t/t9821-git-p4-path-encoding.sh b/t/t9821-git-p4-path-encoding.sh
new file mode 100755
index 000..1626fc5
--- /dev/null
+++ b/t/t9821-git-p4-path-encoding.sh
@@ -0,0 +1,38 @@
+#!/bin/sh
+
+test_description='Clone repositories with non ASCII paths'
+
+. ./lib-git-p4.sh
+
+UTF8_ESCAPED="a-\303\244_o-\303\266_u-\303\274.txt"
+
+test_expect_success 'start p4d' '
+ start_p4d
+'
+
+test_expect_success 'Create a repo containing iso8859-1 encoded paths' '
+ cd "$cli" &&
+
+ ISO8859="$(printf "$UTF8_ESCAPED" | iconv -f utf-8 -t iso8859-1)" &&
+ >"$ISO8859" &&
+ p4 add "$ISO8859" &&
+ p4 submit -d "test commit"
+'
Sorry for being persistant,
but you can't create files with names that are ISO-8859-1 encoded under Mac OS,
we end up like this:
a-%E4_o-%F6_u-%FC.txt
(And I'm still not convinced, that we need to call iconv each time we execute
the TC,
for a string that is always the same.
The string can be converted once, and embedded in the TC:
The following should work under Mac OS (but I don't have p4 to test it)
ISO8859_ESCAPED="a-\303\244_o-\303\266_u-\303\274.txt"
UTF8_ESCAPED="\141\055\303\203\302\244\137\157\055\303\203\302\266\137\165\055\303\203\302\274\056\164\170\164"
ISO8859=$(printf "$ISO8859_ESCAPED")
+
+test_expect_success 'Clone repo containing iso8859-1 encoded paths' '
+ git p4 clone --destination="$git" --path-encoding=iso8859-1 //depot &&
+ test_when_finished cleanup_git &&
+ (
+ cd "$git" &&
+ printf "$UTF8_ESCAPED\n" >expect &&
+ test_config core.quotepath false &&
+ git ls-files >actual &&
+ test_cmp expect actual
+ )
+'
The ls-files can be written shorter (if we like short code)
+ git -c core.quotepath=false ls-files >actual &&
--
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
Git Deployment Multiple Existing Environments
Hello, I am kind of new to Git and I have a question regarding using Git for a website. I have searched a lot but haven't found a solution yet. We already have 3-4 environments setup on our Windows servers without Git and each environment already has code which is different from each other. There are three environments Live UAT Test (has the latest code) And then developers have their local copies. We write and test the code locally and manually move each point from one environment to other using merging software and test at each environment. Now we want to use git because manually moving the code is a lengthy process. Also as the developers have local copies, it is very difficult to manage code. Code is written locally by the team and then after testing locally it is first merged with "Test" environment code, then "UAT" and then, finally with "Live". So we have two concerns: There is different code already existing on these environments. Testing the code on each environment using the web server. What is the best way to go about it? As I am new to git more details will be helpful, like commands to use. Thanks, -- 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
Re: Git Deployment Multiple Existing Environments
On Tuesday, September 01, 2015 09:50:45 AM Sukhwinder Singh wrote: > Hello, > I am kind of new to Git and I have a question regarding using Git for a > website. I have searched a lot but haven't found a solution yet. We > already have 3-4 environments setup on our Windows servers without Git and > each environment already has code which is different from each other. > > There are three environments > Live > UAT > Test (has the latest code) > > > And then developers have their local copies. > > We write and test the code locally and manually move each point from one > environment to other using merging software and test at each environment. > Now we want to use git because manually moving the code is a lengthy > process. Also as the developers have local copies, it is very difficult to > manage code. > > Code is written locally by the team and then after testing locally it is > first merged with "Test" environment code, then "UAT" and then, finally > with "Live". So we have two concerns: > > There is different code already existing on these environments. > Testing the code on each environment using the web server. > > What is the best way to go about it? As I am new to git more details will > be helpful, like commands to use. > > Thanks, -- There are douzends of different git deployment scripts on github from minimal to multi-data-center-enterprise-grade. I wrote this one for my workplace: https://github.com/comsolit/comsolit_deploy The README contains links to alternative solutions. Thomas Koch -- 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
Re: [PATCH 2/2] date: make "local" orthogonal to date format
On Mon, Aug 31, 2015 at 06:05:09PM -0400, Jeff King wrote:
> On Mon, Aug 31, 2015 at 05:33:37PM -0400, Jeff King wrote:
>
> > > diff --git a/date.c b/date.c
> > > index aa57cad..3aa8002 100644
> > > --- a/date.c
> > > +++ b/date.c
> > > @@ -817,9 +817,7 @@ void parse_date_format(const char *format, struct
> > > date_mode *mode)
> > > if (!skip_prefix(p, ":", &p))
> > > die("date format missing colon separator: %s", format);
> > > mode->strftime_fmt = xstrdup(p);
> > > - }
> > > -
> > > - if (*p)
> > > + } else if (*p)
> > > die("unknown date-mode modifier: %s", p);
> >
> > Yeah, that works. We could also advance "p" in the DATE_STRFTIME
> > conditional, but I think your solution is less ugly.
> >
> > Thanks for debugging my mess.
>
> By the way, I was imagining you would pick these up and add to them with
> more tests and documentation. If that's the case, please feel free to
> squash that in and keep my signoff. If not, then I can post a re-roll
> after waiting for other comments.
OK, I'll send them with some additions to t6300 built on top, although
it may take a couple of days.
The other documentation improvements feel like an independent topic that
isn't necessary for this series to graduate; I'd prefer not to block
this waiting for those changes.
--
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
Re: Git's inconsistent command line options
On Mon, Aug 31, 2015 at 10:25:58AM -0400, Barry Warsaw wrote: > On Aug 31, 2015, at 05:10 PM, Duy Nguyen wrote: > > >I'm probably shot down for this. But could we go with a clean plate > >and create a new command prefix (something like git-next, git2, or > >gt...)? We could then redesign the entire UI without worrying about > >backward compatibility. At some point we can start to deprecate "git" > >and encourage to use the new command prefix only. Of course somebody > >has to go over all the commands and options to propose some consistent > >UI, then more discussions and coding so it could likely follow the > >path of pack v4.. > > `git` itself could also be a thin wrapper which consulted a configuration > variable to see which version of the ui to expose. > > "All problems in computer science can be solved by another level of > indirection" Except for poor performance, simplicity, and bad ideas. The Git project does not break backwards compatibility. Let's let Python3 serve as a good lesson on why not to do that! ;-p While a script writer could write, "git -c core.cliversion=1 ...", no one does that, no one wants to do that, and it just seems like a bad idea that's best left unexplored. The only idea in this thread that's user-friendly would be a new Git that still supported the entirety of the existing, perfectly-good CLI interface and *also* accepted some new "consistent" user interface. Otherwise, this entire thread seems like a big non-issue. The existing CLI hasn't hurt adoption, and tossing a config option at it only makes it worse. The best config is no config. There really are ony a few corner cases that would need to be tweaked to support --named-subcommands style, and after that is done, is Git really that much easier to use? Maybe a little bit, but not enough that warrants breaking existing scripts IMO. --- David -- 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
git dockerfile.
Hi, Greetings everybody! I am looking for a Dockerfile for git which will _build_ git from source on ppc64le platform. I want to build git with different versions (say top-of-the-tree, latest-stable etc.) and it would be good if there is a dockerfile present alongwith the code which can build the source code with minimal changes (or better if there is a dockerfile available for each released version). This will avoid many incoherent copies of dockerfile floating around the Net. I tried to search on Internet but haven't come across any dockerfile which will build git source. I would highly appreciate if somebody could point me to a source where such dockerfile is available. I saw some communication on git mailing list, but so far haven't found any docker file. If there is no dockerfile available, I am willing to create one for ppc64le and commit it to git source. Please advise. Thanks, Atul. -- 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
Re: [PATCH v3] git-p4: add "--path-encoding" option
On 01 Sep 2015, at 06:37, Torsten Bögershausen wrote: > On 01/09/15 00:10, larsxschnei...@gmail.com wrote: >> From: Lars Schneider >> >> Signed-off-by: Lars Schneider >> --- >> Documentation/git-p4.txt| 5 + >> git-p4.py | 6 ++ >> t/t9821-git-p4-path-encoding.sh | 38 ++ >> 3 files changed, 49 insertions(+) >> create mode 100755 t/t9821-git-p4-path-encoding.sh >> >> > >> + >> +test_expect_success 'Create a repo containing iso8859-1 encoded paths' ' >> +cd "$cli" && >> + >> +ISO8859="$(printf "$UTF8_ESCAPED" | iconv -f utf-8 -t iso8859-1)" && >> +>"$ISO8859" && >> +p4 add "$ISO8859" && >> +p4 submit -d "test commit" >> +' > Sorry for being persistant, > but you can't create files with names that are ISO-8859-1 encoded under Mac > OS, > we end up like this: > > a-%E4_o-%F6_u-%FC.txt You are right. However, my goal is not to create a file with ISO-8859-1 characters in Mac OS. My goal is to create this file in Perforce and this approach seems to work. > > (And I'm still not convinced, that we need to call iconv each time we execute > the TC, > for a string that is always the same. > The string can be converted once, and embedded in the TC: > The following should work under Mac OS (but I don't have p4 to test it) > > ISO8859_ESCAPED="a-\303\244_o-\303\266_u-\303\274.txt" > > UTF8_ESCAPED="\141\055\303\203\302\244\137\157\055\303\203\302\266\137\165\055\303\203\302\274\056\164\170\164" > > ISO8859=$(printf "$ISO8859_ESCAPED”) OK. However, how about this? UTF8_ESCAPED="a-\303\244_o-\303\266_u-\303\274.txt" ISO8859_ESCAPED="\141\55\344\137\157\55\366\137\165\55\374\56\164\170\164" # You can generate the ISO8859_ESCAPED with the following command: # printf "$UTF8_ESCAPED" | \ # iconv -f utf-8 -t iso8859-1 | \ # xxd -ps -u -c 1 | xargs bash -c 'for v; do echo "ibase=16; obase=8; $v" | bc; done' bash | \ # tr "\n" "\\" > >> + >> +test_expect_success 'Clone repo containing iso8859-1 encoded paths' ' >> +git p4 clone --destination="$git" --path-encoding=iso8859-1 //depot && >> +test_when_finished cleanup_git && >> +( >> +cd "$git" && >> +printf "$UTF8_ESCAPED\n" >expect && >> +test_config core.quotepath false && >> +git ls-files >actual && >> +test_cmp expect actual >> +) >> +' >> > The ls-files can be written shorter (if we like short code) > > + git -c core.quotepath=false ls-files >actual && Fixed. Thank you! -- 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
Re: [PATCH v14 04/13] ref-filter: implement an `align` atom
On Mon, Aug 31, 2015 at 11:32 PM, Eric Sunshine wrote:
> On Mon, Aug 31, 2015 at 1:28 PM, Matthieu Moy
> wrote:
>> Eric Sunshine writes:
>>>
>>> Warning about unrecognized atoms may indeed be a good idea, however,
>>> the current behavior isn't a huge problem since user discovers the
>>> error when the output fails to match his expectation.
>>
>> It's a bit worse than it seems: without this change, using --format
>> '%(align)%(end)' results in Git complaining about %(end) without
>> matching atom, which is really confusing since you do have a %(align) (I
>> got it for real while testing a preliminary version).
>>
>>> This behavior of ignoring unrecognized atoms predates your work,
>>> doesn't it? If so, it's probably not something you need to address in
>>> this series.
>>
>> I wouldn't insist in having it in the series, but now that it's here, I
>> think we can keep it (if only to shorten the interdiff for the next
>> iteration).
>
> The unstated subtext in my original question is that the approach
> taken by this patch of warning only about unrecognized %(align) is too
> special-case; if it's going to warn at all, it should do so
> generically for any unrecognized %(atom). Special-casing the warning
> about malformed %(align) sets a poor precedent; it's code which will
> eventually need to be removed anyhow when the generic warning
> mechanism is eventually implemented.
Probably, I could just have a check within the align block and maybe build a
general case later.
Like this:
else if (skip_prefix(name, "align", &valp)) {
struct align *align = &v->u.align;
struct strbuf **s;
if (valp[0] != ':')
die(_("format: usage %%(align:,)"));
else
valp++;
..
}
--
Regards,
Karthik Nayak
--
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
Re: [PATCH v14 04/13] ref-filter: implement an `align` atom
Karthik Nayak writes:
> Like this:
>
> else if (skip_prefix(name, "align", &valp)) {
> struct align *align = &v->u.align;
> struct strbuf **s;
>
> if (valp[0] != ':')
> die(_("format: usage %%(align:,)"));
> else
> valp++;
> ..
>
> }
Checking the string's correctness as you read it seems good to me, yes.
But as you say, you should make this a bit more generic, for example by
putting your "if/else" in a helper function, that other atoms could use.
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
--
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
Re: [PATCH v3] git-p4: add "--path-encoding" option
On 01 Sep 2015, at 01:13, Junio C Hamano wrote: > larsxschnei...@gmail.com writes: > >> From: Lars Schneider >> > > Here is a space for you to describe what it does and why it is a > good idea to have it. How about this: Perforce keeps the encoding of a path as given by the originating OS. Git expects paths encoded as UTF-8. Add an option to tell git-p4 what encoding Perforce had used for the paths. This encoding is used to transcode the paths to UTF-8. As an example, Perforce on Windows uses “cp1252” to encode path names. > >> Signed-off-by: Lars Schneider >> --- >> Documentation/git-p4.txt| 5 + >> git-p4.py | 6 ++ >> t/t9821-git-p4-path-encoding.sh | 38 ++ > > I'll move this to 9822, as 9821 is taken by another git-p4 test, > while queuing. OK. I wasn’t sure how this is handled. Just for my understanding: As soon as a TC number is occupied in one of the official branches (master/next/pu/maint) then the next number should be used, right? > >> diff --git a/Documentation/git-p4.txt b/Documentation/git-p4.txt >> index 82aa5d6..14bb79c 100644 >> --- a/Documentation/git-p4.txt >> +++ b/Documentation/git-p4.txt >> @@ -252,6 +252,11 @@ Git repository: >> Use a client spec to find the list of interesting files in p4. >> See the "CLIENT SPEC" section below. >> >> +--path-encoding :: >> +The encoding to use when reading p4 client paths. With this option >> +non ASCII paths are properly stored in Git. For example, the encoding >> +'cp1252' is often used on Windows systems. >> + > > Is this something that needs to be consistently given while > interacting with the same P4 depot (in which case you may want to > allow this to be given only once, record the value in the config and > never allow it to be updated once it is set, or something)? Good idea! I will add this. However, I really wonder why nobody tripped over this rather obvious bug already. @Luke: Can you double check my fix? Two general questions: (1) I saw that the path encoding fix is already on “pu”. What is more convenient for you? Do you want to get a [PATCH v4] with one commit including the changes mentioned above or do you want me to keep the v3 commit and add a v4 commit on top? (2) Is there a CI setup for Git somewhere accessible on the Internet? How about building and testing Git on Travis (https://travis-ci.org/git/git)? Thanks, Lars -- 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
Re: Git's inconsistent command line options
On Sep 01, 2015, at 02:28 AM, David Aguilar wrote: >While a script writer could write, "git -c core.cliversion=1 ...", >no one does that, no one wants to do that, and it just seems >like a bad idea that's best left unexplored. Sure, no one will do that from the command line, but I don't think people generally change their preferences that often. Much more likely is that they'll `git config` a more permanent choice for their shell usage and then just use straight up "git" with the new ui. -c would be reserved for scripts which hard code a particular ui. >Otherwise, this entire thread seems like a big non-issue. The existing CLI >hasn't hurt adoption... A significant factor driving git adoption is network effects. That's highly motivating to overcome discomfort or confusion with the cli. Once you've lost your beginner's mind, you are much less aware of the cli inconsistencies and disconnects from other vcses. The latter might not affect new users whose only experience with vcses is git, but it presents a steeper learning curve for folks migrating from other tools. >...and tossing a config option at it only makes it worse. The best config is >no config. git already has no shortage of configuration options. ;) Cheers, -Barry pgp26Yg4JQ0kL.pgp Description: OpenPGP digital signature
Re: [PATCH v3] git-p4: add "--path-encoding" option
On 2015-09-01 14.47, Lars Schneider wrote: >>> +test_expect_success 'Create a repo containing iso8859-1 encoded paths' ' >>> >> +cd "$cli" && >>> >> + >>> >> +ISO8859="$(printf "$UTF8_ESCAPED" | iconv -f utf-8 -t >>> >> iso8859-1)" && >>> >> +>"$ISO8859" && >>> >> +p4 add "$ISO8859" && >>> >> +p4 submit -d "test commit" >>> >> +' >> > Sorry for being persistant, >> > but you can't create files with names that are ISO-8859-1 encoded under >> > Mac OS, >> > we end up like this: >> > >> > a-%E4_o-%F6_u-%FC.txt > You are right. However, my goal is not to create a file with ISO-8859-1 > characters in Mac OS. My goal is to create this file in Perforce and this > approach seems to work. > >> > But this line creates a file, doesn't it ? >"$ISO8859" && (I just wonder how this works on you machine ) And, may be, we could fill the file with some content, to be double-sure that the file name conversion works with Perforce ? like echo content >"$ISO8859" && and test the content later ? -- 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
Re: [PATCH v3] git-p4: add "--path-encoding" option
On 01 Sep 2015, at 16:35, Torsten Bögershausen wrote: > On 2015-09-01 14.47, Lars Schneider wrote: +test_expect_success 'Create a repo containing iso8859-1 encoded paths' ' >> +cd "$cli" && >> + >> +ISO8859="$(printf "$UTF8_ESCAPED" | iconv -f utf-8 -t >> iso8859-1)" && >> +>"$ISO8859" && >> +p4 add "$ISO8859" && >> +p4 submit -d "test commit" >> +' Sorry for being persistant, but you can't create files with names that are ISO-8859-1 encoded under Mac OS, we end up like this: a-%E4_o-%F6_u-%FC.txt >> You are right. However, my goal is not to create a file with ISO-8859-1 >> characters in Mac OS. My goal is to create this file in Perforce and this >> approach seems to work. >> > But this line creates a file, doesn't it ? >> "$ISO8859" && > > (I just wonder how this works on you machine ) I tested it on OS X (HPFS) and Linux (ext4). Can you reproduce problems on your machine? If yes, what is your OS/filesystem? > And, may be, we could fill the file with some content, to be double-sure that > the file name conversion works with Perforce ? > > like > echo content >"$ISO8859" && > > and test the content later ? OK, I will add this. Thanks, Lars -- 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
Re: [PATCH v14 04/13] ref-filter: implement an `align` atom
On Tue, Sep 1, 2015 at 6:41 PM, Matthieu Moy
wrote:
> Karthik Nayak writes:
>
>> Like this:
>>
>> else if (skip_prefix(name, "align", &valp)) {
>> struct align *align = &v->u.align;
>> struct strbuf **s;
>>
>> if (valp[0] != ':')
>> die(_("format: usage %%(align:,)"));
>> else
>> valp++;
>> ..
>>
>> }
>
> Checking the string's correctness as you read it seems good to me, yes.
>
> But as you say, you should make this a bit more generic, for example by
> putting your "if/else" in a helper function, that other atoms could use.
Okay then, I'll keep that in mind :)
--
Regards,
Karthik Nayak
--
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
[PATCH] mailmap: update my entry with new email address
My 'demon' email address is no longer functional since, after 16+ years with demon, I have had to change my ISP. :( Also, take the opportunity to remove my middle name, which I only use on official documents (or in the GECOS field when creating a user account on unix). Signed-off-by: Ramsay Jones --- Hi Junio, So, my home move caused a (reluctant) change in ISP too. :( This, in turn, left me without any internet access for just over three weeks; I was climbing the walls! ATB, Ramsay Jones .mailmap | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.mailmap b/.mailmap index ece2951..e5b4126 100644 --- a/.mailmap +++ b/.mailmap @@ -186,7 +186,7 @@ Philip Jägenstedt Philipp A. Hartmann Philippe Bruhat Ralf Thielow -Ramsay Allan Jones +Ramsay Jones René Scharfe Robert Fitzsimons Robert Shearman -- 2.5.0 -- 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
[PATCH] path.c: make 'common_list' a file local symbol
Commit 04afda89 ("refs: clean up common_list", 26-08-2015) changed
the type of the 'common_list' symbol from an array of 'formatted'
strings to an array of struct containing the same data. However, in
addition it also (inadvertently) changed the visibility of the
symbol from file local to external.
In order to revert the visibility of the symbol to file local, add
the static modifier to the declaration of 'common_list'.
Noticed by sparse (symbol 'common_list' was not declared. Should it
be static?).
Signed-off-by: Ramsay Jones
---
Hi David,
If you need to re-roll the patches on your 'dt/refs-bisection' branch, could you
please squash this into the relevant patch.
Thanks!
ATB,
Ramsay Jones
path.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/path.c b/path.c
index 9d32d19..a80eaf7 100644
--- a/path.c
+++ b/path.c
@@ -100,7 +100,7 @@ struct common_dir {
const char *dirname;
};
-struct common_dir common_list[] = {
+static struct common_dir common_list[] = {
{ 0, 1, 0, "branches" },
{ 0, 1, 0, "hooks" },
{ 0, 1, 0, "info" },
--
2.5.0
--
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
Re: [PATCH] mailmap: update my entry with new email address
Hi Ramsay, On 2015-09-01 17:50, Ramsay Jones wrote: > diff --git a/.mailmap b/.mailmap > index ece2951..e5b4126 100644 > --- a/.mailmap > +++ b/.mailmap > @@ -186,7 +186,7 @@ Philip Jägenstedt > > Philipp A. Hartmann > Philippe Bruhat > Ralf Thielow > -Ramsay Allan Jones > +Ramsay Jones > René Scharfe > Robert Fitzsimons > Robert Shearman The idea of the .mailmap file is to fix inconsistent names in the history. For example, if you look e.g. at commit 6ebdee5af47df0c64354e452419015a694c25f5f, you will see that your middle name was recorded, but if you look e.g. at f84df81f654aeb0ac4582e0b3be162cba6ea5232 you will see that it was not always recorded with your changes. Therefore I would recommend leaving the old line in place, and just *adding* another one. Besides, for consistency I would leave the middle name in there. Otherwise, people might be wondering why their shortlog calls bring up some other name. In other words, if you add a new line, you really want to use the same clear name as before. You actually do not even need to add a new line if you plan on submitting patches with your middle name. Remember: the main purpose of .mailmap is to support shortlog, i.e. appropriate grouping of patches by person (such as the two commits I mentioned above). Ciao, Johannes P.S.: That's a major bummer on your three forced-offline weeks. I feel with ya! -- 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
Re: [PATCHv3 2/3] submodule: implement `module-name` as a builtin helper
On Mon, Aug 31, 2015 at 7:31 PM, Eric Sunshine wrote:
> On Mon, Aug 31, 2015 at 8:40 PM, Stefan Beller wrote:
>> This implements the helper `module_name` in C instead of shell,
>> yielding a nice performance boost.
>>
>> Before this patch, I measured a time (best out of three):
>>
>> $ time ./t7400-submodule-basic.sh >/dev/null
>> real0m11.066s
>> user0m3.348s
>> sys 0m8.534s
>>
>> With this patch applied I measured (also best out of three)
>>
>> $ time ./t7400-submodule-basic.sh >/dev/null
>> real0m10.063s
>> user0m3.044s
>> sys 0m7.487s
>>
>> Signed-off-by: Stefan Beller
>> ---
>> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
>> index 44310f5..91bd420 100644
>> --- a/builtin/submodule--helper.c
>> +++ b/builtin/submodule--helper.c
>> @@ -102,16 +105,38 @@ static int module_list(int argc, const char **argv,
>> const char *prefix)
>> +static int module_name(int argc, const char **argv, const char *prefix)
>> +{
>> + const struct submodule *sub;
>> +
>> + if (argc != 1)
>> + usage("git submodule--helper module_name \n");
>
> usage(_("..."));
>
> s/module_name/module-name/
>
> I think you also need to drop the newline from the usage() argument.
>
>> + gitmodules_config();
>> + sub = submodule_from_path(null_sha1, argv[0]);
>> +
>> + if (!sub)
>> + die(N_("No submodule mapping found in .gitmodules for path
>> '%s'"),
>> + argv[0]);
>> +
>> + printf("%s\n", sub->name);
>> +
>> + return 0;
>> +}
>> +
>> int cmd_submodule__helper(int argc, const char **argv, const char *prefix)
>> {
>> if (argc < 2)
>> die(N_("fatal: submodule--helper subcommand must be called
>> with "
>> - "a subcommand, which is module-list\n"));
>> + "a subcommand, which are module-list,
>> module-name\n"));
>
> Manually maintaining this list is likely to become a maintenance issue
> pretty quickly. Isn't there an OPT_CMDMODE for this sort of thing?
If we were using OPT_CMDMODE, we'd need to have all the options in
one array, passing it to the option parser. I don't think we could have
2 layers of option parsing here. If the first layer only contains the
check for argv[1] (list, name, clone), then it would fail not knowing the
arguments for each of these sub commands.
If we parse all the options together, then we would need to have lots of
if (cmd == name)
die ("it makes no sense to give --reference or --depth");
also the variables where the options are passed into would not be
function specific, but file global.
>
> Also, it would like be more readable if the possible commands were
> printed one per line rather than all squashed together.
>
>> if (!strcmp(argv[1], "module-list"))
>> return module_list(argc - 1, argv + 1, prefix);
>>
>> + if (!strcmp(argv[1], "module-name"))
>> + return module_name(argc - 2, argv + 2, prefix);
>> +
>> die(N_("fatal: '%s' is not a valid submodule--helper subcommand, "
>> - "which is module-list\n"),
>> + "which are module-list, module-name\n"),
>
> And, it's duplicated here, making it even more of a maintenance problem.
>
>> argv[1]);
>> }
Maybe we can do it similar to git.c to have our own array with name
and function pointers,
which can be used both in the help text as well as the actual function call.
--
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
Re: [PATCH] t7300: fix broken && chains
erik elfström writes: > On Mon, Aug 31, 2015 at 6:58 PM, Junio C Hamano wrote: >> >> ... It may have been >> better if you didn't do "while we are here" and corrected only the >> &&-chain in patch 1/2 and then updated the style of the tests to >> take advantage of the newer facilities recent test-lib has in a >> separate patch 2/2, but this will do at least for now. >> >> Will queue. >> >> Thanks. > > I can do a re-roll with the chain fix in the first patch and a more > thorough modernization of t7300 in separate patches if you'd like? I > almost went this way for v1 but decided to limit the scope for the > first version. I'd say it is too much work for something that we already have reviewed and queued ;-) Let's have this patch as-is. Modernization and clean-up from time to time is a good thing to do, but we only have limited review bandwidth, so let's not overdo it. Thanks. -- 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
Re: making refs/bisect/ per worktree (was Re: (unknown))
David Turner writes: > This version of the patch series squashes in Junio's comment fix and > the xstrndup fix. > > In addition, it removes refs/worktree, and just makes refs/bisect > per-worktree. If we later discover that other things need to be > per-worktree, we can do refs/worktree, but for now, this is > backwards-compatible so we'll do this. I've already squashed the "make common_list[] static" from Ramsay into 1/3, so no need to resend for that one. Let's wait for a few days to see if people spot any more issues, merge to and cook in 'next' for the remainder of this cycle, and aim to merge this down to 'master' in the first batch after the upcoming release. Thanks. -- 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
Re: Git's inconsistent command line options
(Administrivia) please do not cull CC list when replying. Barry Warsaw writes: > On Sep 01, 2015, at 02:28 AM, David Aguilar wrote: > >>While a script writer could write, "git -c core.cliversion=1 ...", >>no one does that, no one wants to do that, and it just seems >>like a bad idea that's best left unexplored. > > Sure, no one will do that from the command line, but I don't think people > generally change their preferences that often. Much more likely is that > they'll `git config` a more permanent choice for their shell usage and then > just use straight up "git" with the new ui. -c would be reserved for scripts > which hard code a particular ui. That way, you are forcing all the existing scripts to be updated to say "git -c ..." for _all_ invocations of Git they have, aren't you? That is simply unworkable. By the way, you can review what has already been said in this discussion here: http://thread.gmane.org/gmane.comp.version-control.git/276509/focus=276589 -- 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
GSoC 2015 is over
Hi, The Google Summer of Code 2015 is officially over. We had two students (Paul and Karthik), and both of them passed. 100 % success :-). I didn't follow closely Paul's work, but we now have C builtins for "pull" and "am" (both in master) instead of shell scripts. Karthik worked on the unification of "for-each-ref", "tag" and "branch". We already have more options for "for-each-ref". tag and branch are being ported to use the same library code as "for-each-ref" (2 patch series under review). I consider this GSoC as a great success and a pleasant experience. Congratulation to Paul and Karthik, and a warm "thank you" to everybody who contributed: administrators, mentors, reviewers, and obviously Junio! (not to mention Google, who made all this possible) Thanks all! -- Matthieu Moy http://www-verimag.imag.fr/~moy/ -- 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
Windows path handling changed between versions
I recently upgraded from Windows Git 1.6.2 to 2.5.0 and found myself unable to rebase. Turns out paths didn't used to be case-sensitive and now they are, causing a number of operations to halt. A repo created by pointing at the directory c:\core\guidewire\Dev\2.4 would (I suppose) technically have been invalid the whole time because Windows reports the current path as C:\core\guidewire\Dev\2.4 , but msys Git 1.6.2 evidently made a case-insensitive path comparison so the discrepancy was suppressed. The proximate cause of errors was git rev-parse --is-inside-work-tree which would output 'false' even inside the working tree. "--is-inside-git-dir" also printed 'false' in directories where it should have said 'true'. I actually missed the problem in plain sight at first, because I created a new repo (in which everything worked as expected), and then did a directory diff... the worktree paths were different but I only noticed the names, not the case difference in the drive letter. More details in this SO question: http://stackoverflow.com/q/32280644/2835086 I was able to repair my existing repos by changing the 'worktree' value in gitconfig - s/c/C/ did the trick - but the whole thing was a surprise. Is this a bug in the current version? Windows doesn't distinguish on case, so maybe applications shouldn't either. Was this a bug in the prior version? Maybe creating a repo with a worktree path that doesn't match the file system should have been an error from the very beginning. Was this user error? Maybe I did something wrong and should have known better, but got away with it for a while. Any feedback welcome, -gws -- 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
Re: [PATCH] mailmap: update my entry with new email address
On Tue, Sep 1, 2015 at 9:11 AM, Johannes Schindelin wrote: > Hi Ramsay, > > On 2015-09-01 17:50, Ramsay Jones wrote: > >> diff --git a/.mailmap b/.mailmap >> index ece2951..e5b4126 100644 >> --- a/.mailmap >> +++ b/.mailmap >> @@ -186,7 +186,7 @@ Philip Jägenstedt >> >> Philipp A. Hartmann >> Philippe Bruhat >> Ralf Thielow >> -Ramsay Allan Jones >> +Ramsay Jones >> René Scharfe >> Robert Fitzsimons >> Robert Shearman > > The idea of the .mailmap file is to fix inconsistent names in the history. > For example, if you look e.g. at commit > 6ebdee5af47df0c64354e452419015a694c25f5f, you will see that your middle name > was recorded, but if you look e.g. at > f84df81f654aeb0ac4582e0b3be162cba6ea5232 you will see that it was not always > recorded with your changes. I would argue a mailmap file serves a dual purpose. The first purpose as you said is to correct inconsistent or erroneous names (who is s...@local.host anyway ?) The second purpose is just as Ramsay used it to map different identities to one person/email address. As Ramsay lost his access to the email, you would want to use his new email, say when looking for the right reviewer using git blame. Then even old commits (with the old email recorded) should rather display the new email, such that you get it right the first time. > > Therefore I would recommend leaving the old line in place, and just *adding* > another one. I would recommending taking this patch specifically for the second purpose. > > Besides, for consistency I would leave the middle name in there. Otherwise, > people might be wondering why their shortlog calls bring up some other name. > In other words, if you add a new line, you really want to use the same clear > name as before. You can use the long form Name1 Name2 in the .mailmap file to always show the Name1 and email1. > > You actually do not even need to add a new line if you plan on submitting > patches with your middle name. Remember: the main purpose of .mailmap is to > support shortlog, i.e. appropriate grouping of patches by person (such as the > two commits I mentioned above). > > Ciao, > Johannes > > P.S.: That's a major bummer on your three forced-offline weeks. I feel with > ya! Sometimes I'd love to be offline for 3 weeks. But being forced to? No thanks! > -- > 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 -- 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
Re: [PATCH] push: don't show Done with --quiet --porcelain
Josh Rabinowitz writes:
> Change so 'git push --porcelain --quiet' emits no text when there
> is no error. This makes the --quiet option here more consistent with
> other git commands.
>
> Signed-off-by: josh rabinowitz
> ---
The rationale given in 77555854 (git-push: make git push --porcelain
print "Done", 2010-02-26) does not apply when "--quiet" is in use,
because we do give the rejection notice to the standard output even
under "--quiet", so the calling script can tell between the case
where we couldn't reach the remote side (i.e. no rejection notice)
and the case where we reached them and they rejected (i.e. they will
tell us why the push was rejected) when "git push" reports a failure
with its exit status.
For that matter, I am not sure if this "Done" introduced by 77555854
is really needed even when "--quiet" is not in effect.
In either case, saying "Done" after talking to the remote end
already is an established part of the output meant for Porcelain
when "--porcelain" option is in use. So I do not think changing it
is a good idea. Existing scripts that read from "--porcelain" output
would be expecting the line to be there.
> transport.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/transport.c b/transport.c
> index 40692f8..0021b3f 100644
> --- a/transport.c
> +++ b/transport.c
> @@ -1209,7 +1209,7 @@ int transport_push(struct transport *transport,
> transport_update_tracking_ref(transport->remote, ref,
> verbose);
> }
>
> - if (porcelain && !push_ret)
> + if (!quiet && porcelain && !push_ret)
> puts("Done");
> else if (!quiet && !ret && !transport_refs_pushed(remote_refs))
> fprintf(stderr, "Everything up-to-date\n");
> --
> 2.3.2 (Apple Git-55)
--
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
Re: [PATCH] mailmap: update my entry with new email address
Hi Johannes, On 01/09/15 17:11, Johannes Schindelin wrote: > Hi Ramsay, > > On 2015-09-01 17:50, Ramsay Jones wrote: > >> diff --git a/.mailmap b/.mailmap >> index ece2951..e5b4126 100644 >> --- a/.mailmap >> +++ b/.mailmap >> @@ -186,7 +186,7 @@ Philip Jägenstedt >> >> Philipp A. Hartmann >> Philippe Bruhat >> Ralf Thielow >> -Ramsay Allan Jones >> +Ramsay Jones >> René Scharfe >> Robert Fitzsimons >> Robert Shearman > The idea of the .mailmap file is to fix inconsistent names in the history. > For example, if you look e.g. at commit > 6ebdee5af47df0c64354e452419015a694c25f5f, you will see that your middle name > was recorded, but if you look e.g. at > f84df81f654aeb0ac4582e0b3be162cba6ea5232 you will see that it was not always > recorded with your changes. Hmm, I assume you are using 'git log' or 'git show' to look at these; try this: $ git show --use-mailmap 6ebdee5af47df0c64354e452419015a694c25f5 $ git log -1 --use-mailmap f84df81f654aeb0ac4582e0b3be162cba6ea523 or indeed, this: $ git check-mailmap "Ramsay Allan Jones " Ramsay Jones $ Also, 'git shortlog' seems to be working just fine. (The first few commits, which include my middle name, were a mistake; back then git used to look into the GECOS field if you didn't set your name in the config ...). [If it isn't clear, I don't want to see my middle name!] Yes, I was a little surprised that '--use-mailmap' was not on by default, but then I have a very vague recollection of a discussion on the list which decided otherwise. (I'm far too lazy to search the archives ...). ATB, Ramsay Jones > > Therefore I would recommend leaving the old line in place, and just *adding* > another one. > > Besides, for consistency I would leave the middle name in there. Otherwise, > people might be wondering why their shortlog calls bring up some other name. > In other words, if you add a new line, you really want to use the same clear > name as before. > > You actually do not even need to add a new line if you plan on submitting > patches with your middle name. Remember: the main purpose of .mailmap is to > support shortlog, i.e. appropriate grouping of patches by person (such as the > two commits I mentioned above). > > Ciao, > Johannes > > P.S.: That's a major bummer on your three forced-offline weeks. I feel with > ya! > -- > 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 > -- 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
Re: [PATCH v3] git-p4: add "--path-encoding" option
Lars Schneider writes: >> I'll move this to 9822, as 9821 is taken by another git-p4 test, >> while queuing. > OK. I wasn’t sure how this is handled. Just for my understanding: As > soon as a TC number is occupied in one of the official branches > (master/next/pu/maint) then the next number should be used, right? That would help me great deal (as I'd be doing that if you didn't). > Two general questions: > > (1) I saw that the path encoding fix is already on “pu”. What is > more convenient for you? Do you want to get a [PATCH v4] with one > commit including the changes mentioned above or do you want me to keep > the v3 commit and add a v4 commit on top? The convention is to do [PATCH v$n+1] (i.e. reroll) before the topic is merged to 'next' (i.e. while in 'pu'). When further improvements are needed after the topic is merged to 'next', we switch to incremental improvements. > (2) Is there a CI setup for Git somewhere accessible on the Internet? > How about building and testing Git on Travis > (https://travis-ci.org/git/git)? Sorry but you are asking the right question to a wrong guy ;-) There may be people who have already arranged something like that, who can speak up. -- 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
Re: Windows path handling changed between versions
Hi Geofrey, On 2015-09-01 18:55, Geofrey Sanders wrote: > I recently upgraded from Windows Git 1.6.2 to 2.5.0 and found myself > unable to rebase. Turns out paths didn't used to be case-sensitive and > now they are, causing a number of operations to halt. A repo created > by pointing at the directory > c:\core\guidewire\Dev\2.4 > would (I suppose) technically have been invalid the whole time because > Windows reports the current path as > C:\core\guidewire\Dev\2.4 > , but msys Git 1.6.2 evidently made a case-insensitive path comparison > so the discrepancy was suppressed. Are you sure about that? I seem to recall that `pwd` changed behavior between MSys and MSys2, but Git never made case-insensitive comparisons. It might help me to understand what is going on if I could have preciser information. What exactly do you mean by "A repo created by pointing at ..."? Could you type out the Git commands you used? > The proximate cause of errors was > git rev-parse --is-inside-work-tree > which would output 'false' even inside the working tree. Ah, you are apparently talking about a worktree separate from your repository? > "--is-inside-git-dir" also printed 'false' in directories where it > should have said 'true'. Again, I really need preciser information about this: *How* did you end up in that directory? Did you use Git Bash or Git CMD? Did you call `cd` with a relative path, a POSIX path or a POSIX-ified full DOS path? > I actually missed the problem in plain sight > at first, because I created a new repo (in which everything worked as > expected), and then did a directory diff... the worktree paths were > different but I only noticed the names, not the case difference in the > drive letter. More details in this SO question: > http://stackoverflow.com/q/32280644/2835086 Please understand that I have a lot of tickets to juggle about and that it is a bit unfair to send me onto a goose chase. I would have preferred a proper GitHub issue, as the "Contribute" section of https://git-for-windows.github.io/ explicitly asks for, but I am okay with discussing this ticket on the mailing list. But studying a StackOverflow thread in addition is a bit much... next, people would ask me to search their Twitter feed for the little tid bit of information I need to help. So please summarize that StackOverflow question, and while we are at it: StackOverflow suggests coming up with a Minimal, Complete and Verifiable Example. That would be a nice thing to have. Maybe you find it in you to come up with that MCVE. > I was able to repair my existing repos by changing the 'worktree' > value in gitconfig - s/c/C/ did the trick - but the whole thing was a > surprise. > > Is this a bug in the current version? Windows doesn't distinguish on > case, so maybe applications shouldn't either. > Was this a bug in the prior version? Maybe creating a repo with a > worktree path that doesn't match the file system should have been an > error from the very beginning. > Was this user error? Maybe I did something wrong and should have known > better, but got away with it for a while. I think there is a good chance we can fix this, although a 1.x -> 2.x jump always suggests that certain things change in a backwards-incompatible manner. Looking forward to more detailed information and that MCVE, Johannes -- 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
Re: [PATCH v3] git-p4: add "--path-encoding" option
Lars Schneider writes: > On 01 Sep 2015, at 01:13, Junio C Hamano wrote: > >> larsxschnei...@gmail.com writes: >> >>> From: Lars Schneider >>> >> >> Here is a space for you to describe what it does and why it is a >> good idea to have it. > How about this: > > Perforce keeps the encoding of a path as given by the originating > OS. Git expects paths encoded as UTF-8. Add an option to tell git-p4 > what encoding Perforce had used for the paths. This encoding is used > to transcode the paths to UTF-8. As an example, Perforce on Windows > uses “cp1252” to encode path names. Very readable. Does "Perforce on Windows" always use cp1252, or is it more correct to say "often uses" here? -- 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
Re: [PATCH] t7300: fix broken && chains
On Tue, Sep 01, 2015 at 08:27:59AM +0200, erik elfström wrote: > ( > echo "100644 $o5 0a" > echo "100644 $o0 0c" > echo "16 $c1 0d" > ) >expected && > > I'd estimate that there are hundreds of these (see t3030 for > examples). I'm not sure if you care about these? As you say they are > not really very interesting cases. I think patches for these are OK, but no, I don't consider it a high priority if they are harmless (IMHO the real value of the patches is that it removes the noise from the output of your script, so you can find any cases that _do_ matter). -Peff -- 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
Re: [PATCH v14 00/13] Port tag.c to use ref-filter.c
On Mon, Aug 31, 2015 at 5:06 PM, Karthik Nayak wrote: > On Mon, Aug 31, 2015 at 1:01 PM, Matthieu Moy > wrote: >> Karthik Nayak writes: >> >>> * We perform quoting on each layer of nested alignment. >> >> I do not understand why. >> >> For example, using the tip of karthik/exp on GitHub (on top of this >> series, d91419b (ref-filter: adopt get_head_description() from branch.c, >> 2015-08-23)): >> >> git for-each-ref --shell \ >> --format 'x=%(if)foo%(then)%(align:10)XXX%(end)%(else) not foo %(end)' >> >> I'd expect an output like: >> >> x='XXX ' >> >> and instead I get: >> >> x=''\''XXX '\''' >> >> which assigns the value 'XXX ' (including the quotes) to $x. I do >> not see a use-case for this (well, I could imagine one where we would >> later call eval "$x", that seems rather far-fetched). >> >> I think the quoting should be: >> >> 1) When the stack contains only the initial element, quote individual >>atoms. >> >> 2) When the stack contains exactly two elements and encountering a %(end) >>or %(else), quote the entire strbuf of the 2nd level when appending to >>the 1st level. >> >> 3) When the stack contains more than two elements, perform no quoting at >>all. The quoting will be done later by #2. >> > > Yea, That's what Eric was saying, I even made changes which sum up to > what you're saying :) > >> I found a segfault while testing: >> >> $ git for-each-ref --format >> 'x=%(if)%(align:10)%(end)%(then)%(align:10)XXX%(end)%(else)%(end)' --shell >> zsh: segmentation fault >> > > I wouldn't worry about this ATM, I have made so many changes that the > tip is barely changed to reflect those, though I'll have a look at it > :) > Was a dereferencing error, fixed it now :) -- Regards, Karthik Nayak -- 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
Re: GSoC 2015 is over
On Tue, Sep 1, 2015 at 10:25 PM, Matthieu Moy wrote: > Hi, > > The Google Summer of Code 2015 is officially over. We had two students > (Paul and Karthik), and both of them passed. 100 % success :-). > Congrats Paul :) > I didn't follow closely Paul's work, but we now have C builtins for > "pull" and "am" (both in master) instead of shell scripts. Karthik > worked on the unification of "for-each-ref", "tag" and "branch". We > already have more options for "for-each-ref". tag and branch are being > ported to use the same library code as "for-each-ref" (2 patch series > under review). > > I consider this GSoC as a great success and a pleasant experience. > Congratulation to Paul and Karthik, and a warm "thank you" to everybody > who contributed: administrators, mentors, reviewers, and obviously > Junio! (not to mention Google, who made all this possible) > > Thanks all! > Thanks to everyone for helping us through this. Especially my mentors Christian Couder and Matthieu Moy, thanks for guiding me through the project and sparing time for reviews and suggestions, also thanks to everyone else for their suggestions over the course of the project, especially Junio and Eric :) Theres still a lot to do with respect to the project, and I dedicate myself to finishing it completely. And even continue contributing to Git even after that. Has been a pleasant experience overall. -- Regards, Karthik Nayak -- 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
Re: Git's inconsistent command line options
On Sep 01, 2015, at 09:42 AM, Junio C Hamano wrote: >That way, you are forcing all the existing scripts to be updated to >say "git -c ..." for _all_ invocations of Git they have, aren't you? No, why? If the default value enables the current ui, then no scripts need changing. Users can enable the new ui for their own benefit at their own pace. If you eventually want to make the new ui the default, provide a sufficient transition period. Cheers, -Barry pgpxOXAlcusGr.pgp Description: OpenPGP digital signature
Re: [PATCH 3/3] submodule: implement `module_clone` as a builtin helper
took all suggestions except the one below.
>
> if (strbuf_getcwd(&sb))
> die_errno(...);
> strbuf_addf(&sb, "/%s, sm_gitdir);
> free(sm_gitdir);
> sm_gitdir = strbuf_detach(&sb, NULL);
>
>> + }
>> +
>> + if (strbuf_getcwd(&sb))
>> + die_errno("unable to get current working directory");
>
> The conditional block just above here also gets 'cwd'. If you move
> this code above the !is_absolute_path(sm_gitdir) conditional block,
> then you can avoid the duplication.
I don't get it. We need to have strbuf_getcwd twice no matter how we
arrange the code
as we strbuf_detach will empty the strbuf.
Do you mean to call strbuf_getcwd just once and then xstrdup the value,
then reset the strbuf to just contain the cwd and append the other string ?
>
>> + strbuf_addf(&sb, "/%s", path);
>> +
>> + p = git_pathdup_submodule(path, "config");
>> + if (!p)
>> + die("Could not get submodule directory for '%s'", path);
>> + git_config_set_in_file(p, "core.worktree",
>> + relative_path(sb.buf, sm_gitdir, &rel_path));
>> + strbuf_release(&sb);
>> + free((char *)sm_gitdir);
>> + return 0;
>> +}
--
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
Re: Git's inconsistent command line options
On Tue, Sep 1, 2015 at 10:50 AM, Barry Warsaw wrote: > On Sep 01, 2015, at 09:42 AM, Junio C Hamano wrote: > >>That way, you are forcing all the existing scripts to be updated to >>say "git -c ..." for _all_ invocations of Git they have, aren't you? > > No, why? If the default value enables the current ui, then no scripts need > changing. Users can enable the new ui for their own benefit at their own > pace. If you eventually want to make the new ui the default, provide a > sufficient transition period. > > Cheers, > -Barry So say I am a user who wants to take the new command set. And as I am lazy to type it all the time I just do: $ git config --global command-version 2 Now I have all the new fancy stuff when I type it directly into the terminal. But when I run one of the old scripts my coworker gave me (which is used to the old notion), it must adhere to the old command world. How do you now figure out if this is interactive or script? -- 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
Re: [PATCH] path.c: make 'common_list' a file local symbol
Ramsay Jones writes:
> Commit 04afda89 ("refs: clean up common_list", 26-08-2015) changed
> the type of the 'common_list' symbol from an array of 'formatted'
> strings to an array of struct containing the same data. However, in
> addition it also (inadvertently) changed the visibility of the
> symbol from file local to external.
>
> In order to revert the visibility of the symbol to file local, add
> the static modifier to the declaration of 'common_list'.
>
> Noticed by sparse (symbol 'common_list' was not declared. Should it
> be static?).
>
> Signed-off-by: Ramsay Jones
> ---
>
> Hi David,
>
> If you need to re-roll the patches on your 'dt/refs-bisection' branch, could
> you
> please squash this into the relevant patch.
>
> Thanks!
Thanks. I've squashed this into David's [v5 1/3] while queuing.
>
> ATB,
> Ramsay Jones
>
> path.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/path.c b/path.c
> index 9d32d19..a80eaf7 100644
> --- a/path.c
> +++ b/path.c
> @@ -100,7 +100,7 @@ struct common_dir {
> const char *dirname;
> };
>
> -struct common_dir common_list[] = {
> +static struct common_dir common_list[] = {
> { 0, 1, 0, "branches" },
> { 0, 1, 0, "hooks" },
> { 0, 1, 0, "info" },
--
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
Re: [PATCH] git-quiltimport: add commandline option --series
Juerg Haefliger writes:
> The quilt series file doesn't have to be located in the same directory
> with the patches and can be named differently than 'series' as well. This
> patch adds a commandline option to allow for a non-standard series
> filename and location.
>
> Signed-off-by: Juerg Haefliger
> ---
> Documentation/git-quiltimport.txt | 11 +--
> git-quiltimport.sh| 16 ++--
> 2 files changed, 23 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/git-quiltimport.txt
> b/Documentation/git-quiltimport.txt
> index d64388c..ff633b0 100644
> --- a/Documentation/git-quiltimport.txt
> +++ b/Documentation/git-quiltimport.txt
> @@ -10,6 +10,7 @@ SYNOPSIS
>
> [verse]
> 'git quiltimport' [--dry-run | -n] [--author ] [--patches ]
> + [--series ]
>
>
> DESCRIPTION
> @@ -42,13 +43,19 @@ OPTIONS
> information can be found in the patch description.
>
> --patches ::
> - The directory to find the quilt patches and the
> - quilt series file.
> + The directory to find the quilt patches.
> +
> The default for the patch directory is patches
> or the value of the $QUILT_PATCHES environment
> variable.
>
> +--series ::
> + The quilt series file.
> ++
> +The default for the series file is /series
> +or the value of the $QUILT_SERIES environment
> +variable.
> +
Makes sense.
I am not a quilt user, but the Internet finds pages like
http://lists.gnu.org/archive/html/quilt-dev/2009-12/msg00021.html
and http://www.man-online.org/page/1-quilt/, both of which tell me
that QUILT_SERIES is an environment variable that has established
meaning, and the new use in this patch is consistent with the
established practice.
Thanks, will apply.
> GIT
> ---
> Part of the linkgit:git[1] suite
> diff --git a/git-quiltimport.sh b/git-quiltimport.sh
> index 167d79f..6d3a88d 100755
> --- a/git-quiltimport.sh
> +++ b/git-quiltimport.sh
> @@ -6,7 +6,8 @@ git quiltimport [options]
> --
> n,dry-run dry run
> author= author name and email address for patches without any
> -patches= path to the quilt series and patches
> +patches= path to the quilt patches
> +series= path to the quilt series file
> "
> SUBDIRECTORY_ON=Yes
> . git-sh-setup
> @@ -27,6 +28,10 @@ do
> shift
> QUILT_PATCHES="$1"
> ;;
> + --series)
> + shift
> + QUILT_SERIES="$1"
> + ;;
> --)
> shift
> break;;
> @@ -53,6 +58,13 @@ if ! [ -d "$QUILT_PATCHES" ] ; then
> exit 1
> fi
>
> +# Quilt series file
> +: ${QUILT_SERIES:=$QUILT_PATCHES/series}
> +if ! [ -e "$QUILT_SERIES" ] ; then
> + echo "The \"$QUILT_SERIES\" file does not exist."
> + exit 1
> +fi
> +
> # Temporary directories
> tmp_dir="$GIT_DIR"/rebase-apply
> tmp_msg="$tmp_dir/msg"
> @@ -135,5 +147,5 @@ do
> commit=$( (echo "$SUBJECT"; echo; cat "$tmp_msg") | git
> commit-tree $tree -p $commit) &&
> git update-ref -m "quiltimport: $patch_name" HEAD $commit ||
> exit 4
> fi
> -done 3<"$QUILT_PATCHES/series"
> +done 3<"$QUILT_SERIES"
> rm -rf $tmp_dir || exit 5
--
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
[PATCHv4 1/3] submodule: implement `list` as a builtin helper
Most of the submodule operations work on a set of submodules.
Calculating and using this set is usually done via:
module_list "$@" | {
while read mode sha1 stage sm_path
do
# the actual operation
done
}
Currently the function `module_list` is implemented in the
git-submodule.sh as a shell script wrapping a perl script.
The rewrite is in C, such that it is faster and can later be
easily adapted when other functions are rewritten in C.
git-submodule.sh, similar to the builtin commands, will navigate
to the top-most directory of the repository and keep the
subdirectory as a variable. As the helper is called from
within the git-submodule.sh script, we are already navigated
to the root level, but the path arguments are still relative
to the subdirectory we were in when calling git-submodule.sh.
That's why there is a `--prefix` option pointing to an alternative
path which to anchor relative path arguments.
Signed-off-by: Stefan Beller
---
.gitignore | 1 +
Makefile| 1 +
builtin.h | 1 +
builtin/submodule--helper.c | 137
git-submodule.sh| 54 ++---
git.c | 1 +
6 files changed, 147 insertions(+), 48 deletions(-)
create mode 100644 builtin/submodule--helper.c
diff --git a/.gitignore b/.gitignore
index 4fd81ba..1c2f832 100644
--- a/.gitignore
+++ b/.gitignore
@@ -155,6 +155,7 @@
/git-status
/git-stripspace
/git-submodule
+/git-submodule--helper
/git-svn
/git-symbolic-ref
/git-tag
diff --git a/Makefile b/Makefile
index 24b636d..d434e73 100644
--- a/Makefile
+++ b/Makefile
@@ -901,6 +901,7 @@ BUILTIN_OBJS += builtin/shortlog.o
BUILTIN_OBJS += builtin/show-branch.o
BUILTIN_OBJS += builtin/show-ref.o
BUILTIN_OBJS += builtin/stripspace.o
+BUILTIN_OBJS += builtin/submodule--helper.o
BUILTIN_OBJS += builtin/symbolic-ref.o
BUILTIN_OBJS += builtin/tag.o
BUILTIN_OBJS += builtin/unpack-file.o
diff --git a/builtin.h b/builtin.h
index 839483d..924e6c4 100644
--- a/builtin.h
+++ b/builtin.h
@@ -119,6 +119,7 @@ extern int cmd_show(int argc, const char **argv, const char
*prefix);
extern int cmd_show_branch(int argc, const char **argv, const char *prefix);
extern int cmd_status(int argc, const char **argv, const char *prefix);
extern int cmd_stripspace(int argc, const char **argv, const char *prefix);
+extern int cmd_submodule__helper(int argc, const char **argv, const char
*prefix);
extern int cmd_symbolic_ref(int argc, const char **argv, const char *prefix);
extern int cmd_tag(int argc, const char **argv, const char *prefix);
extern int cmd_tar_tree(int argc, const char **argv, const char *prefix);
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
new file mode 100644
index 000..16d9abe
--- /dev/null
+++ b/builtin/submodule--helper.c
@@ -0,0 +1,137 @@
+#include "builtin.h"
+#include "cache.h"
+#include "parse-options.h"
+#include "quote.h"
+#include "pathspec.h"
+#include "dir.h"
+#include "utf8.h"
+
+static const struct cache_entry **ce_entries;
+static int ce_alloc, ce_used;
+
+static int module_list_compute(int argc, const char **argv,
+ const char *prefix,
+ struct pathspec *pathspec)
+{
+ int i, result = 0;
+ char *max_prefix, *ps_matched = NULL;
+ int max_prefix_len;
+ parse_pathspec(pathspec, 0,
+ PATHSPEC_PREFER_FULL |
+ PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP,
+ prefix, argv);
+
+ /* Find common prefix for all pathspec's */
+ max_prefix = common_prefix(pathspec);
+ max_prefix_len = max_prefix ? strlen(max_prefix) : 0;
+
+ if (pathspec->nr)
+ ps_matched = xcalloc(pathspec->nr, 1);
+
+ if (read_cache() < 0)
+ die(_("index file corrupt"));
+
+ for (i = 0; i < active_nr; i++) {
+ const struct cache_entry *ce = active_cache[i];
+
+ if (!match_pathspec(pathspec, ce->name, ce_namelen(ce),
+ max_prefix_len, ps_matched,
+ S_ISGITLINK(ce->ce_mode) |
S_ISDIR(ce->ce_mode)))
+ continue;
+
+ if (S_ISGITLINK(ce->ce_mode)) {
+ ALLOC_GROW(ce_entries, ce_used + 1, ce_alloc);
+ ce_entries[ce_used++] = ce;
+ }
+
+ while (i + 1 < active_nr && !strcmp(ce->name, active_cache[i +
1]->name))
+ /*
+* Skip entries with the same name in different stages
+* to make sure an entry is returned only once.
+*/
+ i++;
+ }
+ free(max_prefix);
+
+ if (ps_matched && report_path_error(ps_matched, pathspec, prefix))
+ result = -1;
+
+
[PATCHv4 2/3] submodule: implement `name` as a builtin helper
This implements the helper `module_name` in C instead of shell,
yielding a nice performance boost.
Before this patch, I measured a time (best out of three):
$ time ./t7400-submodule-basic.sh >/dev/null
real0m11.066s
user0m3.348s
sys 0m8.534s
With this patch applied I measured (also best out of three)
$ time ./t7400-submodule-basic.sh >/dev/null
real0m10.063s
user0m3.044s
sys 0m7.487s
Signed-off-by: Stefan Beller
---
builtin/submodule--helper.c | 22 ++
git-submodule.sh| 32 +++-
2 files changed, 29 insertions(+), 25 deletions(-)
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 16d9abe..f5e408a 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -5,6 +5,9 @@
#include "pathspec.h"
#include "dir.h"
#include "utf8.h"
+#include "submodule.h"
+#include "submodule-config.h"
+#include "string-list.h"
static const struct cache_entry **ce_entries;
static int ce_alloc, ce_used;
@@ -102,6 +105,24 @@ static int module_list(int argc, const char **argv, const
char *prefix)
return 0;
}
+static int module_name(int argc, const char **argv, const char *prefix)
+{
+ const struct submodule *sub;
+
+ if (argc != 2)
+ usage("git submodule--helper module_name \n");
+
+ gitmodules_config();
+ sub = submodule_from_path(null_sha1, argv[1]);
+
+ if (!sub)
+ die(N_("No submodule mapping found in .gitmodules for path
'%s'"),
+ argv[1]);
+
+ printf("%s\n", sub->name);
+
+ return 0;
+}
struct cmd_struct {
const char *cmd;
@@ -110,6 +131,7 @@ struct cmd_struct {
static struct cmd_struct commands[] = {
{"list", module_list},
+ {"name", module_name},
};
int cmd_submodule__helper(int argc, const char **argv, const char *prefix)
diff --git a/git-submodule.sh b/git-submodule.sh
index 95c04fc..2be8da2 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -178,24 +178,6 @@ get_submodule_config () {
printf '%s' "${value:-$default}"
}
-
-#
-# Map submodule path to submodule name
-#
-# $1 = path
-#
-module_name()
-{
- # Do we have "submodule..path = $1" defined in .gitmodules
file?
- sm_path="$1"
- re=$(printf '%s\n' "$1" | sed -e 's/[].[^$\\*]/\\&/g')
- name=$( git config -f .gitmodules --get-regexp '^submodule\..*\.path$' |
- sed -n -e 's|^submodule\.\(.*\)\.path '"$re"'$|\1|p' )
- test -z "$name" &&
- die "$(eval_gettext "No submodule mapping found in .gitmodules for path
'\$sm_path'")"
- printf '%s\n' "$name"
-}
-
#
# Clone a submodule
#
@@ -498,7 +480,7 @@ cmd_foreach()
then
displaypath=$(relative_path "$sm_path")
say "$(eval_gettext "Entering '\$prefix\$displaypath'")"
- name=$(module_name "$sm_path")
+ name=$(git submodule--helper name "$sm_path")
(
prefix="$prefix$sm_path/"
clear_local_git_env
@@ -554,7 +536,7 @@ cmd_init()
while read mode sha1 stage sm_path
do
die_if_unmatched "$mode"
- name=$(module_name "$sm_path") || exit
+ name=$(git submodule--helper name "$sm_path") || exit
displaypath=$(relative_path "$sm_path")
@@ -636,7 +618,7 @@ cmd_deinit()
while read mode sha1 stage sm_path
do
die_if_unmatched "$mode"
- name=$(module_name "$sm_path") || exit
+ name=$(git submodule--helper name "$sm_path") || exit
displaypath=$(relative_path "$sm_path")
@@ -758,7 +740,7 @@ cmd_update()
echo >&2 "Skipping unmerged submodule $prefix$sm_path"
continue
fi
- name=$(module_name "$sm_path") || exit
+ name=$(git submodule--helper name "$sm_path") || exit
url=$(git config submodule."$name".url)
branch=$(get_submodule_config "$name" branch master)
if ! test -z "$update"
@@ -1022,7 +1004,7 @@ cmd_summary() {
# Respect the ignore setting for --for-status.
if test -n "$for_status"
then
- name=$(module_name "$sm_path")
+ name=$(git submodule--helper name "$sm_path")
ignore_config=$(get_submodule_config "$name"
ignore none)
test $status != A && test $ignore_config = all
&& continue
fi
@@ -1184,7 +1166,7 @@ cmd_status()
while read mode sha1 stage sm_path
do
die_if_unmatched "$mode"
- name=$(module_name "$sm_path") || exit
[PATCHv4 0/3] submodule--helper: Have some refactoring only patches first
I added all suggestions from Eric and rewrote the main function
to not have hardcoded all the commands we're introducing.
diff to patch series 3 below.
Stefan Beller (3):
submodule: implement `list` as a builtin helper
submodule: implement `name` as a builtin helper
submodule: implement `clone` as a builtin helper
.gitignore | 1 +
Makefile| 1 +
builtin.h | 1 +
builtin/submodule--helper.c | 299
git-submodule.sh| 166 +++-
git.c | 1 +
6 files changed, 319 insertions(+), 150 deletions(-)
create mode 100644 builtin/submodule--helper.c
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 0669641..63f535a 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -33,7 +33,7 @@ static int module_list_compute(int argc, const char **argv,
ps_matched = xcalloc(pathspec->nr, 1);
if (read_cache() < 0)
- die("index file corrupt");
+ die(_("index file corrupt"));
for (i = 0; i < active_nr; i++) {
const struct cache_entry *ce = active_cache[i];
@@ -79,7 +79,7 @@ static int module_list(int argc, const char **argv, const
char *prefix)
};
const char *const git_submodule_helper_usage[] = {
- N_("git submodule--helper module_list [--prefix=]
[...]"),
+ N_("git submodule--helper list [--prefix=] [...]"),
NULL
};
@@ -110,21 +110,20 @@ static int module_name(int argc, const char **argv, const
char *prefix)
{
const struct submodule *sub;
- if (argc != 1)
+ if (argc != 2)
usage("git submodule--helper module_name \n");
gitmodules_config();
- sub = submodule_from_path(null_sha1, argv[0]);
+ sub = submodule_from_path(null_sha1, argv[1]);
if (!sub)
die(N_("No submodule mapping found in .gitmodules for path
'%s'"),
- argv[0]);
+ argv[1]);
printf("%s\n", sub->name);
return 0;
}
-
static int clone_submodule(const char *path, const char *gitdir, const char
*url,
const char *depth, const char *reference, int quiet)
{
@@ -166,7 +165,7 @@ static int module_clone(int argc, const char **argv, const
char *prefix)
struct strbuf rel_path = STRBUF_INIT;
struct strbuf sb = STRBUF_INIT;
- struct option module_update_options[] = {
+ struct option module_clone_options[] = {
OPT_STRING(0, "prefix", &alternative_path,
N_("path"),
N_("alternative anchor for relative paths")),
@@ -189,38 +188,41 @@ static int module_clone(int argc, const char **argv,
const char *prefix)
OPT_END()
};
- static const char * const git_submodule_helper_usage[] = {
- N_("git submodule--helper update [--prefix=] [--quiet]
[--remote] [-N|--no-fetch]"
- "[-f|--force] [--rebase|--merge] [--reference ]"
- "[--depth ] [--recursive] [--] [...]"),
+ const char * const git_submodule_helper_usage[] = {
+ N_("git submodule--helper clone [--prefix=] [--quiet] "
+ "[--reference ] [--name ] [--url ]"
+ "[--depth ] [--] [...]"),
NULL
};
- argc = parse_options(argc, argv, prefix, module_update_options,
+ argc = parse_options(argc, argv, prefix, module_clone_options,
git_submodule_helper_usage, 0);
strbuf_addf(&sb, "%s/modules/%s", get_git_dir(), name);
sm_gitdir = strbuf_detach(&sb, NULL);
if (!file_exists(sm_gitdir)) {
- safe_create_leading_directories_const(sm_gitdir);
+ if (safe_create_leading_directories_const(sm_gitdir) < 0)
+ die(_("could not create directory '%s'"), sm_gitdir);
if (clone_submodule(path, sm_gitdir, url, depth, reference,
quiet))
- die(N_("Clone of '%s' into submodule path '%s' failed"),
+ die(N_("clone of '%s' into submodule path '%s' failed"),
url, path);
} else {
- safe_create_leading_directories_const(path);
- unlink(sm_gitdir);
+ if (safe_create_leading_directories_const(path) < 0)
+ die(_("could not create directory '%s'"), path);
+ if (unlink(sm_gitdir) < 0)
+ die_errno(_("failed to delete '%s'"), sm_gitdir);
}
/* Write a .git file in the submodule to redirect to the superproject.
*/
- if (alternative_path && !strcmp(alternative_path, "")) {
+ if (alternative_path && *alternative_path)) {
p = relative_path(p
[PATCHv4 3/3] submodule: implement `clone` as a builtin helper
This converts implements the helper `module_clone`. This functionality is
needed for converting `git submodule update` later on, which we want to
add threading to.
Signed-off-by: Stefan Beller
---
builtin/submodule--helper.c | 140
git-submodule.sh| 80 +
2 files changed, 143 insertions(+), 77 deletions(-)
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index f5e408a..63f535a 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -8,6 +8,7 @@
#include "submodule.h"
#include "submodule-config.h"
#include "string-list.h"
+#include "run-command.h"
static const struct cache_entry **ce_entries;
static int ce_alloc, ce_used;
@@ -123,6 +124,144 @@ static int module_name(int argc, const char **argv, const
char *prefix)
return 0;
}
+static int clone_submodule(const char *path, const char *gitdir, const char
*url,
+ const char *depth, const char *reference, int quiet)
+{
+ struct child_process cp;
+ child_process_init(&cp);
+
+ argv_array_push(&cp.args, "clone");
+ argv_array_push(&cp.args, "--no-checkout");
+ if (quiet)
+ argv_array_push(&cp.args, "--quiet");
+ if (depth && *depth)
+ argv_array_pushl(&cp.args, "--depth", depth, NULL);
+ if (reference && *reference)
+ argv_array_pushl(&cp.args, "--reference", reference, NULL);
+ if (gitdir && *gitdir)
+ argv_array_pushl(&cp.args, "--separate-git-dir", gitdir, NULL);
+
+ argv_array_push(&cp.args, url);
+ argv_array_push(&cp.args, path);
+
+ cp.git_cmd = 1;
+ cp.env = local_repo_env;
+
+ cp.no_stdin = 1;
+ cp.no_stdout = 1;
+ cp.no_stderr = 1;
+
+ return run_command(&cp);
+}
+
+static int module_clone(int argc, const char **argv, const char *prefix)
+{
+ const char *path = NULL, *name = NULL, *url = NULL;
+ const char *reference = NULL, *depth = NULL;
+ const char *alternative_path = NULL, *p;
+ int quiet = 0;
+ FILE *submodule_dot_git;
+ char *sm_gitdir;
+ struct strbuf rel_path = STRBUF_INIT;
+ struct strbuf sb = STRBUF_INIT;
+
+ struct option module_clone_options[] = {
+ OPT_STRING(0, "prefix", &alternative_path,
+ N_("path"),
+ N_("alternative anchor for relative paths")),
+ OPT_STRING(0, "path", &path,
+ N_("path"),
+ N_("where the new submodule will be cloned to")),
+ OPT_STRING(0, "name", &name,
+ N_("string"),
+ N_("name of the new submodule")),
+ OPT_STRING(0, "url", &url,
+ N_("string"),
+ N_("url where to clone the submodule from")),
+ OPT_STRING(0, "reference", &reference,
+ N_("string"),
+ N_("reference repository")),
+ OPT_STRING(0, "depth", &depth,
+ N_("string"),
+ N_("depth for shallow clones")),
+ OPT__QUIET(&quiet, "Suppress output for cloning a submodule"),
+ OPT_END()
+ };
+
+ const char * const git_submodule_helper_usage[] = {
+ N_("git submodule--helper clone [--prefix=] [--quiet] "
+ "[--reference ] [--name ] [--url ]"
+ "[--depth ] [--] [...]"),
+ NULL
+ };
+
+ argc = parse_options(argc, argv, prefix, module_clone_options,
+git_submodule_helper_usage, 0);
+
+ strbuf_addf(&sb, "%s/modules/%s", get_git_dir(), name);
+ sm_gitdir = strbuf_detach(&sb, NULL);
+
+ if (!file_exists(sm_gitdir)) {
+ if (safe_create_leading_directories_const(sm_gitdir) < 0)
+ die(_("could not create directory '%s'"), sm_gitdir);
+ if (clone_submodule(path, sm_gitdir, url, depth, reference,
quiet))
+ die(N_("clone of '%s' into submodule path '%s' failed"),
+ url, path);
+ } else {
+ if (safe_create_leading_directories_const(path) < 0)
+ die(_("could not create directory '%s'"), path);
+ if (unlink(sm_gitdir) < 0)
+ die_errno(_("failed to delete '%s'"), sm_gitdir);
+ }
+
+ /* Write a .git file in the submodule to redirect to the superproject.
*/
+ if (alternative_path && *alternative_path)) {
+ p = relative_path(path, alternative_path, &sb);
+ strbuf_reset(&sb);
+ } else
+ p = path;
+
+ if (safe_create_leading_directories_const(p) < 0)
+ die(_("could not create directory '%s'"), p);
+
+ strbuf_addf(
[PATCH v15 01/13] ref-filter: move `struct atom_value` to ref-filter.c
Since atom_value is only required for the internal working of
ref-filter it doesn't belong in the public header.
Helped-by: Eric Sunshine
Mentored-by: Christian Couder
Mentored-by: Matthieu Moy
Signed-off-by: Karthik Nayak
---
ref-filter.c | 5 +
ref-filter.h | 5 +
2 files changed, 6 insertions(+), 4 deletions(-)
diff --git a/ref-filter.c b/ref-filter.c
index 46963a5..e53c77e 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -55,6 +55,11 @@ static struct {
{ "color" },
};
+struct atom_value {
+ const char *s;
+ unsigned long ul; /* used for sorting when not FIELD_STR */
+};
+
/*
* An atom is a valid field atom listed above, possibly prefixed with
* a "*" to denote deref_tag().
diff --git a/ref-filter.h b/ref-filter.h
index 6bf27d8..45026d0 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -16,10 +16,7 @@
#define FILTER_REFS_INCLUDE_BROKEN 0x1
#define FILTER_REFS_ALL 0x2
-struct atom_value {
- const char *s;
- unsigned long ul; /* used for sorting when not FIELD_STR */
-};
+struct atom_value;
struct ref_sorting {
struct ref_sorting *next;
--
2.5.0
--
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
[PATCH v15 00/13] port builtin/tag.c to use ref-filter APIs.
The previous iteration of this series is found here:
http://article.gmane.org/gmane.comp.version-control.git/276779
Changes in this version:
* Make %(contents:lines=X) use existing ref-filter code rather
than reply completely on the code borrowed from tag.c.
* Make struct align and struct contents static variables inside
atom_value and put them inside an union.
* Since there was some redundant code movement, I moved most of
the code to the top. (Caused a noisy interdiff but should be more
benifital in the long run).
* Move the error checking of `align` into the block for processing
the align atom.
* Remove introduction of format_ref_array_item(), probably re-introduce
it if/when required.
* More tests added for %(contents:lines=X) and made them cleaner to read.
* Only the second element of the stack performs quoting at the end, all
elements after it perform no quoting themselves and rely on the quoting
performed by element two.
Karthik Nayak (13):
ref-filter: move `struct atom_value` to ref-filter.c
ref-filter: introduce ref_formatting_state and ref_formatting_stack
utf8: add function to align a string into given strbuf
ref-filter: introduce handler function for each atom
ref-filter: implement an `align` atom
ref-filter: add option to filter out tags, branches and remotes
ref-filter: add support for %(contents:lines=X)
ref-filter: add support to sort by version
ref-filter: add option to match literal pattern
tag.c: use 'ref-filter' data structures
tag.c: use 'ref-filter' APIs
tag.c: implement '--format' option
tag.c: implement '--merged' and '--no-merged' options
Documentation/git-for-each-ref.txt | 15 +-
Documentation/git-tag.txt | 27 ++-
builtin/for-each-ref.c | 1 +
builtin/tag.c | 368 +++
ref-filter.c | 382 -
ref-filter.h | 25 ++-
refs.c | 9 +
refs.h | 1 +
t/t6302-for-each-ref-filter.sh | 173 +
t/t7004-tag.sh | 47 -
utf8.c | 21 ++
utf8.h | 15 ++
12 files changed, 710 insertions(+), 374 deletions(-)
Interdiff between v14 and v15:
diff --git a/Documentation/git-for-each-ref.txt
b/Documentation/git-for-each-ref.txt
index 1b48b95..d039f40 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -133,8 +133,8 @@ align::
`` is either left, right or middle and `` is
the total length of the content with alignment. If the
contents length is more than the width then no alignment is
- performed. If used with '--quote' everything in between %(align:..)
- and %(end) is quoted.
+ performed. If used with '--quote' everything in between
+ %(align:..) and %(end) is quoted.
In addition to the above, for commit and tag objects, the header
field names (`tree`, `parent`, `object`, `type`, and `tag`) can
@@ -148,8 +148,8 @@ The complete message in a commit and tag object is
`contents`.
Its first line is `contents:subject`, where subject is the concatenation
of all lines of the commit message up to the first blank line. The next
line is 'contents:body', where body is all of the lines after the first
-blank line. Finally, the optional GPG signature is `contents:signature`.
-The first `N` lines of the object is obtained using `contents:lines=N`.
+blank line. The optional GPG signature is `contents:signature`. The
+first `N` lines of the message is obtained using `contents:lines=N`.
For sorting purposes, fields with numeric values sort in numeric
order (`objectsize`, `authordate`, `committerdate`, `taggerdate`).
diff --git a/ref-filter.c b/ref-filter.c
index f268cd7..b37d57a 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -61,6 +61,8 @@ static struct {
{ "contents:lines" },
};
+#define REF_FORMATTING_STATE_INIT { 0, NULL }
+
struct align {
align_type position;
unsigned int width;
@@ -71,8 +73,6 @@ struct contents {
struct object_id oid;
};
-#define REF_FORMATTING_STATE_INIT { 0, NULL }
-
struct ref_formatting_stack {
struct ref_formatting_stack *prev;
struct strbuf output;
@@ -87,8 +87,10 @@ struct ref_formatting_state {
struct atom_value {
const char *s;
- struct align *align;
- struct contents *contents;
+ union {
+ struct align align;
+ struct contents contents;
+ } u;
void (*handler)(struct atom_value *atomv, struct ref_formatting_state
*state);
unsigned long ul; /* used for sorting when not FIELD_STR */
};
@@ -183,6 +185,30 @@ static void quote_formatting(struct strbuf *s, const char
*str, int quote_style)
}
}
+static void align_handler(struct ref_formatting_stack *stack)
+{
+ struct align *align = (struct align
[PATCH v15 02/13] ref-filter: introduce ref_formatting_state and ref_formatting_stack
Introduce ref_formatting_state which will hold the formatted output
strbuf instead of directly printing to stdout. This will help us in
creating modifier atoms which modify the format specified before
printing to stdout.
Implement a stack machinery for ref_formatting_state, this allows us
to push and pop elements onto the stack. Whenever we pop an element
from the stack, the strbuf from that element is appended to the strbuf
of the next element on the stack, this will allow us to support
nesting of modifier atoms.
Rename some functions to reflect the changes made:
print_value() -> append_atom()
emit()-> append_literal()
Mentored-by: Christian Couder
Mentored-by: Matthieu Moy
Signed-off-by: Karthik Nayak
---
ref-filter.c | 78 +---
1 file changed, 59 insertions(+), 19 deletions(-)
diff --git a/ref-filter.c b/ref-filter.c
index e53c77e..432cea0 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -55,6 +55,18 @@ static struct {
{ "color" },
};
+#define REF_FORMATTING_STATE_INIT { 0, NULL }
+
+struct ref_formatting_stack {
+ struct ref_formatting_stack *prev;
+ struct strbuf output;
+};
+
+struct ref_formatting_state {
+ int quote_style;
+ struct ref_formatting_stack *stack;
+};
+
struct atom_value {
const char *s;
unsigned long ul; /* used for sorting when not FIELD_STR */
@@ -129,6 +141,27 @@ int parse_ref_filter_atom(const char *atom, const char *ep)
return at;
}
+static void push_stack_element(struct ref_formatting_stack **stack)
+{
+ struct ref_formatting_stack *s = xcalloc(1, sizeof(struct
ref_formatting_stack));
+
+ strbuf_init(&s->output, 0);
+ s->prev = *stack;
+ *stack = s;
+}
+
+static void pop_stack_element(struct ref_formatting_stack **stack)
+{
+ struct ref_formatting_stack *current = *stack;
+ struct ref_formatting_stack *prev = current->prev;
+
+ if (prev)
+ strbuf_addbuf(&prev->output, ¤t->output);
+ strbuf_release(¤t->output);
+ free(current);
+ *stack = prev;
+}
+
/*
* In a format string, find the next occurrence of %(atom).
*/
@@ -1195,30 +1228,27 @@ void ref_array_sort(struct ref_sorting *sorting, struct
ref_array *array)
qsort(array->items, array->nr, sizeof(struct ref_array_item *),
compare_refs);
}
-static void print_value(struct atom_value *v, int quote_style)
+static void append_atom(struct atom_value *v, struct ref_formatting_state
*state)
{
- struct strbuf sb = STRBUF_INIT;
- switch (quote_style) {
+ struct strbuf *s = &state->stack->output;
+
+ switch (state->quote_style) {
case QUOTE_NONE:
- fputs(v->s, stdout);
+ strbuf_addstr(s, v->s);
break;
case QUOTE_SHELL:
- sq_quote_buf(&sb, v->s);
+ sq_quote_buf(s, v->s);
break;
case QUOTE_PERL:
- perl_quote_buf(&sb, v->s);
+ perl_quote_buf(s, v->s);
break;
case QUOTE_PYTHON:
- python_quote_buf(&sb, v->s);
+ python_quote_buf(s, v->s);
break;
case QUOTE_TCL:
- tcl_quote_buf(&sb, v->s);
+ tcl_quote_buf(s, v->s);
break;
}
- if (quote_style != QUOTE_NONE) {
- fputs(sb.buf, stdout);
- strbuf_release(&sb);
- }
}
static int hex1(char ch)
@@ -1239,8 +1269,10 @@ static int hex2(const char *cp)
return -1;
}
-static void emit(const char *cp, const char *ep)
+static void append_literal(const char *cp, const char *ep, struct
ref_formatting_state *state)
{
+ struct strbuf *s = &state->stack->output;
+
while (*cp && (!ep || cp < ep)) {
if (*cp == '%') {
if (cp[1] == '%')
@@ -1248,13 +1280,13 @@ static void emit(const char *cp, const char *ep)
else {
int ch = hex2(cp + 1);
if (0 <= ch) {
- putchar(ch);
+ strbuf_addch(s, ch);
cp += 3;
continue;
}
}
}
- putchar(*cp);
+ strbuf_addch(s, *cp);
cp++;
}
}
@@ -1262,19 +1294,24 @@ static void emit(const char *cp, const char *ep)
void show_ref_array_item(struct ref_array_item *info, const char *format, int
quote_style)
{
const char *cp, *sp, *ep;
+ struct strbuf *final_buf;
+ struct ref_formatting_state state = REF_FORMATTING_STATE_INIT;
+
+ state.quote_style = quote_style;
+ push_stack_element(&state.stack);
for (cp = format; *cp && (sp = find_next(cp)); cp = ep + 1) {
[PATCH v15 03/13] utf8: add function to align a string into given strbuf
Add strbuf_utf8_align() which will align a given string into a strbuf
as per given align_type and width. If the width is greater than the
string length then no alignment is performed.
Helped-by: Eric Sunshine
Mentored-by: Christian Couder
Mentored-by: Matthieu Moy
Signed-off-by: Karthik Nayak
---
utf8.c | 21 +
utf8.h | 15 +++
2 files changed, 36 insertions(+)
diff --git a/utf8.c b/utf8.c
index 28e6d76..00e10c8 100644
--- a/utf8.c
+++ b/utf8.c
@@ -644,3 +644,24 @@ int skip_utf8_bom(char **text, size_t len)
*text += strlen(utf8_bom);
return 1;
}
+
+void strbuf_utf8_align(struct strbuf *buf, align_type position, unsigned int
width,
+ const char *s)
+{
+ int slen = strlen(s);
+ int display_len = utf8_strnwidth(s, slen, 0);
+ int utf8_compensation = slen - display_len;
+
+ if (display_len >= width) {
+ strbuf_addstr(buf, s);
+ return;
+ }
+
+ if (position == ALIGN_LEFT)
+ strbuf_addf(buf, "%-*s", width + utf8_compensation, s);
+ else if (position == ALIGN_MIDDLE) {
+ int left = (width - display_len) / 2;
+ strbuf_addf(buf, "%*s%-*s", left, "", width - left +
utf8_compensation, s);
+ } else if (position == ALIGN_RIGHT)
+ strbuf_addf(buf, "%*s", width + utf8_compensation, s);
+}
diff --git a/utf8.h b/utf8.h
index 5a9e94b..7930b44 100644
--- a/utf8.h
+++ b/utf8.h
@@ -55,4 +55,19 @@ int mbs_chrlen(const char **text, size_t *remainder_p, const
char *encoding);
*/
int is_hfs_dotgit(const char *path);
+typedef enum {
+ ALIGN_LEFT,
+ ALIGN_MIDDLE,
+ ALIGN_RIGHT
+} align_type;
+
+/*
+ * Align the string given and store it into a strbuf as per the
+ * 'position' and 'width'. If the given string length is larger than
+ * 'width' than then the input string is not truncated and no
+ * alignment is done.
+ */
+void strbuf_utf8_align(struct strbuf *buf, align_type position, unsigned int
width,
+ const char *s);
+
#endif
--
2.5.0
--
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
[PATCH v15 05/13] ref-filter: implement an `align` atom
Implement an `align` atom which left-, middle-, or right-aligns the
content between %(align:..) and %(end).
It is followed by `:,`, where the `` is
either left, right or middle and `` is the size of the area
into which the content will be placed. If the content between
%(align:) and %(end) is more than the width then no alignment is
performed. e.g. to align a refname atom to the middle with a total
width of 40 we can do: --format="%(align:middle,40)%(refname)%(end)".
We have an `at_end` function for each element of the stack which is to
be called when the `end` atom is encountered. Using this we implement
the aling_handler() for the `align` atom, this aligns the final strbuf
by calling `strbuf_utf8_align()` from utf8.c.
Ensure that quote formatting is performed on the whole of
%(align)...%(end) rather than individual atoms inside. We skip quote
formatting for individual atoms when the current stack element is
handling an %(align) atom and perform quote formatting at the end when
we encounter the %(end) atom.
Add documentation and tests for the same.
Mentored-by: Christian Couder
Mentored-by: Matthieu Moy
Signed-off-by: Karthik Nayak
---
Documentation/git-for-each-ref.txt | 9
ref-filter.c | 102 -
t/t6302-for-each-ref-filter.sh | 85 +++
3 files changed, 195 insertions(+), 1 deletion(-)
diff --git a/Documentation/git-for-each-ref.txt
b/Documentation/git-for-each-ref.txt
index e49d578..cac3128 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -127,6 +127,15 @@ color::
Change output color. Followed by `:`, where names
are described in `color.branch.*`.
+align::
+ Left-, middle-, or right-align the content between %(align:..)
+ and %(end). Followed by `:,`, where the
+ `` is either left, right or middle and `` is
+ the total length of the content with alignment. If the
+ contents length is more than the width then no alignment is
+ performed. If used with '--quote' everything in between
+ %(align:..) and %(end) is quoted.
+
In addition to the above, for commit and tag objects, the header
field names (`tree`, `parent`, `object`, `type`, and `tag`) can
be used to specify the value in the header field.
diff --git a/ref-filter.c b/ref-filter.c
index a993216..3a5f0a7 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -10,6 +10,7 @@
#include "quote.h"
#include "ref-filter.h"
#include "revision.h"
+#include "utf8.h"
typedef enum { FIELD_STR, FIELD_ULONG, FIELD_TIME } cmp_type;
@@ -53,13 +54,22 @@ static struct {
{ "flag" },
{ "HEAD" },
{ "color" },
+ { "align" },
+ { "end" },
};
#define REF_FORMATTING_STATE_INIT { 0, NULL }
+struct align {
+ align_type position;
+ unsigned int width;
+};
+
struct ref_formatting_stack {
struct ref_formatting_stack *prev;
struct strbuf output;
+ void (*at_end)(struct ref_formatting_stack *stack);
+ void *cb_data;
};
struct ref_formatting_state {
@@ -69,6 +79,7 @@ struct ref_formatting_state {
struct atom_value {
const char *s;
+ struct align align;
void (*handler)(struct atom_value *atomv, struct ref_formatting_state
*state);
unsigned long ul; /* used for sorting when not FIELD_STR */
};
@@ -163,9 +174,28 @@ static void quote_formatting(struct strbuf *s, const char
*str, int quote_style)
}
}
+static void align_handler(struct ref_formatting_stack *stack)
+{
+ struct align *align = (struct align *)stack->cb_data;
+ struct strbuf s = STRBUF_INIT;
+
+ strbuf_utf8_align(&s, align->position, align->width, stack->output.buf);
+ strbuf_swap(&stack->output, &s);
+ strbuf_release(&s);
+}
+
static void append_atom(struct atom_value *v, struct ref_formatting_state
*state)
{
- quote_formatting(&state->stack->output, v->s, state->quote_style);
+ /*
+* Quote formatting is only done when the stack has a single
+* element. Otherwise quote formatting is done on the
+* element's entire output strbuf when the %(end) atom is
+* encountered.
+*/
+ if (!state->stack->prev)
+ quote_formatting(&state->stack->output, v->s,
state->quote_style);
+ else
+ strbuf_addstr(&state->stack->output, v->s);
}
static void push_stack_element(struct ref_formatting_stack **stack)
@@ -189,6 +219,37 @@ static void pop_stack_element(struct ref_formatting_stack
**stack)
*stack = prev;
}
+static void align_atom_handler(struct atom_value *atomv, struct
ref_formatting_state *state)
+{
+ struct ref_formatting_stack *new;
+
+ push_stack_element(&state->stack);
+ new = state->stack;
+ new->at_end = align_handler;
+ new->cb_data = &atomv->align;
+}
+
+static void end_atom_handler(struct atom_value *atomv, struct
ref
[PATCH v15 04/13] ref-filter: introduce handler function for each atom
Introduce a handler function for each atom, which is called when the
atom is processed in show_ref_array_item().
In this context make append_atom() as the default handler function and
extract quote_formatting() out of append_atom(). Bump this to the top.
Mentored-by: Christian Couder
Mentored-by: Matthieu Moy
Signed-off-by: Karthik Nayak
---
ref-filter.c | 54 ++
1 file changed, 30 insertions(+), 24 deletions(-)
diff --git a/ref-filter.c b/ref-filter.c
index 432cea0..a993216 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -69,6 +69,7 @@ struct ref_formatting_state {
struct atom_value {
const char *s;
+ void (*handler)(struct atom_value *atomv, struct ref_formatting_state
*state);
unsigned long ul; /* used for sorting when not FIELD_STR */
};
@@ -141,6 +142,32 @@ int parse_ref_filter_atom(const char *atom, const char *ep)
return at;
}
+static void quote_formatting(struct strbuf *s, const char *str, int
quote_style)
+{
+ switch (quote_style) {
+ case QUOTE_NONE:
+ strbuf_addstr(s, str);
+ break;
+ case QUOTE_SHELL:
+ sq_quote_buf(s, str);
+ break;
+ case QUOTE_PERL:
+ perl_quote_buf(s, str);
+ break;
+ case QUOTE_PYTHON:
+ python_quote_buf(s, str);
+ break;
+ case QUOTE_TCL:
+ tcl_quote_buf(s, str);
+ break;
+ }
+}
+
+static void append_atom(struct atom_value *v, struct ref_formatting_state
*state)
+{
+ quote_formatting(&state->stack->output, v->s, state->quote_style);
+}
+
static void push_stack_element(struct ref_formatting_stack **stack)
{
struct ref_formatting_stack *s = xcalloc(1, sizeof(struct
ref_formatting_stack));
@@ -662,6 +689,8 @@ static void populate_value(struct ref_array_item *ref)
const char *formatp;
struct branch *branch = NULL;
+ v->handler = append_atom;
+
if (*name == '*') {
deref = 1;
name++;
@@ -1228,29 +1257,6 @@ void ref_array_sort(struct ref_sorting *sorting, struct
ref_array *array)
qsort(array->items, array->nr, sizeof(struct ref_array_item *),
compare_refs);
}
-static void append_atom(struct atom_value *v, struct ref_formatting_state
*state)
-{
- struct strbuf *s = &state->stack->output;
-
- switch (state->quote_style) {
- case QUOTE_NONE:
- strbuf_addstr(s, v->s);
- break;
- case QUOTE_SHELL:
- sq_quote_buf(s, v->s);
- break;
- case QUOTE_PERL:
- perl_quote_buf(s, v->s);
- break;
- case QUOTE_PYTHON:
- python_quote_buf(s, v->s);
- break;
- case QUOTE_TCL:
- tcl_quote_buf(s, v->s);
- break;
- }
-}
-
static int hex1(char ch)
{
if ('0' <= ch && ch <= '9')
@@ -1307,7 +1313,7 @@ void show_ref_array_item(struct ref_array_item *info,
const char *format, int qu
if (cp < sp)
append_literal(cp, sp, &state);
get_ref_atom_value(info, parse_ref_filter_atom(sp + 2, ep),
&atomv);
- append_atom(atomv, &state);
+ atomv->handler(atomv, &state);
}
if (*cp) {
sp = cp + strlen(cp);
--
2.5.0
--
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
[PATCH v15 09/13] ref-filter: add option to match literal pattern
From: Karthik Nayak
Since 'ref-filter' only has an option to match path names add an
option for plain fnmatch pattern-matching.
This is to support the pattern matching options which are used in `git
tag -l` and `git branch -l` where we can match patterns like `git tag
-l foo*` which would match all tags which has a "foo*" pattern.
Mentored-by: Christian Couder
Mentored-by: Matthieu Moy
Signed-off-by: Karthik Nayak
---
builtin/for-each-ref.c | 1 +
ref-filter.c | 40 +---
ref-filter.h | 3 ++-
3 files changed, 40 insertions(+), 4 deletions(-)
diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
index 40f343b..4e9f6c2 100644
--- a/builtin/for-each-ref.c
+++ b/builtin/for-each-ref.c
@@ -68,6 +68,7 @@ int cmd_for_each_ref(int argc, const char **argv, const char
*prefix)
git_config(git_default_config, NULL);
filter.name_patterns = argv;
+ filter.match_as_path = 1;
filter_refs(&array, &filter, FILTER_REFS_ALL |
FILTER_REFS_INCLUDE_BROKEN);
ref_array_sort(sorting, &array);
diff --git a/ref-filter.c b/ref-filter.c
index a545fd4..b37d57a 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1140,9 +1140,33 @@ static int commit_contains(struct ref_filter *filter,
struct commit *commit)
/*
* Return 1 if the refname matches one of the patterns, otherwise 0.
+ * A pattern can be a literal prefix (e.g. a refname "refs/heads/master"
+ * matches a pattern "refs/heads/mas") or a wildcard (e.g. the same ref
+ * matches "refs/heads/mas*", too).
+ */
+static int match_pattern(const char **patterns, const char *refname)
+{
+ /*
+* When no '--format' option is given we need to skip the prefix
+* for matching refs of tags and branches.
+*/
+ (void)(skip_prefix(refname, "refs/tags/", &refname) ||
+ skip_prefix(refname, "refs/heads/", &refname) ||
+ skip_prefix(refname, "refs/remotes/", &refname) ||
+ skip_prefix(refname, "refs/", &refname));
+
+ for (; *patterns; patterns++) {
+ if (!wildmatch(*patterns, refname, 0, NULL))
+ return 1;
+ }
+ return 0;
+}
+
+/*
+ * Return 1 if the refname matches one of the patterns, otherwise 0.
* A pattern can be path prefix (e.g. a refname "refs/heads/master"
- * matches a pattern "refs/heads/") or a wildcard (e.g. the same ref
- * matches "refs/heads/m*",too).
+ * matches a pattern "refs/heads/" but not "refs/heads/m") or a
+ * wildcard (e.g. the same ref matches "refs/heads/m*", too).
*/
static int match_name_as_path(const char **pattern, const char *refname)
{
@@ -1163,6 +1187,16 @@ static int match_name_as_path(const char **pattern,
const char *refname)
return 0;
}
+/* Return 1 if the refname matches one of the patterns, otherwise 0. */
+static int filter_pattern_match(struct ref_filter *filter, const char *refname)
+{
+ if (!*filter->name_patterns)
+ return 1; /* No pattern always matches */
+ if (filter->match_as_path)
+ return match_name_as_path(filter->name_patterns, refname);
+ return match_pattern(filter->name_patterns, refname);
+}
+
/*
* Given a ref (sha1, refname), check if the ref belongs to the array
* of sha1s. If the given ref is a tag, check if the given tag points
@@ -1269,7 +1303,7 @@ static int ref_filter_handler(const char *refname, const
struct object_id *oid,
if (!(kind & filter->kind))
return 0;
- if (*filter->name_patterns &&
!match_name_as_path(filter->name_patterns, refname))
+ if (!filter_pattern_match(filter, refname))
return 0;
if (filter->points_at.nr && !match_points_at(&filter->points_at,
oid->hash, refname))
diff --git a/ref-filter.h b/ref-filter.h
index ef25b6e..a5cfa5e 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -59,7 +59,8 @@ struct ref_filter {
} merge;
struct commit *merge_commit;
- unsigned int with_commit_tag_algo : 1;
+ unsigned int with_commit_tag_algo : 1,
+ match_as_path : 1;
unsigned int kind,
lines;
};
--
2.5.0
--
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
[PATCH v15 07/13] ref-filter: add support for %(contents:lines=X)
In 'tag.c' we can print N lines from the annotation of the tag using
the '-n' option. Copy code from 'tag.c' to 'ref-filter' and
modify it to support appending of N lines from the annotation of tags
to the given strbuf.
Implement %(contents:lines=X) where X lines of the given object are
obtained.
Add documentation and test for the same.
Mentored-by: Christian Couder
Mentored-by: Matthieu Moy
Signed-off-by: Karthik Nayak
---
Documentation/git-for-each-ref.txt | 3 ++-
builtin/tag.c | 4 +++
ref-filter.c | 53 +++---
ref-filter.h | 3 ++-
t/t6302-for-each-ref-filter.sh | 52 +
5 files changed, 109 insertions(+), 6 deletions(-)
diff --git a/Documentation/git-for-each-ref.txt
b/Documentation/git-for-each-ref.txt
index cac3128..98eb027 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -148,7 +148,8 @@ The complete message in a commit and tag object is
`contents`.
Its first line is `contents:subject`, where subject is the concatenation
of all lines of the commit message up to the first blank line. The next
line is 'contents:body', where body is all of the lines after the first
-blank line. Finally, the optional GPG signature is `contents:signature`.
+blank line. The optional GPG signature is `contents:signature`. The
+first `N` lines of the message is obtained using `contents:lines=N`.
For sorting purposes, fields with numeric values sort in numeric
order (`objectsize`, `authordate`, `committerdate`, `taggerdate`).
diff --git a/builtin/tag.c b/builtin/tag.c
index 471d6b1..b0bc1c5 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -185,6 +185,10 @@ static enum contains_result contains(struct commit
*candidate,
return contains_test(candidate, want);
}
+/*
+ * Currently modified and used in ref-filter as append_lines(), will
+ * eventually be removed as we port tag.c to use ref-filter APIs.
+ */
static void show_tag_lines(const struct object_id *oid, int lines)
{
int i;
diff --git a/ref-filter.c b/ref-filter.c
index 430c80b..0e58fee 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -56,6 +56,7 @@ static struct {
{ "color" },
{ "align" },
{ "end" },
+ { "contents:lines" },
};
#define REF_FORMATTING_STATE_INIT { 0, NULL }
@@ -65,6 +66,11 @@ struct align {
unsigned int width;
};
+struct contents {
+ unsigned int lines;
+ struct object_id oid;
+};
+
struct ref_formatting_stack {
struct ref_formatting_stack *prev;
struct strbuf output;
@@ -79,7 +85,10 @@ struct ref_formatting_state {
struct atom_value {
const char *s;
- struct align align;
+ union {
+ struct align align;
+ struct contents contents;
+ } u;
void (*handler)(struct atom_value *atomv, struct ref_formatting_state
*state);
unsigned long ul; /* used for sorting when not FIELD_STR */
};
@@ -226,7 +235,7 @@ static void align_atom_handler(struct atom_value *atomv,
struct ref_formatting_s
push_stack_element(&state->stack);
new = state->stack;
new->at_end = align_handler;
- new->cb_data = &atomv->align;
+ new->cb_data = &atomv->u.align;
}
static void end_atom_handler(struct atom_value *atomv, struct
ref_formatting_state *state)
@@ -624,6 +633,33 @@ static void find_subpos(const char *buf, unsigned long sz,
*nonsiglen = *sig - buf;
}
+/*
+ * If 'lines' is greater than 0, append that many lines from the given
+ * 'buf' of length 'size' to the given strbuf.
+ */
+static void append_lines(struct strbuf *out, const char *buf, unsigned long
size, int lines)
+{
+ int i;
+ const char *sp, *eol;
+ size_t len;
+
+ if ((sp = strstr(buf, "\n\n")) && (sp <= buf + size))
+ size += 2;
+
+ sp = buf;
+
+ for (i = 0; i < lines && sp < buf + size; i++) {
+ if (i)
+ strbuf_addstr(out, "\n");
+ eol = memchr(sp, '\n', size - (sp - buf));
+ len = eol ? eol - sp : size - (sp - buf);
+ strbuf_add(out, sp, len);
+ if (!eol)
+ break;
+ sp = eol + 1;
+ }
+}
+
/* See grab_values */
static void grab_sub_body_contents(struct atom_value *val, int deref, struct
object *obj, void *buf, unsigned long sz)
{
@@ -634,6 +670,7 @@ static void grab_sub_body_contents(struct atom_value *val,
int deref, struct obj
for (i = 0; i < used_atom_cnt; i++) {
const char *name = used_atom[i];
struct atom_value *v = &val[i];
+ const char *valp = NULL;
if (!!deref != (*name == '*'))
continue;
if (deref)
@@ -643,7 +680,8 @@ static void grab_sub_body_contents(struct atom_value *val,
int deref, struct obj
[PATCH v15 13/13] tag.c: implement '--merged' and '--no-merged' options
Use 'ref-filter' APIs to implement the '--merged' and '--no-merged'
options into 'tag.c'. The '--merged' option lets the user to only list
tags merged into the named commit. The '--no-merged' option lets the
user to only list tags not merged into the named commit. If no object
is provided it assumes HEAD as the object.
Add documentation and tests for the same.
Mentored-by: Christian Couder
Mentored-by: Matthieu Moy
Signed-off-by: Karthik Nayak
---
Documentation/git-tag.txt | 7 ++-
builtin/tag.c | 6 +-
t/t7004-tag.sh| 27 +++
3 files changed, 38 insertions(+), 2 deletions(-)
diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
index 0c7f4e6..3803bf7 100644
--- a/Documentation/git-tag.txt
+++ b/Documentation/git-tag.txt
@@ -14,7 +14,7 @@ SYNOPSIS
'git tag' -d ...
'git tag' [-n[]] -l [--contains ] [--points-at ]
[--column[=] | --no-column] [--create-reflog] [--sort=]
- [--format=] [...]
+ [--format=] [--[no-]merged []] [...]
'git tag' -v ...
DESCRIPTION
@@ -165,6 +165,11 @@ This option is only applicable when listing tags without
annotation lines.
that of linkgit:git-for-each-ref[1]. When unspecified,
defaults to `%(refname:short)`.
+--[no-]merged []::
+ Only list tags whose tips are reachable, or not reachable
+ if '--no-merged' is used, from the specified commit ('HEAD'
+ if not specified).
+
CONFIGURATION
-
By default, 'git tag' in sign-with-default mode (-s) will use your
diff --git a/builtin/tag.c b/builtin/tag.c
index dc4494d..9fa1400 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -23,7 +23,7 @@ static const char * const git_tag_usage[] = {
N_("git tag [-a | -s | -u ] [-f] [-m | -F ]
[]"),
N_("git tag -d ..."),
N_("git tag -l [-n[]] [--contains ] [--points-at ]"
- "\n\t\t[--format=] [...]"),
+ "\n\t\t[--format=] [--[no-]merged []]
[...]"),
N_("git tag -v ..."),
NULL
};
@@ -358,6 +358,8 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
OPT_COLUMN(0, "column", &colopts, N_("show tag list in
columns")),
OPT_CONTAINS(&filter.with_commit, N_("print only tags that
contain the commit")),
OPT_WITH(&filter.with_commit, N_("print only tags that contain
the commit")),
+ OPT_MERGED(&filter, N_("print only tags that are merged")),
+ OPT_NO_MERGED(&filter, N_("print only tags that are not
merged")),
OPT_CALLBACK(0 , "sort", sorting_tail, N_("key"),
N_("field name to sort on"),
&parse_opt_ref_sorting),
{
@@ -416,6 +418,8 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
die(_("--contains option is only allowed with -l."));
if (filter.points_at.nr)
die(_("--points-at option is only allowed with -l."));
+ if (filter.merge_commit)
+ die(_("--merged and --no-merged option are only allowed with
-l"));
if (cmdmode == 'd')
return for_each_tag_name(argv, delete_tag);
if (cmdmode == 'v')
diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index 8987fb1..3dd2f51 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -1531,4 +1531,31 @@ test_expect_success '--format should list tags as per
format given' '
test_cmp expect actual
'
+test_expect_success 'setup --merged test tags' '
+ git tag mergetest-1 HEAD~2 &&
+ git tag mergetest-2 HEAD~1 &&
+ git tag mergetest-3 HEAD
+'
+
+test_expect_success '--merged cannot be used in non-list mode' '
+ test_must_fail git tag --merged=mergetest-2 foo
+'
+
+test_expect_success '--merged shows merged tags' '
+ cat >expect <<-\EOF &&
+ mergetest-1
+ mergetest-2
+ EOF
+ git tag -l --merged=mergetest-2 mergetest-* >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success '--no-merged show unmerged tags' '
+ cat >expect <<-\EOF &&
+ mergetest-3
+ EOF
+ git tag -l --no-merged=mergetest-2 mergetest-* >actual &&
+ test_cmp expect actual
+'
+
test_done
--
2.5.0
--
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
[PATCH v15 10/13] tag.c: use 'ref-filter' data structures
From: Karthik Nayak
Make 'tag.c' use 'ref-filter' data structures and make changes to
support the new data structures. This is a part of the process
of porting 'tag.c' to use 'ref-filter' APIs.
This is a temporary step before porting 'tag.c' to use 'ref-filter'
completely. As this is a temporary step, most of the code
introduced here will be removed when 'tag.c' is ported over to use
'ref-filter' APIs.
Mentored-by: Christian Couder
Mentored-by: Matthieu Moy
Signed-off-by: Karthik Nayak
---
builtin/tag.c | 106 +++---
1 file changed, 57 insertions(+), 49 deletions(-)
diff --git a/builtin/tag.c b/builtin/tag.c
index b0bc1c5..fe66f7b 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -17,6 +17,7 @@
#include "gpg-interface.h"
#include "sha1-array.h"
#include "column.h"
+#include "ref-filter.h"
static const char * const git_tag_usage[] = {
N_("git tag [-a | -s | -u ] [-f] [-m | -F ]
[]"),
@@ -34,15 +35,6 @@ static const char * const git_tag_usage[] = {
static int tag_sort;
-struct tag_filter {
- const char **patterns;
- int lines;
- int sort;
- struct string_list tags;
- struct commit_list *with_commit;
-};
-
-static struct sha1_array points_at;
static unsigned int colopts;
static int match_pattern(const char **patterns, const char *ref)
@@ -61,19 +53,20 @@ static int match_pattern(const char **patterns, const char
*ref)
* removed as we port tag.c to use the ref-filter APIs.
*/
static const unsigned char *match_points_at(const char *refname,
- const unsigned char *sha1)
+ const unsigned char *sha1,
+ struct sha1_array *points_at)
{
const unsigned char *tagged_sha1 = NULL;
struct object *obj;
- if (sha1_array_lookup(&points_at, sha1) >= 0)
+ if (sha1_array_lookup(points_at, sha1) >= 0)
return sha1;
obj = parse_object(sha1);
if (!obj)
die(_("malformed object at '%s'"), refname);
if (obj->type == OBJ_TAG)
tagged_sha1 = ((struct tag *)obj)->tagged->sha1;
- if (tagged_sha1 && sha1_array_lookup(&points_at, tagged_sha1) >= 0)
+ if (tagged_sha1 && sha1_array_lookup(points_at, tagged_sha1) >= 0)
return tagged_sha1;
return NULL;
}
@@ -228,12 +221,24 @@ free_return:
free(buf);
}
+static void ref_array_append(struct ref_array *array, const char *refname)
+{
+ size_t len = strlen(refname);
+ struct ref_array_item *ref = xcalloc(1, sizeof(struct ref_array_item) +
len + 1);
+ memcpy(ref->refname, refname, len);
+ ref->refname[len] = '\0';
+ REALLOC_ARRAY(array->items, array->nr + 1);
+ array->items[array->nr++] = ref;
+}
+
static int show_reference(const char *refname, const struct object_id *oid,
int flag, void *cb_data)
{
- struct tag_filter *filter = cb_data;
+ struct ref_filter_cbdata *data = cb_data;
+ struct ref_array *array = data->array;
+ struct ref_filter *filter = data->filter;
- if (match_pattern(filter->patterns, refname)) {
+ if (match_pattern(filter->name_patterns, refname)) {
if (filter->with_commit) {
struct commit *commit;
@@ -244,12 +249,12 @@ static int show_reference(const char *refname, const
struct object_id *oid,
return 0;
}
- if (points_at.nr && !match_points_at(refname, oid->hash))
+ if (filter->points_at.nr && !match_points_at(refname,
oid->hash, &filter->points_at))
return 0;
if (!filter->lines) {
- if (filter->sort)
- string_list_append(&filter->tags, refname);
+ if (tag_sort)
+ ref_array_append(array, refname);
else
printf("%s\n", refname);
return 0;
@@ -264,36 +269,36 @@ static int show_reference(const char *refname, const
struct object_id *oid,
static int sort_by_version(const void *a_, const void *b_)
{
- const struct string_list_item *a = a_;
- const struct string_list_item *b = b_;
- return versioncmp(a->string, b->string);
+ const struct ref_array_item *a = *((struct ref_array_item **)a_);
+ const struct ref_array_item *b = *((struct ref_array_item **)b_);
+ return versioncmp(a->refname, b->refname);
}
-static int list_tags(const char **patterns, int lines,
-struct commit_list *with_commit, int sort)
+static int list_tags(struct ref_filter *filter, int sort)
{
- struct tag_filter filter;
+ struct ref_array array;
+ struct ref_filter_cbdata data;
+
+ memset(&array, 0, si
[PATCH v15 06/13] ref-filter: add option to filter out tags, branches and remotes
From: Karthik Nayak
Add a function called 'for_each_fullref_in()' to refs.{c,h} which
iterates through each ref for the given path without trimming the path
and also accounting for broken refs, if mentioned.
Add 'filter_ref_kind()' in ref-filter.c to check the kind of ref being
handled and return the kind to 'ref_filter_handler()', where we
discard refs which we do not need and assign the kind to needed refs.
Mentored-by: Christian Couder
Mentored-by: Matthieu Moy
Signed-off-by: Karthik Nayak
---
ref-filter.c | 63 +++-
ref-filter.h | 13 +++--
refs.c | 9 +
refs.h | 1 +
4 files changed, 79 insertions(+), 7 deletions(-)
diff --git a/ref-filter.c b/ref-filter.c
index 3a5f0a7..430c80b 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1163,6 +1163,34 @@ static struct ref_array_item *new_ref_array_item(const
char *refname,
return ref;
}
+static int filter_ref_kind(struct ref_filter *filter, const char *refname)
+{
+ unsigned int i;
+
+ static struct {
+ const char *prefix;
+ unsigned int kind;
+ } ref_kind[] = {
+ { "refs/heads/" , FILTER_REFS_BRANCHES },
+ { "refs/remotes/" , FILTER_REFS_REMOTES },
+ { "refs/tags/", FILTER_REFS_TAGS}
+ };
+
+ if (filter->kind == FILTER_REFS_BRANCHES ||
+ filter->kind == FILTER_REFS_REMOTES ||
+ filter->kind == FILTER_REFS_TAGS)
+ return filter->kind;
+ else if (!strcmp(refname, "HEAD"))
+ return FILTER_REFS_DETACHED_HEAD;
+
+ for (i = 0; i < ARRAY_SIZE(ref_kind); i++) {
+ if (starts_with(refname, ref_kind[i].prefix))
+ return ref_kind[i].kind;
+ }
+
+ return FILTER_REFS_OTHERS;
+}
+
/*
* A call-back given to for_each_ref(). Filter refs and keep them for
* later object processing.
@@ -1173,6 +1201,7 @@ static int ref_filter_handler(const char *refname, const
struct object_id *oid,
struct ref_filter *filter = ref_cbdata->filter;
struct ref_array_item *ref;
struct commit *commit = NULL;
+ unsigned int kind;
if (flag & REF_BAD_NAME) {
warning("ignoring ref with broken name %s", refname);
@@ -1184,6 +1213,15 @@ static int ref_filter_handler(const char *refname, const
struct object_id *oid,
return 0;
}
+ /*
+* Get the current ref kind. If we're filtering tags, remotes or local
branches
+* only then the current ref-kind is nothing but filter->kind and
filter_ref_kind()
+* will only return that value.
+*/
+ kind = filter_ref_kind(filter, refname);
+ if (!(kind & filter->kind))
+ return 0;
+
if (*filter->name_patterns &&
!match_name_as_path(filter->name_patterns, refname))
return 0;
@@ -1215,6 +1253,7 @@ static int ref_filter_handler(const char *refname, const
struct object_id *oid,
REALLOC_ARRAY(ref_cbdata->array->items, ref_cbdata->array->nr + 1);
ref_cbdata->array->items[ref_cbdata->array->nr++] = ref;
+ ref->kind = kind;
return 0;
}
@@ -1291,17 +1330,31 @@ int filter_refs(struct ref_array *array, struct
ref_filter *filter, unsigned int
{
struct ref_filter_cbdata ref_cbdata;
int ret = 0;
+ unsigned int broken = 0;
ref_cbdata.array = array;
ref_cbdata.filter = filter;
+ if (type & FILTER_REFS_INCLUDE_BROKEN)
+ broken = 1;
+ filter->kind = type & FILTER_REFS_KIND_MASK;
+
/* Simple per-ref filtering */
- if (type & (FILTER_REFS_ALL | FILTER_REFS_INCLUDE_BROKEN))
- ret = for_each_rawref(ref_filter_handler, &ref_cbdata);
- else if (type & FILTER_REFS_ALL)
- ret = for_each_ref(ref_filter_handler, &ref_cbdata);
- else if (type)
+ if (!filter->kind)
die("filter_refs: invalid type");
+ else {
+ if (filter->kind == FILTER_REFS_BRANCHES)
+ ret = for_each_fullref_in("refs/heads/",
ref_filter_handler, &ref_cbdata, broken);
+ else if (filter->kind == FILTER_REFS_REMOTES)
+ ret = for_each_fullref_in("refs/remotes/",
ref_filter_handler, &ref_cbdata, broken);
+ else if (filter->kind == FILTER_REFS_TAGS)
+ ret = for_each_fullref_in("refs/tags/",
ref_filter_handler, &ref_cbdata, broken);
+ else if (filter->kind & FILTER_REFS_ALL)
+ ret = for_each_fullref_in("", ref_filter_handler,
&ref_cbdata, broken);
+ if (!ret && (filter->kind & FILTER_REFS_DETACHED_HEAD))
+ head_ref(ref_filter_handler, &ref_cbdata);
+ }
+
/* Filters that need revision walking */
if (filter->merge_commit)
diff --git a/ref-filter.h b/ref-filter.h
index
[PATCH v15 11/13] tag.c: use 'ref-filter' APIs
From: Karthik Nayak
Make 'tag.c' use 'ref-filter' APIs for iterating through refs, sorting
and printing of refs. This removes most of the code used in 'tag.c'
replacing it with calls to the 'ref-filter' library.
Make 'tag.c' use the 'filter_refs()' function provided by 'ref-filter'
to filter out tags based on the options set.
For printing tags we use 'show_ref_array_item()' function provided by
'ref-filter'.
We improve the sorting option provided by 'tag.c' by using the sorting
options provided by 'ref-filter'. This causes the test 'invalid sort
parameter on command line' in t7004 to fail, as 'ref-filter' throws an
error for all sorting fields which are incorrect. The test is changed
to reflect the same.
Modify documentation for the same.
Mentored-by: Christian Couder
Mentored-by: Matthieu Moy
Signed-off-by: Karthik Nayak
---
Documentation/git-tag.txt | 16 ++-
builtin/tag.c | 344 ++
t/t7004-tag.sh| 8 +-
3 files changed, 52 insertions(+), 316 deletions(-)
diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
index 84f6496..3ac4a96 100644
--- a/Documentation/git-tag.txt
+++ b/Documentation/git-tag.txt
@@ -13,7 +13,7 @@ SYNOPSIS
[ | ]
'git tag' -d ...
'git tag' [-n[]] -l [--contains ] [--points-at ]
- [--column[=] | --no-column] [--create-reflog] [...]
+ [--column[=] | --no-column] [--create-reflog] [--sort=]
[...]
'git tag' -v ...
DESCRIPTION
@@ -94,14 +94,16 @@ OPTIONS
using fnmatch(3)). Multiple patterns may be given; if any of
them matches, the tag is shown.
---sort=::
- Sort in a specific order. Supported type is "refname"
- (lexicographic order), "version:refname" or "v:refname" (tag
+--sort=::
+ Sort based on the key given. Prefix `-` to sort in
+ descending order of the value. You may use the --sort= option
+ multiple times, in which case the last key becomes the primary
+ key. Also supports "version:refname" or "v:refname" (tag
names are treated as versions). The "version:refname" sort
order can also be affected by the
- "versionsort.prereleaseSuffix" configuration variable. Prepend
- "-" to reverse sort order. When this option is not given, the
- sort order defaults to the value configured for the 'tag.sort'
+ "versionsort.prereleaseSuffix" configuration variable.
+ The keys supported are the same as those in `git for-each-ref`.
+ Sort order defaults to the value configured for the 'tag.sort'
variable if it exists, or lexicographic order otherwise. See
linkgit:git-config[1].
diff --git a/builtin/tag.c b/builtin/tag.c
index fe66f7b..2d348f4 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -28,278 +28,34 @@ static const char * const git_tag_usage[] = {
NULL
};
-#define STRCMP_SORT 0 /* must be zero */
-#define VERCMP_SORT 1
-#define SORT_MASK 0x7fff
-#define REVERSE_SORT0x8000
-
-static int tag_sort;
-
static unsigned int colopts;
-static int match_pattern(const char **patterns, const char *ref)
-{
- /* no pattern means match everything */
- if (!*patterns)
- return 1;
- for (; *patterns; patterns++)
- if (!wildmatch(*patterns, ref, 0, NULL))
- return 1;
- return 0;
-}
-
-/*
- * This is currently duplicated in ref-filter.c, and will eventually be
- * removed as we port tag.c to use the ref-filter APIs.
- */
-static const unsigned char *match_points_at(const char *refname,
- const unsigned char *sha1,
- struct sha1_array *points_at)
-{
- const unsigned char *tagged_sha1 = NULL;
- struct object *obj;
-
- if (sha1_array_lookup(points_at, sha1) >= 0)
- return sha1;
- obj = parse_object(sha1);
- if (!obj)
- die(_("malformed object at '%s'"), refname);
- if (obj->type == OBJ_TAG)
- tagged_sha1 = ((struct tag *)obj)->tagged->sha1;
- if (tagged_sha1 && sha1_array_lookup(points_at, tagged_sha1) >= 0)
- return tagged_sha1;
- return NULL;
-}
-
-static int in_commit_list(const struct commit_list *want, struct commit *c)
-{
- for (; want; want = want->next)
- if (!hashcmp(want->item->object.sha1, c->object.sha1))
- return 1;
- return 0;
-}
-
-/*
- * The entire code segment for supporting the --contains option has been
- * copied over to ref-filter.{c,h}. This will be deleted evetually when
- * we port tag.c to use ref-filter APIs.
- */
-enum contains_result {
- CONTAINS_UNKNOWN = -1,
- CONTAINS_NO = 0,
- CONTAINS_YES = 1
-};
-
-/*
- * Test whether the candidate or one of its parents is contained in the list.
- * Do not recurse to find out, though, but return -1 if inconclusive.
- */
-static enum contains_re
[PATCH v15 08/13] ref-filter: add support to sort by version
From: Karthik Nayak
Add support to sort by version using the "v:refname" and
"version:refname" option. This is achieved by using the 'versioncmp()'
function as the comparing function for qsort.
This option is included to support sorting by versions in `git tag -l`
which will eventually be ported to use ref-filter APIs.
Add documentation and tests for the same.
Mentored-by: Christian Couder
Mentored-by: Matthieu Moy
Signed-off-by: Karthik Nayak
---
Documentation/git-for-each-ref.txt | 3 +++
ref-filter.c | 15 ++-
ref-filter.h | 3 ++-
t/t6302-for-each-ref-filter.sh | 36
4 files changed, 51 insertions(+), 6 deletions(-)
diff --git a/Documentation/git-for-each-ref.txt
b/Documentation/git-for-each-ref.txt
index 98eb027..d039f40 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -155,6 +155,9 @@ For sorting purposes, fields with numeric values sort in
numeric
order (`objectsize`, `authordate`, `committerdate`, `taggerdate`).
All other fields are used to sort in their byte-value order.
+There is also an option to sort by versions, this can be done by using
+the fieldname `version:refname` or its alias `v:refname`.
+
In any case, a field name that refers to a field inapplicable to
the object referred by the ref does not cause an error. It
returns an empty string instead.
diff --git a/ref-filter.c b/ref-filter.c
index 0e58fee..a545fd4 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -11,6 +11,8 @@
#include "ref-filter.h"
#include "revision.h"
#include "utf8.h"
+#include "git-compat-util.h"
+#include "version.h"
typedef enum { FIELD_STR, FIELD_ULONG, FIELD_TIME } cmp_type;
@@ -1416,19 +1418,19 @@ static int cmp_ref_sorting(struct ref_sorting *s,
struct ref_array_item *a, stru
get_ref_atom_value(a, s->atom, &va);
get_ref_atom_value(b, s->atom, &vb);
- switch (cmp_type) {
- case FIELD_STR:
+ if (s->version)
+ cmp = versioncmp(va->s, vb->s);
+ else if (cmp_type == FIELD_STR)
cmp = strcmp(va->s, vb->s);
- break;
- default:
+ else {
if (va->ul < vb->ul)
cmp = -1;
else if (va->ul == vb->ul)
cmp = 0;
else
cmp = 1;
- break;
}
+
return (s->reverse) ? -cmp : cmp;
}
@@ -1561,6 +1563,9 @@ int parse_opt_ref_sorting(const struct option *opt, const
char *arg, int unset)
s->reverse = 1;
arg++;
}
+ if (skip_prefix(arg, "version:", &arg) ||
+ skip_prefix(arg, "v:", &arg))
+ s->version = 1;
len = strlen(arg);
s->atom = parse_ref_filter_atom(arg, arg+len);
return 0;
diff --git a/ref-filter.h b/ref-filter.h
index ab76b22..ef25b6e 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -28,7 +28,8 @@ struct atom_value;
struct ref_sorting {
struct ref_sorting *next;
int atom; /* index into used_atom array (internal) */
- unsigned reverse : 1;
+ unsigned reverse : 1,
+ version : 1;
};
struct ref_array_item {
diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh
index 75da32f..d95b781 100755
--- a/t/t6302-for-each-ref-filter.sh
+++ b/t/t6302-for-each-ref-filter.sh
@@ -218,4 +218,40 @@ test_expect_success '`%(contents:lines=-1)` should fail' '
test_must_fail git for-each-ref --format="%(refname:short)
|%(contents:lines=-1)"
'
+test_expect_success 'setup for version sort' '
+ test_commit foo1.3 &&
+ test_commit foo1.6 &&
+ test_commit foo1.10
+'
+
+test_expect_success 'version sort' '
+ git for-each-ref --sort=version:refname --format="%(refname:short)"
refs/tags/ | grep "foo" >actual &&
+ cat >expect <<-\EOF &&
+ foo1.3
+ foo1.6
+ foo1.10
+ EOF
+ test_cmp expect actual
+'
+
+test_expect_success 'version sort (shortened)' '
+ git for-each-ref --sort=v:refname --format="%(refname:short)"
refs/tags/ | grep "foo" >actual &&
+ cat >expect <<-\EOF &&
+ foo1.3
+ foo1.6
+ foo1.10
+ EOF
+ test_cmp expect actual
+'
+
+test_expect_success 'reverse version sort' '
+ git for-each-ref --sort=-version:refname --format="%(refname:short)"
refs/tags/ | grep "foo" >actual &&
+ cat >expect <<-\EOF &&
+ foo1.10
+ foo1.6
+ foo1.3
+ EOF
+ test_cmp expect actual
+'
+
test_done
--
2.5.0
--
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
[PATCH v15 12/13] tag.c: implement '--format' option
Implement the '--format' option provided by 'ref-filter'.
This lets the user list tags as per desired format similar
to the implementation in 'git for-each-ref'.
Add tests and documentation for the same.
Mentored-by: Christian Couder
Mentored-by: Matthieu Moy
Signed-off-by: Karthik Nayak
---
Documentation/git-tag.txt | 8 +++-
builtin/tag.c | 22 +-
t/t7004-tag.sh| 12
3 files changed, 32 insertions(+), 10 deletions(-)
diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
index 3ac4a96..0c7f4e6 100644
--- a/Documentation/git-tag.txt
+++ b/Documentation/git-tag.txt
@@ -13,7 +13,8 @@ SYNOPSIS
[ | ]
'git tag' -d ...
'git tag' [-n[]] -l [--contains ] [--points-at ]
- [--column[=] | --no-column] [--create-reflog] [--sort=]
[...]
+ [--column[=] | --no-column] [--create-reflog] [--sort=]
+ [--format=] [...]
'git tag' -v ...
DESCRIPTION
@@ -158,6 +159,11 @@ This option is only applicable when listing tags without
annotation lines.
The object that the new tag will refer to, usually a commit.
Defaults to HEAD.
+::
+ A string that interpolates `%(fieldname)` from the object
+ pointed at by a ref being shown. The format is the same as
+ that of linkgit:git-for-each-ref[1]. When unspecified,
+ defaults to `%(refname:short)`.
CONFIGURATION
-
diff --git a/builtin/tag.c b/builtin/tag.c
index 2d348f4..dc4494d 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -23,17 +23,17 @@ static const char * const git_tag_usage[] = {
N_("git tag [-a | -s | -u ] [-f] [-m | -F ]
[]"),
N_("git tag -d ..."),
N_("git tag -l [-n[]] [--contains ] [--points-at ]"
- "\n\t\t[...]"),
+ "\n\t\t[--format=] [...]"),
N_("git tag -v ..."),
NULL
};
static unsigned int colopts;
-static int list_tags(struct ref_filter *filter, struct ref_sorting *sorting)
+static int list_tags(struct ref_filter *filter, struct ref_sorting *sorting,
const char *format)
{
struct ref_array array;
- char *format, *to_free = NULL;
+ char *to_free = NULL;
int i;
memset(&array, 0, sizeof(array));
@@ -41,11 +41,13 @@ static int list_tags(struct ref_filter *filter, struct
ref_sorting *sorting)
if (filter->lines == -1)
filter->lines = 0;
- if (filter->lines)
- format = to_free =
xstrfmt("%%(align:15,left)%%(refname:short)%%(end) %%(contents:lines=%d)",
- filter->lines);
- else
- format = "%(refname:short)";
+ if (!format) {
+ if (filter->lines)
+ format = to_free =
xstrfmt("%%(align:15,left)%%(refname:short)%%(end) %%(contents:lines=%d)",
+ filter->lines);
+ else
+ format = "%(refname:short)";
+ }
verify_ref_format(format);
filter_refs(&array, filter, FILTER_REFS_TAGS);
@@ -329,6 +331,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
struct strbuf err = STRBUF_INIT;
struct ref_filter filter;
static struct ref_sorting *sorting = NULL, **sorting_tail = &sorting;
+ const char *format = NULL;
struct option options[] = {
OPT_CMDMODE('l', "list", &cmdmode, N_("list tag names"), 'l'),
{ OPTION_INTEGER, 'n', NULL, &filter.lines, N_("n"),
@@ -361,6 +364,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
OPTION_CALLBACK, 0, "points-at", &filter.points_at,
N_("object"),
N_("print only tags of the object"), 0,
parse_opt_object_name
},
+ OPT_STRING( 0 , "format", &format, N_("format"), N_("format to
use for the output")),
OPT_END()
};
@@ -401,7 +405,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
run_column_filter(colopts, &copts);
}
filter.name_patterns = argv;
- ret = list_tags(&filter, sorting);
+ ret = list_tags(&filter, sorting, format);
if (column_active(colopts))
stop_column_filter();
return ret;
diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index 84153ef..8987fb1 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -1519,4 +1519,16 @@ EOF"
test_cmp expect actual
'
+test_expect_success '--format should list tags as per format given' '
+ cat >expect <<-\EOF &&
+ refname : refs/tags/foo1.10
+ refname : refs/tags/foo1.3
+ refname : refs/tags/foo1.6
+ refname : refs/tags/foo1.6-rc1
+ refname : refs/tags/foo1.6-rc2
+ EOF
+ git tag -l --format="refname : %(refname)" "foo*" >actual &&
+ test_cmp expect actual
+'
+
test_done
--
Re: GSoC 2015 is over
On Tue, Sep 01, 2015 at 06:55:04PM +0200, Matthieu Moy wrote: > I consider this GSoC as a great success and a pleasant experience. > Congratulation to Paul and Karthik, and a warm "thank you" to everybody > who contributed: administrators, mentors, reviewers, and obviously > Junio! (not to mention Google, who made all this possible) I think both students did a great job this year. I was especially pleased to see the degree to which the students were present on the list, posting patches early, getting review, and engaging in discussion. That is how non-GSoC contributors work, and I hope it is how all GSoCs will go in the future. I also want to also say a special thank you to Matthieu. After agreeing to sign up as the backup administrator, he basically did all of the administrative work this year. Also how I hope GSoCs will go in the future. ;) -Peff -- 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
Re: [PATCH 3/3] submodule: implement `module_clone` as a builtin helper
On Tue, Sep 1, 2015 at 1:49 PM, Stefan Beller wrote:
> took all suggestions except the one below.
>
>>
>> if (strbuf_getcwd(&sb))
>> die_errno(...);
>> strbuf_addf(&sb, "/%s, sm_gitdir);
>> free(sm_gitdir);
>> sm_gitdir = strbuf_detach(&sb, NULL);
>>
>>> + }
>>> +
>>> + if (strbuf_getcwd(&sb))
>>> + die_errno("unable to get current working directory");
>>
>> The conditional block just above here also gets 'cwd'. If you move
>> this code above the !is_absolute_path(sm_gitdir) conditional block,
>> then you can avoid the duplication.
>
> I don't get it. We need to have strbuf_getcwd twice no matter how we
> arrange the code
> as we strbuf_detach will empty the strbuf.
>
> Do you mean to call strbuf_getcwd just once and then xstrdup the value,
> then reset the strbuf to just contain the cwd and append the other string ?
Sorry, I have no idea what you are asking.
All I am saying is that you're unnecessarily invoking getcwd() twice.
It's value doesn't change between invocations, so the second call is
entirely redundant and wasteful. You can instead call it just once and
use the result in both places.
--
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
Re: [PATCHv4 3/3] submodule: implement `clone` as a builtin helper
On Tue, Sep 1, 2015 at 2:24 PM, Stefan Beller wrote:
> This converts implements the helper `module_clone`. This functionality is
Mentioned previously[1]: "converts implements"?
[1]: http://article.gmane.org/gmane.comp.version-control.git/276966
> needed for converting `git submodule update` later on, which we want to
> add threading to.
>
> Signed-off-by: Stefan Beller
> ---
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index f5e408a..63f535a 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> +static int module_clone(int argc, const char **argv, const char *prefix)
> +{
> + const char *path = NULL, *name = NULL, *url = NULL;
> + const char *reference = NULL, *depth = NULL;
> + const char *alternative_path = NULL, *p;
> + int quiet = 0;
> + FILE *submodule_dot_git;
> + char *sm_gitdir;
> + struct strbuf rel_path = STRBUF_INIT;
> + struct strbuf sb = STRBUF_INIT;
> +
> + struct option module_clone_options[] = {
> + OPT_STRING(0, "prefix", &alternative_path,
> + N_("path"),
> + N_("alternative anchor for relative paths")),
> + OPT_STRING(0, "path", &path,
> + N_("path"),
> + N_("where the new submodule will be cloned to")),
> + OPT_STRING(0, "name", &name,
> + N_("string"),
> + N_("name of the new submodule")),
> + OPT_STRING(0, "url", &url,
> + N_("string"),
> + N_("url where to clone the submodule from")),
> + OPT_STRING(0, "reference", &reference,
> + N_("string"),
> + N_("reference repository")),
> + OPT_STRING(0, "depth", &depth,
> + N_("string"),
> + N_("depth for shallow clones")),
> + OPT__QUIET(&quiet, "Suppress output for cloning a submodule"),
> + OPT_END()
> + };
> +
> + const char * const git_submodule_helper_usage[] = {
Style: *const
> + N_("git submodule--helper clone [--prefix=] [--quiet] "
> + "[--reference ] [--name ] [--url ]"
> + "[--depth ] [--] [...]"),
> + NULL
> + };
> +
> + argc = parse_options(argc, argv, prefix, module_clone_options,
> +git_submodule_helper_usage, 0);
> +
> + strbuf_addf(&sb, "%s/modules/%s", get_git_dir(), name);
> + sm_gitdir = strbuf_detach(&sb, NULL);
> +
> + if (!file_exists(sm_gitdir)) {
> + if (safe_create_leading_directories_const(sm_gitdir) < 0)
> + die(_("could not create directory '%s'"), sm_gitdir);
> + if (clone_submodule(path, sm_gitdir, url, depth, reference,
> quiet))
> + die(N_("clone of '%s' into submodule path '%s'
> failed"),
> + url, path);
s/N_/_/
> + } else {
> + if (safe_create_leading_directories_const(path) < 0)
> + die(_("could not create directory '%s'"), path);
> + if (unlink(sm_gitdir) < 0)
> + die_errno(_("failed to delete '%s'"), sm_gitdir);
> + }
> +
> + /* Write a .git file in the submodule to redirect to the
> superproject. */
> + if (alternative_path && *alternative_path)) {
> + p = relative_path(path, alternative_path, &sb);
> + strbuf_reset(&sb);
> + } else
> + p = path;
> +
> + if (safe_create_leading_directories_const(p) < 0)
> + die(_("could not create directory '%s'"), p);
> +
> + strbuf_addf(&sb, "%s/.git", p);
> +
> + if (safe_create_leading_directories_const(sb.buf) < 0)
> + die(_("could not create leading directories of '%s'"),
> sb.buf);
> + submodule_dot_git = fopen(sb.buf, "w");
> + if (!submodule_dot_git)
> + die (_("cannot open file '%s': %s"), sb.buf, strerror(errno));
Style: drop space before '('
Also, can't you use die_errno() here?
> + fprintf(submodule_dot_git, "gitdir: %s\n",
> + relative_path(sm_gitdir, path, &rel_path));
> + if (fclose(submodule_dot_git))
> + die(_("could not close file %s"), sb.buf);
> + strbuf_reset(&sb);
> +
> + /* Redirect the worktree of the submodule in the superproject's
> config */
> + if (strbuf_getcwd(&sb))
> + die_errno(_("unable to get current working directory"));
> +
> + if (!is_absolute_path(sm_gitdir)) {
> + if (strbuf_getcwd(&sb))
> + die_errno(_("unable to get current working
> directory"));
Why does this need to call getcwd() on 'sb' when it was called
immediately above the conditional and
Re: [PATCH v3] git-p4: add "--path-encoding" option
On 01 Sep 2015, at 19:35, Junio C Hamano wrote: > Lars Schneider writes: > >> On 01 Sep 2015, at 01:13, Junio C Hamano wrote: >> >>> larsxschnei...@gmail.com writes: >>> From: Lars Schneider >>> >>> Here is a space for you to describe what it does and why it is a >>> good idea to have it. >> How about this: >> >> Perforce keeps the encoding of a path as given by the originating >> OS. Git expects paths encoded as UTF-8. Add an option to tell git-p4 >> what encoding Perforce had used for the paths. This encoding is used >> to transcode the paths to UTF-8. As an example, Perforce on Windows >> uses “cp1252” to encode path names. > > Very readable. Does "Perforce on Windows" always use cp1252, or > is it more correct to say "often uses" here? Thank you! I don’t know if “always” or “often” is better. On my Windows test system it is “always”… but that’s not a valid sample size :-) I searched the Internet for clues around cp1252 and found that a similar patch was submitted to Mercurial just a month ago. The author seconds my cp1252 observation: http://mercurial.808500.n3.nabble.com/PATCH-stable-convert-use-original-local-encoding-when-converting-from-Perfoce-tp4025088p4025094.html - Lars-- 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
Re: [PATCHv4 2/3] submodule: implement `name` as a builtin helper
On Tue, Sep 1, 2015 at 2:24 PM, Stefan Beller wrote:
> This implements the helper `module_name` in C instead of shell,
You probably want s/module_name/name/ or state more explicitly:
Reimplement `module_name` shell function in C as `name`.
or something.
> yielding a nice performance boost.
>
> Signed-off-by: Stefan Beller
> ---
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index 16d9abe..f5e408a 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -102,6 +105,24 @@ static int module_list(int argc, const char **argv,
> const char *prefix)
> +static int module_name(int argc, const char **argv, const char *prefix)
> +{
> + const struct submodule *sub;
> +
> + if (argc != 2)
> + usage("git submodule--helper module_name \n");
Mentioned previously[1]:
usage(_("..."));
s/module_name/name/
s/\\n//
[1]: http://article.gmane.org/gmane.comp.version-control.git/276965
> + gitmodules_config();
> + sub = submodule_from_path(null_sha1, argv[1]);
> +
> + if (!sub)
> + die(N_("No submodule mapping found in .gitmodules for path
> '%s'"),
> + argv[1]);
s/N_/_/
s/No/no/
> + printf("%s\n", sub->name);
> +
> + return 0;
> +}
>
> struct cmd_struct {
> const char *cmd;
> @@ -110,6 +131,7 @@ struct cmd_struct {
>
> static struct cmd_struct commands[] = {
> {"list", module_list},
> + {"name", module_name},
> };
--
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
Re: [PATCH v3] git-p4: add "--path-encoding" option
Lars Schneider writes: > I searched the Internet for clues around cp1252 and found that a > similar patch was submitted to Mercurial just a month ago. The author > seconds my cp1252 observation: > http://mercurial.808500.n3.nabble.com/PATCH-stable-convert-use-original-local-encoding-when-converting-from-Perfoce-tp4025088p4025094.html Thanks for a pointer. I see this bit in that thread: If P4CHARSET is not set explicitly when connecting to a Unicode mode server, a default charset will be chosen based on the client's platform and/or code page. So I would guess that it is reproducible for you as you are always on a client with the same platform and/or code page, and it would be more honest to phrase it as "As an example, Perforce on my Windows box uses..." or something along that line. Thanks. -- 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
Re: Windows path handling changed between versions
I can (eventually) make a proper bug report,in the mean time I'll address a few particulars: * I've been using git from regular CMD with Git's bin/ in my path. Not Git Bash. * I created a new repo by cloning, giving an absolute path for the working tree location. This is the line from the script I used: git clone --separate-git-dir c:\dev\git\%branch% -o MAIN c:\core\guidewire\MAIN c:\core\guidewire\%branch% ** Yes, the working tree and gitdir are in separate locations. (I don't think this would have made a difference.) * I verified the case-sensitivity problem by comparing cd-ing (in regular CMD) to absolute paths with upper- and lower-case drive letters. Starting with 'C' made the rev-parse predicates give the expected answers. It was not my intention to drop a bunch of work on you or anyone; I'm sorry it sounded that way! I started out with an email here on the advice of https://git-scm.com/community. Day-job permitting, I'll reinstall 1.6.2 and determine the minimum steps to demonstrate the problem as I originally observed it. When I've got reproducible test steps, should I open a bug on Github, or come back here? And finally, if you tell me outright this was all my own fault for typing the paths wrongly I won't be offended : ) -gws On Tue, Sep 1, 2015 at 1:30 PM, Johannes Schindelin wrote: > Hi Geofrey, > > On 2015-09-01 18:55, Geofrey Sanders wrote: >> I recently upgraded from Windows Git 1.6.2 to 2.5.0 and found myself >> unable to rebase. Turns out paths didn't used to be case-sensitive and >> now they are, causing a number of operations to halt. A repo created >> by pointing at the directory >> c:\core\guidewire\Dev\2.4 >> would (I suppose) technically have been invalid the whole time because >> Windows reports the current path as >> C:\core\guidewire\Dev\2.4 >> , but msys Git 1.6.2 evidently made a case-insensitive path comparison >> so the discrepancy was suppressed. > > Are you sure about that? I seem to recall that `pwd` changed behavior between > MSys and MSys2, but Git never made case-insensitive comparisons. > > It might help me to understand what is going on if I could have preciser > information. What exactly do you mean by "A repo created by pointing at ..."? > Could you type out the Git commands you used? > >> The proximate cause of errors was >> git rev-parse --is-inside-work-tree >> which would output 'false' even inside the working tree. > > Ah, you are apparently talking about a worktree separate from your repository? > >> "--is-inside-git-dir" also printed 'false' in directories where it >> should have said 'true'. > > Again, I really need preciser information about this: *How* did you end up in > that directory? Did you use Git Bash or Git CMD? Did you call `cd` with a > relative path, a POSIX path or a POSIX-ified full DOS path? > >> I actually missed the problem in plain sight >> at first, because I created a new repo (in which everything worked as >> expected), and then did a directory diff... the worktree paths were >> different but I only noticed the names, not the case difference in the >> drive letter. More details in this SO question: >> http://stackoverflow.com/q/32280644/2835086 > > Please understand that I have a lot of tickets to juggle about and that it is > a bit unfair to send me onto a goose chase. I would have preferred a proper > GitHub issue, as the "Contribute" section of > https://git-for-windows.github.io/ explicitly asks for, but I am okay with > discussing this ticket on the mailing list. But studying a StackOverflow > thread in addition is a bit much... next, people would ask me to search their > Twitter feed for the little tid bit of information I need to help. > > So please summarize that StackOverflow question, and while we are at it: > StackOverflow suggests coming up with a Minimal, Complete and Verifiable > Example. That would be a nice thing to have. Maybe you find it in you to come > up with that MCVE. > >> I was able to repair my existing repos by changing the 'worktree' >> value in gitconfig - s/c/C/ did the trick - but the whole thing was a >> surprise. >> >> Is this a bug in the current version? Windows doesn't distinguish on >> case, so maybe applications shouldn't either. >> Was this a bug in the prior version? Maybe creating a repo with a >> worktree path that doesn't match the file system should have been an >> error from the very beginning. >> Was this user error? Maybe I did something wrong and should have known >> better, but got away with it for a while. > > I think there is a good chance we can fix this, although a 1.x -> 2.x jump > always suggests that certain things change in a backwards-incompatible manner. > > Looking forward to more detailed information and that MCVE, > Johannes -- 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
Re: [PATCHv4 3/3] submodule: implement `clone` as a builtin helper
On Tue, Sep 01, 2015 at 02:52:54PM -0400, Eric Sunshine wrote:
> > + /* Redirect the worktree of the submodule in the superproject's
> > config */
> > + if (strbuf_getcwd(&sb))
> > + die_errno(_("unable to get current working directory"));
> > +
> > + if (!is_absolute_path(sm_gitdir)) {
> > + if (strbuf_getcwd(&sb))
> > + die_errno(_("unable to get current working
> > directory"));
>
> Why does this need to call getcwd() on 'sb' when it was called
> immediately above the conditional and its value hasn't changed?
Even weirder, the next code is:
>
> > + strbuf_addf(&sb, "/%s", sm_gitdir);
> > + free(sm_gitdir);
> > + sm_gitdir = strbuf_detach(&sb, NULL);
> > + }
> > +
> > +
> > + strbuf_addf(&sb, "/%s", path);
So if it _was_ an absolute path, we are adding "/$path" to nothing
(having just detached the strbuf in the conditional above). That seems
weird.
-Peff
--
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
Re: [PATCHv4 3/3] submodule: implement `clone` as a builtin helper
Eric Sunshine writes: > On Tue, Sep 1, 2015 at 2:24 PM, Stefan Beller wrote: >> This converts implements the helper `module_clone`. This functionality is > > Mentioned previously[1]: "converts implements"? > > [1]: http://article.gmane.org/gmane.comp.version-control.git/276966 Stefan, perhaps another round of proofreading before sending would have helped. I am neutral with asterisk around const myself. I think I saw this often spelled with SP on both sides, i.e. $ git grep ' \* const ' \*.c | wc -l 121 $ git grep ' \*const ' \*.c | wc -l 45 Majority of the latter, when viewed without pipe to wc, tells me they are mostly borrowed code. All the other comments from you on this patch makes sense to me. Thanks. -- 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
Re: [PATCHv4 3/3] submodule: implement `clone` as a builtin helper
On Tue, Sep 1, 2015 at 3:05 PM, Jeff King wrote:
> On Tue, Sep 01, 2015 at 02:52:54PM -0400, Eric Sunshine wrote:
>> > + /* Redirect the worktree of the submodule in the superproject's
>> > config */
>> > + if (strbuf_getcwd(&sb))
>> > + die_errno(_("unable to get current working directory"));
>> > +
>> > + if (!is_absolute_path(sm_gitdir)) {
>> > + if (strbuf_getcwd(&sb))
>> > + die_errno(_("unable to get current working
>> > directory"));
>>
>> Why does this need to call getcwd() on 'sb' when it was called
>> immediately above the conditional and its value hasn't changed?
>
> Even weirder, the next code is:
>
>> > + strbuf_addf(&sb, "/%s", sm_gitdir);
>> > + free(sm_gitdir);
>> > + sm_gitdir = strbuf_detach(&sb, NULL);
>> > + }
>> > +
>> > +
>> > + strbuf_addf(&sb, "/%s", path);
>
> So if it _was_ an absolute path, we are adding "/$path" to nothing
> (having just detached the strbuf in the conditional above). That seems
> weird.
Indeed, I saw that too, but didn't mention it since this appears to be
an incomplete refactoring from the previous round, and my hope was
that mentioning the getcwd() oddity would be enough to trigger a
thorough check of the code before sending the next version.
--
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
Re: [PATCHv4 1/3] submodule: implement `list` as a builtin helper
Stefan Beller writes:
> +static int module_list_compute(int argc, const char **argv,
> + const char *prefix,
> + struct pathspec *pathspec)
> +{
> + int i, result = 0;
> + char *max_prefix, *ps_matched = NULL;
> + int max_prefix_len;
> + parse_pathspec(pathspec, 0,
> +PATHSPEC_PREFER_FULL |
> +PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP,
> +prefix, argv);
> +
> + /* Find common prefix for all pathspec's */
> + max_prefix = common_prefix(pathspec);
> + max_prefix_len = max_prefix ? strlen(max_prefix) : 0;
> +
> + if (pathspec->nr)
> + ps_matched = xcalloc(pathspec->nr, 1);
> +
> + if (read_cache() < 0)
> + die(_("index file corrupt"));
> +
> + for (i = 0; i < active_nr; i++) {
> + const struct cache_entry *ce = active_cache[i];
> +
> + if (!match_pathspec(pathspec, ce->name, ce_namelen(ce),
> + max_prefix_len, ps_matched,
> + S_ISGITLINK(ce->ce_mode) |
> S_ISDIR(ce->ce_mode)))
> + continue;
> +
> + if (S_ISGITLINK(ce->ce_mode)) {
> + ALLOC_GROW(ce_entries, ce_used + 1, ce_alloc);
> + ce_entries[ce_used++] = ce;
> + }
> +
> + while (i + 1 < active_nr && !strcmp(ce->name, active_cache[i +
> 1]->name))
> + /*
> + * Skip entries with the same name in different stages
> + * to make sure an entry is returned only once.
> + */
> + i++;
> + }
I hate myself not catching this earlier, but I suspect that this is
not quite a faithful rewrite of the original. It changes behaviour
when a conflicted path records a non submodule in stage #1 and a
submodule in stage #2 (or stage #3), doesn't it?
The original scripted version will see the stage #1 entry and skips
it because it is not a submodule, then will see the stage #2 entry
and because the path is not yet in the %unmerged hash, and it will
push it to @out.
This loop instead sees a non-submodule entry at stage #1, skips it
(because it is not a submodule), then goes on to skip the entries
with the same name, including the stage #2 entry that _is_ a
submodule.
I think the clever "we are done with this entry, so let's skip all
others that have the same name" optimization should be done only
when we did handle an entry with the same name. I.e. something like
this squashed in.
--
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 16d9abe..c4aff0c 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -39,12 +39,13 @@ static int module_list_compute(int argc, const char **argv,
S_ISGITLINK(ce->ce_mode) |
S_ISDIR(ce->ce_mode)))
continue;
- if (S_ISGITLINK(ce->ce_mode)) {
- ALLOC_GROW(ce_entries, ce_used + 1, ce_alloc);
- ce_entries[ce_used++] = ce;
- }
+ if (!S_ISGITLINK(ce->ce_mode))
+ continue;
- while (i + 1 < active_nr && !strcmp(ce->name, active_cache[i +
1]->name))
+ ALLOC_GROW(ce_entries, ce_used + 1, ce_alloc);
+ ce_entries[ce_used++] = ce;
+ while (i + 1 < active_nr &&
+ !strcmp(ce->name, active_cache[i + 1]->name))
/*
* Skip entries with the same name in different stages
* to make sure an entry is returned only once.
--
> +static int module_list(int argc, const char **argv, const char *prefix)
> +{
> + int i;
> + struct pathspec pathspec;
> + const char *alternative_path;
> +
> + struct option module_list_options[] = {
> + OPT_STRING(0, "prefix", &alternative_path,
> +N_("path"),
> +N_("alternative anchor for relative paths")),
> + OPT_END()
> + };
Do we really need this alternative_path variable? The "--prefix"
option that overrides the passed-in variable prefix would be easier
to understand if it touched the same variable, i.e.
--
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 16d9abe..387539f 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -65,10 +66,9 @@ static int module_list(int argc, const char **argv, const
char *prefix)
{
int i;
struct pathspec pathspec;
- const char *alternative_path;
struct option module_list_options[] = {
- OPT_STRING(0, "prefix", &alternative_path,
+
[PATCH 0/2] clarify sideband muxing in GIT_TRACE_PACKET
I happened to be debugging push with GIT_TRACE_PACKET today, and got confused by the mixture of trace from the sideband demuxer and push itself (details in patch 2/2). This series fixes it by showing a distinct prefix for the sideband reads. There's some trickery with start_async() involved, so I've tested it both with and without NO_PTHREADS set. [1/2]: run-command: provide in_async query function [2/2]: pkt-line: show packets in async processes as "sideband" -Peff -- 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
[PATCH 1/2] run-command: provide in_async query function
It's not easy for arbitrary code to find out whether it is
running in an async process or not. A top-level function
which is fed to start_async() can know (you just pass down
an argument saying "you are async"). But that function may
call other global functions, and we would not want to have
to pass the information all the way through the call stack.
Nor can we simply set a global variable, as those may be
shared between async threads and the main thread (if the
platform supports pthreads). We need pthread tricks _or_ a
global variable, depending on how start_async is
implemented.
The callers don't have enough information to do this right,
so let's provide a simple query function that does.
Fortunately we can reuse the existing infrastructure to make
the pthread case simple (and even simplify die_async() by
using our new function).
Signed-off-by: Jeff King
---
run-command.c | 16 +++-
run-command.h | 1 +
2 files changed, 16 insertions(+), 1 deletion(-)
diff --git a/run-command.c b/run-command.c
index 3277cf7..c8029f2 100644
--- a/run-command.c
+++ b/run-command.c
@@ -595,7 +595,7 @@ static NORETURN void die_async(const char *err, va_list
params)
{
vreportf("fatal: ", err, params);
- if (!pthread_equal(main_thread, pthread_self())) {
+ if (in_async()) {
struct async *async = pthread_getspecific(async_key);
if (async->proc_in >= 0)
close(async->proc_in);
@@ -614,6 +614,13 @@ static int async_die_is_recursing(void)
return ret != NULL;
}
+int in_async(void)
+{
+ if (!main_thread_set)
+ return 0; /* no asyncs started yet */
+ return !pthread_equal(main_thread, pthread_self());
+}
+
#else
static struct {
@@ -653,6 +660,12 @@ int git_atexit(void (*handler)(void))
}
#define atexit git_atexit
+static int process_is_async;
+int in_async(void)
+{
+ return process_is_async;
+}
+
#endif
int start_async(struct async *async)
@@ -712,6 +725,7 @@ int start_async(struct async *async)
if (need_out)
close(fdout[0]);
git_atexit_clear();
+ process_is_async = 1;
exit(!!async->proc(proc_in, proc_out, async->data));
}
diff --git a/run-command.h b/run-command.h
index 5b4425a..629fab7 100644
--- a/run-command.h
+++ b/run-command.h
@@ -118,5 +118,6 @@ struct async {
int start_async(struct async *async);
int finish_async(struct async *async);
+int in_async(void);
#endif
--
2.5.1.739.g7891f6b
--
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
[PATCH 2/2] pkt-line: show packets in async processes as "sideband"
If you run "GIT_TRACE_PACKET=1 git push", you may get
confusing output like (line prefixes omitted for clarity):
packet: push< \1000eunpack ok0019ok refs/heads/master
packet: push< unpack ok
packet: push< ok refs/heads/master
packet: push<
packet: push<
Why do we see the data twice, once apparently wrapped inside
another pkt-line, and once unwrapped? Why do we get two
flush packets?
The answer is that we start an async process to demux the
sideband data. The first entry comes from the sideband
process reading the data, and the second from push itself.
Likewise, the first flush is inside the demuxed packet, and
the second is an actual sideband flush.
We can make this a bit more clear by marking the sideband
demuxer explicitly as "sideband" rather than "push". The
most elegant way to do this would be to simply call
packet_trace_identity() inside the sideband demuxer. But we
can't do that reliably, because it relies on a global
variable, which might be shared if pthreads are in use.
What we really need is thread-local storage for
packet_trace_identity. But the async code does not provide
an interface for that, and it would be messy to add it here
(we'd have to care about pthreads, initializing our
pthread_key_t ahead of time, etc).
So instead, let us just assume that any async process is
handling sideband data. That's always true now, and is
likely to remain so in the future.
The output looks like:
packet: sideband< \1000eunpack ok0019ok refs/heads/master
packet: push< unpack ok
packet: push< ok refs/heads/master
packet: push<
packet: sideband<
Signed-off-by: Jeff King
---
pkt-line.c | 8 +++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/pkt-line.c b/pkt-line.c
index 08a1427..62fdb37 100644
--- a/pkt-line.c
+++ b/pkt-line.c
@@ -1,5 +1,6 @@
#include "cache.h"
#include "pkt-line.h"
+#include "run-command.h"
char packet_buffer[LARGE_PACKET_MAX];
static const char *packet_trace_prefix = "git";
@@ -11,6 +12,11 @@ void packet_trace_identity(const char *prog)
packet_trace_prefix = xstrdup(prog);
}
+static const char *get_trace_prefix(void)
+{
+ return in_async() ? "sideband" : packet_trace_prefix;
+}
+
static int packet_trace_pack(const char *buf, unsigned int len, int sideband)
{
if (!sideband) {
@@ -57,7 +63,7 @@ static void packet_trace(const char *buf, unsigned int len,
int write)
strbuf_init(&out, len+32);
strbuf_addf(&out, "packet: %12s%c ",
- packet_trace_prefix, write ? '>' : '<');
+ get_trace_prefix(), write ? '>' : '<');
/* XXX we should really handle printable utf8 */
for (i = 0; i < len; i++) {
--
2.5.1.739.g7891f6b
--
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
Re: [PATCH 1/2] run-command: provide in_async query function
On Tue, Sep 01, 2015 at 04:22:43PM -0400, Jeff King wrote:
> diff --git a/run-command.c b/run-command.c
> index 3277cf7..c8029f2 100644
> --- a/run-command.c
> +++ b/run-command.c
This diff is a little funny to read because the interesting context is
far away, and it is not immediately obvious why we are defining the
function twice. This:
> @@ -595,7 +595,7 @@ static NORETURN void die_async(const char *err, va_list
> params)
> {
> vreportf("fatal: ", err, params);
>
> - if (!pthread_equal(main_thread, pthread_self())) {
> + if (in_async()) {
> struct async *async = pthread_getspecific(async_key);
> if (async->proc_in >= 0)
> close(async->proc_in);
> @@ -614,6 +614,13 @@ static int async_die_is_recursing(void)
> return ret != NULL;
> }
>
> +int in_async(void)
> +{
> + if (!main_thread_set)
> + return 0; /* no asyncs started yet */
> + return !pthread_equal(main_thread, pthread_self());
> +}
> +
> #else
is all inside #ifndef NO_PTHREADS, and this:
> @@ -653,6 +660,12 @@ int git_atexit(void (*handler)(void))
> }
> #define atexit git_atexit
>
> +static int process_is_async;
> +int in_async(void)
> +{
> + return process_is_async;
> +}
> +
> #endif
is the "#else" case where we fork.
-Peff
--
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
Re: Git crash on different versions, including current
Hi Johannes, > If you copy the entire `GoogleTestExtension` folder somewhere else, does the crash happen there, still? Can you share a .zip of this folder? Ciao, Johannes I just unzipped the folder to C: and tried again, and it indeed still crashes. Which is probably good news, because it shouldn't be too hard for you guys to reproduce the issue :-) I will upload the zip file in a minute and drop you a private email - it's not exactly secret stuff, but I feel better this way. Feel free to share the zip with other git developers, but please don't make it publicly available somewhere. Thanks... Final note: My workaround was to move the gtest folder somewhere else, commit the outgoing single "gtest deleted" dir, move the dir back to its original place, add all (which worked fine) and finally committed. All the best, Christian -- H.L. Mencken: "Say what you will about the Ten Commandments, you must always come back to the pleasant fact that there are only ten of them." -- 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
Re: [PATCH v15 05/13] ref-filter: implement an `align` atom
Karthik Nayak writes:
> We have an `at_end` function for each element of the stack which is to
> be called when the `end` atom is encountered. Using this we implement
> the aling_handler() for the `align` atom, this aligns the final strbuf
align_handler().
> struct ref_formatting_stack {
> struct ref_formatting_stack *prev;
> struct strbuf output;
> + void (*at_end)(struct ref_formatting_stack *stack);
> + void *cb_data;
> };
s/cb_data/at_end_data/ or something, as this is not really about a
function callback. Imagine a fictional future where you add a new
functions at_middle---the readers cannot tell what cb_data is about
at that point.
> +static void end_atom_handler(struct atom_value *atomv, struct
> ref_formatting_state *state)
> +{
> + struct ref_formatting_stack *current = state->stack;
> + struct strbuf s = STRBUF_INIT;
> +
> + if (!current->at_end)
> + die(_("format: `end` atom used without a supporting atom"));
Not a show-stopper, but we may need some wordsmithing for "a
supporting atom" here; an end-user would not know what it is.
> + current->at_end(current);
> +
> + /*
> + * Perform quote formatting when the stack element is that of
> + * a modifier atom and right above the first stack element.
> + */
> + if (!state->stack->prev->prev) {
> + quote_formatting(&s, current->output.buf, state->quote_style);
> + strbuf_swap(¤t->output, &s);
> + }
> + strbuf_release(&s);
> + pop_stack_element(&state->stack);
> +}
Nice.
> @@ -687,6 +748,7 @@ static void populate_value(struct ref_array_item *ref)
> int deref = 0;
> const char *refname;
> const char *formatp;
> + const char *valp;
> struct branch *branch = NULL;
>
> v->handler = append_atom;
> @@ -754,6 +816,42 @@ static void populate_value(struct ref_array_item *ref)
> else
> v->s = " ";
> continue;
> + } else if (skip_prefix(name, "align", &valp)) {
This looked as if you are willing to take %(align) in addition to
%(align:...), but...
> + struct align *align = &v->align;
> + struct strbuf **s;
> +
> + if (valp[0] != ':')
> + die(_("format: usage
> %%(align:,)"));
... apparently that is not what is happening. Why not skip "align:"
with colon as the prefix, then?
> + else
> + valp++;
> + s = strbuf_split_str(valp, ',', 0);
> +
> + /* If the position is given trim the ',' from the first
> strbuf */
> + if (s[1])
> + strbuf_setlen(s[0], s[0]->len - 1);
> +
> + if (strtoul_ui(s[0]->buf, 10, &align->width))
> + die(_("positive width expected align:%s"),
> s[0]->buf);
> +
> + if (!s[1])
> + align->position = ALIGN_LEFT;
> + else if (!strcmp(s[1]->buf, "left"))
> + align->position = ALIGN_LEFT;
> + else if (!strcmp(s[1]->buf, "right"))
> + align->position = ALIGN_RIGHT;
> + else if (!strcmp(s[1]->buf, "middle"))
> + align->position = ALIGN_MIDDLE;
> + else
> + die(_("improper format entered align:%s"),
> s[1]->buf);
This does not reject %(align:40,left,junk), no? Before "s[1] does
not exist so default to left align", you would want
if (s[2])
die("align:width,position followed by garbage: ,%s", s[2]->buf);
I have a few observations; these are not necessarily we would want
to change in the scope of this series, though.
- The design of strbuf_split_buf API feels screwy. A variant of
this function that strips the terminator at the end would be what
most callers would want. Granted, leaving the terminator in the
resulting buffer does let the caller tell if the input ended with
an incomplete line that lacked the final terminator, but for all
s[i] for 0 <= i < N-1 where s[N] is the first element that is
NULL, they must end with the terminator---otherwise the elements
would not have split into the array in the first place. "By
keeping the terminator, you can tell which one of the possible
terminators was used" could be a valid rationale for the API if
the function allowed more than one terminators, but that does not
apply here, either.
- I would have expected the above code to look more like this:
width = -1; position = ALIGN_LEFT;
s = strbuf_split_str(valp, ',', 0);
while (*s) {
if (s[1])
strbuf_setlen(*s, *s->len - 1);
if
Re: [PATCH v15 06/13] ref-filter: add option to filter out tags, branches and remotes
Karthik Nayak writes:
> + if (!filter->kind)
> die("filter_refs: invalid type");
> + else {
> + if (filter->kind == FILTER_REFS_BRANCHES)
> + ret = for_each_fullref_in("refs/heads/",
> ref_filter_handler, &ref_cbdata, broken);
> + else if (filter->kind == FILTER_REFS_REMOTES)
> + ret = for_each_fullref_in("refs/remotes/",
> ref_filter_handler, &ref_cbdata, broken);
> + else if (filter->kind == FILTER_REFS_TAGS)
> + ret = for_each_fullref_in("refs/tags/",
> ref_filter_handler, &ref_cbdata, broken);
> + else if (filter->kind & FILTER_REFS_ALL)
> + ret = for_each_fullref_in("", ref_filter_handler,
> &ref_cbdata, broken);
This if/else if/else if/ cascade and ...
> + if (!ret && (filter->kind & FILTER_REFS_DETACHED_HEAD))
> + head_ref(ref_filter_handler, &ref_cbdata);
> + }
> +
>
> /* Filters that need revision walking */
> if (filter->merge_commit)
> diff --git a/ref-filter.h b/ref-filter.h
> index 45026d0..0913ba9 100644
> --- a/ref-filter.h
> +++ b/ref-filter.h
> @@ -13,8 +13,15 @@
> #define QUOTE_PYTHON 4
> #define QUOTE_TCL 8
>
> -#define FILTER_REFS_INCLUDE_BROKEN 0x1
> -#define FILTER_REFS_ALL 0x2
> +#define FILTER_REFS_INCLUDE_BROKEN 0x0001
> +#define FILTER_REFS_TAGS 0x0002
> +#define FILTER_REFS_BRANCHES 0x0004
> +#define FILTER_REFS_REMOTES0x0008
> +#define FILTER_REFS_OTHERS 0x0010
... the bit assignment here conceptually do not mesh well with each
other. These bits look as if I can ask for both tags and branches
by passing 0x0006, and if the code above were
empty the result set;
if (filter->kind & FILTER_REFS_BRANCHES)
add in things from refs/heads/ to the result set;
if (filter->kind & FILTER_REFS_TAGS)
add in things from refs/tags/ to the result set;
...
without "else if", that would conceptually make sense.
Alternatively, if the code does not (and will not ever) support such
an arbitrary mixing of bits and intends to only allow "one of
BRANCHES, REMOTES, TAGS or ALL, and no other choice", then it is
wrong to pretend as if they can be mixed by defining the constant
with values with non-overlapping bit patterns. If you defined these
constants as
#define FILTER_REFS_TAGS 100
#define FILTER_REFS_BRANCHES 101
#define FILTER_REFS_REMOTES102
#define FILTER_REFS_OTHERS 103
then nobody would be think that the function can collect both tags
and branches by passing FILTER_REFS_TAGS|FILTER_REFS_BRANCHES.
The former, i.e. keep the bits distinct and allow them to be OR'ed
together by updating the code to allow such callers, would be more
preferrable, of course.
--
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
[PATCH v2 0/6] Make "local" orthogonal to date format
Jeff's first patch is unmodified but I've squashed the fixed currently on "pu" into the second. I also realised while adding the tests that "raw-local" is meaningless so I've modified the code to reject it in the same way as "relative-local". Jeff King (2): fast-import: switch crash-report date to iso8601 date: make "local" orthogonal to date format John Keeping (4): t6300: introduce test_date() helper t6300: make UTC and local dates different t6300: add test for "raw" date format t6300: add tests for "-local" date formats Documentation/rev-list-options.txt | 21 -- builtin/blame.c| 1 - cache.h| 2 +- date.c | 77 +--- fast-import.c | 2 +- t/t6300-for-each-ref.sh| 139 +++-- 6 files changed, 140 insertions(+), 102 deletions(-) -- 2.5.0.466.g9af26fa -- 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
[PATCH v2 1/6] fast-import: switch crash-report date to iso8601
From: Jeff King
When fast-import emits a crash report, it does so in the
user's local timezone. But because we omit the timezone
completely for DATE_LOCAL, a reader of the report does not
immediately know which time zone was used. Let's switch this
to ISO8601 instead, which includes the time zone.
This does mean we will show the time in UTC, but that's not
a big deal. A crash report like this will either be looked
at immediately (in which case nobody even looks at the
timestamp), or it will be passed along to a developer to
debug, in which case the original timezone is less likely to
be of interest.
Signed-off-by: Jeff King
Signed-off-by: John Keeping
---
fast-import.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fast-import.c b/fast-import.c
index 6c7c3c9..adcbfc6 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -424,7 +424,7 @@ static void write_crash_report(const char *err)
fprintf(rpt, "fast-import crash report:\n");
fprintf(rpt, "fast-import process: %"PRIuMAX"\n", (uintmax_t)
getpid());
fprintf(rpt, "parent process : %"PRIuMAX"\n", (uintmax_t)
getppid());
- fprintf(rpt, "at %s\n", show_date(time(NULL), 0, DATE_MODE(LOCAL)));
+ fprintf(rpt, "at %s\n", show_date(time(NULL), 0,
DATE_MODE(ISO8601)));
fputc('\n', rpt);
fputs("fatal: ", rpt);
--
2.5.0.466.g9af26fa
--
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
[PATCH v2 2/6] date: make "local" orthogonal to date format
From: Jeff King
Most of our "--date" modes are about the format of the date:
which items we show and in what order. But "--date=local" is
a bit of an oddball. It means "show the date in the normal
format, but using the local timezone". The timezone we use
is orthogonal to the actual format, and there is no reason
we could not have "localized iso8601", etc.
This patch adds a "local" boolean field to "struct
date_mode", and drops the DATE_LOCAL element from the
date_mode_type enum (it's now just DATE_NORMAL plus
local=1). The new feature is accessible to users by adding
"-local" to any date mode (e.g., "iso-local"), and we retain
"local" as an alias for "default-local" for backwards
compatibility.
Signed-off-by: Jeff King
Signed-off-by: John Keeping
---
This is Jeff's original patch with my fixup for DATE_STRFTIME squashed
in and a new change to reject "raw-local" (in both Documentation/ and
date.c).
Documentation/rev-list-options.txt | 21 ---
builtin/blame.c| 1 -
cache.h| 2 +-
date.c | 77 +-
4 files changed, 67 insertions(+), 34 deletions(-)
diff --git a/Documentation/rev-list-options.txt
b/Documentation/rev-list-options.txt
index a9b808f..5d28133 100644
--- a/Documentation/rev-list-options.txt
+++ b/Documentation/rev-list-options.txt
@@ -702,12 +702,16 @@ include::pretty-options.txt[]
--date=(relative|local|default|iso|iso-strict|rfc|short|raw)::
Only takes effect for dates shown in human-readable format, such
as when using `--pretty`. `log.date` config variable sets a default
- value for the log command's `--date` option.
+ value for the log command's `--date` option. By default, dates
+ are shown in the original time zone (either committer's or
+ author's). If `-local` is appended to the format (e.g.,
+ `iso-local`), the user's local time zone is used instead.
+
`--date=relative` shows dates relative to the current time,
-e.g. ``2 hours ago''.
+e.g. ``2 hours ago''. The `-local` option cannot be used with
+`--raw` or `--relative`.
+
-`--date=local` shows timestamps in user's local time zone.
+`--date=local` is an alias for `--date=default-local`.
+
`--date=iso` (or `--date=iso8601`) shows timestamps in a ISO 8601-like format.
The differences to the strict ISO 8601 format are:
@@ -730,10 +734,15 @@ format, often found in email messages.
`--date=format:...` feeds the format `...` to your system `strftime`.
Use `--date=format:%c` to show the date in your system locale's
preferred format. See the `strftime` manual for a complete list of
-format placeholders.
+format placeholders. When using `-local`, the correct syntax is
+`--date=format-local:...`.
+
-`--date=default` shows timestamps in the original time zone
-(either committer's or author's).
+`--date=default` is the default format, and is similar to
+`--date=rfc2822`, with a few exceptions:
+
+ - there is no comma after the day-of-week
+
+ - the time zone is omitted when the local time zone is used
ifdef::git-rev-list[]
--header::
diff --git a/builtin/blame.c b/builtin/blame.c
index 4db01c1..6fd1a63 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -2600,7 +2600,6 @@ parse_done:
fewer display columns. */
blame_date_width = utf8_strwidth(_("4 years, 11 months ago")) +
1; /* add the null */
break;
- case DATE_LOCAL:
case DATE_NORMAL:
blame_date_width = sizeof("Thu Oct 19 16:00:04 2006 -0700");
break;
diff --git a/cache.h b/cache.h
index 4e25271..9a91b1d 100644
--- a/cache.h
+++ b/cache.h
@@ -1091,7 +1091,6 @@ struct date_mode {
DATE_NORMAL = 0,
DATE_RELATIVE,
DATE_SHORT,
- DATE_LOCAL,
DATE_ISO8601,
DATE_ISO8601_STRICT,
DATE_RFC2822,
@@ -1099,6 +1098,7 @@ struct date_mode {
DATE_RAW
} type;
const char *strftime_fmt;
+ int local;
};
/*
diff --git a/date.c b/date.c
index 8f91569..f048416 100644
--- a/date.c
+++ b/date.c
@@ -166,6 +166,7 @@ struct date_mode *date_mode_from_type(enum date_mode_type
type)
if (type == DATE_STRFTIME)
die("BUG: cannot create anonymous strftime date_mode struct");
mode.type = type;
+ mode.local = 0;
return &mode;
}
@@ -189,7 +190,7 @@ const char *show_date(unsigned long time, int tz, const
struct date_mode *mode)
return timebuf.buf;
}
- if (mode->type == DATE_LOCAL)
+ if (mode->local)
tz = local_tzoffset(time);
tm = time_to_tm(time, tz);
@@ -232,7 +233,7 @@ const char *show_date(unsigned long time, int tz, const
struct date_mode *mode)
tm->tm_mday,
tm->tm_hour, tm->tm_min, tm->tm_sec,
[PATCH v2 3/6] t6300: introduce test_date() helper
This moves the setup of the "expected" file inside the test case. The
helper function has the advantage that we can use SQ in the file content
without needing to escape the quotes.
Signed-off-by: John Keeping
---
I considered moving the test_expect_success into the helper, like with
test_atom earlier in the file, but it doesn't make the code much more
concise and we still need either to setup the output outside the test
case or to escape SQ inside SQ.
t/t6300-for-each-ref.sh | 73 ++---
1 file changed, 21 insertions(+), 52 deletions(-)
diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index 7c9bec7..5fdb964 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -146,85 +146,54 @@ test_expect_success 'Check invalid format specifiers are
errors' '
test_must_fail git for-each-ref --format="%(authordate:INVALID)"
refs/heads
'
-cat >expected <<\EOF
-'refs/heads/master' 'Mon Jul 3 17:18:43 2006 +0200' 'Mon Jul 3 17:18:44 2006
+0200'
-'refs/tags/testtag' 'Mon Jul 3 17:18:45 2006 +0200'
-EOF
+test_date () {
+ f=$1
+ committer_date=$2 &&
+ author_date=$3 &&
+ tagger_date=$4 &&
+ cat >expected <<-EOF &&
+ 'refs/heads/master' '$committer_date' '$author_date'
+ 'refs/tags/testtag' '$tagger_date'
+ EOF
+ (
+ git for-each-ref --shell --format="%(refname)
%(committerdate${f:+:$f}) %(authordate${f:+:$f})" refs/heads &&
+ git for-each-ref --shell --format="%(refname)
%(taggerdate${f:+:$f})" refs/tags
+ ) >actual &&
+ test_cmp expected actual
+}
test_expect_success 'Check unformatted date fields output' '
- (git for-each-ref --shell --format="%(refname) %(committerdate)
%(authordate)" refs/heads &&
- git for-each-ref --shell --format="%(refname) %(taggerdate)" refs/tags)
>actual &&
- test_cmp expected actual
+ test_date "" "Mon Jul 3 17:18:43 2006 +0200" "Mon Jul 3 17:18:44 2006
+0200" "Mon Jul 3 17:18:45 2006 +0200"
'
test_expect_success 'Check format "default" formatted date fields output' '
- f=default &&
- (git for-each-ref --shell --format="%(refname) %(committerdate:$f)
%(authordate:$f)" refs/heads &&
- git for-each-ref --shell --format="%(refname) %(taggerdate:$f)"
refs/tags) >actual &&
- test_cmp expected actual
+ test_date default "Mon Jul 3 17:18:43 2006 +0200" "Mon Jul 3 17:18:44
2006 +0200" "Mon Jul 3 17:18:45 2006 +0200"
'
# Don't know how to do relative check because I can't know when this script
# is going to be run and can't fake the current time to git, and hence can't
# provide expected output. Instead, I'll just make sure that "relative"
# doesn't exit in error
-#
-#cat >expected <<\EOF
-#
-#EOF
-#
test_expect_success 'Check format "relative" date fields output' '
f=relative &&
(git for-each-ref --shell --format="%(refname) %(committerdate:$f)
%(authordate:$f)" refs/heads &&
git for-each-ref --shell --format="%(refname) %(taggerdate:$f)"
refs/tags) >actual
'
-cat >expected <<\EOF
-'refs/heads/master' '2006-07-03' '2006-07-03'
-'refs/tags/testtag' '2006-07-03'
-EOF
-
test_expect_success 'Check format "short" date fields output' '
- f=short &&
- (git for-each-ref --shell --format="%(refname) %(committerdate:$f)
%(authordate:$f)" refs/heads &&
- git for-each-ref --shell --format="%(refname) %(taggerdate:$f)"
refs/tags) >actual &&
- test_cmp expected actual
+ test_date short 2006-07-03 2006-07-03 2006-07-03
'
-cat >expected <<\EOF
-'refs/heads/master' 'Mon Jul 3 15:18:43 2006' 'Mon Jul 3 15:18:44 2006'
-'refs/tags/testtag' 'Mon Jul 3 15:18:45 2006'
-EOF
-
test_expect_success 'Check format "local" date fields output' '
- f=local &&
- (git for-each-ref --shell --format="%(refname) %(committerdate:$f)
%(authordate:$f)" refs/heads &&
- git for-each-ref --shell --format="%(refname) %(taggerdate:$f)"
refs/tags) >actual &&
- test_cmp expected actual
+ test_date local "Mon Jul 3 15:18:43 2006" "Mon Jul 3 15:18:44 2006"
"Mon Jul 3 15:18:45 2006"
'
-cat >expected <<\EOF
-'refs/heads/master' '2006-07-03 17:18:43 +0200' '2006-07-03 17:18:44 +0200'
-'refs/tags/testtag' '2006-07-03 17:18:45 +0200'
-EOF
-
test_expect_success 'Check format "iso8601" date fields output' '
- f=iso8601 &&
- (git for-each-ref --shell --format="%(refname) %(committerdate:$f)
%(authordate:$f)" refs/heads &&
- git for-each-ref --shell --format="%(refname) %(taggerdate:$f)"
refs/tags) >actual &&
- test_cmp expected actual
+ test_date iso8601 "2006-07-03 17:18:43 +0200" "2006-07-03 17:18:44
+0200" "2006-07-03 17:18:45 +0200"
'
-cat >expected <<\EOF
-'refs/heads/master' 'Mon, 3 Jul 2006 17:18:43 +0200' 'Mon, 3 Jul 2006 17:18:44
+0200'
-'refs/tags/testtag' 'Mon, 3 Jul 2006 17:18:45 +0200'
-EOF
-
test_expect_success 'Check format "rfc2822" date fields outp
[PATCH v2 5/6] t6300: add test for "raw" date format
Signed-off-by: John Keeping --- t/t6300-for-each-ref.sh | 4 1 file changed, 4 insertions(+) diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh index 9e0096f..2e76ca9 100755 --- a/t/t6300-for-each-ref.sh +++ b/t/t6300-for-each-ref.sh @@ -196,6 +196,10 @@ test_expect_success 'Check format "rfc2822" date fields output' ' test_date rfc2822 "Tue, 4 Jul 2006 01:18:43 +0200" "Tue, 4 Jul 2006 01:18:44 +0200" "Tue, 4 Jul 2006 01:18:45 +0200" ' +test_expect_success 'Check format "raw" date fields output' ' + test_date raw "1151968723 +0200" "1151968724 +0200" "1151968725 +0200" +' + test_expect_success 'Check format of strftime date fields' ' echo "my date is 2006-07-04" >expected && git for-each-ref \ -- 2.5.0.466.g9af26fa -- 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
[PATCH v2 4/6] t6300: make UTC and local dates different
By setting the UTC time to 23:18:43 the date in +0200 is the following
day, 2006-07-04. This will ensure that the test for "short-local" to be
added in a following patch tests for different output from the "short"
format.
Signed-off-by: John Keeping
---
t/t6300-for-each-ref.sh | 48
1 file changed, 24 insertions(+), 24 deletions(-)
diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index 5fdb964..9e0096f 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -8,8 +8,8 @@ test_description='for-each-ref test'
. ./test-lib.sh
. "$TEST_DIRECTORY"/lib-gpg.sh
-# Mon Jul 3 15:18:43 2006 +
-datestamp=1151939923
+# Mon Jul 3 23:18:43 2006 +
+datestamp=1151968723
setdate_and_increment () {
GIT_COMMITTER_DATE="$datestamp +0200"
datestamp=$(expr "$datestamp" + 1)
@@ -61,21 +61,21 @@ test_atom head object ''
test_atom head type ''
test_atom head '*objectname' ''
test_atom head '*objecttype' ''
-test_atom head author 'A U Thor 1151939924 +0200'
+test_atom head author 'A U Thor 1151968724 +0200'
test_atom head authorname 'A U Thor'
test_atom head authoremail ''
-test_atom head authordate 'Mon Jul 3 17:18:44 2006 +0200'
-test_atom head committer 'C O Mitter 1151939923 +0200'
+test_atom head authordate 'Tue Jul 4 01:18:44 2006 +0200'
+test_atom head committer 'C O Mitter 1151968723 +0200'
test_atom head committername 'C O Mitter'
test_atom head committeremail ''
-test_atom head committerdate 'Mon Jul 3 17:18:43 2006 +0200'
+test_atom head committerdate 'Tue Jul 4 01:18:43 2006 +0200'
test_atom head tag ''
test_atom head tagger ''
test_atom head taggername ''
test_atom head taggeremail ''
test_atom head taggerdate ''
-test_atom head creator 'C O Mitter 1151939923 +0200'
-test_atom head creatordate 'Mon Jul 3 17:18:43 2006 +0200'
+test_atom head creator 'C O Mitter 1151968723 +0200'
+test_atom head creatordate 'Tue Jul 4 01:18:43 2006 +0200'
test_atom head subject 'Initial'
test_atom head contents:subject 'Initial'
test_atom head body ''
@@ -96,7 +96,7 @@ test_atom tag parent ''
test_atom tag numparent ''
test_atom tag object $(git rev-parse refs/tags/testtag^0)
test_atom tag type 'commit'
-test_atom tag '*objectname' '67a36f10722846e891fbada1ba48ed035de75581'
+test_atom tag '*objectname' 'ea122842f48be4afb2d1fc6a4b96c05885ab7463'
test_atom tag '*objecttype' 'commit'
test_atom tag author ''
test_atom tag authorname ''
@@ -107,18 +107,18 @@ test_atom tag committername ''
test_atom tag committeremail ''
test_atom tag committerdate ''
test_atom tag tag 'testtag'
-test_atom tag tagger 'C O Mitter 1151939925 +0200'
+test_atom tag tagger 'C O Mitter 1151968725 +0200'
test_atom tag taggername 'C O Mitter'
test_atom tag taggeremail ''
-test_atom tag taggerdate 'Mon Jul 3 17:18:45 2006 +0200'
-test_atom tag creator 'C O Mitter 1151939925 +0200'
-test_atom tag creatordate 'Mon Jul 3 17:18:45 2006 +0200'
-test_atom tag subject 'Tagging at 1151939927'
-test_atom tag contents:subject 'Tagging at 1151939927'
+test_atom tag taggerdate 'Tue Jul 4 01:18:45 2006 +0200'
+test_atom tag creator 'C O Mitter 1151968725 +0200'
+test_atom tag creatordate 'Tue Jul 4 01:18:45 2006 +0200'
+test_atom tag subject 'Tagging at 1151968727'
+test_atom tag contents:subject 'Tagging at 1151968727'
test_atom tag body ''
test_atom tag contents:body ''
test_atom tag contents:signature ''
-test_atom tag contents 'Tagging at 1151939927
+test_atom tag contents 'Tagging at 1151968727
'
test_atom tag HEAD ' '
@@ -163,11 +163,11 @@ test_date () {
}
test_expect_success 'Check unformatted date fields output' '
- test_date "" "Mon Jul 3 17:18:43 2006 +0200" "Mon Jul 3 17:18:44 2006
+0200" "Mon Jul 3 17:18:45 2006 +0200"
+ test_date "" "Tue Jul 4 01:18:43 2006 +0200" "Tue Jul 4 01:18:44 2006
+0200" "Tue Jul 4 01:18:45 2006 +0200"
'
test_expect_success 'Check format "default" formatted date fields output' '
- test_date default "Mon Jul 3 17:18:43 2006 +0200" "Mon Jul 3 17:18:44
2006 +0200" "Mon Jul 3 17:18:45 2006 +0200"
+ test_date default "Tue Jul 4 01:18:43 2006 +0200" "Tue Jul 4 01:18:44
2006 +0200" "Tue Jul 4 01:18:45 2006 +0200"
'
# Don't know how to do relative check because I can't know when this script
@@ -181,23 +181,23 @@ test_expect_success 'Check format "relative" date fields
output' '
'
test_expect_success 'Check format "short" date fields output' '
- test_date short 2006-07-03 2006-07-03 2006-07-03
+ test_date short 2006-07-04 2006-07-04 2006-07-04
'
test_expect_success 'Check format "local" date fields output' '
- test_date local "Mon Jul 3 15:18:43 2006" "Mon Jul 3 15:18:44 2006"
"Mon Jul 3 15:18:45 2006"
+ test_date local "Mon Jul 3 23:18:43 2006" "Mon Jul 3 23:18:44 2006"
"Mon Jul 3 23:18:45 2006"
'
test_expect_success 'Check format "iso8601" date fields output' '
- test_date iso8601 "2006-07-03 17:18:43 +0200" "2006-07-03 17:1
[PATCH v2 6/6] t6300: add tests for "-local" date formats
Signed-off-by: John Keeping --- t/t6300-for-each-ref.sh | 32 1 file changed, 32 insertions(+) diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh index 2e76ca9..c3aee70 100755 --- a/t/t6300-for-each-ref.sh +++ b/t/t6300-for-each-ref.sh @@ -170,6 +170,10 @@ test_expect_success 'Check format "default" formatted date fields output' ' test_date default "Tue Jul 4 01:18:43 2006 +0200" "Tue Jul 4 01:18:44 2006 +0200" "Tue Jul 4 01:18:45 2006 +0200" ' +test_expect_success 'Check format "default-local" date fields output' ' + test_date default-local "Mon Jul 3 23:18:43 2006" "Mon Jul 3 23:18:44 2006" "Mon Jul 3 23:18:45 2006" +' + # Don't know how to do relative check because I can't know when this script # is going to be run and can't fake the current time to git, and hence can't # provide expected output. Instead, I'll just make sure that "relative" @@ -180,10 +184,18 @@ test_expect_success 'Check format "relative" date fields output' ' git for-each-ref --shell --format="%(refname) %(taggerdate:$f)" refs/tags) >actual ' +test_expect_success 'Format "relative-local" is not allowed' ' + test_must_fail git for-each-ref --format="%(authordate:relative-local)" refs/heads +' + test_expect_success 'Check format "short" date fields output' ' test_date short 2006-07-04 2006-07-04 2006-07-04 ' +test_expect_success 'Check format "short-local" date fields output' ' + test_date short-local 2006-07-03 2006-07-03 2006-07-03 +' + test_expect_success 'Check format "local" date fields output' ' test_date local "Mon Jul 3 23:18:43 2006" "Mon Jul 3 23:18:44 2006" "Mon Jul 3 23:18:45 2006" ' @@ -192,14 +204,26 @@ test_expect_success 'Check format "iso8601" date fields output' ' test_date iso8601 "2006-07-04 01:18:43 +0200" "2006-07-04 01:18:44 +0200" "2006-07-04 01:18:45 +0200" ' +test_expect_success 'Check format "iso8601-local" date fields output' ' + test_date iso8601-local "2006-07-03 23:18:43 +" "2006-07-03 23:18:44 +" "2006-07-03 23:18:45 +" +' + test_expect_success 'Check format "rfc2822" date fields output' ' test_date rfc2822 "Tue, 4 Jul 2006 01:18:43 +0200" "Tue, 4 Jul 2006 01:18:44 +0200" "Tue, 4 Jul 2006 01:18:45 +0200" ' +test_expect_success 'Check format "rfc2822-local" date fields output' ' + test_date rfc2822-local "Mon, 3 Jul 2006 23:18:43 +" "Mon, 3 Jul 2006 23:18:44 +" "Mon, 3 Jul 2006 23:18:45 +" +' + test_expect_success 'Check format "raw" date fields output' ' test_date raw "1151968723 +0200" "1151968724 +0200" "1151968725 +0200" ' +test_expect_success 'Format "raw-local" is not allowed' ' + test_must_fail git for-each-ref --format="%(authordate:raw-local)" refs/heads +' + test_expect_success 'Check format of strftime date fields' ' echo "my date is 2006-07-04" >expected && git for-each-ref \ @@ -208,6 +232,14 @@ test_expect_success 'Check format of strftime date fields' ' test_cmp expected actual ' +test_expect_success 'Check format of strftime-local date fields' ' + echo "my date is 2006-07-03" >expected && + git for-each-ref \ + --format="%(authordate:format-local:my date is %Y-%m-%d)" \ + refs/heads >actual && + test_cmp expected actual +' + test_expect_success 'exercise strftime with odd fields' ' echo >expected && git for-each-ref --format="%(authordate:format:)" refs/heads >actual && -- 2.5.0.466.g9af26fa -- 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
Re: [PATCH 1/2] run-command: provide in_async query function
Jeff King writes:
> It's not easy for arbitrary code to find out whether it is
> running in an async process or not. A top-level function
> which is fed to start_async() can know (you just pass down
> an argument saying "you are async"). But that function may
> call other global functions, and we would not want to have
> to pass the information all the way through the call stack.
>
> Nor can we simply set a global variable, as those may be
> shared between async threads and the main thread (if the
> platform supports pthreads). We need pthread tricks _or_ a
> global variable, depending on how start_async is
> implemented.
>
> The callers don't have enough information to do this right,
> so let's provide a simple query function that does.
> Fortunately we can reuse the existing infrastructure to make
> the pthread case simple (and even simplify die_async() by
> using our new function).
>
> Signed-off-by: Jeff King
> ---
What is not immediately obvious from the above description is why a
code may want to care if it is in_async() in the first place.
If there weren't the die_async() update, the readers might have been
left utterly baffled (or they can somehow see this is related to
2/2) but it is a bit hard to arrange in "git log" as going to child
is harder.
The patch looks good. Thanks.
> run-command.c | 16 +++-
> run-command.h | 1 +
> 2 files changed, 16 insertions(+), 1 deletion(-)
>
> diff --git a/run-command.c b/run-command.c
> index 3277cf7..c8029f2 100644
> --- a/run-command.c
> +++ b/run-command.c
> @@ -595,7 +595,7 @@ static NORETURN void die_async(const char *err, va_list
> params)
> {
> vreportf("fatal: ", err, params);
>
> - if (!pthread_equal(main_thread, pthread_self())) {
> + if (in_async()) {
> struct async *async = pthread_getspecific(async_key);
> if (async->proc_in >= 0)
> close(async->proc_in);
> @@ -614,6 +614,13 @@ static int async_die_is_recursing(void)
> return ret != NULL;
> }
>
> +int in_async(void)
> +{
> + if (!main_thread_set)
> + return 0; /* no asyncs started yet */
> + return !pthread_equal(main_thread, pthread_self());
> +}
> +
> #else
>
> static struct {
> @@ -653,6 +660,12 @@ int git_atexit(void (*handler)(void))
> }
> #define atexit git_atexit
>
> +static int process_is_async;
> +int in_async(void)
> +{
> + return process_is_async;
> +}
> +
> #endif
>
> int start_async(struct async *async)
> @@ -712,6 +725,7 @@ int start_async(struct async *async)
> if (need_out)
> close(fdout[0]);
> git_atexit_clear();
> + process_is_async = 1;
> exit(!!async->proc(proc_in, proc_out, async->data));
> }
>
> diff --git a/run-command.h b/run-command.h
> index 5b4425a..629fab7 100644
> --- a/run-command.h
> +++ b/run-command.h
> @@ -118,5 +118,6 @@ struct async {
>
> int start_async(struct async *async);
> int finish_async(struct async *async);
> +int in_async(void);
>
> #endif
--
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
Re: [PATCH 2/2] pkt-line: show packets in async processes as "sideband"
Jeff King writes:
> What we really need is thread-local storage for
> packet_trace_identity. But the async code does not provide
> an interface for that, and it would be messy to add it here
> (we'd have to care about pthreads, initializing our
> pthread_key_t ahead of time, etc).
True.
> So instead, let us just assume that any async process is
> handling sideband data. That's always true now, and is
> likely to remain so in the future.
Hmm, does Stefan's thread-pool thing interact with this decision in
any way?
>
> The output looks like:
>
>packet: sideband< \1000eunpack ok0019ok refs/heads/master
>packet: push< unpack ok
>packet: push< ok refs/heads/master
>packet: push<
>packet: sideband<
>
> Signed-off-by: Jeff King
> ---
> pkt-line.c | 8 +++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/pkt-line.c b/pkt-line.c
> index 08a1427..62fdb37 100644
> --- a/pkt-line.c
> +++ b/pkt-line.c
> @@ -1,5 +1,6 @@
> #include "cache.h"
> #include "pkt-line.h"
> +#include "run-command.h"
>
> char packet_buffer[LARGE_PACKET_MAX];
> static const char *packet_trace_prefix = "git";
> @@ -11,6 +12,11 @@ void packet_trace_identity(const char *prog)
> packet_trace_prefix = xstrdup(prog);
> }
>
> +static const char *get_trace_prefix(void)
> +{
> + return in_async() ? "sideband" : packet_trace_prefix;
> +}
> +
> static int packet_trace_pack(const char *buf, unsigned int len, int sideband)
> {
> if (!sideband) {
> @@ -57,7 +63,7 @@ static void packet_trace(const char *buf, unsigned int len,
> int write)
> strbuf_init(&out, len+32);
>
> strbuf_addf(&out, "packet: %12s%c ",
> - packet_trace_prefix, write ? '>' : '<');
> + get_trace_prefix(), write ? '>' : '<');
>
> /* XXX we should really handle printable utf8 */
> for (i = 0; i < len; i++) {
--
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
[PATCH] rerere: release lockfile in non-writing functions
There's a bug in builtin/am.c in which we take a lock on
MERGE_RR recursively. But rather than fix am.c, this patch
fixes the confusing interface from rerere.c that caused the
bug. Read on for the gory details.
The setup_rerere() function both reads the existing MERGE_RR
file, and takes MERGE_RR.lock. In the rerere() and
rerere_forget() functions, we end up in write_rr(), which
will then commit the lock file.
But for functions like rerere_clear() that do not write to
MERGE_RR, we expect the caller to have handled
setup_rerere(). That caller would then need to release the
lockfile, but it can't; the lock struct is local to
rerere.c.
For builtin/rerere.c, this is OK. We run a single rerere
operation and then exit immediately, which has the side
effect of rolling back the lockfile.
But in builtin/am.c, this is actively wrong. If we run "git
am -3 --skip", we call setup-rerere twice without releasing
the lock:
1. The "--skip" causes us to call am_rerere_clear(), which
calls setup_rerere(), but never drops the lock.
2. We then proceed to the next patch.
3. The "--3way" may cause us to call rerere() to handle
conflicts in that patch, but we are already holding the
lock. The lockfile code dies with:
BUG: prepare_tempfile_object called for active object
We could fix this by having rerere_clear() call
rollback_lock_file(). But it feels a bit odd for it to roll
back a lockfile that it did not itself take. So let's
simplify the interface further, and handle setup_rerere in
the function itself, taking away the question from the
caller over whether they need to do so.
We can give rerere_gc() the same treatment, as well (even
though it doesn't have any callers besides builtin/rerere.c
at this point). Note that these functions don't take flags
from their callers to pass along to setup_rerere; that's OK,
because the flags would not be meaningful for what they are
doing.
Both of those functions need to hold the lock because even
though they do not write to MERGE_RR, they are still writing
and should be protected from a simultaneous "rerere" run.
But rerere_remaining(), "rerere diff", and "rerere status"
are all read-only operations. They want to setup_rerere(),
but do not care about taking the lock in the first place.
Since our update of MERGE_RR is the usual atomic rename done
by commit_lock_file, they can just do a lockless read. For
that, we teach setup_rerere a READONLY flag to avoid the
lock.
As a bonus, this pushes builtin/rerere.c's setup_rerere call
closer to the functions that use it. Which means that "git
rerere totally-bogus-command" will no longer silently
exit(0) in a repository without rerere enabled.
Signed-off-by: Jeff King
---
builtin/am.c | 5 -
builtin/rerere.c | 18 +-
rerere.c | 17 +++--
rerere.h | 1 +
t/t4150-am.sh| 36
5 files changed, 61 insertions(+), 16 deletions(-)
diff --git a/builtin/am.c b/builtin/am.c
index 27165a6..83b3d86 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -2057,11 +2057,6 @@ static int clean_index(const unsigned char *head, const
unsigned char *remote)
static void am_rerere_clear(void)
{
struct string_list merge_rr = STRING_LIST_INIT_DUP;
- int fd = setup_rerere(&merge_rr, 0);
-
- if (fd < 0)
- return;
-
rerere_clear(&merge_rr);
string_list_clear(&merge_rr, 1);
}
diff --git a/builtin/rerere.c b/builtin/rerere.c
index 7afadd2..12535c9 100644
--- a/builtin/rerere.c
+++ b/builtin/rerere.c
@@ -50,7 +50,7 @@ static int diff_two(const char *file1, const char *label1,
int cmd_rerere(int argc, const char **argv, const char *prefix)
{
struct string_list merge_rr = STRING_LIST_INIT_DUP;
- int i, fd, autoupdate = -1, flags = 0;
+ int i, autoupdate = -1, flags = 0;
struct option options[] = {
OPT_SET_INT(0, "rerere-autoupdate", &autoupdate,
@@ -79,18 +79,16 @@ int cmd_rerere(int argc, const char **argv, const char
*prefix)
return rerere_forget(&pathspec);
}
- fd = setup_rerere(&merge_rr, flags);
- if (fd < 0)
- return 0;
-
if (!strcmp(argv[0], "clear")) {
rerere_clear(&merge_rr);
} else if (!strcmp(argv[0], "gc"))
rerere_gc(&merge_rr);
- else if (!strcmp(argv[0], "status"))
+ else if (!strcmp(argv[0], "status")) {
+ if (setup_rerere(&merge_rr, flags | RERERE_READONLY) < 0)
+ return 0;
for (i = 0; i < merge_rr.nr; i++)
printf("%s\n", merge_rr.items[i].string);
- else if (!strcmp(argv[0], "remaining")) {
+ } else if (!strcmp(argv[0], "remaining")) {
rerere_remaining(&merge_rr);
for (i = 0; i < merge_rr.nr; i++) {
if (merge_rr.items[i].util != RERERE_RESOLVED)
@@ -100,13 +98,15 @@ int cmd_rerere(int arg
Re: [PATCH v2 2/6] date: make "local" orthogonal to date format
John Keeping writes: > This is Jeff's original patch with my fixup for DATE_STRFTIME squashed > in and a new change to reject "raw-local" (in both Documentation/ and > date.c). Even in --date=raw, we do show the timezone offset, so I do not necessarily agree that raw-local is nonsensical. That's the only difference between the one I queued yesterday and this one. -- 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
Re: [PATCH 1/2] run-command: provide in_async query function
On Tue, Sep 01, 2015 at 03:09:56PM -0700, Junio C Hamano wrote: > Jeff King writes: > > > It's not easy for arbitrary code to find out whether it is > > running in an async process or not. A top-level function > > which is fed to start_async() can know (you just pass down > > an argument saying "you are async"). But that function may > > call other global functions, and we would not want to have > > to pass the information all the way through the call stack. > > > > Nor can we simply set a global variable, as those may be > > shared between async threads and the main thread (if the > > platform supports pthreads). We need pthread tricks _or_ a > > global variable, depending on how start_async is > > implemented. > > > > The callers don't have enough information to do this right, > > so let's provide a simple query function that does. > > Fortunately we can reuse the existing infrastructure to make > > the pthread case simple (and even simplify die_async() by > > using our new function). > > > > Signed-off-by: Jeff King > > --- > > What is not immediately obvious from the above description is why a > code may want to care if it is in_async() in the first place. > > If there weren't the die_async() update, the readers might have been > left utterly baffled (or they can somehow see this is related to > 2/2) but it is a bit hard to arrange in "git log" as going to child > is harder. > > The patch looks good. Thanks. Yeah, I almost mentioned "...in the next patch we'll need this", but I wasn't sure how to bring that up without going into the complicated reasoning in that patch. I don't know if it's worth adding "we don't have any callers yet, but we will add one in a moment". -Peff -- 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
Re: [PATCH v2 3/6] t6300: introduce test_date() helper
Nicely done. -- 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
Re: [PATCH 2/2] pkt-line: show packets in async processes as "sideband"
On Tue, Sep 1, 2015 at 3:13 PM, Junio C Hamano wrote:
> Jeff King writes:
>
>> What we really need is thread-local storage for
>> packet_trace_identity. But the async code does not provide
>> an interface for that, and it would be messy to add it here
>> (we'd have to care about pthreads, initializing our
>> pthread_key_t ahead of time, etc).
>
> True.
>
>> So instead, let us just assume that any async process is
>> handling sideband data. That's always true now, and is
>> likely to remain so in the future.
>
> Hmm, does Stefan's thread-pool thing interact with this decision in
> any way?
I do not plan to actually fetch from inside the thread pool, but each thread
is just a proxy for starting a new process doing the fetch and getting
the output
in order.
That seems to be the least amount of work to me. Very long term we may want to
do the fetch directly in a thread pool worker, but then we can also
add the thread
local storage interface.
>>
>> The output looks like:
>>
>>packet: sideband< \1000eunpack ok0019ok refs/heads/master
>>packet: push< unpack ok
>>packet: push< ok refs/heads/master
>>packet: push<
>>packet: sideband<
>>
>> Signed-off-by: Jeff King
>> ---
>> pkt-line.c | 8 +++-
>> 1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/pkt-line.c b/pkt-line.c
>> index 08a1427..62fdb37 100644
>> --- a/pkt-line.c
>> +++ b/pkt-line.c
>> @@ -1,5 +1,6 @@
>> #include "cache.h"
>> #include "pkt-line.h"
>> +#include "run-command.h"
>>
>> char packet_buffer[LARGE_PACKET_MAX];
>> static const char *packet_trace_prefix = "git";
>> @@ -11,6 +12,11 @@ void packet_trace_identity(const char *prog)
>> packet_trace_prefix = xstrdup(prog);
>> }
>>
>> +static const char *get_trace_prefix(void)
>> +{
>> + return in_async() ? "sideband" : packet_trace_prefix;
>> +}
>> +
>> static int packet_trace_pack(const char *buf, unsigned int len, int
>> sideband)
>> {
>> if (!sideband) {
>> @@ -57,7 +63,7 @@ static void packet_trace(const char *buf, unsigned int
>> len, int write)
>> strbuf_init(&out, len+32);
>>
>> strbuf_addf(&out, "packet: %12s%c ",
>> - packet_trace_prefix, write ? '>' : '<');
>> + get_trace_prefix(), write ? '>' : '<');
>>
>> /* XXX we should really handle printable utf8 */
>> for (i = 0; i < len; i++) {
--
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
Re: [PATCH 2/2] pkt-line: show packets in async processes as "sideband"
On Tue, Sep 01, 2015 at 03:13:25PM -0700, Junio C Hamano wrote: > > So instead, let us just assume that any async process is > > handling sideband data. That's always true now, and is > > likely to remain so in the future. > > Hmm, does Stefan's thread-pool thing interact with this decision in > any way? I don't think so. It is true that: start_async(...); finish_async(...); pthread_create(foo); would incorrectly report in_async() inside the foo function (because we know that async has been kicked off, and we know that we are in a thread that is not the main one). But I don't think it matters in the current code base, because we tend to use async for I/O tasks like muxing/demuxing, and real threads in CPU-heavy tasks like pack-objects. Still, I admit it is a little gross, and may be a problem in the future. An alternate approach would be to have the async system provide a thread-local storage abstraction. With pthreads, we'd build on pthread_setspecific(), and without, it becomes a simple global variable. That's more work, but a lot less error-prone, and it may come in handy in the future. -Peff -- 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
Re: [PATCH v2 2/6] date: make "local" orthogonal to date format
On Tue, Sep 01, 2015 at 03:16:50PM -0700, Junio C Hamano wrote: > John Keeping writes: > > > This is Jeff's original patch with my fixup for DATE_STRFTIME squashed > > in and a new change to reject "raw-local" (in both Documentation/ and > > date.c). > > Even in --date=raw, we do show the timezone offset, so I do not > necessarily agree that raw-local is nonsensical. That's the only > difference between the one I queued yesterday and this one. Yeah, that's why I didn't change it in the original. But to be honest, I cannot imagine any case where that is _useful_, so I do not mind at all to declare it off-limits, even though it is not nonsensical (though it is a little strange to ask for "raw" data and then ask for it to be munged). IOW, I do not mind either way, but the fact that we have to _add_ code to disallow it makes me slightly in favor of allowing it. :) -Peff -- 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
Re: [PATCH v2 3/6] t6300: introduce test_date() helper
On Tue, Sep 1, 2015 at 5:55 PM, John Keeping wrote:
> This moves the setup of the "expected" file inside the test case. The
> helper function has the advantage that we can use SQ in the file content
> without needing to escape the quotes.
>
> Signed-off-by: John Keeping
> ---
> diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
> index 7c9bec7..5fdb964 100755
> --- a/t/t6300-for-each-ref.sh
> +++ b/t/t6300-for-each-ref.sh
> @@ -146,85 +146,54 @@ test_expect_success 'Check invalid format specifiers
> are errors' '
> test_must_fail git for-each-ref --format="%(authordate:INVALID)"
> refs/heads
> '
>
> -cat >expected <<\EOF
> -'refs/heads/master' 'Mon Jul 3 17:18:43 2006 +0200' 'Mon Jul 3 17:18:44 2006
> +0200'
> -'refs/tags/testtag' 'Mon Jul 3 17:18:45 2006 +0200'
> -EOF
> +test_date () {
> + f=$1
f=$1 &&
> + committer_date=$2 &&
> + author_date=$3 &&
> + tagger_date=$4 &&
> + cat >expected <<-EOF &&
> + 'refs/heads/master' '$committer_date' '$author_date'
> + 'refs/tags/testtag' '$tagger_date'
> + EOF
> + (
> + git for-each-ref --shell --format="%(refname)
> %(committerdate${f:+:$f}) %(authordate${f:+:$f})" refs/heads &&
> + git for-each-ref --shell --format="%(refname)
> %(taggerdate${f:+:$f})" refs/tags
> + ) >actual &&
> + test_cmp expected actual
> +}
--
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
Re: [PATCH 2/2] pkt-line: show packets in async processes as "sideband"
On Tue, Sep 01, 2015 at 03:23:06PM -0700, Stefan Beller wrote: > > Hmm, does Stefan's thread-pool thing interact with this decision in > > any way? > > I do not plan to actually fetch from inside the thread pool, but each thread > is just a proxy for starting a new process doing the fetch and getting > the output > in order. Ah, right, I think I misunderstood Junio's question. Yes, if we start calling cmd_fetch() from inside the threads, things may get confusing. I'll see how painful the thread storage approach would be. -Peff -- 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
Re: [PATCH v2 3/6] t6300: introduce test_date() helper
On Tue, Sep 01, 2015 at 10:55:41PM +0100, John Keeping wrote: > I considered moving the test_expect_success into the helper, like with > test_atom earlier in the file, but it doesn't make the code much more > concise and we still need either to setup the output outside the test > case or to escape SQ inside SQ. Moving it inside also makes it harder to test_expect_failure. :) > test_expect_success 'Check unformatted date fields output' ' > - (git for-each-ref --shell --format="%(refname) %(committerdate) > %(authordate)" refs/heads && > - git for-each-ref --shell --format="%(refname) %(taggerdate)" refs/tags) > >actual && > - test_cmp expected actual > + test_date "" "Mon Jul 3 17:18:43 2006 +0200" "Mon Jul 3 17:18:44 2006 > +0200" "Mon Jul 3 17:18:45 2006 +0200" I notice we end up with rather long lines for some of these. Would: test_date "" <<-\EOF Mon Jul 3 17:18:43 2006 +0200 Mon Jul 3 17:18:44 2006 +0200 Mon Jul 3 17:18:45 2006 +0200 EOF be more readable? -Peff -- 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
Re: [PATCH 2/2] pkt-line: show packets in async processes as "sideband"
On Tue, Sep 1, 2015 at 3:26 PM, Jeff King wrote: > On Tue, Sep 01, 2015 at 03:23:06PM -0700, Stefan Beller wrote: > >> > Hmm, does Stefan's thread-pool thing interact with this decision in >> > any way? >> >> I do not plan to actually fetch from inside the thread pool, but each thread >> is just a proxy for starting a new process doing the fetch and getting >> the output >> in order. > > Ah, right, I think I misunderstood Junio's question. Yes, if we start > calling cmd_fetch() from inside the threads, things may get confusing. > > I'll see how painful the thread storage approach would be. I think that may be part of the thread pool API eventually. I don't plan on relying on thread local storage, which is why I did not add it there, though I guess it can be added in there quite conveniently. > > -Peff -- 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
Re: [PATCH v2 2/6] date: make "local" orthogonal to date format
On Tue, Sep 01, 2015 at 03:16:50PM -0700, Junio C Hamano wrote:
> John Keeping writes:
>
> > This is Jeff's original patch with my fixup for DATE_STRFTIME squashed
> > in and a new change to reject "raw-local" (in both Documentation/ and
> > date.c).
>
> Even in --date=raw, we do show the timezone offset, so I do not
> necessarily agree that raw-local is nonsensical. That's the only
> difference between the one I queued yesterday and this one.
I suspect it depends on the interpretation of "raw"; the code currently
interprets raw to mean "exactly what exists in the commit/tag", in which
case converting it to the local timezone is wrong. But the
documentation describes "raw" as "the raw Git %s %z format", and if we
interpret it to mean "Git's internal date format" then "raw-local" makes
sense.
The alternative would be the patch below as a preparatory step.
-- >8 --
diff --git a/date.c b/date.c
index f048416..345890f 100644
--- a/date.c
+++ b/date.c
@@ -175,12 +175,6 @@ const char *show_date(unsigned long time, int tz, const
struct date_mode *mode)
struct tm *tm;
static struct strbuf timebuf = STRBUF_INIT;
- if (mode->type == DATE_RAW) {
- strbuf_reset(&timebuf);
- strbuf_addf(&timebuf, "%lu %+05d", time, tz);
- return timebuf.buf;
- }
-
if (mode->type == DATE_RELATIVE) {
struct timeval now;
@@ -193,6 +187,12 @@ const char *show_date(unsigned long time, int tz, const
struct date_mode *mode)
if (mode->local)
tz = local_tzoffset(time);
+ if (mode->type == DATE_RAW) {
+ strbuf_reset(&timebuf);
+ strbuf_addf(&timebuf, "%lu %+05d", time, tz);
+ return timebuf.buf;
+ }
+
tm = time_to_tm(time, tz);
if (!tm) {
tm = time_to_tm(0, 0);
--
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
Re: [PATCH 2/2] pkt-line: show packets in async processes as "sideband"
On Tue, Sep 01, 2015 at 03:31:41PM -0700, Stefan Beller wrote: > > Ah, right, I think I misunderstood Junio's question. Yes, if we start > > calling cmd_fetch() from inside the threads, things may get confusing. > > > > I'll see how painful the thread storage approach would be. > > I think that may be part of the thread pool API eventually. > > I don't plan on relying on thread local storage, which is why I did > not add it there, > though I guess it can be added in there quite conveniently. I think the task_queue and the async code are two separate things, though. The task_queue, when we do not have threads, falls back to doing things serially in a single process. Changing "thread local" variables there is OK, but tasks need to be aware that they might affect subsequent tasks. Whereas for async code, we fall back to doing things in a forked sub-process. And there "thread local" really is local to the async process, no cleanup required. -Peff -- 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
Re: [PATCH v2 2/6] date: make "local" orthogonal to date format
On Tue, Sep 01, 2015 at 11:33:08PM +0100, John Keeping wrote: > > Even in --date=raw, we do show the timezone offset, so I do not > > necessarily agree that raw-local is nonsensical. That's the only > > difference between the one I queued yesterday and this one. > > I suspect it depends on the interpretation of "raw"; the code currently > interprets raw to mean "exactly what exists in the commit/tag", in which > case converting it to the local timezone is wrong. But the > documentation describes "raw" as "the raw Git %s %z format", and if we > interpret it to mean "Git's internal date format" then "raw-local" makes > sense. > > The alternative would be the patch below as a preparatory step. Ah, right, I forgot that we need to refactor show_date() to actually do the right thing. I think I'd be in favor of just disallowing "raw-local", then. -Peff -- 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
Re: [PATCH v2 3/6] t6300: introduce test_date() helper
On Tue, Sep 01, 2015 at 06:31:58PM -0400, Jeff King wrote: > On Tue, Sep 01, 2015 at 10:55:41PM +0100, John Keeping wrote: > > > I considered moving the test_expect_success into the helper, like with > > test_atom earlier in the file, but it doesn't make the code much more > > concise and we still need either to setup the output outside the test > > case or to escape SQ inside SQ. > > Moving it inside also makes it harder to test_expect_failure. :) > > > test_expect_success 'Check unformatted date fields output' ' > > - (git for-each-ref --shell --format="%(refname) %(committerdate) > > %(authordate)" refs/heads && > > - git for-each-ref --shell --format="%(refname) %(taggerdate)" refs/tags) > > >actual && > > - test_cmp expected actual > > + test_date "" "Mon Jul 3 17:18:43 2006 +0200" "Mon Jul 3 17:18:44 2006 > > +0200" "Mon Jul 3 17:18:45 2006 +0200" > > I notice we end up with rather long lines for some of these. Would: > > test_date "" <<-\EOF > Mon Jul 3 17:18:43 2006 +0200 > Mon Jul 3 17:18:44 2006 +0200 > Mon Jul 3 17:18:45 2006 +0200 > EOF > > be more readable? We could just do: test_date "" \ "Tue Jul 4 01:18:43 2006 +0200" \ "Tue Jul 4 01:18:44 2006 +0200" \ "Tue Jul 4 01:18:45 2006 +0200" I'm not entirely sure why I didn't now that I look at 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
Re: [PATCH v2 3/6] t6300: introduce test_date() helper
On Tue, Sep 01, 2015 at 11:40:13PM +0100, John Keeping wrote: > > I notice we end up with rather long lines for some of these. Would: > > > > test_date "" <<-\EOF > > Mon Jul 3 17:18:43 2006 +0200 > > Mon Jul 3 17:18:44 2006 +0200 > > Mon Jul 3 17:18:45 2006 +0200 > > EOF > > > > be more readable? > > We could just do: > > test_date "" \ > "Tue Jul 4 01:18:43 2006 +0200" \ > "Tue Jul 4 01:18:44 2006 +0200" \ > "Tue Jul 4 01:18:45 2006 +0200" > > I'm not entirely sure why I didn't now that I look at it! Yeah, that also looks good, and is probably less confusing that the here-doc. -Peff -- 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
Re: [PATCH v2 2/6] date: make "local" orthogonal to date format
OK by me. Thanks, I also forgot that need for preparatory code movement. On Tue, Sep 1, 2015 at 3:39 PM, Jeff King wrote: > On Tue, Sep 01, 2015 at 11:33:08PM +0100, John Keeping wrote: > >> > Even in --date=raw, we do show the timezone offset, so I do not >> > necessarily agree that raw-local is nonsensical. That's the only >> > difference between the one I queued yesterday and this one. >> >> I suspect it depends on the interpretation of "raw"; the code currently >> interprets raw to mean "exactly what exists in the commit/tag", in which >> case converting it to the local timezone is wrong. But the >> documentation describes "raw" as "the raw Git %s %z format", and if we >> interpret it to mean "Git's internal date format" then "raw-local" makes >> sense. >> >> The alternative would be the patch below as a preparatory step. > > Ah, right, I forgot that we need to refactor show_date() to actually do > the right thing. > > I think I'd be in favor of just disallowing "raw-local", then. > > -Peff -- 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
Re: [PATCH v2 0/6] Make "local" orthogonal to date format
On Tue, Sep 01, 2015 at 10:55:38PM +0100, John Keeping wrote: > Jeff's first patch is unmodified but I've squashed the fixed currently > on "pu" into the second. I also realised while adding the tests that > "raw-local" is meaningless so I've modified the code to reject it in the > same way as "relative-local". > > Jeff King (2): > fast-import: switch crash-report date to iso8601 > date: make "local" orthogonal to date format > > John Keeping (4): > t6300: introduce test_date() helper > t6300: make UTC and local dates different > t6300: add test for "raw" date format > t6300: add tests for "-local" date formats Looks like we've agreed on disallowing "raw-local"[1] for the 2nd patch. The other 4 all look good to me, with the exception of the line-wrapping on 3/6. Thanks for taking care of this. -Peff [1] I do think the error message for "relative-local is nonsense" could perhaps be more explanatory, but I couldn't come up with any better wording. But if you have ideas, feel free to switch 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
