patchAdd ls-files --eol-staged, --eol-worktree

Make it possible to show the line endings of files. Files which are staged and/or files in the working tree:

git ls-files --eol-staged git ls-files --eol-worktree

Both will show an output like this:

empty empty_file bin binary_file_or_with_cr_handled_as_binary txt-crlf text_file_with_crlf txt-mix text_file_with_crlf_and_lf txt-lf text_file_with_lf txt text_file_with_no_eol_at_all

Implementation details: Make struct text_stat, is_binary() and gather_stats() from convert.c public, add a new function get_convert_stats_ascii() and use it in and use them in ls-files. git ls-files --eol-staged will give a line like this:

Signed-off-by: Torsten Bögershausen <tboegi@web.de>
---
This needs to go on top of tb/t0027-crlf

builtin/ls-files.c | 21 +++++++++++++++++++++ convert.c | 51 +++++++++++++++++++++++++++++++++++++++++---------- convert.h | 14 ++++++++++++++ 3 files changed, 76 insertions(+), 10 deletions(-)

On Sat, Oct 17, 2015 at 4:18 PM, Torsten Bögershausen <tboegi@web.de> wrote:

s/and use it in and use them in/and use them in/

"... line like this:" what?

Whitespace-damaged patch?

Style: unnecessary braces

Style: unnecessary braces (both cases)

Style: unnecessary braces

Style: unnecessary blank line

Is this name too generic? This implementation is specialized for summarizing EOL state, but it is conceivable that there may be other meaningful textual representations of struct text_stat, as well, so perhaps this name ought to better reflect the EOL-centric textual representation of this implementation.

Would it make sense to distinguish between an empty file/blob and one which was not found?

    if (!data)
        return "missing";
    if (!size)
        return "empy";

Style: unnecessary parentheses on 'return'

Style (nit): unnecessary 'else'

Is it a good idea for this otherwise general purpose function to be conflating presentation with EOL classification? I would have expected this function to return the raw classification string without trailing whitespace ("empty", "bin", "txt-mix", etc.), and leave it up to the caller to pad the string if alignment is desired.

Is 'flags' an appropriate name? Presently, it's more of a "mode" than a flag. If it is a flag, then you typically would use an unsigned type.

If you can't foresee any additional "flags", and this really is a "mode", then it might be clearer to split this into two functions -- one for blobs and one for files -- and callers can invoke whichever is appropriate.

Unnecessary initialization since all  branches assign 'ret' unconditionally.

gather_stats_ascii() deals gracefully with data== NULL, so it can safely be outside the 'if (data)' conditional. Okay.

POSIX says that free(NULL) is safe, so the conditional is unnecessary.

Style: space before {

If gather_stats_ascii() should distinguish error and empty-file cases, then checking the return value of strbuf_read_file() would be appropriate.

s/isBlob/flags/ or s/isBlob/mode/ or something.

Having read the source code, I know what 'hint' is for, but its purpose is not obvious when just reading this prototype. Also, do you expect 'hint' to prove worthwhile or might it be a case of premature optimization and interface complexity? (Genuine question.)

From: "Torsten Bögershausen" <tboegi@web.de>

I think that this last generic 'txt' should be explicit, e.g. 'txt-no-eols', so that the categories are explicit and mutually exclusive.

Also, does this need a documentation update for the options, and can the distinction between txt and bin be documented /referenced.

Philip 

Torsten Bögershausen <tboegi@web.de> writes:

This _may_ be readable by who implemented it, but I cannot tell what you are trying to say above; "like this" does not help me at all deciphering.

Do I get the above after doing this?

    $ >empty
    $ >empty2
    $ git ls-files --eol-worktree
    empty empty_file
    empty2 empty_file

or do you mean this?

    $ >empty_file
    $ >empty_file2
    $ git ls-files --eol-worktree
    empty empty_file empty_file2

or do you produce output one path at a time?

    $ >empty_file
    $ >empty_file2
    $ git ls-files --eol-worktree
    empty empty_file
    empty empty_file2

Does it help to have this category at all? This gives the user an indication that this file consists on a single incomplete line and differentiates it from a file with the same single line with LF or CR/LF line ending. What happens when I prepend to these three files another copy of the same line with LF or CR/LF line ending? You get 6 variations:

 1. line LF line
 2. line LF line LF
 3. line LF line CRLF
 4. line CRLF line
 5. line CRLF line LF
 6. line CRLF line CRLF
    
If you say 1 and 2 are with LF, 4 and 6 are with CRLF, eveyrything
else is mixed, then you are losing the distinction between 1 and 2
(and 4 and 6) that you made when the files were a single liner (with
or without the incomplete line ending).  Is that desirable?

I wonder if it would be easier for the scripts that process the output from this command to handle if the report said what combination of _three_ possible line-ending is used. i.e. does the file contain a line that ends with LF? does the file contain a line that ends with CRLF? does the file contain a line with missing EOL?

This is even worse than the "like this" above ;-)

