Re: [PATCH] docs: submitting-patches: improve the base commit explanation
On Wed, Nov 15, 2023 at 09:49:31AM -0800, Kees Cook wrote: > On Wed, Nov 15, 2023 at 06:03:30PM +0100, Borislav Petkov wrote: > > From: "Borislav Petkov (AMD)" > > > > After receiving a second patchset this week without knowing which tree > > it applies on and trying to apply it on the obvious ones and failing, > > make sure the base tree information which needs to be supplied in the > > 0th message of the patchset is spelled out more explicitly. > > > > Also, make the formulations stronger as this really is a requirement and > > not only a useful thing anymore. > > > > Signed-off-by: Borislav Petkov (AMD) > > Yup, I wonder if making "--base=auto" a default in git might be a good > idea too? Not a bad idea. And if not needed, one can simply ignore it when reading the cover letter. CCing the git ML for comment/opinions. Thx. -- Regards/Gruss, Boris. https://people.kernel.org/tglx/notes-about-netiquette
Re: [git issue] git am failed for patches of converting the format of source codes from dos to unix
On 09/27, Beyondhorizon Zheng wrote: > [git issue] git am failed for patches of converting the file format of > source codes from dos to unix > > Git version: git version 2.23.0 > Host PC: ubuntu 16.04.10 > Reporter: Shuang Zheng > > I have submitted a patch which convert the file format of source file > from dos to unix with command: > dos2unix misc/acrn-config/config_app/controller.py > then submit a patch with this change: > git add controller.py > git commit -m “change file format” > git format-patch HEAD~1 Here you are preparing a patch of your last commit. Good. > git am 0001-change-file-format.patch Running this right after 'git format-patch' will try to apply the patch, but your latest commit already includes those changes. So there is nothing to apply, and 'git am' fails. Normally you would not use 'format-patch' and then try to apply the patch again, but rather send it to someone else, who could then apply your changes. You could use 'git am -3 0001-change-file-format.patch', which falls back to a three-way merge, which should tell you something like: No changes -- Patch already applied. If you want to check that the patch applies correctly, you can use 'git reset --hard HEAD~1' (make sure you don't have any untracked changes you don't want to loose first), and then use the 'git am' command from above. Hope that helps. > errors as below: > Applying: change file format > error: patch failed: misc/acrn-config/config_app/controller.py:1 > error: misc/acrn-config/config_app/controller.py: patch does not apply > Patch failed at 0001 fix format > hint: Use 'git am --show-current-patch' to see the failed patch > When you have resolved this problem, run "git am --continue". > If you prefer to skip this patch, run "git am --skip" instead. > To restore the original branch and stop patching, run "git am --abort".
[git issue] git am failed for patches of converting the format of source codes from dos to unix
[git issue] git am failed for patches of converting the file format of source codes from dos to unix Git version: git version 2.23.0 Host PC: ubuntu 16.04.10 Reporter: Shuang Zheng I have submitted a patch which convert the file format of source file from dos to unix with command: dos2unix misc/acrn-config/config_app/controller.py then submit a patch with this change: git add controller.py git commit -m “change file format” git format-patch HEAD~1 git am 0001-change-file-format.patch errors as below: Applying: change file format error: patch failed: misc/acrn-config/config_app/controller.py:1 error: misc/acrn-config/config_app/controller.py: patch does not apply Patch failed at 0001 fix format hint: Use 'git am --show-current-patch' to see the failed patch When you have resolved this problem, run "git am --continue". If you prefer to skip this patch, run "git am --skip" instead. To restore the original branch and stop patching, run "git am --abort".
[PATCH 0/4] git-gui: GIT_ASK_YESNO/GIT_ASKPASS patches from Git for Windows
This is another set of patches from Git for Windows' fork that have been sitting there since 2010, providing cross-platform GUI helpers to ask the user a question or allow typing in a password. This patch series was first submitted as https://github.com/patthoyts/git-gui/pull/5 which was ignored for almost three years, then re-submitted as https://github.com/prati0100/git-gui/pull/3 which was rejected in favor of a contribution by mail. The patches are based on Git GUI's master branch at https://github.com/prati0100/git-gui/. Heiko Voigt (1): git-gui: provide question helper for retry fallback on Windows Johannes Schindelin (3): git gui: set GIT_ASKPASS=git-gui--askpass if not set yet git-gui--askyesno: allow overriding the window title git-gui--askyesno (mingw): use Git for Windows' icon, if available Makefile | 2 ++ git-gui--askyesno | 68 +++ git-gui.sh| 6 + 3 files changed, 76 insertions(+) create mode 100755 git-gui--askyesno base-commit: 12d29c326551a6570594db525bea42ad9cea8028 Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-358%2Fdscho%2Fgit-gui-askpass-v1 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-358/dscho/git-gui-askpass-v1 Pull-Request: https://github.com/gitgitgadget/git/pull/358 -- gitgitgadget
Re: git-gui: missing some patches from git?
On 19/09/19 12:11PM, Denton Liu wrote: > On Fri, Sep 20, 2019 at 12:33:59AM +0530, Pratyush Yadav wrote: > > On 19/09/19 11:47AM, Denton Liu wrote: > > > For the record, you could do a > > > > > > git cherry-pick -Xsubtree=git-gui 00ddc9d13c 7560f547e6 > > > > > > to bring them over instead of manually recreating the changes yourself. > > > Personally, I'd prefer the cherry-picked commits as it'd preserve > > > authorship information but I'm not sure how Junio feels. > > > > I'm not sure how this will work internally, but won't this also pull all > > the ancestors of those commits into git-gui? That is bloat I'd rather > > avoid. > > > > I tried creating branches for those two commits and then did a subtree > > Since those two commits have parents that are found in git.git, you'll > pull the whole history of git.git if you try doing this. > > > pull, and that is what happened. The repo size went up from around 6M to > > 72M. Will cherry-picking avoid that? > > > > Yes, when you cherry-pick, you're essentially replaying the patch from > the old tree onto the new tree and recording a fresh commit from it. The > new commit is completely separate from the one it's based on so you > won't end up pulling in any ancestry information and, as a result, you > won't pull the rest of git's history. > > In any case, give it a try. It doesn't hurt to experiment and play > around with it. It works. Thanks. -- Regards, Pratyush Yadav
Re: git-gui: missing some patches from git?
On Thu, Sep 19, 2019 at 12:11:05PM -0700, Denton Liu wrote: > > > For the record, you could do a > > > > > > git cherry-pick -Xsubtree=git-gui 00ddc9d13c 7560f547e6 I forgot one more thing, if you end up doing this, you should probably sign-off on your cherry-picks by doing `-s`.
Re: git-gui: missing some patches from git?
On Fri, Sep 20, 2019 at 12:33:59AM +0530, Pratyush Yadav wrote: > On 19/09/19 11:47AM, Denton Liu wrote: > > On Fri, Sep 20, 2019 at 12:02:58AM +0530, Pratyush Yadav wrote: > > > Hi Junio, > > > > > > On 18/09/19 10:49AM, Junio C Hamano wrote: > > > > Pratyush Yadav writes: > > > > You should be able to merge this (and all other git-gui topics > > > > already in my tree Denton pointed out) to your 'master'. If you > > > > then make a trial merge of the result back into my tree with "git > > > > merge -Xsubtree=git-gui", it should result in "already up to date", > > > > i.e. a noop merge. > > > > > > I pulled all the changes into git-gui. I had to manually backport two > > > commits: > > > > > > * 7560f547e6 (treewide: correct several "up-to-date" to "up to date", * > > > 2017-08-23) > > > * 00ddc9d13c (Fix build with core.autocrlf=true, 2017-05-09) > > > > > > because they touched other parts of git, that were not in git-gui. > > > > > > > For the record, you could do a > > > > git cherry-pick -Xsubtree=git-gui 00ddc9d13c 7560f547e6 > > > > to bring them over instead of manually recreating the changes yourself. > > Personally, I'd prefer the cherry-picked commits as it'd preserve > > authorship information but I'm not sure how Junio feels. > > I'm not sure how this will work internally, but won't this also pull all > the ancestors of those commits into git-gui? That is bloat I'd rather > avoid. > > I tried creating branches for those two commits and then did a subtree Since those two commits have parents that are found in git.git, you'll pull the whole history of git.git if you try doing this. > pull, and that is what happened. The repo size went up from around 6M to > 72M. Will cherry-picking avoid that? > Yes, when you cherry-pick, you're essentially replaying the patch from the old tree onto the new tree and recording a fresh commit from it. The new commit is completely separate from the one it's based on so you won't end up pulling in any ancestry information and, as a result, you won't pull the rest of git's history. In any case, give it a try. It doesn't hurt to experiment and play around with it. > And if it won't, how about munging a patch created by format-patch to > get the authorship information without having to pull all the ancestors? > > > From a correctness perspective, however, I compared my results after > > doing that with yours and it's identical. > > > > If it looks all good, I'll put all this on my 'master' and re-send > > > the pull request. > > > > I took a look as well and the end result looks good to me too. > > Thanks. > > -- > Regards, > Pratyush Yadav
Re: git-gui: missing some patches from git?
On 19/09/19 11:47AM, Denton Liu wrote: > On Fri, Sep 20, 2019 at 12:02:58AM +0530, Pratyush Yadav wrote: > > Hi Junio, > > > > On 18/09/19 10:49AM, Junio C Hamano wrote: > > > Pratyush Yadav writes: > > > You should be able to merge this (and all other git-gui topics > > > already in my tree Denton pointed out) to your 'master'. If you > > > then make a trial merge of the result back into my tree with "git > > > merge -Xsubtree=git-gui", it should result in "already up to date", > > > i.e. a noop merge. > > > > I pulled all the changes into git-gui. I had to manually backport two > > commits: > > > > * 7560f547e6 (treewide: correct several "up-to-date" to "up to date", * > > 2017-08-23) > > * 00ddc9d13c (Fix build with core.autocrlf=true, 2017-05-09) > > > > because they touched other parts of git, that were not in git-gui. > > > > For the record, you could do a > > git cherry-pick -Xsubtree=git-gui 00ddc9d13c 7560f547e6 > > to bring them over instead of manually recreating the changes yourself. > Personally, I'd prefer the cherry-picked commits as it'd preserve > authorship information but I'm not sure how Junio feels. I'm not sure how this will work internally, but won't this also pull all the ancestors of those commits into git-gui? That is bloat I'd rather avoid. I tried creating branches for those two commits and then did a subtree pull, and that is what happened. The repo size went up from around 6M to 72M. Will cherry-picking avoid that? And if it won't, how about munging a patch created by format-patch to get the authorship information without having to pull all the ancestors? > From a correctness perspective, however, I compared my results after > doing that with yours and it's identical. > > If it looks all good, I'll put all this on my 'master' and re-send > > the pull request. > > I took a look as well and the end result looks good to me too. Thanks. -- Regards, Pratyush Yadav
Re: git-gui: missing some patches from git?
On Fri, Sep 20, 2019 at 12:02:58AM +0530, Pratyush Yadav wrote: > Hi Junio, > > On 18/09/19 10:49AM, Junio C Hamano wrote: > > Pratyush Yadav writes: > > You should be able to merge this (and all other git-gui topics > > already in my tree Denton pointed out) to your 'master'. If you > > then make a trial merge of the result back into my tree with "git > > merge -Xsubtree=git-gui", it should result in "already up to date", > > i.e. a noop merge. > > I pulled all the changes into git-gui. I had to manually backport two > commits: > > * 7560f547e6 (treewide: correct several "up-to-date" to "up to date", * > 2017-08-23) > * 00ddc9d13c (Fix build with core.autocrlf=true, 2017-05-09) > > because they touched other parts of git, that were not in git-gui. > For the record, you could do a git cherry-pick -Xsubtree=git-gui 00ddc9d13c 7560f547e6 to bring them over instead of manually recreating the changes yourself. Personally, I'd prefer the cherry-picked commits as it'd preserve authorship information but I'm not sure how Junio feels. >From a correctness perspective, however, I compared my results after doing that with yours and it's identical. > I then did a trial merge into your 'master', and running a diff, > git.git's version of git-gui is identical to mine. So everything seems > to be correct. > > Instead of doing this on my master, I did it on a separate branch just > to be sure I don't mess something up. You can find it on > https://github.com/prati0100/git-gui.git on the branch 'trial_merge'. > > These are the new commits introduced: > > 7435f916d3 git-gui: fix build with core.autocrlf=true > 834e3ec31e git-gui: correct "up-to-date" to "up to date" > fecfccf9ff Merge branches 'js/msgfmt-on-windows', 'tz/fsf-address-update', > 'jn/reproducible-build', 'ls/no-double-utf8-author-name', > 'js/misc-git-gui-stuff', 'bb/ssh-key-files', 'bp/bind-kp-enter', > 'cb/ttk-style' and 'py/call-do-quit-before-exit' of ../git into trial_merge > f7a8834ba4 Merge branch 'bp/amend-toggle-bind' > ec7424e1a6 git-gui: add hotkey to toggle "Amend Last Commit" > 6c8ec8c30a Merge branch 'bw/commit-scrollbuffer' > acfa495519 Merge branch 'bw/amend-checkbutton' > da08d559b7 git-gui: add horizontal scrollbar to commit buffer > ba41b5b335 git-gui: convert new/amend commit radiobutton to checkbutton > c77abf0460 Merge branch 'py/revert-hunks-lines' > 5a2bb62180 Merge branch 'bp/widget-focus-hotkeys' > e07446ed5f git-gui: add hotkeys to set widget focus > a4fa2f0a4c git-gui: allow undoing last revert > 2ccdfb1c78 git-gui: return early when patch fails to apply > 62bd99934b git-gui: allow reverting selected hunk > 5f0a516de9 git-gui: allow reverting selected lines > > The top 3 commits are of note. The top 2 are the manual backports. The > third is the octopus merge of all the topics not in git-gui yet. The > rest are just other topics that were introduced in my fork recently. > > If it looks all good, I'll put all this on my 'master' and re-send the > pull request. I took a look as well and the end result looks good to me too. > > -- > Regards, > Pratyush Yadav
Re: git-gui: missing some patches from git?
Hi Junio, On 18/09/19 10:49AM, Junio C Hamano wrote: > Pratyush Yadav writes: > You should be able to merge this (and all other git-gui topics > already in my tree Denton pointed out) to your 'master'. If you > then make a trial merge of the result back into my tree with "git > merge -Xsubtree=git-gui", it should result in "already up to date", > i.e. a noop merge. I pulled all the changes into git-gui. I had to manually backport two commits: * 7560f547e6 (treewide: correct several "up-to-date" to "up to date", * 2017-08-23) * 00ddc9d13c (Fix build with core.autocrlf=true, 2017-05-09) because they touched other parts of git, that were not in git-gui. I then did a trial merge into your 'master', and running a diff, git.git's version of git-gui is identical to mine. So everything seems to be correct. Instead of doing this on my master, I did it on a separate branch just to be sure I don't mess something up. You can find it on https://github.com/prati0100/git-gui.git on the branch 'trial_merge'. These are the new commits introduced: 7435f916d3 git-gui: fix build with core.autocrlf=true 834e3ec31e git-gui: correct "up-to-date" to "up to date" fecfccf9ff Merge branches 'js/msgfmt-on-windows', 'tz/fsf-address-update', 'jn/reproducible-build', 'ls/no-double-utf8-author-name', 'js/misc-git-gui-stuff', 'bb/ssh-key-files', 'bp/bind-kp-enter', 'cb/ttk-style' and 'py/call-do-quit-before-exit' of ../git into trial_merge f7a8834ba4 Merge branch 'bp/amend-toggle-bind' ec7424e1a6 git-gui: add hotkey to toggle "Amend Last Commit" 6c8ec8c30a Merge branch 'bw/commit-scrollbuffer' acfa495519 Merge branch 'bw/amend-checkbutton' da08d559b7 git-gui: add horizontal scrollbar to commit buffer ba41b5b335 git-gui: convert new/amend commit radiobutton to checkbutton c77abf0460 Merge branch 'py/revert-hunks-lines' 5a2bb62180 Merge branch 'bp/widget-focus-hotkeys' e07446ed5f git-gui: add hotkeys to set widget focus a4fa2f0a4c git-gui: allow undoing last revert 2ccdfb1c78 git-gui: return early when patch fails to apply 62bd99934b git-gui: allow reverting selected hunk 5f0a516de9 git-gui: allow reverting selected lines The top 3 commits are of note. The top 2 are the manual backports. The third is the octopus merge of all the topics not in git-gui yet. The rest are just other topics that were introduced in my fork recently. If it looks all good, I'll put all this on my 'master' and re-send the pull request. -- Regards, Pratyush Yadav
Re: git-gui: missing some patches from git?
On 18/09/19 10:49AM, Junio C Hamano wrote: > Pratyush Yadav writes: > > > Assuming I have git.git cloned in ../git (relative to git-gui.git), I > > ran: > > > > git pull -Xsubtree=git-gui ../git $branches > > > > instead of: > > > > git merge $branches > > > > because git-gui's tree doesn't have those commits and branches yet, so > > we can't merge straight away. This seems to have worked, but I thought > > I'd mention it in case it would cause some subtle problems. > > I am not sure what your $branches above are referring to, but if you The $branches is from Denton's script (its in his email that I replied to) that lists all the branches merged into git/git-gui that are not in git-gui. So it should essentially be all the branches not in my tree that should be. > run "git log" in the resulting git-gui repository and see any of the > commits that are about git and not git-gui, then it is not a subtle > but a grave problem. The reason why git-gui and gitk are separate > projects is because they do not want to contaminate their history > with stuff that are not related to them (e.g. changes to "git > fast-export"). No, I was only pulling in merges related to git-gui into my tree. I do not intend to pull in any other changes. > Taking one of the commits Denton found as an example: > > >>js/misc-git-gui-stuff: 76756d67061076c046973bff2089ad49f5dc2eb6 > > (by the way, this can also be found by running > > git log --first-parent --min-parents=2 master -- git-gui Yes, that is what I was doing. Denton's script just made the tedious task of finding all those out easier. > in git itself; 02a5f25d956be187bc0 merged it in as its second > parent. The second parents of these merges you would find are > candidates---you may already have some, and you may need to merge > others to your tree; there may be some that are direct updates to > git-gui/ part of my tree (which shouldn't have been done but > mistakes happen). > > You should be able to merge this (and all other git-gui topics > already in my tree Denton pointed out) to your 'master'. If you > then make a trial merge of the result back into my tree with "git > merge -Xsubtree=git-gui", it should result in "already up to date", > i.e. a noop merge. Well, technically it won't be a noop merge because my master has some changes that haven't been pulled by you yet, but I get the point. Thanks. I'll test this out. -- Regards, Pratyush Yadav
Re: git-gui: missing some patches from git?
Pratyush Yadav writes: > Assuming I have git.git cloned in ../git (relative to git-gui.git), I > ran: > > git pull -Xsubtree=git-gui ../git $branches > > instead of: > > git merge $branches > > because git-gui's tree doesn't have those commits and branches yet, so > we can't merge straight away. This seems to have worked, but I thought > I'd mention it in case it would cause some subtle problems. I am not sure what your $branches above are referring to, but if you run "git log" in the resulting git-gui repository and see any of the commits that are about git and not git-gui, then it is not a subtle but a grave problem. The reason why git-gui and gitk are separate projects is because they do not want to contaminate their history with stuff that are not related to them (e.g. changes to "git fast-export"). Taking one of the commits Denton found as an example: >> js/misc-git-gui-stuff: 76756d67061076c046973bff2089ad49f5dc2eb6 (by the way, this can also be found by running git log --first-parent --min-parents=2 master -- git-gui in git itself; 02a5f25d956be187bc0 merged it in as its second parent. The second parents of these merges you would find are candidates---you may already have some, and you may need to merge others to your tree; there may be some that are direct updates to git-gui/ part of my tree (which shouldn't have been done but mistakes happen). You should be able to merge this (and all other git-gui topics already in my tree Denton pointed out) to your 'master'. If you then make a trial merge of the result back into my tree with "git merge -Xsubtree=git-gui", it should result in "already up to date", i.e. a noop merge. Thanks.
Re: git-gui: missing some patches from git?
Birger Skogeng Pedersen writes: > For example, I created a patch back in March 2018[3]. Junio pulled it > so the changes are really there in git/git-gui/git-gui.sh (see this[4] > line). This was while there was no git-gui maintainer. I guess the > change never got merged to git-gui, but directly to git. Thanks for noticing. I do recall forking Pat's tree, applying a few patches and pulling the result to git itself while asking Pat to pull from me to get these patches. Perhaps these were not merged back before Pratyush inherited the tree.
Re: git-gui: missing some patches from git?
On Wed, Sep 18, 2019 at 08:44:04PM +0530, Pratyush Yadav wrote: > On 18/09/19 02:27AM, Denton Liu wrote: > > On Wed, Sep 18, 2019 at 09:02:37AM +0200, Birger Skogeng Pedersen wrote: > > > Hi Pratyush, > > > > > > > > > I was comparing your git-gui repo[1] with the source code of > > > git/git-gui[2]. There seems to be a couple of things missing. > > > > > > For example, I created a patch back in March 2018[3]. Junio pulled it > > > so the changes are really there in git/git-gui/git-gui.sh (see this[4] > > > line). This was while there was no git-gui maintainer. I guess the > > > change never got merged to git-gui, but directly to git. > > > > > > Not sure what you should to about it, I just wanted to let you know. > > This is something I've been aware of, but I have followed the strategy > of ignoring the problem till someone complains. Well, that someone has > now complained. > > I'm not particularly comfortable with cross-tree/sub-tree merges, so > I've been dreading doing this for a while. Guess now its time to get my > hands dirty. > > > > > > > [1] https://github.com/prati0100/git-gui > > > [2] https://github.com/gitster/git/tree/master/git-gui > > > [3] > > > https://public-inbox.org/git/20180302100148.23899-1-birge...@gmail.com/ > > > [4] https://github.com/gitster/git/blob/master/git-gui/git-gui.sh#L3885 > > > > > > > > > Birger > > > > As an exercise in writing throwaway scripts, I created this monstrosity. > > If you're interested in merging all of the git-gui branches that came > > from mainline back into git-gui's master, perhaps we could do something > > like this: > > Ah! Thanks a lot for this. This reduces some of the work I've been > dreading. > > > > > #!/bin/sh > > > > branches= > > # note that all instances of "master" refer to git.git's "master" > > # also, 5ab7227 is the latest commit in Pat's git-gui repo > > for c in $(git rev-list --children master 5ab7227 | grep ^5ab7227 | cut > > -d' ' -f2-) > > do > > merge_commit=$(git rev-list $c..master --ancestry-path --merges > > | tail -n1) > > branch_name=$(git show -s --format=%s $merge_commit | sed -e > > "s/Merge branch '\\([^']*\\)' of .*/\\1/") > > > > #echo $branch_name: $(git rev-parse $merge_commit^2) > > git branch -f "$branch_name" $merge_commit^2 > > branches="$branches $branch_name" > > done > > # this also assumes git-gui's master is checked out > > git merge $branches > > Assuming I have git.git cloned in ../git (relative to git-gui.git), I > ran: > > git pull -Xsubtree=git-gui ../git $branches > > instead of: > > git merge $branches > > because git-gui's tree doesn't have those commits and branches yet, so > we can't merge straight away. This seems to have worked, but I thought > I'd mention it in case it would cause some subtle problems. Did you run the big for loop in git.git to create the branches and then the `git pull` in git-gui? That should work. > > > This script should resurrect all of the branches that were based on > > 5ab7227 from mainline's master. Then (assuming you have git-gui's > > master checked out), it should do a big octopus merge to bring all of > > the changes in. > > > > We end up with the following branches being merged: > > > > js/msgfmt-on-windows: 492595cfc70f97cd99d4c460db1ba01b73dab932 > > This branch is already in git-gui (with the exception of one commit. > More on that below). > > > tz/fsf-address-update: 63100874c1653dd6a137f74143eda322550eabc7 > > jn/reproducible-build: 474642b4a47c74a1f277955d7387d1886546fa01 > > ls/no-double-utf8-author-name: 331450f18a7fd298ddd6b85cc5e8ed9dba09f9da > > js/misc-git-gui-stuff: 76756d67061076c046973bff2089ad49f5dc2eb6 > > bb/ssh-key-files: 6a47fa0efa342daa53c6386538fda313420351a5 > > bp/bind-kp-enter: 146a6f1097f451c6b6d332916a515b7ce8c07e9a > > cb/ttk-style: f50d5055bf9bb2aa35e629d31943334afc4a9f10 > > py/call-do-quit-before-exit: 5440eb0ea2651c45a0e46f2335ecbb8d1f42c584 > > > > Then perhaps you could do a request-pull and development could continue > > on your fork? > > > > Not sure if this is even desirable but here's the script just in case it > > ends up useful. I had fun writing it. > > Just to pick your brain: in what case would this not be desirable? I'm not 100% about the accuracy of the script. In particular, merge_commit=$(git rev-list $c..master --ancestry-path --merges | tail -n1) is super hacky and makes a lot of assumptions. It'd probably be best if someone else also takes a look. > > > Also note that we end up missing two commits that made changes to > > git-gui/ under mainline git (not directly to the git-gui repo): > > > > * 7560f547e6 (treewide: correct several "up-to-date" to "up to date", > > 2017-08-23) > > * 00ddc9d13c (Fix build with core.autocrlf=true, 2017-05-09) > > One more commit that is missing: 492595cfc7 (git-gui (MinGW):
Re: git-gui: missing some patches from git?
On 18/09/19 02:27AM, Denton Liu wrote: > On Wed, Sep 18, 2019 at 09:02:37AM +0200, Birger Skogeng Pedersen wrote: > > Hi Pratyush, > > > > > > I was comparing your git-gui repo[1] with the source code of > > git/git-gui[2]. There seems to be a couple of things missing. > > > > For example, I created a patch back in March 2018[3]. Junio pulled it > > so the changes are really there in git/git-gui/git-gui.sh (see this[4] > > line). This was while there was no git-gui maintainer. I guess the > > change never got merged to git-gui, but directly to git. > > > > Not sure what you should to about it, I just wanted to let you know. This is something I've been aware of, but I have followed the strategy of ignoring the problem till someone complains. Well, that someone has now complained. I'm not particularly comfortable with cross-tree/sub-tree merges, so I've been dreading doing this for a while. Guess now its time to get my hands dirty. > > > > [1] https://github.com/prati0100/git-gui > > [2] https://github.com/gitster/git/tree/master/git-gui > > [3] https://public-inbox.org/git/20180302100148.23899-1-birge...@gmail.com/ > > [4] https://github.com/gitster/git/blob/master/git-gui/git-gui.sh#L3885 > > > > > > Birger > > As an exercise in writing throwaway scripts, I created this monstrosity. > If you're interested in merging all of the git-gui branches that came > from mainline back into git-gui's master, perhaps we could do something > like this: Ah! Thanks a lot for this. This reduces some of the work I've been dreading. > > #!/bin/sh > > branches= > # note that all instances of "master" refer to git.git's "master" > # also, 5ab7227 is the latest commit in Pat's git-gui repo > for c in $(git rev-list --children master 5ab7227 | grep ^5ab7227 | cut > -d' ' -f2-) > do > merge_commit=$(git rev-list $c..master --ancestry-path --merges > | tail -n1) > branch_name=$(git show -s --format=%s $merge_commit | sed -e > "s/Merge branch '\\([^']*\\)' of .*/\\1/") > > #echo $branch_name: $(git rev-parse $merge_commit^2) > git branch -f "$branch_name" $merge_commit^2 > branches="$branches $branch_name" > done > # this also assumes git-gui's master is checked out > git merge $branches Assuming I have git.git cloned in ../git (relative to git-gui.git), I ran: git pull -Xsubtree=git-gui ../git $branches instead of: git merge $branches because git-gui's tree doesn't have those commits and branches yet, so we can't merge straight away. This seems to have worked, but I thought I'd mention it in case it would cause some subtle problems. > This script should resurrect all of the branches that were based on > 5ab7227 from mainline's master. Then (assuming you have git-gui's > master checked out), it should do a big octopus merge to bring all of > the changes in. > > We end up with the following branches being merged: > > js/msgfmt-on-windows: 492595cfc70f97cd99d4c460db1ba01b73dab932 This branch is already in git-gui (with the exception of one commit. More on that below). > tz/fsf-address-update: 63100874c1653dd6a137f74143eda322550eabc7 > jn/reproducible-build: 474642b4a47c74a1f277955d7387d1886546fa01 > ls/no-double-utf8-author-name: 331450f18a7fd298ddd6b85cc5e8ed9dba09f9da > js/misc-git-gui-stuff: 76756d67061076c046973bff2089ad49f5dc2eb6 > bb/ssh-key-files: 6a47fa0efa342daa53c6386538fda313420351a5 > bp/bind-kp-enter: 146a6f1097f451c6b6d332916a515b7ce8c07e9a > cb/ttk-style: f50d5055bf9bb2aa35e629d31943334afc4a9f10 > py/call-do-quit-before-exit: 5440eb0ea2651c45a0e46f2335ecbb8d1f42c584 > > Then perhaps you could do a request-pull and development could continue > on your fork? > > Not sure if this is even desirable but here's the script just in case it > ends up useful. I had fun writing it. Just to pick your brain: in what case would this not be desirable? > Also note that we end up missing two commits that made changes to > git-gui/ under mainline git (not directly to the git-gui repo): > > * 7560f547e6 (treewide: correct several "up-to-date" to "up to date", > 2017-08-23) > * 00ddc9d13c (Fix build with core.autocrlf=true, 2017-05-09) One more commit that is missing: 492595cfc7 (git-gui (MinGW): make use of MSys2's msgfmt, 2017-07-25) This commit is comes from the merge of js/msgfmt-on-windows, which has all the commits from the merge 5ab7227 (Merge remote-tracking branch 'philoakley/dup-gui', 2017-03-18) in git-gui. While merging js/msgfmt-on-windows should get this commit into git-gui, I'd rather have it separate, > > Hope any of this is useful to anyone, It is very useful. Thanks :) -- Regards, Pratyush Yadav
Re: git-gui: missing some patches from git?
On Wed, Sep 18, 2019 at 09:02:37AM +0200, Birger Skogeng Pedersen wrote: > Hi Pratyush, > > > I was comparing your git-gui repo[1] with the source code of > git/git-gui[2]. There seems to be a couple of things missing. > > For example, I created a patch back in March 2018[3]. Junio pulled it > so the changes are really there in git/git-gui/git-gui.sh (see this[4] > line). This was while there was no git-gui maintainer. I guess the > change never got merged to git-gui, but directly to git. > > Not sure what you should to about it, I just wanted to let you know. > > [1] https://github.com/prati0100/git-gui > [2] https://github.com/gitster/git/tree/master/git-gui > [3] https://public-inbox.org/git/20180302100148.23899-1-birge...@gmail.com/ > [4] https://github.com/gitster/git/blob/master/git-gui/git-gui.sh#L3885 > > > Birger As an exercise in writing throwaway scripts, I created this monstrosity. If you're interested in merging all of the git-gui branches that came from mainline back into git-gui's master, perhaps we could do something like this: #!/bin/sh branches= # note that all instances of "master" refer to git.git's "master" # also, 5ab7227 is the latest commit in Pat's git-gui repo for c in $(git rev-list --children master 5ab7227 | grep ^5ab7227 | cut -d' ' -f2-) do merge_commit=$(git rev-list $c..master --ancestry-path --merges | tail -n1) branch_name=$(git show -s --format=%s $merge_commit | sed -e "s/Merge branch '\\([^']*\\)' of .*/\\1/") #echo $branch_name: $(git rev-parse $merge_commit^2) git branch -f "$branch_name" $merge_commit^2 branches="$branches $branch_name" done # this also assumes git-gui's master is checked out git merge $branches This script should resurrect all of the branches that were based on 5ab7227 from mainline's master. Then (assuming you have git-gui's master checked out), it should do a big octopus merge to bring all of the changes in. We end up with the following branches being merged: js/msgfmt-on-windows: 492595cfc70f97cd99d4c460db1ba01b73dab932 tz/fsf-address-update: 63100874c1653dd6a137f74143eda322550eabc7 jn/reproducible-build: 474642b4a47c74a1f277955d7387d1886546fa01 ls/no-double-utf8-author-name: 331450f18a7fd298ddd6b85cc5e8ed9dba09f9da js/misc-git-gui-stuff: 76756d67061076c046973bff2089ad49f5dc2eb6 bb/ssh-key-files: 6a47fa0efa342daa53c6386538fda313420351a5 bp/bind-kp-enter: 146a6f1097f451c6b6d332916a515b7ce8c07e9a cb/ttk-style: f50d5055bf9bb2aa35e629d31943334afc4a9f10 py/call-do-quit-before-exit: 5440eb0ea2651c45a0e46f2335ecbb8d1f42c584 Then perhaps you could do a request-pull and development could continue on your fork? Not sure if this is even desirable but here's the script just in case it ends up useful. I had fun writing it. Also note that we end up missing two commits that made changes to git-gui/ under mainline git (not directly to the git-gui repo): * 7560f547e6 (treewide: correct several "up-to-date" to "up to date", 2017-08-23) * 00ddc9d13c (Fix build with core.autocrlf=true, 2017-05-09) Hope any of this is useful to anyone, Denton
git-gui: missing some patches from git?
Hi Pratyush, I was comparing your git-gui repo[1] with the source code of git/git-gui[2]. There seems to be a couple of things missing. For example, I created a patch back in March 2018[3]. Junio pulled it so the changes are really there in git/git-gui/git-gui.sh (see this[4] line). This was while there was no git-gui maintainer. I guess the change never got merged to git-gui, but directly to git. Not sure what you should to about it, I just wanted to let you know. [1] https://github.com/prati0100/git-gui [2] https://github.com/gitster/git/tree/master/git-gui [3] https://public-inbox.org/git/20180302100148.23899-1-birge...@gmail.com/ [4] https://github.com/gitster/git/blob/master/git-gui/git-gui.sh#L3885 Birger
Re: [PATCH v2 1/2] diff, log doc: say "patch text" instead of "patches"
On Mon, 16 Sep 2019 at 22:46, Johannes Sixt wrote: > > Am 16.09.19 um 21:58 schrieb Junio C Hamano: > > I wonder if the result becomes even clearer if we dropped "instead > > of the usual output". It is a given that presence of an option > > would change the behaviour, so "instead of the usual" does not add > > any value in the context of the explanation we are giving. > > Agreed. > > > Also I question the value of the "running git diff without --raw > > option" sentence; "diff --stat" is also a way to suppress the patch > > text and see only the overview; I know it is not a new problem this > > patch introduces, but the objective of this patch is clarify about > > the generation of output in patch format, so... > > You have a point here, too. > > Below is v2 of just patch 1/2. 2/2 remains unchanged. I've added > git-show to the enumeration. Yeah, this makes sense. Tested -- this renders fine. Martin
[PATCH v2 1/2] diff, log doc: say "patch text" instead of "patches"
Am 16.09.19 um 21:58 schrieb Junio C Hamano: > I wonder if the result becomes even clearer if we dropped "instead > of the usual output". It is a given that presence of an option > would change the behaviour, so "instead of the usual" does not add > any value in the context of the explanation we are giving. Agreed. > Also I question the value of the "running git diff without --raw > option" sentence; "diff --stat" is also a way to suppress the patch > text and see only the overview; I know it is not a new problem this > patch introduces, but the objective of this patch is clarify about > the generation of output in patch format, so... You have a point here, too. Below is v2 of just patch 1/2. 2/2 remains unchanged. I've added git-show to the enumeration. --- 8< --- diff, log doc: say "patch text" instead of "patches" A poster on Stackoverflow was confused that the documentation of git-log promised to generate "patches" or "patch files" with -p, but there were none to be found. Rewrite the corresponding paragraph to talk about "patch text" to avoid the confusion. Shorten the language to say "X does Y" in place of "X does not Z, but Y". Cross-reference the referred-to commands like the rest of the file does. Enumerate git-show because it includes the description as well. Mention porcelain commands before plumbing commands because I guess that the paragraph is read more frequently in their context. Signed-off-by: Johannes Sixt --- Documentation/diff-generate-patch.txt | 20 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/Documentation/diff-generate-patch.txt b/Documentation/diff-generate-patch.txt index f10ca410ad..c482c94e6b 100644 --- a/Documentation/diff-generate-patch.txt +++ b/Documentation/diff-generate-patch.txt @@ -1,11 +1,15 @@ -Generating patches with -p --- - -When "git-diff-index", "git-diff-tree", or "git-diff-files" are run -with a `-p` option, "git diff" without the `--raw` option, or -"git log" with the "-p" option, they -do not produce the output described above; instead they produce a -patch file. You can customize the creation of such patches via the +Generating patch text with -p +- + +Running +linkgit:git-diff[1], +linkgit:git-log[1], +linkgit:git-show[1], +linkgit:git-diff-index[1], +linkgit:git-diff-tree[1], or +linkgit:git-diff-files[1] +with the `-p` option produces patch text. +You can customize the creation of patch text via the `GIT_EXTERNAL_DIFF` and the `GIT_DIFF_OPTS` environment variables. What the -p option produces is slightly different from the traditional -- 2.23.0.95.gcfbaf7d16f
Re: [PATCH 1/2] diff, log doc: say "patch text" instead of "patches"
Martin Ågren writes: > On Sun, 15 Sep 2019 at 21:26, Johannes Sixt wrote: >> I do not have the toolchain to check that a correct result is produced. > > But I do. I've tested this patch and 2/2 with AsciiDoc 8.6.10 and > Asciidoctor 1.5.5, as well as with Asciidoctor 2.0.10 (on top of brian's > recent patch so that it builds to begin with). They all render this > nicely. > > Both of these patches seem like good changes to me. Thanks, both. I am neutral between "patch" and "patch text", but if the latter is more easily understood by readers, that would be great. "patch *file*" certainly does sound misleading. I wonder if the result becomes even clearer if we dropped "instead of the usual output". It is a given that presence of an option would change the behaviour, so "instead of the usual" does not add any value in the context of the explanation we are giving. Also I question the value of the "running git diff without --raw option" sentence; "diff --stat" is also a way to suppress the patch text and see only the overview; I know it is not a new problem this patch introduces, but the objective of this patch is clarify about the generation of output in patch format, so...
Re: [PATCH 1/2] diff, log doc: say "patch text" instead of "patches"
On Sun, 15 Sep 2019 at 21:26, Johannes Sixt wrote: > I do not have the toolchain to check that a correct result is produced. But I do. I've tested this patch and 2/2 with AsciiDoc 8.6.10 and Asciidoctor 1.5.5, as well as with Asciidoctor 2.0.10 (on top of brian's recent patch so that it builds to begin with). They all render this nicely. Both of these patches seem like good changes to me. Martin
[PATCH 1/2] diff, log doc: say "patch text" instead of "patches"
A poster on Stackoverflow was confused that the documentation of git-log promised to generate "patches" or "patch files" with -p, but there were none to be found. Rewrite the corresponding paragraph to talk about "patch text" to avoid the confusion. Shorten the language to say "X does Y" in place of "X does not Z, but Y". Cross-reference the referred-to commands like the rest of the file does. Mention porcelain commands before plumbing commands because I guess that the paragraph is read more frequently in their context. Signed-off-by: Johannes Sixt --- I do not have the toolchain to check that a correct result is produced. Documentation/diff-generate-patch.txt | 17 + 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/Documentation/diff-generate-patch.txt b/Documentation/diff-generate-patch.txt index f10ca410ad..c6bbb2ac22 100644 --- a/Documentation/diff-generate-patch.txt +++ b/Documentation/diff-generate-patch.txt @@ -1,11 +1,12 @@ -Generating patches with -p --- - -When "git-diff-index", "git-diff-tree", or "git-diff-files" are run -with a `-p` option, "git diff" without the `--raw` option, or -"git log" with the "-p" option, they -do not produce the output described above; instead they produce a -patch file. You can customize the creation of such patches via the +Generating patch text with -p +- + +Running +linkgit:git-diff[1] without the `--raw` option, +or linkgit:git-log[1], linkgit:git-diff-index[1], linkgit:git-diff-tree[1], +or linkgit:git-diff-files[1] with the `-p` option +produces patch text instead of the usual output. +You can customize the creation of patch text via the `GIT_EXTERNAL_DIFF` and the `GIT_DIFF_OPTS` environment variables. What the -p option produces is slightly different from the traditional -- 2.23.0.93.g91d3f15def
Re: How to report issues or provide patches for the documentation IN OTHER LANGUAGES?
Robin Kuzmin writes: > At this page (in Russian) > > https://git-scm.com/book/ru/v2/%D0%98%D0%BD%D1%81%D1%82%D1%80%D1%83%D0%BC%D0%B5%D0%BD%D1%82%D1%8B-Git-%D0%9F%D0%BE%D0%B4%D0%BC%D0%BE%D0%B4%D1%83%D0%BB%D0%B8 > I see: > вы рассмотрим > I expected: > мы рассмотрим > And I fail to find the exact file to fix. I fail to find a clear > description of how to provide a fix/pull-request (or a least to report > the issue). I'll only respond to the "Subject" as I am not involved in that book project. 1. Visit https://git-scm.com/book/ and find "The source of this book is hosted on GitHub. Patches, suggestions and comments are welcome." 'hosted on GitHub' has link. Click. 2. We are taken to github.com/progit/progit2; among the list of files, I see CONTRIBUTING.md, which sounds promising. Click. 3. Very helpful notes about how the project expects contributions to them appear; at the end, there is Translations section, with a link to TRANSLATING.md. Click. 4. There seems to be a table that maps language to a separate repository. -ru seems to map to https://github.com/progit/progit2-ru but I cannot even read the language and cannot grok the left hand side column of the table where it has the name of the language X-<. Just guess, have faith and click. 5. There is a search box at the top of the page. I blindly copy and paste "вы рассмотрим" from your message and click "In this repository". Then I change my mind and enclose the two-word search term inside a pair of double quotes and do the same. 6. I find only one instance of the combination of two words in the result. I cannot read/understand much of the things I see during this journey, but hopefully I helped you a bit?
How to report issues or provide patches for the documentation IN OTHER LANGUAGES?
At this page (in Russian) https://git-scm.com/book/ru/v2/%D0%98%D0%BD%D1%81%D1%82%D1%80%D1%83%D0%BC%D0%B5%D0%BD%D1%82%D1%8B-Git-%D0%9F%D0%BE%D0%B4%D0%BC%D0%BE%D0%B4%D1%83%D0%BB%D0%B8 I see: вы рассмотрим I expected: мы рассмотрим And I fail to find the exact file to fix. I fail to find a clear description of how to provide a fix/pull-request (or a least to report the issue). --- Join us at CppCon 2019 in September! Robin Kuzmin, CppCon 2019 Speaker Liaison, speak...@cppcon.org. (Please allow 2 - 4 days for my reply) Robin Kuzmin, kuzmin.ro...@gmail.com.
Re: Where do I send patches for git-gui?
On 25/07/19 5:04 PM, Christian Couder wrote: Hi Pratyush, On Wed, Jul 24, 2019 at 11:43 PM Pratyush Yadav wrote: I have a quick little feature to add to git-gui, and I'm wondering where should I discuss it and send patches. The git-gui repo [0] has no readme I can see that would point me in the right direction. Googling around didn't get me anything either. Should I send it here on this list or is it somewhere else? It seems to me that people have been sending patches to git-gui to this mailing list indeed. All right, I'll send it here. According to the following discussions: - https://public-inbox.org/git/xmqqbm36w7hl@gitster-ct.c.googlers.com/ - https://public-inbox.org/git/nycvar.qro.7.76.6.1905272135280.28...@tvgsbejvaqbjf.bet/ Pat Thoyts (in CC) used to be the git-gui maintainer but we haven't been hearing from him for a long time, so we are looking for a new maintainer. Ah, that's too bad that there is no active maintainer. I recently switched my text editor from Atom to Vim, and the only thing I am missing is Atom's gui for looking at diffs and resolving merge conflicts. I've only used git-gui for a couple days, but it looks like just the tool I need. I can maintain it (as long as I'm using the tool at least). I'm not particularly fluent with Tcl or Tk, but I'm learning them as I implement the features I need. Will I get commit access to the main git repo, or should I use a fork and send the changes over here, and one of you folks will merge them? Also, is the project even actively maintained any more? The last commit was in 2017. -- Regards, Pratyush Yadav
Re: Where do I send patches for git-gui?
Hi, On Wed, 24 Jul 2019, Pratyush Yadav wrote: > I have a quick little feature to add to git-gui, and I'm wondering > where should I discuss it and send patches. The git-gui repo [0] has > no readme I can see that would point me in the right direction. > Googling around didn't get me anything either. > > Should I send it here on this list or is it somewhere else? > > Also, is the project even actively maintained any more? The last commit was in > 2017. > > [0] http://repo.or.cz/w/git-gui.git/ Sadly, it seems that we do not have any active Git GUI maintainer, indeed. The latest repository for Git GUI seems to have been https://github.com/patthoyts/git-gui, but even that has fallen dormant. Your best bet is to send the patch to this list, and we'll just have to try to work together to our best abilities to review and improve and integrate it. Ciao, Johannes
Re: Where do I send patches for git-gui?
Hi Pratyush, On Wed, Jul 24, 2019 at 11:43 PM Pratyush Yadav wrote: > > I have a quick little feature to add to git-gui, and I'm wondering where > should I discuss it and send patches. The git-gui repo [0] has no readme > I can see that would point me in the right direction. Googling around > didn't get me anything either. > > Should I send it here on this list or is it somewhere else? It seems to me that people have been sending patches to git-gui to this mailing list indeed. According to the following discussions: - https://public-inbox.org/git/xmqqbm36w7hl@gitster-ct.c.googlers.com/ - https://public-inbox.org/git/nycvar.qro.7.76.6.1905272135280.28...@tvgsbejvaqbjf.bet/ Pat Thoyts (in CC) used to be the git-gui maintainer but we haven't been hearing from him for a long time, so we are looking for a new maintainer. > Also, is the project even actively maintained any more? The last commit > was in 2017.
Where do I send patches for git-gui?
Hi, I have a quick little feature to add to git-gui, and I'm wondering where should I discuss it and send patches. The git-gui repo [0] has no readme I can see that would point me in the right direction. Googling around didn't get me anything either. Should I send it here on this list or is it somewhere else? Also, is the project even actively maintained any more? The last commit was in 2017. [0] http://repo.or.cz/w/git-gui.git/ -- Regards, Pratyush Yadav
Re: rebase drops patches that have since been reverted
Am 12.06.19 um 17:03 schrieb Shawn Landden: > If a patch has been applied upstream AND THEN reverted, rebase still > drops the patch, requiring the use of relative rebase git rebase -i > HEAD~5 et cetera. > > git rebase should detect reverts as well. You have the same patch that upstream has. Perhaps you cherry-picked it from them, or they cherry-picked it from you. In any case, they thought the patch was a good one. Later upstream found a problem with the patch and decided to revert it. Certainly, they didn't do this just for fun; they had a good reason. Now you are rebasing against upstream. IOW, you are accepting the authority of the upstream code. Then, why on earth would you not want to accept their authorative word that the cherry-picked patch was bad and needed to be reverted? If you later find out that your patch is definitely needed, then it is on you to prove that, for example, by including it again in your patch series. That must be an explicit decision on your part, not an accident that happens. TLDR: Git worked as designed. -- Hannes
rebase drops patches that have since been reverted
If a patch has been applied upstream AND THEN reverted, rebase still drops the patch, requiring the use of relative rebase git rebase -i HEAD~5 et cetera. git rebase should detect reverts as well. -Shawn Landden
Re: separating regression test patches from fixes, was Re: [PATCH 3/3] cherry-pick --continue: remember options
Hi Junio, On Mon, 18 Mar 2019, Junio C Hamano wrote: > Duy Nguyen writes: > > > On Thu, Mar 14, 2019 at 9:10 PM Johannes Schindelin > > wrote: > >> In any case, before we get better tooling to work around these issues, I > >> still think it makes a ton of sense to encourage proper separation of > >> concerns: to keep patches that introduce regression tests demonstrating a > >> breakage separate from patches that fix the breakage. It would certainly > >> help me (e.g. when staring at a range diff). > > > > Then perhaps improve the tools now because these separate patches > > enter 'master' and stay in the history forever. One of the problems I > > have with separating tests from the actual code change is the > > description of the problem often stays on the test commit, which I > > can't see in git-log if I'm searching for the code change. And no > > sometimes I can't just look at the parent commit if I filter code by > > path (and with --full-diff) > > I do not think the phrase "separation of concerns" is applied > correctly here. You might currently still think so, so let me give you material to re-think that stance: - If you truly care about contributors of all stripes, you also care about contributors who report bugs, contributors who turn bug reports into reproducible regression test cases, contributors who verify regressions, etc. And if you really want to give your respect to, say, the contributors who provide such regression test cases, you will accept them as full commit authors. And that fact alone makes it a separate concern, notwithstanding that in my case, the contributor who wrote the regression test case and the contributor who fixed the regression happened to be the same person: me. That fact simply does not invalidate that there are different concerns at play. You probably even know people who are good at one of those concerns, but not at the other. - If it true what you say, that a bug fix should be in the same commit as the regression test case designed to prevent the bug from being reintroduced, then both should live in the same file, because it is really the same concern. But test code lives in a separate area of the source code, is not even "shipped with the product", *because* it is a separate concern. - There is an entire industry built on books and training advertising Test Driven Development (TDD). If you really believe what you said, that regression tests are really the same concern as fixing the regressions, then you claim that all of them are wrong. Which would be a, how can I say it, erm, bold statement indeed. - You did brush aside the example I gave: Hannes Sixt applied a patch that demonstrated the breakage, or more correctly: that *should* have demonstrated a breakage. Let's not brush aside this example. Let's not ignore it just like that. This commit addressed a very specific concern: it verified that there is a breakage. Or actually, that in this case, there was *no* breakage. By not separating those concerns, as you would have me do it, I would have made it harder to demonstrate that this bug fix was not even necessary in git.git's `master`. I would have made it harder to bisect *where* in Git for Windows' patch thicket the regression was introduced. You might now say that it should have been easy to separate the concern after the fact, to extract it from a commit that adds both fix and regression test case. But by that token, you should squash all contributions into a single, big, honking commit that do way too much "because it is easy to extract what you need". I agree, that sounds ridiculous. And even saying that it would be easy to extract the test case from the commit already admits that there is a good chunk that you can extract *that can live on its own*, i.e. is a separate concern. - We introduced, specifically, the shell function `test_expect_failure` to allow demonstrating breakages. If we really thought that that has no value, if there should not be any commit that introduces a regression test case without fixing any bug at the same time, if that was not a concern, then we should get rid of that function. But experience tells us, of course, that we need to keep it. - There is *semantic* meaning in `test_expect_failure`. Depriving the commit history from the commits that introduce such test cases demonstrating bugs, and from commits that fix those bugs, makes it impossible to perform otherwise simple queries along the lines "how many regressions did fix in September 2017?". I know you are a fan of software archaeology as much as I am (maybe even a bit more), so hopefully you find at least this argument convincing. > The concern of
Re: separating regression test patches from fixes, was Re: [PATCH 3/3] cherry-pick --continue: remember options
Duy Nguyen writes: > One of the problems I > have with separating tests from the actual code change is the > description of the problem often stays on the test commit, which I > can't see in git-log if I'm searching for the code change. In the message you are reponding to, Dscho made it sound as if I am reviewing only from my MUA, but most of my reviews are done after the patches are tentatively applied (it is a separate issue if the result is found worth keeping and merged to 'pu'), so our workflows are not so different. It is not like "must have them separate" is the need shared among those who prefer to review in-tree. I do not want a logically single patch split into two. And I find your "find the other half" of a pair of patch that is artificially split into two a real problem for me, too. If you split a single patch into two, depending on which half you find first, finding the other half is either trivial (as you can just follow the parent pointer to $THAT_THING^) or hard (you'd need to have something that binds everything together, like 'pu', and grep for "git log ..pu" trying to find what its most relevant child is).
Re: separating regression test patches from fixes, was Re: [PATCH 3/3] cherry-pick --continue: remember options
Duy Nguyen writes: > On Thu, Mar 14, 2019 at 9:10 PM Johannes Schindelin > wrote: >> In any case, before we get better tooling to work around these issues, I >> still think it makes a ton of sense to encourage proper separation of >> concerns: to keep patches that introduce regression tests demonstrating a >> breakage separate from patches that fix the breakage. It would certainly >> help me (e.g. when staring at a range diff). > > Then perhaps improve the tools now because these separate patches > enter 'master' and stay in the history forever. One of the problems I > have with separating tests from the actual code change is the > description of the problem often stays on the test commit, which I > can't see in git-log if I'm searching for the code change. And no > sometimes I can't just look at the parent commit if I filter code by > path (and with --full-diff) I do not think the phrase "separation of concerns" is applied correctly here. The concern of a simple single-patch is to fix a bug---the test that shows what bug was fixed and protects the code by ensuring that a reintroduction of the bug gets noticed is an integral part of it. It does not make much sense to artificially split it into two. It's like arguing for adding declarations for new functions and global variables in *.h files in a step before a separate step adds their implementations in *.c files, claiming that the first step designs the interface (which is sufficient for writing the client code) and the second one gives an implementation of the interface (which can be replaced later), and they address two separate concerns. And then you would find that there are some compilers that warn against unimplemented functions and undefined variables. The "solution" would be to enclose the whole thing that was added in the first "*.h only patch" inside "#if 0/#endif" to hide it from the compiler ;-) That in fact looks quite similar to how "test only patch" marks the new tests as expecting failure in the beginning. Neither is truly useful when applied to a context different from where it is originally developed for.
Re: separating regression test patches from fixes, was Re: [PATCH 3/3] cherry-pick --continue: remember options
On Thu, Mar 14, 2019 at 9:10 PM Johannes Schindelin wrote: > In any case, before we get better tooling to work around these issues, I > still think it makes a ton of sense to encourage proper separation of > concerns: to keep patches that introduce regression tests demonstrating a > breakage separate from patches that fix the breakage. It would certainly > help me (e.g. when staring at a range diff). Then perhaps improve the tools now because these separate patches enter 'master' and stay in the history forever. One of the problems I have with separating tests from the actual code change is the description of the problem often stays on the test commit, which I can't see in git-log if I'm searching for the code change. And no sometimes I can't just look at the parent commit if I filter code by path (and with --full-diff) -- Duy
separating regression test patches from fixes, was Re: [PATCH 3/3] cherry-pick --continue: remember options
Hi Junio, On Thu, 14 Mar 2019, Junio C Hamano wrote: > Johannes Schindelin writes: > > > And it is especially nice that you make it easy to verify that there > > is a bug in the first place, by separating the concern of > > demonstrating it from the concern to fix it. > > For a multi-patch series, breaking out the verification test out of the > fixing patch is OK. It is a different story for a simple change that > could be made in a single step to artificially separate the test into a > separate step. An early "test_expect_failure" would not stop the test > when applied to a different codebase as a standalone "does the breakage > exist in this unrelated version?" even if the "test only" patch might > appear easier to apply (as opposed to applying only the t/ part of the > patch that expects success and seeing it actually fail). This keeps coming up, and I feel that in the past, I have only managed to upset you, not to convince you that my point of view has at least *some* merit. So please let me try again, this time with a cooler head. As you know, I find it rather cumbersome to review code changes on the Git mailing list. Unlike others, I do not suffer from issues revolving around HTML parts and/or mailer issues, and I do try to just work with the limitations of the medium. But sometimes the diff just does not give enough context. Or I really would need to look at it with `--color-moved` or `--color-words` or `--patience` or `-w`. Or yet other advanced diff options. So I would like to have the code changes locally, in a branch in my worktree. And at least for me, code review often does not end there. I need to dive into the commit history to see e.g. why a line that a patch wants to removed was added in the first place, i.e. read the commit message and see how the surrounding code changed. So I would like to have not only some re-applied changes locally, but really a clone of the branch that the contributor themselves had locally. And as my understanding of code review is first and foremost to ensure the correctness of the code (which is why I want to leave the more tedious parts of code review that do not really require a human to review, or even fix, to automation), in some cases I really need to run the code (or the regression test) locally, e.g. to see whether it is a Windows-specific issue, or whether it affects this or that branch. The necessity for this kind of thing was demonstrated rather well by Hannes Sixt recently, where he ran a regression test that I added as part of https://github.com/gitgitgadget/git/pull/96 (mail thread here: https://public-inbox.org/git/pull.96.git.gitgitgad...@gmail.com/), and figured out that this regression does not even exist in git.git's `master` branch. Therefore it don't need no fixing there, either. So now that we established how important it is to go back to the local Git worktree for many in-depth reviews, let's see how easy/convenient that is right now. In our currently established process, you have to have mutt with your own custom scripts. Kidding! But it is undeniable that without serious scripting (that nobody seems to be able to do in a way that can be shared between reviewers), it is super hard to do anything but a pure diff-based review (which would not have caught the problem described above). So how can we make this easier? How can we make it less painful to review code changes beyond the bare minimum diff-based review which, let's face it, encourages "white space review", i.e. focusing on problems that can be seen from the diff, but that are really of little consequence when it comes to the health of our code base? What it boils down to: it needs to be easier to pull down patches and to recreate local branches for them. It is in this context that I find it much better to separate patches that add regression test cases from patches that fix the regressions. How often did I encounter problems to apply the diff from public-inbox? I cannot count the number that happened/happens to me. And usually the diff does not apply, many more times in the part that touches libgit.a code than in the part that touches the test suite. And how many times did `git am -3` not help me because I lacked the prerequisite objects? Again, I cannot count that number, it happend/happens *so* often. But the regression test patches usually apply cleanly, or if they don't, chances are that I have the prerequisite objects, because we rarely change regression tests locally, we only add to them. And even if I do not have those prerequisite objects, those patches usually *add* code, so it is easier to resolve the problems. Note: I sometimes want the regression test locally not so much to verify that it demonstrates a current bug, but sometimes to look at the code flow, to see which parts of the code are executed. That is why I like to have that part
Re: [PATCH 0/8] Resend of GIT_TEST_PROTOCOL_VERSION patches
On Thu, Feb 07 2019, Jeff King wrote: > On Wed, Feb 06, 2019 at 11:20:32PM +0100, Ævar Arnfjörð Bjarmason wrote: > >> >> So far we've had the convention that these GIT_TEST_* variables, >> >> e.g. the one for the commit graph, work the same way. Thus we guarantee >> >> that we get (in theory) 100% coverage even when running the tests in >> >> this special mode. I think it's better to keep it as-is. >> > >> > But what's the point of that? Don't you always have to run the test >> > suite _twice_, once with the special variable and once without? >> > Otherwise, you are not testing one case or the other. >> > >> > Or are you arguing that one might set many special variables in one go >> > (to prefer running the suite only twice, instead of 2^N times). In which >> > case we are better off running the test (as opposed to skipping it), as >> > it might use one of the _other_ special variables besides >> > GIT_TEST_PROTOCOL_VERSION. >> > >> > I can buy that line of reasoning. It still doesn't cover all cases that >> > a true 2^N test would, but that clearly isn't going to be practical. >> >> Maybe I'm misunderstanding what you're proposing, but as an example, >> let's say the test suite is just these two tests: >> >> test_expect_success 'some unrelated thing' '...' >> test_expect_success 'test protocol v2' 'GIT_TEST_PROTOCOL_VERSION=2 ...' >> >> And GIT_TEST_PROTOCOL_VERSION=0 is the default, let's say I want to test >> with GIT_TEST_PROTOCOL_VERSION=1 for whatever reason, >> >> I'd still like both tests to be run, not just 1/2 with >> GIT_TEST_PROTOCOL_VERSION=1 and 2/2 skipped because it's explicitly >> testing for the GIT_TEST_PROTOCOL_VERSION=2 case, whereas I asked for a >> GIT_TEST_PROTOCOL_VERSION=1. > > But that's my "why". The second test will run identically in both runs, > regardless of your setting of GIT_TEST_PROTOCOL_VERSION. So there's > value if you're only running the suite once in getting full coverage, > but if you are literally going to run it with and without, then you're > running the exact same code twice for no reason. And you have to run it > both with and without, since otherwise all of the _other_ tests aren't > seeing both options. Yeah, by always running the 2nd test regardless of what GIT_TEST_PROTOCOL_VERSION=* is set to we're wasting CPU if we know we're going to run both with GIT_TEST_PROTOCOL_VERSION=1 and GIT_TEST_PROTOCOL_VERSION=2. But we don't know that, and in terms of CPU & time the tests that rely on any given GIT_TEST_* flag are such a tiny part of the test suite, that I think it's fine to run them twice in such a scenario to guard against the case when we just run in one more or the other, and not both. >> IOW the point of these tests is to piggy-back on the tests that *aren't* >> aware of the feature you're trying to test. So >> e.g. GIT_TEST_COMMIT_GRAPH=true should run our whole test suite with the >> commit graph, and *also* those tests that are explicitly aware of the >> commit graph, including those that for some reason would want to test >> for the case where it isn't enabled (to e.g. test that --contains works >> without the graph). >> >> Otherwise I can't say "I care more about GIT_TEST_COMMIT_GRAPH=true than >> not", and run just one test run with that, because I'll have blind spots >> in the commit graph tests themselves, and would then need to do another >> run with GIT_TEST_COMMIT_GRAPH= set to make sure I have 100% coverage. > > So if we are still talking about the same 2-test setup above, which only > has a GIT_TEST_PROTOCOL_VERSION override, then yeah, I get the point of > being able to run a "stock" test, and then one with _both_ of the flags > (GIT_TEST_PROTOCOL_VERSION and GIT_TEST_COMMIT_GRAPH) because you don't > quite know how each test will interact with all of the "other" flags. Yeah that's another reason not to skip them, although we could imagine a prereq where we skip the v2 tests if GIT_TEST_PROTOCOL_VERSION=1 is set *and* no other GIT_TEST_*=* flag is set, but I think that would also be a bad idea. > So now I'm not 100% sure I understand what you're talking about, but I > think maybe we are actually in agreement. ;) I think there's two ways to view these GIT_TEST_FOO=BAR facilities: 1. Run all tests with "FOO" set to "BAR", skip those (via prereq) we know would break in that mode. 2. Ditto, but if a test says "I'm a test for FOO=!BAR" leave it alone. #2 is what we have now. I read your <20190206213458.gc12...@sigill.intra.peff.net> as suggesting that we should move to #1. You're correct that moving to #1 would force us to more exhaustively mark things so that if the default moves to "BAR" we just have to flip a switch.
Re: [PATCH 0/8] Resend of GIT_TEST_PROTOCOL_VERSION patches
On Wed, Feb 06, 2019 at 11:20:32PM +0100, Ævar Arnfjörð Bjarmason wrote: > >> So far we've had the convention that these GIT_TEST_* variables, > >> e.g. the one for the commit graph, work the same way. Thus we guarantee > >> that we get (in theory) 100% coverage even when running the tests in > >> this special mode. I think it's better to keep it as-is. > > > > But what's the point of that? Don't you always have to run the test > > suite _twice_, once with the special variable and once without? > > Otherwise, you are not testing one case or the other. > > > > Or are you arguing that one might set many special variables in one go > > (to prefer running the suite only twice, instead of 2^N times). In which > > case we are better off running the test (as opposed to skipping it), as > > it might use one of the _other_ special variables besides > > GIT_TEST_PROTOCOL_VERSION. > > > > I can buy that line of reasoning. It still doesn't cover all cases that > > a true 2^N test would, but that clearly isn't going to be practical. > > Maybe I'm misunderstanding what you're proposing, but as an example, > let's say the test suite is just these two tests: > > test_expect_success 'some unrelated thing' '...' > test_expect_success 'test protocol v2' 'GIT_TEST_PROTOCOL_VERSION=2 ...' > > And GIT_TEST_PROTOCOL_VERSION=0 is the default, let's say I want to test > with GIT_TEST_PROTOCOL_VERSION=1 for whatever reason, > > I'd still like both tests to be run, not just 1/2 with > GIT_TEST_PROTOCOL_VERSION=1 and 2/2 skipped because it's explicitly > testing for the GIT_TEST_PROTOCOL_VERSION=2 case, whereas I asked for a > GIT_TEST_PROTOCOL_VERSION=1. But that's my "why". The second test will run identically in both runs, regardless of your setting of GIT_TEST_PROTOCOL_VERSION. So there's value if you're only running the suite once in getting full coverage, but if you are literally going to run it with and without, then you're running the exact same code twice for no reason. And you have to run it both with and without, since otherwise all of the _other_ tests aren't seeing both options. > IOW the point of these tests is to piggy-back on the tests that *aren't* > aware of the feature you're trying to test. So > e.g. GIT_TEST_COMMIT_GRAPH=true should run our whole test suite with the > commit graph, and *also* those tests that are explicitly aware of the > commit graph, including those that for some reason would want to test > for the case where it isn't enabled (to e.g. test that --contains works > without the graph). > > Otherwise I can't say "I care more about GIT_TEST_COMMIT_GRAPH=true than > not", and run just one test run with that, because I'll have blind spots > in the commit graph tests themselves, and would then need to do another > run with GIT_TEST_COMMIT_GRAPH= set to make sure I have 100% coverage. So if we are still talking about the same 2-test setup above, which only has a GIT_TEST_PROTOCOL_VERSION override, then yeah, I get the point of being able to run a "stock" test, and then one with _both_ of the flags (GIT_TEST_PROTOCOL_VERSION and GIT_TEST_COMMIT_GRAPH) because you don't quite know how each test will interact with all of the "other" flags. So now I'm not 100% sure I understand what you're talking about, but I think maybe we are actually in agreement. ;) -Peff
Re: [PATCH 0/8] Resend of GIT_TEST_PROTOCOL_VERSION patches
On Wed, Feb 06 2019, Jeff King wrote: > On Wed, Feb 06, 2019 at 10:52:15PM +0100, Ævar Arnfjörð Bjarmason wrote: > >> > I wonder if it would be more obvious what's going on if we instead had a >> > prereq like: >> > >> > test_expect_success !PROTO_V2 'ls-remote --symref' ' >> > ... >> > ' >> > >> > and just skipped those tests entirely (and in a way that appears in the >> > TAP output). >> > >> > I think it would also future-proof us a bit for v2 becoming the default >> > (i.e., when GIT_TEST_PROTOCOL_VERSION being blank does mean "use v2"). >> > >> > I dunno. It probably doesn't matter all that much, so it may not be >> > worth going back and changing at this point. Just a thought. >> >> So far we've had the convention that these GIT_TEST_* variables, >> e.g. the one for the commit graph, work the same way. Thus we guarantee >> that we get (in theory) 100% coverage even when running the tests in >> this special mode. I think it's better to keep it as-is. > > But what's the point of that? Don't you always have to run the test > suite _twice_, once with the special variable and once without? > Otherwise, you are not testing one case or the other. > > Or are you arguing that one might set many special variables in one go > (to prefer running the suite only twice, instead of 2^N times). In which > case we are better off running the test (as opposed to skipping it), as > it might use one of the _other_ special variables besides > GIT_TEST_PROTOCOL_VERSION. > > I can buy that line of reasoning. It still doesn't cover all cases that > a true 2^N test would, but that clearly isn't going to be practical. Maybe I'm misunderstanding what you're proposing, but as an example, let's say the test suite is just these two tests: test_expect_success 'some unrelated thing' '...' test_expect_success 'test protocol v2' 'GIT_TEST_PROTOCOL_VERSION=2 ...' And GIT_TEST_PROTOCOL_VERSION=0 is the default, let's say I want to test with GIT_TEST_PROTOCOL_VERSION=1 for whatever reason, I'd still like both tests to be run, not just 1/2 with GIT_TEST_PROTOCOL_VERSION=1 and 2/2 skipped because it's explicitly testing for the GIT_TEST_PROTOCOL_VERSION=2 case, whereas I asked for a GIT_TEST_PROTOCOL_VERSION=1. IOW the point of these tests is to piggy-back on the tests that *aren't* aware of the feature you're trying to test. So e.g. GIT_TEST_COMMIT_GRAPH=true should run our whole test suite with the commit graph, and *also* those tests that are explicitly aware of the commit graph, including those that for some reason would want to test for the case where it isn't enabled (to e.g. test that --contains works without the graph). Otherwise I can't say "I care more about GIT_TEST_COMMIT_GRAPH=true than not", and run just one test run with that, because I'll have blind spots in the commit graph tests themselves, and would then need to do another run with GIT_TEST_COMMIT_GRAPH= set to make sure I have 100% coverage.
Re: [PATCH 0/8] Resend of GIT_TEST_PROTOCOL_VERSION patches
On Wed, Feb 06, 2019 at 10:52:15PM +0100, Ævar Arnfjörð Bjarmason wrote: > > I wonder if it would be more obvious what's going on if we instead had a > > prereq like: > > > > test_expect_success !PROTO_V2 'ls-remote --symref' ' > > ... > > ' > > > > and just skipped those tests entirely (and in a way that appears in the > > TAP output). > > > > I think it would also future-proof us a bit for v2 becoming the default > > (i.e., when GIT_TEST_PROTOCOL_VERSION being blank does mean "use v2"). > > > > I dunno. It probably doesn't matter all that much, so it may not be > > worth going back and changing at this point. Just a thought. > > So far we've had the convention that these GIT_TEST_* variables, > e.g. the one for the commit graph, work the same way. Thus we guarantee > that we get (in theory) 100% coverage even when running the tests in > this special mode. I think it's better to keep it as-is. But what's the point of that? Don't you always have to run the test suite _twice_, once with the special variable and once without? Otherwise, you are not testing one case or the other. Or are you arguing that one might set many special variables in one go (to prefer running the suite only twice, instead of 2^N times). In which case we are better off running the test (as opposed to skipping it), as it might use one of the _other_ special variables besides GIT_TEST_PROTOCOL_VERSION. I can buy that line of reasoning. It still doesn't cover all cases that a true 2^N test would, but that clearly isn't going to be practical. -Peff
Re: [PATCH 0/8] Resend of GIT_TEST_PROTOCOL_VERSION patches
On Wed, Feb 06 2019, Jeff King wrote:
> On Tue, Feb 05, 2019 at 04:21:14PM -0800, Jonathan Tan wrote:
>
>> This is on the latest master (8feddda32c ("Fifth batch for 2.21",
>> 2019-02-05)) + js/protocol-advertise-multi.
>>
>> This is a resend of [1], which was built on the result of merging many
>> branches. Now that most of the branches have been merged to master, I
>> have rebased it on master. The only branch that I needed to merge was
>> js/protocol-advertise-multi.
>
> Thanks for working on this. With the exception of the final patch, this
> all seems pretty sane to me from a quick look.
>
> There is one thing that your test patches made me wonder. When we have
> to make an exception to a test (i.e., that doesn't work under v2), you
> do it by unsetting GIT_TEST_PROTOCOL_VERSION in the environment. That
> means we'll actually run the test, but not with the version that the
> caller specified.
>
> I wonder if it would be more obvious what's going on if we instead had a
> prereq like:
>
> test_expect_success !PROTO_V2 'ls-remote --symref' '
> ...
> '
>
> and just skipped those tests entirely (and in a way that appears in the
> TAP output).
>
> I think it would also future-proof us a bit for v2 becoming the default
> (i.e., when GIT_TEST_PROTOCOL_VERSION being blank does mean "use v2").
>
> I dunno. It probably doesn't matter all that much, so it may not be
> worth going back and changing at this point. Just a thought.
So far we've had the convention that these GIT_TEST_* variables,
e.g. the one for the commit graph, work the same way. Thus we guarantee
that we get (in theory) 100% coverage even when running the tests in
this special mode. I think it's better to keep it as-is.
Re: [PATCH 0/8] Resend of GIT_TEST_PROTOCOL_VERSION patches
On Tue, Feb 05, 2019 at 04:21:14PM -0800, Jonathan Tan wrote:
> This is on the latest master (8feddda32c ("Fifth batch for 2.21",
> 2019-02-05)) + js/protocol-advertise-multi.
>
> This is a resend of [1], which was built on the result of merging many
> branches. Now that most of the branches have been merged to master, I
> have rebased it on master. The only branch that I needed to merge was
> js/protocol-advertise-multi.
Thanks for working on this. With the exception of the final patch, this
all seems pretty sane to me from a quick look.
There is one thing that your test patches made me wonder. When we have
to make an exception to a test (i.e., that doesn't work under v2), you
do it by unsetting GIT_TEST_PROTOCOL_VERSION in the environment. That
means we'll actually run the test, but not with the version that the
caller specified.
I wonder if it would be more obvious what's going on if we instead had a
prereq like:
test_expect_success !PROTO_V2 'ls-remote --symref' '
...
'
and just skipped those tests entirely (and in a way that appears in the
TAP output).
I think it would also future-proof us a bit for v2 becoming the default
(i.e., when GIT_TEST_PROTOCOL_VERSION being blank does mean "use v2").
I dunno. It probably doesn't matter all that much, so it may not be
worth going back and changing at this point. Just a thought.
-Peff
[PATCH 0/8] Resend of GIT_TEST_PROTOCOL_VERSION patches
This is on the latest master (8feddda32c ("Fifth batch for 2.21",
2019-02-05)) + js/protocol-advertise-multi.
This is a resend of [1], which was built on the result of merging many
branches. Now that most of the branches have been merged to master, I
have rebased it on master. The only branch that I needed to merge was
js/protocol-advertise-multi.
js/protocol-advertise-multi has not been merged to next yet, but the
general idea seems good to me, and I am making review of that patch one
of my priorities. But if anyone is concerned that this set will be
delayed by js/protocol-advertise-multi, I can extract the parts I need
into a patch in this set (most likely just the part that unconditionally
calls ssh with SendEnv in connect.c), although it is difficult to
explain the necessity of that without reference to
js/protocol-advertise-multi. Let me know what you think.
Besides the rebase, this is unchanged from [1]. There is some discussion
there between Ævar, Junio, and I, thinking that it is OK to merge this
even if we didn't eliminate all errors. Right now, only 3 test cases
fail with GIT_TEST_PROTOCOL_VERSION=2.
Patch 1 implements the test variable, patches 2-7 eliminate false
negatives (with explanations), and patch 8 fixes a genuine bug.
[1] https://public-inbox.org/git/cover.1547677183.git.jonathanta...@google.com/
Jonathan Tan (8):
tests: define GIT_TEST_PROTOCOL_VERSION
tests: always test fetch of unreachable with v0
t5503: fix overspecification of trace expectation
t5512: compensate for v0 only sending HEAD symrefs
t5700: only run with protocol version 1
tests: fix protocol version for overspecifications
t5552: compensate for v2 filtering ref adv.
remote-curl: in v2, fill credentials if needed
protocol.c | 17 --
remote-curl.c| 9 +-
t/README | 3 ++
t/t5400-send-pack.sh | 2 +-
t/t5500-fetch-pack.sh| 4 ++-
t/t5503-tagfollow.sh | 2 +-
t/t5512-ls-remote.sh | 18 ---
t/t5515-fetch-merge-logic.sh | 4 +++
t/t5516-fetch-push.sh| 22 ++---
t/t5539-fetch-http-shallow.sh| 5 ++-
t/t5541-http-push-smart.sh | 14 +++--
t/t5551-http-fetch-smart.sh | 47 +---
t/t5552-skipping-fetch-negotiator.sh | 5 ++-
t/t5700-protocol-v1.sh | 3 ++
t/t7406-submodule-update.sh | 5 ++-
15 files changed, 128 insertions(+), 32 deletions(-)
--
2.19.0.271.gfe8321ec05.dirty
Re: RFE: git-patch-id should handle patches without leading "diff"
Jonathan Nieder writes: >> So it seems most sensible to me if this is going to be supported that we >> go a bit beyond the call of duty and fake up the start of it, namely: >> >> --- a/arch/x86/kernel/process.c >> +++ b/arch/x86/kernel/process.c >> >> To be: >> >> diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c >> --- a/arch/x86/kernel/process.c >> +++ b/arch/x86/kernel/process.c > > Right. We may want to handle diff.mnemonicPrefix as well. I definitely think under the --stable option, we should pretend as if the canonical a/ vs b/ prefixes were given with the "diff --git" header, just like we try to reverse the effect of diff-orderfile, etc. I am unsure what the right behaviour under --unstable is, though.
Re: RFE: git-patch-id should handle patches without leading "diff"
Ævar Arnfjörð Bjarmason wrote: > On Fri, Dec 07 2018, Jonathan Nieder wrote: >> The patch-id appears to only care about the diff text, so it should be >> able to handle this. So if we have a better heuristic for where the >> diff starts, it would be good to use it. > > No, the patch-id doesn't just care about the diff, it cares about the > context before the diff too. Sorry, I did a bad job of communicating. When I said "diff text", I was including context. [...] > Observe that the diff --git line matters, we hash it: > > $ git diff-tree -p HEAD~.. | git patch-id > 5870d115b7e2a9a936ab8fdc254932234413c710 > > $ git diff-tree --src-prefix=a/ --dst-prefix=b/ -p HEAD~.. | git patch-id > --stable > 5870d115b7e2a9a936ab8fdc254932234413c710 > > $ git diff-tree --src-prefix=x/ --dst-prefix=y/ -p HEAD~.. | git patch-id > --stable > 4cd136f2b98760150f700ac6a5b126389d6d05a7 > Oh, hm. That's unfortunate. [...] > So it seems most sensible to me if this is going to be supported that we > go a bit beyond the call of duty and fake up the start of it, namely: > > --- a/arch/x86/kernel/process.c > +++ b/arch/x86/kernel/process.c > > To be: > > diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c > --- a/arch/x86/kernel/process.c > +++ b/arch/x86/kernel/process.c Right. We may want to handle diff.mnemonicPrefix as well. Jonathan
Re: RFE: git-patch-id should handle patches without leading "diff"
On Fri, Dec 07 2018, Jonathan Nieder wrote: > Hi, > > Konstantin Ryabitsev wrote: > >> Every now and again I come across a patch sent to LKML without a leading >> "diff a/foo b/foo" -- usually produced by quilt. E.g.: >> >> https://lore.kernel.org/lkml/20181125185004.151077...@linutronix.de/ >> >> I am guessing quilt does not bother including the leading "diff a/foo >> b/foo" because it's redundant with the next two lines, however this >> remains a valid patch recognized by git-am. >> >> If you pipe that patch via git-patch-id, it produces nothing, but if I >> put in the leading "diff", like so: >> >> diff a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c >> >> then it properly returns "fb3ae17451bc619e3d7f0dd647dfba2b9ce8992e". > > Interesting. As Ævar mentioned, the relevant code is > > /* Ignore commit comments */ > if (!patchlen && !starts_with(line, "diff ")) > continue; > > which is trying to handle a case where a line that is special to the > parser appears before the diff begins. > > The patch-id appears to only care about the diff text, so it should be > able to handle this. So if we have a better heuristic for where the > diff starts, it would be good to use it. No, the patch-id doesn't just care about the diff, it cares about the context before the diff too. See this patch: $ git diff-tree --src-prefix=x/ --dst-prefix=y/ -p HEAD~.. diff --git x/refs/files-backend.c y/refs/files-backend.c index 9183875dad..dd8abe9185 100644 --- x/refs/files-backend.c +++ y/refs/files-backend.c @@ -180,7 +180,8 @@ static void files_reflog_path(struct files_ref_store *refs, break; case REF_TYPE_OTHER_PSEUDOREF: case REF_TYPE_MAIN_PSEUDOREF: - return files_reflog_path_other_worktrees(refs, sb, refname); + files_reflog_path_other_worktrees(refs, sb, refname); + break; case REF_TYPE_NORMAL: strbuf_addf(sb, "%s/logs/%s", refs->gitcommondir, refname); break; Observe that the diff --git line matters, we hash it: $ git diff-tree -p HEAD~.. | git patch-id 5870d115b7e2a9a936ab8fdc254932234413c710 $ git diff-tree --src-prefix=a/ --dst-prefix=b/ -p HEAD~.. | git patch-id --stable 5870d115b7e2a9a936ab8fdc254932234413c710 $ git diff-tree --src-prefix=x/ --dst-prefix=y/ -p HEAD~.. | git patch-id --stable 4cd136f2b98760150f700ac6a5b126389d6d05a7 The thing it doesn't care about is the "index" between the "diff" and patch: $ git diff-tree --src-prefix=x/ --dst-prefix=y/ -p HEAD~.. | grep -v ^index | git patch-id --stable 4cd136f2b98760150f700ac6a5b126389d6d05a7 We also care about the +++ and --- lines: $ git diff-tree --src-prefix=x/ --dst-prefix=y/ -p HEAD~.. | grep -v ^index | perl -pe 's/^(\+\+\+|---).*/$1/g' | git patch-id 56985c2c38cce6079de2690082e1770a8e81214c Then we normalize the @@ line, e.g.: $ git diff-tree --src-prefix=x/ --dst-prefix=y/ -p HEAD~.. | grep -v ^index | git patch-id 4cd136f2b98760150f700ac6a5b126389d6d05a7 $ git diff-tree --src-prefix=x/ --dst-prefix=y/ -p HEAD~.. | grep -v ^index | perl -pe 's/\d+/123/g' | git patch-id 4cd136f2b98760150f700ac6a5b126389d6d05a7 There's other caveats (see the code, e.g. "strip space") but to a first approximation a patch id is a hash of something that looks like this: diff --git x/refs/files-backend.c y/refs/files-backend.c --- x/refs/files-backend.c +++ y/refs/files-backend.c @@ -123,123 +123,123 @@ static void files_reflog_path(struct files_ref_store *refs, break; case REF_TYPE_OTHER_PSEUDOREF: case REF_TYPE_MAIN_PSEUDOREF: - return files_reflog_path_other_worktrees(refs, sb, refname); + files_reflog_path_other_worktrees(refs, sb, refname); + break; case REF_TYPE_NORMAL: strbuf_addf(sb, "%s/logs/%s", refs->gitcommondir, refname); break; Which means that accepting a patch like this as input would actually give you a different patch-id than if it had the proper header. So it seems most sensible to me if this is going to be supported that we go a bit beyond the call of duty and fake up the start of it, namely: --- a/arch/x86/kernel/process.c +++ b/arch/x86/kernel/process.c To be: diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c --- a/arch/x86/kernel/process.c +++ b/arch/x86/kernel/process.c It'll make the state machine a bit more complex, but IMO it would suck more if w
Re: RFE: git-patch-id should handle patches without leading "diff"
Hi, Konstantin Ryabitsev wrote: > Every now and again I come across a patch sent to LKML without a leading > "diff a/foo b/foo" -- usually produced by quilt. E.g.: > > https://lore.kernel.org/lkml/20181125185004.151077...@linutronix.de/ > > I am guessing quilt does not bother including the leading "diff a/foo > b/foo" because it's redundant with the next two lines, however this > remains a valid patch recognized by git-am. > > If you pipe that patch via git-patch-id, it produces nothing, but if I > put in the leading "diff", like so: > > diff a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c > > then it properly returns "fb3ae17451bc619e3d7f0dd647dfba2b9ce8992e". Interesting. As Ævar mentioned, the relevant code is /* Ignore commit comments */ if (!patchlen && !starts_with(line, "diff ")) continue; which is trying to handle a case where a line that is special to the parser appears before the diff begins. The patch-id appears to only care about the diff text, so it should be able to handle this. So if we have a better heuristic for where the diff starts, it would be good to use it. "git apply" uses apply.c::find_header, which is more permissive. Maybe it would be possible to unify these somehow. (I haven't looked closely enough to tell how painful that would be.) Thanks and hope that helps, Jonathan
Re: RFE: git-patch-id should handle patches without leading "diff"
On Fri, Dec 07 2018, Konstantin Ryabitsev wrote: > Hi, all: > > Every now and again I come across a patch sent to LKML without a leading > "diff a/foo b/foo" -- usually produced by quilt. E.g.: > > https://lore.kernel.org/lkml/20181125185004.151077...@linutronix.de/ > > I am guessing quilt does not bother including the leading "diff a/foo > b/foo" because it's redundant with the next two lines, however this > remains a valid patch recognized by git-am. > > If you pipe that patch via git-patch-id, it produces nothing, but if I > put in the leading "diff", like so: > > diff a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c > > then it properly returns "fb3ae17451bc619e3d7f0dd647dfba2b9ce8992e". > > Can we please teach git-patch-id to work without the leading diff a/foo > b/foo, same as git-am? > > Best, > -K The state machine is sensitive there being a "diff" line, then "index" etc. diff --git a/builtin/patch-id.c b/builtin/patch-id.c index 970d0d30b4..b99e4455fd 100644 --- a/builtin/patch-id.c +++ b/builtin/patch-id.c @@ -97,7 +97,9 @@ static int get_one_patchid(struct object_id *next_oid, struct object_id *result, } /* Ignore commit comments */ - if (!patchlen && !starts_with(line, "diff ")) + if (!patchlen && starts_with(line, "--- a/")) + ; + else if (!patchlen && !starts_with(line, "diff ")) continue; /* Parsing diff header? */ This would make it produce a patch-id for that input, however note that I've done "--- a/" there, with just "--- " (which is legit) we'd get confused and start earlier before the diffstat. So if you're interested in having this I leave it to you to run with this & write tests for it, but more convincingly run it on the git & LKML archives and see that the output is the same (or just extra in case where we now find patches) with --stable etc.
RFE: git-patch-id should handle patches without leading "diff"
Hi, all: Every now and again I come across a patch sent to LKML without a leading "diff a/foo b/foo" -- usually produced by quilt. E.g.: https://lore.kernel.org/lkml/20181125185004.151077...@linutronix.de/ I am guessing quilt does not bother including the leading "diff a/foo b/foo" because it's redundant with the next two lines, however this remains a valid patch recognized by git-am. If you pipe that patch via git-patch-id, it produces nothing, but if I put in the leading "diff", like so: diff a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c then it properly returns "fb3ae17451bc619e3d7f0dd647dfba2b9ce8992e". Can we please teach git-patch-id to work without the leading diff a/foo b/foo, same as git-am? Best, -K signature.asc Description: PGP signature
How to add edges to visualize cherry-picks and ported patches
Hi, I hope this is the right place for this answer. If not, plaese point me to a more appropriate place. A project "P2" [2] forked from another project "P1" [1] quite some time ago, both repos share a common history up to some point. After this point, P2 cherry-picked commits from P1, but did not merge P1 any longer. Unfortunately the author of P2 did not use any mechanism (e.g. an intermediate branch) to allow tracking up to which point P1 commits are considered. Thus the graph looks like this: P1: --A--B--C--X--X--D--E--X--X--X--F-- P2: --Y--Y--A--B--C--D--E--Y--F-- I would like to add edges (say: another branch and merge-commits) to the graph to make it look something like this: P1: --A--B--C--X--X--D--E--X--X--X--F-- \ \ \ o---o---o---new branch / / / P2: --Y--Y--A--B--C---D--EY--F-- Of course the new branch should contain the same code as the respective P2-commit. So at the end, the new branch couls become the P2's new "master" branch. How can I achieve this (which commands to use)? Is there some way to automate this? (e.g. based on `git cherry`) Thanks in advance for any answer [1] https://github.com/siacs/Conversations [2] https://github.com/kriztan/Pix-Art-Messenger -- Regards Hartmut Goebel | Hartmut Goebel | h.goe...@crazy-compilers.com | | www.crazy-compilers.com | compilers which you thought are impossible |
Re: [PATCH] Makefile: add pending semantic patches
SZEDER Gábor writes: >> > inifinite recursion)? Or are they "correct but not immediately >> > necessary" (e.g. because calling read_cache() does not hurt until >> > that function gets removed, so rewriting the callers to call >> > read_index() with &the_index may be correct but not immediately >> > necessary)? >> >> the latter. I assume correctness (of the semantic patch) to be a given, > > I'm afraid we can't assume that. As far as correctness is concerned, > I think semantic patches are not different from any other code we > write, i.e. they are potentially buggy. Perhaps even more so than the > "usual" Git code, because we have long experience writing C and shell > code (with all their quirks and error-proneness), but none of us is > really an expert in writing semantic patches. All correct. And applying semantic patches generated from buggy sources can produce buggy code, just like merging a buggy topic branch does. These days, I ran coccicheck at the tip of 'pu' (even though this cost me quite a lot a few times every day) and feed its findings back to authors of topics that introduce new ones, so that their topics next time do not need the fix-up at the tip of 'pu' in the next integration cycle. That way, the changes mechanically suggested by coccicheck can still be reviewed in small bite-sized chunks and hopefully possible problems caused by buggy sources can either be found in the review process, or discovered at the tip of 'pu'.
Re: [PATCH] coccicheck: introduce 'pending' semantic patches
On Fri, Nov 09, 2018 at 04:10:52PM -0800, Stefan Beller wrote: > From: SZEDER Gábor > > Teach `make coccicheck` to avoid patches named "*.pending.cocci" and > handle them separately in a new `make coccicheck-pending` instead. > This means that we can separate "critical" patches from "FYI" patches. > The former target can continue causing Travis to fail its static > analysis job, while the latter can let us keep an eye on ongoing > (pending) transitions without them causing too much fallout. > > Document the intended use-cases around these two targets. > As the process around the pending patches is not yet fully explored, > leave that out. > > Based-on-work-by: SZEDER Gábor Hm, do I need to sign off? Well, I sign off now anyway, and then Junio can take it if necessary or just ignore it if not. Signed-off-by: SZEDER Gábor > Signed-off-by: Stefan Beller > Signed-off-by: Junio C Hamano > --- > > I dialed back on the workflow, as we may want to explore it first > before writing it down. Yeah, I too think that's better to wait with the workflow details until we gather some experience about how it works out in practice. Thanks for following it through.
Re: [PATCH] Makefile: add pending semantic patches
On Fri, Nov 09, 2018 at 01:58:01PM -0800, Stefan Beller wrote:
> On Thu, Nov 8, 2018 at 9:18 PM Junio C Hamano wrote:
> > Are they wrong changes (e.g. a carelessly written read_cache() to
> > read_index(&the_index) conversion may munge the implementation of
> > read_cache(...) { return read_index(&the_index, ...); } and make
> > inifinite recursion)? Or are they "correct but not immediately
> > necessary" (e.g. because calling read_cache() does not hurt until
> > that function gets removed, so rewriting the callers to call
> > read_index() with &the_index may be correct but not immediately
> > necessary)?
>
> the latter. I assume correctness (of the semantic patch) to be a given,
I'm afraid we can't assume that. As far as correctness is concerned,
I think semantic patches are not different from any other code we
write, i.e. they are potentially buggy. Perhaps even more so than the
"usual" Git code, because we have long experience writing C and shell
code (with all their quirks and error-proneness), but none of us is
really an expert in writing semantic patches.
Cases in point:
- 6afedba8c9 (object_id.cocci: match only expressions of type
'struct object_id', 2018-10-15)
- 279ffad17d (coccinelle: avoid wrong transformation suggestions
from commit.cocci, 2018-04-30)
- cd9a4b6d93 (cocci: use format keyword instead of a literal string,
2018-01-19); though this one is probably a bug in Coccinelle
itself
- c2bb0c1d1e (cocci: avoid self-references in object_id
transformations, 2016-11-01)
Re: [PATCH] coccicheck: introduce 'pending' semantic patches
On 2018.11.09 16:10, Stefan Beller wrote: > From: SZEDER Gábor > > Teach `make coccicheck` to avoid patches named "*.pending.cocci" and > handle them separately in a new `make coccicheck-pending` instead. > This means that we can separate "critical" patches from "FYI" patches. > The former target can continue causing Travis to fail its static > analysis job, while the latter can let us keep an eye on ongoing > (pending) transitions without them causing too much fallout. > > Document the intended use-cases around these two targets. > As the process around the pending patches is not yet fully explored, > leave that out. > > Based-on-work-by: SZEDER Gábor > Signed-off-by: Stefan Beller > Signed-off-by: Junio C Hamano > --- > > I dialed back on the workflow, as we may want to explore it first > before writing it down. > > Stefan Looks good to me. Reviewed-by: Josh Steadmon
Re: [PATCH] coccicheck: introduce 'pending' semantic patches
On Sat, 10 Nov 2018 at 01:10, Stefan Beller wrote: > I dialed back on the workflow, as we may want to explore it first > before writing it down. Makes sense. FWIW, this iteration looks good to me. Martin
[PATCH] coccicheck: introduce 'pending' semantic patches
From: SZEDER Gábor Teach `make coccicheck` to avoid patches named "*.pending.cocci" and handle them separately in a new `make coccicheck-pending` instead. This means that we can separate "critical" patches from "FYI" patches. The former target can continue causing Travis to fail its static analysis job, while the latter can let us keep an eye on ongoing (pending) transitions without them causing too much fallout. Document the intended use-cases around these two targets. As the process around the pending patches is not yet fully explored, leave that out. Based-on-work-by: SZEDER Gábor Signed-off-by: Stefan Beller Signed-off-by: Junio C Hamano --- I dialed back on the workflow, as we may want to explore it first before writing it down. Stefan Makefile | 7 +-- contrib/coccinelle/README | 41 +++ 2 files changed, 46 insertions(+), 2 deletions(-) diff --git a/Makefile b/Makefile index bbfbb4292d..6bc4654828 100644 --- a/Makefile +++ b/Makefile @@ -2740,9 +2740,12 @@ endif then \ echo '' SPATCH result: $@; \ fi -coccicheck: $(addsuffix .patch,$(wildcard contrib/coccinelle/*.cocci)) +coccicheck: $(addsuffix .patch,$(filter-out %.pending.cocci,$(wildcard contrib/coccinelle/*.cocci))) -.PHONY: coccicheck +# See contrib/coccinelle/README +coccicheck-pending: $(addsuffix .patch,$(wildcard contrib/coccinelle/*.pending.cocci)) + +.PHONY: coccicheck coccicheck-pending ### Installation rules diff --git a/contrib/coccinelle/README b/contrib/coccinelle/README index 9c2f8879c2..f0e80bd7f0 100644 --- a/contrib/coccinelle/README +++ b/contrib/coccinelle/README @@ -1,2 +1,43 @@ This directory provides examples of Coccinelle (http://coccinelle.lip6.fr/) semantic patches that might be useful to developers. + +There are two types of semantic patches: + + * Using the semantic transformation to check for bad patterns in the code; + The target 'make coccicheck' is designed to check for these patterns and + it is expected that any resulting patch indicates a regression. + The patches resulting from 'make coccicheck' are small and infrequent, + so once they are found, they can be sent to the mailing list as per usual. + + Example for introducing new patterns: + 67947c34ae (convert "hashcmp() != 0" to "!hasheq()", 2018-08-28) + b84c783882 (fsck: s/++i > 1/i++/, 2018-10-24) + + Example of fixes using this approach: + 248f66ed8e (run-command: use strbuf_addstr() for adding a string to + a strbuf, 2018-03-25) + f919ffebed (Use MOVE_ARRAY, 2018-01-22) + + These types of semantic patches are usually part of testing, c.f. + 0860a7641b (travis-ci: fail if Coccinelle static analysis found something + to transform, 2018-07-23) + + * Using semantic transformations in large scale refactorings throughout + the code base. + + When applying the semantic patch into a real patch, sending it to the + mailing list in the usual way, such a patch would be expected to have a + lot of textual and semantic conflicts as such large scale refactorings + change function signatures that are used widely in the code base. + A textual conflict would arise if surrounding code near any call of such + function changes. A semantic conflict arises when other patch series in + flight introduce calls to such functions. + + So to aid these large scale refactorings, semantic patches can be used. + However we do not want to store them in the same place as the checks for + bad patterns, as then automated builds would fail. + That is why semantic patches 'contrib/coccinelle/*.pending.cocci' + are ignored for checks, and can be applied using 'make coccicheck-pending'. + + This allows to expose plans of pending large scale refactorings without + impacting the bad pattern checks. -- 2.19.1.930.g4563a0d9d0-goog
Re: [PATCH] Makefile: add pending semantic patches
On Thu, Nov 8, 2018 at 9:18 PM Junio C Hamano wrote:
>
> Stefan Beller writes:
>
> > From: SZEDER Gábor
> >
> > Add a description and place on how to use coccinelle for large refactorings
> > that happen only once.
> >
> > Based-on-work-by: SZEDER Gábor
> > Signed-off-by: Stefan Beller
> > ---
> >
> > I consider including this patch in a resend instead.
> > It outlays the basics of such a new workflow, which we can refine later.
>
> Thanks for tying loose ends.
>
> > diff --git a/contrib/coccinelle/README b/contrib/coccinelle/README
> > index 9c2f8879c2..fa09d1abcc 100644
> > --- a/contrib/coccinelle/README
> > +++ b/contrib/coccinelle/README
> > @@ -1,2 +1,62 @@
> > This directory provides examples of Coccinelle (http://coccinelle.lip6.fr/)
> > semantic patches that might be useful to developers.
> > +
> > +There are two types of semantic patches:
> > +
> > + * Using the semantic transformation to check for bad patterns in the code;
> > + This is what the original target 'make coccicheck' is designed to do and
> > + it is expected that any resulting patch indicates a regression.
> > + The patches resulting from 'make coccicheck' are small and infrequent,
> > + so once they are found, they can be sent to the mailing list as per
> > usual.
> > +
> > + Example for introducing new patterns:
> > + 67947c34ae (convert "hashcmp() != 0" to "!hasheq()", 2018-08-28)
> > + b84c783882 (fsck: s/++i > 1/i++/, 2018-10-24)
> > +
> > + Example of fixes using this approach:
> > + 248f66ed8e (run-command: use strbuf_addstr() for adding a string to
> > + a strbuf, 2018-03-25)
> > + f919ffebed (Use MOVE_ARRAY, 2018-01-22)
> > +
> > + These types of semantic patches are usually part of testing, c.f.
> > + 0860a7641b (travis-ci: fail if Coccinelle static analysis found
> > something
> > + to transform, 2018-07-23)
>
> Yup, and I think what we have in 'pu' (including your the_repository
> stuff) falls into this category.
My impression was that the_repository is strongly second category
as the_repository.cocci doesn't fix bad smells of code, but proposes
a refactoring that we agreed on doing.
> I've been paying attention to "make coccicheck" produced patches for
> the past few weeks, and so far, I found it _much_ _much_ more
> pleasant, compared to having to worry about merge conflicts with the
> topics in flight that changes day to day (not just because we add
> new topics or update existing topics, but also the order of the
> merge changes as topics mature at different rates and jumps over
> each other in master..pu history), that "make coccicheck" after
> topics are merged to integration branches fixes these issues up as
> needed.
So from your end we would not need the "pending" category as long
as the follow ups come in a timely manner?
>
> > + 3) Apply the semantic patch only partially, where needed for the patch
> > series
> > + that motivates the large scale refactoring and then build that series
> > + on top of it.
>
> It is not quite clear what "needed for the patch series" really
> means in the context of this paragraph. What are the changes that
> are not needed, that is still produced if we ran "make coccicheck"?
An example for "needed" would be
3f21279f50..bd8737176b
whereas "not needed" is what is in
"treewide: apply cocci patch".
The treewide conversion of e.g. unuse_commit_buffer to
repo_unuse_commit_buffer is nice, but "needed" only in
its followup patch that converts logmsg_reencode as that
calls into the unuse_commit_buffer.
> Are they wrong changes (e.g. a carelessly written read_cache() to
> read_index(&the_index) conversion may munge the implementation of
> read_cache(...) { return read_index(&the_index, ...); } and make
> inifinite recursion)? Or are they "correct but not immediately
> necessary" (e.g. because calling read_cache() does not hurt until
> that function gets removed, so rewriting the callers to call
> read_index() with &the_index may be correct but not immediately
> necessary)?
the latter. I assume correctness (of the semantic patch) to be a given,
but this is all about timing, i.e. how can I send the series without breaking
others in flight.
>
> > + By applying the semantic patch only partially where needed, the
> > series
> > + is less likely to conflict with other series in flight.
>
> That is correct.
>
> > + To make it
Re: [PATCH] Makefile: add pending semantic patches
On Thu, Nov 8, 2018 at 8:56 PM Martin Ågren wrote: > I haven't followed the original discussion too carefully, so I'll read > this like someone new to the topic probably would. Thanks! > A nit, perhaps, but I was genuinely confused at first. The subject is > "Makefile: add pending semantic patches", but this patch doesn't add > any. It adds Makefile-support for such patches though, and it defines > the entire concept of pending semantic patches. How about "coccicheck: > introduce 'pending' semantic patches"? > > > Add a description and place on how to use coccinelle for large refactorings > > that happen only once. > > A bit confused about "and place". Based on my understanding from reading > the remainder of this patch, maybe: > > Teach `make coccicheck` to avoid patches named "*.pending.cocci" and > handle them separately in a new `make coccicheck-pending` instead. > This means that we can separate "critical" patches from "FYI" patches. > The former target can continue causing Travis to fail its static > analysis job, while the latter can let us keep an eye on ongoing > (pending) transitions without them causing too much fallout. > > Document the intended use-cases and processes around these two > targets. Both suggested title and new commit message make sense. > > > This directory provides examples of Coccinelle (http://coccinelle.lip6.fr/) > > semantic patches that might be useful to developers. > > + > > +There are two types of semantic patches: > > + > > + * Using the semantic transformation to check for bad patterns in the code; > > + This is what the original target 'make coccicheck' is designed to do and > > Is it relevant that this was the "original" target? (Genuine question.) No. I can omit that part. > > > + it is expected that any resulting patch indicates a regression. > > + The patches resulting from 'make coccicheck' are small and infrequent, > > + so once they are found, they can be sent to the mailing list as per > > usual. > > + > > + Example for introducing new patterns: > > + 67947c34ae (convert "hashcmp() != 0" to "!hasheq()", 2018-08-28) > > + b84c783882 (fsck: s/++i > 1/i++/, 2018-10-24) > > + > > + Example of fixes using this approach: > > + 248f66ed8e (run-command: use strbuf_addstr() for adding a string to > > + a strbuf, 2018-03-25) > > + f919ffebed (Use MOVE_ARRAY, 2018-01-22) > > + > > + These types of semantic patches are usually part of testing, c.f. > > + 0860a7641b (travis-ci: fail if Coccinelle static analysis found > > something > > + to transform, 2018-07-23) > > Very nicely described, nice with the examples to quickly give a feeling > about how/when to use this. Thanks. > > > + * Using semantic transformations in large scale refactorings throughout > > + the code base. > > + > > + When applying the semantic patch into a real patch, sending it to the > > + mailing list in the usual way, such a patch would be expected to have a > > + lot of textual and semantic conflicts as such large scale refactorings > > + change function signatures that are used widely in the code base. > > + A textual conflict would arise if surrounding code near any call of such > > + function changes. A semantic conflict arises when other patch series in > > + flight introduce calls to such functions. > > OK, I'm with you. > > > + So to aid these large scale refactorings, semantic patches can be used, > > + using the process as follows: > > + > > + 1) Figure out what kind of large scale refactoring we need > > + -> This is usually done by another unrelated series. > > "This"? The figuring out, or the refactoring? Also, "unrelated"? The need and type of what kind of large scale refactoring are usually determined by a patch series, that is not refactoring for the sake of refactoring, but it wants to achieve a specific goal, unrelated to large refactorings per se. The large refactoring is just another tool that a developer can use to make their original series happen much faster. So "unrelated" == "not the large scale refactoring, as that may come as an preparatory series, but to have a preparatory series it may be good to showcase why we need the preparatory series" > > > + 2) Create the sematic patch needed for the large scale refactoring > > s/sematic/semantic/ yup. > > > + and store it in contrib/coccinelle/*.pending
Re: [PATCH] Makefile: add pending semantic patches
Stefan Beller writes:
> From: SZEDER Gábor
>
> Add a description and place on how to use coccinelle for large refactorings
> that happen only once.
>
> Based-on-work-by: SZEDER Gábor
> Signed-off-by: Stefan Beller
> ---
>
> I consider including this patch in a resend instead.
> It outlays the basics of such a new workflow, which we can refine later.
Thanks for tying loose ends.
> diff --git a/contrib/coccinelle/README b/contrib/coccinelle/README
> index 9c2f8879c2..fa09d1abcc 100644
> --- a/contrib/coccinelle/README
> +++ b/contrib/coccinelle/README
> @@ -1,2 +1,62 @@
> This directory provides examples of Coccinelle (http://coccinelle.lip6.fr/)
> semantic patches that might be useful to developers.
> +
> +There are two types of semantic patches:
> +
> + * Using the semantic transformation to check for bad patterns in the code;
> + This is what the original target 'make coccicheck' is designed to do and
> + it is expected that any resulting patch indicates a regression.
> + The patches resulting from 'make coccicheck' are small and infrequent,
> + so once they are found, they can be sent to the mailing list as per usual.
> +
> + Example for introducing new patterns:
> + 67947c34ae (convert "hashcmp() != 0" to "!hasheq()", 2018-08-28)
> + b84c783882 (fsck: s/++i > 1/i++/, 2018-10-24)
> +
> + Example of fixes using this approach:
> + 248f66ed8e (run-command: use strbuf_addstr() for adding a string to
> + a strbuf, 2018-03-25)
> + f919ffebed (Use MOVE_ARRAY, 2018-01-22)
> +
> + These types of semantic patches are usually part of testing, c.f.
> + 0860a7641b (travis-ci: fail if Coccinelle static analysis found something
> + to transform, 2018-07-23)
Yup, and I think what we have in 'pu' (including your the_repository
stuff) falls into this category.
I've been paying attention to "make coccicheck" produced patches for
the past few weeks, and so far, I found it _much_ _much_ more
pleasant, compared to having to worry about merge conflicts with the
topics in flight that changes day to day (not just because we add
new topics or update existing topics, but also the order of the
merge changes as topics mature at different rates and jumps over
each other in master..pu history), that "make coccicheck" after
topics are merged to integration branches fixes these issues up as
needed.
> + 3) Apply the semantic patch only partially, where needed for the patch
> series
> + that motivates the large scale refactoring and then build that series
> + on top of it.
It is not quite clear what "needed for the patch series" really
means in the context of this paragraph. What are the changes that
are not needed, that is still produced if we ran "make coccicheck"?
Are they wrong changes (e.g. a carelessly written read_cache() to
read_index(&the_index) conversion may munge the implementation of
read_cache(...) { return read_index(&the_index, ...); } and make
inifinite recursion)? Or are they "correct but not immediately
necessary" (e.g. because calling read_cache() does not hurt until
that function gets removed, so rewriting the callers to call
read_index() with &the_index may be correct but not immediately
necessary)?
> + By applying the semantic patch only partially where needed, the series
> + is less likely to conflict with other series in flight.
That is correct.
> + To make it possible to apply the semantic patch partially, there needs
> + to be mechanism for backwards compatibility to keep those places
> working
> + where the semantic patch is not applied. This can be done via a
> + wrapper function that has the exact name and signature as the function
> + to be changed.
OK, so this argues for leaving read_cache()-like things to help
other in-flight topics, while a change to encourage the use of
read_index() takes place. That also makes sense, and this directly
relates to "less likely to conflict" benefit you mentioned above.
But it is still unclear to me then what are "necessary".
... goes and thinks ...
OK, so a series that allows a codepath to work on an arbitrary
in-core istate, for example, may need to update a function to take
istate and use it to call read_index(istate...), and the old code in
such a call chain must have used read_cache(), always operating on
&the_index. For the purpose of that series, it does not matter if
other codepaths that are not involved in the callchain the series
wants to update are still only working on &the_index, so a change to
turn read_cache() into read_index(&the_index) is *not* necessary
(but still would be correct) and should be left out of the series.
But any change the series makes to the callchain in question that
turns read_cache() into read_index() with something call-specific
(not &the_index) is a necesary one. Is that a reasonable example
of what these paragraphs wanted to convey with the distinction
between "needed for the patch series" and other changes?
Re: [PATCH] Makefile: add pending semantic patches
On Thu, 8 Nov 2018 at 21:53, Stefan Beller wrote: > > From: SZEDER Gábor > I haven't followed the original discussion too carefully, so I'll read this like someone new to the topic probably would. A nit, perhaps, but I was genuinely confused at first. The subject is "Makefile: add pending semantic patches", but this patch doesn't add any. It adds Makefile-support for such patches though, and it defines the entire concept of pending semantic patches. How about "coccicheck: introduce 'pending' semantic patches"? > Add a description and place on how to use coccinelle for large refactorings > that happen only once. A bit confused about "and place". Based on my understanding from reading the remainder of this patch, maybe: Teach `make coccicheck` to avoid patches named "*.pending.cocci" and handle them separately in a new `make coccicheck-pending` instead. This means that we can separate "critical" patches from "FYI" patches. The former target can continue causing Travis to fail its static analysis job, while the latter can let us keep an eye on ongoing (pending) transitions without them causing too much fallout. Document the intended use-cases and processes around these two targets. > This directory provides examples of Coccinelle (http://coccinelle.lip6.fr/) > semantic patches that might be useful to developers. > + > +There are two types of semantic patches: > + > + * Using the semantic transformation to check for bad patterns in the code; > + This is what the original target 'make coccicheck' is designed to do and Is it relevant that this was the "original" target? (Genuine question.) > + it is expected that any resulting patch indicates a regression. > + The patches resulting from 'make coccicheck' are small and infrequent, > + so once they are found, they can be sent to the mailing list as per usual. > + > + Example for introducing new patterns: > + 67947c34ae (convert "hashcmp() != 0" to "!hasheq()", 2018-08-28) > + b84c783882 (fsck: s/++i > 1/i++/, 2018-10-24) > + > + Example of fixes using this approach: > + 248f66ed8e (run-command: use strbuf_addstr() for adding a string to > + a strbuf, 2018-03-25) > + f919ffebed (Use MOVE_ARRAY, 2018-01-22) > + > + These types of semantic patches are usually part of testing, c.f. > + 0860a7641b (travis-ci: fail if Coccinelle static analysis found something > + to transform, 2018-07-23) Very nicely described, nice with the examples to quickly give a feeling about how/when to use this. > + * Using semantic transformations in large scale refactorings throughout > + the code base. > + > + When applying the semantic patch into a real patch, sending it to the > + mailing list in the usual way, such a patch would be expected to have a > + lot of textual and semantic conflicts as such large scale refactorings > + change function signatures that are used widely in the code base. > + A textual conflict would arise if surrounding code near any call of such > + function changes. A semantic conflict arises when other patch series in > + flight introduce calls to such functions. OK, I'm with you. > + So to aid these large scale refactorings, semantic patches can be used, > + using the process as follows: > + > + 1) Figure out what kind of large scale refactoring we need > + -> This is usually done by another unrelated series. "This"? The figuring out, or the refactoring? Also, "unrelated"? > + 2) Create the sematic patch needed for the large scale refactoring s/sematic/semantic/ > + and store it in contrib/coccinelle/*.pending.cocci > + -> The suffix containing 'pending' is important to differentiate > + this case from the other use case of checking for bad patterns. Good. > + 3) Apply the semantic patch only partially, where needed for the patch > series > + that motivates the large scale refactoring and then build that series > + on top of it. > + By applying the semantic patch only partially where needed, the series > + is less likely to conflict with other series in flight. > + To make it possible to apply the semantic patch partially, there needs > + to be mechanism for backwards compatibility to keep those places > working > + where the semantic patch is not applied. This can be done via a > + wrapper function that has the exact name and signature as the function > + to be changed. > + > + 4) Send the series as usual, including only the needed parts of the > + large scale refactoring Trailing period. OK, I think I get it,
[PATCH] Makefile: add pending semantic patches
From: SZEDER Gábor Add a description and place on how to use coccinelle for large refactorings that happen only once. Based-on-work-by: SZEDER Gábor Signed-off-by: Stefan Beller --- I consider including this patch in a resend instead. It outlays the basics of such a new workflow, which we can refine later. Thanks, Stefan Makefile | 7 +++-- contrib/coccinelle/README | 60 +++ 2 files changed, 65 insertions(+), 2 deletions(-) diff --git a/Makefile b/Makefile index b08d5ea258..e5abfe4cef 100644 --- a/Makefile +++ b/Makefile @@ -2739,9 +2739,12 @@ endif then \ echo '' SPATCH result: $@; \ fi -coccicheck: $(addsuffix .patch,$(wildcard contrib/coccinelle/*.cocci)) +coccicheck: $(addsuffix .patch,$(filter-out %.pending.cocci,$(wildcard contrib/coccinelle/*.cocci))) -.PHONY: coccicheck +# See contrib/coccinelle/README +coccicheck-pending: $(addsuffix .patch,$(wildcard contrib/coccinelle/*.pending.cocci)) + +.PHONY: coccicheck coccicheck-pending ### Installation rules diff --git a/contrib/coccinelle/README b/contrib/coccinelle/README index 9c2f8879c2..fa09d1abcc 100644 --- a/contrib/coccinelle/README +++ b/contrib/coccinelle/README @@ -1,2 +1,62 @@ This directory provides examples of Coccinelle (http://coccinelle.lip6.fr/) semantic patches that might be useful to developers. + +There are two types of semantic patches: + + * Using the semantic transformation to check for bad patterns in the code; + This is what the original target 'make coccicheck' is designed to do and + it is expected that any resulting patch indicates a regression. + The patches resulting from 'make coccicheck' are small and infrequent, + so once they are found, they can be sent to the mailing list as per usual. + + Example for introducing new patterns: + 67947c34ae (convert "hashcmp() != 0" to "!hasheq()", 2018-08-28) + b84c783882 (fsck: s/++i > 1/i++/, 2018-10-24) + + Example of fixes using this approach: + 248f66ed8e (run-command: use strbuf_addstr() for adding a string to + a strbuf, 2018-03-25) + f919ffebed (Use MOVE_ARRAY, 2018-01-22) + + These types of semantic patches are usually part of testing, c.f. + 0860a7641b (travis-ci: fail if Coccinelle static analysis found something + to transform, 2018-07-23) + + * Using semantic transformations in large scale refactorings throughout + the code base. + + When applying the semantic patch into a real patch, sending it to the + mailing list in the usual way, such a patch would be expected to have a + lot of textual and semantic conflicts as such large scale refactorings + change function signatures that are used widely in the code base. + A textual conflict would arise if surrounding code near any call of such + function changes. A semantic conflict arises when other patch series in + flight introduce calls to such functions. + + So to aid these large scale refactorings, semantic patches can be used, + using the process as follows: + + 1) Figure out what kind of large scale refactoring we need + -> This is usually done by another unrelated series. + 2) Create the sematic patch needed for the large scale refactoring + and store it in contrib/coccinelle/*.pending.cocci + -> The suffix containing 'pending' is important to differentiate + this case from the other use case of checking for bad patterns. + 3) Apply the semantic patch only partially, where needed for the patch series + that motivates the large scale refactoring and then build that series + on top of it. + By applying the semantic patch only partially where needed, the series + is less likely to conflict with other series in flight. + To make it possible to apply the semantic patch partially, there needs + to be mechanism for backwards compatibility to keep those places working + where the semantic patch is not applied. This can be done via a + wrapper function that has the exact name and signature as the function + to be changed. + + 4) Send the series as usual, including only the needed parts of the + large scale refactoring + + Later steps (not necessarily by the original author) are to apply the + semantic patch in a way that do not produce lots of conflicts, for example + by consulting `git diff --numstat origin/master..origin/pu` and the changes + of the semantic patch. -- 2.19.1.930.g4563a0d9d0-goog
Re: [PATCH 01/24] Makefile: add pending semantic patches
Stefan Beller writes: > [Missing: SZEDERs sign off, so I also do not sign off] At least to me, based on my reading of DCO in Documentation/SubmittingPatches, this reasoning does not make much sense.
[PATCH 01/24] Makefile: add pending semantic patches
From: SZEDER Gábor There are basically two main use cases for semantic patches: - To avoid undesirable code patterns, e.g. we should not use sha1_to_hex(oid.hash) or strbuf_addf(&sb, "fixed string"), but use oid_to_hex(&oid) or strbuf_addstr(&sb, "fixed string") instead. Note that in these cases we don't remove the functions sha1_to_hex() or strbuf_addf(), because there are good reasons to use them in other scenarios. Our semantic patches under 'contrib/coccinelle/' fall into this category, and we have 'make coccicheck' and the static analysis build job on Travis CI to catch these undesirable code patterns preferably early, and to prevent them from entering our codebase. - To perform one-off code transformations, e.g. to modify a function's name and/or signature and convert all its callsites; see e.g. commits abef9020e3 (sha1_file: convert sha1_object_info* to object_id, 2018-03-12) and b4f5aca40e (sha1_file: convert read_sha1_file to struct object_id, 2018-03-12). To allows semantic patches of the second category, we'll introduce the concept of "pending" semantic patches, stored in 'contrib/coccinelle/.pending.cocci' files, modifying 'make coccicheck' to skip them, and adding the new 'make coccicheck-pending' target to make it convenient to apply them. [Missing: SZEDERs sign off, so I also do not sign off] --- Makefile | 6 -- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/Makefile b/Makefile index b08d5ea258..6ea2bbd7f5 100644 --- a/Makefile +++ b/Makefile @@ -2739,9 +2739,11 @@ endif then \ echo '' SPATCH result: $@; \ fi -coccicheck: $(addsuffix .patch,$(wildcard contrib/coccinelle/*.cocci)) +coccicheck: $(addsuffix .patch,$(filter-out %.pending.cocci,$(wildcard contrib/coccinelle/*.cocci))) -.PHONY: coccicheck +coccicheck-pending: $(addsuffix .patch,$(wildcard contrib/coccinelle/*.pending.cocci)) + +.PHONY: coccicheck coccicheck-pending ### Installation rules -- 2.19.1.930.g4563a0d9d0-goog
Re: New semantic patches vs. in-flight topics [was: Re: [PATCH 00/19] Bring more repository handles into our code base]
On Wed, Oct 24, 2018 at 6:59 PM SZEDER Gábor wrote: > > On Mon, Oct 22, 2018 at 11:54:06AM -0700, Stefan Beller wrote: > > > For the sake of a good history, I would think running 'make coccicheck' > > and applying the resulting patches would be best as part of the (dirty) > > merge of any topic that proposes new semantic patches, but that would > > add load to Junio as it would be an extra step during the merge. > > > > One could argue that the step of applying such transformations into > > the dirty merge is cheaper than resolving merge conflicts that are > > had when the topic includes the transformation. > > Please consider that merge commits' have uglier diffs than regular > commits, and that merge commits cause additional complications when > 'git bisect' points the finger at them, both of which are exacerbated > by additional changes squeezed into evil merges. > > > > Consequently, 'make coccicheck' won't run clean and the > > > static analysis build job will fail until all those topics reach > > > 'master', and the remaining transformations are applied on top. > > > > > > This was (and still is!) an issue with the hasheq()/oideq() series > > > as well: that series was added on 2018-08-28, and the static > > > analysis build job is red on 'pu' ever since. See the follow-up > > > patch e43d2dcce1 (more oideq/hasheq conversions, 2018-10-02), and > > > one more follow-up will be necessary after the builtin stash topic > > > is merged to 'master'. > > > > In my understanding this follow up is a feature, as it helps to avoid > > merge conflicts with other topics in flight. > > I don't see how such a follow up patch helps to avoid merge conflicts. Well, you merge first (the new topic and the cocci patches), and then do the transformation. But that is putting a lot more work on Junio as the way to integrate is not just merge a new topic into the pile of topics (whether it is pu/next/master), but to first perform a merge of the topic and the coccinelle patches, apply the transformation and then merge to the pile, assuming the pile is already transformed (as it happened in "treewide: apply cocci patch" in next/pu). > > as 'make coccicheck' is an integral part of your review? > > Erm, right, "review" was not the right word here. Anyway, as it is, > 'make coccicheck' is an integral part of our automated tests, not only > on Travis CI but on the upcoming Azure thing as well. I just try to > pay attention to its results and the results of a bunch of my > additional builds, and complain or even send a fix when something goes > reproducibly wrong. This has certainly became more cumbersome with > the permanently failing static analysis build job in the last couple > of weeks. I seem to not pay as much attention as I should to these, mostly because they are unreliable on the aggregate level (a failure there most likely means another topic than the one I am interested broke; except in this case, where we discuss the fallout there via this topic.) > > I like the approach of having separate classes of semantic patches: > > (a) the regular "we need to keep checking these" as they address > > undesirable code patterns, which is what we currently have, > > and what 'make coccicheck' would complain about. > > (b) The pending patches as you propose. However I would > > argue that we'd not want to include the transformation into > > the same patch as then the patch will have merge conflicts. > > Since we have a lot of parallel running topics, merge conflicts are > basically unavoidable anyway. If the conflicts from the > transformation are really that severe, then perhaps the whole series > should be postponed to a calmer, more suitable time. Merge conflicts of this kind could be avoided, by running the transformation on both sides before merging (or not running them on both sides, but only after merging). So maybe for these larger 'the_repository.pending.cocci' patches we could get them into the tree and wait until all (most) of the topics are including that semantic patch in their base, such that application of the patch is easy whether before or after writing a series (as the semantic patch is in its base). Another short term plan would be renaming the_repository.cocci such that it would break the 'make coccicheck' > In the case of 'the_repository.cocci', merging its transformations > into 'pu' resulted in only four conflicts, and I found all four on the > easy side to resolve. I don't
[PATCH v2 0/3] Allow choosing the SSL backend cURL uses (plus related patches)
This topic branch brings support for choosing cURL's SSL backend (a feature
developed in Git for Windows' context) at runtime via http.sslBackend, and
two more patches that are related (and only of interest for Windows users).
Changes since v1:
* Reworded the commit message of v1's patch 2/3, to talk about the original
design instead of "an earlier iteration" that was never contributed to
the Git mailing list.
* Changed the confusing >= 7.44.0 to < 7.44.0.
Note: I had prepared
https://github.com/dscho/git/commit/81e8c9a4006c919747a0b6a287f28f25799fcaf4
, intended to be included in v2, but Junio came up with
https://public-inbox.org/git/xmqqsh0uln5c.fsf...@gitster-ct.c.googlers.com/
in the meantime, which I like better.
Brendan Forster (1):
http: add support for disabling SSL revocation checks in cURL
Johannes Schindelin (2):
http: add support for selecting SSL backends at runtime
http: when using Secure Channel, ignore sslCAInfo by default
Documentation/config.txt | 21
http.c | 71 +++-
2 files changed, 91 insertions(+), 1 deletion(-)
base-commit: 5a0cc8aca797dbd7d2be3b67458ff880ed45cddf
Published-As:
https://github.com/gitgitgadget/git/releases/tags/pr-46%2Fdscho%2Fhttp-ssl-backend-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git
pr-46/dscho/http-ssl-backend-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/46
Range-diff vs v1:
1: 8c5ecdb6c = 1: 85bd0fb27 http: add support for selecting SSL backends at
runtime
2: 764791d13 ! 2: 951383695 http: add support for disabling SSL revocation
checks in cURL
@@ -14,10 +14,10 @@
This is only supported in cURL 7.44 or later.
-Note: an earlier iteration tried to use the config setting
-http.schannel.checkRevoke, but the http.* config settings can be
limited
-to specific URLs via http..* (which would mistake `schannel` for
a
-URL).
+Note: originally, we wanted to call the config setting
+`http.schannel.checkRevoke`. This, however, does not work: the
`http.*`
+config settings can be limited to specific URLs via `http..*`
+(and this feature would mistake `schannel` for a URL).
Helped by Agustín Martín Barbero.
@@ -77,7 +77,7 @@
+ curl_easy_setopt(result, CURLOPT_SSL_OPTIONS,
CURLSSLOPT_NO_REVOKE);
+#else
+ warning("CURLSSLOPT_NO_REVOKE not applied to curl SSL options
because\n"
-+ "your curl version is too old (>= 7.44.0)");
++ "your curl version is too old (< 7.44.0)");
+#endif
+ }
+
3: 9927e4ce6 = 3: a5f937a36 http: when using Secure Channel, ignore
sslCAInfo by default
--
gitgitgadget
Re: New semantic patches vs. in-flight topics [was: Re: [PATCH 00/19] Bring more repository handles into our code base]
On Mon, Oct 22, 2018 at 07:39:35PM +0200, SZEDER Gábor wrote: > I don't really like how this or the previous RFC patch series deal > with semantic patches (or how some past patch series dealt with them, > for that matter), for various reasons: > [..] I am a little late to this thread, but it really seems to me that the crux of the issue here: > - 'make coccicheck' won't run clean (and the static analysis build > job on Travis CI will fail) for all commits following adding the > new semantic patches but before applying the resulting > transformations. > > - These semantic patches interact badly with 'pu' and 'next', > because those integration branches can contain topic branches > adding new code that should be transformed by these semanic > patches. Consequently, 'make coccicheck' won't run clean and the > static analysis build job will fail until all those topics reach > 'master', and the remaining transformations are applied on top. Is that we are considering it a failure for "pu" to have pending coccinelle patches. Are there any coccicheck transformations that aren't just "we prefer to do it like X instead of Y"? Changes that actually break things should be caught by the compiler (either by their nature, or because we take care to rename or change interfaces when making a semantic change to a function). I think that's getting at what you're saying here: > - Is it really necessary to carry these semantic patches in-tree? > Let me ellaborate. There are basically two main use cases for > semantic patches: > > - To avoid undesirable code patterns, e.g. we should not use > [...] > - To perform one-off code transformations, e.g. to modify a I am mostly thinking of your first type here. And I think it makes sense for people to avoid submitting code that does not pass "make coccicheck" _as it was at the state of their branch_. But for cocci changes that are in flight at the same time as their branch, I do not see any need to strictly adhere to them. The code is "undesirable", not "wrong". For the second type, I agree that we do not necessarily need to carry them in-tree. Eventually the transformation happens, and nobody would use the "old" way because it doesn't work anymore. Problem solved. I do not mind carrying them for a while as a convenience to people who will need to fix up their topics in flight (or more likely to the maintainer, who will need to fixup the merge). But we should make sure not to forget to remove them when their usefulness has passed. Likewise, we should not forget to occasionally run "make coccicheck" on master to make sure people have a clean base to build on. If Junio is able to do that, then great. But other people can also do so and submit patches (to go on their respective topics, or to be a new mass-cleanup topic). I guess there is some lag there if Junio pushes out a master branch that fails coccicheck, because contributors may build on it before somebody gets around to fixing it. > Having said that, it's certainly easier to double-check the > resulting transformations when one can apply the semantic > patches locally, and doing so is easier when the semantic > patches are in tree than when they must be copy-pasted from a > commit message. I've wondered if we could have a script that pulls a coccinelle snippet from a commit message and runs it. It may be a hassle to find the right commit, though (you'd start the procedure from "oops, my compile now seems broken; what was the change that I need to apply to adapt?"). > - A new semantic patch should be added as "pending", e.g. to the > file 'the_repository.pending.cocci', together with the resulting > transformations in the same commit. > > This way neither 'make coccicheck' nor the static analysis build > job would complain in the topic branch or in the two integration > branches. And if they do complain, then we would know right away > that they complain because of a well-established semantic patch. > Yet, anyone interested could run 'make coccicheck-pending' to see > where are we heading. OK, makes sense. > - The author of the "pending" semanting patch should then keep an > eye on already cooking topics: whether any of them contain new > code that should be transformed, and how they progress to > 'master', and sending followup patch(es) with the remaining > transformations when applicable. > > Futhermore, the author should also pay attention to any new topics > that branch off af
Re: New semantic patches vs. in-flight topics [was: Re: [PATCH 00/19] Bring more repository handles into our code base]
On Mon, Oct 22, 2018 at 11:54:06AM -0700, Stefan Beller wrote: > For the sake of a good history, I would think running 'make coccicheck' > and applying the resulting patches would be best as part of the (dirty) > merge of any topic that proposes new semantic patches, but that would > add load to Junio as it would be an extra step during the merge. > > One could argue that the step of applying such transformations into > the dirty merge is cheaper than resolving merge conflicts that are > had when the topic includes the transformation. Please consider that merge commits' have uglier diffs than regular commits, and that merge commits cause additional complications when 'git bisect' points the finger at them, both of which are exacerbated by additional changes squeezed into evil merges. > > Consequently, 'make coccicheck' won't run clean and the > > static analysis build job will fail until all those topics reach > > 'master', and the remaining transformations are applied on top. > > > > This was (and still is!) an issue with the hasheq()/oideq() series > > as well: that series was added on 2018-08-28, and the static > > analysis build job is red on 'pu' ever since. See the follow-up > > patch e43d2dcce1 (more oideq/hasheq conversions, 2018-10-02), and > > one more follow-up will be necessary after the builtin stash topic > > is merged to 'master'. > > In my understanding this follow up is a feature, as it helps to avoid > merge conflicts with other topics in flight. I don't see how such a follow up patch helps to avoid merge conflicts. There were topics that branched off before the introduction of oideq() into 'master', therefore they couldn't make use of this new function until they were merged to 'master' as well, so they added their own !oidcmp() calls. That follow up patch was necessary to transform these new !oidcmp() calls after those topics reached 'master'. Merge conflicts had nothing to do with it. So this follow up patch is not a feature, but rather an inherent consequence of the project's branching model, with lots of parallel running topics branching off at different points and progressing at different speeds. > > This makes it harder to review other patch series. > > as 'make coccicheck' is an integral part of your review? Erm, right, "review" was not the right word here. Anyway, as it is, 'make coccicheck' is an integral part of our automated tests, not only on Travis CI but on the upcoming Azure thing as well. I just try to pay attention to its results and the results of a bunch of my additional builds, and complain or even send a fix when something goes reproducibly wrong. This has certainly became more cumbersome with the permanently failing static analysis build job in the last couple of weeks. > > How about introducing the concept of "pending" semantic patches, > > stored in 'contrib/coccinelle/.pending.cocci' files, modifying > > 'make coccicheck' to skip them, and adding the new 'make > > coccicheck-pending' target to make it convenient to apply them, e.g. > > something like the simple patch at the end. > > > > So the process would go something like this: > > > > - A new semantic patch should be added as "pending", e.g. to the > > file 'the_repository.pending.cocci', together with the resulting > > transformations in the same commit. > > > > This way neither 'make coccicheck' nor the static analysis build > > job would complain in the topic branch or in the two integration > > branches. And if they do complain, then we would know right away > > that they complain because of a well-established semantic patch. > > Yet, anyone interested could run 'make coccicheck-pending' to see > > where are we heading. > > > > - The author of the "pending" semanting patch should then keep an > > eye on already cooking topics: whether any of them contain new > > code that should be transformed, and how they progress to > > 'master', and sending followup patch(es) with the remaining > > transformations when applicable. > > > > Futhermore, the author should also pay attention to any new topics > > that branch off after the "pending" semantic patch, and whether > > any of them introduce code to be transformed, warning their > > authors as necessary. > > > > - Finally, after all the dust settled, the dev should follow up with > > a patc
Re: New semantic patches vs. in-flight topics [was: Re: [PATCH 00/19] Bring more repository handles into our code base]
Stefan Beller writes: >> Anyway, even though "make coccicheck" does not run in subsecond, >> I've updated my machinery to rebuild the integration branches so >> that I can optionally queue generated coccicheck patches, and what I >> pushed out tonight has one at the tip of 'pu' and also another at >> the tip of 'next'. The latter seems to be passing all archs and >> executing Windows run. > > That is pretty exciting! > > Looking at the commit in next, you also included the suggestion > from [1] to use a postincrement instead of a preincrement and I got > excited to see how we express such a thing in coccinelle, > but it turns out that it slipped in unrelated to the coccinelle patches. See below, which was sitting in my working tree. > How would we go from here? > It is not obvious to me how such changes would be integrated, > as regenerating them on top of pu will not help getting these changes > merged down, and applying the semantic patch on next (once > sb/more-repo-in-api lands in next) would created the merge conflicts for > all topics that are merged to next after that series. Conflicts with later topics is indeed worrysome. That is why I did it as an experiment. If it becomes too painful, I'd probably stop doing it while merging to anything other than 'pu', and then we can follow the more distributed approach along the lines of what Szeder suggested, to see how smoothly it goes. -- >8 -- Subject: [PATCH] cocci: simplify "if (++u > 1)" to "if (u++)" It is more common to use post-increment than pre-increment when the side effect is the primary thing we want in our code and in C in general (unlike C++). Initializing a variable to 0, incrementing it every time we do something, and checking if we have already done that thing to guard the code to do that thing, is easier to understand when written if (u++) ; /* we've done that! */ else do_it(); /* just once. */ but if you try to use pre-increment, you end up with a less natural looking if (++u > 1) Signed-off-by: Junio C Hamano --- contrib/coccinelle/preincr.cocci | 5 + 1 file changed, 5 insertions(+) create mode 100644 contrib/coccinelle/preincr.cocci diff --git a/contrib/coccinelle/preincr.cocci b/contrib/coccinelle/preincr.cocci new file mode 100644 index 00..7fe1e8d2d9 --- /dev/null +++ b/contrib/coccinelle/preincr.cocci @@ -0,0 +1,5 @@ +@ preincrement @ +identifier i; +@@ +- ++i > 1 ++ i++ -- 2.19.1-542-gc4df23f792
Re: New semantic patches vs. in-flight topics [was: Re: [PATCH 00/19] Bring more repository handles into our code base]
On Tue, Oct 23, 2018 at 2:38 AM Junio C Hamano wrote: > > Junio C Hamano writes: > > > I actually think this round does a far nicer job playing well with > > other topics than any earlier series. The pain you are observing I > > think come primarily from my not making the best use of these > > patches. > > > > Steppng back a bit, I'd imagine in an ideal world where "make > > coccicheck" can be done instantaneously _and_ the spatch machinery > > is just as reliable as C compilers > > > > What I _could_ do (and what I did do only for one round of pushing > > out 'pu') is to queue a coccinelle transformation at the tip of > > integration branches. If "make coccicheck" ran in subsecond, I > > could even automate it in the script that is used to rebuild 'pu' > > every day, ... > > Anyway, even though "make coccicheck" does not run in subsecond, > I've updated my machinery to rebuild the integration branches so > that I can optionally queue generated coccicheck patches, and what I > pushed out tonight has one at the tip of 'pu' and also another at > the tip of 'next'. The latter seems to be passing all archs and > executing Windows run. That is pretty exciting! Looking at the commit in next, you also included the suggestion from [1] to use a postincrement instead of a preincrement and I got excited to see how we express such a thing in coccinelle, but it turns out that it slipped in unrelated to the coccinelle patches. How would we go from here? It is not obvious to me how such changes would be integrated, as regenerating them on top of pu will not help getting these changes merged down, and applying the semantic patch on next (once sb/more-repo-in-api lands in next) would created the merge conflicts for all topics that are merged to next after that series. [1] https://public-inbox.org/git/xmqqin1wyxvz@gitster-ct.c.googlers.com/
Re: New semantic patches vs. in-flight topics [was: Re: [PATCH 00/19] Bring more repository handles into our code base]
Carlo Arenas writes: > On Tue, Oct 23, 2018 at 2:40 AM Junio C Hamano wrote: >> >> The tip of 'pu' has trouble with -Wunused on Apple around the >> delta-islands series. > > FWIW the "problem" is actually with -Wunused-function and is AFAIK not > related to the semantic changes or Apple (AKA macOS) Oh, don't get me wrong. By Apple, I meant "the versions of compiler used on the Apple build at TravisCI site". I could have sent the above two lines in a separate topic, as the issue does not have anything to do with "new semantic patches" discussion, but I thought it would be obvious to everybody who is reading the thread---it seems I thought wrong ;-)
Re: New semantic patches vs. in-flight topics [was: Re: [PATCH 00/19] Bring more repository handles into our code base]
On Tue, Oct 23, 2018 at 2:40 AM Junio C Hamano wrote: > > The tip of 'pu' has trouble with -Wunused on Apple around the > delta-islands series. FWIW the "problem" is actually with -Wunused-function and is AFAIK not related to the semantic changes or Apple (AKA macOS) Indeed, I saw this issue before also in Linux (Fedora Rawhide) when using the latest clang and presumed it was just expected because the topic branch was most likely incomplete. Carlo
Re: New semantic patches vs. in-flight topics [was: Re: [PATCH 00/19] Bring more repository handles into our code base]
Junio C Hamano writes: > I actually think this round does a far nicer job playing well with > other topics than any earlier series. The pain you are observing I > think come primarily from my not making the best use of these > patches. > > Steppng back a bit, I'd imagine in an ideal world where "make > coccicheck" can be done instantaneously _and_ the spatch machinery > is just as reliable as C compilers > > What I _could_ do (and what I did do only for one round of pushing > out 'pu') is to queue a coccinelle transformation at the tip of > integration branches. If "make coccicheck" ran in subsecond, I > could even automate it in the script that is used to rebuild 'pu' > every day, ... Anyway, even though "make coccicheck" does not run in subsecond, I've updated my machinery to rebuild the integration branches so that I can optionally queue generated coccicheck patches, and what I pushed out tonight has one at the tip of 'pu' and also another at the tip of 'next'. The latter seems to be passing all archs and executing Windows run. The tip of 'pu' has trouble with -Wunused on Apple around the delta-islands series. next: https://travis-ci.org/git/git/builds/444969406 pu: https://travis-ci.org/git/git/builds/444969342
Re: New semantic patches vs. in-flight topics [was: Re: [PATCH 00/19] Bring more repository handles into our code base]
Stefan Beller writes: > Am I overestimating or misunderstanding rerere here? Yes. > Would it be realistic for next and master branch instead of pu? > > I'd be wary for the master branch, as we may not want to rely on > spatch without review. (It can produce funny white space issues, > but seems to produce working/correct code) Yes, that is why I think it is a bit too late to do the "fixup with spatch" after merging to 'next' and 'master'.
Re: New semantic patches vs. in-flight topics [was: Re: [PATCH 00/19] Bring more repository handles into our code base]
> Stepping back a bit, I'd imagine in an ideal world where "make
> coccicheck" can be done instantaneously _and_ the spatch machinery
> is just as reliable as C compilers.
> [...]
> Now we do not live in that ideal world and [...]
> such a series will have zero
> chance of landing in 'pu', unless we freeze the world.
I wonder if we could approximate the ideal world with
rerere+spatch a bit more:
1) I resend the series that includes "apply cocci patches"
as the last patch and you queue it as usual
2) The first time such a series is merged, you'd merge
HEAD^ (i.e. excluding the "apply the semantic patch)
to pu instead. I view this as a yet-to-be invented mode
'--theirs-is-stale-use-tree-instead=THEIRS~1^{tree}',
then run spatch to reproduce the last patch into the
dirty merge (which has pu and the last patch as parent)
This step is done to 'pre-heat' the rerere cache.
3) Any further integration (e.g. rebuilding pu) would
benefit from the hot rerere cache and very little work
is actually required as the conflicts are resolved by rerere.
Am I overestimating or misunderstanding rerere here?
> What I _could_ do (and what I did do only for one round of pushing
> out 'pu') is to queue a coccinelle transformation at the tip of
> integration branches. If "make coccicheck" ran in subsecond, I
> could even automate it in the script that is used to rebuild 'pu'
> every day, so that after merging each and every topic, do the "make
> coccicheck" and apply resulting .cocci.patch files and squash that
> into the merge commit.
>
> But with the current "make coccicheck" performance, that is not
> realistic.
Would it be realistic for next and master branch instead of pu?
I'd be wary for the master branch, as we may not want to rely on
spatch without review. (It can produce funny white space issues,
but seems to produce working/correct code)
> I am wondering if it is feasible to do it at the tip of 'pu' (which
> is rebuilt two to three times a day), 'next' (which is updated once
> or twice a week) and 'master'.
We could even optimize that, by checking if contrib/cocci/ has
changes for the new tip of next/master respectively.
Another thing I wonder is if we care about the distinction between
the (a) pending changes as described by SZEDER, as we introduce
these deliberately, whereas (b) undesirable code patterns
(e.g. free and null instead of FREE_AND_NULL macro) might be
caught and reported in pu/next and then someone learns from it.
Automatic rewriting the (b) cases seems not just as desirable as
(a), where we do it purely to avoid resolving merge conflicts by
hand.
> I find that your "pending" idea may be nicer, as it distributes the
> load. Whoever wants to change the world order by updating the .cocci
> rules is primarily responsible for making it happen without breaking
> the world during the transition. That's more scalable.
... and I think SZEDER considers the current world broken as
'make coccicheck' returns non-empty, so it sounds to me as if
the current transition is thought less-than-optimal.
Re: New semantic patches vs. in-flight topics [was: Re: [PATCH 00/19] Bring more repository handles into our code base]
SZEDER Gábor writes:
> I don't really like how this or the previous RFC patch series deal
> with semantic patches (or how some past patch series dealt with them,
> for that matter), for various reasons:
> ...
> How about introducing the concept of "pending" semantic patches,
> stored in 'contrib/coccinelle/.pending.cocci' files, modifying
> 'make coccicheck' to skip them, and adding the new 'make
> coccicheck-pending' target to make it convenient to apply them, e.g.
> something like the simple patch at the end.
>
> So the process would go something like this:
>
> - A new semantic patch should be added as "pending", e.g. to the
> file 'the_repository.pending.cocci', together with the resulting
> transformations in the same commit.
>
> This way neither 'make coccicheck' nor the static analysis build
> job would complain in the topic branch or in the two integration
> branches. And if they do complain, then we would know right away
> that they complain because of a well-established semantic patch.
> Yet, anyone interested could run 'make coccicheck-pending' to see
> where are we heading.
>
> - The author of the "pending" semanting patch should then keep an
> eye on already cooking topics: whether any of them contain new
> code that should be transformed, and how they progress to
> 'master', and sending followup patch(es) with the remaining
> transformations when applicable.
>
> Futhermore, the author should also pay attention to any new topics
> that branch off after the "pending" semantic patch, and whether
> any of them introduce code to be transformed, warning their
> authors as necessary.
>
> - Finally, after all the dust settled, the dev should follow up with
> a patch to:
>
> - promote the "penging" patch to '.cocci', if its purpose
> is to avoid undesirable code patterns in the future, or
>
> - remove the semantic patch, if it was used in a one-off
> transformation.
> ...
> Thoughts?
I actually think this round does a far nicer job playing well with
other topics than any earlier series. The pain you are observing I
think come primarily from my not making the best use of these
patches.
Steppng back a bit, I'd imagine in an ideal world where "make
coccicheck" can be done instantaneously _and_ the spatch machinery
is just as reliable as C compilers. In such a world, I may rename
all the *.c files in my tree to *.c.in, make the very first step of
the "make all" to run coccicheck and transform *.c.in to *.c before
starting the compilation. There is no need to have changes to
*.c.in that spatch would fix. Such an idealized set-up has a few
nice property.
- Mechanical conflict resolutions between topics in flight and a
series that modifies or adds new .cocci rules would greatly be
reduced.
- Still, *.c files that are "compiled" from *.c.in file used by
each build will by definition cocci-clean.
- Bugs like the one that required 6afedba8 ("object_id.cocci: match
only expressions of type 'struct object_id'", 2018-10-15) are
still possible, but some of them may be caught by C compilers
that inspect the result of spatch compilation from *.c.in to *.c
Now we do not live in that ideal world and there is no separate
"turn *.c.in into *.c" step in our build procedure. In such a
"real" world, if we had a rule like "each individual commit must
pass 'make coccicheck' cleanly", anybody who does such a merge and
resolve huge conflics would essentially be wasting time on something
that the tool could do, and then the result of the merge would
further need to be amended for semantic conflicts (i.e. the other
branch may have introduced new instances of old pattern .cocci
patches in our branch would want to transform)). By not insisting
on cocci-cleanness at each commit level, we can gain (or at least
sumulate gaining) some benefit that we would reap in the ideal
world, and Stefan's latest series already does so for the former.
If we insisted that these patches must be accompanied with the
result of the cocci transformations, such a series will have zero
chance of landing in 'pu', unless we freeze the world.
What I _could_ do (and what I did do only for one round of pushing
out 'pu') is to queue a coccinelle transformation at the tip of
integration branches. If "make coccicheck" ran in subsecond, I
could even automate it in the script that is used to rebuild 'pu'
every day, so that after merging each and every topic, do the "make
coccicheck" and apply resulting
Re: New semantic patches vs. in-flight topics [was: Re: [PATCH 00/19] Bring more repository handles into our code base]
On Mon, Oct 22, 2018 at 10:39 AM SZEDER Gábor wrote: > > On Tue, Oct 16, 2018 at 04:35:31PM -0700, Stefan Beller wrote: > > the last patch (applying the semantic patches) has been omitted as that > > would produce a lot of merge conflicts. Without that patch, this merges > > cleanly to next. > > > > As for when to apply the semantic patches, I wondered if we would prefer a > > dirty merge > > (created via "make coccicheck && git apply > > contrib/coccinelle/the_repository.cocci.patch") > > of the semantic patches, or if we'd actually trickle in the changes over > > some time? > > > This series takes another approach as it doesn't change the signature of > > functions, but introduces new functions that can deal with arbitrary > > repositories, keeping the old function signature around using a shallow > > wrapper. > > > > Additionally each patch adds a semantic patch, that would port from the > > old to > > the new function. These semantic patches are all applied in the very > > last patch, > > but we could omit applying the last patch if it causes too many merge > > conflicts > > and trickl in the semantic patches over time when there are no merge > > conflicts. > > I don't really like how this or the previous RFC patch series deal > with semantic patches (or how some past patch series dealt with them, > for that matter), for various reasons: > > - Applying the transformations from several semantic patches in one > single patch makes it harder to review it, because we won't know > which change came from which semantic patch. Good point, so to improve the series sb/more-repo-in-api, I could send the application of the semantic patch just after each patch that adds another semantic patching rule? I personally dislike applying the semantic patch in the same patch as where the semantic rule was introduced, as then the mechanical conversions (from the semantic patch) drown out reviewers attention to the manual changes. > For comparison, see the patch series adding hasheq()/oideq(), > merged in 769af0fd9e (Merge branch 'jk/cocci', 2018-09-17), in > particular the four "convert to " patches. The four patches "convert to only contain the semantic patch as well as its effects, the manual changes are separated out to later patches, which is quite nice. > - 'make coccicheck' won't run clean (and the static analysis build > job on Travis CI will fail) for all commits following adding the > new semantic patches but before applying the resulting > transformations. > > - These semantic patches interact badly with 'pu' and 'next', > because those integration branches can contain topic branches > adding new code that should be transformed by these semanic > patches. And I thought of this as a feature. There is no merge conflict and it still compiles, which makes Junios work easy. Of course it put the load elsewhere. :/ For the sake of a good history, I would think running 'make coccicheck' and applying the resulting patches would be best as part of the (dirty) merge of any topic that proposes new semantic patches, but that would add load to Junio as it would be an extra step during the merge. One could argue that the step of applying such transformations into the dirty merge is cheaper than resolving merge conflicts that are had when the topic includes the transformation. > Consequently, 'make coccicheck' won't run clean and the > static analysis build job will fail until all those topics reach > 'master', and the remaining transformations are applied on top. > > This was (and still is!) an issue with the hasheq()/oideq() series > as well: that series was added on 2018-08-28, and the static > analysis build job is red on 'pu' ever since. See the follow-up > patch e43d2dcce1 (more oideq/hasheq conversions, 2018-10-02), and > one more follow-up will be necessary after the builtin stash topic > is merged to 'master'. In my understanding this follow up is a feature, as it helps to avoid merge conflicts with other topics in flight. > This makes it harder to review other patch series. as 'make coccicheck' is an integral part of your review? > - Is it really necessary to carry these semantic patches in-tree? > Let me ellaborate. There are basically two main use cases for > semantic patches: > > - To avoid undesirable code patterns, e.g. we should not use > sha1_to_hex(oid.hash) or strbuf_addf(&sb, "fixed string"), but > us
New semantic patches vs. in-flight topics [was: Re: [PATCH 00/19] Bring more repository handles into our code base]
On Tue, Oct 16, 2018 at 04:35:31PM -0700, Stefan Beller wrote:
> the last patch (applying the semantic patches) has been omitted as that
> would produce a lot of merge conflicts. Without that patch, this merges
> cleanly to next.
>
> As for when to apply the semantic patches, I wondered if we would prefer a
> dirty merge
> (created via "make coccicheck && git apply
> contrib/coccinelle/the_repository.cocci.patch")
> of the semantic patches, or if we'd actually trickle in the changes over some
> time?
> This series takes another approach as it doesn't change the signature of
> functions, but introduces new functions that can deal with arbitrary
> repositories, keeping the old function signature around using a shallow
> wrapper.
>
> Additionally each patch adds a semantic patch, that would port from the
> old to
> the new function. These semantic patches are all applied in the very last
> patch,
> but we could omit applying the last patch if it causes too many merge
> conflicts
> and trickl in the semantic patches over time when there are no merge
> conflicts.
I don't really like how this or the previous RFC patch series deal
with semantic patches (or how some past patch series dealt with them,
for that matter), for various reasons:
- Applying the transformations from several semantic patches in one
single patch makes it harder to review it, because we won't know
which change came from which semantic patch.
For comparison, see the patch series adding hasheq()/oideq(),
merged in 769af0fd9e (Merge branch 'jk/cocci', 2018-09-17), in
particular the four "convert to " patches.
- 'make coccicheck' won't run clean (and the static analysis build
job on Travis CI will fail) for all commits following adding the
new semantic patches but before applying the resulting
transformations.
- These semantic patches interact badly with 'pu' and 'next',
because those integration branches can contain topic branches
adding new code that should be transformed by these semanic
patches. Consequently, 'make coccicheck' won't run clean and the
static analysis build job will fail until all those topics reach
'master', and the remaining transformations are applied on top.
This was (and still is!) an issue with the hasheq()/oideq() series
as well: that series was added on 2018-08-28, and the static
analysis build job is red on 'pu' ever since. See the follow-up
patch e43d2dcce1 (more oideq/hasheq conversions, 2018-10-02), and
one more follow-up will be necessary after the builtin stash topic
is merged to 'master'.
This makes it harder to review other patch series.
- Is it really necessary to carry these semantic patches in-tree?
Let me ellaborate. There are basically two main use cases for
semantic patches:
- To avoid undesirable code patterns, e.g. we should not use
sha1_to_hex(oid.hash) or strbuf_addf(&sb, "fixed string"), but
use oid_to_hex(&oid) or strbuf_addstr(&sb, "fixed string")
instead. Note that in these cases we don't remove the
functions sha1_to_hex() or strbuf_addf(), because there are
good reasons to use them in other scenarios.
Our semantic patches under 'contrib/coccinelle/' fall into
this category, and we have 'make coccicheck' and the static
analysis build job on Travis CI to catch these undesirable
code patterns preferably early, and to prevent them from
entering our codebase.
- To perform one-off code transformations, e.g. to modify a
function's name and/or signature and convert all its
callsites; see e.g. commits abef9020e3 (sha1_file: convert
sha1_object_info* to object_id, 2018-03-12) and b4f5aca40e
(sha1_file: convert read_sha1_file to struct object_id,
2018-03-12).
As far as I understand this patch series falls into this
category: once the conversion is complete the old functions
will be removed. After that there will be no use for these
semanic patches.
Having said that, it's certainly easier to double-check the
resulting transformations when one can apply the semantic
patches locally, and doing so is easier when the semantic
patches are in tree than when they must be copy-pasted from a
commit message.
OK, that was already long. Now, can we do better?
How about introducing the concept of "pending" semantic patches,
stored in 'contrib/coccinelle/.pending.cocci' files, modifying
'make coccicheck' to skip them, and adding the new 'make
coccichec
[PATCH 0/3] Allow choosing the SSL backend cURL uses (plus related patches)
This topic branch brings support for choosing cURL's SSL backend (a feature developed in Git for Windows' context) at runtime via http.sslBackend, and two more patches that are related (and only of interest for Windows users). Brendan Forster (1): http: add support for disabling SSL revocation checks in cURL Johannes Schindelin (2): http: add support for selecting SSL backends at runtime http: when using Secure Channel, ignore sslCAInfo by default Documentation/config.txt | 21 http.c | 71 +++- 2 files changed, 91 insertions(+), 1 deletion(-) base-commit: 5a0cc8aca797dbd7d2be3b67458ff880ed45cddf Published-As: https://github.com/gitgitgadget/git/releases/tags/pr-46%2Fdscho%2Fhttp-ssl-backend-v1 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-46/dscho/http-ssl-backend-v1 Pull-Request: https://github.com/gitgitgadget/git/pull/46 -- gitgitgadget
[PATCH 19/19] Apply semantic patches from previous patches
Previous commits added some cocci rules, but did not patch the whole tree,
as to not dilute the focus for reviewing the previous patches.
This patch is generated by 'make coccicheck' and applying the resulting
diff, which was white space damaged (>8 spaces after a tab) in blame.c,
which has been fixed.
Signed-off-by: Stefan Beller
---
apply.c | 6 ++--
archive.c| 5 +--
bisect.c | 5 +--
blame.c | 15 +
builtin/am.c | 2 +-
builtin/blame.c | 4 +--
builtin/cat-file.c | 21 +++-
builtin/checkout.c | 4 +--
builtin/commit.c | 13 +---
builtin/describe.c | 4 +--
builtin/difftool.c | 3 +-
builtin/fast-export.c| 7 ++--
builtin/fmt-merge-msg.c | 8 +++--
builtin/grep.c | 2 +-
builtin/index-pack.c | 8 +++--
builtin/log.c| 4 +--
builtin/merge-base.c | 2 +-
builtin/merge-tree.c | 9 --
builtin/mktag.c | 3 +-
builtin/name-rev.c | 2 +-
builtin/notes.c | 12 ---
builtin/pack-objects.c | 22 +
builtin/reflog.c | 5 +--
builtin/replace.c| 2 +-
builtin/shortlog.c | 5 +--
builtin/show-branch.c| 4 +--
builtin/tag.c| 4 +--
builtin/unpack-file.c| 2 +-
builtin/unpack-objects.c | 3 +-
builtin/verify-commit.c | 2 +-
bundle.c | 2 +-
combine-diff.c | 2 +-
commit-graph.c | 8 ++---
commit.c | 15 +
config.c | 2 +-
diff.c | 3 +-
dir.c| 2 +-
entry.c | 3 +-
fast-import.c| 7 ++--
fsck.c | 9 +++---
grep.c | 3 +-
http-push.c | 3 +-
log-tree.c | 3 +-
mailmap.c| 2 +-
match-trees.c| 4 +--
merge-blobs.c| 6 ++--
merge-recursive.c| 13
negotiator/default.c | 6 ++--
negotiator/skipping.c| 2 +-
notes-cache.c| 5 +--
notes-merge.c| 4 +--
notes-utils.c| 2 +-
notes.c | 10 +++---
pretty.c | 5 +--
read-cache.c | 5 +--
remote-testsvn.c | 4 +--
remote.c | 2 +-
rerere.c | 5 +--
revision.c | 12 +++
sequencer.c | 55 +++-
sha1-file.c | 3 +-
sha1-name.c | 9 +++---
shallow.c| 4 +--
submodule-config.c | 3 +-
t/helper/test-revision-walking.c | 3 +-
tag.c| 5 +--
tree-walk.c | 6 ++--
tree.c | 5 +--
walker.c | 2 +-
xdiff-interface.c| 2 +-
70 files changed, 254 insertions(+), 180 deletions(-)
diff --git a/apply.c b/apply.c
index fdae1d423b..5ac284b7e8 100644
--- a/apply.c
+++ b/apply.c
@@ -3187,7 +3187,8 @@ static int apply_binary(struct apply_state *state,
unsigned long size;
char *result;
- result = read_object_file(&oid, &type, &size);
+ result = repo_read_object_file(the_repository, &oid, &type,
+ &size);
if (!result)
return error(_("the necessary postimage %s for "
"'%s' cannot be read"),
@@ -3249,7 +3250,8 @@ static int read_blob_object(struct strbuf *buf, const
struct object_id *oid, uns
unsigned long sz;
char *result;
- result = read_object_file(oid, &type, &sz);
+ result = repo_read_object_file(the_repository, oid, &type,
+ &sz);
if (!result)
return -1;
/* XXX read_sha1_file NUL-terminates */
diff --git a/archive.c b/archive.c
index 994495af05..70e5eed535 100644
--- a/archive.c
+++ b/archive.c
@@ -55,7 +55,8 @@ static void format_subst(const struct commit *commit,
strbuf_add(&fmt, b + 8, c - b - 8);
strbuf_add(buf, src, b - src);
- format_commit_message(commit, fmt.buf,
Re: [PATCH v5 05/21] range-diff: also show the diff between patches
On 08/13, Johannes Schindelin wrote:
> Hi Thomas,
>
> On Sun, 12 Aug 2018, Thomas Gummerer wrote:
>
> > On 08/10, Johannes Schindelin via GitGitGadget wrote:
> > > From: Johannes Schindelin
> >
> > [...]
> >
> > I don't think this handles "--" quite as would be expected. Trying to
> > use "git range-diff -- js/range-diff-v4...HEAD" I get:
> >
> > $ ./git range-diff -- js/range-diff-v4...HEAD
> > error: need two commit ranges
> > usage: git range-diff [] ..
> > ..
> >or: git range-diff [] ...
> >or: git range-diff []
> >
> > --creation-factor
> > Percentage by which creation is weighted
> > --no-dual-color color both diff and diff-between-diffs
> >
> > while what I would have expected is to actually get a range diff.
> > This happens because after we break out of the loop we don't add the
> > actual ranges to argv, but just skip them instead.
>
> Ouch, good point.
>
> > I think something like the following should be squashed in to this
> > patch.
> >
> > --->8---
> > diff --git a/builtin/range-diff.c b/builtin/range-diff.c
> > index ef3ba22e29..132574c57a 100644
> > --- a/builtin/range-diff.c
> > +++ b/builtin/range-diff.c
> > @@ -53,6 +53,11 @@ int cmd_range_diff(int argc, const char **argv, const
> > char *prefix)
> > else
> > i += c;
> > }
> > + if (i < argc && !strcmp("--", argv[i])) {
> > + i++; j++;
> > + while (i < argc)
> > + argv[j++] = argv[i++];
> > + }
> > argc = j;
> > diff_setup_done(&diffopt);
>
> I do not think that is correct. The original idea was for the first
> `parse_options()` call to keep the dashdash, for the second one to keep
> the dashdash, too, and for the final one to swallow it.
>
> Also, if `i < argc` at this point, we already know that `argv[i]` refers
> to the dashdash, otherwise the previous loop would not have exited early.
>
> I went with this simple version instead:
>
> while (i < argc)
> argv[j++] = argv[i++];
Right, that's much better, thanks!
> Thanks!
> Dscho
[PATCH v6 05/21] range-diff: also show the diff between patches
From: Johannes Schindelin
Just like tbdiff, we now show the diff between matching patches. This is
a "diff of two diffs", so it can be a bit daunting to read for the
beginner.
An alternative would be to display an interdiff, i.e. the hypothetical
diff which is the result of first reverting the old diff and then
applying the new diff.
Especially when rebasing frequently, an interdiff is often not feasible,
though: if the old diff cannot be applied in reverse (due to a moving
upstream), an interdiff can simply not be inferred.
This commit brings `range-diff` closer to feature parity with regard
to tbdiff.
To make `git range-diff` respect e.g. color.diff.* settings, we have
to adjust git_branch_config() accordingly.
Note: while we now parse diff options such as --color, the effect is not
yet the same as in tbdiff, where also the commit pairs would be colored.
This is left for a later commit.
Note also: while tbdiff accepts the `--no-patches` option to suppress
these diffs between patches, we prefer the `-s` (or `--no-patch`) option
that is automatically supported via our use of diff_opt_parse().
And finally note: to support diff options, we have to call
`parse_options()` such that it keeps unknown options, and then loop over
those and let `diff_opt_parse()` handle them. After that loop, we have
to call `parse_options()` again, to make sure that no unknown options
are left.
Helped-by: Thomas Gummerer
Helped-by: Eric Sunshine
Signed-off-by: Johannes Schindelin
---
builtin/range-diff.c | 31 +--
range-diff.c | 34 +++---
range-diff.h | 4 +++-
3 files changed, 63 insertions(+), 6 deletions(-)
diff --git a/builtin/range-diff.c b/builtin/range-diff.c
index 94c1f362c..3b06ed944 100644
--- a/builtin/range-diff.c
+++ b/builtin/range-diff.c
@@ -2,6 +2,7 @@
#include "builtin.h"
#include "parse-options.h"
#include "range-diff.h"
+#include "config.h"
static const char * const builtin_range_diff_usage[] = {
N_("git range-diff [] .. .."),
@@ -13,15 +14,40 @@ NULL
int cmd_range_diff(int argc, const char **argv, const char *prefix)
{
int creation_factor = 60;
+ struct diff_options diffopt = { NULL };
struct option options[] = {
OPT_INTEGER(0, "creation-factor", &creation_factor,
N_("Percentage by which creation is weighted")),
OPT_END()
};
- int res = 0;
+ int i, j, res = 0;
struct strbuf range1 = STRBUF_INIT, range2 = STRBUF_INIT;
+ git_config(git_diff_ui_config, NULL);
+
+ diff_setup(&diffopt);
+ diffopt.output_format = DIFF_FORMAT_PATCH;
+
argc = parse_options(argc, argv, NULL, options,
+builtin_range_diff_usage, PARSE_OPT_KEEP_UNKNOWN |
+PARSE_OPT_KEEP_DASHDASH | PARSE_OPT_KEEP_ARGV0);
+
+ for (i = j = 1; i < argc && strcmp("--", argv[i]); ) {
+ int c = diff_opt_parse(&diffopt, argv + i, argc - i, prefix);
+
+ if (!c)
+ argv[j++] = argv[i++];
+ else
+ i += c;
+ }
+ while (i < argc)
+ argv[j++] = argv[i++];
+ argc = j;
+ diff_setup_done(&diffopt);
+
+ /* Make sure that there are no unparsed options */
+ argc = parse_options(argc, argv, NULL,
+options + ARRAY_SIZE(options) - 1, /* OPT_END */
builtin_range_diff_usage, 0);
if (argc == 2) {
@@ -59,7 +85,8 @@ int cmd_range_diff(int argc, const char **argv, const char
*prefix)
usage_with_options(builtin_range_diff_usage, options);
}
- res = show_range_diff(range1.buf, range2.buf, creation_factor);
+ res = show_range_diff(range1.buf, range2.buf, creation_factor,
+ &diffopt);
strbuf_release(&range1);
strbuf_release(&range2);
diff --git a/range-diff.c b/range-diff.c
index 2d94200d3..71883a4b7 100644
--- a/range-diff.c
+++ b/range-diff.c
@@ -6,6 +6,7 @@
#include "hashmap.h"
#include "xdiff-interface.h"
#include "linear-assignment.h"
+#include "diffcore.h"
struct patch_util {
/* For the search for an exact match */
@@ -258,7 +259,31 @@ static const char *short_oid(struct patch_util *util)
return find_unique_abbrev(&util->oid, DEFAULT_ABBREV);
}
-static void output(struct string_list *a, struct string_list *b)
+static struct diff_filespec *get_filespec(const char *name, const char *p)
+{
+ struct diff_filespec *spec = alloc_filespec(name);
+
+ fill_filespec(spec, &null_oid, 0, 0644);
+ spec->data = (char *)p;
+ spec->size = strlen(p);
+ spec->should_munmap = 0;
Re: [PATCH v5 05/21] range-diff: also show the diff between patches
Hi Thomas,
On Sun, 12 Aug 2018, Thomas Gummerer wrote:
> On 08/10, Johannes Schindelin via GitGitGadget wrote:
> > From: Johannes Schindelin
> >
> > [..]
> >
> > @@ -13,15 +14,38 @@ NULL
> > int cmd_range_diff(int argc, const char **argv, const char *prefix)
> > {
> > int creation_factor = 60;
> > + struct diff_options diffopt = { NULL };
> > struct option options[] = {
> > OPT_INTEGER(0, "creation-factor", &creation_factor,
> > N_("Percentage by which creation is weighted")),
> > OPT_END()
> > };
> > - int res = 0;
> > + int i, j, res = 0;
> > struct strbuf range1 = STRBUF_INIT, range2 = STRBUF_INIT;
> >
> > + git_config(git_diff_ui_config, NULL);
> > +
> > + diff_setup(&diffopt);
> > + diffopt.output_format = DIFF_FORMAT_PATCH;
> > +
> > argc = parse_options(argc, argv, NULL, options,
> > +builtin_range_diff_usage, PARSE_OPT_KEEP_UNKNOWN |
> > +PARSE_OPT_KEEP_DASHDASH | PARSE_OPT_KEEP_ARGV0);
> > +
> > + for (i = j = 1; i < argc && strcmp("--", argv[i]); ) {
> > + int c = diff_opt_parse(&diffopt, argv + i, argc - i, prefix);
> > +
> > + if (!c)
> > + argv[j++] = argv[i++];
> > + else
> > + i += c;
> > + }
>
> I don't think this handles "--" quite as would be expected. Trying to
> use "git range-diff -- js/range-diff-v4...HEAD" I get:
>
> $ ./git range-diff -- js/range-diff-v4...HEAD
> error: need two commit ranges
> usage: git range-diff [] ..
> ..
>or: git range-diff [] ...
>or: git range-diff []
>
> --creation-factor
> Percentage by which creation is weighted
> --no-dual-color color both diff and diff-between-diffs
>
> while what I would have expected is to actually get a range diff.
> This happens because after we break out of the loop we don't add the
> actual ranges to argv, but just skip them instead.
Ouch, good point.
> I think something like the following should be squashed in to this
> patch.
>
> --->8---
> diff --git a/builtin/range-diff.c b/builtin/range-diff.c
> index ef3ba22e29..132574c57a 100644
> --- a/builtin/range-diff.c
> +++ b/builtin/range-diff.c
> @@ -53,6 +53,11 @@ int cmd_range_diff(int argc, const char **argv, const char
> *prefix)
> else
> i += c;
> }
> + if (i < argc && !strcmp("--", argv[i])) {
> + i++; j++;
> + while (i < argc)
> + argv[j++] = argv[i++];
> + }
> argc = j;
> diff_setup_done(&diffopt);
I do not think that is correct. The original idea was for the first
`parse_options()` call to keep the dashdash, for the second one to keep
the dashdash, too, and for the final one to swallow it.
Also, if `i < argc` at this point, we already know that `argv[i]` refers
to the dashdash, otherwise the previous loop would not have exited early.
I went with this simple version instead:
while (i < argc)
argv[j++] = argv[i++];
Thanks!
Dscho
> --->8---
>
> > + argc = j;
> > + diff_setup_done(&diffopt);
> > +
> > + /* Make sure that there are no unparsed options */
> > + argc = parse_options(argc, argv, NULL,
> > +options + ARRAY_SIZE(options) - 1, /* OPT_END */
> > builtin_range_diff_usage, 0);
> >
> > if (argc == 2) {
> > @@ -59,7 +83,8 @@ int cmd_range_diff(int argc, const char **argv, const
> > char *prefix)
> > usage_with_options(builtin_range_diff_usage, options);
> > }
> >
> > - res = show_range_diff(range1.buf, range2.buf, creation_factor);
> > + res = show_range_diff(range1.buf, range2.buf, creation_factor,
> > + &diffopt);
> >
> > strbuf_release(&range1);
> > strbuf_release(&range2);
> > diff --git a/range-diff.c b/range-diff.c
> > index 2d94200d3..71883a4b7 100644
> > --- a/range-diff.c
> > +++ b/range-diff.c
> > @@ -6,6 +6,7 @@
> > #include "hashmap.h"
> > #include "xdiff-interface.h"
> > #include "linear-assignment.h"
> > +#include "diffcore.h"
> >
> > struct patch_util {
> > /* For the search for an exact match */
> > @@ -258,7 +259,31 @@ static const char *short_oid(struct patch_util *util)
> > return find_unique_abbrev(&util->oid, DEFAULT_ABBREV);
> > }
> >
> > -static void output(struct string_list *a, struct string_list *b)
> > +static struct diff_filespec *get_filespec(const char *name, const char *p)
> > +{
> > + struct diff_filespec *spec = alloc_filespec(name);
> > +
> > + fill_filespec(spec, &null_oid, 0, 0644);
> > + spec->data = (char *)p;
> > + spec->size = strlen(p);
> > + spec->should_munmap = 0;
> > + spec->is_stdin = 1;
> > +
> > + return spec;
> > +}
> > +
> > +static void patch_diff(const char *a, const char *b,
> > + struct diff_options
Re: [PATCH v5 05/21] range-diff: also show the diff between patches
Hi Dscho,
On 08/10, Johannes Schindelin via GitGitGadget wrote:
> From: Johannes Schindelin
>
> [..]
>
> @@ -13,15 +14,38 @@ NULL
> int cmd_range_diff(int argc, const char **argv, const char *prefix)
> {
> int creation_factor = 60;
> + struct diff_options diffopt = { NULL };
> struct option options[] = {
> OPT_INTEGER(0, "creation-factor", &creation_factor,
> N_("Percentage by which creation is weighted")),
> OPT_END()
> };
> - int res = 0;
> + int i, j, res = 0;
> struct strbuf range1 = STRBUF_INIT, range2 = STRBUF_INIT;
>
> + git_config(git_diff_ui_config, NULL);
> +
> + diff_setup(&diffopt);
> + diffopt.output_format = DIFF_FORMAT_PATCH;
> +
> argc = parse_options(argc, argv, NULL, options,
> + builtin_range_diff_usage, PARSE_OPT_KEEP_UNKNOWN |
> + PARSE_OPT_KEEP_DASHDASH | PARSE_OPT_KEEP_ARGV0);
> +
> + for (i = j = 1; i < argc && strcmp("--", argv[i]); ) {
> + int c = diff_opt_parse(&diffopt, argv + i, argc - i, prefix);
> +
> + if (!c)
> + argv[j++] = argv[i++];
> + else
> + i += c;
> + }
I don't think this handles "--" quite as would be expected. Trying to
use "git range-diff -- js/range-diff-v4...HEAD" I get:
$ ./git range-diff -- js/range-diff-v4...HEAD
error: need two commit ranges
usage: git range-diff [] ..
..
or: git range-diff [] ...
or: git range-diff []
--creation-factor
Percentage by which creation is weighted
--no-dual-color color both diff and diff-between-diffs
while what I would have expected is to actually get a range diff.
This happens because after we break out of the loop we don't add the
actual ranges to argv, but just skip them instead.
I think something like the following should be squashed in to this
patch.
--->8---
diff --git a/builtin/range-diff.c b/builtin/range-diff.c
index ef3ba22e29..132574c57a 100644
--- a/builtin/range-diff.c
+++ b/builtin/range-diff.c
@@ -53,6 +53,11 @@ int cmd_range_diff(int argc, const char **argv, const char
*prefix)
else
i += c;
}
+ if (i < argc && !strcmp("--", argv[i])) {
+ i++; j++;
+ while (i < argc)
+ argv[j++] = argv[i++];
+ }
argc = j;
diff_setup_done(&diffopt);
--->8---
> + argc = j;
> + diff_setup_done(&diffopt);
> +
> + /* Make sure that there are no unparsed options */
> + argc = parse_options(argc, argv, NULL,
> + options + ARRAY_SIZE(options) - 1, /* OPT_END */
>builtin_range_diff_usage, 0);
>
> if (argc == 2) {
> @@ -59,7 +83,8 @@ int cmd_range_diff(int argc, const char **argv, const char
> *prefix)
> usage_with_options(builtin_range_diff_usage, options);
> }
>
> - res = show_range_diff(range1.buf, range2.buf, creation_factor);
> + res = show_range_diff(range1.buf, range2.buf, creation_factor,
> + &diffopt);
>
> strbuf_release(&range1);
> strbuf_release(&range2);
> diff --git a/range-diff.c b/range-diff.c
> index 2d94200d3..71883a4b7 100644
> --- a/range-diff.c
> +++ b/range-diff.c
> @@ -6,6 +6,7 @@
> #include "hashmap.h"
> #include "xdiff-interface.h"
> #include "linear-assignment.h"
> +#include "diffcore.h"
>
> struct patch_util {
> /* For the search for an exact match */
> @@ -258,7 +259,31 @@ static const char *short_oid(struct patch_util *util)
> return find_unique_abbrev(&util->oid, DEFAULT_ABBREV);
> }
>
> -static void output(struct string_list *a, struct string_list *b)
> +static struct diff_filespec *get_filespec(const char *name, const char *p)
> +{
> + struct diff_filespec *spec = alloc_filespec(name);
> +
> + fill_filespec(spec, &null_oid, 0, 0644);
> + spec->data = (char *)p;
> + spec->size = strlen(p);
> + spec->should_munmap = 0;
> + spec->is_stdin = 1;
> +
> + return spec;
> +}
> +
> +static void patch_diff(const char *a, const char *b,
> + struct diff_options *diffopt)
> +{
> + diff_queue(&diff_queued_diff,
> +get_filespec("a", a), get_filespec("b", b));
> +
> + diffcore_std(diffopt);
> + diff_flush(diffopt);
> +}
> +
> +static void output(struct string_list *a, struct string_list *b,
> +struct diff_options *diffopt)
> {
> int i = 0, j = 0;
>
> @@ -300,6 +325,9 @@ static void output(struct string_list *a, struct
> string_list *b)
> printf("%d: %s ! %d: %s\n",
> b_util->matching + 1, short_oid(a_util),
> j + 1, short_oid(b_util));
> + if (!(diffopt->output_format & DIFF_FORMAT_NO_OUTPUT))
[PATCH v5 05/21] range-diff: also show the diff between patches
From: Johannes Schindelin
Just like tbdiff, we now show the diff between matching patches. This is
a "diff of two diffs", so it can be a bit daunting to read for the
beginner.
An alternative would be to display an interdiff, i.e. the hypothetical
diff which is the result of first reverting the old diff and then
applying the new diff.
Especially when rebasing frequently, an interdiff is often not feasible,
though: if the old diff cannot be applied in reverse (due to a moving
upstream), an interdiff can simply not be inferred.
This commit brings `range-diff` closer to feature parity with regard
to tbdiff.
To make `git range-diff` respect e.g. color.diff.* settings, we have
to adjust git_branch_config() accordingly.
Note: while we now parse diff options such as --color, the effect is not
yet the same as in tbdiff, where also the commit pairs would be colored.
This is left for a later commit.
Note also: while tbdiff accepts the `--no-patches` option to suppress
these diffs between patches, we prefer the `-s` (or `--no-patch`) option
that is automatically supported via our use of diff_opt_parse().
And finally note: to support diff options, we have to call
`parse_options()` such that it keeps unknown options, and then loop over
those and let `diff_opt_parse()` handle them. After that loop, we have
to call `parse_options()` again, to make sure that no unknown options
are left.
Helped-by: Thomas Gummerer
Helped-by: Eric Sunshine
Signed-off-by: Johannes Schindelin
---
builtin/range-diff.c | 29 +++--
range-diff.c | 34 +++---
range-diff.h | 4 +++-
3 files changed, 61 insertions(+), 6 deletions(-)
diff --git a/builtin/range-diff.c b/builtin/range-diff.c
index 94c1f362c..19192ab31 100644
--- a/builtin/range-diff.c
+++ b/builtin/range-diff.c
@@ -2,6 +2,7 @@
#include "builtin.h"
#include "parse-options.h"
#include "range-diff.h"
+#include "config.h"
static const char * const builtin_range_diff_usage[] = {
N_("git range-diff [] .. .."),
@@ -13,15 +14,38 @@ NULL
int cmd_range_diff(int argc, const char **argv, const char *prefix)
{
int creation_factor = 60;
+ struct diff_options diffopt = { NULL };
struct option options[] = {
OPT_INTEGER(0, "creation-factor", &creation_factor,
N_("Percentage by which creation is weighted")),
OPT_END()
};
- int res = 0;
+ int i, j, res = 0;
struct strbuf range1 = STRBUF_INIT, range2 = STRBUF_INIT;
+ git_config(git_diff_ui_config, NULL);
+
+ diff_setup(&diffopt);
+ diffopt.output_format = DIFF_FORMAT_PATCH;
+
argc = parse_options(argc, argv, NULL, options,
+builtin_range_diff_usage, PARSE_OPT_KEEP_UNKNOWN |
+PARSE_OPT_KEEP_DASHDASH | PARSE_OPT_KEEP_ARGV0);
+
+ for (i = j = 1; i < argc && strcmp("--", argv[i]); ) {
+ int c = diff_opt_parse(&diffopt, argv + i, argc - i, prefix);
+
+ if (!c)
+ argv[j++] = argv[i++];
+ else
+ i += c;
+ }
+ argc = j;
+ diff_setup_done(&diffopt);
+
+ /* Make sure that there are no unparsed options */
+ argc = parse_options(argc, argv, NULL,
+options + ARRAY_SIZE(options) - 1, /* OPT_END */
builtin_range_diff_usage, 0);
if (argc == 2) {
@@ -59,7 +83,8 @@ int cmd_range_diff(int argc, const char **argv, const char
*prefix)
usage_with_options(builtin_range_diff_usage, options);
}
- res = show_range_diff(range1.buf, range2.buf, creation_factor);
+ res = show_range_diff(range1.buf, range2.buf, creation_factor,
+ &diffopt);
strbuf_release(&range1);
strbuf_release(&range2);
diff --git a/range-diff.c b/range-diff.c
index 2d94200d3..71883a4b7 100644
--- a/range-diff.c
+++ b/range-diff.c
@@ -6,6 +6,7 @@
#include "hashmap.h"
#include "xdiff-interface.h"
#include "linear-assignment.h"
+#include "diffcore.h"
struct patch_util {
/* For the search for an exact match */
@@ -258,7 +259,31 @@ static const char *short_oid(struct patch_util *util)
return find_unique_abbrev(&util->oid, DEFAULT_ABBREV);
}
-static void output(struct string_list *a, struct string_list *b)
+static struct diff_filespec *get_filespec(const char *name, const char *p)
+{
+ struct diff_filespec *spec = alloc_filespec(name);
+
+ fill_filespec(spec, &null_oid, 0, 0644);
+ spec->data = (char *)p;
+ spec->size = strlen(p);
+ spec->should_munmap = 0;
+ spec->is_stdin = 1;
+
+ return spec;
+}
+
+
Re: [PATCH v4 05/21] range-diff: also show the diff between patches
Hi Eric, On Fri, 10 Aug 2018, Eric Sunshine wrote: > On Fri, Aug 10, 2018 at 5:12 PM Johannes Schindelin > wrote: > > On Mon, 30 Jul 2018, Eric Sunshine wrote: > > > I think you can attain the desired behavior by making a final > > > parse_options() call with empty 'options' list after the call to > > > diff_setup_done(). It's pretty much a one-line fix, but can probably > > > be done as an incremental change rather than rerolling. > > > > But then we would have to keep `--` in the first, and not in the second > > parse_options() call, right? We would also have to handle that `--` > > properly in the loop that calls diff_opt_parse(), I think. > > A bit more involved than just a one-line fix, but I guess I'll give it a > > try. > > It's something that could easily wait until after this series lands. > After all, it's just a slightly confusing error message, not some > fundamental problem. > > As for '--', I'll have to go back and look at the code. I thought I > had thought it all through at the time I made the suggestion, but my > brain needs a refresh by now. Your suggestion might have started out by trying to fix the error message, but after staring at the code for a couple of minutes, I think the issue would have left all kinds of opportunities for bad user experience. So I fixed it ;-) Ciao, Dscho
Re: [PATCH v4 05/21] range-diff: also show the diff between patches
On Fri, Aug 10, 2018 at 5:12 PM Johannes Schindelin wrote: > On Mon, 30 Jul 2018, Eric Sunshine wrote: > > I think you can attain the desired behavior by making a final > > parse_options() call with empty 'options' list after the call to > > diff_setup_done(). It's pretty much a one-line fix, but can probably > > be done as an incremental change rather than rerolling. > > But then we would have to keep `--` in the first, and not in the second > parse_options() call, right? We would also have to handle that `--` > properly in the loop that calls diff_opt_parse(), I think. > A bit more involved than just a one-line fix, but I guess I'll give it a > try. It's something that could easily wait until after this series lands. After all, it's just a slightly confusing error message, not some fundamental problem. As for '--', I'll have to go back and look at the code. I thought I had thought it all through at the time I made the suggestion, but my brain needs a refresh by now.
Re: [PATCH v4 05/21] range-diff: also show the diff between patches
Hi Eric, On Mon, 30 Jul 2018, Eric Sunshine wrote: > On Mon, Jul 30, 2018 at 5:26 PM Thomas Gummerer wrote: > > On 07/30, Johannes Schindelin wrote: > > > On Sun, 29 Jul 2018, Thomas Gummerer wrote: > > > > There's one more thing that I noticed here: > > > > > > > > git range-diff --no-patches > > > > fatal: single arg format requires a symmetric range > > > > > > > I immediately thought of testing for a leading `-` of the remaining > > > argument, but I could imagine that somebody enterprisey uses > > > > > > git range-diff -- -my-first-attempt...-my-second-attempt > > > > > > and I do not really want to complexify the code... Ideas? > > > > Good point. I can't really come up with a good option right now > > either. It's not too bad, as users just typed the command, so it > > should be easy enough to see from the previous line what went wrong. > > I think you can attain the desired behavior by making a final > parse_options() call with empty 'options' list after the call to > diff_setup_done(). It's pretty much a one-line fix, but can probably > be done as an incremental change rather than rerolling. But then we would have to keep `--` in the first, and not in the second parse_options() call, right? We would also have to handle that `--` properly in the loop that calls diff_opt_parse(), I think. A bit more involved than just a one-line fix, but I guess I'll give it a try. Ciao, Dscho
Re: [PATCH v4 05/21] range-diff: also show the diff between patches
Hi Thomas,
On Mon, 30 Jul 2018, Thomas Gummerer wrote:
> On 07/30, Johannes Schindelin wrote:
> >
> > On Sun, 29 Jul 2018, Thomas Gummerer wrote:
> >
> > > On 07/29, Eric Sunshine wrote:
> > > > On Sun, Jul 29, 2018 at 3:04 PM Thomas Gummerer
> > > > wrote:
> > > > > On 07/21, Johannes Schindelin via GitGitGadget wrote:
> > > > > > Just like tbdiff, we now show the diff between matching patches.
> > > > > > This is
> > > > > > a "diff of two diffs", so it can be a bit daunting to read for the
> > > > > > beginner.
> > > > > > [...]
> > > > > > Note also: while tbdiff accepts the `--no-patches` option to
> > > > > > suppress
> > > > > > these diffs between patches, we prefer the `-s` option that is
> > > > > > automatically supported via our use of diff_opt_parse().
> > > > >
> > > > > One slightly unfortunate thing here is that we don't show these
> > > > > options in 'git range-diff -h', which would be nice to have. I don't
> > > > > know if that's possible in git right now, if it's not easily possible,
> > > > > I definitely wouldn't want to delay this series for that, and we could
> > > > > just add it to the list of possible future enhancements that other
> > > > > people mentioned.
> > > >
> > > > This issue is not specific to git-range-diff; it's shared by other
> > > > commands which inherit diff options via diff_opt_parse(). For
> > > > instance, "git log -h" doesn't show diff-related options either, yet
> > > > it accepts them.
> > >
> > > Fair enough, that makes sense. Thanks for the pointer!
> > >
> > > There's one more thing that I noticed here:
> > >
> > > git range-diff --no-patches
> > > fatal: single arg format requires a symmetric range
> > >
> > > Which is a slightly confusing error message. In contrast git log does
> > > the following on an unrecognized argument:
> > >
> > > git log --no-patches
> > > fatal: unrecognized argument: --no-patches
> > >
> > > which is a little better I think. I do however also thing the "fatal:
> > > single arg format requires a symmetric range" is useful when someone
> > > genuinely tries to use the single argument version of the command. So
> > > I don't know what a good solution for this would be.
> >
> > I immediately thought of testing for a leading `-` of the remaining
> > argument, but I could imagine that somebody enterprisey uses
> >
> > git range-diff -- -my-first-attempt...-my-second-attempt
> >
> > and I do not really want to complexify the code... Ideas?
>
> Good point. I can't really come up with a good option right now
> either. It's not too bad, as users just typed the command, so it
> should be easy enough to see from the previous line what went wrong.
>
> One potential option may be to turn "die(_("single arg format requires
> a symmetric range"));" into an 'error()', and show the usage? I think
> that may be nice anyway, as "symmetric range" may not be immediately
> obvious to everyone, but together with the usage it may be clearer?
Agreed. Will be made so.
Ciao,
Dscho
>
> > > > > > diff --git a/range-diff.c b/range-diff.c
> > > > > > @@ -300,6 +325,9 @@ static void output(struct string_list *a,
> > > > > > struct string_list *b)
> > > > > > printf("%d: %s ! %d: %s\n",
> > > > > > b_util->matching + 1,
> > > > > > short_oid(a_util),
> > > > > > j + 1, short_oid(b_util));
> > > > > > + if (!(diffopt->output_format &
> > > > > > DIFF_FORMAT_NO_OUTPUT))
> > > > >
> > > > > Looking at this line, it looks like it would be easy to support
> > > > > '--no-patches' as well, which may be slightly easier to understand
> > > > > that
> > > > > '-s' to someone new to the command. But again that can be added later
> > > > > if someone actually cares about it.
> > > >
> > > > What wasn't mentioned (but was implied) by the commit message is that
> > > > "-s" is short for "--no-patch", which also comes for free via
> > > > diff_opt_parse(). True, "--no-patch" isn't spelled exactly the same as
> > > > "--no-patches", but git-range-diff isn't exactly a perfect tbdiff
> > > > clone, so hopefully not a git problem. Moreover, "--no-patch" is
> > > > internally consistent within the Git builtin commands.
> > >
> > > Makes sense, thanks! "--no-patch" does make sense to me. There's
> > > still a lot of command line flags in git to learn for me, even after
> > > all this time using it ;) Might be nice to spell it out in the commit
> > > message for someone like me, especially as "--no-patches" is already
> > > mentioned. Though I guess most regulars here would know about
> > > "--no-patch", so maybe it's not worth it. Anyway that is definitely
> > > not worth another round here.
> >
> > Sure, but not many users learn from reading the commit history...
> >
> > :-)
> >
> > Ciao,
> > Dscho
>
Re: [PATCH v4 05/21] range-diff: also show the diff between patches
On Mon, Jul 30, 2018 at 5:26 PM Thomas Gummerer wrote: > On 07/30, Johannes Schindelin wrote: > > On Sun, 29 Jul 2018, Thomas Gummerer wrote: > > > There's one more thing that I noticed here: > > > > > > git range-diff --no-patches > > > fatal: single arg format requires a symmetric range > > > > > I immediately thought of testing for a leading `-` of the remaining > > argument, but I could imagine that somebody enterprisey uses > > > > git range-diff -- -my-first-attempt...-my-second-attempt > > > > and I do not really want to complexify the code... Ideas? > > Good point. I can't really come up with a good option right now > either. It's not too bad, as users just typed the command, so it > should be easy enough to see from the previous line what went wrong. I think you can attain the desired behavior by making a final parse_options() call with empty 'options' list after the call to diff_setup_done(). It's pretty much a one-line fix, but can probably be done as an incremental change rather than rerolling.
Re: [PATCH v4 05/21] range-diff: also show the diff between patches
On 07/30, Johannes Schindelin wrote:
> Hi Thomas & Eric,
>
> On Sun, 29 Jul 2018, Thomas Gummerer wrote:
>
> > On 07/29, Eric Sunshine wrote:
> > > On Sun, Jul 29, 2018 at 3:04 PM Thomas Gummerer
> > > wrote:
> > > > On 07/21, Johannes Schindelin via GitGitGadget wrote:
> > > > > Just like tbdiff, we now show the diff between matching patches. This
> > > > > is
> > > > > a "diff of two diffs", so it can be a bit daunting to read for the
> > > > > beginner.
> > > > > [...]
> > > > > Note also: while tbdiff accepts the `--no-patches` option to suppress
> > > > > these diffs between patches, we prefer the `-s` option that is
> > > > > automatically supported via our use of diff_opt_parse().
> > > >
> > > > One slightly unfortunate thing here is that we don't show these
> > > > options in 'git range-diff -h', which would be nice to have. I don't
> > > > know if that's possible in git right now, if it's not easily possible,
> > > > I definitely wouldn't want to delay this series for that, and we could
> > > > just add it to the list of possible future enhancements that other
> > > > people mentioned.
> > >
> > > This issue is not specific to git-range-diff; it's shared by other
> > > commands which inherit diff options via diff_opt_parse(). For
> > > instance, "git log -h" doesn't show diff-related options either, yet
> > > it accepts them.
> >
> > Fair enough, that makes sense. Thanks for the pointer!
> >
> > There's one more thing that I noticed here:
> >
> > git range-diff --no-patches
> > fatal: single arg format requires a symmetric range
> >
> > Which is a slightly confusing error message. In contrast git log does
> > the following on an unrecognized argument:
> >
> > git log --no-patches
> > fatal: unrecognized argument: --no-patches
> >
> > which is a little better I think. I do however also thing the "fatal:
> > single arg format requires a symmetric range" is useful when someone
> > genuinely tries to use the single argument version of the command. So
> > I don't know what a good solution for this would be.
>
> I immediately thought of testing for a leading `-` of the remaining
> argument, but I could imagine that somebody enterprisey uses
>
> git range-diff -- -my-first-attempt...-my-second-attempt
>
> and I do not really want to complexify the code... Ideas?
Good point. I can't really come up with a good option right now
either. It's not too bad, as users just typed the command, so it
should be easy enough to see from the previous line what went wrong.
One potential option may be to turn "die(_("single arg format requires
a symmetric range"));" into an 'error()', and show the usage? I think
that may be nice anyway, as "symmetric range" may not be immediately
obvious to everyone, but together with the usage it may be clearer?
> > > > > diff --git a/range-diff.c b/range-diff.c
> > > > > @@ -300,6 +325,9 @@ static void output(struct string_list *a, struct
> > > > > string_list *b)
> > > > > printf("%d: %s ! %d: %s\n",
> > > > > b_util->matching + 1, short_oid(a_util),
> > > > > j + 1, short_oid(b_util));
> > > > > + if (!(diffopt->output_format &
> > > > > DIFF_FORMAT_NO_OUTPUT))
> > > >
> > > > Looking at this line, it looks like it would be easy to support
> > > > '--no-patches' as well, which may be slightly easier to understand that
> > > > '-s' to someone new to the command. But again that can be added later
> > > > if someone actually cares about it.
> > >
> > > What wasn't mentioned (but was implied) by the commit message is that
> > > "-s" is short for "--no-patch", which also comes for free via
> > > diff_opt_parse(). True, "--no-patch" isn't spelled exactly the same as
> > > "--no-patches", but git-range-diff isn't exactly a perfect tbdiff
> > > clone, so hopefully not a git problem. Moreover, "--no-patch" is
> > > internally consistent within the Git builtin commands.
> >
> > Makes sense, thanks! "--no-patch" does make sense to me. There's
> > still a lot of command line flags in git to learn for me, even after
> > all this time using it ;) Might be nice to spell it out in the commit
> > message for someone like me, especially as "--no-patches" is already
> > mentioned. Though I guess most regulars here would know about
> > "--no-patch", so maybe it's not worth it. Anyway that is definitely
> > not worth another round here.
>
> Sure, but not many users learn from reading the commit history...
>
> :-)
>
> Ciao,
> Dscho
Re: [PATCH v4 05/21] range-diff: also show the diff between patches
Hi Thomas & Eric,
On Sun, 29 Jul 2018, Thomas Gummerer wrote:
> On 07/29, Eric Sunshine wrote:
> > On Sun, Jul 29, 2018 at 3:04 PM Thomas Gummerer
> > wrote:
> > > On 07/21, Johannes Schindelin via GitGitGadget wrote:
> > > > Just like tbdiff, we now show the diff between matching patches. This is
> > > > a "diff of two diffs", so it can be a bit daunting to read for the
> > > > beginner.
> > > > [...]
> > > > Note also: while tbdiff accepts the `--no-patches` option to suppress
> > > > these diffs between patches, we prefer the `-s` option that is
> > > > automatically supported via our use of diff_opt_parse().
> > >
> > > One slightly unfortunate thing here is that we don't show these
> > > options in 'git range-diff -h', which would be nice to have. I don't
> > > know if that's possible in git right now, if it's not easily possible,
> > > I definitely wouldn't want to delay this series for that, and we could
> > > just add it to the list of possible future enhancements that other
> > > people mentioned.
> >
> > This issue is not specific to git-range-diff; it's shared by other
> > commands which inherit diff options via diff_opt_parse(). For
> > instance, "git log -h" doesn't show diff-related options either, yet
> > it accepts them.
>
> Fair enough, that makes sense. Thanks for the pointer!
>
> There's one more thing that I noticed here:
>
> git range-diff --no-patches
> fatal: single arg format requires a symmetric range
>
> Which is a slightly confusing error message. In contrast git log does
> the following on an unrecognized argument:
>
> git log --no-patches
> fatal: unrecognized argument: --no-patches
>
> which is a little better I think. I do however also thing the "fatal:
> single arg format requires a symmetric range" is useful when someone
> genuinely tries to use the single argument version of the command. So
> I don't know what a good solution for this would be.
I immediately thought of testing for a leading `-` of the remaining
argument, but I could imagine that somebody enterprisey uses
git range-diff -- -my-first-attempt...-my-second-attempt
and I do not really want to complexify the code... Ideas?
> > > > diff --git a/range-diff.c b/range-diff.c
> > > > @@ -300,6 +325,9 @@ static void output(struct string_list *a, struct
> > > > string_list *b)
> > > > printf("%d: %s ! %d: %s\n",
> > > > b_util->matching + 1, short_oid(a_util),
> > > > j + 1, short_oid(b_util));
> > > > + if (!(diffopt->output_format &
> > > > DIFF_FORMAT_NO_OUTPUT))
> > >
> > > Looking at this line, it looks like it would be easy to support
> > > '--no-patches' as well, which may be slightly easier to understand that
> > > '-s' to someone new to the command. But again that can be added later
> > > if someone actually cares about it.
> >
> > What wasn't mentioned (but was implied) by the commit message is that
> > "-s" is short for "--no-patch", which also comes for free via
> > diff_opt_parse(). True, "--no-patch" isn't spelled exactly the same as
> > "--no-patches", but git-range-diff isn't exactly a perfect tbdiff
> > clone, so hopefully not a git problem. Moreover, "--no-patch" is
> > internally consistent within the Git builtin commands.
>
> Makes sense, thanks! "--no-patch" does make sense to me. There's
> still a lot of command line flags in git to learn for me, even after
> all this time using it ;) Might be nice to spell it out in the commit
> message for someone like me, especially as "--no-patches" is already
> mentioned. Though I guess most regulars here would know about
> "--no-patch", so maybe it's not worth it. Anyway that is definitely
> not worth another round here.
Sure, but not many users learn from reading the commit history...
:-)
Ciao,
Dscho
Re: [PATCH v4 05/21] range-diff: also show the diff between patches
On 07/29, Eric Sunshine wrote:
> On Sun, Jul 29, 2018 at 3:04 PM Thomas Gummerer wrote:
> > On 07/21, Johannes Schindelin via GitGitGadget wrote:
> > > Just like tbdiff, we now show the diff between matching patches. This is
> > > a "diff of two diffs", so it can be a bit daunting to read for the
> > > beginner.
> > > [...]
> > > Note also: while tbdiff accepts the `--no-patches` option to suppress
> > > these diffs between patches, we prefer the `-s` option that is
> > > automatically supported via our use of diff_opt_parse().
> >
> > One slightly unfortunate thing here is that we don't show these
> > options in 'git range-diff -h', which would be nice to have. I don't
> > know if that's possible in git right now, if it's not easily possible,
> > I definitely wouldn't want to delay this series for that, and we could
> > just add it to the list of possible future enhancements that other
> > people mentioned.
>
> This issue is not specific to git-range-diff; it's shared by other
> commands which inherit diff options via diff_opt_parse(). For
> instance, "git log -h" doesn't show diff-related options either, yet
> it accepts them.
Fair enough, that makes sense. Thanks for the pointer!
There's one more thing that I noticed here:
git range-diff --no-patches
fatal: single arg format requires a symmetric range
Which is a slightly confusing error message. In contrast git log does
the following on an unrecognized argument:
git log --no-patches
fatal: unrecognized argument: --no-patches
which is a little better I think. I do however also thing the "fatal:
single arg format requires a symmetric range" is useful when someone
genuinely tries to use the single argument version of the command. So
I don't know what a good solution for this would be.
> > > diff --git a/range-diff.c b/range-diff.c
> > > @@ -300,6 +325,9 @@ static void output(struct string_list *a, struct
> > > string_list *b)
> > > printf("%d: %s ! %d: %s\n",
> > > b_util->matching + 1, short_oid(a_util),
> > > j + 1, short_oid(b_util));
> > > + if (!(diffopt->output_format &
> > > DIFF_FORMAT_NO_OUTPUT))
> >
> > Looking at this line, it looks like it would be easy to support
> > '--no-patches' as well, which may be slightly easier to understand that
> > '-s' to someone new to the command. But again that can be added later
> > if someone actually cares about it.
>
> What wasn't mentioned (but was implied) by the commit message is that
> "-s" is short for "--no-patch", which also comes for free via
> diff_opt_parse(). True, "--no-patch" isn't spelled exactly the same as
> "--no-patches", but git-range-diff isn't exactly a perfect tbdiff
> clone, so hopefully not a git problem. Moreover, "--no-patch" is
> internally consistent within the Git builtin commands.
Makes sense, thanks! "--no-patch" does make sense to me. There's
still a lot of command line flags in git to learn for me, even after
all this time using it ;) Might be nice to spell it out in the commit
message for someone like me, especially as "--no-patches" is already
mentioned. Though I guess most regulars here would know about
"--no-patch", so maybe it's not worth it. Anyway that is definitely
not worth another round here.
Re: [PATCH v4 05/21] range-diff: also show the diff between patches
On Sun, Jul 29, 2018 at 3:04 PM Thomas Gummerer wrote:
> On 07/21, Johannes Schindelin via GitGitGadget wrote:
> > Just like tbdiff, we now show the diff between matching patches. This is
> > a "diff of two diffs", so it can be a bit daunting to read for the
> > beginner.
> > [...]
> > Note also: while tbdiff accepts the `--no-patches` option to suppress
> > these diffs between patches, we prefer the `-s` option that is
> > automatically supported via our use of diff_opt_parse().
>
> One slightly unfortunate thing here is that we don't show these
> options in 'git range-diff -h', which would be nice to have. I don't
> know if that's possible in git right now, if it's not easily possible,
> I definitely wouldn't want to delay this series for that, and we could
> just add it to the list of possible future enhancements that other
> people mentioned.
This issue is not specific to git-range-diff; it's shared by other
commands which inherit diff options via diff_opt_parse(). For
instance, "git log -h" doesn't show diff-related options either, yet
it accepts them.
> > diff --git a/range-diff.c b/range-diff.c
> > @@ -300,6 +325,9 @@ static void output(struct string_list *a, struct
> > string_list *b)
> > printf("%d: %s ! %d: %s\n",
> > b_util->matching + 1, short_oid(a_util),
> > j + 1, short_oid(b_util));
> > + if (!(diffopt->output_format & DIFF_FORMAT_NO_OUTPUT))
>
> Looking at this line, it looks like it would be easy to support
> '--no-patches' as well, which may be slightly easier to understand that
> '-s' to someone new to the command. But again that can be added later
> if someone actually cares about it.
What wasn't mentioned (but was implied) by the commit message is that
"-s" is short for "--no-patch", which also comes for free via
diff_opt_parse(). True, "--no-patch" isn't spelled exactly the same as
"--no-patches", but git-range-diff isn't exactly a perfect tbdiff
clone, so hopefully not a git problem. Moreover, "--no-patch" is
internally consistent within the Git builtin commands.
Re: [PATCH v4 05/21] range-diff: also show the diff between patches
On 07/21, Johannes Schindelin via GitGitGadget wrote:
> From: Johannes Schindelin
>
> Just like tbdiff, we now show the diff between matching patches. This is
> a "diff of two diffs", so it can be a bit daunting to read for the
> beginner.
>
> An alternative would be to display an interdiff, i.e. the hypothetical
> diff which is the result of first reverting the old diff and then
> applying the new diff.
>
> Especially when rebasing often, an interdiff is often not feasible,
> though: if the old diff cannot be applied in reverse (due to a moving
> upstream), an interdiff can simply not be inferred.
>
> This commit brings `range-diff` closer to feature parity with regard
> to tbdiff.
>
> To make `git range-diff` respect e.g. color.diff.* settings, we have
> to adjust git_branch_config() accordingly.
>
> Note: while we now parse diff options such as --color, the effect is not
> yet the same as in tbdiff, where also the commit pairs would be colored.
> This is left for a later commit.
>
> Note also: while tbdiff accepts the `--no-patches` option to suppress
> these diffs between patches, we prefer the `-s` option that is
> automatically supported via our use of diff_opt_parse().
One slightly unfortunate thing here is that we don't show these
options in 'git range-diff -h', which would be nice to have. I don't
know if that's possible in git right now, if it's not easily possible,
I definitely wouldn't want to delay this series for that, and we could
just add it to the list of possible future enhancements that other
people mentioned.
> Signed-off-by: Johannes Schindelin
> ---
> builtin/range-diff.c | 25 ++---
> range-diff.c | 34 +++---
> range-diff.h | 4 +++-
> 3 files changed, 56 insertions(+), 7 deletions(-)
>
> [...]
>
> diff --git a/range-diff.c b/range-diff.c
> index 2d94200d3..71883a4b7 100644
> --- a/range-diff.c
> +++ b/range-diff.c
> @@ -6,6 +6,7 @@
> #include "hashmap.h"
> #include "xdiff-interface.h"
> #include "linear-assignment.h"
> +#include "diffcore.h"
>
> struct patch_util {
> /* For the search for an exact match */
> @@ -258,7 +259,31 @@ static const char *short_oid(struct patch_util *util)
> return find_unique_abbrev(&util->oid, DEFAULT_ABBREV);
> }
>
> -static void output(struct string_list *a, struct string_list *b)
> +static struct diff_filespec *get_filespec(const char *name, const char *p)
> +{
> + struct diff_filespec *spec = alloc_filespec(name);
> +
> + fill_filespec(spec, &null_oid, 0, 0644);
> + spec->data = (char *)p;
> + spec->size = strlen(p);
> + spec->should_munmap = 0;
> + spec->is_stdin = 1;
> +
> + return spec;
> +}
> +
> +static void patch_diff(const char *a, const char *b,
> + struct diff_options *diffopt)
> +{
> + diff_queue(&diff_queued_diff,
> +get_filespec("a", a), get_filespec("b", b));
> +
> + diffcore_std(diffopt);
> + diff_flush(diffopt);
> +}
> +
> +static void output(struct string_list *a, struct string_list *b,
> +struct diff_options *diffopt)
> {
> int i = 0, j = 0;
>
> @@ -300,6 +325,9 @@ static void output(struct string_list *a, struct
> string_list *b)
> printf("%d: %s ! %d: %s\n",
> b_util->matching + 1, short_oid(a_util),
> j + 1, short_oid(b_util));
> + if (!(diffopt->output_format & DIFF_FORMAT_NO_OUTPUT))
Looking at this line, it looks like it would be easy to support
'--no-patches' as well, which may be slightly easier to understand that
'-s' to someone new to the command. But again that can be added later
if someone actually cares about it.
> + patch_diff(a->items[b_util->matching].string,
> +b->items[j].string, diffopt);
> a_util->shown = 1;
> j++;
> }
> @@ -307,7 +335,7 @@ static void output(struct string_list *a, struct
> string_list *b)
> }
>
> int show_range_diff(const char *range1, const char *range2,
> - int creation_factor)
> + int creation_factor, struct diff_options *diffopt)
> {
> int res = 0;
>
> @@ -322,7 +350,7 @@ int show_range_diff(const char *range1, const char
> *range2,
> if (!res) {
> find_exact_matches(&branch1, &bran
Re: [PATCH 4/5] coccinelle: put sane filenames into output patches
On 7/23/2018 9:50 AM, SZEDER Gábor wrote: Coccinelle outputs its suggested transformations as patches, whose header looks something like this: --- commit.c +++ /tmp/cocci-output-19250-7ae78a-commit.c Note the lack of 'diff --opts ' line, the differing number of path components on the --- and +++ lines, and the nonsensical filename on the +++ line. 'patch -p0' can still apply these patches, as it takes the filename to be modified from the --- line. Alas, 'git apply' can't, because it takes the filename from the +++ line, and then complains about the nonexisting file. Pass the '--patch .' options to Coccinelle via the SPATCH_FLAGS 'make' variable, as it seems to make it generate proper context diff patches, with the header starting with a 'diff ...' line and containing sane filenames. The resulting 'contrib/coccinelle/*.cocci.patch' files then can be applied both with 'git apply' and 'patch' (even without '-p0'). Signed-off-by: SZEDER Gábor --- Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Makefile b/Makefile index 73e2d16926..72ea29df4e 100644 --- a/Makefile +++ b/Makefile @@ -564,7 +564,7 @@ SPATCH = spatch export TCL_PATH TCLTK_PATH SPARSE_FLAGS = -SPATCH_FLAGS = --all-includes +SPATCH_FLAGS = --all-includes --patch . Thank you for this! These garbage filenames cause significant pain when trying to apply the suggested patch.
[PATCH 4/5] coccinelle: put sane filenames into output patches
Coccinelle outputs its suggested transformations as patches, whose header looks something like this: --- commit.c +++ /tmp/cocci-output-19250-7ae78a-commit.c Note the lack of 'diff --opts ' line, the differing number of path components on the --- and +++ lines, and the nonsensical filename on the +++ line. 'patch -p0' can still apply these patches, as it takes the filename to be modified from the --- line. Alas, 'git apply' can't, because it takes the filename from the +++ line, and then complains about the nonexisting file. Pass the '--patch .' options to Coccinelle via the SPATCH_FLAGS 'make' variable, as it seems to make it generate proper context diff patches, with the header starting with a 'diff ...' line and containing sane filenames. The resulting 'contrib/coccinelle/*.cocci.patch' files then can be applied both with 'git apply' and 'patch' (even without '-p0'). Signed-off-by: SZEDER Gábor --- Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Makefile b/Makefile index 73e2d16926..72ea29df4e 100644 --- a/Makefile +++ b/Makefile @@ -564,7 +564,7 @@ SPATCH = spatch export TCL_PATH TCLTK_PATH SPARSE_FLAGS = -SPATCH_FLAGS = --all-includes +SPATCH_FLAGS = --all-includes --patch . -- 2.18.0.408.g42635c01bc
[PATCH v4 05/21] range-diff: also show the diff between patches
From: Johannes Schindelin
Just like tbdiff, we now show the diff between matching patches. This is
a "diff of two diffs", so it can be a bit daunting to read for the
beginner.
An alternative would be to display an interdiff, i.e. the hypothetical
diff which is the result of first reverting the old diff and then
applying the new diff.
Especially when rebasing often, an interdiff is often not feasible,
though: if the old diff cannot be applied in reverse (due to a moving
upstream), an interdiff can simply not be inferred.
This commit brings `range-diff` closer to feature parity with regard
to tbdiff.
To make `git range-diff` respect e.g. color.diff.* settings, we have
to adjust git_branch_config() accordingly.
Note: while we now parse diff options such as --color, the effect is not
yet the same as in tbdiff, where also the commit pairs would be colored.
This is left for a later commit.
Note also: while tbdiff accepts the `--no-patches` option to suppress
these diffs between patches, we prefer the `-s` option that is
automatically supported via our use of diff_opt_parse().
Signed-off-by: Johannes Schindelin
---
builtin/range-diff.c | 25 ++---
range-diff.c | 34 +++---
range-diff.h | 4 +++-
3 files changed, 56 insertions(+), 7 deletions(-)
diff --git a/builtin/range-diff.c b/builtin/range-diff.c
index 3881da246..093202117 100644
--- a/builtin/range-diff.c
+++ b/builtin/range-diff.c
@@ -2,6 +2,7 @@
#include "builtin.h"
#include "parse-options.h"
#include "range-diff.h"
+#include "config.h"
static const char * const builtin_range_diff_usage[] = {
N_("git range-diff [] .. .."),
@@ -13,16 +14,33 @@ NULL
int cmd_range_diff(int argc, const char **argv, const char *prefix)
{
int creation_factor = 60;
+ struct diff_options diffopt = { NULL };
struct option options[] = {
OPT_INTEGER(0, "creation-factor", &creation_factor,
N_("Percentage by which creation is weighted")),
OPT_END()
};
- int res = 0;
+ int i, j, res = 0;
struct strbuf range1 = STRBUF_INIT, range2 = STRBUF_INIT;
+ git_config(git_diff_ui_config, NULL);
+
+ diff_setup(&diffopt);
+ diffopt.output_format = DIFF_FORMAT_PATCH;
+
argc = parse_options(argc, argv, NULL, options,
-builtin_range_diff_usage, 0);
+builtin_range_diff_usage, PARSE_OPT_KEEP_UNKNOWN);
+
+ for (i = j = 0; i < argc; ) {
+ int c = diff_opt_parse(&diffopt, argv + i, argc - i, prefix);
+
+ if (!c)
+ argv[j++] = argv[i++];
+ else
+ i += c;
+ }
+ argc = j;
+ diff_setup_done(&diffopt);
if (argc == 2) {
if (!strstr(argv[0], ".."))
@@ -57,7 +75,8 @@ int cmd_range_diff(int argc, const char **argv, const char
*prefix)
usage_with_options(builtin_range_diff_usage, options);
}
- res = show_range_diff(range1.buf, range2.buf, creation_factor);
+ res = show_range_diff(range1.buf, range2.buf, creation_factor,
+ &diffopt);
strbuf_release(&range1);
strbuf_release(&range2);
diff --git a/range-diff.c b/range-diff.c
index 2d94200d3..71883a4b7 100644
--- a/range-diff.c
+++ b/range-diff.c
@@ -6,6 +6,7 @@
#include "hashmap.h"
#include "xdiff-interface.h"
#include "linear-assignment.h"
+#include "diffcore.h"
struct patch_util {
/* For the search for an exact match */
@@ -258,7 +259,31 @@ static const char *short_oid(struct patch_util *util)
return find_unique_abbrev(&util->oid, DEFAULT_ABBREV);
}
-static void output(struct string_list *a, struct string_list *b)
+static struct diff_filespec *get_filespec(const char *name, const char *p)
+{
+ struct diff_filespec *spec = alloc_filespec(name);
+
+ fill_filespec(spec, &null_oid, 0, 0644);
+ spec->data = (char *)p;
+ spec->size = strlen(p);
+ spec->should_munmap = 0;
+ spec->is_stdin = 1;
+
+ return spec;
+}
+
+static void patch_diff(const char *a, const char *b,
+ struct diff_options *diffopt)
+{
+ diff_queue(&diff_queued_diff,
+ get_filespec("a", a), get_filespec("b", b));
+
+ diffcore_std(diffopt);
+ diff_flush(diffopt);
+}
+
+static void output(struct string_list *a, struct string_list *b,
+ struct diff_options *diffopt)
{
int i = 0, j = 0;
@@ -300,6 +325,9 @@ static void output(struct string_list *a, struct
string_list *b)
printf("%d: %s ! %d:
Re: [PATCH v2] add -p: fix counting empty context lines in edited patches
Jeff Felchner writes: > Hey all, I assumed this was going to be in 2.18, but I'm still having the > same issue. What's the plan for release of this? You assumed wrong ;-) A patch written on June 11th that is already deep into pre-release freeze, unless it is about fixing a regression during the same cycle, would never be in the release tagged on 21st. It is already a part of the 'master' branch after v2.18, so v2.19 would be the first feature release that would see it (unless we discover problems in that change and need to revert it, that is).
Re: [PATCH v2] add -p: fix counting empty context lines in edited patches
Hey all, I assumed this was going to be in 2.18, but I'm still having the same
issue. What's the plan for release of this?
> On 2018 Jun 11, at 4:46, Phillip Wood wrote:
>
> From: Phillip Wood
>
> recount_edited_hunk() introduced in commit 2b8ea7f3c7 ("add -p:
> calculate offset delta for edited patches", 2018-03-05) required all
> context lines to start with a space, empty lines are not counted. This
> was intended to avoid any recounting problems if the user had
> introduced empty lines at the end when editing the patch. However this
> introduced a regression into 'git add -p' as it seems it is common for
> editors to strip the trailing whitespace from empty context lines when
> patches are edited thereby introducing empty lines that should be
> counted. 'git apply' knows how to deal with such empty lines and POSIX
> states that whether or not there is an space on an empty context line
> is implementation defined [1].
>
> Fix the regression by counting lines that consist solely of a newline
> as well as lines starting with a space as context lines and add a test
> to prevent future regressions.
>
> [1] http://pubs.opengroup.org/onlinepubs/9699919799/utilities/diff.html
>
> Reported-by: Mahmoud Al-Qudsi
> Reported-by: Oliver Joseph Ash
> Reported-by: Jeff Felchner
> Signed-off-by: Phillip Wood
> ---
>
> Thanks for the feedback, the only changes since v1 are to fix the
> commit message to match what was in pu and to change '$_' to '$mode'
> in the comparison as I think that is clearer. In the end I decided to
> leave the tests as they are.
>
> git-add--interactive.perl | 2 +-
> t/t3701-add-interactive.sh | 43 ++
> 2 files changed, 44 insertions(+), 1 deletion(-)
>
> diff --git a/git-add--interactive.perl b/git-add--interactive.perl
> index ab022ec073..8361ef45e7 100755
> --- a/git-add--interactive.perl
> +++ b/git-add--interactive.perl
> @@ -1047,7 +1047,7 @@ sub recount_edited_hunk {
> $o_cnt++;
> } elsif ($mode eq '+') {
> $n_cnt++;
> - } elsif ($mode eq ' ') {
> + } elsif ($mode eq ' ' or $mode eq "\n") {
> $o_cnt++;
> $n_cnt++;
> }
> diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
> index e5c66f7500..f1bb879ea4 100755
> --- a/t/t3701-add-interactive.sh
> +++ b/t/t3701-add-interactive.sh
> @@ -175,6 +175,49 @@ test_expect_success 'real edit works' '
> diff_cmp expected output
> '
>
> +test_expect_success 'setup file' '
> + test_write_lines a "" b "" c >file &&
> + git add file &&
> + test_write_lines a "" d "" c >file
> +'
> +
> +test_expect_success 'setup patch' '
> + SP=" " &&
> + NULL="" &&
> + cat >patch <<-EOF
> + @@ -1,4 +1,4 @@
> + a
> + $NULL
> + -b
> + +f
> + $SP
> + c
> + EOF
> +'
> +
> +test_expect_success 'setup expected' '
> + cat >expected <<-EOF
> + diff --git a/file b/file
> + index b5dd6c9..f910ae9 100644
> + --- a/file
> + +++ b/file
> + @@ -1,5 +1,5 @@
> + a
> + $SP
> + -f
> + +d
> + $SP
> + c
> + EOF
> +'
> +
> +test_expect_success 'edit can strip spaces from empty context lines' '
> + test_write_lines e n q | git add -p 2>error &&
> + test_must_be_empty error &&
> + git diff >output &&
> + diff_cmp expected output
> +'
> +
> test_expect_success 'skip files similarly as commit -a' '
> git reset &&
> echo file >.gitignore &&
> --
> 2.17.0
>
Re: [PATCH v2 0/4] Automatic transfer encoding for patches
LGTM, thanks for the v2.
[PATCH v2 0/4] Automatic transfer encoding for patches
This series introduces an "auto" value for git send-email --transfer-encoding that uses 8bit when possible (i.e. when lines are 998 octets or shorter) and quoted-printable otherwise; it then makes this the default behavior. It also makes --validate aware of transfer encoding so it doesn't complain when using quoted-printable or base64. Changes from v1: * Update commit messages to refer to RFC 5322. * Add a missing space. * Remove the needless capture of stderr. * Define "suitable transfer encoding". * Invert test to better capture failures. * Wrap --validate code in an if block instead of returning early. * Update documentation to reflect correct, modern RFC. brian m. carlson (4): send-email: add an auto option for transfer encoding send-email: accept long lines with suitable transfer encoding send-email: automatically determine transfer-encoding docs: correct RFC specifying email line length Documentation/git-send-email.txt | 17 ++ git-send-email.perl | 46 +- t/t9001-send-email.sh| 57 3 files changed, 91 insertions(+), 29 deletions(-)
