[PATCH 0/5] fast-export: prepare for remote helpers awesomeness

[PATCH 0/5] fast-export: prepare for remote helpers awesomeness

[Sverre Rabbelier]

During a funathon with a lot of sheep baaah-ing in the background, we
discovered this ancient fast-export bug while working on finishing
git-remote-hg. So, here it goes.

Johannes Schindelin (4):
  fast-export: do not refer to non-existing marks
  setup_revisions: remember whether a ref was positive or not
  fast-export: do not export negative refs
  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  |   55 +++++++++++++++++++++++++++++++++++++++--------
 object.h               |    2 +-
 revision.c             |   12 ++++++----
 t/t9350-fast-export.sh |   11 +++++++++
 4 files changed, 64 insertions(+), 16 deletions(-)

-- 
1.7.6.385.g91185.dirty

[PATCH 1/5] 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 basec remote helpers.

Signed-off-by: Sverre Rabbelier <srabbelier@gmail.com>
---

  Obviously the real occurance of the breakage is not as artificial
  as this test case, but it's a very simple minimal reproducer.

 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 f823c05..ed0417a 100755
--- a/t/t9350-fast-export.sh
+++ b/t/t9350-fast-export.sh
@@ -414,4 +414,15 @@ test_expect_success SYMLINKS 'directory becomes symlink'        '
 	(cd result && git show master:foo)
 '
 
+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.6.385.g91185.dirty

[PATCH 2/5] 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').

Signed-off-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Signed-off-by: Sverre Rabbelier <srabbelier@gmail.com>
---

  We encountered this one while staring at the code and trying to
  figure out if our existing solution was sufficient.

 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 becef85..92743c8 100644
--- a/builtin/fast-export.c
+++ b/builtin/fast-export.c
@@ -512,9 +512,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--) {
@@ -526,9 +537,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.6.385.g91185.dirty

[PATCH 3/5] setup_revisions: remember whether a ref was positive or not

[Sverre Rabbelier]

From: Johannes Schindelin <Johannes.Schindelin@gmx.de>

This will be required by fast-export, when no commits were
exported, but the refs should be set, of course.

Signed-off-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Signed-off-by: Sverre Rabbelier <srabbelier@gmail.com>
---

  This is just in preperation of fixing the actual bug.

 object.h   |    2 +-
 revision.c |   12 +++++++-----
 2 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/object.h b/object.h
index b6618d9..a28cd45 100644
--- a/object.h
+++ b/object.h
@@ -12,7 +12,7 @@ struct object_array {
 	struct object_array_entry {
 		struct object *item;
 		const char *name;
-		unsigned mode;
+		unsigned mode, flags;
 	} *objects;
 };
 
diff --git a/revision.c b/revision.c
index c46cfaa..72c3ee5 100644
--- a/revision.c
+++ b/revision.c
@@ -131,7 +131,7 @@ void mark_parents_uninteresting(struct commit *commit)
 	}
 }
 
-static void add_pending_object_with_mode(struct rev_info *revs, struct object *obj, const char *name, unsigned mode)
+static void add_pending_object_with_mode(struct rev_info *revs, struct object *obj, const char *name, unsigned mode, unsigned flags)
 {
 	if (!obj)
 		return;
@@ -152,11 +152,12 @@ static void add_pending_object_with_mode(struct rev_info *revs, struct object *o
 			return;
 	}
 	add_object_array_with_mode(obj, name, &revs->pending, mode);
+	revs->pending.objects[revs->pending.nr-1].flags = flags;
 }
 
 void add_pending_object(struct rev_info *revs, struct object *obj, const char *name)
 {
-	add_pending_object_with_mode(revs, obj, name, S_IFINVALID);
+	add_pending_object_with_mode(revs, obj, name, S_IFINVALID, 0);
 }
 
 void add_head_to_pending(struct rev_info *revs)
@@ -1073,7 +1074,8 @@ int handle_revision_arg(const char *arg, struct rev_info *revs,
 			} else
 				a->object.flags |= flags_exclude;
 			b->object.flags |= flags;
-			add_pending_object(revs, &a->object, this);
+			add_pending_object_with_mode(revs, &a->object, this,
+						     S_IFINVALID, flags_exclude);
 			add_pending_object(revs, &b->object, next);
 			return 0;
 		}
@@ -1103,7 +1105,7 @@ int handle_revision_arg(const char *arg, struct rev_info *revs,
 	if (!cant_be_filename)
 		verify_non_filename(revs->prefix, arg);
 	object = get_reference(revs, arg, sha1, flags ^ local_flags);
-	add_pending_object_with_mode(revs, object, arg, mode);
+	add_pending_object_with_mode(revs, object, arg, mode, local_flags);
 	return 0;
 }
 
@@ -1708,7 +1710,7 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
 		if (get_sha1_with_mode(revs->def, sha1, &mode))
 			die("bad default revision '%s'", revs->def);
 		object = get_reference(revs, revs->def, sha1, 0);
-		add_pending_object_with_mode(revs, object, revs->def, mode);
+		add_pending_object_with_mode(revs, object, revs->def, mode, 0);
 	}
 
 	/* Did the user ask for any diff output? Run the diff! */
-- 
1.7.6.385.g91185.dirty

[PATCH 4/5] fast-export: do not export negative refs

[Sverre Rabbelier]

From: Johannes Schindelin <Johannes.Schindelin@gmx.de>

When calling `git fast-export master..next` we want to export
refs/heads/next, but not refs/heads/master.

Currently this is not a problem, because negative refs' commits
are never shown. In the next commit this will be changed in order
to make sure that 'master..master' does export master. I.e. even
refs whose commits are not shown are exported.

Signed-off-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Signed-off-by: Sverre Rabbelier <srabbelier@gmail.com>
---

  We noticed this while trying to come up with a test case for the
  bug we found.

 builtin/fast-export.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/builtin/fast-export.c b/builtin/fast-export.c
