From: Steffen Prohaska Date: Wed, 06 Feb 2008 20:35:11 GMT Subject: Re: [PATCH] [v9] safecrlf: Add mechanism to warn about irreversible crlf conversions Message-ID: In-Reply-To: <7vk5lhyir6.fsf@gitster.siamese.dyndns.org> On Feb 6, 2008, at 8:42 PM, Junio C Hamano wrote: > Steffen Prohaska writes: > >> This repaces v9 of the patch and should be applied. >> It contains modifications as suggested by Junio in >> <7vbq6u32mq.fsf@gitster.siamese.dyndns.org> >> >> Steffen >> >> -- >8 -- > > This v9 replaces itself, ah it repaces ;-) Yeah, I recognized this, too; but was to lazy to send another correction ;) > >> git diff, git add, and any other command that calls >> index_fd(..., write_object=1) also behave as requested by >> safecrlf. The user still has the chance to save her data before >> checkout or applying the diff. > > "git diff" writes by calling index_fd(..., write_object=1)??? > > I think warning in "git diff" is probably a good thing to do but > that is not because it writes. The warning would trigger only > when you are comparing what you have in the work tree with > something else, and if you get such a warning, it means what you > can potentially commit (i.e. what you have in the work tree) is > not "autocrlf" safe. You would get the same warning when you > later runn "git add" on such a file anyway, but it is nicer to > catch the potential problem earlier. > > I do not know if the above "why this command should behave this > way with respect to safecrlf" needs to be in the commit log > message. But I think that information should be in the > documentation to help the end users (the end users typically do > not have access to the commit log of git.git). > > The rest of the patch looked sane. Thanks. > > So I'd suggest the following change to the commit log message, > and then another patch at the end squashed into the > Documentation/ part. > > If you agree with these, you can just say "Ok, amend it like > so". Or you can say "Here is an even better replacement." Ok, amend it as you proposed but also squash in the following ... -- >8 -- diff --git a/builtin-apply.c b/builtin-apply.c index d0b3586..3b5618d 100644 --- a/builtin-apply.c +++ b/builtin-apply.c @@ -1430,7 +1430,7 @@ static int read_old_data(struct stat *st, const char *path, struct strbuf *buf) case S_IFREG: if (strbuf_read_file(buf, path, st->st_size) != st->st_size) return error("unable to open or read %s", path); - convert_to_git(path, buf->buf, buf->len, buf, safe_crlf); + convert_to_git(path, buf->buf, buf->len, buf, 0); return 0; default: return -1; -- >8 -- ... because ... [...] > + > +- "git apply" to update a text file with a patch does touch the files > + in the work tree, but the operation is about text files and CRLF > + conversion is about fixing the line ending inconsistencies, so the > + safety does not trigger; ... now after I read this I fully understand what you meant before. But my v9 patch does not match your description without the change to builtin-apply.c from above. In v9 the mechanism was still activated in "git apply". Steffen