* Adding "--ignore-submodules" switch to git-describe
From: Francis Moreau @ 2013-03-01 10:16 UTC (permalink / raw)
To: git
Hello,
Would it make sense to add the option --ignore-submodules (currently
available in git-status) to git-describe when the later is used with
--dirty option ?
Thanks
--
Francis
^ permalink raw reply
* Re: two-way merge corner case bug
From: Jeff King @ 2013-03-01 9:22 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7vwqtulplp.fsf@alter.siamese.dyndns.org>
On Tue, Feb 26, 2013 at 01:41:54PM -0800, Junio C Hamano wrote:
> > Isn't this a repeat of:
> >
> > http://thread.gmane.org/gmane.comp.version-control.git/202440/focus=212316
> [...]
>
> Yeah, I think the patch in your message is a good starting point to
> solve a half of the issue (i.e. unmerged current one should be
> replaced with the version from the "going to" tree). Attached is
> with a slight update.
Thanks for picking this up. I'm not sure, though, about the trigger
here:
> + if (current->ce_flags & CE_CONFLICTED) {
> + if (same(oldtree, newtree) || o->reset) {
> + if (!newtree)
> + return deleted_entry(current, current, o);
> + else
> + return merged_entry(newtree, current, o);
> + }
> + return o->gently ? -1 : reject_merge(current, o);
We have a conflicted state in the index for a path. If o->reset is true,
then we want to throw it out in favor of where we are moving to. So that
part makes sense.
But if we are not in "--reset", and we see that oldtree and newtree are
the same, do we still want to move to newtree? It seems like we are
throwing away the conflicted state that the user has. Usually this isn't
a big deal, but with "-u" it could mean losing any in-progress
resolution in the working tree.
I think this generally shouldn't happen, as "-m" does not operate when
the index contains unmerged entries. So it would really just be a safety
valve for somebody calling into unpack_trees without having done the
index check. IOW, should the same(newtree, oldtree) call be cut there,
and we trigger this just on reset? Any other case would be rejected
(which _shouldn't_ happen, but this keeps us conservative, and if we
find a case where it does happen, we can figure out the semantics then).
> The other half that is not solved by this patch is that we will lose
> Z because merge-recursive removed it when it renamed it and left
> three stages for A in the index, and to "read-tree --reset -u HEAD
> ORIG_HEAD", it looks as if you removed Z yourself while on HEAD and
> want to carry your local change forward to ORIG_HEAD.
>
> I think this too needs to be fixed. We are saying "-u --reset" to
> mean "I want to go there no matter what", not "-u -m" that means "I
> am carrying my changes forward while checking out another branch".
Yeah, hmm. Since we cannot distinguish between such a staged removal and
what merge-recursive deposits in the index, we have to err on on side
(either losing a staged removal, or failing to move to the new state).
What we really want is to detect that Z's removal is related to the
conflicted path A, but we can't do so; that information was never put
into the index in the first place.
We could do this:
diff --git a/unpack-trees.c b/unpack-trees.c
index 3d17108..1e84131 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -1788,7 +1788,7 @@ int twoway_merge(struct cache_entry **src, struct unpack_trees_options *o)
}
}
else if (newtree) {
- if (oldtree && !o->initial_checkout) {
+ if (oldtree && !o->initial_checkout && !o->reset) {
/*
* deletion of the path was staged;
*/
which solves the case you presented. My worry would be that somebody is
using "--reset" but expecting the removal to be carried through
(technically, "--reset" is documented as "-m but discard unmerged
entries", but we are not really treating it that way here). The test
suite passes, but we may not be covering some oddball case.
-Peff
PS I wonder if the "initial_checkout" hack can just go away under this
new rule. Although we don't seem to use "o->reset" for the initial
checkout. Which seems kind of weird. I would think the initial
checkout would actually just be a oneway merge. Is the point to try
to leave any existing working tree contents untouched or something?
^ permalink raw reply related
* Re: [PATCH v8 4/5] Implement line-history search (git log -L)
From: Thomas Rast @ 2013-03-01 8:49 UTC (permalink / raw)
To: Junio C Hamano
Cc: Thomas Rast, git, Bo Yang, Zbigniew Jędrzejewski-Szmek,
Will Palmer
In-Reply-To: <7vmwuogjsm.fsf@alter.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> writes:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> Overall, I like this better than the "log --follow" hack; as the
>> revision traversal is done without any pathspec when being "careful
>> and slow" (aka -M), you do not suffer from the "just use a singleton
>> pathspec globally regardless of what other history paths are being
>> traversed" limitation of "log --follow".
>>
>> The patch series certainly is interesting.
>
> Having said that, I notice that "careful and slow" is just "too slow
> to be usable" even on a small tree like ours. Try running
>
> $ git log -M -L:get_name:builtin/describe.c
>
> and see how long you have to wait until you hit the first line of
> output.
I'll dig some more. It *should* be essentially the following times
taken together:
$ time git log --raw -M --topo-order >/dev/null
real 0m5.448s
user 0m4.599s
sys 0m0.794s
$ time git log -L:get_name:builtin/describe.c >/dev/null
real 0m0.832s
user 0m0.796s
sys 0m0.032s
$ time git log -L:get_name:builtin-describe.c 81b50f3ce40^ >/dev/null
real 0m0.489s
user 0m0.465s
sys 0m0.022s
So I'm losing a factor of about 4 somewhere, which I can't explain right
now.
It could be improved further if --follow was fixed, because then the
first step should be much faster than diffing *all* the trees.
--
Thomas Rast
trast@{inf,student}.ethz.ch
^ permalink raw reply
* Re: Subtree in Git
From: Kindjal @ 2013-03-01 2:28 UTC (permalink / raw)
To: git
In-Reply-To: <2DDAA35052EA4F88A6EAC4FBDDF7FCCD@rr-dav.id.au>
David Michael Barr <b <at> rr-dav.id.au> writes:
> From a quick survey, it appears there are no more than 55 patches
> squashed into the submitted patch.
> As I have an interest in git-subtree for maintaining the out-of-tree
> version of vcs-svn/ and a desire to improve my rebase-fu, I am tempted
> to make some sense of the organic growth that happened on GitHub.
> It doesn't appear that anyone else is willing to do this, so I doubt
> there will be any duplication of effort.
>
What is the status of the work on git-subtree described in this thread?
It looks like it's stalled.
^ permalink raw reply
* Re: how to check pending commits to be pushed?
From: Patricia Bittencourt Egeland @ 2013-03-01 0:32 UTC (permalink / raw)
To: Joachim Schmitz; +Cc: git
In-Reply-To: <kgo9d6$oru$1@ger.gmane.org>
Inside the super repo, I used "git submodule foreach", that way (which worked fine):
git submodule foreach "git status || true"
No idea why the line bellow didn't work:
git submodule foreach "git status"
Thanks,
Patricia
On Feb 28, 2013, at 11:58 AM, Joachim Schmitz <jojo@schmitz-digital.de> wrote:
> Patricia Bittencourt Egeland wrote:
>> 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 ?
>
> git status ?
>
> Bye, Jojo
>
> --
> 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
^ permalink raw reply
* Re: [PATCH] diff: Fix rename pretty-print when suffix and prefix overlap
From: Junio C Hamano @ 2013-02-28 22:29 UTC (permalink / raw)
To: Antoine Pelisse; +Cc: git
In-Reply-To: <1362088540-14564-1-git-send-email-apelisse@gmail.com>
Antoine Pelisse <apelisse@gmail.com> writes:
> When considering a rename for two files that have a suffix and a prefix
> that can overlap, a confusing line is shown. As an example, renaming
> "a/b/b/c" to "a/b/c" shows "a/b/{ => }/b/c", instead of "a/b/{b => }/c"
>
> Currently, what we do is calculate the common prefix ("a/b/"), and the
> common suffix ("/b/c"), but the same "/b/" is actually counted both in
> prefix and suffix. Then when calculating the size of the non-common part,
> we end-up with a negative value which is reset to 0, thus the "{ => }".
>
> Do not allow the common suffix to overlap the common prefix and stop
> when reaching a "/" that would be in both.
>
> Also add some test file to place corner-cases we could met (and this one)
> with rename pretty print.
>
> Signed-off-by: Antoine Pelisse <apelisse@gmail.com>
> ---
Hmm, haven't we already applied this with a fix from Thomas Rast and
queued the result to 'next'?
^ permalink raw reply
* Re: [PATCH v8 4/5] Implement line-history search (git log -L)
From: Junio C Hamano @ 2013-02-28 22:23 UTC (permalink / raw)
To: Thomas Rast; +Cc: git, Bo Yang, Zbigniew Jędrzejewski-Szmek, Will Palmer
In-Reply-To: <7vbob4iaxh.fsf@alter.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> writes:
> Overall, I like this better than the "log --follow" hack; as the
> revision traversal is done without any pathspec when being "careful
> and slow" (aka -M), you do not suffer from the "just use a singleton
> pathspec globally regardless of what other history paths are being
> traversed" limitation of "log --follow".
>
> The patch series certainly is interesting.
Having said that, I notice that "careful and slow" is just "too slow
to be usable" even on a small tree like ours. Try running
$ git log -M -L:get_name:builtin/describe.c
and see how long you have to wait until you hit the first line of
output.
If some of the many NEEDSWORK in the code were fixed, this may
become "very slow but tolerable", but in the current shape, I doubt
it would be prudent to advance this to 'next' and further.
^ permalink raw reply
* Re: [PATCH] diff: Fix rename pretty-print when suffix and prefix overlap
From: Antoine Pelisse @ 2013-02-28 22:22 UTC (permalink / raw)
To: Thomas Rast; +Cc: git
In-Reply-To: <874ngw2ii0.fsf@pctrast.inf.ethz.ch>
On Thu, Feb 28, 2013 at 11:14 PM, Thomas Rast <trast@student.ethz.ch> wrote:
> Antoine Pelisse <apelisse@gmail.com> writes:
>
>> diff --git a/diff.c b/diff.c
>> index 9038f19..e1d82c9 100644
>> --- a/diff.c
>> +++ b/diff.c
>> @@ -1177,7 +1177,16 @@ static char *pprint_rename(const char *a, const char *b)
>> - while (a <= old && b <= new && *old == *new) {
>> + /*
>> + * Note:
>> + * if pfx_length is 0, old/new will never reach a - 1 because it
>> + * would mean the whole string is common suffix. But then, the
>> + * whole string would also be a common prefix, and we would not
>> + * have pfx_length equals 0.
>> + */
>> + while (a + pfx_length - 1 <= old &&
>> + b + pfx_length - 1 <= new &&
>> + *old == *new) {
>
> Umm, you still have the broken version here, and the previous patch is
> already in next. I think you should decide for one thing ;-)
Thanks ! I had not paid enough attention to that.
> Either: consider this a reroll; Junio would have to revert the version
> already in next (which isn't _so_ bad, because next will eventually be
> rebuilt) and apply this new version. But if you do that, you should
> squash my change that deals with the underrun issue (I'd be fine with
> that).
I would not have done that without your consent (that's why I kept the
buggy version)
> Or: consider it an incremental improvement on the series, in which case
> you should send only the tests with a new commit message.
That seems like the best solution to me. I will resend later with just
the tests and a new commit message.
Cheers,
Antoine
^ permalink raw reply
* Re: [PATCH] diff: Fix rename pretty-print when suffix and prefix overlap
From: Thomas Rast @ 2013-02-28 22:14 UTC (permalink / raw)
To: Antoine Pelisse; +Cc: git
In-Reply-To: <1362088540-14564-1-git-send-email-apelisse@gmail.com>
Antoine Pelisse <apelisse@gmail.com> writes:
> diff --git a/diff.c b/diff.c
> index 9038f19..e1d82c9 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -1177,7 +1177,16 @@ static char *pprint_rename(const char *a, const char *b)
> - while (a <= old && b <= new && *old == *new) {
> + /*
> + * Note:
> + * if pfx_length is 0, old/new will never reach a - 1 because it
> + * would mean the whole string is common suffix. But then, the
> + * whole string would also be a common prefix, and we would not
> + * have pfx_length equals 0.
> + */
> + while (a + pfx_length - 1 <= old &&
> + b + pfx_length - 1 <= new &&
> + *old == *new) {
Umm, you still have the broken version here, and the previous patch is
already in next. I think you should decide for one thing ;-)
Either: consider this a reroll; Junio would have to revert the version
already in next (which isn't _so_ bad, because next will eventually be
rebuilt) and apply this new version. But if you do that, you should
squash my change that deals with the underrun issue (I'd be fine with
that).
Or: consider it an incremental improvement on the series, in which case
you should send only the tests with a new commit message.
--
Thomas Rast
trast@{inf,student}.ethz.ch
^ permalink raw reply
* [PATCH] diff: Fix rename pretty-print when suffix and prefix overlap
From: Antoine Pelisse @ 2013-02-28 21:55 UTC (permalink / raw)
To: git; +Cc: Antoine Pelisse
In-Reply-To: <CALWbr2yviqF68zF7mBbhaXW7oFar0YRqROBWXwqjo7UNgZNVBQ@mail.gmail.com>
When considering a rename for two files that have a suffix and a prefix
that can overlap, a confusing line is shown. As an example, renaming
"a/b/b/c" to "a/b/c" shows "a/b/{ => }/b/c", instead of "a/b/{b => }/c"
Currently, what we do is calculate the common prefix ("a/b/"), and the
common suffix ("/b/c"), but the same "/b/" is actually counted both in
prefix and suffix. Then when calculating the size of the non-common part,
we end-up with a negative value which is reset to 0, thus the "{ => }".
Do not allow the common suffix to overlap the common prefix and stop
when reaching a "/" that would be in both.
Also add some test file to place corner-cases we could met (and this one)
with rename pretty print.
Signed-off-by: Antoine Pelisse <apelisse@gmail.com>
---
diff.c | 11 +++++++++-
t/t4056-rename-pretty.sh | 54 ++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 64 insertions(+), 1 deletion(-)
create mode 100755 t/t4056-rename-pretty.sh
diff --git a/diff.c b/diff.c
index 9038f19..e1d82c9 100644
--- a/diff.c
+++ b/diff.c
@@ -1177,7 +1177,16 @@ static char *pprint_rename(const char *a, const char *b)
old = a + len_a;
new = b + len_b;
sfx_length = 0;
- while (a <= old && b <= new && *old == *new) {
+ /*
+ * Note:
+ * if pfx_length is 0, old/new will never reach a - 1 because it
+ * would mean the whole string is common suffix. But then, the
+ * whole string would also be a common prefix, and we would not
+ * have pfx_length equals 0.
+ */
+ while (a + pfx_length - 1 <= old &&
+ b + pfx_length - 1 <= new &&
+ *old == *new) {
if (*old == '/')
sfx_length = len_a - (old - a);
old--;
diff --git a/t/t4056-rename-pretty.sh b/t/t4056-rename-pretty.sh
new file mode 100755
index 0000000..806046f
--- /dev/null
+++ b/t/t4056-rename-pretty.sh
@@ -0,0 +1,54 @@
+#!/bin/sh
+
+test_description='Rename pretty print
+
+'
+
+. ./test-lib.sh
+
+test_expect_success nothing_common '
+ mkdir -p a/b/ &&
+ : >a/b/c &&
+ git add a/b/c &&
+ git commit -m. &&
+ mkdir -p c/b/ &&
+ git mv a/b/c c/b/a &&
+ git commit -m. &&
+ git show -M --summary >output &&
+ test_i18ngrep "a/b/c => c/b/a" output
+'
+
+test_expect_success common_prefix '
+ mkdir -p c/d &&
+ git mv c/b/a c/d/e &&
+ git commit -m. &&
+ git show -M --summary >output &&
+ test_i18ngrep "c/{b/a => d/e}" output
+'
+
+test_expect_success common_suffix '
+ mkdir d &&
+ git mv c/d/e d/e &&
+ git commit -m. &&
+ git show -M --summary >output &&
+ test_i18ngrep "{c/d => d}/e" output
+'
+
+test_expect_success common_suffix_prefix '
+ mkdir d/f &&
+ git mv d/e d/f/e &&
+ git commit -m. &&
+ git show -M --summary >output &&
+ test_i18ngrep "d/{ => f}/e" output
+'
+
+test_expect_success common_overlap '
+ mkdir d/f/f &&
+ git mv d/f/e d/f/f/e &&
+ git commit -m. &&
+ git show -M --summary >output &&
+ test_i18ngrep "d/f/{ => f}/e" output
+'
+
+
+test_done
--
1.7.9.5
^ permalink raw reply related
* Re: [PATCH 2/2] describe: Exclude --all --match=PATTERN
From: Junio C Hamano @ 2013-02-28 21:50 UTC (permalink / raw)
To: Greg Price; +Cc: git
In-Reply-To: <7v1uc1jyq0.fsf@alter.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> writes:
> 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.
A fix to the broken semantics may look like this. There are a few
points to note:
* The local variable names "is_tag" and "might_be_tag" were
inconsistent with the rest of the program, where the global
variable "tags" is used to mean "the user gave --tags to allow
lightweight ones to be used". By that definition of the tag, a
ref under refs/tags/ *is* a tag, and a ref that peels to a
different object is an annotated tag. These two variable names
have been fixed.
* The function returns early for a ref outside refs/tags/ when
"--all" is not given with or without this patch. At the end of
the function, it also returned when (!all && !prio), but prio
becomes zero only when the ref is outside refs/tags/ (or the tag
does not match the pattern) in the original code. With this
patch, we reject refs outside refs/tags/ early when "--all" is
not given, so the last-minute check before add_to_known_names()
becomes unnecessary (hence removed).
* If somebody is crazy enough to have an annotated tag under
refs/heads/, the code would treat it as an annotated tag and
assign prio==2 to it, with or without this patch. We may want to
tighten this further by checking with is_tag, but this patch does
not do anything about it; I wanted it to focus on only one bug,
i.e. interaction between "--all" and "--match=<pattern>".
* When "--tags" is not given, we still give an unannotated tag to
add_to_known_names(), only to issue a hint when the given commit
is not describable with annotated tags but it could be described
if "--tags" were given. I think this is optimizing for the wrong
case, and wasting resources.
builtin/describe.c | 41 ++++++++++++++++++++---------------------
1 file changed, 20 insertions(+), 21 deletions(-)
diff --git a/builtin/describe.c b/builtin/describe.c
index 04c185b..b2b740d 100644
--- a/builtin/describe.c
+++ b/builtin/describe.c
@@ -137,40 +137,39 @@ static void add_to_known_names(const char *path,
static int get_name(const char *path, const unsigned char *sha1, int flag, void *cb_data)
{
- int might_be_tag = !prefixcmp(path, "refs/tags/");
+ int is_tag = !prefixcmp(path, "refs/tags/");
unsigned char peeled[20];
- int is_tag, prio;
+ int is_annotated, prio;
- if (!all && !might_be_tag)
+ /* Reject anything outside refs/tags/ unless --all */
+ if (!all && !is_tag)
return 0;
+ /* Accept only tags that match the pattern, if given */
+ if (pattern && (!is_tag || fnmatch(pattern, path + 10, 0)))
+ return 0;
+
+ /* Is it annotated? */
if (!peel_ref(path, peeled)) {
- is_tag = !!hashcmp(sha1, peeled);
+ is_annotated = !!hashcmp(sha1, peeled);
} else {
hashcpy(peeled, sha1);
- is_tag = 0;
+ is_annotated = 0;
}
- /* If --all, then any refs are used.
- * If --tags, then any tags are used.
- * Otherwise only annotated tags are used.
+ /*
+ * By default, we only use annotated tags, but with --tags
+ * we fall back to lightweight ones (even without --tags,
+ * we still remember lightweight ones, only to give hints
+ * in an error message). --all allows any refs to be used.
*/
- if (might_be_tag) {
- if (is_tag)
- prio = 2;
- else
- prio = 1;
-
- if (pattern && fnmatch(pattern, path + 10, 0))
- prio = 0;
- }
+ if (is_annotated)
+ prio = 2;
+ else if (is_tag)
+ prio = 1;
else
prio = 0;
- if (!all) {
- if (!prio)
- return 0;
- }
add_to_known_names(all ? path + 5 : path + 10, peeled, prio, sha1);
return 0;
}
^ permalink raw reply related
* [PATCH v2] contrib/subtree: allow addition of remote branch with name not locally present
From: Paul Campbell @ 2013-02-28 21:41 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>
New tests check to prevent pulling in multiple branches or creating a local
branch for the subtree.
Signed-off-by: Paul Campbell <pcampbell@carnegiecollege.ac.uk>
---
I've rerolled this with alternate tests.
Is there anything else that should be tested for?
contrib/subtree/git-subtree.sh | 15 ++++++++-------
1 file changed, 8 insertions(+), 7 deletions(-)
diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh
index 8a23f58..d04bd25 100755
--- a/contrib/subtree/git-subtree.sh
+++ b/contrib/subtree/git-subtree.sh
@@ -503,13 +503,14 @@ 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"
+ case "$2" in
+ *\**) # Avoid pulling in multiple branches
+ die "'$2' contains a wildcard"
+ ;;
+ *\:*) # Don't create a local branch for the subtree
+ die "'$2' contains a local branch name"
+ ;;
+ esac
"cmd_add_repository" "$@"
else
--
1.8.2.rc1
^ permalink raw reply related
* Re: [PATCH v8 4/5] Implement line-history search (git log -L)
From: Thomas Rast @ 2013-02-28 21:41 UTC (permalink / raw)
To: Junio C Hamano
Cc: Thomas Rast, git, Bo Yang, Zbigniew Jędrzejewski-Szmek,
Will Palmer
In-Reply-To: <7v38wgi38z.fsf@alter.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> writes:
> Thomas Rast <trast@student.ethz.ch> writes:
>
>> Junio C Hamano <gitster@pobox.com> writes:
>>
>>>> +/*
>>>> + * NEEDSWORK: manually building a diff here is not the Right
>>>> + * Thing(tm). log -L should be built into the diff pipeline.
>>>
>>> I am not sure about this design, and do not necessarily agree that
>>> wedging this to the diff pipeline is the right future direction.
>>>
>>> I have a feeling that "log -L" should actually be built around
>>> "blame". You let blame to hit the first parent to take the blame,
>>> and then turn around to show a single "diff-tree" between the child
>>> and the parent with whatever other diff pipeline gizmo the user can
>>> give you from the command line. The blame also tells you what the
>>> "interesting" line range were at the first parent commit you found,
>>> so you can re-run the same thing with an updated range.
>>
>> Hrm, now that you mention it, this is actually a brilliant idea.
>
> I don't know. That is just me handwaving without giving a serious
> end-to-end thought.
Having thought about it for some time, I think we need to figure out
if/what can be shared. I can't shake off the feeling that *something*
should be common between blame and log -L, but I can't exactly say what
so far.
Your suggestion of looking at the first blame hit is almost there. (And
in fact if it did work, it should be rather easy to prototype from blame
--incremental.) However, it works only for additions, not removals.
Lines that were removed do not show up in blame at all, and since a
patch can also _only_ remove lines, blame would not find it even if we
adjust the blamed range at every found commit.
It may be possible to fix that by doing a reverse blame, but I suspect
that runs into yet more trouble when trying to reverse-blame from two
different sides of history.
Then there's a different issue if the order of code flips. Suppose you
have
A1
A2
B1
B2
and later change to
B1
B2
A1
A2
Diffs can fundamentally not express this; they'll only see one side as
unchanged, and the other as completely new. Ideally we'd be able to
track this case correctly -- as blame does? -- no matter which part is
within our tracked range.
Anyway, I believe this should be booked under "future improvements".
I've had it in my own tree for ages and it already does the right thing
most of the time :-)
--
Thomas Rast
trast@{inf,student}.ethz.ch
^ permalink raw reply
* Re: [BUG/PATCH] contrib/subtree: allow addition of remote branch with name not locally present
From: Paul Campbell @ 2013-02-28 21:37 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Adam Tkac, David Greene, Jesper Lyager Nielsen,
Michael Schubert, Techlive Zheng
In-Reply-To: <7vwqtti91k.fsf@alter.siamese.dyndns.org>
On Thu, Feb 28, 2013 at 12:20 AM, Junio C Hamano <gitster@pobox.com> wrote:
> 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.
Thanks for the feedback.
Checking for a ':' or a '*' in the refspec should stop the storage
name and wildcards getting through.
Rerolling the patch with new test.
--
Paul [W] Campbell
^ permalink raw reply
* Re: [PATCH v8 4/5] Implement line-history search (git log -L)
From: Junio C Hamano @ 2013-02-28 20:37 UTC (permalink / raw)
To: Thomas Rast; +Cc: git, Bo Yang, Zbigniew Jędrzejewski-Szmek, Will Palmer
In-Reply-To: <87fw0g6xp4.fsf@pctrast.inf.ethz.ch>
Thomas Rast <trast@student.ethz.ch> writes:
> Junio C Hamano <gitster@pobox.com> writes:
>
>>> +/*
>>> + * NEEDSWORK: manually building a diff here is not the Right
>>> + * Thing(tm). log -L should be built into the diff pipeline.
>>
>> I am not sure about this design, and do not necessarily agree that
>> wedging this to the diff pipeline is the right future direction.
>>
>> I have a feeling that "log -L" should actually be built around
>> "blame". You let blame to hit the first parent to take the blame,
>> and then turn around to show a single "diff-tree" between the child
>> and the parent with whatever other diff pipeline gizmo the user can
>> give you from the command line. The blame also tells you what the
>> "interesting" line range were at the first parent commit you found,
>> so you can re-run the same thing with an updated range.
>
> Hrm, now that you mention it, this is actually a brilliant idea.
I don't know. That is just me handwaving without giving a serious
end-to-end thought.
^ permalink raw reply
* Re: [PATCH v8 0/5] git log -L
From: Junio C Hamano @ 2013-02-28 19:56 UTC (permalink / raw)
To: Thomas Rast; +Cc: git, Bo Yang, Zbigniew Jędrzejewski-Szmek, Will Palmer
In-Reply-To: <cover.1362069310.git.trast@student.ethz.ch>
Thomas Rast <trast@student.ethz.ch> writes:
> For an instructive example, apply it and run
>
> git log -L:archiver:archive.h
>
> in your git.git.
Fun ;-)
This is queued at the tip of 'pu' so that poeple can have fun.
Thanks.
^ permalink raw reply
* Re: [PATCH v8 4/5] Implement line-history search (git log -L)
From: Thomas Rast @ 2013-02-28 19:32 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Bo Yang, Zbigniew Jędrzejewski-Szmek, Will Palmer
In-Reply-To: <7vbob4iaxh.fsf@alter.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> writes:
>> +/*
>> + * NEEDSWORK: manually building a diff here is not the Right
>> + * Thing(tm). log -L should be built into the diff pipeline.
>
> I am not sure about this design, and do not necessarily agree that
> wedging this to the diff pipeline is the right future direction.
>
> I have a feeling that "log -L" should actually be built around
> "blame". You let blame to hit the first parent to take the blame,
> and then turn around to show a single "diff-tree" between the child
> and the parent with whatever other diff pipeline gizmo the user can
> give you from the command line. The blame also tells you what the
> "interesting" line range were at the first parent commit you found,
> so you can re-run the same thing with an updated range.
Hrm, now that you mention it, this is actually a brilliant idea.
--
Thomas Rast
trast@{inf,student}.ethz.ch
^ permalink raw reply
* Re: [PATCH v8 1/5] Refactor parse_loc
From: Thomas Rast @ 2013-02-28 19:24 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Bo Yang, Zbigniew Jędrzejewski-Szmek, Will Palmer
In-Reply-To: <7vobf4icjh.fsf@alter.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> writes:
> 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.
Even though I am moving from builtin/blame.c to line-log.c? I would
otherwise have to call from a rather lib-ish file into a "frontend"
file. I always figured I wasn't supposed to do that.
--
Thomas Rast
trast@{inf,student}.ethz.ch
^ permalink raw reply
* Re: zsh completion broken for file completion
From: Manlio Perillo @ 2013-02-28 18:59 UTC (permalink / raw)
To: Matthieu Moy; +Cc: git, Felipe Contreras
In-Reply-To: <vpqtxowp9e2.fsf@grenoble-inp.fr>
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Il 28/02/2013 19:43, Matthieu Moy ha scritto:
> Hi,
>
> The completion for e.g. "git add file<tab>" is broken in master. I get
> the following result:
>
> git add fo__gitcomp_file:8: command not found: compgen
>
> The guilty commit is fea16b47b60 (Fri Jan 11 19:48:43 2013, Manlio
> Perillo, git-completion.bash: add support for path completion), which
> introduces a new __gitcomp_file function that uses the bash builtin
> "compgen", without redefining the function in git-completion.zsh.
>
> [...]
> diff --git a/contrib/completion/git-completion.zsh b/contrib/completion/git-completion.zsh
> index 4577502..0ba1dcf 100644
> --- a/contrib/completion/git-completion.zsh
> +++ b/contrib/completion/git-completion.zsh
> @@ -60,6 +60,15 @@ __gitcomp_nl ()
> compadd -Q -S "${4- }" -p "${2-}" -- ${=1} && _ret=0
> }
>
> +__gitcomp_file ()
> +{
> + emulate -L zsh
> +
> + local IFS=$'\n'
> + compset -P '*[=:]'
> + compadd -Q -p "${2-}" -- ${=1} && _ret=0
> +}
> +
This patch is implemented in fea16b47b60, but only for the deprecated
zsh compatibility code inside git-completion.bash.
The reason I did not provided a patch for git-completion.zsh was because
there was a bug in this script [1].
If any changes are made to git-completion.zsh, please update
git-completion.bash, too.
[1] Basically, on my system I need the following change at the end of
the file:
-_git
+autoload -U +X compinit && compinit
+compdef _git git gitk
I don't know the reason, however; and it seems that it is a problem
only for me
> [...]
Regards Malio
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/
iEYEARECAAYFAlEvqRUACgkQscQJ24LbaURASgCeILUTXAiZA6Ndf2DHByJfv4nT
2bMAn1gPqSdfIBzb0cexwYNoAuD5j2+O
=sKTR
-----END PGP SIGNATURE-----
^ permalink raw reply
* Re: how to check pending commits to be pushed?
From: Joachim Schmitz @ 2013-02-28 18:58 UTC (permalink / raw)
To: git
In-Reply-To: <C89CA603-2D94-44DB-8576-A296BAB5DA53@linea.gov.br>
Patricia Bittencourt Egeland wrote:
> 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 ?
git status ?
Bye, Jojo
^ permalink raw reply
* zsh completion broken for file completion
From: Matthieu Moy @ 2013-02-28 18:43 UTC (permalink / raw)
To: git, Felipe Contreras, Manlio Perillo
Hi,
The completion for e.g. "git add file<tab>" is broken in master. I get
the following result:
git add fo__gitcomp_file:8: command not found: compgen
The guilty commit is fea16b47b60 (Fri Jan 11 19:48:43 2013, Manlio
Perillo, git-completion.bash: add support for path completion), which
introduces a new __gitcomp_file function that uses the bash builtin
"compgen", without redefining the function in git-completion.zsh.
The following proof-of-concept patch seems to fix the problem for me (I
basically copied the __gitcomp_nl function to __gitcomp_file and removed
the '-S "${4- }"'). The bash version does "compopt -o filenames", I
don't know what zsh equivalent is.
diff --git a/contrib/completion/git-completion.zsh b/contrib/completion/git-completion.zsh
index 4577502..0ba1dcf 100644
--- a/contrib/completion/git-completion.zsh
+++ b/contrib/completion/git-completion.zsh
@@ -60,6 +60,15 @@ __gitcomp_nl ()
compadd -Q -S "${4- }" -p "${2-}" -- ${=1} && _ret=0
}
+__gitcomp_file ()
+{
+ emulate -L zsh
+
+ local IFS=$'\n'
+ compset -P '*[=:]'
+ compadd -Q -p "${2-}" -- ${=1} && _ret=0
+}
+
_git ()
{
local _ret=1
Felipe, you know ZSH completion much better than me. Could you turn this
into a real patch?
Thanks,
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
^ permalink raw reply related
* Re: [PATCH v8 4/5] Implement line-history search (git log -L)
From: Junio C Hamano @ 2013-02-28 17:51 UTC (permalink / raw)
To: Thomas Rast; +Cc: git, Bo Yang, Zbigniew Jędrzejewski-Szmek, Will Palmer
In-Reply-To: <9af548b2a7e4a4da9eb30e99b0223f20788b4fc1.1362069310.git.trast@student.ethz.ch>
Thomas Rast <trast@student.ethz.ch> writes:
> @@ -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.
Perhaps have "^" before that "int"?
> 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)
Lose the extra SP (everywhere in this file).
> +{
> + 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;
> }
This should have been part of the earlier patches, no?
> @@ -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)
> +{
> +...
Yuck.
> +/*
> + * NEEDSWORK: manually building a diff here is not the Right
> + * Thing(tm). log -L should be built into the diff pipeline.
I am not sure about this design, and do not necessarily agree that
wedging this to the diff pipeline is the right future direction.
I have a feeling that "log -L" should actually be built around
"blame". You let blame to hit the first parent to take the blame,
and then turn around to show a single "diff-tree" between the child
and the parent with whatever other diff pipeline gizmo the user can
give you from the command line. The blame also tells you what the
"interesting" line range were at the first parent commit you found,
so you can re-run the same thing with an updated range.
> +/*
> + * 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 */
Indeed ;-)
> + 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);
> + }
The code makes the decoration look-up for the same commit multiple
times in a single codeflow; perhaps this entry point should keep the
look-up result and pass it down to the callchain?
Overall, I like this better than the "log --follow" hack; as the
revision traversal is done without any pathspec when being "careful
and slow" (aka -M), you do not suffer from the "just use a singleton
pathspec globally regardless of what other history paths are being
traversed" limitation of "log --follow".
The patch series certainly is interesting.
^ permalink raw reply
* Re: [PATCH v8 3/5] Export rewrite_parents() for 'log -L'
From: Junio C Hamano @ 2013-02-28 17:19 UTC (permalink / raw)
To: Thomas Rast; +Cc: git, Bo Yang, Zbigniew Jędrzejewski-Szmek, Will Palmer
In-Reply-To: <8269ddf785d10c05e39905041df317b685e85168.1362069310.git.trast@student.ethz.ch>
Thomas Rast <trast@student.ethz.ch> writes:
> 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.
Did you mean log-line.c or line-log.c or something?
> 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
^ permalink raw reply
* Re: [PATCH v8 2/5] blame: introduce $ as "end of file" in -L syntax
From: Junio C Hamano @ 2013-02-28 17:18 UTC (permalink / raw)
To: Thomas Rast; +Cc: git, Bo Yang, Zbigniew Jędrzejewski-Szmek, Will Palmer
In-Reply-To: <7973d90cfcd86a3c15776b8718ad6bd84ee7b4ac.1362069310.git.trast@student.ethz.ch>
Thomas Rast <trast@student.ethz.ch> writes:
> 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>,$'.
Doesn't "-L <begin>" or "-L <begin>," do that already? If it were
to introduce "-L $-4," or "-L$-4,+2", I would understand why the
addition may be useful, but otherwise I do not think it adds much
value.
>
> 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>.
^ 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