Re: [PATCH v2 2/2] git-merge-file: do not add LF at EOF while applying unrelated change
On Wed, Jul 02, 2014 at 04:08:28PM +0200, Johannes Schindelin wrote: >> What could be improved with them? > > Oh, I would name the files more appropriately, for example. That is, > instead of test1.txt I would call it mixed-endings.txt or lf-only.txt or > some such. > > And instead of the Latin version of Psalm 23, I would put lines into the > files that describe their own role in the test, i.e. > > unchanged > ends with a carriage return > ends with a line feed > unchanged > > or similar. > > Please keep in mind that this critique is most likely on my *own* work, > for all I know *I* introduced those files. I asked to have something in mind if I return to this. Thanks for the notes. -- Max -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/2] git-merge-file: do not add LF at EOF while applying unrelated change
Hi Max, On Wed, 2 Jul 2014, Max Kirillov wrote: > On Mon, Jun 30, 2014 at 04:55:10PM +0200, Johannes Schindelin wrote: > > I just wish the tests were a little easier to understand... > > What could be improved with them? Oh, I would name the files more appropriately, for example. That is, instead of test1.txt I would call it mixed-endings.txt or lf-only.txt or some such. And instead of the Latin version of Psalm 23, I would put lines into the files that describe their own role in the test, i.e. unchanged ends with a carriage return ends with a line feed unchanged or similar. Please keep in mind that this critique is most likely on my *own* work, for all I know *I* introduced those files. > By the way, for "\r\n" eol it did even worse, adding just "\n". And I > guess it still adds just "\n" for union merge. Should file merge > consider the core.eol? I think it should, and for the conflict markers > also, it looks ugly when whole file has "\r\n" but the conflict markers > have "\n". But then git-merge-file could not be used outside of > repository, I guess. Oh, why not? It could read the configuration if it's inside a working directory, and just read /etc/gitconfig and $HOME/.gitconfig when outside... > In general, I wish file merging (and diffing) were more tolerant of the > line endings in input. Because in windows environment, when people have > different core.autocrlf, it becomes quite frustrating to always get > conflicts and changes. Amen! Ciao, Dscho -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/2] git-merge-file: do not add LF at EOF while applying unrelated change
On Mon, Jun 30, 2014 at 04:55:10PM +0200, Johannes Schindelin wrote: > I just wish the tests were a little easier to understand... What could be improved with them? > Having said that, here is my ACK for the current revision > of the patch series Thanks. By the way, for "\r\n" eol it did even worse, adding just "\n". And I guess it still adds just "\n" for union merge. Should file merge consider the core.eol? I think it should, and for the conflict markers also, it looks ugly when whole file has "\r\n" but the conflict markers have "\n". But then git-merge-file could not be used outside of repository, I guess. In general, I wish file merging (and diffing) were more tolerant of the line endings in input. Because in windows environment, when people have different core.autocrlf, it becomes quite frustrating to always get conflicts and changes. -- Max -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/2] git-merge-file: do not add LF at EOF while applying unrelated change
Johannes Schindelin writes: > Hi Max, > > On Sun, 29 Jun 2014, Max Kirillov wrote: > >> diff --git a/xdiff/xmerge.c b/xdiff/xmerge.c >> index 9e13b25..625198e 100644 >> --- a/xdiff/xmerge.c >> +++ b/xdiff/xmerge.c >> @@ -245,11 +245,11 @@ static int xdl_fill_merge_buffer(xdfenv_t *xe1, const >> char *name1, >>dest ? dest + size : NULL); >> /* Postimage from side #1 */ >> if (m->mode & 1) >> -size += xdl_recs_copy(xe1, m->i1, m->chg1, 1, >> +size += xdl_recs_copy(xe1, m->i1, m->chg1, >> (m->mode & 2), >>dest ? dest + size : >> NULL); >> /* Postimage from side #2 */ >> if (m->mode & 2) >> -size += xdl_recs_copy(xe2, m->i2, m->chg2, 1, >> +size += xdl_recs_copy(xe2, m->i2, m->chg2, 0, >>dest ? dest + size : >> NULL); >> } else >> continue; > > Makes sense to me, especially with the nice explanation in the commit > message. > Having said that, here is my ACK for the current revision of the patch > series ... Thanks, both. Queued. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/2] git-merge-file: do not add LF at EOF while applying unrelated change
Hi Max, On Sun, 29 Jun 2014, Max Kirillov wrote: > diff --git a/xdiff/xmerge.c b/xdiff/xmerge.c > index 9e13b25..625198e 100644 > --- a/xdiff/xmerge.c > +++ b/xdiff/xmerge.c > @@ -245,11 +245,11 @@ static int xdl_fill_merge_buffer(xdfenv_t *xe1, const > char *name1, > dest ? dest + size : NULL); > /* Postimage from side #1 */ > if (m->mode & 1) > - size += xdl_recs_copy(xe1, m->i1, m->chg1, 1, > + size += xdl_recs_copy(xe1, m->i1, m->chg1, > (m->mode & 2), > dest ? dest + size : > NULL); > /* Postimage from side #2 */ > if (m->mode & 2) > - size += xdl_recs_copy(xe2, m->i2, m->chg2, 1, > + size += xdl_recs_copy(xe2, m->i2, m->chg2, 0, > dest ? dest + size : > NULL); > } else > continue; Makes sense to me, especially with the nice explanation in the commit message. I just wish the tests were a little easier to understand... It is probably my fault because I insisted on using a text that has *nothing* to do with Git. These days, I would probably have used better file names and would have used file contents that reflect the purpose in the tests (i.e. a line saying "This line ends with a carriage return" or some such). Having said that, here is my ACK for the current revision of the patch series (because I know how much work it would be to fix the issue I described above, and it is an *entirely different* issue from the one you fixed with this series, too). Ciao, Dscho -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
