{"thread":{"id":"40581","subject":"[PATCH] Add ls-files --eol-staged, --eol-worktree","startedAt":"2015-10-17T20:18:06Z","lastAt":"2015-10-19T06:32:14Z","messageCount":8,"participants":["Torsten Bögershausen","Eric Sunshine","Philip Oakley","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"271859","messageId":"D68CC6D0-3FD5-4423-A9E2-905DF18E893F@web.de","threadId":"40581","inReplyTo":null,"subject":"[PATCH] Add ls-files --eol-staged, --eol-worktree","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2015-10-17T20:18:06Z","receivedAt":"2015-10-17T20:18:06Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"Make it possible to show the line endings of files.\nFiles which are staged and/or files in the working tree:\n\ngit ls-files --eol-staged\ngit ls-files --eol-worktree\n\nBoth will show an output like this:\n\nempty    empty_file\nbin      binary_file_or_with_cr_handled_as_binary\ntxt-crlf text_file_with_crlf\ntxt-mix  text_file_with_crlf_and_lf\ntxt-lf   text_file_with_lf\ntxt      text_file_with_no_eol_at_all\n\nImplementation details:\nMake struct text_stat, is_binary() and gather_stats() from convert.c\npublic, add a new function get_convert_stats_ascii() and use it\nin and use them in ls-files.\ngit ls-files --eol-staged will give a line like this:\n\nSigned-off-by: Torsten Bögershausen <tboegi@web.de>\n---\nThis needs to go on top of tb/t0027-crlf\n\nbuiltin/ls-files.c | 21 +++++++++++++++++++++\nconvert.c          | 51 +++++++++++++++++++++++++++++++++++++++++----------\nconvert.h          | 14 ++++++++++++++\n3 files changed, 76 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/ls-files.c b/builtin/ls-files.c\nindex b6a7cb0..c989e94 100644\n--- a/builtin/ls-files.c\n+++ b/builtin/ls-files.c\n@@ -27,6 +27,8 @@ static int show_killed;\nstatic int show_valid_bit;\nstatic int line_terminator = '\\n';\nstatic int debug_mode;\n+static int show_eol_staged;\n+static int show_eol_wt;\n\nstatic const char *prefix;\nstatic int max_prefix_len;\n@@ -68,6 +70,11 @@ static void show_dir_entry(const char *tag, struct dir_entry *ent)\n\t\treturn;\n\n\tfputs(tag, stdout);\n+\tif (show_eol_wt) {\n+\t\tprintf(\"%s \", get_convert_stats_ascii(ent->name,\n+\t\t\t\t\t\t\t\t\t\t\t\t\t\t\t\t\t\t\t\t\tGET_CONVERT_STATS_ASCII_WT, 0));\n+\t}\n+\n\twrite_name(ent->name);\n}\n\n@@ -170,6 +177,14 @@ static void show_ce_entry(const char *tag, const struct cache_entry *ce)\n\t\t       find_unique_abbrev(ce->sha1,abbrev),\n\t\t       ce_stage(ce));\n\t}\n+\tif (show_eol_staged) {\n+\t\tprintf(\"%s \",\n+\t\t\t\t\t get_convert_stats_ascii(ce->name, GET_CONVERT_STATS_ASCII_BLOB, 0));\n+\t}\n+\tif (show_eol_wt) {\n+\t\tprintf(\"%s \", get_convert_stats_ascii(ce->name,GET_CONVERT_STATS_ASCII_WT,\n+\t\t\t\t\t\t\t\t\t\t\t\t\t\t\t\t\t\t\t\t\tce->ce_stat_data.sd_size));\n+\t}\n\twrite_name(ce->name);\n\tif (debug_mode) {\n\t\tconst struct stat_data *sd = &ce->ce_stat_data;\n@@ -206,6 +221,10 @@ static void show_ru_info(void)\n\t\t\tprintf(\"%s%06o %s %d\\t\", tag_resolve_undo, ui->mode[i],\n\t\t\t       find_unique_abbrev(ui->sha1[i], abbrev),\n\t\t\t       i + 1);\n+\t\t\tif (show_eol_wt) {\n+\t\t\t\tprintf(\"%s \",\n+\t\t\t\t\t\t\t get_convert_stats_ascii(path, GET_CONVERT_STATS_ASCII_WT, 0));\n+\t\t\t}\n\t\t\twrite_name(path);\n\t\t}\n\t}\n@@ -433,6 +452,8 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)\n\t\tOPT_BIT(0, \"directory\", &dir.flags,\n\t\t\tN_(\"show 'other' directories' names only\"),\n\t\t\tDIR_SHOW_OTHER_DIRECTORIES),\n+\t\tOPT_BOOL(0, \"eol-staged\", &show_eol_staged, N_(\"show line endings of the staged file\")),\n+\t\tOPT_BOOL(0, \"eol-worktree\", &show_eol_wt, N_(\"show line endings of the file in work tree\")),\n\t\tOPT_NEGBIT(0, \"empty-directory\", &dir.flags,\n\t\t\tN_(\"don't show empty directories\"),\n\t\t\tDIR_HIDE_EMPTY_DIRECTORIES),\ndiff --git a/convert.c b/convert.c\nindex 814e814..a1c24cd 100644\n--- a/convert.c\n+++ b/convert.c\n@@ -22,15 +22,7 @@ enum crlf_action {\n\tCRLF_AUTO\n};\n\n-struct text_stat {\n-\t/* NUL, CR, LF and CRLF counts */\n-\tunsigned nul, cr, lf, crlf;\n-\n-\t/* These are just approximations! */\n-\tunsigned printable, nonprintable;\n-};\n-\n-static void gather_stats(const char *buf, unsigned long size, struct text_stat *stats)\n+void gather_stats(const char *buf, unsigned long size, struct text_stat *stats)\n{\n\tunsigned long i;\n\n@@ -76,7 +68,7 @@ static void gather_stats(const char *buf, unsigned long size, struct text_stat *\n/*\n * The same heuristics as diff.c::mmfile_is_binary()\n */\n-static int is_binary(unsigned long size, struct text_stat *stats)\n+int is_binary(unsigned long size, struct text_stat *stats)\n{\n\n\tif (stats->nul)\n@@ -95,6 +87,45 @@ static int is_binary(unsigned long size, struct text_stat *stats)\n\treturn 0;\n}\n\n+\n+const char *gather_stats_ascii(const char *data, unsigned long size)\n+{\n+\tstruct text_stat stats;\n+\tif (!data || !size)\n+\t\treturn(\"empty   \");\n+\tgather_stats(data, size, &stats);\n+\tif (is_binary(size, &stats))\n+\t\treturn(\"bin     \");\n+\telse if (stats.cr != stats.crlf)\n+\t\treturn(\"bin     \");\n+\telse if (stats.crlf && (stats.crlf == stats.lf))\n+\t\treturn(\"txt-crlf\");\n+\telse if (stats.crlf && stats.lf)\n+\t\treturn(\"txt-mix \");\n+\telse if (stats.lf)\n+\t\treturn(\"txt-lf  \");\n+\telse\n+\t\treturn(\"txt     \");\n+}\n+\n+const char *get_convert_stats_ascii(const char *path, int flags, size_t hint)\n+{\n+\tconst char *ret = \"\";\n+\tif (flags & GET_CONVERT_STATS_ASCII_BLOB) {\n+\t\tunsigned long sz;\n+\t\tvoid *data = read_blob_data_from_cache(path, &sz);\n+\t\tret = gather_stats_ascii(data, sz);\n+\t\tif (data)\n+\t\t\tfree(data);\n+\t} else if (flags & GET_CONVERT_STATS_ASCII_WT){\n+\t\tstruct strbuf sb = STRBUF_INIT;\n+\t\tstrbuf_read_file(&sb, path, hint);\n+\t\tret = gather_stats_ascii(sb.buf, sb.len);\n+\t\tstrbuf_release(&sb);\n+\t}\n+\treturn ret;\n+}\n+\nstatic enum eol output_eol(enum crlf_action crlf_action)\n{\n\tswitch (crlf_action) {\ndiff --git a/convert.h b/convert.h\nindex d9d853c..566cf0e 100644\n--- a/convert.h\n+++ b/convert.h\n@@ -31,6 +31,20 @@ enum eol {\n#endif\n};\n\n+struct text_stat {\n+\t/* NUL, CR, LF and CRLF counts */\n+\tunsigned nul, cr, lf, crlf;\n+\n+\t/* These are just approximations! */\n+\tunsigned printable, nonprintable;\n+};\n+void gather_stats(const char *buf, unsigned long size, struct text_stat *stats);\n+int is_binary(unsigned long size, struct text_stat *stats);\n+const char *gather_stats_ascii(const char *buf, unsigned long size);\n+#define GET_CONVERT_STATS_ASCII_BLOB           (1<<0)\n+#define GET_CONVERT_STATS_ASCII_WT             (1<<1)\n+const char *get_convert_stats_ascii(const char *path, int isBlob, size_t hint);\n+\nextern enum eol core_eol;\n\n/* returns 1 if *dst was used */\n-- \n2.6.1.443.g36d7748\n"},{"id":"271863","messageId":"CAPig+cQAmp8QPoO8GDJEUU6T2uSZo465Q64-BcQ1o_DJNDAuAA@mail.gmail.com","threadId":"40581","inReplyTo":"D68CC6D0-3FD5-4423-A9E2-905DF18E893F@web.de","subject":"Re: [PATCH] Add ls-files --eol-staged, --eol-worktree","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-10-18T01:10:13Z","receivedAt":"2015-10-18T01:10:13Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sat, Oct 17, 2015 at 4:18 PM, Torsten Bögershausen <tboegi@web.de> wrote:\n> Make it possible to show the line endings of files.\n> Files which are staged and/or files in the working tree:\n>\n> git ls-files --eol-staged\n> git ls-files --eol-worktree\n>\n> Both will show an output like this:\n>\n> empty    empty_file\n> bin      binary_file_or_with_cr_handled_as_binary\n> txt-crlf text_file_with_crlf\n> txt-mix  text_file_with_crlf_and_lf\n> txt-lf   text_file_with_lf\n> txt      text_file_with_no_eol_at_all\n>\n> Implementation details:\n> Make struct text_stat, is_binary() and gather_stats() from convert.c\n> public, add a new function get_convert_stats_ascii() and use it\n> in and use them in ls-files.\n\ns/and use it in and use them in/and use them in/\n\n> git ls-files --eol-staged will give a line like this:\n\n\"... line like this:\" what?\n\n> Signed-off-by: Torsten Bögershausen <tboegi@web.de>\n> ---\n> --- a/builtin/ls-files.c\n> +++ b/builtin/ls-files.c\n> @@ -68,6 +70,11 @@ static void show_dir_entry(const char *tag, struct dir_entry *ent)\n>                 return;\n>\n>         fputs(tag, stdout);\n> +       if (show_eol_wt) {\n> +               printf(\"%s \", get_convert_stats_ascii(ent->name,\n> +                                                                                                                                                                       GET_CONVERT_STATS_ASCII_WT, 0));\n\nWhitespace-damaged patch?\n\n> +       }\n\nStyle: unnecessary braces\n\n> +\n>         write_name(ent->name);\n> }\n>\n> @@ -170,6 +177,14 @@ static void show_ce_entry(const char *tag, const struct cache_entry *ce)\n>                        find_unique_abbrev(ce->sha1,abbrev),\n>                        ce_stage(ce));\n>         }\n> +       if (show_eol_staged) {\n> +               printf(\"%s \",\n> +                                        get_convert_stats_ascii(ce->name, GET_CONVERT_STATS_ASCII_BLOB, 0));\n> +       }\n> +       if (show_eol_wt) {\n> +               printf(\"%s \", get_convert_stats_ascii(ce->name,GET_CONVERT_STATS_ASCII_WT,\n> +                                                                                                                                                                       ce->ce_stat_data.sd_size));\n> +       }\n\nStyle: unnecessary braces (both cases)\n\n>         write_name(ce->name);\n>         if (debug_mode) {\n>                 const struct stat_data *sd = &ce->ce_stat_data;\n> @@ -206,6 +221,10 @@ static void show_ru_info(void)\n>                         printf(\"%s%06o %s %d\\t\", tag_resolve_undo, ui->mode[i],\n>                                find_unique_abbrev(ui->sha1[i], abbrev),\n>                                i + 1);\n> +                       if (show_eol_wt) {\n> +                               printf(\"%s \",\n> +                                                        get_convert_stats_ascii(path, GET_CONVERT_STATS_ASCII_WT, 0));\n> +                       }\n\nStyle: unnecessary braces\n\n>                         write_name(path);\n>                 }\n>         }\n> @@ -433,6 +452,8 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)\n>                 OPT_BIT(0, \"directory\", &dir.flags,\n>                         N_(\"show 'other' directories' names only\"),\n>                         DIR_SHOW_OTHER_DIRECTORIES),\n> +               OPT_BOOL(0, \"eol-staged\", &show_eol_staged, N_(\"show line endings of the staged file\")),\n> +               OPT_BOOL(0, \"eol-worktree\", &show_eol_wt, N_(\"show line endings of the file in work tree\")),\n>                 OPT_NEGBIT(0, \"empty-directory\", &dir.flags,\n>                         N_(\"don't show empty directories\"),\n>                         DIR_HIDE_EMPTY_DIRECTORIES),\n> --- a/convert.c\n> +++ b/convert.c\n> @@ -95,6 +87,45 @@ static int is_binary(unsigned long size, struct text_stat *stats)\n>         return 0;\n> }\n>\n> +\n\nStyle: unnecessary blank line\n\n> +const char *gather_stats_ascii(const char *data, unsigned long size)\n\nIs this name too generic? This implementation is specialized for\nsummarizing EOL state, but it is conceivable that there may be other\nmeaningful textual representations of struct text_stat, as well, so\nperhaps this name ought to better reflect the EOL-centric textual\nrepresentation of this implementation.\n\n> +{\n> +       struct text_stat stats;\n> +       if (!data || !size)\n> +               return(\"empty   \");\n\nWould it make sense to distinguish between an empty file/blob and one\nwhich was not found?\n\n    if (!data)\n        return \"missing\";\n    if (!size)\n        return \"empy\";\n\n> +       gather_stats(data, size, &stats);\n> +       if (is_binary(size, &stats))\n> +               return(\"bin     \");\n\nStyle: unnecessary parentheses on 'return'\n\n> +       else if (stats.cr != stats.crlf)\n\nStyle (nit): unnecessary 'else'\n\n> +               return(\"bin     \");\n> +       else if (stats.crlf && (stats.crlf == stats.lf))\n> +               return(\"txt-crlf\");\n> +       else if (stats.crlf && stats.lf)\n> +               return(\"txt-mix \");\n> +       else if (stats.lf)\n> +               return(\"txt-lf  \");\n> +       else\n> +               return(\"txt     \");\n> +}\n\nIs it a good idea for this otherwise general purpose function to be\nconflating presentation with EOL classification? I would have expected\nthis function to return the raw classification string without trailing\nwhitespace (\"empty\", \"bin\", \"txt-mix\", etc.), and leave it up to the\ncaller to pad the string if alignment is desired.\n\n> +const char *get_convert_stats_ascii(const char *path, int flags, size_t hint)\n\nIs 'flags' an appropriate name? Presently, it's more of a \"mode\" than\na flag. If it is a flag, then you typically would use an unsigned\ntype.\n\nIf you can't foresee any additional \"flags\", and this really is a\n\"mode\", then it might be clearer to split this into two functions --\none for blobs and one for files -- and callers can invoke whichever is\nappropriate.\n\n> +{\n> +       const char *ret = \"\";\n\nUnnecessary initialization since all  branches assign 'ret' unconditionally.\n\n> +       if (flags & GET_CONVERT_STATS_ASCII_BLOB) {\n> +               unsigned long sz;\n> +               void *data = read_blob_data_from_cache(path, &sz);\n> +               ret = gather_stats_ascii(data, sz);\n\ngather_stats_ascii() deals gracefully with data== NULL, so it can\nsafely be outside the 'if (data)' conditional. Okay.\n\n> +               if (data)\n> +                       free(data);\n\nPOSIX says that free(NULL) is safe, so the conditional is unnecessary.\n\n> +       } else if (flags & GET_CONVERT_STATS_ASCII_WT){\n\nStyle: space before {\n\n> +               struct strbuf sb = STRBUF_INIT;\n> +               strbuf_read_file(&sb, path, hint);\n\nIf gather_stats_ascii() should distinguish error and empty-file cases,\nthen checking the return value of strbuf_read_file() would be\nappropriate.\n\n> +               ret = gather_stats_ascii(sb.buf, sb.len);\n> +               strbuf_release(&sb);\n> +       }\n> +       return ret;\n> +}\n> +\n> --- a/convert.h\n> +++ b/convert.h\n> @@ -31,6 +31,20 @@ enum eol {\n> +struct text_stat {\n> +       /* NUL, CR, LF and CRLF counts */\n> +       unsigned nul, cr, lf, crlf;\n> +\n> +       /* These are just approximations! */\n> +       unsigned printable, nonprintable;\n> +};\n> +void gather_stats(const char *buf, unsigned long size, struct text_stat *stats);\n> +int is_binary(unsigned long size, struct text_stat *stats);\n> +const char *gather_stats_ascii(const char *buf, unsigned long size);\n> +#define GET_CONVERT_STATS_ASCII_BLOB           (1<<0)\n> +#define GET_CONVERT_STATS_ASCII_WT             (1<<1)\n> +const char *get_convert_stats_ascii(const char *path, int isBlob, size_t hint);\n\ns/isBlob/flags/ or s/isBlob/mode/ or something.\n\nHaving read the source code, I know what 'hint' is for, but its\npurpose is not obvious when just reading this prototype. Also, do you\nexpect 'hint' to prove worthwhile or might it be a case of premature\noptimization and interface complexity? (Genuine question.)\n"},{"id":"271866","messageId":"3D2D9DFC85644BD1A6A39E819D90ACAA@PhilipOakley","threadId":"40581","inReplyTo":"D68CC6D0-3FD5-4423-A9E2-905DF18E893F@web.de","subject":"Re: [PATCH] Add ls-files --eol-staged, --eol-worktree","fromName":"Philip Oakley","fromEmail":"philipoakley@iee.org","sentAt":"2015-10-18T10:03:37Z","receivedAt":"2015-10-18T10:03:37Z","isPatch":true,"sender":{"key":"philipoakley@iee.email","avatar":"https://avatars.githubusercontent.com/u/914343?v=4"},"body":"From: \"Torsten Bögershausen\" <tboegi@web.de>\n> Make it possible to show the line endings of files.\n> Files which are staged and/or files in the working tree:\n>\n> git ls-files --eol-staged\n> git ls-files --eol-worktree\n>\n> Both will show an output like this:\n>\n> empty    empty_file\n> bin      binary_file_or_with_cr_handled_as_binary\n> txt-crlf text_file_with_crlf\n> txt-mix  text_file_with_crlf_and_lf\n> txt-lf   text_file_with_lf\n> txt      text_file_with_no_eol_at_all\n\nI think that this last generic 'txt' should be explicit, e.g. 'txt-no-eols', \nso that the categories are explicit and mutually exclusive.\n\nAlso, does this need a documentation update for the options, and can the \ndistinction between txt and bin be documented /referenced.\n\n>\n> Implementation details:\n> Make struct text_stat, is_binary() and gather_stats() from convert.c\n> public, add a new function get_convert_stats_ascii() and use it\n> in and use them in ls-files.\n> git ls-files --eol-staged will give a line like this:\n>\n> Signed-off-by: Torsten Bögershausen <tboegi@web.de>\n> ---\n> This needs to go on top of tb/t0027-crlf\n>\n> builtin/ls-files.c | 21 +++++++++++++++++++++\n> convert.c          | 51 \n> +++++++++++++++++++++++++++++++++++++++++----------\n> convert.h          | 14 ++++++++++++++\n> 3 files changed, 76 insertions(+), 10 deletions(-)\n>\n> diff --git a/builtin/ls-files.c b/builtin/ls-files.c\n> index b6a7cb0..c989e94 100644\n> --- a/builtin/ls-files.c\n> +++ b/builtin/ls-files.c\n> @@ -27,6 +27,8 @@ static int show_killed;\n> static int show_valid_bit;\n> static int line_terminator = '\\n';\n> static int debug_mode;\n> +static int show_eol_staged;\n> +static int show_eol_wt;\n>\n> static const char *prefix;\n> static int max_prefix_len;\n> @@ -68,6 +70,11 @@ static void show_dir_entry(const char *tag, struct \n> dir_entry *ent)\n> return;\n>\n> fputs(tag, stdout);\n> + if (show_eol_wt) {\n> + printf(\"%s \", get_convert_stats_ascii(ent->name,\n> + GET_CONVERT_STATS_ASCII_WT, 0));\n> + }\n> +\n> write_name(ent->name);\n> }\n>\n> @@ -170,6 +177,14 @@ static void show_ce_entry(const char *tag, const \n> struct cache_entry *ce)\n>        find_unique_abbrev(ce->sha1,abbrev),\n>        ce_stage(ce));\n> }\n> + if (show_eol_staged) {\n> + printf(\"%s \",\n> + get_convert_stats_ascii(ce->name, GET_CONVERT_STATS_ASCII_BLOB, 0));\n> + }\n> + if (show_eol_wt) {\n> + printf(\"%s \", \n> get_convert_stats_ascii(ce->name,GET_CONVERT_STATS_ASCII_WT,\n> + ce->ce_stat_data.sd_size));\n> + }\n> write_name(ce->name);\n> if (debug_mode) {\n> const struct stat_data *sd = &ce->ce_stat_data;\n> @@ -206,6 +221,10 @@ static void show_ru_info(void)\n> printf(\"%s%06o %s %d\\t\", tag_resolve_undo, ui->mode[i],\n>        find_unique_abbrev(ui->sha1[i], abbrev),\n>        i + 1);\n> + if (show_eol_wt) {\n> + printf(\"%s \",\n> + get_convert_stats_ascii(path, GET_CONVERT_STATS_ASCII_WT, 0));\n> + }\n> write_name(path);\n> }\n> }\n> @@ -433,6 +452,8 @@ int cmd_ls_files(int argc, const char **argv, const \n> char *cmd_prefix)\n> OPT_BIT(0, \"directory\", &dir.flags,\n> N_(\"show 'other' directories' names only\"),\n> DIR_SHOW_OTHER_DIRECTORIES),\n> + OPT_BOOL(0, \"eol-staged\", &show_eol_staged, N_(\"show line endings of the \n> staged file\")),\n> + OPT_BOOL(0, \"eol-worktree\", &show_eol_wt, N_(\"show line endings of the \n> file in work tree\")),\n> OPT_NEGBIT(0, \"empty-directory\", &dir.flags,\n> N_(\"don't show empty directories\"),\n> DIR_HIDE_EMPTY_DIRECTORIES),\n> diff --git a/convert.c b/convert.c\n> index 814e814..a1c24cd 100644\n> --- a/convert.c\n> +++ b/convert.c\n> @@ -22,15 +22,7 @@ enum crlf_action {\n> CRLF_AUTO\n> };\n>\n> -struct text_stat {\n> - /* NUL, CR, LF and CRLF counts */\n> - unsigned nul, cr, lf, crlf;\n> -\n> - /* These are just approximations! */\n> - unsigned printable, nonprintable;\n> -};\n> -\n> -static void gather_stats(const char *buf, unsigned long size, struct \n> text_stat *stats)\n> +void gather_stats(const char *buf, unsigned long size, struct text_stat \n> *stats)\n> {\n> unsigned long i;\n>\n> @@ -76,7 +68,7 @@ static void gather_stats(const char *buf, unsigned long \n> size, struct text_stat *\n> /*\n> * The same heuristics as diff.c::mmfile_is_binary()\n> */\n> -static int is_binary(unsigned long size, struct text_stat *stats)\n> +int is_binary(unsigned long size, struct text_stat *stats)\n> {\n>\n> if (stats->nul)\n> @@ -95,6 +87,45 @@ static int is_binary(unsigned long size, struct \n> text_stat *stats)\n> return 0;\n> }\n>\n> +\n> +const char *gather_stats_ascii(const char *data, unsigned long size)\n> +{\n> + struct text_stat stats;\n> + if (!data || !size)\n> + return(\"empty   \");\n> + gather_stats(data, size, &stats);\n> + if (is_binary(size, &stats))\n> + return(\"bin     \");\n> + else if (stats.cr != stats.crlf)\n> + return(\"bin     \");\n> + else if (stats.crlf && (stats.crlf == stats.lf))\n> + return(\"txt-crlf\");\n> + else if (stats.crlf && stats.lf)\n> + return(\"txt-mix \");\n> + else if (stats.lf)\n> + return(\"txt-lf  \");\n> + else\n> + return(\"txt     \");\n> +}\n> +\n> +const char *get_convert_stats_ascii(const char *path, int flags, size_t \n> hint)\n> +{\n> + const char *ret = \"\";\n> + if (flags & GET_CONVERT_STATS_ASCII_BLOB) {\n> + unsigned long sz;\n> + void *data = read_blob_data_from_cache(path, &sz);\n> + ret = gather_stats_ascii(data, sz);\n> + if (data)\n> + free(data);\n> + } else if (flags & GET_CONVERT_STATS_ASCII_WT){\n> + struct strbuf sb = STRBUF_INIT;\n> + strbuf_read_file(&sb, path, hint);\n> + ret = gather_stats_ascii(sb.buf, sb.len);\n> + strbuf_release(&sb);\n> + }\n> + return ret;\n> +}\n> +\n> static enum eol output_eol(enum crlf_action crlf_action)\n> {\n> switch (crlf_action) {\n> diff --git a/convert.h b/convert.h\n> index d9d853c..566cf0e 100644\n> --- a/convert.h\n> +++ b/convert.h\n> @@ -31,6 +31,20 @@ enum eol {\n> #endif\n> };\n>\n> +struct text_stat {\n> + /* NUL, CR, LF and CRLF counts */\n> + unsigned nul, cr, lf, crlf;\n> +\n> + /* These are just approximations! */\n> + unsigned printable, nonprintable;\n> +};\n> +void gather_stats(const char *buf, unsigned long size, struct text_stat \n> *stats);\n> +int is_binary(unsigned long size, struct text_stat *stats);\n> +const char *gather_stats_ascii(const char *buf, unsigned long size);\n> +#define GET_CONVERT_STATS_ASCII_BLOB           (1<<0)\n> +#define GET_CONVERT_STATS_ASCII_WT             (1<<1)\n> +const char *get_convert_stats_ascii(const char *path, int isBlob, size_t \n> hint);\n> +\n> extern enum eol core_eol;\n>\n> /* returns 1 if *dst was used */\n> -- \n> 2.6.1.443.g36d7748\n>\n> --\nPhilip \n"},{"id":"271879","messageId":"xmqqpp0caxlw.fsf@gitster.mtv.corp.google.com","threadId":"40581","inReplyTo":"D68CC6D0-3FD5-4423-A9E2-905DF18E893F@web.de","subject":"Re: [PATCH] Add ls-files --eol-staged, --eol-worktree","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-10-18T17:35:23Z","receivedAt":"2015-10-18T17:35:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Torsten Bögershausen <tboegi@web.de> writes:\n\n> Make it possible to show the line endings of files.\n> Files which are staged and/or files in the working tree:\n>\n> git ls-files --eol-staged\n> git ls-files --eol-worktree\n>\n> Both will show an output like this:\n>\n> empty    empty_file\n> bin      binary_file_or_with_cr_handled_as_binary\n> txt-crlf text_file_with_crlf\n> txt-mix  text_file_with_crlf_and_lf\n> txt-lf   text_file_with_lf\n> txt      text_file_with_no_eol_at_all\n\nThis _may_ be readable by who implemented it, but I cannot tell what\nyou are trying to say above; \"like this\" does not help me at all\ndeciphering.\n\nDo I get the above after doing this?\n\n    $ >empty\n    $ >empty2\n    $ git ls-files --eol-worktree\n    empty empty_file\n    empty2 empty_file\n\nor do you mean this?\n\n    $ >empty_file\n    $ >empty_file2\n    $ git ls-files --eol-worktree\n    empty empty_file empty_file2\n\nor do you produce output one path at a time?\n\n    $ >empty_file\n    $ >empty_file2\n    $ git ls-files --eol-worktree\n    empty empty_file\n    empty empty_file2\n\n> txt      text_file_with_no_eol_at_all\n\nDoes it help to have this category at all?  This gives the user an\nindication that this file consists on a single incomplete line and\ndifferentiates it from a file with the same single line with LF or\nCR/LF line ending.  What happens when I prepend to these three files\nanother copy of the same line with LF or CR/LF line ending?  You get\n6 variations:\n\n 1. line LF line\n 2. line LF line LF\n 3. line LF line CRLF\n 4. line CRLF line\n 5. line CRLF line LF\n 6. line CRLF line CRLF\n    \nIf you say 1 and 2 are with LF, 4 and 6 are with CRLF, eveyrything\nelse is mixed, then you are losing the distinction between 1 and 2\n(and 4 and 6) that you made when the files were a single liner (with\nor without the incomplete line ending).  Is that desirable?\n\nI wonder if it would be easier for the scripts that process the\noutput from this command to handle if the report said what\ncombination of _three_ possible line-ending is used.  i.e. does the\nfile contain a line that ends with LF? does the file contain a line\nthat ends with CRLF? does the file contain a line with missing EOL?\n\n\n> Implementation details:\n> Make struct text_stat, is_binary() and gather_stats() from convert.c\n> public, add a new function get_convert_stats_ascii() and use it\n> in and use them in ls-files.\n> git ls-files --eol-staged will give a line like this:\n>\n> Signed-off-by: Torsten Bögershausen <tboegi@web.de>\n> ---\n\nThis is even worse than the \"like this\" above ;-)\n"},{"id":"271882","messageId":"xmqq8u70auz2.fsf@gitster.mtv.corp.google.com","threadId":"40581","inReplyTo":"xmqqpp0caxlw.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] Add ls-files --eol-staged, --eol-worktree","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-10-18T18:32:17Z","receivedAt":"2015-10-18T18:32:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> If you say 1 and 2 are with LF, 4 and 6 are with CRLF, eveyrything\n> else is mixed, then you are losing the distinction between 1 and 2\n> (and 4 and 6) that you made when the files were a single liner (with\n> or without the incomplete line ending).  Is that desirable?\n\nContinuing this line of thought further.  My answer is \"it may not\nbe desirable, but it is sufficient to decide what line ending to use\nwhen I am adding a new line to the existing contents\".  That is, to\na file with a single incomplete line, I can add my new line with\neither LF or CRLF and the resulting one will become LF (or CRLF)\nthat ends in an incomplete line.  To a file whose lines consistently\nuse LF, I can only add my new line with LF, whether the original ends\nin an incomplete line.\n\nSo from that point of view,...\n\n> I wonder if it would be easier for the scripts that process the\n> output from this command to handle if the report said what\n> combination of _three_ possible line-ending is used.  i.e. does the\n> file contain a line that ends with LF? does the file contain a line\n> that ends with CRLF? does the file contain a line with missing EOL?\n\n... instead of saying there are three possible line-endings, we can\nstick to two, i.e. \"is it text or binary\" followed by \"among two\npossible endings, which ones are used\", i.e.\n\n    text\n    text-lf\n    text-crlf\n    text-lf-crlf\n\nwhich matches the last four lines of what you had in the \"like this\"\nexample (and I prefer \"mixed\" over \"lf-crlf\").\n\nSo I am OK with the categorization after all with respect to the\npossible incomplete line at the end.  But if that is what the\nfeature is designed for, the documentation must say it very\nclearly, i.e. \"this is to allow you decide what line ending to use\nwhen you add a new line to the existing contents\" or something.\n\nAnd viewed from that angle, there is no reason to special-case an\nempty line.  Knowing \"binary\" is important because you want to be\nable to say \"whether LF or CRLF, you do *not* want to add your new\nline to this binary file.\"; \"empty\" is just like \"text\"---you can\nuse either and get a coherent result.\n\nSo I'd suggest sticking to these classification tokens:\n\n\tbinary\n        text\n        crlf\n        mixed\n        lf\n\nThe \"adding my line at the beginning of the file\" script can do\nsomething like this using them (here I am simplifying by making your\nfeature available to \"git get-eol\" command that takes a single path\nand does your computation):\n\n\tcase \"$(git get-eol file)\" in\n\ttext | lf)\n        \tprintf \"%s\\n\" \"$mine\"\n                ;;\n\tcrlf)\n        \tprintf \"%s\\r\\n\" \"$mine\"\n                ;;\n        *) # that is 'binary' or 'mixed'\n\t\tdie \"do not muck with the contents of file\"\n                ;;\n\tesac\n\tcat file\n\nAlso this points at another direction of using the three independent\nline ending conventions I suggested earlier.  What you want to\nappend your lines?  You would want to know if the file ends with an\nincomplete line, so you would rather want to be told with a set of\ncategories like this instead:\n\n\tbinary\n\tincomplete\n        incomplete,crlf\n        incomplete,mixed\n        incomplete,lf\n        crlf\n        mixed\n        lf\n\nNote that an empty file will get an empty string as the grouping\nabove, as it does not have any line ending (i.e. no crlf/mixed/lf),\ndoes not end with an incomplete line and is not a binary file.\n\nAnd the using script would become:\n\n\texisting=$(git get-eol file)\n\teol='\\n'\n\tcase \",$existing\" in\n\t,binary | *,mixed)\n\t\tdie \"do not muck with the contents of file\"\n\t\t;;\n\t,*crlf)\n\t\teol='\\r\\n'\n                ;;\n\tesac\n        cat file\n        case \"$existing,\" in\n        incomplete,*)\n\t\tprintf \"$eol\"\n                ;;\n\tesac\n        printf \"%s$eol\" \"$mine\"\n"},{"id":"271883","messageId":"xmqq4mhoatna.fsf@gitster.mtv.corp.google.com","threadId":"40581","inReplyTo":"D68CC6D0-3FD5-4423-A9E2-905DF18E893F@web.de","subject":"Re: [PATCH] Add ls-files --eol-staged, --eol-worktree","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-10-18T19:00:57Z","receivedAt":"2015-10-18T19:00:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Torsten Bögershausen <tboegi@web.de> writes:\n\n> Make it possible to show the line endings of files.\n> Files which are staged and/or files in the working tree:\n>\n> git ls-files --eol-staged\n> git ls-files --eol-worktree\n\nTwo unrelated (to the issues raised in other review responses)\nissues in the UI:\n\n - While I can see how the new feature would be useful, I am not\n   convinced that it is a good idea to add it to ls-files.  Does the\n   option work well with other existing options like -s, -t, etc?\n   Does it make sense to combine it with other options like -m, -d,\n   etc?  I have this suspicion that \"check-attr\", \"check-ignore\",\n   etc.  may give a better model that fits this feature better,\n   i.e. \"git check-eol\".\n\n - When you have an operation that works on contents in the working\n   tree, the established command line convention to alter the\n   operation to work on contents in the index is with \"--cached\",\n   and the operation by default would work on the contents in the\n   working tee without \"--worktree\".  See gitcli(7).\n\n   If I were doing this as part of \"ls-files\", I would add a new\n   \"--get-eol\" option that inspects the working tree, and make\n   \"--get-eol --cached\" do the same for the contents in the index,\n   for consistency.  If I were doing \"git check-eol\", then the\n   default mode of the operation would read from the working tree,\n   and \"git check-eol --cached\" would read from the index.\n\n   If the operation can work on both the contents in the index and\n   in the working tree at the same time, we use \"--index\" instead of\n   \"--cached\", but I do not think it applies here (only a small\n   number of commands work on both to begin with, e.g. \"apply\").\n"},{"id":"271891","messageId":"56247E3F.40804@web.de","threadId":"40581","inReplyTo":"xmqq4mhoatna.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] Add ls-files --eol-staged, --eol-worktree","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2015-10-19T05:23:11Z","receivedAt":"2015-10-19T05:23:11Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"\n\nOn 18/10/15 21:00, Junio C Hamano wrote:\n> Torsten Bögershausen <tboegi@web.de> writes:\n>\n>> Make it possible to show the line endings of files.\n>> Files which are staged and/or files in the working tree:\n>>\n>> git ls-files --eol-staged\n>> git ls-files --eol-worktree\n> Two unrelated (to the issues raised in other review responses)\n> issues in the UI:\n>\n>   - While I can see how the new feature would be useful, I am not\n>     convinced that it is a good idea to add it to ls-files.  Does the\n>     option work well with other existing options like -s, -t, etc?\n>     Does it make sense to combine it with other options like -m, -d,\n>     etc?  I have this suspicion that \"check-attr\", \"check-ignore\",\n>     etc.  may give a better model that fits this feature better,\n>     i.e. \"git check-eol\".\n>\n(This should answer all comments, thanks everybody\n@Erics Sunshine: Thanks for the review, dropped you from cc list because web.de \ncan't find an MX record)\n\n\nI like this idea:\n\nbinary\ntext\ncrlf\nmixed\nlf\n\n\n----------------\n$ git ls-files --eol-staged -s\n  [snip]\n  100644 981f810e80008d878d6a5af1331c89dc093c5927 0       txt-lf worktree.c\n\n---------------------\n$ rm  Documentation/RelNotes/2.7.0.txt\n$ echo \"/* */\" >>builtin/ls-files.c\n$ ls-files -m --eol-worktree\n   empty    Documentation/RelNotes/2.7.0.txt\n   txt-lf   builtin/ls-files.c\n-----------------------\n(The empty is a bug, thanks Eric)\n------------------------------\n$ ./git-ls-files --eol-worktree -t\n[snip]\n$ H txt-lf   zlib.c\n-------------------------\nMy understanding is that the eol options work togther with the existing option,\nas long as it makes sense (but see below)\n\n\"git check-attr\" will even report attributes for a file, that doesn't even exist.\n\"git ls-files is a command which by default operates on the staged area, unless \nI mis-understand it.\n\nAnd that is the main purpose:\nTell me how which line endings your staged files have, and I can tell you that \nyou may\nconsider to normalize these files because \"git status\", \"git blame\" consider \nthese files as changed.\n\n(From that point of view,\n\"git ls-files --eol\" could be the way to report the staged eols.\nBut then users would ask:\nbut why can't you tell me what I have in my worktree ?\n\n\"git ls-files --eol-worktree\" could be the answer (or \"git ls-files -o \n--eol-worktree\" )\n\nI was thinking about adding \"git check-eol\", but didn't want to introduce just \nanother command,\nas the syntax and options (-z, -o -x -X) overlap much with ls-files\n\nIs it the common understanding to add a new command is the best solution ?\n"},{"id":"271892","messageId":"xmqqvba39xn5.fsf@gitster.mtv.corp.google.com","threadId":"40581","inReplyTo":"56247E3F.40804@web.de","subject":"Re: [PATCH] Add ls-files --eol-staged, --eol-worktree","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-10-19T06:32:14Z","receivedAt":"2015-10-19T06:32:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Torsten Bögershausen <tboegi@web.de> writes:\n\n> I like this idea:\n>\n> binary\n> text\n> crlf\n> mixed\n> lf\n\nIf you really like it, it would mean that my attempt to use Socratic\nmethod to enlighten you why the above is not a good idea failed X-<.\n\n> ----------------\n> $ git ls-files --eol-staged -s\n>  [snip]\n>  100644 981f810e80008d878d6a5af1331c89dc093c5927 0       txt-lf worktree.c\n\nDoes it even make sense to give --eol-worktree in this case?\n\n> My understanding is that the eol options work togther with the existing option,\n> as long as it makes sense (but see below)\n>\n> \"git check-attr\" will even report attributes for a file, that doesn't even exist.\n\nBoth \"ls-files -o/-i\" talk about untracked paths, so that is not a\nvery useful and valid objection, is it?\n\n> \"git ls-files is a command which by default operates on the staged\n> area, unless I mis-understand it.\n\nIt is even worse than that.  It is true that \"ls-files [-s]\" is\nabout \"--cached\" and there is no equivalent to show the working tree\nversion.  But \"-t\", \"-d\", etc. are not about the state in the index\nnor the state in the working tree.  They are about the relationship\nbetween these two states.\n\nWhat the new operation wants to do, if I understand correctly, is\neither check the blob contents in the working tree or in the index,\nwhich is not a good fit with what the rest of \"ls-files\" does for\nexactly that reason.  The inability to mix -s with --eol-worktree\nis another natural consequence of this.\n\n> I was thinking about adding \"git check-eol\", but didn't want to\n> introduce just another command,\n\nBetween adding a new command that does one thing well and whose user\ninteraction is coherent with the rest of the system, and adding a\nnew operation mode to an existing command and makes the user\ninteraction of that existing command more incoherent by introducing\ntwo variants --foo and --foo-worktree when there is no existing\noption that has similar variant pair, I'd say we prefer to see a new\ncommand.\n\nThe -z output, and --stdin input are what we would want to have for\nthe new command, but I do not think we want it to know -o, -x or -X.\nYou would instead pipe output from ls-files with these options to\nthe new command run with the --stdin option.\n"}]}