[PATCH 0/3] fast-export fixes

[PATCH 0/3] fast-export fixes

[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

[PATCH 1/3] t9350: point out that refs are not updated correctly

[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

[PATCH 2/3] fast-export: do not refer to non-existing marks

[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

[PATCH 3/3] fast-export: output reset command for commandline revs

[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

Re: [PATCH 1/3] t9350: point out that refs are not updated correctly

[Jonathan Nieder]

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

Re: [PATCH 2/3] fast-export: do not refer to non-existing marks

[Jonathan Nieder]

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?

Re: [PATCH 3/3] fast-export: output reset command for commandline revs

[Jonathan Nieder]

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.

Re: [PATCH 1/3] t9350: point out that refs are not updated correctly

[Sverre Rabbelier]

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

Re: [PATCH 2/3] fast-export: do not refer to non-existing marks

[Sverre Rabbelier]

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

Re: [PATCH 3/3] fast-export: output reset command for commandline revs

[Sverre Rabbelier]

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

Re: [PATCH 3/3] fast-export: output reset command for commandline revs

[Junio C Hamano]

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

Re: [PATCH 3/3] fast-export: output reset command for commandline revs

[Junio C Hamano]

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.

Re: [PATCH 3/3] fast-export: output reset command for commandline revs

[Jonathan Nieder]

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

Re: [PATCH 1/3] t9350: point out that refs are not updated correctly

[Jonathan Nieder]

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.

Re: [PATCH 3/3] fast-export: output reset command for commandline revs

[Thomas Rast]

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

Re: [PATCH 1/3] t9350: point out that refs are not updated correctly

[Felipe Contreras]

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

Re: [PATCH 3/3] fast-export: output reset command for commandline revs

[Felipe Contreras]

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

Re: [PATCH 1/3] t9350: point out that refs are not updated correctly

[Jonathan Nieder]

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

Re: [PATCH 1/3] t9350: point out that refs are not updated correctly

[Felipe Contreras]

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

Re: [PATCH 1/3] t9350: point out that refs are not updated correctly

[Jonathan Nieder]

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

Re: [PATCH 1/3] t9350: point out that refs are not updated correctly

[Johannes Schindelin]

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

Re: [PATCH 1/3] t9350: point out that refs are not updated correctly

[Felipe Contreras]

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

Re: [PATCH 1/3] t9350: point out that refs are not updated correctly

[Jonathan Nieder]

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

Re: [PATCH 1/3] t9350: point out that refs are not updated correctly

[Felipe Contreras]

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

Re: [PATCH 1/3] t9350: point out that refs are not updated correctly

[Felipe Contreras]

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

Re: [PATCH 1/3] t9350: point out that refs are not updated correctly

[Jonathan Nieder]

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

Re: [PATCH 1/3] t9350: point out that refs are not updated correctly

[Sverre Rabbelier]

On Wed, Oct 24, 2012 at 10:28 PM, Jonathan Nieder <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

Re: [PATCH 1/3] t9350: point out that refs are not updated correctly

[Felipe Contreras]

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

Re: [PATCH 1/3] t9350: point out that refs are not updated correctly

[Felipe Contreras]

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

Re: [PATCH 1/3] t9350: point out that refs are not updated correctly

[Jonathan Nieder]

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

Re: [PATCH 1/3] t9350: point out that refs are not updated correctly

[Sverre Rabbelier]

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

Re: [PATCH 1/3] t9350: point out that refs are not updated correctly

[Felipe Contreras]

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

Re: [PATCH 1/3] t9350: point out that refs are not updated correctly

[Felipe Contreras]

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

Re: [PATCH 1/3] t9350: point out that refs are not updated correctly

[Sverre Rabbelier]

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

Re: [PATCH 1/3] t9350: point out that refs are not updated correctly

[Jonathan Nieder]

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

Re: [PATCH 1/3] t9350: point out that refs are not updated correctly

[Jonathan Nieder]

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.

Re: [PATCH 1/3] t9350: point out that refs are not updated correctly

[Sverre Rabbelier]

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

Re: [PATCH 1/3] t9350: point out that refs are not updated correctly

[Jonathan Nieder]

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?

Re: [PATCH 1/3] t9350: point out that refs are not updated correctly

[Sverre Rabbelier]

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

Re: [PATCH 1/3] t9350: point out that refs are not updated correctly

[Felipe Contreras]

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

Re: [PATCH 1/3] t9350: point out that refs are not updated correctly

[Felipe Contreras]

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

Re: [PATCH 2/3] fast-export: do not refer to non-existing marks

[Johannes Schindelin]

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