Re: [PATCH v5] Add another option for receive.denyCurrentBranch
Hi Junio,
On Mon, 1 Dec 2014, Junio C Hamano wrote:
Junio C Hamano gits...@pobox.com writes:
Johannes Schindelin johannes.schinde...@gmx.de writes:
+static const char *update_worktree(unsigned char *sha1)
+{
+...
+ const char *work_tree = git_work_tree_cfg ? git_work_tree_cfg : ..;
I overlooked this one, but is there a reason why this has to look at
an internal implementatino detail which is git_work_tree_cfg,
instead of asking get_git_work_tree()?
I am wondering if that reason is a valid rationale to fix whatever
makes get_git_work_tree() unsuitable for this purpose.
Cc'ing Duy who has been working on the part of the system to
determine and set-up work-tree and git-dir.
Answering myself...
This is because you know receive-pack runs inside the $GIT_DIR,
whether it is a bare or non-bare repository, so either core.worktree
points at a directory that is otherwise unrelated to the $GIT_DIR
(but is the correct $GIT_WORK_TREE), or the top of the working tree
must be .. for a non-bare repository. I haven't checked the code,
but we probably are not even doing the repository/working-tree
discovery to set up the data necessary for get_git_work_tree() to
give us a correct answer.
Excellent analysis. Indeed, we do not enter the
setup_git_directory_gently() path that would interpret core.worktree and
call set_git_work_tree() explicitly. Instead, we are using the
enter_repo() code path in cmd_receive_pack() because only minimal setup is
required for the default operation of git receive-pack.
Doing an optional set-up to make get_git_work_tree() work would
involve more damage to the codebase than necessary, I would suspect,
so let's keep this part of the code as-is.
Indeed, that is the exact reason why I did not want to go down that rabbit
hole. There are so many things that are skipped when using enter_repo()
instead of setup_git_directory() that the performance impact of switching
alone would probably pose a major regression for big hosters like GitHub
or Atlassian.
I have to admit also that I never used the work tree feature myself,
therefore the support for it is pretty much untested (I *think* that I
briefly tested it years ago, when I came up with the original
updateInstead patch, but that is *long* ago). We could of course simply go
the safe route and error out if we detect that git_work_tree_cfg is set,
leaving the work to support it and to add a proper unit test to somebody
who actually needs this. Hmm?
Ciao,
Johannes
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] receive-pack: support push-to-checkout hook
Hi Junio, On Mon, 1 Dec 2014, Junio C Hamano wrote: When receive.denyCurrentBranch is set to updateInstead, this hook can be used to override the built-in push-to-deploy logic, which insists that the working tree and the index must be unchanged relative to HEAD. The hook receives the commit with which the tip of the current is going to be updated, and is responsible to make any necessary changes to the working tree and to the index to bring them to the desired state when the tip of the current branch is updated to the new commit. For example, the hook can simply run git read-tree -u -m HEAD $1 to the workflow to emulate 'git fetch' going in the reverse direction with 'git push' better than the push-to-deploy logic, as the two-tree form of read-tree -u -m is essentially the same as git checkout that switches branches while keeping the local changes in the working tree that do not interfere with the difference between the branches. I like it. The only sad part is that the already huge test suite is enlarged by yet another extensive set of test cases (and those tests might not really need to be that extensive because they essentially only need to make sure that the hook is run successfully *instead* of trying to update the working directory, i.e. a simple 'touch yep' hook would have been enough). It starts to be painful to run the complete test suite, not only on Windows (where this has been a multi-hour endeavor for me for ages already). BuildHive (CloudBees' very kind offer of Jenkins CI for Open Source, integrated conveniently with GitHub) already takes over an hour to run the Git test suite – and BuildHive runs on Linux, not Windows! That means that everytime I push into a GitHub Pull Request, I have to wait for a full hour to know whether everything is groovy. Worse: when Git for Windows contributors (yes! they exist!) push into their Pull Requests, I have to wait for a full hour before I can merge, unless I want to merge something that the test suite did not validate! So maybe it is time to start thinking about conciser tests that verify the bare minimum, especially for rarely exercised functionality? I mean, testing is always a balance between too much and too little. And at this point, I am afraid that several well-intended, but overly extensive tests increase the overall runtime of make test to a point where developers *avoid* running it, costing more time in the long run than necessary. In this particular case, I think that we really, really *just* need to verify that the presence of the hook switches off the default behavior of updateInstead. *Nothing* else is needed to verify that this particular functionality hasn't regressed. I.e. something like: +test_expect_success 'updateInstead with push-to-checkout hook' ' + rm -fr testrepo + git clone . testrepo + ( + cd testrepo + echo unclean path1 + git config receive.denyCurrentBranch updateInstead + echo 'touch yep' | write_script .git/hooks/push-to-checkout + ) + git push testrepo HEAD^:refs/heads/master + test unclean = $(cat testrepo/path1) + test -f testrepo/yep +' would be more appropriate (although it probably has one or three bugs, given that I wrote this in the mailer). Ciao, Johannes
Re: [PATCH] introduce git root
On Tue, Dec 2, 2014 at 8:04 AM, Jeff King p...@peff.net wrote: On Mon, Dec 01, 2014 at 05:17:22AM +0100, Christian Couder wrote: On Mon, Dec 1, 2014 at 4:04 AM, Junio C Hamano gits...@pobox.com wrote: If I were redoing this today, I would probably nominate the git potty as such a kitchen synk command. We have --man-path that shows the location of the manual pages, --exec-path[=path] that either shows or allows us to override the path to the subcommands, and --show-prefix, --show-toplevel, and friends may feel quite at home there. I wonder if we could reuse git config which is already a kitchen synk command to get/set a lot of parameters. Maybe we could dedicate a git or virtual or proc or sys (like /proc or /sys in Linux) namespace for these special config parameters that would not necessarily reflect something in the config file. git config git.man-path would be the same as git --man-path. git config git.root would be the same as git rev-parse --show-toplevel. git config git.exec-path mypath would allow us to override the path to the subcommands, probably by saving something in the config file. What would: git config git.root foo That would output an error message saying that we cannot change the git.root value. git config git.root That would output the same as git rev-parse --show-toplevel. output? No matter what the answer is, I do not relish the thought of trying to explain it in the documentation. :) Yeah, maybe it is better if we don't reuse git config. There is also git var, which is a catch-all for printing some deduced environmental defaults. I'd be just as happy to see it go away, though. Yeah, maybe we could use git var for more variables. Having: git --exec-path git --toplevel git --author-ident all work would make sense to me (I often get confused between git --foo and git rev-parse --foo when trying to get the exec-path and git-dir). And I don't think it's too late to move in this direction. We'd have to keep the old interfaces around, of course, but it would immediately improve discoverability and consistency. I don't like reusing the git command for that puropose, because it clutters it and makes it difficult to improve. For example let's suppose that we decide to have a git info command. It could work like this: $ git info sequence.editor vim $ git info core.editor emacs $ git info --verbose sequence.editor sequence.editor = vim GIT_EDITOR = emacs core.editor = nano $ git info --verbose core.editor GIT_EDITOR = emacs core.editor = nano So the --verbose option could explain why the sequence.editor is vim by displaying the all the relevant options with their values from the most important to the least important. Best, Christian. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [BUG] Documentation: git log: --exit-code undocumented?
Junio C Hamano gits...@pobox.com writes: David Kastrup d...@gnu.org writes: I disagree that --exit-code does nothing: it indicates whether the listed log is empty. So for example git log -1 --exit-code a..b /dev/null can be used to figure out whether a is a proper ancestor of b or not. Hmph. $ git log --exit-code master..maint /dev/null; echo $? 0 $ git log --exit-code maint..master /dev/null; echo $? 1 That is a strange way to use --exit-code. What's the best way then to tell if a is an ancestor of b? -- Sergey. -- 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/19] Add git-list-files
On Tue, Dec 2, 2014 at 3:02 AM, Junio C Hamano gits...@pobox.com wrote: Does this contain a lot of borrowed code or something? The style violation in the patches are unusually high, even compared with your other series. The first one is from coreutils, but I reformatted (and trimmed) to fit Git. I recall you had a script to spot style violation, right? Where can I find the script? Else I'll reread CodingGuidlines again and go through the patch. I've tried to fix it up and will push out the result on 'pu' if things work OK, but this does not even have tests, so it is unlikely that it would break anything but itself ;-) Heh.. ;) Next version will come with tests. Thanks for the reminder. -- Duy -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 00/19] Add git-list-files
On Tue, Dec 2, 2014 at 12:42 PM, Jeff King p...@peff.net wrote: On Sun, Nov 30, 2014 at 03:55:48PM +0700, Nguyễn Thái Ngọc Duy wrote: This is something else that's been sitting in my tree for a while now. It adds git list-files, intended to be aliased as ls with your favourite display options. When I read the subject, I thought why isn't this called git-ls?. Then when I read this paragraph, I thought if the point is for everybody to make their own ls alias, why do we need list-files at all, instead of just adding options to ls-files? I'll let you decide which (if any) you'd like to answer. :) My guesses: 1. If it were git-ls, it would stomp on existing aliases people have constructed. Yes, Matthew Moy (I think) at least had this git-ls alias and he did complain. Also we could not agree on what should be the good defaults for git-ls if it's built in. 2. If it is a wrapper around ls-files, then the options may be constrained; ls-files already squats on useful options like -d (which, if we are matching traditional ls, is more like our -t). Also right. I want to keep option names close to GNU ls. As a side note, I wonder if it would be sensible to whitelist some commands as porcelain, and allow aliases to override them (either entirely, or just to add-in some options). Agreed. Maybe not all porcelain (some like git-branch almost functions like plumbing). We also need away to stop alias (e.g. in scripts). In scripts I can specify full path to a command to make sure I won't hit an alias. I guess we can't do the same here. The closet to full path is git-cmd form, as opposed to git cmd form) but I think we don't want to bring back git-cmd. Maybe just a git --no-alias cmd and GIT_NO_ALIAS.. And if people now can override standard commands, I think it makes sense to ship a default alias set (with lower priority than user-defined aliases). After all people need to check twice if the command they type really means what they think it is.. -- Duy -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 24/34] checkout: reject if the branch is already checked out elsewhere
On Tue, Dec 2, 2014 at 12:39 AM, Junio C Hamano gits...@pobox.com wrote: Sorry, what is a hic? Off topic. It's the sound (in Vietnamese) when you inhale through your nose, e.g. like when you cry.. I know there's an equivalent in English, just can't remember it now. -- Duy -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 24/34] checkout: reject if the branch is already checked out elsewhere
On Tue, Dec 2, 2014 at 12:04 PM, Mark Levedahl mleved...@gmail.com wrote:
On 12/01/2014 12:39 PM, Junio C Hamano wrote:
Sorry, what is a hic? If this were an existing feature like
git-new-workdir, even though it is from contrib, making it impossible to do
something that used to be possible, even if that something is what mere
mortals would never want to to to avoid risking confusion, would be a
regression that needs an escape hatch. But this is a new feature. I am not
sure why you need to make this overridable in the first place. Those who
want to have multiple checkouts of the same commit can just detach HEAD at
the same commit in multiple working trees, as the first thing they need to
do would be to run git reset --hard $branch to synchronize the HEAD and
the working tree state to work in the other out-of-sync repositories either
case anyway.
Yes, detached HEADS allow multiple checkouts, but now the user needs another
system to record what $branch was for each checked out tree or needs to
resort to forensics using various git-branch / git-log invocations to find
the most-likely value. So, I do not find detached HEADS useful in general,
and specifically not for this case. Duy's latest addition
('--ignore-other-worktrees') would, so far as I see, allow this feature to
replace git-new-workdir in my uses, but without the addition it cannot.
I'm ok either way. So I'll let you and Junio (and maybe others) sort
this out. No objection means --ignore-other-worktrees is in.
FWIW git-branch usually can show the original branch of detached head
(must not always). I don't think we have a plumbing equivalent for it
though. People can tail -1 $GIT_DIR/logs/HEAD| sed .. but that seems
hacky. I do like read-only ref concept where we can keep ref name
(especially tags) in HEAD until the next commit. But it didn't go
anywhere
--
Duy
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 09/19] Add git-list-files, a user friendly version of ls-files and more
On Tue, Dec 2, 2014 at 9:50 AM, Eric Sunshine sunsh...@sunshineco.com wrote: On Sunday, November 30, 2014, Nguyễn Thái Ngọc Duy pclo...@gmail.com wrote: This is more user friendly version of ls-files: * it's automatically colored and columnized * it refreshes the index like all porcelain commands * it defaults to non-recursive behavior like ls * :(glob) is on by default so '*.c' means a.c but not a/b.c, use '**/*.c' for that. * auto pager The name 'ls' is not taken. It is left for the user to make an alias with better default options. I understand that your original version was named git-ls and that you renamed it to git-list-files in order to leave 'ls' available so users can create an 'ls' alias specifying their own default options. Would it make sense, however, to restore the name to git-ls and allow users to set default options via a config variable instead? Doing so would make the short-and-sweet git-ls command work for all users out-of-the-box, which might be well appreciated by Unix users. Or I just make git-ls the first alias shipped by default.. I don't really like using config var to define default options. Sounds like a workaround to our alias. Jeff raised it elsewhere in this thread: http://thread.gmane.org/gmane.comp.version-control.git/260423/focus=260538 -- Duy -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] tree.c: update read_tree_recursive callback to pass strbuf as base
On Tue, Dec 2, 2014 at 2:32 AM, Junio C Hamano gits...@pobox.com wrote: Nguyễn Thái Ngọc Duy pclo...@gmail.com writes: This allows the callback to use 'base' as a temporary buffer to quickly assemble full path without extra allocation. The callback has to restore it afterwards of course. Hmph, what's the quote around 'without' doing there? because it's only true if you haven't used up all preallocated space in strbuf. If someone passes an empty strbuf, then underneath strbuf may do a few realloc until the buffer is large enough. -- 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
[PATCH v5 0/1] http: Add Accept-Language header if possible
Changes since v4 * Fix styles as Junio C Hamano suggested. * Limit number of languages and length of Accept-Language header. Yi EungJun (1): http: Add Accept-Language header if possible http.c | 154 + remote-curl.c | 2 + t/t5550-http-fetch-dumb.sh | 31 + 3 files changed, 187 insertions(+) -- 2.2.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 v5 1/1] http: Add Accept-Language header if possible
From: Yi EungJun eungjun...@navercorp.com
Add an Accept-Language header which indicates the user's preferred
languages defined by $LANGUAGE, $LC_ALL, $LC_MESSAGES and $LANG.
Examples:
LANGUAGE= -
LANGUAGE=ko:en - Accept-Language: ko, en;q=0.9, *;q=0.1
LANGUAGE=ko LANG=en_US.UTF-8 - Accept-Language: ko, *;q=0.1
LANGUAGE= LANG=en_US.UTF-8 - Accept-Language: en-US, *;q=0.1
This gives git servers a chance to display remote error messages in
the user's preferred language.
Limit the number of languages to 1,000 because q-value must not be
smaller than 0.001, and limit the length of Accept-Language header to
4,000 bytes for some HTTP servers which cannot accept such long header.
Signed-off-by: Yi EungJun eungjun...@navercorp.com
---
http.c | 154 +
remote-curl.c | 2 +
t/t5550-http-fetch-dumb.sh | 31 +
3 files changed, 187 insertions(+)
diff --git a/http.c b/http.c
index 040f362..69624af 100644
--- a/http.c
+++ b/http.c
@@ -68,6 +68,8 @@ static struct curl_slist *no_pragma_header;
static struct active_request_slot *active_queue_head;
+static struct strbuf *cached_accept_language;
+
size_t fread_buffer(char *ptr, size_t eltsize, size_t nmemb, void *buffer_)
{
size_t size = eltsize * nmemb;
@@ -515,6 +517,9 @@ void http_cleanup(void)
cert_auth.password = NULL;
}
ssl_cert_password_required = 0;
+
+ if (cached_accept_language)
+ strbuf_release(cached_accept_language);
}
struct active_request_slot *get_active_slot(void)
@@ -986,6 +991,149 @@ static void extract_content_type(struct strbuf *raw,
struct strbuf *type,
strbuf_addstr(charset, ISO-8859-1);
}
+/*
+ * Guess the user's preferred languages from the value in LANGUAGE environment
+ * variable and LC_MESSAGES locale category.
+ *
+ * The result can be a colon-separated list like ko:ja:en.
+ */
+static const char *get_preferred_languages(void)
+{
+ const char *retval;
+
+ retval = getenv(LANGUAGE);
+ if (retval *retval)
+ return retval;
+
+ retval = setlocale(LC_MESSAGES, NULL);
+ if (retval *retval
+ strcmp(retval, C)
+ strcmp(retval, POSIX))
+ return retval;
+
+ return NULL;
+}
+
+/*
+ * Get an Accept-Language header which indicates user's preferred languages.
+ *
+ * Examples:
+ * LANGUAGE= -
+ * LANGUAGE=ko:en - Accept-Language: ko, en; q=0.9, *; q=0.1
+ * LANGUAGE=ko_KR.UTF-8:sr@latin - Accept-Language: ko-KR, sr; q=0.9, *;
q=0.1
+ * LANGUAGE=ko LANG=en_US.UTF-8 - Accept-Language: ko, *; q=0.1
+ * LANGUAGE= LANG=en_US.UTF-8 - Accept-Language: en-US, *; q=0.1
+ * LANGUAGE= LANG=C -
+ */
+static struct strbuf *get_accept_language(void)
+{
+ const char *lang_begin, *pos;
+ int q, max_q;
+ int num_langs;
+ int decimal_places;
+ int is_codeset_or_modifier = 0;
+ char q_format[32];
+ /*
+* MAX_LANGS must not be larger than 1,000. If it is larger than that,
+* q-value will be smaller than 0.001, the minimum q-value the HTTP
+* specification allows [1].
+*
+* [1]: http://tools.ietf.org/html/rfc7231#section-5.3.1
+*/
+ const int MAX_LANGS = 1000;
+ const int MAX_SIZE_OF_HEADER = 4000;
+ int last_size = 0;
+
+ if (cached_accept_language)
+ return cached_accept_language;
+
+ cached_accept_language = xmalloc(sizeof(struct strbuf));
+ strbuf_init(cached_accept_language, 0);
+ lang_begin = get_preferred_languages();
+
+ /* Don't add Accept-Language header if no language is preferred. */
+ if (!lang_begin)
+ return cached_accept_language;
+
+ /* Count number of preferred lang_begin to decide precision of
q-factor. */
+ for (num_langs = 1, pos = lang_begin; *pos; pos++)
+ if (*pos == ':')
+ num_langs++;
+
+ /* Decide the precision for q-factor on number of preferred lang_begin.
*/
+ num_langs += 1; /* for '*' */
+
+ if (MAX_LANGS num_langs)
+ num_langs = MAX_LANGS;
+
+ for (max_q = 1, decimal_places = 0;
+ max_q num_langs;
+ decimal_places++, max_q *= 10);
+
+ sprintf(q_format, ;q=0.%%0%dd, decimal_places);
+
+ q = max_q;
+
+ strbuf_addstr(cached_accept_language, Accept-Language: );
+
+ /*
+* Convert a list of colon-separated locale values [1][2] to a list of
+* comma-separated language tags [3] which can be used as a value of
+* Accept-Language header.
+*
+* [1]: http://pubs.opengroup.org/onlinepubs/007908799/xbd/envvar.html
+* [2]:
http://www.gnu.org/software/libc/manual/html_node/Using-gettextized-software.html
+* [3]: http://tools.ietf.org/html/rfc7231#section-5.3.5
+*/
+ for (pos =
RE: [PATCH] git add -i: allow list (un)selection by regexp
From 9096652a71666920ae8d59dd4317d536ba974d5b Mon Sep 17 00:00:00 2001
From: Aarni Koskela a...@iki.fi
Date: Tue, 2 Dec 2014 13:56:15 +0200
Subject: [PATCH] git-add--interactive: allow list (un)selection by regular
expression
Teach `list_and_choose` to allow `/regexp` and `-/regexp` syntax to
select items based on regular expression match.
This feature works in all list menus in `git-add--interactive`, and is not
limited to file menus only.
For instance, in file lists, `/\.c$` will select all files whose extension
is `.c`. In option menus, such as the main menu, `/pa` could be used to
choose the `patch` option.
Signed-off-by: Aarni Koskela a...@iki.fi
---
Thank you for the insightful comments, Junio, and sorry for the confusion
regarding email-patch formatting. Hoping I get it right this time.
Usually the responsibility to ensure correctness lies on the person who
proposes a change, not those who relies on the continued correct operation
of the existing code.
You're of course absolutely right. My point was that I can't think of an use
case where one would need to otherwise have / or -/ as the first characters
of input in a list_and_choose situation, but someone else might.
[...] but is this about the selection that happens after showing you a
list of filenames to choose from?
I clarified this in the commit message. Selection by regexp works in all
list_and_choose situations, including the main menu of `git add -i`, hence
option.
Regarding the unchoose quantifier -- yes, silly me.
And regarding error checking for the regular expression, you're right -- the
program promptly blew up when entering an invalid regexp. I incorporated your
suggestion for error checking, with the addition of using the `error_msg` sub
for colorized error reporting.
Best regards,
Aarni Koskela
git-add--interactive.perl | 49 +++
1 file changed, 49 insertions(+)
diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index 1fadd69..28e4c2d 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -483,6 +483,8 @@ sub is_valid_prefix {
!($prefix =~ /[\s,]/) # separators
!($prefix =~ /^-/) # deselection
!($prefix =~ /^\d+/) # selection
+ !($prefix =~ /^\//)# regexp selection
+ !($prefix =~ /^-\//) # regexp unselection
($prefix ne '*') # all wildcard
($prefix ne '?');# prompt help
}
@@ -585,6 +587,50 @@ sub list_and_choose {
prompt_help_cmd();
next TOPLOOP;
}
+ if ($line =~ /^(-)?\/(.+)$/) {
+ # The first capture group (-) being missing means
choose is
+ # requested. If the first group exists at all,
unchoose is
+ # requested.
+ my $choose = !(defined $1);
+
+ # Validate the regular expression and complain if
compilation failed.
+ my $re = eval { qr/$2/ };
+ if (!$re) {
+ error_msg Invalid regular expression:\n $@\n;
+ next TOPLOOP;
+ }
+
+ my $found = 0;
+ for ($i = 0; $i @stuff; $i++) {
+ my $val = $stuff[$i];
+
+ # Figure out the display value for $val.
+ # Some lists passed to list_and_choose contain
+ # items other than strings; in order to match
+ # regexps against them, we need to extract the
+ # displayed string. The logic here is
approximately
+ # equivalent to the display logic above.
+
+ my $ref = ref $val;
+ if ($ref eq 'ARRAY') {
+ $val = $val-[0];
+ }
+ elsif ($ref eq 'HASH') {
+ $val = $val-{VALUE};
+ }
+
+ # Match the string value against the regexp,
+ # then act accordingly.
+
+ if ($val =~ $re) {
+ $chosen[$i] = $choose;
+ $found = $found || $choose;
+ last if $choose $opts-{SINGLETON};
+ }
+ }
+ last if $found ($opts-{IMMEDIATE});
+ next TOPLOOP;
+ }
for my $choice (split(/[\s,]+/, $line)) {
my $choose = 1;
my ($bottom, $top);
@@ -635,6 +681,7 @@ sub
Re: [PATCH 3/3] ls-tree: disable negative pathspec because it's not supported
On Tue, Dec 2, 2014 at 2:40 AM, Junio C Hamano gits...@pobox.com wrote: Nguyễn Thái Ngọc Duy pclo...@gmail.com writes: Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com Hmph, that's sad. Should the below say test_expect_failure without test_must_fail, anticipating a fix later? Not a fix from me any time soon (I still need to improve pathspec support in git-mv). If the git-list-files series goes well, I do plan to make it list trees and it should support all pathspec without the fear of subtly breaking a plumbing. But I will change it to expect_failure unless you change your mind. -- 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
Git Bash for Mac
Good Morning, My name is Sefath, and I was wondering when i could start using Git for Mac. I’m completely new to coding, and I wanted to start with HTML. However, when I tried installing git bash on my mac, it doesn’t work. Maybe it isn’t compatible with OS X Yosmite? I would really love to start learning code, and it sucks that I can’t because of a reason like this. Thank you for reading, Sefath Chowdhury-- 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: [BUG] Documentation: git log: --exit-code undocumented?
On Tue, Dec 02, 2014 at 02:30:31PM +0300, Sergey Organov wrote: Junio C Hamano gits...@pobox.com writes: David Kastrup d...@gnu.org writes: I disagree that --exit-code does nothing: it indicates whether the listed log is empty. So for example git log -1 --exit-code a..b /dev/null can be used to figure out whether a is a proper ancestor of b or not. Hmph. $ git log --exit-code master..maint /dev/null; echo $? 0 $ git log --exit-code maint..master /dev/null; echo $? 1 That is a strange way to use --exit-code. What's the best way then to tell if a is an ancestor of b? git merge-base --is-ancestor a b -- 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/19] Add git-list-files
Jeff King schrieb am 02.12.2014 um 06:42: On Sun, Nov 30, 2014 at 03:55:48PM +0700, Nguyễn Thái Ngọc Duy wrote: This is something else that's been sitting in my tree for a while now. It adds git list-files, intended to be aliased as ls with your favourite display options. When I read the subject, I thought why isn't this called git-ls?. Then when I read this paragraph, I thought if the point is for everybody to make their own ls alias, why do we need list-files at all, instead of just adding options to ls-files? I'll let you decide which (if any) you'd like to answer. :) My guesses: 1. If it were git-ls, it would stomp on existing aliases people have constructed. 2. If it is a wrapper around ls-files, then the options may be constrained; ls-files already squats on useful options like -d (which, if we are matching traditional ls, is more like our -t). I somewhat feel like (1) can be mitigated by the fact that your command is what people were probably trying to approximate with their aliases, and that as porcelain it should be very configurable (so they should be able to accomplish the same things as their aliases). But I dunno. I do not have an ls alias, so I am biased. :) As a side note, I wonder if it would be sensible to whitelist some commands as porcelain, and allow aliases to override them (either entirely, or just to add-in some options). -Peff I'd like to second all that (+1, like). User friendly listing of files in the git repo is dearly needed, and following names and default behaviour of mv/cp/ls is a way to follow the principle of least surprise. While ls might be an alias for many, I'm sure stage was for quite a few people, too. We should be able to take any new name for new command, regardless of aliases people may be using. Allowing to alias at least porcelain commands, at least to the extent of adding default options, is something we've talked about before and which would have prevented us from the increasing bloat by the default changing config knobs. git -c ... somehow took us down the other road. I'm still dreaming of a git future where either git foo --bar=baz is equivalent to git -c foo.bar=baz foo (i.e. unify the naming), or we are simply able to alias foo to foo --bar=baz if that is what we like as default (i.e. get rid of many of the special config knobs). Right now, we have two sets of options with often differing names. Also, we could ship a few commonly used aliases (such as co=checkout, ci=commit, st=status) which could be overriden easily. Michael -- 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: tests do not work with gpg 2.1
Jeff King schrieb am 28.11.2014 um 17:50: [updated subject, as this is not specific to the v2.2.0 release at all] On Fri, Nov 28, 2014 at 10:48:51AM +0100, Michael J Gruber wrote: Are you running gnome_keyring_deamon by any chance? It think it runs by default in Gnome, claims to offer gpg_agent functionality but does not seem to do so fully. I.e., its presence may keep gpg2.1 from starting its own gpg-agent. But gpg2.1 (gnupg modern branch) needs a new gpg-agent which knows how to handle secret keys for gpg2.1. (I may take a shot at trying, but I'm on Fedora - they're slow and special in all things gpg/crypto. And compiling gpg2.1 means compiling all the bits and pieces that monster consists of these days...) I'm not running the gnome daemon (I do normally run gpg-agent, though), and I can reproduce. You get the passphrase prompt, Steven didn't, if I understood correctly. You can continue successfully by hitting OK, Steven coudn't hit anything... I wanted to try experimenting today with making sure GPG_AGENT_INFO was unset in the environment. But despite nothing changing (i.e., before I even cleared that variable), I'm getting totally different results. Now when I run t4202, I get no agent prompt, and just: ok 40 - dotdot is a parent directory expecting success: 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 Switched to a new branch 'signed' gpg: skipped C O Mitter commit...@example.com: No secret key gpg: signing failed: No secret key error: gpg failed to sign the data fatal: failed to write commit object That is how things turned for Steven, afaik. And then a subsequent run gives me: rm: cannot remove '/home/peff/compile/git/t/trash directory.t4202-log/gpghome/private-keys-v1.d/19D48118D24877F59C2AE86FEC8C3E90694B2631.key': Permission denied rm: cannot remove '/home/peff/compile/git/t/trash directory.t4202-log/gpghome/private-keys-v1.d/E0C803F8BC3BCC4990E174E05936A7636E99.key': Permission denied rm: cannot remove '/home/peff/compile/git/t/trash directory.t4202-log/gpghome/private-keys-v1.d/FCFAC48BF12AC0FCC32B69AB90AA7B1891382C29.key': Permission denied rm: cannot remove '/home/peff/compile/git/t/trash directory.t4202-log/gpghome/private-keys-v1.d/D50A866904B91C0C49A3F6059584F4A09807D330.key': Permission denied FATAL: Cannot prepare test area It seems that it creates the private-keys directory without the 'x' bit: $ ls -ld trash*/gpghome/private-keys-v1.d drw--- 2 peff peff 4096 Nov 28 11:45 trash directory.t4202-log/gpghome/private-keys-v1.d/ So that's weird, and doubly so that it is behaving differently than it was last night. Obviously _something_ must have change. Maybe something related to the state of my running agent, I guess. -Peff I think if you unset GPG_AGENT_INFO, gpg2.1 thinks there is no agent, starts it's own and talks to it via a socket directly (no env variable). Now that one seems come with different options (regarding pinentry) so that it can't even ask you for a passphrase. That private-keys directory is from the first run of gpg2.1 on a pre-2.1 GPGHOME. It converts the old secring db to that new dir of entries and uses that instead. Regarding the umask: That may actually be fallout from e7f224f (t/lib-gpg: make gpghome files writable, 2014-10-24) where I didn't expect directories to be present in gpghome. Maybe i should change chmod 0700 gpghome chmod 0600 gpghome/* to chmod -R o+w gpghome/ though I felt somehow safer with the explicit permissions. Michael -- 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] receive-pack: support push-to-checkout hook
Johannes Schindelin schrieb am 02.12.2014 um 09:47: Hi Junio, On Mon, 1 Dec 2014, Junio C Hamano wrote: When receive.denyCurrentBranch is set to updateInstead, this hook can be used to override the built-in push-to-deploy logic, which insists that the working tree and the index must be unchanged relative to HEAD. The hook receives the commit with which the tip of the current is going to be updated, and is responsible to make any necessary changes to the working tree and to the index to bring them to the desired state when the tip of the current branch is updated to the new commit. For example, the hook can simply run git read-tree -u -m HEAD $1 to the workflow to emulate 'git fetch' going in the reverse direction with 'git push' better than the push-to-deploy logic, as the two-tree form of read-tree -u -m is essentially the same as git checkout that switches branches while keeping the local changes in the working tree that do not interfere with the difference between the branches. I like it. The only sad part is that the already huge test suite is enlarged by yet another extensive set of test cases (and those tests might not really need to be that extensive because they essentially only need to make sure that the hook is run successfully *instead* of trying to update the working directory, i.e. a simple 'touch yep' hook would have been enough). It starts to be painful to run the complete test suite, not only on Windows (where this has been a multi-hour endeavor for me for ages already). BuildHive (CloudBees' very kind offer of Jenkins CI for Open Source, integrated conveniently with GitHub) already takes over an hour to run the Git test suite – and BuildHive runs on Linux, not Windows! That means that everytime I push into a GitHub Pull Request, I have to wait for a full hour to know whether everything is groovy. Worse: when Git for Windows contributors (yes! they exist!) push into their Pull Requests, I have to wait for a full hour before I can merge, unless I want to merge something that the test suite did not validate! So maybe it is time to start thinking about conciser tests that verify the bare minimum, especially for rarely exercised functionality? I mean, testing is always a balance between too much and too little. And at this point, I am afraid that several well-intended, but overly extensive tests increase the overall runtime of make test to a point where developers *avoid* running it, costing more time in the long run than necessary. In this particular case, I think that we really, really *just* need to verify that the presence of the hook switches off the default behavior of updateInstead. *Nothing* else is needed to verify that this particular functionality hasn't regressed. I.e. something like: +test_expect_success 'updateInstead with push-to-checkout hook' ' + rm -fr testrepo + git clone . testrepo + ( + cd testrepo + echo unclean path1 + git config receive.denyCurrentBranch updateInstead + echo 'touch yep' | write_script .git/hooks/push-to-checkout + ) + git push testrepo HEAD^:refs/heads/master + test unclean = $(cat testrepo/path1) + test -f testrepo/yep +' would be more appropriate (although it probably has one or three bugs, given that I wrote this in the mailer). Ciao, Johannes How about reusing the prerequisites feature for that? We could either mark the minimal tests, or mark the others similar to how we do with the (extra) expensive tests. Your config.mk would then determine which tests are executed. Michael -- 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] receive-pack: support push-to-checkout hook
Hi Michael, On Tue, 2 Dec 2014, Michael J Gruber wrote: Johannes Schindelin schrieb am 02.12.2014 um 09:47: The only sad part is that the already huge test suite is enlarged by yet another extensive set of test cases (and those tests might not really need to be that extensive because they essentially only need to make sure that the hook is run successfully *instead* of trying to update the working directory, i.e. a simple 'touch yep' hook would have been enough). It starts to be painful to run the complete test suite, not only on Windows (where this has been a multi-hour endeavor for me for ages already). BuildHive (CloudBees' very kind offer of Jenkins CI for Open Source, integrated conveniently with GitHub) already takes over an hour to run the Git test suite – and BuildHive runs on Linux, not Windows! How about reusing the prerequisites feature for that? We could either mark the minimal tests, or mark the others similar to how we do with the (extra) expensive tests. Your config.mk would then determine which tests are executed. In general, you are correct. And we already have the test_have_prereq EXPENSIVE precedent. In this particular case, I question the value of the extent of the tests: the only thing we really need to test is that the new hook really overrides the default behavior, not all kinds of real-world simulations that *use* that behavior. In other words, it is my opinion that the difference between the touch yep test I demonstrated and the test originally suggested is the amount of time it takes to run, not the extent to which the new code ist actually verified. Ciao, Johannes
[PATCH] t/lib-gpg: adjust permissions for gnupg 2.1
Before gnupg 2.1 (aka modern branch), gpghome would contain only files which allowed t/lib-gpg.sh to set permissions explicitely, and we did that since 28a1b07 (t/lib-gpg: adjust permissions for gnupg 2.1, 2014-12-02) in order to adjust wrong permissions from a checkout on ro file systems. gnupg 2.1 creates a new directory in gpghome which would get its x bit removed. Adjust and use +X so that any directory would get its x bit set. This also keeps the x bit on files which had it set for whatever wrong reason, but we care only about having at least the necessary permissions for the tests to run. Signed-off-by: Michael J Gruber g...@drmicha.warpmail.net --- Something like this? Untested for lack of gpg2.1 t/lib-gpg.sh | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/t/lib-gpg.sh b/t/lib-gpg.sh index cd2baef..25ca12d 100755 --- a/t/lib-gpg.sh +++ b/t/lib-gpg.sh @@ -17,8 +17,7 @@ else # Name and email: C O Mitter commit...@example.com # No password given, to enable non-interactive operation. cp -R $TEST_DIRECTORY/lib-gpg ./gpghome - chmod 0700 gpghome - chmod 0600 gpghome/* + chmod -R u+rwX gpghome GNUPGHOME=$(pwd)/gpghome export GNUPGHOME test_set_prereq GPG -- 2.2.0.rc3.286.g888a711 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: git status / git diff -C not detecting file copy
Jeff, Thanks much for the detailed answer and analysis. Does either of you want to try your hand at a patch? Just enabling copies should be a one-liner. Making it configurable is more involved, but should also be pretty straightforward. I'll have to pass on this. I'm absolutely not familiar with the Git source code nor with the contributing guidelines and process at this time. I'm currently working on libgit2 and trying to clone the git status behavior and that's how I discovered this file copy detection issue. In the meantime, shouldn't we patch the docs? That should be trivial for sure. - Pol -- 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] Add another option for receive.denyCurrentBranch
Johannes Schindelin johannes.schinde...@gmx.de writes: This is because you know receive-pack runs inside the $GIT_DIR, whether it is a bare or non-bare repository, so either core.worktree points at a directory that is otherwise unrelated to the $GIT_DIR (but is the correct $GIT_WORK_TREE), or the top of the working tree must be .. for a non-bare repository And this reasoning may be broken, unfortunately. In a repository with separate-git-dir, we enter $GIT_DIR that is pointed by the gitdir: $over_there thing, and once we go there, we have no linkage back to find where the working tree is unless there is core.worktree set, do we? This feature (with or without the push-to-checkout hook, as that shares exactly the same logic that discovers, or fails to do so, where the working tree is) needs to be documented with an entry in the BUGS section, saying that it will not work in a repository that is tied to its working tree via the gitdir: mechanism. It actually is a lot worse than merely it will not work, when this problem ever manifests itself. The use of this mechanism in such a repository will destroy the contents of a wrong directory that happens to be the parent directory of a repository pointed by the gitdir: mechanism, unless core.worktree is set. Fortunately, real submodule directories found in the .git/modules/ directory of the superproject, even though they are bound to their checkout locations in the working tree of the superproject using gitdir: mechanism, do maintain their core.worktree when git submodule manages them, so the use of the mechanism in submodule setting may be safe. -- 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: RCS Keywords in Git done right
I've finished testing this work in larger repositories.
While the approach is performant and works nicely in small repos, but
in larger repos one of the requirements for the correctness of
substitutions slows things down (1 or 2 minutes to perform checkouts
between branches with 10,000+ files).
The operation that is slowing things down is discovering the relative
complement of commits between the common files of two branches (i.e.,
which files are common between two branches but differ in their latest
commit).
My current approach is:
1) find files common between @ @{-1}, ls-tree --full-tree
--name-only -r both branches, take the intersection
2) find current branch's commits for common files, for each file in
intersection log -1 --format=%H $current_branch -- $file
3) find common files where latest commits differ, for each file in
intersection keep the file if current branche's latest commit does not
equal prior branch's latest commit
4) overwrite all kept files with the results of git-archive
It is steps 2 3 that consume the most time in a large repo with
large intersections of common files between branches.
I've tried to conceive of other ways to arriving at the same
filename/latest current branch commit hash pairs where filenames
are common between branches and latest current branch commit hash
differs from latest prior branch commit hash. I've thought maybe I
could traverse commits starting from merge-base instead of traversing
files, but that doesn't seem like it would be a huge improvement.
I'm sure internal to git in C there would be a better/faster way (and
it would probably look like writing Btrieve queries). Can anyone think
of a good solution for the intersection of files and complement of
commits using only the git CLI tools?
Thanks,
Derek
--
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] receive-pack: support push-to-checkout hook
Johannes Schindelin johannes.schinde...@gmx.de writes: In this particular case, I think that we really, really *just* need to verify that the presence of the hook switches off the default behavior of updateInstead. *Nothing* else is needed to verify that this particular functionality hasn't regressed. I.e. something like: +test_expect_success 'updateInstead with push-to-checkout hook' ' + rm -fr testrepo + git clone . testrepo + ( + cd testrepo + echo unclean path1 + git config receive.denyCurrentBranch updateInstead + echo 'touch yep' | write_script .git/hooks/push-to-checkout + ) + git push testrepo HEAD^:refs/heads/master + test unclean = $(cat testrepo/path1) + test -f testrepo/yep +' would be more appropriate (although it probably has one or three bugs, given that I wrote this in the mailer). Not really. You need to remember that we write tests not to show off the new shiny, but to protect essential invariants from being broken by careless others attempting to rewrite the implementation in the future. What needs to be tested for the feature are at the minimum: * The hook is not triggered when denycurrent is set to updateInstead; the posted version does not even test this, but it should. * The hook is run with the right settings, so that git commands it runs will operate on the right repository and its working tree, but without overspecifying how that right settings happens [*1*]. * Non-zero exit from the hook will fail git push, and zero exit from the hook will allow git push to pass. * Whether the hook returns a success or a failure, the working tree and the index is in the state the hook expects to give the user (i.e. nobody else further munges the working tree and the index after hook returns) [*2*]. The patch I posted is minimum to do so. Compared to that, the yep test is not checking anything of the importance, and only insists on a single immaterial detail (i.e. hook must be run after cd'ing to the top of the working tree). I fail to see why it could be more appropriate. [Footnote] *1* Your version above assumes that the hook must run in the working tree, but it does not have to, if GIT_DIR and GIT_WORK_TREE are both exported; your test is overspecifying what should happen, and will reject a legitimate implementation of the feature. *2* A hook may muck with the index or with the working tree and then return a failure. I do not offhand see why a failing push should be allowed to contaminate the working tree that way, but whatever the hook does is what the user wishes, and we should not lose data coming from that wish. The hook the test uses refrains from touching the working tree or the index when it fails, so the test checks that the working tree and the index are left as-is when it happens. -- 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] receive-pack: support push-to-checkout hook
Hi Junio, On Tue, 2 Dec 2014, Junio C Hamano wrote: Johannes Schindelin johannes.schinde...@gmx.de writes: In this particular case, I think that we really, really *just* need to verify that the presence of the hook switches off the default behavior of updateInstead. *Nothing* else is needed to verify that this particular functionality hasn't regressed. I.e. something like: +test_expect_success 'updateInstead with push-to-checkout hook' ' + rm -fr testrepo + git clone . testrepo + ( + cd testrepo + echo unclean path1 + git config receive.denyCurrentBranch updateInstead + echo 'touch yep' | write_script .git/hooks/push-to-checkout + ) + git push testrepo HEAD^:refs/heads/master + test unclean = $(cat testrepo/path1) + test -f testrepo/yep +' would be more appropriate (although it probably has one or three bugs, given that I wrote this in the mailer). Not really. You need to remember that we write tests not to show off the new shiny, but to protect essential invariants from being broken by careless others attempting to rewrite the implementation in the future. Fair enough. You are the boss. I am not, therefore it does not matter what I think, Johannes -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5] Add another option for receive.denyCurrentBranch
Hi Junio, On Tue, 2 Dec 2014, Junio C Hamano wrote: This feature [...] needs to be documented with an entry in the BUGS section, saying that it will not work in a repository that is tied to its working tree via the gitdir: mechanism. Fair enough. But which BUGS section? Should I add one to `git-push.txt` or `git-receive-pack.txt`? Technically, it should be the latter, but nobody's gonna find it there... Ciao, Johannes -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] receive-pack: support push-to-checkout hook
Johannes Schindelin johannes.schinde...@gmx.de writes: Not really. You need to remember that we write tests not to show off the new shiny, but to protect essential invariants from being broken by careless others attempting to rewrite the implementation in the future. Fair enough. You are the boss. I am not, therefore it does not matter what I think, It is not that it does not matter because you are not the boss; it is just that when you are wrong, you are wrong. You said in your response to Michael: the difference between the touch yep ... and the test originally suggested is ... not the extent to which the new code is actually verified. If your verification is based on faith, you are correct. You may be verifying the code to the right extent, i.e. Yes, what I wrote is actually run, and I write perfect code every time, so what it does must be correct, as long as it gets run. But I do not have faith in people who will be touching the relevant code in the future; Yes, it is triggered is far from satisfactory without faith like yours in the code being tested. I actually do not have much faith in what I write myself ;-). That is why we have tests. -- 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: RCS Keywords in Git done right
My current approach is:
1) find files common between @ @{-1}, ls-tree --full-tree
--name-only -r both branches, take the intersection
2) find current branch's commits for common files, for each file in
intersection log -1 --format=%H $current_branch -- $file
3) find common files where latest commits differ, for each file in
intersection keep the file if current branche's latest commit does not
equal prior branch's latest commit
4) overwrite all kept files with the results of git-archive
PS: In large repos, I can dump the entire contents of the repo out of
git-archive faster than I can look up the commits of common files
between two branches for a more limited and surgical dump from
git-archive (say, 30 seconds to dump everything out of git-archive vs.
1 minute 30 seconds to find the intersection of files and look up the
latest commits).
--
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] receive-pack: support push-to-checkout hook
Hi Junio, On Tue, 2 Dec 2014, Junio C Hamano wrote: Johannes Schindelin johannes.schinde...@gmx.de writes: Not really. You need to remember that we write tests not to show off the new shiny, but to protect essential invariants from being broken by careless others attempting to rewrite the implementation in the future. Fair enough. You are the boss. I am not, therefore it does not matter what I think, It is not that it does not matter because you are not the boss; it is just that when you are wrong, you are wrong. Please, there is no need to get emotional, let alone personal. I am not really interested in challenging your policy regarding the test suite, even if it does hurt my development style where I want to run the test suite frequently but its tests just take too long because their focus is more on thoroughness rather than trying to save time in the manner I suggested (i.e. by only lightly testing obscure code paths that will be executed rarely, risking bugs in favor of adding tests when fixing said bugs when – and if – they arise). There is nothing inherently wrong in the way you want to have the test suite, it is a matter of preference, that is all. I would like a more light-weight test suite that runs much faster, you want a thorough one, even if it takes more time to run. So: you are the boss, you do the things you do, and my opinion does not matter. I say this most pragmatically, to save more time by ending this discussion now. There are no hard feelings on my side. Ciao, Johannes
Re: [PATCH 2/2] receive-pack: support push-to-checkout hook
Johannes Schindelin johannes.schinde...@gmx.de writes: ... (i.e. by only lightly testing obscure code paths that will be executed rarely, risking bugs in favor of adding tests when fixing said bugs when – and if – they arise). I'd like to learn a bit more about this part, not because I want to say you are wrong, but because I want to find out what you say is practically useful and can be adopted by the project. The part I find most troublesome is this. Without tests covering obscure cases, how would you expect to notice when the codebase regresses, at which point you plan to fix and add tests for the fix? Not that I think the minimally these need to be tested list I sent earlier are anything obscure (they are all essential part of the feature; without them working, the feature would not be useful). -- 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] Add another option for receive.denyCurrentBranch
Junio C Hamano gits...@pobox.com writes: Johannes Schindelin johannes.schinde...@gmx.de writes: This is because you know receive-pack runs inside the $GIT_DIR, whether it is a bare or non-bare repository, so either core.worktree points at a directory that is otherwise unrelated to the $GIT_DIR (but is the correct $GIT_WORK_TREE), or the top of the working tree must be .. for a non-bare repository And this reasoning may be broken, unfortunately. In a repository with separate-git-dir, we enter $GIT_DIR that is pointed by the gitdir: $over_there thing, and once we go there, we have no linkage back to find where the working tree is unless there is core.worktree set, do we? This feature (with or without the push-to-checkout hook, as that shares exactly the same logic that discovers, or fails to do so, where the working tree is) needs to be documented with an entry in the BUGS section, saying that it will not work in a repository that is tied to its working tree via the gitdir: mechanism. It actually is a lot worse than merely it will not work, when this problem ever manifests itself. The use of this mechanism in such a repository will destroy the contents of a wrong directory that happens to be the parent directory of a repository pointed by the gitdir: mechanism, unless core.worktree is set. Fortunately, real submodule directories found in the .git/modules/ directory of the superproject, even though they are bound to their checkout locations in the working tree of the superproject using gitdir: mechanism, do maintain their core.worktree when git submodule manages them, so the use of the mechanism in submodule setting may be safe. Actually the other part of the system that uses gitdir: mechanism, i.e. git init --separate-git-dir $real $worktree, does add the core.worktree configuration in $real/config pointing at $worktree, so the above is simply me being overly worried. If somebody uses the gitdir: mechanism without setting core.worktree to point back, that is a misconfigured repository/worktree pair, so there is no need for any BUGS entry. Sorry about the noise. -- 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] introduce git root
Jeff King p...@peff.net writes: There is also git var, which is a catch-all for printing some deduced environmental defaults. I'd be just as happy to see it go away, though. Having: git --exec-path git --toplevel git --author-ident all work would make sense to me (I often get confused between git --foo and git rev-parse --foo when trying to get the exec-path and git-dir). And I don't think it's too late to move in this direction. We'd have to keep the old interfaces around, of course, but it would immediately improve discoverability and consistency. Yeah, I too think the above makes sense. I forgot about var, but it should go at the same time we move kitchen-sink options out of rev-parse. One less command to worry about at the UI level means you do not have to say if you want to learn about X, ask 'var', if you want to learn about Y, ask 'rev-parse', use 'git' itself for Z. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git add -i: allow list (un)selection by regexp
Aarni Koskela aarni.kosk...@andersinnovations.com writes:
From 9096652a71666920ae8d59dd4317d536ba974d5b Mon Sep 17 00:00:00 2001
From: Aarni Koskela a...@iki.fi
Date: Tue, 2 Dec 2014 13:56:15 +0200
Subject: [PATCH] git-add--interactive: allow list (un)selection by regular
expression
Remove the three lines from the top, move the content on Subject: to
the subject of the e-mail.
Other than that, everything I see in this message is very well
done.
Thanks, will queue.
Teach `list_and_choose` to allow `/regexp` and `-/regexp` syntax to
select items based on regular expression match.
This feature works in all list menus in `git-add--interactive`, and is not
limited to file menus only.
For instance, in file lists, `/\.c$` will select all files whose extension
is `.c`. In option menus, such as the main menu, `/pa` could be used to
choose the `patch` option.
Signed-off-by: Aarni Koskela a...@iki.fi
---
Thank you for the insightful comments, Junio, and sorry for the confusion
regarding email-patch formatting. Hoping I get it right this time.
Usually the responsibility to ensure correctness lies on the person who
proposes a change, not those who relies on the continued correct operation
of the existing code.
You're of course absolutely right. My point was that I can't think of an use
case where one would need to otherwise have / or -/ as the first
characters
of input in a list_and_choose situation, but someone else might.
[...] but is this about the selection that happens after showing you a
list of filenames to choose from?
I clarified this in the commit message. Selection by regexp works in all
list_and_choose situations, including the main menu of `git add -i`, hence
option.
Regarding the unchoose quantifier -- yes, silly me.
And regarding error checking for the regular expression, you're right -- the
program promptly blew up when entering an invalid regexp. I incorporated your
suggestion for error checking, with the addition of using the `error_msg` sub
for colorized error reporting.
Best regards,
Aarni Koskela
git-add--interactive.perl | 49
+++
1 file changed, 49 insertions(+)
diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index 1fadd69..28e4c2d 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -483,6 +483,8 @@ sub is_valid_prefix {
!($prefix =~ /[\s,]/) # separators
!($prefix =~ /^-/) # deselection
!($prefix =~ /^\d+/) # selection
+ !($prefix =~ /^\//)# regexp selection
+ !($prefix =~ /^-\//) # regexp unselection
($prefix ne '*') # all wildcard
($prefix ne '?');# prompt help
}
@@ -585,6 +587,50 @@ sub list_and_choose {
prompt_help_cmd();
next TOPLOOP;
}
+ if ($line =~ /^(-)?\/(.+)$/) {
+ # The first capture group (-) being missing means
choose is
+ # requested. If the first group exists at all,
unchoose is
+ # requested.
+ my $choose = !(defined $1);
+
+ # Validate the regular expression and complain if
compilation failed.
+ my $re = eval { qr/$2/ };
+ if (!$re) {
+ error_msg Invalid regular expression:\n $@\n;
+ next TOPLOOP;
+ }
+
+ my $found = 0;
+ for ($i = 0; $i @stuff; $i++) {
+ my $val = $stuff[$i];
+
+ # Figure out the display value for $val.
+ # Some lists passed to list_and_choose contain
+ # items other than strings; in order to match
+ # regexps against them, we need to extract the
+ # displayed string. The logic here is
approximately
+ # equivalent to the display logic above.
+
+ my $ref = ref $val;
+ if ($ref eq 'ARRAY') {
+ $val = $val-[0];
+ }
+ elsif ($ref eq 'HASH') {
+ $val = $val-{VALUE};
+ }
+
+ # Match the string value against the regexp,
+ # then act accordingly.
+
+ if ($val =~ $re) {
+ $chosen[$i] = $choose;
+ $found = $found || $choose;
+ last if $choose $opts-{SINGLETON};
+ }
+ }
+ last if
Re: [PATCH 24/34] checkout: reject if the branch is already checked out elsewhere
Duy Nguyen pclo...@gmail.com writes:
FWIW git-branch usually can show the original branch of detached head
(must not always). I don't think we have a plumbing equivalent for it
though. People can tail -1 $GIT_DIR/logs/HEAD| sed .. but that seems
hacky.
@{-1}, i.e. the last branch I checked out?
I do like read-only ref concept where we can keep ref name
(especially tags) in HEAD until the next commit. But it didn't go
anywhere
Remind me. That sounds somewhat interesting.
--
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: RCS Keywords in Git done right
PPS: Sounds like I need Peff's git-blame-tree from here: https://github.com/peff/git/compare/jk/faster-blame-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: http-protocol question
Jeff King p...@peff.net writes: For a public repository, it might make sense to provide a config option to loosen the is_our_ref check completely (i.e., to just has_sha1_file). But such an option does not yet exist. In principle, yes, but that cannot be has_sha1_file(); it has to have a fully connected healthy history behind it. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC] [PATCH] remote: add new --fetch option for set-url
On Saturday 29 November 2014 13:31:18 Philip Oakley wrote: From: Peter Wu pe...@lekensteyn.nl Ok, I will make a clear note about the default (without --only) behavior having weird behavior for historical reasons. Are you really OK with --only=both? It sounds a bit odd (mathematically speaking it is correct as fetch and push are both partitions that form the whole set if you ignore the historical behavior). How about : s/--only/--direction/ or some suitable abbreviation (--dirn ?) In the next version of the patch I went for three separate options, --fetch, --push and --both: http://article.gmane.org/gmane.comp.version-control.git/260213 (Juno, Jeff: ping?) The option --direction=fetch|push|both is a bit longer and --dirn can be mistaken for directory N. -- Kind regards, Peter https://lekensteyn.nl -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: git status / git diff -C not detecting file copy
Jeff King p...@peff.net writes: Interestingly, the rename behavior dates all the way back to: commit 753fd78458b6d7d0e65ce0ebe7b62e1bc55f3992 Author: Linus Torvalds torva...@ppc970.osdl.org Date: Fri Jun 17 15:34:19 2005 -0700 Use -M instead of -C for git diff and git status The C in -C may stand for Cool, but it's also pretty slow, since right now it leaves all unmodified files to be tested even if there are no new files at all. That just ends up being unacceptably slow for big projects, especially if it's not all in the cache. ... To get a rough sense of how much effort is entailed in the various options, here are git log --raw timings for git.git (all timings are warm cache, best-of-five, wall clock time): The rationale of the change talks about big projects and your analysis and argument to advocate reversion of that change is based on git.git? What is going on here? Also our history is fairly unusual in that we do not have a lot of renames (there was one large s|builtin-|builtin/| rename event, but that is about it), which may not be a good example to base such a design decision on. It is a fine idea to make this configurable (status.renames = -C or something, perhaps?), though. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Git Bash for Mac
On Tue, Dec 2, 2014 at 1:21 PM, Nizamuddin Chowdhury mchowdhur...@gmail.com wrote: Good Morning, My name is Sefath, and I was wondering when i could start using Git for Mac. I’m completely new to coding, and I wanted to start with HTML. However, when I tried installing git bash on my mac, it doesn’t work. Maybe it isn’t compatible with OS X Yosmite? I would really love to start learning code, and it sucks that I can’t because of a reason like this. git bash is a windows port for git, so that's not suitable for OSX. You'll need to have a Mac build of git, which you can find here: http://git-scm.com/download/mac -- 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: http-protocol question
On Tue, Dec 02, 2014 at 09:45:06AM -0800, Junio C Hamano wrote: Jeff King p...@peff.net writes: For a public repository, it might make sense to provide a config option to loosen the is_our_ref check completely (i.e., to just has_sha1_file). But such an option does not yet exist. In principle, yes, but that cannot be has_sha1_file(); it has to have a fully connected healthy history behind it. I thought about that, but I wonder if it matters whether we check up front. We are not advertising it, but only trying our best to fulfill the want from the other side. So either: 1. We check immediately whether we have the full history behind it, and reject the want otherwise. 2. We start pack-objects on it, and the first thing it will do is collect the full set of objects to send. If it doesn't have them, it will barf. Probably (1) would produce nicer error messages, but (2) is a lot more efficient. I dunno. I am not volunteering to implement, so I will leave thinking on it more to somebody who wants to do so. :) -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC][PATCH] send-email: add --[no-]xmailer option
On Mon, Mar 24, 2014 at 09:38:27PM +, Luis Henriques wrote:
Add --[no-]xmailer that allows a user to disable adding the 'X-Mailer:'
header to the email being sent.
Ping
It's been a while since I sent this patch. Is there any interest in
having this switch in git-send-email?
I honestly don't like disclosing too much information about my system,
in this case which MUA I'm using and its version.
Cheers,
--
Luís
Signed-off-by: Luis Henriques hen...@camandro.org
---
Documentation/config.txt | 1 +
Documentation/git-send-email.txt | 3 +++
git-send-email.perl | 12 ++--
3 files changed, 14 insertions(+), 2 deletions(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 73c8973..c33d5a1 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -,6 +,7 @@ sendemail.smtpserveroption::
sendemail.smtpuser::
sendemail.thread::
sendemail.validate::
+sendemail.xmailer::
See linkgit:git-send-email[1] for description.
sendemail.signedoffcc::
diff --git a/Documentation/git-send-email.txt
b/Documentation/git-send-email.txt
index f0e57a5..fab6264 100644
--- a/Documentation/git-send-email.txt
+++ b/Documentation/git-send-email.txt
@@ -131,6 +131,9 @@ Note that no attempts whatsoever are made to validate the
encoding.
Specify encoding of compose message. Default is the value of the
'sendemail.composeencoding'; if that is unspecified, UTF-8 is assumed.
+--xmailer::
+ Prevent adding the X-Mailer: header. Default value is
+ 'sendemail.xmailer'.
Sending
~~~
diff --git a/git-send-email.perl b/git-send-email.perl
index fdb0029..8789124 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -54,6 +54,7 @@ git send-email [options] file | directory | rev-list
options
--[no-]bcc str * Email Bcc:
--subject str * Email Subject:
--in-reply-to str * Email In-Reply-To:
+--[no-]xmailer * Don't add X-Mailer: header. Default
on.
--[no-]annotate* Review each patch that will be sent in
an editor.
--compose * Open an editor for introduction.
--compose-encoding str * Encoding to assume for introduction.
@@ -174,6 +175,9 @@ my $force = 0;
my $multiedit;
my $editor;
+# Usage of X-Mailer email header
+my $xmailer;
+
sub do_edit {
if (!defined($editor)) {
$editor = Git::command_oneline('var', 'GIT_EDITOR');
@@ -214,7 +218,8 @@ my %config_bool_settings = (
signedoffcc = [\$signed_off_by_cc, undef], # Deprecated
validate = [\$validate, 1],
multiedit = [\$multiedit, undef],
-annotate = [\$annotate, undef]
+annotate = [\$annotate, undef],
+xmailer = [\$xmailer, 1]
);
my %config_settings = (
@@ -311,6 +316,7 @@ my $rc = GetOptions(h = \$help,
8bit-encoding=s = \$auto_8bit_encoding,
compose-encoding=s = \$compose_encoding,
force = \$force,
+ xmailer! = \$xmailer,
);
usage() if $help;
@@ -1144,8 +1150,10 @@ To: $to${ccline}
Subject: $subject
Date: $date
Message-Id: $message_id
-X-Mailer: git-send-email $gitversion
;
+ if ($xmailer) {
+ $header .= X-Mailer: git-send-email $gitversion\n;
+ }
if ($reply_to) {
$header .= In-Reply-To: $reply_to\n;
--
1.9.1
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: git status / git diff -C not detecting file copy
On Tue, Dec 02, 2014 at 09:57:07AM -0800, Junio C Hamano wrote: To get a rough sense of how much effort is entailed in the various options, here are git log --raw timings for git.git (all timings are warm cache, best-of-five, wall clock time): The rationale of the change talks about big projects and your analysis and argument to advocate reversion of that change is based on git.git? What is going on here? I find that git.git is often a useful and easy thing to time to extrapolate to other projects. It's 1/10th-1/20th the size of the kernel (both in tree size and commit depth), which I do consider a big project (and I have a feeling is what Linus was talking about). Of course, performance numbers do not always scale linearly with repo size. I didn't show the full numbers for the kernel, but they are: log --raw: 0m53.587s log --raw -M:0m55.424s log --raw -C:1m02.733s log --raw -C -C: killed after 10 minutes There are ~20K commits that introduce files in the kernel (about 10x what git.git had). So renames add well under a millisecond to each of those diffs, and a single -C adds a third of a millisecond. Which is pretty in-line with the git.git findings (it is not linear here, but actually fairly constant. This makes sense, as it scales with the size of the commit, not the size of the tree). And as I noted, -C -C is rather expensive (I gave some estimated timings earlier; you could probably come up with something more accurate by doing smarter sampling). Also our history is fairly unusual in that we do not have a lot of renames (there was one large s|builtin-|builtin/| rename event, but that is about it), which may not be a good example to base such a design decision on. I think the work scales not with the number of actual renames, but with the number of commits where we even bother to look at renames at all (i.e., ones with an 'A' diff-status). And my estimates assume that we pay zero cost for other diffs, and attribute all of the extra time to those diffs. So I think frequency of rename (or 'A') events does not impact the estimate of the impact on a single git status run. What does impact it is the _size_ of each commit. If you quite frequently add a new file while touching tens of thousands of other files, then the performance will be a lot more noticeable. And both git.git and linux.git are probably better than some other projects about having small commits. Still, though. I stand by my earlier conclusions. Even with commits ten times as large as the kernel's, you are talking about 3ms added to a status run (and again, only when you hit such a gigantic commit _and_ it has an 'A' file). It is a fine idea to make this configurable (status.renames = -C or something, perhaps?), though. I think it would be OK to move to -C as a default, but I agree it is nicer if it is configurable, as it gives a safety hatch for people in pathological repos to drop back to the old behavior (or even turn off renames altogether). -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/19] Add git-list-files
On Tue, Dec 02, 2014 at 06:45:52PM +0700, Duy Nguyen wrote: As a side note, I wonder if it would be sensible to whitelist some commands as porcelain, and allow aliases to override them (either entirely, or just to add-in some options). Agreed. Maybe not all porcelain (some like git-branch almost functions like plumbing). Yeah, many things straddle the plumbing/porcelain line (e.g., commit is porcelain, but it's basically the only sane way for scripts to make a commit, so many use it). So I'd pick just a few things that should be safe to override. We also need away to stop alias (e.g. in scripts). Do we? I think the point of allowing this only for porcelain is that you do not have to care about scripts. That is, a script running git ls would get whatever the user's preferences are for ls output. A script parsing the output of ls deserves whatever crap it gets. In scripts I can specify full path to a command to make sure I won't hit an alias. I guess we can't do the same here. The closet to full path is git-cmd form, as opposed to git cmd form) but I think we don't want to bring back git-cmd. Maybe just a git --no-alias cmd and GIT_NO_ALIAS.. Yeah, I think --no-alias/GIT_NO_ALIAS would work. But the problem is one of compatibility. Scripts are not written to specify no-alias, so you cannot just turn on the override-commands-with-aliases feature immediately (and likewise, scripts have little incentive to bother annotating their calls if it the override feature is not enabled). -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/RFC v2] Squashed changes for multiple worktrees vs. submodules
Am 01.12.2014 um 00:27 schrieb Max Kirillov:
builtin/checkout.c: use absolute path instead of given argument for
picking worktree name, it happens to be needed because for submodule
checkout the new worktree is always .
environment.c: add GIT_COMMON_DIR to local_repo_env
git-submodule.sh: implement automatic cloning of main repository
and checkout to new worktree at submodule update --init
path.c, setup.c, submodule.c: fix diff --submodule when
submodule is a linked worktree
t/t7410-submodule-checkout-to.sh: tests for all the above
Signed-off-by: Max Kirillov m...@max630.net
---
Hi.
Thanks for including my 2 patches.
But, while hacking the submodule init I became more convinced that the
modules directory should be common and submodules in checkout should be
a checkouts of the submodule. Because this is looks like concept of
submodules, that they are unique for the lifetime of repository, even if
they do not exist in all revisions. And if anybody want to use fully
independent checkout they can be always checked out manually. Actually,
after a submodule is initialized and have a proper gitlink, it can be
updated and inquired regardless of where it points to.
If I understand you correctly you want to put the submodule's common
git dir under the superproject's common git dir. I agree that that
makes most sense as the default, but having the possibility to use a
common git dir for submodule's of different superprojects would be
nice to have for some setups, e.g. CI-servers. But that can be added
later.
So that one I think is not needed. I have instead some changes to
git-submodule, but have not prepared them yet as an exportable history.
I am submitting here squashed changes which I have so far, to give an
idea where it goes. I'll try to prepare a proper patch series as soon as
I can.
Thanks. I just didn't quite understand why you had to do so many
changes to git-submodule.sh. Wouldn't it be sufficient to just
update module_clone()?
If the superproject uses a common git dir I'd expect module_clone()
to set up the local superproject's worktree .git/modules/name
referencing the /modules/name directory of the superproject's
common git dir as the submodule's common git dir. So instead of a
clone of the submodule's upstream it would put a multiple worktree
version of the submodule under .git/modules/name. Then there
should be no further difference between a submodule that borrows
from the common git dir an one that doesn't.
Am I missing something about how the common dir thingy works? Or
maybe that .git/modules/name is bare is a problem here?
They contain change $gmane/258173, which I think is important,
especially because it is required not only for initialization but for
regular work also, and changes for initialization of submodules.
They are rebased on top of you patches excluding the 34/34 patch.
builtin/checkout.c | 25 ++---
cache.h | 1 +
environment.c| 1 +
git-submodule.sh | 94 ++
path.c | 24 -
setup.c | 17 +++-
submodule.c | 28 ++
t/t7410-submodule-checkout-to.sh | 201 +++
8 files changed, 332 insertions(+), 59 deletions(-)
create mode 100755 t/t7410-submodule-checkout-to.sh
diff --git a/builtin/checkout.c b/builtin/checkout.c
index 953b763..78154ae 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -858,27 +858,29 @@ static int prepare_linked_checkout(const struct
checkout_opts *opts,
{
struct strbuf sb_git = STRBUF_INIT, sb_repo = STRBUF_INIT;
struct strbuf sb = STRBUF_INIT;
- const char *path = opts-new_worktree, *name;
+ struct strbuf sb_path = STRBUF_INIT;
+ const char *name;
struct stat st;
struct child_process cp;
int counter = 0, len, ret;
if (!new-commit)
die(_(no branch specified));
- if (file_exists(path) !is_empty_dir(path))
- die(_('%s' already exists), path);
+ strbuf_add_absolute_path(sb_path, opts-new_worktree);
+ if (file_exists(sb_path.buf) !is_empty_dir(sb_path.buf))
+ die(_('%s' already exists), sb_path.buf);
- len = strlen(path);
- while (len is_dir_sep(path[len - 1]))
+ len = sb_path.len;
+ while (len is_dir_sep(sb_path.buf[len - 1]))
len--;
- for (name = path + len - 1; name path; name--)
+ for (name = sb_path.buf + len - 1; name sb_path.buf; name--)
if (is_dir_sep(*name)) {
name++;
break;
}
strbuf_addstr(sb_repo,
- git_path(worktrees/%.*s, (int)(path + len - name),
name));
+ git_path(worktrees/%.*s, (int)(sb_path.buf + len -
name), name));
len = sb_repo.len;
if
Re: [PATCH] t/lib-gpg: adjust permissions for gnupg 2.1
On Tue, Dec 02, 2014 at 02:40:27PM +0100, Michael J Gruber wrote: Before gnupg 2.1 (aka modern branch), gpghome would contain only files which allowed t/lib-gpg.sh to set permissions explicitely, and we did that since 28a1b07 (t/lib-gpg: adjust permissions for gnupg 2.1, 2014-12-02) in order to adjust wrong permissions from a checkout on ro file systems. I think this 28a1b07 is wrong. Did you mean e7f224f? gnupg 2.1 creates a new directory in gpghome which would get its x bit removed. Thanks for digging in this. The story is a little more tricky, though, and I do not think this patch is strictly necessary. We copy lib-gpg/* to the trash directory, and only run gpg on it there. So it is there that gpg2.1 will munge the files, _after_ we have copied and done our chmod. And that works fine with the current code. The problem came when I was trying to test/debug, and outside of the tests did cd lib-gpg gpg2 That munged my lib-gpg directory, and the resulting breakage was copied into each subsequent trash directory. So while your patch is not necessary, it is a nice defense against this sort of manual munging, or against future patches which add more directories. But... Adjust and use +X so that any directory would get its x bit set. This also keeps the x bit on files which had it set for whatever wrong reason, but we care only about having at least the necessary permissions for the tests to run. Taking a step back, though, I am not sure I understand the reasoning behind the original e7f224f. The rationale in the commit message is that we want to make sure that the files are writable. But why would they not be? They are created by cp -R, so unless your umask does not allow the owner to write to the files, they should be writable, no? And if your umask is set that way, lots of things are going to break. And indeed, if I remove the chmods completely, like: diff --git a/t/lib-gpg.sh b/t/lib-gpg.sh index cd2baef..6ee4bb6 100755 --- a/t/lib-gpg.sh +++ b/t/lib-gpg.sh @@ -17,8 +17,6 @@ else # Name and email: C O Mitter commit...@example.com # No password given, to enable non-interactive operation. cp -R $TEST_DIRECTORY/lib-gpg ./gpghome - chmod 0700 gpghome - chmod 0600 gpghome/* GNUPGHOME=$(pwd)/gpghome export GNUPGHOME test_set_prereq GPG the tests run fine for me. What am I missing? I do think the original 0700 chmod _is_ useful, though. But not because it makes sure things are writable, but because it makes sure that it is _not_ world-readable. GPG complains about the lax permissions (of course it does not know that the keyrings are not really secrets in this case). However, this does not actually prevent the tests from running successfully. So from my perspective, the simplest thing is to keep the original chmod 0700 for that reason (or make it chmod go-rwx, if you like), and drop the inner chmod completely (effectively reverting e7f224f). But again, perhaps there is some case that it covers that I do not understand. -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: tests do not work with gpg 2.1
On Tue, Dec 02, 2014 at 01:55:31PM +0100, Michael J Gruber wrote: That private-keys directory is from the first run of gpg2.1 on a pre-2.1 GPGHOME. It converts the old secring db to that new dir of entries and uses that instead. Thanks for untangling this. As I mentioned elsewhere in the thread, it was just that I had munged my parent lib-gpg directory. Cleaning that up fixed the problem I was seeing, and I could proceed with experimenting. I think if you unset GPG_AGENT_INFO, gpg2.1 thinks there is no agent, starts it's own and talks to it via a socket directly (no env variable). Now that one seems come with different options (regarding pinentry) so that it can't even ask you for a passphrase. If I unset GPG_AGENT_INFO, I still get the original behavior; a pop-up dialog that asks for the passphrase (and feeding it the empty passphrase works). My differing behavior from Steven may just be quirks in our setup, or maybe it is the fact that I still have gpg1 installed. I think the fundamental problem, though, is just that gpg2.1 cannot seamlessly handle the case of a keyring with no passphrase. I am sure this is not a well-tested case, since GPG devs likely would say you're doing it wrong. But obviously it makes sense here for testing purposes. I'm not sure if the most expedient path is trying to convince gpg developers that it's a bug, or if there is some workaround (like --passphrase-file /dev/null or something). I've been using the patch below to test, and am tempted to offer it for inclusion. But if we need to hack up the gpg command-line just for the tests, then lib-gpg.sh would end up setting gpg.program, and that would override what my patch is doing anyway. -- 8 -- Subject: Makefile: provide build-time config of gpg program If the user hasn't configured gpg.program, we fallback to running just gpg. Since it _can_ be overridden by run-time config, this is sufficient for most people who have some specific gpg they want to run. However, there are two reasons we might want a build-time configuration, too: 1. A binary package may want to hard-code a matching gpg without requiring that the user set up their PATH or config explicitly. 2. When running the test scripts, it's hard to debug tests using an alternate GPG, as it would involve tweaking each individual test script to set the gpg path. Let's provide a Makefile knob for tweaking this. Signed-off-by: Jeff King p...@peff.net --- Makefile| 6 ++ gpg-interface.c | 2 +- 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/Makefile b/Makefile index 827006b..e3c1ec1 100644 --- a/Makefile +++ b/Makefile @@ -400,6 +400,7 @@ INSTALL = install RPMBUILD = rpmbuild TCL_PATH = tclsh TCLTK_PATH = wish +GPG_PATH = gpg XGETTEXT = xgettext MSGFMT = msgfmt PTHREAD_LIBS = -lpthread @@ -1503,6 +1504,10 @@ SHELL_PATH_CQ_SQ = $(subst ','\'',$(SHELL_PATH_CQ)) BASIC_CFLAGS += -DSHELL_PATH='$(SHELL_PATH_CQ_SQ)' endif +GPG_PATH_CQ = $(subst ,\,$(subst \,\\,$(GPG_PATH))) +GPG_PATH_CQ_SQ = $(subst ','\'',$(GPG_PATH_CQ)) +BASIC_CFLAGS += -DGPG_PATH='$(GPG_PATH_CQ_SQ)' + GIT_USER_AGENT_SQ = $(subst ','\'',$(GIT_USER_AGENT)) GIT_USER_AGENT_CQ = $(subst ,\,$(subst \,\\,$(GIT_USER_AGENT))) GIT_USER_AGENT_CQ_SQ = $(subst ','\'',$(GIT_USER_AGENT_CQ)) @@ -2038,6 +2043,7 @@ GIT-BUILD-OPTIONS: FORCE @echo SHELL_PATH=\''$(subst ','\'',$(SHELL_PATH_SQ))'\' $@ @echo PERL_PATH=\''$(subst ','\'',$(PERL_PATH_SQ))'\' $@ @echo DIFF=\''$(subst ','\'',$(subst ','\'',$(DIFF)))'\' $@ + @echo GPG_PATH=\''$(subst ','\'',$(subst ','\'',$(GPG_PATH)))'\' $@ @echo PYTHON_PATH=\''$(subst ','\'',$(PYTHON_PATH_SQ))'\' $@ @echo TAR=\''$(subst ','\'',$(subst ','\'',$(TAR)))'\' $@ @echo NO_CURL=\''$(subst ','\'',$(subst ','\'',$(NO_CURL)))'\' $@ diff --git a/gpg-interface.c b/gpg-interface.c index 68b0c81..67c6e35 100644 --- a/gpg-interface.c +++ b/gpg-interface.c @@ -5,7 +5,7 @@ #include sigchain.h static char *configured_signing_key; -static const char *gpg_program = gpg; +static const char *gpg_program = GPG_PATH; #define PGP_SIGNATURE -BEGIN PGP SIGNATURE- #define PGP_MESSAGE -BEGIN PGP MESSAGE- -- 2.2.0.390.gf60752d -- 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: tests do not work with gpg 2.1
On Tue, Dec 02, 2014 at 04:21:33PM -0500, Jeff King wrote: I'm not sure if the most expedient path is trying to convince gpg developers that it's a bug, or if there is some workaround (like --passphrase-file /dev/null or something). I've been using the patch below to test, and am tempted to offer it for inclusion. But if we need to hack up the gpg command-line just for the tests, then lib-gpg.sh would end up setting gpg.program, and that would override what my patch is doing anyway. So...I tried that. So many things went wrong. :) For one thing, the build-time GPG_PATH patch I posted is not quite enough. We would probably want to pass it down to the test scripts, too, as they run gpg --version to figure out whether we have gpg or not. Secondly, you cannot set gpg.program to gpg2 --passphrase-file /dev/null, because we do not use the shell to exec gpg.program. This is unlike most of the rest of git-spawned programs, but of course changing it has compatibility problems. We'd probably want gpg.command or something as an alternative. And finally, after convincing git to really use --passphrase-file, I find that it does not fix the problem at all. GPG still insists on opening an agent window. Nor does --batch help. So I dunno. Maybe there is some clever way to work around it, but I do not know it. -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: git status / git diff -C not detecting file copy
On Tue, Dec 2, 2014 at 5:55 PM, Jeff King p...@peff.net wrote: So from these timings, I'd conclude that: 1. It's probably fine to turn on copies for git status. 2. It's probably even OK to use -C -C for some projects. Even though 22s looks scary there, that's only 11ms for git.git (remember, spread across 2000 commits). For linux.git, it's much, much worse. I killed my -C -C run after 10 minutes, and it had only gone through 1/20th of the commits. Extrapolating, you're looking at 500ms or so added to a git status run. So you'd almost certainly want this to be configurable. Does either of you want to try your hand at a patch? Just enabling copies should be a one-liner. Making it configurable is more involved, but should also be pretty straightforward. I'm interested in taking a stab at a patch, but I'd like to confirm which way to go. Based on Junio's reply I'm not certain the simple patch could get accepted (assuming I do all the submission parts properly and the implementation itself passes review). Does that mean the only real option is the configurable patch? -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: git status / git diff -C not detecting file copy
On Wed, Dec 03, 2014 at 08:40:47AM +1100, Bryan Turner wrote: On Tue, Dec 2, 2014 at 5:55 PM, Jeff King p...@peff.net wrote: So from these timings, I'd conclude that: 1. It's probably fine to turn on copies for git status. 2. It's probably even OK to use -C -C for some projects. Even though 22s looks scary there, that's only 11ms for git.git (remember, spread across 2000 commits). For linux.git, it's much, much worse. I killed my -C -C run after 10 minutes, and it had only gone through 1/20th of the commits. Extrapolating, you're looking at 500ms or so added to a git status run. So you'd almost certainly want this to be configurable. Does either of you want to try your hand at a patch? Just enabling copies should be a one-liner. Making it configurable is more involved, but should also be pretty straightforward. I'm interested in taking a stab at a patch, but I'd like to confirm which way to go. Based on Junio's reply I'm not certain the simple patch could get accepted (assuming I do all the submission parts properly and the implementation itself passes review). Does that mean the only real option is the configurable patch? I think this is the part where you get to use your judgement, and decide what you think the maintainer will take. :) Personally, I would probably go for the configurable version, as it is not that much harder, and is a nicer end-point. Junio gave an example elsewhere in which the config option value would look like -C -C. I'd consider trying to match diff.renames instead, which takes false/true/copies for its three levels. It may make sense to teach both places copies-harder or something similar, for completeness. -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/RFC v2] Squashed changes for multiple worktrees vs. submodules
On Tue, Dec 02, 2014 at 09:45:24PM +0100, Jens Lehmann wrote: But, while hacking the submodule init I became more convinced that the modules directory should be common and submodules in checkout should be a checkouts of the submodule. Because this is looks like concept of submodules, that they are unique for the lifetime of repository, even if they do not exist in all revisions. And if anybody want to use fully independent checkout they can be always checked out manually. Actually, after a submodule is initialized and have a proper gitlink, it can be updated and inquired regardless of where it points to. If I understand you correctly you want to put the submodule's common git dir under the superproject's common git dir. I agree that that makes most sense as the default, but having the possibility to use a common git dir for submodule's of different superprojects would be nice to have for some setups, e.g. CI-servers. But that can be added later. So far there is no separation of .git/config for different worktrees. As submodules rely on the config their separation cannot be done fully without changing that. So this should be really left for some later improvements. As a user I am currently perfectly satisfied with manually checking out or even cloning submodules inplace, I don't do it often. Thanks. I just didn't quite understand why you had to do so many changes to git-submodule.sh. Wouldn't it be sufficient to just update module_clone()? Thanks, I should try it. Probably I had the opposite idea in mind - keep module_clone as untouched as possible. Maybe I should see how it's going to look if I move all worktrees logic there. -- Max -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC] [PATCH] remote: add new --fetch option for set-url
Peter Wu pe...@lekensteyn.nl writes:
On Saturday 29 November 2014 13:31:18 Philip Oakley wrote:
From: Peter Wu pe...@lekensteyn.nl
Ok, I will make a clear note about the default (without --only)
behavior
having weird behavior for historical reasons. Are you really OK with
--only=both? It sounds a bit odd (mathematically speaking it is
correct
as fetch and push are both partitions that form the whole set if you
ignore the historical behavior).
How about :
s/--only/--direction/
or some suitable abbreviation (--dirn ?)
In the next version of the patch I went for three separate options,
--fetch, --push and --both:
http://article.gmane.org/gmane.comp.version-control.git/260213
(Juno, Jeff: ping?)
The option --direction=fetch|push|both is a bit longer and --dirn can
be mistaken for directory N.
If we have to have three variants, --{fetch,push,both} would be the
easiest to understand among the possibilities listed above, I would
think.
--
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
configure failed to detect no asciidoc
I just downloaded the latest git, and tried to build with: make configure ./configure make all doc build failed while building doc, asciidoc not found I would have thought the configure would have detected that. I downloaded, built, and installed asciidoc, and re-built git, things are mostly good. The documentation is still causing me trouble as my firewall doesn't like the html in Documentation/docbook.xsl, but that's probably a firewall issue. Is there documentation method, not requiring active web access? Mike -- 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] t/lib-gpg: adjust permissions for gnupg 2.1
Jeff King p...@peff.net writes: Taking a step back, though, I am not sure I understand the reasoning behind the original e7f224f. The rationale in the commit message is that we want to make sure that the files are writable. But why would they not be? They are created by cp -R,... Wait. After doing this, $ mkdir -p src/a src/b 2src/a/c chmod a-w src/b src/a/c $ cp -R src dst $ ls -lR dst dst/b and dst/a/c are 0440 (with umask 0027, which makes src/b and src/a/c also 0440, which is copied with cp -R). I was primarily worried about t/lib-gpg/* being read-only from a src-tarball extract when we had a discussion that led to e7f224f7 (t/lib-gpg: make gpghome files writable, 2014-10-24). -- 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
Disabling credential helper?
At $DAYJOB, we have a Git server[0] that supports the smart HTTP protocol. That server can return a 401 if the repository is private or doesn't exist. We have several scripts, some of which run interactively, some not, that we simply want to fail if git fetch gets a non-2xx code. Unfortunately, git is very insistent about trying to use the default credential helper to prompt for a username and password in this case, even opening /dev/tty. We've used GIT_ASKPASS=/bin/echo, which seems to solve the problem, although it's ugly and I'm concerned it might break in the future. Is there a better way to do this? I didn't see one in the documentation or code when I looked. [0] An Atlassian Stash instance. -- brian m. carlson / brian with sandals: Houston, Texas, US +1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only OpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187 signature.asc Description: Digital signature
Re: [PATCH] t/lib-gpg: adjust permissions for gnupg 2.1
On Tue, Dec 02, 2014 at 03:57:50PM -0800, Junio C Hamano wrote: Wait. After doing this, $ mkdir -p src/a src/b 2src/a/c chmod a-w src/b src/a/c $ cp -R src dst $ ls -lR dst dst/b and dst/a/c are 0440 (with umask 0027, which makes src/b and src/a/c also 0440, which is copied with cp -R). Who is running that chmod and why? I know you are trying to simulate somehow they lost their 'w' bit here, but what is that somehow? Git does not track write-bits. So any git checkout should always have the bit set, no? And likewise would any tarball generated by git-archive. Does tar lose it on extraction? I would not think it would do so, short of a broken umask. Confused... -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: configure failed to detect no asciidoc
On Tue, Dec 02, 2014 at 04:32:56PM -0700, Mike Berry wrote: The documentation is still causing me trouble as my firewall doesn't like the html in Documentation/docbook.xsl, but that's probably a firewall issue. Is there documentation method, not requiring active web access? If you have XML catalogs configured properly on your system, xsltproc will use them by default to avoid remote lookups. On Debian, the stylesheets and relevant catalog entries will be installed if you have the docbook-xsl package installed. -- brian m. carlson / brian with sandals: Houston, Texas, US +1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only OpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187 signature.asc Description: Digital signature
Re: [PATCH] git add -i: allow list (un)selection by regexp
On Tue, Dec 2, 2014 at 12:26 PM, Junio C Hamano gits...@pobox.com wrote: Aarni Koskela aarni.kosk...@andersinnovations.com writes: From 9096652a71666920ae8d59dd4317d536ba974d5b Mon Sep 17 00:00:00 2001 From: Aarni Koskela a...@iki.fi Date: Tue, 2 Dec 2014 13:56:15 +0200 Subject: [PATCH] git-add--interactive: allow list (un)selection by regular expression Remove the three lines from the top, move the content on Subject: to the subject of the e-mail. Other than that, everything I see in this message is very well done. Thanks, will queue. This change deserve a documentation update (Documentation/git-add.txt), does it not? Tests too, perhaps (assuming other git-add--interactive selections selection modes are already tested)? -- 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 1/3] tree.c: update read_tree_recursive callback to pass strbuf as base
On Tue, Dec 2, 2014 at 7:11 AM, Duy Nguyen pclo...@gmail.com wrote: On Tue, Dec 2, 2014 at 2:32 AM, Junio C Hamano gits...@pobox.com wrote: Nguyễn Thái Ngọc Duy pclo...@gmail.com writes: This allows the callback to use 'base' as a temporary buffer to quickly assemble full path without extra allocation. The callback has to restore it afterwards of course. Hmph, what's the quote around 'without' doing there? because it's only true if you haven't used up all preallocated space in strbuf. If someone passes an empty strbuf, then underneath strbuf may do a few realloc until the buffer is large enough. Would it be easier to understand if written like this? This allows the callback to use 'base' as a temporary buffer to quickly assemble full path, thus avoiding allocation/deallocation for each iteration step. -- 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: Deprecation warnings under XCode
On Mon, Dec 1, 2014 at 1:04 PM, Junio C Hamano gits...@pobox.com wrote: Torsten Bögershausen tbo...@web.de writes: On 12/01/2014 04:02 AM, Michael Blume wrote: I have no idea whether this should concern anyone, but my mac build of git shows CC imap-send.o imap-send.c:183:36: warning: 'ERR_error_string' is deprecated: first deprecated in OS X 10.7 [-Wdeprecated-declarations] fprintf(stderr, %s: %s\n, func, ERR_error_string(ERR_get_error(), NULL)); ^ Isn't the warning a warning ;-) I don't see this warnings because my openssl comes from /opt/local/include (Mac ports) Does anybody know which new functions exist in Mac OS X versions = 10.7 ? I have not been able to find suitable Mac OS X replacements (nor could I when resubmitting David's series [1] to use CommonCrypto). I am not a Mac person, but is this about APPLE_COMMON_CRYPTO support added in 4dcd7732 (Makefile: add support for Apple CommonCrypto facility, 2013-05-19) and be4c828b (imap-send: eliminate HMAC deprecation warnings on Mac OS X, 2013-05-19)? Specifically, the log message for 4dcd7732 begins like so: Makefile: add support for Apple CommonCrypto facility As of Mac OS X 10.7, Apple deprecated all OpenSSL functions due to OpenSSL ABI instability, thus leading to build warnings. As a replacement, Apple encourages developers to migrate to its own (stable) CommonCrypto facility. In the Makefile we seem to have this: # Define NO_APPLE_COMMON_CRYPTO if you are building on Darwin/Mac OS X # and do not want to use Apple's CommonCrypto library. This allows you # to provide your own OpenSSL library, for example from MacPorts. which makes it sound like using APPLE_COMMON_CRYPTO is the default for Mac. Perhaps those who do want to use CommonCrypto to avoid warnings should not define that macro? It's been a long time [1] since I looked at it, but I believe that David's CommonCrypto patch series only replaced OpenSSL calls for which Apple had provided CommonCrypto replacements. If my memory is correct, there were still plenty of OpenSSL deprecations warnings remaining after his patches (the warnings which started this thread) even without defining NO_APPLE_COMMON_CRYPTO. Thus, David's patches reduced the number of warnings but did not fully eliminate them. Checking again, it still seems to be the case that Apple neglects to provide CommonCrypto replacements for these OpenSSL functions which Apple itself deprecated. [1]: http://thread.gmane.org/gmane.comp.version-control.git/224833 -- 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: Disabling credential helper?
(+peff) Hi, brian m. carlson wrote: We've used GIT_ASKPASS=/bin/echo, which seems to solve the problem, although it's ugly and I'm concerned it might break in the future. Is there a better way to do this? That's a good question. Before falling back to the askpass based prompt, Git tries each credential helper matching the URL in turn, and there doesn't seem to be an option to override that behavior and disable credential helpers. As long as you have no credential helpers configured, your GIT_ASKPASS based approach should work fine. But once you have helpers configured, you're potentially in trouble. I'm wondering if we ought to provide an --no-credential-helpers option to help with this. (Or to go further and provide a way to unset configuration items --- e.g., '-c credential.*=unset'.) Thoughts? Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Deprecation warnings under XCode
On Tue, Dec 2, 2014 at 4:37 PM, Eric Sunshine sunsh...@sunshineco.com wrote: On Mon, Dec 1, 2014 at 1:04 PM, Junio C Hamano gits...@pobox.com wrote: Torsten Bögershausen tbo...@web.de writes: On 12/01/2014 04:02 AM, Michael Blume wrote: I have no idea whether this should concern anyone, but my mac build of git shows CC imap-send.o imap-send.c:183:36: warning: 'ERR_error_string' is deprecated: first deprecated in OS X 10.7 [-Wdeprecated-declarations] fprintf(stderr, %s: %s\n, func, ERR_error_string(ERR_get_error(), NULL)); ^ Isn't the warning a warning ;-) I don't see this warnings because my openssl comes from /opt/local/include (Mac ports) Does anybody know which new functions exist in Mac OS X versions = 10.7 ? I have not been able to find suitable Mac OS X replacements (nor could I when resubmitting David's series [1] to use CommonCrypto). I am not a Mac person, but is this about APPLE_COMMON_CRYPTO support added in 4dcd7732 (Makefile: add support for Apple CommonCrypto facility, 2013-05-19) and be4c828b (imap-send: eliminate HMAC deprecation warnings on Mac OS X, 2013-05-19)? Specifically, the log message for 4dcd7732 begins like so: Makefile: add support for Apple CommonCrypto facility As of Mac OS X 10.7, Apple deprecated all OpenSSL functions due to OpenSSL ABI instability, thus leading to build warnings. As a replacement, Apple encourages developers to migrate to its own (stable) CommonCrypto facility. In the Makefile we seem to have this: # Define NO_APPLE_COMMON_CRYPTO if you are building on Darwin/Mac OS X # and do not want to use Apple's CommonCrypto library. This allows you # to provide your own OpenSSL library, for example from MacPorts. which makes it sound like using APPLE_COMMON_CRYPTO is the default for Mac. Perhaps those who do want to use CommonCrypto to avoid warnings should not define that macro? It's been a long time [1] since I looked at it, but I believe that David's CommonCrypto patch series only replaced OpenSSL calls for which Apple had provided CommonCrypto replacements. If my memory is correct, there were still plenty of OpenSSL deprecations warnings remaining after his patches (the warnings which started this thread) even without defining NO_APPLE_COMMON_CRYPTO. Thus, David's patches reduced the number of warnings but did not fully eliminate them. Checking again, it still seems to be the case that Apple neglects to provide CommonCrypto replacements for these OpenSSL functions which Apple itself deprecated. [1]: http://thread.gmane.org/gmane.comp.version-control.git/224833 Apologies, accidentally sent this from inbox the first time. If there's actually no way to address this, is there a simple way to silence deprecation warnings only in this file? I only ask because overall the git build seems to be extremely quiet, and it seems valuable to preserve that, so that warnings we want to act on stick out. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Disabling credential helper?
On Tue, Dec 02, 2014 at 04:59:53PM -0800, Jonathan Nieder wrote: brian m. carlson wrote: We've used GIT_ASKPASS=/bin/echo, which seems to solve the problem, although it's ugly and I'm concerned it might break in the future. Is there a better way to do this? That's a good question. Before falling back to the askpass based prompt, Git tries each credential helper matching the URL in turn, and there doesn't seem to be an option to override that behavior and disable credential helpers. I think this has nothing at all to do with credential helpers. Git tries the helpers, of which there are none, and falls back to askpass and prompting on the terminal. There is no way to design a helper to say I tried and failed; do not bother prompting on the terminal. Git only sees that no helper provided an answer and uses its internal methods. I did at one point consider making the terminal prompt a credential helper (since it is, after all, no different from git's perspective; it's just a mechanism for somehow coming up with a username/password pair). But people generally thought that was unnecessarily complicated (and it certainly is for the common cases). As long as you have no credential helpers configured, your GIT_ASKPASS based approach should work fine. Yeah, it's fine (as is GIT_ASKPASS=true). You could also provide a credential helper that gives you an empty username and password. But in both cases, I think that git will then feed the empty password to the server again, resulting in an extra useless round-trip. You probably instead want to say stop now, git, there is nothing else to be done. We could teach the credential-helper code to do that (e.g., a helper returns stop=true and we respect that). But I think you can do it reasonably well today by making the input process fail. Sadly setting GIT_ASKPASS to false just makes git complain and then try harder[1]. But you can dissociate git from the terminal, like: $ setsid -w git ls-remote https://github.com/private/repo fatal: could not read Username for 'https://github.com': No such device or address That might have other fallouts if you use process groups for anything. I have no problem with either an option to disable the terminal prompting, or teaching the credential-helper interface to allow helpers to stop lookup, either of which would be cleaner. -Peff [1] Courtesy of 84d7273 (prompt: fall back to terminal if askpass fails, 2012-02-03). -- 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: Disabling credential helper?
Jeff King wrote: On Tue, Dec 02, 2014 at 04:59:53PM -0800, Jonathan Nieder wrote: As long as you have no credential helpers configured, your GIT_ASKPASS based approach should work fine. Yeah, it's fine (as is GIT_ASKPASS=true). You could also provide a credential helper that gives you an empty username and password. But in both cases, I think that git will then feed the empty password to the server again, resulting in an extra useless round-trip. You probably instead want to say stop now, git, there is nothing else to be done. We could teach the credential-helper code to do that (e.g., a helper returns stop=true and we respect that). But I think you can do it reasonably well today by making the input process fail. How can my scripts defend against a credential helper that I didn't set up that e.g. pops up a GUI window to ask for a password? Thanks, Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Disabling credential helper?
On Tue, Dec 02, 2014 at 05:29:50PM -0800, Jonathan Nieder wrote: Jeff King wrote: On Tue, Dec 02, 2014 at 04:59:53PM -0800, Jonathan Nieder wrote: As long as you have no credential helpers configured, your GIT_ASKPASS based approach should work fine. Yeah, it's fine (as is GIT_ASKPASS=true). You could also provide a credential helper that gives you an empty username and password. But in both cases, I think that git will then feed the empty password to the server again, resulting in an extra useless round-trip. You probably instead want to say stop now, git, there is nothing else to be done. We could teach the credential-helper code to do that (e.g., a helper returns stop=true and we respect that). But I think you can do it reasonably well today by making the input process fail. How can my scripts defend against a credential helper that I didn't set up that e.g. pops up a GUI window to ask for a password? Maybe I am misunderstanding the original situation, but I did not think that was the problem. I thought the situation was one where the environment was controlled, but Git still would not do what was wanted (if you did have such a renegade helper, setting GIT_ASKPASS certainly would not help, as it is the fallback). But to answer your question: you can't currently. I would be happy to have a config syntax that means reset this multi-value config option list to nothing, but it does not yet exist. It would be useful for more than just credential-helper config. -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: Our cumbersome mailing list workflow
On Sun, Nov 30, 2014 at 6:46 PM, Junio C Hamano gits...@pobox.com wrote: Michael Haggerty mhag...@alum.mit.edu writes: It seems like a few desirable features are being talked about here, and summarizing the discussion as centralized vs decentralized is too simplistic. What is really important? 1. Convenient and efficient, including for newcomers 2. Usable while offline 3. Usable in pure-text mode 4. Decentralized Something else? So when I started overtaking the ref log series by Ronnie, Ronnies main concern was missing reviewers time. So my idea was to make it as accessible as possible, so the reviewing party can use their time best. However here are a few points, I want to mention: * Having send emails as well as uploaded it to Gerrit, I either needed a ChangeId (Gerrit strictly requires them to track inter-patch diffs), and the mailing list here strictly avoids them, so I was told. Ok, that's my problem as I wasn't following the actual procedure of the Git development model (mailing list only). * That's why I stopped uploads to Gerrit, so I do not need to care about the ChangeIds any more. I am not sure if that improved the quality of my patches though. * I seem to not have found the right workflow with the mailing list yet, as I personally find copying around the inter-patch changelog very inconvenient. Most of the regulars here just need fewer iterations, so I can understand, that you find it less annoying. Hopefully I'll also get used to the nit-picky things and will require less review iterations in the future. How are non-regulars/newcomers, who supposingly need more iterations on a patch, supposed to handle the inter patch change log conveniently? I tried to keep the inter patch changelog be part of the commit message and then just before sending the email, I'd move it the non-permanent section of the email. * Editing patches as text files is hard/annoying. I have setup git send-email, and that works awesome, as I'd only need one command to send off a series. Having a step in between makes it more error-prone. So I do git format-patch and then inject the inter patch change log, check to remove ChangeId and then use git send-email. And at that final manual step I realized I am far from being perfect, so sometimes patches arrive on the mailing list, which are sub quality in the sense, that there are leftovers, i.e. a ChangeId * A possible feature, which just comes to my mind: Would it make sense for format-patch to not just show the diff stats, but also include, on which branch it applies? In git.git this is usually the origin/master branch, but dealing with patch series, building on top of each other that may be a good feature to have. When I had to view a large-ish series by Ronnie on Gerrit, it was fairly painful. The interaction on an individual patch might be more convenient and efficient using a system like Gerrit than via e-mailed patch with reply messages, but as a vehicle to review a large series and see how the whole thing fits together, I did not find pages that made it usable (I am avoiding to say I found it unusable, as that impression may be purely from that I couldn't find a more suitable pages that showed the same information in more usable form, i.e. user inexperience). So you're liking the email workflow more. How do you do the final formatting of an email, such as including the inter patch diff? Speaking of the whole picture, I am hesitant to see us pushed into the here is a central system (or here are federated systems) to handle only the patch reviews direction; our changes result after discussing unrelated features, wishes, or bugs that happen outside of any specific patches with enough frequency, and that is why I prefer everything in one place aspect of the development based on the mailing list. That is not to say that the one place has forever to be the mailing list, though. But the tooling around an e-mail based workflow (e.g. marking threads as worth revisiting for later inspection, saving chosen messages into a mailbox and running git am on it) is already something I am used to. Whatever system we might end up migrating to, the convenience it offers has to beat the convenience of existing workflow to be worth switching to, at least to me as a reviewer/contributor. I do like the way as well to just mark emails unread when I need to work on them later. As the maintainer, I am not worried too much. As long as the mechanism can (1) reach here is a series that is accepted by reviewers whose opinions are trusted efficiently, and (2) allow me to queue the result without mistakes, I can go along with anything reasonable. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/4] refs.c: rename the transaction functions
Stefan Beller wrote: No changes since sending it 5 days ago. branch.c | 13 + builtin/commit.c | 10 +++ builtin/fetch.c| 12 builtin/receive-pack.c | 13 - builtin/replace.c | 10 +++ builtin/tag.c | 10 +++ builtin/update-ref.c | 26 - fast-import.c | 22 +++--- refs.c | 78 +- refs.h | 36 +++ sequencer.c| 12 walker.c | 10 +++ 12 files changed, 126 insertions(+), 126 deletions(-) Reviewed-by: Jonathan Nieder jrnie...@gmail.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: [RFC][PATCH] send-email: add --[no-]xmailer option
Luis Henriques hen...@camandro.org wrote: On Mon, Mar 24, 2014 at 09:38:27PM +, Luis Henriques wrote: Add --[no-]xmailer that allows a user to disable adding the 'X-Mailer:' header to the email being sent. Ping It's been a while since I sent this patch. Is there any interest in having this switch in git-send-email? I wasn't paying attention when the original was sent, but this looks good to me. Acked-by: Eric Wong normalper...@yhbt.net I honestly don't like disclosing too much information about my system, in this case which MUA I'm using and its version. Right on. I would even favor this being the default. Auto-generated Message-Id headers also shows the use of git-send-email; perhaps there can be a way to configure that, too. However, git-send-email respects manually-added Message-Id headers in the original patch, so it's less of a problem, I suppose. -- 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/4] refs.c: rename transaction.updates to transaction.ref_updates
Stefan Beller wrote: The updates are only holding refs not reflogs, so express it to the reader. Signed-off-by: Stefan Beller sbel...@google.com --- refs.c | 14 +++--- 1 file changed, 7 insertions(+), 7 deletions(-) Makes sense. Reviewed-by: Jonathan Nieder jrnie...@gmail.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: Deprecation warnings under XCode
On Tue, Dec 2, 2014 at 8:12 PM, Michael Blume blume.m...@gmail.com wrote: On Tue, Dec 2, 2014 at 4:37 PM, Eric Sunshine sunsh...@sunshineco.com wrote: On Mon, Dec 1, 2014 at 1:04 PM, Junio C Hamano gits...@pobox.com wrote: I am not a Mac person, but is this about APPLE_COMMON_CRYPTO support added in 4dcd7732 (Makefile: add support for Apple CommonCrypto facility, 2013-05-19) and be4c828b (imap-send: eliminate HMAC deprecation warnings on Mac OS X, 2013-05-19)? [...] In the Makefile we seem to have this: # Define NO_APPLE_COMMON_CRYPTO if you are building on Darwin/Mac OS X # and do not want to use Apple's CommonCrypto library. This allows you # to provide your own OpenSSL library, for example from MacPorts. which makes it sound like using APPLE_COMMON_CRYPTO is the default for Mac. Perhaps those who do want to use CommonCrypto to avoid warnings should not define that macro? It's been a long time [1] since I looked at it, but I believe that David's CommonCrypto patch series only replaced OpenSSL calls for which Apple had provided CommonCrypto replacements. If my memory is correct, there were still plenty of OpenSSL deprecations warnings remaining after his patches (the warnings which started this thread) even without defining NO_APPLE_COMMON_CRYPTO. Thus, David's patches reduced the number of warnings but did not fully eliminate them. Checking again, it still seems to be the case that Apple neglects to provide CommonCrypto replacements for these OpenSSL functions which Apple itself deprecated. [1]: http://thread.gmane.org/gmane.comp.version-control.git/224833 If there's actually no way to address this, is there a simple way to silence deprecation warnings only in this file? I only ask because overall the git build seems to be extremely quiet, and it seems valuable to preserve that, so that warnings we want to act on stick out. An individual developer can add '-Wno-deprecated-declarations' to CFLAGS to suppress these warnings, however, that's pretty much a sledge hammer which would impact deprecations from all included headers, not just Apple's. For this reason, we probably wouldn't want to make this the default. The potentially lesser evil would be this small patch (minus Gmail whitespace damage) which disables the deprecation warnings only for Apple's headers: - 8 - diff --git a/git-compat-util.h b/git-compat-util.h index 400e921..709e84f 100644 --- a/git-compat-util.h +++ b/git-compat-util.h @@ -211,6 +211,8 @@ extern char *gitbasename(char *); #endif #ifndef NO_OPENSSL +#define __AVAILABILITY_MACROS_USES_AVAILABILITY 0 +#define MAC_OS_X_VERSION_MIN_REQUIRED MAC_OS_X_VERSION_10_6 #include openssl/ssl.h #include openssl/err.h #endif - 8 - It's still mildly heavy-handed, in that it could silence legitimate Apple deprecations, but it does give us a clean build with little fuss. An alternative would be to relegate these #defines to the Darwin section of the Makefile if placing them in git-compat-util.h seems too invasive. Considering that Mac OS X is now at 10.10 and these deprecations commenced with Mac OS X 10.7 in July 2011 (3.5 years ago), and Apple still has not provided drop-in CommonCrypto equivalents, it seems unlikely that they will do so any time soon. Consequently, suppressing these otherwise unavoidable warnings may be the best we can do. I'm willing to formalize and submit this as a proper patch if it's not considered too disgusting by the powers-that-be. -- 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 3/4] refs.c: add a transaction function to append a reflog entry
Stefan Beller wrote:
When performing a reflog transaction update, only write to the reflog iff
msg is non-NULL. This can then be combined with REFLOG_TRUNCATE to perform
an update that only truncates but does not write. This change only affects
whether or not a reflog entry should be generated and written. If msg == NULL
then no such entry will be written.
Orthogonal to this we have a boolean flag REFLOG_TRUNCATE which is used to
tell the transaction system to truncate the reflog and thus discard all
previous users.
At the current time the only place where we use msg == NULL is also the
place, where we use REFLOG_TRUNCATE. Even though these two settings are
currently only ever used together it still makes sense to have them through
two separate knobs.
This allows future consumers of this API that may want to do things
differently. For example someone can do:
msg=Reflog truncated by Bob because ... + REFLOG_TRUNCATE
and have it truncate the log and have it start fresh with an initial message
that explains the log was truncated. This API allows that.
During one transaction we allow to make multiple reflog updates to the
same ref. This means we only need to lock the reflog once, during the first
update that touches the reflog, and that all further updates can just write
the
reflog entry since the reflog is already locked.
I'm having trouble parsing all of the above. Can you explain the
motivation of the patch in a sentence or so? Afterward that, if the
API is not self-explanatory, there could be a short explanation of it
(e.g., a list of functions and how they get used).
[...]
--- a/refs.c
+++ b/refs.c
@@ -3557,6 +3557,12 @@ struct transaction {
struct ref_update **ref_updates;
size_t alloc;
size_t nr;
+
+ /*
+ * Sorted list of reflogs to be committed,
+ * the util points to the lock_file
+ */
+ struct string_list reflog_updates;
Grammar nit: where there is a comma here, there should be the end of a
sentence.
[...]
@@ -3564,7 +3570,10 @@ struct transaction *transaction_begin(struct strbuf
*err)
{
assert(err);
- return xcalloc(1, sizeof(struct transaction));
+ struct transaction *ret = xcalloc(1, sizeof(struct transaction));
+ string_list_init(ret-reflog_updates, 1);
Can do
ret-reflog_updates.strdup_strings = 1;
instead, since calloc already zeroed the memory.
[...]
+/* Returns a fd, -1 on error. */
+static int get_reflog_updates_fd(struct transaction *transaction,
+ const char *refname,
+ struct strbuf *err)
+{
+ char *path;
+ struct lock_file *lock;
+ struct string_list_item *item = string_list_insert(
+ transaction-reflog_updates,
+ refname);
+ if (!item-util) {
It can be clearer to handle the simple case first:
if (item-util) {
lock = item-util;
return lock-fd;
}
item-util = xcalloc(...);
+ item-util = xcalloc(1, sizeof(struct lock_file));
+ lock = item-util;
+ path = git_path(logs/locks/%s, refname);
+ if (safe_create_leading_directories(path)) {
+ strbuf_addf(err,
+ creating temporary directories %s failed.,
+ path);
Looking at other callers, looks like something like
if (scld(path)) {
strbuf_addf(err, could not create leading directories of '%s':
%s,
path, strerror(errno));
is common. That way, the message includes details from errno, it's
clear that one of the leading directories to $path, not $path itself,
was what could not be created, and there is no trailing '.' at the end
of the message.
+ if (hold_lock_file_for_update(lock, path, 0) 0) {
+ strbuf_addf(err,
+ creating temporary file %s failed.,
+ path);
hold_lock_file_for_update() is weird and has its own special error
message writing function:
unable_to_lock_message(path, errno, err);
That lets it give advice to the user about why writing the .lock
file failed and how to fix it.
I have a series that simplifies by making it write directly to a
strbuf passed as an argument, but that's orthogonal to this patch.
[...]
+int transaction_update_reflog(struct transaction *transaction,
+ const char *refname,
+ const unsigned char *new_sha1,
+ const unsigned char *old_sha1,
+ const char *email,
+ unsigned long timestamp, int tz,
+ const char *msg, int flags,
+ struct strbuf *err)
This is an
Re: [PATCH 4/4] reflog.c: use a reflog transaction when writing during expire
Stefan Beller wrote: --- a/builtin/reflog.c +++ b/builtin/reflog.c [...] @@ -290,7 +291,7 @@ static int unreachable(struct expire_reflog_cb *cb, struct commit *commit, unsig static int expire_reflog_ent(unsigned char *osha1, unsigned char *nsha1, const char *email, unsigned long timestamp, int tz, - const char *message, void *cb_data) + const char *message, void *cb_data, struct strbuf *err) This doesn't match the signature of each_reflog_ent_fn. Would putting err in the cb_data struct work? Curious, Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC][PATCH] send-email: add --[no-]xmailer option
On Dec 2, 2014, at 18:34, Eric Wong wrote:
Luis Henriques hen...@camandro.org wrote:
On Mon, Mar 24, 2014 at 09:38:27PM +, Luis Henriques wrote:
Add --[no-]xmailer that allows a user to disable adding the 'X-
Mailer:'
header to the email being sent.
Ping
It's been a while since I sent this patch. Is there any interest in
having this switch in git-send-email?
I wasn't paying attention when the original was sent, but this
looks good to me.
Acked-by: Eric Wong normalper...@yhbt.net
I honestly don't like disclosing too much information about my
system,
in this case which MUA I'm using and its version.
Right on. I would even favor this being the default.
I fully agree with you.
Auto-generated Message-Id headers also shows the use of git-send-
email;
perhaps there can be a way to configure that, too. However,
git-send-email respects manually-added Message-Id headers in the
original patch, so it's less of a problem, I suppose.
It can be hashed like so to avoid leaking information:
diff --git a/git-send-email.orig b/git-send-email.new
index f3d75e8..d0b4bff 100755
--- a/git-send-email.orig
+++ b/git-send-email.new
@@ -27,6 +27,7 @@ use Data::Dumper;
use Term::ANSIColor;
use File::Temp qw/ tempdir tempfile /;
use File::Spec::Functions qw(catfile);
+use Digest::MD5 qw(md5_hex);
use Error qw(:try);
use Git;
@@ -901,8 +903,10 @@ sub make_message_id {
require Sys::Hostname;
$du_part = 'user@' . Sys::Hostname::hostname();
}
- my $message_id_template = %s-git-send-email-%s;
+ my $message_id_template = %s-git-send-email-%s;
$message_id = sprintf($message_id_template, $uniq, $du_part);
+ @_ = split /@/, $message_id;
+ $message_id = ''.substr(md5_hex($_[0]),
0,31).'@'.substr(md5_hex($_[1]),1,31).'';
#print new message id = $message_id\n; # Was useful for debugging
}
---
--Kyle
--
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: Our cumbersome mailing list workflow
Stefan Beller wrote: How are non-regulars/newcomers, who supposingly need more iterations on a patch, supposed to handle the inter patch change log conveniently? I think this is one of the more important issues. I don't think there's any reason that newcomers should need more iterations than regulars to finish a patch. Regulars are actually held to a higher standard, so they are likely to need more iterations. A common mistake for newcomers, that I haven't learned yet how to warn properly against, is to keep re-sending minor iterations on a patch too quickly. Some ways to avoid that: * feel free to respond to review comments with something like how about this? and a copy/pasted block of code that just addresses that one comment. That way, you can clear up ambiguity and avoid the work of applying that change to the entire patch if it ends up seeming like a bad idea. This also avoids having to re-send a larger patch or series multiple times to clear up a small ambiguity from a review. * be proactive. Look for other examples of the same issue that a reviewer pointed out once so they don't have to find it again elsewhere in the next iteration. Run the testsuite. Build with the flags from https://kernel.googlesource.com/pub/scm/git/git/+/todo/Make#106 in CFLAGS in config.mak. Proofread and try to read as though you knew nothing about the patch to anticipate what reviewers will find. * feel free to get more review out-of-band, too. If you're still playing with ideas and want someone to take a quick glance before the patches are in reviewable form, you can do that and say so (e.g., with 'RFC/' before 'PATCH' in the subject line). Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH/RFC] rerere: error out on autoupdate failure
We have been silently tolerating errors by returning early with an
error that the caller ignores since rerere.autoupdate was introduced
in v1.6.0-rc0~120^2 (2008-06-22). So on error (for example if the
index is already locked), rerere can return success silently without
updating the index or with only some items in the index updated.
Better to treat such failures as a fatal error so the operator can
figure out what is wrong and fix it.
Signed-off-by: Jonathan Nieder jrnie...@gmail.com
---
I ran into this while auditing hold_locked_index callers. Tests
still pass after this change. Sensible?
rerere.c | 14 +-
1 file changed, 5 insertions(+), 9 deletions(-)
diff --git a/rerere.c b/rerere.c
index 1b0555f..195f663 100644
--- a/rerere.c
+++ b/rerere.c
@@ -477,27 +477,23 @@ out:
static struct lock_file index_lock;
-static int update_paths(struct string_list *update)
+static void update_paths(struct string_list *update)
{
int i;
- int fd = hold_locked_index(index_lock, 0);
- int status = 0;
- if (fd 0)
- return -1;
+ hold_locked_index(index_lock, 1);
for (i = 0; i update-nr; i++) {
struct string_list_item *item = update-items[i];
if (add_file_to_cache(item-string, ADD_CACHE_IGNORE_ERRORS))
- status = -1;
+ die(staging updated %s failed, item-string);
}
- if (!status active_cache_changed) {
+ if (active_cache_changed) {
if (write_locked_index(the_index, index_lock, COMMIT_LOCK))
die(Unable to write new index file);
- } else if (fd = 0)
+ } else
rollback_lock_file(index_lock);
- return status;
}
static int do_plain_rerere(struct string_list *rr, int fd)
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 0/14] Re: copy.c: make copy_fd preserve meaningful errno
Stefan Beller wrote: On Mon, Nov 17, 2014 at 4:48 PM, Jonathan Nieder jrnie...@gmail.com wrote: After this patch, setting errno is not part of the contract of copy_fd, so the bug Ronnie was fixing is gone. But it's a little more invasive. What do you think? I really like that approach and would be happy if Thanks for looking it over, and sorry for the delay. Here's a series that carries out that approach. The patch I'm least happy with in this series is 12/14, mostly because it's just so noisy. It would be safe (and non-leaky) to leave out most of those strbuf_release calls since nobody appends to 'err' in the non-error case, but always free-ing means that normal escape analysis can work. I could be convinced to go either way. Jonathan Nieder (14): strbuf: introduce strbuf_prefixf() add_to_alternates_file: respect GIT_OBJECT_DIRECTORY copy_fd: pass error message back through a strbuf hold_lock_file_for_append: pass error message back through a strbuf lock_packed_refs: pass error message back through a strbuf lockfile: introduce flag for locks outside .git fast-import: use message from lockfile API when writing marks fails credentials: use message from lockfile API when locking ~/.git-credentials fails config: use message from lockfile API when locking config file fails rerere: error out on autoupdate failure hold_locked_index: pass error message back through a strbuf hold_lock_file_for_update: pass error message back through a strbuf lockfile: remove unused function 'unable_to_lock_die' lockfile: make 'unable_to_lock_message' private Documentation/technical/api-lockfile.txt | 41 ++- builtin/add.c| 12 +++-- builtin/apply.c | 15 -- builtin/checkout-index.c | 10 +++- builtin/checkout.c | 55 ++-- builtin/clone.c | 12 - builtin/commit.c | 36 + builtin/describe.c | 11 ++-- builtin/diff.c | 12 +++-- builtin/gc.c | 8 ++- builtin/merge.c | 14 +++-- builtin/mv.c | 5 +- builtin/read-tree.c | 9 +++- builtin/reset.c | 10 +++- builtin/rm.c | 9 +++- builtin/update-index.c | 10 ++-- bundle.c | 10 ++-- cache-tree.c | 18 +-- cache.h | 4 +- config.c | 14 +++-- convert.c| 6 ++- copy.c | 32 credential-store.c | 8 ++- fast-import.c| 9 ++-- lockfile.c | 89 ++-- lockfile.h | 13 +++-- merge-recursive.c| 13 +++-- merge.c | 17 -- read-cache.c | 7 +-- refs.c | 28 ++ refs.h | 8 +-- rerere.c | 24 + sequencer.c | 33 +--- sha1_file.c | 17 -- shallow.c| 16 -- strbuf.c | 16 ++ strbuf.h | 4 ++ test-scrap-cache-tree.c | 5 +- 38 files changed, 434 insertions(+), 226 deletions(-) -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 01/14] strbuf: introduce strbuf_prefixf()
When preparing an error message in a strbuf, it can be convenient
to add a formatted string to the beginning:
if (transaction_commit(t, err)) {
strbuf_prefixf(err, cannot fetch '%s': , remotename);
return -1;
}
The new strbuf_prefixf is like strbuf_addf, except it writes its
result to the beginning of a strbuf instead of the end.
The current implementation uses strlen(strfmt(fmt, ...)) extra bytes
at the end of the strbuf as temporary scratch space for convenience
and simplicity. A later patch can optimize if needed.
Signed-off-by: Jonathan Nieder jrnie...@gmail.com
---
strbuf.c | 16
strbuf.h | 4
2 files changed, 20 insertions(+)
diff --git a/strbuf.c b/strbuf.c
index 0346e74..3f4aaa3 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -219,6 +219,22 @@ void strbuf_addf(struct strbuf *sb, const char *fmt, ...)
va_end(ap);
}
+void strbuf_prefixf(struct strbuf *sb, const char *fmt, ...)
+{
+ va_list ap;
+ size_t pos, len;
+
+ pos = sb-len;
+
+ va_start(ap, fmt);
+ strbuf_vaddf(sb, fmt, ap);
+ va_end(ap);
+
+ len = sb-len - pos;
+ strbuf_insert(sb, 0, sb-buf + pos, len);
+ strbuf_remove(sb, pos + len, len);
+}
+
static void add_lines(struct strbuf *out,
const char *prefix1,
const char *prefix2,
diff --git a/strbuf.h b/strbuf.h
index 652b6c4..5dae48e 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -158,6 +158,10 @@ extern void strbuf_vaddf(struct strbuf *sb, const char
*fmt, va_list ap);
extern void strbuf_add_lines(struct strbuf *sb, const char *prefix, const char
*buf, size_t size);
+/* Like strbuf_addf but insert at the front of sb instead of appending. */
+__attribute__((format (printf,2,3)))
+extern void strbuf_prefixf(struct strbuf *sb, const char *fmt, ...);
+
/*
* Append s to sb, with the characters '', '', '' and '' converted
* into XML entities.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 02/14] add_to_alternates_file: respect GIT_OBJECT_DIRECTORY
The objects directory is spelled as get_object_directory(), not
git_path(objects). Some other code still hard-codes the objects/
directory name, so in the long term we may want to get rid of the
pretense of support for GIT_OBJECT_DIRECTORY altogether, but this
makes the code more consistent for now.
While at it, split variable declarations from the rest of the
function. This makes the function a little easier to read, at the
cost of some vertical space.
Signed-off-by: Jonathan Nieder jrnie...@gmail.com
---
This is mainly for consistency / cleanliness. I wouldn't mind
dropping it.
The rest of 'git clone' is not careful about paying attention to
GIT_OBJECT_DIRECTORY either. I suspect GIT_OBJECT_DIRECTORY was a bit
of a failed experiment.
sha1_file.c | 12 +---
1 file changed, 9 insertions(+), 3 deletions(-)
diff --git a/sha1_file.c b/sha1_file.c
index d7f1838..e1945e2 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -403,9 +403,15 @@ void read_info_alternates(const char * relative_base, int
depth)
void add_to_alternates_file(const char *reference)
{
- struct lock_file *lock = xcalloc(1, sizeof(struct lock_file));
- int fd = hold_lock_file_for_append(lock,
git_path(objects/info/alternates), LOCK_DIE_ON_ERROR);
- char *alt = mkpath(%s\n, reference);
+ struct lock_file *lock;
+ int fd;
+ char *alt;
+
+ lock = xcalloc(1, sizeof(*lock));
+ fd = hold_lock_file_for_append(lock, mkpath(%s/info/alternates,
+ get_object_directory()),
+ LOCK_DIE_ON_ERROR);
+ alt = mkpath(%s\n, reference);
write_or_die(fd, alt, strlen(alt));
if (commit_lock_file(lock))
die(could not close alternates file);
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 03/14] copy_fd: pass error message back through a strbuf
This way, callers can put the message in context or even avoid
printing the message altogether.
Currently hold_lock_file_for_append tries to save errno in order to
produce a meaningful message about the failure and tries to print a
second message using errno. Unfortunately the errno it uses is not
meaningful because copy_fd makes no effort to set a meaningful errno
value. This change is preparation for simplifying the
hold_lock_file_for_append behavior.
No user-visible change intended yet.
Signed-off-by: Jonathan Nieder jrnie...@gmail.com
---
The title feature.
cache.h| 2 +-
convert.c | 6 +-
copy.c | 32 +---
lockfile.c | 6 +-
4 files changed, 32 insertions(+), 14 deletions(-)
diff --git a/cache.h b/cache.h
index 99ed096..ddaa30f 100644
--- a/cache.h
+++ b/cache.h
@@ -1479,7 +1479,7 @@ extern const char *git_mailmap_blob;
extern void maybe_flush_or_die(FILE *, const char *);
__attribute__((format (printf, 2, 3)))
extern void fprintf_or_die(FILE *, const char *fmt, ...);
-extern int copy_fd(int ifd, int ofd);
+extern int copy_fd(int ifd, int ofd, struct strbuf *err);
extern int copy_file(const char *dst, const char *src, int mode);
extern int copy_file_with_time(const char *dst, const char *src, int mode);
extern void write_or_die(int fd, const void *buf, size_t count);
diff --git a/convert.c b/convert.c
index 9a5612e..e301447 100644
--- a/convert.c
+++ b/convert.c
@@ -358,7 +358,11 @@ static int filter_buffer_or_fd(int in, int out, void *data)
if (params-src) {
write_err = (write_in_full(child_process.in, params-src,
params-size) 0);
} else {
- write_err = copy_fd(params-fd, child_process.in);
+ struct strbuf err = STRBUF_INIT;
+ write_err = copy_fd(params-fd, child_process.in, err);
+ if (write_err)
+ error(copy-fd: %s, err.buf);
+ strbuf_release(err);
}
if (close(child_process.in))
diff --git a/copy.c b/copy.c
index f2970ec..828661a 100644
--- a/copy.c
+++ b/copy.c
@@ -1,19 +1,22 @@
#include cache.h
-int copy_fd(int ifd, int ofd)
+int copy_fd(int ifd, int ofd, struct strbuf *err)
{
+ assert(err);
+
while (1) {
char buffer[8192];
ssize_t len = xread(ifd, buffer, sizeof(buffer));
if (!len)
break;
if (len 0) {
- return error(copy-fd: read returned %s,
-strerror(errno));
+ strbuf_addf(err, read returned %s, strerror(errno));
+ return -1;
+ }
+ if (write_in_full(ofd, buffer, len) 0) {
+ strbuf_addf(err, write returned %s, strerror(errno));
+ return -1;
}
- if (write_in_full(ofd, buffer, len) 0)
- return error(copy-fd: write returned %s,
-strerror(errno));
}
return 0;
}
@@ -33,7 +36,8 @@ static int copy_times(const char *dst, const char *src)
int copy_file(const char *dst, const char *src, int mode)
{
- int fdi, fdo, status;
+ int fdi, fdo;
+ struct strbuf err = STRBUF_INIT;
mode = (mode 0111) ? 0777 : 0666;
if ((fdi = open(src, O_RDONLY)) 0)
@@ -42,15 +46,21 @@ int copy_file(const char *dst, const char *src, int mode)
close(fdi);
return fdo;
}
- status = copy_fd(fdi, fdo);
+ if (copy_fd(fdi, fdo, err)) {
+ close(fdi);
+ close(fdo);
+ error(copy-fd: %s, err.buf);
+ strbuf_release(err);
+ return -1;
+ }
+ strbuf_release(err);
close(fdi);
if (close(fdo) != 0)
return error(%s: close error: %s, dst, strerror(errno));
-
- if (!status adjust_shared_perm(dst))
+ if (adjust_shared_perm(dst))
return -1;
- return status;
+ return 0;
}
int copy_file_with_time(const char *dst, const char *src, int mode)
diff --git a/lockfile.c b/lockfile.c
index 4f16ee7..b3020f3 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -182,6 +182,7 @@ int hold_lock_file_for_update(struct lock_file *lk, const
char *path, int flags)
int hold_lock_file_for_append(struct lock_file *lk, const char *path, int
flags)
{
int fd, orig_fd;
+ struct strbuf err = STRBUF_INIT;
fd = lock_file(lk, path, flags);
if (fd 0) {
@@ -202,9 +203,11 @@ int hold_lock_file_for_append(struct lock_file *lk, const
char *path, int flags)
errno = save_errno;
return -1;
}
- } else if (copy_fd(orig_fd, fd)) {
+ } else if (copy_fd(orig_fd, fd, err)) {
int save_errno = errno;
+ error(copy-fd: %s, err.buf);
[PATCH 04/14] hold_lock_file_for_append: pass error message back through a strbuf
This way, the code does not need to carefully safeguard errno to allow
callers to print a reasonable error message when they choose to do
some cleanup before die()-ing.
Fixes a bug waiting to happen where copy_fd would clobber the errno
passed back via hold_lock_file_for_append from read() or write() when
flags did not contain LOCK_DIE_ON_ERROR. Luckily the only caller uses
flags == LOCK_DIE_ON_ERROR, avoiding that bug in practice.
Reported-by: Ronnie Sahlberg sahlb...@google.com
Signed-off-by: Jonathan Nieder jrnie...@gmail.com
---
The advertised bugfix.
lockfile.c | 29 ++---
lockfile.h | 3 ++-
sha1_file.c | 7 ++-
3 files changed, 18 insertions(+), 21 deletions(-)
diff --git a/lockfile.c b/lockfile.c
index b3020f3..8685c68 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -179,45 +179,36 @@ int hold_lock_file_for_update(struct lock_file *lk, const
char *path, int flags)
return fd;
}
-int hold_lock_file_for_append(struct lock_file *lk, const char *path, int
flags)
+int hold_lock_file_for_append(struct lock_file *lk, const char *path,
+ int flags, struct strbuf *err)
{
int fd, orig_fd;
- struct strbuf err = STRBUF_INIT;
+
+ assert(!(flags LOCK_DIE_ON_ERROR));
+ assert(err !err-len);
fd = lock_file(lk, path, flags);
if (fd 0) {
- if (flags LOCK_DIE_ON_ERROR)
- unable_to_lock_die(path, errno);
+ unable_to_lock_message(path, errno, err);
return fd;
}
orig_fd = open(path, O_RDONLY);
if (orig_fd 0) {
if (errno != ENOENT) {
- int save_errno = errno;
-
- if (flags LOCK_DIE_ON_ERROR)
- die(cannot open '%s' for copying, path);
+ strbuf_addf(err, cannot open '%s' for copying: %s,
+ path, strerror(errno));
rollback_lock_file(lk);
- error(cannot open '%s' for copying, path);
- errno = save_errno;
return -1;
}
- } else if (copy_fd(orig_fd, fd, err)) {
- int save_errno = errno;
-
- error(copy-fd: %s, err.buf);
- strbuf_release(err);
- if (flags LOCK_DIE_ON_ERROR)
- exit(128);
+ } else if (copy_fd(orig_fd, fd, err)) {
+ strbuf_prefixf(err, cannot copy '%s': , path);
close(orig_fd);
rollback_lock_file(lk);
- errno = save_errno;
return -1;
} else {
close(orig_fd);
}
- strbuf_release(err);
return fd;
}
diff --git a/lockfile.h b/lockfile.h
index cd2ec95..ca36a1d 100644
--- a/lockfile.h
+++ b/lockfile.h
@@ -75,7 +75,8 @@ extern void unable_to_lock_message(const char *path, int err,
struct strbuf *buf);
extern NORETURN void unable_to_lock_die(const char *path, int err);
extern int hold_lock_file_for_update(struct lock_file *, const char *path,
int);
-extern int hold_lock_file_for_append(struct lock_file *, const char *path,
int);
+extern int hold_lock_file_for_append(struct lock_file *, const char *path,
+int, struct strbuf *err);
extern FILE *fdopen_lock_file(struct lock_file *, const char *mode);
extern char *get_locked_file_path(struct lock_file *);
extern int commit_lock_file_to(struct lock_file *, const char *path);
diff --git a/sha1_file.c b/sha1_file.c
index e1945e2..6c0ab3b 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -406,17 +406,22 @@ void add_to_alternates_file(const char *reference)
struct lock_file *lock;
int fd;
char *alt;
+ struct strbuf err = STRBUF_INIT;
lock = xcalloc(1, sizeof(*lock));
fd = hold_lock_file_for_append(lock, mkpath(%s/info/alternates,
get_object_directory()),
- LOCK_DIE_ON_ERROR);
+ 0, err);
+ if (fd 0)
+ die(%s, err.buf);
alt = mkpath(%s\n, reference);
write_or_die(fd, alt, strlen(alt));
if (commit_lock_file(lock))
die(could not close alternates file);
if (alt_odb_tail)
link_alt_odb_entries(alt, strlen(alt), '\n', NULL, 0);
+
+ strbuf_release(err);
}
int foreach_alt_odb(alt_odb_fn fn, void *cb)
--
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 05/14] lock_packed_refs: pass error message back through a strbuf
This saves us from having to be careful about preserving errno and
makes it more explicit in the die-on-error case that the caller is
exiting without a chance to clean up.
Signed-off-by: Jonathan Nieder jrnie...@gmail.com
---
builtin/clone.c | 6 +-
refs.c | 17 ++---
refs.h | 8 ++--
3 files changed, 17 insertions(+), 14 deletions(-)
diff --git a/builtin/clone.c b/builtin/clone.c
index d5e7532..b07d740 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -495,8 +495,10 @@ static struct ref *wanted_peer_refs(const struct ref *refs,
static void write_remote_refs(const struct ref *local_refs)
{
const struct ref *r;
+ struct strbuf err = STRBUF_INIT;
- lock_packed_refs(LOCK_DIE_ON_ERROR);
+ if (lock_packed_refs(err))
+ die(%s, err.buf);
for (r = local_refs; r; r = r-next) {
if (!r-peer_ref)
@@ -506,6 +508,8 @@ static void write_remote_refs(const struct ref *local_refs)
if (commit_packed_refs())
die_errno(unable to overwrite old ref-pack file);
+
+ strbuf_release(err);
}
static void write_followtags(const struct ref *refs, const char *msg)
diff --git a/refs.c b/refs.c
index 5ff457e..917f8fc 100644
--- a/refs.c
+++ b/refs.c
@@ -2371,13 +2371,15 @@ static int write_packed_entry_fn(struct ref_entry
*entry, void *cb_data)
return 0;
}
-/* This should return a meaningful errno on failure */
-int lock_packed_refs(int flags)
+int lock_packed_refs(struct strbuf *err)
{
struct packed_ref_cache *packed_ref_cache;
- if (hold_lock_file_for_update(packlock, git_path(packed-refs),
flags) 0)
+ if (hold_lock_file_for_update(packlock, git_path(packed-refs), 0)
0) {
+ unable_to_lock_message(git_path(packed-refs), errno, err);
return -1;
+ }
+
/*
* Get the current packed-refs while holding the lock. If the
* packed-refs file has been modified since we last read it,
@@ -2565,11 +2567,13 @@ static void prune_refs(struct ref_to_prune *r)
int pack_refs(unsigned int flags)
{
struct pack_refs_cb_data cbdata;
+ struct strbuf err = STRBUF_INIT;
memset(cbdata, 0, sizeof(cbdata));
cbdata.flags = flags;
- lock_packed_refs(LOCK_DIE_ON_ERROR);
+ if (lock_packed_refs(err))
+ die(%s, err.buf);
cbdata.packed_refs = get_packed_refs(ref_cache);
do_for_each_entry_in_dir(get_loose_refs(ref_cache), 0,
@@ -2579,6 +2583,7 @@ int pack_refs(unsigned int flags)
die_errno(unable to overwrite old ref-pack file);
prune_refs(cbdata.ref_to_prune);
+ strbuf_release(err);
return 0;
}
@@ -2657,10 +2662,8 @@ int repack_without_refs(const char **refnames, int n,
struct strbuf *err)
if (i == n)
return 0; /* no refname exists in packed refs */
- if (lock_packed_refs(0)) {
- unable_to_lock_message(git_path(packed-refs), errno, err);
+ if (lock_packed_refs(err))
return -1;
- }
packed = get_packed_refs(ref_cache);
/* Remove refnames from the cache */
diff --git a/refs.h b/refs.h
index 2bc3556..ca6a567 100644
--- a/refs.h
+++ b/refs.h
@@ -119,12 +119,8 @@ extern int for_each_rawref(each_ref_fn, void *);
extern void warn_dangling_symref(FILE *fp, const char *msg_fmt, const char
*refname);
extern void warn_dangling_symrefs(FILE *fp, const char *msg_fmt, const struct
string_list *refnames);
-/*
- * Lock the packed-refs file for writing. Flags is passed to
- * hold_lock_file_for_update(). Return 0 on success.
- * Errno is set to something meaningful on error.
- */
-extern int lock_packed_refs(int flags);
+/* Lock the packed-refs file for writing. Returns 0 on success. */
+extern int lock_packed_refs(struct strbuf *err);
/*
* Add a reference to the in-memory packed reference cache. This may
--
2.2.0.rc0.207.ga3a616c
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 06/14] lockfile: introduce flag for locks outside .git
When the lockfile API was introduced, it was only used for the index
file and error messages like
fatal: Unable to create '/path/to/foo.lock': File exists.
If no other git process is currently running, this probably means a
git process crashed in this repository earlier. Make sure no other git
process is running and remove the file manually to continue.
were appropriate advice for that case.
Nowadays, the lockfile API is used for other files that need similar
lock-then-update semantics, including files not associated to any
repository, such as /etc/gitconfig, ../my.bundle, and /tmp/temp.marks.
Add a flag that makes the message stop referring to this repository
for such cases.
This should make it possible to print nicer error messages from
such non-core users of the lockfile API.
This introduces the flag. Later patches will use it.
Signed-off-by: Jonathan Nieder jrnie...@gmail.com
---
Documentation/technical/api-lockfile.txt | 12
builtin/update-index.c | 2 +-
lockfile.c | 26 +-
lockfile.h | 5 +++--
refs.c | 4 ++--
5 files changed, 35 insertions(+), 14 deletions(-)
diff --git a/Documentation/technical/api-lockfile.txt
b/Documentation/technical/api-lockfile.txt
index 93b5f23..fa60cfd 100644
--- a/Documentation/technical/api-lockfile.txt
+++ b/Documentation/technical/api-lockfile.txt
@@ -124,6 +124,18 @@ LOCK_NO_DEREF::
for backwards-compatibility reasons can be a symbolic link
containing the name of the referred-to-reference.
+LOCK_OUTSIDE_REPOSITORY:
+
+ When the .lock file being created already exists, the error
+ message usually explains that another git process must have
+ crashed in the same Git repository. If `LOCK_OUTSIDE_REPOSITORY`
+ is set, then the message is replaced with something more
+ appropriate when updating files that are not inside a
+ repository.
++
+For example, this flag should be set when locking a configuration
+file in the user's home directory.
+
LOCK_DIE_ON_ERROR::
If a lock is already taken for the file, `die()` with an error
diff --git a/builtin/update-index.c b/builtin/update-index.c
index b0e3dc9..56abd18 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -943,7 +943,7 @@ int cmd_update_index(int argc, const char **argv, const
char *prefix)
if (newfd 0) {
if (refresh_args.flags REFRESH_QUIET)
exit(128);
- unable_to_lock_die(get_index_file(), lock_error);
+ unable_to_lock_die(get_index_file(), 0, lock_error);
}
if (write_locked_index(the_index, lock_file, COMMIT_LOCK))
die(Unable to write new index file);
diff --git a/lockfile.c b/lockfile.c
index 8685c68..102584f 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -149,24 +149,32 @@ static int lock_file(struct lock_file *lk, const char
*path, int flags)
return lk-fd;
}
-void unable_to_lock_message(const char *path, int err, struct strbuf *buf)
+void unable_to_lock_message(const char *path, int flags, int err,
+ struct strbuf *buf)
{
- if (err == EEXIST) {
+ if (err != EEXIST) {
+ strbuf_addf(buf, Unable to create '%s.lock': %s,
+ absolute_path(path), strerror(err));
+ } else if (flags LOCK_OUTSIDE_REPOSITORY) {
+ strbuf_addf(buf, Unable to create '%s.lock': %s.\n\n
+ If no other git process is currently running, this
probably means\n
+ another git process crashed earlier. Make sure no other
git process\n
+ is running and remove the file manually to continue.,
+ absolute_path(path), strerror(err));
+ } else {
strbuf_addf(buf, Unable to create '%s.lock': %s.\n\n
If no other git process is currently running, this
probably means a\n
git process crashed in this repository earlier. Make sure
no other git\n
process is running and remove the file manually to
continue.,
absolute_path(path), strerror(err));
- } else
- strbuf_addf(buf, Unable to create '%s.lock': %s,
- absolute_path(path), strerror(err));
+ }
}
-NORETURN void unable_to_lock_die(const char *path, int err)
+NORETURN void unable_to_lock_die(const char *path, int flags, int err)
{
struct strbuf buf = STRBUF_INIT;
- unable_to_lock_message(path, err, buf);
+ unable_to_lock_message(path, flags, err, buf);
die(%s, buf.buf);
}
@@ -175,7 +183,7 @@ int hold_lock_file_for_update(struct lock_file *lk, const
char *path, int flags)
{
int fd = lock_file(lk, path,
[PATCH 07/14] fast-import: use message from lockfile API when writing marks fails
The usual way to handle errors from hold_lock_file_for_update is to
get a message from unable_to_lock_message or unable_to_lock_die, which
can explain to the user how to check for other git processes running
concurrently and unlink the .lock file if safe.
fast-import didn't use the unable_to_lock_message helper because at
the time it started using the lockfile API (v1.5.1-rc1~69^2~2,
2007-03-07) that helper didn't exist. Later it still was not
appropriate to use that helper because the message assumed the file
being locked was inside a git repository. Now there is a flag to
indicate that this lockfile is not part of the repository, so we can
finally use the helper.
Signed-off-by: Jonathan Nieder jrnie...@gmail.com
---
fast-import.c | 11 ---
1 file changed, 8 insertions(+), 3 deletions(-)
diff --git a/fast-import.c b/fast-import.c
index d0bd285..bf8faa9 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -1799,9 +1799,14 @@ static void dump_marks(void)
if (!export_marks_file)
return;
- if (hold_lock_file_for_update(mark_lock, export_marks_file, 0) 0) {
- failure |= error(Unable to write marks file %s: %s,
- export_marks_file, strerror(errno));
+ if (hold_lock_file_for_update(mark_lock, export_marks_file,
+ LOCK_OUTSIDE_REPOSITORY) 0) {
+ struct strbuf err = STRBUF_INIT;
+
+ unable_to_lock_message(export_marks_file,
+ LOCK_OUTSIDE_REPOSITORY, errno, err);
+ failure |= error(%s, err.buf);
+ strbuf_release(err);
return;
}
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 08/14] credentials: use message from lockfile API when locking ~/.git-credentials fails
If writing $HOME/.git-credentials.lock (or locking another file
specified with the --file option) fails due to the lock file already
existing, chances are that it is because another git process already
locked the file. Use the message from the lockfile API that says so,
to help the user to look for running git processes and remove the
stray .lock file when it is safe.
Signed-off-by: Jonathan Nieder jrnie...@gmail.com
---
credential-store.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/credential-store.c b/credential-store.c
index d435514..cd71156 100644
--- a/credential-store.c
+++ b/credential-store.c
@@ -55,8 +55,8 @@ static void print_line(struct strbuf *buf)
static void rewrite_credential_file(const char *fn, struct credential *c,
struct strbuf *extra)
{
- if (hold_lock_file_for_update(credential_lock, fn, 0) 0)
- die_errno(unable to get credential storage lock);
+ hold_lock_file_for_update(credential_lock, fn,
+ LOCK_DIE_ON_ERROR | LOCK_OUTSIDE_REPOSITORY);
if (extra)
print_line(extra);
parse_credential_file(fn, c, NULL, print_line);
--
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 09/14] config: use message from lockfile API when locking config file fails
The message from the lockfile API includes advice about how to remove
a stale lock file after a previous git process crashed.
Pass the LOCK_OUTSIDE_REPOSITORY flag to avoid confusing error
messages that imply the lockfile is under .git. These functions (and
'git config --file') are useful for arbitrary files in git config
format, including both files outside .git such as /etc/gitconfig and
$XDG_CONFIG_HOME/git/config and files under .git such as
$GIT_DIR/config.
Signed-off-by: Jonathan Nieder jrnie...@gmail.com
---
config.c | 20
1 file changed, 16 insertions(+), 4 deletions(-)
diff --git a/config.c b/config.c
index 15a2983..dacde5f 100644
--- a/config.c
+++ b/config.c
@@ -1940,9 +1940,15 @@ int git_config_set_multivar_in_file(const char
*config_filename,
* contents of .git/config will be written into it.
*/
lock = xcalloc(1, sizeof(struct lock_file));
- fd = hold_lock_file_for_update(lock, config_filename, 0);
+ fd = hold_lock_file_for_update(lock, config_filename,
+ LOCK_OUTSIDE_REPOSITORY);
if (fd 0) {
- error(could not lock config file %s: %s, config_filename,
strerror(errno));
+ struct strbuf err = STRBUF_INIT;
+
+ unable_to_lock_message(config_filename,
+ LOCK_OUTSIDE_REPOSITORY, errno, err);
+ error(%s, err.buf);
+ strbuf_release(err);
free(store.key);
ret = CONFIG_NO_LOCK;
goto out_free;
@@ -2211,9 +2217,15 @@ int git_config_rename_section_in_file(const char
*config_filename,
config_filename = filename_buf = git_pathdup(config);
lock = xcalloc(1, sizeof(struct lock_file));
- out_fd = hold_lock_file_for_update(lock, config_filename, 0);
+ out_fd = hold_lock_file_for_update(lock, config_filename,
+ LOCK_OUTSIDE_REPOSITORY);
if (out_fd 0) {
- ret = error(could not lock config file %s, config_filename);
+ struct strbuf err = STRBUF_INIT;
+
+ unable_to_lock_message(config_filename,
+ LOCK_OUTSIDE_REPOSITORY, errno, err);
+ ret = error(%s, err.buf);
+ strbuf_release(err);
goto out;
}
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 10/14] rerere: error out on autoupdate failure
We have been silently tolerating errors by returning early with an
error that the caller ignores since rerere.autoupdate was introduced
in v1.6.0-rc0~120^2 (2008-06-22). So on error (for example if the
index is already locked), rerere can return success silently without
updating the index or with only some items in the index updated.
Better to treat such failures as a fatal error so the operator can
figure out what is wrong and fix it.
Signed-off-by: Jonathan Nieder jrnie...@gmail.com
---
Also sent separately at
http://thread.gmane.org/gmane.comp.version-control.git/260623
rerere.c | 14 +-
1 file changed, 5 insertions(+), 9 deletions(-)
diff --git a/rerere.c b/rerere.c
index 1b0555f..195f663 100644
--- a/rerere.c
+++ b/rerere.c
@@ -477,27 +477,23 @@ out:
static struct lock_file index_lock;
-static int update_paths(struct string_list *update)
+static void update_paths(struct string_list *update)
{
int i;
- int fd = hold_locked_index(index_lock, 0);
- int status = 0;
- if (fd 0)
- return -1;
+ hold_locked_index(index_lock, 1);
for (i = 0; i update-nr; i++) {
struct string_list_item *item = update-items[i];
if (add_file_to_cache(item-string, ADD_CACHE_IGNORE_ERRORS))
- status = -1;
+ die(staging updated %s failed, item-string);
}
- if (!status active_cache_changed) {
+ if (active_cache_changed) {
if (write_locked_index(the_index, index_lock, COMMIT_LOCK))
die(Unable to write new index file);
- } else if (fd = 0)
+ } else
rollback_lock_file(index_lock);
- return status;
}
static int do_plain_rerere(struct string_list *rr, int fd)
--
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 11/14] hold_locked_index: pass error message back through a strbuf
Today hold_locked_index takes a boolean parameter indicating whether
to die with a message or to return -1 with errno indicating the nature
of the problem on error.
Pass back an error message through an 'err' parameter instead. This
method of error reporting was introduced in the ref transaction API
and makes it more obvious when callers are not reporting errors (for
example, this helped catch rerere.autoupdate skipping the autoupdate
when unable to lock the index).
It also makes it easier for callers to clean up before exiting instead
of having to die() right away and paves the way for simplifying
hold_lock_file_for_update error handling in a later patch.
Signed-off-by: Jonathan Nieder jrnie...@gmail.com
---
builtin/add.c| 12 ---
builtin/apply.c | 10 +++--
builtin/checkout-index.c | 10 +++--
builtin/checkout.c | 55 ++--
builtin/clone.c | 6 +-
builtin/commit.c | 27 ++--
builtin/describe.c | 11 +++---
builtin/diff.c | 12 ---
builtin/merge.c | 14 +---
builtin/mv.c | 5 -
builtin/read-tree.c | 9 ++--
builtin/reset.c | 10 +++--
builtin/rm.c | 9 ++--
builtin/update-index.c | 10 -
cache-tree.c | 18
cache.h | 2 +-
merge-recursive.c| 13 +---
merge.c | 17 +++
read-cache.c | 14 +++-
rerere.c | 5 -
sequencer.c | 14 ++--
test-scrap-cache-tree.c | 5 -
22 files changed, 214 insertions(+), 74 deletions(-)
diff --git a/builtin/add.c b/builtin/add.c
index ae6d3e2..e912d77 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -299,6 +299,7 @@ static int add_files(struct dir_struct *dir, int flags)
int cmd_add(int argc, const char **argv, const char *prefix)
{
int exit_status = 0;
+ struct strbuf err = STRBUF_INIT;
struct pathspec pathspec;
struct dir_struct dir;
int flags;
@@ -315,8 +316,10 @@ int cmd_add(int argc, const char **argv, const char
*prefix)
if (add_interactive)
exit(interactive_add(argc - 1, argv + 1, prefix,
patch_interactive));
- if (edit_interactive)
- return(edit_patch(argc, argv, prefix));
+ if (edit_interactive) {
+ strbuf_release(err);
+ return edit_patch(argc, argv, prefix);
+ }
argc--;
argv++;
@@ -344,7 +347,8 @@ int cmd_add(int argc, const char **argv, const char *prefix)
add_new_files = !take_worktree_changes !refresh_only;
require_pathspec = !take_worktree_changes;
- hold_locked_index(lock_file, 1);
+ if (hold_locked_index(lock_file, err) 0)
+ die(%s, err.buf);
flags = ((verbose ? ADD_CACHE_VERBOSE : 0) |
(show_only ? ADD_CACHE_PRETEND : 0) |
@@ -356,6 +360,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
if (require_pathspec argc == 0) {
fprintf(stderr, _(Nothing specified, nothing added.\n));
fprintf(stderr, _(Maybe you wanted to say 'git add .'?\n));
+ strbuf_release(err);
return 0;
}
@@ -446,5 +451,6 @@ finish:
die(_(Unable to write new index file));
}
+ strbuf_release(err);
return exit_status;
}
diff --git a/builtin/apply.c b/builtin/apply.c
index 6696ea4..cda438f 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -4200,6 +4200,7 @@ static int apply_patch(int fd, const char *filename, int
options)
{
size_t offset;
struct strbuf buf = STRBUF_INIT; /* owns the patch text */
+ struct strbuf err = STRBUF_INIT;
struct patch *list = NULL, **listp = list;
int skipped_patch = 0;
@@ -4237,8 +4238,11 @@ static int apply_patch(int fd, const char *filename, int
options)
apply = 0;
update_index = check_index apply;
- if (update_index newfd 0)
- newfd = hold_locked_index(lock_file, 1);
+ if (update_index newfd 0) {
+ newfd = hold_locked_index(lock_file, err);
+ if (newfd 0)
+ die(%s, err.buf);
+ }
if (check_index) {
if (read_cache() 0)
@@ -4254,6 +4258,7 @@ static int apply_patch(int fd, const char *filename, int
options)
if (apply_with_reject)
exit(1);
/* with --3way, we still need to write the index out */
+ strbuf_release(err);
return 1;
}
@@ -4272,6 +4277,7 @@ static int apply_patch(int fd, const char *filename, int
options)
free_patch_list(list);
strbuf_release(buf);
string_list_clear(fn_table, 0);
+
[PATCH 12/14] hold_lock_file_for_update: pass error message back through a strbuf
This makes it more obvious when callers forget to print a message on
error, while still giving callers a chance to clean up before exiting.
Signed-off-by: Jonathan Nieder jrnie...@gmail.com
---
Documentation/technical/api-lockfile.txt | 33 +++-
builtin/apply.c | 5 -
builtin/commit.c | 9 +
builtin/gc.c | 8 ++--
bundle.c | 10 +++---
config.c | 18 ++---
credential-store.c | 8 ++--
fast-import.c| 8 +++-
lockfile.c | 26 ++---
lockfile.h | 8
read-cache.c | 9 +
refs.c | 17 +---
rerere.c | 7 +--
sequencer.c | 19 ++
shallow.c| 16
15 files changed, 101 insertions(+), 100 deletions(-)
diff --git a/Documentation/technical/api-lockfile.txt
b/Documentation/technical/api-lockfile.txt
index fa60cfd..8cb6704 100644
--- a/Documentation/technical/api-lockfile.txt
+++ b/Documentation/technical/api-lockfile.txt
@@ -87,21 +87,8 @@ Error handling
--
The `hold_lock_file_*` functions return a file descriptor on success
-or -1 on failure (unless `LOCK_DIE_ON_ERROR` is used; see below). On
-errors, `errno` describes the reason for failure. Errors can be
-reported by passing `errno` to one of the following helper functions:
-
-unable_to_lock_message::
-
- Append an appropriate error message to a `strbuf`.
-
-unable_to_lock_error::
-
- Emit an appropriate error message using `error()`.
-
-unable_to_lock_die::
-
- Emit an appropriate error message and `die()`.
+or -1 on failure. On errors, a message describing the reason for
+failure is appended to the strbuf specified using the 'err' argument.
Similarly, `commit_lock_file`, `commit_lock_file_to`, and
`close_lock_file` return 0 on success. On failure they set `errno`
@@ -111,7 +98,7 @@ appropriately, do their best to roll back the lockfile, and
return -1.
Flags
-
-The following flags can be passed to `hold_lock_file_for_update` or
+The following flag can be passed to `hold_lock_file_for_update` or
`hold_lock_file_for_append`:
LOCK_NO_DEREF::
@@ -136,22 +123,16 @@ LOCK_OUTSIDE_REPOSITORY:
For example, this flag should be set when locking a configuration
file in the user's home directory.
-LOCK_DIE_ON_ERROR::
-
- If a lock is already taken for the file, `die()` with an error
- message. If this option is not specified, trying to lock a
- file that is already locked returns -1 to the caller.
-
-
The functions
-
hold_lock_file_for_update::
Take a pointer to `struct lock_file`, the path of the file to
- be locked (e.g. `$GIT_DIR/index`) and a flags argument (see
- above). Attempt to create a lockfile for the destination and
- return the file descriptor for writing to the file.
+ be locked (e.g. `$GIT_DIR/index`), a flags argument (see
+ above), and a strbuf for error messages. Attempt to create a
+ lockfile for the destination and return the file descriptor for
+ writing to the file.
hold_lock_file_for_append::
diff --git a/builtin/apply.c b/builtin/apply.c
index cda438f..07626fb 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -3711,6 +3711,7 @@ static void build_fake_ancestor(struct patch *list, const
char *filename)
struct patch *patch;
struct index_state result = { NULL };
static struct lock_file lock;
+ struct strbuf err = STRBUF_INIT;
/* Once we start supporting the reverse patch, it may be
* worth showing the new sha1 prefix, but until then...
@@ -3748,11 +3749,13 @@ static void build_fake_ancestor(struct patch *list,
const char *filename)
die (Could not add %s to temporary index, name);
}
- hold_lock_file_for_update(lock, filename, LOCK_DIE_ON_ERROR);
+ if (hold_lock_file_for_update(lock, filename, 0, err) 0)
+ die(%s, err.buf);
if (write_locked_index(result, lock, COMMIT_LOCK))
die (Could not write temporary index to %s, filename);
discard_index(result);
+ strbuf_release(err);
}
static void stat_patch_list(struct patch *patch)
diff --git a/builtin/commit.c b/builtin/commit.c
index edc4493..f64024c 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -468,10 +468,11 @@ static const char *prepare_index(int argc, const char
**argv, const char *prefix
if (write_locked_index(the_index, index_lock, CLOSE_LOCK))
die(_(unable to write new_index file));
-
[PATCH 13/14] lockfile: remove unused function 'unable_to_lock_die'
The old callers now use the message passed back by
hold_lock_file_for_update / hold_lock_file_for_append instead of
trying to interpret errno.
Signed-off-by: Jonathan Nieder jrnie...@gmail.com
---
lockfile.c | 8
lockfile.h | 1 -
2 files changed, 9 deletions(-)
diff --git a/lockfile.c b/lockfile.c
index a79679b..8d8d5ed 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -170,14 +170,6 @@ void unable_to_lock_message(const char *path, int flags,
int err,
}
}
-NORETURN void unable_to_lock_die(const char *path, int flags, int err)
-{
- struct strbuf buf = STRBUF_INIT;
-
- unable_to_lock_message(path, flags, err, buf);
- die(%s, buf.buf);
-}
-
int hold_lock_file_for_update(struct lock_file *lk, const char *path,
int flags, struct strbuf *err)
{
diff --git a/lockfile.h b/lockfile.h
index 6d0a9bb..b4d29a3 100644
--- a/lockfile.h
+++ b/lockfile.h
@@ -73,7 +73,6 @@ struct lock_file {
extern void unable_to_lock_message(const char *path, int, int err,
struct strbuf *buf);
-extern NORETURN void unable_to_lock_die(const char *path, int, int err);
extern int hold_lock_file_for_update(struct lock_file *, const char *path,
int, struct strbuf *err);
extern int hold_lock_file_for_append(struct lock_file *, const char *path,
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 14/14] lockfile: make 'unable_to_lock_message' private
The old external callers now use the message passed back by
hold_lock_file_for_update / hold_lock_file_for_append instead of
trying to interpret errno.
Signed-off-by: Jonathan Nieder jrnie...@gmail.com
---
That's the end of the series. Thanks for reading.
Thoughts?
lockfile.c | 42 +-
lockfile.h | 2 --
2 files changed, 21 insertions(+), 23 deletions(-)
diff --git a/lockfile.c b/lockfile.c
index 8d8d5ed..7121370 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -98,6 +98,27 @@ static void resolve_symlink(struct strbuf *path)
strbuf_reset(link);
}
+static void unable_to_lock_message(const char *path, int flags, int err,
+ struct strbuf *buf)
+{
+ if (err != EEXIST) {
+ strbuf_addf(buf, Unable to create '%s.lock': %s,
+ absolute_path(path), strerror(err));
+ } else if (flags LOCK_OUTSIDE_REPOSITORY) {
+ strbuf_addf(buf, Unable to create '%s.lock': %s.\n\n
+ If no other git process is currently running, this
probably means\n
+ another git process crashed earlier. Make sure no other
git process\n
+ is running and remove the file manually to continue.,
+ absolute_path(path), strerror(err));
+ } else {
+ strbuf_addf(buf, Unable to create '%s.lock': %s.\n\n
+ If no other git process is currently running, this
probably means a\n
+ git process crashed in this repository earlier. Make sure
no other git\n
+ process is running and remove the file manually to
continue.,
+ absolute_path(path), strerror(err));
+ }
+}
+
static int lock_file(struct lock_file *lk, const char *path,
int flags, struct strbuf *err)
{
@@ -149,27 +170,6 @@ static int lock_file(struct lock_file *lk, const char
*path,
return lk-fd;
}
-void unable_to_lock_message(const char *path, int flags, int err,
- struct strbuf *buf)
-{
- if (err != EEXIST) {
- strbuf_addf(buf, Unable to create '%s.lock': %s,
- absolute_path(path), strerror(err));
- } else if (flags LOCK_OUTSIDE_REPOSITORY) {
- strbuf_addf(buf, Unable to create '%s.lock': %s.\n\n
- If no other git process is currently running, this
probably means\n
- another git process crashed earlier. Make sure no other
git process\n
- is running and remove the file manually to continue.,
- absolute_path(path), strerror(err));
- } else {
- strbuf_addf(buf, Unable to create '%s.lock': %s.\n\n
- If no other git process is currently running, this
probably means a\n
- git process crashed in this repository earlier. Make sure
no other git\n
- process is running and remove the file manually to
continue.,
- absolute_path(path), strerror(err));
- }
-}
-
int hold_lock_file_for_update(struct lock_file *lk, const char *path,
int flags, struct strbuf *err)
{
diff --git a/lockfile.h b/lockfile.h
index b4d29a3..02e26fe 100644
--- a/lockfile.h
+++ b/lockfile.h
@@ -71,8 +71,6 @@ struct lock_file {
#define LOCK_NO_DEREF 1
#define LOCK_OUTSIDE_REPOSITORY 2
-extern void unable_to_lock_message(const char *path, int, int err,
- struct strbuf *buf);
extern int hold_lock_file_for_update(struct lock_file *, const char *path,
int, struct strbuf *err);
extern int hold_lock_file_for_append(struct lock_file *, const char *path,
--
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 04/14] hold_lock_file_for_append: pass error message back through a strbuf
On 12/03/2014 06:14 AM, Jonathan Nieder wrote:
This way, the code does not need to carefully safeguard errno to allow
callers to print a reasonable error message when they choose to do
some cleanup before die()-ing.
Fixes a bug waiting to happen where copy_fd would clobber the errno
passed back via hold_lock_file_for_append from read() or write() when
flags did not contain LOCK_DIE_ON_ERROR. Luckily the only caller uses
flags == LOCK_DIE_ON_ERROR, avoiding that bug in practice.
Reported-by: Ronnie Sahlberg sahlb...@google.com
Signed-off-by: Jonathan Nieder jrnie...@gmail.com
---
The advertised bugfix.
lockfile.c | 29 ++---
lockfile.h | 3 ++-
sha1_file.c | 7 ++-
3 files changed, 18 insertions(+), 21 deletions(-)
diff --git a/lockfile.c b/lockfile.c
index b3020f3..8685c68 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -179,45 +179,36 @@ int hold_lock_file_for_update(struct lock_file *lk, const
char *path, int flags)
return fd;
}
-int hold_lock_file_for_append(struct lock_file *lk, const char *path, int flags)
+int hold_lock_file_for_append(struct lock_file *lk, const char *path,
+ int flags, struct strbuf *err)
{
int fd, orig_fd;
- struct strbuf err = STRBUF_INIT;
+
+ assert(!(flags LOCK_DIE_ON_ERROR));
+ assert(err !err-len);
What do I miss ?
In the old code we die() printing the errno when LOCK_DIE_ON_ERRORis set.
Now we die() in an assert() or two ?
And should'nt we assert in strbuf_addf() instead ?
And in general, does the commit message deserve an update?
Luckily the only caller uses flags == LOCK_DIE_ON_ERROR,
The first impression was: Why do we not simplify the code and remove
the flag completely ?
This feels somewhat like maintaining dead code, which may be used later.
(Unless it will be used in later commit, and the we could mention it here)
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 04/14] hold_lock_file_for_append: pass error message back through a strbuf
Torsten Bögershausen wrote:
On 12/03/2014 06:14 AM, Jonathan Nieder wrote:
--- a/lockfile.c
+++ b/lockfile.c
@@ -179,45 +179,36 @@ int hold_lock_file_for_update(struct lock_file *lk,
const char *path, int flags)
return fd;
}
-int hold_lock_file_for_append(struct lock_file *lk, const char *path, int
flags)
+int hold_lock_file_for_append(struct lock_file *lk, const char *path,
+ int flags, struct strbuf *err)
{
int fd, orig_fd;
-struct strbuf err = STRBUF_INIT;
+
+assert(!(flags LOCK_DIE_ON_ERROR));
+assert(err !err-len);
What do I miss ?
In the old code we die() printing the errno when LOCK_DIE_ON_ERRORis set.
The assertion documents an assumption that no caller will set the
LOCK_DIE_ON_ERROR flag bit. A later patch in the series eliminates
that flag bit completely.
Now we die() in an assert() or two ?
assert() is not safe to use for anything other than documenting
assumptions. It can't be relied on to exit (let alone to exit with a
nice message like die()). So the die() that was removed and assert()
that this patch adds have different purposes.
And should'nt we assert in strbuf_addf() instead ?
strbuf_addf can be used to append to a nonempty strbuf.
[...]
(Unless it will be used in later commit, and the we could mention it here)
Good idea. I'll add to the commit message if rerolling.
Thanks,
Jonathan
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
