Re: [PATCH v2] t/Makefile: add a rule to re-run previously-failed tests

2016-09-01 Thread Sverre Rabbelier
On Thu, Sep 1, 2016 at 1:27 AM, Johannes Schindelin
 wrote:
> On Wed, 31 Aug 2016, Sverre Rabbelier wrote:
>> On Wed, Aug 31, 2016 at 3:36 AM Johannes Schindelin
>>  wrote:
>> > On Tue, 30 Aug 2016, Junio C Hamano wrote:
>> > > Jeff King  writes:
>> > > > Hmm, interesting. Your approach seems reasonable, but I have to wonder
>> > > > if writing the pid in the first place is sane.
>> > > >
>> > > > I started to write up my reasoning in this email, but realized it was
>> > > > rapidly becoming the content of a commit message. So here is that
>> > > > commit.
>> > >
>> > > Sounds sensible; if this makes Dscho's "which ones failed in the
>> > > previous run" simpler, that is even better ;-)
>> >
>> > I did not have the time to dig further before now. There must have been a
>> > good reason why we append the PID.
>> >
>> > Sverre, you added that code in 2d84e9f (Modify test-lib.sh to output stats
>> > to t/test-results/*, 2008-06-08): any idea why the - suffix was
>> > needed?
>>
>> I can't really recall, but I think it may have been related to me
>> doing something like this:
>> 1. Make a change, and start running tests (this takes a long time)
>> 2. Notice a failure, start fixing it, leave tests running to find
>> further failures
>> 3. Finish fix, first tests are still running, start another run in a
>> new terminal (possibly of just the one failed test I was fixing) to
>> see if the fix worked.
>>
>> Without the pid, the second run would clobber the results from the first run.
>>
>>
>> If only past-me was more rigorous about writing good commit messages :P.
>
> :-)
>
> Would present-you disagree with stripping off the - suffix, based on
> your recollections?

No objections, I think it should be fine. If anyone uncovers a
particularly compelling reason later on, it's only a commit away :).

-- 
Cheers,

Sverre Rabbelier


Re: [PATCH v2] t/Makefile: add a rule to re-run previously-failed tests

2016-08-31 Thread Sverre Rabbelier
On Wed, Aug 31, 2016 at 3:36 AM Johannes Schindelin
 wrote:
> On Tue, 30 Aug 2016, Junio C Hamano wrote:
> > Jeff King  writes:
> > > Hmm, interesting. Your approach seems reasonable, but I have to wonder
> > > if writing the pid in the first place is sane.
> > >
> > > I started to write up my reasoning in this email, but realized it was
> > > rapidly becoming the content of a commit message. So here is that
> > > commit.
> >
> > Sounds sensible; if this makes Dscho's "which ones failed in the
> > previous run" simpler, that is even better ;-)
>
> I did not have the time to dig further before now. There must have been a
> good reason why we append the PID.
>
> Sverre, you added that code in 2d84e9f (Modify test-lib.sh to output stats
> to t/test-results/*, 2008-06-08): any idea why the - suffix was
> needed?

I can't really recall, but I think it may have been related to me
doing something like this:
1. Make a change, and start running tests (this takes a long time)
2. Notice a failure, start fixing it, leave tests running to find
further failures
3. Finish fix, first tests are still running, start another run in a
new terminal (possibly of just the one failed test I was fixing) to
see if the fix worked.

Without the pid, the second run would clobber the results from the first run.


If only past-me was more rigorous about writing good commit messages :P.


Re: git am oddity

2014-03-31 Thread Sverre Rabbelier
On Mon, Mar 31, 2014 at 3:15 PM, Junio C Hamano  wrote:
> As you are doing -3 (not the -p3), it would have:
>
>  * noticed that the patch is trying to update "baz/file";
>
>  * noticed that there is no "baz/file" but it could salvage the
>patch by doing a three-way merge, in case that the patch was
>prepared against a tree that moved path "foo/bar/baz" to "baz";
>and
>
>  * such a three-way merge succeeds cleanly for a path whose movement
>was detected correctly.
>
> So it does not look odd at all to me (the use of "-p 3" does look
> odd, but I know this is an effort to come up with a minimum example,
> so it is understandable that it may look contribed ;-).

Ah, we were thinking that 'git am' (when run from a subdirectory),
would apply the patches "from the current directory". So the right
solution was to instead do:

$  git am --directory=foo/bar/baz -p 3 0001-my-test.patch

Thank you,

-- 
Cheers,

Sverre Rabbelier
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


git am oddity

2014-03-31 Thread Sverre Rabbelier
Hi,

I noticed something very odd with git am, and have been able to narrow
it down to a minimal example.

 git init tmp
 cd tmp
 mkdir -p foo/bar/baz
 cd foo/bar/baz
 echo file > file
 git add file
 git commit -m "1"
 echo other > other
 echo more >> file
 git add file other
 git commit -m "my test"
 git format-patch HEAD~..
 git reset --hard HEAD~
 # apply the patch in the current directory, chop off the leading directories
 git am -3 -p 3 0001-my-test.patch
 cd ../../..
 git ls-files

Expected output:
foo/bar/baz/file
foo/bar/baz/other

Actual output:
baz/other # the file addition was applied to the root of the
repository, instead of the current directory
foo/bar/baz/file # the file modification was correctly applied, yay

Is this expected behavior?

-- 
Cheers,

Sverre Rabbelier
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH] git_remote_helpers: remove little used Python library

2013-09-07 Thread Sverre Rabbelier
On Sat, Sep 7, 2013 at 6:19 PM, John Keeping  wrote:
> When it was originally added, the git_remote_helpers library was used as
> part of the tests of the remote-helper interface, but since commit
> fc407f9 (Add new simplified git-remote-testgit, 2012-11-28) a simple
> shell script is used for this.

Acked-by: Sverre Rabbelier 

-- 
Cheers,

Sverre Rabbelier
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 3/6] transport-helper: clarify pushing without refspecs

2013-04-17 Thread Sverre Rabbelier
On Wed, Apr 17, 2013 at 5:05 PM, Felipe Contreras
 wrote:
> This has never worked, since it's inception the code simply skips all
> the refs, essentially telling fast-export to do nothing.

Makes sense.

--
Cheers,

Sverre Rabbelier
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 1/3] fast-export: add --signed-tags=warn-strip mode

2013-04-16 Thread Sverre Rabbelier
On Tue, Apr 16, 2013 at 9:48 PM, Junio C Hamano  wrote:
> That is a valid point. Nobody has complained that the current
> warning is too noisy, so perhaps the patch is good as-is?

Ah, hadn't realized that. Probably fine then.

--
Cheers,

Sverre Rabbelier
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 1/3] fast-export: add --signed-tags=warn-strip mode

2013-04-15 Thread Sverre Rabbelier
On Mon, Apr 15, 2013 at 9:47 PM, Junio C Hamano  wrote:
> When you see 78 in the output and you know you have 92 tags in the
> repository, is that sufficient to let you go on, or do we also need
> an easy way to tell which ones are those 78 that were stripped and
> the remaining 14 were not stripped?
>
> There is no reason to worry about "some signed tags are stripped but
> not others", so it feels that the number alone should be sufficient,
> I guess.  If those remaining 14 weren't stripped, that is (at least
> at the moment) by definition because they are unsigned, annotated
> tags.

Or because they were not exported? Perhaps "78 tags stripped, 92
exported in total".

--
Cheers,

Sverre Rabbelier
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 1/3] fast-export: add --signed-tags=warn-strip mode

2013-04-15 Thread Sverre Rabbelier
On Sun, Apr 14, 2013 at 3:57 AM, John Keeping  wrote:
> This issues a warning while stripping signatures from signed tags, which
> allows us to use it as default behaviour for remote helpers which cannot
> specify how to handle signed tags.

Perhaps it makes sense to instead count the number of signed tags and
emit "Stripped signature from %d tags"? For example, for git.git it
would be on the order of a hundred warning lines.

--
Cheers,

Sverre Rabbelier
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 2/2] transport-helper: mention helper name when it dies

2013-04-10 Thread Sverre Rabbelier
On Wed, Apr 10, 2013 at 2:28 PM, Jeff King  wrote:
> Now that's the kind of whole-hearted endorsement I strive for. :)

It's nothing wrong with your patch, the main problem is that there's
not really a good place to point users at.

> If you have better wording, I'm open to it. I do note that we don't
> actually have a manpage for "git-remote-https", though we do for others.
> Probably "man git-remote-helpers" is the most sensible thing to point
> the user to. But I don't even think this is worthy of a big advice
> message. It's a bug in the helper, it shouldn't really happen, and
> giving the user a token they can use to report or google for the error
> is probably good enough.

Yeah, exactly. man git-remote-helpers is more a place for developers
to read how to implement a git-remote-helper, not so much a place for
users to read what they are, and/or how to use them.

--
Cheers,

Sverre Rabbelier
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 2/2] transport-helper: mention helper name when it dies

2013-04-10 Thread Sverre Rabbelier
On Wed, Apr 10, 2013 at 2:16 PM, Jeff King  wrote:
> When we try to read from a remote-helper and get EOF or an
> error, we print a message indicating that the helper died.
> However, users may not know that a remote helper was in use
> (e.g., when using git-over-http), or even what a remote
> helper is.
>
> Let's print the name of the helper (e.g., "git-remote-https");
> this makes it more obvious what the program is for, and
> provides a useful token for reporting bugs or searching for
> more information (e.g., in manpages).
>
> Signed-off-by: Jeff King 

Better than nothing:

Acked-by: Sverre Rabbelier 

--
Cheers,

Sverre Rabbelier
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 1/2] transport-helper: report errors properly

2013-04-10 Thread Sverre Rabbelier
On Wed, Apr 10, 2013 at 2:15 PM, Jeff King  wrote:
> From: Felipe Contreras 
>
> If a push fails because the remote-helper died (with
> fast-export), the user does not see any error message. We do
> correctly die with a failed exit code, as we notice that the
> helper has died while reading back the ref status from the
> helper. However, we don't print any message.  This is OK if
> the helper itself printed a useful error message, but we
> cannot count on that; let's let the user know that the
> helper failed.
>
> In the long run, it may make more sense to propagate the
> error back up to push, so that it can present the usual
> status table and give a nicer message. But this is a much
> simpler fix that can help immediately.
>
> While we're adding tests, let's also confirm that the
> remote-helper dying is also detect when importing refs. We
> currently do so robustly when the helper uses the "done"
> feature (and that is what we test).  We cannot do so
> reliably when the helper does not use the "done" feature,
> but it is not even worth testing; the right solution is for
> the helper to start using "done".
>
> Suggested-by: Jeff King 
> Signed-off-by: Felipe Contreras 
> Signed-off-by: Jeff King 

The fixes you made to this patch make a lot of sense, glad to not have
a 'sleep 1' in our tests.

Acked-by: Sverre Rabbelier 

--
Cheers,

Sverre Rabbelier
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH v4] transport-helper: report errors properly

2013-04-08 Thread Sverre Rabbelier
On Mon, Apr 8, 2013 at 7:40 AM, Felipe Contreras
 wrote:
> +   die("Reading from remote helper failed");

Does the user know what a remote helper is? Could we point them at
some helpful docs in case they don't?

--
Cheers,

Sverre Rabbelier
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: Remote helpers and signed tags

2013-04-07 Thread Sverre Rabbelier
On Sun, Apr 7, 2013 at 2:46 PM, Jonathan Nieder  wrote:
> The remote helper infrastructure is certainly being unhelpful here.  I
> wonder if transport-helper should just pass --signed-tag=strip and be
> done with it (leaving open the possibility of a capability to switch
> to --signed-tag=verbatim when someone wants to teach the testgit
> helper to support that).  What do you think?

I think that's (at least for now) the right thing to do. Passing
anything but signed-tag=strip should be triggered by a capability from
the helper, since most helpers won't know how to deal with signed
tags.

--
Cheers,

Sverre Rabbelier
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH] gitremote-helpers(1): clarify refspec behaviour

2013-04-06 Thread Sverre Rabbelier
In Sat, Apr 6, 2013 at 11:13 AM, John Keeping  wrote:
> The documentation says that "If no 'refspec' capability is advertised,
> there is an implied `refspec *:*`" but this is only the case for the
> "import" command.
>
> Since there is a comment in transport-helper.c indicating that this
> default is for historical reasons, change the documentation to clarify
> that a refspec should always be specified.
>
> Signed-off-by: John Keeping 

Acked-by: Sverre Rabbelier 

--
Cheers,

Sverre Rabbelier
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH v3 6/8] git-remote-testpy: hash bytes explicitly

2013-01-26 Thread Sverre Rabbelier
On Sat, Jan 26, 2013 at 8:44 PM, Michael Haggerty  wrote:
> So to handle all of the cases across Python versions as closely as
> possible to the old 2.x code, it might be necessary to make the code
> explicitly depend on the Python version number, like:

Does this all go away if we restrict ourselves to python 2.6 and just
use the b prefix?

--
Cheers,

Sverre Rabbelier
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH v3 2/8] git_remote_helpers: fix input when running under Python 3

2013-01-23 Thread Sverre Rabbelier
On Wed, Jan 23, 2013 at 11:47 AM, John Keeping  wrote:
>> When did we last revisit what minimal python version we are ok with 
>> requiring?
>
> I was wondering if people would weigh in discussing that in response to
> [1] but no one has commented on that part of it.  As another datapoint,
> Brandon Casey was suggesting patching git-p4.py to support Python 2.4
> [2].
>
> [1] http://article.gmane.org/gmane.comp.version-control.git/213920
> [2] http://article.gmane.org/gmane.comp.version-control.git/214048

I for one would be happy to kill off support for anything older than
2.6 (which had it's latest release on October 1st, 2008).

Junio, how have we decided in the past which version of x to support?

--
Cheers,

Sverre Rabbelier
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH v3 2/8] git_remote_helpers: fix input when running under Python 3

2013-01-23 Thread Sverre Rabbelier
On Sun, Jan 20, 2013 at 5:15 AM, John Keeping  wrote:
> Although 2to3 will fix most issues in Python 2 code to make it run under
> Python 3, it does not handle the new strict separation between byte
> strings and unicode strings.  There is one instance in
> git_remote_helpers where we are caught by this, which is when reading
> refs from "git for-each-ref".
>
> Fix this by operating on the returned string as a byte string rather
> than a unicode string.  As this method is currently only used internally
> by the class this does not affect code anywhere else.
>
> Note that we cannot use byte strings in the source as the 'b' prefix is
> not supported before Python 2.7 so in order to maintain compatibility
> with the maximum range of Python versions we use an explicit call to
> encode().

The three patches that deal with .encode() stuff (2, 7, 8) make me a
bit uncomfortable, as they add some significant complexity to our
python code. Is this the recommended way to deal with this (similar to
the other patch where you linked to the python wiki explaining)?

As one datapoint, it seems that it's actually Python 2.6 that
introduces the b prefix.

http://www.python.org/dev/peps/pep-3112/

When did we last revisit what minimal python version we are ok with requiring?

--
Cheers,

Sverre Rabbelier
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH v3 3/8] git_remote_helpers: Force rebuild if python version changes

2013-01-23 Thread Sverre Rabbelier
On Sun, Jan 20, 2013 at 5:15 AM, John Keeping  wrote:
> When different version of python are used to build via distutils, the
> behaviour can change.  Detect changes in version and pass --force in
> this case.
>
> Signed-off-by: John Keeping 

Someone else's review on this would be appreciated, the idea sounds
sane but I can't really comment on the implementation.

--
Cheers,

Sverre Rabbelier
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH v3 1/8] git_remote_helpers: Allow building with Python 3

2013-01-23 Thread Sverre Rabbelier
On Sun, Jan 20, 2013 at 5:15 AM, John Keeping  wrote:
> Change inline Python to call "print" as a function not a statement.
>
> This is harmless because Python 2 will see the parentheses as redundant
> grouping but they are necessary to run this code with Python 3.
>
> Signed-off-by: John Keeping 

Acked-by: Sverre Rabbelier 

--
Cheers,

Sverre Rabbelier
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH v2 4/8] git_remote_helpers: use 2to3 if building with Python 3

2013-01-18 Thread Sverre Rabbelier
Assuming you tried this out on both 2.x and 3.x:

Acked-by: Sverre Rabbelier 

On Fri, Jan 18, 2013 at 2:32 AM, John Keeping  wrote:
> On Thu, Jan 17, 2013 at 09:15:08PM -0800, Sverre Rabbelier wrote:
>> On Thu, Jan 17, 2013 at 10:53 AM, John Keeping  wrote:
>> > [1] http://wiki.python.org/moin/PortingPythonToPy3k
>>
>> This link seems dead.
>
> Looks like the Python wiki is down [1].
>
> I'll replace it with [2] since the content is similar and it should be
> easier to find a mirror of the Python documentation than of the wiki.
>
> [1] http://pyfound.blogspot.co.uk/2013/01/wikipythonorg-compromised.html
> [2] http://docs.python.org/3.3/howto/pyporting.html#during-installation
>
>
> John



-- 
Cheers,

Sverre Rabbelier
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH v2 4/8] git_remote_helpers: use 2to3 if building with Python 3

2013-01-17 Thread Sverre Rabbelier
On Thu, Jan 17, 2013 at 10:53 AM, John Keeping  wrote:
> [1] http://wiki.python.org/moin/PortingPythonToPy3k

This link seems dead.

--
Cheers,

Sverre Rabbelier
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH v2 7/8] git-remote-testpy: don't do unbuffered text I/O

2013-01-17 Thread Sverre Rabbelier
On Thu, Jan 17, 2013 at 10:54 AM, John Keeping  wrote:
> -sys.stdin = os.fdopen(sys.stdin.fileno(), 'r', 0)
> +sys.stdin = os.fdopen(sys.stdin.fileno(), 'rb', 0)

It is not immediately obvious why you would open stdin in rb mode,
please add a comment.

--
Cheers,

Sverre Rabbelier
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH v2 8/8] git-remote-testpy: call print as a function

2013-01-17 Thread Sverre Rabbelier
Looks harmless enough.

Acked-by: Sverre Rabbelier 

On Thu, Jan 17, 2013 at 10:54 AM, John Keeping  wrote:
> This is harmless in Python 2, which sees the parentheses as redundant
> grouping, but is required for Python 3.  Since this is the only change
> required to make this script just run under Python 3 without needing
> 2to3 it seems worthwhile.
>
> The case of an empty print must be handled specially because in that
> case Python 2 will interpret '()' as an empty tuple and print it as
> '()'; inserting an empty string fixes this.
>
> Signed-off-by: John Keeping 
> ---
>  git-remote-testpy.py | 28 ++--
>  1 file changed, 14 insertions(+), 14 deletions(-)
>
> diff --git a/git-remote-testpy.py b/git-remote-testpy.py
> index bc5e3cf..ccdb2dc 100644
> --- a/git-remote-testpy.py
> +++ b/git-remote-testpy.py
> @@ -87,9 +87,9 @@ def do_capabilities(repo, args):
>  """Prints the supported capabilities.
>  """
>
> -print "import"
> -print "export"
> -print "refspec refs/heads/*:%s*" % repo.prefix
> +print("import")
> +print("export")
> +print("refspec refs/heads/*:%s*" % repo.prefix)
>
>  dirname = repo.get_base_path(repo.gitdir)
>
> @@ -98,11 +98,11 @@ def do_capabilities(repo, args):
>
>  path = os.path.join(dirname, 'git.marks')
>
> -print "*export-marks %s" % path
> +print("*export-marks %s" % path)
>  if os.path.exists(path):
> -print "*import-marks %s" % path
> +print("*import-marks %s" % path)
>
> -print # end capabilities
> +print('') # end capabilities
>
>
>  def do_list(repo, args):
> @@ -115,16 +115,16 @@ def do_list(repo, args):
>
>  for ref in repo.revs:
>  debug("? refs/heads/%s", ref)
> -print "? refs/heads/%s" % ref
> +print("? refs/heads/%s" % ref)
>
>  if repo.head:
>  debug("@refs/heads/%s HEAD" % repo.head)
> -print "@refs/heads/%s HEAD" % repo.head
> +print("@refs/heads/%s HEAD" % repo.head)
>  else:
>  debug("@refs/heads/master HEAD")
> -print "@refs/heads/master HEAD"
> +print("@refs/heads/master HEAD")
>
> -print # end list
> +print('') # end list
>
>
>  def update_local_repo(repo):
> @@ -164,7 +164,7 @@ def do_import(repo, args):
>  ref = line[7:].strip()
>  refs.append(ref)
>
> -print "feature done"
> +print("feature done")
>
>  if os.environ.get("GIT_REMOTE_TESTGIT_FAILURE"):
>  die('Told to fail')
> @@ -172,7 +172,7 @@ def do_import(repo, args):
>  repo = update_local_repo(repo)
>  repo.exporter.export_repo(repo.gitdir, refs)
>
> -    print "done"
> +print("done")
>
>
>  def do_export(repo, args):
> @@ -192,8 +192,8 @@ def do_export(repo, args):
>  repo.non_local.push(repo.gitdir)
>
>  for ref in changed:
> -print "ok %s" % ref
> -print
> +print("ok %s" % ref)
> +print('')
>
>
>  COMMANDS = {
> --
> 1.8.1.1.260.g99b33f4.dirty
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
Cheers,

Sverre Rabbelier
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 0/6] Improve remote helper documentation

2012-12-07 Thread Sverre Rabbelier
On Fri, Dec 7, 2012 at 11:09 AM, Junio C Hamano  wrote:
> A second ping to people who have touched transport-helper.c,
> remote-testsvn.c, git-remote-testgit, and contrib/remote-helpers/ in
> the past 18 months for comments.  I've re-read the documentation
> updates myself and didn't find anything majorly objectionable.

FWIW:

Acked-by: Sverre Rabbelier 

-- 
Cheers,

Sverre Rabbelier
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH v5 15/15] fast-export: don't handle uninteresting refs

2012-11-26 Thread Sverre Rabbelier
On Mon, Nov 26, 2012 at 11:26 AM, Johannes Schindelin
 wrote:
>> We added rev_cmdline_info since then so that we can tell what refs
>> were given from the command line in what way, and I thought that we
>> applied a patch from Sverre that uses it instead of the object
>> flags.  Am I misremembering things?
>
> It does sound so familiar that I am intended to claim that you remember
> things correctly.

FWIW, I implemented that in
http://thread.gmane.org/gmane.comp.version-control.git/184874 but
didn't do the work to get it merged.

-- 
Cheers,

Sverre Rabbelier
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH v5 11/15] remote-testgit: make clear the 'done' feature

2012-11-21 Thread Sverre Rabbelier
On Wed, Nov 21, 2012 at 10:11 AM, Junio C Hamano  wrote:
> I'd state it like this, but I may have guessed what Felipe intended
> incorrectly.
>
> remote-testgit: advertise "done" feature and write "done" ourselves
>
> Instead of letting "fast-export" advertise the feature and ending
> its stream with "done", do it ourselves.  This way, it would make it
> more clear to people who want to write their own remote-helper to
> produce fast-export streams without using "fast-export
> --use-done-feature" that they are supposed to end their stream with
>     "done".

With that commit message:

Acked-by: Sverre Rabbelier 

-- 
Cheers,

Sverre Rabbelier
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH v3 4/4] fast-export: make sure refs are updated properly

2012-10-30 Thread Sverre Rabbelier
On Tue, Oct 30, 2012 at 5:37 PM, Jonathan Nieder  wrote:
> Felipe Contreras wrote:
>
>> --- a/builtin/fast-export.c
>> +++ b/builtin/fast-export.c
>> @@ -523,11 +523,16 @@ static void get_tags_and_duplicates(struct 
>> object_array *pending,
>>   typename(e->item->type));
>>   continue;
>>   }
>> - if (commit->util) {
>> - /* more than one name for the same object */
>> +
>> + /*
>> +  * This ref will not be updated through a commit, lets make
>> +  * sure it gets properly upddated eventually.
>> +  */
>> + if (commit->util || commit->object.flags & SHOWN) {
>>   if (!(commit->object.flags & UNINTERESTING))
>>   string_list_append(extra_refs, 
>> full_name)->util = commit;
>> - } else
>> + }
>> + if (!commit->util)
>>   commit->util = full_name;
>
> Here's an explanation of why the above makes sense to me.
>
> get_tags_and_duplicates() gets called after the marks import and
> before the revision walk.  It walks through the revs from the
> commandline and for each one:
>
>  - peels it to a refname, and then to a commit
>  - stores the refname so fast-export knows what arg to pass to
>the "commit" command during the revision walk
>  - if it already had a refname stored, instead adds the
>(refname, commit) pair to the extra_refs list, so fast-export
>knows to add a "reset" command later.
>
> If the commit already has the SHOWN flag set because it was pointed to
> by a mark, it is not going to come up in the revision walk, so it will
> not be mentioned in the output stream unless it is added to
> extra_refs.  That's what this patch does.
>
> Incidentally, the change from "else" to "if (!commit->util)" is
> unnecessary because if a commit is already SHOWN then it will not be
> encountered in the revision walk so commit->util does not need to be
> set.
>
> If the commit does not have the SHOWN or UNINTERESTING flag set but it
> is going to get the UNINTERESTING flag set during the walk because of
> a negative commit listed on the command line, this patch won't help.

