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