* [PATCH 0/3] fast-export fixes @ 2011-11-05 23:23 Sverre Rabbelier 2011-11-05 23:23 ` [PATCH 1/3] t9350: point out that refs are not updated correctly Sverre Rabbelier ` (2 more replies) 0 siblings, 3 replies; 42+ messages in thread From: Sverre Rabbelier @ 2011-11-05 23:23 UTC (permalink / raw) To: Junio C Hamano, Jonathan Nieder, Jeff King, Git List, Daniel Barkalow, Ramkumar Cc: Sverre Rabbelier Dscho and I worked on this patch series during a mini-hackathon in late July, but Junio held the series back since he saw a more elegant way to tackle the problem that would pave the way to solve a problem he was having. Since then I've had very little time to work on git, so I was very glad to have the chance to work on this during another mini-hackathon in Amsterdam today. I've used the new rev_info mechanism Junio introduced, and while I can't say I completely understand how the tag_of_filtered_mode bit works, I'm happy to say that all the tests pass now :). Johannes Schindelin (2): fast-export: do not refer to non-existing marks setup_revisions: remember whether a ref was positive or not Sverre Rabbelier (1): t9350: point out that refs are not updated correctly builtin/fast-export.c | 48 ++++++++++++++++++++++++++++++++++++++++++------ t/t9350-fast-export.sh | 11 +++++++++++ 2 files changed, 53 insertions(+), 6 deletions(-) -- 1.7.8.rc0.36.g67522.dirty ^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH 1/3] t9350: point out that refs are not updated correctly 2011-11-05 23:23 [PATCH 0/3] fast-export fixes Sverre Rabbelier @ 2011-11-05 23:23 ` Sverre Rabbelier 2011-11-06 4:31 ` Jonathan Nieder 2012-10-24 17:52 ` Felipe Contreras 2011-11-05 23:23 ` [PATCH 2/3] fast-export: do not refer to non-existing marks Sverre Rabbelier 2011-11-05 23:23 ` [PATCH 3/3] fast-export: output reset command for commandline revs Sverre Rabbelier 2 siblings, 2 replies; 42+ messages in thread From: Sverre Rabbelier @ 2011-11-05 23:23 UTC (permalink / raw) To: Junio C Hamano, Jonathan Nieder, Jeff King, Git List, Daniel Barkalow, Ramkumar Cc: Sverre Rabbelier This happens only when the corresponding commits are not exported in the current fast-export run. This can happen either when the relevant commit is already marked, or when the commit is explicitly marked as UNINTERESTING with a negative ref by another argument. This breaks fast-export based remote helpers, as they use marks files to store which commits have already been seen. The call graph is something as follows: $ # push master to remote repo $ git fast-export --{im,ex}port-marks=marksfile master $ # make a commit on master and push it to remote $ git fast-export --{im,ex}port-marks=marksfile master $ # run `git branch foo` and push it to remote $ git fast-export --{im,ex}port-marks=marksfile foo When fast-export imports the marksfile and sees that all commits in foo are marked as UNINTERESTING (they have already been exported while pushing master), it exits without doing anything. However, what we want is for it to reset 'foo' to the already-exported commit. Either way demonstrates the problem, and since this is the most succint way to demonstrate the problem it is implemented by passing master..master on the commandline. Signed-off-by: Sverre Rabbelier <srabbelier@gmail.com> --- t/t9350-fast-export.sh | 11 +++++++++++ 1 files changed, 11 insertions(+), 0 deletions(-) diff --git a/t/t9350-fast-export.sh b/t/t9350-fast-export.sh index 950d0ff..74914dc 100755 --- a/t/t9350-fast-export.sh +++ b/t/t9350-fast-export.sh @@ -440,4 +440,15 @@ test_expect_success 'fast-export quotes pathnames' ' ) ' +cat > expected << EOF +reset refs/heads/master +from $(git rev-parse master) + +EOF + +test_expect_failure 'refs are updated even if no commits need to be exported' ' + git fast-export master..master > actual && + test_cmp expected actual +' + test_done -- 1.7.8.rc0.36.g67522.dirty ^ permalink raw reply related [flat|nested] 42+ messages in thread
* Re: [PATCH 1/3] t9350: point out that refs are not updated correctly 2011-11-05 23:23 ` [PATCH 1/3] t9350: point out that refs are not updated correctly Sverre Rabbelier @ 2011-11-06 4:31 ` Jonathan Nieder 2011-11-06 19:38 ` Sverre Rabbelier 2012-10-24 17:52 ` Felipe Contreras 1 sibling, 1 reply; 42+ messages in thread From: Jonathan Nieder @ 2011-11-06 4:31 UTC (permalink / raw) To: Sverre Rabbelier Cc: Junio C Hamano, Jeff King, Git List, Daniel Barkalow, Ramkumar Ramachandra, Dmitry Ivankov, Johannes Schindelin, Ævar Arnfjörð Bjarmason, Eric Herman, Fernando Vezzosi Sverre Rabbelier wrote: > This happens only when the corresponding commits are not exported in > the current fast-export run. This can happen either when the relevant > commit is already marked, or when the commit is explicitly marked > as UNINTERESTING with a negative ref by another argument. The above "This" has no antecedent. I guess you mean that fast-export writes no output when passed a range of the form A..A. > This breaks fast-export based remote helpers, Makes sense. > as they use marks > files to store which commits have already been seen. The call graph > is something as follows: > > $ # push master to remote repo > $ git fast-export --{im,ex}port-marks=marksfile master > $ # make a commit on master and push it to remote > $ git fast-export --{im,ex}port-marks=marksfile master > $ # run `git branch foo` and push it to remote > $ git fast-export --{im,ex}port-marks=marksfile foo > > When fast-export imports the marksfile and sees that all commits in > foo are marked as UNINTERESTING Hmm, I didn't know about this behavior. Would it be possible to add a test for it, too? > t/t9350-fast-export.sh | 11 +++++++++++ > 1 files changed, 11 insertions(+), 0 deletions(-) With or without the change suggested above, this new test seems to me like a good thing, even though in the longer term it might be nicer to teach fast-export to understand a syntax like git fast-import ^master master:master Put another way, the possibility of something nicer later shouldn't stop us from adding an incremental refinement that improves things today. Thanks for working on this, Jonathan ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 1/3] t9350: point out that refs are not updated correctly 2011-11-06 4:31 ` Jonathan Nieder @ 2011-11-06 19:38 ` Sverre Rabbelier 2011-11-07 9:32 ` Jonathan Nieder 0 siblings, 1 reply; 42+ messages in thread From: Sverre Rabbelier @ 2011-11-06 19:38 UTC (permalink / raw) To: Jonathan Nieder Cc: Junio C Hamano, Jeff King, Git List, Daniel Barkalow, Ramkumar Ramachandra, Dmitry Ivankov, Johannes Schindelin, Ævar Arnfjörð, Eric Herman, Fernando Vezzosi Heya, On Sun, Nov 6, 2011 at 05:31, Jonathan Nieder <jrnieder@gmail.com> wrote: >> This happens only when the corresponding commits are not exported in >> the current fast-export run. This can happen either when the relevant >> commit is already marked, or when the commit is explicitly marked >> as UNINTERESTING with a negative ref by another argument. > > The above "This" has no antecedent. I guess you mean that > fast-export writes no output when passed a range of the form A..A. Well, it's referring to the subject, how about: "When a commit has not been exported in the current fast-export run its ref is not updated correctly. This can happen ...". >> as they use marks >> files to store which commits have already been seen. The call graph >> is something as follows: >> >> $ # push master to remote repo >> $ git fast-export --{im,ex}port-marks=marksfile master >> $ # make a commit on master and push it to remote >> $ git fast-export --{im,ex}port-marks=marksfile master >> $ # run `git branch foo` and push it to remote >> $ git fast-export --{im,ex}port-marks=marksfile foo >> >> When fast-export imports the marksfile and sees that all commits in >> foo are marked as UNINTERESTING > > Hmm, I didn't know about this behavior. Would it be possible to add > a test for it, too? What behavior are you referring to here? What kind of test would you want added? >> t/t9350-fast-export.sh | 11 +++++++++++ >> 1 files changed, 11 insertions(+), 0 deletions(-) > > With or without the change suggested above, this new test seems to me > like a good thing, even though in the longer term it might be nicer to > teach fast-export to understand a syntax like > > git fast-import ^master master:master > > Put another way, the possibility of something nicer later shouldn't > stop us from adding an incremental refinement that improves things > today. Yes, extending the capabilities of fast-export is needed if we want the remote-helpers to be as powerful as native git. -- Cheers, Sverre Rabbelier ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 1/3] t9350: point out that refs are not updated correctly 2011-11-06 19:38 ` Sverre Rabbelier @ 2011-11-07 9:32 ` Jonathan Nieder 0 siblings, 0 replies; 42+ messages in thread From: Jonathan Nieder @ 2011-11-07 9:32 UTC (permalink / raw) To: Sverre Rabbelier Cc: Junio C Hamano, Jeff King, Git List, Daniel Barkalow, Ramkumar Ramachandra, Dmitry Ivankov, Johannes Schindelin, Ævar Arnfjörð, Eric Herman, Fernando Vezzosi Sverre Rabbelier wrote: > On Sun, Nov 6, 2011 at 05:31, Jonathan Nieder <jrnieder@gmail.com> wrote: >>> as they use marks >>> files to store which commits have already been seen. The call graph >>> is something as follows: [...] >>> $ # run `git branch foo` and push it to remote >>> $ git fast-export --{im,ex}port-marks=marksfile foo >>> >>> When fast-export imports the marksfile and sees that all commits in >>> foo are marked as UNINTERESTING >> >> Hmm, I didn't know about this behavior. Would it be possible to add >> a test for it, too? > > What behavior are you referring to here? What kind of test would you want added? I meant I hadn't remembered that marks result in commits being marked as UNINTERESTING (even though the manpage warned me), and that it's possible a priori that fast-export could be broken when you run git fast-export --import-marks=marksfile master even without breaking git fast-export master..master But don't worry about it --- I can try it as a follow-on when this series next visits the list if that doesn't sound like fun. :) FWIW, with the clarifications to the commit message Junio made, I'm happy with this patch. ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 1/3] t9350: point out that refs are not updated correctly 2011-11-05 23:23 ` [PATCH 1/3] t9350: point out that refs are not updated correctly Sverre Rabbelier 2011-11-06 4:31 ` Jonathan Nieder @ 2012-10-24 17:52 ` Felipe Contreras 2012-10-24 18:08 ` Jonathan Nieder 2012-10-24 21:41 ` Johannes Schindelin 1 sibling, 2 replies; 42+ messages in thread From: Felipe Contreras @ 2012-10-24 17:52 UTC (permalink / raw) To: Sverre Rabbelier Cc: Junio C Hamano, Jonathan Nieder, Jeff King, Git List, Daniel Barkalow, Ramkumar Ramachandra, Dmitry Ivankov, Johannes Schindelin, Ævar Arnfjörð Bjarmason, Eric Herman, Fernando Vezzosi Hi, Joined late to the party :) On Sun, Nov 6, 2011 at 12:23 AM, Sverre Rabbelier <srabbelier@gmail.com> wrote: > This happens only when the corresponding commits are not exported in > the current fast-export run. This can happen either when the relevant > commit is already marked, or when the commit is explicitly marked > as UNINTERESTING with a negative ref by another argument. > > This breaks fast-export based remote helpers, as they use marks > files to store which commits have already been seen. The call graph > is something as follows: > > $ # push master to remote repo > $ git fast-export --{im,ex}port-marks=marksfile master > $ # make a commit on master and push it to remote > $ git fast-export --{im,ex}port-marks=marksfile master > $ # run `git branch foo` and push it to remote > $ git fast-export --{im,ex}port-marks=marksfile foo That is correctly, but try this: $ git fast-export --{im,ex}port-marks=marksfile foo foo Now foo is updated. > When fast-export imports the marksfile and sees that all commits in > foo are marked as UNINTERESTING (they have already been exported > while pushing master), it exits without doing anything. However, > what we want is for it to reset 'foo' to the already-exported commit. > > Either way demonstrates the problem, and since this is the most > succint way to demonstrate the problem it is implemented by passing > master..master on the commandline. > > Signed-off-by: Sverre Rabbelier <srabbelier@gmail.com> > --- > t/t9350-fast-export.sh | 11 +++++++++++ > 1 files changed, 11 insertions(+), 0 deletions(-) > > diff --git a/t/t9350-fast-export.sh b/t/t9350-fast-export.sh > index 950d0ff..74914dc 100755 > --- a/t/t9350-fast-export.sh > +++ b/t/t9350-fast-export.sh > @@ -440,4 +440,15 @@ test_expect_success 'fast-export quotes pathnames' ' > ) > ' > > +cat > expected << EOF > +reset refs/heads/master > +from $(git rev-parse master) > + > +EOF > + > +test_expect_failure 'refs are updated even if no commits need to be exported' ' > + git fast-export master..master > actual && > + test_cmp expected actual > +' > + > test_done This test is completely wrong. 1) Where are the marks file? 2) master..master shouldn't export anything 3) Why do you expect a SHA-1? It could be a mark. I decided to write my own this way: --- cat > expected << EOF reset refs/heads/master from ##mark## EOF test_expect_failure 'refs are updated even if no commits need to be exported' ' cp tmp-marks /tmp git fast-export --import-marks=tmp-marks \ --export-marks=tmp-marks master | true && git fast-export --import-marks=tmp-marks \ --export-marks=tmp-marks master > actual && mark=$(grep $(git rev-parse master) tmp-marks | cut -f 1 -d " ") sed -i -e "s/##mark##/$mark/" expected && test_cmp expected actual ' --- Yes, it's true this fails, but change to 'master master', and then it works. This can be easily fixed by this patch: diff --git a/builtin/fast-export.c b/builtin/fast-export.c index 12220ad..3b4c2d6 100644 --- a/builtin/fast-export.c +++ b/builtin/fast-export.c @@ -523,10 +523,13 @@ 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 updated eventually. + */ + if (commit->util || commit->object.flags & SHOWN) string_list_append(extra_refs, full_name)->util = commit; - else + if (!commit->util) commit->util = full_name; } } Now if you specify a ref it will get updated regardless. However, this points to another bug: % git fast-export --{im,ex}port-marks=/tmp/marks master ^foo foo.foo The foo ref will be reset _twice_ because all pending refs after the first one get reset no matter how they were specified. 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. Cheers. -- Felipe Contreras ^ permalink raw reply related [flat|nested] 42+ messages in thread
* Re: [PATCH 1/3] t9350: point out that refs are not updated correctly 2012-10-24 17:52 ` Felipe Contreras @ 2012-10-24 18:08 ` Jonathan Nieder 2012-10-24 19:09 ` Felipe Contreras 2012-10-24 21:41 ` Johannes Schindelin 1 sibling, 1 reply; 42+ messages in thread From: Jonathan Nieder @ 2012-10-24 18:08 UTC (permalink / raw) To: Felipe Contreras Cc: Sverre Rabbelier, Junio C Hamano, Jeff King, Git List, Daniel Barkalow, Ramkumar Ramachandra, Dmitry Ivankov, Johannes Schindelin, Ævar Arnfjörð Bjarmason, Eric Herman, Fernando Vezzosi Hi Felipe, Felipe Contreras wrote: > This test is completely wrong. > > 1) Where are the marks file? > 2) master..master shouldn't export anything Why shouldn't master..master export anything? It means "update the master ref; we already have all commits up to and including master^0". The underlying problem is that fast-export takes rev-list arguments as parameters, which is unfortunately only an approximation to what is really intended. Ideally it would separately take a list of refs to import and rev-list arguments representing the commits we already have. Hoping that clarifies, Jonathan ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 1/3] t9350: point out that refs are not updated correctly 2012-10-24 18:08 ` Jonathan Nieder @ 2012-10-24 19:09 ` Felipe Contreras 2012-10-24 19:11 ` Jonathan Nieder 0 siblings, 1 reply; 42+ messages in thread From: Felipe Contreras @ 2012-10-24 19:09 UTC (permalink / raw) To: Jonathan Nieder Cc: Sverre Rabbelier, Junio C Hamano, Jeff King, Git List, Daniel Barkalow, Ramkumar Ramachandra, Dmitry Ivankov, Johannes Schindelin, Ævar Arnfjörð, Eric Herman, Fernando Vezzosi On Wed, Oct 24, 2012 at 8:08 PM, Jonathan Nieder <jrnieder@gmail.com> wrote: > Hi Felipe, > > Felipe Contreras wrote: > >> This test is completely wrong. >> >> 1) Where are the marks file? >> 2) master..master shouldn't export anything > > Why shouldn't master..master export anything? It means "update the > master ref; we already have all commits up to and including master^0". Does it mean that? I don't think so, but let's assume that's the case. We don't have all those commits; without the marks we have nothing. Or what exactly do you mean by 'we'? Go to your git.git repository, and run this: % git git init /tmp/git % git fast-export master^..master | git --git-dir=/tmp/git/.git fast-import What do you expect? I expect a single commit, and that's what we get, now do the same with 'master..master', what do you expect? How about 'git fast-export ^master'? Do you expect to get anything there? Or what about '^master master'? Without marks these idioms don't make any sense. Now lets assume that marks were meant to be there. If 'master..master' is supposed to update master, then what is 'master' supposed to do? % git fast-export --{im,ex}port-marks=/tmp/marks master..master vs. % git fast-export --{im,ex}port-marks=/tmp/marks master Either way, my patch will make 'master..master' throw a reset (if the marks are present, I haven't tried without them), I don't think it should, but that's a different story, and a different patch fix. > The underlying problem is that fast-export takes rev-list arguments as > parameters, which is unfortunately only an approximation to what is > really intended. Ideally it would separately take a list of refs to > import and rev-list arguments representing the commits we already > have. The commits we already have (exported before) are stored in the marks. Maybe we can store the refs there as well, but that would not change the semantics of refspecs, nor the fact that we need the marks. Cheers. -- Felipe Contreras ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 1/3] t9350: point out that refs are not updated correctly 2012-10-24 19:09 ` Felipe Contreras @ 2012-10-24 19:11 ` Jonathan Nieder 2012-10-25 4:19 ` Felipe Contreras 0 siblings, 1 reply; 42+ messages in thread From: Jonathan Nieder @ 2012-10-24 19:11 UTC (permalink / raw) To: Felipe Contreras Cc: Sverre Rabbelier, Junio C Hamano, Jeff King, Git List, Daniel Barkalow, Ramkumar Ramachandra, Dmitry Ivankov, Johannes Schindelin, Ævar Arnfjörð, Eric Herman, Fernando Vezzosi Felipe Contreras wrote: > Does it mean that? I don't think so, but let's assume that's the case. > > We don't have all those commits; without the marks we have nothing. Or > what exactly do you mean by 'we'? Not everyone uses marks. Ciao, Jonathan ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 1/3] t9350: point out that refs are not updated correctly 2012-10-24 19:11 ` Jonathan Nieder @ 2012-10-25 4:19 ` Felipe Contreras 2012-10-25 4:27 ` Jonathan Nieder 0 siblings, 1 reply; 42+ messages in thread From: Felipe Contreras @ 2012-10-25 4:19 UTC (permalink / raw) To: Jonathan Nieder Cc: Sverre Rabbelier, Junio C Hamano, Jeff King, Git List, Daniel Barkalow, Ramkumar Ramachandra, Dmitry Ivankov, Johannes Schindelin, Ævar Arnfjörð, Eric Herman, Fernando Vezzosi On Wed, Oct 24, 2012 at 9:11 PM, Jonathan Nieder <jrnieder@gmail.com> wrote: > Felipe Contreras wrote: > >> Does it mean that? I don't think so, but let's assume that's the case. >> >> We don't have all those commits; without the marks we have nothing. Or >> what exactly do you mean by 'we'? > > Not everyone uses marks. When you don't have marks you have to export *everything* that you are interested. If you want all the history from the root to master, then that's what you will get (and you specify 'master'), if you want only the commit pointed to master and nothing else that's what you will get (with 'master^..master'), but when you do 'master..master', you get nothing, because that's what you asked for. Again, if you don't have marks, I don't see what you expect to be exported with 'master..master', even with marks, I don't see what you expect. -- Felipe Contreras ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 1/3] t9350: point out that refs are not updated correctly 2012-10-25 4:19 ` Felipe Contreras @ 2012-10-25 4:27 ` Jonathan Nieder 2012-10-25 5:18 ` Felipe Contreras 0 siblings, 1 reply; 42+ messages in thread From: Jonathan Nieder @ 2012-10-25 4:27 UTC (permalink / raw) To: Felipe Contreras Cc: Sverre Rabbelier, Junio C Hamano, Jeff King, Git List, Daniel Barkalow, Ramkumar Ramachandra, Dmitry Ivankov, Johannes Schindelin, Ævar Arnfjörð, Eric Herman, Fernando Vezzosi Felipe Contreras wrote: > Again, if you don't have marks, I don't see what you expect to be > exported with 'master..master', even with marks, I don't see what you > expect. And that's fine. Unless you were trying to do some work and this lack of understanding got in the way. In that case, with a calmer and more humble approach you might find people willing to help you. Maybe they will learn something from you, too. Ciao, Jonathan ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 1/3] t9350: point out that refs are not updated correctly 2012-10-25 4:27 ` Jonathan Nieder @ 2012-10-25 5:18 ` Felipe Contreras 2012-10-25 5:28 ` Jonathan Nieder 0 siblings, 1 reply; 42+ messages in thread From: Felipe Contreras @ 2012-10-25 5:18 UTC (permalink / raw) To: Jonathan Nieder Cc: Sverre Rabbelier, Junio C Hamano, Jeff King, Git List, Daniel Barkalow, Ramkumar Ramachandra, Dmitry Ivankov, Johannes Schindelin, Ævar Arnfjörð, Eric Herman, Fernando Vezzosi On Thu, Oct 25, 2012 at 6:27 AM, Jonathan Nieder <jrnieder@gmail.com> wrote: > Felipe Contreras wrote: > >> Again, if you don't have marks, I don't see what you expect to be >> exported with 'master..master', even with marks, I don't see what you >> expect. > > And that's fine. Unless you were trying to do some work and this lack > of understanding got in the way. What is fine? What lack of understanding? You still haven't said what you expect the output to be. Consider this repo: --- git init test cd test echo one >> file git add --all git commit -m 'one' echo two >> file git commit -m 'one' --- What *exactly* should the output of 'git fast-export master..master' be? I say nothing, what do you say? > In that case, with a calmer and more humble approach you might find > people willing to help you. Maybe they will learn something from you, > too. I don't need help, I am helping you, I was asked to take a look at this patch series. If you don't want my help, then by all means, keep this series rotting, it has being doing so for the past year without anybody complaining. Cheers. -- Felipe Contreras ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 1/3] t9350: point out that refs are not updated correctly 2012-10-25 5:18 ` Felipe Contreras @ 2012-10-25 5:28 ` Jonathan Nieder 2012-10-25 5:39 ` Sverre Rabbelier 2012-10-25 5:40 ` Felipe Contreras 0 siblings, 2 replies; 42+ messages in thread From: Jonathan Nieder @ 2012-10-25 5:28 UTC (permalink / raw) To: Felipe Contreras Cc: Sverre Rabbelier, Junio C Hamano, Jeff King, Git List, Daniel Barkalow, Ramkumar Ramachandra, Dmitry Ivankov, Johannes Schindelin, Ævar Arnfjörð, Eric Herman, Fernando Vezzosi Felipe Contreras wrote: > I don't need help, I am helping you, I was asked to take a look at > this patch series. If you don't want my help, then by all means, keep > this series rotting, it has being doing so for the past year without > anybody complaining. Ah, so _that_ (namely getting Sverre's remote helper to work) is the work you were trying to do. Thanks for explaining. If I understand correctly, it is possible to get Sverre's remote helper to work without affecting this particular testcase. From that point of view I think you were on the right track. 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. Thanks, Jonathan ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 1/3] t9350: point out that refs are not updated correctly 2012-10-25 5:28 ` Jonathan Nieder @ 2012-10-25 5:39 ` Sverre Rabbelier 2012-10-25 5:50 ` Felipe Contreras 2012-10-25 5:40 ` Felipe Contreras 1 sibling, 1 reply; 42+ messages in thread From: Sverre Rabbelier @ 2012-10-25 5:39 UTC (permalink / raw) To: Jonathan Nieder Cc: Felipe Contreras, Junio C Hamano, Jeff King, Git List, Daniel Barkalow, Ramkumar Ramachandra, Dmitry Ivankov, Johannes Schindelin, Ævar Arnfjörð, Eric Herman, Fernando Vezzosi On Wed, Oct 24, 2012 at 10:28 PM, Jonathan Nieder <jrnieder@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 ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 1/3] t9350: point out that refs are not updated correctly 2012-10-25 5:39 ` Sverre Rabbelier @ 2012-10-25 5:50 ` Felipe Contreras 2012-10-25 6:07 ` Sverre Rabbelier 0 siblings, 1 reply; 42+ messages in thread From: Felipe Contreras @ 2012-10-25 5:50 UTC (permalink / raw) To: Sverre Rabbelier Cc: Jonathan Nieder, Junio C Hamano, Jeff King, Git List, Daniel Barkalow, Ramkumar Ramachandra, Dmitry Ivankov, Johannes Schindelin, Ævar Arnfjörð, Eric Herman, Fernando Vezzosi On Thu, Oct 25, 2012 at 7:39 AM, Sverre Rabbelier <srabbelier@gmail.com> wrote: > On Wed, Oct 24, 2012 at 10:28 PM, Jonathan Nieder <jrnieder@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). Only if the remote helper is using marks, and this particular patch is adding a test-case without any use of marks at all. IOW; this test is testing something completely different, which happens to fix the original issue, but this is not the only way to fix, and in IMO certainly not the best. As I showed in my script above: $ git checkout master $ git branch next $ git push hg://example.com master $ git push hg://example.com next This works just fine. Go ahead, apply my patch, and run it, the second branch gets updated. It will fail this test, but that's because the test is not testing what it should: that *when using marks* the second branch exported is ignored. This test does that: --- cat > expected << EOF reset refs/heads/master from ##mark## EOF test_expect_failure 'refs are updated even if no commits need to be exported' ' cp tmp-marks /tmp git fast-export --import-marks=tmp-marks \ --export-marks=tmp-marks master | true && git fast-export --import-marks=tmp-marks \ --export-marks=tmp-marks master > actual && mark=$(grep $(git rev-parse master) tmp-marks | cut -f 1 -d " ") sed -i -e "s/##mark##/$mark/" expected && test_cmp expected actual ' --- -- Felipe Contreras ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 1/3] t9350: point out that refs are not updated correctly 2012-10-25 5:50 ` Felipe Contreras @ 2012-10-25 6:07 ` Sverre Rabbelier 2012-10-25 6:19 ` Felipe Contreras 0 siblings, 1 reply; 42+ messages in thread From: Sverre Rabbelier @ 2012-10-25 6:07 UTC (permalink / raw) To: Felipe Contreras Cc: Jonathan Nieder, Junio C Hamano, Jeff King, Git List, Daniel Barkalow, Ramkumar Ramachandra, Dmitry Ivankov, Johannes Schindelin, Ævar Arnfjörð, Eric Herman, Fernando Vezzosi On Wed, Oct 24, 2012 at 10:50 PM, Felipe Contreras <felipe.contreras@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 ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 1/3] t9350: point out that refs are not updated correctly 2012-10-25 6:07 ` Sverre Rabbelier @ 2012-10-25 6:19 ` Felipe Contreras 2012-10-25 7:06 ` Sverre Rabbelier 0 siblings, 1 reply; 42+ messages in thread From: Felipe Contreras @ 2012-10-25 6:19 UTC (permalink / raw) To: Sverre Rabbelier Cc: Jonathan Nieder, Junio C Hamano, Jeff King, Git List, Daniel Barkalow, Ramkumar Ramachandra, Dmitry Ivankov, Johannes Schindelin, Ævar Arnfjörð, Eric Herman, Fernando Vezzosi On Thu, Oct 25, 2012 at 8:07 AM, Sverre Rabbelier <srabbelier@gmail.com> wrote: > On Wed, Oct 24, 2012 at 10:50 PM, Felipe Contreras > <felipe.contreras@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. 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 This is with mine: % ./git fast-export --{im,ex}port-marks=/tmp/marks foo1 ^foo2 foo3..foo3 reset refs/heads/foo3 from :21 reset refs/heads/foo2 from :21 reset refs/heads/foo1 from :21 Now tell me again. What is the benefit of your approach? > 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. If 'git fast-export ^master foo' is catastrophic, so is 'git fast-export foo ^master', and that already exports master *today*. > 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. Which is something few users will notice. What they surely notice is that there's no remote-hg they can readily use. Nobody expects all software to be perfect or have all the features from day 1. Something that just fetches a hg repo is already better than the current situation: *nothing*. And BTW, in mercurial a commit can be only on one branch anyway, so you can't have 'foo' and 'master' both pointing to the same commit/revision. Sure bookmarks is another story, but again, I don't think people would prefer remote-hg to stay out because bookmarks don't work _perfectly_. Cheers. -- Felipe Contreras ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 1/3] t9350: point out that refs are not updated correctly 2012-10-25 6:19 ` Felipe Contreras @ 2012-10-25 7:06 ` Sverre Rabbelier 2012-10-25 7:34 ` Jonathan Nieder 0 siblings, 1 reply; 42+ messages in thread From: Sverre Rabbelier @ 2012-10-25 7:06 UTC (permalink / raw) To: Felipe Contreras Cc: Jonathan Nieder, Junio C Hamano, Jeff King, Git List, Daniel Barkalow, Ramkumar Ramachandra, Dmitry Ivankov, Johannes Schindelin, Ævar Arnfjörð, Eric Herman, Fernando Vezzosi On Wed, Oct 24, 2012 at 11:19 PM, Felipe Contreras <felipe.contreras@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 ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 1/3] t9350: point out that refs are not updated correctly 2012-10-25 7:06 ` Sverre Rabbelier @ 2012-10-25 7:34 ` Jonathan Nieder 2012-10-25 7:43 ` Sverre Rabbelier 0 siblings, 1 reply; 42+ messages in thread From: Jonathan Nieder @ 2012-10-25 7:34 UTC (permalink / raw) To: Sverre Rabbelier Cc: Felipe Contreras, Junio C Hamano, Jeff King, Git List, Daniel Barkalow, Ramkumar Ramachandra, Dmitry Ivankov, Johannes Schindelin, Ævar Arnfjörð, Eric Herman, Fernando Vezzosi Sverre Rabbelier wrote: > 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. 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. ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 1/3] t9350: point out that refs are not updated correctly 2012-10-25 7:34 ` Jonathan Nieder @ 2012-10-25 7:43 ` Sverre Rabbelier 2012-10-25 7:48 ` Jonathan Nieder 0 siblings, 1 reply; 42+ messages in thread From: Sverre Rabbelier @ 2012-10-25 7:43 UTC (permalink / raw) To: Jonathan Nieder Cc: Felipe Contreras, Junio C Hamano, Jeff King, Git List, Daniel Barkalow, Ramkumar Ramachandra, Dmitry Ivankov, Johannes Schindelin, Ævar Arnfjörð, Eric Herman, Fernando Vezzosi On Thu, Oct 25, 2012 at 12:34 AM, Jonathan Nieder <jrnieder@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 ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 1/3] t9350: point out that refs are not updated correctly 2012-10-25 7:43 ` Sverre Rabbelier @ 2012-10-25 7:48 ` Jonathan Nieder 2012-10-25 7:50 ` Sverre Rabbelier 0 siblings, 1 reply; 42+ messages in thread From: Jonathan Nieder @ 2012-10-25 7:48 UTC (permalink / raw) To: Sverre Rabbelier Cc: Felipe Contreras, Junio C Hamano, Jeff King, Git List, Daniel Barkalow, Ramkumar Ramachandra, Dmitry Ivankov, Johannes Schindelin, Ævar Arnfjörð, Eric Herman, Fernando Vezzosi 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? ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 1/3] t9350: point out that refs are not updated correctly 2012-10-25 7:48 ` Jonathan Nieder @ 2012-10-25 7:50 ` Sverre Rabbelier 2012-10-25 13:33 ` Felipe Contreras 0 siblings, 1 reply; 42+ messages in thread From: Sverre Rabbelier @ 2012-10-25 7:50 UTC (permalink / raw) To: Jonathan Nieder, Johannes Schindelin Cc: Felipe Contreras, Junio C Hamano, Jeff King, Git List, Daniel Barkalow, Ramkumar Ramachandra, Dmitry Ivankov, Ævar Arnfjörð, Eric Herman, Fernando Vezzosi On Thu, Oct 25, 2012 at 12:48 AM, Jonathan Nieder <jrnieder@gmail.com> 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 ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 1/3] t9350: point out that refs are not updated correctly 2012-10-25 7:50 ` Sverre Rabbelier @ 2012-10-25 13:33 ` Felipe Contreras 0 siblings, 0 replies; 42+ messages in thread From: Felipe Contreras @ 2012-10-25 13:33 UTC (permalink / raw) To: Sverre Rabbelier Cc: Jonathan Nieder, Johannes Schindelin, Junio C Hamano, Jeff King, Git List, Daniel Barkalow, Ramkumar Ramachandra, Dmitry Ivankov, Ævar Arnfjörð, Eric Herman, Fernando Vezzosi On Thu, Oct 25, 2012 at 9:50 AM, Sverre Rabbelier <srabbelier@gmail.com> wrote: > On Thu, Oct 25, 2012 at 12:48 AM, Jonathan Nieder <jrnieder@gmail.com> 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? It is possible to check in revs->pending, but '^foo master' will mark them both as UNINTERESTING, and 'master..master' as well, which again, is what we actually want, because that's how it works in the rest of git. > That might work, maybe Dscho remembers why we did not go with that approach. Because you want 'master..master' to output something, but that's wrong; it's changing the semantics of commitishes, and you don't need that to solve this problem. -- Felipe Contreras ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 1/3] t9350: point out that refs are not updated correctly 2012-10-25 5:28 ` Jonathan Nieder 2012-10-25 5:39 ` Sverre Rabbelier @ 2012-10-25 5:40 ` Felipe Contreras 2012-10-25 5:53 ` Jonathan Nieder 1 sibling, 1 reply; 42+ messages in thread From: Felipe Contreras @ 2012-10-25 5:40 UTC (permalink / raw) To: Jonathan Nieder Cc: Sverre Rabbelier, Junio C Hamano, Jeff King, Git List, Daniel Barkalow, Ramkumar Ramachandra, Dmitry Ivankov, Johannes Schindelin, Ævar Arnfjörð, Eric Herman, Fernando Vezzosi On Thu, Oct 25, 2012 at 7:28 AM, Jonathan Nieder <jrnieder@gmail.com> wrote: > Felipe Contreras wrote: > >> I don't need help, I am helping you, I was asked to take a look at >> this patch series. If you don't want my help, then by all means, keep >> this series rotting, it has being doing so for the past year without >> anybody complaining. > > Ah, so _that_ (namely getting Sverre's remote helper to work) is the > work you were trying to do. Thanks for explaining. No, that's not what I'm doing. I haven't even seen a remote-hg branch from either Sverre, or Johannes. IIRC the msysgit wiki mentions that there were some patches not quite accepted in upstream that prevented the remote-hg from getting upstream. But I don't know which patches are those, I don't know why they are needed, and I haven't even been able to run this stuff. I was told this might be an issue for all remote helpers, and it seems to be the case (albeit a small issue IMO). > If I understand correctly, it is possible to get Sverre's remote > helper to work without affecting this particular testcase. From that > point of view I think you were on the right track. That makes sense. So are there any other patches? > 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. All right, so I run this and get this: % git fast-export master..master reset refs/heads/master from 8c7a786b6c8eae8eac91083cdc9a6e337bc133b0 As an user of fast-export, what do I do with that now? -- Felipe Contreras ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 1/3] t9350: point out that refs are not updated correctly 2012-10-25 5:40 ` Felipe Contreras @ 2012-10-25 5:53 ` Jonathan Nieder 2012-10-25 6:39 ` Felipe Contreras 0 siblings, 1 reply; 42+ messages in thread From: Jonathan Nieder @ 2012-10-25 5:53 UTC (permalink / raw) To: Felipe Contreras Cc: Sverre Rabbelier, Junio C Hamano, Jeff King, Git List, Daniel Barkalow, Ramkumar Ramachandra, Dmitry Ivankov, Johannes Schindelin, Ævar Arnfjörð, Eric Herman, Fernando Vezzosi Felipe Contreras wrote: > All right, so I run this and get this: > > % git fast-export master..master > reset refs/heads/master > from 8c7a786b6c8eae8eac91083cdc9a6e337bc133b0 > > As an user of fast-export, what do I do with that now? You passed "master.." on the command line, indicating that your repository already has commit 8c7a786b6c8eae8eac91083cdc9a6e337bc133b0. Now you can update the "master" branch to point to that commit, as the fast-export output indicates. Jonathan ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 1/3] t9350: point out that refs are not updated correctly 2012-10-25 5:53 ` Jonathan Nieder @ 2012-10-25 6:39 ` Felipe Contreras 2012-10-25 7:18 ` Jonathan Nieder 0 siblings, 1 reply; 42+ messages in thread From: Felipe Contreras @ 2012-10-25 6:39 UTC (permalink / raw) To: Jonathan Nieder Cc: Sverre Rabbelier, Junio C Hamano, Jeff King, Git List, Daniel Barkalow, Ramkumar Ramachandra, Dmitry Ivankov, Johannes Schindelin, Ævar Arnfjörð, Eric Herman, Fernando Vezzosi On Thu, Oct 25, 2012 at 7:53 AM, Jonathan Nieder <jrnieder@gmail.com> wrote: > Felipe Contreras wrote: > >> All right, so I run this and get this: >> >> % git fast-export master..master >> reset refs/heads/master >> from 8c7a786b6c8eae8eac91083cdc9a6e337bc133b0 >> >> As an user of fast-export, what do I do with that now? > > You passed "master.." on the command line, indicating that your > repository already has commit 8c7a786b6c8eae8eac91083cdc9a6e337bc133b0. No I didn't. Maybe I'm not interested in all the old history and I just want to create a repository from that point forward. For example 'git fast-export v1.5.0..master. I don't want no references to objects I don't have, there's no way I can do anything sensible with that SHA-1. % git fast-export master..master | git --git-dir=/tmp/git/.git fast-import fatal: Not a valid commit: 8c7a786b6c8eae8eac91083cdc9a6e337bc133b0 fast-import: dumping crash report to /tmp/git/.git/fast_import_crash_32498 Does it make sense to you that the output of fast-export doesn't work with fast-import? > Now you can update the "master" branch to point to that commit, > as the fast-export output indicates. I don't have that commit, I don't even know what 8c7a786 means. Show me a single remote helper that manually stores SHA-1's and I might believe you, but I doubt that, marks are too convenient. Or show me a script. I doubt there will be any, because otherwise somebody would have pushed for this patch, and there doesn't seem to be too many people. But fine, lets assume it's a valid use-case and people need this... it still has absolutely nothing to do with the original intent of the patch series. The series is in fact doing two things: 1) Use SHA-1's when a mark can't be found 2) Update refs that have been already visited (through marks) These two things are orthogonal to each other, we should have two tests, and in fact, two separate patch series. One will be useful for remote helpers, the other one will be useful to nobody IMO, but that's something that can be discussed there, and I particularly don't care. My test and my patch are good for 2), and so far I haven't seen anybody saying otherwise. Cheers. -- Felipe Contreras ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 1/3] t9350: point out that refs are not updated correctly 2012-10-25 6:39 ` Felipe Contreras @ 2012-10-25 7:18 ` Jonathan Nieder 2012-10-25 16:43 ` Felipe Contreras 0 siblings, 1 reply; 42+ messages in thread From: Jonathan Nieder @ 2012-10-25 7:18 UTC (permalink / raw) To: Felipe Contreras Cc: Sverre Rabbelier, Junio C Hamano, Jeff King, Git List, Daniel Barkalow, Ramkumar Ramachandra, Dmitry Ivankov, Johannes Schindelin, Ævar Arnfjörð, Eric Herman, Fernando Vezzosi Felipe Contreras wrote: > Show me a single remote helper that manually stores SHA-1's and I > might believe you, but I doubt that, marks are too convenient. Oh dear lord. Why are you arguing? Explain how coming to a consensus on this will help accomplish something useful, and then I can explain my point of view. In the meantime, this seems like a waste of time. Let's agree to disagree. Regards, Jonathan ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 1/3] t9350: point out that refs are not updated correctly 2012-10-25 7:18 ` Jonathan Nieder @ 2012-10-25 16:43 ` Felipe Contreras 0 siblings, 0 replies; 42+ messages in thread From: Felipe Contreras @ 2012-10-25 16:43 UTC (permalink / raw) To: Jonathan Nieder Cc: Sverre Rabbelier, Junio C Hamano, Jeff King, Git List, Daniel Barkalow, Ramkumar Ramachandra, Dmitry Ivankov, Johannes Schindelin, Ævar Arnfjörð, Eric Herman, Fernando Vezzosi On Thu, Oct 25, 2012 at 9:18 AM, Jonathan Nieder <jrnieder@gmail.com> wrote: > Felipe Contreras wrote: > >> Show me a single remote helper that manually stores SHA-1's and I >> might believe you, but I doubt that, marks are too convenient. > > Oh dear lord. Why are you arguing? Explain how coming to a consensus > on this will help accomplish something useful, and then I can explain > my point of view. In the meantime, this seems like a waste of time. We don't need to come to a consensus because there is no problem. Nobody has requested this feature, and nobody has faced any problem with this. If you have no evidence of the contrary, that's what I'll believe. I agree it's a waste of time, so let's not talk about the :0 -> SHA-1 feature, or the master..master feature in this thread; they are orthogonal. Cheers. -- Felipe Contreras ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 1/3] t9350: point out that refs are not updated correctly 2012-10-24 17:52 ` Felipe Contreras 2012-10-24 18:08 ` Jonathan Nieder @ 2012-10-24 21:41 ` Johannes Schindelin 2012-10-25 5:13 ` Felipe Contreras 1 sibling, 1 reply; 42+ messages in thread From: Johannes Schindelin @ 2012-10-24 21:41 UTC (permalink / raw) To: Felipe Contreras Cc: Sverre Rabbelier, Junio C Hamano, Jonathan Nieder, Jeff King, Git List, Daniel Barkalow, Ramkumar Ramachandra, Dmitry Ivankov, Ævar Arnfjörð Bjarmason, Eric Herman, Fernando Vezzosi Hi, On Wed, 24 Oct 2012, Felipe Contreras wrote: > 2) master..master shouldn't export anything The underlying issue -- as explained in the thread -- is when you want to update master to a commit that another ref already points to. In that case no commits need to exported, but the ref needs to be updated nevertheless. We just wrote the test in the most convenient way, no need to complicate things more than necessary. Hth, Johannes ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 1/3] t9350: point out that refs are not updated correctly 2012-10-24 21:41 ` Johannes Schindelin @ 2012-10-25 5:13 ` Felipe Contreras 0 siblings, 0 replies; 42+ messages in thread From: Felipe Contreras @ 2012-10-25 5:13 UTC (permalink / raw) To: Johannes Schindelin Cc: Sverre Rabbelier, Junio C Hamano, Jonathan Nieder, Jeff King, Git List, Daniel Barkalow, Ramkumar Ramachandra, Dmitry Ivankov, Ævar Arnfjörð Bjarmason, Eric Herman, Fernando Vezzosi On Wed, Oct 24, 2012 at 11:41 PM, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote: > Hi, > > On Wed, 24 Oct 2012, Felipe Contreras wrote: > >> 2) master..master shouldn't export anything > > The underlying issue -- as explained in the thread -- is when you want to > update master to a commit that another ref already points to. In that case > no commits need to exported, but the ref needs to be updated nevertheless. > > We just wrote the test in the most convenient way, no need to complicate > things more than necessary. That test cannot work, and it shouldn't work. You say you want to 'update master to a commit that another ref already points to'. What other ref? If you want to update master, this is what you do: % git fast-export master What do you expect 'git fast-export master..master' to export? This? --- reset refs/heads/master from $(git rev-parse master) --- What is a remote helper supposed to do with a SHA-1? Nothing, a git SHA-1 is useless to say, a mercurial remote helper. To make sense of it you would need to access the git repository and get the commit object, and that's defeating the purpose of a fast exporter. No, that's not what you want. But at this point there's only one ref in the picture, you said 'update master to a commit that another ref already points to', but there's only one ref, where is the other ref? Maybe your test should do this: % git fast-export foo master But wait, that actually works, except that the output will be nothing close what you expected before, we would get all the commits and files that constitute 'foo', which is actually useful, and what we expect from fast-export, and in addition, master will be updated to the right ref. No, the problem is not only 'update master to a commit that another ref already points to', but that this happens in two different commands, and that can only be done with marks, just like the test I proposed. The original test doesn't expose the problem we are trying to solve, and it shouldn't work anyway. Moreover, what we eventually want to do is support the transport helpers, so how about you run this: --- #!/bin/sh cat > git-remote-foo <<-\EOF #!/bin/sh read l echo $l 1>&2 echo export echo refspec refs/heads/*:refs/foo/origin/* test -e /tmp/marks-git && echo *import-marks /tmp/marks-git echo *export-marks /tmp/marks-git echo read l echo $l 1>&2 echo ? refs/heads/master echo read l echo $l 1>&2 while read l; do echo $l 1>&2 test "$l" == 'done' && exit done EOF chmod +x git-remote-foo export PATH=$PWD:$PATH rm -f /tmp/marks-git ( git init test cd test echo Test >> Test git add --all git commit -m 'Initial commit' git branch foo echo "== master ==" git push foo::test master echo "== foo ==" git push foo::test foo ) --- I get this output with my patch: --- [master (root-commit) b159eff] Initial commit 1 file changed, 1 insertion(+) create mode 100644 Test == master == capabilities list export feature done blob mark :1 data 5 Test reset refs/heads/master commit refs/heads/master mark :2 author Felipe Contreras <felipe.contreras@gmail.com> 1351140987 +0200 committer Felipe Contreras <felipe.contreras@gmail.com> 1351140987 +0200 data 15 Initial commit M 100644 :1 Test done == foo == capabilities list export feature done reset refs/heads/foo from :2 done --- Hey, did you see that? 'foo' is updated, both 'master' and 'foo' point to the same object. What is the problem? -- Felipe Contreras ^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH 2/3] fast-export: do not refer to non-existing marks 2011-11-05 23:23 [PATCH 0/3] fast-export fixes Sverre Rabbelier 2011-11-05 23:23 ` [PATCH 1/3] t9350: point out that refs are not updated correctly Sverre Rabbelier @ 2011-11-05 23:23 ` Sverre Rabbelier 2011-11-06 4:45 ` Jonathan Nieder 2011-11-05 23:23 ` [PATCH 3/3] fast-export: output reset command for commandline revs Sverre Rabbelier 2 siblings, 1 reply; 42+ messages in thread From: Sverre Rabbelier @ 2011-11-05 23:23 UTC (permalink / raw) To: Junio C Hamano, Jonathan Nieder, Jeff King, Git List, Daniel Barkalow, Ramkumar Cc: Sverre Rabbelier From: Johannes Schindelin <Johannes.Schindelin@gmx.de> When calling `git fast-export a..a b` when a and b refer to the same commit, nothing would be exported, and an incorrect reset line would be printed for b ('from :0'). Extract a handle_reset function that deals with this, which can then be re-used in a later commit. Signed-off-by: Johannes Schindelin <Johannes.Schindelin@gmx.de> Signed-off-by: Sverre Rabbelier <srabbelier@gmail.com> --- builtin/fast-export.c | 17 +++++++++++++---- 1 files changed, 13 insertions(+), 4 deletions(-) diff --git a/builtin/fast-export.c b/builtin/fast-export.c index 9836e6b..c4c4391 100644 --- a/builtin/fast-export.c +++ b/builtin/fast-export.c @@ -529,9 +529,20 @@ static void get_tags_and_duplicates(struct object_array *pending, } } +static void handle_reset(const char *name, struct object *object) +{ + int mark = get_object_mark(object); + + if (mark) + printf("reset %s\nfrom :%d\n\n", name, + get_object_mark(object)); + else + printf("reset %s\nfrom %s\n\n", name, + sha1_to_hex(object->sha1)); +} + static void handle_tags_and_duplicates(struct string_list *extra_refs) { - struct commit *commit; int i; for (i = extra_refs->nr - 1; i >= 0; i--) { @@ -543,9 +554,7 @@ static void handle_tags_and_duplicates(struct string_list *extra_refs) break; case OBJ_COMMIT: /* create refs pointing to already seen commits */ - commit = (struct commit *)object; - printf("reset %s\nfrom :%d\n\n", name, - get_object_mark(&commit->object)); + handle_reset(name, object); show_progress(); break; } -- 1.7.8.rc0.36.g67522.dirty ^ permalink raw reply related [flat|nested] 42+ messages in thread
* Re: [PATCH 2/3] fast-export: do not refer to non-existing marks 2011-11-05 23:23 ` [PATCH 2/3] fast-export: do not refer to non-existing marks Sverre Rabbelier @ 2011-11-06 4:45 ` Jonathan Nieder 2011-11-06 19:40 ` Sverre Rabbelier 0 siblings, 1 reply; 42+ messages in thread From: Jonathan Nieder @ 2011-11-06 4:45 UTC (permalink / raw) To: Sverre Rabbelier Cc: Junio C Hamano, Jeff King, Git List, Daniel Barkalow, Ramkumar Ramachandra, Dmitry Ivankov, Johannes Schindelin, Ævar Arnfjörð Bjarmason, Eric Herman, Fernando Vezzosi Hi, Sverre Rabbelier wrote: > When calling `git fast-export a..a b` when a and b refer to the same > commit, nothing would be exported, and an incorrect reset line would > be printed for b ('from :0'). Hm, seems problematic indeed. > Extract a handle_reset function that deals with this, which can then > be re-used in a later commit. So, does this patch drop the confusing behavior and add one that is more intuitive for remote helpers? It's not clear from this description what sort of deal the patch makes and whether it is a good or bad one. [...] > --- a/builtin/fast-export.c > +++ b/builtin/fast-export.c > @@ -529,9 +529,20 @@ static void get_tags_and_duplicates(struct object_array *pending, > } > } > > +static void handle_reset(const char *name, struct object *object) Nit: the other handle_* functions are about acting on objects encountered during revision traversal from the object store. In other words, the things being handled are the git objects. By contrast, this function is about writing a "reset" command to the fast-import stream. I'd be tempted to call it reset_ref() or something like that. > +{ > + int mark = get_object_mark(object); > + > - commit = (struct commit *)object; > - printf("reset %s\nfrom :%d\n\n", name, > - get_object_mark(&commit->object)); > + if (mark) > + printf("reset %s\nfrom :%d\n\n", name, > + get_object_mark(object)); > + else > + printf("reset %s\nfrom %s\n\n", name, > + sha1_to_hex(object->sha1)); Ah --- the functional change is to use a sha1 when there is no mark corresponding to the object. Why is this codepath being run at all when b is excluded by the revision range (a..a a = ^a a a)? Is this the same bug tested for in patch 1/3 or something separate? ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 2/3] fast-export: do not refer to non-existing marks 2011-11-06 4:45 ` Jonathan Nieder @ 2011-11-06 19:40 ` Sverre Rabbelier 2019-01-29 19:41 ` Johannes Schindelin 0 siblings, 1 reply; 42+ messages in thread From: Sverre Rabbelier @ 2011-11-06 19:40 UTC (permalink / raw) To: Jonathan Nieder Cc: Junio C Hamano, Jeff King, Git List, Daniel Barkalow, Ramkumar Ramachandra, Dmitry Ivankov, Johannes Schindelin, Ævar Arnfjörð, Eric Herman, Fernando Vezzosi Heya, On Sun, Nov 6, 2011 at 05:45, Jonathan Nieder <jrnieder@gmail.com> wrote: >> Extract a handle_reset function that deals with this, which can then >> be re-used in a later commit. > > So, does this patch drop the confusing behavior and add one that is > more intuitive for remote helpers? It's not clear from this > description what sort of deal the patch makes and whether it is a good > or bad one. Ah, yes. Perhaps something like: "Extract a reset_ref function that deals with this situation by printing the commit sha1 when no mark has been written yet." > Ah --- the functional change is to use a sha1 when there is no mark > corresponding to the object. > > Why is this codepath being run at all when b is excluded by the > revision range (a..a a = ^a a a)? Is this the same bug tested > for in patch 1/3 or something separate? I must admit that I don't recall how exactly we stumbled on this case. It might even make sense to instead die when we run into this corner case, but I'm not convinced that there's no valid use case for this (which we would block by die-ing). -- Cheers, Sverre Rabbelier ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 2/3] fast-export: do not refer to non-existing marks 2011-11-06 19:40 ` Sverre Rabbelier @ 2019-01-29 19:41 ` Johannes Schindelin 0 siblings, 0 replies; 42+ messages in thread From: Johannes Schindelin @ 2019-01-29 19:41 UTC (permalink / raw) To: Sverre Rabbelier Cc: Jonathan Nieder, Junio C Hamano, Jeff King, Git List, Daniel Barkalow, Ramkumar Ramachandra, Dmitry Ivankov, Ævar Arnfjörð [-- Attachment #1: Type: text/plain, Size: 1666 bytes --] Hi Sverre, On Sun, 6 Nov 2011, Sverre Rabbelier wrote: > On Sun, Nov 6, 2011 at 05:45, Jonathan Nieder <jrnieder@gmail.com> wrote: > >> Extract a handle_reset function that deals with this, which can then > >> be re-used in a later commit. > > > > So, does this patch drop the confusing behavior and add one that is > > more intuitive for remote helpers? It's not clear from this > > description what sort of deal the patch makes and whether it is a good > > or bad one. > > Ah, yes. Perhaps something like: > > "Extract a reset_ref function that deals with this situation by > printing the commit sha1 when no mark has been written yet." > > > Ah --- the functional change is to use a sha1 when there is no mark > > corresponding to the object. > > > > Why is this codepath being run at all when b is excluded by the > > revision range (a..a a = ^a a a)? Is this the same bug tested > > for in patch 1/3 or something separate? > > I must admit that I don't recall how exactly we stumbled on this case. > It might even make sense to instead die when we run into this corner > case, but I'm not convinced that there's no valid use case for this > (which we would block by die-ing). I know, it has been a while since we hacked on this in your tiny room in the Netherlands, and it has been almost as long since this here mail thread stalled, when the consensus back then seemed that this patch is not even necessary. You might find it satisfying that this change, in a slightly different form, made it to `master` recently, more precisely in https://github.com/git/git/commit/530ca19c02b1fa1d13195d24fc76c2926ceecdc2 So: closure, at long last. Ciao, Dscho ^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH 3/3] fast-export: output reset command for commandline revs 2011-11-05 23:23 [PATCH 0/3] fast-export fixes Sverre Rabbelier 2011-11-05 23:23 ` [PATCH 1/3] t9350: point out that refs are not updated correctly Sverre Rabbelier 2011-11-05 23:23 ` [PATCH 2/3] fast-export: do not refer to non-existing marks Sverre Rabbelier @ 2011-11-05 23:23 ` Sverre Rabbelier 2011-11-06 5:01 ` Jonathan Nieder ` (4 more replies) 2 siblings, 5 replies; 42+ messages in thread From: Sverre Rabbelier @ 2011-11-05 23:23 UTC (permalink / raw) To: Junio C Hamano, Jonathan Nieder, Jeff King, Git List, Daniel Barkalow, Ramkumar Cc: Sverre Rabbelier When a revision is specified on the commandline we explicitly output a 'reset' command for it if it was not handled already. This allows for example the remote-helper protocol to use fast-export to create branches that point to a commit that has already been exported. Initial-patch-by: Johannes Schindelin <Johannes.Schindelin@gmx.de> Signed-off-by: Sverre Rabbelier <srabbelier@gmail.com> --- Most of the hard work for this patch was done by Dscho. The rest of it was basically me applying the technique used by jch in c3502fa (25-08-2011 do not include sibling history in --ancestry-path). The if statement dealing with tag_of_filtered_mode is not as elegant as either me or Dscho would have liked, but we couldn't find a better way to determine if a ref is a tag at this point in the code. Additionally, the elem->whence != REV_CMD_RIGHT case should really check if REV_CMD_RIGHT_REF, but as this is not provided by the ref_info structure this is left as is. A result of this is that incorrect input will result in incorrect output, rather than an error message. That is: `git fast-export a..<sha1>` will incorrectly generate a `reset <sha1>` statement in the fast-export stream. The dwim_ref bit is a double work (it has already been done by the caller of this function), but I decided it would be more work to pass this information along than to recompute it for the few commandline refs that were relevant. builtin/fast-export.c | 31 +++++++++++++++++++++++++++++-- t/t9350-fast-export.sh | 2 +- 2 files changed, 30 insertions(+), 3 deletions(-) diff --git a/builtin/fast-export.c b/builtin/fast-export.c index c4c4391..bcfec38 100644 --- a/builtin/fast-export.c +++ b/builtin/fast-export.c @@ -18,6 +18,8 @@ #include "parse-options.h" #include "quote.h" +#define REF_HANDLED (ALL_REV_FLAGS + 1) + static const char *fast_export_usage[] = { "git fast-export [rev-list-opts]", NULL @@ -541,10 +543,34 @@ static void handle_reset(const char *name, struct object *object) sha1_to_hex(object->sha1)); } -static void handle_tags_and_duplicates(struct string_list *extra_refs) +static void handle_tags_and_duplicates(struct rev_info *revs, struct string_list *extra_refs) { int i; + /* even if no commits were exported, we need to export the ref */ + for (i = 0; i < revs->cmdline.nr; i++) { + struct rev_cmdline_entry *elem = &revs->cmdline.rev[i]; + + if (elem->flags & UNINTERESTING) + continue; + + if (elem->whence != REV_CMD_REV && elem->whence != REV_CMD_RIGHT) + continue; + + char *full_name; + dwim_ref(elem->name, strlen(elem->name), elem->item->sha1, &full_name); + + if (!prefixcmp(full_name, "refs/tags/") && + (tag_of_filtered_mode != REWRITE || + !get_object_mark(elem->item))) + continue; + + if (!(elem->flags & REF_HANDLED)) { + handle_reset(full_name, elem->item); + elem->flags |= REF_HANDLED; + } + } + for (i = extra_refs->nr - 1; i >= 0; i--) { const char *name = extra_refs->items[i].string; struct object *object = extra_refs->items[i].util; @@ -698,11 +724,12 @@ int cmd_fast_export(int argc, const char **argv, const char *prefix) } else { handle_commit(commit, &revs); + commit->object.flags |= REF_HANDLED; handle_tail(&commits, &revs); } } - handle_tags_and_duplicates(&extra_refs); + handle_tags_and_duplicates(&revs, &extra_refs); if (export_filename) export_marks(export_filename); diff --git a/t/t9350-fast-export.sh b/t/t9350-fast-export.sh index 74914dc..ea7dc21 100755 --- a/t/t9350-fast-export.sh +++ b/t/t9350-fast-export.sh @@ -446,7 +446,7 @@ from $(git rev-parse master) EOF -test_expect_failure 'refs are updated even if no commits need to be exported' ' +test_expect_success 'refs are updated even if no commits need to be exported' ' git fast-export master..master > actual && test_cmp expected actual ' -- 1.7.8.rc0.36.g67522.dirty ^ permalink raw reply related [flat|nested] 42+ messages in thread
* Re: [PATCH 3/3] fast-export: output reset command for commandline revs 2011-11-05 23:23 ` [PATCH 3/3] fast-export: output reset command for commandline revs Sverre Rabbelier @ 2011-11-06 5:01 ` Jonathan Nieder 2011-11-06 19:48 ` Sverre Rabbelier 2011-11-07 5:52 ` Junio C Hamano ` (3 subsequent siblings) 4 siblings, 1 reply; 42+ messages in thread From: Jonathan Nieder @ 2011-11-06 5:01 UTC (permalink / raw) To: Sverre Rabbelier Cc: Junio C Hamano, Jeff King, Git List, Daniel Barkalow, Ramkumar Ramachandra, Dmitry Ivankov, Johannes Schindelin, Ævar Arnfjörð Bjarmason, Eric Herman, Fernando Vezzosi Sverre Rabbelier wrote: > When a revision is specified on the commandline we explicitly output > a 'reset' command for it if it was not handled already. This allows > for example the remote-helper protocol to use fast-export to create > branches that point to a commit that has already been exported. > > Initial-patch-by: Johannes Schindelin <Johannes.Schindelin@gmx.de> > Signed-off-by: Sverre Rabbelier <srabbelier@gmail.com> Thanks. I'd suggest squashing in the test from patch 1/3 for easy reference (since each patch makes the other easier to understand). > --- > The if statement dealing with tag_of_filtered_mode is not as > elegant as either me or Dscho would have liked, but we couldn't > find a better way to determine if a ref is a tag at this point > in the code. > > Additionally, the elem->whence != REV_CMD_RIGHT case should really > check if REV_CMD_RIGHT_REF, but as this is not provided by the > ref_info structure this is left as is. A result of this is that > incorrect input will result in incorrect output, rather than an > error message. That is: `git fast-export a..<sha1>` will > incorrectly generate a `reset <sha1>` statement in the fast-export > stream. > > The dwim_ref bit is a double work (it has already been done by the > caller of this function), but I decided it would be more work to > pass this information along than to recompute it for the few > commandline refs that were relevant. These details seem like good details for the commit message, so the next puzzled person looking at the code can see what behavior is deliberate and what are the incidental side-effects. The "git fast-export a..$(git rev-parse HEAD^{commit})" case sounds worth a test. > --- a/builtin/fast-export.c > +++ b/builtin/fast-export.c > @@ -18,6 +18,8 @@ > #include "parse-options.h" > #include "quote.h" > > +#define REF_HANDLED (ALL_REV_FLAGS + 1) Could TMP_MARK be used for this? [...] > @@ -541,10 +543,34 @@ static void handle_reset(const char *name, struct object *object) > sha1_to_hex(object->sha1)); > } > > -static void handle_tags_and_duplicates(struct string_list *extra_refs) > +static void handle_tags_and_duplicates(struct rev_info *revs, struct string_list *extra_refs) > { > int i; > > + /* even if no commits were exported, we need to export the ref */ > + for (i = 0; i < revs->cmdline.nr; i++) { Might be clearer in a new function. > + struct rev_cmdline_entry *elem = &revs->cmdline.rev[i]; > + > + if (elem->flags & UNINTERESTING) > + continue; > + > + if (elem->whence != REV_CMD_REV && elem->whence != REV_CMD_RIGHT) > + continue; Oh, neat. > + > + char *full_name; declaration-after-statement > + dwim_ref(elem->name, strlen(elem->name), elem->item->sha1, &full_name); > + > + if (!prefixcmp(full_name, "refs/tags/") && What happens if dwim_ref fails, perhaps because a ref was deleted in the meantime? > + (tag_of_filtered_mode != REWRITE || > + !get_object_mark(elem->item))) > + continue; Style nit: this would be easier to read if the "if" condition doesn't line up with the code below it: if (!prefixcmp(full_name, "refs/tags/")) { if (tag_of_filtered_mode != REWRITE || !get_object_mark(elem->item)) continue; } If tag_of_filtered_mode == ABORT, we are going to die() soon, right? So this seems to be about tag_of_filtered_mode == DROP --- makes sense. When does the !get_object_mark() case come up? > + > + if (!(elem->flags & REF_HANDLED)) { > + handle_reset(full_name, elem->item); > + elem->flags |= REF_HANDLED; > + } Just curious: is the REF_HANDLED handling actually needed? What would happen if fast-export included the redundant resets? > + } [...] > --- a/t/t9350-fast-export.sh > +++ b/t/t9350-fast-export.sh > @@ -446,7 +446,7 @@ from $(git rev-parse master) > > EOF > > -test_expect_failure 'refs are updated even if no commits need to be exported' ' > +test_expect_success 'refs are updated even if no commits need to be exported' ' > git fast-export master..master > actual && > test_cmp expected actual > ' Thanks for a pleasant read. ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 3/3] fast-export: output reset command for commandline revs 2011-11-06 5:01 ` Jonathan Nieder @ 2011-11-06 19:48 ` Sverre Rabbelier 2011-11-07 8:58 ` Jonathan Nieder 0 siblings, 1 reply; 42+ messages in thread From: Sverre Rabbelier @ 2011-11-06 19:48 UTC (permalink / raw) To: Jonathan Nieder Cc: Junio C Hamano, Jeff King, Git List, Daniel Barkalow, Ramkumar Ramachandra, Dmitry Ivankov, Johannes Schindelin, Ævar Arnfjörð, Eric Herman, Fernando Vezzosi Heya, On Sun, Nov 6, 2011 at 06:01, Jonathan Nieder <jrnieder@gmail.com> wrote: > Thanks. I'd suggest squashing in the test from patch 1/3 for easy > reference (since each patch makes the other easier to understand). Yes, agreed. The initial series was 5 patches in total, but splitting it out for such a small series (and small patch at that) makes less sense. > These details seem like good details for the commit message, so the > next puzzled person looking at the code can see what behavior is > deliberate and what are the incidental side-effects. All of it? I wasn't sure what part should go in the commit message. > The "git fast-export a..$(git rev-parse HEAD^{commit})" case sounds > worth a test. A test_must_fail? >> +#define REF_HANDLED (ALL_REV_FLAGS + 1) > > Could TMP_MARK be used for this? I don't know its usage, is it? > -static void handle_tags_and_duplicates(struct string_list *extra_refs) >> +static void handle_tags_and_duplicates(struct rev_info *revs, struct string_list *extra_refs) >> { >> int i; >> >> + /* even if no commits were exported, we need to export the ref */ >> + for (i = 0; i < revs->cmdline.nr; i++) { > > Might be clearer in a new function. Yes, probably. handle_cmdline_refs? >> + struct rev_cmdline_entry *elem = &revs->cmdline.rev[i]; >> + >> + if (elem->flags & UNINTERESTING) >> + continue; >> + >> + if (elem->whence != REV_CMD_REV && elem->whence != REV_CMD_RIGHT) >> + continue; > > Oh, neat. Yes, I must admit that this bit was easier than I dreaded it would be (I must admit that's been a large reason that I haven't taken the time to work on this till now). With the fast-export and remote-helper tests to guide me, I was able to code-by-accident the right conditions here :). >> + >> + char *full_name; > > declaration-after-statement Ah, yes. >> + dwim_ref(elem->name, strlen(elem->name), elem->item->sha1, &full_name); >> + >> + if (!prefixcmp(full_name, "refs/tags/") && > > What happens if dwim_ref fails, perhaps because a ref was deleted in > the meantime? That would be bad. I assumed that we have a lock on the refs, should I add back the die check that's done by the other dwim_ref caller? >> + (tag_of_filtered_mode != REWRITE || >> + !get_object_mark(elem->item))) >> + continue; > > Style nit: this would be easier to read if the "if" condition doesn't > line up with the code below it: > > if (!prefixcmp(full_name, "refs/tags/")) { > if (tag_of_filtered_mode != REWRITE || > !get_object_mark(elem->item)) > continue; > } Yeah, that does look better :). > If tag_of_filtered_mode == ABORT, we are going to die() soon, right? I don't know to be honest, perhaps we would have already died by now? I don't know the details of how the tag_of_filtered_mode part is implemented. > So this seems to be about tag_of_filtered_mode == DROP --- makes > sense. > > When does the !get_object_mark() case come up? Eh, it has something to do with it being a replacement (rather than the same), maybe? This is mostly just taken from Dscho's original patch. >> + if (!(elem->flags & REF_HANDLED)) { >> + handle_reset(full_name, elem->item); >> + elem->flags |= REF_HANDLED; >> + } > > Just curious: is the REF_HANDLED handling actually needed? What > would happen if fast-export included the redundant resets? That would just be sloppy :). I don't think anything particularly bad would happen. > Thanks for a pleasant read. Thanks for the review. -- Cheers, Sverre Rabbelier ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 3/3] fast-export: output reset command for commandline revs 2011-11-06 19:48 ` Sverre Rabbelier @ 2011-11-07 8:58 ` Jonathan Nieder 0 siblings, 0 replies; 42+ messages in thread From: Jonathan Nieder @ 2011-11-07 8:58 UTC (permalink / raw) To: Sverre Rabbelier Cc: Junio C Hamano, Jeff King, Git List, Daniel Barkalow, Ramkumar Ramachandra, Dmitry Ivankov, Johannes Schindelin, Ævar Arnfjörð, Eric Herman, Fernando Vezzosi Sverre Rabbelier wrote: > On Sun, Nov 6, 2011 at 06:01, Jonathan Nieder <jrnieder@gmail.com> wrote: >> These details seem like good details for the commit message, so the >> next puzzled person looking at the code can see what behavior is >> deliberate and what are the incidental side-effects. > > All of it? I wasn't sure what part should go in the commit message. Yeah. My rule when in doubt has been to just include everything that would remain meaningful over time that I could be putting in a cover letter for the patch. The hard part is to try to be concise in doing so. >> The "git fast-export a..$(git rev-parse HEAD^{commit})" case sounds >> worth a test. > > A test_must_fail? Yep. >>> +#define REF_HANDLED (ALL_REV_FLAGS + 1) >> >> Could TMP_MARK be used for this? > > I don't know its usage, is it? Since handle_tags_and_duplicates() happens after the main revision traversal, it would be safe. But it's probably not good style. Any later revwalk would be confused by or clobber that flag. My actual worry was that if there are too many rev flags some day, this REF_HANDLED could wrap around to 0. Now I see that custom per-command flags are not so rare --- it is just this idiom for allocating them by adding 1 to the all-ones bitmask that is unusual. The most common idiom is to simply start with 1u<<16: #define REF_HANDLED (1u<<16) unpack-objects uses 1u<<20 instead. blame starts with 1u<<12. reflog starts with 1u<<10. A part of me wishes the command-specific flags were allocated in revision.h like the standard ones so one could write #define REF_HANDLED REVFLAGSUSR1 by analogy with SIGUSR1, or that there were some other mechanism for avoiding collisions. >>> + dwim_ref(elem->name, strlen(elem->name), elem->item->sha1, &full_name); >>> + >>> + if (!prefixcmp(full_name, "refs/tags/") && >> >> What happens if dwim_ref fails, perhaps because a ref was deleted in >> the meantime? > > That would be bad. I assumed that we have a lock on the refs, should I > add back the die check that's done by the other dwim_ref caller? Sure, there's a lock. It doesn't stop a non-git process-gone-mad like /bin/rm from deleting a file under .git/refs. :) die()-ing on error sounds sane. [...] >> If tag_of_filtered_mode == ABORT, we are going to die() soon, right? > > I don't know to be honest, perhaps we would have already died by now? It's the handle_tag() call, later in handle_tags_and_duplicates(). [...] >> When does the !get_object_mark() case come up? > > Eh, it has something to do with it being a replacement (rather than > the same), maybe? This is mostly just taken from Dscho's original > patch. Ah, this is similar to the mysterious case from patch 2/3. Probably this is the "git fast-export a..a" case, where 'a' was not dumped because UNINTERESTING but we still want to reset refs/tags/a to point to it. But won't handle_tag() write tags refs/heads/a from :0 [tagger, etc] when we get to it? Side question: should the for (i = extra_refs->nr - 1; i >= 0; i--) { loop should be earlier in the function and set REF_HANDLED where appropriate, to avoid resets for these objects, too? [...] >> Just curious: is the REF_HANDLED handling actually needed? What >> would happen if fast-export included the redundant resets? > > That would just be sloppy :). I don't think anything particularly bad > would happen. I suppose this is needed to avoid pointless changes in output which could break git's or other projects' test suites without good reason. Makes sense. Thanks for the clarifications. Jonathan ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 3/3] fast-export: output reset command for commandline revs 2011-11-05 23:23 ` [PATCH 3/3] fast-export: output reset command for commandline revs Sverre Rabbelier 2011-11-06 5:01 ` Jonathan Nieder @ 2011-11-07 5:52 ` Junio C Hamano 2011-11-07 5:53 ` Junio C Hamano ` (2 subsequent siblings) 4 siblings, 0 replies; 42+ messages in thread From: Junio C Hamano @ 2011-11-07 5:52 UTC (permalink / raw) To: Sverre Rabbelier Cc: Jonathan Nieder, Jeff King, Git List, Daniel Barkalow, Ramkumar Ramachandra, Dmitry Ivankov, Johannes Schindelin, Ævar Arnfjörð Bjarmason, Eric Herman, Fernando Vezzosi Sverre Rabbelier <srabbelier@gmail.com> writes: > Additionally, the elem->whence != REV_CMD_RIGHT case should really > check if REV_CMD_RIGHT_REF, but as this is not provided by the > ref_info structure this is left as is. I am not sure what you mean by REV_CMD_RIGHT_REF here. Do you mean "We are only interested in the RHS endpoint of A...B syntax (i.e. B) but only when it is a refname and not an arbitrary SHA-1 expression (e.g. even though next~4 in "master...next~4" is a RHS endpoint, it is not a ref, and we do not want it)"? I think the distinction you are trying to express ("is it a ref and if so what exact refname resolve_ref() would produce, or is it just the name of a random commit?") is a very useful thing in general, but it is orthogonal to what existing REV_CMD_* are trying to express, which is "where did they come from", that you can read from the name of the field "whence". Perhaps we would want to add a new field "const char *ref" to "struct rev_cmdline_entry" to record the additional information you want perhaps by storing the result of resolve_ref() if it is a ref and NULL otherwise. Would it be too much work to add it to perfect this series? By the way, REV_CMD_REF is meant to mean "the user did not explicitly name this but it came as a result of iterating over refs/something/ namespace", and does not mean "this is a tip of some ref" (they happen to be all refs, but "obtained by iteration, not by explicit naming" is the more important reason for marking them as such). As they are numerous, if you are going to add that "const char *ref" field to rev_cmdline_entry, we may want to either leave it NULL for REV_CMD_REF entries (the name field already has that information anyway), or have it point at its name field (we need to audit the codepath to free the name and ref fields if we go that route). ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 3/3] fast-export: output reset command for commandline revs 2011-11-05 23:23 ` [PATCH 3/3] fast-export: output reset command for commandline revs Sverre Rabbelier 2011-11-06 5:01 ` Jonathan Nieder 2011-11-07 5:52 ` Junio C Hamano @ 2011-11-07 5:53 ` Junio C Hamano 2011-11-30 16:56 ` Thomas Rast 2012-10-24 18:02 ` Felipe Contreras 4 siblings, 0 replies; 42+ messages in thread From: Junio C Hamano @ 2011-11-07 5:53 UTC (permalink / raw) To: Sverre Rabbelier Cc: Jonathan Nieder, Jeff King, Git List, Daniel Barkalow, Ramkumar Ramachandra, Dmitry Ivankov, Johannes Schindelin, Ævar Arnfjörð Bjarmason, Eric Herman, Fernando Vezzosi Sverre Rabbelier <srabbelier@gmail.com> writes: > +static void handle_tags_and_duplicates(struct rev_info *revs, struct string_list *extra_refs) > { > int i; > > + /* even if no commits were exported, we need to export the ref */ > + for (i = 0; i < revs->cmdline.nr; i++) { > + struct rev_cmdline_entry *elem = &revs->cmdline.rev[i]; > + > + if (elem->flags & UNINTERESTING) > + continue; > + > + if (elem->whence != REV_CMD_REV && elem->whence != REV_CMD_RIGHT) > + continue; > + > + char *full_name; > + dwim_ref(elem->name, strlen(elem->name), elem->item->sha1, &full_name); Just a nit I've already fixed locally (iow no need to resend only to fix this) but this is decl-after-stmt. ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 3/3] fast-export: output reset command for commandline revs 2011-11-05 23:23 ` [PATCH 3/3] fast-export: output reset command for commandline revs Sverre Rabbelier ` (2 preceding siblings ...) 2011-11-07 5:53 ` Junio C Hamano @ 2011-11-30 16:56 ` Thomas Rast 2012-10-24 18:02 ` Felipe Contreras 4 siblings, 0 replies; 42+ messages in thread From: Thomas Rast @ 2011-11-30 16:56 UTC (permalink / raw) To: Sverre Rabbelier Cc: Junio C Hamano, Jonathan Nieder, Jeff King, Git List, Daniel Barkalow, Ramkumar Ramachandra, Dmitry Ivankov, Johannes Schindelin, Ævar Arnfjörð Bjarmason, Eric Herman, Fernando Vezzosi Sverre Rabbelier wrote: > When a revision is specified on the commandline we explicitly output > a 'reset' command for it if it was not handled already. This allows > for example the remote-helper protocol to use fast-export to create > branches that point to a commit that has already been exported. > > Initial-patch-by: Johannes Schindelin <Johannes.Schindelin@gmx.de> > Signed-off-by: Sverre Rabbelier <srabbelier@gmail.com> My apologies if this is redundant, I'm not up to speed on progress here. But a crash in t9350.19 caught my eye: checking known breakage: ( cd limit-by-paths && git fast-export master~2..master~1 > output && test_cmp output expected ) ==23766== Invalid read of size 1 ==23766== at 0x4FD21E: prefixcmp (strbuf.c:9) ==23766== by 0x42B936: handle_tags_and_duplicates (fast-export.c:563) ==23766== by 0x42C274: cmd_fast_export (fast-export.c:732) ==23766== by 0x4051F1: run_builtin (git.c:308) ==23766== by 0x40538B: handle_internal_command (git.c:466) ==23766== by 0x4054A5: run_argv (git.c:512) ==23766== by 0x40562C: main (git.c:585) ==23766== Address 0x0 is not stack'd, malloc'd or (recently) free'd ==23766== { <insert_a_suppression_name_here> Memcheck:Addr1 fun:prefixcmp fun:handle_tags_and_duplicates fun:cmd_fast_export fun:run_builtin fun:handle_internal_command fun:run_argv fun:main } ==23766== ==23766== Process terminating with default action of signal 11 (SIGSEGV) ==23766== Access not within mapped region at address 0x0 ==23766== at 0x4FD21E: prefixcmp (strbuf.c:9) ==23766== by 0x42B936: handle_tags_and_duplicates (fast-export.c:563) ==23766== by 0x42C274: cmd_fast_export (fast-export.c:732) ==23766== by 0x4051F1: run_builtin (git.c:308) ==23766== by 0x40538B: handle_internal_command (git.c:466) ==23766== by 0x4054A5: run_argv (git.c:512) ==23766== by 0x40562C: main (git.c:585) The crash is hidden by the fact that the test is test_expect_failure. It bisects to this commit. Perhaps we should distinguish between test_expect_failure and test_expect_crash?... -- Thomas Rast trast@{inf,student}.ethz.ch ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 3/3] fast-export: output reset command for commandline revs 2011-11-05 23:23 ` [PATCH 3/3] fast-export: output reset command for commandline revs Sverre Rabbelier ` (3 preceding siblings ...) 2011-11-30 16:56 ` Thomas Rast @ 2012-10-24 18:02 ` Felipe Contreras 4 siblings, 0 replies; 42+ messages in thread From: Felipe Contreras @ 2012-10-24 18:02 UTC (permalink / raw) To: Sverre Rabbelier Cc: Junio C Hamano, Jonathan Nieder, Jeff King, Git List, Daniel Barkalow, Ramkumar Ramachandra, Dmitry Ivankov, Johannes Schindelin, Ævar Arnfjörð Bjarmason, Eric Herman, Fernando Vezzosi On Sun, Nov 6, 2011 at 12:23 AM, Sverre Rabbelier <srabbelier@gmail.com> wrote: > When a revision is specified on the commandline we explicitly output > a 'reset' command for it if it was not handled already. This allows > for example the remote-helper protocol to use fast-export to create > branches that point to a commit that has already been exported. This simpler patch does the same, doesn't it? diff --git a/builtin/fast-export.c b/builtin/fast-export.c index 12220ad..3b4c2d6 100644 --- a/builtin/fast-export.c +++ b/builtin/fast-export.c @@ -523,10 +523,13 @@ 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 updated eventually. + */ + if (commit->util || commit->object.flags & SHOWN) string_list_append(extra_refs, full_name)->util = commit; - else + if (!commit->util) commit->util = full_name; } } > Initial-patch-by: Johannes Schindelin <Johannes.Schindelin@gmx.de> > Signed-off-by: Sverre Rabbelier <srabbelier@gmail.com> > --- > > Most of the hard work for this patch was done by Dscho. The rest of > it was basically me applying the technique used by jch in c3502fa > (25-08-2011 do not include sibling history in --ancestry-path). > > The if statement dealing with tag_of_filtered_mode is not as > elegant as either me or Dscho would have liked, but we couldn't > find a better way to determine if a ref is a tag at this point > in the code. Which is needed why? Right now if I do: % git fast-export --{im,ex}port-marks=/tmp/marks foo1 tag-to-foo1 Where tag-to-foo1 is a tag that that points to foo1, I get a reset for that. > Additionally, the elem->whence != REV_CMD_RIGHT case should really > check if REV_CMD_RIGHT_REF, but as this is not provided by the > ref_info structure this is left as is. A result of this is that > incorrect input will result in incorrect output, rather than an > error message. That is: `git fast-export a..<sha1>` will > incorrectly generate a `reset <sha1>` statement in the fast-export > stream. I don't see the point of this. Besides, you can check the return value of dwim_ref, if it's not 1, then you shouldn't generate a reset. > The dwim_ref bit is a double work (it has already been done by the > caller of this function), but I decided it would be more work to > pass this information along than to recompute it for the few > commandline refs that were relevant. It's already stored in commit->util, you don't need to do that. As I said, I think the patch above does the trick, and it even has the advantage of not having the above a..<SHA-1> issues. Cheers. -- Felipe Contreras ^ permalink raw reply related [flat|nested] 42+ messages in thread
end of thread, other threads:[~2019-01-29 19:42 UTC | newest] Thread overview: 42+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2011-11-05 23:23 [PATCH 0/3] fast-export fixes Sverre Rabbelier 2011-11-05 23:23 ` [PATCH 1/3] t9350: point out that refs are not updated correctly Sverre Rabbelier 2011-11-06 4:31 ` Jonathan Nieder 2011-11-06 19:38 ` Sverre Rabbelier 2011-11-07 9:32 ` Jonathan Nieder 2012-10-24 17:52 ` Felipe Contreras 2012-10-24 18:08 ` Jonathan Nieder 2012-10-24 19:09 ` Felipe Contreras 2012-10-24 19:11 ` Jonathan Nieder 2012-10-25 4:19 ` Felipe Contreras 2012-10-25 4:27 ` Jonathan Nieder 2012-10-25 5:18 ` Felipe Contreras 2012-10-25 5:28 ` Jonathan Nieder 2012-10-25 5:39 ` Sverre Rabbelier 2012-10-25 5:50 ` Felipe Contreras 2012-10-25 6:07 ` Sverre Rabbelier 2012-10-25 6:19 ` Felipe Contreras 2012-10-25 7:06 ` Sverre Rabbelier 2012-10-25 7:34 ` Jonathan Nieder 2012-10-25 7:43 ` Sverre Rabbelier 2012-10-25 7:48 ` Jonathan Nieder 2012-10-25 7:50 ` Sverre Rabbelier 2012-10-25 13:33 ` Felipe Contreras 2012-10-25 5:40 ` Felipe Contreras 2012-10-25 5:53 ` Jonathan Nieder 2012-10-25 6:39 ` Felipe Contreras 2012-10-25 7:18 ` Jonathan Nieder 2012-10-25 16:43 ` Felipe Contreras 2012-10-24 21:41 ` Johannes Schindelin 2012-10-25 5:13 ` Felipe Contreras 2011-11-05 23:23 ` [PATCH 2/3] fast-export: do not refer to non-existing marks Sverre Rabbelier 2011-11-06 4:45 ` Jonathan Nieder 2011-11-06 19:40 ` Sverre Rabbelier 2019-01-29 19:41 ` Johannes Schindelin 2011-11-05 23:23 ` [PATCH 3/3] fast-export: output reset command for commandline revs Sverre Rabbelier 2011-11-06 5:01 ` Jonathan Nieder 2011-11-06 19:48 ` Sverre Rabbelier 2011-11-07 8:58 ` Jonathan Nieder 2011-11-07 5:52 ` Junio C Hamano 2011-11-07 5:53 ` Junio C Hamano 2011-11-30 16:56 ` Thomas Rast 2012-10-24 18:02 ` Felipe Contreras
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.