Re: Re: [RFC/WIP PATCH 0/4] less ignorance of submodules for ignore=all
On Thu, Dec 05, 2013 at 09:51:31PM +0100, Jens Lehmann wrote: Am 05.12.2013 00:19, schrieb Heiko Voigt: On Wed, Dec 04, 2013 at 02:32:46PM -0800, Junio C Hamano wrote: This series tries to achieve the following goals for the submodule.name.ignore=all configuration or the --ignore-submodules=all command line switch. Thanks for the summary. * Make git status never ignore submodule changes that got somehow in the index. Currently when ignore=all is specified they are and thus secretly committed. Basically always show exactly what will be committed. Yes, what's in the index should always be shown as such even when the user chose to ignore the work tree differences of the submodule. * Make add ignore submodules that have the ignore=all configuration when not explicitly naming a certain submodule (i.e. using git add .). That way ignore=all submodules are not added to the index by default. That can be overridden by using the -f switch so it behaves the same as with untracked files specified in one of the ignore files except that submodules are actually tracked. I think we should do this part in a different series, as everybody seems to agree that this should be fixed that way and it has nothing to do with what is ignored in submodule history. So how about I put the two points above into a separate series? IMO, add and status belong together in this case. * Let diff always show submodule changes between revisions or between a revision and the index. Only worktree changes should be ignored with ignore=all. * Generally speaking: Make everything that displays diffs in history, diffs between revisions or between a revision and the index always show submodules changes (only the commit ids) even if a submodule is specified as ignore=all. I'm not so sure about that. Some scripts really want to ignore the history of submodules when comparing a rev to the index: git-filter-branch.sh: git diff-index -r --name-only --ignore-submodules $commit git-pull.sh:git diff-index --cached --name-status -r --ignore-submodules HEAD -- git-rebase--merge.sh: if ! git diff-index --quiet --ignore-submodules HEAD -- git-sh-setup.sh: if ! git diff-index --cached --quiet --ignore-submodules HEAD -- git-stash.sh: git diff-index --quiet --cached HEAD --ignore-submodules -- I didn't check each site in detail, but I suspect each ignore option was added on purpose to fix a problem. That means we still need all (at least when diffing rev-index). Unfortunately that area is not covered well in our tests, I only got breakage from the filter-branch tests when teaching all to only ignore work tree changes (see at the end on how I did that). Well all hits are on diff-index which is plumbing. If it is required for some (internal) scripts to completely ignore submodules I think it is ok to do so just for plumbing with a commandline option like that. But I am not sure whether this should actually be configurable. So I'm currently in favor of adding a new worktree-value which will only ignore the work tree changes of submodules, which seems just what the floating submodule use case needs. But it looks like we need to keep all. But that will just add more complexity to the already complex topic of submodules. There are already enough possibilities to get confused with submodules. I would like to avoid making it more complex. From the feedback we get now from Sergey I take that not many users have actually been using the 'all' option. Otherwise there would have been more complaints. So the only thing we have to worry about are scripts and those we could cover with plumbing commands. * If ignore=all for a submodule and a diff would usually involve the worktree we will show the diff of the commit ids between the current index and the requested revision. I agree if we make that ignore=worktree. I do think that it is a good thing to make what git add . does and what git status . reports consistent, and git add . that does not add everything may be a good step in that direction Yup, as written above I'd propose to start with that too. (another possible solution may be to admit that ignore=all was a mistake and remove that special case altogether, so that git status will always report a submodule that does not match what is in the HEAD and/or index). No, looking at the git-scripts that use it together with diff-index it wasn't a mistake. But we might be missing a less drastic option ;-) Well, as said above diff-index is plumbing and as such allowed to do non-user friendly stuff. For all the other commands I propose to never hide stuff from the user. E.g. take update-index there you can actually mark any index entry as assume unchanged which will make git stop checking it for changes (like the submodule.name.ignore=all setting for submodules). IMO that is a
Re: [RFC/WIP PATCH 0/4] less ignorance of submodules for ignore=all
Jens Lehmann jens.lehm...@web.de writes: I didn't check each site in detail, but I suspect each ignore option was added on purpose to fix a problem. That means we still need all (at least when diffing rev-index). Unfortunately that area is not covered well in our tests, I only got breakage from the filter-branch tests when teaching all to only ignore work tree changes (see at the end on how I did that). So I'm currently in favor of adding a new worktree-value which will only ignore the work tree changes of submodules, which seems just what the floating submodule use case needs. Could you help me clarify what it exactly mean to only ignore the work tree changes? Specifically, if I have a submodule directory whose (1) HEAD points at a commit that is the same as the commit that is recorded by the top-level's gitlink, (2) the index may or may not match HEAD, and (3) the working tree contents may or may not match the index or the HEAD, does it only have the work tree changes? If the HEAD in the submodule directory is different from the commit recorded by the top-level's gitlink, but the index and the working tree match that different HEAD, I am guessing that it no longer is with only the work tree changes and shown as modified. If that is the suggestion, it goes back to the very original Git submodule behavour where we compare $submoduledir/.git/HEAD with the commit object name expected by the top-level and say the submodule does not have any change when and only when these two object names are the same, which sounds like a very sensible default behaviour (besides, it is very cheap to check ;-). -- 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: [RFC/WIP PATCH 0/4] less ignorance of submodules for ignore=all
Am 05.12.2013 00:19, schrieb Heiko Voigt: On Wed, Dec 04, 2013 at 02:32:46PM -0800, Junio C Hamano wrote: This series tries to achieve the following goals for the submodule.name.ignore=all configuration or the --ignore-submodules=all command line switch. Thanks for the summary. * Make git status never ignore submodule changes that got somehow in the index. Currently when ignore=all is specified they are and thus secretly committed. Basically always show exactly what will be committed. Yes, what's in the index should always be shown as such even when the user chose to ignore the work tree differences of the submodule. * Make add ignore submodules that have the ignore=all configuration when not explicitly naming a certain submodule (i.e. using git add .). That way ignore=all submodules are not added to the index by default. That can be overridden by using the -f switch so it behaves the same as with untracked files specified in one of the ignore files except that submodules are actually tracked. I think we should do this part in a different series, as everybody seems to agree that this should be fixed that way and it has nothing to do with what is ignored in submodule history. * Let diff always show submodule changes between revisions or between a revision and the index. Only worktree changes should be ignored with ignore=all. * Generally speaking: Make everything that displays diffs in history, diffs between revisions or between a revision and the index always show submodules changes (only the commit ids) even if a submodule is specified as ignore=all. I'm not so sure about that. Some scripts really want to ignore the history of submodules when comparing a rev to the index: git-filter-branch.sh: git diff-index -r --name-only --ignore-submodules $commit git-pull.sh:git diff-index --cached --name-status -r --ignore-submodules HEAD -- git-rebase--merge.sh: if ! git diff-index --quiet --ignore-submodules HEAD -- git-sh-setup.sh:if ! git diff-index --cached --quiet --ignore-submodules HEAD -- git-stash.sh: git diff-index --quiet --cached HEAD --ignore-submodules -- I didn't check each site in detail, but I suspect each ignore option was added on purpose to fix a problem. That means we still need all (at least when diffing rev-index). Unfortunately that area is not covered well in our tests, I only got breakage from the filter-branch tests when teaching all to only ignore work tree changes (see at the end on how I did that). So I'm currently in favor of adding a new worktree-value which will only ignore the work tree changes of submodules, which seems just what the floating submodule use case needs. But it looks like we need to keep all. * If ignore=all for a submodule and a diff would usually involve the worktree we will show the diff of the commit ids between the current index and the requested revision. I agree if we make that ignore=worktree. I do think that it is a good thing to make what git add . does and what git status . reports consistent, and git add . that does not add everything may be a good step in that direction Yup, as written above I'd propose to start with that too. (another possible solution may be to admit that ignore=all was a mistake and remove that special case altogether, so that git status will always report a submodule that does not match what is in the HEAD and/or index). No, looking at the git-scripts that use it together with diff-index it wasn't a mistake. But we might be missing a less drastic option ;-) I think it was too early to add ignore=all back then when the ignoring was implemented. We did not think through all implications. Since people have always been requesting the floating model and as it seems started using it I am not so sure whether there is not a valid use case. Maybe Sergey can shed some light on their actual use case and why they do not care about the precise revision most of the time. You maybe right about not thinking things thoroughly through, but we helped people that rightfully complained when the (then new) submodule awareness broke their scripts. For example the case that all developers always want to work with some HEAD revision of all submodules and the build system then integrates their changes on a regular basis. When all went well it creates commits with the precise revisions. This way they have some stable points as fallback or for releases. Thats at least the use case I can think of but maybe there are others. And that could be the worktree value. Below is a hack that disables the diffing of rev and index, but not that against the work tree. It breaks t4027-diff-submodule.sh, t7003-filter-branch.sh and t7508-status.sh as expected: --8 diff --git a/diff.c b/diff.c index e34bf97..ed66a01 100644 --- a/diff.c +++ b/diff.c @@ -4813,7 +4813,7 @@ static int
Re: [RFC/WIP PATCH 0/4] less ignorance of submodules for ignore=all
On Wed, Dec 04, 2013 at 11:16:59PM +0100, Heiko Voigt wrote: * The git diff $commit question Junio mentioned here[1] it does not yet show diffs of ignore=all submodules. Forgot to add this here: [1] http://article.gmane.org/gmane.comp.version-control.git/238348 -- 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: [RFC/WIP PATCH 0/4] less ignorance of submodules for ignore=all
Heiko Voigt hvo...@hvoigt.net writes: This is my current work in progress. Sergey it would be awesome if you could test these and tell me whether the behaviour is what you would expect. Once that is settled I will add some tests and possibly clean up some code. Since nobody spoke against this change of behavior I assume that we agree on the general approach I am taking here. If not please speak up now so we can work something out and save me implementation time ;-) Whats still missing is: Before listing what's missing, can you describe what the general approach is? After all, that is what you are assuming that has got a silent concensus, but without getting it spelled out, others would easily miss what they agreed to. I do think that it is a good thing to make what git add . does and what git status . reports consistent, and git add . that does not add everything may be a good step in that direction (another possible solution may be to admit that ignore=all was a mistake and remove that special case altogether, so that git status will always report a submodule that does not match what is in the HEAD and/or index). -- 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: Re: [RFC/WIP PATCH 0/4] less ignorance of submodules for ignore=all
On Wed, Dec 04, 2013 at 02:32:46PM -0800, Junio C Hamano wrote: Heiko Voigt hvo...@hvoigt.net writes: This is my current work in progress. Sergey it would be awesome if you could test these and tell me whether the behaviour is what you would expect. Once that is settled I will add some tests and possibly clean up some code. Since nobody spoke against this change of behavior I assume that we agree on the general approach I am taking here. If not please speak up now so we can work something out and save me implementation time ;-) Whats still missing is: Before listing what's missing, can you describe what the general approach is? After all, that is what you are assuming that has got a silent concensus, but without getting it spelled out, others would easily miss what they agreed to. Definitely, sorry I missed that (isn't it obvious ;-)): This series tries to achieve the following goals for the submodule.name.ignore=all configuration or the --ignore-submodules=all command line switch. * Make git status never ignore submodule changes that got somehow in the index. Currently when ignore=all is specified they are and thus secretly committed. Basically always show exactly what will be committed. * Make add ignore submodules that have the ignore=all configuration when not explicitly naming a certain submodule (i.e. using git add .). That way ignore=all submodules are not added to the index by default. That can be overridden by using the -f switch so it behaves the same as with untracked files specified in one of the ignore files except that submodules are actually tracked. * Let diff always show submodule changes between revisions or between a revision and the index. Only worktree changes should be ignored with ignore=all. * Generally speaking: Make everything that displays diffs in history, diffs between revisions or between a revision and the index always show submodules changes (only the commit ids) even if a submodule is specified as ignore=all. * If ignore=all for a submodule and a diff would usually involve the worktree we will show the diff of the commit ids between the current index and the requested revision. I do think that it is a good thing to make what git add . does and what git status . reports consistent, and git add . that does not add everything may be a good step in that direction (another possible solution may be to admit that ignore=all was a mistake and remove that special case altogether, so that git status will always report a submodule that does not match what is in the HEAD and/or index). I think it was too early to add ignore=all back then when the ignoring was implemented. We did not think through all implications. Since people have always been requesting the floating model and as it seems started using it I am not so sure whether there is not a valid use case. Maybe Sergey can shed some light on their actual use case and why they do not care about the precise revision most of the time. For example the case that all developers always want to work with some HEAD revision of all submodules and the build system then integrates their changes on a regular basis. When all went well it creates commits with the precise revisions. This way they have some stable points as fallback or for releases. Thats at least the use case I can think of but maybe there are others. Cheers Heiko -- 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
