Re: [PATCH] travis-ci: run previously failed tests first, then slowest to fastest

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 1, 2016, 18:17 UTC
Message-ID: <xmqqlh74wb0r.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20160130081306.GA2931@ecki.hitronhub.home>

Clemens Buchacher <drizzd@aon.at> writes:

> Ok, then let's take a step back. I do not actually care if git diff and
> friends say the worktree is clean or not.

You may not, but many existing scripts people have do.

Show 5 quoted lines

> But I know that I did not make
> any modifications to the worktree, because I just did git reset --hard.
> And now I want to use commands like cherry-pick and checkout without
> failure. But they can fail, because they essentially use git diff to
> check if there are worktree changes, and if so refuse to overwrite them.

Yes, exactly.

Show 5 quoted lines

> So, if the check "Am I allowed to modify the worktree file?", would go
> the extra mile to also check if the worktree is clean in the sense that
> convert_to_worktree(index state) matches the worktree. If this is the
> case, then it is safe to modify the file because it is the committed
> state, and can be recovered.

So in essense, the proposed "fix" is "let's fix it in the right way"?

The way we defined "would we lose some changes that are only in the working tree?", aka "is the working tree dirty wrt the index?", has been to check if "git add -u" would change the states in the index. And for scripted Porcelains and end-user scripts, "git diff-files", aka "what change would 'git add -u' make to the states in the index?", has been the command to do the same check.

Your proposal is to redefine "is the working tree dirty?"; it would check if "git checkout -f" would change what is in the working tree.

I agree that indeed is "would we lose some changes that are only in the working tree", and I think we can do that transparently for "internal" commands, i.e. without any end-user impact, as the new check would behave identically when they have sane contents--the difference between the current check and the new check only exists when the contents in the index contradicts what the user specifies for to-git conversion via eol or clean filter.

We would need a way for our scripted Porcelains and end-user scripts to ask that new question, though, but I think that is not something insurmountable. A new option to "diff-files" or something, perhaps, would be workable, but having a new "git require-clean-work-tree" plumbing, which would replace require_clean_work_tree shell helper in git-sh-setup, may be conceptually much cleaner, because the new definition of "working tree being clean" is no longer tied to what "diff" should say.

I like that as a general direction.

> Regarding performance impact: We only need to do this extra check if the
> usual check convert_to_git(work tree) against index state fails, and
> conversion is in effect.

How would you detect the failure, though? Having contents in the index that contradicts the attributes and eol settings affects the cleanliness both ways. Running the working tree contents via to-git conversion and hashing may match the blob in the index, declaring that the index entry is "clean", but passing the blob to to-worktree conversion may produce result different from what is in the worktree, which is "falsely clean". That is an equally important case that is opposite from what we have been primarily discussing, which is "falsely dirty".

Show 8 quoted lines

>> Besides, I do not think the above approach really solves the issue,
>> either.  After "git reset --hard" to have the contents in the index
>> dumped to the working tree, if your core.autocrlf is flipped,
>
> Indeed, if the user configuration changes, then we cannot always detect
> this (at least if the filter is an external program, and the behavior of
> that changes). But the user is in control of that, and we can document
> this limitation.

That argument does not result in a very useful result, though. Because the user is in control of what attributes and eol settings are in effect in her repository, we can just document that the current check will give unspecified result if the indexed contents contradict with that setting, e.g. when you have CRLF encoded data in the index but the eol conversion assumes LF in the repository. But this discussion is an attempt to do better than that, no?

Show 6 quoted lines

> On the other hand, a user who simply follows an upstream repository by
> doing git pull all the time, and who does not make changes to their
> configuration, can still run into this issue, because upstream could
> change .gitattributes. This part we could actually detect by hashing the
> attributes for each index entry, and if that changes we re-evaluate the
> file state.

If this has to bloat each index entry, I do not think solving the problem is worth that cost of that overhead. I'd rather just say "if you have inconsistent data, here is a workaround using 'reset' and then 'reset --hard'" and be done with it.

> This is also an issue only if a smudge filter is in place. The eol
> conversion which only acts in the convert_to_git direction is not
> affected.

IIRC, autocrlf=true would strip CR at the end of line in to-git conversion, and would add CR in to-worktree conversion. So some eol conversion may only act in to-git, but some others do affect both, and without needing you to touch attributes.