Possible BUG with git-rev-list --all in a StGit repository

Possible BUG with git-rev-list --all in a StGit repository

[Marco Costalba]

In a StGit repository the --all option causes a lot of spurious
revisions, possibly stgit related.

$ git branch
* master
  origin
  test

$ git rev-list master origin test -- src/settingsimpl.cpp | wc
     13      13     533

$ git rev-list --all -- src/settingsimpl.cpp | wc
     26      26    1066


The extra revisions have shortlogs of the kind of:

 push        a3bc76fd0bdd154149c26a3c208f0344e9cd873b
 new e7baf56544cd8b4f8601a35fad274b8de97fd558
refresh     8fa01a56a40b04ed9c6d006c669ca9d370176728

From qgit these are easily seen from file history tab of a file
modified by stgit patches or when filtering in main view on the same
file.

Shouldn't 'git-rev-list --all'  print  *the same output* of when the
list  with all branches is given in command line?

Thanks
Marco

P.S:
$ git version
git version 1.4.4.1.g7002
$ stg --version
Stacked GIT 0.11
git version 1.4.4.1.g7002
Python version 2.4.3 (#2, Oct  6 2006, 15:32:41)

Re: Possible BUG with git-rev-list --all in a StGit repository

[Junio C Hamano]

"Marco Costalba" <mcostalba@gmail.com> writes:

> In a StGit repository the --all option causes a lot of spurious
> revisions, possibly stgit related.
>
> $ git branch
> * master
>  origin
>  test
>
> $ git rev-list master origin test -- src/settingsimpl.cpp | wc
>     13      13     533
>
> $ git rev-list --all -- src/settingsimpl.cpp | wc
>     26      26    1066
>
>
> The extra revisions have shortlogs of the kind of:
>
> push        a3bc76fd0bdd154149c26a3c208f0344e9cd873b
> new e7baf56544cd8b4f8601a35fad274b8de97fd558
> refresh     8fa01a56a40b04ed9c6d006c669ca9d370176728
>
>>From qgit these are easily seen from file history tab of a file
> modified by stgit patches or when filtering in main view on the same
> file.
>
> Shouldn't 'git-rev-list --all'  print  *the same output* of when the
> list  with all branches is given in command line?

Should it?  The "--all" option is about "all refs", not "all
user branches" and it has been so from the beginning.  For one
thing it has to do the reachability thing also for tags
(otherwise it cannot be used as the upstream for git-repack
pipeline).

You are looking at .git/refs/bases/ refs that StGIT uses for its
internal bookkeeping.

Re: Possible BUG with git-rev-list --all in a StGit repository

[Marco Costalba]

>
> You are looking at .git/refs/bases/ refs that StGIT uses for its
> internal bookkeeping.
>
Ok.

Anyway, getting garbage when asking for a git-rev-list --all if in a
StGit repo at least could be considered a little integration issue.

Internal bookkeeing should be, well,  _internal_  :-)

Re: Possible BUG with git-rev-list --all in a StGit repository

[Marco Costalba]

On 11/27/06, Marco Costalba <mcostalba@gmail.com> wrote:
> >
> > You are looking at .git/refs/bases/ refs that StGIT uses for its
> > internal bookkeeping.
> >
> Ok.
>
> Anyway, getting garbage when asking for a git-rev-list --all if in a
> StGit repo at least could be considered a little integration issue.
>
> Internal bookkeeing should be, well,  _internal_  :-)
>

Re: Possible BUG with git-rev-list --all in a StGit repository

[Junio C Hamano]

"Marco Costalba" <mcostalba@gmail.com> writes:

> Could a possible '--all-branches' new option come to rescue?

I doubt it.  Next thing people would start talking about is what
to do with the remote tracking branches, and what we are talking
about is rev-list, one of the lower level of plumbing that would
be better left without knowing much about the Porcelain's use of
refs/ namespaces.

If you (as a Porcelain) want to get all refs under refs/heads/,
there are (unfortunately) two ways to get that list.  I would
suggest obtain the refs you want that way, pass them as command
line arguments to rev-list.

$ git for-each-ref --format='%(refname)' refs/heads
$ git show-ref --heads | sed -e 's/^[^ ]* //'

