something fishy with Git commit and log from file

PATCH In review Pascal Obry 6 participants 15 messages Jun 15, 2016

Share & tools

Pascal Obry

author

2016-06-15 22:45 UTC #0 lore raw

The following command fails on my repository:

   $ git commit --file=clog
   fatal: could not read log file 'clog': No such file or directory

   $ cat clog
   toto

Using the following command the commit pass without problem:

   $ cat clog | git commit --file=-

With GDB I get:

$ gdb --args git ci --file=clog
This GDB was configured as "i686-pc-cygwin"...
(gdb) break strbuf_read_file
Breakpoint 1 at 0x44e64a: file strbuf.c, line 301.
(gdb) run
Starting program: /usr/local/bin/git.exe ci --file=clog

Breakpoint 1, strbuf_read_file (sb=0x22cac0, path=0x22ccfa "clog", hint=0) at strbuf.c:301
301 fd = open(path, O_RDONLY);
(gdb) print path
$1 = 0x0

???? outch, this is strange, or a gdb artifact?

(gdb) n
302 if (fd < 0)
(gdb) print fd
$2 = -1
(gdb) print *0x22ccfa
$3 = 1735355491
(gdb) print (char)*0x22ccfa
$4 = 99 'c'
(gdb) print (char)*0x22ccfb
$5 = 108 'l'
(gdb) print (char)*0x22ccfc
$6 = 111 'o'
(gdb) print (char)*0x22ccfd
$7 = 103 'g'
(gdb) print (char)*0x22ccfe
$8 = 0 '\0'

But looks like path really contains 'clog'!!!

No luck for now to find the problem. Note that the same command pass fine on other repositories. At this point this looks really like some kind of memory corruption...

I'm on Windows, using Git for Cygwin and the compiler is:

$ gcc --version
gcc (GCC) 3.4.4 (cygming special, gdc 0.12, using dmd 0.125)

Pascal.

Signature · 8 lines

--|------------------------------------------------------
--| Pascal Obry                           Team-Ada Member
--| 45, rue Gabriel Peri - 78114 Magny Les Hameaux FRANCE
--|------------------------------------------------------
--|              http://www.obry.net
--| "The best way to travel is by means of imagination"
--|
--| gpg --keyserver wwwkeys.pgp.net --recv-key C1082595

Luciano Rocha

2016-06-15 22:45 UTC #1 lore raw

Quoted 22 lines · Pascal Obry

The following command fails on my repository:

    $ git commit --file=clog
    fatal: could not read log file 'clog': No such file or directory

    $ cat clog
    toto

Using the following command the commit pass without problem:

    $ cat clog | git commit --file=-

With GDB I get:

 $ gdb --args git ci --file=clog
 This GDB was configured as "i686-pc-cygwin"...
 (gdb) break strbuf_read_file
 Breakpoint 1 at 0x44e64a: file strbuf.c, line 301.
 (gdb) run
 Starting program: /usr/local/bin/git.exe ci --file=clog

Quoted 7 lines

Breakpoint 1, strbuf_read_file (sb=0x22cac0, path=0x22ccfa "clog", hint=0) at strbuf.c:301
301 fd = open(path, O_RDONLY);
(gdb) print path
$1 = 0x0
???? outch, this is strange, or a gdb artifact?

gdb artifact. The breakpoint info shows the correct value.

Could you at that point run the following?

(gdb) p get_current_dir_name()
$1 = ... "..."

If the returned value doesn't match the directory you were in, then there's a bug in git.

If the function doesn't exist, try:
(gdb) p getwd(malloc(2048))

Regards,
Luciano Rocha

Signature · 2 lines

Luciano Rocha <[email protected]>
Eurotux Informática, S.A. <http://www.eurotux.com/>

Pascal Obry

author

2016-06-15 22:45 UTC #2 lore raw

Luciano,

Quoted 2 lines

If the function doesn't exist, try:
(gdb) p getwd(malloc(2048))

I got:

(gdb) p (char *)getwd(malloc(2048))
$3 = 0x903680 "/home/obry/dev/repositories/git/AWS"

And this is not the directory I was in. It is the Git root. I was under /home/obry/dev/repositories/git/AWS/regtests when running the command.

So definitely a Git bug! Can be reproduced with:

   $ mkdir repo && cd repo
   $ git init
   $ mkdir dir
   $ cd dir
   $ echo file > file
   $ echo log > log
   $ git add file
   $ git commit --file=log
   fatal: could not read log file 'log': No such file or directory

Pascal.

Signature · 8 lines

--|------------------------------------------------------
--| Pascal Obry                           Team-Ada Member
--| 45, rue Gabriel Peri - 78114 Magny Les Hameaux FRANCE
--|------------------------------------------------------
--|              http://www.obry.net
--| "The best way to travel is by means of imagination"
--|
--| gpg --keyserver wwwkeys.pgp.net --recv-key C1082595

Junio C Hamano

2016-06-15 22:45 UTC #3 lore raw

Quoted 11 lines · Pascal Obry

So definitely a Git bug! Can be reproduced with:

   $ mkdir repo && cd repo
   $ git init
   $ mkdir dir
   $ cd dir
   $ echo file > file
   $ echo log > log
   $ git add file
   $ git commit --file=log
   fatal: could not read log file 'log': No such file or directory

Try it without cding down to "dir".

Pascal Obry

author

2016-06-15 22:45 UTC #4 lore raw

Quoted 14 lines · Junio C Hamano

Quoted 11 lines · Pascal Obry

So definitely a Git bug! Can be reproduced with:

   $ mkdir repo && cd repo
   $ git init
   $ mkdir dir
   $ cd dir
   $ echo file > file
   $ echo log > log
   $ git add file
   $ git commit --file=log
   fatal: could not read log file 'log': No such file or directory

Quoted 1 line

Try it without cding down to "dir".

Yes it works. It also works if I do:

    $ echo log > ../log

instead of

    $ echo log > log

Git is looking for the log file at the Git root.

Pascal.

Signature · 8 lines

--|------------------------------------------------------
--| Pascal Obry                           Team-Ada Member
--| 45, rue Gabriel Peri - 78114 Magny Les Hameaux FRANCE
--|------------------------------------------------------
--|              http://www.obry.net
--| "The best way to travel is by means of imagination"
--|
--| gpg --keyserver wwwkeys.pgp.net --recv-key C1082595

Junio C Hamano

2016-06-15 22:45 UTC #5 lore raw

Quoted 15 lines · Pascal Obry

Quoted 14 lines · Junio C Hamano
Quoted 11 lines · Pascal Obry
So definitely a Git bug! Can be reproduced with:
   $ mkdir repo && cd repo
   $ git init
   $ mkdir dir
   $ cd dir
   $ echo file > file
   $ echo log > log
   $ git add file
   $ git commit --file=log
   fatal: could not read log file 'log': No such file or directory

Quoted 3 lines

Quoted 1 line
Try it without cding down to "dir".
Yes it works.

Perhaps something like this. This must be another one of those regressions introduced in C rewrite.

builtin-commit.c +4 -1

index b783e6e..fcc9c59 100644
@@ -469,7 +469,10 @@ static int prepare_to_commit(const char *index_file, const char *prefix)
 			die("could not read log from standard input");
 		hook_arg1 = "message";
 	} else if (logfile) {
-		if (strbuf_read_file(&sb, logfile, 0) < 0)
+		const char *lf = logfile;
+		if (prefix)
+			lf = prefix_filename(prefix, strlen(prefix), logfile);
+		if (strbuf_read_file(&sb, lf, 0) < 0)
 			die("could not read log file '%s': %s",
 			    logfile, strerror(errno));
 		hook_arg1 = "message";

Pascal Obry

author

2016-06-15 22:45 UTC #6 lore raw

Quoted 2 lines · Junio C Hamano

Perhaps something like this. This must be another one of those regressions introduced in C rewrite.

Works fine now. Thanks.

Pascal.

Signature · 8 lines

--|------------------------------------------------------
--| Pascal Obry                           Team-Ada Member
--| 45, rue Gabriel Peri - 78114 Magny Les Hameaux FRANCE
--|------------------------------------------------------
--|              http://www.obry.net
--| "The best way to travel is by means of imagination"
--|
--| gpg --keyserver wwwkeys.pgp.net --recv-key C1082595

Junio C Hamano

2016-06-15 22:45 UTC #7 lore raw

PATCH 1/1 files given on the command line are relative to $cwd

In reply to Junio C Hamano

When running "git commit -F file" and "git tag -F file" from a subdirectory, we should take it as relative to the directory we started from, not relative to the top-level directory.

This adds a helper function "parse_options_fix_filename()" to make it more convenient to fix this class of issues. Ideally, parse_options() should support a new type of option, "OPT_FILENAME", to do this uniformly, but this patch is meant to go to 'maint' to fix it minimally.

One thing to note is that value for "commit template file" that comes from the command line is taken as relative to $cwd just like other parameters, but when it comes from the configuration varilable 'commit.template', it is taken as relative to the working tree root as before. I think this difference actually is sensible (not that I particularly think commit.template itself is sensible).

Signed-off-by Junio C Hamano <[email protected]>

---
 builtin-commit.c  |   11 +++++++----
 builtin-tag.c     |    1 +
 parse-options.c   |   12 ++++++++++++
 parse-options.h   |    2 ++
 t/t7500-commit.sh |   11 +++++++++++
 5 files changed, 33 insertions(+), 4 deletions(-)

builtin-commit.c +7 -4

index bcbea38..0c6d1f4 100644
@@ -45,7 +45,7 @@ static enum {
 	COMMIT_PARTIAL,
 } commit_style;
 
-static char *logfile, *force_author;
+static const char *logfile, *force_author;
 static const char *template_file;
 static char *edit_message, *use_message;
 static char *author_name, *author_email, *author_date;

@@ -700,11 +700,14 @@ static int message_is_empty(struct strbuf *sb, int start)
 }
 
 static int parse_and_validate_options(int argc, const char *argv[],
-				      const char * const usage[])
+				      const char * const usage[],
+				      const char *prefix)
 {
 	int f = 0;
 
 	argc = parse_options(argc, argv, builtin_commit_options, usage, 0);
+	logfile = parse_options_fix_filename(prefix, logfile);
+	template_file = parse_options_fix_filename(prefix, template_file);
 
 	if (logfile || message.len || use_message)
 		use_editor = 0;

@@ -814,7 +817,7 @@ int cmd_status(int argc, const char **argv, const char *prefix)
 	if (wt_status_use_color == -1)
 		wt_status_use_color = git_use_color_default;
 
-	argc = parse_and_validate_options(argc, argv, builtin_status_usage);
+	argc = parse_and_validate_options(argc, argv, builtin_status_usage, prefix);
 
 	index_file = prepare_index(argc, argv, prefix);

@@ -907,7 +910,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 
 	git_config(git_commit_config, NULL);
 
-	argc = parse_and_validate_options(argc, argv, builtin_commit_usage);
+	argc = parse_and_validate_options(argc, argv, builtin_commit_usage, prefix);
 
 	index_file = prepare_index(argc, argv, prefix);

builtin-tag.c +1 -0

index 3c97c69..3f77ba9 100644
@@ -411,6 +411,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 	git_config(git_tag_config, NULL);
 
 	argc = parse_options(argc, argv, options, git_tag_usage, 0);
+	msgfile = parse_options_fix_filename(prefix, msgfile);
 
 	if (keyid) {
 		sign = 1;

parse-options.c +12 -0

index f8d52e2..d771bf4 100644
@@ -425,3 +425,15 @@ int parse_opt_approxidate_cb(const struct option *opt, const char *arg,
 	*(unsigned long *)(opt->value) = approxidate(arg);
 	return 0;
 }
+
+/*
+ * This should really be OPTION_FILENAME type as a part of
+ * parse_options that take prefix to do this while parsing.
+ */
+extern const char *parse_options_fix_filename(const char *prefix, const char *file)
+{
+	if (!file || !prefix || is_absolute_path(file))
+		return file;
+	return prefix_filename(prefix, strlen(prefix), file);
+}
+

parse-options.h +2 -0

index 4ee443d..13ad158 100644
@@ -123,4 +123,6 @@ extern int parse_opt_approxidate_cb(const struct option *, const char *, int);
 	  "use <n> digits to display SHA-1s", \
 	  PARSE_OPT_OPTARG, &parse_opt_abbrev_cb, 0 }
 
+extern const char *parse_options_fix_filename(const char *prefix, const char *file);
+
 #endif

t/t7500-commit.sh +11 -0

index baed6ce..026d787 100755
@@ -138,4 +138,15 @@ test_expect_success '--signoff' '
 	diff expect output
 '
 
+test_expect_success 'commit message from file' '
+	mkdir subdir &&
+	echo "Log in top directory" >log &&
+	echo "Log in sub directory" >subdir/log &&
+	(
+		cd subdir &&
+		git commit --allow-empty -F log
+	) &&
+	commit_msg_is "Log in sub directory"
+'
+
 test_done

Samuel Tardieu

2016-06-15 22:45 UTC #8 lore raw

Re: [PATCH] files given on the command line are relative to $cwd

Quoted 1 line

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
"Junio" == Junio C Hamano <[email protected]> writes:

Junio> When running "git commit -F file" and "git tag -F file" from a Junio> subdirectory, we should take it as relative to the directory we Junio> started from, not relative to the top-level directory.

Don't we have the same problem with "git show"? If you go into the "gitweb" directory of the GIT source, "git show HEAD:README" will show you the toplevel "README" instead of the one in the "gitweb" directory.

Sam

Signature · 1 line

Samuel Tardieu -- [email protected] -- http://www.rfc1149.net/

Luciano Rocha

2016-06-15 22:45 UTC #9 lore raw

Re: [PATCH] files given on the command line are relative to $cwd

Quoted 10 lines · Samuel Tardieu

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
"Junio" == Junio C Hamano <[email protected]> writes:
Junio> When running "git commit -F file" and "git tag -F file" from a Junio> subdirectory, we should take it as relative to the directory we Junio> started from, not relative to the top-level directory.
Don't we have the same problem with "git show"? If you go into the "gitweb" directory of the GIT source, "git show HEAD:README" will show you the toplevel "README" instead of the one in the "gitweb" directory.

No, git show has different semantics. It has been discussed often in this list.

For instance:
http://thread.gmane.org/gmane.comp.version-control.git/68786/focus=68852

Signature · 2 lines

Luciano Rocha <[email protected]>
Eurotux Informática, S.A. <http://www.eurotux.com/>

Junio C Hamano

2016-06-15 22:45 UTC #10 lore raw

Re: [PATCH] files given on the command line are relative to $cwd

Quoted 7 lines · Luciano Rocha

Quoted 6 lines · Samuel Tardieu
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
"Junio" == Junio C Hamano <[email protected]> writes:
Junio> When running "git commit -F file" and "git tag -F file" from a Junio> subdirectory, we should take it as relative to the directory we Junio> started from, not relative to the top-level directory.

Quoted 7 lines

Quoted 4 lines
Don't we have the same problem with "git show"? If you go into the "gitweb" directory of the GIT source, "git show HEAD:README" will show you the toplevel "README" instead of the one in the "gitweb" directory.
No, git show has different semantics. It has been discussed often in this list.

You are half correct --- it is not show but tree:path syntax.

Pierre Habouzit

2016-06-15 22:45 UTC #11 lore raw

Re: [PATCH] files given on the command line are relative to $cwd

In reply to Junio C Hamano

Quoted 8 lines · Junio C Hamano

When running "git commit -F file" and "git tag -F file" from a subdirectory, we should take it as relative to the directory we started from, not relative to the top-level directory.
This adds a helper function "parse_options_fix_filename()" to make it more convenient to fix this class of issues. Ideally, parse_options() should support a new type of option, "OPT_FILENAME", to do this uniformly, but this patch is meant to go to 'maint' to fix it minimally.

I'm going in vacation tomorrow so I'm not likely to do that soon, but
I agree it's sensible.

Signature · 3 lines

·O·  Pierre Habouzit
··O                                                [email protected]
OOO                                                http://www.madism.org

Olivier Marin

2016-06-15 22:45 UTC #12 lore raw

Re: [PATCH] files given on the command line are relative to $cwd

In reply to Junio C Hamano

builtin-commit.c static int message_is_empty(struct strbuf *sb, int start) · line 709

Quoted 10 lines · Junio C Hamano

static int parse_and_validate_options(int argc, const char *argv[],

builtin-commit.c +3 -1

-				      const char * const usage[])
+				      const char * const usage[],
+				      const char *prefix)
 {
 	int f = 0;
 
 	argc = parse_options(argc, argv, builtin_commit_options, usage, 0);
+	logfile = parse_options_fix_filename(prefix, logfile);

It breaks the "git commit -F -" case, no?

Olivier.

Junio C Hamano

2016-06-15 22:45 UTC #13 lore raw

Re: [PATCH] files given on the command line are relative to $cwd

Quoted 9 lines · Olivier Marin

Quoted 8 lines · Junio C Hamano
static int parse_and_validate_options(int argc, const char *argv[],
builtin-commit.c +2 -1
-				      const char * const usage[])
+				      const char * const usage[],
+				      const char *prefix)
 {
 	int f = 0;

Quoted 4 lines

builtin-commit.c static int message_is_empty(struct strbuf *sb, int start) · line 709
Quoted 2 lines
builtin-commit.c +1 -0
 	argc = parse_options(argc, argv, builtin_commit_options, usage, 0);
+	logfile = parse_options_fix_filename(prefix, logfile);
It breaks the "git commit -F -" case, no?

Does it? Ah, yeah, t7500 #15 does not go down to a subdirectory.

Junio C Hamano

2016-06-15 22:45 UTC #14 lore raw

Re: [PATCH] files given on the command line are relative to $cwd

Quoted 11 lines · Junio C Hamano

Quoted 9 lines · Olivier Marin
Quoted 8 lines · Junio C Hamano
static int parse_and_validate_options(int argc, const char *argv[],
(quoted diff) +2 -1
-				      const char * const usage[])
+				      const char * const usage[],
+				      const char *prefix)
 {
 	int f = 0;

Quoted 6 lines

Quoted 4 lines
Review
Quoted 2 lines
(quoted diff) +1 -0
 	argc = parse_options(argc, argv, builtin_commit_options, usage, 0);
+	logfile = parse_options_fix_filename(prefix, logfile);
It breaks the "git commit -F -" case, no?
Does it? Ah, yeah, t7500 #15 does not go down to a subdirectory.

Ok, this squashed in on top of the previous one should cover the case.

Thanks for saving me in time from a major embarrassment. I already tagged 1.5.6.5 with the botched one but haven't pushed it out, so I can safely rewind.

---

 parse-options.c   |    2 +-
 t/t7500-commit.sh |    8 ++++++++
 2 files changed, 9 insertions(+), 1 deletions(-)

parse-options.c +1 -1

index d771bf4..12c8822 100644
@@ -432,7 +432,7 @@ int parse_opt_approxidate_cb(const struct option *opt, const char *arg,
  */
 extern const char *parse_options_fix_filename(const char *prefix, const char *file)
 {
-	if (!file || !prefix || is_absolute_path(file))
+	if (!file || !prefix || is_absolute_path(file) || !strcmp("-", file))
 		return file;
 	return prefix_filename(prefix, strlen(prefix), file);
 }

t/t7500-commit.sh +8 -0

index 2ab791b..823256a 100755
@@ -159,4 +159,12 @@ test_expect_success 'commit message from file (2)' '
 	commit_msg_is "Log in sub directory"
 '
 
+test_expect_success 'commit message from stdin' '
+	(
+		cd subdir &&
+		echo "Log with foo word" | git commit --allow-empty -F -
+	) &&
+	commit_msg_is "Log with foo word"
+'
+
 test_done