Re: [PATCH v5 15/15] fast-export: don't handle uninteresting refs
Sverre Rabbelier writes: > 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. Ah, OK. Should I expect an updated series then? How would it interact with the recent work by Felipe? -- 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 15/15] fast-export: don't handle uninteresting refs
Hi Junio, On Mon, 26 Nov 2012, Junio C Hamano wrote: > Johannes Schindelin writes: > > > If you changed your stance on the patch Sverre and I sent to fix this, > > we could get a non-partial fix for this. > > This is long time ago so I may be misremembering the details, but I > thought the original patch was (ab)using object flags to mark "this was > explicitly asked for, even though some other range operation may have > marked it uninteresting". Because it predated the introduction of the > rev_cmdline_info mechanism to record what was mentioned on the command > line separately from what objects are uninteresting (i.e. object flags), > it may have been one convenient way to record this information, but it > still looked unnecessarily ugly hack to me, in that it allocated scarce > object flag bits to represent a narrow special case (iirc, only a > freestanding "A" on the command line but not "A" spelled in "B..A", or > something), making it more expensive to record other kinds of command > line information in a way consistent with the approach chosen (we do not > want to waste object flag bits in order to record "this was right hand > side tip of the symmetric difference range" and such). Good to know. I will find some time to look at rev_cmdline_info and patch my patch. > If you are calling "do not waste object flags to represent one > special case among endless number of possibilities, as it will make > it impossible to extend it" my stance, that hasn't changed. > > 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. Ciao, Johannes -- 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
Johannes Schindelin writes: > If you changed your stance on the patch Sverre and I sent to fix this, we > could get a non-partial fix for this. This is long time ago so I may be misremembering the details, but I thought the original patch was (ab)using object flags to mark "this was explicitly asked for, even though some other range operation may have marked it uninteresting". Because it predated the introduction of the rev_cmdline_info mechanism to record what was mentioned on the command line separately from what objects are uninteresting (i.e. object flags), it may have been one convenient way to record this information, but it still looked unnecessarily ugly hack to me, in that it allocated scarce object flag bits to represent a narrow special case (iirc, only a freestanding "A" on the command line but not "A" spelled in "B..A", or something), making it more expensive to record other kinds of command line information in a way consistent with the approach chosen (we do not want to waste object flag bits in order to record "this was right hand side tip of the symmetric difference range" and such). If you are calling "do not waste object flags to represent one special case among endless number of possibilities, as it will make it impossible to extend it" my stance, that hasn't changed. 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? -- 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
Hi Junio, On Sun, 25 Nov 2012, Junio C Hamano wrote: > From: Johannes Schindelin > Subject: Re: [PATCH v3 4/4] fast-export: make sure refs are updated properly > Date: Fri, 2 Nov 2012 16:17:14 +0100 (CET) > Message-ID: > > > (which is $gmane/208946) that says: > > Note that > > $ git branch foo master~1 > $ git fast-export foo master~1..master > > still does not update the "foo" ref, but a partial fix is better > than no fix. If you changed your stance on the patch Sverre and I sent to fix this, we could get a non-partial fix for this. You wanted a fix for a bigger problem, though, which I am unwilling to fix because it is not my itch to scratch and I have to balance my time. Ciao, Johannes -- 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
Felipe Contreras writes: > On Wed, Nov 21, 2012 at 8:48 PM, Jeff King wrote: > ... > I would like to understand that that even means. What behavior is > currently broken? I do not know if this is the same as what Peff was referring to, but I found this message in the discussion thread during my absense. From: Johannes Schindelin Subject: Re: [PATCH v3 4/4] fast-export: make sure refs are updated properly Date: Fri, 2 Nov 2012 16:17:14 +0100 (CET) Message-ID: (which is $gmane/208946) that says: Note that $ git branch foo master~1 $ git fast-export foo master~1..master still does not update the "foo" ref, but a partial fix is better than no fix. -- 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 21.11.2012, at 06:08, Junio C Hamano wrote: > Jonathan Nieder writes: > >> Never mind that others have said that that's not the current interface >> (I don't yet see why it would be a good interface after a transition, >> but maybe it would be). Still, hopefully that clarifies the intended >> meaning. > > Care to explain how the current interface is supposed to work, how > fast-export and transport-helper should interact with remote helpers > that adhere to the current interface, and how well/correctly the > current implementation of these pieces work? Yes, please! > > What I am trying to get at is to see where the problem lies. Felipe > sees bugs in the aggregated whole. Is the root cause of the problems > he sees some breakages in the current interface? Is the interface > designed right but the problem is that the implementation of the > transport-helper is buggy and driving fast-export incorrectly? Or is > the implementation of the fast-export buggy and emitting wrong results, > even though the transport-helper is driving fast-export correctly? > Something else? > > I see Felipe keeps repeating that there are bugs, and keeps posting > patches to change fast-export, but I haven't seen a concrete "No, > the reason why you see these problems is because you are not using > the interface correctly; the currrent interface is fine. Here is > how you can fix your program" from "others". I was wondering about the same, actually... Moreover, I started to try to understand more about this, but found this a bit difficult. Apparently I am primarily supposed to learn about remote helpers by reverse engineering the (sparsely commented, if at all) existing ones. The fact that remote helpers can implement different subsets of the feature spectrum complicates this further. Overall, my impression is that there are two kinds of remote helpers: 1) Some are git-to-git helpers, which allow access to another git repos via some intermediate media / protocol (via http, ssh, ...). Those use either connect, or fetch+push. They do not need marks, because they can use the git sha1s. Examples (together with the capabilities they claim to implement): - remote-curl: fetch, option, push - remote-ext: connect - remote-fd: connect 2) Some are interfaces to foreign systems (bzr, hg, mediawiki, ...). They cannot use sha1s and must use marks (at least that is how I understand felipe's explanation). These tools use import combined with either export, or push. Examples: - git-remote-mediawiki: import, push, refspec (its capabilities command also prints "list", but that seems to be a bug?) - git-remote-hg: import, export, refspec, import-marks, export-marks (both the msysgit one and felipe's - git-remote-bzr: import, push (the one from https://github.com/lelutin/git-remote-bzr) - git-remote-bzr (felipe's): import, export, refspec, *import-marks, *export-marks (but why the * ?) Does that sound about right? If so, can somebody give me a hint when a type 2 helper would use "export" and when "push"? And while I am at it: git-remote-helpers.txt does not mention the "export", "import-marks" and "export-marks" capabilities. Could somebody who knows what they do look into fixing that? Overall, that doc helped me a bit, but it is more a reference to somebody who already understands in detail how remote helpers work, and who just wants to look up some specific detail :-(. Some hints on when to implement which capabilities might be useful (similar to the "Tips and Tricks" section in git-fast-import.txt). As it is, felipe's recent explanation on why he thinks marks are essential for remote-helpers (I assume he was only referring to type 2 helpers, though) was one of the most enlightening texts I read on the whole subject so far (then again, I am fairly new to this list, so I may have missed lots of past goodness). Anyway, it would be nice if this could be augmented by "somebody from the other camp" ;). Cheers, Max -- 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 Wed, Nov 21, 2012 at 8:48 PM, Jeff King wrote: > On Tue, Nov 20, 2012 at 09:08:36PM -0800, Junio C Hamano wrote: > >> With such a one-sided discussion, I've been having a hard time >> convincing myself if Felipe's effort is making the interface better, >> or just breaking it even more for existing remote helpers, only to >> fit his world model better. > > Felipe responded in more detail, but I will just add the consensus we > came to earlier in the discussion: the series does make things better > for users of fast-export that use marks, but does not make things any > better for users of negative refs on the command line. However, I do not > think that it makes things worse for them, either (neither by changing > the behavior negatively, nor by making the code harder for a more > complete fix later). Patch 14 changes the behavior depending on the marks, patch 15 doesn't. This patch is mostly orthogonal to marks. Before without marks: % git branch unintresting master % git fast-export master ^uninteresting reset refs/heads/uninteresting from :0 Before with marks: % git fast-export --import-marks=marks master ^uninteresting reset refs/heads/uninteresting from :100 See? In both cases git is doing something the user doesn't want, nor specified. After my patch nothing gets updated, because nothing was specified to be updated. I'm not going to bother explaining why other people objected to this patch (again), which is indeed related to marks, they should do it for themselves. Let me reaffirm that no valid reason has put forward to object to this patch. > So while fixing everybody might be nice I would like to understand that that even means. What behavior is currently broken? And for who? And how is this patch related to that? > [1] There are other possible use cases for fast-export which might > benefit from negative refs working more sanely, but since they are > in the minority and are not being made worse, I think the partial > fix is OK. Which ones? I don't think this is a partial fix. Nobody has put forward such a use-case. Cheers. -- Felipe Contreras -- 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 Wed, Nov 21, 2012 at 11:30 PM, Max Horn wrote: > 2) Some are interfaces to foreign systems (bzr, hg, mediawiki, ...). They > cannot use sha1s and must use marks (at least that is how I understand > felipe's explanation). These tools use import combined with either export, or > push. Examples: > > - git-remote-mediawiki: import, push, refspec > (its capabilities command also prints "list", but that seems to be a bug?) I don't think remote mediawiki uses marks. They are strictly not needed by 'import' because the remote helper has full control over this operation. For example, in my remote helpers, I store the previous tip of the branches, so when git ask for 'import refs/heads/master', I only import the new commits. I named this file 'marks' anyway, but they are not marks for fast-import. > - git-remote-hg: import, export, refspec, import-marks, export-marks > (both the msysgit one and felipe's > - git-remote-bzr: import, push > (the one from https://github.com/lelutin/git-remote-bzr) > - git-remote-bzr (felipe's): import, export, refspec, *import-marks, > *export-marks > (but why the * ?) AFAIK both of my remote helpers have * in them, they are to denote they are required. > Does that sound about right? If so, can somebody give me a hint when a type 2 > helper would use "export" and when "push"? I don't know when would be appropriate to use push, there's another git-remote-bzr that uses the bzr-git infrastructure, and uses push. I picked export/import because I think they are the easiest to implement to get all the features, and should be efficient. > And while I am at it: git-remote-helpers.txt does not mention the "export", > "import-marks" and "export-marks" capabilities. Could somebody who knows what > they do look into fixing that? Overall, that doc helped me a bit, but it is > more a reference to somebody who already understands in detail how remote > helpers work, and who just wants to look up some specific detail :-(. Some > hints on when to implement which capabilities might be useful (similar to the > "Tips and Tricks" section in git-fast-import.txt). I've had problems finding out the right information, but I hope the git-remote-testgit I wrote in bash in less than 90 lines of code should give a pretty good idea of how the whole thing works. > As it is, felipe's recent explanation on why he thinks marks are essential > for remote-helpers (I assume he was only referring to type 2 helpers, though) > was one of the most enlightening texts I read on the whole subject so far > (then again, I am fairly new to this list, so I may have missed lots of past > goodness). Anyway, it would be nice if this could be augmented by "somebody > from the other camp" ;). Wouldn't that be nice? Cheers. -- Felipe Contreras -- 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 Tue, Nov 20, 2012 at 09:08:36PM -0800, Junio C Hamano wrote: > With such a one-sided discussion, I've been having a hard time > convincing myself if Felipe's effort is making the interface better, > or just breaking it even more for existing remote helpers, only to > fit his world model better. Felipe responded in more detail, but I will just add the consensus we came to earlier in the discussion: the series does make things better for users of fast-export that use marks, but does not make things any better for users of negative refs on the command line. However, I do not think that it makes things worse for them, either (neither by changing the behavior negatively, nor by making the code harder for a more complete fix later). So while fixing everybody might be nice, there is no need to hold up progress for the marks case. Which, as he has noted, is probably the sanest way to implement a remote-helper[1]. -Peff [1] There are other possible use cases for fast-export which might benefit from negative refs working more sanely, but since they are in the minority and are not being made worse, I think the partial fix is OK. -- 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
Felipe Contreras writes: > They have been marked as UNINTERESTING for a reason, lets respect that. > ... > The current behavior is most certainly not what we want. After this > patch, nothing gets exported, because nothing was selected (everything > is UNINTERESTING). The old behaviour was an incorrect "workaround" that has been superseded by your 14/15 "make sure updated refs get updated", no? Mentioning that would help people realize that this patch would not cause regression on them, I would think. -- 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 Wed, Nov 21, 2012 at 6:08 AM, Junio C Hamano wrote: > I see Felipe keeps repeating that there are bugs, and keeps posting > patches to change fast-export, but I haven't seen a concrete "No, > the reason why you see these problems is because you are not using > the interface correctly; the currrent interface is fine. Here is > how you can fix your program" from "others". > > With such a one-sided discussion, I've been having a hard time > convincing myself if Felipe's effort is making the interface better, > or just breaking it even more for existing remote helpers, only to > fit his world model better. IIRC you mentioned something about this mailing list being focused on *technical* merit. I've explained as much as I could, but at the end of the talk, talk is cheap, the code speaks for itself. I added a new very very very simple testgit remote helper, so anybody can see what's going on, and figure out how the interface could be used wrong. Anybody can modify the bash version of git-remote-testgit and say 'no, the interface is not broken, here is how you push and pull without marks'. How hard is it to hack 82 lines of bash code? But lets assume my testgit is fatally broken, would you, Junio, accept these patches if I show the same broken behavior with the python git-remote-testgit? I'm afraid I have to point out the hard truth; the reason why nobody is doing that is because a) the interface is truly broken b) if they try, they most likely would fail, and that would prove they were wrong in previous discussion, or c) not enough familiarity with the code. I don't want to point fingers, nor do I intend to offend anybody, but I cannot find any other explanation of why this patch series, which is obviously correct (to me), doesn't receive any feedback, even though in theory, it should be very very very easy to show what's wrong with the series. The tests are there, and the remote helper is as simple as it gets. There's nothing else but fast-export and transport-helper to blame for the issues. It's that simple. Cheers. -- Felipe Contreras -- 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
Jonathan Nieder writes: > Never mind that others have said that that's not the current interface > (I don't yet see why it would be a good interface after a transition, > but maybe it would be). Still, hopefully that clarifies the intended > meaning. Care to explain how the current interface is supposed to work, how fast-export and transport-helper should interact with remote helpers that adhere to the current interface, and how well/correctly the current implementation of these pieces work? What I am trying to get at is to see where the problem lies. Felipe sees bugs in the aggregated whole. Is the root cause of the problems he sees some breakages in the current interface? Is the interface designed right but the problem is that the implementation of the transport-helper is buggy and driving fast-export incorrectly? Or is the implementation of the fast-export buggy and emitting wrong results, even though the transport-helper is driving fast-export correctly? Something else? I see Felipe keeps repeating that there are bugs, and keeps posting patches to change fast-export, but I haven't seen a concrete "No, the reason why you see these problems is because you are not using the interface correctly; the currrent interface is fine. Here is how you can fix your program" from "others". With such a one-sided discussion, I've been having a hard time convincing myself if Felipe's effort is making the interface better, or just breaking it even more for existing remote helpers, only to fit his world model better. Help? -- 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 Wed, Nov 21, 2012 at 5:17 AM, Jonathan Nieder wrote: > Junio C Hamano wrote: >> Felipe Contreras writes: > >>> Of course, transport-helper shouldn't even be specifying the negative >>> (^) refs, but that's another story. >> >> Hrm, I am not sure I understand what you mean by this. >> >> How should it be telling the fast-export up to what commit the >> receiving end should already have the history for (hence they do not >> need to be sent)? Or are you advocating to re-send the entire >> history down to the root commit every time? > > I think Felipe has mentioned before that he considers it the remote > helper's responsibility to keep track of what commits have already > been imported, for example using a marks file. It's not the remote helper, fast-export does that. > Never mind that others have said that that's not the current interface > (I don't yet see why it would be a good interface after a transition, > but maybe it would be). Still, hopefully that clarifies the intended > meaning. The current interface is broken. not ok 16 - pulling without marks # TODO known breakage not ok 17 - pushing without marks # TODO known breakage See? A remote helper without marks doesn't work. -- Felipe Contreras -- 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
Junio C Hamano wrote: > Felipe Contreras writes: >> Of course, transport-helper shouldn't even be specifying the negative >> (^) refs, but that's another story. > > Hrm, I am not sure I understand what you mean by this. > > How should it be telling the fast-export up to what commit the > receiving end should already have the history for (hence they do not > need to be sent)? Or are you advocating to re-send the entire > history down to the root commit every time? I think Felipe has mentioned before that he considers it the remote helper's responsibility to keep track of what commits have already been imported, for example using a marks file. Never mind that others have said that that's not the current interface (I don't yet see why it would be a good interface after a transition, but maybe it would be). Still, hopefully that clarifies the intended meaning. Hope that helps, Jonathan -- 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
Felipe Contreras writes: > Of course, transport-helper shouldn't even be specifying the negative > (^) refs, but that's another story. Hrm, I am not sure I understand what you mean by this. How should it be telling the fast-export up to what commit the receiving end should already have the history for (hence they do not need to be sent)? Or are you advocating to re-send the entire history down to the root commit every time? -- 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
[PATCH v5 15/15] fast-export: don't handle uninteresting refs
They have been marked as UNINTERESTING for a reason, lets respect that.
Currently the first ref is handled properly, but not the rest, so:
% git fast-export master ^master
Would currently throw a reset for master (2nd ref), which is not what we
want.
% git fast-export master ^foo ^bar ^roo
% git fast-export master salsa..tacos
Even if all these refs point to the same object; foo, bar, roo, salsa,
and tacos would all get a reset, and to a non-existing object (invalid
mark :0).
And even more, it would only happen if the ref is pointing to exactly
the same commit, but not otherwise:
% git fast-export ^next next
reset refs/heads/next
from :0
% git fast-export ^next next^{commit}
# nothing
% git fast-export ^next next~0
# nothing
% git fast-export ^next next~1
# nothing
% git fast-export ^next next~2
# nothing
The reason this happens is that before traversing the commits,
fast-export checks if any of the refs point to the same object, and any
duplicated ref gets added to a list in order to issue 'reset' commands
after the traversing. Unfortunately, it's not even checking if the
commit is flagged as UNINTERESTING. The fix of course, is to do
precisely that.
The current behavior is most certainly not what we want. After this
patch, nothing gets exported, because nothing was selected (everything
is UNINTERESTING).
Signed-off-by: Felipe Contreras
---
builtin/fast-export.c | 4 +++-
t/t9350-fast-export.sh | 6 ++
2 files changed, 9 insertions(+), 1 deletion(-)
diff --git a/builtin/fast-export.c b/builtin/fast-export.c
index 26f6d1c..7a310e4 100644
--- a/builtin/fast-export.c
+++ b/builtin/fast-export.c
@@ -529,7 +529,9 @@ static void get_tags_and_duplicates(struct object_array
*pending,
* sure it gets properly upddated eventually.
*/
if (commit->util || commit->object.flags & SHOWN)
- string_list_append(extra_refs, full_name)->util =
commit;
+ if (!(commit->object.flags & UNINTERESTING))
+ string_list_append(extra_refs, full_name)->util
= commit;
+
if (!commit->util)
commit->util = full_name;
}
diff --git a/t/t9350-fast-export.sh b/t/t9350-fast-export.sh
index 67a7372..9b53ba7 100755
--- a/t/t9350-fast-export.sh
+++ b/t/t9350-fast-export.sh
@@ -469,4 +469,10 @@ test_expect_success 'refs are updated even if no commits
need to be exported' '
test_cmp expected actual
'
+test_expect_success 'proper extra refs handling' '
+ git fast-export master ^master master..master > actual &&
+ echo -n > expected &&
+ test_cmp expected actual
+'
+
test_done
--
1.8.0
--
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
