* Re: [PATCHv4 6/6] git-send-email: use git credential to obtain password
From: Michal Nazarewicz @ 2013-02-27 16:09 UTC (permalink / raw)
To: Junio C Hamano, Matthieu Moy; +Cc: peff, git
In-Reply-To: <7vehg1kb09.fsf@alter.siamese.dyndns.org>
[-- Attachment #1: Type: text/plain, Size: 3715 bytes --]
On Wed, Feb 27 2013, Junio C Hamano <gitster@pobox.com> wrote:
> Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
>
>> Michal Nazarewicz <mpn@google.com> writes:
>>
>>> + $auth = Git::credential({
>>> + 'protocol' => 'smtp',
>>> + 'host' => join(':', $smtp_server, $smtp_server_port),
>>
>> At this point, $smtp_server_port is not always defined. I just tested
>> and got
>>
>> Use of uninitialized value $smtp_server_port in join or string at
>> git-send-email line 1077.
>>
>> Other than that, the whole series looks good.
>
> Given that there is another place that conditionally append ":$port"
> to the host string, I think we should follow suit here. Perhaps
> like the attached diff?
Damn meetings, you beat me to it… I was just about to send a patch. ;)
> Thanks for a review.
>
>
> git-send-email.perl | 14 ++++++++++----
> 1 file changed, 10 insertions(+), 4 deletions(-)
>
> diff --git a/git-send-email.perl b/git-send-email.perl
> index 76bbfc3..c3501d9 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -1045,6 +1045,14 @@ sub maildomain {
> return maildomain_net() || maildomain_mta() || 'localhost.localdomain';
> }
>
> +sub smtp_host_string {
> + if (defined $smtp_server_port) {
> + return "$smtp_server:$smtp_server_port";
> + } else {
> + return $smtp_server;
> + }
> +}
> +
> # Returns 1 if authentication succeeded or was not necessary
> # (smtp_user was not specified), and 0 otherwise.
>
> @@ -1065,7 +1073,7 @@ sub smtp_auth_maybe {
> # reject credentials.
> $auth = Git::credential({
> 'protocol' => 'smtp',
> - 'host' => join(':', $smtp_server, $smtp_server_port),
> + 'host' => smtp_host_string(),
> 'username' => $smtp_authuser,
> # if there's no password, "git credential fill" will
> # give us one, otherwise it'll just pass this one.
> @@ -1188,9 +1196,7 @@ sub send_message {
> else {
> require Net::SMTP;
> $smtp_domain ||= maildomain();
> - $smtp ||= Net::SMTP->new((defined $smtp_server_port)
> - ? "$smtp_server:$smtp_server_port"
> - : $smtp_server,
> + $smtp ||= Net::SMTP->new(smtp_host_string(),
> Hello => $smtp_domain,
> Debug => $debug_net_smtp);
> if ($smtp_encryption eq 'tls' && $smtp) {
>From reading of SMTP.pm, it seems that this could be changed to:
- $smtp ||= Net::SMTP->new((defined $smtp_server_port)
- ? "$smtp_server:$smtp_server_port"
- : $smtp_server,
+ $smtp ||= Net::SMTP->new($smtp_server,
+ Port => $smtp_server_port,
and than the other part would become:
@@ -1060,12 +1060,17 @@ sub smtp_auth_maybe {
Authen::SASL->import(qw(Perl));
};
+ my $host = $smtp_server;
+ if (defined $smtp_server_port) {
+ $host .= ':' . $smtp_server_port;
+ }
+
# TODO: Authentication may fail not because credentials were
# invalid but due to other reasons, in which we should not
# reject credentials.
$auth = Git::credential({
'protocol' => 'smtp',
- 'host' => join(':', $smtp_server, $smtp_server_port),
+ 'host' => $host,
'username' => $smtp_authuser,
# if there's no password, "git credential fill" will
# give us one, otherwise it'll just pass this one.
Either way, looks good to me.
--
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of o' \,=./ `o
..o | Computer Science, Michał “mina86” Nazarewicz (o o)
ooo +----<email/xmpp: mpn@google.com>--------------ooO--(_)--Ooo--
[-- Attachment #2.1: Type: text/plain, Size: 0 bytes --]
[-- Attachment #2.2: Type: application/pgp-signature, Size: 835 bytes --]
^ permalink raw reply
* Re: [PATCHv4 6/6] git-send-email: use git credential to obtain password
From: Matthieu Moy @ 2013-02-27 16:13 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Michal Nazarewicz, peff, git
In-Reply-To: <7vehg1kb09.fsf@alter.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> writes:
> diff --git a/git-send-email.perl b/git-send-email.perl
> index 76bbfc3..c3501d9 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -1045,6 +1045,14 @@ sub maildomain {
> return maildomain_net() || maildomain_mta() || 'localhost.localdomain';
> }
>
> +sub smtp_host_string {
> + if (defined $smtp_server_port) {
> + return "$smtp_server:$smtp_server_port";
> + } else {
> + return $smtp_server;
> + }
> +}
> +
> # Returns 1 if authentication succeeded or was not necessary
> # (smtp_user was not specified), and 0 otherwise.
>
> @@ -1065,7 +1073,7 @@ sub smtp_auth_maybe {
> # reject credentials.
> $auth = Git::credential({
> 'protocol' => 'smtp',
> - 'host' => join(':', $smtp_server, $smtp_server_port),
> + 'host' => smtp_host_string(),
> 'username' => $smtp_authuser,
> # if there's no password, "git credential fill" will
> # give us one, otherwise it'll just pass this one.
> @@ -1188,9 +1196,7 @@ sub send_message {
> else {
> require Net::SMTP;
> $smtp_domain ||= maildomain();
> - $smtp ||= Net::SMTP->new((defined $smtp_server_port)
> - ? "$smtp_server:$smtp_server_port"
> - : $smtp_server,
> + $smtp ||= Net::SMTP->new(smtp_host_string(),
> Hello => $smtp_domain,
> Debug => $debug_net_smtp);
> if ($smtp_encryption eq 'tls' && $smtp) {
Seems obviously correct. I also did a basic test and it worked smoothly.
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
^ permalink raw reply
* Re: Git Notes - Search Functionality
From: Johan Herland @ 2013-02-27 15:44 UTC (permalink / raw)
To: krishna chaitanya kurnala; +Cc: git
In-Reply-To: <CAFZnXLJCiq4G+_ZStWmjAGePWCD6mhske7Y4=oe2h==F0BYVLQ@mail.gmail.com>
On Wed, Feb 27, 2013 at 4:34 PM, krishna chaitanya kurnala
<kkc4al@gmail.com> wrote:
> Hello All
>
> I working on Git Notes. I want to know if there is an easy way to
> obtain a list of all "namespaces"(For eg., git notes --ref=namespace
> ... ) with notes objects in a specific git repository.
The notes namespaces are stored as regular refs under refs/notes/*.
Here's one way to list the available namespaces:
git for-each-ref refs/notes/
Hope this helps,
...Johan
> We can easily
> create, edit, merge git notes with commands if we know the namespaces
> and/or the sha. But, for example, Has anyone tried to search for a
> string in a git notes objects for that project etc?
> The closest i can think of is using some options with git logs, for
> example, git log --show-notes=* --format="%H %N" etc.
>
> Appreciate your time.
>
> thanks
> Krishna Chaitanya
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Johan Herland, <johan@herland.net>
www.herland.net
^ permalink raw reply
* Re: [PATCHv4 6/6] git-send-email: use git credential to obtain password
From: Junio C Hamano @ 2013-02-27 16:29 UTC (permalink / raw)
To: Matthieu Moy; +Cc: Michal Nazarewicz, peff, git
In-Reply-To: <vpqy5e9zqe9.fsf@grenoble-inp.fr>
Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> diff --git a/git-send-email.perl b/git-send-email.perl
>> index 76bbfc3..c3501d9 100755
>> --- a/git-send-email.perl
>> +++ b/git-send-email.perl
>> @@ -1045,6 +1045,14 @@ sub maildomain {
>> return maildomain_net() || maildomain_mta() || 'localhost.localdomain';
>> }
>>
>> +sub smtp_host_string {
>> + if (defined $smtp_server_port) {
>> + return "$smtp_server:$smtp_server_port";
>> + } else {
>> + return $smtp_server;
>> + }
>> +}
>> +
>> # Returns 1 if authentication succeeded or was not necessary
>> # (smtp_user was not specified), and 0 otherwise.
>>
>> @@ -1065,7 +1073,7 @@ sub smtp_auth_maybe {
>> # reject credentials.
>> $auth = Git::credential({
>> 'protocol' => 'smtp',
>> - 'host' => join(':', $smtp_server, $smtp_server_port),
>> + 'host' => smtp_host_string(),
>> 'username' => $smtp_authuser,
>> # if there's no password, "git credential fill" will
>> # give us one, otherwise it'll just pass this one.
>> @@ -1188,9 +1196,7 @@ sub send_message {
>> else {
>> require Net::SMTP;
>> $smtp_domain ||= maildomain();
>> - $smtp ||= Net::SMTP->new((defined $smtp_server_port)
>> - ? "$smtp_server:$smtp_server_port"
>> - : $smtp_server,
>> + $smtp ||= Net::SMTP->new(smtp_host_string(),
>> Hello => $smtp_domain,
>> Debug => $debug_net_smtp);
>> if ($smtp_encryption eq 'tls' && $smtp) {
>
> Seems obviously correct. I also did a basic test and it worked smoothly.
OK, I'll squash it in.
Thanks.
^ permalink raw reply
* Re: [PATCH] name-hash.c: fix endless loop with core.ignorecase=true
From: Junio C Hamano @ 2013-02-27 16:53 UTC (permalink / raw)
To: Karsten Blees
Cc: Jeff King, Duy Nguyen, kusmabite, Ramkumar Ramachandra,
Robert Zeh, Git List, finnag
In-Reply-To: <512E1C0F.3050506@gmail.com>
Karsten Blees <karsten.blees@gmail.com> writes:
> With core.ignorecase=true, name-hash.c builds a case insensitive index of
> all tracked directories. Currently, the existing cache entry structures are
> added multiple times to the same hashtable (with different name lengths and
> hash codes). However, there's only one dir_next pointer, which gets
> completely messed up in case of hash collisions. In the worst case, this
> causes an endless loop if ce == ce->dir_next:
>
> ---8<---
> # "V/", "V/XQANY/" and "WURZAUP/" all have the same hash_name
> mkdir V
> mkdir V/XQANY
> mkdir WURZAUP
> touch V/XQANY/test
> git init
> git config core.ignorecase true
> git add .
> git status
> ---8<---
Instead of using the scissors mark to confuse "am -c", indenting
this block would have been easier to later readers.
Also it is somewhat a shame that we do not use the above sample
collisions in a new test case.
> +static struct dir_entry *hash_dir_entry(struct index_state *istate,
> + struct cache_entry *ce, int namelen, int add)
> {
> /*
> * Throw each directory component in the hash for quick lookup
> * during a git status. Directory components are stored with their
> - * closing slash. Despite submodules being a directory, they never
> - * reach this point, because they are stored without a closing slash
> - * in the cache.
Is the description of submodule no longer relevant?
> - * Note that the cache_entry stored with the directory does not
> - * represent the directory itself. It is a pointer to an existing
> - * filename, and its only purpose is to represent existence of the
> - * directory in the cache. It is very possible multiple directory
> - * hash entries may point to the same cache_entry.
Is this paragraph no longer relevant? It seems to me that it still
holds true, given the way how dir->ce points at the given ce.
> + * closing slash.
> */
> + struct dir_entry *dir, *p;
> +
> + /* get length of parent directory */
> + while (namelen > 0 && !is_dir_sep(ce->name[namelen - 1]))
> + namelen--;
> + if (namelen <= 0)
> + return NULL;
> +
> + /* lookup existing entry for that directory */
> + dir = find_dir_entry(istate, ce->name, namelen);
> + if (add && !dir) {
> ...
> }
> +
> + /* add or release reference to this entry (and parents if 0) */
> + p = dir;
> + if (add) {
> + while (p && !(p->nr++))
> + p = p->parent;
> + } else {
> + while (p && p->nr && !(--p->nr))
> + p = p->parent;
> + }
Can we free the entry when its refcnt goes down to zero? If yes, is
it worth doing so?
> +
> + return dir;
> }
>
> static void hash_index_entry(struct index_state *istate, struct cache_entry *ce)
> @@ -74,7 +111,7 @@ static void hash_index_entry(struct index_state *istate, struct cache_entry *ce)
> if (ce->ce_flags & CE_HASHED)
> return;
> ce->ce_flags |= CE_HASHED;
> - ce->next = ce->dir_next = NULL;
> + ce->next = NULL;
> hash = hash_name(ce->name, ce_namelen(ce));
> pos = insert_hash(hash, ce, &istate->name_hash);
> if (pos) {
> @@ -82,8 +119,8 @@ static void hash_index_entry(struct index_state *istate, struct cache_entry *ce)
> *pos = ce;
> }
>
> - if (ignore_case)
> - hash_index_entry_directories(istate, ce);
> + if (ignore_case && !(ce->ce_flags & CE_UNHASHED))
> + hash_dir_entry(istate, ce, ce_namelen(ce), 1);
> }
>
> static void lazy_init_name_hash(struct index_state *istate)
> @@ -99,11 +136,33 @@ static void lazy_init_name_hash(struct index_state *istate)
>
> void add_name_hash(struct index_state *istate, struct cache_entry *ce)
> {
> + /* if already hashed, add reference to directory entries */
> + if (ignore_case && (ce->ce_flags & CE_STATE_MASK) == CE_STATE_MASK)
> + hash_dir_entry(istate, ce, ce_namelen(ce), 1);
Instead of a single function with "are we adding or removing?"
parameter, it would be a lot easier to read the callers if they are
wrapped in two helpers, add_dir_entry() and del_dir_entry() or
something, especially when the add=[0|1] parameter is constant for
each and every callsite (i.e. the codeflow determines it, not the
data).
> ce->ce_flags &= ~CE_UNHASHED;
> if (istate->name_hash_initialized)
> hash_index_entry(istate, ce);
> }
>
> +/*
> + * We don't actually *remove* it, we can just mark it invalid so that
> + * we won't find it in lookups.
> + *
> + * Not only would we have to search the lists (simple enough), but
> + * we'd also have to rehash other hash buckets in case this makes the
> + * hash bucket empty (common). So it's much better to just mark
> + * it.
> + */
> +void remove_name_hash(struct index_state *istate, struct cache_entry *ce)
> +{
> + /* if already hashed, release reference to directory entries */
> + if (ignore_case && (ce->ce_flags & CE_STATE_MASK) == CE_HASHED)
> + hash_dir_entry(istate, ce, ce_namelen(ce), 0);
And here as well.
> +
> + ce->ce_flags |= CE_UNHASHED;
> +}
> +
> static int slow_same_name(const char *name1, int len1, const char *name2, int len2)
> {
> if (len1 != len2)
> @@ -137,18 +196,7 @@ static int same_name(const struct cache_entry *ce, const char *name, int namelen
> if (!icase)
> return 0;
>
> - /*
> - * If the entry we're comparing is a filename (no trailing slash), then compare
> - * the lengths exactly.
> - */
> - if (name[namelen - 1] != '/')
> - return slow_same_name(name, namelen, ce->name, len);
> -
> - /*
> - * For a directory, we point to an arbitrary cache_entry filename. Just
> - * make sure the directory portion matches.
> - */
> - return slow_same_name(name, namelen, ce->name, namelen < len ? namelen : len);
> + return slow_same_name(name, namelen, ce->name, len);
Hmph, what is this change about? Nobody calls same_name() with a
directory name anymore or something?
Thanks.
^ permalink raw reply
* ESR's reposurgeon action stamps
From: Joel C. Salomon @ 2013-02-27 20:13 UTC (permalink / raw)
To: Git Mailing List; +Cc: Eric S. Raymond
Since it's intended as a cross-VCS tool, Eric Raymond’s reposurgeon
does not use hashes to refer to commits, but instead uses “action
stamps”. From <http://www.catb.org/~esr/reposurgeon/reposurgeon.html#reference-lifting>:
> An action stamp is an RFC3339 timestamp, followed by a '!', followed
> by a committer email address, followed by ':' and a 1-origin sequence
> number if the date/time pair is not unique in the repository; it
> refers to a commit without being VCS-specific. Thus, instead of
> "commit 304a53c2" or "r2355", "2011-10-25T15:11:09Z!fred@foonly.com".
Would there be any interest at all in extending the various Git
utilities to understand this style of reference?
—Joel Salomon
^ permalink raw reply
* Re: [PATCH 2/2] describe: Exclude --all --match=PATTERN
From: Junio C Hamano @ 2013-02-27 20:20 UTC (permalink / raw)
To: Greg Price; +Cc: git
In-Reply-To: <20130225053152.GI5688@biohazard-cafe.mit.edu>
Greg Price <price@MIT.EDU> writes:
> Currently when --all is passed, the effect of --match is only
> to demote non-matching tags to be treated like non-tags. This
> is puzzling behavior and not consistent with the documentation,
> especially with the suggested usage of avoiding information leaks.
> The combination of --all and --match is an oxymoron anyway, so
> just forbid it.
I am not sure if this is (1) "behaviour is sometimes useful in
narrow cases but is not explained well", (2) "behaviour does not
make sense in any situation", or (3) "the combination can make sense
if corrected, but the current behaviour is buggy". If it is (2) or
(3), I think it makes sense to forbid the combination. Also, if it
is (3), we should later come up with an improved behaviour and then
re-enable the combination.
Without "--all" the command considers only the annotated tags to
base the descripion on, and with "--all", a ref that is not
annotated tags can be used as a base, but with a lower priority (if
an annotated tag can describe a given commit, that tag is used).
So naïvely I would expect "--all" and "--match" to base the
description on refs that match the pattern without limiting the
choice of base to annotated tags, and refs that do not match the
given pattern should not appear even as the last resort. It appears
to me that the current situation is (3).
Will queue and cook in 'next'; thanks.
>
> Signed-off-by: Greg Price <price@mit.edu>
> ---
> This should be applied after the preceding patch; I mistakenly omitted
> the '1/2' in its subject line.
>
> Documentation/git-describe.txt | 3 ++-
> builtin/describe.c | 3 +++
> 2 files changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/git-describe.txt b/Documentation/git-describe.txt
> index 711040d..fd5d8f2 100644
> --- a/Documentation/git-describe.txt
> +++ b/Documentation/git-describe.txt
> @@ -83,7 +83,8 @@ OPTIONS
> --match <pattern>::
> Only consider tags matching the given `glob(7)` pattern,
> excluding the "refs/tags/" prefix. This can be used to avoid
> - leaking private tags from the repository.
> + leaking private tags from the repository. This option is
> + incompatible with `--all`.
>
> --always::
> Show uniquely abbreviated commit object as fallback.
> diff --git a/builtin/describe.c b/builtin/describe.c
> index 04c185b..90a72af 100644
> --- a/builtin/describe.c
> +++ b/builtin/describe.c
> @@ -435,6 +435,9 @@ int cmd_describe(int argc, const char **argv, const char *prefix)
> if (longformat && abbrev == 0)
> die(_("--long is incompatible with --abbrev=0"));
>
> + if (pattern && all)
> + die(_("--match is incompatible with --all"));
> +
> if (contains) {
> const char **args = xmalloc((7 + argc) * sizeof(char *));
> int i = 0;
^ permalink raw reply
* Re: https_proxy and libcurl
From: Phil Hord @ 2013-02-27 21:16 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git@vger.kernel.org
In-Reply-To: <7v7gm0c8ub.fsf@alter.siamese.dyndns.org>
On Fri, Feb 22, 2013 at 4:55 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Phil Hord <phil.hord@gmail.com> writes:
>
>> I have been unable to clone via http proxy because of a wrongly
>> configured proxy setup in my lab.
>>
>> I had this env:
>>
>> http_proxy=http://proxy.myco.com
>> https_proxy=https://proxy.myco.com
>>
>> The problem is that libcurl ignores the protocol part of the proxy
>> url, and it defaults to port 1080. wget honors the protocol specifier,
>> but it defaults to port 80 if none is given.
>
> IIRC, the historical norm is to set these to <host>:<port>.
>
> So many people mistakenly write them with <method>:// that some
> tools over time learned to strip and ignore that prefix, though.
On the server in question, I use wget to retrieve packages in another
script. When our firewall was tightened in the past, this script
broke. I fixed it by setting the https_proxy as I listed above. This
satisfied wget and I got on with life.
I do not need git connectivity to remotes from this server very often.
But when I tried, it failed with an error message that did not
implicate the proxy server. But it was the proxy server that returned
the error 500.
error: The requested URL returned error: 500 while accessing
https://github.com/git/git/info/refs?service=git-upload-pack
>
>> The fix was to specify the port explicitly, like this:
>>
>> http_proxy=proxy.myco.com:80
>> https_proxy=proxy.myco.com:443
>
> Yeah, that is the correct syntax to use. Is there anything you want
> Git to do to be more helpful?
Maybe Git can tell me more about proxy failures. Or maybe Git doesn't
know because curl hides the knowledge. Something like this:
error: The proxy server at proxy.myco.com:1080
returned error: 500 while accessing
https://github.com/git/git/info/refs?service=git-upload-pack
Maybe Git can warn me that the protocol prefix (if provided) is going
to be ignored or that I have failed to provide a port. Either of
these would be "above and beyond" the norm. But since curl is internal
to Git and its errors are digested by Git, it is less obvious where to
begin looking to solve such a problem; git doesn't provide the
plethora of connection-debugging output options that curl itself does.
Fwiw, the proxy info originated with our IT department who mostly had
to answer the question for Windows users and servers. Windows appears
to default to port 8080 when no port is specified. Port 8080 would
also have worked, but curl defaults to 1080. It seems that the curl
default of 1080 is the only one that caused me trouble.
My basest hope is that someone suffering similar troubles will find
this thread in a search for "git proxy error".
Phil
^ permalink raw reply
* Re: [PATCH] name-hash.c: fix endless loop with core.ignorecase=true
From: Karsten Blees @ 2013-02-27 21:52 UTC (permalink / raw)
To: Junio C Hamano
Cc: Jeff King, Duy Nguyen, kusmabite, Ramkumar Ramachandra,
Robert Zeh, Git List, finnag
In-Reply-To: <7v621dk8aa.fsf@alter.siamese.dyndns.org>
Am 27.02.2013 17:53, schrieb Junio C Hamano:
> Karsten Blees <karsten.blees@gmail.com> writes:
>
>> With core.ignorecase=true, name-hash.c builds a case insensitive index of
>> all tracked directories. Currently, the existing cache entry structures are
>> added multiple times to the same hashtable (with different name lengths and
>> hash codes). However, there's only one dir_next pointer, which gets
>> completely messed up in case of hash collisions. In the worst case, this
>> causes an endless loop if ce == ce->dir_next:
>>
>> ---8<---
>> # "V/", "V/XQANY/" and "WURZAUP/" all have the same hash_name
>> mkdir V
>> mkdir V/XQANY
>> mkdir WURZAUP
>> touch V/XQANY/test
>> git init
>> git config core.ignorecase true
>> git add .
>> git status
>> ---8<---
>
> Instead of using the scissors mark to confuse "am -c", indenting
> this block would have been easier to later readers.
>
> Also it is somewhat a shame that we do not use the above sample
> collisions in a new test case.
>
Duly noted.
Is there a way to run 'git status' with timeout? A test that doesn't complete (instead of failing) isn't nice...
>> +static struct dir_entry *hash_dir_entry(struct index_state *istate,
>> + struct cache_entry *ce, int namelen, int add)
>> {
>> /*
>> * Throw each directory component in the hash for quick lookup
>> * during a git status. Directory components are stored with their
>> - * closing slash. Despite submodules being a directory, they never
>> - * reach this point, because they are stored without a closing slash
>> - * in the cache.
>
> Is the description of submodule no longer relevant?
>
>> - * Note that the cache_entry stored with the directory does not
>> - * represent the directory itself. It is a pointer to an existing
>> - * filename, and its only purpose is to represent existence of the
>> - * directory in the cache. It is very possible multiple directory
>> - * hash entries may point to the same cache_entry.
>
> Is this paragraph no longer relevant? It seems to me that it still
> holds true, given the way how dir->ce points at the given ce.
>
I interpreted this as an explanation why it was safe to add the same cache_entry to the same name_hash multiple times...now that we have separate dir_entries and index_state.dir_hash, that's no longer a problem. But rereading that paragraph again, it is still mostly true (except for the 'existance' part, which is solved by reference counting).
>> + * closing slash.
>> */
>> + struct dir_entry *dir, *p;
>> +
>> + /* get length of parent directory */
>> + while (namelen > 0 && !is_dir_sep(ce->name[namelen - 1]))
>> + namelen--;
>> + if (namelen <= 0)
>> + return NULL;
>> +
>> + /* lookup existing entry for that directory */
>> + dir = find_dir_entry(istate, ce->name, namelen);
>> + if (add && !dir) {
>> ...
>> }
>> +
>> + /* add or release reference to this entry (and parents if 0) */
>> + p = dir;
>> + if (add) {
>> + while (p && !(p->nr++))
>> + p = p->parent;
>> + } else {
>> + while (p && p->nr && !(--p->nr))
>> + p = p->parent;
>> + }
>
> Can we free the entry when its refcnt goes down to zero? If yes, is
> it worth doing so?
>
There's no remove_hash in hash.[ch], and dir_entry.next may point to another dir_entry with the same hash code, so we must not free the memory (same problem as CE_UNHASHED).
>> +
>> + return dir;
>> }
>>
>> static void hash_index_entry(struct index_state *istate, struct cache_entry *ce)
>> @@ -74,7 +111,7 @@ static void hash_index_entry(struct index_state *istate, struct cache_entry *ce)
>> if (ce->ce_flags & CE_HASHED)
>> return;
>> ce->ce_flags |= CE_HASHED;
>> - ce->next = ce->dir_next = NULL;
>> + ce->next = NULL;
>> hash = hash_name(ce->name, ce_namelen(ce));
>> pos = insert_hash(hash, ce, &istate->name_hash);
>> if (pos) {
>> @@ -82,8 +119,8 @@ static void hash_index_entry(struct index_state *istate, struct cache_entry *ce)
>> *pos = ce;
>> }
>>
>> - if (ignore_case)
>> - hash_index_entry_directories(istate, ce);
>> + if (ignore_case && !(ce->ce_flags & CE_UNHASHED))
>> + hash_dir_entry(istate, ce, ce_namelen(ce), 1);
>> }
>>
>> static void lazy_init_name_hash(struct index_state *istate)
>> @@ -99,11 +136,33 @@ static void lazy_init_name_hash(struct index_state *istate)
>>
>> void add_name_hash(struct index_state *istate, struct cache_entry *ce)
>> {
>> + /* if already hashed, add reference to directory entries */
>> + if (ignore_case && (ce->ce_flags & CE_STATE_MASK) == CE_STATE_MASK)
>> + hash_dir_entry(istate, ce, ce_namelen(ce), 1);
>
> Instead of a single function with "are we adding or removing?"
> parameter, it would be a lot easier to read the callers if they are
> wrapped in two helpers, add_dir_entry() and del_dir_entry() or
> something, especially when the add=[0|1] parameter is constant for
> each and every callsite (i.e. the codeflow determines it, not the
> data).
>
OK
>> ce->ce_flags &= ~CE_UNHASHED;
>> if (istate->name_hash_initialized)
>> hash_index_entry(istate, ce);
>> }
>>
>> +/*
>> + * We don't actually *remove* it, we can just mark it invalid so that
>> + * we won't find it in lookups.
>> + *
>> + * Not only would we have to search the lists (simple enough), but
>> + * we'd also have to rehash other hash buckets in case this makes the
>> + * hash bucket empty (common). So it's much better to just mark
>> + * it.
>> + */
>> +void remove_name_hash(struct index_state *istate, struct cache_entry *ce)
>> +{
>> + /* if already hashed, release reference to directory entries */
>> + if (ignore_case && (ce->ce_flags & CE_STATE_MASK) == CE_HASHED)
>> + hash_dir_entry(istate, ce, ce_namelen(ce), 0);
>
> And here as well.
>
>> +
>> + ce->ce_flags |= CE_UNHASHED;
>> +}
>> +
>> static int slow_same_name(const char *name1, int len1, const char *name2, int len2)
>> {
>> if (len1 != len2)
>> @@ -137,18 +196,7 @@ static int same_name(const struct cache_entry *ce, const char *name, int namelen
>> if (!icase)
>> return 0;
>>
>> - /*
>> - * If the entry we're comparing is a filename (no trailing slash), then compare
>> - * the lengths exactly.
>> - */
>> - if (name[namelen - 1] != '/')
>> - return slow_same_name(name, namelen, ce->name, len);
>> -
>> - /*
>> - * For a directory, we point to an arbitrary cache_entry filename. Just
>> - * make sure the directory portion matches.
>> - */
>> - return slow_same_name(name, namelen, ce->name, namelen < len ? namelen : len);
>> + return slow_same_name(name, namelen, ce->name, len);
>
> Hmph, what is this change about? Nobody calls same_name() with a
> directory name anymore or something?
>
dir_entries (with trailing /) are in index_state.dir_hash, so we wouldn't expect to find anything in index_state.name_hash, especially not a cache_entry. find_dir_entry simply uses strncasecmp, as we only do directory indexing with core.ignorecase=true.
> Thanks.
>
^ permalink raw reply
* how to check pending commits to be pushed?
From: Patricia Bittencourt Egeland @ 2013-02-27 22:02 UTC (permalink / raw)
To: git
Hi,
Does someone know how to identify pending commits to be pushed in a local super repository (with quite some submodules), with git-1.6.5-1 ?
Thanks,
Patricia
^ permalink raw reply
* Re: git subtree workflow
From: Paul Campbell @ 2013-02-27 23:37 UTC (permalink / raw)
To: Ben McCann; +Cc: git
In-Reply-To: <CAH3cagOtcR3fgoXK-Xd8vm48q0GPQtYMkWiqQGgydb27HeO0wQ@mail.gmail.com>
On Tue, Feb 26, 2013 at 9:04 AM, Ben McCann <ben@benmccann.com> wrote:
> I'm fairly new to git and am trying to determine if git subtree would
> be helpful for managing our company's codebase, which has several
> repos some of which depend on each other. All the examples I've seen
> make sense to me as a one-time operation to merge separate repos into
> one monolithic one or to split one monolithic repo into separate
> repos. I'm having a harder time understanding how this fits into a
> larger picture and what the workflow for working with subtree would
> be.
The git subtree command does support pushing and pulling changes
to/from a subtree to a remote repo. This operation can be repeated
over time as the underlying git subtree split command produces an
accurate extract from the subtree.
For example:
$ git clone $superrepo super
$ cd super
$ git subtree add --prefix=child $childrepo master
$ split_head=$(git subtree split --prefix=child)
git subtree split creates a synthetic copy of the child subtree and
returns the SHA1 for the head of that subtree.
$ cd ..
$ git clone $childrepo child
$ cd child
$ child_head=$(git rev-parse master)
$split_head and $child_head match.
You could pass either $*_head to gitk and be able to browse identical
trees and histories.
$ gitk $child_head &
$ cd ../super
$ gitk $split_head &
The only difference you should see is that the tree in the super
project doesn't have any branches or tags from the child repo.
> If I have a bunch of repos on GitHub and some depend on each other,
> how would I set them up to work with subtree? Would GitHub continue to
> host them as is, host a merged monolithic repo, or host both a
> monolithic repo and the splitted out repos? The exact answer probably
> varies, but I imagine there's basic workflow that would satisfy 80% of
> users. If I have GitHub host both monolithic and splitted out repos,
> it seems unclear as to how I keep those repos in sync and make sure
> all the developers in our company push their changes to both repos.
This is what I'm using git-subtree for. I have a super project which
holds child repos, some of which hold other child repos. You can
develop on all of your projects within the super project and still
periodically pull and push updates with the remote child repos.
Someone who doesn't know about your child repos can take the
monolithic repo and commit to it as normal. You can then pull their
commits and push any changes to the child repos. Probably to a
non-master branch so you they can be integrated like other pull
requests.
Caveats:
1) I would probably avoid the --squash option as I suspect that might
mess with $split_head == $child_head. Whether or not it would have any
impact of your ability to push or pull I don't know. I've not tried
it.
2) When you commit you should ideally not commit a set of files that
are in more than one subtree. You're commit history when you push back
to the source repo for a subtree might look a bit odd if it's
referring to other parts of the super project or it's subtrees.
--
Paul [W] Campbell
^ permalink raw reply
* [BUG/PATCH] contrib/subtree: allow addition of remote branch with name not locally present
From: Paul Campbell @ 2013-02-27 23:51 UTC (permalink / raw)
To: git
Cc: Adam Tkac, David Greene, Jesper Lyager Nielsen, Michael Schubert,
Techlive Zheng
cmd_add() attempts to check for the validity of refspec for the repository
it is about to add as a subtree. It tries to do so before contacting the
repository. If the refspec happens to exist locally (say 'master') then
the test passes and the repo is fetched. If the refspec doesn't exist
locally then the test fails and the remote repo is never contacted.
Removing the tests still works as the git fetch command fails with the
perfectly accurate error:
fatal: Couldn't find remote ref <refspec>
Signed-off-by: Paul Campbell <pcampbell@carnegiecollege.ac.uk>
---
I must confess to not understanding the comment preceding the
rev-parse test. Given that it is against the rev-parse and not the
cmd_add_repository call I'm assuming it can be removed.
contrib/subtree/git-subtree.sh | 8 --------
1 file changed, 8 deletions(-)
diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh
index 8a23f58..9a38b18 100755
--- a/contrib/subtree/git-subtree.sh
+++ b/contrib/subtree/git-subtree.sh
@@ -503,14 +503,6 @@ cmd_add()
"cmd_add_commit" "$@"
elif [ $# -eq 2 ]; then
- # Technically we could accept a refspec here but we're
- # just going to turn around and add FETCH_HEAD under the
- # specified directory. Allowing a refspec might be
- # misleading because we won't do anything with any other
- # branches fetched via the refspec.
- git rev-parse -q --verify "$2^{commit}" >/dev/null ||
- die "'$2' does not refer to a commit"
-
"cmd_add_repository" "$@"
else
say "error: parameters were '$@'"
--
1.8.2.rc1
--
Paul [W] Campbell
^ permalink raw reply related
* [PATCH v2] name-hash.c: fix endless loop with core.ignorecase=true
From: Karsten Blees @ 2013-02-27 23:57 UTC (permalink / raw)
To: Junio C Hamano
Cc: Jeff King, Duy Nguyen, kusmabite, Ramkumar Ramachandra,
Robert Zeh, Git List, finnag
In-Reply-To: <512E8014.3090107@gmail.com>
With core.ignorecase=true, name-hash.c builds a case insensitive index of
all tracked directories. Currently, the existing cache entry structures are
added multiple times to the same hashtable (with different name lengths and
hash codes). However, there's only one dir_next pointer, which gets
completely messed up in case of hash collisions. In the worst case, this
causes an endless loop if ce == ce->dir_next (see t7062).
Use a separate hashtable and separate structures for the directory index
so that each directory entry has its own next pointer. Use reference
counting to track which directory entry contains files.
There are only slight changes to the name-hash.c API:
- new free_name_hash() used by read_cache.c::discard_index()
- remove_name_hash() takes an additional index_state parameter
- index_name_exists() for a directory (trailing '/') may return a cache
entry that has been removed (CE_UNHASHED). This is not a problem as the
return value is only used to check if the directory exists (dir.c) or to
normalize casing of directory names (read-cache.c).
Getting rid of cache_entry.dir_next reduces memory consumption, especially
with core.ignorecase=false (which doesn't use that member at all).
With core.ignorecase=true, building the directory index is slightly faster
as we add / check the parent directory first (instead of going through all
directory levels for each file in the index). E.g. with WebKit (~200k
files, ~7k dirs), time spent in lazy_init_name_hash is reduced from 176ms
to 130ms.
Signed-off-by: Karsten Blees <blees@dcon.de>
---
Also available here:
https://github.com/kblees/git/tree/kb/name-hash-fix-endless-loop-v2
git pull git://github.com/kblees/git.git kb/name-hash-fix-endless-loop-v2
cache.h | 17 +---
name-hash.c | 182 +++++++++++++++++++++++++++++++----------
read-cache.c | 9 +-
t/t7062-wtstatus-ignorecase.sh | 20 +++++
4 files changed, 166 insertions(+), 62 deletions(-)
create mode 100755 t/t7062-wtstatus-ignorecase.sh
diff --git a/cache.h b/cache.h
index e493563..898e346 100644
--- a/cache.h
+++ b/cache.h
@@ -131,7 +131,6 @@ struct cache_entry {
unsigned int ce_namelen;
unsigned char sha1[20];
struct cache_entry *next;
- struct cache_entry *dir_next;
char name[FLEX_ARRAY]; /* more */
};
@@ -267,25 +266,15 @@ struct index_state {
unsigned name_hash_initialized : 1,
initialized : 1;
struct hash_table name_hash;
+ struct hash_table dir_hash;
};
extern struct index_state the_index;
/* Name hashing */
extern void add_name_hash(struct index_state *istate, struct cache_entry *ce);
-/*
- * We don't actually *remove* it, we can just mark it invalid so that
- * we won't find it in lookups.
- *
- * Not only would we have to search the lists (simple enough), but
- * we'd also have to rehash other hash buckets in case this makes the
- * hash bucket empty (common). So it's much better to just mark
- * it.
- */
-static inline void remove_name_hash(struct cache_entry *ce)
-{
- ce->ce_flags |= CE_UNHASHED;
-}
+extern void remove_name_hash(struct index_state *istate, struct cache_entry *ce);
+extern void free_name_hash(struct index_state *istate);
#ifndef NO_THE_INDEX_COMPATIBILITY_MACROS
diff --git a/name-hash.c b/name-hash.c
index 942c459..6d7e198 100644
--- a/name-hash.c
+++ b/name-hash.c
@@ -32,38 +32,96 @@ static unsigned int hash_name(const char *name, int namelen)
return hash;
}
-static void hash_index_entry_directories(struct index_state *istate, struct cache_entry *ce)
+struct dir_entry {
+ struct dir_entry *next;
+ struct dir_entry *parent;
+ struct cache_entry *ce;
+ int nr;
+ unsigned int namelen;
+};
+
+static struct dir_entry *find_dir_entry(struct index_state *istate,
+ const char *name, unsigned int namelen)
+{
+ unsigned int hash = hash_name(name, namelen);
+ struct dir_entry *dir;
+
+ for (dir = lookup_hash(hash, &istate->dir_hash); dir; dir = dir->next)
+ if (dir->namelen == namelen &&
+ !strncasecmp(dir->ce->name, name, namelen))
+ return dir;
+ return NULL;
+}
+
+static struct dir_entry *hash_dir_entry(struct index_state *istate,
+ struct cache_entry *ce, int namelen)
{
/*
* Throw each directory component in the hash for quick lookup
* during a git status. Directory components are stored with their
* closing slash. Despite submodules being a directory, they never
* reach this point, because they are stored without a closing slash
- * in the cache.
+ * in index_state.name_hash (as ordinary cache_entries).
*
- * Note that the cache_entry stored with the directory does not
- * represent the directory itself. It is a pointer to an existing
- * filename, and its only purpose is to represent existence of the
- * directory in the cache. It is very possible multiple directory
- * hash entries may point to the same cache_entry.
+ * Note that the cache_entry stored with the dir_entry merely
+ * supplies the name of the directory (up to dir_entry.namelen). We
+ * track the number of 'active' files in a directory in dir_entry.nr,
+ * so we can tell if the directory is still relevant, e.g. for git
+ * status. However, if cache_entries are removed, we cannot pinpoint
+ * an exact cache_entry that's still active. It is very possible that
+ * multiple dir_entries point to the same cache_entry.
*/
- unsigned int hash;
- void **pos;
+ struct dir_entry *dir;
+
+ /* get length of parent directory */
+ while (namelen > 0 && !is_dir_sep(ce->name[namelen - 1]))
+ namelen--;
+ if (namelen <= 0)
+ return NULL;
+
+ /* lookup existing entry for that directory */
+ dir = find_dir_entry(istate, ce->name, namelen);
+ if (!dir) {
+ /* not found, create it and add to hash table */
+ void **pdir;
+ unsigned int hash = hash_name(ce->name, namelen);
- const char *ptr = ce->name;
- while (*ptr) {
- while (*ptr && *ptr != '/')
- ++ptr;
- if (*ptr == '/') {
- ++ptr;
- hash = hash_name(ce->name, ptr - ce->name);
- pos = insert_hash(hash, ce, &istate->name_hash);
- if (pos) {
- ce->dir_next = *pos;
- *pos = ce;
- }
+ dir = xcalloc(1, sizeof(struct dir_entry));
+ dir->namelen = namelen;
+ dir->ce = ce;
+
+ pdir = insert_hash(hash, dir, &istate->dir_hash);
+ if (pdir) {
+ dir->next = *pdir;
+ *pdir = dir;
}
+
+ /* recursively add missing parent directories */
+ dir->parent = hash_dir_entry(istate, ce, namelen - 1);
}
+ return dir;
+}
+
+static void add_dir_entry(struct index_state *istate, struct cache_entry *ce)
+{
+ /* Add reference to the directory entry (and parents if 0). */
+ struct dir_entry *dir = hash_dir_entry(istate, ce, ce_namelen(ce));
+ while (dir && !(dir->nr++))
+ dir = dir->parent;
+}
+
+static void remove_dir_entry(struct index_state *istate, struct cache_entry *ce)
+{
+ /*
+ * Release reference to the directory entry (and parents if 0).
+ *
+ * Note: we do not remove / free the entry because there's no
+ * hash.[ch]::remove_hash and dir->next may point to other entries
+ * that are still valid, so we must not free the memory.
+ */
+ struct dir_entry *dir = hash_dir_entry(istate, ce, ce_namelen(ce));
+ while (dir && dir->nr && !(--dir->nr))
+ dir = dir->parent;
}
static void hash_index_entry(struct index_state *istate, struct cache_entry *ce)
@@ -74,7 +132,7 @@ static void hash_index_entry(struct index_state *istate, struct cache_entry *ce)
if (ce->ce_flags & CE_HASHED)
return;
ce->ce_flags |= CE_HASHED;
- ce->next = ce->dir_next = NULL;
+ ce->next = NULL;
hash = hash_name(ce->name, ce_namelen(ce));
pos = insert_hash(hash, ce, &istate->name_hash);
if (pos) {
@@ -82,8 +140,8 @@ static void hash_index_entry(struct index_state *istate, struct cache_entry *ce)
*pos = ce;
}
- if (ignore_case)
- hash_index_entry_directories(istate, ce);
+ if (ignore_case && !(ce->ce_flags & CE_UNHASHED))
+ add_dir_entry(istate, ce);
}
static void lazy_init_name_hash(struct index_state *istate)
@@ -99,11 +157,33 @@ static void lazy_init_name_hash(struct index_state *istate)
void add_name_hash(struct index_state *istate, struct cache_entry *ce)
{
+ /* if already hashed, add reference to directory entries */
+ if (ignore_case && (ce->ce_flags & CE_STATE_MASK) == CE_STATE_MASK)
+ add_dir_entry(istate, ce);
+
ce->ce_flags &= ~CE_UNHASHED;
if (istate->name_hash_initialized)
hash_index_entry(istate, ce);
}
+/*
+ * We don't actually *remove* it, we can just mark it invalid so that
+ * we won't find it in lookups.
+ *
+ * Not only would we have to search the lists (simple enough), but
+ * we'd also have to rehash other hash buckets in case this makes the
+ * hash bucket empty (common). So it's much better to just mark
+ * it.
+ */
+void remove_name_hash(struct index_state *istate, struct cache_entry *ce)
+{
+ /* if already hashed, release reference to directory entries */
+ if (ignore_case && (ce->ce_flags & CE_STATE_MASK) == CE_HASHED)
+ remove_dir_entry(istate, ce);
+
+ ce->ce_flags |= CE_UNHASHED;
+}
+
static int slow_same_name(const char *name1, int len1, const char *name2, int len2)
{
if (len1 != len2)
@@ -137,18 +217,7 @@ static int same_name(const struct cache_entry *ce, const char *name, int namelen
if (!icase)
return 0;
- /*
- * If the entry we're comparing is a filename (no trailing slash), then compare
- * the lengths exactly.
- */
- if (name[namelen - 1] != '/')
- return slow_same_name(name, namelen, ce->name, len);
-
- /*
- * For a directory, we point to an arbitrary cache_entry filename. Just
- * make sure the directory portion matches.
- */
- return slow_same_name(name, namelen, ce->name, namelen < len ? namelen : len);
+ return slow_same_name(name, namelen, ce->name, len);
}
struct cache_entry *index_name_exists(struct index_state *istate, const char *name, int namelen, int icase)
@@ -164,27 +233,54 @@ struct cache_entry *index_name_exists(struct index_state *istate, const char *na
if (same_name(ce, name, namelen, icase))
return ce;
}
- if (icase && name[namelen - 1] == '/')
- ce = ce->dir_next;
- else
- ce = ce->next;
+ ce = ce->next;
}
/*
- * Might be a submodule. Despite submodules being directories,
+ * When looking for a directory (trailing '/'), it might be a
+ * submodule or a directory. Despite submodules being directories,
* they are stored in the name hash without a closing slash.
- * When ignore_case is 1, directories are stored in the name hash
- * with their closing slash.
+ * When ignore_case is 1, directories are stored in a separate hash
+ * table *with* their closing slash.
*
* The side effect of this storage technique is we have need to
+ * lookup the directory in a separate hash table, and if not found
* remove the slash from name and perform the lookup again without
* the slash. If a match is made, S_ISGITLINK(ce->mode) will be
* true.
*/
if (icase && name[namelen - 1] == '/') {
+ struct dir_entry *dir = find_dir_entry(istate, name, namelen);
+ if (dir && dir->nr)
+ return dir->ce;
+
ce = index_name_exists(istate, name, namelen - 1, icase);
if (ce && S_ISGITLINK(ce->ce_mode))
return ce;
}
return NULL;
}
+
+static int free_dir_entry(void *entry, void *unused)
+{
+ struct dir_entry *dir = entry;
+ while (dir) {
+ struct dir_entry *next = dir->next;
+ free(dir);
+ dir = next;
+ }
+ return 0;
+}
+
+void free_name_hash(struct index_state *istate)
+{
+ if (!istate->name_hash_initialized)
+ return;
+ istate->name_hash_initialized = 0;
+ if (ignore_case)
+ /* free directory entries */
+ for_each_hash(&istate->dir_hash, free_dir_entry, NULL);
+
+ free_hash(&istate->name_hash);
+ free_hash(&istate->dir_hash);
+}
diff --git a/read-cache.c b/read-cache.c
index 827ae55..47eb9d8 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -46,7 +46,7 @@ static void replace_index_entry(struct index_state *istate, int nr, struct cache
{
struct cache_entry *old = istate->cache[nr];
- remove_name_hash(old);
+ remove_name_hash(istate, old);
set_index_entry(istate, nr, ce);
istate->cache_changed = 1;
}
@@ -460,7 +460,7 @@ int remove_index_entry_at(struct index_state *istate, int pos)
struct cache_entry *ce = istate->cache[pos];
record_resolve_undo(istate, ce);
- remove_name_hash(ce);
+ remove_name_hash(istate, ce);
istate->cache_changed = 1;
istate->cache_nr--;
if (pos >= istate->cache_nr)
@@ -483,7 +483,7 @@ void remove_marked_cache_entries(struct index_state *istate)
for (i = j = 0; i < istate->cache_nr; i++) {
if (ce_array[i]->ce_flags & CE_REMOVE)
- remove_name_hash(ce_array[i]);
+ remove_name_hash(istate, ce_array[i]);
else
ce_array[j++] = ce_array[i];
}
@@ -1515,8 +1515,7 @@ int discard_index(struct index_state *istate)
istate->cache_changed = 0;
istate->timestamp.sec = 0;
istate->timestamp.nsec = 0;
- istate->name_hash_initialized = 0;
- free_hash(&istate->name_hash);
+ free_name_hash(istate);
cache_tree_free(&(istate->cache_tree));
istate->initialized = 0;
diff --git a/t/t7062-wtstatus-ignorecase.sh b/t/t7062-wtstatus-ignorecase.sh
new file mode 100755
index 0000000..73709db
--- /dev/null
+++ b/t/t7062-wtstatus-ignorecase.sh
@@ -0,0 +1,20 @@
+#!/bin/sh
+
+test_description='git-status with core.ignorecase=true'
+
+. ./test-lib.sh
+
+test_expect_success 'status with hash collisions' '
+ # note: "V/", "V/XQANY/" and "WURZAUP/" produce the same hash code
+ # in name-hash.c::hash_name
+ mkdir V &&
+ mkdir V/XQANY &&
+ mkdir WURZAUP &&
+ touch V/XQANY/test &&
+ git config core.ignorecase true &&
+ git add . &&
+ # test is successful if git status completes (no endless loop)
+ git status
+'
+
+test_done
--
1.8.1.2.7987.g4a34b82
^ permalink raw reply related
* Re: [BUG/PATCH] contrib/subtree: allow addition of remote branch with name not locally present
From: Junio C Hamano @ 2013-02-28 0:20 UTC (permalink / raw)
To: Paul Campbell
Cc: git, Adam Tkac, David Greene, Jesper Lyager Nielsen,
Michael Schubert, Techlive Zheng
In-Reply-To: <CALeLG_ndDWyPWRBQtKV4WswZUKRpNQzCAXmB0f0yoKGsAYMUig@mail.gmail.com>
Paul Campbell <pcampbell@kemitix.net> writes:
> cmd_add() attempts to check for the validity of refspec for the repository
> it is about to add as a subtree. It tries to do so before contacting the
> repository. If the refspec happens to exist locally (say 'master') then
> the test passes and the repo is fetched. If the refspec doesn't exist
> locally then the test fails and the remote repo is never contacted.
>
> Removing the tests still works as the git fetch command fails with the
> perfectly accurate error:
>
> fatal: Couldn't find remote ref <refspec>
>
> Signed-off-by: Paul Campbell <pcampbell@carnegiecollege.ac.uk>
> ---
>
> I must confess to not understanding the comment preceding the
> rev-parse test. Given that it is against the rev-parse and not the
> cmd_add_repository call I'm assuming it can be removed.
This is contrib/ material and I do not use the command, so anything
I say should be taken with a moderate amount of salt, but I think
the comment is talking about _not_ accepting a full storing refspec,
or worse yet wildcard, e.g.
refs/heads/topic:refs/remotes/origin/topic
refs/heads/*:refs/remotes/origin/*
which will not make sense given that you are only adding a single
commit via "cmd_add". Also, if you have already fetched from the
remote, "rev-parse $2^{commit}" at this point will protect against
a typo by the end user.
As you noticed, checking if $2 is a commit using rev-parse _before_
fetching $2 from the remote repository is not a valid way to check
against such errors. So I tend to agree that removing the "die"
will be a good idea.
Typing "tipoc" when the user meant "topic" will error out at the
"git fetch" done in cmd_add_repository, but that fetch will happily
fetch and store whatever a refspec specifies, so you might want to
replace the bogus "rev-parse before fetching" check to "reject
refspec" with something else.
> contrib/subtree/git-subtree.sh | 8 --------
> 1 file changed, 8 deletions(-)
>
> diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh
> index 8a23f58..9a38b18 100755
> --- a/contrib/subtree/git-subtree.sh
> +++ b/contrib/subtree/git-subtree.sh
> @@ -503,14 +503,6 @@ cmd_add()
>
> "cmd_add_commit" "$@"
> elif [ $# -eq 2 ]; then
> - # Technically we could accept a refspec here but we're
> - # just going to turn around and add FETCH_HEAD under the
> - # specified directory. Allowing a refspec might be
> - # misleading because we won't do anything with any other
> - # branches fetched via the refspec.
> - git rev-parse -q --verify "$2^{commit}" >/dev/null ||
> - die "'$2' does not refer to a commit"
> -
> "cmd_add_repository" "$@"
> else
> say "error: parameters were '$@'"
> --
> 1.8.2.rc1
^ permalink raw reply
* Re: [PATCH v2] name-hash.c: fix endless loop with core.ignorecase=true
From: Junio C Hamano @ 2013-02-28 0:27 UTC (permalink / raw)
To: Karsten Blees
Cc: Jeff King, Duy Nguyen, kusmabite, Ramkumar Ramachandra,
Robert Zeh, Git List, finnag
In-Reply-To: <512E9D7C.2030803@gmail.com>
Karsten Blees <karsten.blees@gmail.com> writes:
> With core.ignorecase=true, name-hash.c builds a case insensitive index of
> all tracked directories. Currently, the existing cache entry structures are
> added multiple times to the same hashtable (with different name lengths and
> hash codes). However, there's only one dir_next pointer, which gets
> completely messed up in case of hash collisions. In the worst case, this
> causes an endless loop if ce == ce->dir_next (see t7062).
>
> Use a separate hashtable and separate structures for the directory index
> so that each directory entry has its own next pointer. Use reference
> counting to track which directory entry contains files.
>
> There are only slight changes to the name-hash.c API:
> - new free_name_hash() used by read_cache.c::discard_index()
> - remove_name_hash() takes an additional index_state parameter
> - index_name_exists() for a directory (trailing '/') may return a cache
> entry that has been removed (CE_UNHASHED). This is not a problem as the
> return value is only used to check if the directory exists (dir.c) or to
> normalize casing of directory names (read-cache.c).
>
> Getting rid of cache_entry.dir_next reduces memory consumption, especially
> with core.ignorecase=false (which doesn't use that member at all).
>
> With core.ignorecase=true, building the directory index is slightly faster
> as we add / check the parent directory first (instead of going through all
> directory levels for each file in the index). E.g. with WebKit (~200k
> files, ~7k dirs), time spent in lazy_init_name_hash is reduced from 176ms
> to 130ms.
>
> Signed-off-by: Karsten Blees <blees@dcon.de>
> ---
One thing that still puzzles me is what guarantee we have on the
liftime of these ce's that are borrowed by these dir_hash entries.
There are a few places where we call free(ce) around "aliased"
entries (only happens with ignore_case set). I do not think it is a
new issue (we used to borrow a ce to represent a directory in the
name_hash by using the leading prefix of its name anyway, and this
patch only changes which hash table is used to hold it), and I do
not think it will be an issue for case sensitive systems, so I would
stop being worried about it for now, though ;-)
Thanks, will replace and queue.
> diff --git a/t/t7062-wtstatus-ignorecase.sh b/t/t7062-wtstatus-ignorecase.sh
> new file mode 100755
> index 0000000..73709db
> --- /dev/null
> +++ b/t/t7062-wtstatus-ignorecase.sh
> @@ -0,0 +1,20 @@
> +#!/bin/sh
> +
> +test_description='git-status with core.ignorecase=true'
> +
> +. ./test-lib.sh
> +
> +test_expect_success 'status with hash collisions' '
> + # note: "V/", "V/XQANY/" and "WURZAUP/" produce the same hash code
> + # in name-hash.c::hash_name
> + mkdir V &&
> + mkdir V/XQANY &&
> + mkdir WURZAUP &&
> + touch V/XQANY/test &&
> + git config core.ignorecase true &&
> + git add . &&
> + # test is successful if git status completes (no endless loop)
> + git status
> +'
> +
> +test_done
^ permalink raw reply
* Re: [PATCH 2/4] config: drop file pointer validity check in get_next_char()
From: Heiko Voigt @ 2013-02-28 0:42 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, git, Jens Lehmann
In-Reply-To: <20130227075257.GA4685@sandbox-ub>
On Wed, Feb 27, 2013 at 08:52:57AM +0100, Heiko Voigt wrote:
> On Tue, Feb 26, 2013 at 03:05:56PM -0500, Jeff King wrote:
> > On Tue, Feb 26, 2013 at 08:40:23PM +0100, Heiko Voigt wrote:
> >
> > > The only location where cf is set in this file is in do_config_from().
> > > This function has only one callsite which is config_from_file(). In
> > > config_from_file() its ensured that the f member is set to non-zero.
> >
> > Makes sense, although...
> >
> > > - if (cf && ((f = cf->f) != NULL)) {
> > > + if (cf) {
> > > + FILE *f = cf->f;
> >
> > Couldn't we say the same thing about "cf" here (i.e., that it would
> > never be NULL)? Can we just get rid of this conditional entirely?
>
> That might be true. I will look into it. Just wanted to get rid of an
> extra callback in my series.
I had a look and it might be true that cf will never be NULL in a code
path. Nevertheless its much harder to verify by looking at the code
since its a global variable. get_next_char() is called from all over the
place and I would have to look at all the code paths. As far as I know
static global variables are always initialized to zero so its safe to
check even if has not yet been explicitly initialized.
The statement if cf is not NULL all members will be initialized is much
simpler to verify since its just one place now and two places after this
series.
Cheers Heiko
^ permalink raw reply
* Re: [PATCH 2/4] config: drop file pointer validity check in get_next_char()
From: Heiko Voigt @ 2013-02-28 0:54 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, git, Jens Lehmann
In-Reply-To: <20130228004137.GA12948@sandbox-ub>
On Thu, Feb 28, 2013 at 01:41:47AM +0100, Heiko Voigt wrote:
> On Wed, Feb 27, 2013 at 08:52:57AM +0100, Heiko Voigt wrote:
> > On Tue, Feb 26, 2013 at 03:05:56PM -0500, Jeff King wrote:
> > > On Tue, Feb 26, 2013 at 08:40:23PM +0100, Heiko Voigt wrote:
> > >
> > > > The only location where cf is set in this file is in do_config_from().
> > > > This function has only one callsite which is config_from_file(). In
> > > > config_from_file() its ensured that the f member is set to non-zero.
> > >
> > > Makes sense, although...
> > >
> > > > - if (cf && ((f = cf->f) != NULL)) {
> > > > + if (cf) {
> > > > + FILE *f = cf->f;
> > >
> > > Couldn't we say the same thing about "cf" here (i.e., that it would
> > > never be NULL)? Can we just get rid of this conditional entirely?
> >
> > That might be true. I will look into it. Just wanted to get rid of an
> > extra callback in my series.
>
> I had a look and it might be true that cf will never be NULL in a code
> path. Nevertheless its much harder to verify by looking at the code
> since its a global variable. get_next_char() is called from all over the
> place and I would have to look at all the code paths. As far as I know
> static global variables are always initialized to zero so its safe to
> check even if has not yet been explicitly initialized.
>
> The statement if cf is not NULL all members will be initialized is much
> simpler to verify since its just one place now and two places after this
> series.
To add some empirical information: I just ran the testsuite without the
conditional and it still passes. To me it only make sense to start the
parsing with cf initialized. But I am not familiar enough with the code
to judge whether it is safe to assume this.
Cheers Heiko
^ permalink raw reply
* [PATCH v8 2/5] blame: introduce $ as "end of file" in -L syntax
From: Thomas Rast @ 2013-02-28 16:38 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Bo Yang, Zbigniew Jędrzejewski-Szmek,
Will Palmer
In-Reply-To: <cover.1362069310.git.trast@student.ethz.ch>
To save the user a lookup of the last line number, introduce $ as a
shorthand for the last line. This is mostly useful to spell "until
the end of the file" as '-L<begin>,$'.
Signed-off-by: Thomas Rast <trast@student.ethz.ch>
---
Documentation/line-range-format.txt | 6 ++++++
line-log.c | 8 ++++++++
2 files changed, 14 insertions(+)
diff --git a/Documentation/line-range-format.txt b/Documentation/line-range-format.txt
index 265bc23..9ce0688 100644
--- a/Documentation/line-range-format.txt
+++ b/Documentation/line-range-format.txt
@@ -16,3 +16,9 @@ starting at the line given by <start>.
This is only valid for <end> and will specify a number
of lines before or after the line given by <start>.
+
+
+- `$`
++
+A literal dollar sign can be used as a shorthand for the last line in
+the file.
++
diff --git a/line-log.c b/line-log.c
index a24a86b..b167b00 100644
--- a/line-log.c
+++ b/line-log.c
@@ -15,6 +15,14 @@ const char *parse_loc(const char *spec, nth_line_fn_t nth_line,
regmatch_t match[1];
/*
+ * $ is a synonym for "the end of the file".
+ */
+ if (spec[0] == '$') {
+ *ret = lines;
+ return spec + 1;
+ }
+
+ /*
* Allow "-L <something>,+20" to mean starting at <something>
* for 20 lines, or "-L <something>,-5" for 5 lines ending at
* <something>.
--
1.8.2.rc1.388.g1bd82c8
^ permalink raw reply related
* [PATCH v8 0/5] git log -L
From: Thomas Rast @ 2013-02-28 16:38 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Bo Yang, Zbigniew Jędrzejewski-Szmek,
Will Palmer
Here's another shot :-)
For an instructive example, apply it and run
git log -L:archiver:archive.h
in your git.git.
Last time around, apart from some minor issues, there was one main
point discussed[1]: it should be possible to do the line-log in two
separate steps, one for filtering and one for the diffing.
That would have some really nice advantages, in particular it would
let us generate the diff within the usual diff chain. That way it
would work together with e.g. --word-diff automatically.
However, with the whole diff chain IMHO already being in some
disarray, that's such a far-off goal (and a good example of gsoc scope
creep!) that I can't see myself working on it in the near future
unless someone decides to pay me for working on Git. Meanwhile, this
seems to be the only blocker for the inclusion of a feature for which
all feedback has been "WANT!" since it worked for the first time.
So I'd rather settle for including this slightly hacky version, and
then improving on it incrementally.
There are more things that need to be done: my rewrite for readability
(see [1] again) lost the support for -C and -C -C, so it no longer is
any use with big code moves. The only thing that currently works is
-M, which will track the ranges across a file rename that can be
detected by the diff engine (i.e., it works by letting the diff engine
form a rename filepair).
Changes since v7 include:
- An all-new patch that allows for funcname matching: -L:pattern:file.
I find it makes calling line-log (or blame, for that matter) much
more natural. I'm not exactly married to the syntax; in particular
it feels weird in blame where you have to say 'git blame -L:foo file'.
If you have better ideas, please shout.
- Tests! Yay. Also fixes for bugs found by the tests ;-)
- Dropped the unnecessary patch "Export three functions from diff.c".
Bo Yang (2):
Refactor parse_loc
Export rewrite_parents() for 'log -L'
Thomas Rast (3):
blame: introduce $ as "end of file" in -L syntax
Implement line-history search (git log -L)
log -L: :pattern:file syntax to find by funcname
Documentation/blame-options.txt | 19 +-
Documentation/git-log.txt | 22 +
Documentation/line-range-format.txt | 31 +
Makefile | 2 +
builtin/blame.c | 99 +--
builtin/log.c | 31 +
line-log.c | 1433 +++++++++++++++++++++++++++++++++++
line-log.h | 80 ++
log-tree.c | 4 +
revision.c | 22 +-
revision.h | 16 +-
t/t4211-line-log.sh | 42 +
t/t4211/expect.beginning-of-file | 43 ++
t/t4211/expect.end-of-file | 62 ++
t/t4211/expect.move-support-f | 40 +
t/t4211/expect.simple-f | 59 ++
t/t4211/expect.simple-f-to-main | 100 +++
t/t4211/expect.simple-main | 68 ++
t/t4211/expect.simple-main-to-end | 70 ++
t/t4211/expect.two-ranges | 102 +++
t/t4211/expect.vanishes-early | 39 +
t/t4211/history.export | 330 ++++++++
t/t8003-blame-corner-cases.sh | 6 +
23 files changed, 2601 insertions(+), 119 deletions(-)
create mode 100644 Documentation/line-range-format.txt
create mode 100644 line-log.c
create mode 100644 line-log.h
create mode 100755 t/t4211-line-log.sh
create mode 100644 t/t4211/expect.beginning-of-file
create mode 100644 t/t4211/expect.end-of-file
create mode 100644 t/t4211/expect.move-support-f
create mode 100644 t/t4211/expect.simple-f
create mode 100644 t/t4211/expect.simple-f-to-main
create mode 100644 t/t4211/expect.simple-main
create mode 100644 t/t4211/expect.simple-main-to-end
create mode 100644 t/t4211/expect.two-ranges
create mode 100644 t/t4211/expect.vanishes-early
create mode 100644 t/t4211/history.export
--
1.8.2.rc1.388.g1bd82c8
^ permalink raw reply
* [PATCH v8 3/5] Export rewrite_parents() for 'log -L'
From: Thomas Rast @ 2013-02-28 16:38 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Bo Yang, Zbigniew Jędrzejewski-Szmek,
Will Palmer
In-Reply-To: <cover.1362069310.git.trast@student.ethz.ch>
From: Bo Yang <struggleyb.nku@gmail.com>
The function rewrite_one is used to rewrite a single
parent of the current commit, and is used by rewrite_parents
to rewrite all the parents.
Decouple the dependence between them by making rewrite_one
a callback function that is passed to rewrite_parents. Then
export rewrite_parents for reuse by the line history browser.
We will use this function in line.c.
Signed-off-by: Bo Yang <struggleyb.nku@gmail.com>
Signed-off-by: Thomas Rast <trast@student.ethz.ch>
---
revision.c | 13 ++++---------
revision.h | 10 ++++++++++
2 files changed, 14 insertions(+), 9 deletions(-)
diff --git a/revision.c b/revision.c
index ef60205..46319d5 100644
--- a/revision.c
+++ b/revision.c
@@ -2173,12 +2173,6 @@ int prepare_revision_walk(struct rev_info *revs)
return 0;
}
-enum rewrite_result {
- rewrite_one_ok,
- rewrite_one_noparents,
- rewrite_one_error
-};
-
static enum rewrite_result rewrite_one(struct rev_info *revs, struct commit **pp)
{
struct commit_list *cache = NULL;
@@ -2200,12 +2194,13 @@ static enum rewrite_result rewrite_one(struct rev_info *revs, struct commit **pp
}
}
-static int rewrite_parents(struct rev_info *revs, struct commit *commit)
+int rewrite_parents(struct rev_info *revs, struct commit *commit,
+ rewrite_parent_fn_t rewrite_parent)
{
struct commit_list **pp = &commit->parents;
while (*pp) {
struct commit_list *parent = *pp;
- switch (rewrite_one(revs, &parent->item)) {
+ switch (rewrite_parent(revs, &parent->item)) {
case rewrite_one_ok:
break;
case rewrite_one_noparents:
@@ -2371,7 +2366,7 @@ enum commit_action simplify_commit(struct rev_info *revs, struct commit *commit)
if (action == commit_show &&
!revs->show_all &&
revs->prune && revs->dense && want_ancestry(revs)) {
- if (rewrite_parents(revs, commit) < 0)
+ if (rewrite_parents(revs, commit, rewrite_one) < 0)
return commit_error;
}
return action;
diff --git a/revision.h b/revision.h
index 5da09ee..640110d 100644
--- a/revision.h
+++ b/revision.h
@@ -241,4 +241,14 @@ enum commit_action {
extern enum commit_action get_commit_action(struct rev_info *revs, struct commit *commit);
extern enum commit_action simplify_commit(struct rev_info *revs, struct commit *commit);
+enum rewrite_result {
+ rewrite_one_ok,
+ rewrite_one_noparents,
+ rewrite_one_error
+};
+
+typedef enum rewrite_result (*rewrite_parent_fn_t)(struct rev_info *revs, struct commit **pp);
+
+extern int rewrite_parents(struct rev_info *revs, struct commit *commit,
+ rewrite_parent_fn_t rewrite_parent);
#endif
--
1.8.2.rc1.388.g1bd82c8
^ permalink raw reply related
* [PATCH v8 1/5] Refactor parse_loc
From: Thomas Rast @ 2013-02-28 16:38 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Bo Yang, Zbigniew Jędrzejewski-Szmek,
Will Palmer
In-Reply-To: <cover.1362069310.git.trast@student.ethz.ch>
From: Bo Yang <struggleyb.nku@gmail.com>
We want to use the same style of -L n,m argument for 'git log -L' as
for git-blame. Refactor the argument parsing of the range arguments
from builtin/blame.c to the (new) file that will hold the 'git log -L'
logic.
To accommodate different data structures in blame and log -L, the file
contents are abstracted away; parse_range_arg takes a callback that it
uses to get the contents of a line of the (notional) file.
The new test is for a case that made me pause during debugging: the
'blame -L with invalid end' test was the only one that noticed an
outright failure to parse the end *at all*. So make a more explicit
test for that.
Signed-off-by: Bo Yang <struggleyb.nku@gmail.com>
Signed-off-by: Thomas Rast <trast@student.ethz.ch>
---
Documentation/blame-options.txt | 19 +------
Documentation/line-range-format.txt | 18 +++++++
Makefile | 2 +
builtin/blame.c | 99 +++-------------------------------
line-log.c | 105 ++++++++++++++++++++++++++++++++++++
line-log.h | 23 ++++++++
t/t8003-blame-corner-cases.sh | 6 +++
7 files changed, 163 insertions(+), 109 deletions(-)
create mode 100644 Documentation/line-range-format.txt
create mode 100644 line-log.c
create mode 100644 line-log.h
diff --git a/Documentation/blame-options.txt b/Documentation/blame-options.txt
index b0d31df..6998d9f 100644
--- a/Documentation/blame-options.txt
+++ b/Documentation/blame-options.txt
@@ -13,24 +13,7 @@
Annotate only the given line range. <start> and <end> can take
one of these forms:
- - number
-+
-If <start> or <end> is a number, it specifies an
-absolute line number (lines count from 1).
-+
-
-- /regex/
-+
-This form will use the first line matching the given
-POSIX regex. If <end> is a regex, it will search
-starting at the line given by <start>.
-+
-
-- +offset or -offset
-+
-This is only valid for <end> and will specify a number
-of lines before or after the line given by <start>.
-+
+include::line-range-format.txt[]
-l::
Show long rev (Default: off).
diff --git a/Documentation/line-range-format.txt b/Documentation/line-range-format.txt
new file mode 100644
index 0000000..265bc23
--- /dev/null
+++ b/Documentation/line-range-format.txt
@@ -0,0 +1,18 @@
+- number
++
+If <start> or <end> is a number, it specifies an
+absolute line number (lines count from 1).
++
+
+- /regex/
++
+This form will use the first line matching the given
+POSIX regex. If <end> is a regex, it will search
+starting at the line given by <start>.
++
+
+- +offset or -offset
++
+This is only valid for <end> and will specify a number
+of lines before or after the line given by <start>.
++
diff --git a/Makefile b/Makefile
index 26d3332..76831fe 100644
--- a/Makefile
+++ b/Makefile
@@ -683,6 +683,7 @@ LIB_H += help.h
LIB_H += http.h
LIB_H += kwset.h
LIB_H += levenshtein.h
+LIB_H += line-log.h
LIB_H += list-objects.h
LIB_H += ll-merge.h
LIB_H += log-tree.h
@@ -798,6 +799,7 @@ LIB_OBJS += hex.o
LIB_OBJS += ident.o
LIB_OBJS += kwset.o
LIB_OBJS += levenshtein.o
+LIB_OBJS += line-log.o
LIB_OBJS += list-objects.o
LIB_OBJS += ll-merge.o
LIB_OBJS += lockfile.o
diff --git a/builtin/blame.c b/builtin/blame.c
index 86100e9..c933b15 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -21,6 +21,7 @@
#include "parse-options.h"
#include "utf8.h"
#include "userdiff.h"
+#include "line-log.h"
static char blame_usage[] = N_("git blame [options] [rev-opts] [rev] [--] file");
@@ -566,11 +567,16 @@ static void dup_entry(struct blame_entry *dst, struct blame_entry *src)
dst->score = 0;
}
-static const char *nth_line(struct scoreboard *sb, int lno)
+static const char *nth_line(struct scoreboard *sb, long lno)
{
return sb->final_buf + sb->lineno[lno];
}
+static const char *nth_line_cb(void *data, long lno)
+{
+ return nth_line((struct scoreboard *)data, lno);
+}
+
/*
* It is known that lines between tlno to same came from parent, and e
* has an overlap with that range. it also is known that parent's
@@ -1927,83 +1933,6 @@ static const char *add_prefix(const char *prefix, const char *path)
}
/*
- * Parsing of (comma separated) one item in the -L option
- */
-static const char *parse_loc(const char *spec,
- struct scoreboard *sb, long lno,
- long begin, long *ret)
-{
- char *term;
- const char *line;
- long num;
- int reg_error;
- regex_t regexp;
- regmatch_t match[1];
-
- /* Allow "-L <something>,+20" to mean starting at <something>
- * for 20 lines, or "-L <something>,-5" for 5 lines ending at
- * <something>.
- */
- if (1 < begin && (spec[0] == '+' || spec[0] == '-')) {
- num = strtol(spec + 1, &term, 10);
- if (term != spec + 1) {
- if (spec[0] == '-')
- num = 0 - num;
- if (0 < num)
- *ret = begin + num - 2;
- else if (!num)
- *ret = begin;
- else
- *ret = begin + num;
- return term;
- }
- return spec;
- }
- num = strtol(spec, &term, 10);
- if (term != spec) {
- *ret = num;
- return term;
- }
- if (spec[0] != '/')
- return spec;
-
- /* it could be a regexp of form /.../ */
- for (term = (char *) spec + 1; *term && *term != '/'; term++) {
- if (*term == '\\')
- term++;
- }
- if (*term != '/')
- return spec;
-
- /* try [spec+1 .. term-1] as regexp */
- *term = 0;
- begin--; /* input is in human terms */
- line = nth_line(sb, begin);
-
- if (!(reg_error = regcomp(®exp, spec + 1, REG_NEWLINE)) &&
- !(reg_error = regexec(®exp, line, 1, match, 0))) {
- const char *cp = line + match[0].rm_so;
- const char *nline;
-
- while (begin++ < lno) {
- nline = nth_line(sb, begin);
- if (line <= cp && cp < nline)
- break;
- line = nline;
- }
- *ret = begin;
- regfree(®exp);
- *term++ = '/';
- return term;
- }
- else {
- char errbuf[1024];
- regerror(reg_error, ®exp, errbuf, 1024);
- die("-L parameter '%s': %s", spec + 1, errbuf);
- }
-}
-
-/*
* Parsing of -L option
*/
static void prepare_blame_range(struct scoreboard *sb,
@@ -2011,15 +1940,7 @@ static void prepare_blame_range(struct scoreboard *sb,
long lno,
long *bottom, long *top)
{
- const char *term;
-
- term = parse_loc(bottomtop, sb, lno, 1, bottom);
- if (*term == ',') {
- term = parse_loc(term + 1, sb, lno, *bottom + 1, top);
- if (*term)
- usage(blame_usage);
- }
- if (*term)
+ if (parse_range_arg(bottomtop, nth_line_cb, sb, lno, bottom, top))
usage(blame_usage);
}
@@ -2569,10 +2490,6 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
bottom = top = 0;
if (bottomtop)
prepare_blame_range(&sb, bottomtop, lno, &bottom, &top);
- if (bottom && top && top < bottom) {
- long tmp;
- tmp = top; top = bottom; bottom = tmp;
- }
if (bottom < 1)
bottom = 1;
if (top < 1)
diff --git a/line-log.c b/line-log.c
new file mode 100644
index 0000000..a24a86b
--- /dev/null
+++ b/line-log.c
@@ -0,0 +1,105 @@
+#include "git-compat-util.h"
+#include "line-log.h"
+
+/*
+ * Parse one item in the -L option
+ */
+const char *parse_loc(const char *spec, nth_line_fn_t nth_line,
+ void *data, long lines, long begin, long *ret)
+{
+ char *term;
+ const char *line;
+ long num;
+ int reg_error;
+ regex_t regexp;
+ regmatch_t match[1];
+
+ /*
+ * Allow "-L <something>,+20" to mean starting at <something>
+ * for 20 lines, or "-L <something>,-5" for 5 lines ending at
+ * <something>.
+ */
+ if (begin != -1 && (spec[0] == '+' || spec[0] == '-')) {
+ num = strtol(spec + 1, &term, 10);
+ if (term != spec + 1) {
+ if (spec[0] == '-')
+ num = 0 - num;
+ if (0 < num)
+ *ret = begin + num - 2;
+ else if (!num)
+ *ret = begin;
+ else
+ *ret = begin + num;
+ return term;
+ }
+ return spec;
+ }
+ num = strtol(spec, &term, 10);
+ if (term != spec) {
+ *ret = num;
+ return term;
+ }
+ if (spec[0] != '/')
+ return spec;
+
+ /* it could be a regexp of form /.../ */
+ for (term = (char *) spec + 1; *term && *term != '/'; term++) {
+ if (*term == '\\')
+ term++;
+ }
+ if (*term != '/')
+ return spec;
+
+ /* try [spec+1 .. term-1] as regexp */
+ *term = 0;
+ if (begin == -1)
+ begin = 1;
+ begin--; /* input is in human terms */
+ line = nth_line(data, begin);
+
+ if (!(reg_error = regcomp(®exp, spec + 1, REG_NEWLINE)) &&
+ !(reg_error = regexec(®exp, line, 1, match, 0))) {
+ const char *cp = line + match[0].rm_so;
+ const char *nline;
+
+ while (begin++ < lines) {
+ nline = nth_line(data, begin);
+ if (line <= cp && cp < nline)
+ break;
+ line = nline;
+ }
+ *ret = begin;
+ regfree(®exp);
+ *term++ = '/';
+ return term;
+ } else {
+ char errbuf[1024];
+ regerror(reg_error, ®exp, errbuf, 1024);
+ die("-L parameter '%s': %s", spec + 1, errbuf);
+ }
+}
+
+int parse_range_arg(const char *arg, nth_line_fn_t nth_line_cb,
+ void *cb_data, long lines, long *begin, long *end)
+{
+ arg = parse_loc(arg, nth_line_cb, cb_data, lines, -1, begin);
+
+ if (*arg == ',') {
+ arg = parse_loc(arg+1, nth_line_cb, cb_data, lines, *begin+1, end);
+ if (*begin > *end) {
+ long tmp = *begin;
+ *begin = *end;
+ *end = tmp;
+ }
+ }
+
+ if (*begin <= 0)
+ *begin = 1;
+ if (*end > lines)
+ *end = lines;
+
+ if (*arg)
+ return -1;
+
+ return 0;
+}
diff --git a/line-log.h b/line-log.h
new file mode 100644
index 0000000..22b1c80
--- /dev/null
+++ b/line-log.h
@@ -0,0 +1,23 @@
+#ifndef LINE_LOG_H
+#define LINE_LOG_H
+
+/*
+ * Parse one item in an -L begin,end option w.r.t. the notional file
+ * object 'cb_data' consisting of 'lines' lines.
+ *
+ * The 'nth_line_cb' callback is used to determine the start of the
+ * line 'lno' inside the 'cb_data'. The caller is expected to already
+ * have a suitable map at hand to make this a constant-time lookup.
+ *
+ * Returns 0 in case of success and -1 if there was an error. The
+ * caller should print a usage message in the latter case.
+ */
+
+typedef const char *(*nth_line_fn_t)(void *data, long lno);
+
+extern int parse_range_arg(const char *arg,
+ nth_line_fn_t nth_line_cb,
+ void *cb_data, long lines,
+ long *begin, long *end);
+
+#endif /* LINE_LOG_H */
diff --git a/t/t8003-blame-corner-cases.sh b/t/t8003-blame-corner-cases.sh
index 230143c..e7cac1d 100755
--- a/t/t8003-blame-corner-cases.sh
+++ b/t/t8003-blame-corner-cases.sh
@@ -175,6 +175,12 @@ test_expect_success 'blame -L with invalid end' '
grep "has only 2 lines" errors
'
+test_expect_success 'blame parses <end> part of -L' '
+ git blame -L1,1 tres >out &&
+ cat out &&
+ test $(wc -l < out) -eq 1
+'
+
test_expect_success 'indent of line numbers, nine lines' '
git blame nine_lines >actual &&
test $(grep -c " " actual) = 0
--
1.8.2.rc1.388.g1bd82c8
^ permalink raw reply related
* [PATCH v8 5/5] log -L: :pattern:file syntax to find by funcname
From: Thomas Rast @ 2013-02-28 16:38 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Bo Yang, Zbigniew Jędrzejewski-Szmek,
Will Palmer
In-Reply-To: <cover.1362069310.git.trast@student.ethz.ch>
This new syntax finds a funcname matching /pattern/, and then takes from there
up to (but not including) the next funcname. So you can say
git log -L:main:main.c
and it will dig up the main() function and show its line-log, provided
there are no other funcnames matching 'main'.
Signed-off-by: Thomas Rast <trast@student.ethz.ch>
---
Documentation/line-range-format.txt | 7 ++
builtin/blame.c | 2 +-
line-log.c | 130 +++++++++++++++++++++++++++++++++++-
line-log.h | 3 +-
t/t4211-line-log.sh | 3 +-
t/t4211/expect.simple-f-to-main | 100 +++++++++++++++++++++++++++
t/t4211/expect.simple-main-to-end | 70 +++++++++++++++++++
7 files changed, 310 insertions(+), 5 deletions(-)
create mode 100644 t/t4211/expect.simple-f-to-main
create mode 100644 t/t4211/expect.simple-main-to-end
diff --git a/Documentation/line-range-format.txt b/Documentation/line-range-format.txt
index 9ce0688..5279a76 100644
--- a/Documentation/line-range-format.txt
+++ b/Documentation/line-range-format.txt
@@ -22,3 +22,10 @@ of lines before or after the line given by <start>.
A literal dollar sign can be used as a shorthand for the last line in
the file.
+
+
+- :regex
++
+If the entire range is of the form :regex, it denotes the range from
+the first funcname line that matches <regex>, up to the next funcname
+line.
++
diff --git a/builtin/blame.c b/builtin/blame.c
index c933b15..b7d210f 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -1940,7 +1940,7 @@ static void prepare_blame_range(struct scoreboard *sb,
long lno,
long *bottom, long *top)
{
- if (parse_range_arg(bottomtop, nth_line_cb, sb, lno, bottom, top))
+ if (parse_range_arg(bottomtop, nth_line_cb, sb, lno, bottom, top, sb->path))
usage(blame_usage);
}
diff --git a/line-log.c b/line-log.c
index 789b7c4..a74bbaf 100644
--- a/line-log.c
+++ b/line-log.c
@@ -11,6 +11,7 @@
#include "strbuf.h"
#include "log-tree.h"
#include "graph.h"
+#include "userdiff.h"
#include "line-log.h"
void range_set_grow (struct range_set *rs, size_t extra)
@@ -531,9 +532,130 @@ const char *parse_loc(const char *spec, nth_line_fn_t nth_line,
}
}
+static int match_funcname(xdemitconf_t *xecfg, const char *bol, const char *eol)
+{
+ if (xecfg) {
+ char buf[1];
+ return xecfg->find_func(bol, eol - bol, buf, 1,
+ xecfg->find_func_priv) >= 0;
+ }
+
+ if (bol == eol)
+ return 0;
+ if (isalpha(*bol) || *bol == '_' || *bol == '$')
+ return 1;
+ return 0;
+}
+
+static const char *find_funcname_matching_regexp(xdemitconf_t *xecfg, const char *start,
+ regex_t *regexp)
+{
+ int reg_error;
+ regmatch_t match[1];
+ while (1) {
+ const char *bol, *eol;
+ reg_error = regexec(regexp, start, 1, match, 0);
+ if (reg_error == REG_NOMATCH)
+ return NULL;
+ else if (reg_error) {
+ char errbuf[1024];
+ regerror(reg_error, regexp, errbuf, 1024);
+ die("-L parameter: regexec() failed: %s", errbuf);
+ }
+ /* determine extent of line matched */
+ bol = start+match[0].rm_so;
+ eol = start+match[0].rm_eo;
+ while (bol > start && *bol != '\n')
+ bol--;
+ if (*bol == '\n')
+ bol++;
+ while (*eol && *eol != '\n')
+ eol++;
+ if (*eol == '\n')
+ eol++;
+ /* is it a funcname line? */
+ if (match_funcname(xecfg, (char*) bol, (char*) eol))
+ return bol;
+ start = eol;
+ }
+}
+
+static const char *parse_range_funcname(const char *arg, nth_line_fn_t nth_line_cb,
+ void *cb_data, long lines, long *begin, long *end,
+ const char *path)
+{
+ const char *pattern;
+ const char *term;
+ struct userdiff_driver *drv;
+ xdemitconf_t *xecfg = NULL;
+ const char *start;
+ const char *p;
+ int reg_error;
+ regex_t regexp;
+
+ pattern = arg+1;
+ term = (char*) strchr(pattern, ':');
+ if (term) {
+ assert(!begin);
+ return term;
+ }
+ /* all of the rest is the regex */
+ term = pattern + strlen(pattern);
+
+ start = nth_line_cb(cb_data, 0);
+
+ drv = userdiff_find_by_path(path);
+ if (drv && drv->funcname.pattern) {
+ const struct userdiff_funcname *pe = &drv->funcname;
+ xecfg = xcalloc(1, sizeof(*xecfg));
+ xdiff_set_find_func(xecfg, pe->pattern, pe->cflags);
+ }
+
+ reg_error = regcomp(®exp, pattern, REG_NEWLINE);
+ if (reg_error) {
+ char errbuf[1024];
+ regerror(reg_error, ®exp, errbuf, 1024);
+ die("-L parameter '%s': %s", pattern, errbuf);
+ }
+
+ p = find_funcname_matching_regexp(xecfg, (char*) start, ®exp);
+ if (!p)
+ die("-L parameter '%s': no match", pattern);
+ *begin = 0;
+ while (p > nth_line_cb(cb_data, *begin))
+ (*begin)++;
+
+ if (*begin >= lines)
+ die("-L parameter '%s' matches at EOF", pattern);
+
+ *end = *begin+1;
+ while (*end < lines) {
+ const char *bol = nth_line_cb(cb_data, *end);
+ const char *eol = nth_line_cb(cb_data, *end+1);
+ if (match_funcname(xecfg, bol, eol))
+ break;
+ (*end)++;
+ }
+
+ regfree(®exp);
+ free(xecfg);
+
+ /* compensate for 1-based numbering */
+ (*begin)++;
+
+ return term;
+}
+
int parse_range_arg(const char *arg, nth_line_fn_t nth_line_cb,
- void *cb_data, long lines, long *begin, long *end)
+ void *cb_data, long lines, long *begin, long *end,
+ const char *path)
{
+ if (*arg == ':') {
+ arg = parse_range_funcname(arg, nth_line_cb, cb_data, lines, begin, end, path);
+ if (*arg)
+ return -1;
+ }
+
arg = parse_loc(arg, nth_line_cb, cb_data, lines, -1, begin);
if (*arg == ',') {
@@ -553,6 +675,9 @@ int parse_range_arg(const char *arg, nth_line_fn_t nth_line_cb,
const char *skip_range_arg(const char *arg)
{
+ if (*arg == ':')
+ return parse_range_funcname(arg, NULL, NULL, 0, NULL, NULL, NULL);
+
arg = parse_loc(arg, NULL, NULL, 0, -1, 0);
if (*arg == ',')
@@ -681,7 +806,8 @@ static const char *nth_line(void *data, long line)
cb_data.line_ends = ends;
if (parse_range_arg(range_part, nth_line, &cb_data,
- lines, &begin, &end))
+ lines, &begin, &end,
+ spec->path))
die("malformed -L argument '%s'", range_part);
if (begin < 1)
begin = 1;
diff --git a/line-log.h b/line-log.h
index e3ec094..071e739 100644
--- a/line-log.h
+++ b/line-log.h
@@ -20,7 +20,8 @@
extern int parse_range_arg(const char *arg,
nth_line_fn_t nth_line_cb,
void *cb_data, long lines,
- long *begin, long *end);
+ long *begin, long *end,
+ const char *path);
/*
* Scan past a range argument that could be parsed by
diff --git a/t/t4211-line-log.sh b/t/t4211-line-log.sh
index 639a16f..d488690 100755
--- a/t/t4211-line-log.sh
+++ b/t/t4211-line-log.sh
@@ -13,14 +13,15 @@ canned_test () {
git log $1 >actual &&
test_cmp \"\$TEST_DIRECTORY\"/t4211/expect.$2 actual
"
- cp actual "$TEST_DIRECTORY"/t4211/expect.$2
}
canned_test "-L 4,12:a.c simple" simple-f
canned_test "-L 4,+9:a.c simple" simple-f
canned_test "-L '/long f/,/^}/:a.c' simple" simple-f
+canned_test "-L :f:a.c simple" simple-f-to-main
canned_test "-L '/main/,/^}/:a.c' simple" simple-main
+canned_test "-L :main:a.c simple" simple-main-to-end
canned_test "-L 1,+4:a.c simple" beginning-of-file
canned_test "-L 0,+5:a.c simple" beginning-of-file
diff --git a/t/t4211/expect.simple-f-to-main b/t/t4211/expect.simple-f-to-main
new file mode 100644
index 0000000..a475768
--- /dev/null
+++ b/t/t4211/expect.simple-f-to-main
@@ -0,0 +1,100 @@
+commit 39b6eb2d5b706d3322184a169f666f25ed3fbd00
+Author: Thomas Rast <trast@student.ethz.ch>
+Date: Thu Feb 28 10:45:41 2013 +0100
+
+ touch comment
+
+diff --git a/a.c b/a.c
+--- a/a.c
++++ b/a.c
+@@ -3,14 +3,14 @@
+ long f(long x)
+ {
+ int s = 0;
+ while (x) {
+ x >>= 1;
+ s++;
+ }
+ return s;
+ }
+
+ /*
+- * A comment.
++ * This is only an example!
+ */
+
+
+commit a6eb82647d5d67f893da442f8f9375fd89a3b1e2
+Author: Thomas Rast <trast@student.ethz.ch>
+Date: Thu Feb 28 10:45:16 2013 +0100
+
+ touch both functions
+
+diff --git a/a.c b/a.c
+--- a/a.c
++++ b/a.c
+@@ -3,14 +3,14 @@
+-int f(int x)
++long f(long x)
+ {
+ int s = 0;
+ while (x) {
+ x >>= 1;
+ s++;
+ }
+ return s;
+ }
+
+ /*
+ * A comment.
+ */
+
+
+commit f04fb20f2c77850996cba739709acc6faecc58f7
+Author: Thomas Rast <trast@student.ethz.ch>
+Date: Thu Feb 28 10:44:55 2013 +0100
+
+ change f()
+
+diff --git a/a.c b/a.c
+--- a/a.c
++++ b/a.c
+@@ -3,13 +3,14 @@
+ int f(int x)
+ {
+ int s = 0;
+ while (x) {
+ x >>= 1;
+ s++;
+ }
++ return s;
+ }
+
+ /*
+ * A comment.
+ */
+
+
+commit de4c48ae814792c02a49c4c3c0c757ae69c55f6a
+Author: Thomas Rast <trast@student.ethz.ch>
+Date: Thu Feb 28 10:44:48 2013 +0100
+
+ initial
+
+diff --git a/a.c b/a.c
+--- /dev/null
++++ b/a.c
+@@ -0,0 +3,13 @@
++int f(int x)
++{
++ int s = 0;
++ while (x) {
++ x >>= 1;
++ s++;
++ }
++}
++
++/*
++ * A comment.
++ */
++
diff --git a/t/t4211/expect.simple-main-to-end b/t/t4211/expect.simple-main-to-end
new file mode 100644
index 0000000..8480bd9
--- /dev/null
+++ b/t/t4211/expect.simple-main-to-end
@@ -0,0 +1,70 @@
+commit 4659538844daa2849b1a9e7d6fadb96fcd26fc83
+Author: Thomas Rast <trast@student.ethz.ch>
+Date: Thu Feb 28 10:48:43 2013 +0100
+
+ change back to complete line
+
+diff --git a/a.c b/a.c
+--- a/a.c
++++ b/a.c
+@@ -18,5 +18,7 @@
+ int main ()
+ {
+ printf("%ld\n", f(15));
+ return 0;
+-}
+\ No newline at end of file
++}
++
++/* incomplete lines are bad! */
+
+commit 100b61a6f2f720f812620a9d10afb3a960ccb73c
+Author: Thomas Rast <trast@student.ethz.ch>
+Date: Thu Feb 28 10:48:10 2013 +0100
+
+ change to an incomplete line at end
+
+diff --git a/a.c b/a.c
+--- a/a.c
++++ b/a.c
+@@ -18,5 +18,5 @@
+ int main ()
+ {
+ printf("%ld\n", f(15));
+ return 0;
+-}
++}
+\ No newline at end of file
+
+commit a6eb82647d5d67f893da442f8f9375fd89a3b1e2
+Author: Thomas Rast <trast@student.ethz.ch>
+Date: Thu Feb 28 10:45:16 2013 +0100
+
+ touch both functions
+
+diff --git a/a.c b/a.c
+--- a/a.c
++++ b/a.c
+@@ -17,5 +17,5 @@
+ int main ()
+ {
+- printf("%d\n", f(15));
++ printf("%ld\n", f(15));
+ return 0;
+ }
+
+commit de4c48ae814792c02a49c4c3c0c757ae69c55f6a
+Author: Thomas Rast <trast@student.ethz.ch>
+Date: Thu Feb 28 10:44:48 2013 +0100
+
+ initial
+
+diff --git a/a.c b/a.c
+--- /dev/null
++++ b/a.c
+@@ -0,0 +16,5 @@
++int main ()
++{
++ printf("%d\n", f(15));
++ return 0;
++}
--
1.8.2.rc1.388.g1bd82c8
^ permalink raw reply related
* [PATCH v8 4/5] Implement line-history search (git log -L)
From: Thomas Rast @ 2013-02-28 16:38 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Bo Yang, Zbigniew Jędrzejewski-Szmek,
Will Palmer
In-Reply-To: <cover.1362069310.git.trast@student.ethz.ch>
This is a rewrite of much of Bo's work, mainly in an effort to split
it into smaller, easier to understand routines.
The algorithm is built around the struct range_set, which encodes a
series of line ranges as intervals [a,b). This is used in two
contexts:
* A set of lines we are tracking (which will change as we dig through
history).
* To encode diffs, as pairs of ranges.
The main routine is range_set_map_across_diff(). It processes the
diff between a commit C and some parent P. It determines which diff
hunks are relevant to the ranges tracked in C, and computes the new
ranges for P.
The algorithm is then simply to process history in topological order
from newest to oldest, computing ranges and (partial) diffs. At
branch points, we need to merge the ranges we are watching. We will
find that many commits do not affect the chosen ranges, and mark them
TREESAME (in addition to those already filtered by pathspec limiting).
Another pass of history simplification then gets rid of such commits.
This is wired as an extra filtering pass in the log machinery. This
currently only reduces code duplication, but should allow for other
simplifications and options to be used.
Finally, we hook a diff printer into the output chain. Ideally we
would wire directly into the diff logic, to optionally use features
like word diff. However, that will require some major reworking of
the diff chain, so we completely replace the output with our own diff
for now.
Signed-off-by: Bo Yang <struggleyb.nku@gmail.com>
Signed-off-by: Thomas Rast <trast@student.ethz.ch>
---
Documentation/git-log.txt | 22 +
builtin/log.c | 31 +
line-log.c | 1208 +++++++++++++++++++++++++++++++++++++-
line-log.h | 56 ++
log-tree.c | 4 +
revision.c | 9 +
revision.h | 6 +-
t/t4211-line-log.sh | 41 ++
t/t4211/expect.beginning-of-file | 43 ++
t/t4211/expect.end-of-file | 62 ++
t/t4211/expect.move-support-f | 40 ++
t/t4211/expect.simple-f | 59 ++
t/t4211/expect.simple-main | 68 +++
t/t4211/expect.two-ranges | 102 ++++
t/t4211/expect.vanishes-early | 39 ++
t/t4211/history.export | 330 +++++++++++
16 files changed, 2112 insertions(+), 8 deletions(-)
create mode 100755 t/t4211-line-log.sh
create mode 100644 t/t4211/expect.beginning-of-file
create mode 100644 t/t4211/expect.end-of-file
create mode 100644 t/t4211/expect.move-support-f
create mode 100644 t/t4211/expect.simple-f
create mode 100644 t/t4211/expect.simple-main
create mode 100644 t/t4211/expect.two-ranges
create mode 100644 t/t4211/expect.vanishes-early
create mode 100644 t/t4211/history.export
diff --git a/Documentation/git-log.txt b/Documentation/git-log.txt
index 69db578..8727c60 100644
--- a/Documentation/git-log.txt
+++ b/Documentation/git-log.txt
@@ -69,6 +69,23 @@ produced by --stat etc.
Note that only message is considered, if also a diff is shown
its size is not included.
+-L <start>,<end>:<file>::
+ Trace the evolution of the line range given by "<start>,<end>"
+ within the <file>. You may not give any pathspec limiters.
+ This is currently limited to a walk starting from a single
+ revision, i.e., you may only give zero or one positive
+ revision arguments.
+
+<start> and <end> can take one of these forms:
+
+include::line-range-format.txt[]
+You can specify this option more than once.
+
+
+--full-line-diff::
+ Always print the interesting range even if the current commit
+ does not change any line of the range.
+
[\--] <path>...::
Show only commits that are enough to explain how the files
that match the specified paths came to be. See "History
@@ -138,6 +155,11 @@ Examples
This makes sense only when following a strict policy of merging all
topic branches when staying on a single integration branch.
+git log -L '/int main/',/^}/:main.c::
+
+ Shows how the function `main()` in the file 'main.c' evolved
+ over time.
+
`git log -3`::
Limits the number of commits to show to 3.
diff --git a/builtin/log.c b/builtin/log.c
index 8f0b2e8..17b94ef 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -19,6 +19,7 @@
#include "remote.h"
#include "string-list.h"
#include "parse-options.h"
+#include "line-log.h"
#include "branch.h"
#include "streaming.h"
#include "version.h"
@@ -41,6 +42,12 @@
NULL
};
+struct line_opt_callback_data {
+ struct rev_info *rev;
+ const char *prefix;
+ struct string_list args;
+};
+
static int parse_decoration_style(const char *var, const char *value)
{
switch (git_config_maybe_bool(var, value)) {
@@ -75,6 +82,19 @@ static int decorate_callback(const struct option *opt, const char *arg, int unse
return 0;
}
+static int log_line_range_callback(const struct option *option, const char *arg, int unset)
+{
+ struct line_opt_callback_data *data = option->value;
+
+ if (!arg)
+ return -1;
+
+ data->rev->line_level_traverse = 1;
+ string_list_append(&data->args, arg);
+
+ return 0;
+}
+
static void cmd_log_init_defaults(struct rev_info *rev)
{
if (fmt_pretty)
@@ -97,6 +117,7 @@ static void cmd_log_init_finish(int argc, const char **argv, const char *prefix,
{
struct userformat_want w;
int quiet = 0, source = 0, mailmap = 0;
+ static struct line_opt_callback_data line_cb = {0, 0, STRING_LIST_INIT_DUP};
const struct option builtin_log_options[] = {
OPT_BOOLEAN(0, "quiet", &quiet, N_("suppress diff output")),
@@ -104,9 +125,15 @@ static void cmd_log_init_finish(int argc, const char **argv, const char *prefix,
OPT_BOOLEAN(0, "use-mailmap", &mailmap, N_("Use mail map file")),
{ OPTION_CALLBACK, 0, "decorate", NULL, NULL, N_("decorate options"),
PARSE_OPT_OPTARG, decorate_callback},
+ OPT_CALLBACK('L', NULL, &line_cb, "n,m:file",
+ "Process line range n,m in file, counting from 1",
+ log_line_range_callback),
OPT_END()
};
+ line_cb.rev = rev;
+ line_cb.prefix = prefix;
+
mailmap = use_mailmap_config;
argc = parse_options(argc, argv, prefix,
builtin_log_options, builtin_log_usage,
@@ -160,6 +187,10 @@ static void cmd_log_init_finish(int argc, const char **argv, const char *prefix,
rev->show_decorations = 1;
load_ref_decorations(decoration_style);
}
+
+ if (rev->line_level_traverse)
+ line_log_init(rev, line_cb.prefix, &line_cb.args);
+
setup_pager();
}
diff --git a/line-log.c b/line-log.c
index b167b00..789b7c4 100644
--- a/line-log.c
+++ b/line-log.c
@@ -1,6 +1,442 @@
#include "git-compat-util.h"
+#include "cache.h"
+#include "tag.h"
+#include "blob.h"
+#include "tree.h"
+#include "diff.h"
+#include "commit.h"
+#include "decorate.h"
+#include "revision.h"
+#include "xdiff-interface.h"
+#include "strbuf.h"
+#include "log-tree.h"
+#include "graph.h"
#include "line-log.h"
+void range_set_grow (struct range_set *rs, size_t extra)
+{
+ ALLOC_GROW(rs->ranges, rs->nr + extra, rs->alloc);
+}
+
+/* Either initialization would be fine */
+#define RANGE_SET_INIT {0}
+
+void range_set_init (struct range_set *rs, size_t prealloc)
+{
+ rs->alloc = rs->nr = 0;
+ rs->ranges = NULL;
+ if (prealloc)
+ range_set_grow(rs, prealloc);
+}
+
+void range_set_release (struct range_set *rs)
+{
+ free(rs->ranges);
+ rs->alloc = rs->nr = 0;
+ rs->ranges = NULL;
+}
+
+/* dst must be uninitialized! */
+void range_set_copy (struct range_set *dst, struct range_set *src)
+{
+ range_set_init(dst, src->nr);
+ memcpy(dst->ranges, src->ranges, src->nr*sizeof(struct range_set));
+ dst->nr = src->nr;
+}
+void range_set_move (struct range_set *dst, struct range_set *src)
+{
+ range_set_release(dst);
+ dst->ranges = src->ranges;
+ dst->nr = src->nr;
+ dst->alloc = src->alloc;
+ src->ranges = NULL;
+ src->alloc = src->nr = 0;
+}
+
+/* tack on a _new_ range _at the end_ */
+void range_set_append (struct range_set *rs, long a, long b)
+{
+ assert(a <= b);
+ assert(rs->nr == 0 || rs->ranges[rs->nr-1].end <= a);
+ range_set_grow(rs, 1);
+ rs->ranges[rs->nr].start = a;
+ rs->ranges[rs->nr].end = b;
+ rs->nr++;
+}
+
+static int range_cmp (const void *_r, const void *_s)
+{
+ const struct range *r = _r;
+ const struct range *s = _s;
+
+ /* this could be simply 'return r.start-s.start', but for the types */
+ if (r->start == s->start)
+ return 0;
+ if (r->start < s->start)
+ return -1;
+ return 1;
+}
+
+/*
+ * Helper: In-place pass of sorting and merging the ranges in the
+ * range set, to re-establish the invariants after another operation
+ *
+ * NEEDSWORK currently not needed
+ */
+static void sort_and_merge_range_set (struct range_set *rs)
+{
+ int i;
+ int o = 1; /* output cursor */
+
+ qsort(rs->ranges, rs->nr, sizeof(struct range), range_cmp);
+
+ for (i = 1; i < rs->nr; i++) {
+ if (rs->ranges[i].start <= rs->ranges[o-1].end) {
+ rs->ranges[o-1].end = rs->ranges[i].end;
+ } else {
+ rs->ranges[o].start = rs->ranges[i].start;
+ rs->ranges[o].end = rs->ranges[i].end;
+ o++;
+ }
+ }
+ assert(o <= rs->nr);
+ rs->nr = o;
+}
+
+/*
+ * Union of range sets (i.e., sets of line numbers). Used to merge
+ * them when searches meet at a common ancestor.
+ *
+ * This is also where the ranges are consolidated into canonical form:
+ * overlapping and adjacent ranges are merged, and empty ranges are
+ * removed.
+ */
+static void range_set_union (struct range_set *out,
+ struct range_set *a, struct range_set *b)
+{
+ int i = 0, j = 0, o = 0;
+ struct range *ra = a->ranges;
+ struct range *rb = b->ranges;
+ /* cannot make an alias of out->ranges: it may change during grow */
+
+ assert(out->nr == 0);
+ while (i < a->nr || j < b->nr) {
+ struct range *new;
+ if (i < a->nr && j < b->nr) {
+ if (ra[i].start < rb[j].start)
+ new = &ra[i++];
+ else if (ra[i].start > rb[j].start)
+ new = &rb[j++];
+ else if (ra[i].end < rb[j].end)
+ new = &ra[i++];
+ else
+ new = &rb[j++];
+ } else if (i < a->nr) /* b exhausted */
+ new = &ra[i++];
+ else /* a exhausted */
+ new = &rb[j++];
+ if (new->start == new->end)
+ ; /* empty range */
+ else if (!o || out->ranges[o-1].end < new->start) {
+ range_set_grow(out, 1);
+ out->ranges[o].start = new->start;
+ out->ranges[o].end = new->end;
+ o++;
+ } else if (out->ranges[o-1].end < new->end) {
+ out->ranges[o-1].end = new->end;
+ }
+ }
+ out->nr = o;
+}
+
+/*
+ * Difference of range sets (out = a \ b). Pass the "interesting"
+ * ranges as 'a' and the target side of the diff as 'b': it removes
+ * the ranges for which the commit is responsible.
+ */
+static void range_set_difference (struct range_set *out,
+ struct range_set *a, struct range_set *b)
+{
+ int i, j = 0;
+ for (i = 0; i < a->nr; i++) {
+ long start = a->ranges[i].start;
+ long end = a->ranges[i].end;
+ while (start < end) {
+ while (j < b->nr && start >= b->ranges[j].end)
+ /*
+ * a: |-------
+ * b: ------|
+ */
+ j++;
+ if (j >= b->nr || end < b->ranges[j].start) {
+ /*
+ * b exhausted, or
+ * a: ----|
+ * b: |----
+ */
+ range_set_append(out, start, end);
+ break;
+ }
+ if (start >= b->ranges[j].start) {
+ /*
+ * a: |--????
+ * b: |------|
+ */
+ start = b->ranges[j].end;
+ } else if (end > b->ranges[j].start) {
+ /*
+ * a: |-----|
+ * b: |--?????
+ */
+ if (start < b->ranges[j].start)
+ range_set_append(out, start, b->ranges[j].start);
+ start = b->ranges[j].end;
+ }
+ }
+ }
+}
+
+static void diff_ranges_init (struct diff_ranges *diff)
+{
+ range_set_init(&diff->parent, 0);
+ range_set_init(&diff->target, 0);
+}
+
+static void diff_ranges_release (struct diff_ranges *diff)
+{
+ range_set_release(&diff->parent);
+ range_set_release(&diff->target);
+}
+
+void line_log_data_init(struct line_log_data *r)
+{
+ memset(r, 0, sizeof(struct line_log_data));
+ range_set_init(&r->ranges, 0);
+}
+
+static void line_log_data_clear(struct line_log_data *r)
+{
+ range_set_release(&r->ranges);
+ if (r->pair)
+ diff_free_filepair(r->pair);
+}
+
+static void free_line_log_data(struct line_log_data *r)
+{
+ while (r) {
+ struct line_log_data *next = r->next;
+ line_log_data_clear(r);
+ free(r);
+ r = next;
+ }
+}
+
+static struct line_log_data *
+search_line_log_data(struct line_log_data *list, const char *path,
+ struct line_log_data **insertion_point)
+{
+ struct line_log_data *p = list;
+ if (insertion_point)
+ *insertion_point = NULL;
+ while (p) {
+ int cmp = strcmp(p->spec->path, path);
+ if (!cmp)
+ return p;
+ if (insertion_point && cmp < 0)
+ *insertion_point = p;
+ p = p->next;
+ }
+ return NULL;
+}
+
+static void line_log_data_insert (struct line_log_data **list,
+ struct diff_filespec *spec,
+ long begin, long end)
+{
+ struct line_log_data *ip;
+ struct line_log_data *p = search_line_log_data(*list, spec->path, &ip);
+
+ if (p) {
+ range_set_append(&p->ranges, begin, end);
+ sort_and_merge_range_set(&p->ranges);
+ free_filespec(spec);
+ return;
+ }
+
+ p = xcalloc(1, sizeof(struct line_log_data));
+ p->spec = spec;
+ range_set_append(&p->ranges, begin, end);
+ if (ip) {
+ p->next = ip->next;
+ ip->next = p;
+ } else {
+ p->next = *list;
+ *list = p;
+ }
+}
+
+struct collect_diff_cbdata {
+ struct diff_ranges *diff;
+};
+
+static int collect_diff_cb (long start_a, long count_a,
+ long start_b, long count_b,
+ void *data)
+{
+ struct collect_diff_cbdata *d = data;
+
+ if (count_a >= 0)
+ range_set_append(&d->diff->parent, start_a, start_a + count_a);
+ if (count_b >= 0)
+ range_set_append(&d->diff->target, start_b, start_b + count_b);
+
+ return 0;
+}
+
+static void collect_diff (mmfile_t *parent, mmfile_t *target, struct diff_ranges *out)
+{
+ struct collect_diff_cbdata cbdata = {0};
+ xpparam_t xpp;
+ xdemitconf_t xecfg;
+ xdemitcb_t ecb;
+
+ memset(&xpp, 0, sizeof(xpp));
+ memset(&xecfg, 0, sizeof(xecfg));
+ xecfg.ctxlen = xecfg.interhunkctxlen = 0;
+
+ cbdata.diff = out;
+ xecfg.hunk_func = collect_diff_cb;
+ memset(&ecb, 0, sizeof(ecb));
+ ecb.priv = &cbdata;
+ xdi_diff(parent, target, &xpp, &xecfg, &ecb);
+}
+
+/*
+ * These are handy for debugging. Removing them with #if 0 silences
+ * the "unused function" warning.
+ */
+#if 0
+static void dump_range_set (struct range_set *rs, const char *desc)
+{
+ int i;
+ printf("range set %s (%d items):\n", desc, rs->nr);
+ for (i = 0; i < rs->nr; i++)
+ printf("\t[%ld,%ld]\n", rs->ranges[i].start, rs->ranges[i].end);
+}
+
+static void dump_line_log_data (struct line_log_data *r)
+{
+ char buf[4096];
+ while (r) {
+ snprintf(buf, 4096, "file %s\n", r->spec->path);
+ dump_range_set(&r->ranges, buf);
+ r = r->next;
+ }
+}
+
+static void dump_diff_ranges (struct diff_ranges *diff, const char *desc)
+{
+ int i;
+ assert(diff->parent.nr == diff->target.nr);
+ printf("diff ranges %s (%d items):\n", desc, diff->parent.nr);
+ printf("\tparent\ttarget\n");
+ for (i = 0; i < diff->parent.nr; i++) {
+ printf("\t[%ld,%ld]\t[%ld,%ld]\n",
+ diff->parent.ranges[i].start,
+ diff->parent.ranges[i].end,
+ diff->target.ranges[i].start,
+ diff->target.ranges[i].end);
+ }
+}
+#endif
+
+
+static int ranges_overlap (struct range *a, struct range *b)
+{
+ return !(a->end <= b->start || b->end <= a->start);
+}
+
+/*
+ * Given a diff and the set of interesting ranges, determine all hunks
+ * of the diff which touch (overlap) at least one of the interesting
+ * ranges in the target.
+ */
+static void diff_ranges_filter_touched (struct diff_ranges *out,
+ struct diff_ranges *diff,
+ struct range_set *rs)
+{
+ int i, j = 0;
+
+ assert(out->target.nr == 0);
+
+ for (i = 0; i < diff->target.nr; i++) {
+ while (diff->target.ranges[i].start > rs->ranges[j].end) {
+ j++;
+ if (j == rs->nr)
+ return;
+ }
+ if (ranges_overlap(&diff->target.ranges[i], &rs->ranges[j])) {
+ range_set_append(&out->parent,
+ diff->parent.ranges[i].start,
+ diff->parent.ranges[i].end);
+ range_set_append(&out->target,
+ diff->target.ranges[i].start,
+ diff->target.ranges[i].end);
+ }
+ }
+}
+
+/*
+ * Adjust the line counts in 'rs' to account for the lines
+ * added/removed in the diff.
+ */
+static void range_set_shift_diff (struct range_set *out,
+ struct range_set *rs,
+ struct diff_ranges *diff)
+{
+ int i, j = 0;
+ long offset = 0;
+ struct range *src = rs->ranges;
+ struct range *target = diff->target.ranges;
+ struct range *parent = diff->parent.ranges;
+
+ for (i = 0; i < rs->nr; i++) {
+ while (j < diff->target.nr && src[i].start >= target[j].start) {
+ offset += (parent[j].end-parent[j].start)
+ - (target[j].end-target[j].start);
+ j++;
+ }
+ range_set_append(out, src[i].start+offset, src[i].end+offset);
+ }
+}
+
+/*
+ * Given a diff and the set of interesting ranges, map the ranges
+ * across the diff. That is: observe that the target commit takes
+ * blame for all the + (target-side) ranges. So for every pair of
+ * ranges in the diff that was touched, we remove the latter and add
+ * its parent side.
+ */
+static void range_set_map_across_diff (struct range_set *out,
+ struct range_set *rs,
+ struct diff_ranges *diff,
+ struct diff_ranges **touched_out)
+{
+ struct diff_ranges *touched = xmalloc(sizeof(*touched));
+ struct range_set tmp1 = RANGE_SET_INIT;
+ struct range_set tmp2 = RANGE_SET_INIT;
+
+ diff_ranges_init(touched);
+ diff_ranges_filter_touched(touched, diff, rs);
+ range_set_difference(&tmp1, rs, &touched->target);
+ range_set_shift_diff(&tmp2, &tmp1, diff);
+ range_set_union(out, &tmp2, &touched->parent);
+ range_set_release(&tmp1);
+ range_set_release(&tmp2);
+
+ *touched_out = touched;
+}
+
/*
* Parse one item in the -L option
*/
@@ -18,7 +454,8 @@ const char *parse_loc(const char *spec, nth_line_fn_t nth_line,
* $ is a synonym for "the end of the file".
*/
if (spec[0] == '$') {
- *ret = lines;
+ if (ret)
+ *ret = lines;
return spec + 1;
}
@@ -30,6 +467,8 @@ const char *parse_loc(const char *spec, nth_line_fn_t nth_line,
if (begin != -1 && (spec[0] == '+' || spec[0] == '-')) {
num = strtol(spec + 1, &term, 10);
if (term != spec + 1) {
+ if (!ret)
+ return term;
if (spec[0] == '-')
num = 0 - num;
if (0 < num)
@@ -44,7 +483,8 @@ const char *parse_loc(const char *spec, nth_line_fn_t nth_line,
}
num = strtol(spec, &term, 10);
if (term != spec) {
- *ret = num;
+ if (ret)
+ *ret = num;
return term;
}
if (spec[0] != '/')
@@ -58,6 +498,10 @@ const char *parse_loc(const char *spec, nth_line_fn_t nth_line,
if (*term != '/')
return spec;
+ /* in the scan-only case we are not interested in the regex */
+ if (!ret)
+ return term+1;
+
/* try [spec+1 .. term-1] as regexp */
*term = 0;
if (begin == -1)
@@ -101,13 +545,763 @@ int parse_range_arg(const char *arg, nth_line_fn_t nth_line_cb,
}
}
- if (*begin <= 0)
- *begin = 1;
- if (*end > lines)
- *end = lines;
-
if (*arg)
return -1;
return 0;
}
+
+const char *skip_range_arg(const char *arg)
+{
+ arg = parse_loc(arg, NULL, NULL, 0, -1, 0);
+
+ if (*arg == ',')
+ arg = parse_loc(arg+1, NULL, NULL, 0, 0, 0);
+
+ return arg;
+}
+
+static struct commit *check_single_commit(struct rev_info *revs)
+{
+ struct object *commit = NULL;
+ int found = -1;
+ int i;
+
+ for (i = 0; i < revs->pending.nr; i++) {
+ struct object *obj = revs->pending.objects[i].item;
+ if (obj->flags & UNINTERESTING)
+ continue;
+ while (obj->type == OBJ_TAG)
+ obj = deref_tag(obj, NULL, 0);
+ if (obj->type != OBJ_COMMIT)
+ die("Non commit %s?", revs->pending.objects[i].name);
+ if (commit)
+ die("More than one commit to dig from: %s and %s?",
+ revs->pending.objects[i].name,
+ revs->pending.objects[found].name);
+ commit = obj;
+ found = i;
+ }
+
+ if (!commit)
+ die("No commit specified?");
+
+ return (struct commit *) commit;
+}
+
+static void fill_blob_sha1(struct commit *commit, struct diff_filespec *spec)
+{
+ unsigned mode;
+ unsigned char sha1[20];
+
+ if (get_tree_entry(commit->object.sha1, spec->path,
+ sha1, &mode))
+ die("There is no path %s in the commit", spec->path);
+ fill_filespec(spec, sha1, 1, mode);
+
+ return;
+}
+
+static void fill_line_ends(struct diff_filespec *spec, long *lines,
+ unsigned long **line_ends)
+{
+ int num = 0, size = 50;
+ long cur = 0;
+ unsigned long *ends = NULL;
+ char *data = NULL;
+
+ if (diff_populate_filespec(spec, 0))
+ die("Cannot read blob %s", sha1_to_hex(spec->sha1));
+
+ ends = xmalloc(size * sizeof(*ends));
+ ends[cur++] = 0;
+ data = spec->data;
+ while (num < spec->size) {
+ if (data[num] == '\n' || num == spec->size - 1) {
+ ALLOC_GROW(ends, (cur + 1), size);
+ ends[cur++] = num;
+ }
+ num++;
+ }
+
+ /* shrink the array to fit the elements */
+ ends = xrealloc(ends, cur * sizeof(*ends));
+ *lines = cur-1;
+ *line_ends = ends;
+}
+
+struct nth_line_cb {
+ struct diff_filespec *spec;
+ long lines;
+ unsigned long *line_ends;
+};
+
+static const char *nth_line(void *data, long line)
+{
+ struct nth_line_cb *d = data;
+ assert(d && line <= d->lines);
+ assert(d->spec && d->spec->data);
+
+ if (line == 0)
+ return (char *)d->spec->data;
+ else
+ return (char *)d->spec->data + d->line_ends[line] + 1;
+}
+
+static struct line_log_data *
+parse_lines(struct commit *commit, const char *prefix, struct string_list *args)
+{
+ long lines = 0;
+ unsigned long *ends = NULL;
+ struct nth_line_cb cb_data;
+ struct string_list_item *item;
+ struct line_log_data *ranges = NULL;
+
+ for_each_string_list_item(item, args) {
+ const char *name_part, *range_part;
+ const char *full_name;
+ struct diff_filespec *spec;
+ long begin = 0, end = 0;
+
+ name_part = skip_range_arg(item->string);
+ if (!name_part || *name_part != ':')
+ die("-L argument '%s' not of the form start,end:file",
+ item->string);
+ range_part = xstrndup(item->string, name_part - item->string);
+ name_part++;
+
+ full_name = prefix_path(prefix, prefix ? strlen(prefix) : 0,
+ name_part);
+
+ spec = alloc_filespec(full_name);
+ fill_blob_sha1(commit, spec);
+ fill_line_ends(spec, &lines, &ends);
+ cb_data.spec = spec;
+ cb_data.lines = lines;
+ cb_data.line_ends = ends;
+
+ if (parse_range_arg(range_part, nth_line, &cb_data,
+ lines, &begin, &end))
+ die("malformed -L argument '%s'", range_part);
+ if (begin < 1)
+ begin = 1;
+ if (end < 1)
+ end = lines;
+ begin--;
+ if (lines < end || lines < begin)
+ die("file %s has only %ld lines", name_part, lines);
+ line_log_data_insert(&ranges, spec, begin, end);
+
+ free(ends);
+ ends = NULL;
+ }
+
+ return ranges;
+}
+
+static struct line_log_data *line_log_data_copy_one(struct line_log_data *r)
+{
+ struct line_log_data *ret = xmalloc(sizeof(*ret));
+
+ assert(r);
+ line_log_data_init(ret);
+ range_set_copy(&ret->ranges, &r->ranges);
+
+ ret->spec = r->spec;
+ assert(ret->spec);
+ ret->spec->count++;
+
+ return ret;
+}
+
+static struct line_log_data *
+line_log_data_copy(struct line_log_data *r)
+{
+ struct line_log_data *ret = NULL;
+ struct line_log_data *tmp = NULL, *prev = NULL;
+
+ assert(r);
+ ret = tmp = prev = line_log_data_copy_one(r);
+ r = r->next;
+ while (r) {
+ tmp = line_log_data_copy_one(r);
+ prev->next = tmp;
+ prev = tmp;
+ r = r->next;
+ }
+
+ return ret;
+}
+
+/* merge two range sets across files */
+static struct line_log_data *line_log_data_merge(struct line_log_data *a,
+ struct line_log_data *b)
+{
+ struct line_log_data *head = NULL, **pp = &head;
+
+ while (a || b) {
+ struct line_log_data *src;
+ struct line_log_data *src2 = NULL;
+ struct line_log_data *d;
+ int cmp;
+ if (!a)
+ cmp = 1;
+ else if (!b)
+ cmp = -1;
+ else
+ cmp = strcmp(a->spec->path, b->spec->path);
+ if (cmp < 0) {
+ src = a;
+ a = a->next;
+ } else if (cmp == 0) {
+ src = a;
+ a = a->next;
+ src2 = b;
+ b = b->next;
+ } else {
+ src = b;
+ b = b->next;
+ }
+ d = xmalloc(sizeof(struct line_log_data));
+ line_log_data_init(d);
+ d->spec = src->spec;
+ d->spec->count++;
+ *pp = d;
+ pp = &d->next;
+ if (src2)
+ range_set_union(&d->ranges, &src->ranges, &src2->ranges);
+ else
+ range_set_copy(&d->ranges, &src->ranges);
+ }
+
+ return head;
+}
+
+static void add_line_range(struct rev_info *revs, struct commit *commit,
+ struct line_log_data *range)
+{
+ struct line_log_data *old = NULL;
+ struct line_log_data *new = NULL;
+
+ old = lookup_decoration(&revs->line_log_data, &commit->object);
+ if (old && range) {
+ new = line_log_data_merge(old, range);
+ free_line_log_data(old);
+ } else if (range)
+ new = line_log_data_copy(range);
+
+ if (new)
+ add_decoration(&revs->line_log_data, &commit->object, new);
+}
+
+static void clear_commit_line_range(struct rev_info *revs, struct commit *commit)
+{
+ struct line_log_data *r;
+ r = lookup_decoration(&revs->line_log_data, &commit->object);
+ if (!r)
+ return;
+ free_line_log_data(r);
+ add_decoration(&revs->line_log_data, &commit->object, NULL);
+}
+
+static struct line_log_data *lookup_line_range(struct rev_info *revs,
+ struct commit *commit)
+{
+ struct line_log_data *ret = NULL;
+
+ ret = lookup_decoration(&revs->line_log_data, &commit->object);
+ return ret;
+}
+
+void line_log_init(struct rev_info *rev, const char *prefix, struct string_list *args)
+{
+ struct commit *commit = NULL;
+ struct line_log_data *range;
+
+ commit = check_single_commit(rev);
+ range = parse_lines(commit, prefix, args);
+ add_line_range(rev, commit, range);
+
+ if (!rev->diffopt.detect_rename) {
+ int i, count = 0;
+ struct line_log_data *r = range;
+ const char **paths;
+ while (r) {
+ count++;
+ r = r->next;
+ }
+ paths = xmalloc((count+1)*sizeof(char *));
+ r = range;
+ for (i = 0; i < count; i++) {
+ paths[i] = xstrdup(r->spec->path);
+ r = r->next;
+ }
+ paths[count] = NULL;
+ init_pathspec(&rev->diffopt.pathspec, paths);
+ free(paths);
+ }
+}
+
+static void load_tree_desc(struct tree_desc *desc, void **tree,
+ const unsigned char *sha1)
+{
+ unsigned long size;
+ *tree = read_object_with_reference(sha1, tree_type, &size, NULL);
+ if (!tree)
+ die("Unable to read tree (%s)", sha1_to_hex(sha1));
+ init_tree_desc(desc, *tree, size);
+}
+
+static int count_parents(struct commit *commit)
+{
+ struct commit_list *parents = commit->parents;
+ int count = 0;
+ while (parents) {
+ count++;
+ parents = parents->next;
+ }
+ return count;
+}
+
+static void move_diff_queue(struct diff_queue_struct *dst,
+ struct diff_queue_struct *src)
+{
+ assert(src != dst);
+ memcpy(dst, src, sizeof(struct diff_queue_struct));
+ DIFF_QUEUE_CLEAR(src);
+}
+
+static void queue_diffs(struct diff_options *opt,
+ struct diff_queue_struct *queue,
+ struct commit *commit, struct commit *parent)
+{
+ void *tree1 = NULL, *tree2 = NULL;
+ struct tree_desc desc1, desc2;
+
+ assert(commit);
+ load_tree_desc(&desc2, &tree2, commit->tree->object.sha1);
+ if (parent)
+ load_tree_desc(&desc1, &tree1, parent->tree->object.sha1);
+ else
+ init_tree_desc(&desc1, "", 0);
+
+ DIFF_QUEUE_CLEAR(&diff_queued_diff);
+ diff_tree(&desc1, &desc2, "", opt);
+ diffcore_std(opt);
+ move_diff_queue(queue, &diff_queued_diff);
+
+ if (tree1)
+ free(tree1);
+ if (tree2)
+ free(tree2);
+}
+
+static char *get_nth_line(long line, unsigned long *ends, void *data)
+{
+ if (line == 0)
+ return (char *)data;
+ else
+ return (char *)data + ends[line] + 1;
+}
+
+static void print_line(const char *prefix, char first,
+ long line, unsigned long *ends, void *data,
+ const char *color, const char *reset)
+{
+ char *begin = get_nth_line(line, ends, data);
+ char *end = get_nth_line(line+1, ends, data);
+ int had_nl = 0;
+
+ if (end > begin && end[-1] == '\n') {
+ end--;
+ had_nl = 1;
+ }
+
+ fputs(prefix, stdout);
+ fputs(color, stdout);
+ putchar(first);
+ fwrite(begin, 1, end-begin, stdout);
+ fputs(reset, stdout);
+ putchar('\n');
+ if (!had_nl)
+ fputs("\\ No newline at end of file\n", stdout);
+}
+
+static char *output_prefix(struct diff_options *opt)
+{
+ char *prefix = "";
+
+ if (opt->output_prefix) {
+ struct strbuf *sb = opt->output_prefix(opt, opt->output_prefix_data);
+ prefix = sb->buf;
+ }
+
+ return prefix;
+}
+
+static void dump_diff_hacky_one(struct rev_info *rev, struct line_log_data *range)
+{
+ int i, j = 0;
+ long p_lines, t_lines;
+ unsigned long *p_ends = NULL, *t_ends = NULL;
+ struct diff_filepair *pair = range->pair;
+ struct diff_ranges *diff = &range->diff;
+
+ struct diff_options *opt = &rev->diffopt;
+ char *prefix = output_prefix(opt);
+ const char *c_reset = diff_get_color(opt->use_color, DIFF_RESET);
+ const char *c_frag = diff_get_color(opt->use_color, DIFF_FRAGINFO);
+ const char *c_meta = diff_get_color(opt->use_color, DIFF_METAINFO);
+ const char *c_old = diff_get_color(opt->use_color, DIFF_FILE_OLD);
+ const char *c_new = diff_get_color(opt->use_color, DIFF_FILE_NEW);
+ const char *c_plain = diff_get_color(opt->use_color, DIFF_PLAIN);
+
+ if (!pair || !diff)
+ return;
+
+ if (pair->one->sha1_valid)
+ fill_line_ends(pair->one, &p_lines, &p_ends);
+ fill_line_ends(pair->two, &t_lines, &t_ends);
+
+ printf("%s%sdiff --git a/%s b/%s%s\n", prefix, c_meta, pair->one->path, pair->two->path, c_reset);
+ printf("%s%s--- %s%s%s\n", prefix, c_meta,
+ pair->one->sha1_valid ? "a/" : "",
+ pair->one->sha1_valid ? pair->one->path : "/dev/null",
+ c_reset);
+ printf("%s%s+++ b/%s%s\n", prefix, c_meta, pair->two->path, c_reset);
+ for (i = 0; i < range->ranges.nr; i++) {
+ long p_start, p_end;
+ long t_start = range->ranges.ranges[i].start;
+ long t_end = range->ranges.ranges[i].end;
+ long t_cur = t_start;
+ int j_last;
+
+ while (j < diff->target.nr && diff->target.ranges[j].end < t_start)
+ j++;
+ if (j == diff->target.nr || diff->target.ranges[j].start > t_end)
+ continue;
+
+ /* Scan ahead to determine the last diff that falls in this range */
+ j_last = j;
+ while (j_last < diff->target.nr && diff->target.ranges[j_last].start < t_end)
+ j_last++;
+ if (j_last > j)
+ j_last--;
+
+ /*
+ * Compute parent hunk headers: we know that the diff
+ * has the correct line numbers (but not all hunks).
+ * So it suffices to shift the start/end according to
+ * the line numbers of the first/last hunk(s) that
+ * fall in this range.
+ */
+ if (t_start < diff->target.ranges[j].start)
+ p_start = diff->parent.ranges[j].start - (diff->target.ranges[j].start-t_start);
+ else
+ p_start = diff->parent.ranges[j].start;
+ if (t_end > diff->target.ranges[j_last].end)
+ p_end = diff->parent.ranges[j_last].end + (t_end-diff->target.ranges[j_last].end);
+ else
+ p_end = diff->parent.ranges[j_last].end;
+
+ if (!p_start && !p_end) {
+ p_start = -1;
+ p_end = -1;
+ }
+
+ /* Now output a diff hunk for this range */
+ printf("%s%s@@ -%ld,%ld +%ld,%ld @@%s\n",
+ prefix, c_frag,
+ p_start+1, p_end-p_start, t_start+1, t_end-t_start,
+ c_reset);
+ while (j < diff->target.nr && diff->target.ranges[j].start < t_end) {
+ int k;
+ for (; t_cur < diff->target.ranges[j].start; t_cur++)
+ print_line(prefix, ' ', t_cur, t_ends, pair->two->data,
+ c_plain, c_reset);
+ for (k = diff->parent.ranges[j].start; k < diff->parent.ranges[j].end; k++)
+ print_line(prefix, '-', k, p_ends, pair->one->data,
+ c_old, c_reset);
+ for (; t_cur < diff->target.ranges[j].end && t_cur < t_end; t_cur++)
+ print_line(prefix, '+', t_cur, t_ends, pair->two->data,
+ c_new, c_reset);
+ j++;
+ }
+ for (; t_cur < t_end; t_cur++)
+ print_line(prefix, ' ', t_cur, t_ends, pair->two->data,
+ c_plain, c_reset);
+ }
+
+ free(p_ends);
+ free(t_ends);
+}
+
+/*
+ * NEEDSWORK: manually building a diff here is not the Right
+ * Thing(tm). log -L should be built into the diff pipeline.
+ */
+static void dump_diff_hacky(struct rev_info *rev, struct line_log_data *range)
+{
+ puts(output_prefix(&rev->diffopt));
+ while (range) {
+ dump_diff_hacky_one(rev, range);
+ range = range->next;
+ }
+}
+
+/*
+ * Unlike most other functions, this destructively operates on
+ * 'range'.
+ */
+static int process_diff_filepair(struct rev_info *rev,
+ struct diff_filepair *pair,
+ struct line_log_data *range,
+ struct diff_ranges **diff_out)
+{
+ struct line_log_data *rg = range;
+ struct range_set tmp;
+ struct diff_ranges diff;
+ mmfile_t file_parent, file_target;
+
+ assert(pair->two->path);
+ while (rg) {
+ assert(rg->spec->path);
+ if (!strcmp(rg->spec->path, pair->two->path))
+ break;
+ rg = rg->next;
+ }
+
+ if (!rg)
+ return 0;
+ if (rg->ranges.nr == 0)
+ return 0;
+
+ assert(pair->two->sha1_valid);
+ diff_populate_filespec(pair->two, 0);
+ file_target.ptr = pair->two->data;
+ file_target.size = pair->two->size;
+
+ if (pair->one->sha1_valid) {
+ diff_populate_filespec(pair->one, 0);
+ file_parent.ptr = pair->one->data;
+ file_parent.size = pair->one->size;
+ } else {
+ file_parent.ptr = "";
+ file_parent.size = 0;
+ }
+
+ diff_ranges_init(&diff);
+ collect_diff(&file_parent, &file_target, &diff);
+
+ /* NEEDSWORK should apply some heuristics to prevent mismatches */
+ rg->spec->path = xstrdup(pair->one->path);
+
+ range_set_init(&tmp, 0);
+ range_set_map_across_diff(&tmp, &rg->ranges, &diff, diff_out);
+ range_set_release(&rg->ranges);
+ range_set_move(&rg->ranges, &tmp);
+
+ diff_ranges_release(&diff);
+
+ return ((*diff_out)->parent.nr > 0);
+}
+
+static struct diff_filepair *diff_filepair_dup(struct diff_filepair *pair)
+{
+ struct diff_filepair *new = xmalloc(sizeof(struct diff_filepair));
+ new->one = pair->one;
+ new->two = pair->two;
+ new->one->count++;
+ new->two->count++;
+ return new;
+}
+
+static void free_diffqueues(int n, struct diff_queue_struct *dq)
+{
+ int i, j;
+ for (i = 0; i < n; i++)
+ for (j = 0; j < dq[i].nr; j++)
+ diff_free_filepair(dq[i].queue[j]);
+ free(dq);
+}
+
+static int process_all_files(struct line_log_data **range_out,
+ struct rev_info *rev,
+ struct diff_queue_struct *queue,
+ struct line_log_data *range)
+{
+ int i, changed = 0;
+
+ *range_out = line_log_data_copy(range);
+
+ for (i = 0; i < queue->nr; i++) {
+ struct diff_ranges *pairdiff = NULL;
+ if (process_diff_filepair(rev, queue->queue[i], *range_out, &pairdiff)) {
+ struct line_log_data *rg = range;
+ changed++;
+ /* NEEDSWORK tramples over data structures not owned here */
+ while (rg && strcmp(rg->spec->path, queue->queue[i]->two->path))
+ rg = rg->next;
+ assert(rg);
+ rg->pair = diff_filepair_dup(queue->queue[i]);
+ memcpy(&rg->diff, pairdiff, sizeof(struct diff_ranges));
+ }
+ }
+
+ return changed;
+}
+
+int line_log_print(struct rev_info *rev, struct commit *commit)
+{
+ struct line_log_data *range = lookup_line_range(rev, commit);
+
+ show_log(rev);
+ dump_diff_hacky(rev, range);
+ return 1;
+}
+
+static int process_ranges_ordinary_commit(struct rev_info *rev, struct commit *commit)
+{
+ struct commit *parent = NULL;
+ struct diff_queue_struct queue;
+ struct line_log_data *range = lookup_line_range(rev, commit);
+ struct line_log_data *parent_range;
+ int changed;
+
+ if (commit->parents)
+ parent = commit->parents->item;
+
+ queue_diffs(&rev->diffopt, &queue, commit, parent);
+ changed = process_all_files(&parent_range, rev, &queue, range);
+ if (parent)
+ add_line_range(rev, parent, parent_range);
+ return changed;
+}
+
+static int process_ranges_merge_commit(struct rev_info *rev, struct commit *commit)
+{
+ struct line_log_data *range = lookup_line_range(rev, commit);
+ struct diff_queue_struct *diffqueues;
+ struct line_log_data **cand;
+ struct commit **parents;
+ struct commit_list *p;
+ int i;
+ int nparents = count_parents(commit);
+
+ diffqueues = xmalloc(nparents * sizeof(*diffqueues));
+ cand = xmalloc(nparents * sizeof(*cand));
+ parents = xmalloc(nparents * sizeof(*parents));
+
+ p = commit->parents;
+ for (i = 0; i < nparents; i++) {
+ parents[i] = p->item;
+ p = p->next;
+ queue_diffs(&rev->diffopt, &diffqueues[i], commit, parents[i]);
+ }
+
+ for (i = 0; i < nparents; i++) {
+ int changed;
+ cand[i] = NULL;
+ changed = process_all_files(&cand[i], rev, &diffqueues[i], range);
+ if (!changed) {
+ /*
+ * This parent can take all the blame, so we
+ * don't follow any other path in history
+ */
+ add_line_range(rev, parents[i], cand[i]);
+ clear_commit_line_range(rev, commit);
+ commit->parents = xmalloc(sizeof(struct commit_list));
+ commit->parents->item = parents[i];
+ commit->parents->next = NULL;
+ free(parents);
+ free(cand);
+ free_diffqueues(nparents, diffqueues);
+ /* NEEDSWORK leaking like a sieve */
+ return 0;
+ }
+ }
+
+ /*
+ * No single parent took the blame. We add the candidates
+ * from the above loop to the parents.
+ */
+ for (i = 0; i < nparents; i++) {
+ add_line_range(rev, parents[i], cand[i]);
+ }
+
+ clear_commit_line_range(rev, commit);
+ free(parents);
+ free(cand);
+ free_diffqueues(nparents, diffqueues);
+ return 1;
+
+ /* NEEDSWORK evil merge detection stuff */
+ /* NEEDSWORK leaking like a sieve */
+}
+
+static int process_ranges_arbitrary_commit(struct rev_info *rev, struct commit *commit)
+{
+ int changed = 0;
+
+ if (lookup_line_range(rev, commit)) {
+ if (!commit->parents || !commit->parents->next)
+ changed = process_ranges_ordinary_commit(rev, commit);
+ else
+ changed = process_ranges_merge_commit(rev, commit);
+ }
+
+ if (!changed)
+ commit->object.flags |= TREESAME;
+
+ return changed;
+}
+
+static enum rewrite_result line_log_rewrite_one(struct rev_info *rev, struct commit **pp)
+{
+ for (;;) {
+ struct commit *p = *pp;
+ if (p->parents && p->parents->next)
+ return rewrite_one_ok;
+ if (p->object.flags & UNINTERESTING)
+ return rewrite_one_ok;
+ if (!(p->object.flags & TREESAME))
+ return rewrite_one_ok;
+ if (!p->parents)
+ return rewrite_one_noparents;
+ *pp = p->parents->item;
+ }
+}
+
+int line_log_filter(struct rev_info *rev)
+{
+ struct commit *commit;
+ struct commit_list *list = rev->commits;
+ struct commit_list *out = NULL, *cur = NULL;
+
+ list = rev->commits;
+ while (list) {
+ struct commit_list *to_free = NULL;
+ commit = list->item;
+ if (process_ranges_arbitrary_commit(rev, commit)) {
+ if (cur)
+ cur->next = list;
+ else
+ out = list;
+ cur = list;
+ } else
+ to_free = list;
+ list = list->next;
+ free(to_free);
+ }
+ cur->next = NULL;
+
+ list = out;
+ while (list) {
+ rewrite_parents(rev, list->item, line_log_rewrite_one);
+ list = list->next;
+ }
+
+ rev->commits = out;
+
+ return 0;
+}
diff --git a/line-log.h b/line-log.h
index 22b1c80..e3ec094 100644
--- a/line-log.h
+++ b/line-log.h
@@ -1,6 +1,8 @@
#ifndef LINE_LOG_H
#define LINE_LOG_H
+#include "diffcore.h"
+
/*
* Parse one item in an -L begin,end option w.r.t. the notional file
* object 'cb_data' consisting of 'lines' lines.
@@ -20,4 +22,58 @@ extern int parse_range_arg(const char *arg,
void *cb_data, long lines,
long *begin, long *end);
+/*
+ * Scan past a range argument that could be parsed by
+ * 'parse_range_arg', to help the caller determine the start of the
+ * filename in '-L n,m:file' syntax.
+ *
+ * Returns a pointer to the first character after the 'n,m' part, or
+ * NULL in case the argument is obviously malformed.
+ */
+
+extern const char *skip_range_arg(const char *arg);
+
+struct rev_info;
+struct commit;
+
+/* A range [start,end]. Lines are numbered starting at 0, and the
+ * ranges include start but exclude end. */
+struct range {
+ long start, end;
+};
+
+/* A set of ranges. The ranges must always be disjoint and sorted. */
+struct range_set {
+ int alloc, nr;
+ struct range *ranges;
+};
+
+/* A diff, encoded as the set of pre- and post-image ranges where the
+ * files differ. A pair of ranges corresponds to a hunk. */
+struct diff_ranges {
+ struct range_set parent;
+ struct range_set target;
+};
+
+/* Linked list of interesting files and their associated ranges. The
+ * list must be kept sorted by spec->path */
+struct line_log_data {
+ struct line_log_data *next;
+ struct diff_filespec *spec;
+ char status;
+ struct range_set ranges;
+ int arg_alloc, arg_nr;
+ const char **args;
+ struct diff_filepair *pair;
+ struct diff_ranges diff;
+};
+
+extern void line_log_data_init(struct line_log_data *r);
+
+extern void line_log_init(struct rev_info *rev, const char *prefix, struct string_list *args);
+
+extern int line_log_filter(struct rev_info *rev);
+
+extern int line_log_print(struct rev_info *rev, struct commit *commit);
+
#endif /* LINE_LOG_H */
diff --git a/log-tree.c b/log-tree.c
index 5dc45c4..793d2c8 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -9,6 +9,7 @@
#include "string-list.h"
#include "color.h"
#include "gpg-interface.h"
+#include "line-log.h"
struct decoration name_decoration = { "object names" };
@@ -856,6 +857,9 @@ int log_tree_commit(struct rev_info *opt, struct commit *commit)
log.parent = NULL;
opt->loginfo = &log;
+ if (opt->line_level_traverse)
+ return line_log_print(opt, commit);
+
shown = log_tree_diff(opt, commit, &log);
if (!shown && opt->loginfo && opt->always_show_header) {
log.parent = NULL;
diff --git a/revision.c b/revision.c
index 46319d5..3ad9d08 100644
--- a/revision.c
+++ b/revision.c
@@ -13,6 +13,7 @@
#include "decorate.h"
#include "log-tree.h"
#include "string-list.h"
+#include "line-log.h"
#include "mailmap.h"
volatile show_early_output_fn_t show_early_output;
@@ -1896,6 +1897,12 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
if (revs->combine_merges)
revs->ignore_merges = 0;
revs->diffopt.abbrev = revs->abbrev;
+
+ if (revs->line_level_traverse) {
+ revs->limited = 1;
+ revs->topo_order = 1;
+ }
+
diff_setup_done(&revs->diffopt);
grep_commit_pattern_type(GREP_PATTERN_TYPE_UNSPECIFIED,
@@ -2166,6 +2173,8 @@ int prepare_revision_walk(struct rev_info *revs)
return -1;
if (revs->topo_order)
sort_in_topological_order(&revs->commits, revs->lifo);
+ if (revs->line_level_traverse)
+ line_log_filter(revs);
if (revs->simplify_merges)
simplify_merges(revs);
if (revs->children.name)
diff --git a/revision.h b/revision.h
index 640110d..6b2870c 100644
--- a/revision.h
+++ b/revision.h
@@ -96,7 +96,8 @@ struct rev_info {
cherry_mark:1,
bisect:1,
ancestry_path:1,
- first_parent_only:1;
+ first_parent_only:1,
+ line_level_traverse:1;
/* Diff flags */
unsigned int diff:1,
@@ -175,6 +176,9 @@ struct rev_info {
int count_left;
int count_right;
int count_same;
+
+ /* line level range that we are chasing */
+ struct decoration line_log_data;
};
#define REV_TREE_SAME 0
diff --git a/t/t4211-line-log.sh b/t/t4211-line-log.sh
new file mode 100755
index 0000000..639a16f
--- /dev/null
+++ b/t/t4211-line-log.sh
@@ -0,0 +1,41 @@
+#!/bin/sh
+
+test_description='test log -L'
+. ./test-lib.sh
+
+test_expect_success 'setup (import history)' '
+ git fast-import < "$TEST_DIRECTORY"/t4211/history.export &&
+ git reset --hard
+'
+
+canned_test () {
+ test_expect_success "$1" "
+ git log $1 >actual &&
+ test_cmp \"\$TEST_DIRECTORY\"/t4211/expect.$2 actual
+ "
+ cp actual "$TEST_DIRECTORY"/t4211/expect.$2
+}
+
+canned_test "-L 4,12:a.c simple" simple-f
+canned_test "-L 4,+9:a.c simple" simple-f
+canned_test "-L '/long f/,/^}/:a.c' simple" simple-f
+
+canned_test "-L '/main/,/^}/:a.c' simple" simple-main
+
+canned_test "-L 1,+4:a.c simple" beginning-of-file
+canned_test "-L 0,+5:a.c simple" beginning-of-file
+
+canned_test "-L 20:a.c simple" end-of-file
+canned_test "-L 20,'$':a.c simple" end-of-file
+
+test_expect_success "-L 20,10000:a.c (bogus end)" '
+ test_must_fail git log -L 20,10000:a.c simple 2>errors &&
+ grep "has only.*lines" errors
+'
+
+canned_test "-L '/long f/',/^}/:a.c -L /main/,/^}/:a.c simple" two-ranges
+canned_test "-L 24,+1:a.c simple" vanishes-early
+
+canned_test "-L '/long f/,/^}/:b.c' move-support" move-support-f
+
+test_done
diff --git a/t/t4211/expect.beginning-of-file b/t/t4211/expect.beginning-of-file
new file mode 100644
index 0000000..91b4054
--- /dev/null
+++ b/t/t4211/expect.beginning-of-file
@@ -0,0 +1,43 @@
+commit 4a23ae5c98d59a58c6da036156959f2dc9f472ad
+Author: Thomas Rast <trast@student.ethz.ch>
+Date: Thu Feb 28 10:47:40 2013 +0100
+
+ change at very beginning
+
+diff --git a/a.c b/a.c
+--- a/a.c
++++ b/a.c
+@@ -1,3 +1,4 @@
++#include <unistd.h>
+ #include <stdio.h>
+
+ long f(long x)
+
+commit a6eb82647d5d67f893da442f8f9375fd89a3b1e2
+Author: Thomas Rast <trast@student.ethz.ch>
+Date: Thu Feb 28 10:45:16 2013 +0100
+
+ touch both functions
+
+diff --git a/a.c b/a.c
+--- a/a.c
++++ b/a.c
+@@ -1,3 +1,3 @@
+ #include <stdio.h>
+
+-int f(int x)
++long f(long x)
+
+commit de4c48ae814792c02a49c4c3c0c757ae69c55f6a
+Author: Thomas Rast <trast@student.ethz.ch>
+Date: Thu Feb 28 10:44:48 2013 +0100
+
+ initial
+
+diff --git a/a.c b/a.c
+--- /dev/null
++++ b/a.c
+@@ -0,0 +1,3 @@
++#include <stdio.h>
++
++int f(int x)
diff --git a/t/t4211/expect.end-of-file b/t/t4211/expect.end-of-file
new file mode 100644
index 0000000..bd25bb2
--- /dev/null
+++ b/t/t4211/expect.end-of-file
@@ -0,0 +1,62 @@
+commit 4659538844daa2849b1a9e7d6fadb96fcd26fc83
+Author: Thomas Rast <trast@student.ethz.ch>
+Date: Thu Feb 28 10:48:43 2013 +0100
+
+ change back to complete line
+
+diff --git a/a.c b/a.c
+--- a/a.c
++++ b/a.c
+@@ -20,3 +20,5 @@
+ printf("%ld\n", f(15));
+ return 0;
+-}
+\ No newline at end of file
++}
++
++/* incomplete lines are bad! */
+
+commit 100b61a6f2f720f812620a9d10afb3a960ccb73c
+Author: Thomas Rast <trast@student.ethz.ch>
+Date: Thu Feb 28 10:48:10 2013 +0100
+
+ change to an incomplete line at end
+
+diff --git a/a.c b/a.c
+--- a/a.c
++++ b/a.c
+@@ -20,3 +20,3 @@
+ printf("%ld\n", f(15));
+ return 0;
+-}
++}
+\ No newline at end of file
+
+commit a6eb82647d5d67f893da442f8f9375fd89a3b1e2
+Author: Thomas Rast <trast@student.ethz.ch>
+Date: Thu Feb 28 10:45:16 2013 +0100
+
+ touch both functions
+
+diff --git a/a.c b/a.c
+--- a/a.c
++++ b/a.c
+@@ -19,3 +19,3 @@
+- printf("%d\n", f(15));
++ printf("%ld\n", f(15));
+ return 0;
+ }
+
+commit de4c48ae814792c02a49c4c3c0c757ae69c55f6a
+Author: Thomas Rast <trast@student.ethz.ch>
+Date: Thu Feb 28 10:44:48 2013 +0100
+
+ initial
+
+diff --git a/a.c b/a.c
+--- /dev/null
++++ b/a.c
+@@ -0,0 +18,3 @@
++ printf("%d\n", f(15));
++ return 0;
++}
diff --git a/t/t4211/expect.move-support-f b/t/t4211/expect.move-support-f
new file mode 100644
index 0000000..78a8cf1
--- /dev/null
+++ b/t/t4211/expect.move-support-f
@@ -0,0 +1,40 @@
+commit 6ce3c4ff690136099bb17e1a8766b75764726ea7
+Author: Thomas Rast <trast@student.ethz.ch>
+Date: Thu Feb 28 10:49:50 2013 +0100
+
+ another simple change
+
+diff --git a/b.c b/b.c
+--- a/b.c
++++ b/b.c
+@@ -4,9 +4,9 @@
+ long f(long x)
+ {
+ int s = 0;
+ while (x) {
+- x >>= 1;
++ x /= 2;
+ s++;
+ }
+ return s;
+ }
+
+commit e6da343666244ea9e67cbe3f3bd26da860f9fe0e
+Author: Thomas Rast <trast@student.ethz.ch>
+Date: Thu Feb 28 10:49:28 2013 +0100
+
+ move file
+
+diff --git a/b.c b/b.c
+--- /dev/null
++++ b/b.c
+@@ -0,0 +4,9 @@
++long f(long x)
++{
++ int s = 0;
++ while (x) {
++ x >>= 1;
++ s++;
++ }
++ return s;
++}
diff --git a/t/t4211/expect.simple-f b/t/t4211/expect.simple-f
new file mode 100644
index 0000000..a1f5bc4
--- /dev/null
+++ b/t/t4211/expect.simple-f
@@ -0,0 +1,59 @@
+commit a6eb82647d5d67f893da442f8f9375fd89a3b1e2
+Author: Thomas Rast <trast@student.ethz.ch>
+Date: Thu Feb 28 10:45:16 2013 +0100
+
+ touch both functions
+
+diff --git a/a.c b/a.c
+--- a/a.c
++++ b/a.c
+@@ -3,9 +3,9 @@
+-int f(int x)
++long f(long x)
+ {
+ int s = 0;
+ while (x) {
+ x >>= 1;
+ s++;
+ }
+ return s;
+ }
+
+commit f04fb20f2c77850996cba739709acc6faecc58f7
+Author: Thomas Rast <trast@student.ethz.ch>
+Date: Thu Feb 28 10:44:55 2013 +0100
+
+ change f()
+
+diff --git a/a.c b/a.c
+--- a/a.c
++++ b/a.c
+@@ -3,8 +3,9 @@
+ int f(int x)
+ {
+ int s = 0;
+ while (x) {
+ x >>= 1;
+ s++;
+ }
++ return s;
+ }
+
+commit de4c48ae814792c02a49c4c3c0c757ae69c55f6a
+Author: Thomas Rast <trast@student.ethz.ch>
+Date: Thu Feb 28 10:44:48 2013 +0100
+
+ initial
+
+diff --git a/a.c b/a.c
+--- /dev/null
++++ b/a.c
+@@ -0,0 +3,8 @@
++int f(int x)
++{
++ int s = 0;
++ while (x) {
++ x >>= 1;
++ s++;
++ }
++}
diff --git a/t/t4211/expect.simple-main b/t/t4211/expect.simple-main
new file mode 100644
index 0000000..39ce39b
--- /dev/null
+++ b/t/t4211/expect.simple-main
@@ -0,0 +1,68 @@
+commit 4659538844daa2849b1a9e7d6fadb96fcd26fc83
+Author: Thomas Rast <trast@student.ethz.ch>
+Date: Thu Feb 28 10:48:43 2013 +0100
+
+ change back to complete line
+
+diff --git a/a.c b/a.c
+--- a/a.c
++++ b/a.c
+@@ -18,5 +18,5 @@
+ int main ()
+ {
+ printf("%ld\n", f(15));
+ return 0;
+-}
+\ No newline at end of file
++}
+
+commit 100b61a6f2f720f812620a9d10afb3a960ccb73c
+Author: Thomas Rast <trast@student.ethz.ch>
+Date: Thu Feb 28 10:48:10 2013 +0100
+
+ change to an incomplete line at end
+
+diff --git a/a.c b/a.c
+--- a/a.c
++++ b/a.c
+@@ -18,5 +18,5 @@
+ int main ()
+ {
+ printf("%ld\n", f(15));
+ return 0;
+-}
++}
+\ No newline at end of file
+
+commit a6eb82647d5d67f893da442f8f9375fd89a3b1e2
+Author: Thomas Rast <trast@student.ethz.ch>
+Date: Thu Feb 28 10:45:16 2013 +0100
+
+ touch both functions
+
+diff --git a/a.c b/a.c
+--- a/a.c
++++ b/a.c
+@@ -17,5 +17,5 @@
+ int main ()
+ {
+- printf("%d\n", f(15));
++ printf("%ld\n", f(15));
+ return 0;
+ }
+
+commit de4c48ae814792c02a49c4c3c0c757ae69c55f6a
+Author: Thomas Rast <trast@student.ethz.ch>
+Date: Thu Feb 28 10:44:48 2013 +0100
+
+ initial
+
+diff --git a/a.c b/a.c
+--- /dev/null
++++ b/a.c
+@@ -0,0 +16,5 @@
++int main ()
++{
++ printf("%d\n", f(15));
++ return 0;
++}
diff --git a/t/t4211/expect.two-ranges b/t/t4211/expect.two-ranges
new file mode 100644
index 0000000..6109aa0
--- /dev/null
+++ b/t/t4211/expect.two-ranges
@@ -0,0 +1,102 @@
+commit 4659538844daa2849b1a9e7d6fadb96fcd26fc83
+Author: Thomas Rast <trast@student.ethz.ch>
+Date: Thu Feb 28 10:48:43 2013 +0100
+
+ change back to complete line
+
+diff --git a/a.c b/a.c
+--- a/a.c
++++ b/a.c
+@@ -18,5 +18,5 @@
+ int main ()
+ {
+ printf("%ld\n", f(15));
+ return 0;
+-}
+\ No newline at end of file
++}
+
+commit 100b61a6f2f720f812620a9d10afb3a960ccb73c
+Author: Thomas Rast <trast@student.ethz.ch>
+Date: Thu Feb 28 10:48:10 2013 +0100
+
+ change to an incomplete line at end
+
+diff --git a/a.c b/a.c
+--- a/a.c
++++ b/a.c
+@@ -18,5 +18,5 @@
+ int main ()
+ {
+ printf("%ld\n", f(15));
+ return 0;
+-}
++}
+\ No newline at end of file
+
+commit a6eb82647d5d67f893da442f8f9375fd89a3b1e2
+Author: Thomas Rast <trast@student.ethz.ch>
+Date: Thu Feb 28 10:45:16 2013 +0100
+
+ touch both functions
+
+diff --git a/a.c b/a.c
+--- a/a.c
++++ b/a.c
+@@ -3,9 +3,9 @@
+-int f(int x)
++long f(long x)
+ {
+ int s = 0;
+ while (x) {
+ x >>= 1;
+ s++;
+ }
+ return s;
+ }
+@@ -17,5 +17,5 @@
+ int main ()
+ {
+- printf("%d\n", f(15));
++ printf("%ld\n", f(15));
+ return 0;
+ }
+
+commit f04fb20f2c77850996cba739709acc6faecc58f7
+Author: Thomas Rast <trast@student.ethz.ch>
+Date: Thu Feb 28 10:44:55 2013 +0100
+
+ change f()
+
+diff --git a/a.c b/a.c
+--- a/a.c
++++ b/a.c
+@@ -3,8 +3,9 @@
+ int f(int x)
+ {
+ int s = 0;
+ while (x) {
+ x >>= 1;
+ s++;
+ }
++ return s;
+ }
+
+commit de4c48ae814792c02a49c4c3c0c757ae69c55f6a
+Author: Thomas Rast <trast@student.ethz.ch>
+Date: Thu Feb 28 10:44:48 2013 +0100
+
+ initial
+
+diff --git a/a.c b/a.c
+--- /dev/null
++++ b/a.c
+@@ -0,0 +3,8 @@
++int f(int x)
++{
++ int s = 0;
++ while (x) {
++ x >>= 1;
++ s++;
++ }
++}
diff --git a/t/t4211/expect.vanishes-early b/t/t4211/expect.vanishes-early
new file mode 100644
index 0000000..1f7cd06
--- /dev/null
+++ b/t/t4211/expect.vanishes-early
@@ -0,0 +1,39 @@
+commit 4659538844daa2849b1a9e7d6fadb96fcd26fc83
+Author: Thomas Rast <trast@student.ethz.ch>
+Date: Thu Feb 28 10:48:43 2013 +0100
+
+ change back to complete line
+
+diff --git a/a.c b/a.c
+--- a/a.c
++++ b/a.c
+@@ -22,1 +24,1 @@
+-}
+\ No newline at end of file
++/* incomplete lines are bad! */
+
+commit 100b61a6f2f720f812620a9d10afb3a960ccb73c
+Author: Thomas Rast <trast@student.ethz.ch>
+Date: Thu Feb 28 10:48:10 2013 +0100
+
+ change to an incomplete line at end
+
+diff --git a/a.c b/a.c
+--- a/a.c
++++ b/a.c
+@@ -22,1 +22,1 @@
+-}
++}
+\ No newline at end of file
+
+commit de4c48ae814792c02a49c4c3c0c757ae69c55f6a
+Author: Thomas Rast <trast@student.ethz.ch>
+Date: Thu Feb 28 10:44:48 2013 +0100
+
+ initial
+
+diff --git a/a.c b/a.c
+--- /dev/null
++++ b/a.c
+@@ -0,0 +20,1 @@
++}
diff --git a/t/t4211/history.export b/t/t4211/history.export
new file mode 100644
index 0000000..c159794
--- /dev/null
+++ b/t/t4211/history.export
@@ -0,0 +1,330 @@
+blob
+mark :1
+data 157
+#include <stdio.h>
+
+int f(int x)
+{
+ int s = 0;
+ while (x) {
+ x >>= 1;
+ s++;
+ }
+}
+
+/*
+ * A comment.
+ */
+
+int main ()
+{
+ printf("%d\n", f(15));
+ return 0;
+}
+
+reset refs/tags/simple
+commit refs/tags/simple
+mark :2
+author Thomas Rast <trast@student.ethz.ch> 1362044688 +0100
+committer Thomas Rast <trast@student.ethz.ch> 1362044688 +0100
+data 8
+initial
+M 100644 :1 a.c
+
+blob
+mark :3
+data 168
+#include <stdio.h>
+
+int f(int x)
+{
+ int s = 0;
+ while (x) {
+ x >>= 1;
+ s++;
+ }
+ return s;
+}
+
+/*
+ * A comment.
+ */
+
+int main ()
+{
+ printf("%d\n", f(15));
+ return 0;
+}
+
+commit refs/tags/simple
+mark :4
+author Thomas Rast <trast@student.ethz.ch> 1362044695 +0100
+committer Thomas Rast <trast@student.ethz.ch> 1362044695 +0100
+data 11
+change f()
+from :2
+M 100644 :3 a.c
+
+blob
+mark :5
+data 171
+#include <stdio.h>
+
+long f(long x)
+{
+ int s = 0;
+ while (x) {
+ x >>= 1;
+ s++;
+ }
+ return s;
+}
+
+/*
+ * A comment.
+ */
+
+int main ()
+{
+ printf("%ld\n", f(15));
+ return 0;
+}
+
+commit refs/tags/simple
+mark :6
+author Thomas Rast <trast@student.ethz.ch> 1362044716 +0100
+committer Thomas Rast <trast@student.ethz.ch> 1362044716 +0100
+data 21
+touch both functions
+from :4
+M 100644 :5 a.c
+
+blob
+mark :7
+data 185
+#include <stdio.h>
+
+long f(long x)
+{
+ int s = 0;
+ while (x) {
+ x >>= 1;
+ s++;
+ }
+ return s;
+}
+
+/*
+ * This is only an example!
+ */
+
+int main ()
+{
+ printf("%ld\n", f(15));
+ return 0;
+}
+
+commit refs/tags/simple
+mark :8
+author Thomas Rast <trast@student.ethz.ch> 1362044741 +0100
+committer Thomas Rast <trast@student.ethz.ch> 1362044741 +0100
+data 14
+touch comment
+from :6
+M 100644 :7 a.c
+
+blob
+mark :9
+data 205
+#include <unistd.h>
+#include <stdio.h>
+
+long f(long x)
+{
+ int s = 0;
+ while (x) {
+ x >>= 1;
+ s++;
+ }
+ return s;
+}
+
+/*
+ * This is only an example!
+ */
+
+int main ()
+{
+ printf("%ld\n", f(15));
+ return 0;
+}
+
+commit refs/tags/simple
+mark :10
+author Thomas Rast <trast@student.ethz.ch> 1362044860 +0100
+committer Thomas Rast <trast@student.ethz.ch> 1362044860 +0100
+data 25
+change at very beginning
+from :8
+M 100644 :9 a.c
+
+blob
+mark :11
+data 204
+#include <unistd.h>
+#include <stdio.h>
+
+long f(long x)
+{
+ int s = 0;
+ while (x) {
+ x >>= 1;
+ s++;
+ }
+ return s;
+}
+
+/*
+ * This is only an example!
+ */
+
+int main ()
+{
+ printf("%ld\n", f(15));
+ return 0;
+}
+commit refs/tags/simple
+mark :12
+author Thomas Rast <trast@student.ethz.ch> 1362044890 +0100
+committer Thomas Rast <trast@student.ethz.ch> 1362044890 +0100
+data 36
+change to an incomplete line at end
+from :10
+M 100644 :11 a.c
+
+blob
+mark :13
+data 238
+#include <unistd.h>
+#include <stdio.h>
+
+long f(long x)
+{
+ int s = 0;
+ while (x) {
+ x >>= 1;
+ s++;
+ }
+ return s;
+}
+
+/*
+ * This is only an example!
+ */
+
+int main ()
+{
+ printf("%ld\n", f(15));
+ return 0;
+}
+
+/* incomplete lines are bad! */
+
+commit refs/tags/simple
+mark :14
+author Thomas Rast <trast@student.ethz.ch> 1362044923 +0100
+committer Thomas Rast <trast@student.ethz.ch> 1362044923 +0100
+data 29
+change back to complete line
+from :12
+M 100644 :13 a.c
+
+commit refs/tags/move-support
+mark :15
+author Thomas Rast <trast@student.ethz.ch> 1362044968 +0100
+committer Thomas Rast <trast@student.ethz.ch> 1362044968 +0100
+data 10
+move file
+from :14
+D a.c
+M 100644 :13 b.c
+
+blob
+mark :16
+data 237
+#include <unistd.h>
+#include <stdio.h>
+
+long f(long x)
+{
+ int s = 0;
+ while (x) {
+ x /= 2;
+ s++;
+ }
+ return s;
+}
+
+/*
+ * This is only an example!
+ */
+
+int main ()
+{
+ printf("%ld\n", f(15));
+ return 0;
+}
+
+/* incomplete lines are bad! */
+
+commit refs/tags/move-support
+mark :17
+author Thomas Rast <trast@student.ethz.ch> 1362044990 +0100
+committer Thomas Rast <trast@student.ethz.ch> 1362044990 +0100
+data 22
+another simple change
+from :15
+M 100644 :16 b.c
+
+blob
+mark :18
+data 254
+#include <unistd.h>
+#include <stdio.h>
+
+long f(long x);
+
+/*
+ * This is only an example!
+ */
+
+int main ()
+{
+ printf("%ld\n", f(15));
+ return 0;
+}
+
+/* incomplete lines are bad! */
+
+long f(long x)
+{
+ int s = 0;
+ while (x) {
+ x /= 2;
+ s++;
+ }
+ return s;
+}
+
+commit refs/heads/master
+mark :19
+author Thomas Rast <trast@student.ethz.ch> 1362045024 +0100
+committer Thomas Rast <trast@student.ethz.ch> 1362045024 +0100
+data 21
+move within the file
+from :17
+M 100644 :18 b.c
+
+reset refs/heads/master
+from :19
+
--
1.8.2.rc1.388.g1bd82c8
^ permalink raw reply related
* Re: [PATCH v8 1/5] Refactor parse_loc
From: Junio C Hamano @ 2013-02-28 17:16 UTC (permalink / raw)
To: Thomas Rast; +Cc: git, Bo Yang, Zbigniew Jędrzejewski-Szmek, Will Palmer
In-Reply-To: <d22070b416a1787ece6f98a722614a54826b93ce.1362069310.git.trast@student.ethz.ch>
Thomas Rast <trast@student.ethz.ch> writes:
> From: Bo Yang <struggleyb.nku@gmail.com>
>
> We want to use the same style of -L n,m argument for 'git log -L' as
> for git-blame. Refactor the argument parsing of the range arguments
> from builtin/blame.c to the (new) file that will hold the 'git log -L'
> logic.
>
> To accommodate different data structures in blame and log -L, the file
> contents are abstracted away; parse_range_arg takes a callback that it
> uses to get the contents of a line of the (notional) file.
>
> The new test is for a case that made me pause during debugging: the
> 'blame -L with invalid end' test was the only one that noticed an
> outright failure to parse the end *at all*. So make a more explicit
> test for that.
>
> Signed-off-by: Bo Yang <struggleyb.nku@gmail.com>
> Signed-off-by: Thomas Rast <trast@student.ethz.ch>
> ---
> Documentation/blame-options.txt | 19 +------
> Documentation/line-range-format.txt | 18 +++++++
> Makefile | 2 +
> builtin/blame.c | 99 +++-------------------------------
> line-log.c | 105 ++++++++++++++++++++++++++++++++++++
> line-log.h | 23 ++++++++
Was this churn necessary?
It is strange to move existing functions that will be tweaked to be
shared by two different codepaths (blame and line-log) to the new
user.
The only effect this has, as opposed to tweaking the functions in
place and making them extern, is to make it harder to see the tweaks
made while moving the lines, and also make it more cumbersome to
determine the lineage of the code later.
It would have been understandable if they were moved to a new
library-ish file (perhaps "line-range.[ch]"); even though that
approach shares the same downsides, at least it would have a better
excuse "We will share this, so let's move it to a neutral third
place to allow us to hide the implementation details from both
users". The arrangement this patch series makes does not even have
that excuse. The final implementation still stay with one of the
users; the only difference is that it is away from the original user
and close to the new user.
> @@ -1927,83 +1933,6 @@ static const char *add_prefix(const char *prefix, const char *path)
> }
>
> /*
> - * Parsing of (comma separated) one item in the -L option
> - */
> -static const char *parse_loc(const char *spec,
> - struct scoreboard *sb, long lno,
> - long begin, long *ret)
> -{
> - char *term;
> - const char *line;
> - long num;
> - int reg_error;
> - regex_t regexp;
> - regmatch_t match[1];
> -
> - /* Allow "-L <something>,+20" to mean starting at <something>
> - * for 20 lines, or "-L <something>,-5" for 5 lines ending at
> - * <something>.
> - */
> - if (1 < begin && (spec[0] == '+' || spec[0] == '-')) {
Did you fix a bug here?
This original only process -L begin,+20 and -L begin,-4 for the
value of begin larger than 1 while reading the second part that
comes after the comma, but incoming begin must be 2 or more, because
the caller adds one to the result of parsing what comes before the
comma, and "1 < begin" here, not "begin != -1", is done for that
reason.
I noticed these slight differences only after eyeballing the lines
deleted from here and lines added to the other place, but the
differences would have been unnoticed if reviewers were not careful.
Again, was it really necessary to move these functions to the new
file?
> diff --git a/line-log.c b/line-log.c
> new file mode 100644
> index 0000000..a24a86b
> --- /dev/null
> +++ b/line-log.c
> @@ -0,0 +1,105 @@
> +#include "git-compat-util.h"
> +#include "line-log.h"
> +
> +/*
> + * Parse one item in the -L option
> + */
> +const char *parse_loc(const char *spec, nth_line_fn_t nth_line,
> + void *data, long lines, long begin, long *ret)
> +{
> + char *term;
> + const char *line;
> + long num;
> + int reg_error;
> + regex_t regexp;
> + regmatch_t match[1];
> +
> + /*
> + * Allow "-L <something>,+20" to mean starting at <something>
> + * for 20 lines, or "-L <something>,-5" for 5 lines ending at
> + * <something>.
> + */
> + if (begin != -1 && (spec[0] == '+' || spec[0] == '-')) {
> + num = strtol(spec + 1, &term, 10);
> + if (term != spec + 1) {
> + if (spec[0] == '-')
> + num = 0 - num;
> + if (0 < num)
> + *ret = begin + num - 2;
> + else if (!num)
> + *ret = begin;
> + else
> + *ret = begin + num;
> + return term;
> + }
> + return spec;
> + }
> + num = strtol(spec, &term, 10);
> + if (term != spec) {
> + *ret = num;
> + return term;
> + }
> + if (spec[0] != '/')
> + return spec;
> +
> + /* it could be a regexp of form /.../ */
> + for (term = (char *) spec + 1; *term && *term != '/'; term++) {
> + if (*term == '\\')
> + term++;
> + }
> + if (*term != '/')
> + return spec;
> +
> + /* try [spec+1 .. term-1] as regexp */
> + *term = 0;
> + if (begin == -1)
> + begin = 1;
> + begin--; /* input is in human terms */
> + line = nth_line(data, begin);
> +
> + if (!(reg_error = regcomp(®exp, spec + 1, REG_NEWLINE)) &&
> + !(reg_error = regexec(®exp, line, 1, match, 0))) {
> + const char *cp = line + match[0].rm_so;
> + const char *nline;
> +
> + while (begin++ < lines) {
> + nline = nth_line(data, begin);
> + if (line <= cp && cp < nline)
> + break;
> + line = nline;
> + }
> + *ret = begin;
> + regfree(®exp);
> + *term++ = '/';
> + return term;
> + } else {
> + char errbuf[1024];
> + regerror(reg_error, ®exp, errbuf, 1024);
> + die("-L parameter '%s': %s", spec + 1, errbuf);
> + }
> +}
> +
> +int parse_range_arg(const char *arg, nth_line_fn_t nth_line_cb,
> + void *cb_data, long lines, long *begin, long *end)
> +{
This, especially the nth-line function, is a good abstraction.
> + arg = parse_loc(arg, nth_line_cb, cb_data, lines, -1, begin);
> +
> + if (*arg == ',') {
Can the input string begin with a comma, e.g. ",-4" which we may
want to mean the last four lines or something?
> + arg = parse_loc(arg+1, nth_line_cb, cb_data, lines, *begin+1, end);
> + if (*begin > *end) {
> + long tmp = *begin;
> + *begin = *end;
> + *end = tmp;
> + }
> + }
> +
> + if (*begin <= 0)
> + *begin = 1;
> + if (*end > lines)
> + *end = lines;
> +
> + if (*arg)
> + return -1;
> +
> + return 0;
> +}
> diff --git a/line-log.h b/line-log.h
> new file mode 100644
> index 0000000..22b1c80
> --- /dev/null
> +++ b/line-log.h
> @@ -0,0 +1,23 @@
> +#ifndef LINE_LOG_H
> +#define LINE_LOG_H
> +
> +/*
> + * Parse one item in an -L begin,end option w.r.t. the notional file
> + * object 'cb_data' consisting of 'lines' lines.
> + *
> + * The 'nth_line_cb' callback is used to determine the start of the
> + * line 'lno' inside the 'cb_data'. The caller is expected to already
> + * have a suitable map at hand to make this a constant-time lookup.
> + *
> + * Returns 0 in case of success and -1 if there was an error. The
> + * caller should print a usage message in the latter case.
> + */
line number counts from 0 or 1?
> +
> +typedef const char *(*nth_line_fn_t)(void *data, long lno);
> +
> +extern int parse_range_arg(const char *arg,
> + nth_line_fn_t nth_line_cb,
> + void *cb_data, long lines,
> + long *begin, long *end);
> +
> +#endif /* LINE_LOG_H */
> diff --git a/t/t8003-blame-corner-cases.sh b/t/t8003-blame-corner-cases.sh
> index 230143c..e7cac1d 100755
> --- a/t/t8003-blame-corner-cases.sh
> +++ b/t/t8003-blame-corner-cases.sh
> @@ -175,6 +175,12 @@ test_expect_success 'blame -L with invalid end' '
> grep "has only 2 lines" errors
> '
>
> +test_expect_success 'blame parses <end> part of -L' '
> + git blame -L1,1 tres >out &&
> + cat out &&
> + test $(wc -l < out) -eq 1
> +'
> +
> test_expect_success 'indent of line numbers, nine lines' '
> git blame nine_lines >actual &&
> test $(grep -c " " actual) = 0
^ permalink raw reply
* [RFC] optimize set_shared_perm()
From: Torsten Bögershausen @ 2013-02-28 17:17 UTC (permalink / raw)
To: git; +Cc: tboegi
optimize set_shared_perm() in path.c:
- sometimes the chown() function is called even when not needed.
(This can be provoced by running t1301, and adding some debug code)
Save a chmod from 400 to 400, or from 600->600 on these files:
.git/info/refs+
.git/objects/info/packs+
Save chmod on directories from 2770 to 2770:
.git/refs
.git/refs/heads
.git/refs/tags
- as all callers use mode == 0 when caling set_shared_perm(),
the function can be simplified.
- all callers use the macro adjust_shared_perm(path) from cache.h
Convert adjust_shared_perm() from a macro into a function prototype
Signed-off-by: Torsten Bögershausen <tboegi@web.de>
---
cache.h | 3 +--
path.c | 75 +++++++++++++++++++++++++++++++++++------------------------------
2 files changed, 41 insertions(+), 37 deletions(-)
While investigating why cygwin 1.7 failes in t1301, and cygwin 1.5 passes,
I came across a small refactoring.
My apologizes if the refactoring in line 431 is wrong:
if (((shared_repository < 0
? (orig_mode & (FORCE_DIR_SET_GID | 0777))
: (orig_mode & mode)) != mode) &&
chmod(path, (mode & ~S_IFMT)) < 0)
diff --git a/cache.h b/cache.h
index 2b192d2..9ea9c70 100644
--- a/cache.h
+++ b/cache.h
@@ -700,8 +700,7 @@ enum sharedrepo {
PERM_EVERYBODY = 0664
};
int git_config_perm(const char *var, const char *value);
-int set_shared_perm(const char *path, int mode);
-#define adjust_shared_perm(path) set_shared_perm((path), 0)
+int adjust_shared_perm(const char *path);
int safe_create_leading_directories(char *path);
int safe_create_leading_directories_const(const char *path);
int mkdir_in_gitdir(const char *path);
diff --git a/path.c b/path.c
index d3d3f8b..8b7922b 100644
--- a/path.c
+++ b/path.c
@@ -1,14 +1,5 @@
/*
- * I'm tired of doing "vsnprintf()" etc just to open a
- * file, so here's a "return static buffer with printf"
- * interface for paths.
- *
- * It's obviously not thread-safe. Sue me. But it's quite
- * useful for doing things like
- *
- * f = open(mkpath("%s/%s.git", base, name), O_RDONLY);
- *
- * which is what it's designed for.
+ * Different utilitiy functions for path and path names
*/
#include "cache.h"
#include "strbuf.h"
@@ -89,6 +80,13 @@ char *git_pathdup(const char *fmt, ...)
return xstrdup(ret);
}
+/*
+ * I'm tired of doing "vsnprintf()" etc just to open a
+ * file, so here's an interface for paths.
+ *
+ * f = open(mkpath("%s/%s.git", base, name), O_RDONLY);
+ *
+ */
char *mkpathdup(const char *fmt, ...)
{
char *path;
@@ -389,24 +387,13 @@ const char *enter_repo(const char *path, int strict)
return NULL;
}
-int set_shared_perm(const char *path, int mode)
+static int calc_shared_perm(int mode)
{
- struct stat st;
- int tweak, shared, orig_mode;
+ int tweak, shared;
- if (!shared_repository) {
- if (mode)
- return chmod(path, mode & ~S_IFMT);
- return 0;
- }
- if (!mode) {
- if (lstat(path, &st) < 0)
- return -1;
- mode = st.st_mode;
- orig_mode = mode;
- } else
- orig_mode = 0;
- if (shared_repository < 0)
+ if (!shared_repository)
+ return mode;
+ else if (shared_repository < 0)
shared = -shared_repository;
else
shared = shared_repository;
@@ -422,16 +409,34 @@ int set_shared_perm(const char *path, int mode)
else
mode |= tweak;
- if (S_ISDIR(mode)) {
- /* Copy read bits to execute bits */
- mode |= (shared & 0444) >> 2;
- mode |= FORCE_DIR_SET_GID;
- }
+ return mode;
+}
+
+static int calc_shared_perm_dir(int mode)
+{
+ mode = calc_shared_perm(mode);
+
+ /* Copy read bits to execute bits */
+ mode |= (mode & 0444) >> 2;
+ mode |= FORCE_DIR_SET_GID;
+ return mode;
+}
+
+int adjust_shared_perm(const char *path)
+{
+ struct stat st;
+ int old_mode, new_mode;
+ if (lstat(path, &st) < 0)
+ return -1;
+ old_mode = st.st_mode;
+
+ if (S_ISDIR(old_mode))
+ new_mode = calc_shared_perm_dir(old_mode);
+ else
+ new_mode = calc_shared_perm(old_mode);
- if (((shared_repository < 0
- ? (orig_mode & (FORCE_DIR_SET_GID | 0777))
- : (orig_mode & mode)) != mode) &&
- chmod(path, (mode & ~S_IFMT)) < 0)
+ if (((old_mode ^ new_mode) & ~S_IFMT) &&
+ chmod(path, (new_mode & ~S_IFMT)) < 0)
return -2;
return 0;
}
--
1.8.1.1
^ 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