Git development
 help / color / mirror / Atom feed
* RE: newb: Given a commit id, find which branches have it as an ancestor
From: Kelly F. Hickel @ 2009-03-12 19:38 UTC (permalink / raw)
  To: j.sixt; +Cc: git
In-Reply-To: <63BEA5E623E09F4D92233FB12A9F794302E0F9B2@emailmn.mqsoftware.com>

> From: git-owner@vger.kernel.org [mailto:git-owner@vger.kernel.org] On
> Behalf Of Johannes Sixt
> Sent: Thursday, March 12, 2009 10:38 AM
> To: Kelly F. Hickel

> $ git branch -a --contains the-sha1
> 
> -- Hannes
> 

Thanks, that looks like a really useful command.

Unfortunately, in this case it didn't print anything out (neither did
"git branch -r -a sha1").

What I'm beginning to suspect is that all the commits that should have
gone to master went to some unnamed branch.
Is that reasonable/possible/likely?  This commit has a full ancestry,
but doesn't appear to be on any branch.

In the above question there's an assumption that if a branch exists
without a name, then git branch -a --contains wouldn't print anything
out, is that correct?

Thanks,
Kelly

^ permalink raw reply

* Re: Google Summer of Code 2009: GIT
From: saurabh gupta @ 2009-03-12 19:45 UTC (permalink / raw)
  To: david; +Cc: Junio C Hamano, Johannes Schindelin, git
In-Reply-To: <alpine.DEB.1.10.0903121214390.16753@asgard.lang.hm>

On Fri, Mar 13, 2009 at 12:59 AM,  <david@lang.hm> wrote:
> On Fri, 13 Mar 2009, saurabh gupta wrote:
>
> defining terminology that was mentioned before
>
> merge drivers are run by git to do the merges and create the conflict
> markers. git already has a 'plug-in architecture' for these drivers (you can
> define file types and tell git to use a particular merge driver for this
> file type)
>
> merge helpers are run by the users if there is a conflict and make use of
> the markers. depending on what you end up using for conflict markers, you
> may not need to write a merge helper (for OO, if your conflict markers are
> good enough you can use OO to resolve conflicts easily, no need for a new
> tool)
>
>
> with this terminology, you can't do merge helpers without doing the merge
> drivers first (what does the helper look for as an indicator of a conflict?)
>
> I believe that there is a lot of potential for a configurable merge driver
> to support many similar formats.
>
> using the example of XML-based files, configurable options could include
>
> 1. is the file stored compressed or not
>
> 2. does the order of the tags matter
>
> 3. does whitespace matter
>
>  note: #2 and #3 may boil down to 'is this a document with XML markup, or
> are the XML tags the primary content'
>
> 4. how is the conflict marked
>
> 4a. wrap the conflicting tags in a set of tags that look like _
>
> 4b. if the conflict is in the content, not the tags, modify it similar to
> what we do with text today.
>
>  note: this still requires the new driver to decide if there is a conflict
> or not
>
> 4c. other (potentially including calling out to other code for more drastic
> restructuring)
>
>
> with a merge driver along these lines you can handle many different types of
> XML documents.
>
> with SVG you may be able to put the offending tags in different layers
>
> with OO you may be able to put in tags that indicate a merge conflict in a
> way that OO will directly handle
>
> etc.
>
> in many cases you may not even need to create a merge helper or library for
> other software you use. you just need to figure out what sort of
> manipulation would need to be done to to file to mark the conflict in a way
> that existing applications can understand.

Very well described, David. I agree with you and providing these merge
options to the user, merge drivers can do the work and mark the
conflicts according to the option. The work to do is to modify the
merge driver. I think in this way, even people who have only a
terminal can also gain from it. They can choose the apt option to see
the conflict markers in their way. So, the aim is to make merge driver
configurable and create the merged/conflicted file according to the
options.


-- 
Saurabh Gupta
Senior,
NSIT,New Delhi, India

^ permalink raw reply

* Re: Google Summer of Code 2009: GIT
From: david @ 2009-03-12 19:59 UTC (permalink / raw)
  To: saurabh gupta; +Cc: Junio C Hamano, Johannes Schindelin, git
In-Reply-To: <ab9fa62a0903121245m621643bfq3c58557ccc9b266f@mail.gmail.com>

On Fri, 13 Mar 2009, saurabh gupta wrote:

> Very well described, David. I agree with you and providing these merge
> options to the user, merge drivers can do the work and mark the
> conflicts according to the option. The work to do is to modify the
> merge driver. I think in this way, even people who have only a
> terminal can also gain from it. They can choose the apt option to see
> the conflict markers in their way. So, the aim is to make merge driver
> configurable and create the merged/conflicted file according to the
> options.

for the GSOC I suspect that the right thing to do is the define one or 
more merge drivers to create, and list what applications are going to be 
used for testing these merges.

you and the mentor can decide what is a reasonable amount of work.

it may be just doing an XML merge driver is a summer's worth of work, or 
it may be that it's not really enough and you should try to do another one 
or two.

it also may be that there is a lot of overlap between different merge 
drivers, and once you have the XML driver the others become fairly trivial 
to do. (I'm thinking the config file examples I posted earlier in the 
thread)

David Lang

^ permalink raw reply

* Re: Google Summer of Code 2009: GIT
From: saurabh gupta @ 2009-03-12 20:03 UTC (permalink / raw)
  To: david; +Cc: Junio C Hamano, Johannes Schindelin, git
In-Reply-To: <alpine.DEB.1.10.0903121255040.16753@asgard.lang.hm>

On Fri, Mar 13, 2009 at 1:29 AM,  <david@lang.hm> wrote:
> On Fri, 13 Mar 2009, saurabh gupta wrote:
>
>> Very well described, David. I agree with you and providing these merge
>> options to the user, merge drivers can do the work and mark the
>> conflicts according to the option. The work to do is to modify the
>> merge driver. I think in this way, even people who have only a
>> terminal can also gain from it. They can choose the apt option to see
>> the conflict markers in their way. So, the aim is to make merge driver
>> configurable and create the merged/conflicted file according to the
>> options.
>
> for the GSOC I suspect that the right thing to do is the define one or more
> merge drivers to create, and list what applications are going to be used for
> testing these merges.
>
> you and the mentor can decide what is a reasonable amount of work.
>

I will very glad to hear about this thing from the mentor (Johannes
Schindelin, according to wiki). I will try to plan out the things in a
proper way to carry out this project if I get a chance to work on this
for GSoC 2009.

> it may be just doing an XML merge driver is a summer's worth of work, or it
> may be that it's not really enough and you should try to do another one or
> two.
>
> it also may be that there is a lot of overlap between different merge
> drivers, and once you have the XML driver the others become fairly trivial
> to do. (I'm thinking the config file examples I posted earlier in the
> thread)

with the options given to the user, one can handle the config files
also where order doesn't matter and also the whitespaces problem can
also be handled in the similar way.


-- 
Saurabh Gupta
Senior,
NSIT,New Delhi, India

^ permalink raw reply

* Re: Google Summer of Code 2009: GIT
From: Junio C Hamano @ 2009-03-12 20:18 UTC (permalink / raw)
  To: david; +Cc: saurabh gupta, Johannes Schindelin, git
In-Reply-To: <alpine.DEB.1.10.0903121214390.16753@asgard.lang.hm>

david@lang.hm writes:

> defining terminology that was mentioned before
>
> merge drivers are run by git to do the merges and create the conflict
> markers. git already has a 'plug-in architecture' for these drivers
> (you can define file types and tell git to use a particular merge
> driver for this file type)
>
> merge helpers are run by the users if there is a conflict and make use
> of the markers. depending on what you end up using for conflict
> markers, you may not need to write a merge helper (for OO, if your
> conflict markers are good enough you can use OO to resolve conflicts
> easily, no need for a new tool)

Not really.  A merge helper can look at the stages in the index to get the
(original, ours, theirs) tuple and start to work from there (and doing a
helper as a backend of mergetool will be one way to make it easier), and
for such helper the driver does not have to do anything.

