Re: [PATCH RFC] git-am: support any number of signatures
On Tue, Jun 17, 2014 at 11:49:11PM -0700, Junio C Hamano wrote: > On Tue, Jun 17, 2014 at 8:09 PM, Michael S. Tsirkin wrote: > > > > OK, after looking into this for a while, I realize > > this is a special property of the Signed-off-by footer. > > For now I think it's reasonable to just avoid de-duplicating > > other footers if any. Agree? > > Not really. I'd rather see "git am" hardcode as little such policy as > possible. > We do need to support S-o-b footer and the policy we defined for it long time > ago, if only for backward compatiblity, but for any other footers, > policy decision > such as "dedup by default" isn't something "am" should know about. OK happily that's exactly what v2 that I just posted does. Default S-o-b footer gets the existing policy. Any other footers are added on top without any tricky deduplication. -- MST -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Why does gpg.program work for commit but not log?
On Wed, Jun 18, 2014 at 12:18:35AM -0400, Jason Pyeron wrote: > jpyeron@black /projects/microsoft-smartcard-sign/tmp > $ git --version > git version 1.7.9 That's rather old. In the meantime we have: commit 6005dbb9bf21d10b209f7924e305bd04b9ab56d2 Author: Jacob Sarvis Date: Wed Mar 27 10:13:39 2013 -0500 log: read gpg settings for signed commit verification "show --show-signature" and "log --show-signature" do not read the gpg.program setting from git config, even though, commit signing, tag signing, and tag verification honor it. which is in v1.8.3. In general, please double-check your problem against a recent version of "master" when making a bug report. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4] http: fix charset detection of extract_content_type()
On Wed, Jun 18, 2014 at 07:11:53AM +0900, Yi EungJun wrote: > From: Yi EungJun > > extract_content_type() could not extract a charset parameter if the > parameter is not the first one and there is a whitespace and a following > semicolon just before the parameter. For example: > > text/plain; format=fixed ;charset=utf-8 > > And it also could not handle correctly some other cases, such as: > > text/plain; charset=utf-8; format=fixed > text/plain; some-param="a long value with ;semicolons;"; charset=utf-8 > > Thanks-to: Jeff King > Signed-off-by: Yi EungJun Thanks, this version looks good to me. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 3/9] fetch doc: update note on '+' in front of the refspec
On 06/04/2014 12:16 AM, Junio C Hamano wrote: > While it is not *wrong* per-se to say that pulling a rewound/rebased > branch will lead to an unnecessary merge conflict, that is not what > the leading "+" sign to allow non-fast-forward update of remote-tracking > branch is at all. > > Helped-by: Marc Branchaud > Signed-off-by: Junio C Hamano > --- > Documentation/pull-fetch-param.txt | 18 +- > 1 file changed, 9 insertions(+), 9 deletions(-) > > diff --git a/Documentation/pull-fetch-param.txt > b/Documentation/pull-fetch-param.txt > index 18cffc2..41474c5 100644 > --- a/Documentation/pull-fetch-param.txt > +++ b/Documentation/pull-fetch-param.txt > @@ -24,15 +24,15 @@ is updated even if it does not result in a fast-forward > update. > + > [NOTE] > -If the remote branch from which you want to pull is > -modified in non-linear ways such as being rewound and > -rebased frequently, then a pull will attempt a merge with > -an older version of itself, likely conflict, and fail. > -It is under these conditions that you would want to use > -the `+` sign to indicate non-fast-forward updates will > -be needed. There is currently no easy way to determine > -or declare that a branch will be made available in a > -repository with this behavior; the pulling user simply > +When the remote branch you want to fetch is known to > +be rewound and rebased regularly, it is expected that > +its new tip will not be descendant of its previous tip s/will not be descendant/will not be a descendant/ to fix a typo, and maybe s/will not be descendant/will sometimes not be a descendant/ because sometimes it *will* be a descendant. > +(as stored in your remote-tracking branch the last time > +you fetched). You would want > +to use the `+` sign to indicate non-fast-forward updates > +will be needed for such branches. There is no way to > +determine or declare that a branch will be made available > +in a repository with this behavior; the pulling user simply > must know this is the expected usage pattern for a branch. > + > [NOTE] > -- Michael Haggerty mhag...@alum.mit.edu http://softwareswirl.blogspot.com/ -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: progit build instructions and ExtUtils::MakeMaker
[I added a subject to the thread; that is likely part of why you didn't get any responses sooner] On Sun, Jun 01, 2014 at 02:24:54PM -0700, C. Benson Manica wrote: > The documentation for installing git from source here, > http://git-scm.com/book/en/Getting-Started-Installing-Git, incorrectly > fails to mention that the MakeMaker perl module is also required and > is installable via > > $ yum install perl-ExtUtils-MakeMaker The content at git-scm.com/book is pulled from the Creative Commons "Pro Git" book. Errata and corrections are generally handled by opening an issue there: https://github.com/progit/progit/issues Note that ExtUtils::MakeMaker is part of the perl core (and has been for quite a long time). So on many systems, I would not expect it to need to be installed separately. If you propose a change for Pro Git, you may want to find out which systems package it separately and which do not (Debian, for example, does not, but it also does not use yum). > Also, you might want to let people know that you've configured your > mail system for 1987 mode and do not accept HTML-formatted mail. Come on, MIME wasn't standardized until 1996. We're easily state of the art for the mid-1990's. :) -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [puzzled and solved] "shortlog" not quite understanding all "log" options
On Fri, May 30, 2014 at 02:37:02PM -0700, Junio C Hamano wrote: > > I am slightly puzzled why parse_revision_opt does not just call > > handle_revision_pseudo_opt. According to f6aca0dc4, it is because > > pseudo-options need to be acted on in-order, as they affect things like > > subsequent "--not" options, etc. But if we are using parse_options_step, > > shouldn't we be handling the options in order? > > > > I am sure I am just missing something obvious, so do not trouble > > yourself if you do not know the answer offhand. > > Sorry, I don't know ;-) Hopefully I am not wasting your time by responding to an old thread, but I figured this out and wanted to post it for posterity. The answer is that it is not about handling _options_ in order, but that we need to handle pseudo-options in order with non-options, like: foo --not bar Stepping through the options with parseopt will just cover dashed options, but we handle non-option arguments later. So we have to handle the pseudo-arguments like "--not" at the same later time. So there's nothing interesting to clean up or fix here. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 0/3] add strnncmp() function
On Tue, Jun 17, 2014 at 01:08:05PM +0200, Torsten Bögershausen wrote: > On 2014-06-17 09.34, Jeremiah Mahler wrote: > > Add a strnncmp() function which behaves like strncmp() except it takes > > the length of both strings instead of just one. > > > > Then simplify tree-walk.c and unpack-trees.c using this new function. > > Replace all occurrences of name_compare() with strnncmp(). Remove > > name_compare(), which they both had identical copies of. > > > > Version 2 includes suggestions from Jonathan Neider [1]: > > > > - Fix the logic which caused the new strnncmp() to behave differently > > from the old version. Now it is identical to strncmp(). > > > > - Improve description of strnncmp(). > > > > Also, strnncmp() was switched from using memcmp() to strncmp() > > internally to make it clear that this is meant for strings, not > > general buffers. > I don't think this is a good change, for 2 reasons: > - It changes the semantics of existing code, which should be carefully > reviewed, documented and may be put into a seperate commit. > - Looking into the code for memcmp() and strncmp() in libc, > I can see that memcmp() is written in 13 lines of assembler, > (on a 386 system) with a fast > repz cmpsb %es:(%edi),%ds:(%esi) > working as the core engine. > > strncmp() uses 83 lines of assembler, because after each comparison > the code needs to check of the '\0' in both strings. > - I can't see a reason to replace efficient code with less efficient code, > so moving the old function "as is" into a include file, and declare > it "static inline" could be the first step. > That is not true, a rep cmpsb was fast for 486 but is relatively slow for newer processors. For performance a correct answer is to measure it than do blind guess. Are these strings null terminated or is giving a size just a hint? If it is a hint then a plain strcmp could be faster (this depends on implementation). A reason is that for implementations that check more bytes at once it is easier to combine a terminating null mask with difference than trying to first find which of first 16 bytes are different and then compare if it is within size. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
cherry-pick lost
Hi: I delete a file and push to master branch, after code reviewing in gerrit, then click 'Cherry Pick To' button to cherry-pick to release/1.1 branch, and then code review and merge... but now, the cherry-pick commit seems being lost, the should be deleted file is still on release/1.1 branch. I have encountered this circumstance twice. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 0/1] receive-pack: optionally deny case clone refs
On 06/13/2014 11:25 PM, Junio C Hamano wrote: > Ronnie Sahlberg writes: > >> It gets even more hairy : >> If the server has A/a and a/b and you clone it it becomes A/a and A/b >> locally. Then you push back to the server and you end up with three >> refs on the server: A/a A/b and a/b. > > That is part of the transition in deployment. David who wants to > forbid A/a and a/b mixed in his project will surely correct the > situation at the server end so "somebody fetches A/a and a/b and > ends up with A/a and A/b" will not happen. They will fetch A/a and > A/b. > > If a user is with an older Git and he has his own a/c, fetching A/a > and A/b from a corrected central repository will still give the user > a/a and a/b, but then pushing it back will be forbidden. The user's > repository needs to be fixed and installation of Git needs to be > updated to the version with an equivalent of David's "optionally > deny" feature implemented for the fetching side, so that the user > notices the local a/c is bad and not allowed within the context of > his project, deletes it and recreates it as A/c before he can fetch > A/a and A/b from the central repository. > > I agree that the transition may be painful, but as long as the > desired semantics is "If you have A/a, you are not allowed to have > a/a or a/b", it cannot be avoided---in that sense, I view it as a > lower priority issue. > > Having said that, it may indicate that the desired semantics above > may not be the optimal one. Perhaps the flag might want to be "on > this platform, we cannot do case sensitive refs, so pretend as if > all refs are lowercase" instead. I suspect that such a flag may > allow smoother transition than what has been proposed. > > Underlying refs "A/a" and "a/b" can stay as they are in David's > central repository, but ref enumeration with the flag enabled will > return a/a and a/b, and these are the names that will be fetched by > the users. If the user had an existing A/c, then fetching these > will still create A/a and A/b locally, but pushing them back will, > under that flag enabled, be turned into updates to a/a, a/b, and a/c > on the central server side with updated Git. The discussion here has made it pretty clear that, given our current loose reference and reflog storage schemes, it is not possible to implement case-sensitive references or even case-insensitive but case-preserving references correctly on a non-case-sensitive filesystem. We would always have spooky non-local conflicts like A/a vs. a/b. I think we *could* correctly implement * case-folded reference names (e.g., all lower-case; I wonder if that would also apply to HEAD etc.?) * case-folded reference names except for the last component, which could be case-insensitive but case-preserving: refs/heads/MyCrazyBranch. I suppose that many mixed-OS projects effectively use this policy nowadays and that is why we don't hear more complaints about this Git deficiency. If we had an option to use only packed references in a repository, I think we could also implement case-insensitive but case-preserving references on a non-case-preserving filesystem. The packed-refs file would be authoritative WRT case, and the case of the reflog directories and files would be ignored. But I would be nervous about promoting this style, because it will likely cause performance problems for repos that have a lot of refs. To support arbitrary refname policies on arbitrary filesystems, we of course need a different way of storing references and reflogs. Michael -- Michael Haggerty mhag...@alum.mit.edu http://softwareswirl.blogspot.com/ -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[RFC] rebase --root: Empty root commit is replaced with sentinel
`rebase` supports the option `--root` both with and without `--onto`. The case where `--onto` is not specified is handled by creating a sentinel commit and squashing the root commit into it. The sentinel commit refers to the empty tree and does not have a log message associated with it. Its purpose is that `rebase` can rely on having a rebase base even without `--onto`. The combination of `--root` and no `--onto` implies an interactive rebase. When `--preserve-merges` is not specified on the `rebase` command line, `rebase--interactive` uses `--cherry-pick` with git-rev-list to put the initial to-do list together. If the root commit is empty, it is treated as a cherry-pick of the sentinel commit and omitted from the todo-list. This is unexpected because the user does not know of the sentinel commit. Add a test case. Create an empty root commit, run `rebase --root` and check that it is still there. If the branch consists of the root commit only, the bug described above causes the resulting history to consist of the sentinel commit only. If the root commit has children, the resulting history contains neither the root nor the sentinel commit. This behaviour is the same with `--keep-empty`. Signed-off-by: Fabian Ruch --- Notes: Hi, This is not a fix yet. We are currently special casing in `do_pick` and whether the current head is the sentinel commit is not a special case that would fit into `do_pick`'s interface description. What if we added the feature of creating root commits to `do_pick`, using `commit-tree` just like when creating the sentinel commit? We would have to add another special case (`test -z "$onto"`) to where the to-do list is put together in `rebase--interactive`. An empty `$onto` would imply git rev-list $orig_head to form the to-do list. The rebase comment in the commit message editor would have to become something similar to Rebase $shortrevisions as new history , which might be even less confusing than mentioning the hash of the sentinel commit. Fabian t/t3412-rebase-root.sh | 27 +++ 1 file changed, 27 insertions(+) diff --git a/t/t3412-rebase-root.sh b/t/t3412-rebase-root.sh index 0b52105..a4fe3c7 100755 --- a/t/t3412-rebase-root.sh +++ b/t/t3412-rebase-root.sh @@ -278,4 +278,31 @@ test_expect_success 'rebase -i -p --root with conflict (second part)' ' test_cmp expect-conflict-p out ' +test_expect_success 'rebase --root recreates empty root commit' ' + echo Initial >expected.msg && + # commit the empty tree, no parents + empty_tree=$(git hash-object -t tree /dev/null) && + empty_root_commit=$(git commit-tree $empty_tree -F expected.msg) && + git checkout -b empty-root-commit-only $empty_root_commit && + # implies interactive + git rebase --keep-empty --root && + git show --pretty=format:%s HEAD >actual.msg && + test_cmp actual.msg expected.msg +' + +test_expect_success 'rebase --root recreates empty root commit (subsequent commits)' ' + echo Initial >expected.msg && + # commit the empty tree, no parents + empty_tree=$(git hash-object -t tree /dev/null) && + empty_root_commit=$(git commit-tree $empty_tree -F expected.msg) && + git checkout -b empty-root-commit $empty_root_commit && + >file && + git add file && + git commit -m file && + # implies interactive + git rebase --keep-empty --root && + git show --pretty=format:%s HEAD^ >actual.msg && + test_cmp actual.msg expected.msg +' + test_done -- 2.0.0 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 9/9] fetch: allow explicit --refmap to override configuration
On 06/05/2014 08:36 PM, Junio C Hamano wrote: > Marc Branchaud writes: > >> I don't have any objection to the option per se. But I do wonder if there's >> a need to add yet another knob to git just for completeness. Has anyone ever >> needed this? > > It is not a good yardstick, as everybody has survived without it > since Git's inception. The right question to ask is: would it help > new use patterns, or improve existing use patterns? I agree that this feature is pretty esoteric and probably more cognitive load than it's worth. One of your use cases has workarounds shown below. > Two possible scenarios I can think of offhand are > > * using an empty refmap to ensure that your "fetch" this time is >really ephemeral without affecting the longer-term configured >remote-tracking branches Doesn't specifying an explicit URL get around the refspecs configured for the remote? E.g., git fetch $(git config remote.github.url) master Or if we had a way to temporarily unset multivalued configuration values (which we may have soon thanks to the GSoC project of Tanay Abhra), one could use git --unset=remote.github.fetch fetch github master > * grabbing only a few selected branches out of hundreds, e.g. > >$ git fetch https://github.com/gitster/git \ >--refmap=refs/heads/*:refs/remotes/jch/* maint master next +pu > >instead of having to spell its long-hand > >$ git fetch https://github.com/gitster/git \ >refs/heads/maint:refs/remotes/jch/maint \ >refs/heads/master:refs/remotes/jch/master \ >refs/heads/next:refs/remotes/jch/next \ >+refs/heads/pu:refs/remotes/jch/pu I'm not quite sure what your goal is here, but if you want to fetch some branches on the fly without setting up a remote, then git -c remote.github.fetch='refs/heads/*:refs/remotes/jch/*' \ fetch https://github.com/gitster/git maint master next +pu should work, no? > but there may be more useful scenarios other people can come up > with ;-). Michael -- Michael Haggerty mhag...@alum.mit.edu -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: Why does gpg.program work for commit but not log?
> -Original Message- > From: git-ow...@vger.kernel.org > [mailto:git-ow...@vger.kernel.org] On Behalf Of Jeff King > Sent: Wednesday, June 18, 2014 3:37 > To: Jason Pyeron > Cc: git@vger.kernel.org > Subject: Re: Why does gpg.program work for commit but not log? > > On Wed, Jun 18, 2014 at 12:18:35AM -0400, Jason Pyeron wrote: > > > jpyeron@black /projects/microsoft-smartcard-sign/tmp > > $ git --version > > git version 1.7.9 > > That's rather old. In the meantime we have: > > commit 6005dbb9bf21d10b209f7924e305bd04b9ab56d2 > Author: Jacob Sarvis > Date: Wed Mar 27 10:13:39 2013 -0500 > > log: read gpg settings for signed commit verification > > "show --show-signature" and "log --show-signature" > do not read the > gpg.program setting from git config, even though, > commit signing, > tag signing, and tag verification honor it. > > which is in v1.8.3. In general, please double-check your problem > against a recent version of "master" when making a bug report. > I will (try to) compile master and test. This is the latest version in cygwin. -- -=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=- - - - Jason Pyeron PD Inc. http://www.pdinc.us - - Principal Consultant 10 West 24th Street #100- - +1 (443) 269-1555 x333Baltimore, Maryland 21218 - - - -=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=- This message is copyright PD Inc, subject to license 20080407P00. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Why does gpg.program work for commit but not log?
On Wed, Jun 18, 2014 at 08:38:32AM -0400, Jason Pyeron wrote: > > That's rather old. In the meantime we have: > > > > commit 6005dbb9bf21d10b209f7924e305bd04b9ab56d2 > [...] > > I will (try to) compile master and test. This is the latest version in cygwin. To save you some trouble, I actually found that commit by reproducing your problem on Linux and bisecting. So I'm fairly sure it will fix it for you. :) You may want to bug the cygwin packagers about updating their version of git. v1.7.9 is two and a half years old, and since then we have many bugfixes, including some which are specifically targeted at cygwin (which can only lead me to assume a lot of people are building from source on cygwin). -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 0/1] receive-pack: optionally deny case clone refs
On Wed, Jun 18, 2014 at 4:33 AM, Michael Haggerty wrote: > On 06/13/2014 11:25 PM, Junio C Hamano wrote: >> Ronnie Sahlberg writes: >> >>> It gets even more hairy : >>> If the server has A/a and a/b and you clone it it becomes A/a and A/b >>> locally. Then you push back to the server and you end up with three >>> refs on the server: A/a A/b and a/b. >> >> That is part of the transition in deployment. David who wants to >> forbid A/a and a/b mixed in his project will surely correct the >> situation at the server end so "somebody fetches A/a and a/b and >> ends up with A/a and A/b" will not happen. They will fetch A/a and >> A/b. >> >> If a user is with an older Git and he has his own a/c, fetching A/a >> and A/b from a corrected central repository will still give the user >> a/a and a/b, but then pushing it back will be forbidden. The user's >> repository needs to be fixed and installation of Git needs to be >> updated to the version with an equivalent of David's "optionally >> deny" feature implemented for the fetching side, so that the user >> notices the local a/c is bad and not allowed within the context of >> his project, deletes it and recreates it as A/c before he can fetch >> A/a and A/b from the central repository. >> >> I agree that the transition may be painful, but as long as the >> desired semantics is "If you have A/a, you are not allowed to have >> a/a or a/b", it cannot be avoided---in that sense, I view it as a >> lower priority issue. >> >> Having said that, it may indicate that the desired semantics above >> may not be the optimal one. Perhaps the flag might want to be "on >> this platform, we cannot do case sensitive refs, so pretend as if >> all refs are lowercase" instead. I suspect that such a flag may >> allow smoother transition than what has been proposed. >> >> Underlying refs "A/a" and "a/b" can stay as they are in David's >> central repository, but ref enumeration with the flag enabled will >> return a/a and a/b, and these are the names that will be fetched by >> the users. If the user had an existing A/c, then fetching these >> will still create A/a and A/b locally, but pushing them back will, >> under that flag enabled, be turned into updates to a/a, a/b, and a/c >> on the central server side with updated Git. > > The discussion here has made it pretty clear that, given our current > loose reference and reflog storage schemes, it is not possible to > implement case-sensitive references or even case-insensitive but > case-preserving references correctly on a non-case-sensitive filesystem. > We would always have spooky non-local conflicts like A/a vs. a/b. > > I think we *could* correctly implement > > * case-folded reference names (e.g., all lower-case; I wonder if > that would also apply to HEAD etc.?) > > * case-folded reference names except for the last component, which > could be case-insensitive but case-preserving: > refs/heads/MyCrazyBranch. I suppose that many mixed-OS projects > effectively use this policy nowadays and that is why we don't hear > more complaints about this Git deficiency. > > If we had an option to use only packed references in a repository, I > think we could also implement case-insensitive but case-preserving > references on a non-case-preserving filesystem. The packed-refs file > would be authoritative WRT case, and the case of the reflog directories > and files would be ignored. But I would be nervous about promoting this > style, because it will likely cause performance problems for repos that > have a lot of refs. > > To support arbitrary refname policies on arbitrary filesystems, we of > course need a different way of storing references and reflogs. > Agree. I think the transaction work can help here for both the cases of refs (which might be solved by a pacxked-refs only setting) and reflogs. My next two (small) series for transactions : https://github.com/rsahlberg/git/tree/ref-transactions-reflog https://github.com/rsahlberg/git/tree/ref-transactions-rename reworks the handling of reflogs and all reflog API callers. With these two series we get to a stage where the reflog API and implementation both shrinks and becomes more well defined. We essentially end up with only these functions for touching reflogs: check if a reflog exists: extern int reflog_exists(const char *refname); create/initialize an empty reflog: extern int create_reflog(const char *refname); delete a reflog: extern int delete_reflog(const char *refname); these two functions iterate/read over all entries for a particular reflog: int for_each_reflog_ent(const char *refname, each_reflog_ent_fn fn, void *cb_data); int for_each_reflog_ent_reverse(const char *refname, each_reflog_ent_fn fn, void *cb_data); iterate over all reflogs: extern int for_each_reflog(each_ref_fn, void *); write/append an entry to a reflog: int transaction_update_reflog(struct ref_transaction *transaction, When we get to that point it will become much easier if we want to ch
Re: [PATCH v5 10/11] trace: add trace_performance facility to debug performance issues
Am 17.06.2014 19:11, schrieb Junio C Hamano:
> Karsten Blees writes:
>
>> Simple use case (measure one code section):
>>
>> uint64_t start = getnanotime();
>> /* code section to measure */
>> trace_performance_since(start, "foobar");
>>
>> Medium use case (measure consecutive code sections):
>>
>> uint64_t start = getnanotime();
>> /* first code section to measure */
>> start = trace_performance_since(start, "first foobar");
>> /* second code section to measure */
>> trace_performance_since(start, "second foobar");
>>
>> Complex use case (measure repetitive code sections):
>>
>> uint64_t t = 0;
>> for (;;) {
>> /* ignore */
>> t -= getnanotime();
>> /* code section to measure */
>> t += getnanotime();
>> /* ignore */
>> }
>> trace_performance(t, "frotz");
>
> Hmph. Even though trace_performance() makes an extra call to
> getnanotime() in order to return, examples do not use the returned
> value? The second example is a good illustration why it makes sense
> for trace_performance_since(), though.
>
Right, it makes no sense for trace_performance(), and for
trace_performance_since() only if followed by another 'measured' code
section. In that special case, I think it wouldn't hurt if you had to
write:
uint64_t start = getnanotime();
/* first code section to measure */
trace_performance_since(start, "first foobar");
start = getnanotime();
/* second code section to measure */
trace_performance_since(start, "second foobar");
So I guess I'll drop the return value (and the second example, which
is then redundant to the first).
>> +static void trace_performance_vfl(const char *file, int line,
>> + uint64_t nanos, const char *format,
>> + va_list ap)
>> +{
>
> Just being curious, but what does "v" stand for?
>
trace_performance_vfl(, va_list)
vs.
trace_performance_fl(, ...)
Will change to trace_performance_vprintf_fl()
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 09/11] trace: add high resolution timer function to debug performance issues
Am 17.06.2014 18:44, schrieb Junio C Hamano: > Karsten Blees writes: > >> Am 11.06.2014 10:01, schrieb Karsten Blees: >>> the epoch allows using the results (div 10e9) with other time-related APIs. >> >> s/10e9/1e9/ > > That replacement is fine but the "(div 1e9)" still wants to be > clarified. What did you exactly mean by that? If the result is > divided by 10^9 then it yields the number of seconds? > > Thanks. > Div 10^9 yields a time_t. But as not all time-related APIs use time_t, perhaps its best to drop the "(div 1e9)" altogether. That the function returns nanoseconds should be clear enough. Will drop in the next round. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 00/11] add performance tracing facility
Am 12.06.2014 20:30, schrieb Junio C Hamano:
> Karsten Blees writes:
>
>> Here's v5 of the performance tracing patch series, now including a bunch of
>> cleanups and adding timestamp, file and line to all trace output.
>>
>> I'm particularly interested in feedback for the output format. As file names
>> have different lengths, printing file:line as prefix results in unaligned
>> output:
>>
>> > GIT_TRACE=1 git stash list
>> 00:12:10.544266 git.c:512 trace: exec: 'git-stash' 'list'
>> 00:12:10.544266 run-command.c:337 trace: run_command: 'git-stash' 'list'
>> 00:12:10.649779 git.c:312 trace: built-in: git 'rev-parse' '--git-dir'
>>
>> We could add separators to make it easier to parse, e.g.:
>>
>> > GIT_TRACE=1 git stash list
>> [00:12:10.544266 git.c:512] trace: exec: 'git-stash' 'list'
>> [00:12:10.544266 run-command.c:337] trace: run_command: 'git-stash' 'list'
>> [00:12:10.649779 git.c:312] trace: built-in: git 'rev-parse' '--git-dir'
>
> This is easier to parse if " " and ":" are found in the names of
> _our_ source files and "]" isn't, but is that really the case?
>
By "parsing" I actually meant the HumanEye (tm) parser, not lex/yacc and
friends ("[]" just make nice recognizable separators).
However, I think it shouldn't be too complicated to properly align the output,
at least for the majority of 'short' file names in the git code base. E.g.:
00:12:10.544266 git.c:512trace: exec: 'git-stash' 'list'
00:12:10.544266 run-command.c:337trace: run_command: 'git-stash' 'list'
00:12:10.649779 git.c:312trace: built-in: git 'rev-parse'
'--git-dir'
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Possible bug in `git reset` in 1.9
Suppose I have the following branches: * branch-1 with commits A - B - C * branch-2 with commits A - B - C - D Prior to version 1.9, running `git reset --hard D` while branch-1 is checked out will result in changing the current branch HEAD to commit hash D (essentially what update-ref would do). In 1.9.1 (I haven't tested on 1.9.0 yet), however, running `git reset --hard D` while branch-1 is checked out will result in the following output: $ fatal: Could not parse object 'D' I assume that this is not an expected change as nothing about changes to the reset command is present in the change log for 1.9.0 or 1.9.0. Is this a bug or was the previous behavior unexpected as well? Either way, it's a regression in terms of what can be expected based on previous version. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 08/14] refs.c: add a flag to allow reflog updates to truncate the log
Add a flag that allows us to truncate the reflog before we write the
update.
Signed-off-by: Ronnie Sahlberg
---
refs.c | 17 +++--
refs.h | 10 +-
2 files changed, 24 insertions(+), 3 deletions(-)
diff --git a/refs.c b/refs.c
index d673a0f..c33d19e 100644
--- a/refs.c
+++ b/refs.c
@@ -3741,7 +3741,12 @@ int transaction_commit(struct ref_transaction
*transaction,
}
}
- /* Update all reflog files */
+ /*
+* Update all reflog files
+* We have already done all ref updates and deletes.
+* There is not much we can do here if there are any reflog
+* update errors other than complain.
+*/
for (i = 0; i < n; i++) {
struct ref_update *update = updates[i];
@@ -3749,7 +3754,15 @@ int transaction_commit(struct ref_transaction
*transaction,
continue;
if (update->reflog_fd == -1)
continue;
-
+ if (update->flags & REFLOG_TRUNCATE)
+ if (lseek(update->reflog_fd, 0, SEEK_SET) < 0 ||
+ ftruncate(update->reflog_fd, 0)) {
+ error("Could not truncate reflog: %s. %s",
+ update->refname, strerror(errno));
+ rollback_lock_file(&update->reflog_lock);
+ update->reflog_fd = -1;
+ continue;
+ }
if (log_ref_write_fd(update->reflog_fd, update->old_sha1,
update->new_sha1,
update->committer, update->msg)) {
diff --git a/refs.h b/refs.h
index bf52068..f14c8db 100644
--- a/refs.h
+++ b/refs.h
@@ -328,7 +328,15 @@ int transaction_delete_sha1(struct ref_transaction
*transaction,
struct strbuf *err);
/*
- * Append a reflog entry for refname.
+ * Flags controlling transaction_update_reflog().
+ * REFLOG_TRUNCATE: Truncate the reflog.
+ *
+ * Flags >= 0x100 are reserved for internal use.
+ */
+#define REFLOG_TRUNCATE 0x01
+/*
+ * Append a reflog entry for refname. If the REFLOG_TRUNCATE flag is set
+ * this update will first truncate the reflog before writing the entry.
*/
int transaction_update_reflog(struct ref_transaction *transaction,
const char *refname,
--
2.0.0.467.g08c0633
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 05/14] refs.c: add a function to append a reflog entry to a fd
Break out the code to create the string and writing it to the file
descriptor from log_ref_write and into a dedicated function log_ref_write_fd.
For now this is only used from log_ref_write but later on we will call
this function from reflog transactions too which means that we will end
up with only a single place where we write a reflog entry to a file instead
of the current two places (log_ref_write and builtin/reflog.c).
Signed-off-by: Ronnie Sahlberg
---
refs.c | 48 ++--
1 file changed, 30 insertions(+), 18 deletions(-)
diff --git a/refs.c b/refs.c
index 4129de6..f203285 100644
--- a/refs.c
+++ b/refs.c
@@ -2865,15 +2865,37 @@ int log_ref_setup(const char *refname, char *logfile,
int bufsize)
return 0;
}
+static int log_ref_write_fd(int fd, const unsigned char *old_sha1,
+ const unsigned char *new_sha1,
+ const char *committer, const char *msg)
+{
+ int msglen, written;
+ unsigned maxlen, len;
+ char *logrec;
+
+ msglen = msg ? strlen(msg) : 0;
+ maxlen = strlen(committer) + msglen + 100;
+ logrec = xmalloc(maxlen);
+ len = sprintf(logrec, "%s %s %s\n",
+ sha1_to_hex(old_sha1),
+ sha1_to_hex(new_sha1),
+ committer);
+ if (msglen)
+ len += copy_msg(logrec + len - 1, msg) - 1;
+
+ written = len <= maxlen ? write_in_full(fd, logrec, len) : -1;
+ free(logrec);
+ if (written != len)
+ return -1;
+
+ return 0;
+}
+
static int log_ref_write(const char *refname, const unsigned char *old_sha1,
const unsigned char *new_sha1, const char *msg)
{
- int logfd, result, written, oflags = O_APPEND | O_WRONLY;
- unsigned maxlen, len;
- int msglen;
+ int logfd, result, oflags = O_APPEND | O_WRONLY;
char log_file[PATH_MAX];
- char *logrec;
- const char *committer;
if (log_all_ref_updates < 0)
log_all_ref_updates = !is_bare_repository();
@@ -2885,19 +2907,9 @@ static int log_ref_write(const char *refname, const
unsigned char *old_sha1,
logfd = open(log_file, oflags);
if (logfd < 0)
return 0;
- msglen = msg ? strlen(msg) : 0;
- committer = git_committer_info(0);
- maxlen = strlen(committer) + msglen + 100;
- logrec = xmalloc(maxlen);
- len = sprintf(logrec, "%s %s %s\n",
- sha1_to_hex(old_sha1),
- sha1_to_hex(new_sha1),
- committer);
- if (msglen)
- len += copy_msg(logrec + len - 1, msg) - 1;
- written = len <= maxlen ? write_in_full(logfd, logrec, len) : -1;
- free(logrec);
- if (written != len) {
+ result = log_ref_write_fd(logfd, old_sha1, new_sha1,
+ git_committer_info(0), msg);
+ if (result) {
int save_errno = errno;
close(logfd);
error("Unable to append to %s", log_file);
--
2.0.0.467.g08c0633
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 02/14] refs.c: make ref_transaction_delete a wrapper for ref_transaction_update
Signed-off-by: Ronnie Sahlberg
---
refs.c | 17 +
refs.h | 2 +-
2 files changed, 6 insertions(+), 13 deletions(-)
diff --git a/refs.c b/refs.c
index a9f91ab..0eace70 100644
--- a/refs.c
+++ b/refs.c
@@ -3503,24 +3503,17 @@ int ref_transaction_delete(struct ref_transaction
*transaction,
int flags, int have_old, const char *msg,
struct strbuf *err)
{
- struct ref_update *update;
-
if (transaction->state != REF_TRANSACTION_OPEN)
die("BUG: delete called for transaction that is not open");
if (have_old && !old_sha1)
die("BUG: have_old is true but old_sha1 is NULL");
- update = add_update(transaction, refname);
- update->flags = flags;
- update->have_old = have_old;
- if (have_old) {
- assert(!is_null_sha1(old_sha1));
- hashcpy(update->old_sha1, old_sha1);
- }
- if (msg)
- update->msg = xstrdup(msg);
- return 0;
+ if (have_old && is_null_sha1(old_sha1))
+ die("BUG: have_old is true but old_sha1 is null_sha1");
+
+ return ref_transaction_update(transaction, refname, null_sha1,
+ old_sha1, flags, have_old, msg, err);
}
int update_ref(const char *action, const char *refname,
diff --git a/refs.h b/refs.h
index 495740d..20bb152 100644
--- a/refs.h
+++ b/refs.h
@@ -282,7 +282,7 @@ struct ref_transaction *ref_transaction_begin(struct strbuf
*err);
/*
* Add a reference update to transaction. new_sha1 is the value that
- * the reference should have after the update, or zeros if it should
+ * the reference should have after the update, or null_sha1 if it should
* be deleted. If have_old is true and old_sha is not the null_sha1
* then the previous value of the ref must match or the update will fail.
* If have_old is true and old_sha1 is the null_sha1 then the ref must not
--
2.0.0.467.g08c0633
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 14/14] refs.c: remove lock_any_ref_for_update
No one is using this function so we can delete it.
Signed-off-by: Ronnie Sahlberg
---
refs.c | 7 ---
refs.h | 10 +-
2 files changed, 1 insertion(+), 16 deletions(-)
diff --git a/refs.c b/refs.c
index ff98682..95c3eb8 100644
--- a/refs.c
+++ b/refs.c
@@ -2205,13 +2205,6 @@ static struct ref_lock *lock_ref_sha1_basic(const char
*refname,
return NULL;
}
-struct ref_lock *lock_any_ref_for_update(const char *refname,
-const unsigned char *old_sha1,
-int flags, int *type_p)
-{
- return lock_ref_sha1_basic(refname, old_sha1, flags, type_p, NULL, 0);
-}
-
/*
* Write an entry to the packed-refs file for the specified refname.
* If peeled is non-NULL, write it as the entry's peeled value.
diff --git a/refs.h b/refs.h
index 5054388..b674c20 100644
--- a/refs.h
+++ b/refs.h
@@ -178,21 +178,13 @@ extern int ref_exists(const char *);
extern int peel_ref(const char *refname, unsigned char *sha1);
/*
- * Flags controlling lock_any_ref_for_update(), transaction_update_sha1(),
- * transaction_create_sha1(), etc.
+ * Flags controlling transaction_update_sha1(), transaction_create_sha1(), etc.
* REF_NODEREF: act on the ref directly, instead of dereferencing
* symbolic references.
*
* Flags >= 0x100 are reserved for internal use.
*/
#define REF_NODEREF0x01
-/*
- * Locks any ref (for 'HEAD' type refs) and sets errno to something
- * meaningful on failure.
- */
-extern struct ref_lock *lock_any_ref_for_update(const char *refname,
- const unsigned char *old_sha1,
- int flags, int *type_p);
/** Reads log for the value of ref during at_time. **/
extern int read_ref_at(const char *refname, unsigned long at_time, int cnt,
--
2.0.0.467.g08c0633
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 12/14] refs.c: rename log_ref_setup to create_reflog
log_ref_setup is used to do several semi-related things :
* sometimes it will create a new reflog including missing parent directories
and cleaning up any conflicting stale directories in the path.
* fill in a filename buffer for the full path to the reflog.
* unconditionally re-adjust the permissions for the file.
This function is only called from two places: checkout.c where it is always
used to create a reflog and refs.c:log_ref_write where it sometimes are
used to create a reflog and sometimes just used to fill in the filename.
Rename log_ref_setup to create_reflog and change it to only take the
refname as argument to make its signature similar to delete_reflog and
reflog_exists. Change create_reflog to ignore log_all_ref_updates and
"unconditionally" create the reflog when called. Since checkout.c always
wants to create a reflog we can call create_reflog directly and avoid the
temp-and-log_all_ref_update dance.
In log_ref_write, only call create_reflog iff we want to create a reflog
and if the reflog does not yet exist. This means that for the common case
where the log already exists we now only need to perform a single lstat()
instead of a open(O_CREAT)+lstat()+close().
Signed-off-by: Ronnie Sahlberg
---
builtin/checkout.c | 8 +---
refs.c | 22 +-
refs.h | 8 +++-
3 files changed, 17 insertions(+), 21 deletions(-)
diff --git a/builtin/checkout.c b/builtin/checkout.c
index f1dc56e..808c58f 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -583,19 +583,13 @@ static void update_refs_for_switch(const struct
checkout_opts *opts,
if (opts->new_branch) {
if (opts->new_orphan_branch) {
if (opts->new_branch_log && !log_all_ref_updates) {
- int temp;
- char log_file[PATH_MAX];
char *ref_name = mkpath("refs/heads/%s",
opts->new_orphan_branch);
- temp = log_all_ref_updates;
- log_all_ref_updates = 1;
- if (log_ref_setup(ref_name, log_file,
sizeof(log_file))) {
+ if (create_reflog(ref_name)) {
fprintf(stderr, _("Can not do reflog
for '%s'\n"),
opts->new_orphan_branch);
- log_all_ref_updates = temp;
return;
}
- log_all_ref_updates = temp;
}
}
else
diff --git a/refs.c b/refs.c
index 1288c49..9653a01 100644
--- a/refs.c
+++ b/refs.c
@@ -2822,16 +2822,16 @@ static int copy_msg(char *buf, const char *msg)
}
/* This function must set a meaningful errno on failure */
-int log_ref_setup(const char *refname, char *logfile, int bufsize)
+int create_reflog(const char *refname)
{
int logfd, oflags = O_APPEND | O_WRONLY;
+ char logfile[PATH_MAX];
- git_snpath(logfile, bufsize, "logs/%s", refname);
- if (log_all_ref_updates &&
- (starts_with(refname, "refs/heads/") ||
-starts_with(refname, "refs/remotes/") ||
-starts_with(refname, "refs/notes/") ||
-!strcmp(refname, "HEAD"))) {
+ git_snpath(logfile, sizeof(logfile), "logs/%s", refname);
+ if (starts_with(refname, "refs/heads/") ||
+ starts_with(refname, "refs/remotes/") ||
+ starts_with(refname, "refs/notes/") ||
+ !strcmp(refname, "HEAD")) {
if (safe_create_leading_directories(logfile) < 0) {
int save_errno = errno;
error("unable to create directory for %s", logfile);
@@ -2900,16 +2900,20 @@ static int log_ref_write_fd(int fd, const unsigned char
*old_sha1,
static int log_ref_write(const char *refname, const unsigned char *old_sha1,
const unsigned char *new_sha1, const char *msg)
{
- int logfd, result, oflags = O_APPEND | O_WRONLY;
+ int logfd, result = 0, oflags = O_APPEND | O_WRONLY;
char log_file[PATH_MAX];
if (log_all_ref_updates < 0)
log_all_ref_updates = !is_bare_repository();
- result = log_ref_setup(refname, log_file, sizeof(log_file));
+ if (log_all_ref_updates && !reflog_exists(refname))
+ result = create_reflog(refname);
+
if (result)
return result;
+ git_snpath(log_file, sizeof(log_file), "logs/%s", refname);
+
logfd = open(log_file, oflags);
if (logfd < 0)
return 0;
diff --git a/refs.h b/refs.h
index 0564955..e7892fc 100644
--- a/refs.h
+++ b/refs.h
@@ -203,11 +203,6 @@ extern int commit_ref(struct ref_lock *lock);
/** Release any lock taken but not written. **/
extern void unlock_ref(struct ref_lock *lock);
[PATCH v3 10/14] refs.c: allow multiple reflog updates during a single transaction
Allow to make multiple reflog updates to the same ref during a transaction.
This means we only need to lock the reflog once, during the first update
that touches the reflog, and that all further updates can just write the
reflog entry since the reflog is already locked.
This allows us to write code such as:
t = transaction_begin()
transaction_reflog_update(t, "foo", REFLOG_TRUNCATE, NULL);
loop-over-somehting...
transaction_reflog_update(t, "foo", 0, );
transaction_commit(t)
where we first truncate the reflog and then build the new content one line at a
time.
Signed-off-by: Ronnie Sahlberg
---
refs.c | 46 +-
1 file changed, 37 insertions(+), 9 deletions(-)
diff --git a/refs.c b/refs.c
index d6df28d..ad60231 100644
--- a/refs.c
+++ b/refs.c
@@ -30,6 +30,12 @@ static unsigned char refname_disposition[256] = {
*/
#define REF_ISPRUNING 0x0100
/*
+ * Only the first reflog update needs to lock the reflog file. Further updates
+ * just use the lock taken by the first update.
+ */
+#define UPDATE_REFLOG_NOLOCK 0x0200
+
+/*
* Try to read one refname component from the front of refname.
* Return the length of the component found, or -1 if the component is
* not legal. It is legal if it is something reasonable to have under
@@ -3391,7 +3397,7 @@ enum transaction_update_type {
UPDATE_LOG = 1,
};
-/**
+/*
* Information needed for a single ref update. Set new_sha1 to the
* new value or to zero to delete the ref. To check the old value
* while locking the ref, set have_old to 1 and set old_sha1 to the
@@ -3401,7 +3407,7 @@ struct ref_update {
enum transaction_update_type update_type;
unsigned char new_sha1[20];
unsigned char old_sha1[20];
- int flags; /* REF_NODEREF? */
+ int flags; /* REF_NODEREF? or private flags */
int have_old; /* 1 if old_sha1 is valid, 0 otherwise */
struct ref_lock *lock;
int type;
@@ -3409,8 +3415,9 @@ struct ref_update {
/* used by reflog updates */
int reflog_fd;
- struct lock_file reflog_lock;
+ struct lock_file *reflog_lock;
char *committer;
+ struct ref_update *orig_update; /* For UPDATE_REFLOG_NOLOCK */
const char refname[FLEX_ARRAY];
};
@@ -3492,12 +3499,27 @@ int transaction_update_reflog(struct ref_transaction
*transaction,
struct strbuf *err)
{
struct ref_update *update;
+ int i;
if (transaction->state != REF_TRANSACTION_OPEN)
die("BUG: update_reflog called for transaction that is not "
"open");
update = add_update(transaction, refname, UPDATE_LOG);
+ update->flags = flags;
+ for (i = 0; i < transaction->nr - 1; i++) {
+ if (transaction->updates[i]->update_type != UPDATE_LOG)
+ continue;
+ if (!strcmp(transaction->updates[i]->refname,
+ update->refname)) {
+ update->flags |= UPDATE_REFLOG_NOLOCK;
+ update->orig_update = transaction->updates[i];
+ break;
+ }
+ }
+ if (!(update->flags & UPDATE_REFLOG_NOLOCK))
+ update->reflog_lock = xcalloc(1, sizeof(struct lock_file));
+
hashcpy(update->new_sha1, new_sha1);
hashcpy(update->old_sha1, old_sha1);
update->reflog_fd = -1;
@@ -3513,7 +3535,6 @@ int transaction_update_reflog(struct ref_transaction
*transaction,
}
if (msg)
update->msg = xstrdup(msg);
- update->flags = flags;
return 0;
}
@@ -3690,10 +3711,15 @@ int transaction_commit(struct ref_transaction
*transaction,
if (update->update_type != UPDATE_LOG)
continue;
+ if (update->flags & UPDATE_REFLOG_NOLOCK) {
+ update->reflog_fd = update->orig_update->reflog_fd;
+ update->reflog_lock = update->orig_update->reflog_lock;
+ continue;
+ }
update->reflog_fd = hold_lock_file_for_append(
- &update->reflog_lock,
+ update->reflog_lock,
git_path("logs/%s", update->refname),
- 0);
+ LOCK_NODEREF);
if (update->reflog_fd < 0) {
const char *str = "Cannot lock reflog for '%s'. %s";
@@ -3759,7 +3785,7 @@ int transaction_commit(struct ref_transaction
*transaction,
ftruncate(update->reflog_fd, 0)) {
error("Could not truncate reflog: %s. %s",
update->refname, strerror(errno));
- rollback_lock_file(&update->reflog_lock);
+
[PATCH v3 09/14] refs.c: only write reflog update if msg is non-NULL
When performing a reflog transaction update, only write to the reflog iff
msg is non-NULL. This can then be combined with REFLOG_TRUNCATE to perform
an update that only truncates but does not write.
Signed-off-by: Ronnie Sahlberg
---
refs.c | 5 +++--
refs.h | 1 +
2 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/refs.c b/refs.c
index c33d19e..d6df28d 100644
--- a/refs.c
+++ b/refs.c
@@ -3763,8 +3763,9 @@ int transaction_commit(struct ref_transaction
*transaction,
update->reflog_fd = -1;
continue;
}
- if (log_ref_write_fd(update->reflog_fd, update->old_sha1,
-update->new_sha1,
+ if (update->msg &&
+ log_ref_write_fd(update->reflog_fd,
+update->old_sha1, update->new_sha1,
update->committer, update->msg)) {
error("Could write to reflog: %s. %s",
update->refname, strerror(errno));
diff --git a/refs.h b/refs.h
index f14c8db..1d7906c 100644
--- a/refs.h
+++ b/refs.h
@@ -337,6 +337,7 @@ int transaction_delete_sha1(struct ref_transaction
*transaction,
/*
* Append a reflog entry for refname. If the REFLOG_TRUNCATE flag is set
* this update will first truncate the reflog before writing the entry.
+ * If msg is NULL no update will be written to the log.
*/
int transaction_update_reflog(struct ref_transaction *transaction,
const char *refname,
--
2.0.0.467.g08c0633
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 03/14] refs.c: rename the transaction functions
Rename the transaction functions. Remove the leading ref_ from the
names and append _sha1 to the names for functions that create/delete/
update sha1 refs.
Signed-off-by: Ronnie Sahlberg
---
branch.c | 11 ---
builtin/commit.c | 14 -
builtin/fetch.c| 12
builtin/receive-pack.c | 14 -
builtin/replace.c | 10 +++---
builtin/tag.c | 10 +++---
builtin/update-ref.c | 22 +++---
fast-import.c | 22 +++---
refs.c | 82 +-
refs.h | 56 +-
sequencer.c| 12
walker.c | 16 +-
12 files changed, 141 insertions(+), 140 deletions(-)
diff --git a/branch.c b/branch.c
index e0439af..6fa6d78 100644
--- a/branch.c
+++ b/branch.c
@@ -298,13 +298,14 @@ void create_branch(const char *head,
struct ref_transaction *transaction;
struct strbuf err = STRBUF_INIT;
- transaction = ref_transaction_begin(&err);
+ transaction = transaction_begin(&err);
if (!transaction ||
- ref_transaction_update(transaction, ref.buf, sha1,
- null_sha1, 0, !forcing, msg, &err) ||
- ref_transaction_commit(transaction, &err))
+ transaction_update_sha1(transaction, ref.buf, sha1,
+ null_sha1, 0, !forcing, msg,
+ &err) ||
+ transaction_commit(transaction, &err))
die("%s", err.buf);
- ref_transaction_free(transaction);
+ transaction_free(transaction);
}
if (real_ref && track)
diff --git a/builtin/commit.c b/builtin/commit.c
index c499826..bf8d85a 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1762,17 +1762,17 @@ int cmd_commit(int argc, const char **argv, const char
*prefix)
strbuf_insert(&sb, 0, reflog_msg, strlen(reflog_msg));
strbuf_insert(&sb, strlen(reflog_msg), ": ", 2);
- transaction = ref_transaction_begin(&err);
+ transaction = transaction_begin(&err);
if (!transaction ||
- ref_transaction_update(transaction, "HEAD", sha1,
- current_head ?
- current_head->object.sha1 : NULL,
- 0, !!current_head, sb.buf, &err) ||
- ref_transaction_commit(transaction, &err)) {
+ transaction_update_sha1(transaction, "HEAD", sha1,
+ current_head ?
+ current_head->object.sha1 : NULL,
+ 0, !!current_head, sb.buf, &err) ||
+ transaction_commit(transaction, &err)) {
rollback_index_files();
die("%s", err.buf);
}
- ref_transaction_free(transaction);
+ transaction_free(transaction);
unlink(git_path("CHERRY_PICK_HEAD"));
unlink(git_path("REVERT_HEAD"));
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 52f1ebc..baf7929 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -385,22 +385,22 @@ static int s_update_ref(const char *action,
rla = default_rla.buf;
snprintf(msg, sizeof(msg), "%s: %s", rla, action);
- transaction = ref_transaction_begin(&err);
+ transaction = transaction_begin(&err);
if (!transaction ||
- ref_transaction_update(transaction, ref->name, ref->new_sha1,
- ref->old_sha1, 0, check_old, msg, &err))
+ transaction_update_sha1(transaction, ref->name, ref->new_sha1,
+ ref->old_sha1, 0, check_old, msg, &err))
goto fail;
- ret = ref_transaction_commit(transaction, &err);
+ ret = transaction_commit(transaction, &err);
if (ret == UPDATE_REFS_NAME_CONFLICT)
df_conflict = 1;
if (ret)
goto fail;
- ref_transaction_free(transaction);
+ transaction_free(transaction);
return 0;
fail:
- ref_transaction_free(transaction);
+ transaction_free(transaction);
error("%s", err.buf);
strbuf_release(&err);
return df_conflict ? STORE_REF_ERROR_DF_CONFLICT
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 0ed1ddd..dcd1862 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -582,20 +582,20 @@ static char *update(struct command *cmd, struct
shallow_info *si)
update_shallow_ref(cmd, si))
return xstrdup("shallow error");
- transaction = ref_transaction_begin(&err);
+ transaction = transaction_begin(&err);
if (!transaction ||
- ref_transaction
[PATCH v3 13/14] refs.c: make unlock_ref/close_ref/commit_ref static
unlock|close|commit_ref can be made static since there are no more external
callers.
Signed-off-by: Ronnie Sahlberg
---
refs.c | 24
refs.h | 9 -
2 files changed, 12 insertions(+), 21 deletions(-)
diff --git a/refs.c b/refs.c
index 9653a01..ff98682 100644
--- a/refs.c
+++ b/refs.c
@@ -1960,6 +1960,16 @@ int refname_match(const char *abbrev_name, const char
*full_name)
return 0;
}
+static void unlock_ref(struct ref_lock *lock)
+{
+ /* Do not free lock->lk -- atexit() still looks at them */
+ if (lock->lk)
+ rollback_lock_file(lock->lk);
+ free(lock->ref_name);
+ free(lock->orig_ref_name);
+ free(lock);
+}
+
/* This function should make sure errno is meaningful on error */
static struct ref_lock *verify_lock(struct ref_lock *lock,
const unsigned char *old_sha1, int mustexist)
@@ -2769,7 +2779,7 @@ int rename_ref(const char *oldrefname, const char
*newrefname, const char *logms
return 1;
}
-int close_ref(struct ref_lock *lock)
+static int close_ref(struct ref_lock *lock)
{
if (close_lock_file(lock->lk))
return -1;
@@ -2777,7 +2787,7 @@ int close_ref(struct ref_lock *lock)
return 0;
}
-int commit_ref(struct ref_lock *lock)
+static int commit_ref(struct ref_lock *lock)
{
if (commit_lock_file(lock->lk))
return -1;
@@ -2785,16 +2795,6 @@ int commit_ref(struct ref_lock *lock)
return 0;
}
-void unlock_ref(struct ref_lock *lock)
-{
- /* Do not free lock->lk -- atexit() still looks at them */
- if (lock->lk)
- rollback_lock_file(lock->lk);
- free(lock->ref_name);
- free(lock->orig_ref_name);
- free(lock);
-}
-
/*
* copy the reflog message msg to buf, which has been allocated sufficiently
* large, while cleaning up the whitespaces. Especially, convert LF to space,
diff --git a/refs.h b/refs.h
index e7892fc..5054388 100644
--- a/refs.h
+++ b/refs.h
@@ -194,15 +194,6 @@ extern struct ref_lock *lock_any_ref_for_update(const char
*refname,
const unsigned char *old_sha1,
int flags, int *type_p);
-/** Close the file descriptor owned by a lock and return the status */
-extern int close_ref(struct ref_lock *lock);
-
-/** Close and commit the ref locked by the lock */
-extern int commit_ref(struct ref_lock *lock);
-
-/** Release any lock taken but not written. **/
-extern void unlock_ref(struct ref_lock *lock);
-
/** Reads log for the value of ref during at_time. **/
extern int read_ref_at(const char *refname, unsigned long at_time, int cnt,
unsigned char *sha1, char **msg,
--
2.0.0.467.g08c0633
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 04/14] refs.c: add a new update_type field to ref_update
Add a field that describes what type of update this refers to. For now
the only type is UPDATE_SHA1 but we will soon add more types.
Signed-off-by: Ronnie Sahlberg
---
refs.c | 25 +
1 file changed, 21 insertions(+), 4 deletions(-)
diff --git a/refs.c b/refs.c
index 4e3d4c3..4129de6 100644
--- a/refs.c
+++ b/refs.c
@@ -3374,6 +3374,10 @@ int for_each_reflog(each_ref_fn fn, void *cb_data)
return retval;
}
+enum transaction_update_type {
+ UPDATE_SHA1 = 0,
+};
+
/**
* Information needed for a single ref update. Set new_sha1 to the
* new value or to zero to delete the ref. To check the old value
@@ -3381,6 +3385,7 @@ int for_each_reflog(each_ref_fn fn, void *cb_data)
* value or to zero to ensure the ref does not exist before update.
*/
struct ref_update {
+ enum transaction_update_type update_type;
unsigned char new_sha1[20];
unsigned char old_sha1[20];
int flags; /* REF_NODEREF? */
@@ -3444,12 +3449,14 @@ void transaction_free(struct ref_transaction
*transaction)
}
static struct ref_update *add_update(struct ref_transaction *transaction,
-const char *refname)
+const char *refname,
+enum transaction_update_type update_type)
{
size_t len = strlen(refname);
struct ref_update *update = xcalloc(1, sizeof(*update) + len + 1);
strcpy((char *)update->refname, refname);
+ update->update_type = update_type;
ALLOC_GROW(transaction->updates, transaction->nr + 1,
transaction->alloc);
transaction->updates[transaction->nr++] = update;
return update;
@@ -3470,7 +3477,7 @@ int transaction_update_sha1(struct ref_transaction
*transaction,
if (have_old && !old_sha1)
die("BUG: have_old is true but old_sha1 is NULL");
- update = add_update(transaction, refname);
+ update = add_update(transaction, refname, UPDATE_SHA1);
hashcpy(update->new_sha1, new_sha1);
update->flags = flags;
update->have_old = have_old;
@@ -3555,7 +3562,10 @@ static int ref_update_reject_duplicates(struct
ref_update **updates, int n,
struct strbuf *err)
{
int i;
- for (i = 1; i < n; i++)
+ for (i = 1; i < n; i++) {
+ if (updates[i - 1]->update_type != UPDATE_SHA1 ||
+ updates[i]->update_type != UPDATE_SHA1)
+ continue;
if (!strcmp(updates[i - 1]->refname, updates[i]->refname)) {
const char *str =
"Multiple updates for ref '%s' not allowed.";
@@ -3564,6 +3574,7 @@ static int ref_update_reject_duplicates(struct ref_update
**updates, int n,
return 1;
}
+ }
return 0;
}
@@ -3593,10 +3604,12 @@ int transaction_commit(struct ref_transaction
*transaction,
goto cleanup;
}
- /* Acquire all locks while verifying old values */
+ /* Acquire all ref locks while verifying old values */
for (i = 0; i < n; i++) {
struct ref_update *update = updates[i];
+ if (update->update_type != UPDATE_SHA1)
+ continue;
update->lock = lock_ref_sha1_basic(update->refname,
(update->have_old ?
update->old_sha1 :
@@ -3619,6 +3632,8 @@ int transaction_commit(struct ref_transaction
*transaction,
for (i = 0; i < n; i++) {
struct ref_update *update = updates[i];
+ if (update->update_type != UPDATE_SHA1)
+ continue;
if (!is_null_sha1(update->new_sha1)) {
ret = write_ref_sha1(update->lock, update->new_sha1,
update->msg);
@@ -3638,6 +3653,8 @@ int transaction_commit(struct ref_transaction
*transaction,
for (i = 0; i < n; i++) {
struct ref_update *update = updates[i];
+ if (update->update_type != UPDATE_SHA1)
+ continue;
if (update->lock) {
if (delete_ref_loose(update->lock, update->type, err))
ret = -1;
--
2.0.0.467.g08c0633
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 11/14] reflog.c: use a reflog transaction when writing during expire
Use a transaction for all updates during expire_reflog.
Signed-off-by: Ronnie Sahlberg
---
builtin/reflog.c | 84
refs.c | 4 +--
refs.h | 2 +-
3 files changed, 39 insertions(+), 51 deletions(-)
diff --git a/builtin/reflog.c b/builtin/reflog.c
index e8a8fb1..f11fee3 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -32,8 +32,11 @@ struct cmd_reflog_expire_cb {
int recno;
};
+static struct strbuf err = STRBUF_INIT;
+
struct expire_reflog_cb {
- FILE *newlog;
+ struct ref_transaction *t;
+ const char *refname;
enum {
UE_NORMAL,
UE_ALWAYS,
@@ -316,20 +319,18 @@ static int expire_reflog_ent(unsigned char *osha1,
unsigned char *nsha1,
if (cb->cmd->recno && --(cb->cmd->recno) == 0)
goto prune;
- if (cb->newlog) {
- char sign = (tz < 0) ? '-' : '+';
- int zone = (tz < 0) ? (-tz) : tz;
- fprintf(cb->newlog, "%s %s %s %lu %c%04d\t%s",
- sha1_to_hex(osha1), sha1_to_hex(nsha1),
- email, timestamp, sign, zone,
- message);
+ if (cb->t) {
+ if (transaction_update_reflog(cb->t, cb->refname, nsha1, osha1,
+ email, timestamp, tz, message, 0,
+ &err))
+ return -1;
hashcpy(cb->last_kept_sha1, nsha1);
}
if (cb->cmd->verbose)
printf("keep %s", message);
return 0;
prune:
- if (!cb->newlog)
+ if (!cb->t)
printf("would prune %s", message);
else if (cb->cmd->verbose)
printf("prune %s", message);
@@ -353,29 +354,26 @@ static int expire_reflog(const char *ref, const unsigned
char *sha1, int unused,
{
struct cmd_reflog_expire_cb *cmd = cb_data;
struct expire_reflog_cb cb;
- struct ref_lock *lock;
- char *log_file, *newlog_path = NULL;
struct commit *tip_commit;
struct commit_list *tips;
int status = 0;
memset(&cb, 0, sizeof(cb));
+ cb.refname = ref;
- /*
-* we take the lock for the ref itself to prevent it from
-* getting updated.
-*/
- lock = lock_any_ref_for_update(ref, sha1, 0, NULL);
- if (!lock)
- return error("cannot lock ref '%s'", ref);
- log_file = git_pathdup("logs/%s", ref);
if (!reflog_exists(ref))
goto finish;
- if (!cmd->dry_run) {
- newlog_path = git_pathdup("logs/%s.lock", ref);
- cb.newlog = fopen(newlog_path, "w");
+ cb.t = transaction_begin(&err);
+ if (!cb.t) {
+ status |= error("%s", err.buf);
+ goto cleanup;
+ }
+ if (transaction_update_reflog(cb.t, cb.refname, null_sha1, null_sha1,
+ NULL, 0, 0, NULL, REFLOG_TRUNCATE,
+ &err)) {
+ status |= error("%s", err.buf);
+ goto cleanup;
}
-
cb.cmd = cmd;
if (!cmd->expire_unreachable || !strcmp(ref, "HEAD")) {
@@ -407,7 +405,10 @@ static int expire_reflog(const char *ref, const unsigned
char *sha1, int unused,
mark_reachable(&cb);
}
- for_each_reflog_ent(ref, expire_reflog_ent, &cb);
+ if (for_each_reflog_ent(ref, expire_reflog_ent, &cb)) {
+ status |= error("%s", err.buf);
+ goto cleanup;
+ }
if (cb.unreachable_expire_kind != UE_ALWAYS) {
if (cb.unreachable_expire_kind == UE_HEAD) {
@@ -420,32 +421,19 @@ static int expire_reflog(const char *ref, const unsigned
char *sha1, int unused,
}
}
finish:
- if (cb.newlog) {
- if (fclose(cb.newlog)) {
- status |= error("%s: %s", strerror(errno),
- newlog_path);
- unlink(newlog_path);
- } else if (cmd->updateref &&
- (write_in_full(lock->lock_fd,
- sha1_to_hex(cb.last_kept_sha1), 40) != 40 ||
-write_str_in_full(lock->lock_fd, "\n") != 1 ||
-close_ref(lock) < 0)) {
- status |= error("Couldn't write %s",
- lock->lk->filename);
- unlink(newlog_path);
- } else if (rename(newlog_path, log_file)) {
- status |= error("cannot rename %s to %s",
- newlog_path, log_file);
- unlink(newlog_path);
- } else if (cmd->updateref && commit_ref(lock)) {
- status |= error("Couldn't set %s", lock->ref_name);
-
[PATCH v3 00/14] ref-transactions-reflog
These patches can also be found at: https://github.com/rsahlberg/git/tree/ref-transactions-reflog This series is based on, and applies ontop of, the previous 48 patch long ref-transaction series that is now in origin/pu. This series introduces support for reflog updates to the transaction framework and ends up re-factoring reflog.c to use a single atomic transaction for updating both the ref and its reflog. With these changes we also reduce the number of places where we build and write a reflog entry to a single function which makes maintenance easier. Several functions that act on reflogs can now be made private to refs.c since we no longer need to export them. This is version 3: - Update and rebased ontop of the current ref-transactions series. Ronnie Sahlberg (14): refs.c make ref_transaction_create a wrapper to ref_transaction_update refs.c: make ref_transaction_delete a wrapper for ref_transaction_update refs.c: rename the transaction functions refs.c: add a new update_type field to ref_update refs.c: add a function to append a reflog entry to a fd lockfile.c: make hold_lock_file_for_append preserve meaningful errno refs.c: add a transaction function to append a reflog entry refs.c: add a flag to allow reflog updates to truncate the log refs.c: only write reflog update if msg is non-NULL refs.c: allow multiple reflog updates during a single transaction reflog.c: use a reflog transaction when writing during expire refs.c: rename log_ref_setup to create_reflog refs.c: make unlock_ref/close_ref/commit_ref static refs.c: remove lock_any_ref_for_update branch.c | 11 +- builtin/checkout.c | 8 +- builtin/commit.c | 14 +- builtin/fetch.c| 12 +- builtin/receive-pack.c | 14 +- builtin/reflog.c | 84 +-- builtin/replace.c | 10 +- builtin/tag.c | 10 +- builtin/update-ref.c | 22 +-- copy.c | 20 ++- fast-import.c | 22 +-- lockfile.c | 7 +- refs.c | 377 ++--- refs.h | 109 +++--- sequencer.c| 12 +- walker.c | 16 +-- 16 files changed, 448 insertions(+), 300 deletions(-) -- 2.0.0.467.g08c0633 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 07/14] refs.c: add a transaction function to append a reflog entry
Define a new transaction update type, UPDATE_LOG, and a new function
transaction_update_reflog. This function will lock the reflog and append
an entry to it during transaction commit.
Signed-off-by: Ronnie Sahlberg
---
refs.c | 101 -
refs.h | 12
2 files changed, 112 insertions(+), 1 deletion(-)
diff --git a/refs.c b/refs.c
index f203285..d673a0f 100644
--- a/refs.c
+++ b/refs.c
@@ -3388,6 +3388,7 @@ int for_each_reflog(each_ref_fn fn, void *cb_data)
enum transaction_update_type {
UPDATE_SHA1 = 0,
+ UPDATE_LOG = 1,
};
/**
@@ -3405,6 +3406,12 @@ struct ref_update {
struct ref_lock *lock;
int type;
char *msg;
+
+ /* used by reflog updates */
+ int reflog_fd;
+ struct lock_file reflog_lock;
+ char *committer;
+
const char refname[FLEX_ARRAY];
};
@@ -3454,6 +3461,7 @@ void transaction_free(struct ref_transaction *transaction)
for (i = 0; i < transaction->nr; i++) {
free(transaction->updates[i]->msg);
+ free(transaction->updates[i]->committer);
free(transaction->updates[i]);
}
free(transaction->updates);
@@ -3474,6 +3482,42 @@ static struct ref_update *add_update(struct
ref_transaction *transaction,
return update;
}
+int transaction_update_reflog(struct ref_transaction *transaction,
+ const char *refname,
+ const unsigned char *new_sha1,
+ const unsigned char *old_sha1,
+ const unsigned char *email,
+ unsigned long timestamp, int tz,
+ const char *msg, int flags,
+ struct strbuf *err)
+{
+ struct ref_update *update;
+
+ if (transaction->state != REF_TRANSACTION_OPEN)
+ die("BUG: update_reflog called for transaction that is not "
+ "open");
+
+ update = add_update(transaction, refname, UPDATE_LOG);
+ hashcpy(update->new_sha1, new_sha1);
+ hashcpy(update->old_sha1, old_sha1);
+ update->reflog_fd = -1;
+ if (email) {
+ struct strbuf buf = STRBUF_INIT;
+ char sign = (tz < 0) ? '-' : '+';
+ int zone = (tz < 0) ? (-tz) : tz;
+
+ strbuf_addf(&buf, "%s %lu %c%04d", email, timestamp, sign,
+ zone);
+ update->committer = xstrdup(buf.buf);
+ strbuf_release(&buf);
+ }
+ if (msg)
+ update->msg = xstrdup(msg);
+ update->flags = flags;
+
+ return 0;
+}
+
int transaction_update_sha1(struct ref_transaction *transaction,
const char *refname,
const unsigned char *new_sha1,
@@ -3640,7 +3684,28 @@ int transaction_commit(struct ref_transaction
*transaction,
}
}
- /* Perform updates first so live commits remain referenced */
+ /* Lock all reflog files */
+ for (i = 0; i < n; i++) {
+ struct ref_update *update = updates[i];
+
+ if (update->update_type != UPDATE_LOG)
+ continue;
+ update->reflog_fd = hold_lock_file_for_append(
+ &update->reflog_lock,
+ git_path("logs/%s", update->refname),
+ 0);
+ if (update->reflog_fd < 0) {
+ const char *str = "Cannot lock reflog for '%s'. %s";
+
+ ret = -1;
+ if (err)
+ strbuf_addf(err, str, update->refname,
+ strerror(errno));
+ goto cleanup;
+ }
+ }
+
+ /* Perform ref updates first so live commits remain referenced */
for (i = 0; i < n; i++) {
struct ref_update *update = updates[i];
@@ -3676,6 +3741,40 @@ int transaction_commit(struct ref_transaction
*transaction,
}
}
+ /* Update all reflog files */
+ for (i = 0; i < n; i++) {
+ struct ref_update *update = updates[i];
+
+ if (update->update_type != UPDATE_LOG)
+ continue;
+ if (update->reflog_fd == -1)
+ continue;
+
+ if (log_ref_write_fd(update->reflog_fd, update->old_sha1,
+update->new_sha1,
+update->committer, update->msg)) {
+ error("Could write to reflog: %s. %s",
+ update->refname, strerror(errno));
+ rollback_lock_file(&update->reflog_lock);
+ update->reflog_fd = -1;
+ }
+
[PATCH v3 01/14] refs.c make ref_transaction_create a wrapper to ref_transaction_update
Signed-off-by: Ronnie Sahlberg
---
refs.c | 13 ++---
refs.h | 7 ---
2 files changed, 6 insertions(+), 14 deletions(-)
diff --git a/refs.c b/refs.c
index dfbf003..a9f91ab 100644
--- a/refs.c
+++ b/refs.c
@@ -3487,23 +3487,14 @@ int ref_transaction_create(struct ref_transaction
*transaction,
int flags, const char *msg,
struct strbuf *err)
{
- struct ref_update *update;
-
if (transaction->state != REF_TRANSACTION_OPEN)
die("BUG: create called for transaction that is not open");
if (!new_sha1 || is_null_sha1(new_sha1))
die("BUG: create ref with null new_sha1");
- update = add_update(transaction, refname);
-
- hashcpy(update->new_sha1, new_sha1);
- hashclr(update->old_sha1);
- update->flags = flags;
- update->have_old = 1;
- if (msg)
- update->msg = xstrdup(msg);
- return 0;
+ return ref_transaction_update(transaction, refname, new_sha1,
+ null_sha1, flags, 1, msg, err);
}
int ref_transaction_delete(struct ref_transaction *transaction,
diff --git a/refs.h b/refs.h
index db463d0..495740d 100644
--- a/refs.h
+++ b/refs.h
@@ -283,9 +283,10 @@ struct ref_transaction *ref_transaction_begin(struct
strbuf *err);
/*
* Add a reference update to transaction. new_sha1 is the value that
* the reference should have after the update, or zeros if it should
- * be deleted. If have_old is true, then old_sha1 holds the value
- * that the reference should have had before the update, or zeros if
- * it must not have existed beforehand.
+ * be deleted. If have_old is true and old_sha is not the null_sha1
+ * then the previous value of the ref must match or the update will fail.
+ * If have_old is true and old_sha1 is the null_sha1 then the ref must not
+ * already exist and a new ref will be created with new_sha1.
* Function returns 0 on success and non-zero on failure. A failure to update
* means that the transaction as a whole has failed and will need to be
* rolled back.
--
2.0.0.467.g08c0633
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 06/14] lockfile.c: make hold_lock_file_for_append preserve meaningful errno
Update hold_lock_file_for_append and copy_fd to return a meaningful errno
on failure.
Signed-off-by: Ronnie Sahlberg
---
copy.c | 20 +---
lockfile.c | 7 ++-
2 files changed, 19 insertions(+), 8 deletions(-)
diff --git a/copy.c b/copy.c
index a7f58fd..5cb8679 100644
--- a/copy.c
+++ b/copy.c
@@ -9,10 +9,12 @@ int copy_fd(int ifd, int ofd)
if (!len)
break;
if (len < 0) {
- int read_error = errno;
+ int save_errno = errno;
close(ifd);
- return error("copy-fd: read returned %s",
-strerror(read_error));
+ error("copy-fd: read returned %s",
+ strerror(save_errno));
+ errno = save_errno;
+ return -1;
}
while (len) {
int written = xwrite(ofd, buf, len);
@@ -22,12 +24,16 @@ int copy_fd(int ifd, int ofd)
}
else if (!written) {
close(ifd);
- return error("copy-fd: write returned 0");
+ error("copy-fd: write returned 0");
+ errno = EAGAIN;
+ return -1;
} else {
- int write_error = errno;
+ int save_errno = errno;
close(ifd);
- return error("copy-fd: write returned %s",
-strerror(write_error));
+ error("copy-fd: write returned %s",
+ strerror(save_errno));
+ errno = save_errno;
+ return -1;
}
}
}
diff --git a/lockfile.c b/lockfile.c
index a921d77..32f4681 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -217,15 +217,20 @@ int hold_lock_file_for_append(struct lock_file *lk, const
char *path, int flags)
orig_fd = open(path, O_RDONLY);
if (orig_fd < 0) {
if (errno != ENOENT) {
+ int save_errno = errno;
if (flags & LOCK_DIE_ON_ERROR)
die("cannot open '%s' for copying", path);
close(fd);
- return error("cannot open '%s' for copying", path);
+ error("cannot open '%s' for copying", path);
+ errno = save_errno;
+ return -1;
}
} else if (copy_fd(orig_fd, fd)) {
+ int save_errno = errno;
if (flags & LOCK_DIE_ON_ERROR)
exit(128);
close(fd);
+ errno = save_errno;
return -1;
}
return fd;
--
2.0.0.467.g08c0633
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: Why does gpg.program work for commit but not log?
-- -=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=- - - - Jason Pyeron PD Inc. http://www.pdinc.us - - Principal Consultant 10 West 24th Street #100- - +1 (443) 269-1555 x333Baltimore, Maryland 21218 - - - -=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=- This message is copyright PD Inc, subject to license 20080407P00. > -Original Message- > From: git-ow...@vger.kernel.org > [mailto:git-ow...@vger.kernel.org] On Behalf Of Jeff King > Sent: Wednesday, June 18, 2014 3:37 > To: Jason Pyeron > Cc: git@vger.kernel.org > Subject: Re: Why does gpg.program work for commit but not log? > > On Wed, Jun 18, 2014 at 12:18:35AM -0400, Jason Pyeron wrote: > > > jpyeron@black /projects/microsoft-smartcard-sign/tmp > > $ git --version > > git version 1.7.9 > > That's rather old. In the meantime we have: > > commit 6005dbb9bf21d10b209f7924e305bd04b9ab56d2 > Author: Jacob Sarvis > Date: Wed Mar 27 10:13:39 2013 -0500 > > log: read gpg settings for signed commit verification > > "show --show-signature" and "log --show-signature" > do not read the > gpg.program setting from git config, even though, > commit signing, > tag signing, and tag verification honor it. > > which is in v1.8.3. In general, please double-check your problem > against a recent version of "master" when making a bug report. Ok, so now I will be using locally compiled git. jpyeron@black /projects/microsoft-smartcard-sign/tmp $ /projects/git/local-build/bin/git --version git version 1.8.4.21.g992c386 jpyeron@black /projects/microsoft-smartcard-sign/tmp $ GIT_TRACE=1 /projects/git/local-build/bin/git log --show-signature trace: built-in: git 'log' '--show-signature' trace: run_command: 'less' trace: exec: 'less' trace: run_command: '/projects/microsoft-smartcard-sign/tmp/bin/logginggpg.sh' '--status-fd=1' '--verify' '/tmp/.git_vtag_tmp66WxpM' '-' commit 38afa1f4d0c73fd47d5788310a1a2080aa0abbba -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 2/5] refs.c: return error instead of dying when locking fails during transaction
Change lock_ref_sha1_basic to return an error instead of dying when we fail to lock a file during a transaction. Signed-off-by: Ronnie Sahlberg --- refs.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/refs.c b/refs.c index 11dcb07..a644e7c 100644 --- a/refs.c +++ b/refs.c @@ -2195,7 +2195,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname, */ goto retry; else - unable_to_lock_index_die(ref_file, errno); + goto error_return; } return old_sha1 ? verify_lock(lock, old_sha1, mustexist) : lock; -- 2.0.0.467.g08c0633 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 4/5] refs.c: update rename_ref to use a transaction
Change refs.c to use a single transaction to copy/rename both the refs and
its reflog. Since we are no longer using rename() to move the reflog file
we no longer need to disallow rename_ref for refs with a symlink for its
reflog so we can remove that test from the testsuite.
Change the function to return 1 on failure instead of either -1 or 1.
Signed-off-by: Ronnie Sahlberg
---
refs.c| 192 ++
t/t3200-branch.sh | 7 --
2 files changed, 65 insertions(+), 134 deletions(-)
diff --git a/refs.c b/refs.c
index 63c2122..89e5bc0 100644
--- a/refs.c
+++ b/refs.c
@@ -2616,81 +2616,42 @@ int delete_ref(const char *refname, const unsigned char
*sha1, int delopt)
return 0;
}
-/*
- * People using contrib's git-new-workdir have .git/logs/refs ->
- * /some/other/path/.git/logs/refs, and that may live on another device.
- *
- * IOW, to avoid cross device rename errors, the temporary renamed log must
- * live into logs/refs.
- */
-#define TMP_RENAMED_LOG "logs/refs/.tmp-renamed-log"
+struct rename_reflog_cb {
+ struct ref_transaction *transaction;
+ const char *refname;
+ struct strbuf *err;
+};
-static int rename_tmp_log(const char *newrefname)
+static int rename_reflog_ent(unsigned char *osha1, unsigned char *nsha1,
+const char *email, unsigned long timestamp, int tz,
+const char *message, void *cb_data)
{
- int attempts_remaining = 4;
-
- retry:
- switch (safe_create_leading_directories(git_path("logs/%s",
newrefname))) {
- case SCLD_OK:
- break; /* success */
- case SCLD_VANISHED:
- if (--attempts_remaining > 0)
- goto retry;
- /* fall through */
- default:
- error("unable to create directory for %s", newrefname);
- return -1;
- }
+ struct rename_reflog_cb *cb = cb_data;
- if (rename(git_path(TMP_RENAMED_LOG), git_path("logs/%s", newrefname)))
{
- if ((errno==EISDIR || errno==ENOTDIR) && --attempts_remaining >
0) {
- /*
-* rename(a, b) when b is an existing
-* directory ought to result in ISDIR, but
-* Solaris 5.8 gives ENOTDIR. Sheesh.
-*/
- if (remove_empty_directories(git_path("logs/%s",
newrefname))) {
- error("Directory not empty: logs/%s",
newrefname);
- return -1;
- }
- goto retry;
- } else if (errno == ENOENT && --attempts_remaining > 0) {
- /*
-* Maybe another process just deleted one of
-* the directories in the path to newrefname.
-* Try again from the beginning.
-*/
- goto retry;
- } else {
- error("unable to move logfile "TMP_RENAMED_LOG" to
logs/%s: %s",
- newrefname, strerror(errno));
- return -1;
- }
- }
- return 0;
+ return transaction_update_reflog(cb->transaction, cb->refname,
+nsha1, osha1, email, timestamp, tz,
+message, 0, cb->err);
}
-static int write_ref_sha1(struct ref_lock *lock, const unsigned char *sha1,
- const char *logmsg);
-
int rename_ref(const char *oldrefname, const char *newrefname, const char
*logmsg)
{
- unsigned char sha1[20], orig_sha1[20];
- int flag = 0, logmoved = 0;
- struct ref_lock *lock;
- struct stat loginfo;
- int log = !lstat(git_path("logs/%s", oldrefname), &loginfo);
+ unsigned char sha1[20];
+ int flag = 0, log;
+ struct ref_transaction *transaction = NULL;
+ struct strbuf err = STRBUF_INIT;
const char *symref = NULL;
+ struct rename_reflog_cb cb;
- if (log && S_ISLNK(loginfo.st_mode))
- return error("reflog for %s is a symlink", oldrefname);
-
- symref = resolve_ref_unsafe(oldrefname, orig_sha1, 1, &flag);
- if (flag & REF_ISSYMREF)
- return error("refname %s is a symbolic ref, renaming it is not
supported",
+ symref = resolve_ref_unsafe(oldrefname, sha1, 1, &flag);
+ if (flag & REF_ISSYMREF) {
+ error("refname %s is a symbolic ref, renaming it is not
supported",
oldrefname);
- if (!symref)
- return error("refname %s not found", oldrefname);
+ return 1;
+ }
+ if (!symref) {
+ error("refname %s not found", oldrefname);
+ return 1;
+ }
if (!is_refname_available(newrefname, get_packe
[PATCH v3 3/5] refs.c: use packed refs when deleting refs during a transaction
Make the deletion of refs during a transaction more atomic.
Start by first copying all loose refs we will be deleting to the packed
refs file and then commit the packed refs file. Then re-lock the packed refs
file to avoid anyone else from modifying the refs we are to delete during
this transaction and keep it locked until we are finished.
Since all refs we are about to delete are now safely held in the packed refs
file we can proceed to immediately unlink any loose refs for those refs
and still be fully rollback-able.
The exception is for refs that can not be resolved. Those refs are never
added to the packed refs and will just be un-rollback-ably deleted during
commit.
By deleting all the loose refs at the start of the transaction we make make
it possible to both delete one ref and then re-create a different ref in
the same transaction, even if both the old-to-be-deleted and the
new-to-be-created refs would otherwise conflict in the file-system.
Example: rename m->m/m
In that example we want to delete the file 'm' so that we make room so
that we can create a directory with the same name in order to lock and
write to the ref m/m and its lock-file m/m.lock.
If there is a failure during the commit phase we can rollback without losing
any refs since we have so far only deleted the loose refs but we still have
the refs stored in the packed refs file.
Once we have finished all updates for refs and their reflogs we can repack
the packed refs file and remove the to-be-deleted refs from the packed refs,
at which point all the deleted refs will disappear in one atomic rename
operation.
Signed-off-by: Ronnie Sahlberg
---
builtin/remote.c | 13 +++--
refs.c | 150 +++
2 files changed, 127 insertions(+), 36 deletions(-)
diff --git a/builtin/remote.c b/builtin/remote.c
index 401feb3..beb6e34 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -755,6 +755,8 @@ static int remove_branches(struct string_list *branches)
branch_names = xmalloc(branches->nr * sizeof(*branch_names));
for (i = 0; i < branches->nr; i++)
branch_names[i] = branches->items[i].string;
+ if (lock_packed_refs(0))
+ result |= unable_to_lock_error(git_path("packed-refs"), errno);
result |= repack_without_refs(branch_names, branches->nr, NULL);
free(branch_names);
@@ -1332,9 +1334,14 @@ static int prune_remote(const char *remote, int dry_run)
delete_refs = xmalloc(states.stale.nr * sizeof(*delete_refs));
for (i = 0; i < states.stale.nr; i++)
delete_refs[i] = states.stale.items[i].util;
- if (!dry_run)
- result |= repack_without_refs(delete_refs,
- states.stale.nr, NULL);
+ if (!dry_run) {
+ if (lock_packed_refs(0))
+ result |= unable_to_lock_error(
+ git_path("packed-refs"), errno);
+ else
+ result |= repack_without_refs(delete_refs,
+ states.stale.nr, NULL);
+ }
free(delete_refs);
}
diff --git a/refs.c b/refs.c
index a644e7c..63c2122 100644
--- a/refs.c
+++ b/refs.c
@@ -1322,7 +1322,7 @@ static struct ref_entry *get_packed_ref(const char
*refname)
/*
* A loose ref file doesn't exist; check for a packed ref. The
- * options are forwarded from resolve_safe_unsafe().
+ * options are forwarded from resolve_ref_unsafe().
*/
static const char *handle_missing_loose_ref(const char *refname,
unsigned char *sha1,
@@ -1379,7 +1379,6 @@ const char *resolve_ref_unsafe(const char *refname,
unsigned char *sha1, int rea
}
git_snpath(path, sizeof(path), "%s", refname);
-
/*
* We might have to loop back here to avoid a race
* condition: first we lstat() the file, then we try
@@ -2510,6 +2509,9 @@ static int curate_packed_ref_fn(struct ref_entry *entry,
void *cb_data)
return 0;
}
+/*
+ * Must be called with packed refs already locked (and sorted)
+ */
int repack_without_refs(const char **refnames, int n, struct strbuf *err)
{
struct ref_dir *packed;
@@ -2522,19 +2524,12 @@ int repack_without_refs(const char **refnames, int n,
struct strbuf *err)
if (get_packed_ref(refnames[i]))
break;
- /* Avoid locking if we have nothing to do */
- if (i == n)
+ /* Avoid processing if we have nothing to do */
+ if (i == n) {
+ rollback_packed_refs();
return 0; /* no refname exists in packed refs */
-
- if (lock_packed_refs(0)) {
- if (err) {
- unable_to_lock_mess
[PATCH v3 0/5] ref-transactions-rename
These patches can also be found at: https://github.com/rsahlberg/git/tree/ref-transactions-rename This series is based on, and applies ontop of, the previous ref-transactions-reflog series, also found at my githup repo. This series updates the reflog handling and converts rename_ref to use a single transaction to delete the old ref, create the new ref and to move the reflog. In order to make the transaction fully atomic we also introduce support for using the packed refs file for performing the delete-and-create that is done during the rename. This means that the rename operation is fully atomic for all external observers and there is no longer a point in time where the sha1 is unreferenced, after the old ref has been deleted but before the new ref has been created. Additionally, since we now use the transaction API for the reflog changes we no longer need to disallow renames for refs whose reflogs are symbolic links. Version 3: - Updated to build ontop of current ref-transactions-reflog Ronnie Sahlberg (5): refs.c: allow passing raw git_committer_info as email to _update_reflog refs.c: return error instead of dying when locking fails during transaction refs.c: use packed refs when deleting refs during a transaction refs.c: update rename_ref to use a transaction refs.c: rollback the lockfile before we die() in repack_without_refs builtin/remote.c | 13 +- refs.c| 368 +- refs.h| 1 + t/t3200-branch.sh | 7 -- 4 files changed, 209 insertions(+), 180 deletions(-) -- 2.0.0.467.g08c0633 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 1/5] refs.c: allow passing raw git_committer_info as email to _update_reflog
In many places in the code we do not have access to the individual fields
in the committer data. Instead we might only have access to prebaked data
such as what is returned by git_committer_info() containing a string
that consists of email, timestamp, zone etc.
This makes it inconvenient to use transaction_update_reflog since it means
you would have to first parse git_committer_info before you can call
update_reflog.
Add a new flag REFLOG_EMAIL_IS_COMMITTER to _update_reflog to tell it
that what we pass in as email is already the fully baked committer string
we can use as-is.
Signed-off-by: Ronnie Sahlberg
---
refs.c | 20
refs.h | 1 +
2 files changed, 13 insertions(+), 8 deletions(-)
diff --git a/refs.c b/refs.c
index 95c3eb8..11dcb07 100644
--- a/refs.c
+++ b/refs.c
@@ -3521,14 +3521,18 @@ int transaction_update_reflog(struct ref_transaction
*transaction,
hashcpy(update->old_sha1, old_sha1);
update->reflog_fd = -1;
if (email) {
- struct strbuf buf = STRBUF_INIT;
- char sign = (tz < 0) ? '-' : '+';
- int zone = (tz < 0) ? (-tz) : tz;
-
- strbuf_addf(&buf, "%s %lu %c%04d", email, timestamp, sign,
- zone);
- update->committer = xstrdup(buf.buf);
- strbuf_release(&buf);
+ if (flags & REFLOG_EMAIL_IS_COMMITTER)
+ update->committer = xstrdup(email);
+ else {
+ struct strbuf buf = STRBUF_INIT;
+ char sign = (tz < 0) ? '-' : '+';
+ int zone = (tz < 0) ? (-tz) : tz;
+
+ strbuf_addf(&buf, "%s %lu %c%04d", email, timestamp,
+ sign, zone);
+ update->committer = xstrdup(buf.buf);
+ strbuf_release(&buf);
+ }
}
if (msg)
update->msg = xstrdup(msg);
diff --git a/refs.h b/refs.h
index b674c20..469d27f 100644
--- a/refs.h
+++ b/refs.h
@@ -315,6 +315,7 @@ int transaction_delete_sha1(struct ref_transaction
*transaction,
* Flags >= 0x100 are reserved for internal use.
*/
#define REFLOG_TRUNCATE 0x01
+#define REFLOG_EMAIL_IS_COMMITTER 0x02
/*
* Append a reflog entry for refname. If the REFLOG_TRUNCATE flag is set
* this update will first truncate the reflog before writing the entry.
--
2.0.0.467.g08c0633
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Possible bug in `gitreset` in 1.9
Followup on this, it looks like the local repository actually didn't contain branch-2. So this doesn't appear to be an issue. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
What's cooking in git.git (Jun 2014, #04; Tue, 17)
Here are the topics that have been cooking. Commits prefixed with '-' are only in 'pu' (proposed updates) while commits prefixed with '+' are in 'next'. Many topics that have been cooking in 'next' during the previous cycle, totalling close to 300 individual patches, are in 'master' now. We have also accumulated some fixes we need to merge down to 'maint' and cut a v2.0.1 sometime next week. You can find the changes described here in the integration branches of the repositories listed at http://git-blame.blogspot.com/p/git-public-repositories.html -- [Graduated to "master"] * mt/patch-id-stable (2014-06-10) 1 commit - patch-id: change default to stable Teaches "git patch-id" to compute the patch ID that does not change when the files in a single patch is reordered. As this new algorithm is backward incompatible, the last bit to flip it to be the default is left out of 'master' for now. * as/pretty-truncate (2014-05-21) 5 commits (merged to 'next' on 2014-06-10 at d8147a2) + pretty.c: format string with truncate respects logOutputEncoding + t4205, t6006: add tests that fail with i18n.logOutputEncoding set + t4205 (log-pretty-format): use `tformat` rather than `format` + t4041, t4205, t6006, t7102: don't hardcode tested encoding value + t4205 (log-pretty-formats): don't hardcode SHA-1 in expected outputs Originally merged to 'next' on 2014-05-23 * bg/xcalloc-nmemb-then-size (2014-05-27) 12 commits (merged to 'next' on 2014-06-10 at eddb5bc) + transport-helper.c: rearrange xcalloc arguments + remote.c: rearrange xcalloc arguments + reflog-walk.c: rearrange xcalloc arguments + pack-revindex.c: rearrange xcalloc arguments + notes.c: rearrange xcalloc arguments + imap-send.c: rearrange xcalloc arguments + http-push.c: rearrange xcalloc arguments + diff.c: rearrange xcalloc arguments + config.c: rearrange xcalloc arguments + commit.c: rearrange xcalloc arguments + builtin/remote.c: rearrange xcalloc arguments + builtin/ls-remote.c: rearrange xcalloc arguments Originally merged to 'next' on 2014-06-06 Like calloc(3), xcalloc() takes nmemb and then size. * cb/byte-order (2014-05-30) 3 commits (merged to 'next' on 2014-06-10 at 63db8ee) + compat/bswap.h: fix endianness detection + compat/bswap.h: restore preference __BIG_ENDIAN over BIG_ENDIAN + compat/bswap.h: detect endianness on more platforms that don't use BYTE_ORDER Originally merged to 'next' on 2014-05-30 Compatibility enhancement for Solaris. * cc/replace-edit (2014-05-19) 10 commits (merged to 'next' on 2014-06-10 at ff69722) + Documentation: replace: describe new --edit option + replace: add --edit to usage string + replace: add tests for --edit + replace: die early if replace ref already exists + replace: refactor checking ref validity + replace: make sure --edit results in a different object + replace: add --edit option + replace: factor object resolution out of replace_object + replace: use OPT_CMDMODE to handle modes + replace: refactor command-mode determination (this branch is used by cc/replace-graft.) Originally merged to 'next' on 2014-05-19 "git replace" learns a new "--edit" option. * dt/refs-check-refname-component-optim (2014-06-05) 1 commit (merged to 'next' on 2014-06-10 at 4560669) + refs.c: optimize check_refname_component() (this branch is used by dt/refs-check-refname-component-sse42.) Originally merged to 'next' on 2014-06-06 * fc/remote-helper-refmap (2014-04-21) 8 commits (merged to 'next' on 2014-06-10 at 8cd8cf8) + transport-helper: remove unnecessary strbuf resets + transport-helper: add support to delete branches + fast-export: add support to delete refs + fast-import: add support to delete refs + transport-helper: add support to push symbolic refs + transport-helper: add support for old:new refspec + fast-export: add new --refspec option + fast-export: improve argument parsing Originally merged to 'next' on 2014-04-22 Allow remote-helper/fast-import based transport to rename the refs while transferring the history. * ib/test-selectively-run (2014-06-06) 4 commits (merged to 'next' on 2014-06-10 at 1235570) + t-*.sh: fix the GIT_SKIP_TESTS sub-tests + test-lib: '--run' to run only specific tests + test-lib: tests skipped by GIT_SKIP_TESTS say so + test-lib: document short options in t/README Originally merged to 'next' on 2014-06-06 Allow specifying only certain individual test pieces to be run using a range notation (e.g. "t1234-test.sh --run='1-4 6 8 9-'"). * jk/argv-array-for-child-process (2014-05-15) 7 commits (merged to 'next' on 2014-06-10 at 07a167b) + argv-array: drop "detach" code + get_importer: use run-command's internal argv_array + get_exporter: use argv_array + get_helper: use run-command's internal argv_array + git_connect: use argv_array + run_column_filter: use argv_array + run-command: store an optional argv_array Originally merged t
Re: [PATCH 0/7] Second part of msysgit/unicode
Stepan Kasal writes: > Hello Karsten, > > On Tue, Jun 17, 2014 at 11:06:52AM +0200, Karsten Blees wrote: >> Am 11.06.2014 11:37, schrieb Stepan Kasal: >> > This is the second part of the time-proven unicode suport branch from >> > msysgit. >> > This batch is a collection of small independent changes, limited to >> > mingw.c. >> > The only exception is the last patch: it changes gitk and git-gui. >> >> I'm missing the other two "Unicode file name" patches (and "Win32: fix >> detection > > indeed. I noticed that after sending the plan quoted above. > Luckily, the gitk/git-gui patch was not accepted and has to be > resubmitted. > > So the plan for future submissions is: > > 1) two "Unicode file name" patches (with "fix... is_dir_empty" > squashed) > 2) the environament patches from your unicode branch (several > patches) > 3) "color term" (and env. var. TERM); updated according to your > instructions, thus sent separately after the series > 4) resubmit gitk / git-gui (have separate maintainers) > > This is work in progress, I suppose to mail 1) and 2) in a few days. > > Stepan In the meantime, are Windows folks happy with the four topics queued on 'pu' so far? I would like to start moving them down to 'next' and to 'master' soonish. They consist of these individual patches: $ git shortlog ^master \ sk/mingw-dirent \ sk/mingw-main \ sk/mingw-uni-console \ sk/mingw-unicode-spawn-args Johannes Schindelin (1): Win32: let mingw_execve() return an int Karsten Blees (18): Win32 dirent: remove unused dirent.d_ino member Win32 dirent: remove unused dirent.d_reclen member Win32 dirent: change FILENAME_MAX to MAX_PATH Win32 dirent: clarify #include directives Win32 dirent: improve dirent implementation Win32: move main macro to a function Win32: support Unicode console output Win32: detect console streams more reliably Win32: warn if the console font doesn't support Unicode Win32: add Unicode conversion functions Win32: Thread-safe windows console output Win32: fix broken pipe detection Win32: reliably detect console pipe handles Win32: simplify internal mingw_spawn* APIs Win32: fix potential multi-threading issue MinGW: disable CRT command line globbing Win32: Unicode arguments (outgoing) Win32: Unicode arguments (incoming) Stepan Kasal (1): mingw: avoid const warning Thanks. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 10/11] trace: add trace_performance facility to debug performance issues
Karsten Blees writes:
> Right, it makes no sense for trace_performance(), and for
> trace_performance_since() only if followed by another 'measured' code
> section. In that special case, I think it wouldn't hurt if you had to
> write:
>
> uint64_t start = getnanotime();
> /* first code section to measure */
> trace_performance_since(start, "first foobar");
>
> start = getnanotime();
> /* second code section to measure */
> trace_performance_since(start, "second foobar");
>
> So I guess I'll drop the return value (and the second example, which
> is then redundant to the first).
That also sounds OK to me.
>>> +static void trace_performance_vfl(const char *file, int line,
>>> + uint64_t nanos, const char *format,
>>> + va_list ap)
>>> +{
>>
>> Just being curious, but what does "v" stand for?
>>
>
> trace_performance_vfl(, va_list)
> vs.
> trace_performance_fl(, ...)
>
> Will change to trace_performance_vprintf_fl()
Ah, OK. The name with 'vprintf' in it does sound better.
Thanks.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 5/5] refs.c: rollback the lockfile before we die() in repack_without_refs
Signed-off-by: Ronnie Sahlberg
---
refs.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/refs.c b/refs.c
index 89e5bc0..582591b 100644
--- a/refs.c
+++ b/refs.c
@@ -2548,8 +2548,10 @@ int repack_without_refs(const char **refnames, int n,
struct strbuf *err)
/* Remove any other accumulated cruft */
do_for_each_entry_in_dir(packed, 0, curate_packed_ref_fn,
&refs_to_delete);
for_each_string_list_item(ref_to_delete, &refs_to_delete) {
- if (remove_entry(packed, ref_to_delete->string) == -1)
+ if (remove_entry(packed, ref_to_delete->string) == -1) {
+ rollback_packed_refs();
die("internal error");
+ }
}
/* Write what remains */
--
2.0.0.467.g08c0633
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RFC] git-am: support any number of signatures
Junio C Hamano writes: > On Tue, Jun 17, 2014 at 8:09 PM, Michael S. Tsirkin wrote: >> >> OK, after looking into this for a while, I realize >> this is a special property of the Signed-off-by footer. >> For now I think it's reasonable to just avoid de-duplicating >> other footers if any. Agree? > > Not really. I'd rather see "git am" hardcode as little such policy as > possible. > We do need to support S-o-b footer and the policy we defined for it long time > ago, if only for backward compatiblity, but for any other footers, > policy decision > such as "dedup by default" isn't something "am" should know about. By the way, "append without looking for dups" is a policy decision that is as bad to have as "append with dedup". I'd rather not to see "am.signoff", or any name that implies what the "-s" option to the command is about for that matter, to be used in futzing with the trailers other than S-o-b in any way. As I understand it, our longer term goal is to defer that task, including the user-programmable policy decisions, to something like the 'trailer' Christian started. I suspect that it may add unnecessary work later if we overloaded "signoff" with a similar feature with the change under discussion. I would feel safer to see it outlined how we envision to transition to a more generic 'trailer' solution later if we were to enhance "am" with "am.signoff" now. Thanks. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: cherry-pick lost
guangai@travelzen.com writes: > I delete a file and push to master branch, after code reviewing > in gerrit, then click 'Cherry Pick To' button to cherry-pick to > release/1.1 branch, and then code review and merge... Gerrit folks, for which this list may not be the best place to get in touch with, may know a lot better than anybody on this list what "Cherry Pick To" button does, how it does so, what the typical user errors are, etc. https://code.google.com/p/gerrit/ tells me that perhaps you would have better luck with these places: https://groups.google.com/forum/#!forum/repo-discuss IRC freenode #gerrit -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RFC] git-am: support any number of signatures
On Wed, Jun 18, 2014 at 10:51:04AM -0700, Junio C Hamano wrote: > Junio C Hamano writes: > > > On Tue, Jun 17, 2014 at 8:09 PM, Michael S. Tsirkin wrote: > >> > >> OK, after looking into this for a while, I realize > >> this is a special property of the Signed-off-by footer. > >> For now I think it's reasonable to just avoid de-duplicating > >> other footers if any. Agree? > > > > Not really. I'd rather see "git am" hardcode as little such policy as > > possible. > > We do need to support S-o-b footer and the policy we defined for it long > > time > > ago, if only for backward compatiblity, but for any other footers, > > policy decision > > such as "dedup by default" isn't something "am" should know about. > > By the way, "append without looking for dups" is a policy decision > that is as bad to have as "append with dedup". > > I'd rather not to see "am.signoff", or any name that implies what > the "-s" option to the command is about for that matter, to be used > in futzing with the trailers other than S-o-b in any way. As I > understand it, our longer term goal is to defer that task, including > the user-programmable policy decisions, to something like the > 'trailer' Christian started. > > I suspect that it may add unnecessary work later if we overloaded > "signoff" with a similar feature with the change under discussion. > I would feel safer to see it outlined how we envision to transition > to a more generic 'trailer' solution later if we were to enhance > "am" with "am.signoff" now. > > Thanks. I'll need to look at trailers, if indeed they are a superset of this functionality, I will transition to trailers when they land on master. In this case seems easier for me to keep this out tree patch for now, Good thing I didn't spend time writing docs and tests :) -- MST -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 0/5] cleanup duplicate name_compare() functions
Version 3 of the patch series to cleanup duplicate name_compare() functions (previously was 'add strnncmp() function' [1]). This version goes in a slightly different direction than the previous version. Before I was trying to add a strnncmp() function so I could remove duplicate copies of the name_compare() function in tree-walk.c and unpack-trees.c. But then Torsten Bögershausen pointed out that there is a cache_name_compare() function which is nearly identical to name_compare() [2]*. * cache_name_compare() is not identical to name_compare(). The former returns +1, -1, whereas the latter returns +N, -N. But there is no place where name_compare() was used that needed the magnitude so this change would not alter its behavior. So I decided why not generalize the name of cache_name_compare() by renaming it to name_compare(), since it doesn't do anything with caches, other than being part of cache.h and read-cache.c. Then the duplicate name_compare() functions can be removed and the few places that used cache_name_compare() can be renamed to name_compare(). It cleans up the code with a minimal number of changes. It keeps existing functions instead of creating new ones. And there are several other functions in cache.h that are similarly named '*name_compare' so it follows the already established style. Also, the name_compare() now uses memcmp() as it did originally instead of using strncmp() as it did in the last version. [1]: http://marc.info/?l=git&m=140299051431479&w=2 [2]: http://marc.info/?l=git&m=140300329403706&w=2 Jeremiah Mahler (5): cache: rename cache_name_compare() to name_compare() tree-walk.c: remove name_compare() function unpack-trees.c: remove name_compare() function dir.c: rename to name_compare() name-hash.c: rename to name_compare() cache.h| 2 +- dir.c | 3 +-- name-hash.c| 2 +- read-cache.c | 23 +-- tree-walk.c| 10 -- unpack-trees.c | 11 --- 6 files changed, 16 insertions(+), 35 deletions(-) -- 2.0.0 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 1/5] cache: rename cache_name_compare() to name_compare()
The cache_name_compare() function is not specific to a cache.
Make its name more general by renaming it to name_compare().
Simplify cache_name_stage_compare() via name_compare().
Where lengths are involved, change int to size_t.
Signed-off-by: Jeremiah Mahler
---
cache.h | 2 +-
read-cache.c | 23 +--
2 files changed, 14 insertions(+), 11 deletions(-)
diff --git a/cache.h b/cache.h
index c498a30..e3205fe 100644
--- a/cache.h
+++ b/cache.h
@@ -1027,7 +1027,7 @@ extern int validate_headref(const char *ref);
extern int base_name_compare(const char *name1, int len1, int mode1, const
char *name2, int len2, int mode2);
extern int df_name_compare(const char *name1, int len1, int mode1, const char
*name2, int len2, int mode2);
-extern int cache_name_compare(const char *name1, int len1, const char *name2,
int len2);
+extern int name_compare(const char *name1, size_t len1, const char *name2,
size_t len2);
extern int cache_name_stage_compare(const char *name1, int len1, int stage1,
const char *name2, int len2, int stage2);
extern void *read_object_with_reference(const unsigned char *sha1,
diff --git a/read-cache.c b/read-cache.c
index 9f56d76..158241d 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -434,18 +434,26 @@ int df_name_compare(const char *name1, int len1, int
mode1,
return c1 - c2;
}
-int cache_name_stage_compare(const char *name1, int len1, int stage1, const
char *name2, int len2, int stage2)
+int name_compare(const char *name1, size_t len1, const char *name2, size_t
len2)
{
- int len = len1 < len2 ? len1 : len2;
- int cmp;
-
- cmp = memcmp(name1, name2, len);
+ size_t min_len = (len1 < len2) ? len1 : len2;
+ int cmp = memcmp(name1, name2, min_len);
if (cmp)
return cmp;
if (len1 < len2)
return -1;
if (len1 > len2)
return 1;
+ return 0;
+}
+
+int cache_name_stage_compare(const char *name1, int len1, int stage1, const
char *name2, int len2, int stage2)
+{
+ int cmp;
+
+ cmp = name_compare(name1, len1, name2, len2);
+ if (cmp)
+ return cmp;
if (stage1 < stage2)
return -1;
@@ -454,11 +462,6 @@ int cache_name_stage_compare(const char *name1, int len1,
int stage1, const char
return 0;
}
-int cache_name_compare(const char *name1, int len1, const char *name2, int
len2)
-{
- return cache_name_stage_compare(name1, len1, 0, name2, len2, 0);
-}
-
static int index_name_stage_pos(const struct index_state *istate, const char
*name, int namelen, int stage)
{
int first, last;
--
2.0.0
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 2/5] tree-walk.c: remove name_compare() function
Remove the duplicate name_compare() function and use the one provided by
read-cache.c.
Signed-off-by: Jeremiah Mahler
---
Notes:
There is one small difference between the old function and the new one.
The old one returned -N and +N whereas the new one returns -1 and +1.
However, there is no place where the magnitude was needed, so this
change will not alter its behavior.
tree-walk.c | 10 --
1 file changed, 10 deletions(-)
diff --git a/tree-walk.c b/tree-walk.c
index 4dc86c7..5dd9a71 100644
--- a/tree-walk.c
+++ b/tree-walk.c
@@ -144,16 +144,6 @@ struct tree_desc_x {
struct tree_desc_skip *skip;
};
-static int name_compare(const char *a, int a_len,
- const char *b, int b_len)
-{
- int len = (a_len < b_len) ? a_len : b_len;
- int cmp = memcmp(a, b, len);
- if (cmp)
- return cmp;
- return (a_len - b_len);
-}
-
static int check_entry_match(const char *a, int a_len, const char *b, int
b_len)
{
/*
--
2.0.0
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 5/5] name-hash.c: rename to name_compare()
Rename the call to cache_name_compare() to name_compare(). Signed-off-by: Jeremiah Mahler --- name-hash.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/name-hash.c b/name-hash.c index be7c4ae..e2bea88 100644 --- a/name-hash.c +++ b/name-hash.c @@ -179,7 +179,7 @@ static int same_name(const struct cache_entry *ce, const char *name, int namelen * Always do exact compare, even if we want a case-ignoring comparison; * we do the quick exact one first, because it will be the common case. */ - if (len == namelen && !cache_name_compare(name, namelen, ce->name, len)) + if (len == namelen && !name_compare(name, namelen, ce->name, len)) return 1; if (!icase) -- 2.0.0 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 4/5] dir.c: rename to name_compare()
Rename the call to cache_name_compare() to name_compare(). Signed-off-by: Jeremiah Mahler --- Notes: This is a case where cache_name_compare() was used even though it had nothing to do with a cache. The new name makes it clear that no cache is involved. dir.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/dir.c b/dir.c index 797805d..e65888d 100644 --- a/dir.c +++ b/dir.c @@ -1354,8 +1354,7 @@ static int cmp_name(const void *p1, const void *p2) const struct dir_entry *e1 = *(const struct dir_entry **)p1; const struct dir_entry *e2 = *(const struct dir_entry **)p2; - return cache_name_compare(e1->name, e1->len, - e2->name, e2->len); + return name_compare(e1->name, e1->len, e2->name, e2->len); } static struct path_simplify *create_simplify(const char **pathspec) -- 2.0.0 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 3/5] unpack-trees.c: remove name_compare() function
Remove the duplicate name_compare() function and use the one provided by
read-cache.c.
Signed-off-by: Jeremiah Mahler
---
Notes:
There is one small difference between the old function and the new one.
The old one returned -N and +N whereas the new one returns -1 and +1.
However, there is no place where the magnitude was needed, so this
change will not alter its behavior.
unpack-trees.c | 11 ---
1 file changed, 11 deletions(-)
diff --git a/unpack-trees.c b/unpack-trees.c
index 4a9cdf2..c4a97ca 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -629,17 +629,6 @@ static int unpack_failed(struct unpack_trees_options *o,
const char *message)
return -1;
}
-/* NEEDSWORK: give this a better name and share with tree-walk.c */
-static int name_compare(const char *a, int a_len,
- const char *b, int b_len)
-{
- int len = (a_len < b_len) ? a_len : b_len;
- int cmp = memcmp(a, b, len);
- if (cmp)
- return cmp;
- return (a_len - b_len);
-}
-
/*
* The tree traversal is looking at name p. If we have a matching entry,
* return it. If name p is a directory in the index, do not return
--
2.0.0
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 2/5] tree-walk.c: remove name_compare() function
Jeremiah Mahler wrote: > Remove the duplicate name_compare() function and use the one provided by > read-cache.c. I'd squash this into patch 1/5. > --- > Notes: > There is one small difference between the old function and the new one. > The old one returned -N and +N whereas the new one returns -1 and +1. > However, there is no place where the magnitude was needed, so this > change will not alter its behavior. This is useful information for anyone looking back at the patch in the future, so it belongs above the three-dash divider. Thanks, Jonathan -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 3/5] unpack-trees.c: remove name_compare() function
Jeremiah Mahler wrote: > unpack-trees.c | 11 --- > 1 file changed, 11 deletions(-) Same thoughts as patch 2/5. :) Thanks, Jonathan -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 4/5] dir.c: rename to name_compare()
Jeremiah Mahler wrote: > This is a case where cache_name_compare() was used even though it had > nothing to do with a cache. The new name makes it clear that no cache > is involved. That's a perfect sort of thing to put in the commit message. ;-) Unlike patches 2 and 3, this could make sense to me as a separate patch from 1/5. Except... how does git work at all with patch 1 and without this patch? I thought that patch removed the public cache_name_compare function. Would it make sense to delay the removal of cache_name_compare until a patch at the end of the series? The patch is small enough that squashing into patch 1 seems fine, too. [...] > Rename the call to cache_name_compare() to name_compare(). It's not actually renaming but calling a different function, right? So I'd say something like read_directory: use name_compare instead of cache_name_compare This is a case where cache_name_compare() was used even though it had nothing to do with a cache. The new name makes it clear that no cache is involved. No functional change intended. Thanks, Jonathan -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 5/5] name-hash.c: rename to name_compare()
Jeremiah Mahler wrote: > name-hash.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Same thoughts as patch 4/5. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 1/5] cache: rename cache_name_compare() to name_compare()
Jeremiah Mahler wrote: > The cache_name_compare() function is not specific to a cache. > Make its name more general by renaming it to name_compare(). Sounds reasonable. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 0/5] cleanup duplicate name_compare() functions
Jeremiah Mahler wrote: > Jeremiah Mahler (5): > cache: rename cache_name_compare() to name_compare() > tree-walk.c: remove name_compare() function > unpack-trees.c: remove name_compare() function > dir.c: rename to name_compare() > name-hash.c: rename to name_compare() > > cache.h| 2 +- > dir.c | 3 +-- > name-hash.c| 2 +- > read-cache.c | 23 +-- > tree-walk.c| 10 -- > unpack-trees.c | 11 --- > 6 files changed, 16 insertions(+), 35 deletions(-) After looking at the patches I suspect this should be a single patch. That way it's bisectable, and the changes outside of read-cache.c are small enough that it's not too much of a burden to review as a single patch. The code change looked good. Thanks and hope that helps, Jonathan -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 0/16] skip_prefix refactoring and cleanups
A while ago[1] we discussed refactoring skip_prefix (or adding something like it) to make it more natural to call. This morning I decided to take a look at doing this, and went down a rabbit hole of cleanups. This is part one of the result. The short of it is that skip_prefix can now be used like this: if (skip_prefix(arg, "--foo=", &value) handle_foo(value); or even: /* arg remains valid if we didn't match! */ if (skip_prefix(arg, "--foo=", &arg)) handle_foo(arg); else if (skip_prefix(arg, "--bar", &arg)) handle_bar(arg); though the latter form is not always useful if the conditional code wants to see all of "arg". [01/16]: parse_diff_color_slot: drop ofs parameter [02/16]: daemon: mark some strings as const [03/16]: avoid using skip_prefix as a boolean These ones are preparatory cleanup. [04/16]: refactor skip_prefix to return a boolean The actual refactoring; it changes the existing callers[2] at the same time. [05/16]: apply: use skip_prefix instead of raw addition [06/16]: fast-import: fix read of uninitialized argv memory [07/16]: transport-helper: avoid reading past end-of-string These three are conversions that actually fix bugs. [08/16]: use skip_prefix to avoid magic numbers [09/16]: use skip_prefix to avoid repeating strings These are the straightforward conversions lumped together by the problem they are solving. [10/16]: fast-import: use skip_prefix for parsing input [11/16]: daemon: use skip_prefix to avoid magic numbers [12/16]: stat_opt: check extra strlen call [13/16]: fast-import: refactor parsing of spaces [14/16]: fetch-pack: refactor parsing in get_ack [15/16]: git: avoid magic number with skip_prefix These ones are variants of the above two that needed extra discussion or attention for various reasons. [16/16]: use skip_prefix to avoid repeated calculations These ones don't solve a specific problem as the patches above do, but I think the code ends up more readable. My conversions are by now means exhaustive. After grepping through all of the starts_with up to about http.c, I decided to call it a day. But we can easily convert more as time goes on. [1] http://thread.gmane.org/gmane.comp.version-control.git/239438/focus=239564 I started from scratch this morning, oblivious to the fact that René posted something very similar to patch 4 in that thread. [2] I test-merged with 'pu'. There is a minor textual conflict with jk/commit-buffer-length that should be easy to resolve. There's also one new caller of skip_prefix added in cc/interpret-trailers. It needs this fix when merged: diff --git a/trailer.c b/trailer.c index eaf692b..987fa29 100644 --- a/trailer.c +++ b/trailer.c @@ -377,8 +377,7 @@ static int git_trailer_config(const char *conf_key, const char *value, void *cb) enum trailer_info_type type; int i; - trailer_item = skip_prefix(conf_key, "trailer."); - if (!trailer_item) + if (!skip_prefix(conf_key, "trailer.", &trailer_item)) return 0; variable_name = strrchr(trailer_item, '.'); -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 02/16] daemon: mark some strings as const
None of these strings is modified; marking them as const
will help later refactoring.
Signed-off-by: Jeff King
---
daemon.c | 14 +++---
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/daemon.c b/daemon.c
index f9c63e9..18818c3 100644
--- a/daemon.c
+++ b/daemon.c
@@ -39,8 +39,8 @@ static int strict_paths;
static int export_all_trees;
/* Take all paths relative to this one if non-NULL */
-static char *base_path;
-static char *interpolated_path;
+static const char *base_path;
+static const char *interpolated_path;
static int base_path_relaxed;
/* Flag indicating client sent extra args. */
@@ -106,12 +106,12 @@ static void NORETURN daemon_die(const char *err, va_list
params)
exit(1);
}
-static const char *path_ok(char *directory)
+static const char *path_ok(const char *directory)
{
static char rpath[PATH_MAX];
static char interp_path[PATH_MAX];
const char *path;
- char *dir;
+ const char *dir;
dir = directory;
@@ -131,7 +131,7 @@ static const char *path_ok(char *directory)
* "~alice/%s/foo".
*/
int namlen, restlen = strlen(dir);
- char *slash = strchr(dir, '/');
+ const char *slash = strchr(dir, '/');
if (!slash)
slash = dir + restlen;
namlen = slash - dir;
@@ -253,7 +253,7 @@ static int daemon_error(const char *dir, const char *msg)
return -1;
}
-static char *access_hook;
+static const char *access_hook;
static int run_access_hook(struct daemon_service *service, const char *dir,
const char *path)
{
@@ -318,7 +318,7 @@ error_return:
return -1;
}
-static int run_service(char *dir, struct daemon_service *service)
+static int run_service(const char *dir, struct daemon_service *service)
{
const char *path;
int enabled = service->enabled;
--
2.0.0.566.gfe3e6b2
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 01/16] parse_diff_color_slot: drop ofs parameter
This function originally took a whole config variable name
("var") and an offset ("ofs"). It checked "var+ofs" against
each color slot, but reported errors using the whole "var".
However, since 8b8e862 (ignore unknown color configuration,
2009-12-12), it returns -1 rather than printing its own
error, and therefore only cares about var+ofs. We can drop
the ofs parameter and teach its sole caller to derive the
pointer itself.
Signed-off-by: Jeff King
---
diff.c | 20 ++--
1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/diff.c b/diff.c
index bba9a55..77c5eb4 100644
--- a/diff.c
+++ b/diff.c
@@ -52,23 +52,23 @@ static char diff_colors[][COLOR_MAXLEN] = {
GIT_COLOR_NORMAL, /* FUNCINFO */
};
-static int parse_diff_color_slot(const char *var, int ofs)
+static int parse_diff_color_slot(const char *var)
{
- if (!strcasecmp(var+ofs, "plain"))
+ if (!strcasecmp(var, "plain"))
return DIFF_PLAIN;
- if (!strcasecmp(var+ofs, "meta"))
+ if (!strcasecmp(var, "meta"))
return DIFF_METAINFO;
- if (!strcasecmp(var+ofs, "frag"))
+ if (!strcasecmp(var, "frag"))
return DIFF_FRAGINFO;
- if (!strcasecmp(var+ofs, "old"))
+ if (!strcasecmp(var, "old"))
return DIFF_FILE_OLD;
- if (!strcasecmp(var+ofs, "new"))
+ if (!strcasecmp(var, "new"))
return DIFF_FILE_NEW;
- if (!strcasecmp(var+ofs, "commit"))
+ if (!strcasecmp(var, "commit"))
return DIFF_COMMIT;
- if (!strcasecmp(var+ofs, "whitespace"))
+ if (!strcasecmp(var, "whitespace"))
return DIFF_WHITESPACE;
- if (!strcasecmp(var+ofs, "func"))
+ if (!strcasecmp(var, "func"))
return DIFF_FUNCINFO;
return -1;
}
@@ -240,7 +240,7 @@ int git_diff_basic_config(const char *var, const char
*value, void *cb)
return -1;
if (starts_with(var, "diff.color.") || starts_with(var, "color.diff."))
{
- int slot = parse_diff_color_slot(var, 11);
+ int slot = parse_diff_color_slot(var + 11);
if (slot < 0)
return 0;
if (!value)
--
2.0.0.566.gfe3e6b2
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 03/16] avoid using skip_prefix as a boolean
There's no point in using: if (skip_prefix(buf, "foo")) over if (starts_with(buf, "foo")) as the point of skip_prefix is to return a pointer to the data after the prefix. Using starts_with is more readable, and will make refactoring skip_prefix easier. Signed-off-by: Jeff King --- builtin/fmt-merge-msg.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/builtin/fmt-merge-msg.c b/builtin/fmt-merge-msg.c index 3906eda..72378e6 100644 --- a/builtin/fmt-merge-msg.c +++ b/builtin/fmt-merge-msg.c @@ -298,7 +298,7 @@ static void credit_people(struct strbuf *out, (them->nr == 1 && me && (me = skip_prefix(me, them->items->string)) != NULL && -skip_prefix(me, " <"))) +starts_with(me, " <"))) return; strbuf_addf(out, "\n%c %s ", comment_line_char, label); add_people_count(out, them); -- 2.0.0.566.gfe3e6b2 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 04/16] refactor skip_prefix to return a boolean
The skip_prefix function returns a pointer to the content
past the prefix, or NULL if the prefix was not found. While
this is nice and simple, in practice it makes it hard to use
for two reasons:
1. When you want to conditionally skip or keep the string
as-is, you have to introduce a second temporary
variable. For example:
tmp = skip_prefix(buf, "foo");
if (tmp)
buf = tmp;
2. It is verbose to check the outcome in a conditional, as
you need extra parentheses to silence compiler
warnings. For example:
if ((cp = skip_prefix(buf, "foo"))
/* do something with cp */
Both of these make it harder to use for long if-chains, and
we tend to use starts_with instead. However, the first line
of "do something" is often to then skip forward in buf past
the prefix, either using a magic constant or with an extra
strlen (which is generally computed at compile time, but
means we are repeating ourselves).
This patch refactors skip_prefix to return a simple boolean,
and to provide the pointer value as an out-parameter. If the
prefix is not found, the out-parameter is untouched. This
lets you write:
if (skip_prefix(arg, "foo ", &arg))
do_foo(arg);
else if (skip_prefix(arg, "bar ", &arg))
do_bar(arg);
Signed-off-by: Jeff King
---
The diffstat is misleading. We actually save lines, except that I added
documentation for the function. :)
advice.c | 5 -
branch.c | 4 ++--
builtin/branch.c | 6 +++---
builtin/clone.c| 11 +++
builtin/commit.c | 5 ++---
builtin/fmt-merge-msg.c| 2 +-
builtin/push.c | 7 +++
builtin/remote.c | 4 +---
column.c | 5 +++--
commit.c | 6 ++
config.c | 3 +--
credential-cache--daemon.c | 6 ++
credential.c | 3 +--
fsck.c | 14 +-
git-compat-util.h | 26 ++
parse-options.c| 16 +---
transport.c| 4 +++-
urlmatch.c | 3 +--
18 files changed, 72 insertions(+), 58 deletions(-)
diff --git a/advice.c b/advice.c
index c50ebdf..9b42033 100644
--- a/advice.c
+++ b/advice.c
@@ -61,9 +61,12 @@ void advise(const char *advice, ...)
int git_default_advice_config(const char *var, const char *value)
{
- const char *k = skip_prefix(var, "advice.");
+ const char *k;
int i;
+ if (!skip_prefix(var, "advice.", &k))
+ return 0;
+
for (i = 0; i < ARRAY_SIZE(advice_config); i++) {
if (strcmp(k, advice_config[i].name))
continue;
diff --git a/branch.c b/branch.c
index 660097b..46e8aa8 100644
--- a/branch.c
+++ b/branch.c
@@ -50,11 +50,11 @@ static int should_setup_rebase(const char *origin)
void install_branch_config(int flag, const char *local, const char *origin,
const char *remote)
{
- const char *shortname = skip_prefix(remote, "refs/heads/");
+ const char *shortname = NULL;
struct strbuf key = STRBUF_INIT;
int rebasing = should_setup_rebase(origin);
- if (shortname
+ if (skip_prefix(remote, "refs/heads/", &shortname)
&& !strcmp(local, shortname)
&& !origin) {
warning(_("Not setting branch %s as its own upstream."),
diff --git a/builtin/branch.c b/builtin/branch.c
index 652b1d2..0591b22 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -294,13 +294,13 @@ static char *resolve_symref(const char *src, const char
*prefix)
{
unsigned char sha1[20];
int flag;
- const char *dst, *cp;
+ const char *dst;
dst = resolve_ref_unsafe(src, sha1, 0, &flag);
if (!(dst && (flag & REF_ISSYMREF)))
return NULL;
- if (prefix && (cp = skip_prefix(dst, prefix)))
- dst = cp;
+ if (prefix)
+ skip_prefix(dst, prefix, &dst);
return xstrdup(dst);
}
diff --git a/builtin/clone.c b/builtin/clone.c
index b12989d..a5b2d9d 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -584,11 +584,11 @@ static void update_remote_refs(const struct ref *refs,
static void update_head(const struct ref *our, const struct ref *remote,
const char *msg)
{
- if (our && starts_with(our->name, "refs/heads/")) {
+ const char *head;
+ if (our && skip_prefix(our->name, "refs/heads/", &head)) {
/* Local default branch link */
create_symref("HEAD", our->name, NULL);
if (!option_bare) {
- const char *head = skip_prefix(our->name,
"refs/heads/");
update_ref(msg, "HEAD", our->old_sha1, NULL, 0,
UPDATE_REFS_DIE_ON_ERR);
install_branch_config(0, head, option_origin,
our->nam
[PATCH 05/16] apply: use skip_prefix instead of raw addition
A submodule diff generally has content like:
-Subproject commit [0-9a-f]{40}
+Subproject commit [0-9a-f]{40}
When we are using "git apply --index" with a submodule, we
first apply the textual diff, and then parse that result to
figure out the new sha1.
If the diff has bogus input like:
-Subproject commit 1234567890123456789012345678901234567890
+bogus
we will parse the "bogus" portion. Our parser assumes that
the buffer starts with "Subproject commit", and blindly
skips past it using strlen(). This can cause us to read
random memory after the buffer.
This problem was unlikely to have come up in practice (since
it requires a malformed diff), and even when it did, we
likely noticed the problem anyway as the next operation was
to call get_sha1_hex on the random memory.
However, we can easily fix it by using skip_prefix to notice
the parsing error.
Signed-off-by: Jeff King
---
builtin/apply.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/builtin/apply.c b/builtin/apply.c
index 9c5724e..bc924ab 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -3847,9 +3847,10 @@ static void add_index_file(const char *path, unsigned
mode, void *buf, unsigned
ce->ce_flags = create_ce_flags(0);
ce->ce_namelen = namelen;
if (S_ISGITLINK(mode)) {
- const char *s = buf;
+ const char *s;
- if (get_sha1_hex(s + strlen("Subproject commit "), ce->sha1))
+ if (!skip_prefix(buf, "Subproject commit ", &s) ||
+ get_sha1_hex(s, ce->sha1))
die(_("corrupt patch for submodule %s"), path);
} else {
if (!cached) {
--
2.0.0.566.gfe3e6b2
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 06/16] fast-import: fix read of uninitialized argv memory
Fast-import shares code between its command-line parser and
the "option" command. To do so, it strips the "--" from any
command-line options and passes them to the option parser.
However, it does not confirm that the option even begins
with "--" before blindly passing "arg + 2".
It does confirm that the option starts with "-", so the only
affected case was:
git fast-import -
which would read uninitialized memory after the argument. We
can fix it by using skip_prefix and checking the result. As
a bonus, this gets rid of some magic numbers.
Signed-off-by: Jeff King
---
fast-import.c | 13 -
1 file changed, 8 insertions(+), 5 deletions(-)
diff --git a/fast-import.c b/fast-import.c
index 6707a66..b2030cc 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -3342,18 +3342,21 @@ static void parse_argv(void)
if (*a != '-' || !strcmp(a, "--"))
break;
- if (parse_one_option(a + 2))
+ if (!skip_prefix(a, "--", &a))
+ die("unknown option %s", a);
+
+ if (parse_one_option(a))
continue;
- if (parse_one_feature(a + 2, 0))
+ if (parse_one_feature(a, 0))
continue;
- if (starts_with(a + 2, "cat-blob-fd=")) {
- option_cat_blob_fd(a + 2 + strlen("cat-blob-fd="));
+ if (skip_prefix(a, "cat-blob-fd=", &a)) {
+ option_cat_blob_fd(a);
continue;
}
- die("unknown option %s", a);
+ die("unknown option --%s", a);
}
if (i != global_argc)
usage(fast_import_usage);
--
2.0.0.566.gfe3e6b2
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 07/16] transport-helper: avoid reading past end-of-string
We detect the "import-marks" capability by looking for that
string, but _without_ a trailing space. Then we skip past it
using strlen("import-marks "), with a space. So if a remote
helper gives us exactly "import-marks", we will read past
the end-of-string by one character.
This is unlikely to be a problem in practice, because such
input is malformed in the first place, and because there is
a good chance that the string has an extra NUL terminator
one character after the original (because it formerly had a
newline in it that we parsed off).
We can fix it by using skip_prefix with "import-marks ",
with the space. The other form appears to be a typo from
a515ebe (transport-helper: implement marks location as
capability, 2011-07-16); "import-marks" has never existed
without an argument, and it should match the "export-marks"
definition above.
Speaking of which, we can also use skip_prefix in a few
other places while we are in the function.
Signed-off-by: Jeff King
---
transport-helper.c | 14 +++---
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/transport-helper.c b/transport-helper.c
index 84c616f..3d8fe7d 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -153,7 +153,7 @@ static struct child_process *get_helper(struct transport
*transport)
write_constant(helper->in, "capabilities\n");
while (1) {
- const char *capname;
+ const char *capname, *arg;
int mandatory = 0;
if (recvline(data, &buf))
exit(128);
@@ -183,19 +183,19 @@ static struct child_process *get_helper(struct transport
*transport)
data->export = 1;
else if (!strcmp(capname, "check-connectivity"))
data->check_connectivity = 1;
- else if (!data->refspecs && starts_with(capname, "refspec ")) {
+ else if (!data->refspecs && skip_prefix(capname, "refspec ",
&arg)) {
ALLOC_GROW(refspecs,
refspec_nr + 1,
refspec_alloc);
- refspecs[refspec_nr++] = xstrdup(capname +
strlen("refspec "));
+ refspecs[refspec_nr++] = xstrdup(arg);
} else if (!strcmp(capname, "connect")) {
data->connect = 1;
} else if (!strcmp(capname, "signed-tags")) {
data->signed_tags = 1;
- } else if (starts_with(capname, "export-marks ")) {
- data->export_marks = xstrdup(capname +
strlen("export-marks "));
- } else if (starts_with(capname, "import-marks")) {
- data->import_marks = xstrdup(capname +
strlen("import-marks "));
+ } else if (skip_prefix(capname, "export-marks ", &arg)) {
+ data->export_marks = xstrdup(arg);
+ } else if (skip_prefix(capname, "import-marks ", &arg)) {
+ data->import_marks = xstrdup(arg);
} else if (starts_with(capname, "no-private-update")) {
data->no_private_update = 1;
} else if (mandatory) {
--
2.0.0.566.gfe3e6b2
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 08/16] use skip_prefix to avoid magic numbers
It's a common idiom to match a prefix and then skip past it
with a magic number, like:
if (starts_with(foo, "bar"))
foo += 3;
This is easy to get wrong, since you have to count the
prefix string yourself, and there's no compiler check if the
string changes. We can use skip_prefix to avoid the magic
numbers here.
Note that some of these conversions could be much shorter.
For example:
if (starts_with(arg, "--foo=")) {
bar = arg + 6;
continue;
}
could become:
if (skip_prefix(arg, "--foo=", &bar))
continue;
However, I have left it as:
if (skip_prefix(arg, "--foo=", &v)) {
bar = v;
continue;
}
to visually match nearby cases which need to actually
process the string. Like:
if (skip_prefix(arg, "--foo=", &v)) {
bar = atoi(v);
continue;
}
Signed-off-by: Jeff King
---
alias.c| 3 ++-
connect.c | 11 +
convert.c | 4 ++--
daemon.c | 73 ++
diff.c | 65 ++-
fast-import.c | 69 +-
fetch-pack.c | 9
git.c | 18 +++
help.c | 6 +++--
http-backend.c | 11 +
http-push.c| 11 +
11 files changed, 149 insertions(+), 131 deletions(-)
diff --git a/alias.c b/alias.c
index 5efc3d6..758c867 100644
--- a/alias.c
+++ b/alias.c
@@ -5,7 +5,8 @@ static char *alias_val;
static int alias_lookup_cb(const char *k, const char *v, void *cb)
{
- if (starts_with(k, "alias.") && !strcmp(k + 6, alias_key)) {
+ const char *name;
+ if (skip_prefix(k, "alias.", &name) && !strcmp(name, alias_key)) {
if (!v)
return config_error_nonbool(k);
alias_val = xstrdup(v);
diff --git a/connect.c b/connect.c
index 94a6650..37ff018 100644
--- a/connect.c
+++ b/connect.c
@@ -129,6 +129,7 @@ struct ref **get_remote_heads(int in, char *src_buf, size_t
src_len,
char *name;
int len, name_len;
char *buffer = packet_buffer;
+ const char *arg;
len = packet_read(in, &src_buf, &src_len,
packet_buffer, sizeof(packet_buffer),
@@ -140,12 +141,12 @@ struct ref **get_remote_heads(int in, char *src_buf,
size_t src_len,
if (!len)
break;
- if (len > 4 && starts_with(buffer, "ERR "))
- die("remote error: %s", buffer + 4);
+ if (len > 4 && skip_prefix(buffer, "ERR ", &arg))
+ die("remote error: %s", arg);
- if (len == 48 && starts_with(buffer, "shallow ")) {
- if (get_sha1_hex(buffer + 8, old_sha1))
- die("protocol error: expected shallow sha-1,
got '%s'", buffer + 8);
+ if (len == 48 && skip_prefix(buffer, "shallow ", &arg)) {
+ if (get_sha1_hex(arg, old_sha1))
+ die("protocol error: expected shallow sha-1,
got '%s'", arg);
if (!shallow_points)
die("repository on the other end cannot be
shallow");
sha1_array_append(shallow_points, old_sha1);
diff --git a/convert.c b/convert.c
index ab80b72..cb5fbb4 100644
--- a/convert.c
+++ b/convert.c
@@ -1121,9 +1121,9 @@ static int is_foreign_ident(const char *str)
{
int i;
- if (!starts_with(str, "$Id: "))
+ if (!skip_prefix(str, "$Id: ", &str))
return 0;
- for (i = 5; str[i]; i++) {
+ for (i = 0; str[i]; i++) {
if (isspace(str[i]) && str[i+1] != '$')
return 1;
}
diff --git a/daemon.c b/daemon.c
index 18818c3..6d25828 100644
--- a/daemon.c
+++ b/daemon.c
@@ -235,8 +235,10 @@ static int service_enabled;
static int git_daemon_config(const char *var, const char *value, void *cb)
{
- if (starts_with(var, "daemon.") &&
- !strcmp(var + 7, service_looking_at->config_name)) {
+ const char *service;
+
+ if (skip_prefix(var, "daemon.", &service) &&
+ !strcmp(service, service_looking_at->config_name)) {
service_enabled = git_config_bool(var, value);
return 0;
}
@@ -1133,16 +1135,17 @@ int main(int argc, char **argv)
for (i = 1; i < argc; i++) {
char *arg = argv[i];
+ const char *v;
- if (starts_with(arg, "--listen=")) {
- string_list_append(&listen_addr, xstrdup_tolower(arg +
9));
+ if (skip_prefix(arg, "--listen=", &v)) {
+ string_list_append(&listen_addr, xstrdup_tolower(v));
continue;
}
- if (starts_with(arg, "--port=")) {
+
[PATCH 09/16] use skip_prefix to avoid repeating strings
It's a common idiom to match a prefix and then skip past it
with strlen, like:
if (starts_with(foo, "bar"))
foo += strlen("bar");
This avoids magic numbers, but means we have to repeat the
string (and there is no compiler check that we didn't make a
typo in one of the strings).
We can use skip_prefix to handle this case without repeating
ourselves.
Signed-off-by: Jeff King
---
builtin/checkout.c | 4 ++--
builtin/fmt-merge-msg.c | 6 +++---
builtin/log.c | 12 ++--
diff.c | 27 +--
help.c | 7 ---
merge-recursive.c | 15 ---
pretty.c| 3 +--
remote-curl.c | 15 ---
remote.c| 5 ++---
sha1_name.c | 4 +---
10 files changed, 44 insertions(+), 54 deletions(-)
diff --git a/builtin/checkout.c b/builtin/checkout.c
index f1dc56e..463cfee 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -776,8 +776,8 @@ static int switch_branches(const struct checkout_opts *opts,
if (!(flag & REF_ISSYMREF))
old.path = NULL;
- if (old.path && starts_with(old.path, "refs/heads/"))
- old.name = old.path + strlen("refs/heads/");
+ if (old.path)
+ skip_prefix(old.path, "refs/heads/", &old.name);
if (!new->name) {
new->name = "HEAD";
diff --git a/builtin/fmt-merge-msg.c b/builtin/fmt-merge-msg.c
index 3c19241..ad3bc58 100644
--- a/builtin/fmt-merge-msg.c
+++ b/builtin/fmt-merge-msg.c
@@ -100,7 +100,8 @@ static int handle_line(char *line, struct merge_parents
*merge_parents)
{
int i, len = strlen(line);
struct origin_data *origin_data;
- char *src, *origin;
+ char *src;
+ const char *origin;
struct src_data *src_data;
struct string_list_item *item;
int pulling_head = 0;
@@ -164,8 +165,7 @@ static int handle_line(char *line, struct merge_parents
*merge_parents)
origin = line;
string_list_append(&src_data->tag, origin + 4);
src_data->head_status |= 2;
- } else if (starts_with(line, "remote-tracking branch ")) {
- origin = line + strlen("remote-tracking branch ");
+ } else if (skip_prefix(line, "remote-tracking branch ", &origin)) {
string_list_append(&src_data->r_branch, origin);
src_data->head_status |= 2;
} else {
diff --git a/builtin/log.c b/builtin/log.c
index a7ba211..0f59c25 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -872,7 +872,7 @@ static char *find_branch_name(struct rev_info *rev)
int i, positive = -1;
unsigned char branch_sha1[20];
const unsigned char *tip_sha1;
- const char *ref;
+ const char *ref, *v;
char *full_ref, *branch = NULL;
for (i = 0; i < rev->cmdline.nr; i++) {
@@ -888,9 +888,9 @@ static char *find_branch_name(struct rev_info *rev)
ref = rev->cmdline.rev[positive].name;
tip_sha1 = rev->cmdline.rev[positive].item->sha1;
if (dwim_ref(ref, strlen(ref), branch_sha1, &full_ref) &&
- starts_with(full_ref, "refs/heads/") &&
+ skip_prefix(full_ref, "refs/heads/", &v) &&
!hashcmp(tip_sha1, branch_sha1))
- branch = xstrdup(full_ref + strlen("refs/heads/"));
+ branch = xstrdup(v);
free(full_ref);
return branch;
}
@@ -1394,10 +1394,10 @@ int cmd_format_patch(int argc, const char **argv, const
char *prefix)
if (check_head) {
unsigned char sha1[20];
- const char *ref;
+ const char *ref, *v;
ref = resolve_ref_unsafe("HEAD", sha1, 1, NULL);
- if (ref && starts_with(ref, "refs/heads/"))
- branch_name = xstrdup(ref +
strlen("refs/heads/"));
+ if (ref && skip_prefix(ref, "refs/heads/", &v))
+ branch_name = xstrdup(v);
else
branch_name = xstrdup(""); /* no branch */
}
diff --git a/diff.c b/diff.c
index 97db932..2378ae4 100644
--- a/diff.c
+++ b/diff.c
@@ -3395,12 +3395,10 @@ int parse_long_opt(const char *opt, const char **argv,
const char **optarg)
{
const char *arg = argv[0];
- if (arg[0] != '-' || arg[1] != '-')
+ if (!skip_prefix(arg, "--", &arg))
return 0;
- arg += strlen("--");
- if (!starts_with(arg, opt))
+ if (!skip_prefix(arg, opt, &arg))
return 0;
- arg += strlen(opt);
if (*arg == '=') { /* stuck form: --option=value */
*optarg = arg + 1;
return 1;
@@ -3429,8 +3427,7 @@ static int stat_opt(struct diff_options *options, const
char **av)
switch (*arg) {
case '-':
-
[PATCH 10/16] fast-import: use skip_prefix for parsing input
Fast-import does a lot of parsing of commands and
dispatching to sub-functions. For example, given "option
foo", we might recognize "option " using starts_with, and
then hand it off to parse_option() to do the rest.
However, we do not let parse_option know that we have parsed
the first part already. It gets the full buffer, and has to
skip past the uninteresting bits. Some functions simply add
a magic constant:
char *option = command_buf.buf + 7;
Others use strlen:
char *option = command_buf.buf + strlen("option ");
And others use strchr:
char *option = strchr(command_buf.buf, ' ') + 1;
All of these are brittle and easy to get wrong (especially
given that the starts_with call and the code that assumes
the presence of the prefix are far apart). Instead, we can
use skip_prefix, and just pass each handler a pointer to its
arguments.
Signed-off-by: Jeff King
---
fast-import.c | 125 +-
1 file changed, 53 insertions(+), 72 deletions(-)
diff --git a/fast-import.c b/fast-import.c
index a3ffe30..5f17adb 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -371,8 +371,8 @@ static volatile sig_atomic_t checkpoint_requested;
static int cat_blob_fd = STDOUT_FILENO;
static void parse_argv(void);
-static void parse_cat_blob(void);
-static void parse_ls(struct branch *b);
+static void parse_cat_blob(const char *p);
+static void parse_ls(const char *p, struct branch *b);
static void write_branch_report(FILE *rpt, struct branch *b)
{
@@ -1861,6 +1861,8 @@ static int read_next_command(void)
}
for (;;) {
+ const char *p;
+
if (unread_command_buf) {
unread_command_buf = 0;
} else {
@@ -1893,8 +1895,8 @@ static int read_next_command(void)
rc->prev->next = rc;
cmd_tail = rc;
}
- if (starts_with(command_buf.buf, "cat-blob ")) {
- parse_cat_blob();
+ if (skip_prefix(command_buf.buf, "cat-blob ", &p)) {
+ parse_cat_blob(p);
continue;
}
if (command_buf.buf[0] == '#')
@@ -2273,9 +2275,8 @@ static uintmax_t parse_mark_ref_space(const char **p)
return mark;
}
-static void file_change_m(struct branch *b)
+static void file_change_m(const char *p, struct branch *b)
{
- const char *p = command_buf.buf + 2;
static struct strbuf uq = STRBUF_INIT;
const char *endp;
struct object_entry *oe;
@@ -2376,9 +2377,8 @@ static void file_change_m(struct branch *b)
tree_content_set(&b->branch_tree, p, sha1, mode, NULL);
}
-static void file_change_d(struct branch *b)
+static void file_change_d(const char *p, struct branch *b)
{
- const char *p = command_buf.buf + 2;
static struct strbuf uq = STRBUF_INIT;
const char *endp;
@@ -2391,15 +2391,14 @@ static void file_change_d(struct branch *b)
tree_content_remove(&b->branch_tree, p, NULL, 1);
}
-static void file_change_cr(struct branch *b, int rename)
+static void file_change_cr(const char *s, struct branch *b, int rename)
{
- const char *s, *d;
+ const char *d;
static struct strbuf s_uq = STRBUF_INIT;
static struct strbuf d_uq = STRBUF_INIT;
const char *endp;
struct tree_entry leaf;
- s = command_buf.buf + 2;
strbuf_reset(&s_uq);
if (!unquote_c_style(&s_uq, s, &endp)) {
if (*endp != ' ')
@@ -2444,9 +2443,8 @@ static void file_change_cr(struct branch *b, int rename)
leaf.tree);
}
-static void note_change_n(struct branch *b, unsigned char *old_fanout)
+static void note_change_n(const char *p, struct branch *b, unsigned char
*old_fanout)
{
- const char *p = command_buf.buf + 2;
static struct strbuf uq = STRBUF_INIT;
struct object_entry *oe;
struct branch *s;
@@ -2587,7 +2585,7 @@ static int parse_from(struct branch *b)
const char *from;
struct branch *s;
- if (!starts_with(command_buf.buf, "from "))
+ if (!skip_prefix(command_buf.buf, "from ", &from))
return 0;
if (b->branch_tree.tree) {
@@ -2595,7 +2593,6 @@ static int parse_from(struct branch *b)
b->branch_tree.tree = NULL;
}
- from = strchr(command_buf.buf, ' ') + 1;
s = lookup_branch(from);
if (b == s)
die("Can't create a branch from itself: %s", b->name);
@@ -2636,8 +2633,7 @@ static struct hash_list *parse_merge(unsigned int *count)
struct branch *s;
*count = 0;
- while (starts_with(command_buf.buf, "merge ")) {
- from = strchr(command_buf.buf, ' ') + 1;
+ while (skip_prefix(command_buf.buf, "merge ", &from)) {
n = xmalloc(sizeof(*n));
s = lookup_branch(from);
if (s)
@@ -266
[PATCH 11/16] daemon: use skip_prefix to avoid magic numbers
Like earlier cases, we can use skip_prefix to avoid magic
numbers that must match the length of starts_with prefixes.
However, the numbers are a little more complicated here, as
we keep parsing past the prefix. We can solve it by keeping
a running pointer as we parse; its final value is the
location we want.
Signed-off-by: Jeff King
---
daemon.c | 11 ++-
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/daemon.c b/daemon.c
index 6d25828..1eb6631 100644
--- a/daemon.c
+++ b/daemon.c
@@ -626,15 +626,16 @@ static int execute(void)
for (i = 0; i < ARRAY_SIZE(daemon_service); i++) {
struct daemon_service *s = &(daemon_service[i]);
- int namelen = strlen(s->name);
- if (starts_with(line, "git-") &&
- !strncmp(s->name, line + 4, namelen) &&
- line[namelen + 4] == ' ') {
+ const char *arg;
+
+ if (skip_prefix(line, "git-", &arg) &&
+ skip_prefix(arg, s->name, &arg) &&
+ *arg++ == ' ') {
/*
* Note: The directory here is probably context
sensitive,
* and might depend on the actual service being
performed.
*/
- return run_service(line + namelen + 5, s);
+ return run_service(arg, s);
}
}
--
2.0.0.566.gfe3e6b2
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 12/16] stat_opt: check extra strlen call
As in earlier commits, the diff option parser uses
starts_with to find that an argument starts with "--stat-",
and then adds strlen("stat-") to find the rest of the
option.
However, in this case the starts_with and the strlen are
separated across functions, making it easy to call the
latter without the former. Let's use skip_prefix instead of
raw pointer arithmetic to catch such a case.
Signed-off-by: Jeff King
---
Another possibility would be for stat_opt to take only the
prefix-skipped part of the string. But that would involve refactoring
its use of "av" (it needs the whole array because it may need to grab a
follow-on argument).
diff.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/diff.c b/diff.c
index 2378ae4..06bdfb8 100644
--- a/diff.c
+++ b/diff.c
@@ -3422,7 +3422,8 @@ static int stat_opt(struct diff_options *options, const
char **av)
int count = options->stat_count;
int argcount = 1;
- arg += strlen("--stat");
+ if (!skip_prefix(arg, "--stat", &arg))
+ die("BUG: stat option does not begin with --stat: %s", arg);
end = (char *)arg;
switch (*arg) {
--
2.0.0.566.gfe3e6b2
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 13/16] fast-import: refactor parsing of spaces
When we see a file change in a commit, we expect one of:
1. A mark.
2. An "inline" keyword.
3. An object sha1.
The handling of spaces is inconsistent between the three
options. Option 1 calls a sub-function which checks for the
space, but doesn't parse past it. Option 2 parses the space,
then deliberately avoids moving the pointer past it. Option
3 detects the space locally but doesn't move past it.
This is confusing, because it looks like option 1 forgets to
check for the space (it's just buried). And option 2 checks
for "inline ", but only moves strlen("inline") characters
forward, which looks like a bug but isn't.
We can make this more clear by just having each branch move
past the space as it is checked (and we can replace the
doubled use of "inline" with a call to skip_prefix).
Signed-off-by: Jeff King
---
fast-import.c | 20 +++-
1 file changed, 7 insertions(+), 13 deletions(-)
diff --git a/fast-import.c b/fast-import.c
index 5f17adb..55ca7d8 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -2269,7 +2269,7 @@ static uintmax_t parse_mark_ref_space(const char **p)
char *end;
mark = parse_mark_ref(*p, &end);
- if (*end != ' ')
+ if (*end++ != ' ')
die("Missing space after mark: %s", command_buf.buf);
*p = end;
return mark;
@@ -2304,20 +2304,17 @@ static void file_change_m(const char *p, struct branch
*b)
if (*p == ':') {
oe = find_mark(parse_mark_ref_space(&p));
hashcpy(sha1, oe->idx.sha1);
- } else if (starts_with(p, "inline ")) {
+ } else if (skip_prefix(p, "inline ", &p)) {
inline_data = 1;
oe = NULL; /* not used with inline_data, but makes gcc happy */
- p += strlen("inline"); /* advance to space */
} else {
if (get_sha1_hex(p, sha1))
die("Invalid dataref: %s", command_buf.buf);
oe = find_object(sha1);
p += 40;
- if (*p != ' ')
+ if (*p++ != ' ')
die("Missing space after SHA1: %s", command_buf.buf);
}
- assert(*p == ' ');
- p++; /* skip space */
strbuf_reset(&uq);
if (!unquote_c_style(&uq, p, &endp)) {
@@ -2474,20 +2471,17 @@ static void note_change_n(const char *p, struct branch
*b, unsigned char *old_fa
if (*p == ':') {
oe = find_mark(parse_mark_ref_space(&p));
hashcpy(sha1, oe->idx.sha1);
- } else if (starts_with(p, "inline ")) {
+ } else if (skip_prefix(p, "inline ", &p)) {
inline_data = 1;
oe = NULL; /* not used with inline_data, but makes gcc happy */
- p += strlen("inline"); /* advance to space */
} else {
if (get_sha1_hex(p, sha1))
die("Invalid dataref: %s", command_buf.buf);
oe = find_object(sha1);
p += 40;
- if (*p != ' ')
+ if (*p++ != ' ')
die("Missing space after SHA1: %s", command_buf.buf);
}
- assert(*p == ' ');
- p++; /* skip space */
/* */
s = lookup_branch(p);
@@ -3005,6 +2999,8 @@ static struct object_entry *parse_treeish_dataref(const
char **p)
die("Invalid dataref: %s", command_buf.buf);
e = find_object(sha1);
*p += 40;
+ if (*(*p)++ != ' ')
+ die("Missing space after tree-ish: %s",
command_buf.buf);
}
while (!e || e->type != OBJ_TREE)
@@ -3056,8 +3052,6 @@ static void parse_ls(const char *p, struct branch *b)
if (!is_null_sha1(root->versions[1].sha1))
root->versions[1].mode = S_IFDIR;
load_tree(root);
- if (*p++ != ' ')
- die("Missing space after tree-ish: %s",
command_buf.buf);
}
if (*p == '"') {
static struct strbuf uq = STRBUF_INIT;
--
2.0.0.566.gfe3e6b2
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] alloc.c: remove alloc_raw_commit_node() function
In order to encapsulate the setting of the unique commit index, commit
969eba63 ("commit: push commit_index update into alloc_commit_node",
10-06-2014) introduced a (logically private) intermediary allocator
function. However, this function (alloc_raw_commit_node()) was declared
as a public function, which undermines its entire purpose.
Remove the alloc_raw_commit_node() function and inline its code into
the (public) alloc_commit_node() function.
Noticed by sparse ("symbol 'alloc_raw_commit_node' was not declared.
Should it be static?").
Signed-off-by: Ramsay Jones
---
Hi Jeff,
I noticed this while it was still in 'pu', but got distracted and
didn't send this in time ... sorry about that! :(
My first attempt at fixing this involved changing the DEFINE_ALLOCATOR
macro to include a 'scope' parameter so that I could declare the
raw_commit allocator 'static'. Unfortunately, I could not pass the
extern keyword as the scope parameter to all the other allocators,
because that made sparse even more upset - you can't use extern on
a function _definition_. That meant passing an empty argument (or a
comment token) to the scope parameter. This worked for gcc 4.8.2 and
clang 3.4, but I was a little concerned about portability.
This seems a better solution to me. Having said that ... as I'm typing
this I realized that I could have removed the 'commit_count' variable
and used 'commit_allocs' to set c->index instead! :-P Oh well ...
ATB,
Ramsay Jones
alloc.c | 22 ++
1 file changed, 18 insertions(+), 4 deletions(-)
diff --git a/alloc.c b/alloc.c
index eb22a45..124d710 100644
--- a/alloc.c
+++ b/alloc.c
@@ -47,16 +47,30 @@ union any_object {
DEFINE_ALLOCATOR(blob, struct blob)
DEFINE_ALLOCATOR(tree, struct tree)
-DEFINE_ALLOCATOR(raw_commit, struct commit)
DEFINE_ALLOCATOR(tag, struct tag)
DEFINE_ALLOCATOR(object, union any_object)
+static unsigned int commit_allocs;
+
void *alloc_commit_node(void)
{
static int commit_count;
- struct commit *c = alloc_raw_commit_node();
+ static int nr;
+ static struct commit *block;
+ struct commit *c;
+ void *ret;
+
+ if (!nr) {
+ nr = BLOCKING;
+ block = xmalloc(BLOCKING * sizeof(struct commit));
+ }
+ nr--;
+ commit_allocs++;
+ ret = block++;
+ memset(ret, 0, sizeof(struct commit));
+ c = (struct commit *) ret;
c->index = commit_count++;
- return c;
+ return ret;
}
static void report(const char *name, unsigned int count, size_t size)
@@ -72,7 +86,7 @@ void alloc_report(void)
{
REPORT(blob, struct blob);
REPORT(tree, struct tree);
- REPORT(raw_commit, struct commit);
+ REPORT(commit, struct commit);
REPORT(tag, struct tag);
REPORT(object, union any_object);
}
--
2.0.0
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 14/16] fetch-pack: refactor parsing in get_ack
There are several uses of the magic number "line+45" when
parsing ACK lines from the server, and it's rather unclear
why 45 is the correct number. We can make this more clear by
keeping a running pointer as we parse, using skip_prefix to
jump past the first "ACK ", then adding 40 to jump past
get_sha1_hex (which is still magical, but hopefully 40 is
less magical to readers of git code).
Note that this actually puts us at line+44. The original
required some character between the sha1 and further ACK
flags (it is supposed to be a space, but we never enforced
that). We start our search for flags at line+44, which
meanas we are slightly more liberal than the old code.
Signed-off-by: Jeff King
---
I actually think we could tighten this even more and drop the strstrs,
too, like:
arg += 40;
if (*arg++ != ' ')
return ACK;
if (!strcmp(arg, "continue"))
return ACK_continue;
and so on. But I wasn't sure if there was a reason for the use of
strstr. According to pack-protocol.txt, we would only get one at a time,
and always with a single space between them.
fetch-pack.c | 15 +--
1 file changed, 9 insertions(+), 6 deletions(-)
diff --git a/fetch-pack.c b/fetch-pack.c
index 3de3bd5..72ec520 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -189,20 +189,23 @@ static enum ack_type get_ack(int fd, unsigned char
*result_sha1)
{
int len;
char *line = packet_read_line(fd, &len);
+ const char *arg;
if (!len)
die("git fetch-pack: expected ACK/NAK, got EOF");
if (!strcmp(line, "NAK"))
return NAK;
- if (starts_with(line, "ACK ")) {
- if (!get_sha1_hex(line+4, result_sha1)) {
- if (len < 45)
+ if (skip_prefix(line, "ACK ", &arg)) {
+ if (!get_sha1_hex(arg, result_sha1)) {
+ arg += 40;
+ len -= arg - line;
+ if (len < 1)
return ACK;
- if (strstr(line+45, "continue"))
+ if (strstr(arg, "continue"))
return ACK_continue;
- if (strstr(line+45, "common"))
+ if (strstr(arg, "common"))
return ACK_common;
- if (strstr(line+45, "ready"))
+ if (strstr(arg, "ready"))
return ACK_ready;
return ACK;
}
--
2.0.0.566.gfe3e6b2
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 15/16] git: avoid magic number with skip_prefix
After handling options, any leftover arguments should be
commands. However, we pass through "--help" and "--version",
so that we convert them into "git help" and "git version"
respectively.
This is a straightforward use of skip_prefix to avoid a
magic number, but while we are there, it is worth adding a
comment to explain this otherwise confusing behavior.
Signed-off-by: Jeff King
---
Another option would be to teach handle_options to convert "--version"
into "version" itself. That's more disruptive, but I think would be
less confusing.
git.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/git.c b/git.c
index b2bb09e..1537f00 100644
--- a/git.c
+++ b/git.c
@@ -588,8 +588,8 @@ int main(int argc, char **av)
argc--;
handle_options(&argv, &argc, NULL);
if (argc > 0) {
- if (starts_with(argv[0], "--"))
- argv[0] += 2;
+ /* translate --help and --version into commands */
+ skip_prefix(argv[0], "--", &argv[0]);
} else {
/* The user didn't specify a command; give them help */
commit_pager_choice();
--
2.0.0.566.gfe3e6b2
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 16/16] use skip_prefix to avoid repeated calculations
In some cases, we use starts_with to check for a prefix, and
then use an already-calculated prefix length to advance a
pointer past the prefix. There are no magic numbers or
duplicated strings here, but we can still make the code
simpler and more obvious by using skip_prefix.
Signed-off-by: Jeff King
---
help.c | 11 +--
http.c | 3 +--
2 files changed, 6 insertions(+), 8 deletions(-)
diff --git a/help.c b/help.c
index 79e8007..f31f29a 100644
--- a/help.c
+++ b/help.c
@@ -129,7 +129,6 @@ static void list_commands_in_dir(struct cmdnames *cmds,
const char *path,
const char *prefix)
{
- int prefix_len;
DIR *dir = opendir(path);
struct dirent *de;
struct strbuf buf = STRBUF_INIT;
@@ -139,15 +138,15 @@ static void list_commands_in_dir(struct cmdnames *cmds,
return;
if (!prefix)
prefix = "git-";
- prefix_len = strlen(prefix);
strbuf_addf(&buf, "%s/", path);
len = buf.len;
while ((de = readdir(dir)) != NULL) {
+ const char *ent;
int entlen;
- if (!starts_with(de->d_name, prefix))
+ if (!skip_prefix(de->d_name, prefix, &ent))
continue;
strbuf_setlen(&buf, len);
@@ -155,11 +154,11 @@ static void list_commands_in_dir(struct cmdnames *cmds,
if (!is_executable(buf.buf))
continue;
- entlen = strlen(de->d_name) - prefix_len;
- if (has_extension(de->d_name, ".exe"))
+ entlen = strlen(ent);
+ if (has_extension(ent, ".exe"))
entlen -= 4;
- add_cmdname(cmds, de->d_name + prefix_len, entlen);
+ add_cmdname(cmds, ent, entlen);
}
closedir(dir);
strbuf_release(&buf);
diff --git a/http.c b/http.c
index 2b4f6a3..f04621e 100644
--- a/http.c
+++ b/http.c
@@ -1087,11 +1087,10 @@ static int update_url_from_redirect(struct strbuf *base,
if (!strcmp(asked, got->buf))
return 0;
- if (!starts_with(asked, base->buf))
+ if (!skip_prefix(asked, base->buf, &tail))
die("BUG: update_url_from_redirect: %s is not a superset of %s",
asked, base->buf);
- tail = asked + base->len;
tail_len = strlen(tail);
if (got->len < tail_len ||
--
2.0.0.566.gfe3e6b2
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 0/2] dropping manual malloc calculations
While working on the skip_prefix series, I ended up grepping for:
+ strlen("
to find spots in need of skip_prefix. Of course, it turns up many other
nasty ad-hoc calculations. Here's a short series that addresses a few.
There are many more, but hopefully the first patch provides a tool that
can help us in the future.
[1/2]: strbuf: add xstrdup_fmt helper
[2/2]: use xstrdup_fmt in favor of manual size calculations
-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/2] use xstrdup_fmt in favor of manual size calculations
In many parts of the code, we do an ugly and error-prone
malloc like:
const char *fmt = "something %s";
buf = xmalloc(strlen(foo) + 10 + 1);
sprintf(buf, fmt, foo);
This makes the code brittle, and if we ever get the
allocation wrong, is a potential heap overflow. Let's
instead favor xstrdup_fmt, which handles the allocation
automatically, and makes the code shorter and more readable.
Signed-off-by: Jeff King
---
remote.c | 6 +-
unpack-trees.c | 17 ++---
2 files changed, 7 insertions(+), 16 deletions(-)
diff --git a/remote.c b/remote.c
index 0e9459c..792dcee 100644
--- a/remote.c
+++ b/remote.c
@@ -170,7 +170,6 @@ static struct branch *make_branch(const char *name, int len)
{
struct branch *ret;
int i;
- char *refname;
for (i = 0; i < branches_nr; i++) {
if (len ? (!strncmp(name, branches[i]->name, len) &&
@@ -186,10 +185,7 @@ static struct branch *make_branch(const char *name, int
len)
ret->name = xstrndup(name, len);
else
ret->name = xstrdup(name);
- refname = xmalloc(strlen(name) + strlen("refs/heads/") + 1);
- strcpy(refname, "refs/heads/");
- strcpy(refname + strlen("refs/heads/"), ret->name);
- ret->refname = refname;
+ ret->refname = xstrdup_fmt("refs/heads/%s", ret->name);
return ret;
}
diff --git a/unpack-trees.c b/unpack-trees.c
index 97fc995..dd1e06e 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -56,17 +56,15 @@ void setup_unpack_trees_porcelain(struct
unpack_trees_options *opts,
int i;
const char **msgs = opts->msgs;
const char *msg;
- char *tmp;
const char *cmd2 = strcmp(cmd, "checkout") ? cmd : "switch branches";
+
if (advice_commit_before_merge)
msg = "Your local changes to the following files would be
overwritten by %s:\n%%s"
"Please, commit your changes or stash them before you
can %s.";
else
msg = "Your local changes to the following files would be
overwritten by %s:\n%%s";
- tmp = xmalloc(strlen(msg) + strlen(cmd) + strlen(cmd2) - 2);
- sprintf(tmp, msg, cmd, cmd2);
- msgs[ERROR_WOULD_OVERWRITE] = tmp;
- msgs[ERROR_NOT_UPTODATE_FILE] = tmp;
+ msgs[ERROR_WOULD_OVERWRITE] = msgs[ERROR_NOT_UPTODATE_FILE] =
+ xstrdup_fmt(msg, cmd, cmd2);
msgs[ERROR_NOT_UPTODATE_DIR] =
"Updating the following directories would lose untracked files
in it:\n%s";
@@ -76,12 +74,9 @@ void setup_unpack_trees_porcelain(struct
unpack_trees_options *opts,
"Please move or remove them before you can %s.";
else
msg = "The following untracked working tree files would be %s
by %s:\n%%s";
- tmp = xmalloc(strlen(msg) + strlen(cmd) + strlen("removed") +
strlen(cmd2) - 4);
- sprintf(tmp, msg, "removed", cmd, cmd2);
- msgs[ERROR_WOULD_LOSE_UNTRACKED_REMOVED] = tmp;
- tmp = xmalloc(strlen(msg) + strlen(cmd) + strlen("overwritten") +
strlen(cmd2) - 4);
- sprintf(tmp, msg, "overwritten", cmd, cmd2);
- msgs[ERROR_WOULD_LOSE_UNTRACKED_OVERWRITTEN] = tmp;
+
+ msgs[ERROR_WOULD_LOSE_UNTRACKED_REMOVED] = xstrdup_fmt(msg, "removed",
cmd, cmd2);
+ msgs[ERROR_WOULD_LOSE_UNTRACKED_OVERWRITTEN] = xstrdup_fmt(msg,
"overwritten", cmd, cmd2);
/*
* Special case: ERROR_BIND_OVERLAP refers to a pair of paths, we
--
2.0.0.566.gfe3e6b2
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/2] strbuf: add xstrdup_fmt helper
You can use a strbuf to build up a string from parts, and
then detach it. In the general case, you might use multiple
strbuf_add* functions to do the building. However, in many
cases, a single strbuf_addf is sufficient, and we end up
with:
struct strbuf buf = STRBUF_INIT;
...
strbuf_addf(&buf, fmt, some, args);
str = strbuf_detach(&buf, NULL);
We can make this much more readable (and avoid introducing
an extra variable, which can clutter the code) by
introducing a convenience function:
str = xstrdup_fmt(fmt, some, args);
Signed-off-by: Jeff King
---
I'm open to suggestions on the name. This really is the same thing
conceptually as the GNU asprintf(), but the interface is different (that
function takes a pointer-to-pointer as an out-parameter, and returns the
number of characters return).
strbuf.c | 19 +++
strbuf.h | 9 +
2 files changed, 28 insertions(+)
diff --git a/strbuf.c b/strbuf.c
index ac62982..6674d74 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -600,3 +600,22 @@ char *xstrdup_tolower(const char *string)
result[i] = '\0';
return result;
}
+
+char *xstrdup_vfmt(const char *fmt, va_list ap)
+{
+ struct strbuf buf = STRBUF_INIT;
+ strbuf_vaddf(&buf, fmt, ap);
+ return strbuf_detach(&buf, NULL);
+}
+
+char *xstrdup_fmt(const char *fmt, ...)
+{
+ va_list ap;
+ char *ret;
+
+ va_start(ap, fmt);
+ ret = xstrdup_vfmt(fmt, ap);
+ va_end(ap);
+
+ return ret;
+}
diff --git a/strbuf.h b/strbuf.h
index e9ad03e..61818f9 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -187,4 +187,13 @@ extern int fprintf_ln(FILE *fp, const char *fmt, ...);
char *xstrdup_tolower(const char *);
+/*
+ * Create a newly allocated string using printf format. You can do this easily
+ * with a strbuf, but this provides a shortcut to save a few lines.
+ */
+__attribute__((format (printf, 1, 0)))
+char *xstrdup_vfmt(const char *fmt, va_list ap);
+__attribute__((format (printf, 1, 2)))
+char *xstrdup_fmt(const char *fmt, ...);
+
#endif /* STRBUF_H */
--
2.0.0.566.gfe3e6b2
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] alloc.c: remove alloc_raw_commit_node() function
On Wed, Jun 18, 2014 at 08:52:46PM +0100, Ramsay Jones wrote:
> In order to encapsulate the setting of the unique commit index, commit
> 969eba63 ("commit: push commit_index update into alloc_commit_node",
> 10-06-2014) introduced a (logically private) intermediary allocator
> function. However, this function (alloc_raw_commit_node()) was declared
> as a public function, which undermines its entire purpose.
>
> Remove the alloc_raw_commit_node() function and inline its code into
> the (public) alloc_commit_node() function.
>
> Noticed by sparse ("symbol 'alloc_raw_commit_node' was not declared.
> Should it be static?").
>
> Signed-off-by: Ramsay Jones
> ---
>
> Hi Jeff,
>
> I noticed this while it was still in 'pu', but got distracted and
> didn't send this in time ... sorry about that! :(
Yeah, I noticed it while writing the patch but decided it wasn't worth
the trouble to deal with (since after all, it's not advertised to any
callers, the very thing that sparse is complaining about. :) ).
I don't mind fixing it, though I really don't like repeating the
contents of DEFINE_ALLOCATOR. I know it hasn't changed in a while, but
it just feels wrong.
> My first attempt at fixing this involved changing the DEFINE_ALLOCATOR
> macro to include a 'scope' parameter so that I could declare the
> raw_commit allocator 'static'. Unfortunately, I could not pass the
> extern keyword as the scope parameter to all the other allocators,
> because that made sparse even more upset - you can't use extern on
> a function _definition_. That meant passing an empty argument (or a
> comment token) to the scope parameter. This worked for gcc 4.8.2 and
> clang 3.4, but I was a little concerned about portability.
Yeah, passing an empty argument was my first thought, but I don't know
offhand if there are portability concerns.
You could also have DEFINE_ALLOCATOR just fill in the body, and do:
struct blob *alloc_blob_node(void)
{
DEFINE_ALLOCATOR(struct blob);
}
or similar.
-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Importing history to show up in a blame
The only way I have found so far to do this is to merge the branch on to a tmp branch and then back! The only other way I found seems real bad: http://stackoverflow.com/questions/16473363/tell-git-blame-to-use-imported-histo ry And the way below does not work if there are edits already on master (that is non-identical files). Is there a better way? -Jason jpyeron@black /tmp/foo $ git --version git version 1.8.4.21.g992c386 jpyeron@black /tmp/foo $ git init history Initialized empty Git repository in /tmp/foo/history/.git/ jpyeron@black /tmp/foo $ cd history jpyeron@black /tmp/foo/history $ #make source.txt and commit ... jpyeron@black /tmp/foo/history $ git checkout --orphan HISTORICAL Switched to a new branch 'HISTORICAL' jpyeron@black /tmp/foo/history $ # import each historical version and commit ... jpyeron@black /tmp/foo/history $ git log --oneline --graph && git blame source.txt && sha1sum.exe source.txt * 2889460 v6 - latest * 62e4a90 v5 * bfdf128 v4 * 416d32a v3 * 241a7dc v2 * 7ef41ad v1 ^7ef41ad (Jason Pyeron 2014-06-18 13:47:57 -0400 1) 1 the 241a7dc5 (Jason Pyeron 2014-06-18 13:48:53 -0400 2) 2 quick 62e4a90e (Jason Pyeron 2014-06-18 13:50:44 -0400 3) 3 brown bfdf1285 (Jason Pyeron 2014-06-18 13:49:55 -0400 4) 4 fox 416d32a4 (Jason Pyeron 2014-06-18 13:49:34 -0400 5) 5 jumped 416d32a4 (Jason Pyeron 2014-06-18 13:49:34 -0400 6) 6 over 416d32a4 (Jason Pyeron 2014-06-18 13:49:34 -0400 7) 7 the 416d32a4 (Jason Pyeron 2014-06-18 13:49:34 -0400 8) 8 lazy 28894602 (Jason Pyeron 2014-06-18 13:51:03 -0400 9) 9 dogs cd49f005ab64dac61b2d420a7903fcd02b5f373f *source.txt jpyeron@black /tmp/foo/history $ git checkout master Switched to branch 'master' jpyeron@black /tmp/foo/history $ git log --oneline --graph && git blame source.txt && sha1sum.exe source.txt * f25b132 import of latest source ^f25b132 (Jason Pyeron 2014-06-18 13:45:56 -0400 1) 1 the ^f25b132 (Jason Pyeron 2014-06-18 13:45:56 -0400 2) 2 quick ^f25b132 (Jason Pyeron 2014-06-18 13:45:56 -0400 3) 3 brown ^f25b132 (Jason Pyeron 2014-06-18 13:45:56 -0400 4) 4 fox ^f25b132 (Jason Pyeron 2014-06-18 13:45:56 -0400 5) 5 jumped ^f25b132 (Jason Pyeron 2014-06-18 13:45:56 -0400 6) 6 over ^f25b132 (Jason Pyeron 2014-06-18 13:45:56 -0400 7) 7 the ^f25b132 (Jason Pyeron 2014-06-18 13:45:56 -0400 8) 8 lazy ^f25b132 (Jason Pyeron 2014-06-18 13:45:56 -0400 9) 9 dogs cd49f005ab64dac61b2d420a7903fcd02b5f373f *source.txt jpyeron@black /tmp/foo/history $ git checkout HISTORICAL Switched to branch 'HISTORICAL' jpyeron@black /tmp/foo/history $ git branch historymerge jpyeron@black /tmp/foo/history $ git checkout historymerge Switched to branch 'historymerge' jpyeron@black /tmp/foo/history $ git merge master Merge made by the 'recursive' strategy. jpyeron@black /tmp/foo/history $ git checkout master Switched to branch 'master' jpyeron@black /tmp/foo/history $ git merge historymerge Updating f25b132..5a9408a Fast-forward jpyeron@black /tmp/foo/history $ git branch -d historymerge Deleted branch historymerge (was 5a9408a). jpyeron@black /tmp/foo/history $ git log --oneline --graph && git blame source.txt && sha1sum.exe source.txt * 5a9408a Merge branch 'master' into historymerge |\ | * f25b132 import of latest source * 2889460 v6 - latest * 62e4a90 v5 * bfdf128 v4 * 416d32a v3 * 241a7dc v2 * 7ef41ad v1 ^7ef41ad (Jason Pyeron 2014-06-18 13:47:57 -0400 1) 1 the 241a7dc5 (Jason Pyeron 2014-06-18 13:48:53 -0400 2) 2 quick 62e4a90e (Jason Pyeron 2014-06-18 13:50:44 -0400 3) 3 brown bfdf1285 (Jason Pyeron 2014-06-18 13:49:55 -0400 4) 4 fox 416d32a4 (Jason Pyeron 2014-06-18 13:49:34 -0400 5) 5 jumped 416d32a4 (Jason Pyeron 2014-06-18 13:49:34 -0400 6) 6 over 416d32a4 (Jason Pyeron 2014-06-18 13:49:34 -0400 7) 7 the 416d32a4 (Jason Pyeron 2014-06-18 13:49:34 -0400 8) 8 lazy 28894602 (Jason Pyeron 2014-06-18 13:51:03 -0400 9) 9 dogs cd49f005ab64dac61b2d420a7903fcd02b5f373f *source.txt -- -=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=- - - - Jason Pyeron PD Inc. http://www.pdinc.us - - Principal Consultant 10 West 24th Street #100- - +1 (443) 269-1555 x333Baltimore, Maryland 21218 - - - -=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=- This message is copyright PD Inc, subject to license 20080407P00. history.bundle Description: Binary data
[PATCH 0/7] cleaning up determine_author_info
This is another function I ran across in today's cleanups. The memory leak in it has bugged me for a while (even though it's really not a big deal in practice). So this is mostly minor cleanups, but I did find a bug in the commit parser. [1/7]: commit: provide a function to find a header in a buffer [2/7]: record_author_info: fix memory leak on malformed commit [3/7]: record_author_info: use find_commit_header [4/7]: ident_split: store begin/end pairs on their own struct [5/7]: use strbufs in date functions [6/7]: determine_author_info: reuse parsing functions [7/7]: determine_author_info: stop leaking name/email I built it on top of my commit-slab topic, as otherwise you get some textual conflicts in determine_author_info. But I notice that Junio's jk/commit-buffer-length is based on an older master; applying there produces some other minor textual conflicts. I can build it on whatever is convenient and handle the conflicts myself. But if jk/commit-buffer-length is set to graduate soon (as it is marked in WC), I can just hold onto this until then. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/7] commit: provide a function to find a header in a buffer
Usually when we parse a commit, we read it line by line and
handle each header in a single pass (e.g., in parse_commit
and parse_commit_header). Sometimes, however, we only care
about extracting a single header. Code in this situation is
stuck doing an ad-hoc parse of the commit buffer.
Let's provide a reusable function to locate a header within
the commit. The code is modeled after pretty.c's
get_header, which is used to extract the encoding.
Since some callers may not have the "struct commit" to go
along with the buffer, we drop that parameter. The only
thing lost is a warning for truncated commits, but that's
OK. This shouldn't happen in practice, and even if it does,
there's no particular reason that this function needs to
complain about it. It either finds the header it was asked
for, or it doesn't (and in the latter case, the caller can
complain).
Signed-off-by: Jeff King
---
As noted in the comments below, I punted on extracting
multi-line headers like mergetag, because this function only
returns a pointer. It might make sense to wrap it with a
function to pull out a copy of the header, with continuation
lines connected to each other (that's almost what the static
get_header does, but I didn't make it public exactly because
it doesn't handle continuation lines).
That might be a more natural interface than
read_commit_extra_header_lines, which pulls out all headers
except ones that are specifically excluded. I haven't looked
closely enough. But in either case, that could easily come
on top of this.
commit.c | 23 +++
commit.h | 11 +++
pretty.c | 33 ++---
3 files changed, 40 insertions(+), 27 deletions(-)
diff --git a/commit.c b/commit.c
index 11106fb..d04b525 100644
--- a/commit.c
+++ b/commit.c
@@ -1652,3 +1652,26 @@ void print_commit_list(struct commit_list *list,
printf(format, sha1_to_hex(list->item->object.sha1));
}
}
+
+const char *find_commit_header(const char *msg, const char *key, size_t
*out_len)
+{
+ int key_len = strlen(key);
+ const char *line = msg;
+
+ while (line) {
+ const char *eol = strchrnul(line, '\n'), *next;
+
+ if (line == eol)
+ return NULL;
+ next = *eol ? eol + 1 : NULL;
+
+ if (eol - line > key_len &&
+ !strncmp(line, key, key_len) &&
+ line[key_len] == ' ') {
+ *out_len = eol - line - key_len - 1;
+ return line + key_len + 1;
+ }
+ line = next;
+ }
+ return NULL;
+}
diff --git a/commit.h b/commit.h
index 61559a9..7c766e9 100644
--- a/commit.h
+++ b/commit.h
@@ -312,6 +312,17 @@ extern struct commit_extra_header
*read_commit_extra_headers(struct commit *, co
extern void free_commit_extra_headers(struct commit_extra_header *extra);
+/*
+ * Search the commit object contents given by "msg" for the header "key".
+ * Returns a pointer to the start of the header contents, or NULL. The length
+ * of the header, up to the first newline, is returned via out_len.
+ *
+ * Note that some headers (like mergetag) may be multi-line. It is the caller's
+ * responsibility to parse further in this case!
+ */
+extern const char *find_commit_header(const char *msg, const char *key,
+ size_t *out_len);
+
struct merge_remote_desc {
struct object *obj; /* the named object, could be a tag */
const char *name;
diff --git a/pretty.c b/pretty.c
index cc5b45d..6081750 100644
--- a/pretty.c
+++ b/pretty.c
@@ -548,31 +548,11 @@ static void add_merge_info(const struct
pretty_print_context *pp,
strbuf_addch(sb, '\n');
}
-static char *get_header(const struct commit *commit, const char *msg,
- const char *key)
+static char *get_header(const char *msg, const char *key)
{
- int key_len = strlen(key);
- const char *line = msg;
-
- while (line) {
- const char *eol = strchrnul(line, '\n'), *next;
-
- if (line == eol)
- return NULL;
- if (!*eol) {
- warning("malformed commit (header is missing newline):
%s",
- sha1_to_hex(commit->object.sha1));
- next = NULL;
- } else
- next = eol + 1;
- if (eol - line > key_len &&
- !strncmp(line, key, key_len) &&
- line[key_len] == ' ') {
- return xmemdupz(line + key_len + 1, eol - line -
key_len - 1);
- }
- line = next;
- }
- return NULL;
+ size_t len;
+ const char *v = find_commit_header(msg, key, &len);
+ return v ? xmemdupz(v, len) : NULL;
}
static char *replace_encoding_header(char *buf, const char *encoding)
@@ -618,11 +598,10 @@ const char *logmsg_re
[PATCH 2/7] record_author_info: fix memory leak on malformed commit
If we hit the end-of-header without finding an "author"
line, we just return from the function. We should jump to
the fail_exit path to clean up the buffer that we may have
allocated.
Signed-off-by: Jeff King
---
commit.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/commit.c b/commit.c
index d04b525..0c40cfa 100644
--- a/commit.c
+++ b/commit.c
@@ -617,7 +617,7 @@ static void record_author_date(struct author_date_slab
*author_date,
ident_line = skip_prefix(buf, "author ");
if (!ident_line) {
if (!line_end[0] || line_end[1] == '\n')
- return; /* end of header */
+ goto fail_exit; /* end of header */
continue;
}
if (split_ident_line(&ident,
--
2.0.0.566.gfe3e6b2
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 3/7] record_author_info: use find_commit_header
This saves us some manual parsing and makes the code more
readable.
Signed-off-by: Jeff King
---
I suspect there are other sites which could use this helper, too; I
didn't do an exhaustive search.
commit.c | 23 ---
1 file changed, 8 insertions(+), 15 deletions(-)
diff --git a/commit.c b/commit.c
index 0c40cfa..c33431c 100644
--- a/commit.c
+++ b/commit.c
@@ -606,26 +606,19 @@ define_commit_slab(author_date_slab, unsigned long);
static void record_author_date(struct author_date_slab *author_date,
struct commit *commit)
{
- const char *buf, *line_end, *ident_line;
const char *buffer = get_commit_buffer(commit, NULL);
struct ident_split ident;
+ const char *ident_line;
+ size_t ident_len;
char *date_end;
unsigned long date;
- for (buf = buffer; buf; buf = line_end + 1) {
- line_end = strchrnul(buf, '\n');
- ident_line = skip_prefix(buf, "author ");
- if (!ident_line) {
- if (!line_end[0] || line_end[1] == '\n')
- goto fail_exit; /* end of header */
- continue;
- }
- if (split_ident_line(&ident,
-ident_line, line_end - ident_line) ||
- !ident.date_begin || !ident.date_end)
- goto fail_exit; /* malformed "author" line */
- break;
- }
+ ident_line = find_commit_header(buffer, "author", &ident_len);
+ if (!ident_line)
+ goto fail_exit; /* no author line */
+ if (split_ident_line(&ident, ident_line, ident_len) ||
+ !ident.date_begin || !ident.date_end)
+ goto fail_exit; /* malformed "author" line */
date = strtoul(ident.date_begin, &date_end, 10);
if (date_end != ident.date_end)
--
2.0.0.566.gfe3e6b2
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v18 07/48] lockfile.c: make lock_file return a meaningful errno on failurei
There's a typo in the subject line of this commit.
Michael
On 06/17/2014 05:53 PM, Ronnie Sahlberg wrote:
> Making errno when returning from lock_file() meaningful, which should
> fix
>
> * an existing almost-bug in lock_ref_sha1_basic where it assumes
>errno==ENOENT is meaningful and could waste some work on retries
>
> * an existing bug in repack_without_refs where it prints
>strerror(errno) and picks advice based on errno, despite errno
>potentially being zero and potentially having been clobbered by
>that point
>
> Signed-off-by: Ronnie Sahlberg
> ---
> lockfile.c | 17 -
> refs.c | 1 +
> refs.h | 1 +
> 3 files changed, 14 insertions(+), 5 deletions(-)
>
> diff --git a/lockfile.c b/lockfile.c
> index 464031b..a921d77 100644
> --- a/lockfile.c
> +++ b/lockfile.c
> @@ -121,7 +121,7 @@ static char *resolve_symlink(char *p, size_t s)
> return p;
> }
>
> -
> +/* Make sure errno contains a meaningful value on error */
> static int lock_file(struct lock_file *lk, const char *path, int flags)
> {
> /*
> @@ -130,8 +130,10 @@ static int lock_file(struct lock_file *lk, const char
> *path, int flags)
>*/
> static const size_t max_path_len = sizeof(lk->filename) - 5;
>
> - if (strlen(path) >= max_path_len)
> + if (strlen(path) >= max_path_len) {
> + errno = ENAMETOOLONG;
> return -1;
> + }
> strcpy(lk->filename, path);
> if (!(flags & LOCK_NODEREF))
> resolve_symlink(lk->filename, max_path_len);
> @@ -148,9 +150,13 @@ static int lock_file(struct lock_file *lk, const char
> *path, int flags)
> lock_file_list = lk;
> lk->on_list = 1;
> }
> - if (adjust_shared_perm(lk->filename))
> - return error("cannot fix permission bits on %s",
> - lk->filename);
> + if (adjust_shared_perm(lk->filename)) {
> + int save_errno = errno;
> + error("cannot fix permission bits on %s",
> + lk->filename);
> + errno = save_errno;
> + return -1;
> + }
> }
> else
> lk->filename[0] = 0;
> @@ -188,6 +194,7 @@ NORETURN void unable_to_lock_index_die(const char *path,
> int err)
> die("%s", buf.buf);
> }
>
> +/* This should return a meaningful errno on failure */
> int hold_lock_file_for_update(struct lock_file *lk, const char *path, int
> flags)
> {
> int fd = lock_file(lk, path, flags);
> diff --git a/refs.c b/refs.c
> index db05602..e9d53e4 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -2212,6 +2212,7 @@ static int write_packed_entry_fn(struct ref_entry
> *entry, void *cb_data)
> return 0;
> }
>
> +/* This should return a meaningful errno on failure */
> int lock_packed_refs(int flags)
> {
> struct packed_ref_cache *packed_ref_cache;
> diff --git a/refs.h b/refs.h
> index 09d3564..64f25d9 100644
> --- a/refs.h
> +++ b/refs.h
> @@ -82,6 +82,7 @@ extern void warn_dangling_symrefs(FILE *fp, const char
> *msg_fmt, const struct st
> /*
> * Lock the packed-refs file for writing. Flags is passed to
> * hold_lock_file_for_update(). Return 0 on success.
> + * Errno is set to something meaningful on error.
> */
> extern int lock_packed_refs(int flags);
>
>
--
Michael Haggerty
mhag...@alum.mit.edu
http://softwareswirl.blogspot.com/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 4/7] ident_split: store begin/end pairs on their own struct
When we parse an ident line, we end up with several fields,
each with a begin/end pointer into the buffer, like:
const char *name_begin;
const char *name_end;
There is nothing except the field names to indicate that
they are paired. This makes it annoying to write helper
functions for dealing with the sub-fields, as you have to
pass both sides. Instead, let's move them into a single
struct "name", with fields "begin" and "end". This will be
stored identically, but can be passed as a unit.
We have to do a mechanical update of "s/_/./" at each point
of use, but other than that, the fields should behave
identically.
Signed-off-by: Jeff King
---
Suggestions welcome on the name "pointer_pair".
While writing this series, I also noticed that it would be more
convenient to have a pointer/len combination rather than two pointers.
You can convert between them, of course, but I found I was always
converting the other way.
I left it this way because it makes the mass-update mechanical (and
because now that I can pass the pair as a unit, I don't have to write
the same "ident->name_begin, ident->name_end - ident->name_begin" pair
over and over).
builtin/blame.c | 16 +++---
builtin/check-mailmap.c | 8 +++
builtin/commit.c| 26 +++---
builtin/shortlog.c | 8 +++
cache.h | 17 ---
commit.c| 6 +++---
ident.c | 57 +++--
log-tree.c | 2 +-
pretty.c| 36 +++
revision.c | 12 +--
10 files changed, 93 insertions(+), 95 deletions(-)
diff --git a/builtin/blame.c b/builtin/blame.c
index 53f43ab..6e6dddb 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -1599,19 +1599,19 @@ static void get_ac_line(const char *inbuf, const char
*what,
return;
}
- namelen = ident.name_end - ident.name_begin;
- namebuf = ident.name_begin;
+ namelen = ident.name.end - ident.name.begin;
+ namebuf = ident.name.begin;
- maillen = ident.mail_end - ident.mail_begin;
- mailbuf = ident.mail_begin;
+ maillen = ident.mail.end - ident.mail.begin;
+ mailbuf = ident.mail.begin;
- if (ident.date_begin && ident.date_end)
- *time = strtoul(ident.date_begin, NULL, 10);
+ if (ident.date.begin && ident.date.end)
+ *time = strtoul(ident.date.begin, NULL, 10);
else
*time = 0;
- if (ident.tz_begin && ident.tz_end)
- strbuf_add(tz, ident.tz_begin, ident.tz_end - ident.tz_begin);
+ if (ident.tz.begin && ident.tz.end)
+ strbuf_add(tz, ident.tz.begin, ident.tz.end - ident.tz.begin);
else
strbuf_addstr(tz, "(unknown)");
diff --git a/builtin/check-mailmap.c b/builtin/check-mailmap.c
index 8f4d809..65dcbc6 100644
--- a/builtin/check-mailmap.c
+++ b/builtin/check-mailmap.c
@@ -23,10 +23,10 @@ static void check_mailmap(struct string_list *mailmap,
const char *contact)
if (split_ident_line(&ident, contact, strlen(contact)))
die(_("unable to parse contact: %s"), contact);
- name = ident.name_begin;
- namelen = ident.name_end - ident.name_begin;
- mail = ident.mail_begin;
- maillen = ident.mail_end - ident.mail_begin;
+ name = ident.name.begin;
+ namelen = ident.name.end - ident.name.begin;
+ mail = ident.mail.begin;
+ maillen = ident.mail.end - ident.mail.begin;
map_user(mailmap, &mail, &maillen, &name, &namelen);
diff --git a/builtin/commit.c b/builtin/commit.c
index 84cec9a..047cc76 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -514,14 +514,14 @@ static void export_one(const char *var, const char *s,
const char *e, int hack)
static int sane_ident_split(struct ident_split *person)
{
- if (!person->name_begin || !person->name_end ||
- person->name_begin == person->name_end)
+ if (!person->name.begin || !person->name.end ||
+ person->name.begin == person->name.end)
return 0; /* no human readable name */
- if (!person->mail_begin || !person->mail_end ||
- person->mail_begin == person->mail_end)
+ if (!person->mail.begin || !person->mail.end ||
+ person->mail.begin == person->mail.end)
return 0; /* no usable mail */
- if (!person->date_begin || !person->date_end ||
- !person->tz_begin || !person->tz_end)
+ if (!person->date.begin || !person->date.end ||
+ !person->tz.begin || !person->tz.end)
return 0;
return 1;
}
@@ -602,9 +602,9 @@ static void determine_author_info(struct strbuf
*author_ident)
strbuf_addstr(author_ident, fmt_ident(name, email, date, IDENT_STRICT));
if (!split_ident_line(&author, author_ident->buf, author_ident->len) &&
sane_ide
[PATCH 5/7] use strbufs in date functions
Many of the date functions write into fixed-size buffers.
This is a minor pain, as we have to take special
precautions, and frequently end up copying the result into a
strbuf or heap-allocated buffer anyway (for which we
sometimes use strcpy!).
Let's instead teach parse_date, datestamp, etc to write to a
strbuf. The obvious downside is that we might need to
perform a heap allocation where we otherwise would not need
to. However, it turns out that the only two new allocations
required are:
1. In test-date.c, where we don't care about efficiency.
2. In determine_author_info, which is not performance
critical (and where the use of a strbuf will help later
refactoring).
Signed-off-by: Jeff King
---
I don't think the strcpys are a problem in practice, because we're
typically writing fixed-size output generated by parse_date. But I
didn't analyze it too deeply, so you might be able to cause shenanigans
if you can impact GIT_AUTHOR_DATE or something.
builtin/commit.c | 20 ++--
cache.h | 4 ++--
date.c | 13 +++--
fast-import.c| 20 +---
ident.c | 26 +++---
test-date.c | 10 ++
6 files changed, 45 insertions(+), 48 deletions(-)
diff --git a/builtin/commit.c b/builtin/commit.c
index 047cc76..bf770cf 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -526,19 +526,16 @@ static int sane_ident_split(struct ident_split *person)
return 1;
}
-static int parse_force_date(const char *in, char *out, int len)
+static int parse_force_date(const char *in, struct strbuf *out)
{
- if (len < 1)
- return -1;
- *out++ = '@';
- len--;
+ strbuf_addch(out, '@');
- if (parse_date(in, out, len) < 0) {
+ if (parse_date(in, out) < 0) {
int errors = 0;
unsigned long t = approxidate_careful(in, &errors);
if (errors)
return -1;
- snprintf(out, len, "%lu", t);
+ strbuf_addf(out, "%lu", t);
}
return 0;
@@ -548,7 +545,7 @@ static void determine_author_info(struct strbuf
*author_ident)
{
char *name, *email, *date;
struct ident_split author;
- char date_buf[64];
+ struct strbuf date_buf = STRBUF_INIT;
name = getenv("GIT_AUTHOR_NAME");
email = getenv("GIT_AUTHOR_EMAIL");
@@ -594,9 +591,10 @@ static void determine_author_info(struct strbuf
*author_ident)
}
if (force_date) {
- if (parse_force_date(force_date, date_buf, sizeof(date_buf)))
+ strbuf_reset(&date_buf);
+ if (parse_force_date(force_date, &date_buf))
die(_("invalid date format: %s"), force_date);
- date = date_buf;
+ date = date_buf.buf;
}
strbuf_addstr(author_ident, fmt_ident(name, email, date, IDENT_STRICT));
@@ -606,6 +604,8 @@ static void determine_author_info(struct strbuf
*author_ident)
export_one("GIT_AUTHOR_EMAIL", author.mail.begin,
author.mail.end, 0);
export_one("GIT_AUTHOR_DATE", author.date.begin, author.tz.end,
'@');
}
+
+ strbuf_release(&date_buf);
}
static void split_ident_or_die(struct ident_split *id, const struct strbuf
*buf)
diff --git a/cache.h b/cache.h
index 5255661..c7740a8 100644
--- a/cache.h
+++ b/cache.h
@@ -1023,10 +1023,10 @@ enum date_mode {
const char *show_date(unsigned long time, int timezone, enum date_mode mode);
void show_date_relative(unsigned long time, int tz, const struct timeval *now,
struct strbuf *timebuf);
-int parse_date(const char *date, char *buf, int bufsize);
+int parse_date(const char *date, struct strbuf *out);
int parse_date_basic(const char *date, unsigned long *timestamp, int *offset);
int parse_expiry_date(const char *date, unsigned long *timestamp);
-void datestamp(char *buf, int bufsize);
+void datestamp(struct strbuf *out);
#define approxidate(s) approxidate_careful((s), NULL)
unsigned long approxidate_careful(const char *, int *);
unsigned long approxidate_relative(const char *date, const struct timeval
*now);
diff --git a/date.c b/date.c
index 782de95..2c33468 100644
--- a/date.c
+++ b/date.c
@@ -605,7 +605,7 @@ static int match_tz(const char *date, int *offp)
return end - date;
}
-static int date_string(unsigned long date, int offset, char *buf, int len)
+static void date_string(unsigned long date, int offset, struct strbuf *buf)
{
int sign = '+';
@@ -613,7 +613,7 @@ static int date_string(unsigned long date, int offset, char
*buf, int len)
offset = -offset;
sign = '-';
}
- return snprintf(buf, len, "%lu %c%02d%02d", date, sign, offset / 60,
offset % 60);
+ strbuf_addf(buf, "%lu %c%02d%02d", date, sign, offset / 60, offset %
60);
}
/*
@@ -735,13 +735,14 @@ int parse_expiry_date(c
Re: [PATCH v3 01/14] refs.c make ref_transaction_create a wrapper to ref_transaction_update
Ronnie Sahlberg writes:
> Signed-off-by: Ronnie Sahlberg
> ---
> refs.c | 13 ++---
> refs.h | 7 ---
> 2 files changed, 6 insertions(+), 14 deletions(-)
>
> diff --git a/refs.c b/refs.c
> index dfbf003..a9f91ab 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -3487,23 +3487,14 @@ int ref_transaction_create(struct ref_transaction
> *transaction,
> int flags, const char *msg,
> struct strbuf *err)
> {
> - struct ref_update *update;
> -
> if (transaction->state != REF_TRANSACTION_OPEN)
> die("BUG: create called for transaction that is not open");
>
> if (!new_sha1 || is_null_sha1(new_sha1))
> die("BUG: create ref with null new_sha1");
>
> - update = add_update(transaction, refname);
> -
> - hashcpy(update->new_sha1, new_sha1);
> - hashclr(update->old_sha1);
> - update->flags = flags;
> - update->have_old = 1;
> - if (msg)
> - update->msg = xstrdup(msg);
> - return 0;
> + return ref_transaction_update(transaction, refname, new_sha1,
> + null_sha1, flags, 1, msg, err);
> }
Hmmm.
This probably logically belongs to the end of the previous series
and not necessarily tied to reflog transaction, no?
At the beginning of ref_transaction_update() there also is the same
guard on transaction->state, and having both feels somewhat iffy.
Of course it will give a wrong BUG message if we removed the check
from this function, so perhaps the code is OK as-is.
> int ref_transaction_delete(struct ref_transaction *transaction,
> diff --git a/refs.h b/refs.h
> index db463d0..495740d 100644
> --- a/refs.h
> +++ b/refs.h
> @@ -283,9 +283,10 @@ struct ref_transaction *ref_transaction_begin(struct
> strbuf *err);
> /*
> * Add a reference update to transaction. new_sha1 is the value that
> * the reference should have after the update, or zeros if it should
> - * be deleted. If have_old is true, then old_sha1 holds the value
> - * that the reference should have had before the update, or zeros if
> - * it must not have existed beforehand.
> + * be deleted. If have_old is true and old_sha is not the null_sha1
> + * then the previous value of the ref must match or the update will fail.
> + * If have_old is true and old_sha1 is the null_sha1 then the ref must not
> + * already exist and a new ref will be created with new_sha1.
> * Function returns 0 on success and non-zero on failure. A failure to update
> * means that the transaction as a whole has failed and will need to be
> * rolled back.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 6/7] determine_author_info: reuse parsing functions
Rather than parsing the header manually to find the "author"
field, and then parsing its sub-parts, let's use
find_commit_header and split_ident_line. This is shorter and
easier to read, and should do a more careful parsing job.
For example, the current parser could find the end-of-email
right-bracket across a newline (for a malformed commit), and
calculate a bogus gigantic length for the date (by using
"eol - rb").
As a bonus, this also plugs a memory leak when we pull the
date field from an existing commit (we still leak the name
and email buffers, which will be fixed in a later commit).
Signed-off-by: Jeff King
---
The large buffer comes from wrapping around the negative side of the
size_t space. In theory you could wrap far enough to get a buffer that
we can actually allocate (probably only on a 32-bit system), and then
we followup by copying "len" random bytes into it. I doubt an attacker
could get that data out of the program, though, as we then run it
through fmt_ident, which should complain if it's full of garbage.
builtin/commit.c | 61 +---
1 file changed, 32 insertions(+), 29 deletions(-)
diff --git a/builtin/commit.c b/builtin/commit.c
index bf770cf..62abee0 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -541,6 +541,16 @@ static int parse_force_date(const char *in, struct strbuf
*out)
return 0;
}
+static void strbuf_add_pair(struct strbuf *buf, const struct pointer_pair *p)
+{
+ strbuf_add(buf, p->begin, p->end - p->begin);
+}
+
+static char *xmemdupz_pair(const struct pointer_pair *p)
+{
+ return xmemdupz(p->begin, p->end - p->begin);
+}
+
static void determine_author_info(struct strbuf *author_ident)
{
char *name, *email, *date;
@@ -552,42 +562,35 @@ static void determine_author_info(struct strbuf
*author_ident)
date = getenv("GIT_AUTHOR_DATE");
if (author_message) {
- const char *a, *lb, *rb, *eol;
- size_t len;
+ struct ident_split ident;
+ unsigned long len;
+ const char *a;
- a = strstr(author_message_buffer, "\nauthor ");
+ a = find_commit_header(author_message_buffer, "author", &len);
if (!a)
- die(_("invalid commit: %s"), author_message);
-
- lb = strchrnul(a + strlen("\nauthor "), '<');
- rb = strchrnul(lb, '>');
- eol = strchrnul(rb, '\n');
- if (!*lb || !*rb || !*eol)
- die(_("invalid commit: %s"), author_message);
-
- if (lb == a + strlen("\nauthor "))
- /* \nauthor */
- name = xcalloc(1, 1);
- else
- name = xmemdupz(a + strlen("\nauthor "),
- (lb - strlen(" ") -
-(a + strlen("\nauthor ";
- email = xmemdupz(lb + strlen("<"), rb - (lb + strlen("<")));
- len = eol - (rb + strlen("> "));
- date = xmalloc(len + 2);
- *date = '@';
- memcpy(date + 1, rb + strlen("> "), len);
- date[len + 1] = '\0';
+ die(_("commit '%s' lacks author header"),
author_message);
+ if (split_ident_line(&ident, a, len) < 0)
+ die(_("commit '%s' has malformed author line"),
author_message);
+
+ name = xmemdupz_pair(&ident.name);
+ email = xmemdupz_pair(&ident.mail);
+ if (ident.date.begin) {
+ strbuf_reset(&date_buf);
+ strbuf_addch(&date_buf, '@');
+ strbuf_add_pair(&date_buf, &ident.date);
+ strbuf_addch(&date_buf, ' ');
+ strbuf_add_pair(&date_buf, &ident.tz);
+ date = date_buf.buf;
+ }
}
if (force_author) {
- const char *lb = strstr(force_author, " <");
- const char *rb = strchr(force_author, '>');
+ struct ident_split ident;
- if (!lb || !rb)
+ if (split_ident_line(&ident, force_author,
strlen(force_author)) < 0)
die(_("malformed --author parameter"));
- name = xstrndup(force_author, lb - force_author);
- email = xstrndup(lb + 2, rb - (lb + 2));
+ name = xmemdupz_pair(&ident.name);
+ email = xmemdupz_pair(&ident.mail);
}
if (force_date) {
--
2.0.0.566.gfe3e6b2
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 7/7] determine_author_info: stop leaking name/email
When we get the author name and email either from an
existing commit or from the "--author" option, we create a
copy of the strings. We cannot just free() these copies,
since the same pointers may also be pointing to getenv()
storage which we do not own.
Instead, let's treat these the same way as we do the date
buffer: keep a strbuf to be released, and point the bare
pointers at the strbuf.
Signed-off-by: Jeff King
---
builtin/commit.c | 20 +---
1 file changed, 13 insertions(+), 7 deletions(-)
diff --git a/builtin/commit.c b/builtin/commit.c
index 62abee0..72beb7f 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -546,16 +546,20 @@ static void strbuf_add_pair(struct strbuf *buf, const
struct pointer_pair *p)
strbuf_add(buf, p->begin, p->end - p->begin);
}
-static char *xmemdupz_pair(const struct pointer_pair *p)
+static char *set_pair(struct strbuf *buf, struct pointer_pair *p)
{
- return xmemdupz(p->begin, p->end - p->begin);
+ strbuf_reset(buf);
+ strbuf_add_pair(buf, p);
+ return buf->buf;
}
static void determine_author_info(struct strbuf *author_ident)
{
char *name, *email, *date;
struct ident_split author;
- struct strbuf date_buf = STRBUF_INIT;
+ struct strbuf name_buf = STRBUF_INIT,
+ mail_buf = STRBUF_INIT,
+ date_buf = STRBUF_INIT;
name = getenv("GIT_AUTHOR_NAME");
email = getenv("GIT_AUTHOR_EMAIL");
@@ -572,8 +576,8 @@ static void determine_author_info(struct strbuf
*author_ident)
if (split_ident_line(&ident, a, len) < 0)
die(_("commit '%s' has malformed author line"),
author_message);
- name = xmemdupz_pair(&ident.name);
- email = xmemdupz_pair(&ident.mail);
+ name = set_pair(&name_buf, &ident.name);
+ email = set_pair(&mail_buf, &ident.mail);
if (ident.date.begin) {
strbuf_reset(&date_buf);
strbuf_addch(&date_buf, '@');
@@ -589,8 +593,8 @@ static void determine_author_info(struct strbuf
*author_ident)
if (split_ident_line(&ident, force_author,
strlen(force_author)) < 0)
die(_("malformed --author parameter"));
- name = xmemdupz_pair(&ident.name);
- email = xmemdupz_pair(&ident.mail);
+ name = set_pair(&name_buf, &ident.name);
+ email = set_pair(&mail_buf, &ident.mail);
}
if (force_date) {
@@ -608,6 +612,8 @@ static void determine_author_info(struct strbuf
*author_ident)
export_one("GIT_AUTHOR_DATE", author.date.begin, author.tz.end,
'@');
}
+ strbuf_release(&name_buf);
+ strbuf_release(&mail_buf);
strbuf_release(&date_buf);
}
--
2.0.0.566.gfe3e6b2
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v18 07/48] lockfile.c: make lock_file return a meaningful errno on failurei
On 06/17/2014 05:53 PM, Ronnie Sahlberg wrote:
> Making errno when returning from lock_file() meaningful, which should
> fix
>
> * an existing almost-bug in lock_ref_sha1_basic where it assumes
>errno==ENOENT is meaningful and could waste some work on retries
>
> * an existing bug in repack_without_refs where it prints
>strerror(errno) and picks advice based on errno, despite errno
>potentially being zero and potentially having been clobbered by
>that point
>
> Signed-off-by: Ronnie Sahlberg
> ---
> lockfile.c | 17 -
> refs.c | 1 +
> refs.h | 1 +
> 3 files changed, 14 insertions(+), 5 deletions(-)
>
> diff --git a/lockfile.c b/lockfile.c
> index 464031b..a921d77 100644
> --- a/lockfile.c
> +++ b/lockfile.c
> @@ -121,7 +121,7 @@ static char *resolve_symlink(char *p, size_t s)
> return p;
> }
>
> -
> +/* Make sure errno contains a meaningful value on error */
> static int lock_file(struct lock_file *lk, const char *path, int flags)
> {
> /*
> @@ -130,8 +130,10 @@ static int lock_file(struct lock_file *lk, const char
> *path, int flags)
>*/
> static const size_t max_path_len = sizeof(lk->filename) - 5;
>
> - if (strlen(path) >= max_path_len)
> + if (strlen(path) >= max_path_len) {
> + errno = ENAMETOOLONG;
> return -1;
> + }
> strcpy(lk->filename, path);
> if (!(flags & LOCK_NODEREF))
> resolve_symlink(lk->filename, max_path_len);
> @@ -148,9 +150,13 @@ static int lock_file(struct lock_file *lk, const char
> *path, int flags)
> lock_file_list = lk;
> lk->on_list = 1;
> }
> - if (adjust_shared_perm(lk->filename))
> - return error("cannot fix permission bits on %s",
> - lk->filename);
> + if (adjust_shared_perm(lk->filename)) {
> + int save_errno = errno;
> + error("cannot fix permission bits on %s",
> + lk->filename);
> + errno = save_errno;
> + return -1;
> + }
Wouldn't it make sense for error() to save and restore errno instead of
scattering the save/restore code around everywhere? I saw the same type
of code about three commits later, too.
> [...]
Michael
--
Michael Haggerty
mhag...@alum.mit.edu
http://softwareswirl.blogspot.com/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 04/14] refs.c: add a new update_type field to ref_update
Ronnie Sahlberg writes:
> Add a field that describes what type of update this refers to. For now
> the only type is UPDATE_SHA1 but we will soon add more types.
>
> Signed-off-by: Ronnie Sahlberg
> ---
> refs.c | 25 +
> 1 file changed, 21 insertions(+), 4 deletions(-)
>
> diff --git a/refs.c b/refs.c
> index 4e3d4c3..4129de6 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -3374,6 +3374,10 @@ int for_each_reflog(each_ref_fn fn, void *cb_data)
> return retval;
> }
>
> +enum transaction_update_type {
> + UPDATE_SHA1 = 0,
indent with SP?
Unlike an array initialisation, e.g.
int foo[] = { 1,2,3,4,5, };
some compilers we support complain if enum definition ends with a
trailing comma.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v18 10/48] refs.c: verify_lock should set errno to something meaningful
On 06/17/2014 05:53 PM, Ronnie Sahlberg wrote:
> Making errno when returning from verify_lock() meaningful, which
> should almost but not completely fix
>
> * a bug in "git fetch"'s s_update_ref, which trusts the result of an
>errno == ENOTDIR check to detect D/F conflicts
>
> ENOTDIR makes sense as a sign that a file was in the way of a
> directory we wanted to create. Should "git fetch" also look for
> ENOTEMPTY or EEXIST to catch cases where a directory was in the way
> of a file to be created?
>
> Signed-off-by: Ronnie Sahlberg
> ---
> refs.c | 4
> refs.h | 6 +-
> 2 files changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/refs.c b/refs.c
> index 9ea519c..a48f805 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -1932,18 +1932,22 @@ int refname_match(const char *abbrev_name, const char
> *full_name)
> return 0;
> }
>
> +/* This function should make sure errno is meaningful on error */
> static struct ref_lock *verify_lock(struct ref_lock *lock,
> const unsigned char *old_sha1, int mustexist)
> {
> if (read_ref_full(lock->ref_name, lock->old_sha1, mustexist, NULL)) {
> + int save_errno = errno;
> error("Can't verify ref %s", lock->ref_name);
> unlock_ref(lock);
> + errno = save_errno;
> return NULL;
> }
> if (hashcmp(lock->old_sha1, old_sha1)) {
> error("Ref %s is at %s but expected %s", lock->ref_name,
> sha1_to_hex(lock->old_sha1), sha1_to_hex(old_sha1));
> unlock_ref(lock);
> + errno = EBUSY;
> return NULL;
> }
> return lock;
> diff --git a/refs.h b/refs.h
> index 82cc5cb..af4fcdc 100644
> --- a/refs.h
> +++ b/refs.h
> @@ -137,11 +137,15 @@ extern int ref_exists(const char *);
> */
> extern int peel_ref(const char *refname, unsigned char *sha1);
>
> -/** Locks a "refs/" ref returning the lock on success and NULL on failure.
> **/
> +/*
> + * Locks a "refs/" ref returning the lock on success and NULL on failure.
> + * On failure errno is set to something meaningfull.
s/meaningfull/meaningful/
> + */
> extern struct ref_lock *lock_ref_sha1(const char *refname, const unsigned
> char *old_sha1);
>
> /** Locks any ref (for 'HEAD' type refs). */
> #define REF_NODEREF 0x01
> +/* errno is set to something meaningful on failure */
> extern struct ref_lock *lock_any_ref_for_update(const char *refname,
> const unsigned char *old_sha1,
> int flags, int *type_p);
>
--
Michael Haggerty
mhag...@alum.mit.edu
http://softwareswirl.blogspot.com/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v18 16/48] refs.c: add an err argument to delete_ref_loose
On 06/17/2014 05:53 PM, Ronnie Sahlberg wrote: > Add an err argument to delete_loose_ref so that we can pass a descriptive > error string back to the caller. Pass the err argument from transaction > commit to this function so that transaction users will have a nice error > string if the transaction failed due to delete_loose_ref. > > Add a new function unlink_or_err that we can call from delete_ref_loose. This > function is similar to unlink_or_warn except that we can pass it an err > argument. If err is non-NULL the function will populate err instead of > printing a warning(). > > Simplify warn_if_unremovable. > [...] I'm getting test failures starting with this commit: > Test Summary Report > --- > t5514-fetch-multiple.sh (Wstat: 256 Tests: 11 > Failed: 3) > Failed tests: 6, 8-9 > Non-zero exit status: 1 > t6050-replace.sh (Wstat: 256 Tests: 28 > Failed: 1) > Failed test: 15 > Non-zero exit status: 1 > t1400-update-ref.sh (Wstat: 256 Tests: 133 > Failed: 4) > Failed tests: 86-87, 130-131 > Non-zero exit status: 1 > t5540-http-push-webdav.sh(Wstat: 256 Tests: 19 > Failed: 2) > Failed tests: 8-9 > Non-zero exit status: 1 > t5505-remote.sh (Wstat: 256 Tests: 76 > Failed: 5) > Failed tests: 11, 45-48 > Non-zero exit status: 1 > t9903-bash-prompt.sh (Wstat: 256 Tests: 51 > Failed: 1) > Failed test: 19 > Non-zero exit status: 1 > t9300-fast-import.sh (Wstat: 256 Tests: 170 > Failed: 1) > Failed test: 71 > Non-zero exit status: 1 > t6030-bisect-porcelain.sh(Wstat: 256 Tests: 55 > Failed: 47) > Failed tests: 2-5, 7-11, 13-14, 16-30, 32-34, 36-37, 39-44 > 46-55 > Non-zero exit status: 1 > t7512-status-help.sh (Wstat: 256 Tests: 35 > Failed: 1) > Failed test: 27 > Non-zero exit status: 1 > t5516-fetch-push.sh (Wstat: 256 Tests: 80 > Failed: 3) > Failed tests: 47-49 > Non-zero exit status: 1 Let me know if you need more information. Michael -- Michael Haggerty mhag...@alum.mit.edu http://softwareswirl.blogspot.com/ -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v18 11/48] refs.c: make remove_empty_directories alwasy set errno to something sane
There is a typo in the commit log subject line: s/alwasy/always/ Michael On 06/17/2014 05:53 PM, Ronnie Sahlberg wrote: > Making errno when returning from remove_empty_directories() more > obviously meaningful, which should provide some peace of mind for > people auditing lock_ref_sha1_basic. > > Signed-off-by: Ronnie Sahlberg > --- > refs.c | 5 - > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/refs.c b/refs.c > index a48f805..cc69581 100644 > --- a/refs.c > +++ b/refs.c > @@ -1960,14 +1960,16 @@ static int remove_empty_directories(const char *file) >* only empty directories), remove them. >*/ > struct strbuf path; > - int result; > + int result, save_errno; > > strbuf_init(&path, 20); > strbuf_addstr(&path, file); > > result = remove_dir_recursively(&path, REMOVE_DIR_EMPTY_ONLY); > + save_errno = errno; > > strbuf_release(&path); > + errno = save_errno; > > return result; > } > @@ -2056,6 +2058,7 @@ int dwim_log(const char *str, int len, unsigned char > *sha1, char **log) > return logs_found; > } > > +/* This function should make sure errno is meaningful on error */ > static struct ref_lock *lock_ref_sha1_basic(const char *refname, > const unsigned char *old_sha1, > int flags, int *type_p) > -- Michael Haggerty mhag...@alum.mit.edu http://softwareswirl.blogspot.com/ -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v18 14/48] refs.c: log_ref_write should try to return meaningful errno
On 06/17/2014 05:53 PM, Ronnie Sahlberg wrote: > Making errno from write_ref_sha1() meaningful, which should fix > > * a bug in "git checkout -b" where it prints strerror(errno) > despite errno possibly being zero or clobbered > > * a bug in "git fetch"'s s_update_ref, which trusts the result of an > errno == ENOTDIR check to detect D/F conflicts > > Signed-off-by: Ronnie Sahlberg > --- > refs.c | 29 - > 1 file changed, 24 insertions(+), 5 deletions(-) > > diff --git a/refs.c b/refs.c > index 211429d..1f2eb24 100644 > --- a/refs.c > +++ b/refs.c > @@ -1979,6 +1979,7 @@ static int remove_empty_directories(const char *file) > result = remove_dir_recursively(&path, REMOVE_DIR_EMPTY_ONLY); > save_errno = errno; > > + errno = save_errno; > strbuf_release(&path); > errno = save_errno; This new line looks like an accident. > [...] Michael -- Michael Haggerty mhag...@alum.mit.edu http://softwareswirl.blogspot.com/ -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re* [PATCH v3 04/14] refs.c: add a new update_type field to ref_update
Junio C Hamano writes:
> Ronnie Sahlberg writes:
>
>> Add a field that describes what type of update this refers to. For now
>> the only type is UPDATE_SHA1 but we will soon add more types.
>>
>> Signed-off-by: Ronnie Sahlberg
>> ---
>> refs.c | 25 +
>> 1 file changed, 21 insertions(+), 4 deletions(-)
>>
>> diff --git a/refs.c b/refs.c
>> index 4e3d4c3..4129de6 100644
>> --- a/refs.c
>> +++ b/refs.c
>> @@ -3374,6 +3374,10 @@ int for_each_reflog(each_ref_fn fn, void *cb_data)
>> return retval;
>> }
>>
>> +enum transaction_update_type {
>> + UPDATE_SHA1 = 0,
>
> indent with SP?
>
> Unlike an array initialisation, e.g.
>
> int foo[] = { 1,2,3,4,5, };
>
> some compilers we support complain if enum definition ends with a
> trailing comma.
I do recall we had fixes to drop the comma after the last element in
enum definition in the past, in response real compilation breakages
on some platforms. But there is a curious thing:
git grep -A 'enum ' master -- \*.c
tells me that builtin/clean.c would fail to compile for those folks.
Here is an off-topic "fix" that may no longer be needed.
builtin/clean.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/builtin/clean.c b/builtin/clean.c
index 9a91515..27701d2 100644
--- a/builtin/clean.c
+++ b/builtin/clean.c
@@ -48,7 +48,7 @@ enum color_clean {
CLEAN_COLOR_PROMPT = 2,
CLEAN_COLOR_HEADER = 3,
CLEAN_COLOR_HELP = 4,
- CLEAN_COLOR_ERROR = 5,
+ CLEAN_COLOR_ERROR = 5
};
#define MENU_OPTS_SINGLETON01
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [msysGit] Re: [PATCH 0/7] Second part of msysgit/unicode
Am 18.06.2014 19:33, schrieb Junio C Hamano: > In the meantime, are Windows folks happy with the four topics queued > on 'pu' so far? I would like to start moving them down to 'next' > and to 'master' soonish. > > They consist of these individual patches: > > $ git shortlog ^master \ > sk/mingw-dirent \ > sk/mingw-main \ > sk/mingw-uni-console \ > sk/mingw-unicode-spawn-args Topic sk/test-cmp-bin revealed a new breakage in t5000-tar-tree, specifically, the penultimate test "remote tar.gz is allowed by default". I have yet to find out what it is (I suspect a LF-CRLF conversion issue) and whether it is in connection with one of these topics. I haven't had a chance to test the topics in the field. In particular, I have a few files with Shift-JIS content (but ASCII file names), and I would like to see how well I fare with the unicode topics in this situation. -- Hannes -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