Thanks for the thorough explanation. Perhaps some of that could make
it's way into the commit message?

-- 
Cheers,

Sverre Rabbelier
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH v2 4/4] fast-export: make sure refs are updated properly

2012-10-30 Thread Sverre Rabbelier
On Tue, Oct 30, 2012 at 3:18 PM, Felipe Contreras
 wrote:
> Which is expected and correct; the branch already points to the right
> commit, no need for an extra reset.

I think you're correct. Thanks for confirming.

-- 
Cheers,

Sverre Rabbelier
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH v2 4/4] fast-export: make sure refs are updated properly

2012-10-30 Thread Sverre Rabbelier
On Tue, Oct 30, 2012 at 2:35 PM, Felipe Contreras
 wrote:
> On Tue, Oct 30, 2012 at 10:17 PM, Sverre Rabbelier  
> wrote:
>> On Tue, Oct 30, 2012 at 11:47 AM, Felipe Contreras
>>  wrote:
>>> Why would it? We are not changing the way objects are exported, the
>>> only difference is what happens at the end
>>> (handle_tags_and_duplicates()).
>>
>> Because the marking is per-commit, not per-ref, right?
>
> Oh, you meant using marks?

No, I meant the 'SHOWN' flag, doesn't it get added per commit, not per
ref? That is, commit->object.flags & SHOWN refers to the object
underlying the ref. So I suspect this scenario doesn't pass the tests:

git init &&
echo first > content &&
git add content &&
git commit -m "first" &&
git branch first &&
echo two > content &&
git commit -m "second" &&
git branch second &&
git fast-export first > actual &&
test_cmp actual expected_first &&
git fast-export second > actual &&
test_cmp actual expected_second

With expected_first being something like:



And expected_second being something like



-- 
Cheers,

Sverre Rabbelier
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH v2 4/4] fast-export: make sure refs are updated properly

2012-10-30 Thread Sverre Rabbelier
On Tue, Oct 30, 2012 at 11:47 AM, Felipe Contreras
 wrote:
> Why would it? We are not changing the way objects are exported, the
> only difference is what happens at the end
> (handle_tags_and_duplicates()).

Because the marking is per-commit, not per-ref, right? Perhaps you
could add a simple test case to make sure it works as expected?
Something along the lines of the scenario I described in my previous
email?

-- 
Cheers,

Sverre Rabbelier
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH v2 4/4] fast-export: make sure refs are updated properly

2012-10-30 Thread Sverre Rabbelier
On Tue, Oct 30, 2012 at 10:11 AM, Felipe Contreras
 wrote:
> When an object has already been exported (and thus is in the marks) it
> is flagged as SHOWN, so it will not be exported again, even if this time
> it's exported through a different ref.
>
> We don't need the object to be exported again, but we want the ref
> updated, which doesn't happen.
>
> Since we can't know if a ref was exported or not, let's just assume that
> if the commit was marked (flags & SHOWN), the user still wants the ref
> updated.
>
> So:
>
>  % git branch test master
>  % git fast-export $mark_flags master
>  % git fast-export $mark_flags test
>
> Would export 'test' properly.
>
> Additionally, this fixes issues with remote helpers; now they can push
> refs wich objects have already been exported.