Junio C Hamano <gitster@pobox.com> writes:

Continuing this line of thought further. My answer is "it may not be desirable, but it is sufficient to decide what line ending to use when I am adding a new line to the existing contents". That is, to a file with a single incomplete line, I can add my new line with either LF or CRLF and the resulting one will become LF (or CRLF) that ends in an incomplete line. To a file whose lines consistently use LF, I can only add my new line with LF, whether the original ends in an incomplete line.

So from that point of view,...

... instead of saying there are three possible line-endings, we can stick to two, i.e. "is it text or binary" followed by "among two possible endings, which ones are used", i.e.

    text
    text-lf
    text-crlf
    text-lf-crlf

which matches the last four lines of what you had in the "like this" example (and I prefer "mixed" over "lf-crlf").

So I am OK with the categorization after all with respect to the possible incomplete line at the end. But if that is what the feature is designed for, the documentation must say it very clearly, i.e. "this is to allow you decide what line ending to use when you add a new line to the existing contents" or something.

And viewed from that angle, there is no reason to special-case an empty line. Knowing "binary" is important because you want to be able to say "whether LF or CRLF, you do *not* want to add your new line to this binary file."; "empty" is just like "text"---you can use either and get a coherent result.

So I'd suggest sticking to these classification tokens:

	binary
        text
        crlf
        mixed
        lf

The "adding my line at the beginning of the file" script can do something like this using them (here I am simplifying by making your feature available to "git get-eol" command that takes a single path and does your computation):

	case "$(git get-eol file)" in
	text | lf)
        	printf "%s\n" "$mine"
                ;;
	crlf)
        	printf "%s\r\n" "$mine"
                ;;
        *) # that is 'binary' or 'mixed'
		die "do not muck with the contents of file"
                ;;
	esac
	cat file

Also this points at another direction of using the three independent line ending conventions I suggested earlier. What you want to append your lines? You would want to know if the file ends with an incomplete line, so you would rather want to be told with a set of categories like this instead:

	binary
	incomplete
        incomplete,crlf
        incomplete,mixed
        incomplete,lf
        crlf
        mixed
        lf

Note that an empty file will get an empty string as the grouping above, as it does not have any line ending (i.e. no crlf/mixed/lf), does not end with an incomplete line and is not a binary file.

And the using script would become:

	existing=$(git get-eol file)
	eol='\n'
	case ",$existing" in
	,binary | *,mixed)
		die "do not muck with the contents of file"
		;;
	,*crlf)
		eol='\r\n'
                ;;
	esac
        cat file
        case "$existing," in
        incomplete,*)
		printf "$eol"
                ;;
	esac
        printf "%s$eol" "$mine"

Torsten Bögershausen <tboegi@web.de> writes:

Two unrelated (to the issues raised in other review responses) issues in the UI:

 - While I can see how the new feature would be useful, I am not
   convinced that it is a good idea to add it to ls-files.  Does the
   option work well with other existing options like -s, -t, etc?
   Does it make sense to combine it with other options like -m, -d,
   etc?  I have this suspicion that "check-attr", "check-ignore",
   etc.  may give a better model that fits this feature better,
   i.e. "git check-eol".

 - When you have an operation that works on contents in the working
   tree, the established command line convention to alter the
   operation to work on contents in the index is with "--cached",
   and the operation by default would work on the contents in the
   working tee without "--worktree".  See gitcli(7).

   If I were doing this as part of "ls-files", I would add a new
   "--get-eol" option that inspects the working tree, and make
   "--get-eol --cached" do the same for the contents in the index,
   for consistency.  If I were doing "git check-eol", then the
   default mode of the operation would read from the working tree,
   and "git check-eol --cached" would read from the index.

   If the operation can work on both the contents in the index and
   in the working tree at the same time, we use "--index" instead of
   "--cached", but I do not think it applies here (only a small
   number of commands work on both to begin with, e.g. "apply").

On 18/10/15 21:00, Junio C Hamano wrote:

(This should answer all comments, thanks everybody @Erics Sunshine: Thanks for the review, dropped you from cc list because web.de can't find an MX record)

I like this idea:

binary text crlf mixed lf

----------------
$ git ls-files --eol-staged -s
  [snip]
  100644 981f810e80008d878d6a5af1331c89dc093c5927 0       txt-lf worktree.c

---------------------
$ rm  Documentation/RelNotes/2.7.0.txt
$ echo "/* */" >>builtin/ls-files.c
$ ls-files -m --eol-worktree
   empty    Documentation/RelNotes/2.7.0.txt
   txt-lf   builtin/ls-files.c
-----------------------
(The empty is a bug, thanks Eric)
------------------------------
$ ./git-ls-files --eol-worktree -t
[snip]
$ H txt-lf   zlib.c
-------------------------
My understanding is that the eol options work togther with the existing option,
as long as it makes sense (but see below)

"git check-attr" will even report attributes for a file, that doesn't even exist. "git ls-files is a command which by default operates on the staged area, unless I mis-understand it.

And that is the main purpose: Tell me how which line endings your staged files have, and I can tell you that you may consider to normalize these files because "git status", "git blame" consider these files as changed.

(From that point of view, "git ls-files --eol" could be the way to report the staged eols. But then users would ask: but why can't you tell me what I have in my worktree ?

"git ls-files --eol-worktree" could be the answer (or "git ls-files -o --eol-worktree" )

I was thinking about adding "git check-eol", but didn't want to introduce just another command, as the syntax and options (-z, -o -x -X) overlap much with ls-files

Is it the common understanding to add a new command is the best solution ?

Torsten Bögershausen <tboegi@web.de> writes:

If you really like it, it would mean that my attempt to use Socratic method to enlighten you why the above is not a good idea failed X-<.

Does it even make sense to give --eol-worktree in this case?

Both "ls-files -o/-i" talk about untracked paths, so that is not a very useful and valid objection, is it?

It is even worse than that. It is true that "ls-files [-s]" is about "--cached" and there is no equivalent to show the working tree version. But "-t", "-d", etc. are not about the state in the index nor the state in the working tree. They are about the relationship between these two states.

What the new operation wants to do, if I understand correctly, is either check the blob contents in the working tree or in the index, which is not a good fit with what the rest of "ls-files" does for exactly that reason. The inability to mix -s with --eol-worktree is another natural consequence of this.

Between adding a new command that does one thing well and whose user interaction is coherent with the rest of the system, and adding a new operation mode to an existing command and makes the user interaction of that existing command more incoherent by introducing two variants --foo and --foo-worktree when there is no existing option that has similar variant pair, I'd say we prefer to see a new command.

The -z output, and --stdin input are what we would want to have for the new command, but I do not think we want it to know -o, -x or -X. You would instead pipe output from ls-files with these options to the new command run with the --stdin option.