> with this terminology, you can't do merge helpers without doing the
> merge drivers first (what does the helper look for as an indicator of
> a conflict?)

The answer to that question is "the index", and your "you can't" is
too strong.

I agree with you that the original "editor" for the specific type of
document (e.g. OOo, inkskape, ...) can be used to fix things up if a
driver can leave conflict markers in such a way that the helper does not
have to do three-file merge, and that would be a nice thing to have.  But
for a helper that can do a real three-file merge (and in this thread I
think somebody said OOo can do that), then it is Ok for a driver to punt.

But even then, *if* there is a driver *and* it can do trivial merges
cleanly and safely, it would be a huge productivity win, as you do not
have to run a helper every time you see two branching touching the same
document even they did so to edit totally unrelated parts of it.

^ permalink raw reply

* Re: What's cooking in git.git (Mar 2009, #03; Wed, 11)
From: René Scharfe @ 2009-03-12 20:22 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git
In-Reply-To: <7vvdqg5s17.fsf@gitster.siamese.dyndns.org>

Junio C Hamano schrieb:
> Tonight's 'pu' does not pass its self-test and it is expected; I won't be
> fixing it and I'm going to bed now.

This fixes a segfault:

diff --git a/remote.c b/remote.c
index 68c1a84..ea1841e 100644
--- a/remote.c
+++ b/remote.c
@@ -655,9 +655,9 @@ struct remote *remote_get(const char *name)
 	struct remote *ret;
 	int name_given = 0;
 
 	read_config();
-	if (name || strcmp(name, "-"))
+	if (name && strcmp(name, "-"))
 		name_given = 1;
 	else {
 		name = default_remote_name;
 		name_given = explicit_default_remote_name;

^ permalink raw reply related

* Re: What's cooking in git.git (Mar 2009, #03; Wed, 11)
From: Junio C Hamano @ 2009-03-12 20:43 UTC (permalink / raw)
  To: René Scharfe; +Cc: git
In-Reply-To: <49B96F1A.3060001@lsrfire.ath.cx>

René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:

> Junio C Hamano schrieb:
>> Tonight's 'pu' does not pass its self-test and it is expected; I won't be
>> fixing it and I'm going to bed now.
>
> This fixes a segfault:

Oops, you're right.


> diff --git a/remote.c b/remote.c
> index 68c1a84..ea1841e 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -655,9 +655,9 @@ struct remote *remote_get(const char *name)
>  	struct remote *ret;
>  	int name_given = 0;
>  
>  	read_config();
> -	if (name || strcmp(name, "-"))
> +	if (name && strcmp(name, "-"))
>  		name_given = 1;
>  	else {
>  		name = default_remote_name;
>  		name_given = explicit_default_remote_name;

^ permalink raw reply

* Re: Google Summer of Code 2009: GIT
From: david @ 2009-03-12 20:45 UTC (permalink / raw)
  To: saurabh gupta; +Cc: Junio C Hamano, Johannes Schindelin, git
In-Reply-To: <ab9fa62a0903121303v5a6cbf0ax413cc440b9c32e77@mail.gmail.com>

On Fri, 13 Mar 2009, saurabh gupta wrote:

> On Fri, Mar 13, 2009 at 1:29 AM,  <david@lang.hm> wrote:
>> On Fri, 13 Mar 2009, saurabh gupta wrote:
>>
>>> Very well described, David. I agree with you and providing these merge
>>> options to the user, merge drivers can do the work and mark the
>>> conflicts according to the option. The work to do is to modify the
>>> merge driver. I think in this way, even people who have only a
>>> terminal can also gain from it. They can choose the apt option to see
>>> the conflict markers in their way. So, the aim is to make merge driver
>>> configurable and create the merged/conflicted file according to the
>>> options.
>>
>> for the GSOC I suspect that the right thing to do is the define one or more
>> merge drivers to create, and list what applications are going to be used for
>> testing these merges.
>>
>> you and the mentor can decide what is a reasonable amount of work.
>>
>
> I will very glad to hear about this thing from the mentor (Johannes
> Schindelin, according to wiki). I will try to plan out the things in a
> proper way to carry out this project if I get a chance to work on this
> for GSoC 2009.
>
>> it may be just doing an XML merge driver is a summer's worth of work, or it
>> may be that it's not really enough and you should try to do another one or
>> two.
>>
>> it also may be that there is a lot of overlap between different merge
>> drivers, and once you have the XML driver the others become fairly trivial
>> to do. (I'm thinking the config file examples I posted earlier in the
>> thread)
>
> with the options given to the user, one can handle the config files
> also where order doesn't matter and also the whitespaces problem can
> also be handled in the similar way.

when I am mentioning config files here I'm thinking of ones that don't use 
XML (such as the git config file)

a 'paragraph' merge driver could also help with things like a maintainers 
file where the order of the paragaphs doesn't matter, just the content 
inside each one.

that's very similar to re-ordering XML tags, but with a slightly different 
syntax

David Lang

^ permalink raw reply

* Transparently encrypt repository contents with GPG
From: Matthias Nothhaft @ 2009-03-12 21:19 UTC (permalink / raw)
  To: git

Hi,

I'm new to Git but I really already love it. ;-)

I would like to have repository that transparently encrypts and
decrypts all files using GPG.

What I need is a way to automatically modify each file

a) before it is written in the repository
b) after it is read from the repository

Is there a way to get this work somehow? Can someone give me some
hints where I need to begin?

regards,
Matthias

^ permalink raw reply

* Re: Transparently encrypt repository contents with GPG
From: Sverre Rabbelier @ 2009-03-12 21:34 UTC (permalink / raw)
  To: Matthias Nothhaft; +Cc: git
In-Reply-To: <978bdee00903121419o61cd7a87rb55809796bd257d7@mail.gmail.com>

Heya,

On Thu, Mar 12, 2009 at 22:19, Matthias Nothhaft
<matthias.nothhaft@googlemail.com> > What I need is a way to
automatically modify each file
>
> a) before it is written in the repository
> b) after it is read from the repository

Have a look at smudging, you might not need to touch the git source
code at all ;).

-- 
Cheers,

Sverre Rabbelier

^ permalink raw reply

* git apply won't work
From: Martin Paraskevov @ 2009-03-12 23:14 UTC (permalink / raw)
  To: git

Hi all,

I did the following:

edited
did commit 1
edited
did commit 2
edited
did commit 3

Now I want to patch the changes between 2 and 3 onto 1, i.e. have commit 3 but
with the changes from commit 2 removed. I created a branch where I reset
it to commit 1 and then tried to apply the diff between 3 and 2.

The patch however won't patch certain files. It contains the excerpt below,
for example, so it should be patching directory.c but it isn't. The command
I'm running is just:

% git apply filesys.patch

What am I doing wrong?


diff --git a/src/filesys/directory.c b/src/filesys/directory.c
index 0d265d5..31b7fd6 100644
--- a/src/filesys/directory.c
+++ b/src/filesys/directory.c
@@ -2,50 +2,57 @@
 #include <stdio.h>
 #include <string.h>
 #include <list.h>
+#include "filesys/file.h"
 #include "filesys/filesys.h"
 #include "filesys/inode.h"
 #include "threads/malloc.h"
-
-/* A directory. */
-struct dir
-  {
-    struct inode *inode;                /* Backing store. */
-    off_t pos;                          /* Current position. */
-  };

... more lines ...


- Martin

^ permalink raw reply related

* Re: git apply won't work
From: Bryan Donlan @ 2009-03-13  0:09 UTC (permalink / raw)
  To: Martin Paraskevov; +Cc: git
In-Reply-To: <a6d7dc140903121614s229ce97an2cb5737ef46c0421@mail.gmail.com>

