Re: [PATCH v2 1/2] for-each-ref: re-structure code for moving to 'ref-filter'
Also I think Matthieu already commented that "filter" was out of place for that struct. I still think your ref_list is better called ref_array, but that is a minor point. Use of "foo_list" in our codebase is predominantly (because we use "commit_list" very often in the core part of the system) for a linear linked list where you do not have a random access to the items. string-list is misnomer, I would think. ref_array also sounds good, yes! there might be confusion and might be considered a linked list rather than an array. Will change. I think you now know after seeing that 56-patch series ;-) Haha, That definitely helped. If that is the case, I would suggest making that "turn it flex array" a separate step. Sure. -- Regards, Karthik -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: git clone --depth: shallow clone problems
On Thu, Apr 23, 2015 at 04:53:04PM +0200, Michal Pomorski wrote: > tl: skip to the second paragraph > > So here is what I just experienced: > We had an emergency error in an application at work and as the > responsible developer was unavailable, I was asked to check it out and > look into it. > We are a small branch of a bigger company and our connection to the > company's source servers is really slow, so just to quickly start it > up, I decided to take a shallow clone (that's what it is for, right?). > After a while we realized, the clone I have made was not sufficient > and was missing some newest work done on the project. > I did "git fetch --unshallow" which finished surprisingly quickly, and > it did not bring any newer commits. > Unaware of the fine print hiding in the documentation of git clone, I > blamed the repo (and in extension the person who provided me the > address to it). After coming to a realization, a process which cost me > time and embarrassment, I understood what is supposed to be the > "correct" behaviour: > "--single-branch > Clone only the history leading to the tip of a single branch, either > specified by the --branch option or the primary branch remote's HEAD > points at. When creating a shallow clone with the --depth option, this > is the default, unless --no-single-branch is given to fetch the > histories near the tips of all branches." > Of course, the new commits were put on a custom branch, and I knew > that all the time, but I expected the branch to show up eventually, at > least after git fetch --unshallow. > > > I hope you can see the faults in the usability of the commands I was > exposed to: > 1) git clone --depth should: >-warn about only fetching the current HEAD (name it by a real name > if applicable) > and/or >-require the --branch option so that it is not left to chance > (current HEAD could be anything; is it really meaningful to talk about > the current HEAD on a server?) > and/or >-make the --no-single-branch the default... > and maybe >-have the option to clone the most recent commits. --single-branch being the default was because we (at least I, stilll) believed it was the common case, so I don't think we should revert it. But I can see --unshallow documentation is misleading. How about this? I think it's better than what we have. -- 8< -- Subject: [PATCH] fetch-options.txt: clarify what --unshallow does Signed-off-by: Nguyễn Thái Ngọc Duy --- Documentation/fetch-options.txt | 11 --- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/Documentation/fetch-options.txt b/Documentation/fetch-options.txt index 45583d8..63d3452 100644 --- a/Documentation/fetch-options.txt +++ b/Documentation/fetch-options.txt @@ -14,12 +14,17 @@ branch history. Tags for the deepened commits are not fetched. --unshallow:: - If the source repository is complete, convert a shallow - repository to a complete one, removing all the limitations + If the source repository is complete, convert all shallow + refs to complete ones, removing all the limitations imposed by shallow repositories. + If the source repository is shallow, fetch as much as possible so that -the current repository has the same history as the source repository. +the all existing refs of current repository have the same history as in +the source repository. ++ +Note that if the repository is created with --single-branch, which is +default for a shallow clone, only one ref is completed. `--unshallow` +does not fetch all remaining refs from source repository. --update-shallow:: By default when fetching from a shallow repository, -- 2.3.0.rc1.137.g477eb31 -- 8< -- > 2) git fetch --unshallow should convert the clone into an actual > equivalent of a normal, not shallow clone. I was thinking of some way to undo --single-branch too. I don't think it should be the job of --unshallow, maybe a new option, but I can't think of a name better than --really-unshallow :P It's been a while and no one responds to this, perhaps people are not interested in such an option? > 3) The documentation should be improved. The behaviour of --shallow is > described partly in the description of --no-single-branch. This is the > documentation equivalent of the infamous "come from" control flow > structure. Yes. Like this? -- 8< -- Subject: [PATCH] clone.txt: mention --single-branch in --depth Signed-off-by: Nguyễn Thái Ngọc Duy --- Documentation/git-clone.txt | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt index f1f2a3f..9c320da 100644 --- a/Documentation/git-clone.txt +++ b/Documentation/git-clone.txt @@ -185,7 +185,9 @@ objects from the source repository into a pack in the cloned repository. --depth :: Create a 'shallow' clone with a history truncated to the - specified number of revisions. + specified number of revisions. --single-bran
Re: [PATCH] git-new-workdir: add windows compatibility
Hi Paul, On 2015-05-26 14:20, Paul Smith wrote: > On Tue, 2015-05-26 at 11:53 +0200, Johannes Schindelin wrote: >> The biggest problem with `mklink` is that it is only supported on >> Windows Vista and later, while I really like to keep Windows XP >> support in Git for Windows. > > No, the biggest problem with mklink is that you have to have > administrative privileges to use it... from wikipedia: > > http://en.wikipedia.org/wiki/NTFS_symbolic_link It is even worse than that, as you pointed out below: administrators *can* permit non-administrators to create and use of symbolic links. However, from a maintainer's perspective (which is my role in this thread), the compatibility problem is an even worse problem than the permissions. >> The default security settings in Windows Vista/Windows 7 disallow >> non-elevated administrators and all non-administrators from creating >> symbolic links. This behavior can be changed running "secpol.msc" the >> Local Security Policy management console (under: Security Settings >> \Local Policies\User Rights Assignment\Create symbolic links). It can >> be worked around by starting cmd.exe with Run as administrator option >> or the runas command. > > Except even that is not so simple, as various StackOverflow questions > and answers will show (I have to run so I can't look them up now). I > did try to get this to work a year or so ago, and although I'm in no way > a Windows person (so maybe someone else would have better luck) I > couldn't get it to work. For what it is worth, I tried my hand a couple of years ago at the project to move git-new-workdir to use the `.git` *file* and alternates mechanisms, but that does not work because you really need a separate `.git/HEAD`. Ciao, Johannes -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: recovering from "unordered stage entries in index" error
> see these commands, or something else. Could you try again with > GIT_TRACE=/absolute/path/to/some/where instead of GIT_TRACE=2 and post > the content of /abso../some/where? It looks the same as far as I can see: $ GIT_TRACE=/tmp/git-trace git svn fetch fatal: unordered stage entries in index write-tree: command returned error: 128 $ egrep -i 'read|tree|update|index' /tmp/git-trace 13:26:08.169921 git.c:348 trace: built-in: git 'write-tree' $ cat /tmp/git-trace 09:26:07.402735 git.c:557 trace: exec: 'git-svn' 'fetch' 09:26:07.402784 run-command.c:351 trace: run_command: 'git-svn' 'fetch' 09:26:07.688866 git.c:348 trace: built-in: git 'rev-parse' '--show-prefix' 13:26:07.691207 git.c:348 trace: built-in: git 'rev-parse' '--git-dir' 13:26:07.693228 git.c:348 trace: built-in: git 'rev-parse' '--show-cdup' 13:26:07.695594 git.c:348 trace: built-in: git 'config' '--bool' '--get' 'svn.quiet' 13:26:07.697279 git.c:348 trace: built-in: git 'config' '--get' 'svn.authorsprog' 13:26:07.699030 git.c:348 trace: built-in: git 'config' '--get' 'svn.ignorepaths' 13:26:07.700914 git.c:348 trace: built-in: git 'config' '--get' 'svn.authorsfile' 13:26:07.702872 git.c:348 trace: built-in: git 'config' '--bool' '--get' 'svn.followparent' 13:26:07.704949 git.c:348 trace: built-in: git 'config' '--get' 'svn.configdir' 13:26:07.706674 git.c:348 trace: built-in: git 'config' '--bool' '--get' 'svn.localtime' 13:26:07.708590 git.c:348 trace: built-in: git 'config' '--bool' '--get' 'svn.useSvmProps' 13:26:07.710669 git.c:348 trace: built-in: git 'config' '--bool' '--get' 'svn.useSvnsyncProps' 13:26:07.712602 git.c:348 trace: built-in: git 'config' '--bool' '--get' 'svn.noauthcache' 13:26:07.714527 git.c:348 trace: built-in: git 'config' '--get' 'svn.username' 13:26:07.716257 git.c:348 trace: built-in: git 'config' '--int' '--get' 'svn.logwindowsize' 13:26:07.717872 git.c:348 trace: built-in: git 'config' '--bool' '--get' 'svn.noMetadata' 13:26:07.719905 git.c:348 trace: built-in: git 'config' '--get' 'svn.ignorerefs' 13:26:07.721623 git.c:348 trace: built-in: git 'config' '--get' 'svn.includepaths' 13:26:07.723485 git.c:348 trace: built-in: git 'config' '--bool' '--get' 'svn.nocheckout' 13:26:07.725441 git.c:348 trace: built-in: git 'config' '--bool' '--get' 'svn.addauthorfrom' 13:26:07.727125 git.c:348 trace: built-in: git 'config' '--get' 'svn.repackflags' 13:26:07.729179 git.c:348 trace: built-in: git 'config' '--int' '--get' 'svn.repack' 13:26:07.731143 git.c:348 trace: built-in: git 'config' '--bool' '--get' 'svn.uselogauthor' 13:26:07.733139 git.c:348 trace: built-in: git 'config' '--bool' '--get' 'svn.parent' 13:26:07.734911 git.c:348 trace: built-in: git 'config' '--bool' '--get' 'svn.fetchall' 13:26:07.736992 git.c:348 trace: built-in: git 'config' '--get' 'svn.revision' 13:26:07.739398 git.c:348 trace: built-in: git 'rev-parse' '--symbolic' '--all' 13:26:07.744514 git.c:348 trace: built-in: git 'config' '-l' 13:26:07.746770 git.c:348 trace: built-in: git 'config' '-l' 13:26:07.749108 git.c:348 trace: built-in: git 'config' '--bool' 'svn.useSvmProps' 13:26:07.751613 git.c:348 trace: built-in: git 'config' '-l' 13:26:07.907707 git.c:348 trace: built-in: git 'config' '--int' '--get' 'svn-remote.svn.branches-maxRev' 13:26:07.910171 git.c:348 trace: built-in: git 'config' '--int' '--get' 'svn-remote.svn.tags-maxRev' 13:26:07.912608 git.c:348 trace: built-in: git 'config' '--get' 'svn-remote.svn.url' 13:26:07.915539 git.c:348 trace: built-in: git 'config' '--get' 'svn-remote.svn.pushurl' 13:26:07.917825 git.c:348 trace: built-in: git 'config' '--get' 'svn-remote.svn.uuid' 13:26:07.919620 git.c:348 trace: built-in: git 'rev-parse' '--verify' 'refs/remotes/trunk^0' 13:26:07.923752 git.c:348 trace: built-in: git 'rev-list' '--pretty=raw' '--reverse' '74332b7d653cde7ba3b999cc7b0adcfd9d924440..refs/remotes/trunk' '--' 13:26:07.926128 git.c:348 trace: built-in: git 'config' '--get' 'svn-remote.svn.rewriteRoot' 13:26:07.928707 git.c:348 trace: built-in: git 'config' '--get' 'svn-remote.svn.rewriteUUID' 13:26:07.933621 git.c:348 trace: built-in: git 'cat-file' '--batch' 13:26:08.150396 git.c:348 trace: built-in: git 'config' 'svn-remote.svn.branches-maxRev' '231655' 13:26:08.152963 git.c:348 trace: built-in: git 'config' 'svn-remote.svn.tags-maxRev' '231655' 1
Re: recovering from "unordered stage entries in index" error
On Tue, May 26, 2015 at 8:28 PM, McHenry, Matt wrote: >> see these commands, or something else. Could you try again with >> GIT_TRACE=/absolute/path/to/some/where instead of GIT_TRACE=2 and post >> the content of /abso../some/where? > > It looks the same as far as I can see: > > $ GIT_TRACE=/tmp/git-trace git svn fetch > fatal: unordered stage entries in index > write-tree: command returned error: 128 > > $ egrep -i 'read|tree|update|index' /tmp/git-trace > 13:26:08.169921 git.c:348 trace: built-in: git 'write-tree' OK I give up. Can't think of how the index is written, and by whom. -- Duy -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git-new-workdir: add windows compatibility
On Tue, 2015-05-26 at 11:53 +0200, Johannes Schindelin wrote: > The biggest problem with `mklink` is that it is only supported on > Windows Vista and later, while I really like to keep Windows XP > support in Git for Windows. No, the biggest problem with mklink is that you have to have administrative privileges to use it... from wikipedia: http://en.wikipedia.org/wiki/NTFS_symbolic_link > The default security settings in Windows Vista/Windows 7 disallow > non-elevated administrators and all non-administrators from creating > symbolic links. This behavior can be changed running "secpol.msc" the > Local Security Policy management console (under: Security Settings > \Local Policies\User Rights Assignment\Create symbolic links). It can > be worked around by starting cmd.exe with Run as administrator option > or the runas command. Except even that is not so simple, as various StackOverflow questions and answers will show (I have to run so I can't look them up now). I did try to get this to work a year or so ago, and although I'm in no way a Windows person (so maybe someone else would have better luck) I couldn't get it to work. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Pushing and pulling the result of `git replace` and objects/info/alternates
On Tue, May 26, 2015 at 12:39 PM, Christian Couder wrote: > First it looks like you sent the email to me only, so I am replying to you > only. > If this was a mistake, feel free to post this email to the Git mailing list. Thanks, sorry for the mis-post. >> 1) How would Alice push the content to a remote host so that Bob would >> get that automatically? > > I am not sure what you want exactly, but let me try to answer anyway. > > Let's suppose that the content is in another submodule, let's call it > subA, and let's call subB the submodule that should reference content > in subA. Yes, that's the scenario in my script. > If subA has been pushed on the remote host, then Bob can clone subA > first, and then clone subB using the --reference option to point to > the content of subA. Ah. Here's some confusion maybe. Alice pushes subA and subB *and* the supermodule. In my script, these were named calculator, compute and appsuite. The supermodule is the entry point that everyone uses. Bob clones the supermodule, appsuite, and expects that to 'just work' regarding history. So, I want to somehow specify the --reference in the .gitmodules of the appsuite supermodule. Then when Bob runs git submodule update --init, the right thing will be done. > > Please note that I don't know much about git submodules, as I try not > to use them myself, Me too :), but needs must. > so I am not sure there is a way to make them do > exactly what you want. Maybe you should look at the threads about > submodules on the Git mailing list, see who are the people involved > and send an email on the list with those people in CC and a subject > related to submodules and with your specific questions about > submodules in the content. Ok, thanks. I think the solution of running a script after initial clone/update is probably the most suitable for now anyway without getting deeper into git. > For example I don't know if there is a way to tell that subA should be > cloned before subB or something like that. Right. A step of performing actions like this would need to be done after all fetches are done I guess. >> 2) Can git submodules be configured to use particular options when >> cloning particular repos? I see no relevant options in the >> >> https://www.kernel.org/pub/software/scm/git/docs/gitmodules.html >> >> page. > > I don't know. Maybe it's possible to add a > "submodule..cloneOptions" option to specify them. Or maybe it's > possible to use the "submodule..update" config option with a > specific command (see "custom command" in > http://git-scm.com/docs/git-submodule) to do it. Yes, actually the 'custom command' section there might be useful... I might try it. >>> You might also be able to clone using an option like "--config >>> remote.origin.fetch = 'refs/replace/*:refs/replace/*'" to fetch the >>> replace ref when cloning. >> >> Cool, but I guess the same second question applies here about whether >> a submodule can be configured to fetch like that when the user does a >> update --init ? > > Yeah, the same question applies. > >> Otherwise, I'm not sure what you are suggesting. > > I am not suggesting anything else. > >> echo "../../calculator/objects" > >> ../.git/modules/compute/objects/info/alternates >> git replace --graft HEAD $extraction_sha > Maybe use the following instead of the above line: > > git fetch 'refs/replace/*:refs/replace/*' Thanks. >> # And now we see the history from the calculator repo. Great. But, it >> required user action after the clone. > Yeah, but if the 2 above commands are in a script maybe it's > reasonable to ask the user to launch the script once after cloning. Would it be possible to do this in a hook in the 'integration repo' which contains both submodules in the example I posted? Like a fetch hook or something? >>> >>> It is possible to do whatever you want in a hook, but the question is >>> why would you do it in a hook as it looks like it needs to be done >>> only once? >>> >>> Or maybe I am missing something? >> >> The goal is to make it transparent to users, so that no one needs to >> remember to 'do something once', but just gets a working checkout by >> cloning the submodule in the plain, normal, 'what you learn on day >> one' way. That is, >> >> git clone git://some/remote/appsuite appsuite-clone >> cd appsuite-clone >> git submodule update --init >> cd compute >> ls ../.git/modules/compute/objects/info >> git log --oneline >> >> should show the history despite the split. > > Yeah, it would be nice if that would work, but, I am not sure it can > work like that right now. > > And using hooks doesn't change anything as you have to setup those hooks > anyway. > >>> So it looks like you might just need to clone with a few more options >>> than usual. >>> >>> I haven't tested it so please tell me if it works :-) >> >> I changed the last 20 or so lines with one of your suggestions. I put >> the initial rev
LED WALL MOUNT
Dear friend, Hello and Good day! Hope everything goes well . Rebecca here from Jiaweihao , LED/LCD TV brackets, Wall Mounts manufacturer in China for years. Here to introduce our Newest Design table bracket, details as below: a) VESA: 75x75,100x100mm b) Capacity of loading: 10kg c) Table clamp type with key board stand. d) Key board stand and mount height adjustable. Welcome to contact us for any more information! Thanks and best regards Rebecca Skype: Rebecca307619
Re: [PATCH] git-new-workdir: add windows compatibility
Hi, On 2015-05-26 06:03, Junio C Hamano wrote: > Daniel Smith writes: > >> When running on Windows in MinGW, creating symbolic links via ln always >> failed. >> >> Using mklink instead of ln is the recommended method of creating links on >> Windows: >> http://stackoverflow.com/questions/18641864/git-bash-shell-fails-to-create-symbolic-links >> >> Script now branches on Windows to use mklink. This change should not affect >> unix systems. >> >> Signed-off-by: Daniel Smith >> >> Has been tested on Windows 8.1 and OS X Yosemite. >> --- > > Swap the "Has been tested..." and "Signed-off-by:" lines. > > I'll defer to Windows folks if "mklink" is a sensible thing to use > or not; I have no first-hand experience with Windows, but only heard > that links are for admin user only or something like that, so I want > to hear from people whose judgement on Windows matters I trust. The biggest problem with `mklink` is that it is only supported on Windows Vista and later, while I really like to keep Windows XP support in Git for Windows. That is why Karsten Blees' symlink support (emulated via NTFS reparse points) which I just merged into Git for Windows yesterday is so careful about everything. But git-new-workdir is in `contrib/`, anyway, and not installed in Git for Windows by default, therefore it is less critical: interested parties will have to be a bit knowledgeable in any case. So I am fine with it! Ciao, Dscho -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Implementation of git rebase --status
Hi, I would like to implement a new command git rebase --status to inform the user about the current rebase session. Here is a sample of what I think it could look like in case of merge conflict: git rebase --status You are in the middle of a rebase session. The line that paused this session is: pick 848a16f commit with conflicts CONFLICT (content): Merge conflict in file1 Consult and edit remaining actions with git rebase --edit-todo In case of syntax error: git rebase --status You are in the middle of a rebase session. The line that paused this session is: tick 848a16f syntax error SYNTAX ERROR Consult and edit remaining actions with git rebase --edit-todo In case of error during the execution of a script: git rebase --status You are in the middle of a rebase session. The line that paused this session is: exec exit 3 ERROR IN SCRIPT EXECUTION Consult and edit remaining actions with git rebase --edit-todo Do you think it could be usefull or do you have any suggestion? Thanks, Guillaume Pagès -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: git clone --depth: shallow clone problems
Duy Nguyen writes:
> --unshallow::
> - If the source repository is complete, convert a shallow
> - repository to a complete one, removing all the limitations
> + If the source repository is complete, convert all shallow
> + refs to complete ones, removing all the limitations
Define "shallow ref", or better yet explain without introducing such
a new term (I do not think shallow-ness is a property of a ref, by
the way---I think you meant all refs that can reach the points the
history is cauterised by .git/shallow, but we shouldn't assume that
the target audience of this paragraph to know Git as well as I do).
> imposed by shallow repositories.
> +
> If the source repository is shallow, fetch as much as possible so that
> -the current repository has the same history as the source repository.
> +the all existing refs of current repository have the same history as in
> +the source repository.
Makes sense, modulo "so that the all existing refs" sounds strange
to my ears ("all the existing refs" perhaps).
> ++
> +Note that if the repository is created with --single-branch, which is
> +default for a shallow clone, only one ref is completed. `--unshallow`
> +does not fetch all remaining refs from source repository.
I do not think this "Note" is being as helpful as it could be.
- If the repository was created with --single-branch but if the
user later fetched and started tracking other branches, the
statement "only one ref is completed" is untrue; the true version
is "only the existing refs are completed", which leads to a more
important point. It says the same thing as "all existing refs"
above and does not add any useful information.
- The last sentence is less than useful but merely frustrating---it
says what you cannot do with this option, strongly hints that the
writer of the sentence knows what the reader wants to achieve,
but without saying what other way the reader go to achieve it.
Perhaps replace that Note paragraph with something along this line?
+
By fetching and tracking refs that you haven't been tracking,
you can add these new refs to "all refs" to be unshallowed.
>> 2) git fetch --unshallow should convert the clone into an actual
>> equivalent of a normal, not shallow clone.
>
> I was thinking of some way to undo --single-branch too. I don't think
> it should be the job of --unshallow, maybe a new option, but I can't
> think of a name better than --really-unshallow :P
Isn't that just the matter of updating remote.origin.fetch? I do
not think it belongs to "clone" (or "fetch"). Perhaps it belongs to
"remote", where it already shows with "git remote -v" what is
fetched and pushed.
> --depth ::
> Create a 'shallow' clone with a history truncated to the
> - specified number of revisions.
> + specified number of revisions. --single-branch is
> + automatically specified unless the user overrides it with
> + --no-single-branch
Yeah, something like that would be a definite improvement.
By the way, while composing this message, I scratched my head after
typing
$ git clone --shallow=4 --no-local ./git.git ./playpen
Is it just me or do we want to add such a synonym?
Thanks.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Pushing and pulling the result of `git replace` and objects/info/alternates
On Tue, May 26, 2015 at 4:10 PM, Stephen Kelly wrote: > On Tue, May 26, 2015 at 12:39 PM, Christian Couder > wrote: > >>> 1) How would Alice push the content to a remote host so that Bob would >>> get that automatically? >> >> I am not sure what you want exactly, but let me try to answer anyway. >> >> Let's suppose that the content is in another submodule, let's call it >> subA, and let's call subB the submodule that should reference content >> in subA. > > Yes, that's the scenario in my script. > >> If subA has been pushed on the remote host, then Bob can clone subA >> first, and then clone subB using the --reference option to point to >> the content of subA. > > Ah. Here's some confusion maybe. > > Alice pushes subA and subB *and* the supermodule. In my script, these > were named calculator, compute and appsuite. The supermodule is the > entry point that everyone uses. > > Bob clones the supermodule, appsuite, and expects that to 'just work' > regarding history. > > So, I want to somehow specify the --reference in the .gitmodules of > the appsuite supermodule. Then when Bob runs git submodule update > --init, the right thing will be done. Yeah I understand and I am trying to help you do something like that, though I can only talk about some of the steps involved, and this may or may not help you find a complete solution that is good enough for your needs. >> Please note that I don't know much about git submodules, as I try not >> to use them myself, > > Me too :), but needs must. > >> so I am not sure there is a way to make them do >> exactly what you want. Maybe you should look at the threads about >> submodules on the Git mailing list, see who are the people involved >> and send an email on the list with those people in CC and a subject >> related to submodules and with your specific questions about >> submodules in the content. > > Ok, thanks. I think the solution of running a script after initial > clone/update is probably the most suitable for now anyway without > getting deeper into git. Yeah, the user might just run a script instead of "git submodule update --init". This way it doesn't increase the number of steps that have to be performed. >> For example I don't know if there is a way to tell that subA should be >> cloned before subB or something like that. > > Right. A step of performing actions like this would need to be done > after all fetches are done I guess. > >>> 2) Can git submodules be configured to use particular options when >>> cloning particular repos? I see no relevant options in the >>> >>> https://www.kernel.org/pub/software/scm/git/docs/gitmodules.html >>> >>> page. >> >> I don't know. Maybe it's possible to add a >> "submodule..cloneOptions" option to specify them. Or maybe it's >> possible to use the "submodule..update" config option with a >> specific command (see "custom command" in >> http://git-scm.com/docs/git-submodule) to do it. > > Yes, actually the 'custom command' section there might be useful... I > might try it. Great, tell us what you come up with. So it looks like you might just need to clone with a few more options than usual. I haven't tested it so please tell me if it works :-) >>> >>> I changed the last 20 or so lines with one of your suggestions. I put >>> the initial revision and the update on a gist: >>> >>> https://gist.github.com/steveire/a57bc48a460e11284d81/revisions >>> >>> The script I posted is easy to modify if you want to try something >>> out. I would be happy if you would try it out and see if you can make >>> your suggestion work. >> >> I tried it and it looks like it works for me as it works for you. >> >> There is: >> >>> git fetch origin 'refs/replace/*:refs/replace/*' >>> # Don't seem to need this? Why? >>> # Does the push of the replace refs copy them to the remote repo? >>> # How do I find out? >>> # echo "../../calculator/objects" > >>> ../.git/modules/compute/objects/info/alternates >> >> The above comments probably mean that you didn't expect that fetching >> replace refs would also fetch the git objects (commits, trees, blobs, >> ...) pointed to by the replace refs. But that's what fetching does >> with any king of ref (branches, tags, notes and replace refs). > > Actually, what I didn't expect is that > > # Push the replacement to the remote submodule clone > git push origin 'refs/replace/*' > > would push a copy of the content reachable by the 'refs/replace/*', > totally bypassing what I did with info/objects/alternates. Well if you had also setup info/objects/alternates on the server, it would have been used there. When pushing refs, as well as when fetching refs, Git sends the objects pointed to by the refs that are transfered, if those objects are not already available, so that the result is in a consistent state. So if you setup info/objects/alternates on the server before pushing, the server will see the objects as already available in the alternates repo, and they will not be sent to the serv
Re: Re: [PATCH v3 0/4] submodule config lookup API
On Thu, May 21, 2015 at 08:50:11PM +0200, René Scharfe wrote: > Am 21.05.2015 um 19:06 schrieb Heiko Voigt: > >diff --git a/submodule-config.h b/submodule-config.h > >index 9061e4e..58afc83 100644 > >--- a/submodule-config.h > >+++ b/submodule-config.h > >@@ -24,6 +24,6 @@ const struct submodule *submodule_from_name(const unsigned > >char *commit_sha1, > > const char *name); > > const struct submodule *submodule_from_path(const unsigned char > > *commit_sha1, > > const char *path); > >-void submodule_free(void); > >+void submodule_free(); > > Why this change? With void it matches the function's definition. Sorry oversight on my side will remove it. Junio added those on his side but it seems I forgot it in the header. Cheers Heiko -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Re: [PATCH v3 0/4] submodule config lookup API
On Thu, May 21, 2015 at 11:40:44AM -0700, Junio C Hamano wrote:
> Heiko Voigt writes:
>
> > This is finally the next iteration of the submodule config api. The last
> > iteration can be found here:
> >
> > http://article.gmane.org/gmane.comp.version-control.git/252601
> >
> > This iteration fixes the lookup of submodules by name
> > (submodule_from_name()) where one needed to pass in the gitmodule sha1
> > by mistake. To keep it simple for the user and behave as documented we
> > should take the commit sha1 which is now fixed here. We now also test
> > the lookup by name in the api tests.
> >
> > This should be ready for inclusion.
> >
> > Cheers Heiko
> >
> > Here is the interdiff to the last iteration:
> >
> > diff --git a/submodule-config.c b/submodule-config.c
> > index 96623ad..177767d 100644
> > --- a/submodule-config.c
> > +++ b/submodule-config.c
> > @@ -25,6 +25,11 @@ struct submodule_entry {
> > struct submodule *config;
> > };
> >
> > +enum lookup_type {
> > + lookup_name,
> > + lookup_path,
> > +};
>
> Please lose the comma after the last element in enum. Some
> compilers do not like it, I was told.
Fixed.
> > + switch (lookup_type) {
> > + case lookup_name:
> > + submodule = cache_lookup_name(cache, sha1, key);
> > + break;
> > + case lookup_path:
> > + submodule = cache_lookup_path(cache, sha1, key);
> > + break;
>
> Is this too deeply indented?
It seems I accidentially used vim's default indenting. Fixed.
Will wait a little longer if there are more comments than the style
fixes before sending another iteration.
Cheers Heiko
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/2] for-each-ref: re-structure code for moving to 'ref-filter'
Junio C Hamano writes: > Yuck; I can see what you are doing but can you imitate what the more > experienced people (e.g. peff, mhagger) do when restructuring > existing code and do things in smaller increments? Seconded. Some reasons/guide to split: * Split trivial and non-trivial stuff. I can quickly review a "rename-only" patch even if it's a bit long (essentially, I'll check that you did find-and-replace properly), but reviewing a mix of renames and actual code change is hard. * Split controversial and non-controversial stuff. For example, you changed the ordering of fields in a struct. Perhaps it was not a good idea. Perhaps it was a good idea, but then you want this reordering to be alone in its patch so that you can explain why it's a good idea in the commit message (you'll see me use the word "why" a lot when talking about commit messages; not a coincidence). * Split code movement and other stuff. For example extraction of match_name_as_path() would be trivial if made in its own patch. I'd also make a separate patch "introduce the ref_list data-structure" to introduce struct ref_list and basic helper functions (constructors, mutators, destructors). It will take time and may appear to be counter-productive at first, but you'll get used to it, and you'll end up being actually more productive this way (well, maybe not "you" but the set "you + reviewers"). -- Matthieu Moy http://www-verimag.imag.fr/~moy/ -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Implementation of git rebase --status
Guillaume Pages writes: > Do you think it could be usefull or do you have any suggestion? All of your examples say things like: > You are in the middle of a rebase session. > The line that paused this session is: but what if there is no such "line"? IOW, what does the user see when using this new option during a "git rebase" (not "git rebase -i")? Other than that, sounds like a neat thing to do. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git-new-workdir: add windows compatibility
Am 26.05.2015 um 06:03 schrieb Junio C Hamano: > Daniel Smith writes: > >> When running on Windows in MinGW, creating symbolic links via ln always >> failed. >> >> Using mklink instead of ln is the recommended method of creating links on >> Windows: >> http://stackoverflow.com/questions/18641864/git-bash-shell-fails-to-create-symbolic-links >> > > I'll defer to Windows folks if "mklink" is a sensible thing to use > or not; I have no first-hand experience with Windows, but only heard > that links are for admin user only or something like that, so I want > to hear from people whose judgement on Windows matters I trust. > mklink: - is not available on Windows XP - requires special permissions - does not work on network shares (unless enabled via 'fsutil behavior') - only works on file systems that support reparse points (e.g. NTFS, not FAT) AFAICT, the MSys2 symlink() implementation is pretty smart to detect these conditions and fall back to deep copy (aka 'cp -a') if symlinks are not supported. IOW, using 'ln -s' will hopefully "just work" in the upcoming Git for Windows 2, thus trying to fix it for MSys1 / Git for Windows 1.9x is probably a lost cause. Karsten -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Mark trailing whitespace error in del lines of diff
Hi Junio! On Mo, 25 Mai 2015, Junio C Hamano wrote: > "brian m. carlson" writes: > > > I like this idea. > > I don't. > > > My use case is determining whether a patch to a pristine-tar > > repository introduced trailing whitespace (which is not okay) or > > just left it there (which is okay). > > In your use case, where keeping trailing blank that is otherwise not > OK is fine only when the breakage was inherited from the preimage, > wouldn't it be equally fine to keep other kinds of breakages as long > as they were inherited from the preimage? E.g. "The original used > 8-space as leading indent, and you would not use that for your new > lines, but the breakage was inherited from the preimage" would want > to be treated the same way, no? Why trailing blanks so special? It was the one I am interesting in and also the one that I usually try to avoid ;) (In fact, I thought if the other options would be needed, one could add additional suboptions for core.whitespace as well, so one would be able to exactly say, what kind of things one would like to see and which could be different for new lines and old lines). > > So, from that point of view, your "use case" does not justify this > particular implementation that special-cases trailing blanks on > deleted lines and mark them [*1*]. > > If the implementation were addition of a new option to check and > mark all kinds of errors core.whitespace would catch for new lines > also for old lines, then it would be a somewhat different story. I > personally do not find such an option interesting, but at least I > can understand why some people might find it useful. Wouldn't that mean, that one couldn't see different kind of whitespace markings for newlines and deleted lines? I don't know, if one would want that a configuration however. However, as I don't know the codebase very well, I doubt I can implement this. > [Footnote] > > *1* To support your use case with the ultimate ease-of-use, it would > be best if the new option were to squelch the whitespace error on > the new line when it was inherited from the old line, which is > different from showing and marking the breakage on the old line. > But I do not think it can be implemented sanely, so I will not go > there. Aside from the difficulties it would take to do this, personally, I don't like this option. Because I like to see such problems, but just want to know whether a particular patch has introduced the problem or not. Best, Christian -- In den Fragen im gemeinen Leben, wie man etwas am besten tun könnte, wird ein gewisses Maximum gesucht. -- Georg Christoph Lichtenberg -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Mark trailing whitespace error in del lines of diff
Hi Junio! On Mo, 25 Mai 2015, Junio C Hamano wrote: > Christian Brabandt , Christian Brabandt > writes: > > > As far as I can see, this does not break any tests and also the > > behaviour of git-diff --check does not change. > > Even if this change introduced a bug that changed the behaviour > (e.g. say, exited with failure status code when only preimage had > errors), I wouldn't be surprised if no existing test caught such a > breakage. Because the existing tests were written with the > assumption that the code to check whitespace breakages would never > look at preimage, it is plausible that no preimage line used in the > test has any whitespace error in the first place. > > In other words, you'd need to add new tests that change preimage > lines with various kinds of whitespace errors into postimage lines > with and without whitespace errors, and run "diff" with various > combinations of the existing set of core.whitespace values as well > as your new one. > > But as I said in the other message, I think that the approach this > patch takes goes in a wrong direction. Instead of adding a single > "check and highlight this and only kind of breakage on preimage" > option as a new kind to existing "what kind of use of whitespaces > are errors" set, it would be more sensible to add a single "check > and highlight breakages on preimage lines as well" option that is > orthogonal to the existing ones that specify "what kind of use of > whitespaces are errors". Oh well, too bad. It sounded like a good idea... Best, Christian -- Unser Gefühl für Natur gleicht der Empfindung des Kranken für die Gesundheit. -- Friedrich Johann Christoph Schiller -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Query on git submodules
Hi, On Fri, May 22, 2015 at 01:46:24PM +, Frawley, Sarah wrote: > I am a design automation engineer supporting 200+ designers who use > git for hardware design. We also use the submodule feature where we > can have quite complex hierarchy's with 10+ layers. What does this 10+ layers mean? Nested submodule repositories 10 recursion steps deep? > We have experience issues with re-use of design projects was we move >from one derivative to another due to the complexity of the hierarchy >along with lack of discipline (using absolute paths in .gitmodule >files). To enforce more discipline I am currently working on a >pre-commit hook to check the integrity of .gitmodule files. I would be >interested in seeing how other users in the community find submodules >for large scale projects and if there are any best known methods for >using them. I do not have anything to share here since our projects did not have such problems (not that large scale). It would be interesting to see what you come up with. Maybe we can add some of that into core git to support such large scale projects better. So maybe you can share exactly what problems you have (only absolute paths?) or the pre-commit hooks you will use. Cheers Heiko -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Mark trailing whitespace error in del lines of diff
Christian Brabandt writes:
> It was the one I am interesting in and also the one that I usually try
> to avoid ;)
>
> (In fact, I thought if the other options would be needed, one could add
> additional suboptions for core.whitespace as well,...
That road leads to madness. Why should we add 2*N options in the
first place? What valid reason are there, other than "because we
could", to have "diff --ws-check-delelted" and "diff -R" paint
whitespace breakages differently?
I personally do not think I'd use such an option often, but I do
recall running "diff -R" and realized that we fixed whitespace
breakages in the past, which made me feel good (the reason why I
gave "-R" was not to see whitespace breakages in the preimage,
though; it merely was a side effect).
I'll send out two patch series to do the painting part (I didn't
want to touch "--check", as its utility is even more dubious
compared to painting, at least to me).
Here is the first one, a preliminary refactoring.
-- >8 --
Subject: [PATCH 1/2] diff.c: add emit_del_line() and restructure callers of
emit_line_0()
Traditionally, we only had emit_add_line() helper, which knows how
to find and paint whitespace breakages on the given line, because we
only care about whitespace breakages introduced in new lines. The
context lines and old (i.e. deleted) lines are emitted with a
simpler emit_line_0() that paints the entire line in plain or old
colors.
Some people may want to paint whitespace breakages on old lines;
when they see a whitespace breakage on a new line, they can spot
the same kind of whitespace breakage on the corresponding old line
and want to say "Ah, that breakage is there but was inherited from
the original, so let's not fix that for now."
To make such a future enhancement easier, introduce emit_del_line()
and replace direct calls to emit_line_0() with it.
Signed-off-by: Junio C Hamano
---
diff.c | 36
1 file changed, 24 insertions(+), 12 deletions(-)
diff --git a/diff.c b/diff.c
index 7500c55..93c1eb4 100644
--- a/diff.c
+++ b/diff.c
@@ -498,6 +498,15 @@ static void emit_add_line(const char *reset,
}
}
+static void emit_del_line(const char *reset,
+ struct emit_callback *ecbdata,
+ const char *line, int len)
+{
+ const char *set = diff_get_color(ecbdata->color_diff, DIFF_FILE_OLD);
+
+ emit_line_0(ecbdata->opt, set, reset, '-', line, len);
+}
+
static void emit_hunk_header(struct emit_callback *ecbdata,
const char *line, int len)
{
@@ -603,7 +612,6 @@ static void emit_rewrite_lines(struct emit_callback *ecb,
{
const char *endp = NULL;
static const char *nneof = " No newline at end of file\n";
- const char *old = diff_get_color(ecb->color_diff, DIFF_FILE_OLD);
const char *reset = diff_get_color(ecb->color_diff, DIFF_RESET);
while (0 < size) {
@@ -613,8 +621,7 @@ static void emit_rewrite_lines(struct emit_callback *ecb,
len = endp ? (endp - data + 1) : size;
if (prefix != '+') {
ecb->lno_in_preimage++;
- emit_line_0(ecb->opt, old, reset, '-',
- data, len);
+ emit_del_line(reset, ecb, data, len);
} else {
ecb->lno_in_postimage++;
emit_add_line(reset, ecb, data, len);
@@ -1250,17 +1257,22 @@ static void fn_out_consume(void *priv, char *line,
unsigned long len)
return;
}
- if (line[0] != '+') {
- const char *color =
- diff_get_color(ecbdata->color_diff,
- line[0] == '-' ? DIFF_FILE_OLD :
DIFF_PLAIN);
- ecbdata->lno_in_preimage++;
- if (line[0] == ' ')
- ecbdata->lno_in_postimage++;
- emit_line(ecbdata->opt, color, reset, line, len);
- } else {
+ switch (line[0]) {
+ case '+':
ecbdata->lno_in_postimage++;
emit_add_line(reset, ecbdata, line + 1, len - 1);
+ break;
+ case '-':
+ ecbdata->lno_in_preimage++;
+ emit_del_line(reset, ecbdata, line + 1, len - 1);
+ break;
+ default: /* both ' ' and incomplete line */
+ ecbdata->lno_in_preimage++;
+ ecbdata->lno_in_postimage++;
+ emit_line(ecbdata->opt,
+ diff_get_color(ecbdata->color_diff, DIFF_PLAIN),
+ reset, line, len);
+ break;
}
}
--
2.4.1-511-gc1146d5
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Mark trailing whitespace error in del lines of diff
Christian Brabandt writes: >> But as I said in the other message, I think that the approach this >> patch takes goes in a wrong direction. Instead of adding a single >> "check and highlight this and only kind of breakage on preimage" >> option as a new kind to existing "what kind of use of whitespaces >> are errors" set, it would be more sensible to add a single "check >> and highlight breakages on preimage lines as well" option that is >> orthogonal to the existing ones that specify "what kind of use of >> whitespaces are errors". > > Oh well, too bad. It sounded like a good idea... Oh, don't get me wrong. I do not think it is not a good idea (i.e. problem and general strategy to solve it). Problem: Sometimes it is desirable to keep existing whitespace breakages while working on fixes and other changes of substance. The user however gets whitespace errors painted only on new lines in the output from "git diff", and this makes it hard to tell if they were introduced by the user's change or came from the original. Strategy: By painting whitespace breakages in the old lines, the user can eyeball and find the corresponding old lines when whitespace errors on new lines are painted. If the corresponding old lines have the same errors, the user can see they were inherited from the original. These may be a pair of reasonable problem to solve and a general strategy to solve it. What I said was *not* good was the particular execution, the approach that the patch took. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Mark trailing whitespace error in del lines of diff
Junio C Hamano writes:
> I'll send out two patch series to do the painting part (I didn't
> want to touch "--check", as its utility is even more dubious
> compared to painting, at least to me).
And here is the second one.
-- >8 --
Subject: [PATCH 2/2] diff.c: --ws-check-deleted option
Traditionally, we only cared about whitespace breakages introduced
in new lines. Some people want to paint whitespace breakages on old
lines, too. When they see a whitespace breakage on a new line, they
can spot the same kind of whitespace breakage on the corresponding
old line and want to say "Ah, that breakage is there but was
inherited from the original, so let's not fix that for now."
Enable such use with the new option, "--ws-check-deleted".
Signed-off-by: Junio C Hamano
---
Documentation/diff-options.txt | 7 +++
diff.c | 21 -
diff.h | 1 +
3 files changed, 28 insertions(+), 1 deletion(-)
diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index b7c3afe..617cbc6 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt
@@ -282,6 +282,13 @@ ifndef::git-format-patch[]
initial indent of the line are considered whitespace errors.
Exits with non-zero status if problems are found. Not compatible
with --exit-code.
+
+--ws-check-deleted::
+--no-ws-check-deleted::
+ Highlight whitespace errors in deleted lines in the color
+ specified by `color.diff.whitespace`, as well as in added
+ lines.
+
endif::git-format-patch[]
--full-index::
diff --git a/diff.c b/diff.c
index 93c1eb4..44cc234 100644
--- a/diff.c
+++ b/diff.c
@@ -503,8 +503,22 @@ static void emit_del_line(const char *reset,
const char *line, int len)
{
const char *set = diff_get_color(ecbdata->color_diff, DIFF_FILE_OLD);
+ const char *ws = NULL;
- emit_line_0(ecbdata->opt, set, reset, '-', line, len);
+ if (ecbdata->opt->ws_check_deleted) {
+ ws = diff_get_color(ecbdata->color_diff, DIFF_WHITESPACE);
+ if (!*ws)
+ ws = NULL;
+ }
+
+ if (!ws)
+ emit_line_0(ecbdata->opt, set, reset, '-', line, len);
+ else {
+ /* Emit just the prefix, then the rest. */
+ emit_line_0(ecbdata->opt, set, reset, '-', "", 0);
+ ws_check_emit(line, len, ecbdata->ws_rule,
+ ecbdata->opt->file, set, reset, ws);
+ }
}
static void emit_hunk_header(struct emit_callback *ecbdata,
@@ -3819,6 +3833,11 @@ int diff_opt_parse(struct diff_options *options, const
char **av, int ac)
else if (skip_prefix(arg, "--submodule=", &arg))
return parse_submodule_opt(options, arg);
+ else if (!strcmp(arg, "--ws-check-deleted"))
+ options->ws_check_deleted = 1;
+ else if (!strcmp(arg, "--no-ws-check-deleted"))
+ options->ws_check_deleted = 0;
+
/* misc options */
else if (!strcmp(arg, "-z"))
options->line_termination = 0;
diff --git a/diff.h b/diff.h
index f6fdf49..baca5ec 100644
--- a/diff.h
+++ b/diff.h
@@ -138,6 +138,7 @@ struct diff_options {
int dirstat_permille;
int setup;
int abbrev;
+ int ws_check_deleted;
const char *prefix;
int prefix_length;
const char *stat_sep;
--
2.4.1-511-gc1146d5
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 00/56] Convert parts of refs.c to struct object_id
On Mon, May 25, 2015 at 12:40 PM, brian m. carlson wrote: > On Mon, May 25, 2015 at 12:34:59PM -0700, Junio C Hamano wrote: >> [PATCH 01/56] was authored by you but has Michael's sign-off, which >> looked somewhat odd to me, though. > > Yes, it does. He picked it up from me, and signed off, and I took his > branch. I don't believe he changed it, but I didn't check for certain. > So technically, although I wrote it, I also received it from him without > changing it, so both (a) and (c) of the DCO apply. At least in the kernel, the sign offs are also used to track a patchs way of life, so essentially whenever somebody touches that patch (either as an author, or as a patch shoveling sub Lieutenant), you'd add a sign off. So if we were to handle the sign offs just as the kernel people, I would have assumed a sign-off block like Sign-off: you Sign off: Michael Sign-off: you as that would indicate that Michael did not author it from scratch but based his work on yours. That's just my understanding of the sign off process for linux and I guessed we'd follow a very similar process. But no objections from me regarding the signing. All patches have been Skimmed-over-and-run-test-suite-by: Stefan Beller if that helps. > -- > brian m. carlson / brian with sandals: Houston, Texas, US > +1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only > OpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Mark trailing whitespace error in del lines of diff
Hi Junio! On Di, 26 Mai 2015, Junio C Hamano wrote: > Junio C Hamano writes: > > > I'll send out two patch series to do the painting part (I didn't > > want to touch "--check", as its utility is even more dubious > > compared to painting, at least to me). > > And here is the second one. Wow, great and so fast! I really apologize it. Best, Christian -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Mark trailing whitespace error in del lines of diff
Christian Brabandt writes: >> And here is the second one. > > Wow, great and so fast! I really apologize it. No need to apologize, but appreciating would not hurt ;-) Thanks for an interesting idea. I spotted a buglet in 1/2 so I'll queue a fixed version on 'pu' when I push today's intergration result out. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] submodule documentation: Reorder introductory paragraphs
On Mon, May 25, 2015 at 3:00 PM, Junio C Hamano wrote: > Stefan Beller writes: > >> DESCRIPTION >> --- >> +This command will inspect, update and manage submodules. >> >> +Submodules allow you to keep another Git repository in a subdirectory >> +of your repository. The other repository has its own history,... > > The first line somehow bothered me, so I took a random sample of > commands I often use: > > git log >Shows the commit logs. > > git show >Shows one or more objects (blobs, trees, tags and commits). > > git commit >Stores the current contents of the index in a new commit along with a >log message from the user describing the changes. > > git diff >Show changes between the working tree and the index or a tree, changes >between the index and a tree, changes between two trees, changes >between two blob objects, or changes between two files on disk. > > git push >Updates remote refs using local refs, while sending objects necessary >to complete the given refs. > > I _think_ what bothered me was "This command" (drawing the reaction > "eh, what other command are you going to talk about in the help page > for this command?"). Perhaps > > Inspects, updates and manages submodules. > > may match the style of other help pages better. Sounds much better than my patch. > > On the other hand, I probably would not have felt such a strong > "strangeness" if it were described like this: > > This command can help you inspect, update, and manage > submodules. > > I haven't analized it enough to say why it is, but I suspect it has > something to do with (my own) perception that "git submodule" is not > very essential to do any of these things (i.e. .gitmodules is a very > simple text file), but is primarily a helpful wrapper. My perception is that the submodule man page similar to the subtree man page tries to explain an underlying concept as well. The other man pages you quoted don't do that as the concepts are explained elsewhere(?) As a side note: In the Gerrit test suite I use the JGit implementation of the config command to write out .gitmodules files. So maybe `git submodule` can be understood as a specialized form of `git config`. > > The description of "git config", on which I have a similar > perception, seem to match ;-) > > git config >You can query/set/replace/unset options with this command. > -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Implementation of git rebase --status
Junio C Hamano writes: >Guillaume Pages writes: >> Do you think it could be useful or do you have any suggestion? >All of your examples say things like: >> You are in the middle of a rebase session. >> The line that paused this session is: >but what if there is no such "line"? >IOW, what does the user see when using this new option during a "git >rebase" (not "git rebase -i")? I guess it should display the sha1 of the patch that failed in this case. >Other than that, sounds like a neat thing to do. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Mark trailing whitespace error in del lines of diff
Hi Junio! On Di, 26 Mai 2015, Junio C Hamano wrote: > No need to apologize, but appreciating would not hurt ;-) Right. Thanks. Best, Christian -- Trägt der Bauer rote Socken, will er seinen Bullen schocken. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v11 2/5] command-list.txt: add the common groups block
[Re-sending this for on-list completeness. It was sent off-list earlier when I was using an email client capable only of HTML messages.] On Mon, May 25, 2015 at 1:31 PM, Sébastien Guimmara wrote: > On 05/21/2015 08:01 PM, Eric Sunshine wrote: >> On Thu, May 21, 2015 at 1:39 PM, Sébastien Guimmara >> wrote: >>> >>> The ultimate goal is for "git help" to display common commands in >>> groups rather than alphabetically. As a first step, define the >>> groups in a new block, and then assign a group to each >>> common command. >>> >>> Signed-off-by: Sébastien Guimmara >>> --- >>> diff --git a/command-list.txt b/command-list.txt >>> index 181a9c2..32ddab3 100644 >>> --- a/command-list.txt >>> +++ b/command-list.txt >>> @@ -1,3 +1,14 @@ >>> +# common commands are grouped by themes >>> +# these groups are output by 'git help' in the order declared here. >>> +# map each common command in the command list to one of these groups. >>> +### common groups (do not change this line) >>> +init start a working area (see also: git help tutorial) >>> +worktree work on the current change (see also: git help everyday) >>> +info examine the history and state (see also: git help >>> revisions) >>> +history grow, mark and tweak your common history >>> +remote collaborate (see also: git help workflows) >>> + >>> +# List of known git commands. >> >> This is odd. The above line was removed in 1/5 but then re-appears >> here in 2/5. I think the intent is that it should remain removed. >> >>> ### command list (do not change this line) >>> # command name category [deprecated] [common] >>> git-add mainporcelain common > > My mistake. This will be corrected in the next version. Thank you for taking > time to review this series. Junio already made these corrections locally when he picked up the series. Take a look at his 'pu' branch, and you'll find the series there with the corrections[1]. Thus, no need to re-send. [1]: Series currently merged into 'pu' at de905cf0. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 1/1] send-email: Add sendmail email aliases format
On Sat, May 23, 2015 at 1:45 PM, Junio C Hamano wrote: > Allen Hubbe writes: >> Note that this only adds support for a limited subset of the sendmail >> format. The format is is as follows. >> >> : [, ...] >> >> Aliases are specified one per line, and must start on the first column of the >> line. Blank lines are ignored. If the first non whitespace character >> on a line is a '#' symbol, then the whole line is considered a comment, >> and is ignored. >> [...] >> Signed-off-by: Allen Hubbe >> --- > > Thanks. > > A small thing I noticed in the test (and this patch is not adding a > new "breakage"---there are a few existing instances) is the use of > "~/"; it should be spelled "$HOME/" instead for portability (not in > POSIX, even though bash, dash and ksh all seem to understand it). > > I think this round looks good from a cursory read. > > Eric, what do you think? Sorry for the delay. This round looks much better. I do have a few very minor comments, which I'll post in reply to the patch itself, but nothing worth holding the series up. Thanks. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 1/1] send-email: Add sendmail email aliases format
On Saturday, May 23, 2015, Allen Hubbe wrote:
> Note that this only adds support for a limited subset of the sendmail
> format. The format is is as follows.
>
> : [, ...]
>
> Aliases are specified one per line, and must start on the first column of the
> line. Blank lines are ignored. If the first non whitespace character
> on a line is a '#' symbol, then the whole line is considered a comment,
> and is ignored.
> [...]
> Signed-off-by: Allen Hubbe
> ---
>
> Notes:
> This v5 renames the parser 'sendmail' again, from 'simple'.
> Therefore, the subject line is changed again, too.
>
> Previous subject line: send-email: Add simple email aliases format
>
> The format is restricted to a subset of sendmail. When the subset
> diverges from sendmail, the parser warns about the line that diverges,
> and ignores the line. The supported format is described in the
> documentation, as well as the behavior when an unsupported format
> construct is detected.
>
> A badly constructed sentence was corrected in the documentation.
>
> The test case was changed to use a here document, and the unsupported
> comment after an alias was removed from the test case alias file input.
Thanks. This round looks much nicer. A few minor comments below...
> diff --git a/git-send-email.perl b/git-send-email.perl
> index e1e9b1460ced..ffea50094a48 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -487,6 +487,8 @@ sub split_addrs {
> }
>
> my %aliases;
> +
> +
Unnecessary whitespace change sneaked in.
> my %parse_alias = (
> # multiline formats can be supported in the future
> mutt => sub { my $fh = shift; while (<$fh>) {
> @@ -516,6 +518,33 @@ my %parse_alias = (
> }
> } },
>
> + sendmail => sub { my $fh = shift; while (<$fh>) {
> + # ignore comment lines
> + if (/^\s*(?:#.*)?$/) { }
This confused me at first because the comment talks only about
"comment lines", for which a simpler /^\s*#/ would suffice. The regex,
however, actually matches blank lines and comment lines (both of which
get skipped). Either the comment should be fixed or the regex could be
split into two much simpler ones. The splitting into simpler regex's
has the benefit of being easier to comprehend at a glance. For
instance:
next if /^\s*$/;
next if /^\s*#/;
Speaking of 'next', its use here is inconsistent. Due to use of the
if/elsif/else chain, 'next' is not needed at all, yet it is used for
some cases but not others. To be consistent, either use it everywhere
or nowhere.
> + # warn on lines that contain quotes
> + elsif (/"/) {
> + print STDERR "sendmail alias with quotes is not
> supported: $_\n";
> + next;
> + }
> +
> + # warn on lines that continue
> + elsif (/^\s|\\$/) {
> + print STDERR "sendmail continuation line is not
> supported: $_\n";
> + next;
> + }
> +
> + # recognize lines that look like an alias
> + elsif (/^(\S+)\s*:\s*(.+?)$/) {
Observation: Given "foo:bar:baz", this regex will take "foo:bar" as
the key, and "baz" as the value, which is probably not what was
intended, however, it likely doesn't matter much in this case since
colon isn't legal in an email address[1].
[1]: However, I could have sworn that colon was legal in some type of
email address years ago, but I can no longer remember which type it
was. UUCP used '!' in email addresses, so that wasn't it.
> + my ($alias, $addr) = ($1, $2);
> + $aliases{$alias} = [ split_addrs($addr) ];
> + }
> +
> + # warn on lines that are not recognized
> + else {
> + print STDERR "sendmail line is not recognized: $_\n";
> + }}},
> +
> gnus => sub { my $fh = shift; while (<$fh>) {
> if (/\(define-mail-alias\s+"(\S+?)"\s+"(\S+?)"\)/) {
> $aliases{$1} = [ $2 ];
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v11 2/5] command-list.txt: add the common groups block
Eric Sunshine writes: +history grow, mark and tweak your common history +remote collaborate (see also: git help workflows) + +# List of known git commands. >>> >>> This is odd. The above line was removed in 1/5 but then re-appears >>> here in 2/5. I think the intent is that it should remain removed. >>> ### command list (do not change this line) # command name category [deprecated] [common] git-add mainporcelain common >> >> My mistake. This will be corrected in the next version. Thank you for taking >> time to review this series. > > Junio already made these corrections locally when he picked up the > series. Take a look at his 'pu' branch, and you'll find the series > there with the corrections[1]. Thus, no need to re-send. > > [1]: Series currently merged into 'pu' at de905cf0. Yeah, resurrecting "List of known git commands." does look somewhat strange, but looking at what this step does, especially this bit: > diff --git a/command-list.txt b/command-list.txt > index 181a9c2..32ddab3 100644 > --- a/command-list.txt > +++ b/command-list.txt > @@ -1,3 +1,14 @@ > +# common commands are grouped by themes > +# these groups are output by 'git help' in the order declared here. > +# map each common command in the command list to one of these groups. > +### common groups (do not change this line) > +init start a working area (see also: git help tutorial) > +worktree work on the current change (see also: git help everyday) I do not think we would terribly mind an introductory comment that applies to the next "###" block before it, e.g. # list of known git commands; ordered alphabetically # for easy spotting ### command list (do not change this line) For some reason the patch seems to want to spell that comment in all lowercase, so I just imitated it here. In any case, if somebody wants to add such a comment there for symmetry, that can be done as a follow-up patch after dust from these patches settles, I think. Let's have these 5 patches graduate to 'next' without further bikeshedding ;-) -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 1/1] send-email: Add sendmail email aliases format
On Tue, May 26, 2015 at 3:10 PM, Eric Sunshine wrote:
> On Saturday, May 23, 2015, Allen Hubbe wrote:
>> Note that this only adds support for a limited subset of the sendmail
>> format. The format is is as follows.
>>
>> : [, ...]
>>
>> Aliases are specified one per line, and must start on the first column of the
>> line. Blank lines are ignored. If the first non whitespace character
>> on a line is a '#' symbol, then the whole line is considered a comment,
>> and is ignored.
>> [...]
>> Signed-off-by: Allen Hubbe
>> ---
>>
>> Notes:
>> This v5 renames the parser 'sendmail' again, from 'simple'.
>> Therefore, the subject line is changed again, too.
>>
>> Previous subject line: send-email: Add simple email aliases format
>>
>> The format is restricted to a subset of sendmail. When the subset
>> diverges from sendmail, the parser warns about the line that diverges,
>> and ignores the line. The supported format is described in the
>> documentation, as well as the behavior when an unsupported format
>> construct is detected.
>>
>> A badly constructed sentence was corrected in the documentation.
>>
>> The test case was changed to use a here document, and the unsupported
>> comment after an alias was removed from the test case alias file input.
>
> Thanks. This round looks much nicer. A few minor comments below...
>
>> diff --git a/git-send-email.perl b/git-send-email.perl
>> index e1e9b1460ced..ffea50094a48 100755
>> --- a/git-send-email.perl
>> +++ b/git-send-email.perl
>> @@ -487,6 +487,8 @@ sub split_addrs {
>> }
>>
>> my %aliases;
>> +
>> +
>
> Unnecessary whitespace change sneaked in.
>
>> my %parse_alias = (
>> # multiline formats can be supported in the future
>> mutt => sub { my $fh = shift; while (<$fh>) {
>> @@ -516,6 +518,33 @@ my %parse_alias = (
>> }
>> } },
>>
>> + sendmail => sub { my $fh = shift; while (<$fh>) {
>> + # ignore comment lines
>> + if (/^\s*(?:#.*)?$/) { }
>
> This confused me at first because the comment talks only about
> "comment lines", for which a simpler /^\s*#/ would suffice. The regex,
> however, actually matches blank lines and comment lines (both of which
> get skipped). Either the comment should be fixed or the regex could be
> split into two much simpler ones. The splitting into simpler regex's
> has the benefit of being easier to comprehend at a glance. For
> instance:
>
> next if /^\s*$/;
> next if /^\s*#/;
I noticed this too after sending the patch, and I have already changed
the comment to mention blank lines or comment lines.
Splitting the regex would be more simple, but the regex is already
quite simple as it is.
>
> Speaking of 'next', its use here is inconsistent. Due to use of the
> if/elsif/else chain, 'next' is not needed at all, yet it is used for
> some cases but not others. To be consistent, either use it everywhere
> or nowhere.
These used to be `if (foo) { somthing; next; }` while this version was
work in progress, which I changed to elsif with the intention of
removing the next. Thanks for catching the inconsistency. I will
remove the next.
>
>> + # warn on lines that contain quotes
>> + elsif (/"/) {
>> + print STDERR "sendmail alias with quotes is not
>> supported: $_\n";
>> + next;
>> + }
>> +
>> + # warn on lines that continue
>> + elsif (/^\s|\\$/) {
>> + print STDERR "sendmail continuation line is not
>> supported: $_\n";
>> + next;
>> + }
>> +
>> + # recognize lines that look like an alias
>> + elsif (/^(\S+)\s*:\s*(.+?)$/) {
>
> Observation: Given "foo:bar:baz", this regex will take "foo:bar" as
> the key, and "baz" as the value, which is probably not what was
> intended, however, it likely doesn't matter much in this case since
> colon isn't legal in an email address[1].
That's a keen observation. I think it would work simply to use a
non-greedy +? in the first capture group.
>
> [1]: However, I could have sworn that colon was legal in some type of
> email address years ago, but I can no longer remember which type it
> was. UUCP used '!' in email addresses, so that wasn't it.
>
>> + my ($alias, $addr) = ($1, $2);
>> + $aliases{$alias} = [ split_addrs($addr) ];
>> + }
>> +
>> + # warn on lines that are not recognized
>> + else {
>> + print STDERR "sendmail line is not recognized: $_\n";
>> + }}},
>> +
>> gnus => sub { my $fh = shift; while (<$fh>) {
>> if (/\(define-mail-alias\s+"(\S+?)"\s+"(\S+?)"\)/) {
>> $aliases{$1} = [ $2 ];
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body
[PATCH v2 4/4] diff.c: --ws-check-deleted option
Traditionally, we only cared about whitespace breakages introduced
in new lines. Some people want to paint whitespace breakages on old
lines, too. When they see a whitespace breakage on a new line, they
can spot the same kind of whitespace breakage on the corresponding
old line and want to say "Ah, those breakages are there but they
were inherited from the original, so let's not touch them for now."
Enable such use case with the new option, "--ws-check-deleted".
Signed-off-by: Junio C Hamano
---
Documentation/diff-options.txt | 7 +
diff.c | 21 +-
diff.h | 1 +
t/t4015-diff-whitespace.sh | 62 ++
4 files changed, 90 insertions(+), 1 deletion(-)
diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index 6cb083a..701c087 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt
@@ -278,6 +278,13 @@ ifndef::git-format-patch[]
initial indent of the line are considered whitespace errors.
Exits with non-zero status if problems are found. Not compatible
with --exit-code.
+
+--ws-check-deleted::
+--no-ws-check-deleted::
+ Highlight whitespace errors in deleted lines in the color
+ specified by `color.diff.whitespace`, as well as in added
+ lines.
+
endif::git-format-patch[]
--full-index::
diff --git a/diff.c b/diff.c
index 75b1342..30eeaea 100644
--- a/diff.c
+++ b/diff.c
@@ -503,8 +503,22 @@ static void emit_del_line(const char *reset,
const char *line, int len)
{
const char *set = diff_get_color(ecbdata->color_diff, DIFF_FILE_OLD);
+ const char *ws = NULL;
- emit_line_0(ecbdata->opt, set, reset, '-', line, len);
+ if (ecbdata->opt->ws_check_deleted) {
+ ws = diff_get_color(ecbdata->color_diff, DIFF_WHITESPACE);
+ if (!*ws)
+ ws = NULL;
+ }
+
+ if (!ws)
+ emit_line_0(ecbdata->opt, set, reset, '-', line, len);
+ else {
+ /* Emit just the prefix, then the rest. */
+ emit_line_0(ecbdata->opt, set, reset, '-', "", 0);
+ ws_check_emit(line, len, ecbdata->ws_rule,
+ ecbdata->opt->file, set, reset, ws);
+ }
}
static void emit_hunk_header(struct emit_callback *ecbdata,
@@ -3823,6 +3837,11 @@ int diff_opt_parse(struct diff_options *options, const
char **av, int ac)
else if (skip_prefix(arg, "--submodule=", &arg))
return parse_submodule_opt(options, arg);
+ else if (!strcmp(arg, "--ws-check-deleted"))
+ options->ws_check_deleted = 1;
+ else if (!strcmp(arg, "--no-ws-check-deleted"))
+ options->ws_check_deleted = 0;
+
/* misc options */
else if (!strcmp(arg, "-z"))
options->line_termination = 0;
diff --git a/diff.h b/diff.h
index b4a624d..ba6cf1a 100644
--- a/diff.h
+++ b/diff.h
@@ -137,6 +137,7 @@ struct diff_options {
int dirstat_permille;
int setup;
int abbrev;
+ int ws_check_deleted;
const char *prefix;
int prefix_length;
const char *stat_sep;
diff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh
index 4da30e5..8f35475 100755
--- a/t/t4015-diff-whitespace.sh
+++ b/t/t4015-diff-whitespace.sh
@@ -838,4 +838,66 @@ test_expect_success 'diff that introduces a line with only
tabs' '
test_cmp expected current
'
+test_expect_success 'diff that introduces and removes ws breakages' '
+ git reset --hard &&
+ {
+ echo "0. blank-at-eol " &&
+ echo "1. blank-at-eol "
+ } >x &&
+ git commit -a --allow-empty -m preimage &&
+ {
+ echo "0. blank-at-eol " &&
+ echo "1. still-blank-at-eol " &&
+ echo "2. and a new line "
+ } >x &&
+
+ git -c color.diff=always diff |
+ test_decode_color >current &&
+
+ cat >expected <<-\EOF &&
+ diff --git a/x b/x
+ index d0233a2..700886e 100644
+ --- a/x
+ +++ b/x
+ @@ -1,2 +1,3 @@
+0. blank-at-eol
+ -1. blank-at-eol
+ +1. still-blank-at-eol
+ +2. and a new line
+ EOF
+
+ test_cmp expected current
+'
+
+test_expect_success 'the same with --ws-check-deleted' '
+ git reset --hard &&
+ {
+ echo "0. blank-at-eol " &&
+ echo "1. blank-at-eol "
+ } >x &&
+ git commit -a --allow-empty -m preimage &&
+ {
+ echo "0. blank-at-eol " &&
+ echo "1. still-blank-at-eol " &&
+ echo "2. and a new line "
+ } >x &&
+
+ git -c color.diff=always diff --ws-check-deleted |
+ test_decode_color >current &&
+
+ cat >expected <<-\EOF &&
+ diff --git a/x b/x
+ index d0233a2..700886e 100644
+ --- a/x
+
[PATCH v2 3/4] diff.c: add emit_del_line() and update callers of emit_line_0()
Traditionally, we only had emit_add_line() helper, which knows how
to find and paint whitespace breakages on the given line, because we
only care about whitespace breakages introduced in new lines. The
context lines and old (i.e. deleted) lines are emitted with a
simpler emit_line_0() that paints the entire line in plain or old
colors.
Identify callers of emit_line_0() that show deleted lines and
have them call a new helper, emit_del_line(), so that we can later
tweak what is done to deleted lines.
Signed-off-by: Junio C Hamano
---
diff.c | 39 +++
1 file changed, 27 insertions(+), 12 deletions(-)
diff --git a/diff.c b/diff.c
index d1bd534..75b1342 100644
--- a/diff.c
+++ b/diff.c
@@ -498,6 +498,15 @@ static void emit_add_line(const char *reset,
}
}
+static void emit_del_line(const char *reset,
+ struct emit_callback *ecbdata,
+ const char *line, int len)
+{
+ const char *set = diff_get_color(ecbdata->color_diff, DIFF_FILE_OLD);
+
+ emit_line_0(ecbdata->opt, set, reset, '-', line, len);
+}
+
static void emit_hunk_header(struct emit_callback *ecbdata,
const char *line, int len)
{
@@ -603,7 +612,6 @@ static void emit_rewrite_lines(struct emit_callback *ecb,
{
const char *endp = NULL;
static const char *nneof = " No newline at end of file\n";
- const char *old = diff_get_color(ecb->color_diff, DIFF_FILE_OLD);
const char *reset = diff_get_color(ecb->color_diff, DIFF_RESET);
while (0 < size) {
@@ -613,8 +621,7 @@ static void emit_rewrite_lines(struct emit_callback *ecb,
len = endp ? (endp - data + 1) : size;
if (prefix != '+') {
ecb->lno_in_preimage++;
- emit_line_0(ecb->opt, old, reset, '-',
- data, len);
+ emit_del_line(reset, ecb, data, len);
} else {
ecb->lno_in_postimage++;
emit_add_line(reset, ecb, data, len);
@@ -1250,17 +1257,25 @@ static void fn_out_consume(void *priv, char *line,
unsigned long len)
return;
}
- if (line[0] != '+') {
- const char *color =
- diff_get_color(ecbdata->color_diff,
- line[0] == '-' ? DIFF_FILE_OLD :
DIFF_PLAIN);
- ecbdata->lno_in_preimage++;
- if (line[0] == ' ')
- ecbdata->lno_in_postimage++;
- emit_line(ecbdata->opt, color, reset, line, len);
- } else {
+ switch (line[0]) {
+ case '+':
ecbdata->lno_in_postimage++;
emit_add_line(reset, ecbdata, line + 1, len - 1);
+ break;
+ case '-':
+ ecbdata->lno_in_preimage++;
+ emit_del_line(reset, ecbdata, line + 1, len - 1);
+ break;
+ case ' ':
+ ecbdata->lno_in_postimage++;
+ /* fallthru */
+ default:
+ /* ' ' and incomplete line */
+ ecbdata->lno_in_preimage++;
+ emit_line(ecbdata->opt,
+ diff_get_color(ecbdata->color_diff, DIFF_PLAIN),
+ reset, line, len);
+ break;
}
}
--
2.4.1-511-gc1146d5
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 2/4] t4015: separate common setup and per-test expectation
The last two tests in the script were to
- set up color.diff.* slots
- set up an expectation for a single test
- run that test and check the result
but split in a wrong way. It did the first two in the first test
and the third one in the second test. The latter two belong to each
other. This matters when you plan to add more of these tests that
share the common coloring.
While at it, make sure we use a color different from old, which is
also red.
Signed-off-by: Junio C Hamano
---
t/t4015-diff-whitespace.sh | 25 +
1 file changed, 13 insertions(+), 12 deletions(-)
diff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh
index 0bfc7ff..4da30e5 100755
--- a/t/t4015-diff-whitespace.sh
+++ b/t/t4015-diff-whitespace.sh
@@ -810,11 +810,20 @@ test_expect_success 'setup diff colors' '
git config color.diff.old red &&
git config color.diff.new green &&
git config color.diff.commit yellow &&
- git config color.diff.whitespace "normal red" &&
+ git config color.diff.whitespace blue &&
- git config core.autocrlf false &&
+ git config core.autocrlf false
+'
+
+test_expect_success 'diff that introduces a line with only tabs' '
+ git config core.whitespace blank-at-eol &&
+ git reset --hard &&
+ echo "test" >x &&
+ git commit -m "initial" x &&
+ echo "{NTN}" | tr "NT" "\n\t" >>x &&
+ git -c color.diff=always diff | test_decode_color >current &&
- cat >expected <<-\EOF
+ cat >expected <<-\EOF &&
diff --git a/x b/x
index 9daeafb..2874b91 100644
--- a/x
@@ -822,18 +831,10 @@ test_expect_success 'setup diff colors' '
@@ -1 +1,4 @@
test
+{
- +
+ +
+}
EOF
-'
-test_expect_success 'diff that introduces a line with only tabs' '
- git config core.whitespace blank-at-eol &&
- git reset --hard &&
- echo "test" >x &&
- git commit -m "initial" x &&
- echo "{NTN}" | tr "NT" "\n\t" >>x &&
- git -c color.diff=always diff | test_decode_color >current &&
test_cmp expected current
'
--
2.4.1-511-gc1146d5
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 1/4] t4015: modernise style
Move the preparatory steps that create the expected output inside
the test bodies, remove unnecessary blank lines before and after the
test bodies, and drop SP between redirection operator and its target.
Signed-off-by: Junio C Hamano
---
t/t4015-diff-whitespace.sh | 411 +++--
1 file changed, 173 insertions(+), 238 deletions(-)
diff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh
index 604a838..0bfc7ff 100755
--- a/t/t4015-diff-whitespace.sh
+++ b/t/t4015-diff-whitespace.sh
@@ -9,138 +9,144 @@ test_description='Test special whitespace in diff engine.
. ./test-lib.sh
. "$TEST_DIRECTORY"/diff-lib.sh
-# Ray Lehtiniemi's example
+test_expect_success "Ray Lehtiniemi's example" '
+ cat <<-\EOF >x &&
+ do {
+ nothing;
+ } while (0);
+ EOF
+ git update-index --add x &&
-cat << EOF > x
-do {
- nothing;
-} while (0);
-EOF
+ cat <<-\EOF >x &&
+ do
+ {
+ nothing;
+ }
+ while (0);
+ EOF
-git update-index --add x
+ cat <<-\EOF >expect &&
+ diff --git a/x b/x
+ index adf3937..6edc172 100644
+ --- a/x
+ +++ b/x
+ @@ -1,3 +1,5 @@
+ -do {
+ +do
+ +{
+ nothing;
+ -} while (0);
+ +}
+ +while (0);
+ EOF
-cat << EOF > x
-do
-{
- nothing;
-}
-while (0);
-EOF
+ git diff >out &&
+ test_cmp expect out &&
-cat << EOF > expect
-diff --git a/x b/x
-index adf3937..6edc172 100644
a/x
-+++ b/x
-@@ -1,3 +1,5 @@
--do {
-+do
-+{
-nothing;
--} while (0);
-+}
-+while (0);
-EOF
+ git diff -w >out &&
+ test_cmp expect out &&
-git diff > out
-test_expect_success "Ray's example without options" 'test_cmp expect out'
+ git diff -b >out &&
+ test_cmp expect out
+'
-git diff -w > out
-test_expect_success "Ray's example with -w" 'test_cmp expect out'
+test_expect_success 'another test, without options' '
+ tr Q "\015" <<-\EOF >x &&
+ whitespace at beginning
+ whitespace change
+ whitespace in the middle
+ whitespace at end
+ unchanged line
+ CR at endQ
+ EOF
-git diff -b > out
-test_expect_success "Ray's example with -b" 'test_cmp expect out'
+ git update-index x &&
-tr 'Q' '\015' << EOF > x
-whitespace at beginning
-whitespace change
-whitespace in the middle
-whitespace at end
-unchanged line
-CR at endQ
-EOF
+ tr "_" " " <<-\EOF >x &&
+ _ whitespace at beginning
+ whitespace change
+ white space in the middle
+ whitespace at end__
+ unchanged line
+ CR at end
+ EOF
-git update-index x
+ tr "Q_" "\015 " <<-\EOF >expect &&
+ diff --git a/x b/x
+ index d99af23..22d9f73 100644
+ --- a/x
+ +++ b/x
+ @@ -1,6 +1,6 @@
+ -whitespace at beginning
+ -whitespace change
+ -whitespace in the middle
+ -whitespace at end
+ + whitespace at beginning
+ +whitespace change
+ +white space in the middle
+ +whitespace at end__
+unchanged line
+ -CR at endQ
+ +CR at end
+ EOF
-tr '_' ' ' << EOF > x
- whitespace at beginning
-whitespace change
-white space in the middle
-whitespace at end__
-unchanged line
-CR at end
-EOF
+ git diff >out &&
+ test_cmp expect out &&
-tr 'Q_' '\015 ' << EOF > expect
-diff --git a/x b/x
-index d99af23..8b32fb5 100644
a/x
-+++ b/x
-@@ -1,6 +1,6 @@
--whitespace at beginning
--whitespace change
--whitespace in the middle
--whitespace at end
-+ whitespace at beginning
-+whitespace change
-+white space in the middle
-+whitespace at end__
- unchanged line
--CR at endQ
-+CR at end
-EOF
-git diff > out
-test_expect_success 'another test, without options' 'test_cmp expect out'
+ >expect &&
+ git diff -w >out &&
+ test_cmp expect out &&
+
+ git diff -w -b >out &&
+ test_cmp expect out &&
+
+ git diff -w --ignore-space-at-eol >out &&
+ test_cmp expect out &&
+
+ git diff -w -b --ignore-space-at-eol >out &&
+ test_cmp expect out &&
-cat << EOF > expect
-EOF
-git diff -w > out
-test_expect_success 'another test, with -w' 'test_cmp expect out'
-git diff -w -b > out
-test_expect_success 'another test, with -w -b' 'test_cmp expect out'
-git diff -w --ignore-space-at-eol > out
-test_expect_success 'another test, with -w --ignore-space-at-eol' 'test_cmp
expect out'
-git diff -w -b --ignore-space-at-eol > out
-test_expect_success 'another test, with -w -b --ignore-space-at-eol' 'test_cmp
expect out'
-
-tr 'Q_' '\015 ' << EOF > expect
-diff --git a/x b/x
-index d99af23..8b32fb5 100644
a/x
-+++ b/x
-@@ -1,6 +1,6 @@
--whitespace at beginning
-+ whitespace at beginning
- whitespace change
--whitespace in the middle
-+white space in the middle
- whitespace at end__
- unchanged line
- CR at end
-EOF
-git diff -b > out
-test_ex
[PATCH v2 0/5] showing existing ws breakage
We paint whitespace breakages in new (i.e. added or updated) lines when showing the "git diff" output to help people avoid introducing them with their changes. The basic premise is that people would want to avoid touching existing lines only to fix whitespace errors in a patch that does other changes of substance, and that is why we traditionally did not paint whitespace breakages in existing (i.e. deleted or context) lines. However, some people would want to keep existing breakages when they are doing other changes of substance; "new" lines in such a patch would show existing whitespace breakages painted, and it is not apparent if the breakages were inherited from the original or newly introduced. Christian Brabandt had an interesting idea to help users in this situation; why not give them a mode to paint whitespace breakages in "old" (i.e. deleted or was replaced) lines, too? http://thread.gmane.org/gmane.comp.version-control.git/269912/focus=269956 This series builds on that idea but with a different implementation (Christian's original painted trailing whitespaces only). The first three patches are preliminary cleanups. The last one is the interesting bit. Having done this series, I am starting to suspect that painting ws breakages only in deleted lines may not be such a useful thing to do. In order to decide if fixing ws breakages "while at it" is more appropriate, you would need to know if such breakages are prevalent in the original. After all, the line you are updating might be one of only few lines that the original had breakages, in which case you may want to go back to your editor and fix them all while you are at it, or fix only these few ws breakages as a preliminary step. In order to help users do that, the new option may be better not to limit itself to "deleted" lines, but "context and deleted", i.e. "preimage" lines. Junio C Hamano (4): t4015: modernise style t4015: separate common setup and per-test expectation diff.c: add emit_del_line() and update callers of emit_line_0() diff.c: --ws-check-deleted option Documentation/diff-options.txt | 7 + diff.c | 58 +++-- diff.h | 1 + t/t4015-diff-whitespace.sh | 474 - 4 files changed, 290 insertions(+), 250 deletions(-) -- 2.4.1-511-gc1146d5 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] git-p4: fix handling of multi-word P4EDITOR
Luke Diamand writes: > On 07/05/15 23:16, Junio C Hamano wrote: >> Luke Diamand writes: >> > > [Resurrecting old thread] > ... > > To me, that seems to imply that for GIT_WINDOWS_NATIVE, we take the > *second* branch and use "sh", so again, the the code as it stands will > be fine. msysgit uses that path. > ... > > I don't think we need to do anything. msysgit works fine with the > origin "sh", "-c", ... code. Thanks. Let's merge what is in 'pu' down to 'next' then. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 1/1] send-email: Add sendmail email aliases format
On Tue, May 26, 2015 at 3:41 PM, Allen Hubbe wrote:
> On Tue, May 26, 2015 at 3:10 PM, Eric Sunshine
> wrote:
>> On Saturday, May 23, 2015, Allen Hubbe wrote:
>>> diff --git a/git-send-email.perl b/git-send-email.perl
>>> index e1e9b1460ced..ffea50094a48 100755
>>> --- a/git-send-email.perl
>>> +++ b/git-send-email.perl
>>> @@ -516,6 +518,33 @@ my %parse_alias = (
>>> }
>>> } },
>>>
>>> + sendmail => sub { my $fh = shift; while (<$fh>) {
>>> + # ignore comment lines
>>> + if (/^\s*(?:#.*)?$/) { }
>>
>> This confused me at first because the comment talks only about
>> "comment lines", for which a simpler /^\s*#/ would suffice. The regex,
>> however, actually matches blank lines and comment lines (both of which
>> get skipped). Either the comment should be fixed or the regex could be
>> split into two much simpler ones. The splitting into simpler regex's
>> has the benefit of being easier to comprehend at a glance. For
>> instance:
>>
>> next if /^\s*$/;
>> next if /^\s*#/;
>
> I noticed this too after sending the patch, and I have already changed
> the comment to mention blank lines or comment lines.
>
> Splitting the regex would be more simple, but the regex is already
> quite simple as it is.
To be clear, the reason that I brought up the idea of splitting the
regex was that /^\s*$/ and /^\s*#/ are very common idioms which people
can and do recognize and understand at-a-glance without having to
spend time deciphering them. On the other hand, /^\s*(?:#.*)?$/
doesn't lend itself to that sort of instant comprehension; it requires
a certain amount of mental effort to understand.
Anyhow, it's just an idea put forth in case you or someone favors it;
not an outright request for a change.
>>> + # recognize lines that look like an alias
>>> + elsif (/^(\S+)\s*:\s*(.+?)$/) {
>>
>> Observation: Given "foo:bar:baz", this regex will take "foo:bar" as
>> the key, and "baz" as the value, which is probably not what was
>> intended, however, it likely doesn't matter much in this case since
>> colon isn't legal in an email address[1].
>
> That's a keen observation. I think it would work simply to use a
> non-greedy +? in the first capture group.
Yes, that would work. Alternately: /^([^\s:]+)\s*:\s*(.+?)$/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Staging commits with visual diff tools?
Hi Does anybody have code to stage commits using a the visual diff/merge tools supported by git-difftool? Is there support in git itself somewhere, even? I'm looking for something functionally similar to git add -p Looking at the git-difftool source I can see how to write a command to do it, but wanted to check if it had already been implemented. Did I miss a way that already exists? Thanks John -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 1/1] send-email: Add sendmail email aliases format
On Tue, May 26, 2015 at 4:53 PM, Eric Sunshine wrote:
> On Tue, May 26, 2015 at 3:41 PM, Allen Hubbe wrote:
>> On Tue, May 26, 2015 at 3:10 PM, Eric Sunshine
>> wrote:
>>> On Saturday, May 23, 2015, Allen Hubbe wrote:
+ # recognize lines that look like an alias
+ elsif (/^(\S+)\s*:\s*(.+?)$/) {
>>>
>>> Observation: Given "foo:bar:baz", this regex will take "foo:bar" as
>>> the key, and "baz" as the value, which is probably not what was
>>> intended, however, it likely doesn't matter much in this case since
>>> colon isn't legal in an email address[1].
>>
>> That's a keen observation. I think it would work simply to use a
>> non-greedy +? in the first capture group.
>
> Yes, that would work. Alternately: /^([^\s:]+)\s*:\s*(.+?)$/
I will use the non-greedy +? because the resulting expression is easier to read.
I will remove the non-greedy +? from the second capture group. It
serves no purpose there any more. It had been there to allow matching
a trailing backslash after the group, but now lines with trailing
backslash are ignored entirely before reaching here.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v6] send-email: Add sendmail email aliases format
Add support for the sendmail email aliases format.
Synopsis:
: [, ...]
Example:
alice: Alice W Land
bob: Robert Bobbyton
# this is a comment
# this is also a comment
chloe: ch...@example.com
abgroup: alice, bob
bcgrp: bob, chloe, Other
Quoted aliases and quoted addresses are not supported.
Line continuations are not supported.
Warnings are printed for explicitly unsupported constructs, and any
other lines that are not matched by the parser.
Signed-off-by: Allen Hubbe
---
Notes:
This v6 makes the following changes from v5:
* In the documentation:
** Move 'sendmail' to the end of the list of formats.
** Remove the description, synopsis, and example of sendmail aliases.
** Specify exceptions to the sendmail format as a sub-definition.
** Note: A general 'where to find documentation' paragraph will be added
by Junio, appearing either before or after this patch in the series.
* Changes to the parser:
** Reword a comment to mention blank lines and comment lines.
** Resolve inconsistent use of the keyword `next` by not using it.
** Use non-greedy quantifier in the capture group for the alias name.
** Use greedy quantifier in the capture group for email addresses.
* Changes to the test case:
** Test alias input is written to the current dir, not the home dir.
** Note: A fix to other tests to eliminate the use of tilde for the home
dir will be added by Junio, appearing either before or after this
patch in the series.
Documentation/git-send-email.txt | 13 -
git-send-email.perl | 25 +
t/t9001-send-email.sh| 27 +++
3 files changed, 64 insertions(+), 1 deletion(-)
diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt
index 804554609def..36fd0b86353c 100644
--- a/Documentation/git-send-email.txt
+++ b/Documentation/git-send-email.txt
@@ -383,7 +383,18 @@ sendemail.aliasesFile::
sendemail.aliasFileType::
Format of the file(s) specified in sendemail.aliasesFile. Must be
- one of 'mutt', 'mailrc', 'pine', 'elm', or 'gnus'.
+ one of 'mutt', 'mailrc', 'pine', 'elm', or 'gnus', or 'sendmail'.
++
+--
+sendmail;;
+* Quoted aliases and quoted addresses are not supported: lines that
+ contain a `"` symbol are ignored.
+* Line continuations are not supported: lines that start with
+ whitespace characters, or end with a `\` symbol are ignored.
+* Warnings are printed on the standard error output for any
+ explicitly unsupported constructs, and any other lines that are not
+ recognized by the parser.
+--
sendemail.multiEdit::
If true (default), a single editor instance will be spawned to edit
diff --git a/git-send-email.perl b/git-send-email.perl
index e1e9b1460ced..6bedf745e72d 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -516,6 +516,31 @@ my %parse_alias = (
}
} },
+ sendmail => sub { my $fh = shift; while (<$fh>) {
+ # ignore blank lines and comment lines
+ if (/^\s*(?:#.*)?$/) { }
+
+ # warn on lines that contain quotes
+ elsif (/"/) {
+ print STDERR "sendmail alias with quotes is not
supported: $_\n";
+ }
+
+ # warn on lines that continue
+ elsif (/^\s|\\$/) {
+ print STDERR "sendmail continuation line is not
supported: $_\n";
+ }
+
+ # recognize lines that look like an alias
+ elsif (/^(\S+?)\s*:\s*(.+)$/) {
+ my ($alias, $addr) = ($1, $2);
+ $aliases{$alias} = [ split_addrs($addr) ];
+ }
+
+ # warn on lines that are not recognized
+ else {
+ print STDERR "sendmail line is not recognized: $_\n";
+ }}},
+
gnus => sub { my $fh = shift; while (<$fh>) {
if (/\(define-mail-alias\s+"(\S+?)"\s+"(\S+?)"\)/) {
$aliases{$1} = [ $2 ];
diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
index 7be14a4e37f7..01c7ef4d9b67 100755
--- a/t/t9001-send-email.sh
+++ b/t/t9001-send-email.sh
@@ -1549,6 +1549,33 @@ test_expect_success $PREREQ
'sendemail.aliasfile=~/.mailrc' '
grep "^!someone@example\.org!$" commandline1
'
+test_expect_success $PREREQ 'sendemail.aliasfiletype=sendmail' '
+ clean_fake_sendmail && rm -fr outdir &&
+ git format-patch -1 -o outdir &&
+ cat >>.tmp-email-aliases <<-\EOF &&
+ alice: Alice W Land
+ bob: Robert Bobbyton
+ # this is a comment
+ # this is also a comment
+ chloe: ch...@example.com
+ abgroup: alice, bob
+ bcgrp: bob, chloe, Other
+ EOF
+ git config --r
[PATCH 1/3] t4150-am: refactor and clean common setup
Add new functions to keep the setup cleaner:
- setup_temporary_branch: creates a new branch, check it out
and automatically delete it after the test is over
- setup_fixed_branch: creates a fixed branch, which can be re-used
in later tests
Signed-off-by: Remi Lespinet
---
t/t4150-am.sh | 138 --
1 file changed, 47 insertions(+), 91 deletions(-)
diff --git a/t/t4150-am.sh b/t/t4150-am.sh
index 306e6f3..8370951 100755
--- a/t/t4150-am.sh
+++ b/t/t4150-am.sh
@@ -4,6 +4,20 @@ test_description='git am running'
. ./test-lib.sh
+setup_temporary_branch () {
+ tmp_name=${2-"temporary"}
+ git reset --hard &&
+ rm -fr .git/rebase-apply &&
+ test_when_finished "git checkout $1 && git branch -D $tmp_name" &&
+ git checkout -b "$tmp_name" "$1"
+}
+
+setup_fixed_branch () {
+ git reset --hard &&
+ rm -fr .git/rebase-apply &&
+ git checkout -b "$1" "$2"
+}
+
test_expect_success 'setup: messages' '
cat >msg <<-\EOF &&
second
@@ -143,9 +157,7 @@ test_expect_success setup '
'
test_expect_success 'am applies patch correctly' '
- rm -fr .git/rebase-apply &&
- git reset --hard &&
- git checkout first &&
+ setup_temporary_branch first &&
test_tick &&
git am expected &&
echo "Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>"
>>expected &&
@@ -255,9 +257,7 @@ test_expect_success 'am without --keep removes Re: and
[PATCH] stuff' '
'
test_expect_success 'am --keep really keeps the subject' '
- rm -fr .git/rebase-apply &&
- git reset --hard &&
- git checkout HEAD^ &&
+ setup_temporary_branch master2^ &&
git am --keep patch4 &&
test_path_is_missing .git/rebase-apply &&
git cat-file commit HEAD >actual &&
@@ -265,9 +265,7 @@ test_expect_success 'am --keep really keeps the subject' '
'
test_expect_success 'am --keep-non-patch really keeps the non-patch part' '
- rm -fr .git/rebase-apply &&
- git reset --hard &&
- git checkout HEAD^ &&
+ setup_temporary_branch master2^ &&
git am --keep-non-patch patch4 &&
test_path_is_missing .git/rebase-apply &&
git cat-file commit HEAD >actual &&
@@ -275,9 +273,7 @@ test_expect_success 'am --keep-non-patch really keeps the
non-patch part' '
'
test_expect_success 'am -3 falls back to 3-way merge' '
- rm -fr .git/rebase-apply &&
- git reset --hard &&
- git checkout -b lorem2 master2 &&
+ setup_fixed_branch lorem2 master2 &&
sed -n -e "3,\$p" msg >file &&
head -n 9 msg >>file &&
git add file &&
@@ -289,9 +285,7 @@ test_expect_success 'am -3 falls back to 3-way merge' '
'
test_expect_success 'am -3 -p0 can read --no-prefix patch' '
- rm -fr .git/rebase-apply &&
- git reset --hard &&
- git checkout -b lorem3 master2 &&
+ setup_temporary_branch lorem2 &&
sed -n -e "3,\$p" msg >file &&
head -n 9 msg >>file &&
git add file &&
@@ -303,10 +297,8 @@ test_expect_success 'am -3 -p0 can read --no-prefix patch'
'
'
test_expect_success 'am can rename a file' '
+ setup_temporary_branch lorem &&
grep "^rename from" rename.patch &&
- rm -fr .git/rebase-apply &&
- git reset --hard &&
- git checkout lorem^0 &&
git am rename.patch &&
test_path_is_missing .git/rebase-apply &&
git update-index --refresh &&
@@ -314,10 +306,8 @@ test_expect_success 'am can rename a file' '
'
test_expect_success 'am -3 can rename a file' '
+ setup_temporary_branch lorem &&
grep "^rename from" rename.patch &&
- rm -fr .git/rebase-apply &&
- git reset --hard &&
- git checkout lorem^0 &&
git am -3 rename.patch &&
test_path_is_missing .git/rebase-apply &&
git update-index --refresh &&
@@ -325,10 +315,8 @@ test_expect_success 'am -3 can rename a file' '
'
test_expect_success 'am -3 can rename a file after falling back to 3-way
merge' '
+ setup_temporary_branch lorem &&
grep "^rename from" rename-add.patch &&
- rm -fr .git/rebase-apply &&
- git reset --hard &&
- git checkout lorem^0 &&
git am -3 rename-add.patch &&
test_path_is_missing .git/rebase-apply &&
git update-index --refresh &&
@@ -336,9 +324,7 @@ test_expect_success 'am -3 can rename a file after falling
back to 3-way merge'
'
test_expect_success 'am -3 -q is quiet' '
- rm -fr .git/rebase-apply &&
- git checkout -f lorem2 &&
- git reset master2 --hard &&
+ setup_temporary_branch lorem2 &&
sed -n -e "3,\$p" msg >file &&
head -n 9 msg >>file &&
git add file &&
@@ -349,11 +335,9 @@ test_expect_success 'am -3 -q is quiet' '
'
test_expect_success 'am pauses on conflict' '
- rm -fr .git/rebase-apply &&
- git reset --hard &&
- git checkout lorem2
[PATCH/RFC 2/2] git rebase -i: Warn removed or dupplicated commits
Check if commits were removed (i.e. a line was deleted) or dupplicated
(e.g. the same commit is picked twice), can print warnings or abort
git rebase according to the value of the configuration variable
rebase.checkLevel.
Add the configuration variable rebase.checkLevel.
- When unset or set to "IGNORED", no checking is done.
- When set to "WARN", the commits are checked, warnings are
displayed but git rebase still proceeds.
- When set to "ERROR", the commits are checked, warnings are
displayed and the rebase is aborted.
Signed-off-by: Galan Rémi
---
This part of the patch has no test yet, it is more for rfc.
Documentation/config.txt | 8 +
Documentation/git-rebase.txt | 5 +++
git-rebase--interactive.sh | 76
3 files changed, 89 insertions(+)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index d44bc85..2152e27 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2204,6 +2204,14 @@ rebase.autoStash::
successful rebase might result in non-trivial conflicts.
Defaults to false.
+rebase.checkLevel::
+ If set to "warn", git rebase -i will print a warning if some
+ commits are removed (i.e. a line was deleted) or if some
+ commits appear more than one time (e.g. the same commit is
+ picked twice), however the rebase will still proceed. If set
+ to "error", it will print the previous warnings and abort the
+ rebase.
+
receive.advertiseAtomic::
By default, git-receive-pack will advertise the atomic push
capability to its clients. If you don't want to this capability
diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 3cd2ef2..cb05cbb 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -213,6 +213,11 @@ rebase.autoSquash::
rebase.autoStash::
If set to true enable '--autostash' option by default.
+rebase.checkLevel::
+ If set to "warn" print warnings about removed commits and
+ duplicated commits in interactive mode. If set to "error"
+ print the warnings and abort the rebase. No check by default.
+
OPTIONS
---
--onto ::
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index cb749e8..8a837ca 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -837,6 +837,80 @@ add_exec_commands () {
mv "$1.new" "$1"
}
+# Print the list of the sha-1 of the commits
+# from a todo list in a file.
+# $1 : todo-file, $2 : outfile
+todo_list_to_sha_list () {
+ todo_list=$(git stripspace --strip-comments < "$1")
+ temp_file=$(mktemp)
+ echo "$todo_list" > "$temp_file"
+ while read -r command sha1 rest < "$temp_file"
+ do
+ case "$command" in
+ x|"exec")
+ ;;
+ *)
+ echo "$sha1" >> "$2"
+ ;;
+ esac
+ sed -i '1d' "$temp_file"
+ done
+ rm "$temp_file"
+}
+
+# Check if the user dropped some commits by mistake
+# or if there are two identical commits.
+# Behaviour determined by .gitconfig.
+check_commits () {
+ checkLevel=$(git config --get rebase.checkLevel)
+ checkLevel=${checkLevel:-"IGNORE"}
+ # To uppercase
+ checkLevel=$(echo "$checkLevel" | tr '[:lower:]' '[:upper:]')
+
+ case "$checkLevel" in
+ "WARN"|"ERROR")
+ todo_list_to_sha_list "$todo".backup "$todo".oldsha1
+ todo_list_to_sha_list "$todo" "$todo".newsha1
+
+ duplicates=$(sort "$todo".newsha1 | uniq -d)
+
+ echo "$(sort -u "$todo".oldsha1)" > "$todo".oldsha1
+ echo "$(sort -u "$todo".newsha1)" > "$todo".newsha1
+ missing=$(comm -2 -3 "$todo".oldsha1 "$todo".newsha1)
+
+ # check missing commits
+ if ! test -z "$missing"
+ then
+ warn "Warning : some commits may have been dropped
accidentally."
+ warn "Dropped commits:"
+ warn "$missing"
+ warn "To avoid this message, use \"drop\" to
explicitely remove a commit."
+ warn "Use git --config rebase.checkLevel to change"
+ warn "the level of warnings (ignore,warn,error)."
+ warn ""
+
+ if test "$checkLevel" = "ERROR"
+ then
+ die_abort "Rebase aborted due to dropped
commits."
+ fi
+ fi
+
+ # check duplicate commits
+ if ! test -z "$duplicates"
+ then
+ warn "Warning : some commits have been used twice:"
+ warn "$duplicates"
+ warn ""
+ fi
+ ;;
+ "IGNORE")
+
[PATCH 2/3] t4150-am: refactor am -3 tests
Move the creation of the file, commit and branch used in git am -3 tests in a setup test, to avoid creating this setup several time. Signed-off-by: Remi Lespinet --- t/t4150-am.sh | 18 ++ 1 file changed, 6 insertions(+), 12 deletions(-) diff --git a/t/t4150-am.sh b/t/t4150-am.sh index 8370951..8f85098 100755 --- a/t/t4150-am.sh +++ b/t/t4150-am.sh @@ -272,13 +272,17 @@ test_expect_success 'am --keep-non-patch really keeps the non-patch part' ' grep "^\[foo\] third" actual ' +test_expect_success 'setup: am -3' ' + setup_fixed_branch lorem2 master2 && + sed -n -e "3,\$p" msg >file && + head -n 9 msg >>file && + git add file && + test_tick && + git commit -m "copied stuff" +' + test_expect_success 'am -3 falls back to 3-way merge' ' + setup_temporary_branch lorem2 && - setup_fixed_branch lorem2 master2 && - sed -n -e "3,\$p" msg >file && - head -n 9 msg >>file && - git add file && - test_tick && - git commit -m "copied stuff" && git am -3 lorem-move.patch && test_path_is_missing .git/rebase-apply && git diff --exit-code lorem @@ -286,11 +290,6 @@ test_expect_success 'am -3 falls back to 3-way merge' ' test_expect_success 'am -3 -p0 can read --no-prefix patch' ' setup_temporary_branch lorem2 && - sed -n -e "3,\$p" msg >file && - head -n 9 msg >>file && - git add file && - test_tick && - git commit -m "copied stuff" && git am -3 -p0 lorem-zero.patch && test_path_is_missing .git/rebase-apply && git diff --exit-code lorem @@ -325,11 +324,6 @@ test_expect_success 'am -3 can rename a file after falling back to 3-way merge' test_expect_success 'am -3 -q is quiet' ' setup_temporary_branch lorem2 && - sed -n -e "3,\$p" msg >file && - head -n 9 msg >>file && - git add file && - test_tick && - git commit -m "copied stuff" && git am -3 -q lorem-move.patch >output.out 2>&1 && ! test -s output.out ' -- 1.9.1 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 3/3] git-am: add am.threeWay config variable
Add the am.threeWay configuration variable to use the -3 or --3way option of git am by default. When am.threeway is set and not desired for a specific git am command, the --no-3way option can be used to override it. Signed-off-by: Remi Lespinet --- Even if git am will be rewritten soon, the code that will have to be ported is not the most important part of the patch and the tests and documentation parts can be reused. Documentation/config.txt | 7 +++ Documentation/git-am.txt | 6 -- git-am.sh| 7 +++ t/t4150-am.sh| 15 +++ 4 files changed, 33 insertions(+), 2 deletions(-) diff --git a/Documentation/config.txt b/Documentation/config.txt index d44bc85..8e42752 100644 --- a/Documentation/config.txt +++ b/Documentation/config.txt @@ -769,6 +769,13 @@ am.keepcr:: by giving '--no-keep-cr' from the command line. See linkgit:git-am[1], linkgit:git-mailsplit[1]. +am.threeWay:: + If true, git-am will fall back on 3-way merge when the patch + cannot be applied cleanly, in the same way as the '-3' or + '--3-way' option. Can be overridden by giving '--no-3-way' + from the command line. + See linkgit:git-am[1]. + apply.ignoreWhitespace:: When set to 'change', tells 'git apply' to ignore changes in whitespace, in the same way as the '--ignore-space-change' diff --git a/Documentation/git-am.txt b/Documentation/git-am.txt index 0d8ba48..3190c05 100644 --- a/Documentation/git-am.txt +++ b/Documentation/git-am.txt @@ -89,11 +89,13 @@ default. You can use `--no-utf8` to override this. linkgit:git-mailinfo[1]). -3:: ---3way:: +--[no-]3way:: When the patch does not apply cleanly, fall back on 3-way merge if the patch records the identity of blobs it is supposed to apply to and we have those blobs - available locally. + available locally. `am.threeWay` configuration variable + can be used to specify the default behaviour. `--no-3way` + is useful to override `am.threeWay`. --ignore-space-change:: --ignore-whitespace:: diff --git a/git-am.sh b/git-am.sh index 761befb..781507c 100755 --- a/git-am.sh +++ b/git-am.sh @@ -389,6 +389,11 @@ then keepcr=t fi +if test "$(git config --bool --get am.threeWay)" = true +then +threeway=t +fi + while test $# != 0 do case "$1" in @@ -400,6 +405,8 @@ it will be removed. Please do not use it anymore." ;; -3|--3way) threeway=t ;; + --no-3way) + threeway=f ;; -s|--signoff) sign=t ;; -u|--utf8) diff --git a/t/t4150-am.sh b/t/t4150-am.sh index 8f85098..e16ef0e 100755 --- a/t/t4150-am.sh +++ b/t/t4150-am.sh @@ -288,6 +288,21 @@ test_expect_success 'am -3 falls back to 3-way merge' ' git diff --exit-code lorem ' +test_expect_success 'am with config am.threeWay falls back to 3-way merge' ' + setup_temporary_branch lorem2 && + test_config am.threeWay 1 && + git am lorem-move.patch && + test_path_is_missing .git/rebase-apply && + git diff --exit-code lorem +' + +test_expect_success 'am with config am.threeWay overridden by --no-3way' ' + setup_temporary_branch lorem2 && + test_config am.threeWay 1 && + test_must_fail git am --no-3way lorem-move.patch && + test_path_is_dir .git/rebase-apply +' + test_expect_success 'am -3 -p0 can read --no-prefix patch' ' setup_temporary_branch lorem2 && git am -3 -p0 lorem-zero.patch && -- 1.9.1 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH/RFC 1/2] git-rebase -i: Add key word "drop" to remove a commit
Instead of removing a line to remove the commit, you can use the key
word "drop" (just like "pick" or "edit"). It has the same effect as
deleting the line (removing the commit) except that you keep a visual
trace of your actions, allowing a better control and reducing the
possibility of removing a commit by mistake.
Signed-off-by: Galan Rémi
---
Documentation/git-rebase.txt | 3 +++
git-rebase--interactive.sh| 4
t/lib-rebase.sh | 4 ++--
t/t3404-rebase-interactive.sh | 11 +++
4 files changed, 20 insertions(+), 2 deletions(-)
diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 1d01baa..3cd2ef2 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -514,6 +514,9 @@ rebasing.
If you just want to edit the commit message for a commit, replace the
command "pick" with the command "reword".
+If you want to remove a commit, replace the command "pick" by the
+command "drop".
+
If you want to fold two or more commits into one, replace the command
"pick" for the second and subsequent commits with "squash" or "fixup".
If the commits had different authors, the folded commit will be
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index dc3133f..cb749e8 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -152,6 +152,7 @@ Commands:
s, squash = use commit, but meld into previous commit
f, fixup = like "squash", but discard this commit's log message
x, exec = run command (the rest of the line) using shell
+ d, drop = remove commit
These lines can be re-ordered; they are executed from top to bottom.
@@ -515,6 +516,9 @@ do_next () {
do_pick $sha1 "$rest"
record_in_rewritten $sha1
;;
+ drop|d)
+ mark_action_done
+ ;;
reword|r)
comment_for_reflog reword
diff --git a/t/lib-rebase.sh b/t/lib-rebase.sh
index 6bd2522..fdbc900 100644
--- a/t/lib-rebase.sh
+++ b/t/lib-rebase.sh
@@ -14,7 +14,7 @@
# specified line.
#
# " " -- add a line with the specified command
-# ("squash", "fixup", "edit", or "reword") and the SHA1 taken
+# ("squash", "fixup", "edit", "reword" or "drop") and the SHA1 taken
# from the specified line.
#
# "exec_cmd_with_args" -- add an "exec cmd with args" line.
@@ -46,7 +46,7 @@ set_fake_editor () {
action=pick
for line in $FAKE_LINES; do
case $line in
- squash|fixup|edit|reword)
+ squash|fixup|edit|reword|drop)
action="$line";;
exec*)
echo "$line" | sed 's/_/ /g' >> "$1";;
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index ac429a0..1bad068 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -1102,4 +1102,15 @@ test_expect_success 'rebase -i commits that overwrite
untracked files (no ff)' '
test $(git cat-file commit HEAD | sed -ne \$p) = I
'
+test_expect_success 'drop' '
+ git checkout master &&
+ test_when_finished "git checkout master" &&
+ git checkout -b dropBranchTest master &&
+ set_fake_editor &&
+ FAKE_LINES="1 drop 2 3 drop 4 5" git rebase -i --root &&
+ test E = $(git cat-file commit HEAD | sed -ne \$p) &&
+ test C = $(git cat-file commit HEAD^ | sed -ne \$p) &&
+ test A = $(git cat-file commit HEAD^^ | sed -ne \$p)
+'
+
test_done
--
2.4.1.174.g28bfe8e
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[ANNOUNCE] Git v2.4.2
The latest maintenance release Git v2.4.2 is now available at the usual places. The tarballs are found at: https://www.kernel.org/pub/software/scm/git/ The following public repositories all have a copy of the 'v2.4.2' tag and the 'maint' branch that the tag points at: url = https://kernel.googlesource.com/pub/scm/git/git url = git://repo.or.cz/alt-git.git url = https://code.google.com/p/git-core/ url = git://git.sourceforge.jp/gitroot/git-core/git.git url = git://git-core.git.sourceforge.net/gitroot/git-core/git-core url = https://github.com/gitster/git Git v2.4.2 Release Notes Fixes since v2.4.1 -- * "git rev-list --objects $old --not --all" to see if everything that is reachable from $old is already connected to the existing refs was very inefficient. * "hash-object --literally" introduced in v2.2 was not prepared to take a really long object type name. * "git rebase --quiet" was not quite quiet when there is nothing to do. * The completion for "log --decorate=" parameter value was incorrect. * "filter-branch" corrupted commit log message that ends with an incomplete line on platforms with some "sed" implementations that munge such a line. Work it around by avoiding to use "sed". * "git daemon" fails to build from the source under NO_IPV6 configuration (regression in 2.4). * "git stash pop/apply" forgot to make sure that not just the working tree is clean but also the index is clean. The latter is important as a stash application can conflict and the index will be used for conflict resolution. * We have prepended $GIT_EXEC_PATH and the path "git" is installed in (typically "/usr/bin") to $PATH when invoking subprograms and hooks for almost eternity, but the original use case the latter tried to support was semi-bogus (i.e. install git to /opt/foo/git and run it without having /opt/foo on $PATH), and more importantly it has become less and less relevant as Git grew more mainstream (i.e. the users would _want_ to have it on their $PATH). Stop prepending the path in which "git" is installed to users' $PATH, as that would interfere the command search order people depend on (e.g. they may not like versions of programs that are unrelated to Git in /usr/bin and want to override them by having different ones in /usr/local/bin and have the latter directory earlier in their $PATH). Also contains typofixes, documentation updates and trivial code clean-ups. Changes since v2.4.1 are as follows: Eric Sunshine (3): git-hash-object.txt: document --literally option hash-object --literally: fix buffer overrun with extra-long object type t1007: add hash-object --literally tests Jeff King (7): limit_list: avoid quadratic behavior from still_interesting t3903: stop hard-coding commit sha1s t3903: avoid applying onto dirty index stash: require a clean index to apply stop putting argv[0] dirname at front of PATH rebase: silence "git checkout" for noop rebase filter-branch: avoid passing commit message through sed Junio C Hamano (3): write_sha1_file(): do not use a separate sha1[] array daemon: unbreak NO_IPV6 build regression Git 2.4.2 SZEDER Gábor (1): completion: fix and update 'git log --decorate=' options -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
What's cooking in git.git (May 2015, #07; Tue, 26)
Here are the topics that have been cooking. Commits prefixed with
'-' are only in 'pu' (proposed updates) while commits prefixed with
'+' are in 'next'.
The "untracked cache" series is in 'master' now. I do not use it
personally, but it is meant to make life easier for those with large
amount of untracked cruft in their working trees. Please try it out
and report successes (and of course breakages, too).
You can find the changes described here in the integration branches
of the repositories listed at
http://git-blame.blogspot.com/p/git-public-repositories.html
--
[Graduated to "master"]
* jk/rerere-forget-check-enabled (2015-05-14) 1 commit
(merged to 'next' on 2015-05-19 at bfe67dc)
+ rerere: exit silently on "forget" when rerere is disabled
"git rerere forget" in a repository without rerere enabled gave a
cryptic error message; it should be a silent no-op instead.
* nd/untracked-cache (2015-03-12) 24 commits
(merged to 'next' on 2015-05-19 at 26e619b)
+ git-status.txt: advertisement for untracked cache
+ untracked cache: guard and disable on system changes
+ mingw32: add uname()
+ t7063: tests for untracked cache
+ update-index: test the system before enabling untracked cache
+ update-index: manually enable or disable untracked cache
+ status: enable untracked cache
+ untracked-cache: temporarily disable with $GIT_DISABLE_UNTRACKED_CACHE
+ untracked cache: mark index dirty if untracked cache is updated
+ untracked cache: print stats with $GIT_TRACE_UNTRACKED_STATS
+ untracked cache: avoid racy timestamps
+ read-cache.c: split racy stat test to a separate function
+ untracked cache: invalidate at index addition or removal
+ untracked cache: load from UNTR index extension
+ untracked cache: save to an index extension
+ ewah: add convenient wrapper ewah_serialize_strbuf()
+ untracked cache: don't open non-existent .gitignore
+ untracked cache: mark what dirs should be recursed/saved
+ untracked cache: record/validate dir mtime and reuse cached output
+ untracked cache: make a wrapper around {open,read,close}dir()
+ untracked cache: invalidate dirs recursively if .gitignore changes
+ untracked cache: initial untracked cache validation
+ untracked cache: record .gitignore information and dir hierarchy
+ dir.c: optionally compute sha-1 of a .gitignore file
Teach the index to optionally remember already seen untracked files
to speed up "git status" in a working tree with tons of cruft.
* pt/pull-ff-vs-merge-ff (2015-05-18) 2 commits
(merged to 'next' on 2015-05-20 at 064a082)
+ pull: parse pull.ff as a bool or string
+ pull: make pull.ff=true override merge.ff
The pull.ff configuration was supposed to override the merge.ff
configuration, but it didn't.
* pt/pull-log-n (2015-05-18) 1 commit
(merged to 'next' on 2015-05-20 at db6ce78)
+ pull: handle --log=
"git pull --log" and "git pull --no-log" worked as expected, but
"git pull --log=20" did not.
* rs/plug-leak-in-pack-bitmaps (2015-05-19) 1 commit
(merged to 'next' on 2015-05-20 at b70f647)
+ pack-bitmaps: plug memory leak, fix allocation size for recent_bitmaps
The code to read pack-bitmap wanted to allocate a few hundred
pointers to a structure, but by mistake allocated and leaked memory
enough to hold that many actual structures. Correct the allocation
size and also have it on stack, as it is small enough.
--
[New Topics]
* ah/send-email-sendmail-alias (2015-05-25) 2 commits
- t9001: write $HOME/, not ~/, to help shells without tilde expansion
- send-email: add sendmail email aliases format
"git send-email" learned the alias file format used by the sendmail
program (in an abbreviated form).
Reroll coming? What's queued is good enough?
* bc/object-id (2015-05-25) 56 commits
(merged to 'next' on 2015-05-26 at 8d9f645)
+ struct ref_lock: convert old_sha1 member to object_id
+ warn_if_dangling_symref(): convert local variable "junk" to object_id
+ each_ref_fn_adapter(): remove adapter
+ rev_list_insert_ref(): remove unneeded arguments
+ rev_list_insert_ref_oid(): new function, taking an object_oid
+ mark_complete(): remove unneeded arguments
+ mark_complete_oid(): new function, taking an object_oid
+ clear_marks(): rewrite to take an object_id argument
+ mark_complete(): rewrite to take an object_id argument
+ send_ref(): convert local variable "peeled" to object_id
+ upload-pack: rewrite functions to take object_id arguments
+ find_symref(): convert local variable "unused" to object_id
+ find_symref(): rewrite to take an object_id argument
+ write_one_ref(): rewrite to take an object_id argument
+ write_refs_to_temp_dir(): convert local variable sha1 to object_id
+ submodule: rewrite to take an object_id argument
+ shallow: rewrite functions to take object_id arguments
+ handle_one_ref(): rewrite to take an object_id argument
+ add_info_ref(): rewrite to tak
Re: [PATCH] submodule documentation: Reorder introductory paragraphs
On Tue, May 26, 2015 at 10:53:15AM -0700, Stefan Beller wrote: > On Mon, May 25, 2015 at 3:00 PM, Junio C Hamano wrote: > > Stefan Beller writes: > > On the other hand, I probably would not have felt such a strong > > "strangeness" if it were described like this: > > > > This command can help you inspect, update, and manage > > submodules. > > > > I haven't analized it enough to say why it is, but I suspect it has > > something to do with (my own) perception that "git submodule" is not > > very essential to do any of these things (i.e. .gitmodules is a very > > simple text file), but is primarily a helpful wrapper. > > My perception is that the submodule man page similar to the subtree > man page tries to explain an underlying concept as well. The other man > pages you quoted don't do that as the concepts are explained elsewhere(?) > > As a side note: In the Gerrit test suite I use the JGit implementation of > the config command to write out .gitmodules files. So maybe `git submodule` > can be understood as a specialized form of `git config`. I do not agree here. That view is too limited. Since in the case of e.g. 'git submodule add‘ it does not only change the .gitmodules file but adds a gitlink entry to the index, moves the database into .git/modules, ... . And even though it is currently not doing much more it might in the future. E.g. it might make sense to add a 'git submodule gc' command which allows the user to purge unused submodule databases from the .git/modules directory. So I would say it is: "a helper" or "a tool" for submodules. Nothing less nothing more. But on the other hand the same is true for other porcelain commands like e.g. 'git commit'. If you take a look at gitcore-tutorial you could also describe it as a wrapper for write-tree, commit-tree and update-ref to create a commit. Yet the man page says: "Record changes to the repository". So I am not sure where to draw the line between wrapper and essential command. As a user I would see it as quite essential since for adding a submodule I would need to remember a couple of things: * clone the database into .git/modules * create the gitlink file * checkout the files to the desired directory * add the url to the .gitmodules file So why not go with Junios first suggestion and lets drop the "This command can help you..." and say: "Inspect, update and manage submodules". Cheers Heiko -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[RFC/WIP PATCH 00/11] Protocol version 2, again!
"Just give us something to play around with" - Peff at GitMerge 2015 This is another approach for updating the pack protocol of Git. While in the previous attempts I tried to come up with the perfect specification of the new protocol I realized that such an approach doesn't lead anywhere. So this time actual working code is included! The included code is not complete, but a minimal example of git config remote.origin.transportversion 2 git fetch origin master # uses the new protocol! works. In a previous discussion[1] towards version 2 of the pack protocol I wanted to come up with a protocol which even includes negotiating what is done in the protocol exchange, such as having a command "push, fetch, ls-remote" being part of the protocol. This is not a very good approach as it's too much work to be done at the same time. This patch series focusses on just the fetching side, so the first four patches are teaching git-upload-pack about the new pack protocol. The next five patches will teach git fetch and fetch-pack how to use the version 2 protocol. Then there will be a small test and a documentation update. The new protocol works just like the old protocol, except for the capabilities negotiation being before any exchange of real data. This solves the problem of having a first huge chunk of data (the refs advertisement) sent over the wire and just realizing in between that we don't need these data for that operation and no way to cancel. By having a capabilites negotiation first the new protocol may be even more future-proof than the current one, as the capabilites will hopefully be kept small and all larger data transfers will get their own later stage in the protocol. To determine the protocol version we check the ending of the invoked program for an appended version number to see which protocol version we're using in an exchange. At first I thought we should append a unique suffix instead of a version number to make a distinction easier for the kind of protocol we want to talk (There may be the traditional protocol with no suffix, or the "upload-pack-capabilities-first" protocol which will transmit the capabilities first). My preference for a string suffix instead of a sequential number is motiviated by the discussion of sha1 vs sequential numbers to describe a state of a repository. The main difference here is however how often we expect changes. Version 1 of the protocol is current for 10 years by now, so I do not changes to the protocol number often, which makes it suitable for just having a counter appended to the binary. The advantage of just a counting number is its small size, and I think this advantage outweights the disadvantage of having named protocol versions. This series doesn't include an automatic upgrade of the protocol version for later changes if the server supports it, so for now the use of the new protocol needs to be activated manually via setting remote.origin.transportversion. Any comments welcome! Thanks, Stefan [1] http://permalink.gmane.org/gmane.comp.version-control.git/267572 Nguyễn Thái Ngọc Duy (2): upload-pack: make client capability parsing code a separate function upload-pack: only accept capabilities on the first "want" line Stefan Beller (9): upload-pack: move capabilities out of send_ref upload-pack-2: Implement the version 2 of upload-pack transport: add infrastructure to support a protocol version number remote.h: add get_remote_capabilities, request_capabilities fetch-pack: use the configured transport protocol transport: connect_setup appends protocol version number transport: get_refs_via_connect exchanges capabilities before refs. t5544: add a test case for the new protocol Document protocol version 2 .gitignore| 1 + Documentation/technical/pack-protocol.txt | 86 -- Documentation/technical/protocol-capabilities.txt | 15 --- Makefile | 2 + builtin/fetch-pack.c | 17 ++- builtin/fetch.c | 6 + connect.c | 48 +++- fetch-pack.h | 1 + remote.c | 2 + remote.h | 5 + t/t5544-fetch-2.sh| 40 +++ transport-helper.c| 1 + transport.c | 60 +- transport.h | 4 + upload-pack-2.c | 1 + upload-pack.c | 138 +- 16 files changed, 366 insertions(+), 61 deletions(-) create mode 100755 t/t5544-fetch-2.sh create mode 12 upload-pack-2.c -- 2.4.1.345.gab207b6.dirty -- To unsubscribe from this list: send the line "unsubscribe git" in
[RFC/WIP PATCH 08/11] transport: connect_setup appends protocol version number
Signed-off-by: Stefan Beller
---
transport.c | 18 --
1 file changed, 16 insertions(+), 2 deletions(-)
diff --git a/transport.c b/transport.c
index 3ef15f6..33644a6 100644
--- a/transport.c
+++ b/transport.c
@@ -496,15 +496,29 @@ static int set_git_option(struct git_transport_options
*opts,
static int connect_setup(struct transport *transport, int for_push, int
verbose)
{
struct git_transport_data *data = transport->data;
+ const char *remote_program;
+ char *buf = 0;
if (data->conn)
return 0;
+ remote_program = (for_push ? data->options.receivepack
+ : data->options.uploadpack);
+
+ if (transport->smart_options
+ && transport->smart_options->transport_version) {
+ buf = xmalloc(strlen(remote_program) + 12);
+ sprintf(buf, "%s-%d", remote_program,
+ transport->smart_options->transport_version);
+ remote_program = buf;
+ }
+
data->conn = git_connect(data->fd, transport->url,
-for_push ? data->options.receivepack :
-data->options.uploadpack,
+remote_program,
verbose ? CONNECT_VERBOSE : 0);
+ free(buf);
+
return 0;
}
--
2.4.1.345.gab207b6.dirty
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[RFC/WIP PATCH 06/11] remote.h: add get_remote_capabilities, request_capabilities
Instead of calling get_remote_heads as a first command during the
protocol exchange, we need to have fine grained control over the
capability negotiation in version 2 of the protocol.
Introduce get_remote_capabilities, which will just listen to
capabilities of the remote and request_capabilities which will
tell the selection of capabilities to the remote.
Signed-off-by: Stefan Beller
---
connect.c | 48 +++-
remote.h | 3 +++
2 files changed, 50 insertions(+), 1 deletion(-)
diff --git a/connect.c b/connect.c
index c0144d8..bb17618 100644
--- a/connect.c
+++ b/connect.c
@@ -105,8 +105,54 @@ static void annotate_refs_with_symref_info(struct ref *ref)
string_list_clear(&symref, 0);
}
+void get_remote_capabilities(int in, char *src_buf, size_t src_len)
+{
+ struct strbuf capabilities_string = STRBUF_INIT;
+ for (;;) {
+ int len;
+ char *line = packet_buffer;
+ const char *arg;
+
+ len = packet_read(in, &src_buf, &src_len,
+ packet_buffer, sizeof(packet_buffer),
+ PACKET_READ_GENTLE_ON_EOF |
+ PACKET_READ_CHOMP_NEWLINE);
+ if (len < 0)
+ die_initial_contact(0);
+
+ if (!len)
+ break;
+
+ if (len > 4 && skip_prefix(line, "ERR ", &arg))
+ die("remote error: %s", arg);
+
+ if (starts_with(line, "capability:")) {
+ strbuf_addstr(&capabilities_string, line +
strlen("capability:"));
+ strbuf_addch(&capabilities_string, ' ');
+ }
+ }
+ free(server_capabilities);
+ server_capabilities = xmalloc(capabilities_string.len + 1);
+ server_capabilities = strbuf_detach(&capabilities_string, NULL);
+
+ strbuf_release(&capabilities_string);
+}
+
+int request_capabilities(int out)
+{
+ fprintf(stderr, "request_capabilities\n");
+ // todo: send our capabilities
+ packet_write(out, "capability:foo");
+ packet_flush(out);
+}
+
/*
- * Read all the refs from the other end
+ * Read all the refs from the other end,
+ * in is a file descriptor input stream
+ * src_buf and src_len may be an alternative way to specify the input.
+ * list is the output of refs
+ * extra_have is a list to store information learned about sha1 the other side
has.
+ * shallow_points
*/
struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,
struct ref **list, unsigned int flags,
diff --git a/remote.h b/remote.h
index 04e2310..7381ddf 100644
--- a/remote.h
+++ b/remote.h
@@ -146,6 +146,9 @@ int check_ref_type(const struct ref *ref, int flags);
void free_refs(struct ref *ref);
struct sha1_array;
+
+extern void get_remote_capabilities(int in, char *src_buf, size_t src_len);
+extern int request_capabilities(int out);
extern struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,
struct ref **list, unsigned int flags,
struct sha1_array *extra_have,
--
2.4.1.345.gab207b6.dirty
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[RFC/WIP PATCH 02/11] upload-pack: only accept capabilities on the first "want" line
From: Nguyễn Thái Ngọc Duy
pack-protocol.txt says so and fetch-pack also follows it even though
upload-pack is a bit lax. Fix it.
Signed-off-by: Nguyễn Thái Ngọc Duy
Signed-off-by: Stefan Beller
---
upload-pack.c | 7 ++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/upload-pack.c b/upload-pack.c
index 5449ff7..6734777 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -558,6 +558,7 @@ static void receive_needs(void)
struct object_array shallows = OBJECT_ARRAY_INIT;
int depth = 0;
int has_non_tip = 0;
+ int first_want = 1;
shallow_nr = 0;
for (;;) {
@@ -596,7 +597,11 @@ static void receive_needs(void)
die("git upload-pack: protocol error, "
"expected to get sha, not '%s'", line);
- parse_features(line + 45);
+ if (first_want) {
+ parse_features(line + 45);
+ first_want = 0;
+ } else if (line[45])
+ die("garbage at the end of 'want' line %s", line + 45);
o = parse_object(sha1_buf);
if (!o)
--
2.4.1.345.gab207b6.dirty
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[RFC/WIP PATCH 07/11] fetch-pack: use the configured transport protocol
Signed-off-by: Stefan Beller
---
builtin/fetch-pack.c | 17 -
fetch-pack.h | 1 +
2 files changed, 17 insertions(+), 1 deletion(-)
diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index 4a6b340..32dc8b0 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -127,6 +127,10 @@ int cmd_fetch_pack(int argc, const char **argv, const char
*prefix)
args.update_shallow = 1;
continue;
}
+ if (!strcmp("--transport-version", arg)) {
+ args.version = strtol(arg +
strlen("--transport-version"), NULL, 0);
+ continue;
+ }
usage(fetch_pack_usage);
}
@@ -175,7 +179,18 @@ int cmd_fetch_pack(int argc, const char **argv, const char
*prefix)
if (!conn)
return args.diag_url ? 0 : 1;
}
- get_remote_heads(fd[0], NULL, 0, &ref, 0, NULL, &shallow);
+
+ switch (args.version) {
+ default:
+ case 2:
+ get_remote_capabilities(fd[0], NULL, 0);
+ request_capabilities(fd[1]);
+ break;
+ case 1: /* fall through */
+ case 0:
+ get_remote_heads(fd[0], NULL, 0, &ref, 0, NULL, &shallow);
+ break;
+ }
ref = fetch_pack(&args, fd, conn, ref, dest, sought, nr_sought,
&shallow, pack_lockfile_ptr);
diff --git a/fetch-pack.h b/fetch-pack.h
index bb7fd76..b48b4f5 100644
--- a/fetch-pack.h
+++ b/fetch-pack.h
@@ -10,6 +10,7 @@ struct fetch_pack_args {
const char *uploadpack;
int unpacklimit;
int depth;
+ int version;
unsigned quiet:1;
unsigned keep_pack:1;
unsigned lock_pack:1;
--
2.4.1.345.gab207b6.dirty
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[RFC/WIP PATCH 01/11] upload-pack: make client capability parsing code a separate function
From: Nguyễn Thái Ngọc Duy
Signed-off-by: Nguyễn Thái Ngọc Duy
Signed-off-by: Stefan Beller
---
upload-pack.c | 44 +++-
1 file changed, 23 insertions(+), 21 deletions(-)
diff --git a/upload-pack.c b/upload-pack.c
index 745fda8..5449ff7 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -531,6 +531,28 @@ error:
}
}
+static void parse_features(const char *features)
+{
+ if (parse_feature_request(features, "multi_ack_detailed"))
+ multi_ack = 2;
+ else if (parse_feature_request(features, "multi_ack"))
+ multi_ack = 1;
+ if (parse_feature_request(features, "no-done"))
+ no_done = 1;
+ if (parse_feature_request(features, "thin-pack"))
+ use_thin_pack = 1;
+ if (parse_feature_request(features, "ofs-delta"))
+ use_ofs_delta = 1;
+ if (parse_feature_request(features, "side-band-64k"))
+ use_sideband = LARGE_PACKET_MAX;
+ else if (parse_feature_request(features, "side-band"))
+ use_sideband = DEFAULT_PACKET_MAX;
+ if (parse_feature_request(features, "no-progress"))
+ no_progress = 1;
+ if (parse_feature_request(features, "include-tag"))
+ use_include_tag = 1;
+}
+
static void receive_needs(void)
{
struct object_array shallows = OBJECT_ARRAY_INIT;
@@ -540,7 +562,6 @@ static void receive_needs(void)
shallow_nr = 0;
for (;;) {
struct object *o;
- const char *features;
unsigned char sha1_buf[20];
char *line = packet_read_line(0, NULL);
reset_timeout();
@@ -575,26 +596,7 @@ static void receive_needs(void)
die("git upload-pack: protocol error, "
"expected to get sha, not '%s'", line);
- features = line + 45;
-
- if (parse_feature_request(features, "multi_ack_detailed"))
- multi_ack = 2;
- else if (parse_feature_request(features, "multi_ack"))
- multi_ack = 1;
- if (parse_feature_request(features, "no-done"))
- no_done = 1;
- if (parse_feature_request(features, "thin-pack"))
- use_thin_pack = 1;
- if (parse_feature_request(features, "ofs-delta"))
- use_ofs_delta = 1;
- if (parse_feature_request(features, "side-band-64k"))
- use_sideband = LARGE_PACKET_MAX;
- else if (parse_feature_request(features, "side-band"))
- use_sideband = DEFAULT_PACKET_MAX;
- if (parse_feature_request(features, "no-progress"))
- no_progress = 1;
- if (parse_feature_request(features, "include-tag"))
- use_include_tag = 1;
+ parse_features(line + 45);
o = parse_object(sha1_buf);
if (!o)
--
2.4.1.345.gab207b6.dirty
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[RFC/WIP PATCH 03/11] upload-pack: move capabilities out of send_ref
This will make it easier to reuse the capabilities in a later patch.
Signed-off-by: Stefan Beller
---
upload-pack.c | 14 --
1 file changed, 8 insertions(+), 6 deletions(-)
diff --git a/upload-pack.c b/upload-pack.c
index 6734777..a5f75b7 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -716,33 +716,35 @@ static void format_symref_info(struct strbuf *buf, struct
string_list *symref)
strbuf_addf(buf, " symref=%s:%s", item->string, (char
*)item->util);
}
-static int send_ref(const char *refname, const unsigned char *sha1, int flag,
void *cb_data)
-{
- static const char *capabilities = "multi_ack thin-pack side-band"
+static const char *advertise_capabilities = "multi_ack thin-pack side-band"
" side-band-64k ofs-delta shallow no-progress"
" include-tag multi_ack_detailed";
+
+static int send_ref(const char *refname, const unsigned char *sha1, int flag,
void *cb_data)
+{
+
const char *refname_nons = strip_namespace(refname);
unsigned char peeled[20];
if (mark_our_ref(refname, sha1))
return 0;
- if (capabilities) {
+ if (advertise_capabilities) {
struct strbuf symref_info = STRBUF_INIT;
format_symref_info(&symref_info, cb_data);
packet_write(1, "%s %s%c%s%s%s%s agent=%s\n",
sha1_to_hex(sha1), refname_nons,
-0, capabilities,
+0, advertise_capabilities,
allow_tip_sha1_in_want ? " allow-tip-sha1-in-want"
: "",
stateless_rpc ? " no-done" : "",
symref_info.buf,
git_user_agent_sanitized());
strbuf_release(&symref_info);
+ advertise_capabilities = NULL;
} else {
packet_write(1, "%s %s\n", sha1_to_hex(sha1), refname_nons);
}
- capabilities = NULL;
if (!peel_ref(refname, peeled))
packet_write(1, "%s %s^{}\n", sha1_to_hex(peeled),
refname_nons);
return 0;
--
2.4.1.345.gab207b6.dirty
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[RFC/WIP PATCH 05/11] transport: add infrastructure to support a protocol version number
The transport version set via command line argument in
git fetch takes precedence over the configured version.
Signed-off-by: Stefan Beller
---
builtin/fetch.c| 6 ++
remote.c | 2 ++
remote.h | 2 ++
transport-helper.c | 1 +
transport.c| 13 +
transport.h| 4
6 files changed, 28 insertions(+)
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 7910419..d2e1828 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -46,6 +46,7 @@ static const char *recurse_submodules_default;
static int shown_url = 0;
static int refmap_alloc, refmap_nr;
static const char **refmap_array;
+static const char *transport_version;
static int option_parse_recurse_submodules(const struct option *opt,
const char *arg, int unset)
@@ -121,6 +122,9 @@ static struct option builtin_fetch_options[] = {
N_("default mode for recursion"), PARSE_OPT_HIDDEN },
OPT_BOOL(0, "update-shallow", &update_shallow,
N_("accept refs that update .git/shallow")),
+ OPT_STRING('y', "transport-version", &transport_version,
+ N_("transport-version"),
+ N_("specify transport version to be used")),
{ OPTION_CALLBACK, 0, "refmap", NULL, N_("refmap"),
N_("specify fetch refmap"), PARSE_OPT_NONEG, parse_refmap_arg },
OPT_END()
@@ -848,6 +852,8 @@ static struct transport *prepare_transport(struct remote
*remote)
struct transport *transport;
transport = transport_get(remote, NULL);
transport_set_verbosity(transport, verbosity, progress);
+ if (transport_version)
+ set_option(transport, TRANS_OPT_TRANSPORTVERSION,
transport_version);
if (upload_pack)
set_option(transport, TRANS_OPT_UPLOADPACK, upload_pack);
if (keep)
diff --git a/remote.c b/remote.c
index 68901b0..2914d9d 100644
--- a/remote.c
+++ b/remote.c
@@ -476,6 +476,8 @@ static int handle_config(const char *key, const char
*value, void *cb)
key, value);
} else if (!strcmp(subkey, ".vcs")) {
return git_config_string(&remote->foreign_vcs, key, value);
+ } else if (!strcmp(subkey, ".transportversion")) {
+ remote->transport_version = git_config_int(key, value);
}
return 0;
}
diff --git a/remote.h b/remote.h
index 02d66ce..04e2310 100644
--- a/remote.h
+++ b/remote.h
@@ -50,6 +50,8 @@ struct remote {
const char *receivepack;
const char *uploadpack;
+ int transport_version;
+
/*
* for curl remotes only
*/
diff --git a/transport-helper.c b/transport-helper.c
index 5d99a6b..ab3cd5b 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -247,6 +247,7 @@ static int disconnect_helper(struct transport *transport)
}
static const char *unsupported_options[] = {
+ TRANS_OPT_TRANSPORTVERSION,
TRANS_OPT_UPLOADPACK,
TRANS_OPT_RECEIVEPACK,
TRANS_OPT_THIN,
diff --git a/transport.c b/transport.c
index f080e93..3ef15f6 100644
--- a/transport.c
+++ b/transport.c
@@ -479,6 +479,16 @@ static int set_git_option(struct git_transport_options
*opts,
} else if (!strcmp(name, TRANS_OPT_PUSH_CERT)) {
opts->push_cert = !!value;
return 0;
+ } else if (!strcmp(name, TRANS_OPT_TRANSPORTVERSION)) {
+ if (!value)
+ opts->transport_version = 0;
+ else {
+ char *end;
+ opts->transport_version = strtol(value, &end, 0);
+ if (*end)
+ die("transport: invalid transport version
option '%s'", value);
+ }
+ return 0;
}
return 1;
}
@@ -988,6 +998,9 @@ struct transport *transport_get(struct remote *remote,
const char *url)
ret->smart_options->receivepack = "git-receive-pack";
if (remote->receivepack)
ret->smart_options->receivepack = remote->receivepack;
+ if (remote->transport_version)
+ ret->smart_options->transport_version =
+ remote->transport_version;
}
return ret;
diff --git a/transport.h b/transport.h
index 18d2cf8..e07d356 100644
--- a/transport.h
+++ b/transport.h
@@ -14,6 +14,7 @@ struct git_transport_options {
unsigned update_shallow : 1;
unsigned push_cert : 1;
int depth;
+ int transport_version;
const char *uploadpack;
const char *receivepack;
struct push_cas_option *cas;
@@ -162,6 +163,9 @@ struct transport *transport_get(struct remote *, const char
*);
/* Send push certificates */
#define TRANS_OPT_PUSH_CERT "pushcert"
+/* Use a new version of the git protocol */
+#define TRANS_OPT_TRANSPORTVERSION "transportversion"
+
/**
* R
[RFC/WIP PATCH 09/11] transport: get_refs_via_connect exchanges capabilities before refs.
Signed-off-by: Stefan Beller
---
transport.c | 29 +
1 file changed, 25 insertions(+), 4 deletions(-)
diff --git a/transport.c b/transport.c
index 33644a6..1cd9b77 100644
--- a/transport.c
+++ b/transport.c
@@ -526,12 +526,33 @@ static struct ref *get_refs_via_connect(struct transport
*transport, int for_pus
{
struct git_transport_data *data = transport->data;
struct ref *refs;
+ int version = 0;
+ if (transport->smart_options)
+ version = transport->smart_options->transport_version;
connect_setup(transport, for_push, 0);
- get_remote_heads(data->fd[0], NULL, 0, &refs,
-for_push ? REF_NORMAL : 0,
-&data->extra_have,
-&data->shallow);
+ switch (version) {
+ default: /*
+ * Configured a protocol version > 2?
+ * Try version 2 as it's the most future proof.
+ */
+ /* fall through */
+ case 2: /* first talk about capabilities, then get the heads */
+ get_remote_capabilities(data->fd[0], NULL, 0);
+ request_capabilities(data->fd[1]);
+ get_remote_heads(data->fd[0], NULL, 0, &refs,
+for_push ? REF_NORMAL : 0,
+&data->extra_have,
+&data->shallow);
+ break;
+ case 1: /* configured version 1, fall through */
+ case 0: /* unconfigured, use first protocol */
+ get_remote_heads(data->fd[0], NULL, 0, &refs,
+for_push ? REF_NORMAL : 0,
+&data->extra_have,
+&data->shallow);
+ break;
+ }
data->got_remote_heads = 1;
return refs;
--
2.4.1.345.gab207b6.dirty
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[RFC/WIP PATCH 11/11] Document protocol version 2
Signed-off-by: Stefan Beller
---
Documentation/technical/pack-protocol.txt | 86 ---
Documentation/technical/protocol-capabilities.txt | 15
2 files changed, 77 insertions(+), 24 deletions(-)
diff --git a/Documentation/technical/pack-protocol.txt
b/Documentation/technical/pack-protocol.txt
index fc09c63..4e1c82e 100644
--- a/Documentation/technical/pack-protocol.txt
+++ b/Documentation/technical/pack-protocol.txt
@@ -1,11 +1,11 @@
Packfile transfer protocols
===
-Git supports transferring data in packfiles over the ssh://, git:// and
+Git supports transferring data in packfiles over the ssh://, git://, http://
and
file:// transports. There exist two sets of protocols, one for pushing
data from a client to a server and another for fetching data from a
-server to a client. All three transports (ssh, git, file) use the same
-protocol to transfer data.
+server to a client. The three transports (ssh, git, file) use the same
+protocol to transfer data. http is documented in http-protocol.txt.
The processes invoked in the canonical Git implementation are 'upload-pack'
on the server side and 'fetch-pack' on the client side for fetching data;
@@ -14,6 +14,12 @@ data. The protocol functions to have a server tell a client
what is
currently on the server, then for the two to negotiate the smallest amount
of data to send in order to fully update one or the other.
+"upload-pack-2" and "receive-pack-2" are the next generation of
+"upload-pack" and "receive-pack" respectively. The first two are
+referred as "version 2" in this document and pack-capabilities.txt
+while the last two are "version 1". Unless stated otherwise, "version 1"
+is implied.
+
Transports
--
There are three transports over which the packfile protocol is
@@ -42,7 +48,8 @@ hostname parameter, terminated by a NUL byte.
--
git-proto-request = request-command SP pathname NUL [ host-parameter NUL ]
- request-command = "git-upload-pack" / "git-receive-pack" /
+ request-command = "git-upload-pack" / "git-upload-pack-2" /
+ "git-receive-pack" / "git-receive-pack-2" /
"git-upload-archive" ; case sensitive
pathname = *( %x01-ff ) ; exclude NUL
host-parameter= "host=" hostname [ ":" port ]
@@ -124,6 +131,48 @@ has, the first can 'fetch' from the second. This
operation determines
what data the server has that the client does not then streams that
data down to the client in packfile format.
+Capability discovery (v2)
+-
+
+In version 1, capability discovery is part of reference discovery and
+covered in reference discovery section.
+
+In version 2, when the client initially connects, the server
+immediately sends its capabilities to the client followed by a flush.
+Then the client must send the list of server capabilities it wants to
+use to the server.
+
+ S: 00XXcapability:lang\n
+ S: 00XXcapability:thin-pack\n
+ S: 00XXcapability:ofs-delta\n
+ S: 00XXagent:agent=git/2:3.4.5+custom-739-gb850f98\n
+ S:
+
+ C: 00XXcap:thin-pack\n
+ C: 00XXcap:ofs-delta\n
+ C: 00XXcap:lang=en\n
+ C: 00XXagent:agent=git/custom_string\n
+ C:
+
+
+ capability-list = *(capability) [agent LF] flush-pkt
+ capability = PKT-LINE("capability:" keyvaluepair LF)
+ agent= keyvaluepair LF
+ keyvaluepair = 1*(LC_ALPHA / DIGIT / "-" / "_" / "=")
+ LC_ALPHA = %x61-7A
+
+
+The client MUST ignore any data on pkt-lines starting with anything
+different than "capability" for future ease of extension.
+
+The client MUST NOT ask for capabilities the server did not say it
+supports. The server MUST diagnose and abort if capabilities it does
+not understand was requested. The server MUST NOT ignore capabilities
+that client requested and server advertised. As a consequence of these
+rules, server MUST NOT advertise capabilities it does not understand.
+
+See protocol-capabilities.txt for a list of allowed server and client
+capabilities and descriptions.
Reference Discovery
---
@@ -154,10 +203,14 @@ If HEAD is a valid ref, HEAD MUST appear as the first
advertised
ref. If HEAD is not a valid ref, HEAD MUST NOT appear in the
advertisement list at all, but other refs may still appear.
-The stream MUST include capability declarations behind a NUL on the
-first ref. The peeled value of a ref (that is "ref^{}") MUST be
-immediately after the ref itself, if presented. A conforming server
-MUST peel the ref if it's an annotated tag.
+In version 1 the stream MUST include capability declarations behind
+a NUL on the first ref. The peeled value of a ref (that is "ref^{}")
+MUST be immediately after the ref itself, if presented. A conforming
+server MUST peel the ref if it's an annotated tag.
+
+In version 2 the capabilities are already negotiated, so the first ref
+MUST NOT be followed by any capability advertise
[RFC/WIP PATCH 10/11] t5544: add a test case for the new protocol
Signed-off-by: Stefan Beller
---
t/t5544-fetch-2.sh | 40
1 file changed, 40 insertions(+)
create mode 100755 t/t5544-fetch-2.sh
diff --git a/t/t5544-fetch-2.sh b/t/t5544-fetch-2.sh
new file mode 100755
index 000..beee46c
--- /dev/null
+++ b/t/t5544-fetch-2.sh
@@ -0,0 +1,40 @@
+#!/bin/sh
+#
+# Copyright (c) 2015 Stefan Beller
+#
+
+test_description='Testing version 2 of the fetch protocol'
+
+. ./test-lib.sh
+
+mk_repo_pair () {
+ rm -rf client server &&
+ test_create_repo client &&
+ test_create_repo server &&
+ (
+ cd server &&
+ git config receive.denyCurrentBranch warn
+ ) &&
+ (
+ cd client &&
+ git remote add origin ../server
+ git config remote.origin.transportversion 2
+ )
+}
+
+test_expect_success 'setup' '
+ mk_repo_pair &&
+ (
+ cd server &&
+ test_commit one
+ ) &&
+ (
+ cd client &&
+ git fetch origin master
+ )
+'
+
+# More to come here, similar to t5515 having a sub directory full of expected
+# data going over the wire.
+
+test_done
--
2.4.1.345.gab207b6.dirty
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[RFC/WIP PATCH 04/11] upload-pack-2: Implement the version 2 of upload-pack
In upload-pack-2 we send each capability in its own packet.
By reusing the advertise_capabilities and eventually setting it to
NULL we will be able to reuse the methods for refs advertisement.
Signed-off-by: Stefan Beller
---
.gitignore | 1 +
Makefile| 2 ++
upload-pack-2.c | 1 +
upload-pack.c | 77 ++---
4 files changed, 78 insertions(+), 3 deletions(-)
create mode 12 upload-pack-2.c
diff --git a/.gitignore b/.gitignore
index a052419..a3c8ab9 100644
--- a/.gitignore
+++ b/.gitignore
@@ -165,6 +165,7 @@
/git-update-server-info
/git-upload-archive
/git-upload-pack
+/git-upload-pack-2
/git-var
/git-verify-commit
/git-verify-pack
diff --git a/Makefile b/Makefile
index 25a453b..0f3ee41 100644
--- a/Makefile
+++ b/Makefile
@@ -560,6 +560,7 @@ PROGRAM_OBJS += sh-i18n--envsubst.o
PROGRAM_OBJS += shell.o
PROGRAM_OBJS += show-index.o
PROGRAM_OBJS += upload-pack.o
+PROGRAM_OBJS += upload-pack-2.o
PROGRAM_OBJS += remote-testsvn.o
# Binary suffix, set to .exe for Windows builds
@@ -625,6 +626,7 @@ OTHER_PROGRAMS = git$X
# what test wrappers are needed and 'install' will install, in bindir
BINDIR_PROGRAMS_NEED_X += git
BINDIR_PROGRAMS_NEED_X += git-upload-pack
+BINDIR_PROGRAMS_NEED_X += git-upload-pack-2
BINDIR_PROGRAMS_NEED_X += git-receive-pack
BINDIR_PROGRAMS_NEED_X += git-upload-archive
BINDIR_PROGRAMS_NEED_X += git-shell
diff --git a/upload-pack-2.c b/upload-pack-2.c
new file mode 12
index 000..e30a871
--- /dev/null
+++ b/upload-pack-2.c
@@ -0,0 +1 @@
+upload-pack.c
\ No newline at end of file
diff --git a/upload-pack.c b/upload-pack.c
index a5f75b7..84f9ee3 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -716,10 +716,47 @@ static void format_symref_info(struct strbuf *buf, struct
string_list *symref)
strbuf_addf(buf, " symref=%s:%s", item->string, (char
*)item->util);
}
-static const char *advertise_capabilities = "multi_ack thin-pack side-band"
+static char *advertise_capabilities = "multi_ack thin-pack side-band"
" side-band-64k ofs-delta shallow no-progress"
" include-tag multi_ack_detailed";
+/*
+ * Reads the next capability and puts it into dst as a null terminated string.
+ * Returns true if more capabilities can be read.
+ * */
+static int next_capability(char *dst)
+{
+ int len = 0;
+ if (!*advertise_capabilities) {
+ /* make sure to not advertise capabilities afterwards */
+ advertise_capabilities = NULL;
+ return 0;
+ }
+
+ while (advertise_capabilities[len] != '\0' &&
+ advertise_capabilities[len] != ' ')
+ len ++;
+ strncpy(dst, advertise_capabilities, len);
+ dst[len] = '\0';
+
+ advertise_capabilities += len;
+ if (*advertise_capabilities == ' ')
+ advertise_capabilities++;
+
+ return 1;
+}
+
+static void send_capabilities(void)
+{
+ char buf[100];
+
+ while (next_capability(buf))
+ packet_write(1, "capability:%s\n", buf);
+
+ packet_write(1, "agent:%s\n", git_user_agent_sanitized());
+ packet_flush(1);
+}
+
static int send_ref(const char *refname, const unsigned char *sha1, int flag,
void *cb_data)
{
@@ -794,6 +831,28 @@ static void upload_pack(void)
}
}
+static void receive_capabilities(void)
+{
+ int done = 0;
+ while (1) {
+ char *line = packet_read_line(0, NULL);
+ if (!line)
+ break;
+ if (starts_with(line, "capability:"))
+ parse_features(line + strlen("capability:"));
+ }
+}
+
+static void upload_pack_version_2(void)
+{
+ send_capabilities();
+ receive_capabilities();
+
+ /* The rest of the protocol stays the same, capabilities advertising
+ is disabled though. */
+ upload_pack();
+}
+
static int upload_pack_config(const char *var, const char *value, void *unused)
{
if (!strcmp("uploadpack.allowtipsha1inwant", var))
@@ -806,16 +865,24 @@ static int upload_pack_config(const char *var, const char
*value, void *unused)
return parse_hide_refs_config(var, value, "uploadpack");
}
+static int endswith(const char *teststring, const char *ending)
+{
+ int slen = strlen(teststring);
+ int elen = strlen(ending);
+ return !strcmp(teststring + slen - elen, ending);
+}
+
int main(int argc, char **argv)
{
char *dir;
+ const char *cmd;
int i;
int strict = 0;
git_setup_gettext();
packet_trace_identity("upload-pack");
- git_extract_argv0_path(argv[0]);
+ cmd = git_extract_argv0_path(argv[0]);
check_replace_refs = 0;
for (i = 1; i < argc; i++) {
@@ -857,6 +924,10 @@ int main(int argc, char **argv)
die("'%s' does not appear to be a git repository", dir);
git_config(upload_pack_confi
Re: What's cooking in git.git (May 2015, #07; Tue, 26)
> * sb/submodule-doc-intro (2015-05-22) 1 commit > - submodule documentation: reorder introductory paragraphs > > What's the doneness of this one??? I'll try again without a "This command will do ..." introduction, I just did not look into it yet. That said, I was expecting more bike shedding than usual as it's a documentation only change, where it's harder to come up with clean and readable text as compared to code. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/WIP PATCH 02/11] upload-pack: only accept capabilities on the first "want" line
Stefan Beller writes: > From: Nguyễn Thái Ngọc Duy > > pack-protocol.txt says so and fetch-pack also follows it even though > upload-pack is a bit lax. Fix it. Hmm, I actually think the .txt file unsuccessfully tried to close the barn door after horse has long left. The existing clients that read protocol capabilities keep the last-read copy and then overwrite it when a new one came, which is why we couldn't update the protocol by sending only incremental changes, etc. without breaking existing clients. I somehow thought that we already discussed why this was bad the first time Duy's patch was posted, but apparently we didn't. X-<. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/WIP PATCH 07/11] fetch-pack: use the configured transport protocol
Stefan Beller writes:
> @@ -175,7 +179,18 @@ int cmd_fetch_pack(int argc, const char **argv, const
> char *prefix)
> if (!conn)
> return args.diag_url ? 0 : 1;
> }
> - get_remote_heads(fd[0], NULL, 0, &ref, 0, NULL, &shallow);
> +
> + switch (args.version) {
> + default:
> + case 2:
> + get_remote_capabilities(fd[0], NULL, 0);
> + request_capabilities(fd[1]);
> + break;
> + case 1: /* fall through */
> + case 0:
> + get_remote_heads(fd[0], NULL, 0, &ref, 0, NULL, &shallow);
> + break;
> + }
Nice ;-)
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/WIP PATCH 02/11] upload-pack: only accept capabilities on the first "want" line
On Tue, May 26, 2015 at 3:17 PM, Junio C Hamano wrote: > Stefan Beller writes: > >> From: Nguyễn Thái Ngọc Duy >> >> pack-protocol.txt says so and fetch-pack also follows it even though >> upload-pack is a bit lax. Fix it. > > Hmm, I actually think the .txt file unsuccessfully tried to close > the barn door after horse has long left. The existing clients that > read protocol capabilities keep the last-read copy and then > overwrite it when a new one came, which is why we couldn't update > the protocol by sending only incremental changes, etc. without > breaking existing clients. On the other hand I am not aware of any remote implementation handing out capabilities after the first line, so this would not break anything to my knowledge. > > I somehow thought that we already discussed why this was bad the > first time Duy's patch was posted, but apparently we didn't. /me checks the archive. I may have overlooked that part. :( > > X-<. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/WIP PATCH 08/11] transport: connect_setup appends protocol version number
Stefan Beller writes:
> + if (transport->smart_options
> + && transport->smart_options->transport_version) {
> + buf = xmalloc(strlen(remote_program) + 12);
> + sprintf(buf, "%s-%d", remote_program,
> + transport->smart_options->transport_version);
> + remote_program = buf;
> + }
Bikeshedding: so the versioning scheme is that the current one is
zero, and the next one is two? I would have expected that there
would be something like
if (...->version < 2) {
... append "-%d" ...
}
involved.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/WIP PATCH 07/11] fetch-pack: use the configured transport protocol
On Tue, May 26, 2015 at 3:19 PM, Junio C Hamano wrote:
> Stefan Beller writes:
>
>> @@ -175,7 +179,18 @@ int cmd_fetch_pack(int argc, const char **argv, const
>> char *prefix)
>> if (!conn)
>> return args.diag_url ? 0 : 1;
>> }
>> - get_remote_heads(fd[0], NULL, 0, &ref, 0, NULL, &shallow);
>> +
>> + switch (args.version) {
>> + default:
>> + case 2:
>> + get_remote_capabilities(fd[0], NULL, 0);
>> + request_capabilities(fd[1]);
>> + break;
Actually this is wrong, we need to actually fall through from here as well,
so we not only talk capabilities negotiation, but then continue
with get_remote_heads.
>> + case 1: /* fall through */
>> + case 0:
>> + get_remote_heads(fd[0], NULL, 0, &ref, 0, NULL, &shallow);
>> + break;
>> + }
>
> Nice ;-)
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/WIP PATCH 08/11] transport: connect_setup appends protocol version number
On Tue, May 26, 2015 at 3:21 PM, Junio C Hamano wrote:
> Stefan Beller writes:
>
>> + if (transport->smart_options
>> + && transport->smart_options->transport_version) {
>> + buf = xmalloc(strlen(remote_program) + 12);
>> + sprintf(buf, "%s-%d", remote_program,
>> + transport->smart_options->transport_version);
>> + remote_program = buf;
>> + }
>
> Bikeshedding: so the versioning scheme is that the current one is
> zero, and the next one is two? I would have expected that there
> would be something like
I think currently we have version 1. But we don't advertise it, so
I'll call it 0
(the default or unadvertised or however you name it. 0 as in "unsure" maybe)
I thought about future proofing this version a bit. Say version 2 is bad because
I don't have the experience of 10 years Git nor of maintaining large
projects and
you want to make a version 3 soon. And this would support that just fine.
The meaning being: Any version except 0 should have a dedicated
extension -${version}
The 0 is left out for backwards compatibility.
So in a later patch where we want to introduce force-using of old
versions you could
configure upload-pack to be explicit upload-pack-1. The upload-pack-1
version is not
yet there with this series though.
>
> if (...->version < 2) {
> ... append "-%d" ...
> }
>
> involved.
Oh! I see here you would count the current one as 1, which has no
number extension,
and any further would have a -${version}. That would transport the
intention much better
I guess.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/RFC 1/2] git-rebase -i: Add key word "drop" to remove a commit
On Tue, May 26, 2015 at 5:38 PM, Galan Rémi wrote: > git-rebase -i: Add key word "drop" to remove a commit "key word" is unusual. More typical is "keyword". However, perhaps "command" might be even better. Also, custom on this project is not to capitalize, so: git-rebase -i: add command "drop" to remove a commit > Instead of removing a line to remove the commit, you can use the key > word "drop" (just like "pick" or "edit"). It has the same effect as > deleting the line (removing the commit) except that you keep a visual > trace of your actions, allowing a better control and reducing the > possibility of removing a commit by mistake. Nicely explained. Ditto regarding "key word". > Signed-off-by: Galan Rémi > --- > diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt > index 1d01baa..3cd2ef2 100644 > --- a/Documentation/git-rebase.txt > +++ b/Documentation/git-rebase.txt > @@ -514,6 +514,9 @@ rebasing. > If you just want to edit the commit message for a commit, replace the > command "pick" with the command "reword". > > +If you want to remove a commit, replace the command "pick" by the > +command "drop". I think the existing method of removing a commit merits mention here. Perhaps: To drop a commit, delete its line or replace the command "pick" with "drop". Or, if you want to emphasize "drop": To drop a commit, replace the command "pick" with "drop", or just delete its line. > If you want to fold two or more commits into one, replace the command > "pick" for the second and subsequent commits with "squash" or "fixup". > If the commits had different authors, the folded commit will be > diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh > index dc3133f..cb749e8 100644 > --- a/git-rebase--interactive.sh > +++ b/git-rebase--interactive.sh -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/RFC 2/2] git rebase -i: Warn removed or dupplicated commits
On Tue, May 26, 2015 at 5:38 PM, Galan Rémi
wrote:
> git rebase -i: Warn removed or dupplicated commits
s/dupplicated/duplicated/
Also, drop capitalization, and insert "about":
git rebase -i: warn about removed or duplicated commits
> Check if commits were removed (i.e. a line was deleted) or dupplicated
s/dupplicated/duplicated/
> (e.g. the same commit is picked twice), can print warnings or abort
s/can/and/, I think.
> git rebase according to the value of the configuration variable
> rebase.checkLevel.
>
> Add the configuration variable rebase.checkLevel.
> - When unset or set to "IGNORED", no checking is done.
s/IGNORED/IGNORE/
> - When set to "WARN", the commits are checked, warnings are
> displayed but git rebase still proceeds.
> - When set to "ERROR", the commits are checked, warnings are
> displayed and the rebase is aborted.
Why uppercase for these names? Is there precedence for that? I think
lowercase is more common.
> Signed-off-by: Galan Rémi
> ---
> This part of the patch has no test yet, it is more for rfc.
>
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index d44bc85..2152e27 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -2204,6 +2204,14 @@ rebase.autoStash::
> successful rebase might result in non-trivial conflicts.
> Defaults to false.
>
> +rebase.checkLevel::
> + If set to "warn", git rebase -i will print a warning if some
> + commits are removed (i.e. a line was deleted) or if some
> + commits appear more than one time (e.g. the same commit is
> + picked twice), however the rebase will still proceed. If set
> + to "error", it will print the previous warnings and abort the
> + rebase.
The commit message talks about "ignore", but there is no mention here.
Also, what is the default behavior if not specified? That should be documented.
Finally, this talks about lowercase "warn" and "error", whereas the
commit message uses upper case "WARN" and "ERROR", as does the code.
Why the inconsistency?
> receive.advertiseAtomic::
> By default, git-receive-pack will advertise the atomic push
> capability to its clients. If you don't want to this capability
> diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
> index 3cd2ef2..cb05cbb 100644
> --- a/Documentation/git-rebase.txt
> +++ b/Documentation/git-rebase.txt
> @@ -213,6 +213,11 @@ rebase.autoSquash::
> rebase.autoStash::
> If set to true enable '--autostash' option by default.
>
> +rebase.checkLevel::
> + If set to "warn" print warnings about removed commits and
> + duplicated commits in interactive mode. If set to "error"
> + print the warnings and abort the rebase. No check by default.
Ditto: Fails to mention "ignore".
> OPTIONS
> ---
> --onto ::
> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> index cb749e8..8a837ca 100644
> --- a/git-rebase--interactive.sh
> +++ b/git-rebase--interactive.sh
> @@ -837,6 +837,80 @@ add_exec_commands () {
> mv "$1.new" "$1"
> }
>
> +# Print the list of the sha-1 of the commits
> +# from a todo list in a file.
> +# $1 : todo-file, $2 : outfile
> +todo_list_to_sha_list () {
> + todo_list=$(git stripspace --strip-comments < "$1")
> + temp_file=$(mktemp)
> + echo "$todo_list" > "$temp_file"
> + while read -r command sha1 rest < "$temp_file"
On this project it is typical to drop the space after redirection
operators (<, >, >>), however, git-rebase--interactive.sh is filled
with both styles (space and no space after redirection). New code
probably ought to drop the space.
> + do
> + case "$command" in
> + x|"exec")
> + ;;
> + *)
> + echo "$sha1" >> "$2"
> + ;;
> + esac
> + sed -i '1d' "$temp_file"
> + done
> + rm "$temp_file"
> +}
> +
> +# Check if the user dropped some commits by mistake
> +# or if there are two identical commits.
> +# Behaviour determined by .gitconfig.
> +check_commits () {
> + checkLevel=$(git config --get rebase.checkLevel)
> + checkLevel=${checkLevel:-"IGNORE"}
Minor aside: Unnecessary quoting increases the noise level, thus
making the code slightly more difficult to read. This could just as
well have been:
checkLevel=${checkLevel:-IGNORE}
There are plenty of other places throughout this patch which exhibit
the same shortcoming, but I won't point them out individually.
> + # To uppercase
> + checkLevel=$(echo "$checkLevel" | tr '[:lower:]' '[:upper:]')
Is there precedence elsewhere for recognizing uppercase and lowercase
variants of config values?
> + case "$checkLevel" in
> + "WARN"|"ERROR")
> + todo_list_to_sha_list "$todo".backup "$todo".oldsha1
> + todo_list_to_sha_list "$todo" "$todo".n
Re: [PATCH v3 00/56] Convert parts of refs.c to struct object_id
On Tue, May 26, 2015 at 10:37:29AM -0700, Stefan Beller wrote: > On Mon, May 25, 2015 at 12:40 PM, brian m. carlson > wrote: > > On Mon, May 25, 2015 at 12:34:59PM -0700, Junio C Hamano wrote: > >> [PATCH 01/56] was authored by you but has Michael's sign-off, which > >> looked somewhat odd to me, though. > > > > Yes, it does. He picked it up from me, and signed off, and I took his > > branch. I don't believe he changed it, but I didn't check for certain. > > So technically, although I wrote it, I also received it from him without > > changing it, so both (a) and (c) of the DCO apply. > > At least in the kernel, the sign offs are also used to track a patchs way > of life, so essentially whenever somebody touches that patch (either as > an author, or as a patch shoveling sub Lieutenant), you'd add a sign off. > > So if we were to handle the sign offs just as the kernel people, I would > have assumed a sign-off block like > > Sign-off: you > Sign off: Michael > Sign-off: you > > as that would indicate that Michael did not author it from scratch but > based his work on yours. That's just my understanding of the sign off > process for linux and I guessed we'd follow a very similar process. But > no objections from me regarding the signing. If Junio would like to add my sign-off to the end, he's welcome to do so: Signed-off-by: brian m. carlson -- brian m. carlson / brian with sandals: Houston, Texas, US +1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only OpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187 signature.asc Description: Digital signature
Re: [RFC/WIP PATCH 04/11] upload-pack-2: Implement the version 2 of upload-pack
On Tue, May 26, 2015 at 6:01 PM, Stefan Beller wrote:
> In upload-pack-2 we send each capability in its own packet.
> By reusing the advertise_capabilities and eventually setting it to
> NULL we will be able to reuse the methods for refs advertisement.
>
> Signed-off-by: Stefan Beller
> ---
> diff --git a/upload-pack-2.c b/upload-pack-2.c
> new file mode 12
> index 000..e30a871
> --- /dev/null
> +++ b/upload-pack-2.c
> @@ -716,10 +716,47 @@ static void format_symref_info(struct strbuf *buf,
> struct string_list *symref)
> strbuf_addf(buf, " symref=%s:%s", item->string, (char
> *)item->util);
> }
>
> -static const char *advertise_capabilities = "multi_ack thin-pack side-band"
> +static char *advertise_capabilities = "multi_ack thin-pack side-band"
> " side-band-64k ofs-delta shallow no-progress"
> " include-tag multi_ack_detailed";
>
> +/*
> + * Reads the next capability and puts it into dst as a null terminated
> string.
> + * Returns true if more capabilities can be read.
> + * */
> +static int next_capability(char *dst)
> +{
> + int len = 0;
> + if (!*advertise_capabilities) {
> + /* make sure to not advertise capabilities afterwards */
> + advertise_capabilities = NULL;
You set advertise_capabilities to NULL here but then unconditionally
dereference that NULL in the if-statement just above (if someone calls
next_capability() again). Seems fishy (and crashy) to me. Either don't
dereference the NULL or don't bother setting it to NULL (in which
case, you'll dereference and find '\0', which should serve the same
purpose).
> + return 0;
> + }
> +
> + while (advertise_capabilities[len] != '\0' &&
> + advertise_capabilities[len] != ' ')
> + len ++;
Style: len++
> + strncpy(dst, advertise_capabilities, len);
> + dst[len] = '\0';
> +
> + advertise_capabilities += len;
> + if (*advertise_capabilities == ' ')
> + advertise_capabilities++;
If for some reason, someone happens to add an extra space between
capabilities in advertise_capabilities, then the capability returned
by the next invocation of next_capability() be zero-length, which is
probably undesirable, and certainly not expected by the caller.
Skipping whitespace in a loop would be more robust.
> + return 1;
> +}
I realize that this is RFC, but my overall reaction to
next_capability() in its current form is that it's ugly . A somewhat
cleaner approach would be to pass some state into next_capability()
and have it modify that state rather than the global
advertise_capabilities. The passed-in state could be as simple as a
'const char *' which initially points at the start of
advertise_capabilities and then gets advanced; until, finally, it
points at the '\0' at the end of advertise_capabilities.
> +static void send_capabilities(void)
> +{
> + char buf[100];
> +
> + while (next_capability(buf))
> + packet_write(1, "capability:%s\n", buf);
> +
> + packet_write(1, "agent:%s\n", git_user_agent_sanitized());
> + packet_flush(1);
> +}
> +
> static int send_ref(const char *refname, const unsigned char *sha1, int
> flag, void *cb_data)
> {
>
> @@ -794,6 +831,28 @@ static void upload_pack(void)
> }
> }
>
> +static void receive_capabilities(void)
> +{
> + int done = 0;
> + while (1) {
> + char *line = packet_read_line(0, NULL);
If you declare this 'const char *', then it becomes much more obvious
that it's not your responsibility to free() the result (and,
tangentially, that you have no intention of modifying the content).
> + if (!line)
> + break;
> + if (starts_with(line, "capability:"))
> + parse_features(line + strlen("capability:"));
See skip_prefix().
> + }
> +}
> +
> +static void upload_pack_version_2(void)
> +{
> + send_capabilities();
> + receive_capabilities();
> +
> + /* The rest of the protocol stays the same, capabilities advertising
> + is disabled though. */
/*
* This is a multi-line
* comment.
*/
> + upload_pack();
> +}
> +
> static int upload_pack_config(const char *var, const char *value, void
> *unused)
> {
> if (!strcmp("uploadpack.allowtipsha1inwant", var))
> @@ -806,16 +865,24 @@ static int upload_pack_config(const char *var, const
> char *value, void *unused)
> return parse_hide_refs_config(var, value, "uploadpack");
> }
>
> +static int endswith(const char *teststring, const char *ending)
> +{
> + int slen = strlen(teststring);
> + int elen = strlen(ending);
You may be confident today that no callers are supplying an 'ending'
which is longer than 'teststring', but someone may some day break that
assumption. At the very least, for robustness, add an assert() or
die() if 'elen' is greater than 'slen'.
Re: [RFC/WIP PATCH 06/11] remote.h: add get_remote_capabilities, request_capabilities
On Tue, May 26, 2015 at 6:01 PM, Stefan Beller wrote:
> Instead of calling get_remote_heads as a first command during the
> protocol exchange, we need to have fine grained control over the
> capability negotiation in version 2 of the protocol.
>
> Introduce get_remote_capabilities, which will just listen to
> capabilities of the remote and request_capabilities which will
> tell the selection of capabilities to the remote.
>
> Signed-off-by: Stefan Beller
> ---
> diff --git a/connect.c b/connect.c
> index c0144d8..bb17618 100644
> --- a/connect.c
> +++ b/connect.c
> @@ -105,8 +105,54 @@ static void annotate_refs_with_symref_info(struct ref
> *ref)
> string_list_clear(&symref, 0);
> }
>
> +void get_remote_capabilities(int in, char *src_buf, size_t src_len)
> +{
> + struct strbuf capabilities_string = STRBUF_INIT;
> + for (;;) {
> + int len;
> + char *line = packet_buffer;
> + const char *arg;
> +
> + len = packet_read(in, &src_buf, &src_len,
> + packet_buffer, sizeof(packet_buffer),
> + PACKET_READ_GENTLE_ON_EOF |
> + PACKET_READ_CHOMP_NEWLINE);
> + if (len < 0)
> + die_initial_contact(0);
> +
> + if (!len)
> + break;
> +
> + if (len > 4 && skip_prefix(line, "ERR ", &arg))
The 'len > 4' check is needed because there's no guarantee that 'line'
is NUL-terminated. Correct?
> + die("remote error: %s", arg);
> +
> + if (starts_with(line, "capability:")) {
> + strbuf_addstr(&capabilities_string, line +
> strlen("capability:"));
skip_prefix() maybe?
> + strbuf_addch(&capabilities_string, ' ');
> + }
> + }
> + free(server_capabilities);
> + server_capabilities = xmalloc(capabilities_string.len + 1);
> + server_capabilities = strbuf_detach(&capabilities_string, NULL);
So, you're allocating a buffer for server_capabilities and then
immediately throwing away (leaking) that buffer. Seems fishy.
> + strbuf_release(&capabilities_string);
Not outright incorrect, but you've just detached capabilities_string's
buffer, so releasing it doesn't buy anything.
> +}
> +
> +int request_capabilities(int out)
> +{
> + fprintf(stderr, "request_capabilities\n");
> + // todo: send our capabilities
> + packet_write(out, "capability:foo");
> + packet_flush(out);
> +}
> +
> /*
> - * Read all the refs from the other end
> + * Read all the refs from the other end,
> + * in is a file descriptor input stream
> + * src_buf and src_len may be an alternative way to specify the input.
The bit about src_buf and src_len conveys little or nothing. After
reading it, I'm as clueless to their purpose as I would be if they
were undocumented.
> + * list is the output of refs
> + * extra_have is a list to store information learned about sha1 the other
> side has.
> + * shallow_points
'shallow_points' what?
> */
> struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,
> struct ref **list, unsigned int flags,
> diff --git a/remote.h b/remote.h
> index 04e2310..7381ddf 100644
> --- a/remote.h
> +++ b/remote.h
> @@ -146,6 +146,9 @@ int check_ref_type(const struct ref *ref, int flags);
> void free_refs(struct ref *ref);
>
> struct sha1_array;
> +
> +extern void get_remote_capabilities(int in, char *src_buf, size_t src_len);
> +extern int request_capabilities(int out);
> extern struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,
> struct ref **list, unsigned int flags,
> struct sha1_array *extra_have,
> --
> 2.4.1.345.gab207b6.dirty
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/WIP PATCH 08/11] transport: connect_setup appends protocol version number
On Tue, May 26, 2015 at 6:01 PM, Stefan Beller wrote:
> Signed-off-by: Stefan Beller
> ---
> diff --git a/transport.c b/transport.c
> index 3ef15f6..33644a6 100644
> --- a/transport.c
> +++ b/transport.c
> @@ -496,15 +496,29 @@ static int set_git_option(struct git_transport_options
> *opts,
> static int connect_setup(struct transport *transport, int for_push, int
> verbose)
> {
> struct git_transport_data *data = transport->data;
> + const char *remote_program;
> + char *buf = 0;
Naming this 'to_free' would make its purpose more obvious[1], and be
more consistent with code elsewhere in the project.
[1]: http://article.gmane.org/gmane.comp.version-control.git/256125/
> if (data->conn)
> return 0;
>
> + remote_program = (for_push ? data->options.receivepack
> + : data->options.uploadpack);
> +
> + if (transport->smart_options
> + && transport->smart_options->transport_version) {
> + buf = xmalloc(strlen(remote_program) + 12);
> + sprintf(buf, "%s-%d", remote_program,
> + transport->smart_options->transport_version);
> + remote_program = buf;
> + }
> +
> data->conn = git_connect(data->fd, transport->url,
> -for_push ? data->options.receivepack :
> -data->options.uploadpack,
> +remote_program,
> verbose ? CONNECT_VERBOSE : 0);
>
> + free(buf);
> +
> return 0;
> }
>
> --
> 2.4.1.345.gab207b6.dirty
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/WIP PATCH 08/11] transport: connect_setup appends protocol version number
Stefan Beller writes:
> On Tue, May 26, 2015 at 3:21 PM, Junio C Hamano wrote:
>
>>
>> if (...->version < 2) {
>> ... append "-%d" ...
>> }
>>
>> involved.
>
> Oh! I see here you would count the current one as 1, which has no
> number extension, and any further would have a -${version}. That
> would transport the intention much better I guess.
Yeah, except that I screwed up my comparison. Obviously, I should
have said "If version is 2 or later, then append -%d to the name,
otherwise use the name as-is".
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 00/56] Convert parts of refs.c to struct object_id
"brian m. carlson" writes: > If Junio would like to add my sign-off to the end, he's welcome to do > so: > > Signed-off-by: brian m. carlson Heh, too late. Thanks for explaining the true flow of patches, though. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/WIP PATCH 10/11] t5544: add a test case for the new protocol
On Tue, May 26, 2015 at 6:01 PM, Stefan Beller wrote:
> Signed-off-by: Stefan Beller
> ---
> diff --git a/t/t5544-fetch-2.sh b/t/t5544-fetch-2.sh
> new file mode 100755
> index 000..beee46c
> --- /dev/null
> +++ b/t/t5544-fetch-2.sh
> @@ -0,0 +1,40 @@
> +#!/bin/sh
> +#
> +# Copyright (c) 2015 Stefan Beller
> +#
> +
> +test_description='Testing version 2 of the fetch protocol'
> +
> +. ./test-lib.sh
> +
> +mk_repo_pair () {
> + rm -rf client server &&
> + test_create_repo client &&
> + test_create_repo server &&
> + (
> + cd server &&
> + git config receive.denyCurrentBranch warn
> + ) &&
> + (
> + cd client &&
> + git remote add origin ../server
Broken &&-chain.
> + git config remote.origin.transportversion 2
> + )
> +}
> +
> +test_expect_success 'setup' '
> + mk_repo_pair &&
> + (
> + cd server &&
> + test_commit one
> + ) &&
> + (
> + cd client &&
> + git fetch origin master
> + )
> +'
> +
> +# More to come here, similar to t5515 having a sub directory full of expected
> +# data going over the wire.
> +
> +test_done
> --
> 2.4.1.345.gab207b6.dirty
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/WIP PATCH 09/11] transport: get_refs_via_connect exchanges capabilities before refs.
On Tue, May 26, 2015 at 6:01 PM, Stefan Beller wrote:
> transport: get_refs_via_connect exchanges capabilities before refs.
s/exchanges/exchange/
s/\.$//
> Signed-off-by: Stefan Beller
> ---
> transport.c | 29 +
> 1 file changed, 25 insertions(+), 4 deletions(-)
>
> diff --git a/transport.c b/transport.c
> index 33644a6..1cd9b77 100644
> --- a/transport.c
> +++ b/transport.c
> @@ -526,12 +526,33 @@ static struct ref *get_refs_via_connect(struct
> transport *transport, int for_pus
> {
> struct git_transport_data *data = transport->data;
> struct ref *refs;
> + int version = 0;
>
> + if (transport->smart_options)
> + version = transport->smart_options->transport_version;
> connect_setup(transport, for_push, 0);
> - get_remote_heads(data->fd[0], NULL, 0, &refs,
> -for_push ? REF_NORMAL : 0,
> -&data->extra_have,
> -&data->shallow);
> + switch (version) {
> + default: /*
> + * Configured a protocol version > 2?
> + * Try version 2 as it's the most future proof.
> + */
> + /* fall through */
> + case 2: /* first talk about capabilities, then get the heads
> */
> + get_remote_capabilities(data->fd[0], NULL, 0);
> + request_capabilities(data->fd[1]);
> + get_remote_heads(data->fd[0], NULL, 0, &refs,
> +for_push ? REF_NORMAL : 0,
> +&data->extra_have,
> +&data->shallow);
> + break;
> + case 1: /* configured version 1, fall through */
> + case 0: /* unconfigured, use first protocol */
> + get_remote_heads(data->fd[0], NULL, 0, &refs,
> +for_push ? REF_NORMAL : 0,
> +&data->extra_have,
> +&data->shallow);
> + break;
> + }
> data->got_remote_heads = 1;
>
> return refs;
> --
> 2.4.1.345.gab207b6.dirty
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/WIP PATCH 00/11] Protocol version 2, again!
On Tue, May 26, 2015 at 03:01:04PM -0700, Stefan Beller wrote: > "Just give us something to play around with" - Peff at GitMerge 2015 Sounds like something I would say. > The new protocol works just like the old protocol, except for > the capabilities negotiation being before any exchange of real data. I like this approach. We all know that one next step is going to be a "show me only these refs" capability negotiation of some kind. But it's good to keep the version-breaking changes separated from that, and this should be enough to bootstrap that conversation later. If I understand correctly, your proposed protocol allows for a single back-and-forth of capabilities followed by the server speaking the ref advertisement. So it is worth thinking a moment how we might have a more involved conversation before the advertisement if we want to do so in the future. I think in the simplest case, the server claims to understand the "foo" extension, and then the client says "I wish to use foo". And then a rule of "foo" might be that the conversation continues in some way before sending the advertisement. Like (each line is a pkt-line): ... S: foo S: flush ... C: foo S: Here's some extra foo data. C: Now I respond to that foo data. S: Now the foo conversation is done. Here's the ref advertisement. What if there are multiple such extensions? E.g., if the client asks for both "foo" and "bar", and both require extra back-and-forth messages? Which conversation happens first? Maybe the rule is just "whichever one the client asked for first in the capabilities list". And I think in general we want to avoid protocol round-trips if we can (so we'd prefer the client say "refspec=refs/heads/*", and not "I understand refspecs, too! Let's have a conversation about which ones I'm interested in."). But I think it's worth giving it a little thought to make sure we don't paint ourselves in a corner. > My preference for a string suffix instead of a sequential number is > motiviated by the discussion of sha1 vs sequential numbers to describe > a state of a repository. The main difference here is however how often > we expect changes. Version 1 of the protocol is current for 10 years by > now, so I do not changes to the protocol number often, which makes it > suitable for just having a counter appended to the binary. I think I prefer a number. I'm really hoping that v2 lasts even longer than v1 has, because the capability negotiation should allow us to extend it from within rather than breaking compatibility. As a minor nit, I think I like "upload-pack-v2" better than "upload-pack-2". But I can live with it either way. :) > This series doesn't include an automatic upgrade of the protocol version > for later changes if the server supports it, so for now the use of the new > protocol needs to be activated manually via setting > remote.origin.transportversion. I think that's a good start. Last time we discussed it, I think everybody was more or less on board with client probing (so v1 would start to say "btw, I speak v2", and then client would set its remote.origin.transportversion flag). That can come later, and we are not painting ourselves into a corner. I think we also discussed passing the version information out-of-band. So over git-daemon, as "git-upload-pack repo\0host=...\0\0version=2", for example. I _think_ we are also fine to build that on top of what you have here. If the version information makes it through to upload-pack, then we can do v2, and if not, we are no worse off than we were. HTTP can do a similar out-of-band thing, but I think ssh is mostly out of luck. The best I could think of was passing an environment variable, but ssh typically only lets through a few variables. We could abuse PATH or something, but that's getting pretty nasty. Anyway, that is all for the future. The only reason I mention it is to make sure that we are not closing any future doors, and I don't think we are. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/RFC 1/2] git-rebase -i: Add key word "drop" to remove a commit
Hi Rémi,
On 2015-05-26 23:38, Galan Rémi wrote:
> Instead of removing a line to remove the commit, you can use the key
> word "drop" (just like "pick" or "edit"). It has the same effect as
> deleting the line (removing the commit) except that you keep a visual
> trace of your actions, allowing a better control and reducing the
> possibility of removing a commit by mistake.
Please note that you can already just comment-out the line if you need to keep
a visual trace.
Alternatively, you can replace the `pick` command by `noop`.
If you really need the `drop` command (with which I am not 100% happy because I
already envisage users appending a `drop A` to an edit script "pick A; pick B;
pick C" and expecting A *not to be picked*), then it is better to just add the
`drop` part to the already existing `noop` clause:
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index f7deeb0..8355be8 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -489,7 +489,7 @@ do_next () {
rm -f "$msg" "$author_script" "$amend" || exit
read -r command sha1 rest < "$todo"
case "$command" in
- "$comment_char"*|''|noop)
+ "$comment_char"*|''|noop|drop)
mark_action_done
;;
pick|p)
Ciao,
Johannes
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 0/4] showing existing ws breakage
We paint whitespace breakages in new (i.e. added or updated) lines when showing the "git diff" output to help people avoid introducing them with their changes. The basic premise is that people would want to avoid touching existing lines only to fix whitespace errors in a patch that does other changes of substance, and that is why we traditionally did not paint whitespace breakages in existing (i.e. deleted or context) lines. However, some people would want to keep existing breakages when they are doing other changes of substance; "new" lines in such a patch would show existing whitespace breakages painted, and it is not apparent if the breakages were inherited from the original or newly introduced. Christian Brabandt had an interesting idea to help users in this situation; why not give them a mode to paint whitespace breakages in "old" (i.e. deleted or was replaced) lines, too? http://thread.gmane.org/gmane.comp.version-control.git/269912/focus=269956 This series is a reroll of the previous one http://thread.gmane.org/gmane.comp.version-control.git/269972 which built on Christian'sidea, but with a different implementation (Christian's original painted trailing whitespaces only). The first two patches are unchanged since v2; they are preliminary clean-ups. The third one extends the corresponding patch since v2; not only a helper for "deleted" lines but also another helper for "context" lines is added. The fourth one in v2 used a new option "--[no-]ws-check-deleted", but in this round a new option "--ws-error-highlight=" is defined instead. With that, you can say diff --ws-error-highlight=new,old to say "I want to see whitespace errors on new and old lines", and diff --ws-error-highlight=new,old,context diff --ws-error-highlight=all can be used to also see whitespace errors on context lines. Being able to see whitespace errors on context lines, i.e. the ones that were there in the original and you left intact, would help you see how prevalent whitespace errors are in the original and hopefully would make it easier for you to decide if a separate preliminary clean-up to only fix these whitespace errors is warranted. Junio C Hamano (4): t4015: modernise style t4015: separate common setup and per-test expectation diff.c: add emit_del_line() and emit_context_line() diff.c: --ws-error-highlight= option Documentation/diff-options.txt | 10 + diff.c | 122 -- diff.h | 5 + t/t4015-diff-whitespace.sh | 508 ++--- 4 files changed, 385 insertions(+), 260 deletions(-) -- 2.4.2-503-g3e4528a -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 3/4] diff.c: add emit_del_line() and emit_context_line()
Traditionally, we only had emit_add_line() helper, which knows how
to find and paint whitespace breakages on the given line, because we
only care about whitespace breakages introduced in new lines. The
context lines and old (i.e. deleted) lines are emitted with a
simpler emit_line_0() that paints the entire line in plain or old
colors.
Identify callers of emit_line_0() that show deleted lines and
context lines, have them call new helpers, emit_del_line() and
emit_context_line(), so that we can later tweak what is done to
these two classes of lines.
Signed-off-by: Junio C Hamano
---
diff.c | 50 ++
1 file changed, 38 insertions(+), 12 deletions(-)
diff --git a/diff.c b/diff.c
index d1bd534..c575c45 100644
--- a/diff.c
+++ b/diff.c
@@ -498,6 +498,24 @@ static void emit_add_line(const char *reset,
}
}
+static void emit_del_line(const char *reset,
+ struct emit_callback *ecbdata,
+ const char *line, int len)
+{
+ const char *set = diff_get_color(ecbdata->color_diff, DIFF_FILE_OLD);
+
+ emit_line_0(ecbdata->opt, set, reset, '-', line, len);
+}
+
+static void emit_context_line(const char *reset,
+ struct emit_callback *ecbdata,
+ const char *line, int len)
+{
+ const char *set = diff_get_color(ecbdata->color_diff, DIFF_PLAIN);
+
+ emit_line_0(ecbdata->opt, set, reset, ' ', line, len);
+}
+
static void emit_hunk_header(struct emit_callback *ecbdata,
const char *line, int len)
{
@@ -603,7 +621,6 @@ static void emit_rewrite_lines(struct emit_callback *ecb,
{
const char *endp = NULL;
static const char *nneof = " No newline at end of file\n";
- const char *old = diff_get_color(ecb->color_diff, DIFF_FILE_OLD);
const char *reset = diff_get_color(ecb->color_diff, DIFF_RESET);
while (0 < size) {
@@ -613,8 +630,7 @@ static void emit_rewrite_lines(struct emit_callback *ecb,
len = endp ? (endp - data + 1) : size;
if (prefix != '+') {
ecb->lno_in_preimage++;
- emit_line_0(ecb->opt, old, reset, '-',
- data, len);
+ emit_del_line(reset, ecb, data, len);
} else {
ecb->lno_in_postimage++;
emit_add_line(reset, ecb, data, len);
@@ -1250,17 +1266,27 @@ static void fn_out_consume(void *priv, char *line,
unsigned long len)
return;
}
- if (line[0] != '+') {
- const char *color =
- diff_get_color(ecbdata->color_diff,
- line[0] == '-' ? DIFF_FILE_OLD :
DIFF_PLAIN);
- ecbdata->lno_in_preimage++;
- if (line[0] == ' ')
- ecbdata->lno_in_postimage++;
- emit_line(ecbdata->opt, color, reset, line, len);
- } else {
+ switch (line[0]) {
+ case '+':
ecbdata->lno_in_postimage++;
emit_add_line(reset, ecbdata, line + 1, len - 1);
+ break;
+ case '-':
+ ecbdata->lno_in_preimage++;
+ emit_del_line(reset, ecbdata, line + 1, len - 1);
+ break;
+ case ' ':
+ ecbdata->lno_in_postimage++;
+ ecbdata->lno_in_preimage++;
+ emit_context_line(reset, ecbdata, line + 1, len - 1);
+ break;
+ default:
+ /* incomplete line at the end */
+ ecbdata->lno_in_preimage++;
+ emit_line(ecbdata->opt,
+ diff_get_color(ecbdata->color_diff, DIFF_PLAIN),
+ reset, line, len);
+ break;
}
}
--
2.4.2-503-g3e4528a
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 1/4] t4015: modernise style
Move the preparatory steps that create the expected output inside
the test bodies, remove unnecessary blank lines before and after the
test bodies, and drop SP between redirection operator and its target.
Signed-off-by: Junio C Hamano
---
t/t4015-diff-whitespace.sh | 411 +++--
1 file changed, 173 insertions(+), 238 deletions(-)
diff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh
index 604a838..0bfc7ff 100755
--- a/t/t4015-diff-whitespace.sh
+++ b/t/t4015-diff-whitespace.sh
@@ -9,138 +9,144 @@ test_description='Test special whitespace in diff engine.
. ./test-lib.sh
. "$TEST_DIRECTORY"/diff-lib.sh
-# Ray Lehtiniemi's example
+test_expect_success "Ray Lehtiniemi's example" '
+ cat <<-\EOF >x &&
+ do {
+ nothing;
+ } while (0);
+ EOF
+ git update-index --add x &&
-cat << EOF > x
-do {
- nothing;
-} while (0);
-EOF
+ cat <<-\EOF >x &&
+ do
+ {
+ nothing;
+ }
+ while (0);
+ EOF
-git update-index --add x
+ cat <<-\EOF >expect &&
+ diff --git a/x b/x
+ index adf3937..6edc172 100644
+ --- a/x
+ +++ b/x
+ @@ -1,3 +1,5 @@
+ -do {
+ +do
+ +{
+ nothing;
+ -} while (0);
+ +}
+ +while (0);
+ EOF
-cat << EOF > x
-do
-{
- nothing;
-}
-while (0);
-EOF
+ git diff >out &&
+ test_cmp expect out &&
-cat << EOF > expect
-diff --git a/x b/x
-index adf3937..6edc172 100644
a/x
-+++ b/x
-@@ -1,3 +1,5 @@
--do {
-+do
-+{
-nothing;
--} while (0);
-+}
-+while (0);
-EOF
+ git diff -w >out &&
+ test_cmp expect out &&
-git diff > out
-test_expect_success "Ray's example without options" 'test_cmp expect out'
+ git diff -b >out &&
+ test_cmp expect out
+'
-git diff -w > out
-test_expect_success "Ray's example with -w" 'test_cmp expect out'
+test_expect_success 'another test, without options' '
+ tr Q "\015" <<-\EOF >x &&
+ whitespace at beginning
+ whitespace change
+ whitespace in the middle
+ whitespace at end
+ unchanged line
+ CR at endQ
+ EOF
-git diff -b > out
-test_expect_success "Ray's example with -b" 'test_cmp expect out'
+ git update-index x &&
-tr 'Q' '\015' << EOF > x
-whitespace at beginning
-whitespace change
-whitespace in the middle
-whitespace at end
-unchanged line
-CR at endQ
-EOF
+ tr "_" " " <<-\EOF >x &&
+ _ whitespace at beginning
+ whitespace change
+ white space in the middle
+ whitespace at end__
+ unchanged line
+ CR at end
+ EOF
-git update-index x
+ tr "Q_" "\015 " <<-\EOF >expect &&
+ diff --git a/x b/x
+ index d99af23..22d9f73 100644
+ --- a/x
+ +++ b/x
+ @@ -1,6 +1,6 @@
+ -whitespace at beginning
+ -whitespace change
+ -whitespace in the middle
+ -whitespace at end
+ + whitespace at beginning
+ +whitespace change
+ +white space in the middle
+ +whitespace at end__
+unchanged line
+ -CR at endQ
+ +CR at end
+ EOF
-tr '_' ' ' << EOF > x
- whitespace at beginning
-whitespace change
-white space in the middle
-whitespace at end__
-unchanged line
-CR at end
-EOF
+ git diff >out &&
+ test_cmp expect out &&
-tr 'Q_' '\015 ' << EOF > expect
-diff --git a/x b/x
-index d99af23..8b32fb5 100644
a/x
-+++ b/x
-@@ -1,6 +1,6 @@
--whitespace at beginning
--whitespace change
--whitespace in the middle
--whitespace at end
-+ whitespace at beginning
-+whitespace change
-+white space in the middle
-+whitespace at end__
- unchanged line
--CR at endQ
-+CR at end
-EOF
-git diff > out
-test_expect_success 'another test, without options' 'test_cmp expect out'
+ >expect &&
+ git diff -w >out &&
+ test_cmp expect out &&
+
+ git diff -w -b >out &&
+ test_cmp expect out &&
+
+ git diff -w --ignore-space-at-eol >out &&
+ test_cmp expect out &&
+
+ git diff -w -b --ignore-space-at-eol >out &&
+ test_cmp expect out &&
-cat << EOF > expect
-EOF
-git diff -w > out
-test_expect_success 'another test, with -w' 'test_cmp expect out'
-git diff -w -b > out
-test_expect_success 'another test, with -w -b' 'test_cmp expect out'
-git diff -w --ignore-space-at-eol > out
-test_expect_success 'another test, with -w --ignore-space-at-eol' 'test_cmp
expect out'
-git diff -w -b --ignore-space-at-eol > out
-test_expect_success 'another test, with -w -b --ignore-space-at-eol' 'test_cmp
expect out'
-
-tr 'Q_' '\015 ' << EOF > expect
-diff --git a/x b/x
-index d99af23..8b32fb5 100644
a/x
-+++ b/x
-@@ -1,6 +1,6 @@
--whitespace at beginning
-+ whitespace at beginning
- whitespace change
--whitespace in the middle
-+white space in the middle
- whitespace at end__
- unchanged line
- CR at end
-EOF
-git diff -b > out
-test_ex
[PATCH v3 4/4] diff.c: --ws-error-highlight= option
Traditionally, we only cared about whitespace breakages introduced
in new lines. Some people want to paint whitespace breakages on old
lines, too. When they see a whitespace breakage on a new line, they
can spot the same kind of whitespace breakage on the corresponding
old line and want to say "Ah, those breakages are there but they
were inherited from the original, so let's not touch them for now."
Introduce `--ws-error-highlight=` option, that lets them pass
a comma separated list of `old`, `new`, and `context` to specify
what lines to highlight whitespace errors on.
Signed-off-by: Junio C Hamano
---
Documentation/diff-options.txt | 10 +
diff.c | 84 +---
diff.h | 5 +++
t/t4015-diff-whitespace.sh | 96 ++
4 files changed, 179 insertions(+), 16 deletions(-)
diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index 6cb083a..85a6547 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt
@@ -278,6 +278,16 @@ ifndef::git-format-patch[]
initial indent of the line are considered whitespace errors.
Exits with non-zero status if problems are found. Not compatible
with --exit-code.
+
+--ws-error-highlight=::
+ Highlight whitespace errors on lines specified by
+ in the color specified by `color.diff.whitespace`.
+ is a comma separated list of `old`, `new`, `context`. When
+ this option is not given, only whitespace errors in `new`
+ lines are highlighted. E.g. `--ws-error-highlight=new,old`
+ highlights whitespace errors on both deleted and added lines.
+ `all` can be used as a short-hand for `old,new,context`.
+
endif::git-format-patch[]
--full-index::
diff --git a/diff.c b/diff.c
index c575c45..34012b4 100644
--- a/diff.c
+++ b/diff.c
@@ -478,42 +478,57 @@ static int new_blank_line_at_eof(struct emit_callback
*ecbdata, const char *line
return ws_blank_line(line, len, ecbdata->ws_rule);
}
-static void emit_add_line(const char *reset,
- struct emit_callback *ecbdata,
- const char *line, int len)
+static void emit_line_checked(const char *reset,
+ struct emit_callback *ecbdata,
+ const char *line, int len,
+ enum color_diff color,
+ unsigned ws_error_highlight,
+ char sign)
{
- const char *ws = diff_get_color(ecbdata->color_diff, DIFF_WHITESPACE);
- const char *set = diff_get_color(ecbdata->color_diff, DIFF_FILE_NEW);
+ const char *set = diff_get_color(ecbdata->color_diff, color);
+ const char *ws = NULL;
- if (!*ws)
- emit_line_0(ecbdata->opt, set, reset, '+', line, len);
- else if (new_blank_line_at_eof(ecbdata, line, len))
+ if (ecbdata->opt->ws_error_highlight & ws_error_highlight) {
+ ws = diff_get_color(ecbdata->color_diff, DIFF_WHITESPACE);
+ if (!*ws)
+ ws = NULL;
+ }
+
+ if (!ws)
+ emit_line_0(ecbdata->opt, set, reset, sign, line, len);
+ else if (sign == '+' && new_blank_line_at_eof(ecbdata, line, len))
/* Blank line at EOF - paint '+' as well */
- emit_line_0(ecbdata->opt, ws, reset, '+', line, len);
+ emit_line_0(ecbdata->opt, ws, reset, sign, line, len);
else {
/* Emit just the prefix, then the rest. */
- emit_line_0(ecbdata->opt, set, reset, '+', "", 0);
+ emit_line_0(ecbdata->opt, set, reset, sign, "", 0);
ws_check_emit(line, len, ecbdata->ws_rule,
ecbdata->opt->file, set, reset, ws);
}
}
-static void emit_del_line(const char *reset,
+static void emit_add_line(const char *reset,
struct emit_callback *ecbdata,
const char *line, int len)
{
- const char *set = diff_get_color(ecbdata->color_diff, DIFF_FILE_OLD);
+ emit_line_checked(reset, ecbdata, line, len,
+ DIFF_FILE_NEW, WSEH_NEW, '+');
+}
- emit_line_0(ecbdata->opt, set, reset, '-', line, len);
+static void emit_del_line(const char *reset,
+ struct emit_callback *ecbdata,
+ const char *line, int len)
+{
+ emit_line_checked(reset, ecbdata, line, len,
+ DIFF_FILE_OLD, WSEH_OLD, '-');
}
static void emit_context_line(const char *reset,
struct emit_callback *ecbdata,
const char *line, int len)
{
- const char *set = diff_get_color(ecbdata->color_diff, DIFF_PLAIN);
-
- emit_line_0(ecbdata->opt, set, reset, ' ', line, len);
+ emit_line_checked(reset, ecbdata, li
[PATCH v3 2/4] t4015: separate common setup and per-test expectation
The last two tests in the script were to
- set up color.diff.* slots
- set up an expectation for a single test
- run that test and check the result
but split in a wrong way. It did the first two in the first test
and the third one in the second test. The latter two belong to each
other. This matters when you plan to add more of these tests that
share the common coloring.
While at it, make sure we use a color different from old, which is
also red.
Signed-off-by: Junio C Hamano
---
t/t4015-diff-whitespace.sh | 25 +
1 file changed, 13 insertions(+), 12 deletions(-)
diff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh
index 0bfc7ff..4da30e5 100755
--- a/t/t4015-diff-whitespace.sh
+++ b/t/t4015-diff-whitespace.sh
@@ -810,11 +810,20 @@ test_expect_success 'setup diff colors' '
git config color.diff.old red &&
git config color.diff.new green &&
git config color.diff.commit yellow &&
- git config color.diff.whitespace "normal red" &&
+ git config color.diff.whitespace blue &&
- git config core.autocrlf false &&
+ git config core.autocrlf false
+'
+
+test_expect_success 'diff that introduces a line with only tabs' '
+ git config core.whitespace blank-at-eol &&
+ git reset --hard &&
+ echo "test" >x &&
+ git commit -m "initial" x &&
+ echo "{NTN}" | tr "NT" "\n\t" >>x &&
+ git -c color.diff=always diff | test_decode_color >current &&
- cat >expected <<-\EOF
+ cat >expected <<-\EOF &&
diff --git a/x b/x
index 9daeafb..2874b91 100644
--- a/x
@@ -822,18 +831,10 @@ test_expect_success 'setup diff colors' '
@@ -1 +1,4 @@
test
+{
- +
+ +
+}
EOF
-'
-test_expect_success 'diff that introduces a line with only tabs' '
- git config core.whitespace blank-at-eol &&
- git reset --hard &&
- echo "test" >x &&
- git commit -m "initial" x &&
- echo "{NTN}" | tr "NT" "\n\t" >>x &&
- git -c color.diff=always diff | test_decode_color >current &&
test_cmp expected current
'
--
2.4.2-503-g3e4528a
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/WIP PATCH 04/11] upload-pack-2: Implement the version 2 of upload-pack
On Tue, May 26, 2015 at 03:01:08PM -0700, Stefan Beller wrote:
> --- a/upload-pack.c
> +++ b/upload-pack.c
> @@ -716,10 +716,47 @@ static void format_symref_info(struct strbuf *buf,
> struct string_list *symref)
> strbuf_addf(buf, " symref=%s:%s", item->string, (char
> *)item->util);
> }
>
> -static const char *advertise_capabilities = "multi_ack thin-pack side-band"
> +static char *advertise_capabilities = "multi_ack thin-pack side-band"
> " side-band-64k ofs-delta shallow no-progress"
> " include-tag multi_ack_detailed";
If we are upload-pack-2, should we advertise that in the capabilities? I
think it may make things easier later if we try to provide some
opportunistic out-of-band data. E.g., if see tell git-daemon:
git-upload-pack repo\0host=whatever\0\0version=2
how do we know whether version=2 was understood and kicked us into v2
mode, versus an old server that ignored it?
It also just makes the protocol more self-describing; from the
conversation you can see which version is in use.
> +static void send_capabilities(void)
> +{
> + char buf[100];
> +
> + while (next_capability(buf))
> + packet_write(1, "capability:%s\n", buf);
Having a static-sized buffer and then passing it to a function which has
no idea of the buffer size seems like a recipe for a buffer overflow. :)
You are fine here because you are parsing the hard-coded capabilities
string, and we know 100 is enough there. But it's nice to avoid such
magic.
Like Eric, I find the whole next_capability thing a little ugly. His
suggestion to pass in the parsing state is an improvement, but I wonder
why we need to parse at all. Can we keep the capabilities as:
const char *capabilities[] = {
"multi_ack",
"thin-pack",
... etc ...
};
and then loop over the array? The v1 writer will need to be modified,
but it should be fairly straightforward (loop and add them to the buffer
rather than dumping the whole string).
Also, do we need the capability noise-word? They all have it, except
for:
> + packet_write(1, "agent:%s\n", git_user_agent_sanitized());
But isn't that basically a capability, too (I agree it is stretching the
definition of "capability", but I think that ship has sailed; the
client's response is not "I support this, too" but "I want to enable
this").
IOW, is there a reason that the initial conversation is not just:
pkt-line("multi_ack");
pkt-line("thin-pack");
...
pkt-line("agent=v2.5.0");
pkt-line();
We already have framing for each capability due to the use of pkt-line.
The "capability:" is just one more thing the client has to parse past.
The keys are already unique up to any "=", so I don't think there is any
ambiguity (e.g., we don't care about "capability:agent"; we have already
assigned a meaning to the term "agent", and will never introduce a
standalone capability with the same name).
Likewise, if we introduce new protocol items here, the client should
either ignore them (if it does not understand them) or know what to do
with them.
> +static void receive_capabilities(void)
> +{
> + int done = 0;
> + while (1) {
> + char *line = packet_read_line(0, NULL);
> + if (!line)
> + break;
> + if (starts_with(line, "capability:"))
> + parse_features(line + strlen("capability:"));
> + }
Use:
const char *cap;
if (skip_prefix(line, "capability:", &cap))
parse_features(cap);
Or better yet, if you take my suggestion above:
parse_features(line);
:)
> +static int endswith(const char *teststring, const char *ending)
> +{
> + int slen = strlen(teststring);
> + int elen = strlen(ending);
> + return !strcmp(teststring + slen - elen, ending);
> +}
Eric mentioned the underflow problems here, but I wonder even more:
what's wrong with the global ends_with() that we already provide?
-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/WIP PATCH 05/11] transport: add infrastructure to support a protocol version number
On Tue, May 26, 2015 at 03:01:09PM -0700, Stefan Beller wrote:
> + OPT_STRING('y', "transport-version", &transport_version,
> +N_("transport-version"),
> +N_("specify transport version to be used")),
Interesting choice for the short option ("-v" would be nice, but
obviously it is taken). Do we want to delay on claiming the
short-and-sweet 'y' until we are sure this is something people will use
a lot? In an ideal world, it is not (i.e., auto-upgrade and other tricks
become good enough that nobody bothers to specify it manually).
-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/WIP PATCH 06/11] remote.h: add get_remote_capabilities, request_capabilities
On Tue, May 26, 2015 at 03:01:10PM -0700, Stefan Beller wrote:
> +void get_remote_capabilities(int in, char *src_buf, size_t src_len)
> +{
> + struct strbuf capabilities_string = STRBUF_INIT;
> + for (;;) {
> + int len;
> + char *line = packet_buffer;
> + const char *arg;
> +
> + len = packet_read(in, &src_buf, &src_len,
> + packet_buffer, sizeof(packet_buffer),
> + PACKET_READ_GENTLE_ON_EOF |
> + PACKET_READ_CHOMP_NEWLINE);
> + if (len < 0)
> + die_initial_contact(0);
> +
> + if (!len)
> + break;
> +
> + if (len > 4 && skip_prefix(line, "ERR ", &arg))
> + die("remote error: %s", arg);
> +
> + if (starts_with(line, "capability:")) {
> + strbuf_addstr(&capabilities_string, line +
> strlen("capability:"));
> + strbuf_addch(&capabilities_string, ' ');
> + }
> + }
I think this is the reverse case of next_capabilities in the upload-pack
side, so I'll make the reverse suggestion. :) Would it make things nicer
if both v1 and v2 parsed the capabilities into a string_list?
> + free(server_capabilities);
> + server_capabilities = xmalloc(capabilities_string.len + 1);
> + server_capabilities = strbuf_detach(&capabilities_string, NULL);
Is this xmalloc line left over? The strbuf_detach will write over it.
> + strbuf_release(&capabilities_string);
No need to release if we just detached.
> +int request_capabilities(int out)
> +{
> + fprintf(stderr, "request_capabilities\n");
Debug cruft, I presume.
> + // todo: send our capabilities
> + packet_write(out, "capability:foo");
No C99 comments. But I think this is just a placeholder. :)
-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/WIP PATCH 06/11] remote.h: add get_remote_capabilities, request_capabilities
On Tue, May 26, 2015 at 11:25:05PM -0400, Eric Sunshine wrote: > > + len = packet_read(in, &src_buf, &src_len, > > + packet_buffer, sizeof(packet_buffer), > > + PACKET_READ_GENTLE_ON_EOF | > > + PACKET_READ_CHOMP_NEWLINE); > > + if (len < 0) > > + die_initial_contact(0); > > + > > + if (!len) > > + break; > > + > > + if (len > 4 && skip_prefix(line, "ERR ", &arg)) > > The 'len > 4' check is needed because there's no guarantee that 'line' > is NUL-terminated. Correct? I think this was just blindly copied from get_remote_heads(). And I think that code was being overly paranoid. Ever since f3a3214 (Make send/receive-pack be closer to doing something interesting, 2005-06-29), the pkt-line reader will add an extra NUL to the buffer to ease cases like this. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/WIP PATCH 07/11] fetch-pack: use the configured transport protocol
On Tue, May 26, 2015 at 03:01:11PM -0700, Stefan Beller wrote:
> diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
> index 4a6b340..32dc8b0 100644
> --- a/builtin/fetch-pack.c
> +++ b/builtin/fetch-pack.c
> @@ -127,6 +127,10 @@ int cmd_fetch_pack(int argc, const char **argv, const
> char *prefix)
> args.update_shallow = 1;
> continue;
> }
> + if (!strcmp("--transport-version", arg)) {
> + args.version = strtol(arg +
> strlen("--transport-version"), NULL, 0);
> + continue;
> + }
You strcmp() the arg here, so there can't be anything at arg +
strlen(...), can there? Did you want:
starts_with(arg, "--transports-version=")
here? Or better yet, skip_prefix().
> - get_remote_heads(fd[0], NULL, 0, &ref, 0, NULL, &shallow);
> +
> + switch (args.version) {
> + default:
> + case 2:
> + get_remote_capabilities(fd[0], NULL, 0);
> + request_capabilities(fd[1]);
> + break;
Should the "default" case be to complain about an unknown version,
rather than fall-through to 2?
-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/WIP PATCH 08/11] transport: connect_setup appends protocol version number
On Tue, May 26, 2015 at 10:09:45PM -0700, Junio C Hamano wrote:
> Stefan Beller writes:
>
> > On Tue, May 26, 2015 at 3:21 PM, Junio C Hamano wrote:
> >
> >>
> >> if (...->version < 2) {
> >> ... append "-%d" ...
> >> }
> >>
> >> involved.
> >
> > Oh! I see here you would count the current one as 1, which has no
> > number extension, and any further would have a -${version}. That
> > would transport the intention much better I guess.
>
> Yeah, except that I screwed up my comparison. Obviously, I should
> have said "If version is 2 or later, then append -%d to the name,
> otherwise use the name as-is".
FWIW, I had similar head-scratching over Stefan's original. I think it's
OK to say "version 1 is magical, and for historical reasons does not
have its number appended". There's no need for us ever to make
"upload-pack-1"; it just introduces more headaches.
-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
