threads / patch / 12077

patchhash-object: don't rely on order of --stdin, -w arguments

Subject: [PATCH] hash-object: don't rely on order of --stdin, -w arguments

## tl;dr

7 messages between Feb 13, 2008 and Feb 21, 2008. Diffs are folded; open one to read it.

replies: 6people: 2as markdown or json

Gerrit Pape· Feb 13, 2008, 19:03 UTC · lore

Fix 'git hash-object --stdin -w' to actually write the object, just as 'git hash-object -w --stdin' does.

Reported by Josh Triplett through
 http://bugs.debian.org/464432
Signed-off-by: Gerrit Pape <pape@smarden.org>
---
 hash-object.c |    8 +++++---
 1 files changed, 5 insertions(+), 3 deletions(-)
Show changes to hash-object.c +5 −3
diff --git a/hash-object.c b/hash-object.c
index 0a58f3f..ff60f0f 100644
--- a/hash-object.c
+++ b/hash-object.c
@@ -41,6 +41,7 @@ int main(int argc, char **argv)
 	const char *prefix = NULL;
 	int prefix_length = -1;
 	int no_more_flags = 0;
+	int hashstdin = 0;
 
 	git_config(git_default_config);
 
@@ -64,9 +65,8 @@ int main(int argc, char **argv)
 			}
 			else if (!strcmp(argv[i], "--help"))
 				usage(hash_object_usage);
-			else if (!strcmp(argv[i], "--stdin")) {
-				hash_stdin(type, write_object);
-			}
+			else if (!strcmp(argv[i], "--stdin"))
+				hashstdin = 1;
 			else
 				usage(hash_object_usage);
 		}
@@ -79,5 +79,7 @@ int main(int argc, char **argv)
 			no_more_flags = 1;
 		}
 	}
+	if (hashstdin)
+		hash_stdin(type, write_object);
 	return 0;
 }
-- 
1.5.4.1
Junio C Hamano· Feb 13, 2008, 19:50 UTC · re: Gerrit Pape · lore

Re: [PATCH] hash-object: don't rely on order of --stdin, -w arguments

Gerrit Pape <pape@smarden.org> writes:
> Fix 'git hash-object --stdin -w' to actually write the object, just as
> 'git hash-object -w --stdin' does.
Can we have additions to the test scripts to illustrate:
 - what used to happen if you had "--stdin -w" on the command
   line (presumably you did not get the object written, which is
   a buggy behaviour and the patch should fix it);
 - what used to happen if you had "-w --stdin --stdin" on the
   command line (I imagine you used to be able to feed two
   separate streams when you are on a terminal by typing ^Ds in
   between?);
 - any other combination that may deserve testing.

in order to (1) show existing bugs the patch is trying to address, (2) demonstrate that the patch fixes them without regressing, and (3) protect the fix from being broken by future changes, pretty please?

Gerrit Pape· Feb 13, 2008, 22:49 UTC · re: Junio C Hamano · lore

[PATCH/selftest] hash-object: don't rely on order of --stdin, -w arguments

Fix 'git hash-object --stdin -w' to actually write the object, just as 'git hash-object -w --stdin' does.

Reported by Josh Triplett through
 http://bugs.debian.org/464432
Signed-off-by: Gerrit Pape <pape@smarden.org>
---
 hash-object.c          |    8 +++++---
 t/t5303-hash-object.sh |   19 +++++++++++++++++++
 2 files changed, 24 insertions(+), 3 deletions(-)
 create mode 100755 t/t5303-hash-object.sh
Show changes to 2 files +24 −3

hash-object.c, t/t5303-hash-object.sh

diff --git a/hash-object.c b/hash-object.c
index 0a58f3f..ff60f0f 100644
--- a/hash-object.c
+++ b/hash-object.c
@@ -41,6 +41,7 @@ int main(int argc, char **argv)
 	const char *prefix = NULL;
 	int prefix_length = -1;
 	int no_more_flags = 0;
+	int hashstdin = 0;
 
 	git_config(git_default_config);
 
@@ -64,9 +65,8 @@ int main(int argc, char **argv)
 			}
 			else if (!strcmp(argv[i], "--help"))
 				usage(hash_object_usage);