On Thu, Mar 12, 2009 at 7:14 PM, Martin Paraskevov
<martin.paraskevov@gmail.com> wrote:
> Hi all,
>
> I did the following:
>
> edited
> did commit 1
> edited
> did commit 2
> edited
> did commit 3
>
> Now I want to patch the changes between 2 and 3 onto 1, i.e. have commit 3 but
> with the changes from commit 2 removed. I created a branch where I reset
> it to commit 1 and then tried to apply the diff between 3 and 2.
>
> The patch however won't patch certain files. It contains the excerpt below,
> for example, so it should be patching directory.c but it isn't. The command
> I'm running is just:
>
> % git apply filesys.patch
>
> What am I doing wrong?


It's hard to say what's going wrong when we can't see the exact
errors, git tree and patch in question, but if you just want to remove
a patch, there are easier ways to do it.

If you want to keep the old patch in the history, but reverse it, do:
  git revert commitid
This will record a new commit that reverses the changes in the
indicated one. This is the recommended way to do things if others have
based work off your branch.

If you want to completely forget about the patch in question, you can
do this using git rebase --interactive:
  git rebase --interactive commitid~
  (an editor will open; remove the patch in question from the list,
save and exit your editor)
  (fix any conflicts that come up as prompted)
This will delete it from history, but will result in commit IDs for
commits after that point changing. If anyone has based work off your
branch, then you might have a hard time merging later.

^ permalink raw reply

* Re: [StGit PATCH 1/5] Check for local changes with "goto"
From: Karl Hasselström @ 2009-03-13  1:57 UTC (permalink / raw)
  To: Catalin Marinas; +Cc: git
In-Reply-To: <20090312120856.2992.48548.stgit@pc1117.cambridge.arm.com>

On 2009-03-12 12:08:56 +0000, Catalin Marinas wrote:

> This is done by default, unless the --keep option is passed, for
> consistency with the "pop" command. The index is checked in the
> Transaction.run() function so that other commands could benefit from
> this feature (off by default).
>
> This behaviour can be overridden by setting the stgit.autokeep option.

Two small nits; otherwise,

  Acked-by: Karl Hasselström <kha@treskal.com>

> -    trans = transaction.StackTransaction(stack, 'goto')
> +    clean_iw = not options.keep and iw or None

Add some parentheses here, please! I know that "and" binds tighter
than "or", but I have to think for too long to remember which is
which, and I'll bet I'm not alone ...

> +    def __assert_index_worktree_clean(self, iw):
> +        if not iw.worktree_clean() or \
> +           not iw.index.is_clean(self.stack.head):
> +            self.__halt('Repository not clean. Use "refresh" or '
> +                        '"status --reset"')

"Repository" is misleading here. Maybe something like

   ix_c = iw.index.is_clean(self.stack.head)
   wt_c = iw.worktree_clean()
   if not ix_c or not wt_c:
       self.__halt('%s not clean. Use "refresh" or "status --reset"'
                   % { (False, True): 'Index', (True, False): 'Worktree',
                       (False, False): 'Index and worktree' }[(ix_c, wt_c)])

-- 
Karl Hasselström, kha@treskal.com
      www.treskal.com/kalle

^ permalink raw reply

* Re: [StGit PATCH 2/5] Add mergetool support to the classic StGit infrastructure
From: Karl Hasselström @ 2009-03-13  2:02 UTC (permalink / raw)
  To: Catalin Marinas; +Cc: git
In-Reply-To: <20090312120902.2992.25192.stgit@pc1117.cambridge.arm.com>

On 2009-03-12 12:09:02 +0000, Catalin Marinas wrote:

> Since Git already has a tool for interactively solving conflicts
> which is highly customisable, there is no need to duplicate this
> feature via the i3merge and i2merge configuration options. The
> user-visible change is that now mergetool is invoked rather than the
> previously customised interactive merging tool.

I agree wholeheartedly with the idea. Just one issue:

> +def mergetool(files = ()):
> +    """Invoke 'git mergetool' to resolve any outstanding conflicts. If 'not
> +    files', all the files in an unmerged state will be processed."""
> +    err = os.system('git mergetool %s' % ' '.join(files))
> +    # check for unmerged entries (prepend 'CONFLICT ' for consistency with
> +    # merge_recursive())
> +    conflicts = ['CONFLICT ' + f for f in get_conflicts()]
> +    if conflicts:

Mmm, os.system()? That'll break things as soon as we have a file name
with a space in it. I'm pretty sure there's something in stgit.run
that you could use.

-- 
Karl Hasselström, kha@treskal.com
      www.treskal.com/kalle

^ permalink raw reply

* [JGIT PATCH 0/5] RevWalk fixes for UNINTERESTING
From: Shawn O. Pearce @ 2009-03-13  2:07 UTC (permalink / raw)
  To: Robin Rosenberg; +Cc: git

Today I uncovered some ugly cases with "jgit rev-list B ^A", where
some commits reachable from A were still being output, even though
we asked that they be excluded.

This series attempts to fix it by forcing date ordering, and delaying
output a little to try and work over any clock skew discovered near
the end of the traversal, just before we give up.

Shawn O. Pearce (5):
  Show critical flags in debug toString() descriptions of rev queues
  Make RevObject.getType implementations final
  Remove the horribly stupid RevSort.START_ORDER
  Fix RevWalk with Linus Torvald's occasional bad commit date hack
  Avoid incorrect output of UNINTERESTING commits when clock skew
    occurs

 .../org/spearce/jgit/revwalk/AbstractRevQueue.java |    5 +
 .../src/org/spearce/jgit/revwalk/DateRevQueue.java |   10 +--
 .../org/spearce/jgit/revwalk/DelayRevQueue.java    |   92 ++++++++++++++++++++
 .../src/org/spearce/jgit/revwalk/FIFORevQueue.java |   10 +--
 .../jgit/revwalk/FixUninterestingGenerator.java    |   77 ++++++++++++++++
 .../src/org/spearce/jgit/revwalk/LIFORevQueue.java |   10 +--
 .../org/spearce/jgit/revwalk/PendingGenerator.java |   59 +++++++++++--
 .../src/org/spearce/jgit/revwalk/RevBlob.java      |    2 +-
 .../src/org/spearce/jgit/revwalk/RevCommit.java    |   15 +++-
 .../src/org/spearce/jgit/revwalk/RevObject.java    |   24 +++++
 .../src/org/spearce/jgit/revwalk/RevSort.java      |   11 ---
 .../src/org/spearce/jgit/revwalk/RevTag.java       |    2 +-
 .../src/org/spearce/jgit/revwalk/RevTree.java      |    2 +-
 .../src/org/spearce/jgit/revwalk/RevWalk.java      |    6 +-
 .../org/spearce/jgit/revwalk/StartGenerator.java   |   27 ++++--
 15 files changed, 296 insertions(+), 56 deletions(-)
 create mode 100644 org.spearce.jgit/src/org/spearce/jgit/revwalk/DelayRevQueue.java
 create mode 100644 org.spearce.jgit/src/org/spearce/jgit/revwalk/FixUninterestingGenerator.java

^ permalink raw reply

* [JGIT PATCH 1/5] Show critical flags in debug toString() descriptions of rev queues
From: Shawn O. Pearce @ 2009-03-13  2:07 UTC (permalink / raw)
  To: Robin Rosenberg; +Cc: git
In-Reply-To: <1236910062-18476-1-git-send-email-spearce@spearce.org>

These can help identify the state of each object, especially the
important UNINTERESTING flag being present (or not).

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
 .../org/spearce/jgit/revwalk/AbstractRevQueue.java |    5 ++++
 .../src/org/spearce/jgit/revwalk/DateRevQueue.java |   10 ++------
 .../src/org/spearce/jgit/revwalk/FIFORevQueue.java |   10 ++------
 .../src/org/spearce/jgit/revwalk/LIFORevQueue.java |   10 ++------
 .../src/org/spearce/jgit/revwalk/RevCommit.java    |   12 ++++++++++
 .../src/org/spearce/jgit/revwalk/RevObject.java    |   23 ++++++++++++++++++++
 6 files changed, 49 insertions(+), 21 deletions(-)