Won't this also export child (or maybe parent) branches that weren't
mentioned? For example:

$ git branch one
$ echo foo > content
$ git commit -m two
$ git fast-export one
$ git fast-export two

I suspect that one of those will export both one and two. If not, this
seems like a great solution to the fast-export problem.

-- 
Cheers,

Sverre Rabbelier
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 1/3] t9350: point out that refs are not updated correctly

2012-10-25 Thread Sverre Rabbelier
On Thu, Oct 25, 2012 at 12:48 AM, Jonathan Nieder  wrote:
> Sverre Rabbelier wrote:
>
>> I know there was a reason why using UNINTERESTING didn't work
>> (otherwise we could've used that to start with, instead of needing
>> Junio's whence solution). I think all refs ended up being marked as
>> UNINTERESTING or somesuch.
>
> True.  Is it be possible to check UNINTERESTING in revs->cmdline
> before the walk?

That might work, maybe Dscho remembers why we did not go with that approach.

-- 
Cheers,

Sverre Rabbelier
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 1/3] t9350: point out that refs are not updated correctly

2012-10-25 Thread Sverre Rabbelier
On Thu, Oct 25, 2012 at 12:34 AM, Jonathan Nieder  wrote:
> If I remember right, '^foo1' is (whence == REV_CMD_REV) with (flags ==
> UNINTERESTING).  That's why sequencer.c checks for unadorned revs like
> this:
>
> if (opts->revs->cmdline.nr == 1 &&
> opts->revs->cmdline.rev->whence == REV_CMD_REV &&
> opts->revs->no_walk &&
> !opts->revs->cmdline.rev->flags) {
>
> Maybe
>
> if (elem->flags & UNINTERESTING)
> continue;
> if (elem->whence == REV_CMD_PARENTS_ONLY)   /* foo^@ */
> continue;
>
> would work well here?  That would handle bizarre cases like "--not
> next..master" (and ordinary cases like "master...next") better, by
> focusing on the semantics instead of syntax.

I know there was a reason why using UNINTERESTING didn't work
(otherwise we could've used that to start with, instead of needing
Junio's whence solution). I think all refs ended up being marked as
UNINTERESTING or somesuch.

-- 
Cheers,

Sverre Rabbelier
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 1/3] t9350: point out that refs are not updated correctly

2012-10-25 Thread Sverre Rabbelier
On Wed, Oct 24, 2012 at 11:19 PM, Felipe Contreras
 wrote:
> Oh really? This is with your patches:
>
> % git fast-export --{im,ex}port-marks=/tmp/marks foo1 ^foo2 foo3..foo3
> reset refs/heads/foo1
> from :21
>
> reset refs/heads/foo3
> from :21
>
> reset refs/heads/foo3
> from :21
>
> reset refs/heads/foo2
> from :21

That's weird, we have this bit:

+   if (elem->whence != REV_CMD_REV && elem->whence != 
REV_CMD_RIGHT)
+   continue;

If I understand correctly that should cause it to only output revs
(e.g. 'foo1') and the rhs side of a have..want spec.
-- 
Cheers,

Sverre Rabbelier
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 1/3] t9350: point out that refs are not updated correctly