-			else if (!strcmp(argv[i], "--stdin")) {
-				hash_stdin(type, write_object);
-			}
+			else if (!strcmp(argv[i], "--stdin"))
+				hashstdin = 1;
 			else
 				usage(hash_object_usage);
 		}
@@ -79,5 +79,7 @@ int main(int argc, char **argv)
 			no_more_flags = 1;
 		}
 	}
+	if (hashstdin)
+		hash_stdin(type, write_object);
 	return 0;
 }
diff --git a/t/t5303-hash-object.sh b/t/t5303-hash-object.sh
new file mode 100755
index 0000000..23dcfcb
--- /dev/null
+++ b/t/t5303-hash-object.sh
@@ -0,0 +1,19 @@
+#!/bin/sh
+
+test_description=git-hash-object
+
+. ./test-lib.sh
+
+test_expect_success \
+    'git hash-object -w --stdin saves the object' \
+    'echo foo | git hash-object -w --stdin &&
+    test -r .git/objects/25/7cc5642cb1a054f08cc83f2d943e56fd3ebe99 &&
+    rm -f .git/objects/25/7cc5642cb1a054f08cc83f2d943e56fd3ebe99'
+    
+test_expect_success \
+    'git hash-object --stdin -w saves the object' \
+    'echo foo | git hash-object --stdin -w &&
+    test -r .git/objects/25/7cc5642cb1a054f08cc83f2d943e56fd3ebe99 &&
+    rm -f .git/objects/25/7cc5642cb1a054f08cc83f2d943e56fd3ebe99'    
+
+test_done
-- 
1.5.4.1
Junio C Hamano· Feb 13, 2008, 23:25 UTC · re: Gerrit Pape · lore

Re: [PATCH/selftest] hash-object: don't rely on order of --stdin, -w arguments

Gerrit Pape <pape@smarden.org> writes:
Show 7 quoted lines
> Fix 'git hash-object --stdin -w' to actually write the object, just as
> 'git hash-object -w --stdin' does.
>
> Reported by Josh Triplett through
>  http://bugs.debian.org/464432
>
> Signed-off-by: Gerrit Pape <pape@smarden.org>

Thanks. I think the patch itself is a good fix to the old design mistake we made.

However, I would _really_ like to see something like the following mentioned in the proposed commit log message for discussion:

    This regresses the use case of running:
        $ git hash-object --stdin Makefile <cache.h
    to obtain hash values for cache.h and then Makefile.  It
    used to report the object names in order, but now it always
    processes --stdin at the end.  It is not an issue if
    everything is file (i.e. "git hash-object cache.h Makefile"
    is an easy replacement), but if existing scripts used the
    option to read from a pipe, this might become problematic.
    Similarly, when typing from terminal:
        $ git hash-object --stdin --stdin
        foo
        ^D
        bar
        ^D
    used to work, but not anymore.  The latter however should
    not be something we need to worry about.  It is an insane
    usage.

Granted, we _should have_ prevented such insane usages (the second one is definitely insane and not so useful, and the first one is but to a much lessor degree). We _should have_ forced scripted users to do these with two separate invocations of "git hash-object", by refusing more than one --stdin and --stdin with filename on the command line. But we didn't, and this earlier design mistake has already been in the field for a long time. People may have been depending on the existing misbehaviour.

So I am Ok with the patch, and I strongly suspect that we should even detect more than one --stdin and --stdin with filename on the command line to reject the usage your saner version behaves differently from before, instead of accepting and silently doing an unexpected thing.

But at the same time I would really like our commit messages, and Release Notes that is based on these commit messages, to be _candid_ about our previous mistakes and the incompatibility we are incurring to the existing users.

Show 5 quoted lines
> +test_expect_success \
> +    'git hash-object -w --stdin saves the object' \
> +    'echo foo | git hash-object -w --stdin &&
> +    test -r .git/objects/25/7cc5642cb1a054f08cc83f2d943e56fd3ebe99 &&
> +    rm -f .git/objects/25/7cc5642cb1a054f08cc83f2d943e56fd3ebe99'

I'd feel better if tests outside t0000 did not hardcode the actual values, like this:

	obname=$(echo foo | git hash-object -w --stdin) &&
        ob=$(expr "$obname" : "\(..\)") &&
        name=$(expr "$obname" : "..\(.*\)") &&
        test -f ".git/objects/$ob/$name"
