patch, 3 partsfast-export fixes

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

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

Sverre Rabbelier wrote:

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.

Makes sense.

Hmm, I didn't know about this behavior. Would it be possible to add a test for it, too?

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

Heya,

On Sun, Nov 6, 2011 at 05:31, Jonathan Nieder <jrnieder@gmail.com> wrote:

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

What behavior are you referring to here? What kind of test would you want added?

Yes, extending the capabilities of fast-export is needed if we want the remote-helpers to be as powerful as native git.

Sverre Rabbelier wrote:

[...]

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.

Hi,

Joined late to the party :)

On Sun, Nov 6, 2011 at 12:23 AM, Sverre Rabbelier <srabbelier@gmail.com> wrote:

That is correctly, but try this: $ git fast-export --{im,ex}port-marks=marksfile foo foo

Now foo is updated.

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:

Hi Felipe,

Felipe Contreras wrote:

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

On Wed, Oct 24, 2012 at 8:08 PM, Jonathan Nieder <jrnieder@gmail.com> 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'?

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

Not everyone uses marks.

Ciao, Jonathan

On Wed, Oct 24, 2012 at 9:11 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:

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

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

On Thu, Oct 25, 2012 at 6:27 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:

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?

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

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

On Wed, Oct 24, 2012 at 10:28 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:

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

On Thu, Oct 25, 2012 at 7:39 AM, Sverre Rabbelier <srabbelier@gmail.com> wrote:

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

On Wed, Oct 24, 2012 at 10:50 PM, Felipe Contreras <felipe.contreras@gmail.com> wrote:

Yes, but as you said:

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.

On Thu, Oct 25, 2012 at 8:07 AM, Sverre Rabbelier <srabbelier@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

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?

If 'git fast-export ^master foo' is catastrophic, so is 'git fast-export foo ^master', and that already exports master *today*.

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.

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

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

On Thu, Oct 25, 2012 at 12:34 AM, Jonathan Nieder <jrnieder@gmail.com> 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.

Sverre Rabbelier wrote:

True. Is it be possible to check UNINTERESTING in revs->cmdline before the walk?

On Thu, Oct 25, 2012 at 12:48 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:

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

On Thu, Oct 25, 2012 at 9:50 AM, Sverre Rabbelier <srabbelier@gmail.com> wrote:

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.

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.

On Thu, Oct 25, 2012 at 7:28 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:

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

That makes sense. So are there any other patches?

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

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

On Thu, Oct 25, 2012 at 7:53 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:

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?

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

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

On Thu, Oct 25, 2012 at 9:18 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:

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.

Hi,

On Wed, 24 Oct 2012, Felipe Contreras wrote:

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

On Wed, Oct 24, 2012 at 11:41 PM, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:

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?

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

Hi,

Sverre Rabbelier wrote:

Hm, seems problematic indeed.

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.

[...]

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.

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?

Heya,

On Sun, Nov 6, 2011 at 05:45, Jonathan Nieder <jrnieder@gmail.com> wrote:

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

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

Hi Sverre,

On Sun, 6 Nov 2011, Sverre Rabbelier wrote:

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

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

Sverre Rabbelier 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).

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.

Could TMP_MARK be used for this?

[...]

Might be clearer in a new function.

Oh, neat.

declaration-after-statement

What happens if dwim_ref fails, perhaps because a ref was deleted in the meantime?

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?

Just curious: is the REF_HANDLED handling actually needed? What would happen if fast-export included the redundant resets?

[...]

Thanks for a pleasant read.

Heya,

On Sun, Nov 6, 2011 at 06:01, Jonathan Nieder <jrnieder@gmail.com> wrote:

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.

All of it? I wasn't sure what part should go in the commit message.

A test_must_fail?

I don't know its usage, is it?

Yes, probably. handle_cmdline_refs?

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

Ah, yes.

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?

Yeah, that does look better :).

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.

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.

That would just be sloppy :). I don't think anything particularly bad would happen.

Thanks for the review.

Sverre Rabbelier wrote:

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.

Yep.

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.

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.

[...]

It's the handle_tag() call, later in handle_tags_and_duplicates().

[...]

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?

[...]

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

Sverre Rabbelier <srabbelier@gmail.com> writes:

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

Sverre Rabbelier <srabbelier@gmail.com> writes:

Just a nit I've already fixed locally (iow no need to resend only to fix this) but this is decl-after-stmt.

Sverre Rabbelier wrote:

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

On Sun, Nov 6, 2011 at 12:23 AM, Sverre Rabbelier <srabbelier@gmail.com> wrote:

This simpler patch does the same, doesn't it?