git am oddity

[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: git am oddity

[Sverre Rabbelier]

On Mon, Mar 31, 2014 at 3:15 PM, Junio C Hamano gits...@pobox.com 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

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

[Sverre Rabbelier]

On Sat, Sep 7, 2013 at 6:19 PM, John Keeping j...@keeping.me.uk 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 srabbel...@gmail.com

-- 
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.

[Sverre Rabbelier]

On Wed, Aug 22, 2012 at 10:38 AM, Junio C Hamano gits...@pobox.com wrote:
 Eric S. Raymond e...@thyrsus.com 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 srabbel...@gmail.com

-- 
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

[Sverre Rabbelier]

On Wed, Oct 17, 2012 at 11:12 AM, Felipe Contreras
felipe.contre...@gmail.com 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] transport-helper: call git fast-import properly

[Sverre Rabbelier]

On Wed, Oct 17, 2012 at 1:27 AM, Felipe Contreras
felipe.contre...@gmail.com 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

[Sverre Rabbelier]

On Wed, Oct 17, 2012 at 10:18 PM, Felipe Contreras
felipe.contre...@gmail.com 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 v3 5/6] tests: add remote-hg tests

[Sverre Rabbelier]

On Sun, Oct 21, 2012 at 10:49 AM, Felipe Contreras
felipe.contre...@gmail.com 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] remote-testgit: properly check for errors

[Sverre Rabbelier]

On Mon, Oct 22, 2012 at 1:56 PM, Felipe Contreras
felipe.contre...@gmail.com 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 1/3] t9350: point out that refs are not updated correctly

[Sverre Rabbelier]

On Wed, Oct 24, 2012 at 10:28 PM, Jonathan Nieder jrnie...@gmail.com 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 1/3] t9350: point out that refs are not updated correctly

[Sverre Rabbelier]

On Wed, Oct 24, 2012 at 10:50 PM, Felipe Contreras
felipe.contre...@gmail.com 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

[Sverre Rabbelier]

On Wed, Oct 24, 2012 at 11:19 PM, Felipe Contreras
felipe.contre...@gmail.com 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

[Sverre Rabbelier]

On Thu, Oct 25, 2012 at 12:34 AM, Jonathan Nieder jrnie...@gmail.com 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 v2 4/4] fast-export: make sure refs are updated properly

[Sverre Rabbelier]

On Tue, Oct 30, 2012 at 10:11 AM, Felipe Contreras
felipe.contre...@gmail.com 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 v2 4/4] fast-export: make sure refs are updated properly

[Sverre Rabbelier]

On Tue, Oct 30, 2012 at 11:47 AM, Felipe Contreras
felipe.contre...@gmail.com 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

[Sverre Rabbelier]

On Tue, Oct 30, 2012 at 2:35 PM, Felipe Contreras
felipe.contre...@gmail.com wrote:
 On Tue, Oct 30, 2012 at 10:17 PM, Sverre Rabbelier srabbel...@gmail.com 
 wrote:
 On Tue, Oct 30, 2012 at 11:47 AM, Felipe Contreras
 felipe.contre...@gmail.com 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:
fast-export stream with the first commit
reset command to set first to the right commit

And expected_second being something like
fast export stream with the first and second command
reset command to set first and second to their respective branches

-- 
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

[Sverre Rabbelier]

On Tue, Oct 30, 2012 at 3:18 PM, Felipe Contreras
felipe.contre...@gmail.com 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 v3 4/4] fast-export: make sure refs are updated properly

[Sverre Rabbelier]

On Tue, Oct 30, 2012 at 5:37 PM, Jonathan Nieder jrnie...@gmail.com 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 v5 11/15] remote-testgit: make clear the 'done' feature

[Sverre Rabbelier]

On Wed, Nov 21, 2012 at 10:11 AM, Junio C Hamano gits...@pobox.com 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 srabbel...@gmail.com

-- 
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

[Sverre Rabbelier]

On Mon, Nov 26, 2012 at 11:26 AM, Johannes Schindelin
johannes.schinde...@gmx.de 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 0/6] Improve remote helper documentation

[Sverre Rabbelier]

On Fri, Dec 7, 2012 at 11:09 AM, Junio C Hamano gits...@pobox.com 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 srabbel...@gmail.com

-- 
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

[Sverre Rabbelier]

Looks harmless enough.

Acked-by: Sverre Rabbelier srabbel...@gmail.com

On Thu, Jan 17, 2013 at 10:54 AM, John Keeping j...@keeping.me.uk 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 j...@keeping.me.uk
 ---
  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 v2 7/8] git-remote-testpy: don't do unbuffered text I/O