Gerrit Pape· Feb 14, 2008, 20:13 UTC · re: Junio C Hamano · lore

[PATCH] hash-object: cleanup handling of command line options

git hash-object used to process the --stdin command line argument before reading subsequent arguments. This caused 'git hash-object --stdin -w' to fail to actually write the object into the database, while '-w --stdin' properly did. Now git hash-object processes --stdin at the end, after all other arguments.

This regresses the use case of running:
   $ git hash-object --stdin Makefile <cache.h

to obtain hash values for cache.h and then Makefile. It used to report the object names in order, but now it always processes --stdin at the end. It is not an issue if everything is file (i.e. "git hash-object cache.h Makefile" is an easy replacement), but if existing scripts used the option to read from a pipe, this might become problematic.

Furthermore git hash-object used to allow multiple --stdin arguments on the command line, supporting insane usage, such as

   $ git hash-object --stdin --stdin
     foo
     ^D
     bar
     ^D
Now git hash-object errors out if --stdin is given more than once.
Reported by Josh Triplett through
 http://bugs.debian.org/464432
Signed-off-by: Gerrit Pape <pape@smarden.org>
---
 hash-object.c          |    7 ++++++-
 t/t5303-hash-object.sh |   35 +++++++++++++++++++++++++++++++++++
 2 files changed, 41 insertions(+), 1 deletions(-)
 create mode 100755 t/t5303-hash-object.sh
Show changes to 2 files +41 −1

hash-object.c, t/t5303-hash-object.sh

diff --git a/hash-object.c b/hash-object.c
index 0a58f3f..67d9922 100644
--- a/hash-object.c
+++ b/hash-object.c
@@ -41,6 +41,7 @@ int main(int argc, char **argv)
 	const char *prefix = NULL;
 	int prefix_length = -1;
 	int no_more_flags = 0;
+	int hashstdin = 0;
 
 	git_config(git_default_config);
 
@@ -65,7 +66,9 @@ int main(int argc, char **argv)
 			else if (!strcmp(argv[i], "--help"))
 				usage(hash_object_usage);
 			else if (!strcmp(argv[i], "--stdin")) {
-				hash_stdin(type, write_object);
+				if (hashstdin)
+					die("Multiple --stdin arguments are not supported");
+				hashstdin = 1;
 			}
 			else
 				usage(hash_object_usage);
@@ -79,5 +82,7 @@ int main(int argc, char **argv)
 			no_more_flags = 1;
 		}
 	}
+	if (hashstdin)
+		hash_stdin(type, write_object);
 	return 0;
 }
diff --git a/t/t5303-hash-object.sh b/t/t5303-hash-object.sh
new file mode 100755
index 0000000..14be455
--- /dev/null
+++ b/t/t5303-hash-object.sh
@@ -0,0 +1,35 @@
+#!/bin/sh
+
+test_description=git-hash-object
+
+. ./test-lib.sh
+
+test_expect_success \
+    'git hash-object -w --stdin saves the object' \
+    'obname=$(echo foo | git hash-object -w --stdin) &&
+    obpath=$(echo $obname | sed -e "s/\(..\)/\1\//") &&
+    test -r .git/objects/"$obpath" &&
+    rm -f .git/objects/"$obpath"'
+    
+test_expect_success \
+    'git hash-object --stdin -w saves the object' \
+    'obname=$(echo foo | git hash-object --stdin -w) &&
+    obpath=$(echo $obname | sed -e "s/\(..\)/\1\//") &&
+    test -r .git/objects/"$obpath" &&
+    rm -f .git/objects/"$obpath"'    
+
+test_expect_success \
+    'git hash-object --stdin file0 <file1 first operates on file0, then file1' \
+    'echo foo > file0 &&
+    obname0=$(git hash-object file0) &&
+    obname1=$(echo bar | git hash-object --stdin) &&
+    obname0new=$(echo bar | git hash-object --stdin file0 | sed -n -e 1p) &&
+    obname1new=$(echo bar | git hash-object --stdin file0 | sed -n -e 2p) &&
+    test "$obname0" = "$obname0new" &&
+    test "$obname1" = "$obname1new"'
+
+test_expect_success \
+    'git hash-object refuses multiple --stdin arguments' \
+    '! git hash-object --stdin --stdin < file0'
+
+test_done
-- 
1.5.4.1
Junio C Hamano· Feb 15, 2008, 17:31 UTC · re: Gerrit Pape · lore