2012-10-24 Thread Sverre Rabbelier
On Wed, Oct 24, 2012 at 10:50 PM, Felipe Contreras
 wrote:
> This works just fine. Go ahead, apply my patch, and run it, the second
> branch gets updated.

Yes, but as you said:

> That is already the case, my patch will cause this to generate the same 
> output:
> % git fast-export --{im,ex}port-marks=/tmp/marks ^foo foo.foo
> Which is still not got, but not catastrophic by any means.

Which is exactly the reason we (Dscho and I during our little
hackathon) went with the approach we did. We considered the approach
you took (if I still had the repository I might even find something
very like your patch in my reflog), but dismissed it for that reason.
By teaching fast-export to properly re-export interesting refs, this
exporting of negated refs does not happen. Additionally, you say it is
not catastrophic, but it _is_, if you run: 'git fast-export ^master
foo', you do not expect master to suddenly show up on the remote side.

I agree that your test more accurately describes what we're testing
(and in fact, it should probably go in the tests for remote helpers).
However, this test points out a shortcoming of fast-export that
prevents us from implementing a cleaner solution to the 'fast-export
push an existing ref' problem.

-- 
Cheers,

Sverre Rabbelier
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 1/3] t9350: point out that refs are not updated correctly

2012-10-24 Thread Sverre Rabbelier
On Wed, Oct 24, 2012 at 10:28 PM, Jonathan Nieder  wrote:
> The testcase is imho correct and does not need changing.  So yes, I
> don't want your help changing it.  I don't suspect you will be using
> "git fast-export $(git rev-parse master)..master".  It is safe and
> good to add additional testcases documenting the syntax that you do
> use, as an independent topic.