[Sverre Rabbelier]

On Thu, Jan 17, 2013 at 10:54 AM, John Keeping j...@keeping.me.uk 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 4/8] git_remote_helpers: use 2to3 if building with Python 3

[Sverre Rabbelier]

On Thu, Jan 17, 2013 at 10:53 AM, John Keeping j...@keeping.me.uk 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 4/8] git_remote_helpers: use 2to3 if building with Python 3

[Sverre Rabbelier]

Assuming you tried this out on both 2.x and 3.x:

Acked-by: Sverre Rabbelier srabbel...@gmail.com

On Fri, Jan 18, 2013 at 2:32 AM, John Keeping j...@keeping.me.uk 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 j...@keeping.me.uk 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 v3 1/8] git_remote_helpers: Allow building with Python 3

[Sverre Rabbelier]

On Sun, Jan 20, 2013 at 5:15 AM, John Keeping j...@keeping.me.uk 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 j...@keeping.me.uk

Acked-by: Sverre Rabbelier srabbel...@gmail.com

--
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

[Sverre Rabbelier]

On Sun, Jan 20, 2013 at 5:15 AM, John Keeping j...@keeping.me.uk 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 j...@keeping.me.uk

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 2/8] git_remote_helpers: fix input when running under Python 3

[Sverre Rabbelier]

On Sun, Jan 20, 2013 at 5:15 AM, John Keeping j...@keeping.me.uk 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 6/8] git-remote-testpy: hash bytes explicitly

[Sverre Rabbelier]

On Sat, Jan 26, 2013 at 8:44 PM, Michael Haggerty mhag...@alum.mit.edu 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] gitremote-helpers(1): clarify refspec behaviour

[Sverre Rabbelier]

In Sat, Apr 6, 2013 at 11:13 AM, John Keeping j...@keeping.me.uk 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 j...@keeping.me.uk

Acked-by: Sverre Rabbelier srabbel...@gmail.com

--
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

[Sverre Rabbelier]

On Sun, Apr 7, 2013 at 2:46 PM, Jonathan Nieder jrnie...@gmail.com 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 v4] transport-helper: report errors properly

[Sverre Rabbelier]

On Mon, Apr 8, 2013 at 7:40 AM, Felipe Contreras
felipe.contre...@gmail.com 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: [PATCH 1/2] transport-helper: report errors properly

[Sverre Rabbelier]

On Wed, Apr 10, 2013 at 2:15 PM, Jeff King p...@peff.net wrote:
 From: Felipe Contreras felipe.contre...@gmail.com

 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 p...@peff.net
 Signed-off-by: Felipe Contreras felipe.contre...@gmail.com
 Signed-off-by: Jeff King p...@peff.net

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 srabbel...@gmail.com

--
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

[Sverre Rabbelier]

On Wed, Apr 10, 2013 at 2:16 PM, Jeff King p...@peff.net 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 p...@peff.net

Better than nothing:

Acked-by: Sverre Rabbelier srabbel...@gmail.com

--
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

[Sverre Rabbelier]

On Wed, Apr 10, 2013 at 2:28 PM, Jeff King p...@peff.net 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 1/3] fast-export: add --signed-tags=warn-strip mode

[Sverre Rabbelier]

On Sun, Apr 14, 2013 at 3:57 AM, John Keeping j...@keeping.me.uk 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 1/3] fast-export: add --signed-tags=warn-strip mode

[Sverre Rabbelier]

On Mon, Apr 15, 2013 at 9:47 PM, Junio C Hamano gits...@pobox.com 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

[Sverre Rabbelier]

On Tue, Apr 16, 2013 at 9:48 PM, Junio C Hamano gits...@pobox.com 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 3/6] transport-helper: clarify pushing without refspecs

[Sverre Rabbelier]

On Wed, Apr 17, 2013 at 5:05 PM, Felipe Contreras
felipe.contre...@gmail.com 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 v2] t/Makefile: add a rule to re-run previously-failed tests

[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: [PATCH v2] t/Makefile: add a rule to re-run previously-failed tests

[Sverre Rabbelier]

On Thu, Sep 1, 2016 at 1:27 AM, Johannes Schindelin
<johannes.schinde...@gmx.de> wrote:
> On Wed, 31 Aug 2016, Sverre Rabbelier wrote:
>> On Wed, Aug 31, 2016 at 3:36 AM Johannes Schindelin
>> <johannes.schinde...@gmx.de> wrote:
>> > On Tue, 30 Aug 2016, Junio C Hamano wrote:
>> > > Jeff King <p...@peff.net> 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