Re: [PATCH] hash-object: cleanup handling of command line options

Gerrit Pape <pape@smarden.org> writes:
Show 7 quoted lines
> This regresses the use case of running:
>
>    $ git hash-object --stdin Makefile <cache.h
>
> to obtain hash values for cache.h and then Makefile.  It used to
> report the object names in order, but now it always processes
> --stdin at the end.

Isn't this change of behaviour more serious than the made up "--stdin --stdin" example? The made-up insane usage is rejected with your patch now, so it would be fine, but that is so insane that we probably even did not need to explicitly reject. On the other hand, running "--stdin file2 <file1" and expecting the output to be given for file1 first and then file2 is much less insane, and it would make more sense to reject it if we cannot support that earlier behaviour. Not rejecting such a use and doing different thing from what we used to will break existing uses silently.

I suppose we could first see if there is -w in the first pass and then in the second pass do exactly what we used to do, if we want to fix this without regressing at all. Then there won't be any regression, even "--stdin --stdin" would keep working.

Except for one case.

Earlier you could "hash-object --stdin -w cache.h <Makefile" to get the hash of Makefile without writing to the object store, and get the hash of cache.h and write that to the object store.

With such a fix with less regression, we will get the two object names in the right order (--stdin first and then explicit filename, as the user wanted to have), but now we write what we get from --stdin to the object database, even though the user did _not_ want to (that is why he wrote -w after --stdin, not before).

Hmmmm. Why is it so bad if "--stdin -w" and "-w --stdin" works differently?

Having said all that, I think "--stdin file2 <file1" behaviour can be kept without regressing by a patch like this on top of your fix, and we can drop the first "regression warning" in the commit log message if we did so.

---
 hash-object.c          |    5 +++++
 t/t5303-hash-object.sh |    4 ++--
 2 files changed, 7 insertions(+), 2 deletions(-)
Show changes to 2 files +7 −2

hash-object.c, t/t5303-hash-object.sh

