* Re: [PATCH 1/2] docs: add a basic description of the config API
From: Junio C Hamano @ 2012-02-08 6:40 UTC (permalink / raw)
To: Nguyen Thai Ngoc Duy; +Cc: Jeff King, git
In-Reply-To: <CACsJy8AU3ZA1=Q3vahhP6Nr=FZNKd7oRJ04mtKVs+uvNqJeVaw@mail.gmail.com>
Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:
>>> The "1 means I understood this" convention is used by userdiff_config. I
>>> don't like that it is unlike every other config callback,...
>>> Looking at the code again, though, ...
>>> Hmm. Yeah. The userdiff calling convention dates back to late 2008....
>>> So I think we could go back and simplify the userdiff_config code now.
>>
>> I remembered where I saw the new "offender"; it was nd/columns
>> topic (Cc'ing Nguyễn).
>
> nd/columns does use "1" convention in git_column_config(), but the
> direct config callback function does not return 1 to config machinery.
> All call sites follow this pattern:
>
> int ret = git_column_config(key, var, "command", &colopts);
> if (ret <= 0) return ret;
>
> I think it's ok.
I too think this should be acceptable, but that is not the point.
Your excuse that "the toplevel callback in my callchain never returns 1,
so overall the nd/columns series is ok" just muddies the water. It means
if later somebody wanted to use inner callback functions you use from the
git_column_config() callchain chain as a toplevel callback for whatever
reason, that will violate the "0 for success, or -1 for error" convention.
More importantly, if somebody wants to turn a top-level callback that
currently returns 0 into a sub callback used by his callback callchain, he
cannot change that existing callback to return 1 to tell him to short
circuit, because for other callers returning 1 would be a violation.
What I was getting at is that we probably should officially declare that
returning 1 to signal success is perfectly acceptable (and it probably
should mean the caller who called the callback function as a sub callback
should return immediately, taking it as a signal that the key has already
been handled), as the primary purpose of this thread to discuss Peff's
patch is to write these rules down.
Of course, all that relies on the audit of the git_config() machinery. I
think it is written to accept non-negative as success, and that is why I
said "I too think this should be acceptable" in the first place.
^ permalink raw reply
* Re: [PATCH 1/2] docs: add a basic description of the config API
From: Nguyen Thai Ngoc Duy @ 2012-02-08 6:55 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, git
In-Reply-To: <7vmx8tzv3l.fsf@alter.siamese.dyndns.org>
On Wed, Feb 8, 2012 at 1:40 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Your excuse that "the toplevel callback in my callchain never returns 1,
> so overall the nd/columns series is ok" just muddies the water. It means
> if later somebody wanted to use inner callback functions you use from the
> git_column_config() callchain chain as a toplevel callback for whatever
> reason, that will violate the "0 for success, or -1 for error" convention.
> More importantly, if somebody wants to turn a top-level callback that
> currently returns 0 into a sub callback used by his callback callchain, he
> cannot change that existing callback to return 1 to tell him to short
> circuit, because for other callers returning 1 would be a violation.
>
> What I was getting at is that we probably should officially declare that
> returning 1 to signal success is perfectly acceptable (and it probably
> should mean the caller who called the callback function as a sub callback
> should return immediately, taking it as a signal that the key has already
> been handled), as the primary purpose of this thread to discuss Peff's
> patch is to write these rules down.
Or allow multiple callbacks from git_config(). The problem with is it
adds another common set of config vars. Now it's common to chain var
sets by doing
int config_callback(...)
{
....
return yet_another_callback(...);
}
int main()
{
git_config(config_callback, ...)
...
where yet_another_callback() hard codes another callback (usually
git_default_config). This is inflexible. If we allow multiple
callbacks, git_column_config() could be changed to return just 0 or -1
(i.e. 1 remains unsuccessful error code).
On second thought, I think calling git_config() multiple times, each
time with one callback, may work too. We may want to cache config
content to avoid accessing files too many times though.
> Of course, all that relies on the audit of the git_config() machinery. I
> think it is written to accept non-negative as success, and that is why I
> said "I too think this should be acceptable" in the first place.
--
Duy
^ permalink raw reply
* [StGit PATCH] Parse commit object header correctly
From: Junio C Hamano @ 2012-02-08 7:33 UTC (permalink / raw)
To: Karl Hasselström, Catalin Marinas
Cc: Andy Green (林安廸), git
In-Reply-To: <7vvcni1r5u.fsf@alter.siamese.dyndns.org>
To allow parsing the header produced by versions of Git newer than the
code written to parse it, all commit parsers are expected to skip unknown
header lines, so that newer types of header lines can be added safely.
The only three things that are promised are:
(1) the header ends with an empty line (just an LF, not "a blank line"),
(2) unknown lines can be skipped, and
(3) a header "field" begins with the field name, followed by a single SP
followed by the value.
The parser used by StGit, introduced by commit cbe4567 (New StGit core
infrastructure: repository operations, 2007-12-19), was accidentally a bit
too loose to lose information, and a bit too strict to raise exception
when dealing with a line it does not understand.
- It used "strip()" to lose whitespaces from both ends, risking a line
with only whitespaces to be mistaken as the end of the header.
- It used "k, v = line.split(None, 1)", blindly assuming that all header
lines (including the ones that the version of StGit may not understand)
can safely be split without raising an exception, which is not true if
there is no SP on the line.
This patch changes the parsing logic so that it:
(1) detects end of the hedaer correctly by treating only an empty line as
such;
(2) handles multi-line fields (a header line that begins with a single SP
is appended to the previous line after removing that leading SP but
retaining the LF between the line and the previous line) correctly;
(3) splits a line at the first SP to find the field name, but only does
so when there actually is SP on the line; and
(4) ignores lines that cannot be understood without barfing.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
* Earlier I sent a minimum parser fix that ignores multi-line fields, as
the fields StGit cares about are all single line. This patch also
teaches multi-line fields to the parser, so that later versions of
StGit can parse and use them if they choose to.
Python is not my primary language, so please take this with a grain of
salt.
Thanks.
stgit/lib/git.py | 41 +++++++++++++++++++++++++++--------------
1 file changed, 27 insertions(+), 14 deletions(-)
diff --git a/stgit/lib/git.py b/stgit/lib/git.py
index 56287f6..f19371b 100644
--- a/stgit/lib/git.py
+++ b/stgit/lib/git.py
@@ -390,21 +390,34 @@ class CommitData(Immutable, Repr):
@return: A new L{CommitData} object
@rtype: L{CommitData}"""
cd = cls(parents = [])
- lines = list(s.splitlines(True))
+ raw_lines = list(s.splitlines(True))
+ lines = []
+ # Collapse multi-line header lines
+ for i in xrange(len(raw_lines)):
+ line = raw_lines[i]
+ if line == '\n':
+ cd.set_message(''.join(raw_lines[i+1:]))
+ break
+ if line[0] == ' ':
+ # continuation line
+ lines[-1] += '\n' + line[1:]
+ else:
+ lines.append(line)
for i in xrange(len(lines)):
- line = lines[i].strip()
- if not line:
- return cd.set_message(''.join(lines[i+1:]))
- key, value = line.split(None, 1)
- if key == 'tree':
- cd = cd.set_tree(repository.get_tree(value))
- elif key == 'parent':
- cd = cd.add_parent(repository.get_commit(value))
- elif key == 'author':
- cd = cd.set_author(Person.parse(value))
- elif key == 'committer':
- cd = cd.set_committer(Person.parse(value))
- assert False
+ line = lines[i].rstrip('\n')
+ ix = line.find(' ')
+ if 0 <= ix:
+ key, value = line[0:ix], line[ix+1:]
+ if key == 'tree':
+ cd = cd.set_tree(repository.get_tree(value))
+ elif key == 'parent':
+ cd = cd.add_parent(repository.get_commit(value))
+ elif key == 'author':
+ cd = cd.set_author(Person.parse(value))
+ elif key == 'committer':
+ cd = cd.set_committer(Person.parse(value))
+ return cd
+
class Commit(GitObject):
"""Represents a git commit object. All the actual data contents of the
^ permalink raw reply related
* Re: filter-branch ignoring gitattributes?
From: norbert.nemec @ 2012-02-08 8:23 UTC (permalink / raw)
To: git
In-Reply-To: <jgglh9$mrv$1@dough.gmane.org>
Meanwhile, I can answer my own question:
The "text" attribute works similar to a filter.
As with filters in general, it is only applied when git detects a
modification in the file. A file change is detected by the file
modification time on disc. To reread all files from disk and re-apply
the filters to all of them, you would typically delete the .git/index
file. In filter-branch, this is not possible, so instead you have to
"touch" all files in the --tree-filter command.
Am 03.02.12 13:55, schrieb norbert.nemec:
> Hi there,
>
> it seems that 'git filter-branch' ignores the setting in
> .git/info/attributes - is that correct?
>
> Does .gitattributes work reliably?
>
> Specifically, I tried
>
> git filter-branch \
> --tree-filter 'echo "* text=auto" > .gitattributes' \
> --tag-name-filter cat
> --prune-empty -- --all
>
> It seems to work, but I am not sure whether I missed anything.
>
> Greetings,
> Norbert
>
^ permalink raw reply
* Re: [StGit PATCH] Parse commit object header correctly
From: Michael Haggerty @ 2012-02-08 10:00 UTC (permalink / raw)
To: Junio C Hamano
Cc: Karl Hasselström, Catalin Marinas,
"Andy Green (林安廸)", git
In-Reply-To: <7vd39pzsmq.fsf_-_@alter.siamese.dyndns.org>
On 02/08/2012 08:33 AM, Junio C Hamano wrote:
> To allow parsing the header produced by versions of Git newer than the
> code written to parse it, all commit parsers are expected to skip unknown
> header lines, so that newer types of header lines can be added safely.
> The only three things that are promised are:
>
> (1) the header ends with an empty line (just an LF, not "a blank line"),
> (2) unknown lines can be skipped, and
> (3) a header "field" begins with the field name, followed by a single SP
> followed by the value.
>
> The parser used by StGit, introduced by commit cbe4567 (New StGit core
> infrastructure: repository operations, 2007-12-19), was accidentally a bit
> too loose to lose information, and a bit too strict to raise exception
> when dealing with a line it does not understand.
>
> - It used "strip()" to lose whitespaces from both ends, risking a line
> with only whitespaces to be mistaken as the end of the header.
>
> - It used "k, v = line.split(None, 1)", blindly assuming that all header
> lines (including the ones that the version of StGit may not understand)
> can safely be split without raising an exception, which is not true if
> there is no SP on the line.
>
> This patch changes the parsing logic so that it:
>
> (1) detects end of the hedaer correctly by treating only an empty line as
> such;
> (2) handles multi-line fields (a header line that begins with a single SP
> is appended to the previous line after removing that leading SP but
> retaining the LF between the line and the previous line) correctly;
> (3) splits a line at the first SP to find the field name, but only does
> so when there actually is SP on the line; and
> (4) ignores lines that cannot be understood without barfing.
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>
> * Earlier I sent a minimum parser fix that ignores multi-line fields, as
> the fields StGit cares about are all single line. This patch also
> teaches multi-line fields to the parser, so that later versions of
> StGit can parse and use them if they choose to.
>
> Python is not my primary language, so please take this with a grain of
> salt.
>
> Thanks.
>
> stgit/lib/git.py | 41 +++++++++++++++++++++++++++--------------
> 1 file changed, 27 insertions(+), 14 deletions(-)
>
> diff --git a/stgit/lib/git.py b/stgit/lib/git.py
> index 56287f6..f19371b 100644
> --- a/stgit/lib/git.py
> +++ b/stgit/lib/git.py
> @@ -390,21 +390,34 @@ class CommitData(Immutable, Repr):
> @return: A new L{CommitData} object
> @rtype: L{CommitData}"""
> cd = cls(parents = [])
> - lines = list(s.splitlines(True))
> + raw_lines = list(s.splitlines(True))
str.splitlines() splits lines at any EOL pattern ('\n', '\r\n' or '\r'
alone). If you want to be sure to split only on '\n', I think the
simplest alternative is
raw_lines = s.split('\n')
str.split() and str.splitlines() already return lists, so it is not
necessary to wrap the result in list().
But please note that str.split() discards the split characters, and if
the last character in the string is '\n' then the last string in the
result list is the empty string.
> + lines = []
> + # Collapse multi-line header lines
> + for i in xrange(len(raw_lines)):
> + line = raw_lines[i]
The two previous lines can be written
for (i, line) in enumerate(raw_lines):
> + if line == '\n':
> + cd.set_message(''.join(raw_lines[i+1:]))
> + break
> + if line[0] == ' ':
> + # continuation line
> + lines[-1] += '\n' + line[1:]
In your original version, lines[-1] would already be LF-terminated, so
this line would create a double-LF in the string.
> + else:
> + lines.append(line)
> for i in xrange(len(lines)):
> - line = lines[i].strip()
> - if not line:
> - return cd.set_message(''.join(lines[i+1:]))
> - key, value = line.split(None, 1)
> - if key == 'tree':
> - cd = cd.set_tree(repository.get_tree(value))
> - elif key == 'parent':
> - cd = cd.add_parent(repository.get_commit(value))
> - elif key == 'author':
> - cd = cd.set_author(Person.parse(value))
> - elif key == 'committer':
> - cd = cd.set_committer(Person.parse(value))
> - assert False
> + line = lines[i].rstrip('\n')
> + ix = line.find(' ')
> + if 0 <= ix:
> + key, value = line[0:ix], line[ix+1:]
The above five lines can be written
for line in lines:
if ' ' in line:
key, value = line.rstrip('\n').split(' ', 1)
or (if the lack of a space should be treated more like an exception)
for line in lines:
try:
key, value = line.rstrip('\n').split(' ', 1)
except ValueError:
continue
> + if key == 'tree':
> + cd = cd.set_tree(repository.get_tree(value))
> + elif key == 'parent':
> + cd = cd.add_parent(repository.get_commit(value))
> + elif key == 'author':
> + cd = cd.set_author(Person.parse(value))
> + elif key == 'committer':
> + cd = cd.set_committer(Person.parse(value))
All in all, I would recommend something like (untested):
@return: A new L{CommitData} object
@rtype: L{CommitData}"""
cd = cls(parents = [])
lines = []
raw_lines = s.split('\n')
# Collapse multi-line header lines
for i, line in enumerate(raw_lines):
if not line:
cd.set_message('\n'.join(raw_lines[i+1:]))
break
if line.startswith(' '):
# continuation line
lines[-1] += '\n' + line[1:]
else:
lines.append(line)
for line in lines:
if ' ' in line:
key, value = line.split(' ', 1)
if key == 'tree':
cd = cd.set_tree(repository.get_tree(value))
elif key == 'parent':
cd = cd.add_parent(repository.get_commit(value))
elif key == 'author':
cd = cd.set_author(Person.parse(value))
elif key == 'committer':
cd = cd.set_committer(Person.parse(value))
return cd
Michael
--
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/
^ permalink raw reply
* Re: cloning a tree which has detached branch checked out
From: Michael S. Tsirkin @ 2012-02-08 10:17 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, Nguyen Thai Ngoc Duy, Jakub Narebski, git
In-Reply-To: <20120207171604.GA26614@sigill.intra.peff.net>
On Tue, Feb 07, 2012 at 12:16:04PM -0500, Jeff King wrote:
> On Tue, Feb 07, 2012 at 09:11:00AM -0800, Junio C Hamano wrote:
>
> > > This particular bug should have been fixed before that, even, with my
> > > c1921c1 (clone: always fetch remote HEAD, 2011-06-07). And it is tested
> > > explicitly in t5707,...
> > [...]
> > What is funny is "error: Trying to write ref HEAD with nonexistant object".
> > "git grep -e nonexistant f3fb0" does not register any hit.
>
> That misspelling of "nonexistent" was fixed by 7be8b3b (Fix typo:
> existant->existent, 2011-06-16), around the same time as my clone patch.
> Which really makes me wonder if the OP is accidentally running an old
> version of git during the tests.
Double checked and you are right: the box actually runs git 1.7.1.
Rechecked with 1.7.9.GIT and it behaves correctly.
My mistake, sorry about the noise.
--
MST
^ permalink raw reply
* Re: [StGit PATCH] Parse commit object header correctly
From: Frans Klaver @ 2012-02-08 10:43 UTC (permalink / raw)
To: Michael Haggerty
Cc: Junio C Hamano, Karl Hasselström, Catalin Marinas,
"Andy Green (林安廸)", git
In-Reply-To: <4F3247CA.1020904@alum.mit.edu>
On Wed, Feb 8, 2012 at 11:00 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
> On 02/08/2012 08:33 AM, Junio C Hamano wrote:
>
>> (1) detects end of the hedaer correctly by treating only an empty line as
>> such;
s/hedaer/header/;
>> + line = lines[i].rstrip('\n')
>> + ix = line.find(' ')
>> + if 0 <= ix:
>> + key, value = line[0:ix], line[ix+1:]
>
> The above five lines can be written
>
> for line in lines:
> if ' ' in line:
> key, value = line.rstrip('\n').split(' ', 1)
>
> or (if the lack of a space should be treated more like an exception)
>
> for line in lines:
> try:
> key, value = line.rstrip('\n').split(' ', 1)
> except ValueError:
> continue
This is generally considered more pythonic: "It's easier to ask for
forgiveness than to get permission".
>
>> + if key == 'tree':
>> + cd = cd.set_tree(repository.get_tree(value))
>> + elif key == 'parent':
>> + cd = cd.add_parent(repository.get_commit(value))
>> + elif key == 'author':
>> + cd = cd.set_author(Person.parse(value))
>> + elif key == 'committer':
>> + cd = cd.set_committer(Person.parse(value))
>
> All in all, I would recommend something like (untested):
>
> @return: A new L{CommitData} object
> @rtype: L{CommitData}"""
> cd = cls(parents = [])
> lines = []
> raw_lines = s.split('\n')
> # Collapse multi-line header lines
> for i, line in enumerate(raw_lines):
> if not line:
> cd.set_message('\n'.join(raw_lines[i+1:]))
> break
> if line.startswith(' '):
> # continuation line
> lines[-1] += '\n' + line[1:]
> else:
> lines.append(line)
>
> for line in lines:
> if ' ' in line:
> key, value = line.split(' ', 1)
> if key == 'tree':
> cd = cd.set_tree(repository.get_tree(value))
> elif key == 'parent':
> cd = cd.add_parent(repository.get_commit(value))
> elif key == 'author':
> cd = cd.set_author(Person.parse(value))
> elif key == 'committer':
> cd = cd.set_committer(Person.parse(value))
> return cd
One could also take the recommended python approach for
switch/case-like if/elif/else statements:
updater = { 'tree': lambda cd, value: cd.set_tree(repository.get_tree(value),
'parent': lambda cd, value:
cd.add_parent(repository.get_commit(value)),
'author': lambda cd, value: cd.set_author(Person.parse(value)),
'committer': lambda cd, value:
cd.set_committer(Person.parse(value))
}
for line in lines:
try:
key, value = line.split(' ', 1)
cd = updater[key](cd, value)
except ValueError:
continue
except KeyError:
continue
It documents about the same, but adds checking on double 'case'
statements. The resulting for loop is rather cleaner and the exception
approach becomes even more logical. I rather like the result, but I
guess it's mostly a matter of taste.
Cheers,
Frans
^ permalink raw reply
* [PATCH v2] Explicitly set X to avoid potential build breakage
From: Michael @ 2012-02-08 10:59 UTC (permalink / raw)
To: git
$X is appended to binary names for Windows builds (ie. git.exe).
Pollution from the environment can inadvertently trigger this behaviour,
resulting in 'git' turning into 'gitwhatever' without warning.
Signed-off-by: Michael Palimaka <kensington@astralcloak.net>
---
Makefile | 3 +++
1 files changed, 3 insertions(+), 0 deletions(-)
diff --git a/Makefile b/Makefile
index 87fb30a..b21d40c 100644
--- a/Makefile
+++ b/Makefile
@@ -460,6 +460,9 @@ PROGRAM_OBJS += http-backend.o
PROGRAM_OBJS += sh-i18n--envsubst.o
PROGRAM_OBJS += credential-store.o
+# Binary suffix used for Windows builds
+X =
+
PROGRAMS += $(patsubst %.o,git-%$X,$(PROGRAM_OBJS))
TEST_PROGRAMS_NEED_X += test-chmtime
--
1.7.8.4
^ permalink raw reply related
* Re: git-subtree Ready for Inspection
From: Jan Krüger @ 2012-02-08 12:35 UTC (permalink / raw)
To: David A. Greene; +Cc: git
In-Reply-To: <87r4y6ougl.fsf@smith.obbligato.org>
Hi Dave,
On 02/08/2012 04:49 AM, David A. Greene wrote:
> I've put up a branch containing git-subtree at:
>
> gitolite@sources.obbligato.org:git.git
A publicly accessible URL would be much more helpful. :-)
-Jan
^ permalink raw reply
* [PATCH] gitweb: Harden parse_commit and parse_commits
From: Jakub Narebski @ 2012-02-08 15:04 UTC (permalink / raw)
To: rajesh boyapati; +Cc: git
In-Reply-To: <201202071753.12436.jnareb@gmail.com>
On Tue, 7 Feb 2012, Jakub Narebski wrote:
> On Mon, 6 Feb 2012, rajesh boyapati wrote:
[...]
> > Then, I restarted gerrit server to take changes.
> > Now the error log of gerrit shows:
>
> > [2012-02-06 11:21:46,726] ERROR
> > com.google.gerrit.httpd.gitweb.GitWebServlet : CGI: fatal: bad revision
> > 'HEAD'
> > [2012-02-06 11:21:49,167] ERROR
> > com.google.gerrit.httpd.gitweb.GitWebServlet : CGI: [Mon Feb 6 11:21:49
> > 2012] gitweb.cgi: Use of uninitialized value $commit_id in open at
> > /usr/lib/cgi-bin/gitweb.cgi line 2817.
> > [2012-02-06 11:21:49,169] ERROR
> > com.google.gerrit.httpd.gitweb.GitWebServlet : CGI: fatal: bad revision ''
> [the same errors repeated few times]
>
> > <<<<<<<<<<<<<<<<
> > Previously, there is a error showing at line 4720. Now, with this patch,
> > that error has gone.
>
> As I said I was able to find a fix only for part of the issue.
> Unfortunately I was not able to reproduce this error in this form.
> Note that the error location doesn't help much, because it is more
> interesting for find which callers of parse_commits() pass undefined
> $commit_id.
>
> I can try to harden parse_commits() against bogus parameters; maybe
> this would help.
Does the following patch help, and does it fix the issue?
(Nb. you can try to simply change filename, and apply it with fuzz
against index.cgi file).
-- >8 -- ----- ----- ----- ----- ----- -- >8 --
From: Jakub Narebski <jnareb@gmail.com>
Subject: [PATCH] gitweb: Harden parse_commit and parse_commits
Gitweb has problems and gives errors when repository it shows is on
unborn branch (HEAD doesn't point to a valid commit), but there exist
other branches.
One of errors that shows in gitweb logs is undefined $commit_id in
parse_commits() subroutine. Therefore we harden both parse_commit()
and parse_commits() against undefined $commit_id, and against no
output from git-rev-list because HEAD doesn't point to a commit.
Reported-by: rajesh boyapati <boyapatisrajesh@gmail.com>
Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
gitweb/gitweb.perl | 7 ++++++-
1 files changed, 6 insertions(+), 1 deletions(-)
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index f9535eb..1181aeb 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -3334,6 +3334,8 @@ sub parse_commit {
my ($commit_id) = @_;
my %co;
+ return unless defined $commit_id;
+
local $/ = "\0";
open my $fd, "-|", git_cmd(), "rev-list",
@@ -3343,7 +3345,9 @@ sub parse_commit {
$commit_id,
"--",
or die_error(500, "Open git-rev-list failed");
- %co = parse_commit_text(<$fd>, 1);
+ my $commit_text = <$fd>;
+ %co = parse_commit_text($commit_text, 1)
+ if defined $commit_text;
close $fd;
return %co;
@@ -3353,6 +3357,7 @@ sub parse_commits {
my ($commit_id, $maxcount, $skip, $filename, @args) = @_;
my @cos;
+ return unless defined $commit_id;
$maxcount ||= 1;
$skip ||= 0;
--
1.7.9
^ permalink raw reply related
* Re: [PATCHv2] tag: add --points-at list option
From: Jeff King @ 2012-02-08 15:31 UTC (permalink / raw)
To: Tom Grennan; +Cc: Junio C Hamano, git, jasampler
In-Reply-To: <20120208014515.GE6264@tgrennan-laptop>
On Tue, Feb 07, 2012 at 05:45:15PM -0800, Tom Grennan wrote:
> > Also, it's not symmetric. What if I say "git tag
> > --points-at=my-lw-v1.7.9"? Then I would get your signed and
> > annotated tags (even though they're _not_ saying anything about
> > ny-lw-v1.7.9), and I would get v1.7.9 (even though it's not saying
> > anything about it either; in fact, it's the opposite!).
>
> Huh? As you noted, the lightweight tag is just an alternate reference,
> so why wouldn't want to see the annotated and signed tags of that common
> object?
>
> $ ./git-tag -l --points-at tomg-lw-v1.7.9
> tomg-annotate-v1.7.9
> tomg-signed-v1.7.9
> v1.7.9
> $ ./git-tag -l --points-at v1.7.9
> tomg-annotate-v1.7.9
> tomg-lw-v1.7.9
> tomg-signed-v1.7.9
Sorry, I should have been more clear here (the word symmetric isn't
right; it _is_ symmetric). My understanding of the point of your
original feature was to mention things that talk about a tag (because
you wanted to know what signatures were made around it).
With tag objects this is easy, because they contain a pointer. But when
it comes to lightweight tags, you cannot tell in which direction the
"talking about" occurred[1]. That is, a lightweight tag of another tag
is just creating a new ref, which looks the same as the old ref. So
something like --points-at cannot say "X talks about Y", because it
might as well have been "Y talks about X".
So I think you are better off to mention both X and Y (or to mention
neither).
> > Your documentation says "Only list annotated or signed tags of the
> > given object", which implies to me that --points-at is an arbitrary
> > object specifier, not a specific tagname.
>
> Yes, I changed that in the patch that I've prepared but will revert this
> if you'd rather not list these lightweight tags.
I'm OK with not mentioning lightweight tags. I just feel it should be
all-or-nothing. It was specifically the "given object" that I took issue
with, since in your examples v1.7.9 was treated differently from its
sha1.
> My reaction when I tested this was, "don't tell me what I already know."
> But consistency with $(git rev-parse ...) seems more important.
> And as you noted, a sha1_array would save code and to me, less code is
> always better.
Thanks. Either I've convinced you, or I've made you so sick of the
discussion that you're agreeing. The system works. :)
-Peff
[1] Actually, a tag object embeds the name of the ref under which it was
originally created (so the refs/tags/v1.7.9 tag has a "tag v1.7.9"
header in it). So in some cases, you _can_ determine the "original" ref
of a lightweight tag versus other refs made about it later. I'm still
not sure --points-at is a good place to try to make that distinction,
though.
^ permalink raw reply
* Re: [PATCHv3] tag: add --points-at list option
From: Jeff King @ 2012-02-08 15:44 UTC (permalink / raw)
To: Tom Grennan; +Cc: git, gitster, jasampler
In-Reply-To: <1328682076-23380-2-git-send-email-tmgrennan@gmail.com>
On Tue, Feb 07, 2012 at 10:21:16PM -0800, Tom Grennan wrote:
> +static const unsigned char *match_points_at(const unsigned char *sha1)
> +{
> + int i;
> + const unsigned char *tagged_sha1 = (unsigned char*)"";
> + struct object *obj = parse_object(sha1);
> +
> + if (obj && obj->type == OBJ_TAG)
> + tagged_sha1 = ((struct tag *)obj)->tagged->sha1;
This is not safe. A sha1 is not NUL-terminated, but is rather _always_
20 bytes. So when the object is not a tag, you do the hashcmp against
your single-byte string literal above, and we end up comparing whatever
garbage is in the data segment after the string literal.
What you want instead is the all-zeros sha1, like:
const unsigned char null_sha1[20] = { 0 };
Though we provide a null_sha1 global already. So doing:
const unsigned char *tagged_sha1 = null_sha1;
would be sufficient.
That being said, I don't know why you want to do both lookups in the
same loop of the points_at. If it's a lightweight tag and the tag
matches, you can get away with not parsing the object at all (although
to be fair, that is the minority case, so it is unlikely to matter).
Also, should we be producing an error if !obj? It would indicate a tag
that points to a bogus object.
> + for (i = 0; i < points_at.nr; i++)
> + if (!hashcmp(points_at.sha1[i], sha1))
> + return sha1;
> + else if (!hashcmp(points_at.sha1[i], tagged_sha1))
> + return tagged_sha1;
> + return NULL;
Why write your own linear search? sha1_array_lookup will do a binary
search for you.
Other than that, the patch looks OK to me.
-Peff
^ permalink raw reply
* Re: [PATCH 1/2] docs: add a basic description of the config API
From: Jeff King @ 2012-02-08 15:48 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Nguyen Thai Ngoc Duy, git
In-Reply-To: <7vmx8tzv3l.fsf@alter.siamese.dyndns.org>
On Tue, Feb 07, 2012 at 10:40:14PM -0800, Junio C Hamano wrote:
> What I was getting at is that we probably should officially declare that
> returning 1 to signal success is perfectly acceptable (and it probably
> should mean the caller who called the callback function as a sub callback
> should return immediately, taking it as a signal that the key has already
> been handled), as the primary purpose of this thread to discuss Peff's
> patch is to write these rules down.
>
> Of course, all that relies on the audit of the git_config() machinery. I
> think it is written to accept non-negative as success, and that is why I
> said "I too think this should be acceptable" in the first place.
I looked through it the other day when this came up, and I believe that
is the case. There is another related rule that should be considered,
though: should the return value from callbacks be propagated via
git_config to its caller?
It is the case already for config files that are read, but not for
config options parsed from the command line. It would not be too hard to
change, but I do not think any current callers care (as I mentioned
earlier, these days with the "void *" data pointer, sane callers will
simply pass in a pointer to a modifiable data area).
-Peff
^ permalink raw reply
* Re: [PATCH 1/2] docs: add a basic description of the config API
From: Jeff King @ 2012-02-08 15:59 UTC (permalink / raw)
To: Nguyen Thai Ngoc Duy; +Cc: Junio C Hamano, git
In-Reply-To: <CACsJy8B1cMBbrUznOp95u8YmfLf5bbzox8g9cuQUwgf-vY1XrQ@mail.gmail.com>
On Wed, Feb 08, 2012 at 01:55:30PM +0700, Nguyen Thai Ngoc Duy wrote:
> Or allow multiple callbacks from git_config(). The problem with is it
> adds another common set of config vars. Now it's common to chain var
> sets by doing
>
> int config_callback(...)
> {
> ....
> return yet_another_callback(...);
> }
>
> int main()
> {
> git_config(config_callback, ...)
> ...
>
> where yet_another_callback() hard codes another callback (usually
> git_default_config). This is inflexible. If we allow multiple
> callbacks, git_column_config() could be changed to return just 0 or -1
> (i.e. 1 remains unsuccessful error code).
I don't think we need multiple callbacks. You could convert any such
call of:
git_config_multiple(cb1, cb2, cb3, NULL, NULL);
into:
int combining_callback(const char *var, const char *value, void *data)
{
if (cb1(var, value, data) < 0)
return -1;
if (cb2(var, value, data) < 0)
return -1;
if (cb3(var, value, data) < 0)
return -1;
return 0;
}
But note that you get the same "data" pointer in each case. If you
wanted different data pointers, you would need more support from the
config machinery. However, I'd rather that be spelled:
git_config(cb1, data1);
git_config(cb2, data2);
git_config(cb3, data3);
and if the efficiency isn't acceptable, then looking into caching the
key/value pairs.
> On second thought, I think calling git_config() multiple times, each
> time with one callback, may work too. We may want to cache config
> content to avoid accessing files too many times though.
Exactly. I would do that first, and then worry about efficiency later if
it comes up. The first time I saw that we cached config multiple times
in a program run (several years ago), I had the same thought. But I
don't think the performance impact for reading the config 2 or 3 times
instead of 1 was even measurable, so I stopped looking into it.
If were to adopt something like the "automatically run this program to
get the config value" proposal that has been floating around (and I am
not saying we should), _then_ I think it might make sense to cache the
values, because the sub-program might be expensive to run.
-Peff
^ permalink raw reply
* Re: [StGit PATCH] Parse commit object header correctly
From: Michael Haggerty @ 2012-02-08 16:17 UTC (permalink / raw)
To: Frans Klaver
Cc: Junio C Hamano, Karl Hasselström, Catalin Marinas,
"Andy Green (林安廸)", git
In-Reply-To: <CAH6sp9P=vNjLycgzoWzRbeEsW-kQ5e4HgGYf2jP1+u9rtWV4dg@mail.gmail.com>
On 02/08/2012 11:43 AM, Frans Klaver wrote:
> On Wed, Feb 8, 2012 at 11:00 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
>> On 02/08/2012 08:33 AM, Junio C Hamano wrote:
>>> + line = lines[i].rstrip('\n')
>>> + ix = line.find(' ')
>>> + if 0 <= ix:
>>> + key, value = line[0:ix], line[ix+1:]
>>
>> The above five lines can be written
>>
>> for line in lines:
>> if ' ' in line:
>> key, value = line.rstrip('\n').split(' ', 1)
>>
>> or (if the lack of a space should be treated more like an exception)
>>
>> for line in lines:
>> try:
>> key, value = line.rstrip('\n').split(' ', 1)
>> except ValueError:
>> continue
>
> This is generally considered more pythonic: "It's easier to ask for
> forgiveness than to get permission".
Given that Junio explicitly wanted to allow lines with no spaces, I
assume that lack of a space is not an error but rather a conceivable
future extension. If my assumption is correct, then it is misleading
(and inefficient) to handle it via an exception.
>>> + if key == 'tree':
>>> + cd = cd.set_tree(repository.get_tree(value))
>>> + elif key == 'parent':
>>> + cd = cd.add_parent(repository.get_commit(value))
>>> + elif key == 'author':
>>> + cd = cd.set_author(Person.parse(value))
>>> + elif key == 'committer':
>>> + cd = cd.set_committer(Person.parse(value))
>>
>> All in all, I would recommend something like (untested):
>>
>> @return: A new L{CommitData} object
>> @rtype: L{CommitData}"""
>> cd = cls(parents = [])
>> lines = []
>> raw_lines = s.split('\n')
>> # Collapse multi-line header lines
>> for i, line in enumerate(raw_lines):
>> if not line:
>> cd.set_message('\n'.join(raw_lines[i+1:]))
>> break
>> if line.startswith(' '):
>> # continuation line
>> lines[-1] += '\n' + line[1:]
>> else:
>> lines.append(line)
>>
>> for line in lines:
>> if ' ' in line:
>> key, value = line.split(' ', 1)
>> if key == 'tree':
>> cd = cd.set_tree(repository.get_tree(value))
>> elif key == 'parent':
>> cd = cd.add_parent(repository.get_commit(value))
>> elif key == 'author':
>> cd = cd.set_author(Person.parse(value))
>> elif key == 'committer':
>> cd = cd.set_committer(Person.parse(value))
>> return cd
>
> One could also take the recommended python approach for
> switch/case-like if/elif/else statements:
>
> updater = { 'tree': lambda cd, value: cd.set_tree(repository.get_tree(value),
> 'parent': lambda cd, value:
> cd.add_parent(repository.get_commit(value)),
> 'author': lambda cd, value: cd.set_author(Person.parse(value)),
> 'committer': lambda cd, value:
> cd.set_committer(Person.parse(value))
> }
> for line in lines:
> try:
> key, value = line.split(' ', 1)
> cd = updater[key](cd, value)
> except ValueError:
> continue
> except KeyError:
> continue
>
> It documents about the same, but adds checking on double 'case'
> statements. The resulting for loop is rather cleaner and the exception
> approach becomes even more logical. I rather like the result, but I
> guess it's mostly a matter of taste.
I know this approach and use it frequently, but when one has to resort
to lambdas and there are only four cases, it becomes IMHO less readable
than the if..else version.
Michael
--
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/
^ permalink raw reply
* Re: [PATCH v2 0/4] Deprecate "not allow as-is commit with i-t-a entries"
From: Junio C Hamano @ 2012-02-08 17:34 UTC (permalink / raw)
To: Nguyen Thai Ngoc Duy; +Cc: git, Jonathan Nieder
In-Reply-To: <CACsJy8DSM3kPXJ4oYexCLs5qp6YdZ4Mf9RrGo78a0tHkRaTe4g@mail.gmail.com>
Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:
> 2012/2/7 Junio C Hamano <gitster@pobox.com>:
>> But when I said "let's admit that this is just fixing an UI mistake, no
>> configuration, no options", I really meant it. Without the backward
>> compatiblity "For now please do not fix this bug for me and keep being
>> buggy until I get used to the non-buggy behaviour" fuss, which we never do
>> to any bugfix.
>
> Ahh I missed something again. Your patch looks good too.
Ok, the strategy part is now behind us, but I have this slight suspicion
(I didn't look at the code nor tried it out myself---I don't have time to
do this myself today) that using this codepath might result in a corrupt
cache-tree, whose entries point at a section of the index it covers as a
subtree of the whole project but with incorrect counters or something like
that. It would be good to make sure this "just ignoring i-t-a" is doing
the right thing not to the resulting commit, but in the resulting index as
well, before we go forward with this change.
^ permalink raw reply
* Re: [PATCHv3] tag: add --points-at list option
From: Tom Grennan @ 2012-02-08 18:43 UTC (permalink / raw)
To: Jeff King; +Cc: git, gitster, jasampler
In-Reply-To: <20120208154442.GB8773@sigill.intra.peff.net>
On Wed, Feb 08, 2012 at 10:44:42AM -0500, Jeff King wrote:
>On Tue, Feb 07, 2012 at 10:21:16PM -0800, Tom Grennan wrote:
>
>> +static const unsigned char *match_points_at(const unsigned char *sha1)
>> +{
>> + int i;
>> + const unsigned char *tagged_sha1 = (unsigned char*)"";
>> + struct object *obj = parse_object(sha1);
>> +
>> + if (obj && obj->type == OBJ_TAG)
>> + tagged_sha1 = ((struct tag *)obj)->tagged->sha1;
>
>This is not safe. A sha1 is not NUL-terminated, but is rather _always_
>20 bytes. So when the object is not a tag, you do the hashcmp against
>your single-byte string literal above, and we end up comparing whatever
>garbage is in the data segment after the string literal.
Yikes! That was dumb.
>What you want instead is the all-zeros sha1, like:
>
> const unsigned char null_sha1[20] = { 0 };
>
>Though we provide a null_sha1 global already. So doing:
>
> const unsigned char *tagged_sha1 = null_sha1;
>
>would be sufficient.
Or just initialize at test tagged_sha1 with NULL.
static const unsigned char *match_points_at(const unsigned char *sha1)
{
int i;
const unsigned char *tagged_sha1 = NULL;
struct object *obj = parse_object(sha1);
if (obj && obj->type == OBJ_TAG)
tagged_sha1 = ((struct tag *)obj)->tagged->sha1;
for (i = 0; i < points_at.nr; i++)
if (!hashcmp(points_at.sha1[i], sha1))
return sha1;
else if (tagged_sha1 &&
!hashcmp(points_at.sha1[i], tagged_sha1))
return tagged_sha1;
return NULL;
}
>That being said, I don't know why you want to do both lookups in the
>same loop of the points_at. If it's a lightweight tag and the tag
>matches, you can get away with not parsing the object at all (although
>to be fair, that is the minority case, so it is unlikely to matter).
Yes, I think your saying that the lightweight search could go before the
tag object search like this.
static const unsigned char *match_points_at(const unsigned char *sha1)
{
const unsigned char *tagged_sha1 = NULL;
struct object *obj = parse_object(sha1);
if (sha1_array_lookup(&points_at, sha1) >= 0)
return sha1;
if (obj && 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;
}
>Also, should we be producing an error if !obj? It would indicate a tag
>that points to a bogus object.
I think the test of (obj) is redundant as this should be caught
by get_sha1() in parse_opt_points_at()
int parse_opt_points_at(const struct option *opt __attribute__ ((unused)),
const char *arg, int unset)
{
unsigned char sha1[20];
if (unset) {
sha1_array_clear(&points_at);
return 0;
}
if (!arg)
return error(_("switch 'points-at' requires an object"));
if (get_sha1(arg, sha1))
return error(_("malformed object name '%s'"), arg);
sha1_array_append(&points_at, sha1);
return 0;
}
>> + for (i = 0; i < points_at.nr; i++)
>> + if (!hashcmp(points_at.sha1[i], sha1))
>> + return sha1;
>> + else if (!hashcmp(points_at.sha1[i], tagged_sha1))
>> + return tagged_sha1;
>> + return NULL;
>
>Why write your own linear search? sha1_array_lookup will do a binary
>search for you.
Well, it's only a linear search of the points_at command arguments.
But by that reasoning, might as well do two sha1_array_lookups like
above and save some code b/c "less code is always better"(TM).
>Other than that, the patch looks OK to me.
Thanks, I'll send what I hope to be the final version later today.
--
TomG
^ permalink raw reply
* Re: [PATCH] Fix build problems related to profile-directed optimization
From: Ted Ts'o @ 2012-02-08 18:53 UTC (permalink / raw)
To: Junio C Hamano, git
In-Reply-To: <1328508017-7277-1-git-send-email-tytso@mit.edu>
Junio, any comments on my most recent spin of this patch? Any changes
you'd like to see?
Thanks,
- Ted
^ permalink raw reply
* Re: [PATCHv3] tag: add --points-at list option
From: Jeff King @ 2012-02-08 18:57 UTC (permalink / raw)
To: Tom Grennan; +Cc: git, gitster, jasampler
In-Reply-To: <20120208184332.GF6264@tgrennan-laptop>
On Wed, Feb 08, 2012 at 10:43:32AM -0800, Tom Grennan wrote:
> >Though we provide a null_sha1 global already. So doing:
> >
> > const unsigned char *tagged_sha1 = null_sha1;
> >
> >would be sufficient.
>
> Or just initialize at test tagged_sha1 with NULL.
Oh yeah, that is even better.
> >That being said, I don't know why you want to do both lookups in the
> >same loop of the points_at. If it's a lightweight tag and the tag
> >matches, you can get away with not parsing the object at all (although
> >to be fair, that is the minority case, so it is unlikely to matter).
>
> Yes, I think your saying that the lightweight search could go before the
> tag object search like this.
Exactly, though:
> static const unsigned char *match_points_at(const unsigned char *sha1)
> {
> const unsigned char *tagged_sha1 = NULL;
> struct object *obj = parse_object(sha1);
>
> if (sha1_array_lookup(&points_at, sha1) >= 0)
> return sha1;
> if (obj && obj->type == OBJ_TAG)
> tagged_sha1 = ((struct tag *)obj)->tagged->sha1;
You can delay the relatively expensive parse_object until you find the
results of the first lookup (though like I said earlier, it is unlikely to
matter, as it only helps in the positive-match case. Out of N tags, you
will likely end up parsing N-1 of them anyway).
> >Also, should we be producing an error if !obj? It would indicate a tag
> >that points to a bogus object.
>
> I think the test of (obj) is redundant as this should be caught
> by get_sha1() in parse_opt_points_at()
No, it's not redundant. get_sha1 is purely about looking up the name and
finding a sha1. parse_object is about looking up the object represented
by that sha1 in the object db. get_sha1 can sometimes involve parsing
objects (e.g., looking for "foo^1" will need to parse the commit object
at "foo"), but does not have to.
Besides which, you are not calling parse_object on the sha1 from
--points-at, but rather the sha1 for each tag ref given to us by
for_each_tag_ref.
> >Why write your own linear search? sha1_array_lookup will do a binary
> >search for you.
>
> Well, it's only a linear search of the points_at command arguments.
> But by that reasoning, might as well do two sha1_array_lookups like
> above and save some code b/c "less code is always better"(TM).
Right. I expect the N to be small in this case, so I doubt it matters.
But two sha1_array_lookups is still asymptotically smaller, because the
expensive operation is hashcmp(). So two binary searches is O(2*lg n),
whereas a linear walk with 2 hashcmps per item is O(2*n).
> >Other than that, the patch looks OK to me.
>
> Thanks, I'll send what I hope to be the final version later today.
Thanks for working on this and being so responsive to review.
-Peff
^ permalink raw reply
* Re: [PATCHv3] tag: add --points-at list option
From: Tom Grennan @ 2012-02-08 18:58 UTC (permalink / raw)
To: Jeff King; +Cc: git, gitster, jasampler
In-Reply-To: <20120208184332.GF6264@tgrennan-laptop>
On Wed, Feb 08, 2012 at 10:43:32AM -0800, Tom Grennan wrote:
>On Wed, Feb 08, 2012 at 10:44:42AM -0500, Jeff King wrote:
>>On Tue, Feb 07, 2012 at 10:21:16PM -0800, Tom Grennan wrote:
>>
>>Also, should we be producing an error if !obj? It would indicate a tag
>>that points to a bogus object.
>
>I think the test of (obj) is redundant as this should be caught
>by get_sha1() in parse_opt_points_at()
I'm wrong. That tests the sha of the point-at argument, not the
sha/objects of the refs/tags entry. I'll add...
if (!obj)
die(_("invalid tag, 'refs/tags/%s'"), refname);
--
TomG
^ permalink raw reply
* [PATCH] column: Fix some compiler and sparse warnings
From: Ramsay Jones @ 2012-02-08 19:13 UTC (permalink / raw)
To: Nguyen Thai Ngoc Duy; +Cc: Junio C Hamano, GIT Mailing-list
Some versions of gcc complain as follows:
CC column.o
column.c: In function `git_config_column':
column.c:313: warning: 'set' might be used uninitialized in \
this function
The 'set' variable is not in fact used uninitialised, but in order to
suppress the warning, we rework the code slightly to ensure gcc does
not mis-diagnose the variable usage.
Also, sparse complains as follows:
SP pager.c
pager.c:134:5: warning: symbol 'term_columns' was not declared. \
Should it be static?
In order to fix the warning, we add an include of the column.h header,
which contains an appropriate extern declaration of term_columns().
Signed-off-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>
---
Hi Nguyen,
If you need to re-roll your "nd/columns" branch, could you please squash
this patch (or some variant) into it. Thanks!
[I haven't checked, but I'm guessing that the pager.c change would be
squashed into commit cb0850f (Save terminal width before setting up pager,
04-02-2012), whereas the column.c change would be squashed into commit
ac21f2b (Add git-column and column mode parsing, 04-02-2012)]
ATB,
Ramsay Jones
column.c | 6 +++---
| 1 +
2 files changed, 4 insertions(+), 3 deletions(-)
diff --git a/column.c b/column.c
index f001021..98328cf 100644
--- a/column.c
+++ b/column.c
@@ -310,16 +310,16 @@ static int parse_option(const char *arg, int len,
{ OPTION, "color", COL_ANSI },
{ OPTION, "dense", COL_DENSE },
};
- int i, set, name_len;
+ int i;
for (i = 0; i < ARRAY_SIZE(opts); i++) {
+ int set = 1, name_len;
+
if (opts[i].type == OPTION) {
if (len > 2 && !strncmp(arg, "no", 2)) {
arg += 2;
len -= 2;
set = 0;
- } else {
- set = 1;
}
}
--git a/pager.c b/pager.c
index 37d554d..fe203a7 100644
--- a/pager.c
+++ b/pager.c
@@ -1,6 +1,7 @@
#include "cache.h"
#include "run-command.h"
#include "sigchain.h"
+#include "column.h"
#ifndef DEFAULT_PAGER
#define DEFAULT_PAGER "less"
--
1.7.9
^ permalink raw reply related
* Re: [StGit PATCH] Parse commit object header correctly
From: Frans Klaver @ 2012-02-08 20:04 UTC (permalink / raw)
To: Michael Haggerty
Cc: Junio C Hamano, Karl Hasselström, Catalin Marinas,
Andy Green (林安廸), git
In-Reply-To: <4F32A014.1000608@alum.mit.edu>
On Wed, 08 Feb 2012 17:17:24 +0100, Michael Haggerty
<mhagger@alum.mit.edu> wrote:
> On 02/08/2012 11:43 AM, Frans Klaver wrote:
>> On Wed, Feb 8, 2012 at 11:00 AM, Michael Haggerty
>> <mhagger@alum.mit.edu> wrote:
>>> On 02/08/2012 08:33 AM, Junio C Hamano wrote:
>>>> + line = lines[i].rstrip('\n')
>>>> + ix = line.find(' ')
>>>> + if 0 <= ix:
>>>> + key, value = line[0:ix], line[ix+1:]
>>>
>>> The above five lines can be written
>>>
>>> for line in lines:
>>> if ' ' in line:
>>> key, value = line.rstrip('\n').split(' ', 1)
>>>
>>> or (if the lack of a space should be treated more like an exception)
>>>
>>> for line in lines:
>>> try:
>>> key, value = line.rstrip('\n').split(' ', 1)
>>> except ValueError:
>>> continue
>>
>> This is generally considered more pythonic: "It's easier to ask for
>> forgiveness than to get permission".
>
> Given that Junio explicitly wanted to allow lines with no spaces, I
> assume that lack of a space is not an error but rather a conceivable
> future extension. If my assumption is correct, then it is misleading
> (and inefficient) to handle it via an exception.
I find the documenting more convincing than the efficiency, but from the
phrasing I think you do too.
>>> for line in lines:
>>> if ' ' in line:
>>> key, value = line.split(' ', 1)
>>> if key == 'tree':
>>> cd = cd.set_tree(repository.get_tree(value))
>>> elif key == 'parent':
>>> cd = cd.add_parent(repository.get_commit(value))
>>> elif key == 'author':
>>> cd = cd.set_author(Person.parse(value))
>>> elif key == 'committer':
>>> cd = cd.set_committer(Person.parse(value))
>>> return cd
>>
>> One could also take the recommended python approach for
>> switch/case-like if/elif/else statements:
>>
>> updater = { 'tree': lambda cd, value:
>> cd.set_tree(repository.get_tree(value),
>> 'parent': lambda cd, value:
>> cd.add_parent(repository.get_commit(value)),
>> 'author': lambda cd, value:
>> cd.set_author(Person.parse(value)),
>> 'committer': lambda cd, value:
>> cd.set_committer(Person.parse(value))
>> }
>> for line in lines:
>> try:
>> key, value = line.split(' ', 1)
>> cd = updater[key](cd, value)
>> except ValueError:
>> continue
>> except KeyError:
>> continue
>>
>> It documents about the same, but adds checking on double 'case'
>> statements. The resulting for loop is rather cleaner and the exception
>> approach becomes even more logical. I rather like the result, but I
>> guess it's mostly a matter of taste.
>
> I know this approach and use it frequently, but when one has to resort
> to lambdas and there are only four cases, it becomes IMHO less readable
> than the if..else version.
Well, as I said, its largely a matter of taste; four items is a corner
case to me when thinking maintainability vs. readability. On the other
hand, this doesn't seem like an oft-changing piece of code, so a longer
list of if..elif..else shouldn't be a problem either.
Frans
^ permalink raw reply
* [PATCHv4] tag: add --points-at list option
From: Tom Grennan @ 2012-02-08 20:12 UTC (permalink / raw)
To: git; +Cc: gitster, peff, jasampler
In-Reply-To: <20120208185750.GA22220@sigill.intra.peff.net>
Please see the following patch that I hoped is the last version of the
"points-at" feature. Thank you for your patience.
Tom Grennan (1):
tag: add --points-at list option
Documentation/git-tag.txt | 5 +++-
builtin/tag.c | 52 ++++++++++++++++++++++++++++++++++++++++++--
2 files changed, 53 insertions(+), 4 deletions(-)
--
1.7.8
^ permalink raw reply
* [PATCHv4] tag: add --points-at list option
From: Tom Grennan @ 2012-02-08 20:12 UTC (permalink / raw)
To: git; +Cc: gitster, peff, jasampler
In-Reply-To: <20120208185750.GA22220@sigill.intra.peff.net>
This filters the list for tags of the given object.
Example,
john$ git tag v1.0-john v1.0
john$ git tag -l --points-at v1.0
v1.0-john
Signed-off-by: Tom Grennan <tmgrennan@gmail.com>
---
Documentation/git-tag.txt | 5 +++-
builtin/tag.c | 52 ++++++++++++++++++++++++++++++++++++++++++--
2 files changed, 53 insertions(+), 4 deletions(-)
diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
index 5ead91e..124ed36 100644
--- a/Documentation/git-tag.txt
+++ b/Documentation/git-tag.txt
@@ -12,7 +12,7 @@ SYNOPSIS
'git tag' [-a | -s | -u <key-id>] [-f] [-m <msg> | -F <file>]
<tagname> [<commit> | <object>]
'git tag' -d <tagname>...
-'git tag' [-n[<num>]] -l [--contains <commit>]
+'git tag' [-n[<num>]] -l [--contains <commit>] [--points-at <object>]
[--column[=<options>] | --no-column] [<pattern>...]
'git tag' -v <tagname>...
@@ -95,6 +95,9 @@ This option is only applicable when listing tags without annotation lines.
--contains <commit>::
Only list tags which contain the specified commit.
+--points-at <object>::
+ Only list tags of the given object.
+
-m <msg>::
--message=<msg>::
Use the given tag message (instead of prompting).
diff --git a/builtin/tag.c b/builtin/tag.c
index 5fbd62c..f3051c7 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -16,11 +16,13 @@
#include "revision.h"
#include "gpg-interface.h"
#include "column.h"
+#include "sha1-array.h"
static const char * const git_tag_usage[] = {
"git tag [-a|-s|-u <key-id>] [-f] [-m <msg>|-F <file>] <tagname> [<head>]",
"git tag -d <tagname>...",
- "git tag -l [-n[<num>]] [<pattern>...]",
+ "git tag -l [-n[<num>]] [--contains <commit>] [--points-at <object>] \\"
+ "\n\t\t[<pattern>...]",
"git tag -v <tagname>...",
NULL
};
@@ -31,6 +33,7 @@ struct tag_filter {
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)
@@ -44,6 +47,24 @@ static int match_pattern(const char **patterns, const char *ref)
return 0;
}
+static const unsigned char *match_points_at(const char *refname,
+ const unsigned char *sha1)
+{
+ 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)
@@ -141,6 +162,9 @@ static int show_reference(const char *refname, const unsigned char *sha1,
return 0;
}
+ if (points_at.nr && !match_points_at(refname, sha1))
+ return 0;
+
if (!filter->lines) {
printf("%s\n", refname);
return 0;
@@ -389,6 +413,23 @@ static int strbuf_check_tag_ref(struct strbuf *sb, const char *name)
return check_refname_format(sb->buf, 0);
}
+int parse_opt_points_at(const struct option *opt __attribute__ ((unused)),
+ const char *arg, int unset)
+{
+ unsigned char sha1[20];
+
+ if (unset) {
+ sha1_array_clear(&points_at);
+ return 0;
+ }
+ if (!arg)
+ return error(_("switch 'points-at' requires an object"));
+ if (get_sha1(arg, sha1))
+ return error(_("malformed object name '%s'"), arg);
+ sha1_array_append(&points_at, sha1);
+ return 0;
+}
+
int cmd_tag(int argc, const char **argv, const char *prefix)
{
struct strbuf buf = STRBUF_INIT;
@@ -432,6 +473,10 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
PARSE_OPT_LASTARG_DEFAULT,
parse_opt_with_commit, (intptr_t)"HEAD",
},
+ {
+ OPTION_CALLBACK, 0, "points-at", NULL, "object",
+ "print only tags of the object", 0, parse_opt_points_at
+ },
OPT_END()
};
@@ -478,8 +523,9 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
}
if (lines != -1)
die(_("-n option is only allowed with -l."));
- if (with_commit)
- die(_("--contains option is only allowed with -l."));
+ if (with_commit || points_at.nr)
+ die(_("--contains and --points-at options "
+ "are only allowed with -l."));
if (delete)
return for_each_tag_name(argv, delete_tag);
if (verify)
--
1.7.8
^ permalink raw reply related
* Re: [PATCHv4] tag: add --points-at list option
From: Jeff King @ 2012-02-08 20:58 UTC (permalink / raw)
To: Tom Grennan; +Cc: git, gitster, jasampler
In-Reply-To: <1328731972-13137-2-git-send-email-tmgrennan@gmail.com>
On Wed, Feb 08, 2012 at 12:12:52PM -0800, Tom Grennan wrote:
> This filters the list for tags of the given object.
> Example,
>
> john$ git tag v1.0-john v1.0
> john$ git tag -l --points-at v1.0
> v1.0-john
And probably "v1.0", as well, in this iteration. :)
The patch content itself looks good to me, except:
> --- a/Documentation/git-tag.txt
> +++ b/Documentation/git-tag.txt
> @@ -12,7 +12,7 @@ SYNOPSIS
> 'git tag' [-a | -s | -u <key-id>] [-f] [-m <msg> | -F <file>]
> <tagname> [<commit> | <object>]
> 'git tag' -d <tagname>...
> -'git tag' [-n[<num>]] -l [--contains <commit>]
> +'git tag' [-n[<num>]] -l [--contains <commit>] [--points-at <object>]
> [--column[=<options>] | --no-column] [<pattern>...]
What's this "column" stuff doing here? The nd/columns topic is still in
"next", isn't it? Did you base this on "next" or "pu"?
Usually topics should be based on master, so they can graduate
independently of each other. In this case, it might make sense to build
on top of jk/maint-tag-show-fixes (d0548a3), but I don't think that is
even necessary here (my fixes ended up not being too closely related, I
think).
Other than that, I think the patch is fine. There are no tests, so
perhaps these should be squashed in:
diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index e93ac73..f61e398 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -1269,4 +1269,43 @@ test_expect_success 'mixing incompatibles modes and options is forbidden' '
test_must_fail git tag -v -s
'
+# check points-at
+
+test_expect_success '--points-at cannot be used in non-list mode' '
+ test_must_fail git tag --points-at=v4.0 foo
+'
+
+test_expect_success '--points-at finds lightweight tags' '
+ echo v4.0 >expect &&
+ git tag --points-at v4.0 >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success '--points-at finds annotated tags of commits' '
+ git tag -m "v4.0, annotated" annotated-v4.0 v4.0 &&
+ echo annotated-v4.0 >expect &&
+ git tag -l --points-at v4.0 "annotated*" >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success '--points-at finds annotated tags of tags' '
+ git tag -m "describing the v4.0 tag object" \
+ annotated-again-v4.0 annotated-v4.0 &&
+ cat >expect <<-\EOF &&
+ annotated-again-v4.0
+ annotated-v4.0
+ EOF
+ git tag --points-at=annotated-v4.0 >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'multiple --points-at are OR-ed together' '
+ cat >expect <<-\EOF &&
+ v2.0
+ v3.0
+ EOF
+ git tag --points-at=v2.0 --points-at=v3.0 >actual &&
+ test_cmp expect actual
+'
+
test_done
--
1.7.9.rc2.14.g3da2b
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox