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

"Adam Brewster" [off-list ref] 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.

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.

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.

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

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

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

On Sat, Jul 05, 2008 at 04:40:32PM -0400, Adam Brewster [off-list ref] 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? ;-)

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

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

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

Miklos Vajna [off-list ref] 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.

Adam Brewster [off-list ref] writes:

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;

On Sat, Jul 5, 2008 at 8:57 PM, Junio C Hamano [off-list ref] 

quoted

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

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

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