diff --git a/hash-object.c b/hash-object.c
index 67d9922..61e7160 100644
--- a/hash-object.c
+++ b/hash-object.c
@@ -75,6 +75,11 @@ int main(int argc, char **argv)
 		}
 		else {
 			const char *arg = argv[i];
+
+			if (hashstdin) {
+				hash_stdin(type, write_object);
+				hashstdin = 0;
+			}
 			if (0 <= prefix_length)
 				arg = prefix_filename(prefix, prefix_length,
 						      arg);
diff --git a/t/t5303-hash-object.sh b/t/t5303-hash-object.sh
index 14be455..7c4f9fa 100755
--- a/t/t5303-hash-object.sh
+++ b/t/t5303-hash-object.sh
@@ -21,8 +21,8 @@ test_expect_success \
 test_expect_success \
     'git hash-object --stdin file0 <file1 first operates on file0, then file1' \
     'echo foo > file0 &&
-    obname0=$(git hash-object file0) &&
-    obname1=$(echo bar | git hash-object --stdin) &&
+    obname0=$(echo bar | git hash-object --stdin) &&
+    obname1=$(git hash-object file0) &&
     obname0new=$(echo bar | git hash-object --stdin file0 | sed -n -e 1p) &&
     obname1new=$(echo bar | git hash-object --stdin file0 | sed -n -e 2p) &&
     test "$obname0" = "$obname0new" &&
Gerrit Pape· Feb 21, 2008, 10:06 UTC · re: Junio C Hamano · lore

[PATCH] hash-object: cleanup handling of command line options

git hash-object used to process the --stdin command line argument before reading subsequent arguments. This caused 'git hash-object --stdin -w' to fail to actually write the object into the database, while '-w --stdin' properly did. Now git hash-object first reads all arguments, and then processes them.

This regresses one insane use case. git hash-object used to allow multiple --stdin arguments on the command line:

   $ git hash-object --stdin --stdin
     foo
     ^D
     bar
     ^D
Now git hash-object errors out if --stdin is given more than once.
Reported by Josh Triplett through
 http://bugs.debian.org/464432
Signed-off-by: Gerrit Pape <pape@smarden.org>
---
On Fri, Feb 15, 2008 at 09:31:10AM -0800, Junio C Hamano wrote:
> Having said all that, I think "--stdin file2 <file1" behaviour
> can be kept without regressing by a patch like this on top of
> your fix, and we can drop the first "regression warning" in the
> commit log message if we did so.

Yes, this seems the better choice. I adapted the commit message a bit, and swapped file0 and file1 in the test description and the selftest where appropriate, it's been mixed up.

Regards, Gerrit.
 hash-object.c          |   12 +++++++++++-
 t/t5303-hash-object.sh |   35 +++++++++++++++++++++++++++++++++++
 2 files changed, 46 insertions(+), 1 deletions(-)
 create mode 100755 t/t5303-hash-object.sh
Show changes to 2 files +46 −1

hash-object.c, t/t5303-hash-object.sh

diff --git a/hash-object.c b/hash-object.c
index 0a58f3f..61e7160 100644
--- a/hash-object.c
+++ b/hash-object.c
@@ -41,6 +41,7 @@ int main(int argc, char **argv)
 	const char *prefix = NULL;
 	int prefix_length = -1;
 	int no_more_flags = 0;
+	int hashstdin = 0;
 
 	git_config(git_default_config);
 
@@ -65,13 +66,20 @@ int main(int argc, char **argv)
 			else if (!strcmp(argv[i], "--help"))
 				usage(hash_object_usage);
 			else if (!strcmp(argv[i], "--stdin")) {
-				hash_stdin(type, write_object);
+				if (hashstdin)
+					die("Multiple --stdin arguments are not supported");
+				hashstdin = 1;
 			}
 			else
 				usage(hash_object_usage);
 		}
 		else {
 			const char *arg = argv[i];
+
+			if (hashstdin) {
+				hash_stdin(type, write_object);
+				hashstdin = 0;
+			}
 			if (0 <= prefix_length)
 				arg = prefix_filename(prefix, prefix_length,
 						      arg);
@@ -79,5 +87,7 @@ int main(int argc, char **argv)
 			no_more_flags = 1;
 		}
 	}
+	if (hashstdin)
+		hash_stdin(type, write_object);
 	return 0;
 }
diff --git a/t/t5303-hash-object.sh b/t/t5303-hash-object.sh
new file mode 100755
index 0000000..543c078
--- /dev/null
+++ b/t/t5303-hash-object.sh
@@ -0,0 +1,35 @@
+#!/bin/sh
+
+test_description=git-hash-object
+
+. ./test-lib.sh
+
+test_expect_success \
+    'git hash-object -w --stdin saves the object' \
+    'obname=$(echo foo | git hash-object -w --stdin) &&
+    obpath=$(echo $obname | sed -e "s/\(..\)/\1\//") &&
+    test -r .git/objects/"$obpath" &&
+    rm -f .git/objects/"$obpath"'
+    
+test_expect_success \
+    'git hash-object --stdin -w saves the object' \
+    'obname=$(echo foo | git hash-object --stdin -w) &&
+    obpath=$(echo $obname | sed -e "s/\(..\)/\1\//") &&
+    test -r .git/objects/"$obpath" &&
+    rm -f .git/objects/"$obpath"'    
+
+test_expect_success \
+    'git hash-object --stdin file1 <file0 first operates on file0, then file1' \
+    'echo foo > file1 &&
+    obname0=$(echo bar | git hash-object --stdin) &&
+    obname1=$(git hash-object file1) &&
+    obname0new=$(echo bar | git hash-object --stdin file1 | sed -n -e 1p) &&
+    obname1new=$(echo bar | git hash-object --stdin file1 | sed -n -e 2p) &&
+    test "$obname0" = "$obname0new" &&
+    test "$obname1" = "$obname1new"'
+
+test_expect_success \
+    'git hash-object refuses multiple --stdin arguments' \
+    '! git hash-object --stdin --stdin < file1'
+
+test_done
-- 
1.5.4.2

← back to recent threads