To re-iterate Dscho's point, the reason for this testcase is that if
you do this:
$ git checkout master
$ git branch next
$ git push hg://example.com master
$ git push hg://example.com next

With the current design, next will not be present on the remote. This
is caused by the fact that git looks at "fast-export ^master next",
sees that it's empty, and decides not to export anything. This patch
series solves that, by having "fast-export ^master next" emit a "from
:42\nreset next" (or something like that, assuming :42 is where master
is currently at).

-- 
Cheers,

Sverre Rabbelier
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH] remote-testgit: properly check for errors

2012-10-22 Thread Sverre Rabbelier
On Mon, Oct 22, 2012 at 1:56 PM, Felipe Contreras
 wrote:
> diff --git a/git-remote-testgit.py b/git-remote-testgit.py
> index 5f3ebd2..b8707e6 100644
> --- a/git-remote-testgit.py
> +++ b/git-remote-testgit.py
> @@ -159,6 +159,11 @@ def do_import(repo, args):
>  ref = line[7:].strip()
>  refs.append(ref)
>
> +print "feature done"

There's not enough context for me to see in which part of the code
this is, import or export? If you advertise 'feature done', shouldn't
you also print 'done' when you finish writing the fast-export stream?
If that's already the case feel free to ignore me :)

-- 
Cheers,