diff --git a/org.spearce.jgit/src/org/spearce/jgit/revwalk/AbstractRevQueue.java b/org.spearce.jgit/src/org/spearce/jgit/revwalk/AbstractRevQueue.java
index 4cf7dae..5bb969d 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/revwalk/AbstractRevQueue.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/revwalk/AbstractRevQueue.java
@@ -115,6 +115,11 @@ int outputType() {
 		return outputType;
 	}
 
+	protected static void describe(final StringBuilder s, final RevCommit c) {
+		s.append(c.toString());
+		s.append('\n');
+	}
+
 	private static class AlwaysEmptyQueue extends AbstractRevQueue {
 		@Override
 		public void add(RevCommit c) {
diff --git a/org.spearce.jgit/src/org/spearce/jgit/revwalk/DateRevQueue.java b/org.spearce.jgit/src/org/spearce/jgit/revwalk/DateRevQueue.java
index f797477..210f985 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/revwalk/DateRevQueue.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/revwalk/DateRevQueue.java
@@ -126,13 +126,9 @@ int outputType() {
 	}
 
 	public String toString() {
-		final StringBuffer s = new StringBuffer();
-		for (Entry q = head; q != null; q = q.next) {
-			s.append(q.commit.name());
-			s.append(' ');
-			s.append(q.commit.commitTime);
-			s.append('\n');
-		}
+		final StringBuilder s = new StringBuilder();
+		for (Entry q = head; q != null; q = q.next)
+			describe(s, q.commit);
 		return s.toString();
 	}
 
diff --git a/org.spearce.jgit/src/org/spearce/jgit/revwalk/FIFORevQueue.java b/org.spearce.jgit/src/org/spearce/jgit/revwalk/FIFORevQueue.java
index 2c4c003..f086928 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/revwalk/FIFORevQueue.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/revwalk/FIFORevQueue.java
@@ -149,14 +149,10 @@ void removeFlag(final int f) {
 	}
 
 	public String toString() {
-		final StringBuffer s = new StringBuffer();
+		final StringBuilder s = new StringBuilder();
 		for (Block q = head; q != null; q = q.next) {
-			for (int i = q.headIndex; i < q.tailIndex; i++) {
-				s.append(q.commits[i].name());
-				s.append(' ');
-				s.append(q.commits[i].commitTime);
-				s.append('\n');
-			}
+			for (int i = q.headIndex; i < q.tailIndex; i++)
+				describe(s, q.commits[i]);
 		}
 		return s.toString();
 	}
diff --git a/org.spearce.jgit/src/org/spearce/jgit/revwalk/LIFORevQueue.java b/org.spearce.jgit/src/org/spearce/jgit/revwalk/LIFORevQueue.java
index 5e885c0..045f7f1 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/revwalk/LIFORevQueue.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/revwalk/LIFORevQueue.java
@@ -104,14 +104,10 @@ boolean anybodyHasFlag(final int f) {
 	}
 
 	public String toString() {
-		final StringBuffer s = new StringBuffer();
+		final StringBuilder s = new StringBuilder();
 		for (Block q = head; q != null; q = q.next) {
-			for (int i = q.headIndex; i < q.tailIndex; i++) {
-				s.append(q.commits[i].name());
-				s.append(' ');
-				s.append(q.commits[i].commitTime);
-				s.append('\n');
-			}
+			for (int i = q.headIndex; i < q.tailIndex; i++)
+				describe(s, q.commits[i]);
 		}
 		return s.toString();
 	}
diff --git a/org.spearce.jgit/src/org/spearce/jgit/revwalk/RevCommit.java b/org.spearce.jgit/src/org/spearce/jgit/revwalk/RevCommit.java
index de11c39..1b25fce 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/revwalk/RevCommit.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/revwalk/RevCommit.java
@@ -388,4 +388,16 @@ public void dispose() {
 		flags &= ~PARSED;
 		buffer = null;
 	}
+
+	@Override
+	public String toString() {
+		final StringBuilder s = new StringBuilder();
+		s.append(Constants.typeString(getType()));
+		s.append(name());
+		s.append(' ');
+		s.append(commitTime);
+		s.append(' ');
+		appendCoreFlags(s);
+		return s.toString();
+	}
 }
diff --git a/org.spearce.jgit/src/org/spearce/jgit/revwalk/RevObject.java b/org.spearce.jgit/src/org/spearce/jgit/revwalk/RevObject.java
index 451205c..7dadb7b 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/revwalk/RevObject.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/revwalk/RevObject.java
@@ -167,4 +167,27 @@ public void remove(final RevFlagSet set) {
 	public void dispose() {
 		// Nothing needs to be done for most objects.
 	}
+
+	@Override
+	public String toString() {
+		final StringBuilder s = new StringBuilder();
+		s.append(Constants.typeString(getType()));
+		s.append(name());
+		s.append(' ');
+		appendCoreFlags(s);
+		return s.toString();
+	}
+
+	/**
+	 * @param s
+	 *            buffer to append a debug description of core RevFlags onto.
+	 */
+	protected void appendCoreFlags(final StringBuilder s) {
+		s.append((flags & RevWalk.TOPO_DELAY) != 0 ? 'o' : '-');
+		s.append((flags & RevWalk.TEMP_MARK) != 0 ? 't' : '-');
+		s.append((flags & RevWalk.REWRITE) != 0 ? 'r' : '-');
+		s.append((flags & RevWalk.UNINTERESTING) != 0 ? 'u' : '-');
+		s.append((flags & RevWalk.SEEN) != 0 ? 's' : '-');
+		s.append((flags & RevWalk.PARSED) != 0 ? 'p' : '-');
+	}
 }
-- 
1.6.2.288.gc3f22

^ permalink raw reply related

* [JGIT PATCH 2/5] Make RevObject.getType implementations final
From: Shawn O. Pearce @ 2009-03-13  2:07 UTC (permalink / raw)
  To: Robin Rosenberg; +Cc: git
In-Reply-To: <1236910062-18476-2-git-send-email-spearce@spearce.org>

These methods should never be overridden once defined by the base
class of RevCommit, RevTree, RevBlob or RevTag.  An override is
only going to provide confusion to calls who rely upon the return
value to know if a downcast is safe.

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
 .../src/org/spearce/jgit/revwalk/RevBlob.java      |    2 +-
 .../src/org/spearce/jgit/revwalk/RevCommit.java    |    3 ++-
 .../src/org/spearce/jgit/revwalk/RevObject.java    |    1 +
 .../src/org/spearce/jgit/revwalk/RevTag.java       |    2 +-
 .../src/org/spearce/jgit/revwalk/RevTree.java      |    2 +-
 5 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/org.spearce.jgit/src/org/spearce/jgit/revwalk/RevBlob.java b/org.spearce.jgit/src/org/spearce/jgit/revwalk/RevBlob.java
index 66cdc02..cf241cf 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/revwalk/RevBlob.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/revwalk/RevBlob.java
@@ -58,7 +58,7 @@ void parse(final RevWalk walk) {
 	}
 	
 	@Override
-	public int getType() {
+	public final int getType() {
 		return Constants.OBJ_BLOB;
 	}
 }
diff --git a/org.spearce.jgit/src/org/spearce/jgit/revwalk/RevCommit.java b/org.spearce.jgit/src/org/spearce/jgit/revwalk/RevCommit.java
index 1b25fce..2a59ec4 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/revwalk/RevCommit.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/revwalk/RevCommit.java
@@ -139,7 +139,7 @@ else if (nParents == 1) {
 	}
 	
 	@Override
-	public int getType() {
+	public final int getType() {
 		return Constants.OBJ_COMMIT;
 	}
 
@@ -393,6 +393,7 @@ public void dispose() {
 	public String toString() {
 		final StringBuilder s = new StringBuilder();
 		s.append(Constants.typeString(getType()));
+		s.append(' ');
 		s.append(name());
 		s.append(' ');
 		s.append(commitTime);
diff --git a/org.spearce.jgit/src/org/spearce/jgit/revwalk/RevObject.java b/org.spearce.jgit/src/org/spearce/jgit/revwalk/RevObject.java
index 7dadb7b..8c7cc23 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/revwalk/RevObject.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/revwalk/RevObject.java
@@ -172,6 +172,7 @@ public void dispose() {
 	public String toString() {
 		final StringBuilder s = new StringBuilder();
 		s.append(Constants.typeString(getType()));
+		s.append(' ');
 		s.append(name());
 		s.append(' ');
 		appendCoreFlags(s);
diff --git a/org.spearce.jgit/src/org/spearce/jgit/revwalk/RevTag.java b/org.spearce.jgit/src/org/spearce/jgit/revwalk/RevTag.java
index aba8744..cace82d 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/revwalk/RevTag.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/revwalk/RevTag.java
@@ -99,7 +99,7 @@ void parseCanonical(final RevWalk walk, final byte[] rawTag)
 	}
 
 	@Override
-	public int getType() {
+	public final int getType() {
 		return Constants.OBJ_TAG;
 	}
 
diff --git a/org.spearce.jgit/src/org/spearce/jgit/revwalk/RevTree.java b/org.spearce.jgit/src/org/spearce/jgit/revwalk/RevTree.java
index e1cd4b5..4d767e4 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/revwalk/RevTree.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/revwalk/RevTree.java
@@ -58,7 +58,7 @@ void parse(final RevWalk walk) {
 	}
 	
 	@Override
-	public int getType() {
+	public final int getType() {
 		return Constants.OBJ_TREE;
 	}
 }
-- 
1.6.2.288.gc3f22

^ permalink raw reply related

* [JGIT PATCH 3/5] Remove the horribly stupid RevSort.START_ORDER
From: Shawn O. Pearce @ 2009-03-13  2:07 UTC (permalink / raw)
  To: Robin Rosenberg; +Cc: git
In-Reply-To: <1236910062-18476-3-git-send-email-spearce@spearce.org>

I must have been on crack the day I wrote 3b27268f49 ("Allow RevWalk
users to ask for FIFO style ordering").  Its a really bad idea.

Applications can get themselves into a situation where they process
one branch long before another, and then abort too early, before all
commits have been correctly flagged UNINTERESTING.

For example, given the graph:

  Z-A------------B
  |             /
  |    ---------
  \   /
   Q-R-S-T-U-V

If B is "interesting" and A,V are UNINTERESTING, without forcing the
commit timestamp ordering in the pending queue we wind up processing
all of B-Z, producing R,Q as "interesting" output in the process, and
terminate before S can even be parsed. Consequently we never push the
UNINTERESTING flag down onto R, and R,Q produced when they shouldn't.

We now require that the pending queue use a DateRevQueue instead of
the FIFORevQueue, so that in the above graph S must be parsed before
we can even consider R or A, even though R,A were reached earlier.
This of course still assumes there is no clock skew between S and R.
The next commit will address some limited clock skew issues.

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
 .../org/spearce/jgit/revwalk/PendingGenerator.java |    8 +++-----
 .../src/org/spearce/jgit/revwalk/RevSort.java      |   11 -----------
 .../src/org/spearce/jgit/revwalk/RevWalk.java      |    6 +++---
 .../org/spearce/jgit/revwalk/StartGenerator.java   |    8 ++------
 4 files changed, 8 insertions(+), 25 deletions(-)

diff --git a/org.spearce.jgit/src/org/spearce/jgit/revwalk/PendingGenerator.java b/org.spearce.jgit/src/org/spearce/jgit/revwalk/PendingGenerator.java
index 25b2966..144e909 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/revwalk/PendingGenerator.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/revwalk/PendingGenerator.java
@@ -62,7 +62,7 @@
 
 	private final RevWalk walker;
 
-	private final AbstractRevQueue pending;
+	private final DateRevQueue pending;
 
 	private final RevFilter filter;
 
@@ -70,7 +70,7 @@
 
 	boolean canDispose;
 
-	PendingGenerator(final RevWalk w, final AbstractRevQueue p,
+	PendingGenerator(final RevWalk w, final DateRevQueue p,
 			final RevFilter f, final int out) {
 		walker = w;
 		pending = p;
@@ -81,9 +81,7 @@ PendingGenerator(final RevWalk w, final AbstractRevQueue p,
 
 	@Override
 	int outputType() {
-		if (pending instanceof DateRevQueue)
-			return output | SORT_COMMIT_TIME_DESC;
-		return output;
+		return output | SORT_COMMIT_TIME_DESC;
 	}
 
 	@Override
diff --git a/org.spearce.jgit/src/org/spearce/jgit/revwalk/RevSort.java b/org.spearce.jgit/src/org/spearce/jgit/revwalk/RevSort.java
index b0a03ad..8c3eaed 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/revwalk/RevSort.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/revwalk/RevSort.java
@@ -49,17 +49,6 @@
 	NONE,
 
 	/**
-	 * Maintain the order commits were marked as starting points.
-	 * <p>
-	 * This strategy is largely a FIFO strategy; commits are enumerated in the
-	 * order they were added to the RevWalk by the application. Parents not yet
-	 * visited are added behind all commits already enqueued for visiting.
-	 * <p>
-	 * This strategy should not be combined with {@link #COMMIT_TIME_DESC}.
-	 */
-	START_ORDER,
-
-	/**
 	 * Sort by commit time, descending (newest first, oldest last).
 	 * <p>
 	 * This strategy can be combined with {@link #TOPO}.
diff --git a/org.spearce.jgit/src/org/spearce/jgit/revwalk/RevWalk.java b/org.spearce.jgit/src/org/spearce/jgit/revwalk/RevWalk.java
index c8ec458..316f722 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/revwalk/RevWalk.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/revwalk/RevWalk.java
@@ -187,7 +187,7 @@ public RevWalk(final Repository repo) {
 		idBuffer = new MutableObjectId();
 		objects = new ObjectIdSubclassMap<RevObject>();
 		roots = new ArrayList<RevCommit>();
-		queue = new FIFORevQueue();
+		queue = new DateRevQueue();
 		pending = new StartGenerator(this);
 		sorting = EnumSet.of(RevSort.NONE);
 		filter = RevFilter.ALL;
@@ -915,7 +915,7 @@ protected void reset(int retainFlags) {
 
 		curs.release();
 		roots.clear();
-		queue = new FIFORevQueue();
+		queue = new DateRevQueue();
 		pending = new StartGenerator(this);
 	}
 
@@ -934,7 +934,7 @@ public void dispose() {
 		objects.clear();
 		curs.release();
 		roots.clear();
-		queue = new FIFORevQueue();
+		queue = new DateRevQueue();
 		pending = new StartGenerator(this);
 	}
 
diff --git a/org.spearce.jgit/src/org/spearce/jgit/revwalk/StartGenerator.java b/org.spearce.jgit/src/org/spearce/jgit/revwalk/StartGenerator.java
index 1b7947f..bf7067a 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/revwalk/StartGenerator.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/revwalk/StartGenerator.java
@@ -108,11 +108,7 @@ RevCommit next() throws MissingObjectException,
 		}
 
 		int pendingOutputType = 0;
-		if (walker.hasRevSort(RevSort.START_ORDER)
-				&& !(q instanceof FIFORevQueue))
-			q = new FIFORevQueue(q);
-		if (walker.hasRevSort(RevSort.COMMIT_TIME_DESC)
-				&& !(q instanceof DateRevQueue))
+		if (!(q instanceof DateRevQueue))
 			q = new DateRevQueue(q);
 		if (tf != TreeFilter.ALL) {
 			rf = AndRevFilter.create(rf, new RewriteTreeFilter(w, tf));
@@ -120,7 +116,7 @@ RevCommit next() throws MissingObjectException,
 		}
 
 		walker.queue = q;
-		g = new PendingGenerator(w, q, rf, pendingOutputType);
+		g = new PendingGenerator(w, (DateRevQueue) q, rf, pendingOutputType);
 
 		if (boundary) {
 			// Because the boundary generator may produce uninteresting
-- 
1.6.2.288.gc3f22

^ permalink raw reply related

* [JGIT PATCH 4/5] Fix RevWalk with Linus Torvald's occasional bad commit date hack
From: Shawn O. Pearce @ 2009-03-13  2:07 UTC (permalink / raw)
  To: Robin Rosenberg; +Cc: git
In-Reply-To: <1236910062-18476-4-git-send-email-spearce@spearce.org>

In git-core commit 7d004199d134c9d465e013064f72dbc04507f6c0 Linus
describes a hack he created to improve the handling of cases where
commit dates are out of order, such as a child commit having a date
older than its parent.  These cases can show up when there is bad
imported data, or significant clock skew between distributed peers.

The original git-core bug report identified a failure in:

  git rev-list A..B C

where commits contained in both A and B were reported, due to out
of order commit dates.

We keep running until the most recent commit in the pending queue
is more recent than the last commit sent to the caller.  If the
pending queue suddenly goes "backwards" due to one of our parent's
having a more recent commit date, this new check ensures we will
keep processing the graph until we see a more consistent cut.

We process an extra OVER_SCAN commits after we decide that all
remaining commits are UNINTERESTING.  Processing these extra
commits ensures flags are carried back further in the graph,
increasing the chances that we correctly mark relevant nodes.

As a result of this hack, we may produce a commit to our caller,
but then later mark it UNINTERESTING if we discover date skew.
To handle such cases, callers could buffer produced commits and
filter out those that are UNINTERESTING, but this somewhat costly
and may not always be necessary.

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
 .../org/spearce/jgit/revwalk/PendingGenerator.java |   51 ++++++++++++++++++--
 1 files changed, 47 insertions(+), 4 deletions(-)

diff --git a/org.spearce.jgit/src/org/spearce/jgit/revwalk/PendingGenerator.java b/org.spearce.jgit/src/org/spearce/jgit/revwalk/PendingGenerator.java
index 144e909..cd24e8f 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/revwalk/PendingGenerator.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/revwalk/PendingGenerator.java
@@ -42,6 +42,7 @@
 import org.spearce.jgit.errors.IncorrectObjectTypeException;
 import org.spearce.jgit.errors.MissingObjectException;
 import org.spearce.jgit.errors.StopWalkException;
+import org.spearce.jgit.lib.ObjectId;
 import org.spearce.jgit.revwalk.filter.RevFilter;
 
 /**
@@ -60,6 +61,24 @@
 
 	private static final int UNINTERESTING = RevWalk.UNINTERESTING;
 
+	/**
+	 * Number of additional commits to scan after we think we are done.
+	 * <p>
+	 * This small buffer of commits is scanned to ensure we didn't miss anything
+	 * as a result of clock skew when the commits were made. We need to set our
+	 * constant to 1 additional commit due to the use of a pre-increment
+	 * operator when accessing the value.
+	 */
+	private static final int OVER_SCAN = 5 + 1;
+
+	/** A commit near the end of time, to initialize {@link #last} with. */
+	private static final RevCommit INIT_LAST;
+
+	static {
+		INIT_LAST = new RevCommit(ObjectId.zeroId());
+		INIT_LAST.commitTime = Integer.MAX_VALUE;
+	}
+
 	private final RevWalk walker;
 
 	private final DateRevQueue pending;
@@ -68,6 +87,17 @@
 
 	private final int output;
 
+	/** Last commit produced to the caller from {@link #next()}. */
+	private RevCommit last = INIT_LAST;
+
+	/**
+	 * Number of commits we have remaining in our over-scan allotment.
+	 * <p>
+	 * Only relevant if there are {@link #UNINTERESTING} commits in the
+	 * {@link #pending} queue.
+	 */
+	private int overScan = OVER_SCAN;
+
 	boolean canDispose;
 
 	PendingGenerator(final RevWalk w, final DateRevQueue p,
@@ -112,14 +142,27 @@ RevCommit next() throws MissingObjectException,
 				walker.carryFlagsImpl(c);
 
 				if ((c.flags & UNINTERESTING) != 0) {
-					if (pending.everbodyHasFlag(UNINTERESTING))
-						throw StopWalkException.INSTANCE;
-					c.dispose();
+					if (pending.everbodyHasFlag(UNINTERESTING)) {
+						final RevCommit n = pending.peek();
+						if (n != null && n.commitTime <= last.commitTime) {
+							// This is too close to call. The next commit we
+							// would pop is before the last one we produced.
+							// We have to keep going to ensure that we carry
+							// flags as much as necessary.
+							//
+							overScan = OVER_SCAN;
+						} else if (--overScan == 0)
+							throw StopWalkException.INSTANCE;
+					} else {
+						overScan = OVER_SCAN;
+					}
+					if (canDispose)
+						c.dispose();
 					continue;
 				}
 
 				if (produce)
-					return c;
+					return last = c;
 				else if (canDispose)
 					c.dispose();
 			}
-- 
1.6.2.288.gc3f22

^ permalink raw reply related

* [JGIT PATCH 5/5] Avoid incorrect output of UNINTERESTING commits when clock skew occurs
From: Shawn O. Pearce @ 2009-03-13  2:07 UTC (permalink / raw)
  To: Robin Rosenberg; +Cc: git
In-Reply-To: <1236910062-18476-5-git-send-email-spearce@spearce.org>

The prior commit added functionality to scan a few extra commits when
we otherwise would have aborted due to everything left being colored
UNINTERESTING.  When that happens we may wind up coloring a commit
that we already produced to the caller, giving the caller an incorrect
result set.

If we insert a fully buffered generator in the pipeline, such as that
used for RevSort.TOPO or RevSort.REVERSE, we can easily filter these
late-colored commits out before we show them to the application.  But
otherwise we delay the output by 6 commits, just long enough to give
PendingGenerator a reasonable chance at getting the coloring right.

This is only an approximation.  It is still possible for commits to
produce when they should be UNINTERESTING, such as if more than the
OVER_SCAN limit had clock skew between two branches and the common
merge base, even if we are fully buffering our output.

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
 .../org/spearce/jgit/revwalk/DelayRevQueue.java    |   92 ++++++++++++++++++++
 .../jgit/revwalk/FixUninterestingGenerator.java    |   77 ++++++++++++++++
 .../org/spearce/jgit/revwalk/PendingGenerator.java |    2 +-
 .../org/spearce/jgit/revwalk/StartGenerator.java   |   23 ++++-
 4 files changed, 189 insertions(+), 5 deletions(-)
 create mode 100644 org.spearce.jgit/src/org/spearce/jgit/revwalk/DelayRevQueue.java
 create mode 100644 org.spearce.jgit/src/org/spearce/jgit/revwalk/FixUninterestingGenerator.java

diff --git a/org.spearce.jgit/src/org/spearce/jgit/revwalk/DelayRevQueue.java b/org.spearce.jgit/src/org/spearce/jgit/revwalk/DelayRevQueue.java
new file mode 100644
index 0000000..1eb7064
--- /dev/null
+++ b/org.spearce.jgit/src/org/spearce/jgit/revwalk/DelayRevQueue.java
@@ -0,0 +1,92 @@
+/*
+ * Copyright (C) 2009, Google Inc.
+ *
+ * All rights reserved.
+ *
+ * Redistribution and use in source and binary forms, with or
+ * without modification, are permitted provided that the following
+ * conditions are met:
+ *
+ * - Redistributions of source code must retain the above copyright
+ *   notice, this list of conditions and the following disclaimer.
+ *
+ * - Redistributions in binary form must reproduce the above
+ *   copyright notice, this list of conditions and the following
+ *   disclaimer in the documentation and/or other materials provided
+ *   with the distribution.
+ *
+ * - Neither the name of the Git Development Community nor the
+ *   names of its contributors may be used to endorse or promote
+ *   products derived from this software without specific prior
+ *   written permission.
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND
+ * CONTRIBUTORS "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES,
+ * INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES
+ * OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
+ * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR
+ * CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+ * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT
+ * NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES;
+ * LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER
+ * CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT,
+ * STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
+ * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF
+ * ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ */
+
+package org.spearce.jgit.revwalk;
+
+import java.io.IOException;
+
+import org.spearce.jgit.errors.IncorrectObjectTypeException;
+import org.spearce.jgit.errors.MissingObjectException;
+
+/**
+ * Delays commits to be at least {@link PendingGenerator#OVER_SCAN} late.
+ * <p>
+ * This helps to "fix up" weird corner cases resulting from clock skew, by
+ * slowing down what we produce to the caller we get a better chance to ensure
+ * PendingGenerator reached back far enough in the graph to correctly mark
+ * commits {@link RevWalk#UNINTERESTING} if necessary.
+ * <p>
+ * This generator should appear before {@link FixUninterestingGenerator} if the
+ * lower level {@link #pending} isn't already fully buffered.
+ */
+final class DelayRevQueue extends Generator {
+	private static final int OVER_SCAN = PendingGenerator.OVER_SCAN;
+
+	private final Generator pending;
+
+	private final FIFORevQueue delay;
+
+	private int size;
+
+	DelayRevQueue(final Generator g) {
+		pending = g;
+		delay = new FIFORevQueue();
+	}
+
+	@Override
+	int outputType() {
+		return pending.outputType();
+	}
+
+	@Override
+	RevCommit next() throws MissingObjectException,
+			IncorrectObjectTypeException, IOException {
+		while (size < OVER_SCAN) {
+			final RevCommit c = pending.next();
+			if (c == null)
+				break;
+			delay.add(c);
+			size++;
+		}
+
+		final RevCommit c = delay.next();
+		if (c == null)
+			return null;
+		size--;
+		return c;
+	}
+}
diff --git a/org.spearce.jgit/src/org/spearce/jgit/revwalk/FixUninterestingGenerator.java b/org.spearce.jgit/src/org/spearce/jgit/revwalk/FixUninterestingGenerator.java
new file mode 100644
index 0000000..6a9ba61
--- /dev/null
+++ b/org.spearce.jgit/src/org/spearce/jgit/revwalk/FixUninterestingGenerator.java
@@ -0,0 +1,77 @@
+/*
+ * Copyright (C) 2009, Google Inc.
+ *
+ * All rights reserved.
+ *
+ * Redistribution and use in source and binary forms, with or
+ * without modification, are permitted provided that the following
+ * conditions are met:
+ *
+ * - Redistributions of source code must retain the above copyright
+ *   notice, this list of conditions and the following disclaimer.
+ *
+ * - Redistributions in binary form must reproduce the above
+ *   copyright notice, this list of conditions and the following
+ *   disclaimer in the documentation and/or other materials provided
+ *   with the distribution.
+ *
+ * - Neither the name of the Git Development Community nor the
+ *   names of its contributors may be used to endorse or promote
+ *   products derived from this software without specific prior
+ *   written permission.
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND
+ * CONTRIBUTORS "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES,
+ * INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES
+ * OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
+ * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR
+ * CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+ * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT
+ * NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES;
+ * LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER
+ * CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT,
+ * STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
+ * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF
+ * ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ */
+
+package org.spearce.jgit.revwalk;
+
+import java.io.IOException;
+
+import org.spearce.jgit.errors.IncorrectObjectTypeException;
+import org.spearce.jgit.errors.MissingObjectException;
+
+/**
+ * Filters out commits marked {@link RevWalk#UNINTERESTING}.
+ * <p>
+ * This generator is only in front of another generator that has fully buffered
+ * commits, such that we are called only after the {@link PendingGenerator} has
+ * exhausted its input queue and given up. It skips over any uninteresting
+ * commits that may have leaked out of the PendingGenerator due to clock skew
+ * being detected in the commit objects.
+ */
+final class FixUninterestingGenerator extends Generator {
+	private final Generator pending;
+
+	FixUninterestingGenerator(final Generator g) {
+		pending = g;
+	}
+
+	@Override
+	int outputType() {
+		return pending.outputType();
+	}
+
+	@Override
+	RevCommit next() throws MissingObjectException,
+			IncorrectObjectTypeException, IOException {
+		for (;;) {
+			final RevCommit c = pending.next();
+			if (c == null)
+				return null;
+			if ((c.flags & RevWalk.UNINTERESTING) == 0)
+				return c;
+		}
+	}
+}
diff --git a/org.spearce.jgit/src/org/spearce/jgit/revwalk/PendingGenerator.java b/org.spearce.jgit/src/org/spearce/jgit/revwalk/PendingGenerator.java
index cd24e8f..ebbb39f 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/revwalk/PendingGenerator.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/revwalk/PendingGenerator.java
@@ -69,7 +69,7 @@
 	 * constant to 1 additional commit due to the use of a pre-increment
 	 * operator when accessing the value.
 	 */
-	private static final int OVER_SCAN = 5 + 1;
+	static final int OVER_SCAN = 5 + 1;
 
 	/** A commit near the end of time, to initialize {@link #last} with. */
 	private static final RevCommit INIT_LAST;
diff --git a/org.spearce.jgit/src/org/spearce/jgit/revwalk/StartGenerator.java b/org.spearce.jgit/src/org/spearce/jgit/revwalk/StartGenerator.java
index bf7067a..3535250 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/revwalk/StartGenerator.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/revwalk/StartGenerator.java
@@ -90,6 +90,7 @@ RevCommit next() throws MissingObjectException,
 			return mbg.next();
 		}
 
+		final boolean uninteresting = q.anybodyHasFlag(RevWalk.UNINTERESTING);
 		boolean boundary = walker.hasRevSort(RevSort.BOUNDARY);
 
 		if (!boundary && walker instanceof ObjectWalk) {
@@ -99,7 +100,7 @@ RevCommit next() throws MissingObjectException,
 			//
 			boundary = true;
 		}
-		if (boundary && !q.anybodyHasFlag(RevWalk.UNINTERESTING)) {
+		if (boundary && !uninteresting) {
 			// If we were not fed uninteresting commits we will never
 			// construct a boundary. There is no reason to include the
 			// extra overhead associated with that in our pipeline.
@@ -107,16 +108,19 @@ RevCommit next() throws MissingObjectException,
 			boundary = false;
 		}
 
+		final DateRevQueue pending;
 		int pendingOutputType = 0;
-		if (!(q instanceof DateRevQueue))
-			q = new DateRevQueue(q);
+		if (q instanceof DateRevQueue)
+			pending = (DateRevQueue)q;
+		else
+			pending = new DateRevQueue(q);
 		if (tf != TreeFilter.ALL) {
 			rf = AndRevFilter.create(rf, new RewriteTreeFilter(w, tf));
 			pendingOutputType |= HAS_REWRITE | NEEDS_REWRITE;
 		}
 
 		walker.queue = q;
-		g = new PendingGenerator(w, (DateRevQueue) q, rf, pendingOutputType);
+		g = new PendingGenerator(w, pending, rf, pendingOutputType);
 
 		if (boundary) {
 			// Because the boundary generator may produce uninteresting
@@ -143,6 +147,17 @@ RevCommit next() throws MissingObjectException,
 			g = new LIFORevQueue(g);
 		if (boundary)
 			g = new BoundaryGenerator(w, g);
+		else if (uninteresting) {
+			// Try to protect ourselves from uninteresting commits producing
+			// due to clock skew in the commit time stamps. Delay such that
+			// we have a chance at coloring enough of the graph correctly,
+			// and then strip any UNINTERESTING nodes that may have leaked
+			// through early.
+			//
+			if (pending.peek() != null)
+				g = new DelayRevQueue(g);
+			g = new FixUninterestingGenerator(g);
+		}
 
 		w.pending = g;
 		return g.next();
-- 
1.6.2.288.gc3f22

^ permalink raw reply related

* Re: [StGit PATCH 3/5] Add automatic git-mergetool invocation to the new infrastructure
From: Karl Hasselström @ 2009-03-13  2:17 UTC (permalink / raw)
  To: Catalin Marinas; +Cc: git
In-Reply-To: <20090312120907.2992.81035.stgit@pc1117.cambridge.arm.com>

On 2009-03-12 12:09:07 +0000, Catalin Marinas wrote:

> This patch adds the IndexAndWorktree.mergetool() function responsible
> for calling 'git mergetool' to interactively solve conflicts. The
> function may also be called from IndexAndWorktree.merge() if the
> standard 'git merge-recursive' fails and 'interactive == True'. The
> 'allow_interactive' parameter is passed to Transaction.push_patch() from
> the functions allowing interactive merging.

Nicely done with the "interactive" and "allow_interactive" arguments;
the policy and the implementation end up at the right levels.

>                  # There were conflicts
> -                conflicts = [l for l in output if l.startswith('CONFLICT')]
> -                raise MergeConflictException(conflicts)
> +                if interactive:
> +                    self.mergetool()
> +                else:
> +                    conflicts = [l for l in output if l.startswith('CONFLICT')]
> +                    raise MergeConflictException(conflicts)

Does the merge tool always resolve all conflicts? If it doesn't, the
two lines in the "else" branch should probably be run unconditionally.

>          except run.RunException, e:
>              raise MergeException('Index/worktree dirty')
> +    def mergetool(self, files = ()):
> +        """Invoke 'git mergetool' on the current IndexAndWorktree to resolve
> +        any outstanding conflicts. If 'not files', all the files in an
> +        unmerged state will be processed."""
> +        err = os.system('git mergetool %s' % ' '.join(files))

Look at how the surrounding code calls git. os.system() will do nasty
things with filenames that require quoting, such as for example
"; rm -rf ~/".

> +        # check for unmerged entries (prepend 'CONFLICT ' for consistency with
> +        # merge())
> +        conflicts = ['CONFLICT ' + f for f in self.index.conflicts()]
> +        if conflicts:
> +            raise MergeConflictException(conflicts)
> +        elif err:
> +            raise MergeException('"git mergetool" failed, exit code: %d' % err)

Ah, you take care of conflicts here too. Hmm. I guess that's fine too,
though there is some code duplication. Maybe a helper function that
takes output as a parameter, and raises MergeConflictException if
necessary?

> +                interactive = allow_interactive and \
> +                        config.get('stgit.autoimerge') == 'yes'

Small style nit: backslash line continuations are ugly. :-)

If you put parentheses around the expression, you can break the line
without a backslash.

-- 
Karl Hasselström, kha@treskal.com
      www.treskal.com/kalle

^ permalink raw reply

* Re: [StGit PATCH 4/5] Convert "sink" to the new infrastructure
From: Karl Hasselström @ 2009-03-13  2:27 UTC (permalink / raw)
  To: Catalin Marinas; +Cc: git
In-Reply-To: <20090312120913.2992.21403.stgit@pc1117.cambridge.arm.com>

Nicely done.

  Acked-by: Karl Hasselström <kha@treskal.com>

On 2009-03-12 12:09:13 +0000, Catalin Marinas wrote:

> +    applied = applied[:insert_idx] + patches + applied[insert_idx:]
> +
> +    unapplied = [p for p in stack.patchorder.unapplied if p not in patches]
> +    hidden = list(stack.patchorder.hidden)
> +
> +    iw = stack.repository.default_iw
> +    clean_iw = not options.keep and iw or None
> +    trans = transaction.StackTransaction(stack, 'sink',
> +                                         check_clean_iw = clean_iw)
> +
> +    try:
> +        trans.reorder_patches(applied, unapplied, hidden, iw)

Hmm. We should maybe have a default value for hidden: the current list
of patches. Not changing the hidden patches is a common operation.

-- 
Karl Hasselström, kha@treskal.com
      www.treskal.com/kalle

^ permalink raw reply

* Re: [StGit PATCH 5/5] Convert "float" to the lib infrastructure
From: Karl Hasselström @ 2009-03-13  2:41 UTC (permalink / raw)
  To: Catalin Marinas; +Cc: git
In-Reply-To: <20090312120918.2992.82713.stgit@pc1117.cambridge.arm.com>

Nice job here too.

On 2009-03-12 12:09:18 +0000, Catalin Marinas wrote:

>  options = [
>      opt('-s', '--series', action = 'store_true',
> -        short = 'Rearrange according to a series file')]
> +        short = 'Rearrange according to a series file')
> +    ] + argparse.keep_option()

This flag should take the filename as a parameter, both because it's
the right thing to do and because it'll make the tab completion work
right (as is, it'll complete on patch names after the -s flag).

Something like

  opt('-s', '--series', type = 'string')

ought to do it.

> +    applied = [p for p in stack.patchorder.applied if p not in patches] + \
> +            patches
> +    unapplied = [p for p in stack.patchorder.unapplied if p not in patches]

It may be just me, but I find "not p in patches" more readable than "p
not in patches". Oh, and the backslash.

Feel free to ignore, of course. :-)

> +    hidden = list(stack.patchorder.hidden)
> +
> +    iw = stack.repository.default_iw
> +    clean_iw = not options.keep and iw or None
> +    trans = transaction.StackTransaction(stack, 'sink',
> +                                         check_clean_iw = clean_iw)
> +
> +    try:
> +        trans.reorder_patches(applied, unapplied, hidden, iw)

That default value for hidden would've come in handy here too!

-- 
Karl Hasselström, kha@treskal.com
      www.treskal.com/kalle

^ permalink raw reply

* Re: Google Summer of Code 2009: GIT
From: saurabh gupta @ 2009-03-13  3:15 UTC (permalink / raw)
  To: david; +Cc: Junio C Hamano, Johannes Schindelin, git
In-Reply-To: <alpine.DEB.1.10.0903121343590.16753@asgard.lang.hm>

On Fri, Mar 13, 2009 at 2:15 AM,  <david@lang.hm> wrote:
>
> when I am mentioning config files here I'm thinking of ones that don't use
> XML (such as the git config file)
>
> a 'paragraph' merge driver could also help with things like a maintainers
> file where the order of the paragaphs doesn't matter, just the content
> inside each one.
>
> that's very similar to re-ordering XML tags, but with a slightly different
> syntax

yes, in that case, I think we can modify the existing text merge
driver somewhat and provide these configuration options to the user.
User can choose the option to configure the merge operation.


-- 
Saurabh Gupta
Senior,
NSIT,New Delhi, India

^ permalink raw reply

* Re: [PATCH 1/7] strbuf: add "include_delim" parameter to "strbuf_split"
From: Christian Couder @ 2009-03-13  4:48 UTC (permalink / raw)
  To: Nanako Shiraishi
  Cc: Junio C Hamano, git, Ingo Molnar, John Tapsell,
	Johannes Schindelin, Pierre Habouzit
In-Reply-To: <20090312190846.6117@nanako3.lavabit.com>

Le jeudi 12 mars 2009, Nanako Shiraishi a écrit :
> Quoting Christian Couder <chriscool@tuxfamily.org>:
> > The "strbuf_split" function used to include the delimiter character
> > at the end of the splited strbufs it produced.
> >
> > This behavior is not wanted in many cases, so this patch adds a new
> > "include_delim" parameter to the function to let us switch it on or
> > off as we want.
>
> Sorry, but I don't understand the above claim. You say "not wanted in
> many cases" but your patch updates the existing callers, all of which do
> want to include the delimiter.

In many programming languages, like Perl and Python for example, there is 
a "split" function that splits strings, and by default the resulting 
strings don't include the delimiter.

In Git there are only 2 existing callers and I think this function could be 
used a lot more if there was a way not to include the delimiter.

In my patch series I add one caller that don't want the delimiter so after 
my patch series there are already half as many callers that don't want the 
delimiter.

And by the way, when I mentored the GSoC sequencer project I suggested to 
Stephan to use strbuf_split, but we also had the problem that the delimiter 
was included.

> The patch would easily justify itself if it made the callers pass 0 to
> the function to decline the delimiter, and as the result it made the
> codepaths that use the result simpler. But I don't think that is what
> your patch does.

Yes, my patch does not do that, because I think including the delimiter is a 
special case of the more general and useful behavior of not including it.

Best regards,
Christian.

^ permalink raw reply


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