[PATCH/v3] bundle.c: added --stdin option to git-bundle

[PATCH/v3] bundle.c: added --stdin option to git-bundle

[Adam Brewster]

Signed-off-by: Adam Brewster <asb@bu.edu>
---
It seems that the consensus is that the other half of my original
patch is no good.  You have some pretty good ideas about how to
correctly address the problem I was trying to solve, and I look
forward to seeing them actually implemented.

For now, I offer separately the modification I made to bundle.c to
allow git-bundle to handle the --stdin option.  There is no
accompanying change to the documentation because it already implies
that this option is available.

 bundle.c |   22 ++++++++++++++++++++--
 1 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/bundle.c b/bundle.c
index 0ba5df1..b44a4af 100644
--- a/bundle.c
+++ b/bundle.c
@@ -227,8 +227,26 @@ int create_bundle(struct bundle_header *header,
const char *path,

        /* write references */
        argc = setup_revisions(argc, argv, &revs, NULL);
-       if (argc > 1)
-               return error("unrecognized argument: %s'", argv[1]);
+
+       for (i = 1; i < argc; i++) {
+               if ( !strcmp(argv[i], "--stdin") ) {
+                       char line[1000];
+                               while (fgets(line, sizeof(line),
stdin) != NULL) {
+                               int len = strlen(line);
+                               if (len && line[len - 1] == '\n')
+                                       line[--len] = '\0';
+                               if (!len)
+                                       break;
+                               if (line[0] == '-')
+                                       die("options not supported in
--stdin mode");
+                               if (handle_revision_arg(line, &revs, 0, 1))
+                                       die("bad revision '%s'", line);
+                       }
+                       continue;
+               }
+
+               return error("unrecognized argument: %s'", argv[i]);
+       }

        for (i = 0; i < revs.pending.nr; i++) {
                struct object_array_entry *e = revs.pending.objects + i;
--
1.5.5.1.211.g65ea3.dirty

Re: [PATCH/v3] bundle.c: added --stdin option to git-bundle

[Jakub Narebski]

On Sat, 5 Jul 2008, Adam Brewster wrote:

> It seems that the consensus is that the other half of my original
> patch is no good.  You have some pretty good ideas about how to
> correctly address the problem I was trying to solve, and I look
> forward to seeing them actually implemented.

It is not that the other half is "no good", it is rather that there
is no consensus how and in what way it should be implemented; be
it separate git-bases command, git-bundle--bases helper script, or
incorporated in git-bundle code; should it be written in Perl or
as shell script (only in case of git-bases or git-bundle--bases),
or should it be written in C.

Adding some documentation, with example usage (example "workflows")
would help adding git-bases to git core... perhaps at start send
it as script in contrib/ ?

> For now, I offer separately the modification I made to bundle.c to
> allow git-bundle to handle the --stdin option.  

That's the way it is preferred here on git mailing list, to not bundle 
non-controversial change with the one that needs (or seem to need) some 
further discussion - do not hold features hostage ;-)

> There is no accompanying change to the documentation because it
> already implies that this option is available.

Adding an example of using `--stdin' to "[git-rev-list-args...]::"
in Documentation/git-bundle.txt would be good.

-- 
Jakub Narebski
Poland

Re: [PATCH/v3] bundle.c: added --stdin option to git-bundle

[Junio C Hamano]

"Adam Brewster" <adambrewster@gmail.com> writes:

> Subject: Re: [PATCH/v3] bundle.c: added --stdin option to git-bundle

When the change is not about implementation detail (in which case you do
want to name the source file and perhaps even a function name), but about
a new feature that is visible to the end-users of a command, we'd want the
message talk in terms of what the new feature does, not how the new
feature is invoked nor where it is implemented.  In other words, something
like these are preferred:

	git-bundle: add --stdin
        Teach git-bundle to read tips and basis from standard input

and don't say "You did" in past tense --- say things in imperative mood
instead, as if you are commanding the person who applies the patch to make
it happen.  Older log entries in our history (e.g. "git log -n 20 v0.99")
may give you a better feel.

And give a few lines of obvious justfication in the body of the commit log
message, e.g.

	This patch allows the caller to feed the revision parameters to
	git-bundle from its standard input.  This way, a script do not
	have to worry about limitation of the length of command line.

to explain why this is good.  In order to explain that you may have to
talk about other things (like what it does and how it does it), but keep
in mind that the primary thing you should talk about is _why_.

> ... because it already implies that this option is available.

If that is the case, please mention in the commit log message something
like "Even though the documentation said "bundle --stdin" is accepted it
didn't.  This patch teaches the option to the command".

But I do not think there is no such implication.  "bundle create" may take
list of positive and negative refs as arguments or --branches, but it does
not take (and it shouldn't -- I do not think it should take --bisect
option, for example) artibrary options that rev-list command accepts.

>  bundle.c |   22 ++++++++++++++++++++--
>  1 files changed, 20 insertions(+), 2 deletions(-)
>
> diff --git a/bundle.c b/bundle.c
> index 0ba5df1..b44a4af 100644
> --- a/bundle.c
> +++ b/bundle.c
> @@ -227,8 +227,26 @@ int create_bundle(struct bundle_header *header,
> const char *path,

Wrapped.

>         /* write references */
>         argc = setup_revisions(argc, argv, &revs, NULL);
> -       if (argc > 1)
> -               return error("unrecognized argument: %s'", argv[1]);
> +
> +       for (i = 1; i < argc; i++) {
> +               if ( !strcmp(argv[i], "--stdin") ) {

Style.

> +                       char line[1000];
> +                               while (fgets(line, sizeof(line),
> stdin) != NULL) {

Too deep indentation.  Wrapped.

> +                               int len = strlen(line);
> +                               if (len && line[len - 1] == '\n')
> +                                       line[--len] = '\0';
> +                               if (!len)
> +                                       break;
> +                               if (line[0] == '-')
> +                                       die("options not supported in
> --stdin mode");
> +                               if (handle_revision_arg(line, &revs, 0, 1))
> +                                       die("bad revision '%s'", line);
> +                       }
> +                       continue;
> +               }
> +
> +               return error("unrecognized argument: %s'", argv[i]);
> +       }

Having said that, I think copying and pasting read_revisions_from_stdin()
in bundle.c is a wrong approach to take.  Probably the function can easily
be split out of builtin-rev-list.c and moved to revision.c or somewhere
(which will be the first patch), and then a separate patch can add a few
lines to call it from bundle.c.

[PATCH v4 0/3]

[Adam Brewster]

Sorry for the idiotic wrapping problems in my last email.

Previously, I was trying to keep from changing any of the important stuff,
like git-rev-list, but I should know better than cut-and-pasting code.

As requested, I've broken the change into a multiple of patches.  First moving
read_revisions_from_stdin to revision.c, next modifying git-bundle to handle
--stdin, and finally a patch adding my old git-basis to contrib.

I think I've corrected all of the style issues you pointed out, and I've also 
tried to craft more informative commit messages.

[PATCH] Move read_revisions_from_stdin from builtin-rev-list.c to revision.c

[Adam Brewster]

Some other commands might like to support the --stdin option like
git-rev-list.  Since they don't want to depend on builtin-rev-list, the
function has to be somewhere else.

Signed-off-by: Adam Brewster <asb@bu.edu>
---
 builtin-rev-list.c |   17 -----------------
 revision.c         |   17 +++++++++++++++++
 2 files changed, 17 insertions(+), 17 deletions(-)
 mode change 100644 => 100755 builtin-rev-list.c
 mode change 100644 => 100755 revision.c

diff --git a/builtin-rev-list.c b/builtin-rev-list.c
old mode 100644
new mode 100755
index 11a7eae..b4a2c44
--- a/builtin-rev-list.c
+++ b/builtin-rev-list.c
@@ -575,23 +575,6 @@ static struct commit_list *find_bisection(struct commit_list *list,
 	return best;
 }
 
-static void read_revisions_from_stdin(struct rev_info *revs)
-{
-	char line[1000];
-
-	while (fgets(line, sizeof(line), stdin) != NULL) {
-		int len = strlen(line);
-		if (len && line[len - 1] == '\n')
-			line[--len] = 0;
-		if (!len)
-			break;
-		if (line[0] == '-')
-			die("options not supported in --stdin mode");
-		if (handle_revision_arg(line, revs, 0, 1))
-			die("bad revision '%s'", line);
-	}
-}
-
 int cmd_rev_list(int argc, const char **argv, const char *prefix)
 {
 	struct commit_list *list;
diff --git a/revision.c b/revision.c
old mode 100644
new mode 100755
index 5a1a948..0191160
--- a/revision.c
+++ b/revision.c
@@ -911,6 +911,23 @@ int handle_revision_arg(const char *arg, struct rev_info *revs,
 	return 0;
 }
 
+void read_revisions_from_stdin(struct rev_info *revs)
+{
+	char line[1000];
+
+	while (fgets(line, sizeof(line), stdin) != NULL) {
+		int len = strlen(line);
+		if (len && line[len - 1] == '\n')
+			line[--len] = '\0';
+		if (!len)
+			break;
+		if (line[0] == '-')
+			die("options not supported in --stdin mode");
+		if (handle_revision_arg(line, revs, 0, 1))
+			die("bad revision '%s'", line);
+	}
+}
+
 static void add_grep(struct rev_info *revs, const char *ptn, enum grep_pat_token what)
 {
 	if (!revs->grep_filter) {
-- 
1.5.5.1.211.g65ea3.dirty

[PATCH] git-bundle: add --stdin

[Adam Brewster]

Teach git-bundle to read revision arguments from stdin like git-rev-list.

This patch allows the caller to feed the revision parameters to git-bundle
from its standard input.  This way, a script do not have to worry about
limitation of the length of command line.

Documentation/git-bundle.txt says that git-bundle takes arguments acceptable
to git-rev-list.  Obviously some arguments that git-rev-list handles don't
make sense for git-bundle (e.g. --bisect) but --stdin is pretty reasonable.

Signed-off-by: Adam Brewster <asb@bu.edu>
---
 bundle.c |   13 +++++++++++--
 1 files changed, 11 insertions(+), 2 deletions(-)
 mode change 100644 => 100755 bundle.c

diff --git a/bundle.c b/bundle.c
old mode 100644
new mode 100755
index 0ba5df1..00b2aab
--- a/bundle.c
+++ b/bundle.c
@@ -178,6 +178,7 @@ int create_bundle(struct bundle_header *header, const char *path,
 	int i, ref_count = 0;
 	char buffer[1024];
 	struct rev_info revs;
+	int read_from_stdin = 0;
 	struct child_process rls;
 	FILE *rls_fout;
 
@@ -227,8 +228,16 @@ int create_bundle(struct bundle_header *header, const char *path,
 
 	/* write references */
 	argc = setup_revisions(argc, argv, &revs, NULL);
-	if (argc > 1)
-		return error("unrecognized argument: %s'", argv[1]);
+
+	for (i = 1; i < argc; i++) {
+		if (!strcmp(argv[i], "--stdin")) {
+			if (read_from_stdin++)
+				die("--stdin given twice?");
+			read_revisions_from_stdin(&revs);
+			continue;
+		}
+		return error("unrecognized argument: %s'", argv[i]);
+	}
 
 	for (i = 0; i < revs.pending.nr; i++) {
 		struct object_array_entry *e = revs.pending.objects + i;
-- 
1.5.5.1.211.g65ea3.dirty

[PATCH] Add git-basis.perl to contrib directory

[Adam Brewster]

Git-basis is a perl script that remembers bases for use by git-bundle.

This script shouldn't be needed because git-push and git-remote should do this
kind of work.  Unfortunately they currently don't so some might find this
script useful.

Signed-off-by: Adam Brewster <asb@bu.edu>
---
 contrib/basis/git-basis.perl |   77 ++++++++++++++++++++++++++++++++++++
 contrib/basis/git-basis.txt  |   90 ++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 167 insertions(+), 0 deletions(-)
 create mode 100755 contrib/basis/git-basis.perl
 create mode 100644 contrib/basis/git-basis.txt

diff --git a/contrib/basis/git-basis.perl b/contrib/basis/git-basis.perl
new file mode 100755
index 0000000..b3e753f
--- /dev/null
+++ b/contrib/basis/git-basis.perl
@@ -0,0 +1,77 @@
+#!/usr/bin/perl
+
+use strict;
+
+use Git;
+
+require Time::Local;
+my $git_epoch = Time::Local::timegm(0, 0, 0, 1, 0, 70);
+
+my $r = Git->repository();
+my $d = $r->repo_path();
+
+if ( ! -d "$d/bases" ) {
+    mkdir("$d/bases") || die "Could not create $d/bases: $!";
+}
+
+if ( $#ARGV == -1 || ($#ARGV == 0 && $ARGV[0] eq '--update') ) {
+    print STDERR "usage: git-basis [--update] basis1...\n";
+    exit;
+} elsif ( $ARGV[0] eq '--update' ) {
+    shift @ARGV;
+
+    my %new = ();
+    while (<STDIN>) {
+	if (!/^^?([a-z0-9]{40})/) {next;}
+	$new{$1} = 1;
+    }
+
+    foreach my $f (@ARGV) {
+	my %these = ();
+	my $fh;
+
+	open $fh, "+<$d/bases/$f" || die "Can't open bases/$f: $!";
+	while (<$fh>) {
+	    if (!/^([a-z0-9]{40})/) {next;}
+	    $these{$1} = 1;
+	}
+
+	print $fh "# ", gmtime() - $git_epoch,
+		" +0000 // ", scalar(localtime()), "\n";
+
+	foreach my $b (keys %new) {
+	    if (exists($these{$b})) {next;}
+	    print $fh "$b\n";
+	}
+	close $fh;
+    }
+} else {
+    my $n = 0;
+    my %basis = ();
+
+    my $f = shift @ARGV;
+    open F, "<$d/bases/$f" || die "Can't open bases/$f: $!";
+    while (<F>) {
+	if (!/^([a-z0-9]{40})/) {next;}
+	$basis{$1} = $n;
+    }
+    close F;
+
+    foreach $f (@ARGV) {
+	open F, "<$d/bases/$f" || die "Can't open bases/$f: $!";
+	while (<F>) {
+	    if (!/^([a-z0-9]{40})/) {next;}
+	    if (!exists($basis{$1})) {next;}
+
+	    if ($basis{$1} == $n) {$basis{$1}++;}
+	    else {delete $basis{$1};}
+	}
+	close F;
+	$n++;
+    }
+
+    foreach my $b (keys %basis) {
+	if ( $basis{$b} != $n ) {next;}
+	print "^$b\n";
+    }
+}
diff --git a/contrib/basis/git-basis.txt b/contrib/basis/git-basis.txt
new file mode 100644
index 0000000..97cfc20
--- /dev/null
+++ b/contrib/basis/git-basis.txt
@@ -0,0 +1,90 @@
+git-basis(1)
+============
+
+NAME
+----
+git-basis - Track sets of references available on remote systems (bases)
+
+SYNOPSIS
+--------
+[verse]
+'git-basis' <basis> [<basis>...]
+'git-basis' --update <basis> [<basis>...] < <object list or bundle>
+
+DESCRIPTION
+-----------
+Maintains lists of objects that are known to be accessible on remote
+computer systems that are not accessible by network.
+
+OPTIONS
+-------
+
+basis::
+	List of bases to operate on.  Any valid filename can be
+	the name of a basis.  Bases that do not exist are taken
+	to be empty.
+
+--update::
+	Tells git-basis to read a list of objects from stdin and
+	add them to each of the given bases.  git-basis produces
+	no output when this option is given.  Bases will be created
+	if necessary.
+
+object list or bundle::
+	Git-basis --update reads object names, one per line from stdin.
+	Leading caret ("^") characters are ignored, as is anything
+	after the object name.  Lines that don't begin with an object
+	name are ignored.  The output of linkgit:git-ls-remote[1] or a
+	bundle created by linkgit:git-bundle[1] are both suitable input.
+
+DISCUSSION
+----------
+git-basis is probably only useful with linkgit:git-bundle[1].
+
+To create a bundle that excludes all objects that are part of my-basis,
+use
+
+git-basis my-basis | git-bundle create my-bundle --all --stdin
+
+To add the objects in my-bundle to my-basis, use
+
+git-basis --update my-basis < my-bundle
+
+DETAILS
+-------
+Bases are stored as plain text files under .git/bases/.  One object
+entry per line.
+
+git-basis without --update reads all of the basis names given on the
+command line, and outputs the intersection of them to stdout, with each
+object prefixed by "^".
+
+git-basis --update reads object names from stdin, and adds all of the
+references to each of the bases listed.  Duplicate references will not
+be listed twice, but otherwise redundant information will be included.
+Each update is prefixed by a line with the current date.
+
+BUGS
+----
+Git-baisis has no undo function.  Once an object is added to a basis,
+it will stay there forever.  If you need to remove objects from a basis,
+use a text editor to alter the file .git/bases/<basis name>.
+
+Git-basis --update does not remove redundant information from bases.
+(Having an object implies that it's parents are also available.)  This
+is done intentionally to make sure git-basis --update is
+non-destructive.
+
+Bug reports are welcome, and patches are encouraged.
+
+SEE ALSO
+--------
+linkgit:git-bundle[1]
+
+AUTHOR
+------
+Written by Adam Brewster <asb@bu.edu>
+
+GIT
+---
+Part of the linkgit:git[1] suite
-- 
1.5.5.1.211.g65ea3.dirty

Re: [PATCH] Move read_revisions_from_stdin from builtin-rev-list.c to revision.c

[Miklos Vajna]

[-- Attachment #1: Type: text/plain, Size: 554 bytes --]

On Sat, Jul 05, 2008 at 04:40:32PM -0400, Adam Brewster <adambrewster@gmail.com> wrote:
> Some other commands might like to support the --stdin option like
> git-rev-list.  Since they don't want to depend on builtin-rev-list, the
> function has to be somewhere else.

I think it's fine to move such a function, but this is a false commit
message, you can use read_revisions_from_stdin() from builtin-bundle if
it lives in builtin-rev-list.c as well.

>  mode change 100644 => 100755 builtin-rev-list.c
>  mode change 100644 => 100755 revision.c

Hm? ;-)

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

[PATCH v5]

[Adam Brewster]

Apparently I'm dumber than I thought.  Here's what they look like without 
random and unnecessary mode changes.  The patch to add git-basis under contrib 
is not affected.

The real reason read_revisions_from_stdin moved to revision.c is because I was 
asked to do it that way.  If my commit message doesn't accurately describe the 
reason for the change, go ahead and edit the message, or let me know what the 
real reason is so I can provide a better message.

Adam

[PATCH] Move read_revisions_from_stdin from builtin-rev-list.c to revision.c

[Adam Brewster]

Some other commands might like to support the --stdin option like
git-rev-list.  Since they don't want to depend on builtin-rev-list, the
function has to be somewhere else.

Signed-off-by: Adam Brewster <asb@bu.edu>
---
 builtin-rev-list.c |   17 -----------------
 revision.c         |   17 +++++++++++++++++
 2 files changed, 17 insertions(+), 17 deletions(-)

diff --git a/builtin-rev-list.c b/builtin-rev-list.c
index 11a7eae..b4a2c44 100644
--- a/builtin-rev-list.c
+++ b/builtin-rev-list.c
@@ -575,23 +575,6 @@ static struct commit_list *find_bisection(struct commit_list *list,
 	return best;
 }
 
-static void read_revisions_from_stdin(struct rev_info *revs)
-{
-	char line[1000];
-
-	while (fgets(line, sizeof(line), stdin) != NULL) {
-		int len = strlen(line);
-		if (len && line[len - 1] == '\n')
-			line[--len] = 0;
-		if (!len)
-			break;
-		if (line[0] == '-')
-			die("options not supported in --stdin mode");
-		if (handle_revision_arg(line, revs, 0, 1))
-			die("bad revision '%s'", line);
-	}
-}
-
 int cmd_rev_list(int argc, const char **argv, const char *prefix)
 {
 	struct commit_list *list;
diff --git a/revision.c b/revision.c
index 5a1a948..0191160 100644
--- a/revision.c
+++ b/revision.c
@@ -911,6 +911,23 @@ int handle_revision_arg(const char *arg, struct rev_info *revs,
 	return 0;
 }
 
+void read_revisions_from_stdin(struct rev_info *revs)
+{
+	char line[1000];
+
+	while (fgets(line, sizeof(line), stdin) != NULL) {
+		int len = strlen(line);
+		if (len && line[len - 1] == '\n')
+			line[--len] = '\0';
+		if (!len)
+			break;
+		if (line[0] == '-')
+			die("options not supported in --stdin mode");
+		if (handle_revision_arg(line, revs, 0, 1))
+			die("bad revision '%s'", line);
+	}
+}
+
 static void add_grep(struct rev_info *revs, const char *ptn, enum grep_pat_token what)
 {
 	if (!revs->grep_filter) {
-- 
1.5.5.1.211.g65ea3.dirty

[PATCH] git-bundle: add --stdin

[Adam Brewster]

Teach git-bundle to read revision arguments from stdin like git-rev-list.

This patch allows the caller to feed the revision parameters to git-bundle
from its standard input.  This way, a script do not have to worry about
limitation of the length of command line.

Documentation/git-bundle.txt says that git-bundle takes arguments acceptable
to git-rev-list.  Obviously some arguments that git-rev-list handles don't
make sense for git-bundle (e.g. --bisect) but --stdin is pretty reasonable.

Signed-off-by: Adam Brewster <adambrewster@gmail.com>
---
 bundle.c |   13 +++++++++++--
 1 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/bundle.c b/bundle.c
index 0ba5df1..00b2aab 100644
--- a/bundle.c
+++ b/bundle.c
@@ -178,6 +178,7 @@ int create_bundle(struct bundle_header *header, const char *path,
 	int i, ref_count = 0;
 	char buffer[1024];
 	struct rev_info revs;
+	int read_from_stdin = 0;
 	struct child_process rls;
 	FILE *rls_fout;
 
@@ -227,8 +228,16 @@ int create_bundle(struct bundle_header *header, const char *path,
 
 	/* write references */
 	argc = setup_revisions(argc, argv, &revs, NULL);
-	if (argc > 1)
-		return error("unrecognized argument: %s'", argv[1]);
+
+	for (i = 1; i < argc; i++) {
+		if (!strcmp(argv[i], "--stdin")) {
+			if (read_from_stdin++)
+				die("--stdin given twice?");
+			read_revisions_from_stdin(&revs);
+			continue;
+		}
+		return error("unrecognized argument: %s'", argv[i]);
+	}
 
 	for (i = 0; i < revs.pending.nr; i++) {
 		struct object_array_entry *e = revs.pending.objects + i;
-- 
1.5.5.1.211.g65ea3.dirty

Re: Teach git-bundle to read revision arguments from stdin like git-rev-list

[Junio C Hamano]

Adam Brewster <adambrewster@gmail.com> writes:

> @@ -227,8 +228,16 @@ int create_bundle(struct bundle_header *header, const char *path,
>  
>  	/* write references */
>  	argc = setup_revisions(argc, argv, &revs, NULL);
> -	if (argc > 1)
> -		return error("unrecognized argument: %s'", argv[1]);
> +
> +	for (i = 1; i < argc; i++) {
> +		if (!strcmp(argv[i], "--stdin")) {
> +			if (read_from_stdin++)
> +				die("--stdin given twice?");

Hmm, do we deeply care about this case?  What bad things coulc happen if
you call read_revisions_from_stdin() twice?

> +			read_revisions_from_stdin(&revs);
> +			continue;
> +		}
> +		return error("unrecognized argument: %s'", argv[i]);
> +	}
>  
>  	for (i = 0; i < revs.pending.nr; i++) {
>  		struct object_array_entry *e = revs.pending.objects + i;

Re: Teach git-bundle to read revision arguments from stdin like git-rev-list

[Adam Brewster]

On Sat, Jul 5, 2008 at 8:57 PM, Junio C Hamano <gitster@pobox.com> 
>> +                     if (read_from_stdin++)
>> +                             die("--stdin given twice?");
>
> Hmm, do we deeply care about this case?  What bad things coulc happen 
if
> you call read_revisions_from_stdin() twice?
>

Presently, it'll actually try to read stdin twice and that won't work.

Also, if you want git-bundle to deal with --stdin --stdin, I'd say that 
git-rev-list should do the same.  I don't really care how this 
particular error case is handled, but I think git-rev-list and 
git-bundle should do the same thing for any given input.

If you prefer to be liberal in what you accept, then you might like 
these two patches that allow git-rev-list and git-bundle to deal with 
--stdin --stdin.

By the way, I'm not exactly sure on the format of these guys.  You said 
you queued some changes yesterday, so these go on top of those.  If 
you want me to start from scratch and give you the whole chain again, I 
can do that too.

[PATCH] git-rev-list: tolerate multiple --stdin options

[Adam Brewster]

There's no reason to fail if the user asks for --stdin twice.  Of course
there's only one stdin, and it can only be read once, and there's no
reason to ask for it twice, but --all --all doesn't make sense, and
that's accepted, so accept this too.

Also, with read_revisions_from_stdin in revision.c where it might be
called by other programs, it's better to check that stdin isn't at eof
before trying to read it.

Signed-off-by: Adam Brewster <asb@bu.edu>
---
 builtin-rev-list.c |    3 ---
 revision.c         |    2 +-
 2 files changed, 1 insertions(+), 4 deletions(-)

diff --git a/builtin-rev-list.c b/builtin-rev-list.c
index b4a2c44..7f7c1a7 100644
--- a/builtin-rev-list.c
+++ b/builtin-rev-list.c
@@ -579,7 +579,6 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)
 {
 	struct commit_list *list;
 	int i;
-	int read_from_stdin = 0;
 	int bisect_show_vars = 0;
 	int bisect_find_all = 0;
 	int quiet = 0;
@@ -616,8 +615,6 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)
 			continue;
 		}
 		if (!strcmp(arg, "--stdin")) {
-			if (read_from_stdin++)
-				die("--stdin given twice?");
 			read_revisions_from_stdin(&revs);
 			continue;
 		}
diff --git a/revision.c b/revision.c
index 0191160..c1550c4 100644
--- a/revision.c
+++ b/revision.c
@@ -914,7 +914,7 @@ int handle_revision_arg(const char *arg, struct rev_info *revs,
 void read_revisions_from_stdin(struct rev_info *revs)
 {
 	char line[1000];
-
+	if (feof(stdin)) return;
 	while (fgets(line, sizeof(line), stdin) != NULL) {
 		int len = strlen(line);
 		if (len && line[len - 1] == '\n')
-- 
1.5.5.1.211.g65ea3.dirty

[PATCH] Teach git-bundle to read revision arguments from stdin like

[Adam Brewster]

This patch allows the caller to feed the revision parameters to
git-bundle from its standard input.  This way, a script do not have to
worry about limitation of the length of command line.

Documentation/git-bundle.txt says that git-bundle takes arguments
acceptable to git-rev-list.  Obviously some arguments that git-rev-list
handles don't make sense for git-bundle (e.g. --bisect) but --stdin is
pretty reasonable.

Signed-off-by: Adam Brewster <asb@bu.edu>
---
 bundle.c |   10 ++++++++--
 1 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/bundle.c b/bundle.c
index 0ba5df1..8d486f3 100644
--- a/bundle.c
+++ b/bundle.c
@@ -227,8 +227,14 @@ int create_bundle(struct bundle_header *header, const char *path,
 
 	/* write references */
 	argc = setup_revisions(argc, argv, &revs, NULL);
-	if (argc > 1)
-		return error("unrecognized argument: %s'", argv[1]);
+
+	for (i = 1; i < argc; i++) {
+		if (!strcmp(argv[i], "--stdin")) {
+			read_revisions_from_stdin(&revs);
+			continue;
+		}
+		return error("unrecognized argument: %s'", argv[i]);
+	}
 
 	for (i = 0; i < revs.pending.nr; i++) {
 		struct object_array_entry *e = revs.pending.objects + i;
-- 
1.5.5.1.211.g65ea3.dirty

Re: [PATCH v5]

[Junio C Hamano]

Adam Brewster <adambrewster@gmail.com> writes:

> The real reason read_revisions_from_stdin moved to revision.c is because I was 
> asked to do it that way.

Yeah, it is simply a bad taste to use helper in builtin-A from builtin-B.
More library-ish files such as revision.c are better home for utility
functions to be shared between builtins and commands.

Here is what I queued.

By the way did you compile test your fix before sending?

-- >8 --
Move read_revisions_from_stdin from builtin-rev-list.c to revision.c

Reading rev-list parameters from the command line can be reused by
commands other than rev-list.  Move this function to more "library-ish"
place to promote code reuse.

Signed-off-by: Adam Brewster <asb@bu.edu>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/builtin-rev-list.c b/builtin-rev-list.c
index 83a7b13..54b6672 100644
--- a/builtin-rev-list.c
+++ b/builtin-rev-list.c
@@ -565,23 +565,6 @@ static struct commit_list *find_bisection(struct commit_list *list,
 	return best;
 }
 
-static void read_revisions_from_stdin(struct rev_info *revs)
-{
-	char line[1000];
-
-	while (fgets(line, sizeof(line), stdin) != NULL) {
-		int len = strlen(line);
-		if (len && line[len - 1] == '\n')
-			line[--len] = 0;
-		if (!len)
-			break;
-		if (line[0] == '-')
-			die("options not supported in --stdin mode");
-		if (handle_revision_arg(line, revs, 0, 1))
-			die("bad revision '%s'", line);
-	}
-}
-
 int cmd_rev_list(int argc, const char **argv, const char *prefix)
 {
 	struct commit_list *list;
diff --git a/revision.c b/revision.c
index fc66755..6ce6042 100644
--- a/revision.c
+++ b/revision.c
@@ -910,6 +910,23 @@ int handle_revision_arg(const char *arg, struct rev_info *revs,
 	return 0;
 }
 
+void read_revisions_from_stdin(struct rev_info *revs)
+{
+	char line[1000];
+
+	while (fgets(line, sizeof(line), stdin) != NULL) {
+		int len = strlen(line);
+		if (len && line[len - 1] == '\n')
+			line[--len] = '\0';
+		if (!len)
+			break;
+		if (line[0] == '-')
+			die("options not supported in --stdin mode");
+		if (handle_revision_arg(line, revs, 0, 1))
+			die("bad revision '%s'", line);
+	}
+}
+
 static void add_grep(struct rev_info *revs, const char *ptn, enum grep_pat_token what)
 {
 	if (!revs->grep_filter) {
diff --git a/revision.h b/revision.h
index abce500..83f364a 100644
--- a/revision.h
+++ b/revision.h
@@ -111,6 +111,8 @@ struct rev_info {
 #define REV_TREE_DIFFERENT	2
 
 /* revision.c */
+void read_revisions_from_stdin(struct rev_info *revs);
+
 typedef void (*show_early_output_fn_t)(struct rev_info *, struct commit_list *);
 volatile show_early_output_fn_t show_early_output;
 

Re: [PATCH v5]

[Adam Brewster]

>
> Yeah, it is simply a bad taste to use helper in builtin-A from builtin-B.
> More library-ish files such as revision.c are better home for utility
> functions to be shared between builtins and commands.
>
> Here is what I queued.
>

Thank you.

> By the way did you compile test your fix before sending?
>

I ran make and test, but I didn't notice the warnings that prompted
the question.  I also forgot to re-check it after re-working the
commits to put everything in order.

> -- >8 --

Thank you again for your help and patience in dealing with my multiple
failed attempts to get this right.

Adam

Re: [PATCH] Move read_revisions_from_stdin from builtin-rev-list.c to revision.c

[Junio C Hamano]

Miklos Vajna <vmiklos@frugalware.org> writes:

> I think it's fine to move such a function, but this is a false commit
> message, you can use read_revisions_from_stdin() from builtin-bundle if
> it lives in builtin-rev-list.c as well.

At the mechanical level, yes you _can_, but it is simply a bad taste to do
so.  More library-ish files such as revision.c are better home for utility
functions to be shared between builtins and commands.

Re: [PATCH v3 1/3] ref-filter: add worktreepath atom

[Jeff King]

On Wed, Dec 19, 2018 at 11:09:59PM -0800, Nickolai Belakovski wrote:

> > Also, why are we replacing it with a single space? Wouldn't the empty
> > string be more customary (and work with the other "if empty, then do
> > this" formatting options)?
> 
> I was just following what was done for HEAD, but overall I agree that
> empty is preferable to single space, will change.

Ah, right, that makes more sense. I do still think for %(HEAD) it's a
little different because it is "+" or a single space, so always one
character.  Here we have some value or not, and in the "not" case for
such things we usually give an empty string (e.g., for %(push),
%(upstream), etc).

> To sum up the hashmap comments:
> -I hadn't thought to re-use the head_ref of worktree as the key.
> That's clever. I like the readability of having separate entries for
> key and value, but I can see the benefit of not having to do an extra
> allocation. I can make up for the readability hit with a comment.

Thanks, that makes sense.

> -Actually, for any valid use case there will only be one instance of
> the map since the entries of used_atom are cached, but regardless it
> makes sense to keep per-atom info in used_atom and global context
> somewhere else, so I'll make that change to make it a static variable
> outside of used_atom.

Ah, right, I forgot there was some magic around used_atom. I do still
agree that the separate static global makes things a little simpler.

> -Will change the lookup logic to remove the extra allocation. Since
> I'm letting the hashmap use its internal comparison function on the
> hash, I don't need to provide a comparison function.

I don't think that works. The default function is always_equal(), which
will treat two entries equal if they have the same hash value. I.e., any
collisions would be considered a match.

> Thanks for all the feedback, will try to turn these around quickly.

Great, thanks! I'll be on vacation for the next two weeks, so I may be
very slow to look at the next iteration. :)

-Peff

[PATCH v4 0/3]

[nbelakovski]

From: Nickolai Belakovski <nbelakovski@gmail.com>

> I don't think that works. The default function is always_equal(), which
> will treat two entries equal if they have the same hash value. I.e., any
> collisions would be considered a match.

You're absolutely right. I've added a compare function, but I left out the
functionality for it to work with an entry passed in as a key. Doing so would
mean the user would have to allocate a worktree struct, which just seems silly
when the ref is all that's needed (and also defeats the purpose of avoiding
extra allocations).

And while most of the hashmap API seems OK, yea, this is definitely awful. It
feels like it should just be able to take a key and return either an entry or
NULL, and do away with entry_or_key and equals_function_data.

Travis-CI results: https://travis-ci.org/nbelakovski/git/builds/471787317

Nickolai Belakovski (3):
  ref-filter: add worktreepath atom
  branch: Mark and color a branch differently if it is checked out in a
    linked worktree
  branch: Add an extra verbose output displaying worktree path for refs
    checked out in a linked worktree

 Documentation/git-for-each-ref.txt |  5 +++
 builtin/branch.c                   | 16 ++++++---
 ref-filter.c                       | 72 +++++++++++++++++++++++++++++++++++++-
 t/t3200-branch.sh                  |  8 ++---
 t/t3203-branch-output.sh           | 21 +++++++++++
 t/t6302-for-each-ref-filter.sh     | 15 ++++++++
 6 files changed, 128 insertions(+), 9 deletions(-)

-- 
2.14.2

Re: [PATCH v4 0/3]

[Jeff King]

On Mon, Dec 24, 2018 at 12:47:53AM -0800, nbelakovski@gmail.com wrote:

> From: Nickolai Belakovski <nbelakovski@gmail.com>
> 
> > I don't think that works. The default function is always_equal(), which
> > will treat two entries equal if they have the same hash value. I.e., any
> > collisions would be considered a match.
> 
> You're absolutely right. I've added a compare function, but I left out the
> functionality for it to work with an entry passed in as a key. Doing so would
> mean the user would have to allocate a worktree struct, which just seems silly
> when the ref is all that's needed (and also defeats the purpose of avoiding
> extra allocations).

Unfortunately, that doesn't quite work. :)

Your compare function has to handle _both_ cases: two keys, or one key
and a keydata.

The former may be called when the hashmap has to compare two entries
internally (e.g., when it has to re-bucket all of the entries after a
resize). Your tests likely wouldn't run into this case in practice,
since you'd only have a handful of worktrees. But if you added, say,
thousands of worktrees and we had to grow the hash midway through the
process, it would segfault.  So you do need to handle the case when your
keydata is NULL.

In theory it would also be used for comparisons if we used a more clever
data structure to hold entries within a bucket. But since we just use a
linked list and linear search for now, we don't.

> And while most of the hashmap API seems OK, yea, this is definitely awful. It
> feels like it should just be able to take a key and return either an entry or
> NULL, and do away with entry_or_key and equals_function_data.

In your case, yeah, equals_function_data is not used at all (but there
are a few call-sites which need it to avoid relying on global data).

But the entry_or_key (coupled with keydata) is the magic that lets the
same function be used for both lookups as well as internal entry
comparisons.

I think having two separate comparison functions would make this a lot
more clear, though likely at the cost of having more boilerplate.

-Peff