Sverre Rabbelier
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH v3 5/6] tests: add remote-hg tests

2012-10-21 Thread Sverre Rabbelier
On Sun, Oct 21, 2012 at 10:49 AM, Felipe Contreras
 wrote:
> From the original remote-hg.
>
> You need git-remote-hg already in your path to run them.
>
> I'm not proposing to include this patch like this, but should make it easier 
> to
> test.

You should also have a look at the tests that were marked as "expected
to fail", since they point out a bug with fast-export. I'd sent a
series to fix that, but didn't follow-up to get it merged:

http://thread.gmane.org/gmane.comp.version-control.git/184874

It'd be great if you want to pick this up (like you did with the tests).

-- 
Cheers,

Sverre Rabbelier
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH] Add new git-remote-hd helper

2012-10-17 Thread Sverre Rabbelier
On Wed, Oct 17, 2012 at 10:18 PM, Felipe Contreras
 wrote:
> Right now I've just added an error when using remote repositories. But
> it seems there's no way around it; if we want to have support for
> remote repos, we need to make a local clone.

My git-remote-hg does the local clone into .git/ using a hash of the
url (although you could just as well use urlencode, basically any way
to safely use a url as a directory name). Have a look if you want.

-- 
Cheers,

Sverre Rabbelier
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH] transport-helper: call git fast-import properly