[PATCH] revision traversal: Add --refs=<pattern> option

[Sergey Vlasov]

Add the --refs=<pattern> option, which can be used to select a
subset of refs matching the specified glob pattern.

Signed-off-by: Sergey Vlasov <vsu@altlinux.ru>
---

 If --all-branches is too specific for the mentioned use case,
 what about adding a more general glob pattern match?

 Documentation/git-rev-list.txt |    9 +++++++++
 revision.c                     |   21 ++++++++++++++++++---
 2 files changed, 27 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-rev-list.txt b/Documentation/git-rev-list.txt
index ec43c0b..d5f99ef 100644
--- a/Documentation/git-rev-list.txt
+++ b/Documentation/git-rev-list.txt
@@ -17,6 +17,7 @@ SYNOPSIS
 	     [ \--remove-empty ]
 	     [ \--not ]
 	     [ \--all ]
+	     [ \--refs=<pattern> ]
 	     [ \--stdin ]
 	     [ \--topo-order ]
 	     [ \--parents ]
@@ -179,6 +180,14 @@ limiting may be applied.
 	Pretend as if all the refs in `$GIT_DIR/refs/` are listed on the
 	command line as '<commit>'.
 
+--refs='pattern'::
+
+	Pretend as if all the refs in `$GIT_DIR/refs/` matching the
+	specified glob pattern are listed on the command line as
+	'<commit>'.  The initial `refs/` part is skipped when matching,
+	but the subsequent `heads/`, `tags/` or `remotes/` part is
+	included in the text to match.
+
 --stdin::
 
 	In addition to the '<commit>' listed on the command
diff --git a/revision.c b/revision.c
index 993bb66..240ff59 100644
--- a/revision.c
+++ b/revision.c
@@ -7,6 +7,7 @@
 #include "refs.h"
 #include "revision.h"
 #include <regex.h>
+#include <fnmatch.h>
 #include "grep.h"
 
 static char *path_name(struct name_path *path, const char *name)
