Re: de-alphabetizing the documentation
On Fri, Jul 06, 2018 at 04:21:47PM -0700, frede...@ofb.net wrote: > I don't think that it's really important to find a "best" ordering for > commands or glossary terms; it's more a matter of finding someone who > is willing to take responsibility for choosing a reasonable ordering. > Presumably the head maintainer of this project could delegate the task > to a qualified volunteer, not a newbie like myself but not necessarily > someone with expert knowledge either. It's too bad that a policy of > not listing things alphabetically wasn't adopted from the beginning of > this project, but I guess that's life. That wasn't that portion of the man page, for better or for worse. We can debate whethher using a non-alphabetical order would be better or worse for everyone; personally, I think the much better pointer is at the very beginning of the git man page, which points people at "man gittutorial" and "man giteveryday". It seems to me that for your stated goal, "git everyday" has a good list of commands that people should learn, complete with a proposed workflow. That's probably the biggest stumbling block of finding an ideal ordering. What's reasonable really depends on your workflow, and there are many different workflows depending on what a particular developer is trying to do. Consider carpentry; for some use cases, a screwdriver is an absolutely critical tool. For others, they might never use it, and instead almost exclusively join two pieces of woods using mortise and tenon joint. Others might use a butt joint, plus glue and nails. All of these different techniques can be used to make a wooden box, and they all involve a very different set of tools. Regards, - Ted
Re: What's (not) cooking
On Fri, Jul 6, 2018 at 3:57 PM, Junio C Hamano wrote:
> I'll be pushing out the integration branches with some updates, but
> there is no change in 'next' and below. The following topics I gave
> a quick look and gave them topic branches, but I had trouble merging
> them in 'pu' and making them work correctly or pass the tests, so
> they are not part of 'pu' in today's pushout.
>
> pk/rebase-in-c
> en/dirty-merge-fixes
> en/t6036-merge-recursive-tests
> en/t6042-insane-merge-rename-testcases
> ds/multi-pack-index
It looks to me like the main problem is that pu itself has lots of
test failures. It seems to bisect down to
kg/gc-auto-windows-workaround. If I revert commit ac9d3fdbebbd ("gc
--auto: clear repository before auto packing", 2018-07-04), then pu
passes tests again for me. With that reverted, I can merge
en/t6036-merge-recursive-tests and
en/t6042-insane-merge-rename-testcases without conflicts and the tests
pass without incident.
The other three topics all have merge conflicts.
en/dirty-merge-fixes has a small conflict with the new topic
nd/use-the-index-compat-less, which I mentioned as a possibility in
the cover letter to my series. I'm happy to do whatever makes it
easiest for you to pick up; I can easily rebase on that topic branch,
but I thought you wanted to see that topic redone first (to avoid
"useless churn"), so I'm unsure what the right next step is.
Re: [PATCH] fsck: check skiplist for object in fsck_blob()
On Wed, Jul 04, 2018 at 01:12:40AM +0100, Ramsay Jones wrote: > > True that we don't even bother doing the parsing with your patch. But I > > think I talked myself out of that part being a significant savings > > elsewhere. > > [Sorry for the late reply - watching football again!] > > I'm not interested in any savings - it would have to be a pretty > wacky repo for there to be much in the way of savings! > > Simply, I have found (for many different reasons) that, if there > is no good reason to execute some code, it is _far_ better to not > do so! ;-) Heh. I also agree with that as a guiding principle. But I _also_ like the principle of "if you do not need to do add this code, do not add it". So the two are a little at odds here. :) > > I'm not sure. This has been running on every push to GitHub for the past > > 6 weeks, and this is the first report. It's hard to say what that means, > > and technically speaking of course this _is_ a regression. > > Hmm, are you concerned about old clients 'transmitting' the > bad data via large hosting sites? (New clients would complain > about a dodgy .gitmodules file, no matter were it came from, > right?) I just meant above that anybody with a broken .gitmodules would have had their push rejected, and we haven't gotten any such reports beyond yours and Mike's. So that's some evidence that it's relatively rare. As far as why those fsck checks are there in the first place, yes, it's about transmitting bad data to unpatched clients. Ultimately the responsibility for not being vulnerable is on the clients themselves. But in practice large hosting sites can help by not being vectors. > Has the definition of the config file syntax changed recently? > If not, then old client versions will see the same syntax errors > as recently 'fixed' versions. So they should error out without > 'looking' at the bad data, right? (ignoring the 'lets fix the > obvious syntax error' idea). No, it hasn't change. So as far as I know, loosening the syntax check does not impact _this_ vulnerability, because it requires a file that can be parsed, and the parser is the same for both cases. It might affect future vulnerabilities. We could also tighten when those come to light, or tighten in a way that blocks the specific bug. But there's potentially some value in putting our foot down _now_ and saying that we're going to enforce certain properties of special files, before it gets any further out-of-hand. And of course I could be wrong. We do occasionally fix bugs in the parser, so I won't be surprised if there's some obscure case where git <2.0 would not be protected or something like that. But frankly, anything that old is probably already vulnerable to other ancient bugs, too. > > Thanks. If we're going to do any loosening, I think we may want to > > address that _first_, so it can go directly on top of the patches in > > v2.17.1 (because it's a bigger issue than the stray message, IMHO). > > Agreed. I probably haven't given it sufficient thought, but my > immediate reaction is to loosen the check - I don't see how > this could be exploited to significantly reduce security. (My lack > of imagination has been noted several times in the past, however!) > > Having said that, I am no security expert, so I will let those who > have security expertise decide what is best to do in this situation. I think you have a grasp on the situation from what you wrote above. What next? I was kind of leaning towards loosening, but it sounded like Junio thought the opposite. One thing I didn't like about the patch I sent earlier is that it removes the option for the admin to say "no, I really do want to enforce this". I don't know if that's of value or not. In an ideal world, I think we'd detect the problem and then react according to the receiver's receive.fsck.* config. And we could just flip the default for receive.fsck.gitmodulesParse to "warning". But IIRC, the fsck code in index-pack does not bother distinguishing between warnings and errors. I think it ought to, but that's too big a change to go on maint. It _might_ work to just flip the default to "ignore". That leaves the paranoid admin with a way to re-enable it if they want, and I _think_ it would be respected by index-pack. [goes and looks at the code] Hmm, we seem to have "info" these days, so maybe that would do what I want. I.e., I wonder if the patch below does everything we'd want. It's late here and I probably won't get back to this until Monday, but you may want to play with it in the meantime. diff --git a/fsck.c b/fsck.c index 48e7e36869..0b0003055e 100644 --- a/fsck.c +++ b/fsck.c @@ -61,7 +61,6 @@ static struct oidset gitmodules_done = OIDSET_INIT; FUNC(ZERO_PADDED_DATE, ERROR) \ FUNC(GITMODULES_MISSING, ERROR) \ FUNC(GITMODULES_BLOB, ERROR) \ - FUNC(GITMODULES_PARSE, ERROR) \ FUNC(GITMODULES_NAME, ERROR) \ FUNC(GITMODULES_SYMLINK, ERROR) \ /* warnings */ \ @@ -76,6
Re: de-alphabetizing the documentation
Hi, Frederick Eaton wrote: > Unfortunately my contribution will have to be limited for the > moment to making this suggestion, as I am extraordinarily busy. I hope > it will not be too burdensome to add this item to your TODO list and > keep it there until a willing volunteer comes along. No problem. If you have time to contribute later, we can wait. :) > For what it's worth, I made extensive changes to the Arch Wiki Git > article back in 2015, following an initial attempt of mine to > understand various tutorials. It was the most prominent wiki-based Git > documentation I could find at the time. The article has of course seen > numerous improvements since then. For reference: https://wiki.archlinux.org/index.php/git > I don't think that it's really important to find a "best" ordering for > commands or glossary terms; it's more a matter of finding someone who > is willing to take responsibility for choosing a reasonable ordering. > Presumably the head maintainer of this project could delegate the task > to a qualified volunteer, not a newbie like myself but not necessarily > someone with expert knowledge either. I'd have to say, when I compare the troubles a new user and a long-timer would run into, I conclude that the long-timers would be more likely to produce worse documentation. It is very difficult to remember how new users see things. The ideal skill set in fact has nothing to do with level of Git expertise: to produce a good result, a good technical writer would ask the right questions to gather information from the experts and then test their documentation on newcomers until it works well. Based on the work you've described already having done, it sounds like you'd be an ideal person to get this going, if you find yourself with time for it. > It's too bad that a policy of > not listing things alphabetically wasn't adopted from the beginning of > this project, but I guess that's life. >From this thread I've been convinced that for this kind of reference document, alphabetical organization within each section is a good organization, provided each section is small enough (as in "git help" output). I'm also a fan of non reference documentation that can use a narrative ordering instead (like "git help core-tutorial", except with more modern commands). > Thanks Eric for the pointer to "git help". This does indeed provide a > finer and better grouping than the man-page (but it also looks like > another candidate for de-alphabetization...!). Indeed, copying that organization over from "git help" to the git(1) manpage may be a good step for any interested people overhearing this conversation. As a first step, how about making git(1) recommend "git help", like this? It already recommends giteveryday(7) but the more interactive first command might be useful for some people. Thoughts? Improvements? -- >8 -- Subject: git doc: recommend "git help" as a starting point The list of subcommands described in git(1) can be overwhelming. Encourage newcomers to run "git help" to get a shorter list of commands as a starting point. Based on a suggestion by Frederick Eaton. Signed-off-by: Jonathan Nieder --- Documentation/git.txt | 9 ++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/Documentation/git.txt b/Documentation/git.txt index dba7f0c18e..0149ce9af0 100644 --- a/Documentation/git.txt +++ b/Documentation/git.txt @@ -23,9 +23,12 @@ unusually rich command set that provides both high-level operations and full access to internals. See linkgit:gittutorial[7] to get started, then see -linkgit:giteveryday[7] for a useful minimum set of -commands. The link:user-manual.html[Git User's Manual] has a more -in-depth introduction. +linkgit:giteveryday[7] and run + +git help + +for a useful minimum set of commands. The link:user-manual.html[Git +User's Manual] has a more in-depth introduction. After you mastered the basic concepts, you can come back to this page to learn what commands Git offers. You can learn more about -- 2.18.0.203.gfac676dfb9
Re: de-alphabetizing the documentation
Thank you Jonathan for signaling your willingness to adopt the documentation philosophy I suggested. That's a quite valuable first step. Unfortunately my contribution will have to be limited for the moment to making this suggestion, as I am extraordinarily busy. I hope it will not be too burdensome to add this item to your TODO list and keep it there until a willing volunteer comes along. For what it's worth, I made extensive changes to the Arch Wiki Git article back in 2015, following an initial attempt of mine to understand various tutorials. It was the most prominent wiki-based Git documentation I could find at the time. The article has of course seen numerous improvements since then. I don't think that it's really important to find a "best" ordering for commands or glossary terms; it's more a matter of finding someone who is willing to take responsibility for choosing a reasonable ordering. Presumably the head maintainer of this project could delegate the task to a qualified volunteer, not a newbie like myself but not necessarily someone with expert knowledge either. It's too bad that a policy of not listing things alphabetically wasn't adopted from the beginning of this project, but I guess that's life. Thanks Eric for the pointer to "git help". This does indeed provide a finer and better grouping than the man-page (but it also looks like another candidate for de-alphabetization...!). Many thanks, Frederick On Fri, Jul 06, 2018 at 02:18:28PM -0700, Jonathan Nieder wrote: > Jonathan Nieder wrote: > > > Ideas? If you start with a proposal, we're happy to help refine it. > > People in the #git channel on irc.freenode.net (wechat.freenode.net) > > might also be useful for inspiration in coming up with a proposal. > > I meant to link to webchat.freenode.net. But > https://kiwiirc.com/nextclient/ may have been a better link to use > anyway. > > Thanks and sorry for the noise, > Jonathan > On Fri, Jul 06, 2018 at 05:32:39PM -0400, Eric Sunshine wrote: > On Fri, Jul 06, 2018 at 02:16:00PM -0700, Jonathan Nieder wrote: > > Frederick Eaton wrote: > > > I wonder if someone familiar with Git could list the commands in an > > > order which makes more sense for learning, for example in the order in > > > which they were invented by Git developers, > > > > Alas, there are plenty of "Main porcelain commands", and I think that > > is where your question comes from. It would be nicer to list just five > > to start, say. > > "git help" makes some attempt at narrowing the list of porcelain > commands likely to be used on an everyday basis (and it categorizes > the list by general activity). Of the 21 commands listed, I use 14-16 > in pretty much every development session, so "git help" might be a > good starting place for someone trying to figure out which commands to > study, or for someone wishing to help focus the documentation a bit > more for beginners. > > --- >8 --- > $ git help > usage: git ... > > These are common Git commands used in various situations: > > start a working area (see also: git help tutorial) >clone Clone a repository into a new directory >init Create an empty Git repository or reinitialize an existing one > > work on the current change (see also: git help everyday) >addAdd file contents to the index >mv Move or rename a file, a directory, or a symlink >reset Reset current HEAD to the specified state >rm Remove files from the working tree and from the index > > examine the history and state (see also: git help revisions) >bisect Use binary search to find the commit that introduced a bug >grep Print lines matching a pattern >logShow commit logs >show Show various types of objects >status Show the working tree status > > grow, mark and tweak your common history >branch List, create, or delete branches >checkout Switch branches or restore working tree files >commit Record changes to the repository >diff Show changes between commits, commit and working tree, etc >merge Join two or more development histories together >rebase Reapply commits on top of another base tip >tagCreate, list, delete or verify a tag object signed with GPG > > collaborate (see also: git help workflows) >fetch Download objects and refs from another repository >pull Fetch from and integrate with another repository or a local > branch >push Update remote refs along with associated objects > > 'git help -a' and 'git help -g' list available subcommands and some > concept guides. See 'git help ' or 'git help ' > to read about a specific subcommand or concept. > --- >8 --- >
Re: What's (not) cooking
Junio C Hamano writes: > I'll be pushing out the integration branches with some updates, but > there is no change in 'next' and below. The following topics I gave > a quick look and gave them topic branches, but I had trouble merging > them in 'pu' and making them work correctly or pass the tests, so > they are not part of 'pu' in today's pushout. > > pk/rebase-in-c > en/dirty-merge-fixes > en/t6036-merge-recursive-tests > en/t6042-insane-merge-rename-testcases > ds/multi-pack-index Also I ran out of time looking at various interesting things that happened during US holiday, and didn't get around to a few interesting topics. So they are not even in the above "not in pu" list, but they did hit my mailbox. I just haven't got around to them yet.
What's (not) cooking
I'll be pushing out the integration branches with some updates, but there is no change in 'next' and below. The following topics I gave a quick look and gave them topic branches, but I had trouble merging them in 'pu' and making them work correctly or pass the tests, so they are not part of 'pu' in today's pushout. pk/rebase-in-c en/dirty-merge-fixes en/t6036-merge-recursive-tests en/t6042-insane-merge-rename-testcases ds/multi-pack-index
Re: [PATCH v3 18/20] completion: support `git range-diff`
"Johannes Schindelin via GitGitGadget"
writes:
> From: Johannes Schindelin
>
> Tab completion of `git range-diff` is very convenient, especially
> given that the revision arguments to specify the commit ranges to
> compare are typically more complex than, say, your grandfather's `git
> log` arguments.
>
> Signed-off-by: Johannes Schindelin
Have three-dash lines here, or perhaps have some validation hook in
the garden-shears tool to notice these leftoer bits we see below?
>
> squash! WIP completion: support `git range-diff`
>
> Revert "WIP completion: support `git range-diff`"
>
> This reverts commit 2e7af652af9e53a19fd947f8ebe37a78043afa49.
> ---
> contrib/completion/git-completion.bash | 14 ++
> 1 file changed, 14 insertions(+)
>
> diff --git a/contrib/completion/git-completion.bash
> b/contrib/completion/git-completion.bash
> index 94c95516e..402490673 100644
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -1976,6 +1976,20 @@ _git_push ()
> __git_complete_remote_or_refspec
> }
>
> +_git_range_diff ()
> +{
> + case "$cur" in
> + --*)
> + __gitcomp "
> + --creation-factor= --dual-color
> + $__git_diff_common_options
> + "
> + return
> + ;;
> + esac
> + __git_complete_revlist
> +}
> +
> _git_rebase ()
> {
> __git_find_repo_path
Re: [PATCH v3 01/20] linear-assignment: a function to solve least-cost assignment problems
"Johannes Schindelin via GitGitGadget" writes: > From: Johannes Schindelin > > The problem solved by the code introduced in this commit goes like this: > given two sets of items, and a cost matrix which says how much it > "costs" to assign any given item of the first set to any given item of > the second, assign all items (except when the sets have different size) > in the cheapest way. > > We use the Jonker-Volgenant algorithm to solve the assignment problem to > answer questions such as: given two different versions of a topic branch > (or iterations of a patch series), what is the best pairing of > commits/patches between the different versions? > > Signed-off-by: Johannes Schindelin > --- Does the "gitgitgadget" thing lie on the Date: e-mail header? Postdating the patch with in-body header is fine, but mailbox tools often use and trust the Date: timestamp when sorting and finding messages etc. so sending a new patch to add linear-assignment.c that is different from what was added 9 weeks ago with "Date: Mon, 30 Apr 2018" header can easily cause me to miss that message when I look for things that happened within the past few weeks, for example.
Re: ag/rebase-i-rewrite-todo, was Re: What's cooking in git.git (Jun 2018, #07; Thu, 28)
Hi Junio, On Fri, 6 Jul 2018, Junio C Hamano wrote: > Johannes Schindelin writes: > > > Of course, at that point I will have to look through those 7 patches > > again, if only to verify that yes, they are still the same. > > That is something the patch author must help the reviewer with, no? > > Have uncontroversial stuff early in the series, concentrate on > stabilizing them before moving on to shiny new toys, then mark them > "unchanged since the last round" after three dashes when sending a > reroll to update later parts of the series that are in flux. After > a few rounds, it may become apparent to reviewers that these early > parts can stand on their own merit as a separate series, on top of > which the remaining patches can build as a separate (sub)topic, at > which time we may have two or more topics, among which the early one > that has become stable may already be 'next'. Okay, I give up. I tried to show you that I thought that Alban's partitioning made sense on its own, that they could (and should) stabilize independently, even if they technically build on one another. In fact, it was I who suggested to keep them in bite-sized patch series. And you simply disagree from your maintaining point of view, and I violently disagree with your decision from the reviewer's point of view. In the end, you win. Dscho
Re: [PATCH 0/2] Avoiding errors when partial cloning a tagged blob
Jonathan Nieder writes: > The upsides are that > ... > - down the line, it should make operations like "fetch just this one > tree" a little simpler, since you can use > >filter blob:none >filter tree:none >want ;-) I think this example, especially without the first line, would have a practical use, i.e. "grab the contents of this directory without recursing", and as such is a very convincing argument to support the approach taken by the solution presented here. > So I'm comfortable with the direction > this series goes in, though I haven't looked at the patches in detail. Likewise. Thanks.
Re: [PATCH 1/2] t4018: add missing test cases for PHP
Kana Natsuno writes:
> A later patch changes the built-in PHP pattern. These test cases
> demonstrate aspects of the pattern that we do not want to change.
>
> Signed-off-by: Kana Natsuno
> ---
> t/t4018/php-class| 4
> t/t4018/php-function | 4
> t/t4018/php-method | 7 +++
> 3 files changed, 15 insertions(+)
> create mode 100644 t/t4018/php-class
> create mode 100644 t/t4018/php-function
> create mode 100644 t/t4018/php-method
>
> diff --git a/t/t4018/php-class b/t/t4018/php-class
> new file mode 100644
> index 000..7785b63
> --- /dev/null
> +++ b/t/t4018/php-class
> @@ -0,0 +1,4 @@
> +class RIGHT
> +{
> +const FOO = 'ChangeMe';
> +}
> diff --git a/t/t4018/php-function b/t/t4018/php-function
> new file mode 100644
> index 000..35717c5
> --- /dev/null
> +++ b/t/t4018/php-function
> @@ -0,0 +1,4 @@
> +function RIGHT()
> +{
> +return 'ChangeMe';
> +}
> diff --git a/t/t4018/php-method b/t/t4018/php-method
> new file mode 100644
> index 000..03af1a6
> --- /dev/null
> +++ b/t/t4018/php-method
> @@ -0,0 +1,7 @@
> +class Klass
> +{
> +public static function RIGHT()
> +{
> +return 'ChangeMe';
> +}
> +}
I no longer speak PHP, and certainly not the modern variant, but
these examples all look good to me.
Whoever invented the convetion to use RIGHT/ChangeMe in t4018 should
get praised---its brilliance shines in new tests like these ;-)
Re: [GSoC][PATCH v2 6/7] rebase -i: rewrite setup_reflog_action() in C
Johannes Schindelin writes: > You recently also suggested this if...else... dance to Pratik, where it > was not at all about doing the same thing slightly differently, but rather > two different things: 1) return an error because execvp() returned an > error, 2) indicate a serious bug (and you did not even suggest using BUG() > IIRC which is also wrong). I was going to avoid wasting time on this topic, but I happened to have a chance to review the execvp() thing. I actually think that is a good example of doing the same thing slightly differently. We want to abort the process when execvp() gives control back to that caller, and we want to do so with die_errno() when we know we have errno (i.e. sane_execvp() gave us negative). When sane_execvp() gave control back to us with non-negative return, we cannot use die_errno(), but we would want to abort the process regardless, as it is an indication that the callee is not sane after all ;-) It is a bug in underlying execvp() that we cannot do much about, though.
Re: [PATCH v3 0/2] grep.c: teach --only-matching to 'git-grep(1)'
Jeff King writes: > FWIW, I like the GNU phrasing. I thought the "non-empty" part was not > all that interesting, but after hearing that BSD behaves differently, it > is probably worth mentioning. > > I think the actual behavior of your patch matches GNU grep, and does not > need changing. Great ;-)
Re: [RFC PATCH v5] Implement --first-parent for git rev-list --bisect
Johannes Schindelin writes: >> And I also do not see a reason why somebody wants to make the dist >> computation to be 1-based (iow, changing the minimum from 0 to 1) or >> one step not to be 1 (iow, giving 11 to e1 and e8), so while I agree >> it is not strictly necessary to cast the concrete distance value in >> stone, I do not see much harm doing so *if* it helps to make it >> simpler the test that is necessary to make sure relative dist values >> assigned to these commits are in correct order. > > I guess that you still want to misunderstand me. Not at all. > So in this case, quite obviously what you want to do is to verify that E > and F get larger dist than e1 and e8. So that is what you test for. Not > some fixed text that might require adjusting in the future for any other > reason than a real bug. The most important part of the above quote is "*if* it helps..." part, and I do think the downside of insisting on dist being exactly 0 for E and F is acceptable than having to write the "E and F must get the same dist value, and e1 and e8 must get the same dist value that is larger than the value that E and F gets, ..." test without doing so *and* still keep the resulting test readable. It is a tradeoff and we are drawing the line differently (I am being more practical here than insisting on requiring absolute minimum and nothing more).
Re: [PATCH 1/8] builtin/receive-pack: use check_signature from gpg-interface
Junio C Hamano writes:
> Henning Schild writes:
>
>> The combination of verify_signed_buffer followed by parse_gpg_output is
>> available as check_signature. Use that instead of implementing it again.
>>
>> Signed-off-by: Henning Schild
>> ---
>
> Makes sense.
>
> When d05b9618 ("receive-pack: GPG-validate push certificates",
> 2014-08-14) implemented the check, there wasn't check_signature()
> available. The commit probably should have done what a4cc18f2
> ("verify-tag: share code with verify-commit", 2015-06-21) later did
> to introduce the check_signature() function by factoring it out of
> commit.c::check_commit_signature() as a preparatory step.
>
> Will queue. Thanks.
Well, I guess I won't queue this version that would waste others'
time, as you'd be rerolling to update variable names and such, so
I'd wait for that (and you in turn would wait for the names and
other discussions to settle).
Thanks anyway.
Re: de-alphabetizing the documentation
On Fri, Jul 06, 2018 at 02:16:00PM -0700, Jonathan Nieder wrote: > Frederick Eaton wrote: > > I wonder if someone familiar with Git could list the commands in an > > order which makes more sense for learning, for example in the order in > > which they were invented by Git developers, > > Alas, there are plenty of "Main porcelain commands", and I think that > is where your question comes from. It would be nicer to list just five > to start, say. "git help" makes some attempt at narrowing the list of porcelain commands likely to be used on an everyday basis (and it categorizes the list by general activity). Of the 21 commands listed, I use 14-16 in pretty much every development session, so "git help" might be a good starting place for someone trying to figure out which commands to study, or for someone wishing to help focus the documentation a bit more for beginners. --- >8 --- $ git help usage: git ... These are common Git commands used in various situations: start a working area (see also: git help tutorial) clone Clone a repository into a new directory init Create an empty Git repository or reinitialize an existing one work on the current change (see also: git help everyday) addAdd file contents to the index mv Move or rename a file, a directory, or a symlink reset Reset current HEAD to the specified state rm Remove files from the working tree and from the index examine the history and state (see also: git help revisions) bisect Use binary search to find the commit that introduced a bug grep Print lines matching a pattern logShow commit logs show Show various types of objects status Show the working tree status grow, mark and tweak your common history branch List, create, or delete branches checkout Switch branches or restore working tree files commit Record changes to the repository diff Show changes between commits, commit and working tree, etc merge Join two or more development histories together rebase Reapply commits on top of another base tip tagCreate, list, delete or verify a tag object signed with GPG collaborate (see also: git help workflows) fetch Download objects and refs from another repository pull Fetch from and integrate with another repository or a local branch push Update remote refs along with associated objects 'git help -a' and 'git help -g' list available subcommands and some concept guides. See 'git help ' or 'git help ' to read about a specific subcommand or concept. --- >8 ---
Re: [PATCH 1/3] ls-tree: make optional
Joshua Nelson writes: > On 07/06/2018 01:01 PM, Junio C Hamano wrote: >> Elijah Newren writes: >> I'd prefer *not* to have such a DWIM in a command like ls-tree, aka plumbing commands, where predictability is worth 1000 times more than ease of typing. >>> >>> Fair enough. However, what if no or are specified, >>> though -- would you be okay with the HEAD being assumed instead of >>> erroring out in that case? >> >> If we wrote ls-tree to do so 12 years ago, then I wouldn't have >> opposed. Changing the behaviour now? Not so sure if it is worth >> having to worry about updating the code, docs and making sure we >> spot all the possible typoes. >> > > I have to say, as a first time contributor, reading this is extremely > discouraging. I'm not being told the patch can be improved, or that I've > made some error that should be corrected. I'm being told that the entire > idea of the patch is unwanted, that it doesn't have a place in a mature > project like git, that only bug fixes and security issues should be > accepted. ... on plumbing commands like ls-tree, where keeping the interface stable is much more important than making it easier to "type". Rules for porcelain commands are entirely different.
Re: [PATCH v3 4/4] builtin/rebase: support running "git rebase "
Pratik Karki writes:
> +static void add_var(struct strbuf *buf, const char *name, const char *value)
> +{
> + strbuf_addstr(buf, name);
> + strbuf_addstr(buf, "=");
> + sq_quote_buf(buf, value);
> + strbuf_addstr(buf, "; ");
> +}
> +
> +static int run_specific_rebase(struct rebase_options *opts)
> +{
> + const char *argv[] = { NULL, NULL };
> + struct strbuf script_snippet = STRBUF_INIT;
> + int status;
> + const char *backend, *backend_func;
> +
> + add_var(_snippet, "GIT_DIR", absolute_path(get_git_dir()));
> +
> + add_var(_snippet, "upstream_name", opts->upstream_name);
> + add_var(_snippet, "upstream",
> + oid_to_hex(>upstream->object.oid));
> + add_var(_snippet, "head_name", opts->head_name);
> + add_var(_snippet, "orig_head", oid_to_hex(>orig_head));
> + add_var(_snippet, "onto", oid_to_hex(>onto->object.oid));
> + add_var(_snippet, "onto_name", opts->onto_name);
> + add_var(_snippet, "revisions", opts->revisions);
Looks alright.
> + switch (opts->type) {
> + case REBASE_AM:
> +...
> + default:
> + BUG("Unhandled rebase type %d", opts->type);
> + break;
> + }
Better.
> + strbuf_addf(_snippet,
> + ". git-rebase--common && . %s && %s",
> + backend, backend_func);
> + argv[0] = script_snippet.buf;
> +
> + status = run_command_v_opt(argv, RUN_USING_SHELL);
This used to use run_command_v_opt_cd_env() to pass extra
environment but now we can do a simpler one. As cmd_rebase()
has called setup_git_directory() to chdir to the top level of
the working tree, we just want to .-source the backend within
the current working directory, so loss of cd is also good ;-)
> + if (status == 0)
> + finish_rebase(opts);
> + else if (status == 2) {
> + struct strbuf dir = STRBUF_INIT;
> +
> + apply_autostash();
> + strbuf_addstr(, opts->state_dir);
> + remove_dir_recursively(, 0);
> + strbuf_release();
> + die("Nothing to do");
> + }
> +
> + strbuf_release(_snippet);
> +
> + return status ? -1 : 0;
> +}
> +
> int cmd_rebase(int argc, const char **argv, const char *prefix)
> {
> + struct rebase_options options = { -1 };
The first field of this struct is "enum rebase_type" defined without
"REBASE_TYPE_UNSPECIFIED = -1" in it. It probably makes sense to
add that constant there so that you can spell this value out.
> @@ -52,5 +194,98 @@ int cmd_rebase(int argc, const char **argv, const char
> *prefix)
> trace_repo_setup(prefix);
> setup_work_tree();
>
> - die("TODO");
> + options.type = REBASE_AM;
> +
> + switch (options.type) {
> + case REBASE_AM:
> + options.state_dir = apply_dir();
> + break;
> + case REBASE_MERGE:
> + case REBASE_INTERACTIVE:
> + case REBASE_PRESERVE_MERGES:
> + options.state_dir = merge_dir();
> + break;
Have "default:" here that barfs with BUG() to help future
introduction of bugs as the code to set options.type grows
complexity over time.
> + }
> + if (!options.root) {
> + if (argc != 2)
> + die("TODO: handle @{upstream}");
> + else {
> + options.upstream_name = argv[1];
> + argc--;
> + argv++;
> + if (!strcmp(options.upstream_name, "-"))
> + options.upstream_name = "@{-1}";
> + }
> + options.upstream = peel_committish(options.upstream_name);
> + if (!options.upstream)
> + die(_("invalid upstream '%s'"), options.upstream_name);
> + } else
> + die("TODO: upstream for --root");
> +
> + /* Make sure the branch to rebase onto is valid. */
> + if (!options.onto_name)
> + options.onto_name = options.upstream_name;
> + if (strstr(options.onto_name, "...")) {
> + die("TODO");
> + } else {
> + options.onto = peel_committish(options.onto_name);
> + if (!options.onto)
> + die(_("Does not point to a valid commit '%s'"),
> + options.onto_name);
> + }
> +
> + /*
> + * If the branch to rebase is given, that is the branch we will rebase
Style: align asterisks.
> + * branch_name -- branch/commit being rebased, or HEAD (already detached)
> + * orig_head -- commit object name of tip of the branch before rebasing
> + * head_name -- refs/heads/ or "detached HEAD"
> + */
> + if (argc > 1)
> + die ("TODO: handle switch_to");
Style: no SP between "die" and "("
Re: [PATCH 0/2] Avoiding errors when partial cloning a tagged blob
Hi, Junio C Hamano wrote: >> Jonathan Tan writes: >>> When cloning a repository with a tagged blob (like the Git repository) >>> with --filter=blob:none, the following message appears: >>> >>> error: missing object referenced by 'refs/tags/junio-gpg-pub' >>> >>> and the resulting repository also fails fsck. [...] >Naïvely, I may expect fsck to follow the > same principle--when it encounters a reference to an object that is > deliberately missing, refrain from complaining, regardless of the > place the offending reference was found, be it inside a tree object > or a ref. > > Perhaps that line of consistent behaviour may be impossible due to > the way the reference is stored inside a tree object and a ref? Yes, exactly. I believe this is why we did not treat refs as promisors when we introduced promised objects. We could revisit that decision and make refs into promisors, storing any extra metadata on the side. That would be a more invasive change than this series does, though. In the model used today, where refs are not promisors, this series is the logical thing to do. > E.g. a reference to an expected-to-be-missing blob still lets us > know that the missing object is expected to be a blob, but a ref > only stores the object name and not its type, so we can tell it is > missing, but we cannot say it is OK to be missing because we expect > it to be a blob, or something? The main downside of saying that filters should not be applied to the objects named directly in a "want" is that such objects could be large. The upsides are that - it makes the protocol more consistent with the current invariants maintained by "git fsck" - down the line, it should make operations like "fetch just this one tree" a little simpler, since you can use filter blob:none filter tree:none want Later we could introduce a separate kind of "want" for which filters apply to the object named directly, too, to provide more flexibility for the client. I can imagine cases where a person would want each of those two behaviors, so I don't think we are incurring unnecessary complexity by always sending the object named in a "want" without such an explicit filtering request. So I'm comfortable with the direction this series goes in, though I haven't looked at the patches in detail. Thanks, Jonathan
Re: [PATCH 1/3] ls-tree: make optional
On 07/06/2018 01:01 PM, Junio C Hamano wrote: > Elijah Newren writes: > >>> I'd prefer *not* to have such a DWIM in a command like ls-tree, aka >>> plumbing commands, where predictability is worth 1000 times more >>> than ease of typing. >> >> Fair enough. However, what if no or are specified, >> though -- would you be okay with the HEAD being assumed instead of >> erroring out in that case? > > If we wrote ls-tree to do so 12 years ago, then I wouldn't have > opposed. Changing the behaviour now? Not so sure if it is worth > having to worry about updating the code, docs and making sure we > spot all the possible typoes. > I have to say, as a first time contributor, reading this is extremely discouraging. I'm not being told the patch can be improved, or that I've made some error that should be corrected. I'm being told that the entire idea of the patch is unwanted, that it doesn't have a place in a mature project like git, that only bug fixes and security issues should be accepted. That makes me not want to contribute.
Re: [PATCH 0/2] Avoiding errors when partial cloning a tagged blob
> Hmph, the approach taken by these two patches smells bad. > > When a blob is deliberately omitted with --fitler=blob:none, the > fsck that encounters an entry in a tree object that is about that > expected-to-be-and-actually-is-missing blob does *not* complain and > that is by design, right? Naïvely, I may expect fsck to follow the > same principle--when it encounters a reference to an object that is > deliberately missing, refrain from complaining, regardless of the > place the offending reference was found, be it inside a tree object > or a ref. The difference between the two is that this tree object is from a packfile with an associated ".promisor" file, indicating that the destinations of any "dangling" links are promisor objects (and thus, fsck and others will not complain about those). We don't have such a mechanism for refs. This was a tradeoff between the desire to still detect repository corruption whenever we can, and to not impact performance greatly when operating on a partial repository. Besides the solution in this patch set, we could (1) allow any ref to point to a missing object, (2) allow a subset of refs to point to a missing object, or (3) have another place in $GIT_DIR that can record the hashes of promisor objects. (1) seems contrary to the repository corruption detection goal that we have. (2) is plausible, but we would have to figure out what to record - it's fine to record the RHS of the default refspec when we clone, but I don't know what we should record when the user subsequently runs "git fetch origin refs/heads/master:refs/heads/master". As for (3), that would require another repository extension since we wouldn't want an old Git to fetch into a new Git repository without updating the promisor-hashes file - but at this point, it seems easier to update the protocol instead. Any comments are welcome, and I'll think about this some more.
Re: de-alphabetizing the documentation
Jonathan Nieder wrote: > Ideas? If you start with a proposal, we're happy to help refine it. > People in the #git channel on irc.freenode.net (wechat.freenode.net) > might also be useful for inspiration in coming up with a proposal. I meant to link to webchat.freenode.net. But https://kiwiirc.com/nextclient/ may have been a better link to use anyway. Thanks and sorry for the noise, Jonathan
Re: de-alphabetizing the documentation
Hi Frederick, Frederick Eaton wrote: > I am trying to learn how to use Git but I've been put off by not > knowing where to start. I would like to start with the 'git' man page, > but it lists the Git subcommands in alphabetical order, rather than in > an order which would be useful for learners. For example, I'm not sure > how often 'git bisect' is used, but it is strange to see it listed > before 'git init' and 'git clone'. [...] > I wonder if someone familiar with Git could list the commands in an > order which makes more sense for learning, for example in the order in > which they were invented by Git developers, That doesn't seem like a useful order pedagogically, but > or in the reverse order of > frequency of use by a typical Git user. That does. Currently the commands are already broken into a few categories: High-level commands Main porcelain commands Ancillary commands Interacting with others Low-level commands Manipulation commands Interrogation commands Synching repositories Internal helper commands While it's alphabetical within each section, overall it is not alphabetical at all! Alas, there are plenty of "Main porcelain commands", and I think that is where your question comes from. It would be nicer to list just five to start, say. Some of the most thoughtful documentation that comes with Git is at https://www.kernel.org/pub/software/scm/git/docs/user-manual.html. It might be useful for inspiration. Ideas? If you start with a proposal, we're happy to help refine it. People in the #git channel on irc.freenode.net (wechat.freenode.net) might also be useful for inspiration in coming up with a proposal. Each of us have our weaknesses for this kind of work: you're telling me you're too new to have a sense of which commands are the first a person would need to learn (and I have no reason to doubt you), while many on this list would have the opposite problem of taking too much for granted and not being able to put themselves in the mind of a newcomer. So we'll have to help each other. tl/dr: if you come up with a proposed "first commands to learn" category with some proposed commands to go in it, we'll be happy to help you with the next steps. [...] > Finally, perhaps the same listing and/or reordering could be done for > other important manual pages, like 'gitglossary'. Presumably > 'gitglossary' should be sorted topologically, so that each term is > defined prior to any terms depending on it. Your help is welcome here as well. Probably a similar kind of categorization, with entries ordered either alphabetically or according to some narrative in each section, would be the easiest to maintain over time. Thanks, Jonathan
Re: [PATCH v3 1/4] rebase: start implementing it as a builtin
Pratik Karki writes:
> +static int use_builtin_rebase(void)
> +{
> + struct child_process cp = CHILD_PROCESS_INIT;
> + struct strbuf out = STRBUF_INIT;
> + int ret;
> +
> + argv_array_pushl(,
> + "config", "--bool", "rebase.usebuiltin", NULL);
> + cp.git_cmd = 1;
> + if (capture_command(, , 6))
> + return 0;
> +
> + strbuf_trim();
> + ret = !strcmp("true", out.buf);
> + strbuf_release();
> + return ret;
> +}
> +
> +int cmd_rebase(int argc, const char **argv, const char *prefix)
> +{
> + /*
> + * NEEDSWORK: Once the builtin rebase has been tested enough
> + * and git-legacy-rebase.sh is retired to contrib/, this preamble
> + * can be removed.
> + */
> +
> + if (!use_builtin_rebase()) {
> + const char *path = mkpath("%s/git-legacy-rebase",
> + git_exec_path());
> +
> + if (sane_execvp(path, (char **)argv) < 0)
> + die_errno("could not exec %s", path);
> + else
> + die("sane_execvp() returned???");
This part got changed from returning 0 since the previous round,
which makes sense, but if we are making this change relative to the
original in be8a90e, just because we mention explicitly that we
imitate that old commit, we must mention how and why we "improved"
on it, in the log message.
In the old original used while converting difftool, if
sane_execvp() that attempts to run the legacy scripted
version returned with non-negative status, the command
silently exited without doing anything with success, but
sane_execvp() should not retun with non-negative status in
the first place, so make sure we die() to notice such an
abnormal case.
or something like that, perhaps (but you should be able to shorten
it further).
Between die() and BUG(), I am not sure which is more appropriate.
It certainly is not _our_ bug if platform execvp() returns success,
so BUG() would not help our developers all that much. But end-users
won't be helped by being told that sane_execvp() returned, so die()
is not all that useful, either. I guess it does not matter that
much bewteen the two, as this is something that is not supposed to
happen anyway ;-)
In any case, this one looks good. Will queue.
Thanks.
de-alphabetizing the documentation
Dear Git Developers, I am trying to learn how to use Git but I've been put off by not knowing where to start. I would like to start with the 'git' man page, but it lists the Git subcommands in alphabetical order, rather than in an order which would be useful for learners. For example, I'm not sure how often 'git bisect' is used, but it is strange to see it listed before 'git init' and 'git clone'. I know that 'gittutorial' exists, and I've indeed worked through it and some other Git tutorials. However, these are more like lists of examples that should come at the end of a typical manual page. I don't think they are intended as canonical references for the program functionality. At some point I would like to use the main 'git' manual page as my primary source of documentation. This would be easier if it were designed to be read straight through. I wonder if someone familiar with Git could list the commands in an order which makes more sense for learning, for example in the order in which they were invented by Git developers, or in the reverse order of frequency of use by a typical Git user. This could easily be derived from the shell history of someone who is reading this. (cat ~/.bash_history; cut -d ";" -f 2 ~/.zsh_history) | grep '^git ' | cut -d ' ' -f 2 | sort | uniq -c | sort -nr I would like to have this list for personal use, but once it is created, perhaps someone could also add it to the main Git man page. The list could be added at the top of the "GIT COMMANDS" section. Alternatively, and I think preferably, the entries in that section could be rearranged into this order. Finally, perhaps the same listing and/or reordering could be done for other important manual pages, like 'gitglossary'. Presumably 'gitglossary' should be sorted topologically, so that each term is defined prior to any terms depending on it. I'm not sure why alphabetical order was decided upon, but it seems less than useful in the information age, when people can easily search for text electronically. Maybe at some point someone decided to "clean up" the presentation by alphabetizing the manual page. As a user who is more concerned with substance, it is annoying to encounter such an emphasis on the superficial, which seems unworthy of such an important piece of software. Thank you, Frederick Eaton
Re: [PATCH 7/8] gpg-interface: introduce new signature format "X509" using gpgsm
Henning Schild writes:
> -enum gpgformats { PGP_FMT };
> +enum gpgformats { PGP_FMT, X509_FMT };
> struct gpg_format_data gpg_formats[] = {
> { .format = "PGP", .program = "gpg",
> .extra_args_verify = { "--keyid-format=long", },
> .sigs = { PGP_SIGNATURE, PGP_MESSAGE, },
> },
> + { .format = "X509", .program = "gpgsm",
> + .extra_args_verify = { NULL },
> + .sigs = {X509_SIGNATURE, NULL, }
Missing SP between "{X" is a bit irritating.
Also the trailing comma (the issue is shared with the PGP side) when
the initializer is smashed on a single line feels pretty much
pointless. If it were multi-line, then such a trailing comma would
help future developers to add a new entry, i.e.
.sigs = {
PGP_SIGNATURE,
PGP_MESSAGE,
+ PGP_SOMETHING_NEW,
}
without touching the last existing entry. But on a single line?
-.sigs = { PGP_SIGNATURE, PGP_MESSAGE }
+.sigs = { PGP_SIGNATURE, PGP_MESSAGE, PGP_SOMETHING_NEW }
is probably prettier without such a trailing comma.
> @@ -190,6 +195,9 @@ int git_gpg_config(const char *var, const char *value,
> void *cb)
> if (!strcmp(var, "gpg.program"))
> return git_config_string(_formats[PGP_FMT].program, var,
>value);
> + if (!strcmp(var, "gpg.programX509"))
> + return git_config_string(_formats[X509_FMT].program, var,
> + value);
This is a git_config() callback, isn't it? A two-level variable
name is given to a callback after downcasing, so nothing will match
"gpg.programX509", I suspect. I see Brian already commented on the
name and the better organization being
- gpg.format defines 'openpgp' or whatever other values;
- gpg..program defines the actual program
where is the value gpg.format would take
(e.g. "gpg.openpgp.program = gnupg"). And I agree with these
suggestions.
Re: [RFC PATCH v5] Implement --first-parent for git rev-list --bisect
Hi Junio,
On Fri, 6 Jul 2018, Junio C Hamano wrote:
> Johannes Schindelin writes:
>
> >> > git rev-list --first-parent --bisect-all F..E >revs &&
> >> > test_line_count = 9 revs &&
> >> > for rev in E e1 e2 e3 e4 e5 e6 e7 e8
> >> > do
> >> > grep "^$(git rev-parse $rev) " revs ||
> >> > {
> >> > echo "$rev not shown" >&2 &&
> >> > return 1
> >> > }
> >> > done &&
> >> > sed -e "s/.*(dist=\([0-9]*\)).*/\1/" revs >actual.dists &&
> >> > sort -r actual.dists >actual.dists.sorted &&
> >> > test_cmp actual.dists.sorted actual.dists
> >>
> > From my point of view, this indicates that you want to set those exact
> > dist values in stone.
>
> As I already said, I do not think it is absolutely necessary to
> declare that the minimum dist is 0 or 1, or how big one step of dist
> is. For those reading from the sidelines, the history we are
> testing this new feature over looks like this
>
> # E dist=0
> #/ \
> # e1 | dist=1
> # | |
> # e2 | dist=2
> # | | ...
> # | | ...
> # e7 | dist=2
> # | |
> # e8 | dist=1
> #\ /
> # F dist=0
>
> Current code will say dist=0 for E and F, dist=1 for e1 and e8,
> etc., and I am fine if the code suddenly start saying that E and F
> (i.e. those at the boundary of the graph) have dist=1 and one hop
> weighs 10 so dist=11 for e1 and e8 (i.e. those at one hop from the
> boundary).
>
> But I am not fine if E and F get larger dist than e1 and e8, or e1
> and e8 get different ones. I do not think the code quoted upfront
> would catch such future breakages.
>
> And I also do not see a reason why somebody wants to make the dist
> computation to be 1-based (iow, changing the minimum from 0 to 1) or
> one step not to be 1 (iow, giving 11 to e1 and e8), so while I agree
> it is not strictly necessary to cast the concrete distance value in
> stone, I do not see much harm doing so *if* it helps to make it
> simpler the test that is necessary to make sure relative dist values
> assigned to these commits are in correct order.
I guess that you still want to misunderstand me.
Because it is not really hard to understand what I said: a good regression
test will verify precisely what you want to ensure, not precisely what the
command's output is.
So in this case, quite obviously what you want to do is to verify that E
and F get larger dist than e1 and e8. So that is what you test for. Not
some fixed text that might require adjusting in the future for any other
reason than a real bug.
Ciao,
Dscho
Re: [PATCH v3 0/2] grep.c: teach --only-matching to 'git-grep(1)'
On Fri, Jul 06, 2018 at 03:15:22PM -0500, Taylor Blau wrote: > On Fri, Jul 06, 2018 at 11:21:06AM -0700, Junio C Hamano wrote: > > Taylor Blau writes: > > > > > I think that this might be clear enough on its own, especially since > > > this is the same as BSD grep on my machine. I think that part_s_ of a > > > line indicates that behavior, but perhaps not. On GNU grep, this is: > > > > > > Print only the matched (non-empty) parts of a matching line, with each > > > such part on a separate output line. > > > > Interesting. I wonder what "git grep -o '^'" would do ;-) > > That invocation prints nothing, but on BSD grep it prints quite a few > blank lines :-). > > I'm hesitant on sending a patch per the hunk of your reply below because > of this. Should we mirror BSD grep's behavior exactly here? I suppose > that we could somehow, but it seems like we might be doing too much to > support what appears to me to be an odd use-case. IMHO the GNU behavior (omitting non-empty matches) makes more sense. And it's also what your patch already does. ;) Although amusingly "git grep -o ^" will still print a ton of "Binary file ... matches". That _also_ matches what GNU grep does. I'm not sure if there's a saner behavior (it really has nothing to do with the funny empty match; any binary file with -o cannot show the normal text line). > > In any case, I find that the GNU phrasing is the most clear among > > the ones I've seen in this thread so far. > > OK. I'm happy to re-send that patch with the GNU phrasing depending on > what others think (and the above). I'll let this cook and collect some > thoughts over the weekend. FWIW, I like the GNU phrasing. I thought the "non-empty" part was not all that interesting, but after hearing that BSD behaves differently, it is probably worth mentioning. I think the actual behavior of your patch matches GNU grep, and does not need changing. -Peff
Re: [PATCH 6/8] gpg-interface: do not hardcode the key string len anymore
Henning Schild writes:
> gnupg does print the keyid followed by a space and the signer comes
> next. The same pattern is also used in gpgsm, but there the key length
> would be 40 instead of 16. Instead of hardcoding the expected length,
> find the first space and calculate it.
>
> Signed-off-by: Henning Schild
> ---
> gpg-interface.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
Nicely explained and nicely implemented.
> diff --git a/gpg-interface.c b/gpg-interface.c
> index cd3b1b568..aa747278e 100644
> --- a/gpg-interface.c
> +++ b/gpg-interface.c
> @@ -88,10 +88,11 @@ static void parse_gpg_output(struct signature_check *sigc)
> sigc->result = sigcheck_gpg_status[i].result;
> /* The trust messages are not followed by key/signer
> information */
> if (sigc->result != 'U') {
> - sigc->key = xmemdupz(found, 16);
> + next = strchrnul(found, ' ');
> + sigc->key = xmemdupz(found, next - found);
> /* The ERRSIG message is not followed by signer
> information */
> if (sigc-> result != 'E') {
> - found += 17;
> + found = next + 1;
> next = strchrnul(found, '\n');
> sigc->signer = xmemdupz(found, next - found);
> }
Re: [PATCH 5/8] t/t7510: check the validation of the new config gpg.format
Henning Schild writes: > Valid values are already covered by all tests that use GPG, now also > test what happens if we go for an invalid one. > > Signed-off-by: Henning Schild > --- > t/t7510-signed-commit.sh | 10 ++ > 1 file changed, 10 insertions(+) > > diff --git a/t/t7510-signed-commit.sh b/t/t7510-signed-commit.sh > index 6e2015ed9..cb523513f 100755 > --- a/t/t7510-signed-commit.sh > +++ b/t/t7510-signed-commit.sh > @@ -227,4 +227,14 @@ test_expect_success GPG 'log.showsignature behaves like > --show-signature' ' > grep "gpg: Good signature" actual > ' > > +test_expect_success GPG 'check gpg config for malformed values' ' > + mv .git/config .git/config.old && > + test_when_finished "mv .git/config.old .git/config" && Hm. Is the damage caused by throwing a bad value at gpg.format designed to be so severe that "test_when_finished test_unconfig ..." cannot recover from? This test script is not about how "git config" is implemented and works, so it would be a good idea for it to be even oblivious to the fact that .git/config is the file being mucked with when we do "git config". I have a suspicion that you can just use test_config (which would arrange "test_when_finished test_unconfig ..." for free). > + git config gpg.format malformed && > + test_expect_code 128 git commit -S --amend -m "fail" 2>result && Is this 128 something we document and have users rely on? Or should we rather say test_must_fail git commit ... here instead? > + test_i18ngrep "malformed value for gpg.format: malformed" result && > + test_i18ngrep "fatal: .*\.git/config" result && > + test_i18ngrep "fatal: .*line 2" result > +' > + > test_done
Re: [PATCH 1/3] send-email: add an auto option for transfer encoding
On Fri, Jul 06, 2018 at 02:01:25AM -0400, Eric Sunshine wrote:
> On Thu, Jul 5, 2018 at 10:24 PM brian m. carlson
> wrote:
> > For most patches, using a transfer encoding of 8bit provides good
> > compatibility with most servers and makes it as easy as possible to view
> > patches. However, there are some patches for which 8bit is not a valid
> > encoding: RFC 5321 specifies that a message must not have lines
> > exceeding 998 octets.
> >
> > Add a transfer encoding value, auto, which indicates that a patch should
> > use 8bit where allowed and quoted-printable otherwise. Choose
> > quoted-printable instead of base64, since base64-encoded plain text is
> > treated as suspicious by some spam filters.
> >
> > Signed-off-by: brian m. carlson
> > ---
> > diff --git a/git-send-email.perl b/git-send-email.perl
> > @@ -1852,13 +1851,16 @@ sub apply_transfer_encoding {
> > + $to = ($message =~ /.{999,}/) ? 'quoted-printable' :'8bit'
> > + if $to eq 'auto';
>
> Style: space after colon: 'quoted-printable' : '8bit'
Will fix.
> > diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
> > +test_expect_success $PREREQ 'long lines with auto encoding are
> > quoted-printable' '
> > + clean_fake_sendmail &&
> > + git send-email \
> > + --from="Example " \
> > + --to=nob...@example.com \
> > + --smtp-server="$(pwd)/fake.sendmail" \
> > + --transfer-encoding=auto \
> > + --no-validate \
> > + longline.patch \
> > + 2>errors &&
>
> Why capture stderr to a file then ignore the file?
Copy-paste error from an earlier test. Will fix.
--
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204
signature.asc
Description: PGP signature
Re: [PATCH 2/3] send-email: accept long lines with suitable transfer encoding
On Fri, Jul 06, 2018 at 08:26:04AM -0400, Drew DeVault wrote:
> On 2018-07-06 2:23 AM, brian m. carlson wrote:
> > diff --git a/git-send-email.perl b/git-send-email.perl
> > index a76953c310..4ea30c4070 100755
> > --- a/git-send-email.perl
> > +++ b/git-send-email.perl
> > @@ -1899,6 +1899,10 @@ sub validate_patch {
> > return $hook_error if $hook_error;
> > }
> >
> > + # Any long lines will be automatically fixed if we use a suitable
> > transfer
> > + # encoding.
> > + return if $xfer_encoding =~ /^(?:auto|quoted-printable|base64)$/;
>
> Rather than returning in this case I'd sooner wrap the length check in
> this test. If additional checks are added in the future it'd be too easy
> to accidentally skip them if the transfer encoding is quoted-printable.
Okay, that's fair. I can do that.
--
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204
signature.asc
Description: PGP signature
Re: [PATCH v3 0/2] grep.c: teach --only-matching to 'git-grep(1)'
On Fri, Jul 06, 2018 at 11:21:06AM -0700, Junio C Hamano wrote: > Taylor Blau writes: > > > I think that this might be clear enough on its own, especially since > > this is the same as BSD grep on my machine. I think that part_s_ of a > > line indicates that behavior, but perhaps not. On GNU grep, this is: > > > > Print only the matched (non-empty) parts of a matching line, with each > > such part on a separate output line. > > Interesting. I wonder what "git grep -o '^'" would do ;-) That invocation prints nothing, but on BSD grep it prints quite a few blank lines :-). I'm hesitant on sending a patch per the hunk of your reply below because of this. Should we mirror BSD grep's behavior exactly here? I suppose that we could somehow, but it seems like we might be doing too much to support what appears to me to be an odd use-case. > > I'm happy to pick either and re-send this patch (2/2) again, if it > > wouldn't be too much to juggle. Otherwise, I can re-roll to v4. > > Please do not re-send a different version of a patch with the same > v$n value. Either re-send, otherwise re-roll, will give us v4, not > v3. > > In any case, I find that the GNU phrasing is the most clear among > the ones I've seen in this thread so far. OK. I'm happy to re-send that patch with the GNU phrasing depending on what others think (and the above). I'll let this cook and collect some thoughts over the weekend. Thanks, Taylor
Re: [PATCH 0/2] Avoiding errors when partial cloning a tagged blob
Jonathan Tan writes: >> When cloning a repository with a tagged blob (like the Git repository) >> with --filter=blob:none, the following message appears: >> >> error: missing object referenced by 'refs/tags/junio-gpg-pub' >> >> and the resulting repository also fails fsck. Hmph, the approach taken by these two patches smells bad. When a blob is deliberately omitted with --fitler=blob:none, the fsck that encounters an entry in a tree object that is about that expected-to-be-and-actually-is-missing blob does *not* complain and that is by design, right? Naïvely, I may expect fsck to follow the same principle--when it encounters a reference to an object that is deliberately missing, refrain from complaining, regardless of the place the offending reference was found, be it inside a tree object or a ref. Perhaps that line of consistent behaviour may be impossible due to the way the reference is stored inside a tree object and a ref? E.g. a reference to an expected-to-be-missing blob still lets us know that the missing object is expected to be a blob, but a ref only stores the object name and not its type, so we can tell it is missing, but we cannot say it is OK to be missing because we expect it to be a blob, or something?
Re: [PATCH 3/8] gpg-interface: add new config to select how to sign a commit
"brian m. carlson" writes: > On Tue, Jul 03, 2018 at 02:38:15PM +0200, Henning Schild wrote: >> Add "gpg.format" where the user can specify which type of signature to >> use for commits. At the moment only "PGP" is supported and the value is >> not even used. This commit prepares for a new types of signatures. > > We typically prefer to have option values specified in lower case. I > also think "openpgp" might be better than "PGP", since that's the name > of the specification and it would avoid any potential unhappiness about > compatibility with PGP or trademarks. I do not know about trademarks, but the suggested "'openpgp' in lowercase" sounds sensible. Some people might even favor doing strcasecmp() on the value; I do not have a strong opinion on that yet.
Re: [PATCH 1/8] builtin/receive-pack: use check_signature from gpg-interface
Henning Schild writes:
> The combination of verify_signed_buffer followed by parse_gpg_output is
> available as check_signature. Use that instead of implementing it again.
>
> Signed-off-by: Henning Schild
> ---
Makes sense.
When d05b9618 ("receive-pack: GPG-validate push certificates",
2014-08-14) implemented the check, there wasn't check_signature()
available. The commit probably should have done what a4cc18f2
("verify-tag: share code with verify-commit", 2015-06-21) later did
to introduce the check_signature() function by factoring it out of
commit.c::check_commit_signature() as a preparatory step.
Will queue. Thanks.
> builtin/receive-pack.c | 17 ++---
> 1 file changed, 2 insertions(+), 15 deletions(-)
>
> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
> index 68d36e0a5..9f0583deb 100644
> --- a/builtin/receive-pack.c
> +++ b/builtin/receive-pack.c
> @@ -629,8 +629,6 @@ static void prepare_push_cert_sha1(struct child_process
> *proc)
> return;
>
> if (!already_done) {
> - struct strbuf gpg_output = STRBUF_INIT;
> - struct strbuf gpg_status = STRBUF_INIT;
> int bogs /* beginning_of_gpg_sig */;
>
> already_done = 1;
> @@ -639,22 +637,11 @@ static void prepare_push_cert_sha1(struct child_process
> *proc)
> oidclr(_cert_oid);
>
> memset(, '\0', sizeof(sigcheck));
> - sigcheck.result = 'N';
>
> bogs = parse_signature(push_cert.buf, push_cert.len);
> - if (verify_signed_buffer(push_cert.buf, bogs,
> - push_cert.buf + bogs, push_cert.len -
> bogs,
> - _output, _status) < 0) {
> - ; /* error running gpg */
> - } else {
> - sigcheck.payload = push_cert.buf;
> - sigcheck.gpg_output = gpg_output.buf;
> - sigcheck.gpg_status = gpg_status.buf;
> - parse_gpg_output();
> - }
> + check_signature(push_cert.buf, bogs, push_cert.buf + bogs,
> + push_cert.len - bogs, );
>
> - strbuf_release(_output);
> - strbuf_release(_status);
> nonce_status = check_nonce(push_cert.buf, bogs);
> }
> if (!is_null_oid(_cert_oid)) {
Re: ag/rebase-i-rewrite-todo, was Re: What's cooking in git.git (Jun 2018, #07; Thu, 28)
Johannes Schindelin writes: > Of course, at that point I will have to look through those 7 patches > again, if only to verify that yes, they are still the same. That is something the patch author must help the reviewer with, no? Have uncontroversial stuff early in the series, concentrate on stabilizing them before moving on to shiny new toys, then mark them "unchanged since the last round" after three dashes when sending a reroll to update later parts of the series that are in flux. After a few rounds, it may become apparent to reviewers that these early parts can stand on their own merit as a separate series, on top of which the remaining patches can build as a separate (sub)topic, at which time we may have two or more topics, among which the early one that has become stable may already be 'next'. > And Alban is not sitting on his hands, either. If you are saying that all of these 13 are constantly in flux, whether these 13 patches are spread across 3 series or a single one (assuming that all 13 are about the same topic that are interdependant), the need to look at their updated incarnation does not change, so I do not know what you are complaining about. > So after reviewing those 13 patches, which undoubtedly will not be > integrated into `next` under the premise that they are still in flux, they > will most likely be joined by another dozen patches until the interactive > rebase is rewritten completely in C. After which time, I will have > reviewed the first 3 patches over 15 times. > > I wish there was a better way.
Re: [PATCH 0/2] Avoiding errors when partial cloning a tagged blob
> When cloning a repository with a tagged blob (like the Git repository) > with --filter=blob:none, the following message appears: > > error: missing object referenced by 'refs/tags/junio-gpg-pub' > > and the resulting repository also fails fsck. > > Patch 1 fixes the protocol documentation and the server side of Git, and > patch 2 makes clone error out when such a situation occurs. > > An argument could be made that we should not merge patch 2 just yet due > to the fact that some server implementations (such as Git and JGit) > still exhibit the old behavior, and the resulting clones (albeit failing > fsck) are still usable, because when attempting to load the blob, Git > will automatically fetch it. I'm on the fence about this, and have > included patch 2 in this patch set nevertheless for completeness. > > Jonathan Tan (2): > upload-pack: send refs' objects despite "filter" > clone: check connectivity even if clone is partial Forgot to mention: this patch set is based on bw/ref-in-want because I needed its one-time-sed functionality.
[PATCH 0/2] Avoiding errors when partial cloning a tagged blob
When cloning a repository with a tagged blob (like the Git repository) with --filter=blob:none, the following message appears: error: missing object referenced by 'refs/tags/junio-gpg-pub' and the resulting repository also fails fsck. Patch 1 fixes the protocol documentation and the server side of Git, and patch 2 makes clone error out when such a situation occurs. An argument could be made that we should not merge patch 2 just yet due to the fact that some server implementations (such as Git and JGit) still exhibit the old behavior, and the resulting clones (albeit failing fsck) are still usable, because when attempting to load the blob, Git will automatically fetch it. I'm on the fence about this, and have included patch 2 in this patch set nevertheless for completeness. Jonathan Tan (2): upload-pack: send refs' objects despite "filter" clone: check connectivity even if clone is partial Documentation/technical/pack-protocol.txt | 4 +- builtin/clone.c | 2 +- list-objects.c| 6 +-- object.h | 2 +- revision.c| 1 + revision.h| 3 +- t/t5317-pack-objects-filter-objects.sh| 16 ++ t/t5616-partial-clone.sh | 64 +++ 8 files changed, 91 insertions(+), 7 deletions(-) -- 2.18.0.203.gfac676dfb9-goog
[PATCH 1/2] upload-pack: send refs' objects despite "filter"
A filter line in a request to upload-pack filters out objects regardless
of whether they are directly referenced by a "want" line or not. This
means that cloning with "--filter=blob:none" (or another filter that
excludes blobs) from a repository with at least one ref pointing to a
blob (for example, the Git repository itself) results in output like the
following:
error: missing object referenced by 'refs/tags/junio-gpg-pub'
and if that particular blob is not referenced by a fetched tree, the
resulting clone fails fsck because there is no object from the remote to
vouch that the missing object is a promisor object.
Update both the protocol and the upload-pack implementation to include
all explicitly specified "want" objects in the packfile regardless of
the filter specification.
Signed-off-by: Jonathan Tan
---
Documentation/technical/pack-protocol.txt | 4 +++-
list-objects.c| 6 +++---
object.h | 2 +-
revision.c| 1 +
revision.h| 3 ++-
t/t5317-pack-objects-filter-objects.sh| 16
t/t5616-partial-clone.sh | 16
7 files changed, 42 insertions(+), 6 deletions(-)
diff --git a/Documentation/technical/pack-protocol.txt
b/Documentation/technical/pack-protocol.txt
index 7fee6b780a..508a344cf1 100644
--- a/Documentation/technical/pack-protocol.txt
+++ b/Documentation/technical/pack-protocol.txt
@@ -284,7 +284,9 @@ information is sent back to the client in the next step.
The client can optionally request that pack-objects omit various
objects from the packfile using one of several filtering techniques.
These are intended for use with partial clone and partial fetch
-operations. See `rev-list` for possible "filter-spec" values.
+operations. An object that does not meet a filter-spec value is
+omitted unless explicitly requested in a 'want' line. See `rev-list`
+for possible filter-spec values.
Once all the 'want's and 'shallow's (and optional 'deepen') are
transferred, clients MUST send a flush-pkt, to tell the server side
diff --git a/list-objects.c b/list-objects.c
index 3eec510357..be4118889a 100644
--- a/list-objects.c
+++ b/list-objects.c
@@ -47,7 +47,7 @@ static void process_blob(struct rev_info *revs,
pathlen = path->len;
strbuf_addstr(path, name);
- if (filter_fn)
+ if (!(obj->flags & USER_GIVEN) && filter_fn)
r = filter_fn(LOFS_BLOB, obj,
path->buf, >buf[pathlen],
filter_data);
@@ -132,7 +132,7 @@ static void process_tree(struct rev_info *revs,
}
strbuf_addstr(base, name);
- if (filter_fn)
+ if (!(obj->flags & USER_GIVEN) && filter_fn)
r = filter_fn(LOFS_BEGIN_TREE, obj,
base->buf, >buf[baselen],
filter_data);
@@ -171,7 +171,7 @@ static void process_tree(struct rev_info *revs,
cb_data, filter_fn, filter_data);
}
- if (filter_fn) {
+ if (!(obj->flags & USER_GIVEN) && filter_fn) {
r = filter_fn(LOFS_END_TREE, obj,
base->buf, >buf[baselen],
filter_data);
diff --git a/object.h b/object.h
index 5c13955000..cf8da6ba83 100644
--- a/object.h
+++ b/object.h
@@ -27,7 +27,7 @@ struct object_array {
/*
* object flag allocation:
- * revision.h: 0-1026
+ * revision.h: 0-10 2526
* fetch-pack.c: 05
* walker.c: 0-2
* upload-pack.c:4 1119
diff --git a/revision.c b/revision.c
index 40fd91ff2b..1b37da988d 100644
--- a/revision.c
+++ b/revision.c
@@ -172,6 +172,7 @@ static void add_pending_object_with_path(struct rev_info
*revs,
strbuf_release();
return; /* do not add the commit itself */
}
+ obj->flags |= USER_GIVEN;
add_object_array_with_path(obj, name, >pending, mode, path);
}
diff --git a/revision.h b/revision.h
index b8c47b98e2..dc8f96366e 100644
--- a/revision.h
+++ b/revision.h
@@ -19,8 +19,9 @@
#define SYMMETRIC_LEFT (1u<<8)
#define PATCHSAME (1u<<9)
#define BOTTOM (1u<<10)
+#define USER_GIVEN (1u<<25) /* given directly by the user */
#define TRACK_LINEAR (1u<<26)
-#define ALL_REV_FLAGS (((1u<<11)-1) | TRACK_LINEAR)
+#define ALL_REV_FLAGS (((1u<<11)-1) | USER_GIVEN | TRACK_LINEAR)
#define DECORATE_SHORT_REFS1
#define DECORATE_FULL_REFS 2
diff --git a/t/t5317-pack-objects-filter-objects.sh
b/t/t5317-pack-objects-filter-objects.sh
index 1b0acc383b..6710c8bc8c 100755
--- a/t/t5317-pack-objects-filter-objects.sh
+++ b/t/t5317-pack-objects-filter-objects.sh
@@ -160,6 +160,22 @@ test_expect_success 'verify
[PATCH 2/2] clone: check connectivity even if clone is partial
The commit that introduced the partial clone feature - 548719fbdc
("clone: partial clone", 2017-12-08) - excluded connectivity checks
for partial clones, but this also meant that it is possible for a clone
to succeed, yet not have all objects either present or promised.
Specifically, if cloning with --filter=blob:none from a repository that
has a tag pointing to a blob, and the blob is not sent in the packfile,
the clone will pass, even if the blob is not referenced by any tree in
the packfile.
Turn on connectivity checks for partial clone.
Signed-off-by: Jonathan Tan
---
builtin/clone.c | 2 +-
t/t5616-partial-clone.sh | 48
2 files changed, 49 insertions(+), 1 deletion(-)
diff --git a/builtin/clone.c b/builtin/clone.c
index 8f86d99c51..fa53550758 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -1201,7 +1201,7 @@ int cmd_clone(int argc, const char **argv, const char
*prefix)
update_remote_refs(refs, mapped_refs, remote_head_points_at,
branch_top.buf, reflog_msg.buf, transport,
- !is_local && !filter_options.choice);
+ !is_local);
update_head(our_head_points_at, remote_head, reflog_msg.buf);
diff --git a/t/t5616-partial-clone.sh b/t/t5616-partial-clone.sh
index 8a2bf86491..44d8e80171 100755
--- a/t/t5616-partial-clone.sh
+++ b/t/t5616-partial-clone.sh
@@ -170,4 +170,52 @@ test_expect_success 'partial clone fetches blobs pointed
to by refs even if norm
git -C dst fsck
'
+. "$TEST_DIRECTORY"/lib-httpd.sh
+start_httpd
+
+# Converts bytes into a form suitable for inclusion in a sed command. For
+# example, "printf 'ab\r\n' | hex_unpack" results in '\x61\x62\x0d\x0a'.
+sed_escape () {
+ perl -e '$/ = undef; $input = <>; print unpack("H2" x length($input),
$input)' |
+ sed 's/\(..\)/\\x\1/g'
+}
+
+test_expect_success 'upon cloning, check that all refs point to objects' '
+ SERVER="$HTTPD_DOCUMENT_ROOT_PATH/server" &&
+ rm -rf "$SERVER" repo &&
+ test_create_repo "$SERVER" &&
+ test_commit -C "$SERVER" foo &&
+ test_config -C "$SERVER" uploadpack.allowfilter 1 &&
+ test_config -C "$SERVER" uploadpack.allowanysha1inwant 1 &&
+
+ # Create a tag pointing to a blob.
+ BLOB=$(echo blob-contents | git -C "$SERVER" hash-object --stdin -w) &&
+ git -C "$SERVER" tag myblob "$BLOB" &&
+
+ # Craft a packfile not including that blob.
+ git -C "$SERVER" rev-parse HEAD |
+ git -C "$SERVER" pack-objects --stdout >incomplete.pack &&
+
+ # Replace the existing packfile with the crafted one. The protocol
+ # requires that the packfile be sent in sideband 1, hence the extra
+ # \x01 byte at the beginning.
+ printf "1,/packfile/!c %04xx01%s" \
+ "$(($(wc -c "$HTTPD_ROOT_PATH/one-time-sed" &&
+
+ # Use protocol v2 because the sed command looks for the "packfile"
+ # section header.
+ test_config -C "$SERVER" protocol.version 2 &&
+ test_must_fail git -c protocol.version=2 clone \
+ --filter=blob:none $HTTPD_URL/one_time_sed/server repo 2>err &&
+
+ grep "did not send all necessary objects" err &&
+
+ # Ensure that the one-time-sed script was used.
+ ! test -e "$HTTPD_ROOT_PATH/one-time-sed"
+'
+
+stop_httpd
+
test_done
--
2.18.0.203.gfac676dfb9-goog
Re: [PATCH] builtin/config: work around an unsized array forward declaration
Jeff King writes: > On Thu, Jul 05, 2018 at 09:50:53PM +0200, Beat Bolli wrote: > >> > Your patch is obviously correct, but I think here there might be an even >> > simpler solution: just bump option_parse_type() below the declaration, >> > since it's the only one that needs it. That hunk is bigger, but the >> > overall diff is simpler, and we don't need to carry that extra wrapper >> > function. >> >> That was dscho's first try in the GitHub issue. It doesn't compile >> because the OPT_CALLBACK* macros in the builtin_config_options >> declaration inserts a pointer to option_parse_type into the array items. >> We need at least one forward declaration, and my patch seemed the least >> intrusive. > > Ah, right, so it actually is mutually recursive. Forward-declaring > option_parse_type() would fix it, along with the reordering. I'm > ambivalent between the available options, then; we might as well go with > what you posted, then, since it's already done. :) Among three, forward declaration of the function with reordering that nobody has written except for in the brain smells the best, and turning an array to a pointer that points at a separate storage looked the worst. I also am OK with what's already posted, too. Thanks.
Re: ag/rebase-i-rewrite-todo, was Re: What's cooking in git.git (Jun 2018, #07; Thu, 28)
Hi Junio,
On Fri, 6 Jul 2018, Junio C Hamano wrote:
> Johannes Schindelin writes:
>
> > I would *strongly* encourage you to allow Alban to go back to the small,
> > incremental patch series he sent before, because it will make it
> > *substantially* easier to not only review, but also develop, and for
> > you to merge.
>
> An organization in which you can make sure that the order of dependency
> and which ones have been updated since previous rounds are clear, even
> to those who are looking from the sidelines ("these 4 patches are to
> replace patch 3, 7 and 8 from the previous round" is already hostile to
> late reviewers and doing so without a pointer to the archive is even
> worse---a full reroll with the unchanged ones marked below the
> three-dash lines would be perfect), would be good. A random collection
> of seemingly separate but actually interdependent topics is very hard to
> work with with limited mental bandwidth.
>
> Once the core of _a_ topic hits 'next', we can go incremental (because
> by definition things get quiet and require small updates by then), but
> not before.
>
> I think the 7 patches in ag/rebase-i-in-c are more or less in good
> shape, modulo the issues pointed out on the list yet to be addressed,
> which I do not think require redesign. Which is good.
You do understand that with your proposed "let's just roll them up into
one big patch series, and just add freely whatever you need on top", these
7 patches (3 of which I reviewed I think four times on the list now, and
more times on GitHub, which is quite taxing on my time) will be soon
joined by 6 more patches: https://github.com/git/git/pull/518
Of course, at that point I will have to look through those 7 patches
again, if only to verify that yes, they are still the same.
And Alban is not sitting on his hands, either.
So after reviewing those 13 patches, which undoubtedly will not be
integrated into `next` under the premise that they are still in flux, they
will most likely be joined by another dozen patches until the interactive
rebase is rewritten completely in C. After which time, I will have
reviewed the first 3 patches over 15 times.
I wish there was a better way.
Ciao,
Dscho
Re: [PATCH v2 06/24] multi-pack-index: load into memory
Eric Sunshine writes:
> On Thu, Jul 5, 2018 at 10:20 AM Derrick Stolee wrote:
>> On 6/25/2018 3:38 PM, Junio C Hamano wrote:
>> While I don't use substitutions in this patch, I do use them in later
>> patches. Here is the final version of this method:
>>
>> midx_read_expect () {
>> NUM_PACKS=$1
>> NUM_OBJECTS=$2
>> NUM_CHUNKS=$3
>> EXTRA_CHUNKS="$5"
>> cat >expect <<-\EOF
>> header: 4d494458 1 $NUM_CHUNKS $NUM_PACKS
>> chunks: pack_names oid_fanout oid_lookup
>> object_offsets$EXTRA_CHUNKS
>> num_objects: $NUM_OBJECTS
>> packs:
>> EOF
>>
>> Using <<-\EOF causes these substitutions to fail. Is there a different
>> way I should construct this method?
>
> When you need to interpolate variables into the here-doc, use <<-EOF;
> when you don't, use <<-\EOF.
I think what was said is "in an early step there is no need to
interpolate but the same here-doc will need substitution in a later
step, and that is why I started an early step with a form without
quoting", which is different from "when should I use the form with
and without quoting?"
I think a reasonable response would have been "then please do use
the quoted form in the early step to help reviewers to let them know
there is not yet any substitutions, and then switch to quote-less form
at the step that starts needing substitution. That way, reviewers can
see the test started to become "interesting" by interpolating variable
bits in the test vector by seeing the line with "<
Re: [PATCH 1/2] l10n: fr: fix a message seen in git bisect
Jean-Noël Avila writes: > OK, Will queue this patch for next round of translation Thanks for an Ack.
Re: [GSoC][PATCH v2 6/7] rebase -i: rewrite setup_reflog_action() in C
Hi Junio,
On Fri, 6 Jul 2018, Junio C Hamano wrote:
> Johannes Schindelin writes:
>
> >> I thought we made this into
> >>
> >>if verbose
> >>return run_command
> >>else
> >>return run_command_silently
> >>
> >> to help readers in the previous round already.
> >
> > FWIW we had quite a couple of reviews in the recent years which pointed
> > out "unnecessary else" after returning or die()ing. Maybe we should make
> > up our minds, and set a consistent rule to follow.
>
> FWIW the pattern you are referring to is
>
> do something;
> if (error detected) {
> return error(...);
> } else {
> perform
> remaining
> actions
> the function needs
> to complete
> its task;
> }
>
> and those reviewers (including me) are absolutely right that such an
> "else" is not just unnecessary but actively harms readability of the
> flow of logic.
>
> I am also absolutely right when I say what is quoted at the top
> makes 100% more sense than
>
> if (verbose)
> return run_command();
> return run_command_silently();
>
> as these two are about doing the same thing a bit differently.
>
> The way to think about the latter is that we won't have this "if
> (verbose)" if there were a variant of run_command() that took a set
> of option bits among which is a verbose bit, but instead would have
> a single call to that function that returns.. So it's not like "in
> an exceptional case, return after calling this function; otherwise
> keep going, which happens to only return after calling another". It
> is more like "here we need to return after spawning a command, but
> depending on this bit, we spawn the command using a different
> function".
>
> Good programers recognize the difference between these two patterns
> without being told, and mentors should teach and GSoC student should
> learn that an overly simplified rule like "when 'if' block ends with
> return, do not write 'else'" is harmful.
You recently also suggested this if...else... dance to Pratik, where it
was not at all about doing the same thing slightly differently, but rather
two different things: 1) return an error because execvp() returned an
error, 2) indicate a serious bug (and you did not even suggest using BUG()
IIRC which is also wrong).
Maybe I am the only one who finds this inconsistent and confusing. If that
is that case, I'll quiet down.
Ciao,
Johannes
Re: [PATCH] git-rebase--{interactive,preserve-merges}: fix formatting of todo help message
Tobias Klauser writes:
> And I'm glad to see that this part will be rewritten in C :-)
>
>> However, your patch also covers this:
>>
>> > git-rebase--preserve-merges.sh | 4 ++--
>>
>> I completely missed that in the earlier patch.
>>
>> Junio, this gets an ACK from me, could you apply the
>> `git-rebase--preserve-merges.sh` part selectively, please?
Here is what I assembled from various pieces in the list archive.
-- >8 --
Subject: [PATCH] git-rebase--preserve-merges: fix formatting of todo help
message
Part of the todo help message in git-rebase--preserve-merges.sh is
unnecessarily indented, making the message look weird. Remove the
extra lines and trailing indent.
This was a minor regression introduced by d48f97aa ("rebase:
reindent function git_rebase__interactive", 2018-03-23) in the 2.18
timeframe. The same issue exists in "rebase -i", but it is being
addressed separately as part of the rewrite of the subcommand into C.
Signed-off-by: Tobias Klauser
Reviewed-by: Johannes Schindelin
Signed-off-by: Junio C Hamano
---
git-rebase--preserve-merges.sh | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/git-rebase--preserve-merges.sh b/git-rebase--preserve-merges.sh
index c51c7828e7..c214c5e4d6 100644
--- a/git-rebase--preserve-merges.sh
+++ b/git-rebase--preserve-merges.sh
@@ -891,9 +891,9 @@ $comment_char $(eval_ngettext \
EOF
append_todo_help
gettext "
- However, if you remove everything, the rebase will be aborted.
+However, if you remove everything, the rebase will be aborted.
- " | git stripspace --comment-lines >>"$todo"
+" | git stripspace --comment-lines >>"$todo"
if test -z "$keep_empty"
then
Re: [PATCH v3 0/2] grep.c: teach --only-matching to 'git-grep(1)'
Taylor Blau writes: > I think that this might be clear enough on its own, especially since > this is the same as BSD grep on my machine. I think that part_s_ of a > line indicates that behavior, but perhaps not. On GNU grep, this is: > > Print only the matched (non-empty) parts of a matching line, with each > such part on a separate output line. Interesting. I wonder what "git grep -o '^'" would do ;-) > I'm happy to pick either and re-send this patch (2/2) again, if it > wouldn't be too much to juggle. Otherwise, I can re-roll to v4. Please do not re-send a different version of a patch with the same v$n value. Either re-send, otherwise re-roll, will give us v4, not v3. In any case, I find that the GNU phrasing is the most clear among the ones I've seen in this thread so far.
Re: [PATCH v2] grep.c: teach 'git grep --only-matching'
Taylor Blau writes: > On Mon, Jun 25, 2018 at 02:59:07PM -0500, Taylor Blau wrote: >> Teach 'git grep --only-matching', a new option to only print the >> matching part(s) of a line. >> >> [ ... ] >> >> diff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt >> index 0de3493b80..078b4e3730 100644 >> --- a/Documentation/git-grep.txt >> +++ b/Documentation/git-grep.txt >> @@ -17,7 +17,7 @@ SYNOPSIS >> [-l | --files-with-matches] [-L | --files-without-match] >> [(-O | --open-files-in-pager) []] >> [-z | --null] >> - [-c | --count] [--all-match] [-q | --quiet] >> + [ -o | --only-matching ] [-c | --count] [--all-match] [-q | --quiet] >> [--max-depth ] >> [--color[=] | --no-color] >> [--break] [--heading] [-p | --show-function] >> @@ -201,6 +201,10 @@ providing this option will cause it to die. >> Output \0 instead of the character that normally follows a >> file name. >> >> +-o:: >> +--only-matching:: >> +Output only the matching part of the lines. > > Junio, > > My apologies that I sent the previous patch with incorrect indentation on this > line. Would you mind queueing this one instead? Surely, and thanks for telling me what difference to look for between the versions. Will replace with the one with indent before "Output only ...".
Re: [RFC PATCH v5] Implement --first-parent for git rev-list --bisect
Johannes Schindelin writes:
>> > git rev-list --first-parent --bisect-all F..E >revs &&
>> > test_line_count = 9 revs &&
>> > for rev in E e1 e2 e3 e4 e5 e6 e7 e8
>> > do
>> > grep "^$(git rev-parse $rev) " revs ||
>> > {
>> > echo "$rev not shown" >&2 &&
>> > return 1
>> > }
>> > done &&
>> > sed -e "s/.*(dist=\([0-9]*\)).*/\1/" revs >actual.dists &&
>> > sort -r actual.dists >actual.dists.sorted &&
>> > test_cmp actual.dists.sorted actual.dists
>>
> From my point of view, this indicates that you want to set those exact
> dist values in stone.
As I already said, I do not think it is absolutely necessary to
declare that the minimum dist is 0 or 1, or how big one step of dist
is. For those reading from the sidelines, the history we are
testing this new feature over looks like this
# E dist=0
#/ \
# e1 | dist=1
# | |
# e2 | dist=2
# | | ...
# | | ...
# e7 | dist=2
# | |
# e8 | dist=1
#\ /
# F dist=0
Current code will say dist=0 for E and F, dist=1 for e1 and e8,
etc., and I am fine if the code suddenly start saying that E and F
(i.e. those at the boundary of the graph) have dist=1 and one hop
weighs 10 so dist=11 for e1 and e8 (i.e. those at one hop from the
boundary).
But I am not fine if E and F get larger dist than e1 and e8, or e1
and e8 get different ones. I do not think the code quoted upfront
would catch such future breakages.
And I also do not see a reason why somebody wants to make the dist
computation to be 1-based (iow, changing the minimum from 0 to 1) or
one step not to be 1 (iow, giving 11 to e1 and e8), so while I agree
it is not strictly necessary to cast the concrete distance value in
stone, I do not see much harm doing so *if* it helps to make it
simpler the test that is necessary to make sure relative dist values
assigned to these commits are in correct order.
Re: [PATCH v2 00/10] rerere: handle nested conflicts
Thomas Gummerer writes: > On 06/05, Thomas Gummerer wrote: >> The previous round was at >> <20180520211210.1248-1-t.gumme...@gmail.com>. >> >> Thanks Junio for the comments on the previous round. >> >> Changes since v2: >> - lowercase the first letter in some error/warning messages before >>marking them for translation >> - wrap paths in output in single quotes, for consistency, and to make >>some of the messages the same as ones that are already translated >> - mark messages in builtin/rerere.c for translation as well, which I >>had previously forgotten. >> - expanded the technical documentation on rerere. The entire >>document is basically rewritten. >> - changed the test in 6/10 to just fake a conflict marker inside of >>one of the hunks instead of using an inner conflict created by a >>merge. This is to make sure the codepath is still hit after we >>handle inner conflicts properly. >> - added tests for handling inner conflict markers >> - added one commit to recalculate the conflict ID when an unresolved >>conflict is committed, and the subsequent operation conflicts again >>in the same file. More explanation in the commit message of that >>commit. > > Now that 2.18 is out (and I'm caught up on the list after being away > from it for a few days), is there any interest in this series? I guess > it was overlooked as it's been sent in the rc phase for 2.18. I deliberately ignored, not because I wasn't interested in it, but because I'd be distracted during the pre-release feature freeze as I'd be heavily intereseted in it. Now is a good time to repost to stir/re-ignite the interest from others, possibly after rebasing on v2.18.0 and polishing further. Thanks. > > I think the most important bit here is 6/10 which fixes a crash that > can happen in "normal" usage of git. The translation bits are also > nice to have I think, but I could send them in a different series if > that's preferred. > > The other patches would be nice to have, but are arguably less > important. > >> range-diff below. A few commits changed enough for range-diff >> to give up showing the differences in those, they are probably best >> reviewed as the whole patch anyway: >> >> [snip] >> >> Thomas Gummerer (10): >> rerere: unify error messages when read_cache fails >> rerere: lowercase error messages >> rerere: wrap paths in output in sq >> rerere: mark strings for translation >> rerere: add some documentation >> rerere: fix crash when conflict goes unresolved >> rerere: only return whether a path has conflicts or not >> rerere: factor out handle_conflict function >> rerere: teach rerere to handle nested conflicts >> rerere: recalculate conflict ID when unresolved conflict is committed >> >> Documentation/technical/rerere.txt | 182 + >> builtin/rerere.c | 4 +- >> rerere.c | 246 ++--- >> t/t4200-rerere.sh | 67 >> 4 files changed, 372 insertions(+), 127 deletions(-) >> create mode 100644 Documentation/technical/rerere.txt >> >> -- >> 2.18.0.rc1.242.g61856ae69 >>
Re: ag/rebase-i-rewrite-todo, was Re: What's cooking in git.git (Jun 2018, #07; Thu, 28)
Johannes Schindelin writes:
> The latest iteration of this is here:
> https://public-inbox.org/git/20180702105717.26386-5-alban.gr...@gmail.com/T/#r8eea71077745d6f2c839acb6200bb8b2bea579d3
Good. I think we have it in tree now.
> I would *strongly* encourage you to allow Alban to go back to the small,
> incremental patch series he sent before, because it will make it
> *substantially* easier to not only review, but also develop, and for you
> to merge.
An organization in which you can make sure that the order of
dependency and which ones have been updated since previous rounds
are clear, even to those who are looking from the sidelines ("these
4 patches are to replace patch 3, 7 and 8 from the previous round"
is already hostile to late reviewers and doing so without a pointer
to the archive is even worse---a full reroll with the unchanged ones
marked below the three-dash lines would be perfect), would be good.
A random collection of seemingly separate but actually
interdependent topics is very hard to work with with limited mental
bandwidth.
Once the core of _a_ topic hits 'next', we can go incremental
(because by definition things get quiet and require small updates by
then), but not before.
I think the 7 patches in ag/rebase-i-in-c are more or less in good
shape, modulo the issues pointed out on the list yet to be
addressed, which I do not think require redesign. Which is good.
Re: [PATCH 4/8] gpg-interface: introduce an abstraction for multiple gpg formats
Martin Ågren writes:
>> +enum gpgformats { PGP_FMT };
>> +struct gpg_format_data gpg_formats[] = {
>> + { .format = "PGP", .program = "gpg",
>> + .extra_args_verify = { "--keyid-format=long", },
>> + .sigs = { PGP_SIGNATURE, PGP_MESSAGE, },
>> + },
>> +};
>
> I think those trailing commas are ok now, but I'm not sure...
>
> I had the same thought about designated initializers. Those should be ok
> now, c.f. cbc0f81d96 (strbuf: use designated initializers in STRBUF_INIT,
> 2017-07-10) and a73b3680c4 (Add and use generic name->id mapping code
> for color slot parsing, 2018-05-26).
As you said, we dipped our toes in designated initializers in both
struct and array, i.e. { .field = init }, { [offset] = init } last
summer and we haven't got complaints from minor platforms so far.
The "comma" thing you are wondering is something else. The comma we
see above is after the last element in an array's initializer (and
the last element in a struct's initializer), which we have been
happily using from very early days (and they are kosher ANSI C).
What we've been avoiding was the comma after the last element in the
enum (in other words, if PGP_FMT had ',' after it in the above
quoted addition, that would have been violation of that rule), as
having such a trailing comma used to be ANSI C violation as well. I
do not recall offhand if we loosened that deliberately.
4b05548f ("enums: omit trailing comma for portability", 2010-05-14),
c9b6782a ("enums: omit trailing comma for portability", 2011-03-16)
Re: [PATCH 1/3] ls-tree: make optional
Elijah Newren writes: >> I'd prefer *not* to have such a DWIM in a command like ls-tree, aka >> plumbing commands, where predictability is worth 1000 times more >> than ease of typing. > > Fair enough. However, what if no or are specified, > though -- would you be okay with the HEAD being assumed instead of > erroring out in that case? If we wrote ls-tree to do so 12 years ago, then I wouldn't have opposed. Changing the behaviour now? Not so sure if it is worth having to worry about updating the code, docs and making sure we spot all the possible typoes.
Re: [PATCH] fast-import: Don't count delta attempts against an empty buffer
Mike Hommey writes:
> On Tue, Jul 03, 2018 at 11:41:42AM -0700, Junio C Hamano wrote:
>> Mike Hommey writes:
>>
>> > When the reference buffer is empty, diff_delta returns NULL without
>> > really attempting anything, yet fast-import counts that as a delta
>> > attempt.
>>
>> But that is an attempt nevertheless, no? Attempted and failed to
>> find anything useful, that is. What problem are you trying to solve
>> and what issue are you trying to address, exactly?
>>
>> ... goes and reads the patch ...
>>
>> > Signed-off-by: Mike Hommey
>> > ---
>> > fast-import.c | 2 +-
>> > 1 file changed, 1 insertion(+), 1 deletion(-)
>> >
>> > diff --git a/fast-import.c b/fast-import.c
>> > index 4d55910ab9..12195d54d7 100644
>> > --- a/fast-import.c
>> > +++ b/fast-import.c
>> > @@ -1076,7 +1076,7 @@ static int store_object(
>> >return 1;
>> >}
>> >
>> > - if (last && last->data.buf && last->depth < max_depth
>> > + if (last && last->data.len && last->data.buf && last->depth < max_depth
>> >&& dat->len > the_hash_algo->rawsz) {
>> >
>> >delta_count_attempts_by_type[type]++;
>>
>> This is a misleading patch as the title and the proposed log message
>> both talk about "attempts are counted but we didn't actually do
>> anything", implying that the only problem is that the counter is not
>> aligned with reality. The fact that the post-context we see here
>> only shows the "counting" part does not help us, either.
>>
>> But the next line in the post-context is actually code that does
>> something important, which is ...
>>
>> delta = diff_delta(last->data.buf, last->data.len,
>> dat->buf, dat->len,
>> , dat->len - the_hash_algo->rawsz);
>> } else
>> delta = NULL;
>>
>>
>> ... and makes the reader realize that the change itself is much
>> better than the patch with 3-line context, the title, and the
>> proposed log message advertises it as ;-)
>>
>> How about selling it this way instead?
>>
>> fast-import: do not call diff_delta() with empty buffer
>>
>> We know diff_delta() returns NULL, saying "no good delta
>> exists for it", when fed an empty data. Check the length of
>> the data in the caller to avoid such a call.
>>
>> This incidentally reduces the number of attempted deltification
>> we see in the final statistics.
>>
>> or something like that?
>
> Fair enough. Do you want me to send a v2 with this description?
For a single-patch topic like this one, if you like what was in the
e-mail verbatim, saying so is sufficient, as I can just use the
material to run "git commit --amend". For anything more involved
(e.g. "oh, then insert a code to do this before that function"
and/or a fix to an earlier patch in a multi-patch series), I'd
prefer a re-submission, which can be processed just the same way as
any other new topic.
Thanks.
Re: [PATCH] gc --auto: release pack files before auto packing
Duy Nguyen writes:
> On Sat, Jun 30, 2018 at 03:38:21PM +0200, Kim Gybels wrote:
>> Teach gc --auto to release pack files before auto packing the repository
>> to prevent failures when removing them.
>>
>> Also teach the test 'fetching with auto-gc does not lock up' to complain
>> when it is no longer triggering an auto packing of the repository.
>>
>> Fixes https://github.com/git-for-windows/git/issues/500
>>
>> Signed-off-by: Kim Gybels
>> ---
>>
>> Patch based on master, since problem doesn't reproduce on maint,
>> however, the improvement to the test might be valuable on maint.
>>
>> builtin/gc.c | 1 +
>> t/t5510-fetch.sh | 2 ++
>> 2 files changed, 3 insertions(+)
>>
>> diff --git a/builtin/gc.c b/builtin/gc.c
>> index ccfb1ceaeb..63b62ab57c 100644
>> --- a/builtin/gc.c
>> +++ b/builtin/gc.c
>> @@ -612,6 +612,7 @@ int cmd_gc(int argc, const char **argv, const char
>> *prefix)
>> return -1;
>>
>> if (!repository_format_precious_objects) {
>> +close_all_packs(the_repository->objects);
>
> We have repo_clear() which does this and potentially closing file
> descriptors on other things as well. I suggest we use it, and before
> any external command is run. Something like
>
> diff --git a/builtin/gc.c b/builtin/gc.c
> index ccfb1ceaeb..a852c71da6 100644
> --- a/builtin/gc.c
> +++ b/builtin/gc.c
> @@ -473,6 +473,13 @@ static int report_last_gc_error(void)
>
> static int gc_before_repack(void)
> {
> + /*
> + * Shut down everything, we should have all the info we need
> + * at this point. Leaving some file descriptors open may
> + * prevent them from being removed on Windows.
> + */
> + repo_clear(the_repository);
> +
> if (pack_refs && run_command_v_opt(pack_refs_cmd.argv, RUN_GIT_CMD))
> return error(FAILED_RUN, pack_refs_cmd.argv[0]);
With
https://public-inbox.org/git/20180509170409.13666-1-pclo...@gmail.com/
the call to repo_clear() on the_repository itself may be safe [*1*],
but if command line parsing code etc. has pointers to in-core object
etc. obtained before this function was called, the codeflow after
this function returns will be quite disturbed. Has anybody in this
review thread audited the codeflow _after_ this function is run to
make sure that invalidating in-core repository here does not cause
us problems? Just checking, because I won't queue this change until
I hear that somebody familiar with the codepath actually did so (I
may get around to do so myself later).
Thanks.
[Footnote]
*1* ... and of course changing the_index to _repo->index would
make it even safer ;-)
Re: as/safecrlf-quiet-fix, was Re: What's cooking in git.git (Jun 2018, #07; Thu, 28)
Johannes Schindelin writes: > On Tue, 3 Jul 2018, Junio C Hamano wrote: > >> Johannes Schindelin writes: >> >> > On Thu, 28 Jun 2018, Junio C Hamano wrote: >> > >> >> * as/safecrlf-quiet-fix (2018-06-11) 1 commit >> >> (merged to 'next' on 2018-06-13 at b163674843) >> >> + config.c: fix regression for core.safecrlf false >> >> >> >> Fix for 2.17-era regression around `core.safecrlf`. >> > >> > Is this `maint` material? >> >> Possibly. Why do you ask? > > This is my feeble attempt at helping you triage. My question was more like > a suggestion. Ah, thanks. That line of suggestion does help, and it would even be more helpful when given before the topic hits 'next' or after 'master' from the workflow's point of view. While the topic is in 'pu' I can change where the topic forks from, and to minimize temptation to stupidly merge an inappropriate topic (e.g. new developments and somewhat dubious fixes to commonly used part of Git) down to 'maint' during the pre-release freeze (i.e. when the tree is otherwise hopefully sleepily quiet and boring), I try to base the (beginning of) good 'maint' material on 'maint' or older, and everything else on the tip of 'master' (or the most recent feature release), when each topic is originally queued in my tree. If I missed a good topic and based it on 'master', it is easier to correct such a mistake before it hits 'next', and a suggestion like above would help me tremendously. After the topic hits 'next', its course is pretty much set, until it graduates to 'master'. About a week or two after such event, I'd go through "merge ... later to maint" entries in the draft release notes and if the topic still looked relevant to the maintenance branch, merge them to 'maint'. Before that happens is another chance to stop me from making a stupid mistake, or remind me of an urgent one that I've been letting wait in 'master' before merging to 'maint'. Thanks.
Re: [GSoC][PATCH v2 6/7] rebase -i: rewrite setup_reflog_action() in C
Johannes Schindelin writes:
>> I thought we made this into
>>
>> if verbose
>> return run_command
>> else
>> return run_command_silently
>>
>> to help readers in the previous round already.
>
> FWIW we had quite a couple of reviews in the recent years which pointed
> out "unnecessary else" after returning or die()ing. Maybe we should make
> up our minds, and set a consistent rule to follow.
FWIW the pattern you are referring to is
do something;
if (error detected) {
return error(...);
} else {
perform
remaining
actions
the function needs
to complete
its task;
}
and those reviewers (including me) are absolutely right that such an
"else" is not just unnecessary but actively harms readability of the
flow of logic.
I am also absolutely right when I say what is quoted at the top
makes 100% more sense than
if (verbose)
return run_command();
return run_command_silently();
as these two are about doing the same thing a bit differently.
The way to think about the latter is that we won't have this "if
(verbose)" if there were a variant of run_command() that took a set
of option bits among which is a verbose bit, but instead would have
a single call to that function that returns.. So it's not like "in
an exceptional case, return after calling this function; otherwise
keep going, which happens to only return after calling another". It
is more like "here we need to return after spawning a command, but
depending on this bit, we spawn the command using a different
function".
Good programers recognize the difference between these two patterns
without being told, and mentors should teach and GSoC student should
learn that an overly simplified rule like "when 'if' block ends with
return, do not write 'else'" is harmful.
Re: Git 2.18: RUNTIME_PREFIX... is it working?
Apologies for the delayed response - I've been out of town - and It looks like Paul is already on the right track. Johannes: I believe the GIT_EXEC_PATH snipped that you listed is not incorrect. It's defined to "gitexecdir_SQ", and RUNTIME_PREFIX expects (and enforces, as you snipped) that this is a relative path in Makefile. On non-RUNTIME_PREFIX builds, it should still be the absolute path, as this is how Git self-locates, so using "gitexecdir_SQ" there makes sense to me. So: RUNTIME_PREFIX=No, gitexecdir_SQ is absolute, GIT_EXEC_PATH is absolute, used to find Git: https://github.com/git/git/blob/ccdcbd54c4475c2238b310f7113ab3075b5abc9c/exec-cmd.c#L281 RUNTIME_PREFIX=YesPlease, gitexecdir_SQ is relative, GIT_EXEC_PATH is relative and used to identify the search root of the Git installation: https://github.com/git/git/blob/ccdcbd54c4475c2238b310f7113ab3075b5abc9c/exec-cmd.c#L40 The dual-use is confusing, and it took me a few to walk back through how it is employed in each scenario. For clarity's sake, it may be worth defining two variables and making one explicitly relative, but I think it is functional as-is. Paul: I used "config.mak" to configure RUNTIME_PREFIX when I used it to the same effect: https://chromium.googlesource.com/infra/infra/+/ca729b99c1f82665b634ef2ff69d93c97dfcda99/recipes/recipe_modules/third_party_packages/git.py#78 I forewent autoconf because I was concerned that the option was too obscure and the configuration too nuanced to be worth adding via flag, as RUNTIME_PREFIX requires some degree of path alignment and is fairly special-case. If you prefer autoconf, though, it sounds like a good thing to add, and I'm happy that you are finding the feature useful! Cheers, -Dan On Fri, Jul 6, 2018 at 5:00 AM Johannes Schindelin wrote: > > Hi Paul, > > On Thu, 5 Jul 2018, Paul Smith wrote: > > > On Wed, 2018-07-04 at 13:22 +0200, Johannes Schindelin wrote: > > > > Basically what happens is that I run configure with > > > > --prefix=/my/install/path --with-gitconfig=etc/gitconfig > > > > --with-gitattributes=etc/gitattributes. > > > > > > > > Then I run make with RUNTIME_PREFIX=YesPlease. > > > > > > Ah. In Git for Windows, we do not use configure. I *think* this > > > points to an incompatibility of the RUNTIME_PREFIX feature with our > > > autoconf support, and this is a grand opportunity for you to step in > > > and help. > > > > > > Essentially, what you will want to do is to implement a new configure > > > option --with-runtime-prefix that then prevents the autoconf script > > > from munging the relative paths in the way it does. > > > > FYI I was able to get this to work by overriding variables on the make > > command line, like this: > > > > make ... RUNTIME_PREFIX=YesPlease \ > > gitexecdir=libexec/git-core \ > > template_dir=share/git-core/templates \ > > sysconfdir=etc > > > > I agree a new autoconf option would be much simpler to use. I'll think > > about it as I happen to have some some experience in these areas ;) ... > > I look forward to reviewing this... > > > but time is limited of course :). > > Yep. Same here ;-) > > Ciao, > Johannes
Re: [GSoC][PATCH v2 6/7] rebase -i: rewrite setup_reflog_action() in C
Hi Junio,
On Tue, 3 Jul 2018, Junio C Hamano wrote:
> Alban Gruin writes:
>
> > +static int run_git_checkout(struct replay_opts *opts, const char *commit,
> > + int verbose, const char *action)
> > +{
> > + struct child_process cmd = CHILD_PROCESS_INIT;
> > +
> > + cmd.git_cmd = 1;
> > +
> > + argv_array_push(, "checkout");
> > + argv_array_push(, commit);
> > + argv_array_pushf(_array, GIT_REFLOG_ACTION "=%s", action);
> > +
> > + if (verbose)
> > + return run_command();
> > + return run_command_silent_on_success();
>
> I thought we made this into
>
> if verbose
> return run_command
> else
> return run_command_silently
>
> to help readers in the previous round already.
FWIW we had quite a couple of reviews in the recent years which pointed
out "unnecessary else" after returning or die()ing. Maybe we should make
up our minds, and set a consistent rule to follow.
Ciao,
Dscho
Re: [PATCH v3 2/4] rebase: refactor common shell functions into their own file
Hi Pratik, On Fri, 6 Jul 2018, Pratik Karki wrote: > The functions present in `git-legacy-rebase.sh` are used by the rebase > backends as they are implemented as shell script functions in the > `git-rebase--` files. > > To make the `builtin/rebase.c` work, we have to provide support via > a Unix shell script snippet that uses these functions and so, we > want to use the rebase backends *directly* from the builtin rebase > without going through `git-legacy-rebase.sh`. > > This commit extracts the functions to a separate file, > `git-rebase--common`, that will be read by `git-legacy-rebase.sh` and > by the shell script snippets which will be used extensively in the > following commits. Good. While this seems to catch all the functions required by the backends, I am fairly certain that the `resolvemsg` variable is used exclusively by the backends, and it should therefore also moved into `git-rebase--common`. See my comments on your https://github.com/git/git/pull/505 for more details. Ciao, Dscho
Re: [PATCH 0/3] Automatic transfer encoding for patches
Overall this series looks good. Thanks for putting the patches together!
Re: [PATCH 2/3] send-email: accept long lines with suitable transfer encoding
On 2018-07-06 2:23 AM, brian m. carlson wrote:
> diff --git a/git-send-email.perl b/git-send-email.perl
> index a76953c310..4ea30c4070 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -1899,6 +1899,10 @@ sub validate_patch {
> return $hook_error if $hook_error;
> }
>
> + # Any long lines will be automatically fixed if we use a suitable
> transfer
> + # encoding.
> + return if $xfer_encoding =~ /^(?:auto|quoted-printable|base64)$/;
Rather than returning in this case I'd sooner wrap the length check in
this test. If additional checks are added in the future it'd be too easy
to accidentally skip them if the transfer encoding is quoted-printable.
[PATCH v3 4/4] builtin/rebase: support running "git rebase "
This patch gives life to the skeleton added in the previous patches:
With this change, we can perform a elementary rebase (without any
options).
It can be tested thusly by:
git -c rebase.usebuiltin=true rebase HEAD~2
The rebase backends (i.e. the shell script functions defined in
`git-rebase--`) are still at work here and the "builtin
rebase"'s purpose is simply to parse the options and set
everything up so that those rebase backends can do their work.
Note: We take an alternative approach here which is *not* to set the
environment variables via `run_command_v_opt_cd_env()` because those
variables would then be visible by processes spawned from the rebase
backends. Instead, we work hard to set them in the shell script snippet.
The next commits will handle and support all the wonderful rebase
options.
Signed-off-by: Pratik Karki
---
builtin/rebase.c | 237 ++-
1 file changed, 236 insertions(+), 1 deletion(-)
diff --git a/builtin/rebase.c b/builtin/rebase.c
index fab7fca37e..71f06f0789 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -9,6 +9,20 @@
#include "exec-cmd.h"
#include "argv-array.h"
#include "dir.h"
+#include "packfile.h"
+#include "checkout.h"
+#include "refs.h"
+#include "quote.h"
+
+static GIT_PATH_FUNC(apply_dir, "rebase-apply");
+static GIT_PATH_FUNC(merge_dir, "rebase-merge");
+
+enum rebase_type {
+ REBASE_AM,
+ REBASE_MERGE,
+ REBASE_INTERACTIVE,
+ REBASE_PRESERVE_MERGES
+};
static int use_builtin_rebase(void)
{
@@ -28,8 +42,136 @@ static int use_builtin_rebase(void)
return ret;
}
+static int apply_autostash(void)
+{
+ warning("TODO");
+ return 0;
+}
+
+struct rebase_options {
+ enum rebase_type type;
+ const char *state_dir;
+ struct commit *upstream;
+ const char *upstream_name;
+ char *head_name;
+ struct object_id orig_head;
+ struct commit *onto;
+ const char *onto_name;
+ const char *revisions;
+ const char *root;
+};
+
+static int finish_rebase(struct rebase_options *opts)
+{
+ struct strbuf dir = STRBUF_INIT;
+ const char *argv_gc_auto[] = { "gc", "--auto", NULL };
+
+ delete_ref(NULL, "REBASE_HEAD", NULL, REF_NO_DEREF);
+ apply_autostash();
+ close_all_packs(the_repository->objects);
+ /*
+* We ignore errors in 'gc --auto', since the
+* user should see them.
+*/
+ run_command_v_opt(argv_gc_auto, RUN_GIT_CMD);
+ strbuf_addstr(, opts->state_dir);
+ remove_dir_recursively(, 0);
+ strbuf_release();
+
+ return 0;
+}
+
+static struct commit *peel_committish(const char *name)
+{
+ struct object *obj;
+ struct object_id oid;
+
+ if (get_oid(name, ))
+ return NULL;
+ obj = parse_object();
+ return (struct commit *)peel_to_type(name, 0, obj, OBJ_COMMIT);
+}
+
+static void add_var(struct strbuf *buf, const char *name, const char *value)
+{
+ strbuf_addstr(buf, name);
+ strbuf_addstr(buf, "=");
+ sq_quote_buf(buf, value);
+ strbuf_addstr(buf, "; ");
+}
+
+static int run_specific_rebase(struct rebase_options *opts)
+{
+ const char *argv[] = { NULL, NULL };
+ struct strbuf script_snippet = STRBUF_INIT;
+ int status;
+ const char *backend, *backend_func;
+
+ add_var(_snippet, "GIT_DIR", absolute_path(get_git_dir()));
+
+ add_var(_snippet, "upstream_name", opts->upstream_name);
+ add_var(_snippet, "upstream",
+oid_to_hex(>upstream->object.oid));
+ add_var(_snippet, "head_name", opts->head_name);
+ add_var(_snippet, "orig_head", oid_to_hex(>orig_head));
+ add_var(_snippet, "onto", oid_to_hex(>onto->object.oid));
+ add_var(_snippet, "onto_name", opts->onto_name);
+ add_var(_snippet, "revisions", opts->revisions);
+
+ switch (opts->type) {
+ case REBASE_AM:
+ backend = "git-rebase--am";
+ backend_func = "git_rebase__am";
+ break;
+ case REBASE_INTERACTIVE:
+ backend = "git-rebase--interactive";
+ backend_func = "git_rebase__interactive";
+ break;
+ case REBASE_MERGE:
+ backend = "git-rebase--merge";
+ backend_func = "git_rebase__merge";
+ break;
+ case REBASE_PRESERVE_MERGES:
+ backend = "git-rebase--preserve-merges";
+ backend_func = "git_rebase__preserve_merges";
+ break;
+ default:
+ BUG("Unhandled rebase type %d", opts->type);
+ break;
+ }
+
+ strbuf_addf(_snippet,
+ ". git-rebase--common && . %s && %s",
+ backend, backend_func);
+ argv[0] = script_snippet.buf;
+
+ status = run_command_v_opt(argv, RUN_USING_SHELL);
+ if (status == 0)
+ finish_rebase(opts);
+
[PATCH v3 3/4] sequencer: refactor the code to detach HEAD to checkout.c
In the upcoming builtin rebase, we will have to start by detaching
the HEAD, just like shell script version does. Essentially, we have
to do the same thing as `git checkout -q ^0 --`, in pure C.
The aforementioned functionality was already present in `sequencer.c`
in `do_reset()` function. But `do_reset()` performs more than detaching
the HEAD, and performs action specific to `sequencer.c`.
So this commit refactors out that part from `do_reset()`, and moves it
to a new function called `detach_head_to()`. As this function has
nothing to do with the sequencer, and everything to do with what `git
checkout -q ^0 --` does, we move that function to checkout.c.
This refactoring actually introduces a slight change in behavior:
previously, the index was locked before parsing the argument to the
todo command `reset`, while it now gets locked *after* that, in the
`detach_head_to()` function.
It does not make a huge difference, and the upside is that this closes
a few (unlikely) code paths where the index would not be unlocked upon
error.
Signed-off-by: Pratik Karki
---
checkout.c | 64 +
checkout.h | 3 +++
sequencer.c | 58 +---
3 files changed, 72 insertions(+), 53 deletions(-)
diff --git a/checkout.c b/checkout.c
index bdefc888ba..da68915fd7 100644
--- a/checkout.c
+++ b/checkout.c
@@ -2,6 +2,11 @@
#include "remote.h"
#include "refspec.h"
#include "checkout.h"
+#include "unpack-trees.h"
+#include "lockfile.h"
+#include "refs.h"
+#include "tree.h"
+#include "cache-tree.h"
struct tracking_name_data {
/* const */ char *src_ref;
@@ -42,3 +47,62 @@ const char *unique_tracking_name(const char *name, struct
object_id *oid)
free(cb_data.dst_ref);
return NULL;
}
+
+int detach_head_to(struct object_id *oid, const char *action,
+ const char *reflog_message)
+{
+ struct strbuf ref_name = STRBUF_INIT;
+ struct tree_desc desc;
+ struct lock_file lock = LOCK_INIT;
+ struct unpack_trees_options unpack_tree_opts;
+ struct tree *tree;
+ int ret = 0;
+
+ if (hold_locked_index(, LOCK_REPORT_ON_ERROR) < 0)
+ return -1;
+
+ memset(_tree_opts, 0, sizeof(unpack_tree_opts));
+ setup_unpack_trees_porcelain(_tree_opts, action);
+ unpack_tree_opts.head_idx = 1;
+ unpack_tree_opts.src_index = _index;
+ unpack_tree_opts.dst_index = _index;
+ unpack_tree_opts.fn = oneway_merge;
+ unpack_tree_opts.merge = 1;
+ unpack_tree_opts.update = 1;
+
+ if (read_cache_unmerged()) {
+ rollback_lock_file();
+ strbuf_release(_name);
+ return error_resolve_conflict(_(action));
+ }
+
+ if (!fill_tree_descriptor(, oid)) {
+ error(_("failed to find tree of %s"), oid_to_hex(oid));
+ rollback_lock_file();
+ free((void *)desc.buffer);
+ strbuf_release(_name);
+ return -1;
+ }
+
+ if (unpack_trees(1, , _tree_opts)) {
+ rollback_lock_file();
+ free((void *)desc.buffer);
+ strbuf_release(_name);
+ return -1;
+ }
+
+ tree = parse_tree_indirect(oid);
+ prime_cache_tree(_index, tree);
+
+ if (write_locked_index(_index, , COMMIT_LOCK) < 0)
+ ret = error(_("could not write index"));
+ free((void *)desc.buffer);
+
+ if (!ret)
+ ret = update_ref(reflog_message, "HEAD", oid,
+NULL, 0, UPDATE_REFS_MSG_ON_ERR);
+
+ strbuf_release(_name);
+ return ret;
+
+}
diff --git a/checkout.h b/checkout.h
index 9980711179..6ce46cf4e8 100644
--- a/checkout.h
+++ b/checkout.h
@@ -10,4 +10,7 @@
*/
extern const char *unique_tracking_name(const char *name, struct object_id
*oid);
+int detach_head_to(struct object_id *oid, const char *action,
+ const char *reflog_message);
+
#endif /* CHECKOUT_H */
diff --git a/sequencer.c b/sequencer.c
index 5354d4d51e..106d518f4d 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -29,6 +29,7 @@
#include "oidset.h"
#include "commit-slab.h"
#include "alias.h"
+#include "checkout.h"
#define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION"
@@ -2756,14 +2757,7 @@ static int do_reset(const char *name, int len, struct
replay_opts *opts)
{
struct strbuf ref_name = STRBUF_INIT;
struct object_id oid;
- struct lock_file lock = LOCK_INIT;
- struct tree_desc desc;
- struct tree *tree;
- struct unpack_trees_options unpack_tree_opts;
- int ret = 0, i;
-
- if (hold_locked_index(, LOCK_REPORT_ON_ERROR) < 0)
- return -1;
+ int i;
if (len == 10 && !strncmp("[new root]", name, len)) {
if (!opts->have_squash_onto) {
@@ -2789,56 +2783,14 @@ static int do_reset(const char *name, int len, struct
replay_opts *opts)
[PATCH v3 1/4] rebase: start implementing it as a builtin
This commit imitates the strategy that was used to convert the
difftool to a builtin. We start by renaming the shell script
`git-rebase.sh` to `git-legacy-rebase.sh` and introduce a
`builtin/rebase.c` that simply executes the shell script version,
unless the config setting `rebase.useBuiltin` is set to `true`.
The motivation behind this is to rewrite all the functionality of the
shell script version in the aforementioned `rebase.c`, one by one and
be able to conveniently test new features by configuring
`rebase.useBuiltin`.
We intentionally avoid reading the config directly to avoid
messing up the GIT_* environment variables when we need to fall back to
exec()ing the shell script. The test of builtin rebase can be done by
`git -c rebase.useBuiltin=true rebase ...`
Signed-off-by: Pratik Karki
---
.gitignore| 1 +
Makefile | 3 +-
builtin.h | 1 +
builtin/rebase.c | 56 +++
git-rebase.sh => git-legacy-rebase.sh | 0
git.c | 6 +++
6 files changed, 66 insertions(+), 1 deletion(-)
create mode 100644 builtin/rebase.c
rename git-rebase.sh => git-legacy-rebase.sh (100%)
diff --git a/.gitignore b/.gitignore
index 3284a1e9b1..ec23959014 100644
--- a/.gitignore
+++ b/.gitignore
@@ -78,6 +78,7 @@
/git-init-db
/git-interpret-trailers
/git-instaweb
+/git-legacy-rebase
/git-log
/git-ls-files
/git-ls-remote
diff --git a/Makefile b/Makefile
index 0cb6590f24..e88fe2e5fb 100644
--- a/Makefile
+++ b/Makefile
@@ -609,7 +609,7 @@ SCRIPT_SH += git-merge-one-file.sh
SCRIPT_SH += git-merge-resolve.sh
SCRIPT_SH += git-mergetool.sh
SCRIPT_SH += git-quiltimport.sh
-SCRIPT_SH += git-rebase.sh
+SCRIPT_SH += git-legacy-rebase.sh
SCRIPT_SH += git-remote-testgit.sh
SCRIPT_SH += git-request-pull.sh
SCRIPT_SH += git-stash.sh
@@ -1059,6 +1059,7 @@ BUILTIN_OBJS += builtin/prune.o
BUILTIN_OBJS += builtin/pull.o
BUILTIN_OBJS += builtin/push.o
BUILTIN_OBJS += builtin/read-tree.o
+BUILTIN_OBJS += builtin/rebase.o
BUILTIN_OBJS += builtin/rebase--helper.o
BUILTIN_OBJS += builtin/receive-pack.o
BUILTIN_OBJS += builtin/reflog.o
diff --git a/builtin.h b/builtin.h
index 0362f1ce25..44651a447f 100644
--- a/builtin.h
+++ b/builtin.h
@@ -202,6 +202,7 @@ extern int cmd_prune_packed(int argc, const char **argv,
const char *prefix);
extern int cmd_pull(int argc, const char **argv, const char *prefix);
extern int cmd_push(int argc, const char **argv, const char *prefix);
extern int cmd_read_tree(int argc, const char **argv, const char *prefix);
+extern int cmd_rebase(int argc, const char **argv, const char *prefix);
extern int cmd_rebase__helper(int argc, const char **argv, const char *prefix);
extern int cmd_receive_pack(int argc, const char **argv, const char *prefix);
extern int cmd_reflog(int argc, const char **argv, const char *prefix);
diff --git a/builtin/rebase.c b/builtin/rebase.c
new file mode 100644
index 00..fab7fca37e
--- /dev/null
+++ b/builtin/rebase.c
@@ -0,0 +1,56 @@
+/*
+ * "git rebase" builtin command
+ *
+ * Copyright (c) 2018 Pratik Karki
+ */
+
+#include "builtin.h"
+#include "run-command.h"
+#include "exec-cmd.h"
+#include "argv-array.h"
+#include "dir.h"
+
+static int use_builtin_rebase(void)
+{
+ struct child_process cp = CHILD_PROCESS_INIT;
+ struct strbuf out = STRBUF_INIT;
+ int ret;
+
+ argv_array_pushl(,
+"config", "--bool", "rebase.usebuiltin", NULL);
+ cp.git_cmd = 1;
+ if (capture_command(, , 6))
+ return 0;
+
+ strbuf_trim();
+ ret = !strcmp("true", out.buf);
+ strbuf_release();
+ return ret;
+}
+
+int cmd_rebase(int argc, const char **argv, const char *prefix)
+{
+ /*
+* NEEDSWORK: Once the builtin rebase has been tested enough
+* and git-legacy-rebase.sh is retired to contrib/, this preamble
+* can be removed.
+*/
+
+ if (!use_builtin_rebase()) {
+ const char *path = mkpath("%s/git-legacy-rebase",
+ git_exec_path());
+
+ if (sane_execvp(path, (char **)argv) < 0)
+ die_errno("could not exec %s", path);
+ else
+ die("sane_execvp() returned???");
+ }
+
+ if (argc != 2)
+ die("Usage: %s ", argv[0]);
+ prefix = setup_git_directory();
+ trace_repo_setup(prefix);
+ setup_work_tree();
+
+ die("TODO");
+}
diff --git a/git-rebase.sh b/git-legacy-rebase.sh
similarity index 100%
rename from git-rebase.sh
rename to git-legacy-rebase.sh
diff --git a/git.c b/git.c
index 9dbe6ffaa7..0e35632464 100644
--- a/git.c
+++ b/git.c
@@ -518,6 +518,12 @@ static struct cmd_struct commands[] = {
{ "pull", cmd_pull, RUN_SETUP | NEED_WORK_TREE },
{ "push", cmd_push, RUN_SETUP },
{ "read-tree", cmd_read_tree,
[PATCH v3 2/4] rebase: refactor common shell functions into their own file
The functions present in `git-legacy-rebase.sh` are used by the rebase
backends as they are implemented as shell script functions in the
`git-rebase--` files.
To make the `builtin/rebase.c` work, we have to provide support via
a Unix shell script snippet that uses these functions and so, we
want to use the rebase backends *directly* from the builtin rebase
without going through `git-legacy-rebase.sh`.
This commit extracts the functions to a separate file,
`git-rebase--common`, that will be read by `git-legacy-rebase.sh` and
by the shell script snippets which will be used extensively in the
following commits.
Signed-off-by: Pratik Karki
---
.gitignore| 1 +
Makefile | 1 +
git-legacy-rebase.sh | 62 +--
git-rebase--common.sh | 61 ++
4 files changed, 64 insertions(+), 61 deletions(-)
create mode 100644 git-rebase--common.sh
diff --git a/.gitignore b/.gitignore
index ec23959014..824141cba1 100644
--- a/.gitignore
+++ b/.gitignore
@@ -117,6 +117,7 @@
/git-read-tree
/git-rebase
/git-rebase--am
+/git-rebase--common
/git-rebase--helper
/git-rebase--interactive
/git-rebase--merge
diff --git a/Makefile b/Makefile
index e88fe2e5fb..163e52ad1a 100644
--- a/Makefile
+++ b/Makefile
@@ -619,6 +619,7 @@ SCRIPT_SH += git-web--browse.sh
SCRIPT_LIB += git-mergetool--lib
SCRIPT_LIB += git-parse-remote
SCRIPT_LIB += git-rebase--am
+SCRIPT_LIB += git-rebase--common
SCRIPT_LIB += git-rebase--interactive
SCRIPT_LIB += git-rebase--preserve-merges
SCRIPT_LIB += git-rebase--merge
diff --git a/git-legacy-rebase.sh b/git-legacy-rebase.sh
index 19bdebb480..240aac507c 100755
--- a/git-legacy-rebase.sh
+++ b/git-legacy-rebase.sh
@@ -102,6 +102,7 @@ case "$(git config --bool commit.gpgsign)" in
true) gpg_sign_opt=-S ;;
*) gpg_sign_opt= ;;
esac
+. git-rebase--common
read_basic_state () {
test -f "$state_dir/head-name" &&
@@ -132,67 +133,6 @@ read_basic_state () {
}
}
-write_basic_state () {
- echo "$head_name" > "$state_dir"/head-name &&
- echo "$onto" > "$state_dir"/onto &&
- echo "$orig_head" > "$state_dir"/orig-head &&
- echo "$GIT_QUIET" > "$state_dir"/quiet &&
- test t = "$verbose" && : > "$state_dir"/verbose
- test -n "$strategy" && echo "$strategy" > "$state_dir"/strategy
- test -n "$strategy_opts" && echo "$strategy_opts" > \
- "$state_dir"/strategy_opts
- test -n "$allow_rerere_autoupdate" && echo "$allow_rerere_autoupdate" >
\
- "$state_dir"/allow_rerere_autoupdate
- test -n "$gpg_sign_opt" && echo "$gpg_sign_opt" >
"$state_dir"/gpg_sign_opt
- test -n "$signoff" && echo "$signoff" >"$state_dir"/signoff
-}
-
-output () {
- case "$verbose" in
- '')
- output=$("$@" 2>&1 )
- status=$?
- test $status != 0 && printf "%s\n" "$output"
- return $status
- ;;
- *)
- "$@"
- ;;
- esac
-}
-
-move_to_original_branch () {
- case "$head_name" in
- refs/*)
- message="rebase finished: $head_name onto $onto"
- git update-ref -m "$message" \
- $head_name $(git rev-parse HEAD) $orig_head &&
- git symbolic-ref \
- -m "rebase finished: returning to $head_name" \
- HEAD $head_name ||
- die "$(eval_gettext "Could not move back to \$head_name")"
- ;;
- esac
-}
-
-apply_autostash () {
- if test -f "$state_dir/autostash"
- then
- stash_sha1=$(cat "$state_dir/autostash")
- if git stash apply $stash_sha1 >/dev/null 2>&1
- then
- echo "$(gettext 'Applied autostash.')" >&2
- else
- git stash store -m "autostash" -q $stash_sha1 ||
- die "$(eval_gettext "Cannot store \$stash_sha1")"
- gettext 'Applying autostash resulted in conflicts.
-Your changes are safe in the stash.
-You can run "git stash pop" or "git stash drop" at any time.
-' >&2
- fi
- fi
-}
-
finish_rebase () {
rm -f "$(git rev-parse --git-path REBASE_HEAD)"
apply_autostash &&
diff --git a/git-rebase--common.sh b/git-rebase--common.sh
new file mode 100644
index 00..5ba0603924
--- /dev/null
+++ b/git-rebase--common.sh
@@ -0,0 +1,61 @@
+
+write_basic_state () {
+ echo "$head_name" > "$state_dir"/head-name &&
+ echo "$onto" > "$state_dir"/onto &&
+ echo "$orig_head" > "$state_dir"/orig-head &&
+ echo "$GIT_QUIET" > "$state_dir"/quiet &&
+ test t = "$verbose" && : > "$state_dir"/verbose
+ test -n "$strategy" && echo "$strategy" > "$state_dir"/strategy
+ test -n "$strategy_opts" && echo "$strategy_opts" > \
+ "$state_dir"/strategy_opts
+
[GSoC] [PATCH v3 0/4] rebase: rewrite rebase in C
As a GSoC project, I have been working on the builtin rebase. The motivation behind the rewrite of rebase i.e. from shell script to C are for following reasons: 1. Writing shell scripts and getting it to production is much faster than doing the equivalent in C but lacks in performance and extra workarounds are needed for non-POSIX platforms. 2. Git for Windows is at loss as the installer size increases due to addition of extra dependencies for the shell scripts which are usually available in POSIX compliant platforms. This series of patches serves to demonstrate a minimal builtin rebase which supports running `git rebase ` and also serves to ask for reviews. Changes since v2: - Fix commit messages of patches. - Acknowledge Junio's reviews. - Demonstrate an alternative approach to setting of environment variables via `run_command_v_opt_cd_env()` which would be visible by spawned process from rebase backends as this is not safe. Pratik Karki (4): rebase: start implementing it as a builtin rebase: refactor common shell functions into their own file sequencer: refactor the code to detach HEAD to checkout.c builtin/rebase: support running "git rebase " .gitignore| 2 + Makefile | 4 +- builtin.h | 1 + builtin/rebase.c | 291 ++ checkout.c| 64 ++ checkout.h| 3 + git-rebase.sh => git-legacy-rebase.sh | 62 +- git-rebase--common.sh | 61 ++ git.c | 6 + sequencer.c | 58 + 10 files changed, 437 insertions(+), 115 deletions(-) create mode 100644 builtin/rebase.c rename git-rebase.sh => git-legacy-rebase.sh (91%) create mode 100644 git-rebase--common.sh -- 2.18.0
Re: Git 2.18: RUNTIME_PREFIX... is it working?
Hi Paul, On Thu, 5 Jul 2018, Paul Smith wrote: > On Wed, 2018-07-04 at 13:22 +0200, Johannes Schindelin wrote: > > > Basically what happens is that I run configure with > > > --prefix=/my/install/path --with-gitconfig=etc/gitconfig > > > --with-gitattributes=etc/gitattributes. > > > > > > Then I run make with RUNTIME_PREFIX=YesPlease. > > > > Ah. In Git for Windows, we do not use configure. I *think* this > > points to an incompatibility of the RUNTIME_PREFIX feature with our > > autoconf support, and this is a grand opportunity for you to step in > > and help. > > > > Essentially, what you will want to do is to implement a new configure > > option --with-runtime-prefix that then prevents the autoconf script > > from munging the relative paths in the way it does. > > FYI I was able to get this to work by overriding variables on the make > command line, like this: > > make ... RUNTIME_PREFIX=YesPlease \ > gitexecdir=libexec/git-core \ > template_dir=share/git-core/templates \ > sysconfdir=etc > > I agree a new autoconf option would be much simpler to use. I'll think > about it as I happen to have some some experience in these areas ;) ... I look forward to reviewing this... > but time is limited of course :). Yep. Same here ;-) Ciao, Johannes
Re: [PATCH 3/8] gpg-interface: add new config to select how to sign a commit
Am Fri, 6 Jul 2018 01:01:48 + schrieb "brian m. carlson" : > On Tue, Jul 03, 2018 at 02:38:15PM +0200, Henning Schild wrote: > > Add "gpg.format" where the user can specify which type of signature > > to use for commits. At the moment only "PGP" is supported and the > > value is not even used. This commit prepares for a new types of > > signatures. > > We typically prefer to have option values specified in lower case. I > also think "openpgp" might be better than "PGP", since that's the name > of the specification and it would avoid any potential unhappiness > about compatibility with PGP or trademarks. Thanks for your input. I was assuming the names to start a discussion and i do not have a preference here. Let us wait for further comments on naming, i will then implement whatever the consensus is or what the maintainer requests. In fact "gpg.format" could be dropped and we could try both "gpg" and "gpgsm" and see where the signing-key is available. I decided to not implement that because gpgsm (unlike) gpg does not return an error when "--list-secret-keys" does not find anything. And we could have the odd case where both would find a matching key. In which case we would need "gpg.format" again to specify which one we prefer. Henning
Re: [PATCH 8/8] gpg-interface t: extend the existing GPG tests with GPGSM
Am Fri, 6 Jul 2018 01:14:47 +
schrieb "brian m. carlson" :
> On Tue, Jul 03, 2018 at 02:38:20PM +0200, Henning Schild wrote:
> > Add test cases to cover the new X509/gpgsm support. Most of them
> > resemble existing ones. They just switch the format to X509 and set
> > the signingkey when creating signatures. Validation of signatures
> > does not need any configuration of git, it does need gpgsm to be
> > configured to trust the key(-chain).
> > We generate a self-signed key for commit...@example.com and
> > configure gpgsm to trust it.
> >
> > Signed-off-by: Henning Schild
> > ---
> > t/lib-gpg.sh | 9 ++-
> > t/lib-gpg/gpgsm-gen-key.in | 6 +
> > t/t4202-log.sh | 66
> > ++
> > t/t5534-push-signed.sh | 52
> > t/t7003-filter-branch.sh |
> > 15 +++ t/t7030-verify-tag.sh | 47
> > +++-- t/t7600-merge.sh | 31
> > ++ 7 files changed, 223 insertions(+), 3
> > deletions(-) create mode 100644 t/lib-gpg/gpgsm-gen-key.in
> >
> > diff --git a/t/lib-gpg.sh b/t/lib-gpg.sh
> > index a5d3b2cba..9dcb4e990 100755
> > --- a/t/lib-gpg.sh
> > +++ b/t/lib-gpg.sh
> > @@ -38,7 +38,14 @@ then
> > "$TEST_DIRECTORY"/lib-gpg/ownertrust &&
> > gpg --homedir "${GNUPGHOME}" /dev/null
> > 2>&1 \ --sign -u commit...@example.com &&
> > - test_set_prereq GPG
> > + test_set_prereq GPG &&
> > + echo | gpgsm --homedir "${GNUPGHOME}" -o
> > "$TEST_DIRECTORY"/lib-gpg/gpgsm.crt.user --passphrase-fd 0
> > --pinentry-mode loopback --generate-key --batch
> > "$TEST_DIRECTORY"/lib-gpg/gpgsm-gen-key.in &&
> > + gpgsm --homedir "${GNUPGHOME}" --import
> > "$TEST_DIRECTORY"/lib-gpg/gpgsm.crt.user &&
> > + gpgsm --homedir "${GNUPGHOME}" -K | grep
> > fingerprint: | cut -d" " -f4 | tr -d '\n' >
> > ${GNUPGHOME}/trustlist.txt &&
> > + echo " S relax" >> ${GNUPGHOME}/trustlist.txt &&
> > + (gpgconf --kill gpg-agent >/dev/null 2>&1 || : ) &&
> > + echo hello | gpgsm --homedir "${GNUPGHOME}" -u
> > commit...@example.com -o /dev/null --sign - 2>&1 &&
> > + test_set_prereq GPGSM
>
> It looks like the GPGSM prerequisite will only be set if the GPG
> prerequisite is set as well. Do we want to consider the case when the
> user might have gpgsm but not gpg?
Nice finding, i should have tried to hide that better ;).
I thought about it when writing the code. There might be distributions
where you can install one without the other. I also introduces a few
tests that rely on the implication, where GPGSM tests on top of GPG.
(i.e. t7030 "create signed tags x509")
The implication is really just there for the tests, not for end-users.
Dropping it would create more variations in testing (make it more
expensive).
I would say it is not worth it at the moment.
Implementing the gpg.format detection by actually calling the "other"
program to find which one knows the key, would shine another light on
that one. But i kind of doubt that idea is a good one.
Henning
Re: [PATCH 0/8] X509 (gpgsm) commit signing support
Am Fri, 6 Jul 2018 01:18:35 + schrieb "brian m. carlson" : > On Tue, Jul 03, 2018 at 02:38:12PM +0200, Henning Schild wrote: > > This series adds support for signing commits with gpgsm. > > > > The first two patches are cleanups of gpg-interface, while the next > > four prepare for the introduction of the actual feature in patch 7. > > Finally patch 8 extends the testsuite to cover the new feature. > > > > This series can be seen as a follow up of a series that appeared > > under the name "gpg-interface: Multiple signing tools" in april > > 2018 [1]. After that series was not merged i decided to get my > > patches ready. The original series aimed at being generic for any > > sort of signing tool, while this series just introduced the X509 > > variant of gpg. (gpgsm) I collected authors and reviewers of that > > first series and already put them on cc. > > Overall, I think this is heading in a good direction. I left a few > comments, but it seemed pretty sane. Thanks, i hope others think so too and that will eventually get merged. Henning
Re: [PATCH 7/8] gpg-interface: introduce new signature format "X509" using gpgsm
Am Fri, 6 Jul 2018 01:10:13 +
schrieb "brian m. carlson" :
> On Tue, Jul 03, 2018 at 02:38:19PM +0200, Henning Schild wrote:
> > This commit allows git to create and check X509 type signatures
> > using gpgsm.
> >
> > Signed-off-by: Henning Schild
> > ---
> > Documentation/config.txt | 5 -
> > gpg-interface.c | 10 +-
> > 2 files changed, 13 insertions(+), 2 deletions(-)
> >
> > diff --git a/Documentation/config.txt b/Documentation/config.txt
> > index c88903399..337df6e48 100644
> > --- a/Documentation/config.txt
> > +++ b/Documentation/config.txt
> > @@ -1828,9 +1828,12 @@ gpg.program::
> > signed, and the program is expected to send the result to
> > its standard output.
> >
> > +gpg.programX509::
>
> I'm not super excited about this name. It seems to indicate we want a
> level of hierarchy involved.
>
> A hierarchy like sign.openpgp.program (falling back to gpg.program)
> and sign.x509.program might be more logical.
>
> > diff --git a/gpg-interface.c b/gpg-interface.c
> > index aa747278e..85d721007 100644
> > --- a/gpg-interface.c
> > +++ b/gpg-interface.c
> > @@ -16,13 +16,18 @@ struct gpg_format_data {
> >
> > #define PGP_SIGNATURE "-BEGIN PGP SIGNATURE-"
> > #define PGP_MESSAGE "-BEGIN PGP MESSAGE-"
> > +#define X509_SIGNATURE "-BEGIN SIGNED MESSAGE-"
> >
> > -enum gpgformats { PGP_FMT };
> > +enum gpgformats { PGP_FMT, X509_FMT };
> > struct gpg_format_data gpg_formats[] = {
> > { .format = "PGP", .program = "gpg",
> > .extra_args_verify = { "--keyid-format=long", },
> > .sigs = { PGP_SIGNATURE, PGP_MESSAGE, },
> > },
> > + { .format = "X509", .program = "gpgsm",
>
> Similarly to my comment about "PGP", I think this would do well as
> "x509".
Another naming discussion, lets keep discussing and i will implement it
once settled.
Henning
Re: [PATCH 2/3] send-email: accept long lines with suitable transfer encoding
On Thu, Jul 5, 2018 at 10:24 PM brian m. carlson wrote: > With --validate (which is the default), we warn about lines exceeding > 998 characters due to the limits specified in RFC 5321. However, if > we're using a suitable transfer encoding (quoted-printable or base64), > we're guaranteed not to have lines exceeding 76 characters, so there's > no need to fail in this case. The auto transfer encoding handles this > specific case, so accept it as well. > > Signed-off-by: brian m. carlson > --- > diff --git a/Documentation/git-send-email.txt > b/Documentation/git-send-email.txt > @@ -401,8 +401,9 @@ have been specified, in which case default to 'compose'. > - * Warn of patches that contain lines longer than 998 > characters; this > - is due to SMTP limits as described by > http://www.ietf.org/rfc/rfc2821.txt. > + * Warn of patches that contain lines longer than 998 > characters unless > + a suitable transfer encoding is used; this is due to > SMTP limits as > + described by http://www.ietf.org/rfc/rfc2821.txt. A reader might like to know what "a suitable transfer encoding" is. Perhaps add a "such as 'auto', 'quoted-printable' and 'base64'" comment? > diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh > @@ -480,6 +480,19 @@ test_expect_success $PREREQ 'long lines with auto > encoding are quoted-printable' > +test_expect_success $PREREQ '--validate passes with certain encodings' ' > + for enc in auto quoted-printable base64 > + do > + git send-email \ > + --from="Example " \ > + --to=nob...@example.com \ > + --smtp-server="$(pwd)/fake.sendmail" \ > + --transfer-encoding=$enc \ > + --validate \ > + $patches longline.patch > + done > +' If you turn this inside out, it will be easier to figure out which encoding fails (if one ever does). That is: for enc in auto quoted-printable base64 do test_expect_success $PREREQ "--validate passes with encoding $enc" ' git send-email ... ' done
Re: [PATCH 1/3] send-email: add an auto option for transfer encoding
On Thu, Jul 5, 2018 at 10:24 PM brian m. carlson
wrote:
> For most patches, using a transfer encoding of 8bit provides good
> compatibility with most servers and makes it as easy as possible to view
> patches. However, there are some patches for which 8bit is not a valid
> encoding: RFC 5321 specifies that a message must not have lines
> exceeding 998 octets.
>
> Add a transfer encoding value, auto, which indicates that a patch should
> use 8bit where allowed and quoted-printable otherwise. Choose
> quoted-printable instead of base64, since base64-encoded plain text is
> treated as suspicious by some spam filters.
>
> Signed-off-by: brian m. carlson
> ---
> diff --git a/git-send-email.perl b/git-send-email.perl
> @@ -1852,13 +1851,16 @@ sub apply_transfer_encoding {
> + $to = ($message =~ /.{999,}/) ? 'quoted-printable' :'8bit'
> + if $to eq 'auto';
Style: space after colon: 'quoted-printable' : '8bit'
> diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
> +test_expect_success $PREREQ 'long lines with auto encoding are
> quoted-printable' '
> + clean_fake_sendmail &&
> + git send-email \
> + --from="Example " \
> + --to=nob...@example.com \
> + --smtp-server="$(pwd)/fake.sendmail" \
> + --transfer-encoding=auto \
> + --no-validate \
> + longline.patch \
> + 2>errors &&
Why capture stderr to a file then ignore the file?
> + grep "Content-Transfer-Encoding: quoted-printable" msgtxt1
> +'
