Re: `git log --graph` with multiple roots is confusing
On Mon, Jun 30, 2014 at 03:04:19AM -0700, Gary Fixler wrote: > I just made a new test repo, added and fetched two unrelated repos, > and then did the log view (all/graph/decorate/oneline), and tacked on > --boundary, but saw no change. The root commits looked the same. There was some discussion a while back on making root commits more apparent in the graph view: http://article.gmane.org/gmane.comp.version-control.git/239580 That topic has stalled, but perhaps you can help push it forward. -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: move detection doesnt take filename into account
On Tue, Jul 01, 2014 at 10:08:15AM -0700, Junio C Hamano wrote: > I didn't think it through but my gut feeling is that we could change > the name similarity score to be the length of the tail part that > matches (e.g. 1.a to a/2.a that has the same two bytes at the tail > is a better match than to a/2.b that does not share any tail, and to > a/1.a that shares the three bytes at the tail is an even better > match). The delta heuristics in pack-objects use pack_name_hash, which claims: /* * This effectively just creates a sortable number from the * last sixteen non-whitespace characters. Last characters * count "most", so things that end in ".c" sort together. */ which might be another option (and seems like a superset of the basename check, short of basenames that are longer than 16 characters). -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
No fchmod() under msygit - Was: Re: [PATCH 00/14] Add submodule test harness
On 07/08/2014 10:25 PM, Ramsay Jones wrote:
On 08/07/14 20:34, Jens Lehmann wrote:
Am 07.07.2014 21:40, schrieb Torsten Bögershausen:
On 2014-07-07 19.05, Junio C Hamano wrote:
Jens Lehmann writes:
Junio, do you want me to resend 02/14 without the non-portable "echo -n"
or could you just squash the following diff in?
Amended locally here already; thanks, both.
There seems to be some other trouble under Mac OS, not yet fully tracked down,
(may be related to the "diff -r")
Torsten sees failures of this kind under Mac OS:
diff -r .git/modules/sub1/config sub1/.git/config
6d5
< worktree = ../../../sub1
8a8
worktree = ../../../sub1
So the config contains the same content, but the worktree setting moved
to a different line. This seems to be the result of setting core.worktree
in the test_git_directory_is_unchanged function just before the "diff -r",
but only under Mac OS.
And Msysgit complains
error: fchmod on c:/xxxt/trash
directory.t7613-merge-submodule/submodule_update_repo/.git/modules/sub1/config.lock
failed: Function not implemented
I'm not sure what this is about, seems to happen during the "cp -R" of
the repo under .git/modules into the submodule.
I haven't looked into this at all, but from the above message, and
noting that fchmod() is not implemented in mingw (see compat/mingw.h
line 91), and the following:
$ git grep -n fchmodcommit daa22c6f8da466bd7a438f1bc27375fd737ffcf3
Author: Eric Wong
Date: Tue May 6 00:17:14 2014 +
config: preserve config file permissions on edits
compat/mingw.h:91:static inline int fchmod(int fildes, mode_t mode)
config.c:1639: if (fchmod(fd, st.st_mode & 0) < 0) {
config.c:1640: error("fchmod on %s failed: %s",
config.c:1818: if (fchmod(out_fd, st.st_mode & 0) < 0) {
config.c:1819: ret = error("fchmod on %s failed: %s",
$
[I happen to be on the pu branch at the moment, so YMMV!]
Both calls to fchmod() above are on config lock files, one
in git_config_set_multivar_in_file() and the other in
git_config_rename_section_in_file().
commit daa22c6f8da466bd7a438f1bc27375fd737ffcf3
Author: Eric Wong
Date: Tue May 6 00:17:14 2014 +
config: preserve config file permissions on edits
(And why is it "& 0" and not "& 0777")
Can we avoid the fchmod() all together ?
--
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] http: Add Accept-Language header if possible
On Wed, Jul 09, 2014 at 02:46:35PM +0900, Yi, EungJun wrote:
> I agree with you. In fact, I tried to get user's preferred language in
> the same way as gettext. It has guess_category_value() to do that and
> the function is good enough because it considers $LANGUAGE, $LC_ALL,
> $LANG, and also system-dependent preferences. But the function does
> not seem a public API and I don't know how can I use the function in
> Git. So I chose to use $LANGUAGE only.
I did some digging, and I think the public API is setlocale with a NULL
parameter, like:
printf("%s\n", setlocale(LC_MESSAGES, NULL));
That still will end up like "en_US.UTF-8", though; I couldn't find any
standard functions for parsing that. It seems like it would be pretty
straightforward to do so, though.
>From my brief reading of rfc2616, that should probably become "en-us",
and any time we add "x-y", we may want to add "x" as a fallback (that is
certainly true for English; I don't know about other languages with
dialects).
-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 00/14] Add submodule test harness
There seems to be some other trouble under Mac OS, not yet fully tracked down, (may be related to the "diff -r") Torsten sees failures of this kind under Mac OS: diff -r .git/modules/sub1/config sub1/.git/config 6d5 < worktree = ../../../sub1 8a8 worktree = ../../../sub1 So the config contains the same content, but the worktree setting moved to a different line. This seems to be the result of setting core.worktree in the test_git_directory_is_unchanged function just before the "diff -r", but only under Mac OS. So I was suspecting diff -r beinng non-portable, but that doesn't seem to be the problem here. (But I wouldn't be surprised if there where problems with diff -r on some Unix systems) Anyway, checking all the files in the working tree seems to be a good thing to do, but that does not necessarily work for .git/config. A "brute force" approach could be to simply run the config file(s) through sort and compare them: sort <.git/modules/sub1/config >expect && sort actual && test_cmp expect actual && rm expect actual && cp git/modules/sub1/config sub1/.git/config [end of scriptlet] And here the "dumps" of the 2 config files: ... Branch remove_sub1 set up to track remote branch remove_sub1 from origin. warning: unable to rmdir sub1: Directory not empty Updating 68c8810..81b9f6a Fast-forward .gitmodules | 4 1 file changed, 4 deletions(-) delete mode 100644 .gitmodules - [core] repositoryformatversion = 0 filemode = true bare = false logallrefupdates = true worktree = ../../../sub1 ignorecase = true precomposeunicode = true [remote "origin"] url = /Users/tb/projects/git/tb.140704_JensLehman/t/trash directory.t7613-merge-submodule/submodule_update_repo/. fetch = +refs/heads/*:refs/remotes/origin/* [branch "master"] remote = origin merge = refs/heads/master [core] repositoryformatversion = 0 filemode = true bare = false logallrefupdates = true ignorecase = true precomposeunicode = true worktree = ../../../sub1 [remote "origin"] url = /Users/tb/projects/git/tb.140704_JensLehman/t/trash directory.t7613-merge-submodule/submodule_update_repo/. fetch = +refs/heads/*:refs/remotes/origin/* [branch "master"] remote = origin merge = refs/heads/master = diff -r .git/modules/sub1/config sub1/.git/config 6d5 < worktree = ../../../sub1 8a8 > worktree = ../../../sub1 not ok 7 - git merge: removed submodule leaves submodule containing a .git directory alone -- 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] http: Add Accept-Language header if possible
2014-07-09 14:10 GMT+09:00 Jeff King : > On Wed, Jul 09, 2014 at 12:54:06AM +0900, Yi EungJun wrote: > >> From: Yi EungJun >> >> Add an Accept-Language header which indicates the user's preferred >> languages defined by 'LANGUAGE' environment variable if the variable is >> not empty. >> >> Example: >> LANGUAGE= -> "" >> LANGUAGE=ko -> "Accept-Language: ko; q=1.000, *; q=0.001" >> LANGUAGE=ko:en -> "Accept-Language: ko; q=1.000, en; q=0.999, *; q=0.001" >> >> This gives git servers a chance to display remote error messages in >> the user's preferred language. > > Should this also take into account other language-related variables? I'd > think $LC_ALL, $LC_MESSAGES, and $LANG would affect it, too. Are > colon-separated values a standard in $LANGUAGE? I have never seen them, > but I admit I am not very knowledgeable about localization issues. > > Also, we do we need to do more parsing? My $LANG is set to en_US.UTF-8. > The encoding part is presumably uninteresting to the remote server. I > also wonder if there are support functions in libc or as part of gettext > that can help us get these values. > > -Peff I agree with you. In fact, I tried to get user's preferred language in the same way as gettext. It has guess_category_value() to do that and the function is good enough because it considers $LANGUAGE, $LC_ALL, $LANG, and also system-dependent preferences. But the function does not seem a public API and I don't know how can I use the function in Git. So I chose to use $LANGUAGE only. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] t/Makefile: always test all lint targets when running tests
On Tue, Jul 8, 2014 at 12:24 PM, Jens Lehmann wrote: > > Am 07.07.2014 20:13, schrieb Junio C Hamano: > > > > So I am not very enthusiastic to see this change myself. > > Ok, I understand we do not want to lightly risk false positives. I > just noticed that I accidentally forgot to sign off this series, so > I'd resend just the first patch with a proper SOB, ok? Nah, let's do both and how it plays out. My not being very enthusiastic myself does not necessarily mean that it is bad for the project. Maybe most people like it and if I cannot bear with it I can always turn it off myself for my environment. I just have a strange feeling that we may be seeing some twisted shell script updates and when the author gets asked why it was written in such a strange way, the answer might turn out to be "just to work around the false positive from the test-lint", which I would not want to see. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] dir: remove PATH_MAX limitation
On Sat, Jul 05, 2014 at 12:42:29AM +0200, Karsten Blees wrote: > Note: this fix just 'abuses' strbuf as string allocator, len is always 0. > prep_exclude() can probably be simplified using more strbuf APIs. Hrm. It looks like you grow it and add some data, but really don't want the length to expand (because the caller depends on it). In other directory-traversal code we follow a pattern like: size_t prefix_len = dir->base.len; strbuf_add(&dir->base, cp, stk->baselen - current); /* use full path in dir->base, then "pop" */ strbuf_setlen(&dir->base, stk->baselen); That makes it a little more obvious that the memcpy matches the strbuf_grow (because it all happens inside strbuf_add). Is it possible to do something like that 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 2/2] t/Makefile: always test all lint targets when running tests
On Mon, Jul 07, 2014 at 11:13:11AM -0700, Junio C Hamano wrote: > Jens Lehmann writes: > > > Only the two targets "test-lint-duplicates" and "test-lint-executable" are > > currently executed when running the test target. This was done on purpose > > when the TEST_LINT variable was added in 81127d74. But as this does not > > include the "test-lint-shell-syntax" target added the same day in commit > > c7ce70ac, it is easy to accidentally add non portable shell constructs > > without noticing that when running the test suite. > > I not running the lint-shell-syntax that is fundamentally flaky to > avoid false positives is very much on purpose. The flakiness is not > the fault of the implementor of the lint-shell-syntax, but comes > from the approach taken to pretend that simple pattern matching can > parse shell scripts. It may not complain on the current set of > scripts, but that is not really by design but by accident. > > So I am not very enthusiastic to see this change myself. Let me play devil's advocate for a moment. Is lint-shell-syntax in fact flaky? I know we discussed false positives when it was originally added, but I think the current implementation tries hard to avoid them. Given that it provides no false positives on the current code base (without many people running it), it seems likely to stay that way. And the cost if we are wrong is either fixing the tool or disabling it (so worst case we are back where we started, modulo a little effort to enable it and then revert). What do we gain? We have an extra line of defense that helps newer shell script writers fix their bugs before they make it to the list. That catches more bugs, and reduces effort for reviewers. And it is exactly these newer shell script writers that need the default flipped; they do not know about portability and the lint target in the first place. I dunno. I am not that enthusiastic about the change, either, but I tend to think it will probably not hurt, and may help. -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: Branch list by date
On Mon, Jul 07, 2014 at 09:54:55PM -0700, Jeremy Apthorp wrote: > I write this missive with dual purpose: firstly to share a potentially > useful tool, and secondly to suggest that this feature (with a less > mind-wrenchingly disgusting implementation) might be included in > mainline git, as for example `git branch [-t] | [--by-time]`. I think what we should aim for is: 1. Teaching git-branch the same sorting as for-each-ref. So first --sort, and then possibly "-t" as an alias for "--sort=committerdate". 2. Teaching git-branch custom output formats. We have "-v" and "-vv", but it should support the full power of for-each-ref's --format atoms. 3. Teach branch and for-each-ref to support readable colors in their formats, like we do for "log --format". 4. Optionally add config options to configure defaults for the above, so "git branch" shows what you want. I'm (slowly) working on a refactor that will unify for-each-ref and branch, which would accomplish (1) and (2). -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] http: Add Accept-Language header if possible
On Wed, Jul 09, 2014 at 12:54:06AM +0900, Yi EungJun wrote: > From: Yi EungJun > > Add an Accept-Language header which indicates the user's preferred > languages defined by 'LANGUAGE' environment variable if the variable is > not empty. > > Example: > LANGUAGE= -> "" > LANGUAGE=ko -> "Accept-Language: ko; q=1.000, *; q=0.001" > LANGUAGE=ko:en -> "Accept-Language: ko; q=1.000, en; q=0.999, *; q=0.001" > > This gives git servers a chance to display remote error messages in > the user's preferred language. Should this also take into account other language-related variables? I'd think $LC_ALL, $LC_MESSAGES, and $LANG would affect it, too. Are colon-separated values a standard in $LANGUAGE? I have never seen them, but I admit I am not very knowledgeable about localization issues. Also, we do we need to do more parsing? My $LANG is set to en_US.UTF-8. The encoding part is presumably uninteresting to the remote server. I also wonder if there are support functions in libc or as part of gettext that can help us get these values. -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 v20 00/48] Use ref transactions
On Tue, Jul 08, 2014 at 11:48:06AM -0700, Junio C Hamano wrote: > I'd say that "if you have foo/bar you cannot have foo" may have > started as an implementation limitation, but the interoperability > requirement with existing versions of Git and with existing > repositories makes it necessary to enforce it the same way as other > rules such as "you cannot have double-dots in name, e.g. foo..bar" > or "no branches whose name begins with a dash", neither of which > comes from any filesystem issues. That a rule can be loosened with > one new backend does not at all mean it is a good idea to loosen it > "because we can" in the first place. To me it makes sense to to have it as an option, for two reasons: 1. If you want to pay the compatibility cost (e.g., because you work with a defined set of users on a small project and can dictate how they set their git options), you get the extra feature. 2. It provides a migration path if we eventually want to move to a default backend that allows it. I admit that I don't care _too_ much, though. My main desire for it is not to store two live branches that overlap, but to store reflogs for deleted branches without conflicting with live branches. And of course all of this is getting rather ahead of ourselves. We do not have _any_ alternate backends yet, nor even yet the infrastructure to make them. -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 v2] log: fix indentation for --graph --show-signature
The git log --graph --show-signature command incorrectly indents the gpg
information about signed commits and merged signed tags. It does not
follow the level of indentation of the current commit.
Example of garbled output:
$ git log --show-signature --graph
* commit 258e0a237cb69aaa587b0a4fb528bb0316b1b776
|\ gpg: Signature made Mon, Jun 30, 2014 13:22:33 EDT using RSA key ID DA08
gpg: Good signature from "Jason Pyeron "
Merge: 727c355 1ca13ed
| | Author: Jason Pyeron
| | Date: Mon Jun 30 13:22:29 2014 -0400
| |
| | Merge of 1ca13ed2271d60ba9 branch - rebranding
| |
| * commit 1ca13ed2271d60ba93d40bcc8db17ced8545f172
| | gpg: Signature made Mon, Jun 23, 2014 9:45:47 EDT using RSA key ID DD37
gpg: Good signature from "Stephen Robert Guglielmo "
gpg: aka "Stephen Robert Guglielmo "
Author: Stephen R Guglielmo
| | Date: Mon Jun 23 09:45:27 2014 -0400
| |
| | Minor URL updates
In log-tree.c modify show_sig_lines() function to call graph_show_oneline()
after each line of gpg information it has printed in order to preserve
the level of indentation for the next output line.
Reported-by: Jason Pyeron
Signed-off-by: Zoltan Klinger
---
Changes since v1:
t/t4202-log.sh file:
* fix broken &&-chain in test cases
* add test_when_finished scripts to test cases to
reset things to master branch
log-tree.c | 1 +
t/t4202-log.sh | 31 +++
2 files changed, 32 insertions(+)
diff --git a/log-tree.c b/log-tree.c
index 10e6844..f13b861 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -365,6 +365,7 @@ static void show_sig_lines(struct rev_info *opt, int
status, const char *bol)
eol = strchrnul(bol, '\n');
printf("%s%.*s%s%s", color, (int)(eol - bol), bol, reset,
*eol ? "\n" : "");
+ graph_show_oneline(opt->graph);
bol = (*eol) ? (eol + 1) : eol;
}
}
diff --git a/t/t4202-log.sh b/t/t4202-log.sh
index cb03d28..99ab7ca 100755
--- a/t/t4202-log.sh
+++ b/t/t4202-log.sh
@@ -3,6 +3,7 @@
test_description='git log'
. ./test-lib.sh
+. "$TEST_DIRECTORY/lib-gpg.sh"
test_expect_success setup '
@@ -841,4 +842,34 @@ test_expect_success 'dotdot is a parent directory' '
test_cmp expect actual
'
+test_expect_success GPG 'log --graph --show-signature' '
+ test_when_finished "git reset --hard && git checkout master" &&
+ git checkout -b signed master &&
+ echo foo >foo &&
+ git add foo &&
+ git commit -S -m signed_commit &&
+ git log --graph --show-signature -n1 signed >actual &&
+ grep "^| gpg: Signature made" actual &&
+ grep "^| gpg: Good signature" actual
+'
+
+test_expect_success GPG 'log --graph --show-signature for merged tag' '
+ test_when_finished "git reset --hard && git checkout master" &&
+ git checkout -b plain master &&
+ echo aaa >bar &&
+ git add bar &&
+ git commit -m bar_commit &&
+ git checkout -b tagged master &&
+ echo bbb >baz &&
+ git add baz &&
+ git commit -m baz_commit &&
+ git tag -s -m signed_tag_msg signed_tag &&
+ git checkout plain &&
+ git merge --no-ff -m msg signed_tag &&
+ git log --graph --show-signature -n1 plain >actual &&
+ grep "^|\\\ merged tag" actual &&
+ grep "^| | gpg: Signature made" actual &&
+ grep "^| | gpg: Good signature" actual
+'
+
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 v4 4/4] cache-tree: Write updated cache-tree after commit
On Wed, Jul 9, 2014 at 12:05 AM, Junio C Hamano wrote: > Duy Nguyen writes: > >> I wonder if we need to update_main_cache_tree() so many times in this >> function. If I read the code correctly, all roads must lead to >> update_main_cache_tree(0) in prepare_to_commit(). > > I think prepare-to-commit is too late; it does not want to know if > the index it was given to create a tree out of is the one that the > user will keep using after this invocation of "git commit" or just a > temporary one used for partial commit. The cache-tree rebuilt there > is what is recorded with commit_tree_extended() in cmd_commit(), but > if you are doing a partial commit, these generic code paths are > given a temporary index file on the filesystem to work on, and > cache-tree in that will not help user's later operation. Right. Thanks for checking. -- Duy -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: t5150-request-pull.sh fails on newest master in Debian
On Wed, 2014-07-09 at 02:52 +0200, Øyvind A. Holm wrote:
> On 3 July 2014 23:55, Øyvind A. Holm wrote:
> > When compiling newest master (v2.0.1-472-g6f92e5f) on Debian 7.5
> > (64-bit), t5150-request-pull.sh fails when compiling with
> >
> > $ make configure
> > $ ./configure --prefix=/usr/local/varprg/git.master.v2.0.1-472-g6f92e5f
> > $ make prefix=/usr/local/varprg/git.master.v2.0.1-472-g6f92e5f
> > $ cd t
> > $ ./t5150-request-pull.sh
>
> FYI, t5150-request-pull.sh passes all tests now on newest master
> (v2.0.1-474-g72c7794) in Debian. There are two new commits on master
> since I wrote this, and the commit that makes things work again is
> 4602f1a ("diff-tree: call free_commit_list() instead of duplicating
> its code"). Reverting this commit brings the failure back.
>
> The whole thing is still a mystery to me, though. I can't see why this
> should have anything to do with the use of ./configure --prefix.
The problem only happens when a ref with an allowed wildcard winds up on
a page boundary (with the wildcard before the page boundary). This
depends intricately on the details of memory allocation, so pretty much
anything could make it come and go.
Does the fix I posted work for you? If not, let me know and I'll look
into it more.
--
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: t5150-request-pull.sh fails on newest master in Debian
On 3 July 2014 23:55, Øyvind A. Holm wrote:
> When compiling newest master (v2.0.1-472-g6f92e5f) on Debian 7.5
> (64-bit), t5150-request-pull.sh fails when compiling with
>
> $ make configure
> $ ./configure --prefix=/usr/local/varprg/git.master.v2.0.1-472-g6f92e5f
> $ make prefix=/usr/local/varprg/git.master.v2.0.1-472-g6f92e5f
> $ cd t
> $ ./t5150-request-pull.sh
FYI, t5150-request-pull.sh passes all tests now on newest master
(v2.0.1-474-g72c7794) in Debian. There are two new commits on master
since I wrote this, and the commit that makes things work again is
4602f1a ("diff-tree: call free_commit_list() instead of duplicating
its code"). Reverting this commit brings the failure back.
The whole thing is still a mystery to me, though. I can't see why this
should have anything to do with the use of ./configure --prefix. I
tested several variants with and without ./configure --prefix, all
tests were run several times and were reproducible every time. Was
this --prefix thing just a red herring, or is it linked to this in
some way?
Also, the only file this commit touches is builtin/diff-tree.c, and
this file hasn't been modified since 2011. Does anyone know what's
going on here?
Cheers,
Øyvind
--
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 v2 08/19] rebase -i: Root commits are replayed with an unnecessary option
Fabian Ruch writes:
> The command line used to recreate root commits specifies the
> effectless option `-C`. It is used to reuse commit message and
> authorship from the named commit but the commit being amended here,
> which is the sentinel commit, already carries the authorship and log
> message of the processed commit. Note that the committer email and
> commit date fields do not match the root commit either way. Remove
> the option. However, `-C` (other than `-c`) does not invoke the
> editor and the `--amend` option invokes it by default. Disable editor
> invocation again by specifying `--no-edit`.
I'd say this is a no-value change, in the sense that it can be
written either way for the same effect and if the original were
written with "--amend --no-edit" then there would be no reason to
change it to "-C $1" because such a change would also be equally a
no-value change.
What exactly are we gaining? Performance? Correctness?
> Signed-off-by: Fabian Ruch
> ---
> git-rebase--interactive.sh | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> index ff04d5d..17836d5 100644
> --- a/git-rebase--interactive.sh
> +++ b/git-rebase--interactive.sh
> @@ -511,7 +511,7 @@ do_pick () {
> --no-post-rewrite -n -q -C $1 &&
> pick_one -n $1 &&
> git commit --allow-empty \
> ---amend --no-post-rewrite -n -C $1 \
> +--amend --no-post-rewrite -n --no-edit \
> ${gpg_sign_opt:+"$gpg_sign_opt"} ||
> die_with_patch $1 "Could not apply $1... $2"
> else
--
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 v2 07/19] rebase -i: The replay of root commits is not shown with --verbose
Fabian Ruch writes:
> The command line used to recreate root commits specifies the
> erroneous option `-q` which suppresses the commit summary message.
> However, git-rebase--interactive tends to tell the user about the
> commits it creates, if she wishes (cf. command line option
> `--verbose`). The code parts handling non-root commits or squash
> commits all output commit summary messages. Do not make the replay of
> root commits an exception. Remove the option.
>
> It is OK to suppress the commit summary when git-commit is used to
> initialize the authorship of the sentinel commit because the
> existence of this additional commit is a detail of
> git-rebase--interactive's implementation. The option `-q` was
> probably introduced as a copy-and-paste error stemming from that part
> of the root commit handling code.
>
> Signed-off-by: Fabian Ruch
> ---
This one I can buy; there is no reason to drop "-q" from both (which
would give us the same thing twice and the one before the "pick_one
-n" runs is not the final one anyway) and the later one that records
the updated tree would be the one to report what it did.
> git-rebase--interactive.sh | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> index 0af96f2..ff04d5d 100644
> --- a/git-rebase--interactive.sh
> +++ b/git-rebase--interactive.sh
> @@ -511,7 +511,7 @@ do_pick () {
> --no-post-rewrite -n -q -C $1 &&
> pick_one -n $1 &&
> git commit --allow-empty \
> ---amend --no-post-rewrite -n -q -C $1 \
> +--amend --no-post-rewrite -n -C $1 \
> ${gpg_sign_opt:+"$gpg_sign_opt"} ||
> die_with_patch $1 "Could not apply $1... $2"
> else
--
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 v2 06/19] rebase -i: Stop on root commits with empty log messages
Fabian Ruch writes:
> The command line used to recreate root commits specifies the
> erroneous option `--allow-empty-message`. If the root commit has an
> empty log message, the replay of this commit should fail and the
> rebase should be interrupted like for any other commit that is on the
> to-do list and has an empty commit message. Remove the option.
>
> The option might have been introduced by copy-and-paste of the first
> part of the command line which initializes the authorship of the
> sentinel commit. Indeed, the sentinel commit has an empty log message
> and this should not trigger a failure, which is why the option
> `--allow-empty-message` is correctly specified here.
The first "commit --amend" uses -C "$1" to give the amended result
not just the authorship but also the log message taken from "$1".
If we are allowing a commit without any message to be used as "$1",
I think --allow-empty-message needs to be there. If "$1" requires
the option here, why doesn't the second one, that records the
updated tree with the metainformation taken from the same commit
"$1" can successfully commit without the option?
Puzzled...
> Add test.
>
> Signed-off-by: Fabian Ruch
> ---
> git-rebase--interactive.sh | 2 +-
> t/t3412-rebase-root.sh | 39 +++
> 2 files changed, 40 insertions(+), 1 deletion(-)
>
> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> index 4c875d5..0af96f2 100644
> --- a/git-rebase--interactive.sh
> +++ b/git-rebase--interactive.sh
> @@ -510,7 +510,7 @@ do_pick () {
> git commit --allow-empty --allow-empty-message --amend \
> --no-post-rewrite -n -q -C $1 &&
> pick_one -n $1 &&
> - git commit --allow-empty --allow-empty-message \
> + git commit --allow-empty \
> --amend --no-post-rewrite -n -q -C $1 \
> ${gpg_sign_opt:+"$gpg_sign_opt"} ||
> die_with_patch $1 "Could not apply $1... $2"
> diff --git a/t/t3412-rebase-root.sh b/t/t3412-rebase-root.sh
> index 0b52105..9867705 100755
> --- a/t/t3412-rebase-root.sh
> +++ b/t/t3412-rebase-root.sh
> @@ -278,4 +278,43 @@ test_expect_success 'rebase -i -p --root with conflict
> (second part)' '
> test_cmp expect-conflict-p out
> '
>
> +test_expect_success 'stop rebase --root on empty root log message' '
> + # create a root commit with a non-empty tree so that rebase does
> + # not fail because of an empty commit, and an empty log message
> + echo root-commit >file &&
> + git add file &&
> + tree=$(git write-tree) &&
> + root=$(git commit-tree $tree + git checkout -b no-message-root-commit $root &&
> + # do not ff because otherwise neither the patch nor the message
> + # are looked at and checked for emptiness
> + test_when_finished git rebase --abort &&
> + test_must_fail env EDITOR=true git rebase -i --force-rebase --root &&
> + echo root-commit >file.expected &&
> + test_cmp file.expected file
> +'
> +
> +test_expect_success 'stop rebase --root on empty child log message' '
> + # create a root commit with a non-empty tree and provide a log
> + # message so that rebase does not fail until the root commit is
> + # successfully replayed
> + echo root-commit >file &&
> + git add file &&
> + tree=$(git write-tree) &&
> + root=$(git commit-tree $tree -m root-commit) &&
> + git checkout -b no-message-child-commit $root &&
> + # create a child commit with a non-empty patch so that rebase
> + # does not fail because of an empty commit, but an empty log
> + # message
> + echo child-commit >file &&
> + git add file &&
> + git commit --allow-empty-message --no-edit &&
> + # do not ff because otherwise neither the patch nor the message
> + # are looked at and checked for emptiness
> + test_when_finished git rebase --abort &&
> + test_must_fail env EDITOR=true git rebase -i --force-rebase --root &&
> + echo child-commit >file.expected &&
> + test_cmp file.expected file
> +'
> +
> test_done
--
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] http: Add Accept-Language header if possible
On Tue, Jul 8, 2014 at 11:54 AM, Yi EungJun wrote:
> From: Yi EungJun
>
> Add an Accept-Language header which indicates the user's preferred
> languages defined by 'LANGUAGE' environment variable if the variable is
> not empty.
>
> Example:
> LANGUAGE= -> ""
> LANGUAGE=ko -> "Accept-Language: ko; q=1.000, *; q=0.001"
> LANGUAGE=ko:en -> "Accept-Language: ko; q=1.000, en; q=0.999, *; q=0.001"
>
> This gives git servers a chance to display remote error messages in
> the user's preferred language.
> ---
> http.c | 43 +++
> t/t5550-http-fetch-dumb.sh | 10 ++
> 2 files changed, 53 insertions(+)
>
> diff --git a/http.c b/http.c
> index 3a28b21..c345616 100644
> --- a/http.c
> +++ b/http.c
> @@ -983,6 +983,47 @@ static void extract_content_type(struct strbuf *raw,
> struct strbuf *type,
> strbuf_addstr(charset, "ISO-8859-1");
> }
>
> +/*
> + * Add an Accept-Language header which indicates user's preferred languages
> + * defined by 'LANGUAGE' environment variable if the variable is not empty.
> + *
> + * Example:
> + * LANGUAGE= -> ""
> + * LANGUAGE=ko -> "Accept-Language: ko; q=1.000, *; q=0.001"
> + * LANGUAGE=ko:en -> "Accept-Language: ko; q=1.000, en; q=0.999, *;
> q=0.001"
> + */
> +static void add_accept_language(struct strbuf *buf)
> +{
> + const char *p1, *p2;
> + float q = 1.000;
> +
> + p1 = getenv("LANGUAGE");
> +
> + if (p1 != NULL && p1[0] != '\0') {
> + strbuf_reset(buf);
It seems wrong to clear 'buf' in a function named add_accept_language().
> + strbuf_addstr(buf, "Accept-Language: ");
> + for (p2 = p1; q > 0.001; p2++) {
> + if ((*p2 == ':' || *p2 == '\0') && p1 != p2) {
> + if (q < 1.0) {
> + strbuf_addstr(buf, ", ");
> + }
> + strbuf_add(buf, p1, p2 - p1);
> + strbuf_addf(buf, "; q=%.3f", q);
> + q -= 0.001;
> + p1 = p2 + 1;
> +
> + if (*p2 == '\0') {
> + break;
> + }
> + }
> + }
> + if (q < 1.0) {
> + strbuf_addstr(buf, ", ");
> + }
> + strbuf_addstr(buf, "*; q=0.001\r\n");
Manually adding "\r\n" is contraindicated. Headers passed to
curl_easy_setopt(c, CURLOPT_HTTPHEADER, headers) must not have "\r\n",
since curl will add terminators itself [1].
[1]: http://curl.haxx.se/libcurl/c/CURLOPT_HTTPHEADER.html
> + }
> +}
> +
> /* http_request() targets */
> #define HTTP_REQUEST_STRBUF0
> #define HTTP_REQUEST_FILE 1
> @@ -1020,6 +1061,8 @@ static int http_request(const char *url,
> fwrite_buffer);
> }
>
> + add_accept_language(&buf);
This is inconsistent with how other headers are handled by this
function. The existing idiom is:
strbuf_add(&buf, ...); /* construct header */
headers = curl_slist_apend(headers, buf.buf);
strbuf_reset(&buf);
> +
> strbuf_addstr(&buf, "Pragma:");
> if (options && options->no_cache)
> strbuf_addstr(&buf, " no-cache");
> diff --git a/t/t5550-http-fetch-dumb.sh b/t/t5550-http-fetch-dumb.sh
> index ac71418..ea15158 100755
> --- a/t/t5550-http-fetch-dumb.sh
> +++ b/t/t5550-http-fetch-dumb.sh
> @@ -196,5 +196,15 @@ test_expect_success 'reencoding is robust to whitespace
> oddities' '
> grep "this is the error message" stderr
> '
>
> +test_expect_success 'git client sends Accept-Language' '
> + GIT_CURL_VERBOSE=1 LANGUAGE=ko:en git clone
> "$HTTPD_URL/accept/language" 2>actual
Broken &&-chain.
> + grep "^Accept-Language: ko; q=1.000, en; q=0.999, \*; q=0.001" actual
Do you want to \-escape the periods? (Or maybe use 'grep -F'?)
> +'
> +
> +test_expect_success 'git client does not send Accept-Language' '
> + GIT_CURL_VERBOSE=1 LANGUAGE= git clone "$HTTPD_URL/accept/language"
> 2>actual
Broken &&-chain.
> + test_must_fail grep "^Accept-Language:" actual
> +'
> +
> stop_httpd
> test_done
> --
> 2.0.1.473.gafdefd9.dirty
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
What's cooking in git.git (Jul 2014, #01; Tue, 8)
Here are the topics that have been cooking. Commits prefixed with '-' are only in 'pu' (proposed updates) while commits prefixed with '+' are in 'next'. 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"] * bc/fix-rebase-merge-skip (2014-06-16) 1 commit (merged to 'next' on 2014-06-20 at 01f81f5) + rebase--merge: fix --skip with two conflicts in a row "git rebase --skip" did not work well when it stopped due to a conflict twice in a row. * dt/refs-check-refname-component-sse (2014-06-18) 1 commit (merged to 'next' on 2014-06-20 at d286027) + refs.c: SSE2 optimizations for check_refname_component Further micro-optimization of a leaf-function. * jk/commit-buffer-length (2014-06-13) 18 commits (merged to 'next' on 2014-06-16 at b2d2d7b) + reuse cached commit buffer when parsing signatures + commit: record buffer length in cache + commit: convert commit->buffer to a slab + commit-slab: provide a static initializer + use get_commit_buffer everywhere + convert logmsg_reencode to get_commit_buffer + use get_commit_buffer to avoid duplicate code + use get_cached_commit_buffer where appropriate + provide helpers to access the commit buffer + provide a helper to set the commit buffer + provide a helper to free commit buffer + sequencer: use logmsg_reencode in get_message + logmsg_reencode: return const buffer + do not create "struct commit" with xcalloc + commit: push commit_index update into alloc_commit_node + alloc: include any-object allocations in alloc_report + replace dangerous uses of strbuf_attach + commit_tree: take a pointer/len pair rather than a const strbuf Move "commit->buffer" out of the in-core commit object and keep track of their lengths. Use this to optimize the code paths to validate GPG signatures in commit objects. * ye/http-extract-charset (2014-06-17) 1 commit (merged to 'next' on 2014-06-20 at 9492bae) + http: fix charset detection of extract_content_type() -- [New Topics] * cc/replace-edit (2014-06-25) 3 commits - replace: use argv_array in export_object - avoid double close of descriptors handed to run_command - replace: replace spaces with tabs in indentation (this branch is used by jk/replace-edit-raw.) Will merge to 'next'. * ep/submodule-code-cleanup (2014-06-30) 1 commit - submodule.c: use the ARRAY_SIZE macro Will merge to 'next'. * jk/replace-edit-raw (2014-06-25) 1 commit - replace: add a --raw mode for --edit (this branch uses cc/replace-edit.) Will merge to 'next'. * jk/strip-suffix (2014-06-30) 9 commits - prepare_packed_git_one: refactor duplicate-pack check - verify-pack: use strbuf_strip_suffix - strbuf: implement strbuf_strip_suffix - index-pack: use strip_suffix to avoid magic numbers - use strip_suffix instead of ends_with in simple cases - replace has_extension with ends_with - implement ends_with via strip_suffix - add strip_suffix function - sha1_file: replace PATH_MAX buffer with strbuf in prepare_packed_git_one() Will merge to 'next'. * jk/tag-contains (2014-06-30) 8 commits - perf: add tests for tag --contains - tag: use commit_contains - commit: provide a fast multi-tip contains function - string-list: add pos to iterator callback - add functions for memory-efficient bitmaps - paint_down_to_common: use prio_queue - tag: factor out decision to stream tags - tag: allow --sort with -n Expecting a reroll. * mg/fix-log-mergetag-color (2014-06-30) 1 commit - log: correctly identify mergetag signature verification status Will merge to 'next'. * mk/merge-incomplete-files (2014-06-30) 2 commits - git-merge-file: do not add LF at EOF while applying unrelated change - t6023-merge-file.sh: fix and mark as broken invalid tests Will merge to 'next'. * rs/status-code-clean-up (2014-06-29) 2 commits (merged to 'next' on 2014-07-08 at db67965) + wt-status: simplify building of summary limit argument + wt-status: use argv_array for environment Will merge to 'master'. * tb/crlf-tests (2014-07-08) 2 commits (merged to 'next' on 2014-07-08 at 40764b7) + t0027: combinations of core.autocrlf, core.eol and text + t0025: rename the test files Will merge to 'master'. * ak/profile-feedback-build (2014-07-08) 4 commits - Fix profile feedback with -jN and add profile-fast - Run the perf test suite for profile feedback too - Don't define away __attribute__ on gcc - Use BASIC_FLAGS for profile feedback Will merge to 'next'. * cb/filter-branch-prune-empty-degenerate-merges (2014-07-01) 1 commit - filter-branch: eliminate duplicate mapped parents Will merge to 'next'. * cc/for-each-mergetag (2014-07-07) 1 commit - commit: add for_each_mergetag() (this branch is used by cc/replace-graft.) Will merge to 'next'. * dt/cache-tree-rep
Re: [PATCH] log: fix indentation for --graph --show-signature
On Tue, Jul 8, 2014 at 7:12 AM, Zoltan Klinger wrote: > The git log --graph --show-signature command incorrectly indents the gpg > information about signed commits and merged signed tags. It does not > follow the level of indentation of the current commit. > > Reported-by: Jason Pyeron > Signed-off-by: Zoltan Klinger > --- > diff --git a/t/t4202-log.sh b/t/t4202-log.sh > index cb03d28..b429aff 100755 > --- a/t/t4202-log.sh > +++ b/t/t4202-log.sh > @@ -3,6 +3,7 @@ > test_description='git log' > > . ./test-lib.sh > +. "$TEST_DIRECTORY/lib-gpg.sh" > > test_expect_success setup ' > > @@ -841,4 +842,32 @@ test_expect_success 'dotdot is a parent directory' ' > test_cmp expect actual > ' > > +test_expect_success GPG 'log --graph --show-signature' ' > + git checkout -b signed master && Do you want test_when_finished 'git reset --hard && git checkout master' && here in case of failure in this test in order to restore sanity for tests which might be added later? > + echo foo >foo && > + git add foo && > + git commit -S -m signed_commit && > + git log --graph --show-signature -n1 signed >actual && > + grep "^| gpg: Signature made" actual && > + grep "^| gpg: Good signature" actual > +' > + > +test_expect_success GPG 'log --graph --show-signature for merged tag' ' > + git checkout -b plain master && > + echo aaa >bar && > + git add bar && > + git commit -m bar_commit Broken &&-chain. > + git checkout -b tagged master && Ditto regarding test_when_finished. > + echo bbb >baz && > + git add baz && > + git commit -m baz_commit Broken &&-chain. > + git tag -s -m signed_tag_msg signed_tag && > + git checkout plain && > + git merge --no-ff -m msg signed_tag && > + git log --graph --show-signature -n1 plain >actual && > + grep "^|\\\ merged tag" actual && > + grep "^| | gpg: Signature made" actual && > + grep "^| | gpg: Good signature" actual > +' > + > 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 00/14] Add submodule test harness
On 08/07/14 21:25, Ramsay Jones wrote:
> On 08/07/14 20:34, Jens Lehmann wrote:
>> Am 07.07.2014 21:40, schrieb Torsten Bögershausen:
>>> On 2014-07-07 19.05, Junio C Hamano wrote:
Jens Lehmann writes:
> Junio, do you want me to resend 02/14 without the non-portable "echo -n"
> or could you just squash the following diff in?
Amended locally here already; thanks, both.
>>>
>>> There seems to be some other trouble under Mac OS, not yet fully tracked
>>> down,
>>> (may be related to the "diff -r")
>>
>> Torsten sees failures of this kind under Mac OS:
>>
>> diff -r .git/modules/sub1/config sub1/.git/config
>> 6d5
>> < worktree = ../../../sub1
>> 8a8
>>> worktree = ../../../sub1
>>
>> So the config contains the same content, but the worktree setting moved
>> to a different line. This seems to be the result of setting core.worktree
>> in the test_git_directory_is_unchanged function just before the "diff -r",
>> but only under Mac OS.
>>
>>> And Msysgit complains
>>> error: fchmod on c:/xxxt/trash
>>> directory.t7613-merge-submodule/submodule_update_repo/.git/modules/sub1/config.lock
>>> failed: Function not implemented
>>
>> I'm not sure what this is about, seems to happen during the "cp -R" of
>> the repo under .git/modules into the submodule.
>
> I haven't looked into this at all, but from the above message, and
> noting that fchmod() is not implemented in mingw (see compat/mingw.h
> line 91), and the following:
>
> $ git grep -n fchmod
> compat/mingw.h:91:static inline int fchmod(int fildes, mode_t mode)
> config.c:1639: if (fchmod(fd, st.st_mode & 0) < 0) {
> config.c:1640: error("fchmod on %s failed: %s",
> config.c:1818: if (fchmod(out_fd, st.st_mode & 0) < 0) {
> config.c:1819: ret = error("fchmod on %s failed: %s",
> $
>
> [I happen to be on the pu branch at the moment, so YMMV!]
>
> Both calls to fchmod() above are on config lock files, one
> in git_config_set_multivar_in_file() and the other in
> git_config_rename_section_in_file().
>
See commit daa22c6f8 ("config: preserve config file permissions
on edits", 06-05-2014).
ATB,
Ramsay Jones
--
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 v2 00/19] Enable options --signoff, --reset-author for pick, reword
Fabian Ruch writes: > Fabian Ruch (19): > rebase -i: Failed reword prints redundant error message > rebase -i: reword complains about empty commit despite --keep-empty > rebase -i: reword executes pre-commit hook on interim commit > rebase -i: Teach do_pick the option --edit > rebase -i: Implement reword in terms of do_pick > rebase -i: Stop on root commits with empty log messages > rebase -i: The replay of root commits is not shown with --verbose > rebase -i: Root commits are replayed with an unnecessary option > rebase -i: Commit only once when rewriting picks > rebase -i: Do not die in do_pick > rebase -i: Teach do_pick the option --amend > rebase -i: Teach do_pick the option --file > rebase -i: Prepare for squash in terms of do_pick --amend > rebase -i: Implement squash in terms of do_pick > rebase -i: Explicitly distinguish replay commands and exec tasks > rebase -i: Parse to-do list command line options > rebase -i: Teach do_pick the option --reset-author > rebase -i: Teach do_pick the option --signoff > rebase -i: Enable options --signoff, --reset-author for pick, reword After "rebase -i:", some begin with lowercase and many begin with capital, which makes the short-log output look distracting. -- 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] gitk: add keybinding to switch to parent commit
Signed-off-by: Max Kirillov
---
Hi.
I was missing this one. Actually the most needed is go to first
parent, though the second also may be useful.
gitk | 12
1 file changed, 12 insertions(+)
diff --git a/gitk b/gitk
index 41e5071..de35fe4 100755
--- a/gitk
+++ b/gitk
@@ -2594,6 +2594,9 @@ proc makewindow {} {
bind $ctext $ctxbut {pop_diff_menu %W %X %Y %x %y}
bind $ctext {focus %W}
bind $ctext <> rehighlight_search_results
+for {set i 1} {$i<10} {incr i} {
+ bind . <$M1B-Key-$i> [list go_to_parent $i]
+}
set maincursor [. cget -cursor]
set textcursor [$ctext cget -cursor]
@@ -3016,6 +3019,7 @@ proc keys {} {
[mc ", n, j Move down one commit"]
[mc ", z, h Go back in history list"]
[mc ", x, l Go forward in history list"]
+[mc "<%s-n>Go to n-th parent of current commit in history list" $M1T]
[mc " Move up one page in commit list"]
[mc "Move down one page in commit list"]
[mc "<%s-Home> Scroll to top of commit list" $M1T]
@@ -7494,6 +7498,14 @@ proc goforw {} {
}
}
+proc go_to_parent {i} {
+global parents curview targetid
+set ps $parents($curview,$targetid)
+if {[llength $ps] >= $i} {
+ selbyid [lindex $ps [expr $i - 1]]
+}
+}
+
proc gettree {id} {
global treefilelist treeidlist diffids diffmergeid treepending
global nullid nullid2
--
2.0.0.526.g5318336
--
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 v2 03/19] rebase -i: reword executes pre-commit hook on interim commit
Fabian Ruch writes:
> The to-do list command `reword` replays a commit like `pick` but lets
> the user also edit the commit's log message. This happens in two
> steps. Firstly, the named commit is cherry-picked. Secondly, the
> commit created in the first step is amended using an unchanged index
> to edit the log message. The pre-commit hook is meant to verify the
> changes introduced by a commit (for instance, catching inserted
> trailing white space). Since the committed tree is not changed
> between the two steps, do not execute the pre-commit hook in the
> second step.
It is not like the second step is built as a child commit of the
result from the first step, recording the same tree without any
change. Why is it a good thing not to run the pre-commit hook (or
other hooks for that matter)? After all, the result of the second
step is what is recorded in the final history; it just feels wrong
not to test that one, even if it were a good idea to test only once.
> Specify the git-commit option `--no-verify` to disable the pre-commit
> hook when editing the log message. Because `--no-verify` also skips
> the commit-msg hook, execute the hook from within
> git-rebase--interactive after the commit is created. Fortunately, the
> commit message is still available in `$GIT_DIR/COMMIT_EDITMSG` after
> git-commit terminates. Caveat: In case the commit-msg hook finds the
> new log message ill-formatted, the user is only notified of the
> failed commit-msg hook but the log message is used for the commit
> anyway. git-commit ought to offer more fine-grained control over
> which hooks are executed.
>
> Signed-off-by: Fabian Ruch
> ---
> git-rebase--interactive.sh | 17 +
> 1 file changed, 13 insertions(+), 4 deletions(-)
>
> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> index 689400e..b50770d 100644
> --- a/git-rebase--interactive.sh
> +++ b/git-rebase--interactive.sh
> @@ -504,10 +504,19 @@ do_next () {
>
> mark_action_done
> do_pick $sha1 "$rest"
> - git commit --allow-empty --amend --no-post-rewrite
> ${gpg_sign_opt:+"$gpg_sign_opt"} || {
> - warn "Could not amend commit after successfully picking
> $sha1... $rest"
> - exit_with_patch $sha1 1
> - }
> + # TODO: Work around the fact that git-commit lets us
> + # disable either both the pre-commit and the commit-msg
> + # hook or none. Disable the pre-commit hook because the
> + # tree is left unchanged but run the commit-msg hook
> + # from here because the log message is altered.
> + git commit --allow-empty --amend --no-post-rewrite -n
> ${gpg_sign_opt:+"$gpg_sign_opt"} &&
> + if test -x "$GIT_DIR"/hooks/commit-msg
> + then
> + "$GIT_DIR"/hooks/commit-msg
> "$GIT_DIR"/COMMIT_EDITMSG
> + fi || {
> + warn "Could not amend commit after successfully
> picking $sha1... $rest"
> + exit_with_patch $sha1 1
> + }
> record_in_rewritten $sha1
> ;;
> edit|e)
--
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 v2 02/19] rebase -i: reword complains about empty commit despite --keep-empty
Fabian Ruch writes:
> Subject: rebase -i: reword complains about empty commit despite --keep-empty
I had to read the title and then the log twice before understanding
that the above is not "change the complaint message" (i.e. "reword
complaints" spelled incorrectly) but is a description of the current
behaviour (i.e. "the command complains when 'reword' is used on an
empty commit") that is not accompanied by an evaluation ("ok, it
complains; are you saying it is a good thing or a bad thing?") or an
action ("if it is a bad thing, what are you doing about it?").
Perhaps
rebase -i: allow rewording an empty commit
or something?
> The to-do list command `reword` replays a commit like `pick` but lets
> the user also edit the commit's log message. If `--keep-empty` is
> passed as option to git-rebase--interactive, empty commits ought to
> be replayed without complaints. However, if the users chooses to
> reword an empty commit by changing the respective to-do list entry
> from
>
> pick fa1afe1 Empty commit
>
> to
>
> reword fa1afe1 Empty commit
>
> , then git-rebase--interactive suddenly fails to replay the empty
> commit. This is especially counterintuitive because `reword` is
> thought of as a `pick` that alters the log message in some way but
> nothing more and the unchanged to-do list entry would not fail.
>
> Handle `reword` by cherry-picking the named commit and editing the
> log message using
>
> git commit --allow-empty --amend
>
> instead of
>
> git commit --amend.
>
> Add test.
>
> Signed-off-by: Fabian Ruch
> ---
> git-rebase--interactive.sh| 2 +-
> t/t3404-rebase-interactive.sh | 8
> 2 files changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> index e733d7f..689400e 100644
> --- a/git-rebase--interactive.sh
> +++ b/git-rebase--interactive.sh
> @@ -504,7 +504,7 @@ do_next () {
>
> mark_action_done
> do_pick $sha1 "$rest"
> - git commit --amend --no-post-rewrite
> ${gpg_sign_opt:+"$gpg_sign_opt"} || {
> + git commit --allow-empty --amend --no-post-rewrite
> ${gpg_sign_opt:+"$gpg_sign_opt"} || {
> warn "Could not amend commit after successfully picking
> $sha1... $rest"
> exit_with_patch $sha1 1
> }
> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> index 8197ed2..9931143 100755
> --- a/t/t3404-rebase-interactive.sh
> +++ b/t/t3404-rebase-interactive.sh
> @@ -75,6 +75,14 @@ test_expect_success 'rebase --keep-empty' '
> test_line_count = 6 actual
> '
>
> +test_expect_success 'rebase --keep-empty' '
> + git checkout emptybranch &&
> + set_fake_editor &&
> + FAKE_LINES="1 reword 2" git rebase --keep-empty -i HEAD~2 &&
> + git log --oneline >actual &&
> + test_line_count = 6 actual
> +'
> +
> test_expect_success 'rebase -i with the exec command' '
> git checkout master &&
> (
--
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 v2 01/19] rebase -i: Failed reword prints redundant error message
Fabian Ruch writes:
> The to-do list command `reword` replays a commit like `pick` but lets
> the user also edit the commit's log message. If the edited log
> message is empty or is found ill-formatted by one of the commit
> hooks, git-rebase--interactive prints three error messages to the
> console.
>
> 1. The git-commit output, which contains all the output from hook
>scripts.
> 2. A rebase diagnosis saying at which task on the to-do list it
>got stuck.
> 3. Generic presumptions about what could have triggered the
>error.
>
> The third message contains redundant information and does not add any
> enlightenment either, which makes the output unnecessarily longish
> and different from the other command's output. For instance, this is
> what the output looks like if the log message is empty (contains
> duplicate Signed-off-by lines).
>
> (1.) Aborting commit due to empty commit message. (Duplicate
> Signed-off-by lines.)
> (2.) Could not amend commit after successfully picking fa1afe1... Some
> change
> (3.) This is most likely due to an empty commit message, or the
> pre-commit hook
> failed. If the pre-commit hook failed, you may need to resolve the
> issue before
> you are able to reword the commit.
>
> Discard the third message.
>
> It is true that a failed hook script might not output any diagnosis...
I think the message originally came from 0becb3e4 (rebase -i:
interrupt rebase when "commit --amend" failed during "reword",
2011-11-30); it seems that the primary point of the change was to
make sure it exits and the warning message may not have been well
thought out, but before discarding the result of somebody else's
work, it may not be a bad idea to ask just in case you may have
overlooked something (Andrew Wong Cc'ed).
> but then the generic message is not of much help either. Since this
> lack of information affects the built-in git commands for commit,
> merge and cherry-pick first, the solution would be to keep track of
> the failed hooks in their output so that the user knows which of her
> hooks require improvement.
>
> Signed-off-by: Fabian Ruch
> ---
> git-rebase--interactive.sh | 3 ---
> 1 file changed, 3 deletions(-)
>
> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> index 7e1eda0..e733d7f 100644
> --- a/git-rebase--interactive.sh
> +++ b/git-rebase--interactive.sh
> @@ -506,9 +506,6 @@ do_next () {
> do_pick $sha1 "$rest"
> git commit --amend --no-post-rewrite
> ${gpg_sign_opt:+"$gpg_sign_opt"} || {
> warn "Could not amend commit after successfully picking
> $sha1... $rest"
> - warn "This is most likely due to an empty commit
> message, or the pre-commit hook"
> - warn "failed. If the pre-commit hook failed, you may
> need to resolve the issue before"
> - warn "you are able to reword the commit."
> exit_with_patch $sha1 1
> }
> record_in_rewritten $sha1
--
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 00/14] Add submodule test harness
On 08/07/14 20:34, Jens Lehmann wrote:
> Am 07.07.2014 21:40, schrieb Torsten Bögershausen:
>> On 2014-07-07 19.05, Junio C Hamano wrote:
>>> Jens Lehmann writes:
>>>
Junio, do you want me to resend 02/14 without the non-portable "echo -n"
or could you just squash the following diff in?
>>>
>>> Amended locally here already; thanks, both.
>>
>> There seems to be some other trouble under Mac OS, not yet fully tracked
>> down,
>> (may be related to the "diff -r")
>
> Torsten sees failures of this kind under Mac OS:
>
> diff -r .git/modules/sub1/config sub1/.git/config
> 6d5
> < worktree = ../../../sub1
> 8a8
>> worktree = ../../../sub1
>
> So the config contains the same content, but the worktree setting moved
> to a different line. This seems to be the result of setting core.worktree
> in the test_git_directory_is_unchanged function just before the "diff -r",
> but only under Mac OS.
>
>> And Msysgit complains
>> error: fchmod on c:/xxxt/trash
>> directory.t7613-merge-submodule/submodule_update_repo/.git/modules/sub1/config.lock
>> failed: Function not implemented
>
> I'm not sure what this is about, seems to happen during the "cp -R" of
> the repo under .git/modules into the submodule.
I haven't looked into this at all, but from the above message, and
noting that fchmod() is not implemented in mingw (see compat/mingw.h
line 91), and the following:
$ git grep -n fchmod
compat/mingw.h:91:static inline int fchmod(int fildes, mode_t mode)
config.c:1639: if (fchmod(fd, st.st_mode & 0) < 0) {
config.c:1640: error("fchmod on %s failed: %s",
config.c:1818: if (fchmod(out_fd, st.st_mode & 0) < 0) {
config.c:1819: ret = error("fchmod on %s failed: %s",
$
[I happen to be on the pu branch at the moment, so YMMV!]
Both calls to fchmod() above are on config lock files, one
in git_config_set_multivar_in_file() and the other in
git_config_rename_section_in_file().
ATB,
Ramsay Jones
--
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/4] use new config API for worktree configurations of submodules
Heiko Voigt writes: > diff --git a/builtin/checkout.c b/builtin/checkout.c > index 07cf555..03ea20d 100644 > --- a/builtin/checkout.c > +++ b/builtin/checkout.c > @@ -18,6 +18,7 @@ > #include "xdiff-interface.h" > #include "ll-merge.h" > #include "resolve-undo.h" > +#include "submodule-config.h" > #include "submodule.h" > #include "argv-array.h" > Hmph. What is this change about? Nobody in checkout.c needs anything new, yet we add a new include? > diff --git a/diff.c b/diff.c > index f72769a..f692a3c 100644 > --- a/diff.c > +++ b/diff.c > @@ -13,6 +13,7 @@ > #include "utf8.h" > #include "userdiff.h" > #include "sigchain.h" > +#include "submodule-config.h" > #include "submodule.h" > #include "ll-merge.h" > #include "string-list.h" Likewise. It is somewhat unclear to me what real change that improves the life of end-users this series brings to us. The "test-submodule-config" test program obviously is new but that does not really count until we see real uses. -- 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 00/14] Add submodule test harness
Am 07.07.2014 21:40, schrieb Torsten Bögershausen: > On 2014-07-07 19.05, Junio C Hamano wrote: >> Jens Lehmann writes: >> >>> Junio, do you want me to resend 02/14 without the non-portable "echo -n" >>> or could you just squash the following diff in? >> >> Amended locally here already; thanks, both. > > There seems to be some other trouble under Mac OS, not yet fully tracked down, > (may be related to the "diff -r") Torsten sees failures of this kind under Mac OS: diff -r .git/modules/sub1/config sub1/.git/config 6d5 < worktree = ../../../sub1 8a8 > worktree = ../../../sub1 So the config contains the same content, but the worktree setting moved to a different line. This seems to be the result of setting core.worktree in the test_git_directory_is_unchanged function just before the "diff -r", but only under Mac OS. > And Msysgit complains > error: fchmod on c:/xxxt/trash > directory.t7613-merge-submodule/submodule_update_repo/.git/modules/sub1/config.lock > failed: Function not implemented I'm not sure what this is about, seems to happen during the "cp -R" of the repo under .git/modules into the submodule. I'm currently investigating both issues (the next steps probably being to install msysgit and to do some Git hacking on a Mac in the family). -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] t/Makefile: always test all lint targets when running tests
Am 07.07.2014 20:13, schrieb Junio C Hamano: > Jens Lehmann writes: > >> Only the two targets "test-lint-duplicates" and "test-lint-executable" are >> currently executed when running the test target. This was done on purpose >> when the TEST_LINT variable was added in 81127d74. But as this does not >> include the "test-lint-shell-syntax" target added the same day in commit >> c7ce70ac, it is easy to accidentally add non portable shell constructs >> without noticing that when running the test suite. > > I not running the lint-shell-syntax that is fundamentally flaky to > avoid false positives is very much on purpose. The flakiness is not > the fault of the implementor of the lint-shell-syntax, but comes > from the approach taken to pretend that simple pattern matching can > parse shell scripts. It may not complain on the current set of > scripts, but that is not really by design but by accident. > > So I am not very enthusiastic to see this change myself. Ok, I understand we do not want to lightly risk false positives. I just noticed that I accidentally forgot to sign off this series, so I'd resend just the first patch with a proper SOB, ok? -- 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 4/4] cache-tree: Write updated cache-tree after commit
Duy Nguyen writes:
> Writing cache tree early in prepare_index() does help hooks, but I
> would say hooks are uncommon case and we could add an option to
> update-index to explicitly rebuild cache-tree, then hooks that do diff
> a lot (or other operations that use cache-tree) could rebuild
> cache-tree by themselves.
Yes, "update-index --update-cache-tree" would be a good addition for
completeness; scripts working with plumbing should be able to do
what built-in Porcelains can. They can of course do "write-tree" in
the meantime so I do not see it as a very high priority, though.
This should apply on top of 'master', and if the series under
discussion turns out to be a good idea, the new call to
update-main-cache-tree I added to this code path should use the
option added by the series that only repairs parts of cache-trees
that can be repaird without writing out new trees, so it is just to
give hints to future people (iow I am not going to apply this patch
myself right now).
builtin/update-index.c | 8
1 file changed, 8 insertions(+)
diff --git a/builtin/update-index.c b/builtin/update-index.c
index ebea285..1ce2274 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -26,6 +26,7 @@ static int allow_remove;
static int allow_replace;
static int info_only;
static int force_remove;
+static int update_cache_tree;
static int verbose;
static int mark_valid_only;
static int mark_skip_worktree_only;
@@ -762,6 +763,8 @@ int cmd_update_index(int argc, const char **argv, const
char *prefix)
OPT_BIT(0, "unmerged", &refresh_args.flags,
N_("refresh even if index contains unmerged entries"),
REFRESH_UNMERGED),
+ OPT_BOOL(0, "update-cache-tree", &update_cache_tree,
+N_("update cache-tree before writing the result out")),
{OPTION_CALLBACK, 0, "refresh", &refresh_args, NULL,
N_("refresh stat information"),
PARSE_OPT_NOARG | PARSE_OPT_NONEG,
@@ -918,6 +921,11 @@ int cmd_update_index(int argc, const char **argv, const
char *prefix)
strbuf_release(&buf);
}
+ if (update_cache_tree && !unmerged_cache()) {
+ update_main_cache_tree(0);
+ active_cache_changed = 1; /* force write-out */
+ }
+
if (active_cache_changed) {
if (newfd < 0) {
if (refresh_args.flags & REFRESH_QUIET)
--
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 v20 00/48] Use ref transactions
Michael Haggerty writes: > Patches 01-19 -- ACK mhagger > Patches 20-42 -- I sent various comments, small to large, concerning > these patches > Patch 43 -- Needs more justification if it is to be acceptable > Patch 44 -- Depends on 43 > Patches 45-48 -- I didn't quite get to these, but... > > Perhaps it would be more appropriate for the rules about reference name > conflicts to be enforced by the backend, since it is the limitations of > the current backend that impose the restrictions. Would that make sense? > > On the other hand, removing the restrictions isn't simply a matter of > picking a different backend, because all Git repositories have to be > able to interact with each other. I'd say that "if you have foo/bar you cannot have foo" may have started as an implementation limitation, but the interoperability requirement with existing versions of Git and with existing repositories makes it necessary to enforce it the same way as other rules such as "you cannot have double-dots in name, e.g. foo..bar" or "no branches whose name begins with a dash", neither of which comes from any filesystem issues. That a rule can be loosened with one new backend does not at all mean it is a good idea to loosen it "because we can" in the first place. > I think it would be good to try to merge the first part of this patch > series to lock in some progress while we continue iterating on the > remainder. I'm satisfied that it is all going in the right direction > and I am thankful to Ronnie for pushing it forward. But handling > 48-patch series is very daunting and I would welcome a split. > > I'm not sure whether patches 01-19 are necessarily the right split > between merge-now/iterate-more; it is more or less an accident that I > stopped after patch 19 on an earlier review. Maybe Ronnie could propose > a logical subset of the commits as being ready to be merged to next in > the nearish term? Yeah, thanks for going through this, and I agree that we would be better off merging the earlier part first. -- 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 4/4] cache-tree: Write updated cache-tree after commit
Duy Nguyen writes:
> ... I know the
> "index_file" in prepare_to_commit() is probably "index.lock" or
> something, but that does not stop us from locking again
> ("index.lock.lock") if we want to update it.
We grabbed the lock on the real index and we have written out the
result of "update-index --refresh" to it (and closed), but we still
want to and do keep the lock while "add -i" works on it. And then
after "add -i" returns, we still have the lock on the real index and
the patch wants to write to it again to store the refreshed cache-tree
under that lock.
It may be the case that the API suite currently lacks a way to allow
the caller to reopen the same "index.lock" file after calling
write-locked-index(CLOSE_LOCK), and taking a lock on "index.lock" to
write into "index.lock.lock" and renaming it to "index.lock" could
be a workaround for it, but doesn't that sound a wrong workaround?
--
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 4/4] cache-tree: Write updated cache-tree after commit
David Turner writes:
> @@ -16,8 +16,26 @@ cmp_cache_tree () {
> # We don't bother with actually checking the SHA1:
> # test-dump-cache-tree already verifies that all existing data is
> # correct.
> -test_shallow_cache_tree () {
> - printf "SHA (%d entries, 0 subtrees)\n" $(git ls-files|wc -l) >expect
> &&
> +generate_expected_cache_tree () {
> + dir="$1${1:+/}" &&
> + parent="$2" &&
> + # ls-files might have foo/bar, foo/bar/baz, and foo/bar/quux
> + # We want to count only foo because it's the only direct child
> + subtrees=$(git ls-files|grep /|cut -d / -f 1|uniq) &&
> + subtree_count=$(echo "$subtrees"|awk '$1 {++c} END {print c}') &&
> + entries=$(git ls-files|wc -l) &&
> + printf "SHA $dir (%d entries, %d subtrees)\n" $entries $subtree_count &&
> + for subtree in $subtrees
> + do
> + cd "$subtree"
> + generate_expected_cache_tree "$dir$subtree" $dir || return 1
> + cd ..
If the || return 1 ever triggers, would the main test process end up
in an unexpected place? A test piece executes test_cache_tree whose
control eventually reaches here and returns failure; the next test
piece will start at a wrong directory, no?
> + done &&
> + dir=$parent
> +}
> +
> +test_cache_tree () {
> + generate_expected_cache_tree >expect &&
> cmp_cache_tree expect
> }
>
> @@ -33,14 +51,14 @@ test_no_cache_tree () {
> cmp_cache_tree expect
> }
>
> -test_expect_failure 'initial commit has cache-tree' '
> +test_expect_success 'initial commit has cache-tree' '
> test_commit foo &&
> - test_shallow_cache_tree
> + test_cache_tree
> '
>
> test_expect_success 'read-tree HEAD establishes cache-tree' '
> git read-tree HEAD &&
> - test_shallow_cache_tree
> + test_cache_tree
> '
--
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 3/4] cache-tree: subdirectory tests
David Turner writes:
> + printf "invalid %s ()\n" "" "$@"
> >expect &&
> + test-dump-cache-tree | \
> + sed -n -e "s/$_x40/SHA/" -e "s/[0-9]* subtrees//g" -e '/#(ref)/d' -e
> '/^invalid /p' >actual &&
You only show lines that begin with "invalid ". Does the first
"redact any object name to S H A" matter? Also do more than one "N
subtrees" appear on an output line?
> + test_cmp expect actual
> }
>
> test_no_cache_tree () {
> @@ -49,6 +50,25 @@ test_expect_success 'git-add invalidates cache-tree' '
> test_invalid_cache_tree
> '
>
> +test_expect_success 'git-add in subdir invalidates cache-tree' '
> + test_when_finished "git reset --hard; git read-tree HEAD" &&
> + mkdir dirx &&
> + echo "I changed this file" >dirx/foo &&
> + git add dirx/foo &&
> + test_invalid_cache_tree
> +'
> +
> +test_expect_success 'git-add in subdir does not invalidate sibling
> cache-tree' '
> + git tag no-children &&
> + test_when_finished "git reset --hard no-children; git read-tree HEAD" &&
> + mkdir dir1 dir2 &&
> + test_commit dir1/a &&
> + test_commit dir2/b &&
> + echo "I changed this file" >dir1/a &&
> + git add dir1/a &&
> + test_invalid_cache_tree dir1/
> +'
> +
> test_expect_success 'update-index invalidates cache-tree' '
> test_when_finished "git reset --hard; git read-tree HEAD" &&
> echo "I changed this file" >foo &&
--
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 4/4] cache-tree: Write updated cache-tree after commit
Duy Nguyen writes: > I wonder if we need to update_main_cache_tree() so many times in this > function. If I read the code correctly, all roads must lead to > update_main_cache_tree(0) in prepare_to_commit(). I think prepare-to-commit is too late; it does not want to know if the index it was given to create a tree out of is the one that the user will keep using after this invocation of "git commit" or just a temporary one used for partial commit. The cache-tree rebuilt there is what is recorded with commit_tree_extended() in cmd_commit(), but if you are doing a partial commit, these generic code paths are given a temporary index file on the filesystem to work on, and cache-tree in that will not help user's later operation. For three main uses of "git commit", prepare_index() does these: - "git commit -a" and "git commit -i $paths" update the index with the new contents from the working tree, fully builds cache-tree in-core to write out the tree objects, and writes the index file to the filesystem. Because this index is the one used after this invocation of "git commit" returns, we have a fully populated cache-tree after this happens. This code path is perfect and there is no need to change. - "git commit" commits the contents of the index as-is, so technically there is no reason for it to update the index on the filesystem at all, but it refreshes the cached stat data to help the "status" part of the command, and if it finds that stat data was stale, updates the index on the filesystem because it is wasteful not to do so. As we would be spending I/O cycles to update the index file in that case anyway, we also rebuild the cache-tree and include that in the updated index. When the cached stat data was already up-to-date, however, we do not update the index on the filesystem, so the series under discussion will change the trade-off by doing one more I/O to write out a new index to the filesystem only to update cache-tree. - "git commit $paths" updates the "real" index with given $paths and writes it out to the filesystem first. This is the index the user will use after "git commit" finishes; traditionally our trade-off was "populate cache-tree only when we do not have to spend any cycle only to do so (i.e. when we are writing trees anyway, or when we read from a tree), and let it degrade as paths are added, removed and modified" and we avoided populating cache-tree in this codepath. The series under discussion will change the trade-off here, too. After it updates this "real" index, it builds another temporary index that represents the tree state to be committed (starting from HEAD and updates with the given $paths), but that will be discarded and we do not want to spend any extra cycle to do anything only to make its later use more efficient (like writing updated cache-tree to it). > If we find out that > we have incomplete cache-tree before that call, we could write the > index one more time after that call, and that will make an extra I/O only to write out cache-tree to the temporary index that we will discard immediately after for a partial commit. -- 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 v20 00/48] Use ref transactions
On 06/20/2014 04:42 PM, Ronnie Sahlberg wrote: > This patch series can also be found at > https://github.com/rsahlberg/git/tree/ref-transactions > > This patch series is based on current master and expands on the transaction > API. It converts all ref updates, inside refs.c as well as external, to use > the > transaction API for updates. This makes most of the ref updates to become > atomic when there are failures locking or writing to a ref. > > This version completes the work to convert all ref updates to use > transactions. > Now that all updates are through transactions I will start working on > cleaning up the reading of refs and to create an api for managing reflogs but > all that will go in a different patch series. > > Version 20: > - Whitespace and style changes suggested by Jun. I spent most of the day on reviewing this patch series, but now I'm out of time again. Here is a summary from my point of view: Patches 01-19 -- ACK mhagger Patches 20-42 -- I sent various comments, small to large, concerning these patches Patch 43 -- Needs more justification if it is to be acceptable Patch 44 -- Depends on 43 Patches 45-48 -- I didn't quite get to these, but... Perhaps it would be more appropriate for the rules about reference name conflicts to be enforced by the backend, since it is the limitations of the current backend that impose the restrictions. Would that make sense? On the other hand, removing the restrictions isn't simply a matter of picking a different backend, because all Git repositories have to be able to interact with each other. So, I don't yet have a considered opinion on the matter. I think it would be good to try to merge the first part of this patch series to lock in some progress while we continue iterating on the remainder. I'm satisfied that it is all going in the right direction and I am thankful to Ronnie for pushing it forward. But handling 48-patch series is very daunting and I would welcome a split. I'm not sure whether patches 01-19 are necessarily the right split between merge-now/iterate-more; it is more or less an accident that I stopped after patch 19 on an earlier review. Maybe Ronnie could propose a logical subset of the commits as being ready to be merged to next in the nearish term? 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
[PATCH] line-log: use commit_list_append() instead of duplicating its code
Signed-off-by: Rene Scharfe --- line-log.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/line-log.c b/line-log.c index 1500101..afcc98d 100644 --- a/line-log.c +++ b/line-log.c @@ -1174,9 +1174,7 @@ static int process_ranges_merge_commit(struct rev_info *rev, struct commit *comm */ add_line_range(rev, parents[i], cand[i]); clear_commit_line_range(rev, commit); - commit->parents = xmalloc(sizeof(struct commit_list)); - commit->parents->item = parents[i]; - commit->parents->next = NULL; + commit_list_append(parents[i], &commit->parents); free(parents); free(cand); free_diffqueues(nparents, diffqueues); -- 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] diff-tree: call free_commit_list() instead of duplicating its code
Signed-off-by: Rene Scharfe
---
builtin/diff-tree.c | 8 ++--
1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/builtin/diff-tree.c b/builtin/diff-tree.c
index be6417d..ce0e019 100644
--- a/builtin/diff-tree.c
+++ b/builtin/diff-tree.c
@@ -22,14 +22,10 @@ static int stdin_diff_commit(struct commit *commit, char
*line, int len)
if (isspace(line[40]) && !get_sha1_hex(line+41, sha1)) {
/* Graft the fake parents locally to the commit */
int pos = 41;
- struct commit_list **pptr, *parents;
+ struct commit_list **pptr;
/* Free the real parent list */
- for (parents = commit->parents; parents; ) {
- struct commit_list *tmp = parents->next;
- free(parents);
- parents = tmp;
- }
+ free_commit_list(commit->parents);
commit->parents = NULL;
pptr = &(commit->parents);
while (line[pos] && !get_sha1_hex(line + pos, sha1)) {
--
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] http: Add Accept-Language header if possible
From: Yi EungJun
Add an Accept-Language header which indicates the user's preferred
languages defined by 'LANGUAGE' environment variable if the variable is
not empty.
Example:
LANGUAGE= -> ""
LANGUAGE=ko -> "Accept-Language: ko; q=1.000, *; q=0.001"
LANGUAGE=ko:en -> "Accept-Language: ko; q=1.000, en; q=0.999, *; q=0.001"
This gives git servers a chance to display remote error messages in
the user's preferred language.
---
http.c | 43 +++
t/t5550-http-fetch-dumb.sh | 10 ++
2 files changed, 53 insertions(+)
diff --git a/http.c b/http.c
index 3a28b21..c345616 100644
--- a/http.c
+++ b/http.c
@@ -983,6 +983,47 @@ static void extract_content_type(struct strbuf *raw,
struct strbuf *type,
strbuf_addstr(charset, "ISO-8859-1");
}
+/*
+ * Add an Accept-Language header which indicates user's preferred languages
+ * defined by 'LANGUAGE' environment variable if the variable is not empty.
+ *
+ * Example:
+ * LANGUAGE= -> ""
+ * LANGUAGE=ko -> "Accept-Language: ko; q=1.000, *; q=0.001"
+ * LANGUAGE=ko:en -> "Accept-Language: ko; q=1.000, en; q=0.999, *; q=0.001"
+ */
+static void add_accept_language(struct strbuf *buf)
+{
+ const char *p1, *p2;
+ float q = 1.000;
+
+ p1 = getenv("LANGUAGE");
+
+ if (p1 != NULL && p1[0] != '\0') {
+ strbuf_reset(buf);
+ strbuf_addstr(buf, "Accept-Language: ");
+ for (p2 = p1; q > 0.001; p2++) {
+ if ((*p2 == ':' || *p2 == '\0') && p1 != p2) {
+ if (q < 1.0) {
+ strbuf_addstr(buf, ", ");
+ }
+ strbuf_add(buf, p1, p2 - p1);
+ strbuf_addf(buf, "; q=%.3f", q);
+ q -= 0.001;
+ p1 = p2 + 1;
+
+ if (*p2 == '\0') {
+ break;
+ }
+ }
+ }
+ if (q < 1.0) {
+ strbuf_addstr(buf, ", ");
+ }
+ strbuf_addstr(buf, "*; q=0.001\r\n");
+ }
+}
+
/* http_request() targets */
#define HTTP_REQUEST_STRBUF0
#define HTTP_REQUEST_FILE 1
@@ -1020,6 +1061,8 @@ static int http_request(const char *url,
fwrite_buffer);
}
+ add_accept_language(&buf);
+
strbuf_addstr(&buf, "Pragma:");
if (options && options->no_cache)
strbuf_addstr(&buf, " no-cache");
diff --git a/t/t5550-http-fetch-dumb.sh b/t/t5550-http-fetch-dumb.sh
index ac71418..ea15158 100755
--- a/t/t5550-http-fetch-dumb.sh
+++ b/t/t5550-http-fetch-dumb.sh
@@ -196,5 +196,15 @@ test_expect_success 'reencoding is robust to whitespace
oddities' '
grep "this is the error message" stderr
'
+test_expect_success 'git client sends Accept-Language' '
+ GIT_CURL_VERBOSE=1 LANGUAGE=ko:en git clone
"$HTTPD_URL/accept/language" 2>actual
+ grep "^Accept-Language: ko; q=1.000, en; q=0.999, \*; q=0.001" actual
+'
+
+test_expect_success 'git client does not send Accept-Language' '
+ GIT_CURL_VERBOSE=1 LANGUAGE= git clone "$HTTPD_URL/accept/language"
2>actual
+ test_must_fail grep "^Accept-Language:" actual
+'
+
stop_httpd
test_done
--
2.0.1.473.gafdefd9.dirty
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v20 44/48] refs.c: call lock_ref_sha1_basic directly from commit
On 06/20/2014 04:43 PM, Ronnie Sahlberg wrote:
> Skip using the lock_any_ref_for_update wrapper and call lock_ref_sha1_basic
> directly from the commit function.
This commit is obviously hostage to whether commit 43/48 is kept.
> Reviewed-by: Jonathan Nieder
> Signed-off-by: Ronnie Sahlberg
> ---
> refs.c | 12 ++--
> 1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/refs.c b/refs.c
> index bccf8c3..f3fab37 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -3598,12 +3598,12 @@ int ref_transaction_commit(struct ref_transaction
> *transaction,
> for (i = 0; i < n; i++) {
> struct ref_update *update = updates[i];
>
> - update->lock = lock_any_ref_for_update(update->refname,
> -(update->have_old ?
> - update->old_sha1 :
> - NULL),
> -update->flags,
> -&update->type);
> + update->lock = lock_ref_sha1_basic(update->refname,
> +(update->have_old ?
> + update->old_sha1 :
> + NULL),
> +update->flags,
> +&update->type);
> if (!update->lock) {
> if (err)
> strbuf_addf(err, "Cannot lock the ref '%s'.",
>
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: [PATCH v20 43/48] refs.c: move the check for valid refname to lock_ref_sha1_basic
On 06/20/2014 04:43 PM, Ronnie Sahlberg wrote:
> Move the check for check_refname_format from lock_any_ref_for_update
> to lock_ref_sha1_basic. At some later stage we will get rid of
> lock_any_ref_for_update completely.
>
> If lock_ref_sha1_basic fails the check_refname_format test, set errno to
> EINVAL before returning NULL. This to guarantee that we will not return an
> error without updating errno.
>
> This leaves lock_any_ref_for_updates as a no-op wrapper which could be
> removed.
> But this wrapper is also called from an external caller and we will soon
> make changes to the signature to lock_ref_sha1_basic that we do not want to
> expose to that caller.
>
> This changes semantics for lock_ref_sha1_basic slightly. With this change
> it is no longer possible to open a ref that has a badly name which breaks
s/badly name/bad name,/
> any codepaths that tries to open and repair badly named refs. The normal refs
s/tries/try/
> API should not allow neither creating nor accessing refs with invalid names.
s/not allow neither/allow neither/
> If we need such recovery code we could add it as an option to git fsck and
> have
> git fsck be the only sanctioned way of bypassing the normal API and checks.
I like the sentiment, but in the real world I'm not sure we can take
such a step based only on good intentions. Which callers would be
affected? Where is this "git fsck" code that would be needed to help
people rescue their repos?
I can also imagine that we will tighten up the check_refname_format
checks in the future; for example, I think it would be a good idea to
prohibit reference names that start with '-' because it is almost
impossible to work with them (their names look like command-line
options). If we ever make a change like that, we will need some amount
of tolerance in git versions around the transition.
So...I like the idea of enforcing refname checks at the lowest level
possible, but I think that the change you propose is too abrupt. I
think it needs either more careful analysis showing that it won't hurt
anybody, or some kind of tooling or non-strict mode that people can use
to fix their repositories.
Michael
> Signed-off-by: Ronnie Sahlberg
> ---
> refs.c | 7 +--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/refs.c b/refs.c
> index 389a55f..bccf8c3 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -2088,6 +2088,11 @@ static struct ref_lock *lock_ref_sha1_basic(const char
> *refname,
> int missing = 0;
> int attempts_remaining = 3;
>
> + if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) {
> + errno = EINVAL;
> + return NULL;
> + }
> +
> lock = xcalloc(1, sizeof(struct ref_lock));
> lock->lock_fd = -1;
>
> @@ -2179,8 +2184,6 @@ struct ref_lock *lock_any_ref_for_update(const char
> *refname,
>const unsigned char *old_sha1,
>int flags, int *type_p)
> {
> - if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL))
> - return NULL;
> return lock_ref_sha1_basic(refname, old_sha1, flags, type_p);
> }
>
>
--
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: [PATCH v20 41/48] refs.c: pass the ref log message to _create/delete/update instead of _commit
On 06/20/2014 04:43 PM, Ronnie Sahlberg wrote: > Change the reference transactions so that we pass the reflog message > through to the create/delete/update function instead of the commit message. > This allows for individual messages for each change in a multi ref > transaction. > > Reviewed-by: Jonathan Nieder > Signed-off-by: Ronnie Sahlberg > --- > [...] > diff --git a/refs.h b/refs.h > index 3b321c2..f24b2c1 100644 > --- a/refs.h > +++ b/refs.h Would you please document the msg parameter in the block comment that precedes these three declarations? Especially important is the fact that the functions make internal copies of msg, so the caller retains ownership of its copy. You might also mention what happens if msg is NULL (which, as far as I can see, is that a reflog entry is created anyway (except in the case of a delete) but that the entry doesn't contain an explanation). > @@ -297,7 +297,7 @@ int ref_transaction_update(struct ref_transaction > *transaction, > const char *refname, > const unsigned char *new_sha1, > const unsigned char *old_sha1, > -int flags, int have_old, > +int flags, int have_old, const char *msg, > struct strbuf *err); > > /* > @@ -312,7 +312,7 @@ int ref_transaction_update(struct ref_transaction > *transaction, > int ref_transaction_create(struct ref_transaction *transaction, > const char *refname, > const unsigned char *new_sha1, > -int flags, > +int flags, const char *msg, > struct strbuf *err); It is noteworthy that ref_transaction_delete() accepts a msg parameter, even though we currently delete a reference's entire reflog when the reference is deleted. I prefer to think of this as a shortcoming of the current reference backend, from which future backends hopefully will not suffer. So I like this design choice. However, I think it is worth noting this dichotomy in the commit message and perhaps also in the function documentation. > /* > @@ -326,7 +326,7 @@ int ref_transaction_create(struct ref_transaction > *transaction, > int ref_transaction_delete(struct ref_transaction *transaction, > const char *refname, > const unsigned char *old_sha1, > -int flags, int have_old, > +int flags, int have_old, const char *msg, > struct strbuf *err); > > /* > @@ -335,7 +335,7 @@ int ref_transaction_delete(struct ref_transaction > *transaction, > * problem. > */ > int ref_transaction_commit(struct ref_transaction *transaction, > -const char *msg, struct strbuf *err); > +struct strbuf *err); > > /* > * Free an existing transaction and all associated data. > [...] 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 v20 40/48] refs.c: add an err argument to delete_ref_loose
On 06/20/2014 04:43 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.
The change to warn_if_unremovable() is orthogonal to the rest of the
commit and should be a separate commit.
> Signed-off-by: Ronnie Sahlberg
> ---
> refs.c| 33 -
> wrapper.c | 14 ++
> 2 files changed, 34 insertions(+), 13 deletions(-)
>
> diff --git a/refs.c b/refs.c
> index 92a06d4..c7d1f3e 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -2544,16 +2544,38 @@ int repack_without_refs(const char **refnames, int n,
> struct strbuf *err)
> return ret;
> }
>
> -static int delete_ref_loose(struct ref_lock *lock, int flag)
> +static int add_err_if_unremovable(const char *op, const char *file,
> + struct strbuf *e, int rc)
This function is only used once. Given also that its purpose is not
that obvious from its signature, it seems to me that the code would be
easier to read if it were inlined.
> +{
> + int err = errno;
> + if (rc < 0 && errno != ENOENT) {
> + strbuf_addf(e, "unable to %s %s: %s",
> + op, file, strerror(errno));
> + errno = err;
> + return -1;
> + }
> + return 0;
> +}
> +
> +static int unlink_or_err(const char *file, struct strbuf *err)
The name of this function is misleading; it sounds like it will try to
unlink the file and if not possible call error(). Maybe a name like
"unlink_or_report" would be less prejudicial.
It might also make sense to move this function to wrapper.c and
implement unlink_or_warn() in terms of it rather than vice versa.
> +{
> + if (err)
> + return add_err_if_unremovable("unlink", file, err,
> + unlink(file));
> + else
> + return unlink_or_warn(file);
> +}
> +
> +static int delete_ref_loose(struct ref_lock *lock, int flag, struct strbuf
> *err)
> {
> if (!(flag & REF_ISPACKED) || flag & REF_ISSYMREF) {
> /* loose */
> - int err, i = strlen(lock->lk->filename) - 5; /* .lock */
> + int res, i = strlen(lock->lk->filename) - 5; /* .lock */
>
> lock->lk->filename[i] = 0;
> - err = unlink_or_warn(lock->lk->filename);
> + res = unlink_or_err(lock->lk->filename, err);
> lock->lk->filename[i] = '.';
> - if (err && errno != ENOENT)
> + if (res)
> return 1;
> }
> return 0;
> @@ -3603,7 +3625,8 @@ int ref_transaction_commit(struct ref_transaction
> *transaction,
> struct ref_update *update = updates[i];
>
> if (update->lock) {
> - ret |= delete_ref_loose(update->lock, update->type);
> + ret |= delete_ref_loose(update->lock, update->type,
> + err);
> if (!(update->flags & REF_ISPRUNING))
> delnames[delnum++] = update->lock->ref_name;
> }
> diff --git a/wrapper.c b/wrapper.c
> index bc1bfb8..740e193 100644
> --- a/wrapper.c
> +++ b/wrapper.c
> @@ -429,14 +429,12 @@ int xmkstemp_mode(char *template, int mode)
>
> static int warn_if_unremovable(const char *op, const char *file, int rc)
> {
> - if (rc < 0) {
> - int err = errno;
> - if (ENOENT != err) {
> - warning("unable to %s %s: %s",
> - op, file, strerror(errno));
> - errno = err;
> - }
> - }
> + int err;
> + if (rc >= 0 || errno == ENOENT)
> + return rc;
> + err = errno;
> + warning("unable to %s %s: %s", op, file, strerror(errno));
> + errno = err;
> return rc;
> }
>
>
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: [PATCH v20 39/48] refs.c: make delete_ref use a transaction
On 06/20/2014 04:43 PM, Ronnie Sahlberg wrote:
> Change delete_ref to use a ref transaction for the deletion. At the same time
> since we no longer have any callers of repack_without_ref we can now delete
> this function.
>
> Change delete_ref to return 0 on success and 1 on failure instead of the
> previous 0 on success either 1 or -1 on failure.
>
> Reviewed-by: Jonathan Nieder
> Signed-off-by: Ronnie Sahlberg
> ---
> refs.c | 34 +-
> 1 file changed, 13 insertions(+), 21 deletions(-)
>
> diff --git a/refs.c b/refs.c
> index 3d070d5..92a06d4 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -2544,11 +2544,6 @@ int repack_without_refs(const char **refnames, int n,
> struct strbuf *err)
> return ret;
> }
>
> -static int repack_without_ref(const char *refname)
> -{
> - return repack_without_refs(&refname, 1, NULL);
> -}
> -
> static int delete_ref_loose(struct ref_lock *lock, int flag)
> {
> if (!(flag & REF_ISPACKED) || flag & REF_ISSYMREF) {
> @@ -2566,24 +2561,21 @@ static int delete_ref_loose(struct ref_lock *lock,
> int flag)
>
> int delete_ref(const char *refname, const unsigned char *sha1, int delopt)
> {
> - struct ref_lock *lock;
> - int ret = 0, flag = 0;
> + struct ref_transaction *transaction;
> + struct strbuf err = STRBUF_INIT;
>
> - lock = lock_ref_sha1_basic(refname, sha1, delopt, &flag);
The old code checked that the old value of refname was sha1, regardless
of whether sha1 was null_sha1. Presumably callers never set sha1 to
null_sha1...
> - if (!lock)
> + transaction = ref_transaction_begin(&err);
> + if (!transaction ||
> + ref_transaction_delete(transaction, refname, sha1, delopt,
> +sha1 && !is_null_sha1(sha1), &err) ||
...But the new code explicitly skips the check if sha1 is null_sha1.
This shouldn't make a practical difference, because presumably callers
never set sha1 to null_sha1. But given that the new policy elsewhere
for "delete" updates is that it is an error for old_sha1 to equal
null_sha1, it seems to me that this extra check shouldn't be here. So I
think this should be changed to
ref_transaction_delete(transaction, refname, sha1, delopt,
!!sha1, &err) ||
> [...]
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: [PATCH v20 37/48] refs.c: remove lock_ref_sha1
On 06/20/2014 04:43 PM, Ronnie Sahlberg wrote: > lock_ref_sha1 was only called from one place in refc.c and only provided > a check that the refname was sane before adding back the initial "refs/" > part of the ref path name, the initial "refs/" that this caller had already > stripped off before calling lock_ref_sha1. > [...] I'm especially glad to see this ugly function disappear! 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: [PATCH v20 33/48] walker.c: use ref transaction for ref updates
On 06/20/2014 04:43 PM, Ronnie Sahlberg wrote:
> Switch to using ref transactions in walker_fetch(). As part of the refactoring
> to use ref transactions we also fix a potential memory leak where in the
> original code if write_ref_sha1() would fail we would end up returning from
> the function without free()ing the msg string.
>
> Note that this function is only called when fetching from a remote HTTP
> repository onto the local (most of the time single-user) repository which
> likely means that the type of collissions that the previous locking would
s/collissions/collisions/
> protect against and cause the fetch to fail for to be even more rare.
Grammatico: s/to be/are/ ?
> Reviewed-by: Jonathan Nieder
> Signed-off-by: Ronnie Sahlberg
> ---
> walker.c | 59 +++
> 1 file changed, 35 insertions(+), 24 deletions(-)
>
> diff --git a/walker.c b/walker.c
> index 1dd86b8..60d9f9e 100644
> --- a/walker.c
> +++ b/walker.c
> @@ -251,39 +251,36 @@ void walker_targets_free(int targets, char **target,
> const char **write_ref)
> int walker_fetch(struct walker *walker, int targets, char **target,
>const char **write_ref, const char *write_ref_log_details)
> {
> - struct ref_lock **lock = xcalloc(targets, sizeof(struct ref_lock *));
> + struct strbuf ref_name = STRBUF_INIT;
> + struct strbuf err = STRBUF_INIT;
> + struct ref_transaction *transaction = NULL;
> unsigned char *sha1 = xmalloc(targets * 20);
> - char *msg;
> - int ret;
> + char *msg = NULL;
> int i;
>
> save_commit_buffer = 0;
>
> - for (i = 0; i < targets; i++) {
> - if (!write_ref || !write_ref[i])
> - continue;
> -
> - lock[i] = lock_ref_sha1(write_ref[i], NULL);
> - if (!lock[i]) {
> - error("Can't lock ref %s", write_ref[i]);
> - goto unlock_and_fail;
> + if (write_ref) {
> + transaction = ref_transaction_begin(&err);
> + if (!transaction) {
> + error("%s", err.buf);
> + goto rollback_and_fail;
> }
> }
> -
Is there some reason why the transaction cannot be built up during a
single iteration over targets, thereby also avoiding the need for the
sha1[20*i] stuff? This seems like exactly the kind of situation where
transactions should *save* code. But perhaps I've overlooked a
dependency between the two loops.
> if (!walker->get_recover)
> for_each_ref(mark_complete, NULL);
>
> for (i = 0; i < targets; i++) {
> if (interpret_target(walker, target[i], &sha1[20 * i])) {
> error("Could not interpret response from server '%s' as
> something to pull", target[i]);
> - goto unlock_and_fail;
> + goto rollback_and_fail;
> }
> if (process(walker, lookup_unknown_object(&sha1[20 * i])))
> - goto unlock_and_fail;
> + goto rollback_and_fail;
> }
>
> if (loop(walker))
> - goto unlock_and_fail;
> + goto rollback_and_fail;
>
> if (write_ref_log_details) {
> msg = xmalloc(strlen(write_ref_log_details) + 12);
> @@ -294,19 +291,33 @@ int walker_fetch(struct walker *walker, int targets,
> char **target,
> for (i = 0; i < targets; i++) {
> if (!write_ref || !write_ref[i])
> continue;
> - ret = write_ref_sha1(lock[i], &sha1[20 * i], msg ? msg : "fetch
> (unknown)");
> - lock[i] = NULL;
> - if (ret)
> - goto unlock_and_fail;
> + strbuf_reset(&ref_name);
> + strbuf_addf(&ref_name, "refs/%s", write_ref[i]);
> + if (ref_transaction_update(transaction, ref_name.buf,
> +&sha1[20 * i], NULL, 0, 0,
> +&err)) {
> + error("%s", err.buf);
> + goto rollback_and_fail;
> + }
> + }
> + if (write_ref) {
> + if (ref_transaction_commit(transaction,
> +msg ? msg : "fetch (unknown)",
> +&err)) {
> + error("%s", err.buf);
> + goto rollback_and_fail;
> + }
> + ref_transaction_free(transaction);
> }
> - free(msg);
>
> + free(msg);
> return 0;
>
> -unlock_and_fail:
> - for (i = 0; i < targets; i++)
> - if (lock[i])
> - unlock_ref(lock[i]);
> +rollback_and_fail:
> + ref_transaction_free(transaction);
> + free(msg);
> + strbuf_release(&err);
> + strbuf_release(&ref_name);
>
> return -1;
> }
>
Michael
--
Michael Haggerty
mhag...@alum.mit.edu
--
Re: [PATCH v20 31/48] receive-pack.c: use a reference transaction for updating the refs
On 06/20/2014 04:43 PM, Ronnie Sahlberg wrote:
> Wrap all the ref updates inside a transaction.
>
> Signed-off-by: Ronnie Sahlberg
> ---
> builtin/receive-pack.c | 96
> +-
> 1 file changed, 63 insertions(+), 33 deletions(-)
>
> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
> index c323081..b51f8ae 100644
> --- a/builtin/receive-pack.c
> +++ b/builtin/receive-pack.c
> [...]
> @@ -647,6 +654,9 @@ static void check_aliased_update(struct command *cmd,
> struct string_list *list)
> char cmd_oldh[41], cmd_newh[41], dst_oldh[41], dst_newh[41];
> int flag;
>
> + if (cmd->error_string)
> + die("BUG: check_alised_update called with failed cmd");
s/check_alised_update/check_aliased_update/
> [...]
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
[PATCH] path: for clarity, rename get_pathname to get_path_buffer
The get_pathname function does not really return path name but rather
a buffer to store pathname in. As such, current name is a bit
confusing. Change the name as to make it clearer what the function is
doing.
Signed-off-by: Michal Nazarewicz
---
path.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
This time sent with Junio's correct address.
diff --git a/path.c b/path.c
index bc804a3..70e2f85 100644
--- a/path.c
+++ b/path.c
@@ -16,7 +16,7 @@ static int get_st_mode_bits(const char *path, int *mode)
static char bad_path[] = "/bad-path/";
-static char *get_pathname(void)
+static char *get_path_buffer(void)
{
static char pathname_array[4][PATH_MAX];
static int index;
@@ -108,7 +108,7 @@ char *mkpath(const char *fmt, ...)
{
va_list args;
unsigned len;
- char *pathname = get_pathname();
+ char *pathname = get_path_buffer();
va_start(args, fmt);
len = vsnprintf(pathname, PATH_MAX, fmt, args);
@@ -120,7 +120,7 @@ char *mkpath(const char *fmt, ...)
char *git_path(const char *fmt, ...)
{
- char *pathname = get_pathname();
+ char *pathname = get_path_buffer();
va_list args;
char *ret;
@@ -158,7 +158,7 @@ void home_config_paths(char **global, char **xdg, char
*file)
char *git_path_submodule(const char *path, const char *fmt, ...)
{
- char *pathname = get_pathname();
+ char *pathname = get_path_buffer();
struct strbuf buf = STRBUF_INIT;
const char *git_dir;
va_list args;
--
2.0.0.526.g5318336
--
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 v20 30/48] refs.c: change update_ref to use a transaction
On 06/20/2014 04:43 PM, Ronnie Sahlberg wrote:
> Change the update_ref helper function to use a ref transaction internally.
>
> Reviewed-by: Jonathan Nieder
> Signed-off-by: Ronnie Sahlberg
> ---
> refs.c | 28
> 1 file changed, 24 insertions(+), 4 deletions(-)
>
> diff --git a/refs.c b/refs.c
> index 8c695ba..4bdccc5 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -3524,11 +3524,31 @@ int update_ref(const char *action, const char
> *refname,
> const unsigned char *sha1, const unsigned char *oldval,
> int flags, enum action_on_err onerr)
> {
> - struct ref_lock *lock;
> - lock = update_ref_lock(refname, oldval, flags, NULL, onerr);
> - if (!lock)
> + struct ref_transaction *t;
> + struct strbuf err = STRBUF_INIT;
> +
> + t = ref_transaction_begin(&err);
> + if (!t ||
> + ref_transaction_update(t, refname, sha1, oldval, flags,
> +!!oldval, &err) ||
> + ref_transaction_commit(t, action, &err)) {
> + const char *str = "update_ref failed for ref '%s': %s";
> +
> + ref_transaction_free(t);
> + switch (onerr) {
> + case UPDATE_REFS_MSG_ON_ERR:
> + error(str, refname, err.buf);
> + break;
> + case UPDATE_REFS_DIE_ON_ERR:
> + die(str, refname, err.buf);
> + break;
> + case UPDATE_REFS_QUIET_ON_ERR:
> + break;
> + }
> + strbuf_release(&err);
> return 1;
> - return update_ref_write(action, refname, sha1, lock, NULL, onerr);
> + }
> + return 0;
> }
>
Should this function be scheduled for the "take strbuf *err argument"
treatment instead of continuing to use an action_on_err parameter?
(Maybe you've changed this later in the patch series?)
I'm not saying this change has to be part of the current patch series,
but let's consider it for the future.
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
[PATCH] path: for clarity, rename get_pathname to get_path_buffer
The get_pathname function does not really return path name but rather
a buffer to store pathname in. As such, current name is a bit
confusing. Change the name as to make it clearer what the function is
doing.
Signed-off-by: Michal Nazarewicz
---
path.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/path.c b/path.c
index bc804a3..70e2f85 100644
--- a/path.c
+++ b/path.c
@@ -16,7 +16,7 @@ static int get_st_mode_bits(const char *path, int *mode)
static char bad_path[] = "/bad-path/";
-static char *get_pathname(void)
+static char *get_path_buffer(void)
{
static char pathname_array[4][PATH_MAX];
static int index;
@@ -108,7 +108,7 @@ char *mkpath(const char *fmt, ...)
{
va_list args;
unsigned len;
- char *pathname = get_pathname();
+ char *pathname = get_path_buffer();
va_start(args, fmt);
len = vsnprintf(pathname, PATH_MAX, fmt, args);
@@ -120,7 +120,7 @@ char *mkpath(const char *fmt, ...)
char *git_path(const char *fmt, ...)
{
- char *pathname = get_pathname();
+ char *pathname = get_path_buffer();
va_list args;
char *ret;
@@ -158,7 +158,7 @@ void home_config_paths(char **global, char **xdg, char
*file)
char *git_path_submodule(const char *path, const char *fmt, ...)
{
- char *pathname = get_pathname();
+ char *pathname = get_path_buffer();
struct strbuf buf = STRBUF_INIT;
const char *git_dir;
va_list args;
--
2.0.0.526.g5318336
--
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 v20 25/48] replace.c: use the ref transaction functions for updates
On 06/20/2014 04:43 PM, Ronnie Sahlberg wrote:
> Update replace.c to use ref transactions for updates.
>
> Reviewed-by: Jonathan Nieder
> Signed-off-by: Ronnie Sahlberg
> ---
> builtin/replace.c | 15 +--
> 1 file changed, 9 insertions(+), 6 deletions(-)
>
> diff --git a/builtin/replace.c b/builtin/replace.c
> index 1bb491d..7528f3d 100644
> --- a/builtin/replace.c
> +++ b/builtin/replace.c
> @@ -153,7 +153,8 @@ static int replace_object_sha1(const char *object_ref,
> unsigned char prev[20];
> enum object_type obj_type, repl_type;
> char ref[PATH_MAX];
> - struct ref_lock *lock;
> + struct ref_transaction *transaction;
> + struct strbuf err = STRBUF_INIT;
>
> obj_type = sha1_object_info(object, NULL);
> repl_type = sha1_object_info(repl, NULL);
> @@ -166,12 +167,14 @@ static int replace_object_sha1(const char *object_ref,
>
> check_ref_valid(object, prev, ref, sizeof(ref), force);
>
> - lock = lock_any_ref_for_update(ref, prev, 0, NULL);
> - if (!lock)
> - die("%s: cannot lock the ref", ref);
> - if (write_ref_sha1(lock, repl, NULL) < 0)
> - die("%s: cannot update the ref", ref);
> + transaction = ref_transaction_begin(&err);
> + if (!transaction ||
> + ref_transaction_update(transaction, ref, repl, prev,
> +0, !is_null_sha1(prev), &err) ||
Same problem here. You need
s/!is_null_sha1(prev)/1/
> + ref_transaction_commit(transaction, NULL, &err))
> + die("%s", err.buf);
>
> + ref_transaction_free(transaction);
> return 0;
> }
>
>
--
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: [PATCH v20 24/48] tag.c: use ref transactions when doing updates
On 06/20/2014 04:43 PM, Ronnie Sahlberg wrote:
> Change tag.c to use ref transactions for all ref updates.
>
> Reviewed-by: Jonathan Nieder
> Signed-off-by: Ronnie Sahlberg
> ---
> builtin/tag.c | 15 +--
> 1 file changed, 9 insertions(+), 6 deletions(-)
>
> diff --git a/builtin/tag.c b/builtin/tag.c
> index c6e8a71..c9bfc9a 100644
> --- a/builtin/tag.c
> +++ b/builtin/tag.c
> @@ -548,7 +548,6 @@ int cmd_tag(int argc, const char **argv, const char
> *prefix)
> struct strbuf ref = STRBUF_INIT;
> unsigned char object[20], prev[20];
> const char *object_ref, *tag;
> - struct ref_lock *lock;
> struct create_tag_options opt;
> char *cleanup_arg = NULL;
> int annotate = 0, force = 0, lines = -1;
> @@ -556,6 +555,8 @@ int cmd_tag(int argc, const char **argv, const char
> *prefix)
> const char *msgfile = NULL, *keyid = NULL;
> struct msg_arg msg = { 0, STRBUF_INIT };
> struct commit_list *with_commit = NULL;
> + struct ref_transaction *transaction;
> + struct strbuf err = STRBUF_INIT;
> struct option options[] = {
> OPT_CMDMODE('l', "list", &cmdmode, N_("list tag names"), 'l'),
> { OPTION_INTEGER, 'n', NULL, &lines, N_("n"),
> @@ -701,11 +702,13 @@ int cmd_tag(int argc, const char **argv, const char
> *prefix)
> if (annotate)
> create_tag(object, tag, &buf, &opt, prev, object);
>
> - lock = lock_any_ref_for_update(ref.buf, prev, 0, NULL);
> - if (!lock)
> - die(_("%s: cannot lock the ref"), ref.buf);
> - if (write_ref_sha1(lock, object, NULL) < 0)
> - die(_("%s: cannot update the ref"), ref.buf);
> + transaction = ref_transaction_begin(&err);
> + if (!transaction ||
> + ref_transaction_update(transaction, ref.buf, object, prev,
> +0, !is_null_sha1(prev), &err) ||
Similar to the error in sequencer.c a few patches later (explained in
more detail in my comment on that patch), here you only do a check if
!is_null_sha1(prev), whereas the old code always did the check. I think
you want
ref_transaction_update(transaction, ref.buf, object, prev,
0, 1, &err) ||
Please check whether you have made the same mistake in other patches.
> [...]
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: [PATCH v20 27/48] sequencer.c: use ref transactions for all ref updates
On 06/20/2014 04:43 PM, Ronnie Sahlberg wrote:
> Change to use ref transactions for all updates to refs.
>
> Reviewed-by: Jonathan Nieder
> Signed-off-by: Ronnie Sahlberg
> ---
> sequencer.c | 24
> 1 file changed, 16 insertions(+), 8 deletions(-)
>
> diff --git a/sequencer.c b/sequencer.c
> index 0a80c58..fd8acaf 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -272,23 +272,31 @@ static int error_dirty_index(struct replay_opts *opts)
> static int fast_forward_to(const unsigned char *to, const unsigned char
> *from,
> int unborn, struct replay_opts *opts)
> {
> - struct ref_lock *ref_lock;
> + struct ref_transaction *transaction;
> struct strbuf sb = STRBUF_INIT;
> - int ret;
> + struct strbuf err = STRBUF_INIT;
>
> read_cache();
> if (checkout_fast_forward(from, to, 1))
> exit(1); /* the callee should have complained already */
> - ref_lock = lock_any_ref_for_update("HEAD", unborn ? null_sha1 : from,
> -0, NULL);
> - if (!ref_lock)
> - return error(_("Failed to lock HEAD during fast_forward_to"));
I think you've changed the semantics when unborn is set. Please note
that lock_any_ref_for_update() behaves differently if old_sha1 is NULL
(when no check is done) vs. when it is null_sha1 (when it verifies that
the reference didn't previously exist). So when unborn is true, the old
code verifies that the reference previously didn't exist...
>
> strbuf_addf(&sb, "%s: fast-forward", action_name(opts));
> - ret = write_ref_sha1(ref_lock, to, sb.buf);
> +
> + transaction = ref_transaction_begin(&err);
> + if (!transaction ||
> + ref_transaction_update(transaction, "HEAD", to, from,
> +0, !unborn, &err) ||
...whereas when unborn is true, the new code does no check at all. I
think you want
ref_transaction_update(transaction, "HEAD",
to, unborn ? null_sha1 : from,
0, 1, &err) ||
> + ref_transaction_commit(transaction, sb.buf, &err)) {
> + ref_transaction_free(transaction);
> + error("%s", err.buf);
> + strbuf_release(&sb);
> + strbuf_release(&err);
> + return -1;
> + }
>
> strbuf_release(&sb);
> - return ret;
> + ref_transaction_free(transaction);
> + return 0;
> }
>
> static int do_recursive_merge(struct commit *base, struct commit *next,
>
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 v20 23/48] refs.c: add transaction.status and track OPEN/CLOSED/ERROR
On 06/20/2014 04:43 PM, Ronnie Sahlberg wrote:
> Track the status of a transaction in a new status field. Check the field for
The status field is not set or used anywhere. The field that you use is
"state".
> sanity, i.e. that status must be OPEN when _commit/_create/_delete or
> _update is called or else die(BUG:...)
>
> Signed-off-by: Ronnie Sahlberg
> ---
> refs.c | 40 +++-
> 1 file changed, 39 insertions(+), 1 deletion(-)
>
> diff --git a/refs.c b/refs.c
> index 9cb7908..8c695ba 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -3387,6 +3387,25 @@ struct ref_update {
> };
>
> /*
> + * Transaction states.
> + * OPEN: The transaction is in a valid state and can accept new updates.
> + * An OPEN transaction can be committed.
> + * CLOSED: If an open transaction is successfully committed the state will
> + * change to CLOSED. No further changes can be made to a CLOSED
> + * transaction.
> + * CLOSED means that all updates have been successfully committed and
> + * the only thing that remains is to free the completed transaction.
> + * ERROR: The transaction has failed and is no longer committable.
> + * No further changes can be made to a CLOSED transaction and it must
> + * be rolled back using transaction_free.
> + */
> +enum ref_transaction_state {
> + REF_TRANSACTION_OPEN = 0,
> + REF_TRANSACTION_CLOSED = 1,
> + REF_TRANSACTION_ERROR = 2,
> +};
> +
> +/*
> * Data structure for holding a reference transaction, which can
> * consist of checks and updates to multiple references, carried out
> * as atomically as possible. This structure is opaque to callers.
> @@ -3395,6 +3414,8 @@ struct ref_transaction {
> struct ref_update **updates;
> size_t alloc;
> size_t nr;
> + enum ref_transaction_state state;
> + int status;
The status field should probably be deleted.
> };
>
> struct ref_transaction *ref_transaction_begin(struct strbuf *err)
> @@ -3437,6 +3458,9 @@ int ref_transaction_update(struct ref_transaction
> *transaction,
> {
> struct ref_update *update;
>
> + if (transaction->state != REF_TRANSACTION_OPEN)
> + die("BUG: update called for transaction that is not open");
> +
> if (have_old && !old_sha1)
> die("BUG: have_old is true but old_sha1 is NULL");
>
> @@ -3457,6 +3481,9 @@ int ref_transaction_create(struct ref_transaction
> *transaction,
> {
> 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");
>
> @@ -3477,6 +3504,9 @@ int ref_transaction_delete(struct ref_transaction
> *transaction,
> {
> 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");
>
> @@ -3532,8 +3562,13 @@ int ref_transaction_commit(struct ref_transaction
> *transaction,
> int n = transaction->nr;
> struct ref_update **updates = transaction->updates;
>
> - if (!n)
> + if (transaction->state != REF_TRANSACTION_OPEN)
> + die("BUG: commit called for transaction that is not open");
> +
> + if (!n) {
> + transaction->state = REF_TRANSACTION_CLOSED;
> return 0;
> + }
>
> /* Allocate work space */
> delnames = xmalloc(sizeof(*delnames) * n);
> @@ -3595,6 +3630,9 @@ int ref_transaction_commit(struct ref_transaction
> *transaction,
> clear_loose_ref_cache(&ref_cache);
>
> cleanup:
> + transaction->state = ret ? REF_TRANSACTION_ERROR
> + : REF_TRANSACTION_CLOSED;
> +
> for (i = 0; i < n; i++)
> if (updates[i]->lock)
> unlock_ref(updates[i]->lock);
>
--
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 v20 20/48] refs.c: change ref_transaction_create to do error checking and return status
I'm in my next attempt to get through your patch series. Sorry for the
long hiatus.
Patches 1-19 look OK aside from a minor typo that I just reported.
See below for a comment on this patch.
On 06/20/2014 04:43 PM, Ronnie Sahlberg wrote:
> Do basic error checking in ref_transaction_create() and make it return
> non-zero on error. Update all callers to check the result of
> ref_transaction_create(). There are currently no conditions in _create that
> will return error but there will be in the future. Add an err argument that
> will be updated on failure.
>
> Reviewed-by: Jonathan Nieder
> Signed-off-by: Ronnie Sahlberg
> ---
> builtin/update-ref.c | 4 +++-
> refs.c | 18 +++--
> refs.h | 55
> +---
> 3 files changed, 63 insertions(+), 14 deletions(-)
>
> diff --git a/builtin/update-ref.c b/builtin/update-ref.c
> index 3067b11..41121fa 100644
> --- a/builtin/update-ref.c
> +++ b/builtin/update-ref.c
> @@ -226,7 +226,9 @@ static const char *parse_cmd_create(struct strbuf *input,
> const char *next)
> if (*next != line_termination)
> die("create %s: extra input: %s", refname, next);
>
> - ref_transaction_create(transaction, refname, new_sha1, update_flags);
> + if (ref_transaction_create(transaction, refname, new_sha1,
> +update_flags, &err))
> + die("%s", err.buf);
>
> update_flags = 0;
> free(refname);
> diff --git a/refs.c b/refs.c
> index 3f05e88..c49f1c6 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -3449,18 +3449,24 @@ int ref_transaction_update(struct ref_transaction
> *transaction,
> return 0;
> }
>
> -void ref_transaction_create(struct ref_transaction *transaction,
> - const char *refname,
> - const unsigned char *new_sha1,
> - int flags)
> +int ref_transaction_create(struct ref_transaction *transaction,
> +const char *refname,
> +const unsigned char *new_sha1,
> +int flags,
> +struct strbuf *err)
> {
> - struct ref_update *update = add_update(transaction, refname);
> + struct ref_update *update;
> +
> + if (!new_sha1 || is_null_sha1(new_sha1))
> + die("BUG: create ref with null new_sha1");
> +
> + update = add_update(transaction, refname);
>
> - assert(!is_null_sha1(new_sha1));
> hashcpy(update->new_sha1, new_sha1);
> hashclr(update->old_sha1);
> update->flags = flags;
> update->have_old = 1;
> + return 0;
> }
>
> void ref_transaction_delete(struct ref_transaction *transaction,
> diff --git a/refs.h b/refs.h
> index c5376ce..33b4383 100644
> --- a/refs.h
> +++ b/refs.h
> @@ -10,6 +10,45 @@ struct ref_lock {
> int force_write;
> };
>
> +/*
> + * A ref_transaction represents a collection of ref updates
> + * that should succeed or fail together.
> + *
> + * Calling sequence
> + *
> + * - Allocate and initialize a `struct ref_transaction` by calling
> + * `ref_transaction_begin()`.
> + *
> + * - List intended ref updates by calling functions like
> + * `ref_transaction_update()` and `ref_transaction_create()`.
> + *
> + * - Call `ref_transaction_commit()` to execute the transaction.
> + * If this succeeds, the ref updates will have taken place and
> + * the transaction cannot be rolled back.
> + *
> + * - At any time call `ref_transaction_free()` to discard the
> + * transaction and free associated resources. In particular,
> + * this rolls back the transaction if it has not been
> + * successfully committed.
> + *
> + * Error handling
> + * --
> + *
> + * On error, transaction functions append a message about what
> + * went wrong to the 'err' argument. The message mentions what
> + * ref was being updated (if any) when the error occurred so it
> + * can be passed to 'die' or 'error' as-is.
> + *
> + * The message is appended to err without first clearing err.
> + * This allows the caller to prepare preamble text to the generated
> + * error message:
> + *
> + * strbuf_addf(&err, "Error while doing foo-bar: ");
> + * if (ref_transaction_update(..., &err)) {
> + * ret = error("%s", err.buf);
> + * goto cleanup;
> + * }
> + */
I don't have a problem with the API, but I think the idiom suggested in
the comment above is a bit silly. Surely one would do the following
instead:
if (ref_transaction_update(..., &err)) {
ret = error("Error while doing foo-bar: %s", err.buf);
goto cleanup;
}
I think it would also be helpful to document whether the error string
that is appended to the strbuf is terminated with a LF.
> [...]
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.k
Re: [PATCH v20 22/48] refs.c: make ref_transaction_begin take an err argument
On 06/20/2014 04:43 PM, Ronnie Sahlberg wrote: > Add an err argument to _begin so that on non-fatal failures in future ref > backends we can report a nice error back to the caller. > While _begin can currently never fail for other reasons than OOM, in which > case we die() anyway, we may add other types of backends in the future. > For example, a hypothetical MySQL backend could fail in _being with s/_being/_begin/ > [...] 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: [PATCH v20 07/48] lockfile.c: make lock_file return a meaningful errno on failurei
On 06/20/2014 04:42 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 > [...] Typo in subject line: s/failurei/failure/ 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
[PATCH] log: fix indentation for --graph --show-signature
The git log --graph --show-signature command incorrectly indents the gpg
information about signed commits and merged signed tags. It does not
follow the level of indentation of the current commit.
Example of garbled output:
$ git log --show-signature --graph
* commit 258e0a237cb69aaa587b0a4fb528bb0316b1b776
|\ gpg: Signature made Mon, Jun 30, 2014 13:22:33 EDT using RSA key ID DA08
gpg: Good signature from "Jason Pyeron "
Merge: 727c355 1ca13ed
| | Author: Jason Pyeron
| | Date: Mon Jun 30 13:22:29 2014 -0400
| |
| | Merge of 1ca13ed2271d60ba9 branch - rebranding
| |
| * commit 1ca13ed2271d60ba93d40bcc8db17ced8545f172
| | gpg: Signature made Mon, Jun 23, 2014 9:45:47 EDT using RSA key ID DD37
gpg: Good signature from "Stephen Robert Guglielmo "
gpg: aka "Stephen Robert Guglielmo "
Author: Stephen R Guglielmo
| | Date: Mon Jun 23 09:45:27 2014 -0400
| |
| | Minor URL updates
In log-tree.c modify show_sig_lines() function to call graph_show_oneline()
after each line of gpg information it has printed in order to preserve
the level of indentation for the next output line.
Reported-by: Jason Pyeron
Signed-off-by: Zoltan Klinger
---
log-tree.c | 1 +
t/t4202-log.sh | 29 +
2 files changed, 30 insertions(+)
diff --git a/log-tree.c b/log-tree.c
index 10e6844..f13b861 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -365,6 +365,7 @@ static void show_sig_lines(struct rev_info *opt, int
status, const char *bol)
eol = strchrnul(bol, '\n');
printf("%s%.*s%s%s", color, (int)(eol - bol), bol, reset,
*eol ? "\n" : "");
+ graph_show_oneline(opt->graph);
bol = (*eol) ? (eol + 1) : eol;
}
}
diff --git a/t/t4202-log.sh b/t/t4202-log.sh
index cb03d28..b429aff 100755
--- a/t/t4202-log.sh
+++ b/t/t4202-log.sh
@@ -3,6 +3,7 @@
test_description='git log'
. ./test-lib.sh
+. "$TEST_DIRECTORY/lib-gpg.sh"
test_expect_success setup '
@@ -841,4 +842,32 @@ test_expect_success 'dotdot is a parent directory' '
test_cmp expect actual
'
+test_expect_success GPG 'log --graph --show-signature' '
+ git checkout -b signed master &&
+ echo foo >foo &&
+ git add foo &&
+ git commit -S -m signed_commit &&
+ git log --graph --show-signature -n1 signed >actual &&
+ grep "^| gpg: Signature made" actual &&
+ grep "^| gpg: Good signature" actual
+'
+
+test_expect_success GPG 'log --graph --show-signature for merged tag' '
+ git checkout -b plain master &&
+ echo aaa >bar &&
+ git add bar &&
+ git commit -m bar_commit
+ git checkout -b tagged master &&
+ echo bbb >baz &&
+ git add baz &&
+ git commit -m baz_commit
+ git tag -s -m signed_tag_msg signed_tag &&
+ git checkout plain &&
+ git merge --no-ff -m msg signed_tag &&
+ git log --graph --show-signature -n1 plain >actual &&
+ grep "^|\\\ merged tag" actual &&
+ grep "^| | gpg: Signature made" actual &&
+ grep "^| | gpg: Good signature" actual
+'
+
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 v4 4/4] cache-tree: Write updated cache-tree after commit
On Tue, Jul 8, 2014 at 7:26 AM, Junio C Hamano wrote:
> Junio C Hamano writes:
>
>>> diff --git a/builtin/commit.c b/builtin/commit.c
>>> index 9cfef6c..5981755 100644
>>> --- a/builtin/commit.c
>>> +++ b/builtin/commit.c
>>> @@ -342,6 +342,8 @@ static char *prepare_index(int argc, const char **argv,
>>> const char *prefix,
>>>
>>> discard_cache();
>>> read_cache_from(index_lock.filename);
>>> +if (update_main_cache_tree(WRITE_TREE_SILENT) >= 0)
>>> +write_cache(fd, active_cache, active_nr);
>>>
>>> commit_style = COMMIT_NORMAL;
>>> return index_lock.filename;
I wonder if we need to update_main_cache_tree() so many times in this
function. If I read the code correctly, all roads must lead to
update_main_cache_tree(0) in prepare_to_commit(). If we find out that
we have incomplete cache-tree before that call, we could write the
index one more time after that call, instead of spreading
update_main_cache_tree() all over prepare_index(). I know the
"index_file" in prepare_to_commit() is probably "index.lock" or
something, but that does not stop us from locking again
("index.lock.lock") if we want to update it.
Writing cache tree early in prepare_index() does help hooks, but I
would say hooks are uncommon case and we could add an option to
update-index to explicitly rebuild cache-tree, then hooks that do diff
a lot (or other operations that use cache-tree) could rebuild
cache-tree by themselves. When the index file is large, every write
pays high penalty so I think trying to write less often is good.
--
Duy
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