@@ -464,18 +465,28 @@ static void limit_list(struct rev_info *
 
 static int all_flags;
 static struct rev_info *all_revs;
+static const char *all_pattern;
 
 static int handle_one_ref(const char *path, const unsigned char *sha1, int flag, void *cb_data)
 {
-	struct object *object = get_reference(all_revs, path, sha1, all_flags);
+	struct object *object;
+
+	if (all_pattern) {
+		if (strncmp(path, "refs/", 5))
+			return 0;
+		if (fnmatch(all_pattern, path + 5, 0))
+			return 0;
+	}
+	object = get_reference(all_revs, path, sha1, all_flags);
 	add_pending_object(all_revs, object, "");
 	return 0;
 }
 
-static void handle_all(struct rev_info *revs, unsigned flags)
+static void handle_all(struct rev_info *revs, unsigned flags, const char *pattern)
 {
 	all_revs = revs;
 	all_flags = flags;
+	all_pattern = pattern;
 	for_each_ref(handle_one_ref, NULL);
 }
 
@@ -800,7 +811,11 @@ int setup_revisions(int argc, const char
 				continue;
 			}
 			if (!strcmp(arg, "--all")) {
-				handle_all(revs, flags);
+				handle_all(revs, flags, NULL);
+				continue;
+			}
+			if (!strncmp(arg, "--refs=", 7)) {
+				handle_all(revs, flags, arg + 7);
 				continue;
 			}
 			if (!strcmp(arg, "--not")) {
-- 
1.4.4.1.gb0b0

Re: [PATCH] revision traversal: Add --refs=<pattern> option

[Junio C Hamano]

Sergey Vlasov <vsu@altlinux.ru> writes:

> Add the --refs=<pattern> option, which can be used to select a
> subset of refs matching the specified glob pattern.
>
> Signed-off-by: Sergey Vlasov <vsu@altlinux.ru>
> ---
>
>  If --all-branches is too specific for the mentioned use case,
>  what about adding a more general glob pattern match?

Traditionally any new option to rev-list must be accompanied
with a matching change to rev-parse.  I do not know offhand how
strictly we should adhere to this rule these days; it depends on
how people's script use rev-list.

Before revision.c "revision walking library" was done, many
Porcelain-ish commands were implemented as a pipeline that plugs
rev-list output to diff-tree.  These shell scripts took
parameters from the command line, and rev-parse was used to
separate parameters (both "flags" that begin with a dash and
"non-flags" that don't) that should be given to rev-list and the
other parameters (meant to be used by the shell script itself
but often are given straight to the downstream diff-tree).  The
rev-parse command has even the --sq option to facilitate this
usage:

	rev_opts=`git rev-parse --sq --default=HEAD --revs "$@"`
	diff_opts=`git rev-parse --sq --no-revs "$@"`
        eval "git-rev-list $rev_opts" |
        eval "git-diff-tree --stdin $diff_opts"

so that it can even pass -S'I want to find this string' to diff-tree
without worrying about spaces.

I personally feel that part of rev-parse outlived its usefulness
(--flags, --no-flags, --revs-only, and --no-revs).  It was a
useful hack, and served us well, but it was a hack.

In that sense it probably is Ok to leave it unmaintained, but it
might be a good idea to plan deprecating it, given that we have
been talking about UI warts.  If there are pipelines that can be
easily formed (with the help of rev-parse "parameter sifter"),
but whose functionality cannot be easily emulated with the
current crop of Porcelain-ish, we should work on polishing the
Porcelain-ish to make the pipelines unnecessary.

The remaining parts of rev-parse (the most important of which is
the --verify option) should probably stay.  The original
question of "list all the branches" can be done with:

	git rev-parse --symbolic --branches

Re: Possible BUG with git-rev-list --all in a StGit repository

[Marco Costalba]

On 11/27/06, Junio C Hamano <junkio@cox.net> wrote:
> "Marco Costalba" <mcostalba@gmail.com> writes:
>
> > Could a possible '--all-branches' new option come to rescue?
>
> I doubt it.  Next thing people would start talking about is what
> to do with the remote tracking branches, and what we are talking
> about is rev-list, one of the lower level of plumbing that would
> be better left without knowing much about the Porcelain's use of
> refs/ namespaces.
>
> If you (as a Porcelain) want to get all refs under refs/heads/,
> there are (unfortunately) two ways to get that list.  I would
> suggest obtain the refs you want that way, pass them as command
> line arguments to rev-list.
>

Unfortunatly that does not work in case a branch and a tag have the same name.

I was bitten by this trying to do what you now suggest, there were a
tag and a branch called 'test', and calling

git-rev-list master origin test

raised a warning.

Only among the _same_ 'family' (branches, tags, etc..) unique names
are enforced.

Re: Possible BUG with git-rev-list --all in a StGit repository

[Junio C Hamano]

"Marco Costalba" <mcostalba@gmail.com> writes:

>> If you (as a Porcelain) want to get all refs under refs/heads/,
>> there are (unfortunately) two ways to get that list.  I would
>> suggest obtain the refs you want that way, pass them as command
>> line arguments to rev-list.
>
> Unfortunatly that does not work in case a branch and a tag have the same name.

I am not quite grokking what the problem you are trying to solve
is, so this is a shot in the dark, but

	git rev-list refs/heads/test refs/tags/test

to disambiguate, perhaps?

Re: Possible BUG with git-rev-list --all in a StGit repository

[Marco Costalba]

On 11/28/06, Junio C Hamano <junkio@cox.net> wrote:
> "Marco Costalba" <mcostalba@gmail.com> writes:
>
> >> If you (as a Porcelain) want to get all refs under refs/heads/,
> >> there are (unfortunately) two ways to get that list.  I would
> >> suggest obtain the refs you want that way, pass them as command
> >> line arguments to rev-list.
> >
> > Unfortunatly that does not work in case a branch and a tag have the same name.
>
> I am not quite grokking what the problem you are trying to solve
> is, so this is a shot in the dark, but
>
>         git rev-list refs/heads/test refs/tags/test
>
> to disambiguate, perhaps?
>

Interesting!

I didn't think about 'full path' explicitation. If the barnches are
always and only under refs/heads this should work.

Thanks