* Re: [PATCH 6/9] t3510 (cherry-pick-sequencer): remove malformed sheet 2
From: Ramkumar Ramachandra @ 2011-12-09 20:30 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: Git List, Junio C Hamano
In-Reply-To: <20111209202449.GI20913@elie.hsd1.il.comcast.net>
Jonathan Nieder wrote:
> Ramkumar Ramachandra wrote:
>
>> By removing the "malformed instruction sheet
>> 2" test in advance, it'll be easier to see the changes made by the
>> next patch.
>
> So, this is a regression in test coverage without a redeeming upside
> other than allowing the next patch to be prettier. Naturally, I don't
> like it.
Without this patch, the diffs of _all_ the future commits in this
series touching this file are totally unreadable. I've noticed that
the diffing algorithm performs especially badly for t/*.sh -- rebasing
tests is generally a huge pain.
-- Ram
^ permalink raw reply
* Re: [PATCH 0/9] Re-roll rr/revert-cherry-pick v2
From: Jonathan Nieder @ 2011-12-09 20:34 UTC (permalink / raw)
To: Ramkumar Ramachandra; +Cc: Git List, Junio C Hamano
In-Reply-To: <1323445326-24637-1-git-send-email-artagnon@gmail.com>
Ramkumar Ramachandra wrote:
> - "revert: report fine-grained error messages from insn parser" arises
> from Jonathan's request to split "revert: allow mixed pick and
> revert instructions".
Just to be clear: I wasn't directly requesting that you do anything.
If I were, then you could carefully read my requirements, fulfill
them, and you would be done.
Instead, I was reviewing the patch and giving my reaction. After
receiving that information, one has plenty of choices:
- add documentation to avoid the confusion the reaction was based on
- rework to fix the underlying problem that caused the reaction
- think carefully about it, conclude that the reviewer is crazy, and
ignore it
- drop the patch
- send out a call for help to get others to help work on the
underlying problem
- ask for clarification
...
>From my point of view as a reviewer, I am happiest when someone
figures out how I missed the point and comes up with some fix that
addresses the underlying problem instead (and, incidentally, gets rid
of the symptom that provoked my reaction on the way).
Well, you get the idea.
^ permalink raw reply
* Re: [PATCH 5/9] t3510 (cherry-pick-sequencer): use exit status
From: Ramkumar Ramachandra @ 2011-12-09 20:36 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: Git List, Junio C Hamano
In-Reply-To: <20111209202149.GH20913@elie.hsd1.il.comcast.net>
Jonathan Nieder wrote:
> Ramkumar Ramachandra wrote:
>
> [...]
>> --- a/t/t3510-cherry-pick-sequence.sh
>> +++ b/t/t3510-cherry-pick-sequence.sh
> [...]
>> @@ -53,7 +53,7 @@ test_expect_success 'cherry-pick persists data on failure' '
>>
>> test_expect_success 'cherry-pick persists opts correctly' '
>> pristine_detach initial &&
>> - test_must_fail git cherry-pick -s -m 1 --strategy=recursive -X patience -X ours base..anotherpick &&
>> + test_expect_code 128 git cherry-pick -s -m 1 --strategy=recursive -X patience -X ours base..anotherpick &&
>> test_path_is_dir .git/sequencer &&
>
> Encountered conflicts, preserving options, but the exit is with status
> 128? Smells like a bug.
No bug. Notice that "-m 1" is used when "initial" isn't a merge
commit. But yeah, I should probably clarify this by changing the
revision range to "initial..anotherpick" so as not to distract the
user.
-- Ram
^ permalink raw reply
* Re: [PATCH 6/9] t3510 (cherry-pick-sequencer): remove malformed sheet 2
From: Jonathan Nieder @ 2011-12-09 20:37 UTC (permalink / raw)
To: Ramkumar Ramachandra; +Cc: Git List, Junio C Hamano
In-Reply-To: <CALkWK0mEP5nDgdosOiquQ_FWbNRZesi38NeCD_yGPvJ8JQxkGg@mail.gmail.com>
Ramkumar Ramachandra wrote:
> I've noticed that
> the diffing algorithm performs especially badly for t/*.sh -- rebasing
> tests is generally a huge pain.
No clue about this particular situation, but I suspect the general
cause for such rebasing trouble is adding tests at the end of the file
(or some other contended place). Better to figure out a logical place
for each test and put it there from the start.
^ permalink raw reply
* Re: [PATCH 8/9] revert: report fine-grained error messages from insn parser
From: Jonathan Nieder @ 2011-12-09 20:47 UTC (permalink / raw)
To: Ramkumar Ramachandra; +Cc: Git List, Junio C Hamano
In-Reply-To: <1323445326-24637-9-git-send-email-artagnon@gmail.com>
Ramkumar Ramachandra wrote:
> --- a/builtin/revert.c
> +++ b/builtin/revert.c
> @@ -719,8 +719,10 @@ static int format_todo(struct strbuf *buf, struct replay_insn_list *todo_list)
> return 0;
> }
>
> -static int parse_insn_line(char *bol, char *eol, struct replay_insn_list *item)
> +static int parse_insn_line(char *bol, char *eol,
> + struct replay_insn_list *item, int lineno)
> {
> + const char *todo_file = git_path(SEQ_TODO_FILE);
> unsigned char commit_sha1[20];
I know that this function does not call git_path() again before the
value is used, so this is safe today, but I do not trust people in the
future to preserve that property (for example, maybe someone will want
to call get_sha1() earlier). Why not wait to call git_path() when it
is time to use the value it returns?
^ permalink raw reply
* Re: [PATCH 4/9] revert: simplify getting commit subject in format_todo()
From: Ramkumar Ramachandra @ 2011-12-09 20:58 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: Git List, Junio C Hamano
In-Reply-To: <20111209194727.GF20913@elie.hsd1.il.comcast.net>
Hi,
Jonathan Nieder wrote:
> Ramkumar Ramachandra wrote:
> [...]
> Can cur->item->buffer be NULL?
As Junio correctly points out in $gmane/186365, no. To quote him:
parse_insn_line() uses lookup_commit_reference() which
calls parse_object() on the object name (and if it is a tag, deref_tag()
will parse the tagged object until we see something that is not a tag), so
we know cur->item is parsed before we see it
>> Also, remove the unnecessary check on cur->item: the previous patch
>> makes sure that instruction sheets with missing commit subjects are
>> parsable.
>
> But now I am confused
> [...]
This part of the commit message is simply awful. Replaced with:
Also remove the unnecessary check on cur->item->buffer:
the lookup_commit_reference() call in parse_insn_line() has already
made sure of this.
-- Ram
^ permalink raw reply
* Re: [PATCH 5/9] t3510 (cherry-pick-sequencer): use exit status
From: Jonathan Nieder @ 2011-12-09 21:00 UTC (permalink / raw)
To: Ramkumar Ramachandra; +Cc: Git List, Junio C Hamano
In-Reply-To: <CALkWK0=a=-4N4aZHuu=5zkNtwmfLsog5WkBVbuJAbYpvaLUsAg@mail.gmail.com>
Ramkumar Ramachandra wrote:
> Jonathan Nieder wrote:
>> Ramkumar Ramachandra wrote:
>>> - test_must_fail git cherry-pick -s -m 1 --strategy=recursive -X patience -X ours base..anotherpick &&
>>> + test_expect_code 128 git cherry-pick -s -m 1 --strategy=recursive -X patience -X ours base..anotherpick &&
>>> test_path_is_dir .git/sequencer &&
>>
>> Encountered conflicts, preserving options, but the exit is with status
>> 128? Smells like a bug.
>
> No bug.
Ok. I'm fuzzy on the details, but is it possible to make this change
in such a way as to make that obvious? For example, perhaps this
should be split into several tests: one to check that such mistaken
use of "-m 1" with non-merge commits correctly interrupts the
cherry-pick and pleads to the user for advice (should it?), another to
check that doing so produces an exit status of 128 (if it should), and
another to make sure that doing so, fixing things up somehow, and
resuming the sequence allows the effect of "-m 1" to carry over to
later commits.
^ permalink raw reply
* Re: [PATCH 9/9] revert: simplify communicating command-line arguments
From: Jonathan Nieder @ 2011-12-09 21:09 UTC (permalink / raw)
To: Ramkumar Ramachandra; +Cc: Git List, Junio C Hamano
In-Reply-To: <CALkWK0m2veE8FmFVTPEqNAmbtvm1sWVHtFt0QOWU=huQFafeBw@mail.gmail.com>
Ramkumar Ramachandra wrote:
> a
> positive exit status can be interpreted as a conflict, but this is
> clearly not the case here. How do we fix this problem? By creating
> an API for "git commit", not by shelling out like this and letting it
> take over the exit status.
That might be a nice thing to do anyway, but I don't see how it would
solve anything. The new "git commit" API would presumably return an
integer or enum value to indicate the result of trying to commit.
Tests in the testsuite for the "git commit" API would use the "git
commit" command, which would expose the newly fine-grained values
somehow. And other people scripting but wanting the porcelain to take
care of basic UI would benefit, too. Right?
Actually, I think cherry-pick returning a positive exit status for
"nothing left to commit after resolving conflicts" would be sensible.
It is "I did what you asked but need your help to determine the final
content of the commit or decide to skip it", rather than "you asked
for something unsensible and I am bailing out".
^ permalink raw reply
* Re: [PATCH 1/2] index_pack: indent find_unresolved_detals one level
From: Junio C Hamano @ 2011-12-09 21:27 UTC (permalink / raw)
To: pclouds; +Cc: git, Shawn Pearce
In-Reply-To: <4ee0be67.05c1e70a.1956.ffff800b@mx.google.com>
pclouds@gmail.com writes:
> From: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
>
> The next patch puts most of the code in one level deeper. By indenting
> separately, it'd be easier to see the actual changes.
Yuck.
Isn't it a sign that "the next patch" should perhaps be helped by a small
helper function that does whatever the part you are indenting here?
^ permalink raw reply
* Re: [PATCH 1/2] run-command: Add checks after execvp fails with EACCES
From: Frans Klaver @ 2011-12-09 21:35 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7vaa71hd5l.fsf@alter.siamese.dyndns.org>
On Fri, 09 Dec 2011 18:23:50 +0100, Junio C Hamano <gitster@pobox.com>
wrote:
> "Frans Klaver" <fransklaver@gmail.com> writes:
>
>>> Wouldn't access(2) with R_OK|X_OK give you exactly what you want
>>> without
>>> this much trouble?
>>
>> I just had a good look through the man page of access(2), and I think
>> it depends. access works for the real uid, which is what I attempted
>> to implement in the above check as well. However, do we actually need
>> to use the real uid or do we need the set uid (geteuid(2))?
>
> Does it matter? We do not use seteuid or setegid ourselves and we do not
> expect to be installed as owned by root with u+s bit set.
That's what I thought, but needed to know for sure that this was the case.
> access(2) checks with real uid exactly because it would not make a
> difference to normal user level programs _and_ it makes it easier for a
> suid programs to check with the real identity, and our use case falls
> into the former, no?
>
Certainly looks like. Thanks. I'll reroll somewhere next week.
Frans
^ permalink raw reply
* Re: [PATCH 2/5] revert: make commit subjects in insn sheet optional
From: Ramkumar Ramachandra @ 2011-12-09 22:30 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: Junio C Hamano, Git List
In-Reply-To: <20111209195042.GG20913@elie.hsd1.il.comcast.net>
Hey,
Jonathan Nieder wrote:
> You can make git misbehave before applying the commit.
Right, it's all in the discussion surrounding $gmane/184031. I
couldn't recall because it was so long ago :\
Thanks.
-- Ram
^ permalink raw reply
* Re: [PATCH 0/9] Re-roll rr/revert-cherry-pick v2
From: Ramkumar Ramachandra @ 2011-12-09 23:03 UTC (permalink / raw)
To: Git List; +Cc: Junio C Hamano, Jonathan Nieder
In-Reply-To: <1323445326-24637-1-git-send-email-artagnon@gmail.com>
Hi,
I'm off on a short vacation next week. I've already fixed up the
series in response to most (if not all) reviews. If anyone's
interested in putting in some finishing touches, it's available here:
https://github.com/artagnon/git.git rr/revert-cherry-pick ;# 195e68e
Thanks.
-- Ram
^ permalink raw reply
* Re: [PATCHv2 0/13] credential helpers
From: Jeff King @ 2011-12-09 23:18 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7vzkf1fwvn.fsf@alter.siamese.dyndns.org>
On Fri, Dec 09, 2011 at 10:00:44AM -0800, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > It works, and it detects truncated output both ways properly (I know
> > because I had to update every test, since the old output was missing the
> > end-of-credential marker).
> >
> > It makes me a little sad, because the original format (relying on EOF)
> > was so Unix-y.
>
> It saddens me, too. A reasonable middle ground would be to stop treating
> an empty input as "no restriction" but "never matches".
>
> I suspect that it is far more likely for a helper to fail (due to
> configuration errors, for example) before it produces any output than
> after it gives some but not all output lines.
Yeah, I think that's a reasonable compromise. Instead of the patch I
posted earlier, how about this:
diff --git a/credential-store.c b/credential-store.c
index a2c2cd0..26f7589 100644
--- a/credential-store.c
+++ b/credential-store.c
@@ -96,7 +96,16 @@ static void store_credential(const char *fn, struct credential *c)
static void remove_credential(const char *fn, struct credential *c)
{
- rewrite_credential_file(fn, c, NULL);
+ /*
+ * Sanity check that we actually have something to match
+ * against. The input we get is a restrictive pattern,
+ * so technically a blank credential means "erase everything".
+ * But it is too easy to accidentally send this, since it is equivalent
+ * to empty input. So explicitly disallow it, and require that the
+ * pattern have some actual content to match.
+ */
+ if (c->protocol || c->host || c->path || c->username)
+ rewrite_credential_file(fn, c, NULL);
}
static int lookup_credential(const char *fn, struct credential *c)
We _could_ modify credential_match() to automatically reject such a
pattern at that level, but it does actually have a use on the lookup
side. In config, a context like "https://example.com/foo.git" would
match each of:
[credential "https://example.com/foo.git"]
helper = ...
[credential "https://example.com"]
helper = ...
[credential "https://"]
helper = ...
[credential]
helper = ...
The final one is an just an extension of the others to the empty pattern
(you could also spell it [credential ""], and it would have the same
effect).
So the "empty pattern" does actually have a use, from the end-users's
point of view. It's just that with removal, it's a little more dangerous
and a little less likely to be useful (as compared to lookup).
-Peff
^ permalink raw reply related
* Re: [PATCHv2 12/13] credentials: add "store" helper
From: Jeff King @ 2011-12-09 23:19 UTC (permalink / raw)
To: git
In-Reply-To: <20111206062305.GL29233@sigill.intra.peff.net>
On Tue, Dec 06, 2011 at 01:23:05AM -0500, Jeff King wrote:
> +int main(int argc, const char **argv)
> +{
> + const char * const usage[] = {
> + "git credential-store [options] <action>",
> + NULL
> + };
> + const char *op;
> + struct credential c = CREDENTIAL_INIT;
> + char *file = NULL;
> + struct option options[] = {
> + OPT_STRING_LIST(0, "file", &file, "path",
> + "fetch and store credentials in <path>"),
> + OPT_END()
> + };
Eek, this should be OPT_STRING, of course. I'll fix it in my next
re-roll, but I wanted to mention it in case anybody is reviewing v2.
-Peff
^ permalink raw reply
* Re: [PATCHv2 0/13] credential helpers
From: Junio C Hamano @ 2011-12-09 23:34 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, git
In-Reply-To: <20111209231800.GA14376@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> Yeah, I think that's a reasonable compromise. Instead of the patch I
> posted earlier, how about this:
>
> diff --git a/credential-store.c b/credential-store.c
> index a2c2cd0..26f7589 100644
> --- a/credential-store.c
> +++ b/credential-store.c
> @@ -96,7 +96,16 @@ static void store_credential(const char *fn, struct credential *c)
>
> static void remove_credential(const char *fn, struct credential *c)
> {
> - rewrite_credential_file(fn, c, NULL);
> + /*
> + * Sanity check that we actually have something to match
> + * against. The input we get is a restrictive pattern,
> + * so technically a blank credential means "erase everything".
> + * But it is too easy to accidentally send this, since it is equivalent
> + * to empty input. So explicitly disallow it, and require that the
> + * pattern have some actual content to match.
> + */
> + if (c->protocol || c->host || c->path || c->username)
> + rewrite_credential_file(fn, c, NULL);
> }
Looks very sane.
> We _could_ modify credential_match() to automatically reject such a
> pattern at that level,...
I do not think that is such a good idea to modify "match()" function
either, as I agree match with empty has its uses, but that does not stop
"rewrite_credential_file()" from being safe by default, no? After all, the
one that makes the decision to drop things that match the pattern is that
function (it chooses to give NULL to match_cb).
> So the "empty pattern" does actually have a use, from the end-users's
> point of view. It's just that with removal, it's a little more dangerous
> and a little less likely to be useful (as compared to lookup).
>
> -Peff
^ permalink raw reply
* Re: [PATCHv2 0/13] credential helpers
From: Jeff King @ 2011-12-09 23:39 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7vehwdcob3.fsf@alter.siamese.dyndns.org>
On Fri, Dec 09, 2011 at 03:34:08PM -0800, Junio C Hamano wrote:
> > We _could_ modify credential_match() to automatically reject such a
> > pattern at that level,...
>
> I do not think that is such a good idea to modify "match()" function
> either, as I agree match with empty has its uses, but that does not stop
> "rewrite_credential_file()" from being safe by default, no? After all, the
> one that makes the decision to drop things that match the pattern is that
> function (it chooses to give NULL to match_cb).
Yeah, you could move it down to that level, but there isn't much point.
rewrite_credential_file is unique to credential-store, and the only two
callers are store_credential (which has its own, stricter rules already)
and remove_credential, which we are modifying here.
Note that I didn't bother with the same safety valve for
credential-cache. It is, after all, a cache that will go away eventually
anyway, so safety is less interesting.
Third-party helpers will have to do their own checks anyway, as in
general I don't plan on them linking directly against git code.
Speaking of which, I hackishly ported Jay's osxkeychain helper to the
new format last night. I'll try to clean that up and post it tonight.
-Peff
^ permalink raw reply
* [PATCH 0/4] git-p4: paths for p4
From: Pete Wyckoff @ 2011-12-09 23:48 UTC (permalink / raw)
To: git; +Cc: Gary Gibbons, Junio C Hamano
There are two problems associated with how we set up paths for
use by p4, fixed in this series. Each fix is accompanied by
another patch that is the unit test.
There is a break in the sequence in the t98* patches here, but
that is on purpose to avoid collision with new tests from other
in-flight patches.
1-2:
in submit, create clientPath if it does not exist
3-4:
in clone, make sure p4 sees an absolute path
Gary Gibbons (2):
git-p4: ensure submit clientPath exists before chdir
git-p4: use absolute directory for PWD env var
Pete Wyckoff (2):
git-p4: submit test for auto-creating clientPath
git-p4: test for absolute PWD problem
contrib/fast-import/git-p4 | 9 ++++++-
t/t9807-submit.sh | 38 ++++++++++++++++++++++++++++++++++
t/t9808-chdir.sh | 49 ++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 94 insertions(+), 2 deletions(-)
create mode 100755 t/t9807-submit.sh
create mode 100755 t/t9808-chdir.sh
--
1.7.8.rc4.42.g8317d
^ permalink raw reply
* [PATCH 1/4] git-p4: ensure submit clientPath exists before chdir
From: Pete Wyckoff @ 2011-12-09 23:48 UTC (permalink / raw)
To: git; +Cc: Gary Gibbons, Junio C Hamano
In-Reply-To: <1323474497-14339-1-git-send-email-pw@padd.com>
From: Gary Gibbons <ggibbons@perforce.com>
Submitting patches back to p4 requires a p4 "client". This
is a mapping from server depot paths into a local directory.
The directory need not exist or be populated with files; only
the mapping on the server is required. When there is no
directory, make git-p4 automatically create it.
[ reword description --pw ]
Signed-off-by: Gary Gibbons <ggibbons@perforce.com>
Signed-off-by: Pete Wyckoff <pw@padd.com>
---
contrib/fast-import/git-p4 | 4 ++++
1 files changed, 4 insertions(+), 0 deletions(-)
diff --git a/contrib/fast-import/git-p4 b/contrib/fast-import/git-p4
index b975d67..99d3abe 100755
--- a/contrib/fast-import/git-p4
+++ b/contrib/fast-import/git-p4
@@ -1099,6 +1099,10 @@ class P4Submit(Command, P4UserMap):
print "Perforce checkout for depot path %s located at %s" % (self.depotPath, self.clientPath)
self.oldWorkingDirectory = os.getcwd()
+ # ensure the clientPath exists
+ if not os.path.exists(self.clientPath):
+ os.makedirs(self.clientPath)
+
chdir(self.clientPath)
print "Synchronizing p4 checkout..."
p4_sync("...")
--
1.7.8.rc4.42.g8317d
^ permalink raw reply related
* [PATCH 2/4] git-p4: submit test for auto-creating clientPath
From: Pete Wyckoff @ 2011-12-09 23:48 UTC (permalink / raw)
To: git; +Cc: Gary Gibbons, Junio C Hamano
In-Reply-To: <1323474497-14339-1-git-send-email-pw@padd.com>
Signed-off-by: Pete Wyckoff <pw@padd.com>
---
t/t9807-submit.sh | 38 ++++++++++++++++++++++++++++++++++++++
1 files changed, 38 insertions(+), 0 deletions(-)
create mode 100755 t/t9807-submit.sh
diff --git a/t/t9807-submit.sh b/t/t9807-submit.sh
new file mode 100755
index 0000000..85ab0d0
--- /dev/null
+++ b/t/t9807-submit.sh
@@ -0,0 +1,38 @@
+#!/bin/sh
+
+test_description='git-p4 submit'
+
+. ./lib-git-p4.sh
+
+test_expect_success 'start p4d' '
+ start_p4d
+'
+
+test_expect_success 'init depot' '
+ (
+ cd "$cli" &&
+ echo file1 >file1 &&
+ p4 add file1 &&
+ p4 submit -d "change 1"
+ )
+'
+
+test_expect_success 'submit with no client dir' '
+ test_when_finished cleanup_git &&
+ "$GITP4" clone --dest="$git" //depot &&
+ (
+ cd "$git" &&
+ echo file2 >file2 &&
+ git add file2 &&
+ git commit -m "git commit 2" &&
+ rm -rf "$cli" &&
+ git config git-p4.skipSubmitEdit true &&
+ "$GITP4" submit
+ )
+'
+
+test_expect_success 'kill p4d' '
+ kill_p4d
+'
+
+test_done
--
1.7.8.rc4.42.g8317d
^ permalink raw reply related
* [PATCH 3/4] git-p4: use absolute directory for PWD env var
From: Pete Wyckoff @ 2011-12-09 23:48 UTC (permalink / raw)
To: git; +Cc: Gary Gibbons, Junio C Hamano
In-Reply-To: <1323474497-14339-1-git-send-email-pw@padd.com>
From: Gary Gibbons <ggibbons@perforce.com>
P4 only looks at the environment variable $PWD to figure out
where it is, so chdir() has code to set that every time. But
when the clone --destination is not an absolute path, PWD will
not be absolute and P4 won't be able to find any files expected
to be in the current directory. Fix this by expanding PWD to
an absolute path.
One place this crops up is when using a P4CONFIG environment
variable to specify P4 parameters, such as P4USER or P4PORT.
Setting P4CONFIG=.p4config works for p4 invocations from the
current directory. But if the value of PWD is not absolute, it
fails.
[ update description --pw ]
Signed-off-by: Gary Gibbons <ggibbons@perforce.com>
Signed-off-by: Pete Wyckoff <pw@padd.com>
---
contrib/fast-import/git-p4 | 5 +++--
1 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/contrib/fast-import/git-p4 b/contrib/fast-import/git-p4
index 99d3abe..0083f86 100755
--- a/contrib/fast-import/git-p4
+++ b/contrib/fast-import/git-p4
@@ -53,9 +53,10 @@ def p4_build_cmd(cmd):
def chdir(dir):
# P4 uses the PWD environment variable rather than getcwd(). Since we're
- # not using the shell, we have to set it ourselves.
- os.environ['PWD']=dir
+ # not using the shell, we have to set it ourselves. This path could
+ # be relative, so go there first, then figure out where we ended up.
os.chdir(dir)
+ os.environ['PWD'] = os.getcwd()
def die(msg):
if verbose:
--
1.7.8.rc4.42.g8317d
^ permalink raw reply related
* [PATCH 4/4] git-p4: test for absolute PWD problem
From: Pete Wyckoff @ 2011-12-09 23:48 UTC (permalink / raw)
To: git; +Cc: Gary Gibbons, Junio C Hamano
In-Reply-To: <1323474497-14339-1-git-send-email-pw@padd.com>
Signed-off-by: Pete Wyckoff <pw@padd.com>
---
t/t9808-chdir.sh | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
1 files changed, 49 insertions(+), 0 deletions(-)
create mode 100755 t/t9808-chdir.sh
diff --git a/t/t9808-chdir.sh b/t/t9808-chdir.sh
new file mode 100755
index 0000000..e6fd681
--- /dev/null
+++ b/t/t9808-chdir.sh
@@ -0,0 +1,49 @@
+#!/bin/sh
+
+test_description='git-p4 relative chdir'
+
+. ./lib-git-p4.sh
+
+test_expect_success 'start p4d' '
+ start_p4d
+'
+
+test_expect_success 'init depot' '
+ (
+ cd "$cli" &&
+ echo file1 >file1 &&
+ p4 add file1 &&
+ p4 submit -d "change 1"
+ )
+'
+
+# P4 reads from P4CONFIG file to find its server params, if the
+# environment variable is set
+test_expect_success 'P4CONFIG and absolute dir clone' '
+ printf "P4PORT=$P4PORT\nP4CLIENT=$P4CLIENT\n" >p4config &&
+ test_when_finished "rm \"$TRASH_DIRECTORY/p4config\"" &&
+ test_when_finished cleanup_git &&
+ (
+ P4CONFIG=p4config && export P4CONFIG &&
+ unset P4PORT P4CLIENT &&
+ "$GITP4" clone --verbose --dest="$git" //depot
+ )
+'
+
+# same thing, but with relative directory name, note missing $ on --dest
+test_expect_success 'P4CONFIG and relative dir clone' '
+ printf "P4PORT=$P4PORT\nP4CLIENT=$P4CLIENT\n" >p4config &&
+ test_when_finished "rm \"$TRASH_DIRECTORY/p4config\"" &&
+ test_when_finished cleanup_git &&
+ (
+ P4CONFIG=p4config && export P4CONFIG &&
+ unset P4PORT P4CLIENT &&
+ "$GITP4" clone --verbose --dest="git" //depot
+ )
+'
+
+test_expect_success 'kill p4d' '
+ kill_p4d
+'
+
+test_done
--
1.7.8.rc4.42.g8317d
^ permalink raw reply related
* Re: [PATCHv2 0/13] credential helpers
From: Junio C Hamano @ 2011-12-09 23:56 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20111209233957.GC10560@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> Speaking of which, I hackishly ported Jay's osxkeychain helper to the
> new format last night. I'll try to clean that up and post it tonight.
;-).
^ permalink raw reply
* Re: Auto update submodules after merge and reset
From: Phil Hord @ 2011-12-09 23:57 UTC (permalink / raw)
To: andreas.t.auer_gtml_37453; +Cc: Jens Lehmann, git
In-Reply-To: <4EE07FCD.8090702@ursus.ath.cx>
On Thu, Dec 8, 2011 at 4:13 AM, <andreas.t.auer_gtml_37453@ursus.ath.cx> wrote:
>
> On 07.12.2011 23:23 Jens Lehmann wrote:
>> > If you have tracking branches, the supermodule can just update the
>> > corresponding branch. If this branch is currently checkedout and
>> > the work area is clean, then the work area is updated, too. If
>> > there is currently a local branch or a diffent super-branch
>> > checked out then the working area should be considered "detached"
>> > from the superproject and not updated.
>>
>> This sounds a lot like the "follow branch tip" model we discussed
>> recently (which could be configured via .gitmodules), but I'm not
>> sure you really are in the same boat here.
>
> When I understood that correctly it was just a configuration to what branch
> should be automatically checked out in the submodule. This seems to be too
> complicated IMO, because when you have different branches in the
> superproject then you may want to have different branches in the submodules,
> too, but you would need to configure that submodule branch in .gitmodules
> for each branch separately. I.e. in the master branch the .gitmodule may
> contain "master", in the maint branch the .gitmodules may have "maint" as
> the branch to follow.
Yes, but maybe you can update this information in the .gitmodules file
easily with a command. Maybe it could be something simpler than "git
sync-gitmodules-branches", but that is essentially what it would do:
it would save the current branch in each submodule as the "tracked"
branch in the .gitmodules file.
The advantages to this, I think, are that
1. Your "submodule A follows branch X" information is explicit in the
.gitmodules file and so it is not hidden when I examine your patch.
(It sounds to me like the refs/super/* branches would necessarily be
hard to find since the refs/ hierarchy is usually meta data about
local and remote branches. Maybe I should think about tags and notes
more, though.)
2. When you change to "submodule A now follows branch Y", this
information can be unambiguously saved in the commit where it occurred
rather than tucked away, again, in refs/super/*.
The disadvantage, maybe, are that you must now use 'git submodule
sync' or something like that to put any .gitmodules changes into
effect.
Or maybe that is an advantage. How often will this branch tracking change?
I like where you are going, though. But I have trouble following your
meaning when you toss around words like "ref" and "HEAD" and "branch"
and "super-branch". Maybe we can set up a strawman repo (virtually or
on github) where you can explain what happens now and what you wish
would happen instead.
For example, I have some repos like this:
super
+--subA
+--subB
I wish I could do this:
cd super && checkout master
to get this:
super (master)
+--subA (master)
+--subB (master)
Or, if I have SubB on super/'master' tracking 'foo', I could get this:
super (master)
+--subA (master)
+--subB (foo)
And I wish these commands would work as if this was all one repository:
cd super && git diff master maint
cd super && git grep foo
cd subA && git grep foo -- :/
cd super && git status
cd subA && git status
But I wonder what this would do:
cd super && git remote -v &&
cd subA && git remote -v
> I do want to follow the tip of the branch, if the superproject has that
> currently checked out. If the superproject checks out a tagged version for a
> rebuild, then the submodule should not follow the tip, but should get a
> detached HEAD of the corresponding commit, just as the superproject. When
> the superproject goes back to the branch, the submodule should go back to
> its tracking branch.
Now this makes sense. I want the same thing. I want to preserve
history on "old" commits, but I want to "advance to tip" on "new"
commits.
The trouble, I think, is in telling the difference between "old" and
"new". I think it means there is a switch, like --auto-follow (or
--no-auto-follow for the alternate if core.auto_follow is set). But
having a config option as the default is likely to break lots of
scripts.
So maybe I need a new command that does this:
git checkout master && git submodule foreach 'git checkout master'
Maybe it's called "git checkout master --recurse-submodules". But I
seem to recall this is already a non-starter for some reason, and
anyway it doesn't solve the "variant branches in some submodules"
problem.
Which brings us back to .gitmodules.
>> > With this concept you could even switch branches in the
>> > superproject and the attached submodules follow - still having no
>> > detached HEAD. When you want to do some local work on the
>> > submodule you checkout a local branch and merge back into the super
>> > branch later.
>>
>> You lost me here. How can you merge a submodule branch into one of
>> the superproject?
>
> It wouldn't work, if the super/* branch would contain a superproject's
> SHA-1, that is right. But as explained above, it points to a commit of the
> submodule.
>>
>>
>> But we would want to have a deterministic update procedure, no? (And
>> what has more freedom than a detached HEAD? ;-)__
>
> I think my proposal would be deterministic.
> And everything where you can commit to has more freedom than a detached HEAD
You can commit to a detached HEAD. I do it all the time. The trick
is in not switching away from a detached HEAD with your local commits
still on it. :-)
>> > Even though it will raise a lot of detailed questions like "should
>> > the refs/super/* be pushed/pulled when syncing the submodule
>> > repositories".
>>
>> I doubt that is a good idea, as that might conflict with the same
>> submodule sitting in a different superproject. But I'm interested to
>> hear how you want to solve that.
>
> The first answer to my question was "yes, you need to transfer the refs or
> you get unreferenced objects" and "no, you can't transfer the refs, because
> they are owned by the superproject, not the submodule."
> But binding a submodule to a superproject makes perfect sense if it is _one_
> project that is split into submodules. In that case you only have one
> superproject for a submodule and for that purpose it would be good workflow.
This is not useful to me, though. Sorry.
> It is even nice to see which commits in the submodule belong to what
> branches in the superproject or to what release version (so tracking
> superproject tags would make sense, too). If you have a submodule that has
> more than one superproject but these are well-defined, it could be solved
> using refspecs (e.g. refs/super/foo/* for one and refs/super/bar/* for the
> other superproject), but currently I can't think of a context where this
> makes sense.
I can, but this does put the cart before the horse. The submodule is
subservient to the super project in all my setups. The submodule is
not aware who owns him. He is a bit like the DAG in reverse. He
knows one direction only (children), not the other (parents).
Phil
^ permalink raw reply
* Re: [PATCH 4/7] refactor git_getpass into generic prompt function
From: Junio C Hamano @ 2011-12-09 23:58 UTC (permalink / raw)
To: Jeff King; +Cc: git, Erik Faye-Lund
In-Reply-To: <20111208083133.GD26409@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> 2. The first series had a special "name" parameter just for generating
> error messages. This drops it in the name of simplicity, so error
> messages have gone from (assuming you don't have a tty):
>
> Could not read password: No such device or address
>
> to:
>
> Could not read 'Username for 'https://example.com': ': No such
> device or address
>
> which is verbose, yes, but contains a little more useful
> information. The formatting is rather unfortunate,...
It also would be unpleasant to i18n it, I suspect.
> + r = getpass(prompt);
> + if (!r)
> + die_errno("could not read '%s'", prompt);
Taking advantage of the "prompt-string"-ness of the message, this might be
a cuter workaround:
fatal: Password: <<could not be read>>
But I do not think it matters that much. Let's queue what you have, and
work out these details in-tree.
^ permalink raw reply
* Re: [PATCH] Set hard limit on delta chain depth
From: Junio C Hamano @ 2011-12-10 0:02 UTC (permalink / raw)
To: Nguyen Thai Ngoc Duy; +Cc: git
In-Reply-To: <CACsJy8AZg3DgZzmPyXhCH9bGBqo9UN7-zLt_feTtpyajf5U1tw@mail.gmail.com>
Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:
> That's interesting. First of all xmalloc() is controlled by us while
> index-pack code might lead to stack overflow exploit (never done it,
> not sure if it's really pratical to do in this case).
What do you exactly mean by "stack overflow exploit"?
If your callee has prepares a stackframe that is not sufficiently big but
carelessly tries to store more than it has space for, such a write can
overflow the stack (without hardware traps) and overwrite return address,
and instead of coming back to you, the control can be transferred to
random places.
But I do not think that is what we are talking about here.
You attempt to write parameters and return address to the area of memory
pointed by your stack pointer, advance the stack pointer to create a stack
frame and the callee attempts to write to its local variables in the newly
allocated stack frame. These memory accesses eventually attempt to touch
memory beyond the address range the kernel gave you page table entries to
be used as your stack space, and hardware traps. If you haven't run out of
the stack, a new page is lazily added to the page table and your attempted
access will succeed. If you are recursing too deeply, you won't be given a
new page and you will be killed by the kernel. That is a rather controlled
death of the process, unlike smashing the contents of the stack to jump to
a randomly chosen place, isn't it?
Of course, some platforms do not have an unwritable gap between the stack
segment that grow downwards and the heap that grow upwards, and also your
stackframe could be larger than such a gap (in this particular callchain I
do not think that is the case), so the above discussion does not apply
universally, though.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox