[RESEND PATCH] fast-import: Cleanup mode setting.

"S_IFREG | mode" probably is only required for 0644 and 0755.

Signed-off-by: Felipe Contreras <redacted>
---
 fast-import.c |    7 ++++---
 1 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/fast-import.c b/fast-import.c
index a6bce66..f0e08ac 100644
--- a/fast-import.c
+++ b/fast-import.c

@@ -1872,12 +1872,13 @@ static void file_change_m(struct branch *b)
 	if (!p)
 		die("Corrupt mode: %s", command_buf.buf);
 	switch (mode) {
+	case 0644:
+	case 0755:
+		mode |= S_IFREG;
 	case S_IFREG | 0644:
 	case S_IFREG | 0755:
 	case S_IFLNK:
 	case S_IFGITLINK:
-	case 0644:
-	case 0755:
 		/* ok */
 		break;
 	default:

@@ -1944,7 +1945,7 @@ static void file_change_m(struct branch *b)
 			    typename(type), command_buf.buf);
 	}
 
-	tree_content_set(&b->branch_tree, p, sha1, S_IFREG | mode, NULL);
+	tree_content_set(&b->branch_tree, p, sha1, mode, NULL);
 }
 
 static void file_change_d(struct branch *b)

-- 
1.6.0.6.5.ga66c

Hi,

On Wed, 14 Jan 2009, Felipe Contreras wrote:

"S_IFREG | mode" probably is only required for 0644 and 0755.

Why should we want to have that patch?  IOW what does it fix, and what 
might it break?

Ciao,
Dscho

Felipe Contreras [off-list ref] wrote:

"S_IFREG | mode" probably is only required for 0644 and 0755.

Signed-off-by: Felipe Contreras <redacted>

Acked-by: Shawn O. Pearce <redacted>

-- 
Shawn.

Johannes Schindelin [off-list ref] wrote:

On Wed, 14 Jan 2009, Felipe Contreras wrote:

quoted

"S_IFREG | mode" probably is only required for 0644 and 0755.

Why should we want to have that patch?  IOW what does it fix, and what 
might it break?

It cleans up the code to make it more readable.

It makes no sense to be doing S_IFREG | S_IFLINK, which happens when
the input is for a symlink.  It doesn't break anything to do that |
operation, but it also looks damn odd when reading the function.

-- 
Shawn.

Hi,

On Tue, 13 Jan 2009, Shawn O. Pearce wrote:

Johannes Schindelin [off-list ref] wrote:

quoted

On Wed, 14 Jan 2009, Felipe Contreras wrote:

quoted

"S_IFREG | mode" probably is only required for 0644 and 0755.

Why should we want to have that patch?  IOW what does it fix, and what 
might it break?

It cleans up the code to make it more readable.

It makes no sense to be doing S_IFREG | S_IFLINK, which happens when
the input is for a symlink.  It doesn't break anything to do that |
operation, but it also looks damn odd when reading the function.

Imagining myself reading the commit message 6 months from now, in all 
likeliness I will have wished that those two paragraphs were in there.  
Verbatim.

Ciao,
Dscho

Johannes Schindelin [off-list ref] wrote:

On Tue, 13 Jan 2009, Shawn O. Pearce wrote:

quoted

It cleans up the code to make it more readable.

It makes no sense to be doing S_IFREG | S_IFLINK, which happens when
the input is for a symlink.  It doesn't break anything to do that |
operation, but it also looks damn odd when reading the function.

Imagining myself reading the commit message 6 months from now, in all 
likeliness I will have wished that those two paragraphs were in there.  
Verbatim.

Maybe Junio or Felipe can copy it into the message.

Or you can use a git note now to attach it to the commit Junio
hasn't yet created, so you can look it up in the future.  :-)

-- 
Shawn.

On Wed, Jan 14, 2009 at 4:29 AM, Shawn O. Pearce [off-list ref] wrote:

Johannes Schindelin [off-list ref] wrote:

quoted

On Tue, 13 Jan 2009, Shawn O. Pearce wrote:

quoted

It cleans up the code to make it more readable.

It makes no sense to be doing S_IFREG | S_IFLINK, which happens when
the input is for a symlink.  It doesn't break anything to do that |
operation, but it also looks damn odd when reading the function.

Imagining myself reading the commit message 6 months from now, in all
likeliness I will have wished that those two paragraphs were in there.
Verbatim.

Maybe Junio or Felipe can copy it into the message.

Or you can use a git note now to attach it to the commit Junio
hasn't yet created, so you can look it up in the future.  :-)

How about the attached patch?

"S_IFREG | mode" probably is only required for 0644 and 0755.

It doesn't make sense to do S_IFREG | S_IFLINK (0100000 | 0120000),
since no bits are changed.

-- 
Felipe Contreras

Felipe Contreras [off-list ref] wrote:

How about the attached patch?

"S_IFREG | mode" probably is only required for 0644 and 0755.

It doesn't make sense to do S_IFREG | S_IFLINK (0100000 | 0120000),
since no bits are changed.

Looks fine to me.

-- 
Shawn.