2012-10-17 Thread Sverre Rabbelier
On Wed, Oct 17, 2012 at 1:27 AM, Felipe Contreras
 wrote:
> The marks options are being ignored right now.

It seems unlikely to me that this never worked, surely no reviewer
would accept a patch that doesn't actually implement the feature?
What's the history here?

-- 
Cheers,

Sverre Rabbelier
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH] Add new git-remote-hd helper

2012-10-17 Thread Sverre Rabbelier
On Wed, Oct 17, 2012 at 11:12 AM, Felipe Contreras
 wrote:
> But fine, lets remove the tests out of the equation (150 lines), the
> number of lines of code still exceeds 3000.

I don't think it's fair to just look at LOC, git-remote-hg when it was
just parsing was fairly simple. Most of the current code is our copy
of the python fast-import library which is only used to support
pushing to mercurial.

-- 
Cheers,

Sverre Rabbelier
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH] Document the --done option.

2012-08-22 Thread Sverre Rabbelier
On Wed, Aug 22, 2012 at 10:38 AM, Junio C Hamano  wrote:
> "Eric S. Raymond"  writes:
>
>> ---
>
> A forgotten Sign-off?
>
> Sverre, the text matches my understanding as well as what be56862
> (fast-import: introduce 'done' command, 2011-07-16) says it did.
> Ack?

Acked-by: Sverre Rabbelier 

-- 
Cheers,

Sverre Rabbelier
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html