index 92743c8..628eab0 100644
--- a/builtin/fast-export.c
+++ b/builtin/fast-export.c
@@ -467,7 +467,7 @@ static void get_tags_and_duplicates(struct object_array *pending,
 		struct commit *commit = commit;
 		char *full_name;
 
-		if (dwim_ref(e->name, strlen(e->name), sha1, &full_name) != 1)
+		if ((e->flags & UNINTERESTING) || dwim_ref(e->name, strlen(e->name), sha1, &full_name) != 1)
 			continue;
 
 		switch (e->item->type) {
-- 
1.7.6.385.g91185.dirty

[PATCH 5/5] setup_revisions: remember whether a ref was positive or not

[Sverre Rabbelier]

From: Johannes Schindelin <Johannes.Schindelin@gmx.de>

This will be required by fast-export, when no commits were
exported, but the refs should be set, of course.

Signed-off-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Signed-off-by: Sverre Rabbelier <srabbelier@gmail.com>
---

  And then finally, the fix. Thanks for reading.

 builtin/fast-export.c  |   36 +++++++++++++++++++++++++++++++-----
 t/t9350-fast-export.sh |    2 +-
 2 files changed, 32 insertions(+), 6 deletions(-)

diff --git a/builtin/fast-export.c b/builtin/fast-export.c
index 628eab0..9c24f4e 100644
--- a/builtin/fast-export.c
+++ b/builtin/fast-export.c
@@ -17,6 +17,8 @@
 #include "utf8.h"
 #include "parse-options.h"
 
+#define REF_HANDLED (ALL_REV_FLAGS + 1)
+
 static const char *fast_export_usage[] = {
 	"git fast-export [rev-list-opts]",
 	NULL
@@ -456,6 +458,7 @@ static void handle_tag(const char *name, struct tag *tag)
 }
 
 static void get_tags_and_duplicates(struct object_array *pending,
+				    struct string_list *refs,
 				    struct string_list *extra_refs)
 {
 	struct tag *tag;
@@ -507,8 +510,11 @@ static void get_tags_and_duplicates(struct object_array *pending,
 		if (commit->util)
 			/* more than one name for the same object */
 			string_list_append(extra_refs, full_name)->util = commit;
-		else
+		else {
 			commit->util = full_name;
+			/* we might need to set this ref explicitly */
+			string_list_append(refs, full_name)->util = commit;
+		}
 	}
 }
 
@@ -524,10 +530,29 @@ 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 string_list *refs, struct string_list *extra_refs)
 {
 	int i;
 
+	/* even if no commits were exported, we need to export the ref */
+	for (i = refs->nr - 1; i >= 0; i--) {
+		const char *name = refs->items[i].string;
+		struct object *object = refs->items[i].util;
+
+		if (!(object->flags & REF_HANDLED)) {
+			if (object->type & OBJ_TAG)
+				handle_tag(name, (struct tag *)object);
+			else {
+				if (!prefixcmp(name, "refs/tags/") &&
+				    (tag_of_filtered_mode != REWRITE ||
+				     !get_object_mark(object)))
+					continue;
+				handle_reset(name, object);
+				object->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;
@@ -617,7 +642,7 @@ int cmd_fast_export(int argc, const char **argv, const char *prefix)
 {
 	struct rev_info revs;
 	struct object_array commits = OBJECT_ARRAY_INIT;
-	struct string_list extra_refs = STRING_LIST_INIT_NODUP;
+	struct string_list refs = STRING_LIST_INIT_NODUP, extra_refs = STRING_LIST_INIT_NODUP;
 	struct commit *commit;
 	char *export_filename = NULL, *import_filename = NULL;
 	struct option options[] = {
@@ -669,7 +694,7 @@ int cmd_fast_export(int argc, const char **argv, const char *prefix)
 	if (import_filename && revs.prune_data.nr)
 		full_tree = 1;
 
-	get_tags_and_duplicates(&revs.pending, &extra_refs);
+	get_tags_and_duplicates(&revs.pending, &refs, &extra_refs);
 
 	if (prepare_revision_walk(&revs))
 		die("revision walk setup failed");
@@ -681,11 +706,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(&refs, &extra_refs);
 
 	if (export_filename)
 		export_marks(export_filename);
diff --git a/t/t9350-fast-export.sh b/t/t9350-fast-export.sh
index ed0417a..d2d7ef8 100755
--- a/t/t9350-fast-export.sh
+++ b/t/t9350-fast-export.sh
@@ -420,7 +420,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.6.385.g91185.dirty

Re: [PATCH 4/5] fast-export: do not export negative refs

[Junio C Hamano]

Sverre Rabbelier <srabbelier@gmail.com> writes:

> ... In the next commit this will be changed in order
> to make sure that 'master..master' does export master.

Sorry, but you need to be much more clear why this could be a good
thing.

Everywhere in git a range A..B means "reachable from B but exclude
everything that is reachable from A", and with that knowledge it is
natural to expect that master..master should yield an empty set. If you
need to change it for some reason, we need to know why, and if you are
doing it for a narrow special case, we need to know why that case is so
special.

Re: [PATCH 3/5] setup_revisions: remember whether a ref was positive or not

[Junio C Hamano]

Sverre Rabbelier <srabbelier@gmail.com> writes:

>  void add_pending_object(struct rev_info *revs, struct object *obj, const char *name)
>  {
> -	add_pending_object_with_mode(revs, obj, name, S_IFINVALID);
> +	add_pending_object_with_mode(revs, obj, name, S_IFINVALID, 0);
>  }

This seems utterly broken.  For example, fmt-merge-msg.c adds "^HEAD" and
of course the flags on the object is UNINTERESTING. Has all the callers of
add_pending_object() been verified? Why is it passing an unconditional 0
and not !!(obj->flags & UNINTERESTING) or something?

If the excuse is "this is only to help fast-export and other callers of
add_pending_object() does not care", that is a sloppy attitude that breaks
maintainability of the code (because it forgets to add "in the current
code nobody looks at the new 'flags' field" to that excuse, and also does
not have any comments around this code that says so); it is questionable
if such a hack belongs to a patch that touches object.h.

> @@ -1073,7 +1074,8 @@ int handle_revision_arg(const char *arg, struct rev_info *revs,
>  			} else
>  				a->object.flags |= flags_exclude;
>  			b->object.flags |= flags;
> -			add_pending_object(revs, &a->object, this);
> +			add_pending_object_with_mode(revs, &a->object, this,
> +						     S_IFINVALID, flags_exclude);
> ...
> @@ -1103,7 +1105,7 @@ int handle_revision_arg(const char *arg, struct rev_info *revs,
>  	if (!cant_be_filename)
>  		verify_non_filename(revs->prefix, arg);
>  	object = get_reference(revs, arg, sha1, flags ^ local_flags);
> -	add_pending_object_with_mode(revs, object, arg, mode);
> +	add_pending_object_with_mode(revs, object, arg, mode, local_flags);
>  	return 0;
>  }

Questionable.  Did the user mean to say Z is positive when he said

	$ git rev-list A B ^C ... --not G H ... ^Z

Re: [PATCH 3/5] setup_revisions: remember whether a ref was positive or not

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Questionable.  Did the user mean to say Z is positive when he said
>
> 	$ git rev-list A B ^C ... --not G H ... ^Z

Having said all that, I think you wanted a way to reconstruct various
different command lines that ends up in the same set of "pending objects",
and I do think that is something we need to address, as our command line
parsing heavily depend on the preprocessing phase of the revision
traversal machinery, and some commands do want to act differently between
the case when they are given "master" vs "master^0" for example (i.e. does
the user mean the branch as a whole or the commit at the tip?  If the
former the command may want to do something to the refs/heads/master while
the latter the command may work only on the commit or outright reject the
request saying "I only work on a branch").

In other words, I am not opposed to an effort to give the callers to the
"pending objects" machinery a better way to discover what the user told us
from the command line, giving them more than just "at the end of the
UNINTERESTING marking here are the objects listed on the command line and
you can look at their flags".  For example, some commands may want to tell
"a..b" and "^a b" apart, and other commands may want to tell what "a" was
when the user asked for exotic things like "a^@" or "a^!".

It may make sense to change the new "flag" field that can express only one
bit in your implementation to something more descriptive to express "what
command line option did this come from".  For example,

	git cmd a...b

that calls setup_revisions() may put "a" (positive), "b" (positive), and
zero or more commits that are their merge bases (negative) in pending
objects queue. Right now, these pending object entries may have some part
of the string being parsed as their name, but we may want to annotate all
the object array entries that result from "a...b" with the same "a...b"
(or a structure that represents its parsed form), so that the caller can
notice things like (1) they came from the single command line argument,
(2) the user didn't list random negative commits but they are meant as
merge bases between a and b, (3) a and b were written as such, not as
their abbreviated object names, etc.

Then "git fast-export master..master" can notice that there are two object
array entries in the pending object queue, they came from the same command
line argument, whose parse tree is a "unsymmetric difference between
string 'master' and string 'master'", and take the last piece to convince
itself that the user is talking about the master branch, even though as
the set arithmetic the result is an empty set, and do something
intelligent about the master branch.

Re: [PATCH 4/5] fast-export: do not export negative refs

[Johannes Schindelin]

Hi Junio,

On Sun, 24 Jul 2011, Junio C Hamano wrote:

> Sverre Rabbelier <srabbelier@gmail.com> writes:
> 
> > ... In the next commit this will be changed in order to make sure that 
> > 'master..master' does export master.
> 
> Sorry, but you need to be much more clear why this could be a good 
> thing.

Maybe we should have written 'master^0..master' or 'blub..blah' where 
'blah' points to the same commit as 'blub'.

The problem is that we need to be able to tell which _ref_ is 
uninteresting, not which _commit_, since one and the same commit can be 
referenced by an uninteresting and an interesting ref.

Hopefully you will excuse our being a little incoherent after a codathon,
Dscho

Re: [PATCH 3/5] setup_revisions: remember whether a ref was positive or not

[Johannes Schindelin]

Hi,

On Sun, 24 Jul 2011, Junio C Hamano wrote:

> Sverre Rabbelier <srabbelier@gmail.com> writes:
> 
> >  void add_pending_object(struct rev_info *revs, struct object *obj, const char *name)
> >  {
> > -	add_pending_object_with_mode(revs, obj, name, S_IFINVALID);
> > +	add_pending_object_with_mode(revs, obj, name, S_IFINVALID, 0);
> >  }
> 
> This seems utterly broken.  For example, fmt-merge-msg.c adds "^HEAD" 
> and of course the flags on the object is UNINTERESTING. Has all the 
> callers of add_pending_object() been verified? Why is it passing an 
> unconditional 0 and not !!(obj->flags & UNINTERESTING) or something?
> 
> If the excuse is "this is only to help fast-export and other callers of 
> add_pending_object() does not care", that is a sloppy attitude that 
> breaks maintainability of the code (because it forgets to add "in the 
> current code nobody looks at the new 'flags' field" to that excuse, and 
> also does not have any comments around this code that says so); it is 
> questionable if such a hack belongs to a patch that touches object.h.

I hope you appreciate our effort not to hack this into the 'mode' 
variable. That variable is exactly in the spirit you mentioned, so I 
figured we'd be fine with a similar approach.

Now that you have made clear that it is not fine, would you please care to 
enlighten me how you would prefer the change to look like that makes the 
argument parsing in revision.c remember which _refs_ (as opposed to 
_commits_) were marked uninteresting?

Thank you very much,
Johannes

Re: [PATCH 3/5] setup_revisions: remember whether a ref was positive or not

[Sverre Rabbelier]

Heya,

On Mon, Jul 25, 2011 at 01:17, Junio C Hamano <gitster@pobox.com> wrote:
> In other words, I am not opposed to an effort to give the callers to the
> "pending objects" machinery a better way to discover what the user told us
> from the command line, giving them more than just "at the end of the
> UNINTERESTING marking here are the objects listed on the command line and
> you can look at their flags".  For example, some commands may want to tell
> "a..b" and "^a b" apart, and other commands may want to tell what "a" was
> when the user asked for exotic things like "a^@" or "a^!".

I agree that such might be useful, but since we currently do not need
something as advanced as that, I'm hesitant to implement something
(far) more advanced than what we need (YAGNI [0]). So I'm thinking
that perhaps there's a suitable middle ground where we stick with the
current flags approach until someone needs something more advanced so
as to make sure that it's implemented in a way that meets an actual
implementer need?

On Sun, Jul 24, 2011 at 21:23, Junio C Hamano <gitster@pobox.com> wrote:
> Sverre Rabbelier <srabbelier@gmail.com> writes:
>
>>  void add_pending_object(struct rev_info *revs, struct object *obj, const char *name)
>>  {
>> -     add_pending_object_with_mode(revs, obj, name, S_IFINVALID);
>> +     add_pending_object_with_mode(revs, obj, name, S_IFINVALID, 0);
>>  }
>
> This seems utterly broken.  For example, fmt-merge-msg.c adds "^HEAD" and
> of course the flags on the object is UNINTERESTING. Has all the callers of
> add_pending_object() been verified? Why is it passing an unconditional 0
> and not !!(obj->flags & UNINTERESTING) or something?

If I understand correctly (and it's not unlikely that I don't), the
'flags' field is used to store the actual flags (not just a boolean).
Would the following be appropriate?

+     add_pending_object_with_mode(revs, obj, name, S_IFINVALID, obj->flags);

Hopefully Dscho remembers enough of our hacking to be able to answer
that question.

If it is, do you still think it's "utterly broken" or would that be
something we can work with?

[0] http://c2.com/xp/YouArentGonnaNeedIt.html

-- 
Cheers,

Sverre Rabbelier

Re: [PATCH 3/5] setup_revisions: remember whether a ref was positive or not

[Junio C Hamano]

Sverre Rabbelier <srabbelier@gmail.com> writes:

> On Sun, Jul 24, 2011 at 21:23, Junio C Hamano <gitster@pobox.com> wrote:
>> Sverre Rabbelier <srabbelier@gmail.com> writes:
>>
>>>  void add_pending_object(struct rev_info *revs, struct object *obj, const char *name)
>>>  {
>>> -     add_pending_object_with_mode(revs, obj, name, S_IFINVALID);
>>> +     add_pending_object_with_mode(revs, obj, name, S_IFINVALID, 0);
>>>  }
>>
>> This seems utterly broken.  For example, fmt-merge-msg.c adds "^HEAD" and
>> of course the flags on the object is UNINTERESTING. Has all the callers of
>> add_pending_object() been verified? Why is it passing an unconditional 0
>> and not !!(obj->flags & UNINTERESTING) or something?
>
> If I understand correctly (and it's not unlikely that I don't), the
> 'flags' field is used to store the actual flags (not just a boolean).
> Would the following be appropriate?
>
> +     add_pending_object_with_mode(revs, obj, name, S_IFINVALID, obj->flags);

I would think that the information you are trying to convey is more in
line with the spirit of "name" field, not "mode". Originally, the pending
object array was to only collect the given arguments at the object level
with "is this interesting or uninteresting?" flag and nothing more, but we
later wanted to have richer information to deduce what the user wanted
from how the user gave the object information to us (i.e. "not just the
tree object named by the object name, but I meant the object at this
path"), primarily to give a better error message.

Let's clarify what I hinted as "some parsed representation of how the revs
were specified" in an earlier message with a concrete example. There is a
code block at the end of builtin/diff.c that says something like:

	If we have more than 2 tree-ishes, and the first one is marked as
	UNINTERESTING, it must have come from "diff A...B". In this case,
	we know ent[0] is one of the merge-bases, ent[ents-2] is A, and
	ent[ents-1] is B, so turn it into "diff ent[0] B".

These entries in ent[] come from the pending objects array, and because
the revision machinery loses information regarding how the command line
arguments were spelled, we have to play such games to _guess_ what the
user meant to say. This will lead to "diff ^C A B" running "diff C B",
instead of barfing with "What are you talking about???", which should
happen instead.

Wouldn't it be wonderful if the revision machinery left richer clue in
each element of the pending object array while parsing, so that the caller
does not have to guess?  For example, suppose that it parsed "A...B" into
this N element array of pending objects:

	^MB0 (the first merge base of A and B)
	^MB1 (the second merge base of A and B)
        ...
	A
	B

In addition to a single "mode" integer, which says if it is supposed to be
a tree or a blob, we could allocate a single structure that records
something like this:

	struct parsed_rev {
                enum {
			SHA1, REF, RANGE, SYMMETRIC_RANGE, REFLOG_ENT, ...
                        // there are others like OBJ^!, OBJ@!, ...
                } kind;
                const char *string;
                union {
			struct {
				const char *real_ref;
			} ref;
			struct {
				struct parsed_rev *bottom;
				struct parsed_rev *top;
                        } range;
			...
                } u;
	};

And then all the elements in the pending object array resulting from
parsing "maint...master" would all point at a single instance of this
"struct parsed_rev" that may read like this:

	{
        	.kind = SYMMETRIC_RANGE;
		.string = "maint...master";
                .u.range = {
                	.bottom = {
                        	.kind = REF;
                                .string = "maint";
                		.u.ref.real_ref = "refs/heads/maint";
			};
                	.top = {
                        	.kind = REF;
                                .string = "master";
                		.u.ref.real_ref = "refs/heads/master";
			};
		};
	};

In a similar fashion, if you wanted to make sure that you do not discard
"master" as totally uninteresting, even though the end user gave you a
list of arguments that ends up marking the commit object "master" as
uninteresting, e.g. "master^0..master", you could do so if we updated the
revision parsing machinery so that the resulting two elements in the
pending array would both point at (in addition to the "uninteresting
commit object 'master^0'") a structure that would look like this:

	{
        	.kind = RANGE;
                .string = "master^0..master";
                .u.range = {
			...
                        .top = {
				.kind = REF;
                                .string = "master";
                		.u.ref.real_ref = "refs/heads/master";
			};
		};
	};

And by looking at .u.range.top, you know that the user meant to do
something to the "master" branch.

Re: [PATCH 3/5] setup_revisions: remember whether a ref was positive or not

[Johannes Schindelin]

Hi Junio,

On Wed, 3 Aug 2011, Junio C Hamano wrote:

> Sverre Rabbelier <srabbelier@gmail.com> writes:
> 
> > On Sun, Jul 24, 2011 at 21:23, Junio C Hamano <gitster@pobox.com> wrote:
> >> Sverre Rabbelier <srabbelier@gmail.com> writes:
> >>
> >>>  void add_pending_object(struct rev_info *revs, struct object *obj, const char *name)
> >>>  {
> >>> -     add_pending_object_with_mode(revs, obj, name, S_IFINVALID);
> >>> +     add_pending_object_with_mode(revs, obj, name, S_IFINVALID, 0);
> >>>  }
> >>
> >> This seems utterly broken.  For example, fmt-merge-msg.c adds "^HEAD" 
> >> and of course the flags on the object is UNINTERESTING. Has all the 
> >> callers of add_pending_object() been verified? Why is it passing an 
> >> unconditional 0 and not !!(obj->flags & UNINTERESTING) or something?

If you do that, you're back to start. Since obj has not the faintest clue 
whether the pending object was added from a negative or a positive ref.

And yes, all the callers have been verified: I don't want them to change. 
I only need to catch the callers inside the revision machinery's argument 
parsing, since that is what I need anyway, to traverse the objects later 
on. And rather than reimplementing the wheel^Wargument parsing, I'd like 
to reuse what is already there and just forgets the single bit of 
information I need.

The other users of the argument parsing can change their callers if 
necessary, when they also need the information about whether a ref was 
positive/negative.

> > If I understand correctly (and it's not unlikely that I don't), the 
> > 'flags' field is used to store the actual flags (not just a boolean). 
> > Would the following be appropriate?
> >
> > +     add_pending_object_with_mode(revs, obj, name, S_IFINVALID, obj->flags);
> 
> I would think that the information you are trying to convey is more in
> line with the spirit of "name" field, not "mode".

No, it is a flag. The parsed argument is a negative ref. It has a name 
(not including the "^") and it has a flag ("negative").

The mode is indeed only relevant for non-commitishs: If you look where a 
pending object is added _with_ a mode other than S_IFINVALID, it is only 
when calling get_sha1_with_mode(). That function indirectly calls 
get_sha1_with_context_1() which sets the mode to ce_mode. In other words, 
the mode is the file mode when specifying a blob/tree as <ref>:<path>. All 
commits (and tags) have a mode set to S_IFINVALID.

> [...]
>
> Wouldn't it be wonderful if the revision machinery left richer clue in
> each element of the pending object array while parsing, so that the caller
> does not have to guess?

That is what we are trying to solve, and rather than to reuse the "mode" I 
thought that it'd be wiser to add new "flags".

Many of the richer clues you refer to could be expressed as such flags, 
including the problem I need to address.

For the only itch I have is to get remote-hg working nicely for me, and 
Sverre said I can only have that if fast-export is running correctly, i.e. 
updating all the refs, even if some other ref already points at the same 
commit.

> In addition to a single "mode" integer, which says if it is supposed to 
> be a tree or a blob, we could allocate a single structure that records 
> something like this:
> 
> 	struct parsed_rev {
>                 enum {
> 			SHA1, REF, RANGE, SYMMETRIC_RANGE, REFLOG_ENT, ...
>                         // there are others like OBJ^!, OBJ@!, ...
>                 } kind;
>                 const char *string;
>                 union {
> 			struct {
> 				const char *real_ref;
> 			} ref;
> 			struct {
> 				struct parsed_rev *bottom;
> 				struct parsed_rev *top;
>                         } range;
> 			...
>                 } u;
> 	};

Is this not a little bit of a big, huge, tremendous overkill? All I need 
right now is a simple flag. Let's not cross bridges at which we haven't 
arrived for another 50 miles yet.

Or in other words: I'd rather stay with a simple, elegant, minimal patch 
that solves the problem at hand while not preventing future enhancements.

Also remember that the pending objects array is used for the complete 
object traversal. If you make that bigger you might end up with a 
substantial increase in memory consumption (which made me think of 
reusing the "mode", but then I thought about the possible side effects in 
the future and rejected that idea).

The only way to convince me to do that complicated stuff is by 
blackmailing me, stating that I cannot have a working remote-hg without 
doing all this nested/union/self-referential struct work I don't need.

Ciao,
Johannes

Re: [PATCH 3/5] setup_revisions: remember whether a ref was positive or not

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> Hi Junio,
>
> On Wed, 3 Aug 2011, Junio C Hamano wrote:
>
>> Sverre Rabbelier <srabbelier@gmail.com> writes:
>> 
>> > On Sun, Jul 24, 2011 at 21:23, Junio C Hamano <gitster@pobox.com> wrote:
>> >> Sverre Rabbelier <srabbelier@gmail.com> writes:
>> >>
>> >>>  void add_pending_object(struct rev_info *revs, struct object *obj, const char *name)
>> >>>  {
>> >>> -     add_pending_object_with_mode(revs, obj, name, S_IFINVALID);
>> >>> +     add_pending_object_with_mode(revs, obj, name, S_IFINVALID, 0);
>> >>>  }
> ...
> If you do that, you're back to start. Since obj has not the faintest clue 
> whether the pending object was added from a negative or a positive ref.

But the point is that this codepath does not have a faintest clue whether
the "obj" parameter is something the end user actively asked for (which
might have been marked as uninteresting for other reasons, namely, because
it is reachable from other negative refs). So passing unconditional 0 is
just as bad.

>> Wouldn't it be wonderful if the revision machinery left richer clue in
>> each element of the pending object array while parsing, so that the caller
>> does not have to guess?
>
> That is what we are trying to solve, and rather than to reuse the "mode" I 
> thought that it'd be wiser to add new "flags".

I never thought about nor suggested touching "mode" ;-) It does not have
enough width for the necessary information.

> Many of the richer clues you refer to could be expressed as such flags, 
> including the problem I need to address.
>
>> In addition to a single "mode" integer, which says if it is supposed to 
>> be a tree or a blob, we could allocate a single structure that records... 
>
> Is this not a little bit of a big, huge, tremendous overkill?

As long as you can show your "flags" can (be extended to) express the same
richness to solve sample problems I mentioned in my response, as well as
your immediate issue, I wouldn't insist implementing a parsed struct/union
that may be a more (and unnecessarily) verbose way to say the same thing.

> Or in other words: I'd rather stay with a simple, elegant, minimal patch 
> that solves the problem at hand while not preventing future enhancements.

We are on the same page, but what I read from the patch didn't show a
clear way forward to extend the "flags" to allow the stuff I mentioned
(and the stuff I didn't but obviously fall into the same category of "we
wish revision parsing machinery left us richer information").

> Also remember that the pending objects array is used for the complete 
> object traversal.

My reading and rememberance of the code around add_parents_to_list() must
be quite different from yours. It is not 2006 anymore.

We didn't even have a separate pending object list type until 1f1e895 (Add
"named object array" concept, 2006-06-19) and used the same object_list,
whose element size _did_ matter as you mention. But that commit allowed us
to give elements on the pending list that came directly from the end user
richer semantics than those of the object_list that we discover during the
traversal, and that is why back at e5709a4 (add add_object_array_with_mode,
2007-04-22) we gave more information to it without having to fatten
object_list elements.

> The only way to convince me to do that complicated stuff is by 
> blackmailing me, stating that I cannot have a working remote-hg without 
> doing all this nested/union/self-referential struct work I don't need.

I would be reluctant to accept a myopic hack that is only good for one
caller and that needs to be ripped out and re-done, especially when we
already know other issues that can be solved cleanly if you go a little
further in the initial round.

As I said, I am not married to the verbose struct/union representation
(the only reason I showed that verbosity was because it allowed me to do
away without having to enumerate all the syntax sugars we already
support); if your "flags" can express the same thing (it may needs to
become a bitfield with enough width, but I highly suspect that you would
also need at least a component that says "this is the string the user gave
us --- the user said 'master', not 'master^0', for example) and is a lot
more compact, that is definitely we want to go with.

Thanks.

Re: [PATCH 3/5] setup_revisions: remember whether a ref was positive or not

[Sverre Rabbelier]

Heya,

On Mon, Aug 8, 2011 at 19:47, Junio C Hamano <gitster@pobox.com> wrote:
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>> If you do that, you're back to start. Since obj has not the faintest clue
>> whether the pending object was added from a negative or a positive ref.
>
> But the point is that this codepath does not have a faintest clue whether
> the "obj" parameter is something the end user actively asked for (which
> might have been marked as uninteresting for other reasons, namely, because
> it is reachable from other negative refs). So passing unconditional 0 is
> just as bad.

Doesn't passing 0 indicate that we _did not_ receive any explicit user
input on this ref, which is exactly what we want to record? If the
user passed us any explicit input on the ref, we record it at the
other call-sites, here, the user told us nothing, so we record exactly
that, nothing.

>> Is this not a little bit of a big, huge, tremendous overkill?
>
> As long as you can show your "flags" can (be extended to) express the same
> richness to solve sample problems I mentioned in my response, as well as
> your immediate issue, I wouldn't insist implementing a parsed struct/union
> that may be a more (and unnecessarily) verbose way to say the same thing.

I cannot recall you ever asking somebody to implement some feature
_nobody needs right now_ while trying to fix a _bug_, why now? I do
not know this code well enough to implement it, and Dscho doesn't have
the time to do it.

>> Or in other words: I'd rather stay with a simple, elegant, minimal patch
>> that solves the problem at hand while not preventing future enhancements.
>
> We are on the same page, but what I read from the patch didn't show a
> clear way forward to extend the "flags" to allow the stuff I mentioned

Nobody needs the stuff you mentioned right now. We do need this to fix
this bug. If someone else does want it at a later date, replacing it
will be exactly as much work with or without this patch.

> I would be reluctant to accept a myopic hack that is only good for one
> caller and that needs to be ripped out and re-done, especially when we
> already know other issues that can be solved cleanly if you go a little
> further in the initial round.

While I understand this reluctance, remember that this "one caller" is
required to fix a bug in the current code. If you had a similar
complaint about the remote-hg.py patches that I haven't sent yet, I
would be more than willing to invest the extra time in addressing
those concerns, since I'm adding new functionality anyway, this is
different.

> As I said, I am not married to the verbose struct/union representation
> (the only reason I showed that verbosity was because it allowed me to do
> away without having to enumerate all the syntax sugars we already
> support); if your "flags" can express the same thing (it may needs to
> become a bitfield with enough width, but I highly suspect that you would
> also need at least a component that says "this is the string the user gave
> us --- the user said 'master', not 'master^0', for example) and is a lot
> more compact, that is definitely we want to go with.

Don't we already store that in the name field?

-- 
Cheers,

Sverre Rabbelier

Re: [PATCH 3/5] setup_revisions: remember whether a ref was positive or not

[Junio C Hamano]

Sverre Rabbelier <srabbelier@gmail.com> writes:

> Nobody needs the stuff you mentioned right now.

Wrong.

"diff ^C A B" is a bug, isn't it? And that is a bug in the current code.

>> I would be reluctant to accept a myopic hack that is only good for one
>> caller and that needs to be ripped out and re-done, especially when we
>> already know other issues that can be solved cleanly if you go a little
>> further in the initial round.
>
> While I understand this reluctance, remember that this "one caller" is
> required to fix a bug in the current code.

Re: [PATCH 3/5] setup_revisions: remember whether a ref was positive or not

[Junio C Hamano]

Sverre Rabbelier <srabbelier@gmail.com> writes:

>> But the point is that this codepath does not have a faintest clue whether
>> the "obj" parameter is something the end user actively asked for (which
>> might have been marked as uninteresting for other reasons, namely, because
>> it is reachable from other negative refs). So passing unconditional 0 is
>> just as bad.
>
> Doesn't passing 0 indicate that we _did not_ receive any explicit user
> input on this ref, which is exactly what we want to record? If the
> user passed us any explicit input on the ref, we record it at the
> other call-sites, here, the user told us nothing, so we record exactly
> that, nothing.

Hmm, which means you have a way to say "explicitly affirmative" vs "no
information", but no way to say "explicitly negative", for example, and
the worse part is that it is unclear if the approach the patch takes is
extensible enough to allow that in the future. That is the kind of "myopic
hack" attitude I did not particularly like in this patch.

"The next person who needs more generic framework can rip out what this
patch does and the work required is the same amount" is not a convincing
argument---it would mean you are burdening that other person with an extra
work to _redo_ what this series does properly, and it is not likely to be
of help for that person after your interest in this codepath has long
waned.

>> As I said, I am not married to the verbose struct/union representation
>> (the only reason I showed that verbosity was because it allowed me to do
>> away without having to enumerate all the syntax sugars we already
>> support); if your "flags" can express the same thing (it may needs to
>> become a bitfield with enough width, but I highly suspect that you would
>> also need at least a component that says "this is the string the user gave
>> us --- the user said 'master', not 'master^0', for example) and is a lot
>> more compact, that is definitely we want to go with.
>
> Don't we already store that in the name field?

Please remind yourself why then it is not sufficient for your patch to
read from the name field please?

After all, wasn't the issue that "master^0..master" yields an empty set
but you somehow wanted to know that the RHS of that dotdot was given as a
positive ref?

Re: [PATCH 3/5] setup_revisions: remember whether a ref was positive or not

[Sverre Rabbelier]

Heya,

On Tue, Aug 9, 2011 at 00:24, Junio C Hamano <gitster@pobox.com> wrote:
> Hmm, which means you have a way to say "explicitly affirmative" vs "no
> information", but no way to say "explicitly negative", for example, and
> the worse part is that it is unclear if the approach the patch takes is
> extensible enough to allow that in the future. That is the kind of "myopic
> hack" attitude I did not particularly like in this patch.

Do we have a way to say explicitly negative on the commandline? Is
there a way to say "I don't want you to decorate this commit as
anything at all"?

> "The next person who needs more generic framework can rip out what this
> patch does and the work required is the same amount" is not a convincing
> argument---it would mean you are burdening that other person with an extra
> work to _redo_ what this series does properly, and it is not likely to be
> of help for that person after your interest in this codepath has long
> waned.

Be fair. How is "2 files changed, 8 insertions(+), 6 deletions(-)"
going to make it any harder at all for someone who is going to be
doing that huge patch series you described earlier?

>> Don't we already store that in the name field?
>
> Please remind yourself why then it is not sufficient for your patch to
> read from the name field please?

Sure, we could do it. But it would be duplicating all the effort
already being done in rev-parse!

> After all, wasn't the issue that "master^0..master" yields an empty set
> but you somehow wanted to know that the RHS of that dotdot was given as a
> positive ref?

The issue was that if I push "master" to origin but I already pushed a
"next" which points at the same commit nothing happens because
git-fast-export doesn't know to emit the "reset master :13878" line.

-- 
Cheers,

Sverre Rabbelier

Re: [PATCH 3/5] setup_revisions: remember whether a ref was positive or not

[Sverre Rabbelier]

Heya,

On Tue, Aug 9, 2011 at 00:07, Junio C Hamano <gitster@pobox.com> wrote:
> Sverre Rabbelier <srabbelier@gmail.com> writes:
>> Nobody needs the stuff you mentioned right now.
>
> Wrong.
>
> "diff ^C A B" is a bug, isn't it? And that is a bug in the current code.

I think I missed something, can you point me to the relevant thread
about this bug in diff? Maybe I can convince the people involved in
fixing that to help out.

-- 
Cheers,

Sverre Rabbelier

Re: [PATCH 3/5] setup_revisions: remember whether a ref was positive or not

[Junio C Hamano]

Sverre Rabbelier <srabbelier@gmail.com> writes:

> On Tue, Aug 9, 2011 at 00:07, Junio C Hamano <gitster@pobox.com> wrote:
>> Sverre Rabbelier <srabbelier@gmail.com> writes:
>>> Nobody needs the stuff you mentioned right now.
>>
>> Wrong.
>>
>> "diff ^C A B" is a bug, isn't it? And that is a bug in the current code.
>
> I think I missed something, can you point me to the relevant thread
> about this bug in diff? Maybe I can convince the people involved in
> fixing that to help out.

It is in this thread; <7vy5zabbz7.fsf@alter.siamese.dyndns.org>

Re: [PATCH 3/5] setup_revisions: remember whether a ref was positive or not

[Sverre Rabbelier]

Heya,

On Tue, Aug 9, 2011 at 00:36, Junio C Hamano <gitster@pobox.com> wrote:
> It is in this thread; <7vy5zabbz7.fsf@alter.siamese.dyndns.org>

Then I don't understand why you're mentioning it in reference to a
problem someone is actually having?

-- 
Cheers,

Sverre Rabbelier

Re: [PATCH 3/5] setup_revisions: remember whether a ref was positive or not

[Junio C Hamano]

Sverre Rabbelier <srabbelier@gmail.com> writes:

> On Tue, Aug 9, 2011 at 00:36, Junio C Hamano <gitster@pobox.com> wrote:
>> It is in this thread; <7vy5zabbz7.fsf@alter.siamese.dyndns.org>
>
> Then I don't understand why you're mentioning it in reference to a
> problem someone is actually having?

What?

Why isn't it a real problem when I have it in the currently released code,
and why is it a real problem if you have it with your wip series that uses
the current code?  Both are real problems that somebody is actually
having.

Re: [PATCH 3/5] setup_revisions: remember whether a ref was positive or not

[Sverre Rabbelier]

Heya,

On Tue, Aug 9, 2011 at 00:46, Junio C Hamano <gitster@pobox.com> wrote:
> Why isn't it a real problem when I have it in the currently released code,
> and why is it a real problem if you have it with your wip series that uses
> the current code?  Both are real problems that somebody is actually
> having.

Because no actual users are actually having this problem (e.g., at
times typing "git diff ^C A B" and are seriously troubled by it), or
at least haven't spoken up? Or do you meant that you are that user?
I'm confused.

-- 
Cheers,

Sverre Rabbelier

Re: [PATCH 3/5] setup_revisions: remember whether a ref was positive or not

[Junio C Hamano]

Sverre Rabbelier <srabbelier@gmail.com> writes:

>> Please remind yourself why then it is not sufficient for your patch to
>> read from the name field please?
>
> Sure, we could do it. But it would be duplicating all the effort
> already being done in rev-parse!
>
>> After all, wasn't the issue that "master^0..master" yields an empty set
>> but you somehow wanted to know that the RHS of that dotdot was given as a
>> positive ref?
>
> The issue was that if I push "master" to origin but I already pushed a
> "next" which points at the same commit nothing happens...

Would we have the same issue with "git bundle", by the way, caused by the
same setup_revisions() limitation?  Over there we would also need to
decide the set of refs that the user wanted to record their values while
computing the set of commits we would need to transfer. 

In this message I am not interested in talking about what implementation
to use to fix it, but just wondering the scope of the same issue and
trying to figure out the (positive) impact of _a_ fix.

Re: [PATCH 3/5] setup_revisions: remember whether a ref was positive or not

[Sverre Rabbelier]

Heya,

On Tue, Aug 9, 2011 at 00:56, Junio C Hamano <gitster@pobox.com> wrote:
> Would we have the same issue with "git bundle", by the way, caused by the
> same setup_revisions() limitation?  Over there we would also need to
> decide the set of refs that the user wanted to record their values while
> computing the set of commits we would need to transfer.

I think so.

$ git init test && cd test
Initialized empty Git repository in /home/sverre/code/test/test/.git/
$ echo content >> file
$ git add file && git commit -m "first"
[master (root-commit) c7f5a27] first
 1 files changed, 1 insertions(+), 0 deletions(-)
 create mode 100644 file
$ (git init ../another && cd ../another && git pull ../test)
Initialized empty Git repository in /home/sverre/code/another/.git/
remote: Counting objects: 3, done.
remote: Total 3 (delta 0), reused 0 (delta 0)
Unpacking objects: 100% (3/3), done.
From ../test
 * branch            HEAD       -> FETCH_HEAD
sverre@laptop-sverre:~/code/test
$ git branch second
$ git bundle create bundle second
Counting objects: 3, done.
Writing objects: 100% (3/3), 215 bytes, done.
Total 3 (delta 0), reused 0 (delta 0)
sverre@laptop-sverre:~/code/test
$ mv bundle ../another/
$ (cd ../another/ && git bundle unbundle bundle)
c7f5a273b11a438bf78aecff0dfcbb8b16344555 refs/heads/second
$ (cd ../another/ && git for-each-ref)
c7f5a273b11a438bf78aecff0dfcbb8b16344555 commit	refs/heads/master

-- 
Cheers,

Sverre Rabbelier

Re: [PATCH 3/5] setup_revisions: remember whether a ref was positive or not

[Junio C Hamano]

Sverre Rabbelier <srabbelier@gmail.com> writes:

> Because no actual users are actually having this problem (e.g., at
> times typing "git diff ^C A B" and are seriously troubled by it), or
> at least haven't spoken up? Or do you meant that you are that user?

Yes, I am that user who have been disturbed by the command line revision
parser limitation for a loooooooong time and have been wanting to see it
properly fixed for quite a while (by myself or by somebody else).

Otherwise why would I have bothered to spend 93 lines to describe it and
its one possible solution that I _think_ is general enough to cover both
of the real issue identified in this thread?

I may probably be being slightly unfair, for two reasons. The primary
reason is because the affected codepath is so central to the UI that
fixing this properly is more important than various random tweaks we make
in other areas (which makes me particularly picky with respect to ensuring
that the future expansion would be easy). Seeing a single-bit "hack" not
from a complete newbie but from two known-to-be-competent long timers of
Git is another.

There are numerous hacks that try to work around the same revision parser
limitation (lossage of information), and I have been hoping that a series
that touch the parser to leave more information during parsing to be
cleanly enhancible to get rid of them (e.g. "git checkout $commit" vs "git
checkout $branch" looks at the command line itself, but it should be able
to call into the revision parser and inspect what the parser gives it
back).

Re: [PATCH 3/5] setup_revisions: remember whether a ref was positive or not

[Sverre Rabbelier]

Heya,

On Tue, Aug 9, 2011 at 01:17, Junio C Hamano <gitster@pobox.com> wrote:
> Yes, I am that user who have been disturbed by the command line revision
> parser limitation for a loooooooong time and have been wanting to see it
> properly fixed for quite a while (by myself or by somebody else).

Aaaah, well that does explain a thing or two :).

> Seeing a single-bit "hack" not
> from a complete newbie but from two known-to-be-competent long timers of
> Git is another.

Perhaps known-to-be-competent, but time-constrained nonetheless.
Sometimes one is forced to chose between fixing something you care
about and doing something in a non-hacky way... There's only so much
time in a day to work on side-projects such as this after all :).

> There are numerous hacks that try to work around the same revision parser
> limitation (lossage of information), and I have been hoping that a series
> that touch the parser to leave more information during parsing to be
> cleanly enhancible to get rid of them (e.g. "git checkout $commit" vs "git
> checkout $branch" looks at the command line itself, but it should be able
> to call into the revision parser and inspect what the parser gives it
> back).

Ok, knowing that, I am somewhat more inclined to have a look at this.
I'm still not sure that I know how to implement the design you
described though. I mean, I understand the general concept, but I have
no idea in what places it would need to hook in to make it work (ditto
on how to fix the diff bug).

-- 
Cheers,

Sverre Rabbelier

Re: [PATCH 3/5] setup_revisions: remember whether a ref was positive or not

[Junio C Hamano]

Sverre Rabbelier <srabbelier@gmail.com> writes:

> Ok, knowing that, I am somewhat more inclined to have a look at this.
> I'm still not sure that I know how to implement the design you
> described though. I mean, I understand the general concept, but I have
> no idea in what places it would need to hook in to make it work (ditto
> on how to fix the diff bug).

It is entirely possible, as Dscho hinted in his response, that your "flag"
word may be sufficient to encode "where did this object (whether it is
marked as uninteresting or interesting) come from? what did the user want
to do with it?" information for all practical purposes, even though I
doubt one bit would be sufficient. So how about ignoring "diff A...B" vs
"diff ^C A B" in the meantime and see if your change to revision.c is
applicable to the same issue in "git bundle" as a starter?

I would imagine that "diff A...B" vs "diff ^C A B" case would be solved by
the former (^C is the common ancestor between A and B) add a flag
COMMON_ANCESTOR to pending object array entry that holds ^C,
SYMMETRIC_LEFT to the one with A and SYMMETRIC_RIGHT to the one with B,
while the latter would stuff one SINGLE_NEGATIVE (^C), and two
SINGLE_POSITIVE (A and B), and the caller would tell between the two, or
something like that.  And "bundle master^0..master" (and similarly your
export stream) would note the RHS of the dot operator with SYMMETRIC_RIGHT
so that "master", even though the commit object gets marked as
uninteresting at the end, can be recovered to be of interest to the end
user.

I am not suggesting the exact flag word names above, but just trying to
enumerate how many kinds would be needed). Your single-bit in the patch
would be SYMMETRIC_RIGHT in the above ugly notation, but you can call it
REF_INTERESTING or whatever.